From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id F2101C6FD1D for ; Mon, 20 Mar 2023 15:56:26 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232172AbjCTP4Z (ORCPT ); Mon, 20 Mar 2023 11:56:25 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40934 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231959AbjCTPz4 (ORCPT ); Mon, 20 Mar 2023 11:55:56 -0400 Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 8405A37F2A for ; Mon, 20 Mar 2023 08:47:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1679327242; x=1710863242; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=5Jg5sukXBppooS0SRPIvmB43VMQVR8T5c5gI2lZ2N+k=; b=lU4mdN2stENZofuOFF3UW4LLaSNqN7VaGv1U5Im/1qQIltqcoSYKmkLO yDVgSIsAyRBssWQCogRBHIv9kyphx9DPWncrZxPYTW/yXMg4dax55sgU4 PqRa2Yx9U0Sk/76A1Pwj2Exl22KJ9EuC770pAvFI/qV/pVu6Ny0bRenCa J2XBlOAa3l5a2nOljVSGw+VdeZVcmCYYZ61LWTEswm7571tA4gAroM7zL BCs4I0UjS/SYn72+heAbllYFtprfXvVNNdeXlcCzTEh+EgM4PFdKzwce7 sJw4RSJle+C9QqcJwGpfJA6CQQQL9Vs27NYFRLSgTvMHFa7XW1/O+cFcJ g==; X-IronPort-AV: E=McAfee;i="6600,9927,10655"; a="319094522" X-IronPort-AV: E=Sophos;i="5.98,276,1673942400"; d="scan'208";a="319094522" Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 20 Mar 2023 08:47:11 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10655"; a="750157496" X-IronPort-AV: E=Sophos;i="5.98,276,1673942400"; d="scan'208";a="750157496" Received: from ndchoksi-mobl3.amr.corp.intel.com (HELO [10.212.160.247]) ([10.212.160.247]) by fmsmga004-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 20 Mar 2023 08:47:09 -0700 Message-ID: Date: Mon, 20 Mar 2023 10:47:08 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Firefox/102.0 Thunderbird/102.8.0 Subject: Re: [PATCH v4] ASoC: tas2781: Add tas2781 driver Content-Language: en-US To: Shenghao Ding <13916275206@139.com>, broonie@kernel.org, lgirdwood@gmail.com, perex@perex.cz Cc: kevin-lu@ti.com, shenghao-ding@ti.com, alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org, x1077012@ti.com, peeyush@ti.com, navada@ti.com References: <20230320150726.20573-1-13916275206@139.com> From: Pierre-Louis Bossart In-Reply-To: <20230320150726.20573-1-13916275206@139.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Quite a few issues with style, indentation, useless inits. There is one change of convention between success/error codes, and confusions in the end with the use of devm_ and manual error handling. > +#include > +#include > +#include > +#include > +#include > +#include > +#include alphabetical order? > +#include "tas2781.h" > + > +#define ERROR_PRAM_CRCCHK 0x0000000 > +#define ERROR_YRAM_CRCCHK 0x0000001 > +#define BINFILE_VER 0 > +#define DRV_VER 1 > +#define PPC_DRIVER_CRCCHK 0x00000200 indentation after define? > +static int fw_parse_program_data_kernel( > + struct tasdevice_priv *tas_dev, struct tasdevice_fw *tas_fmw, > + const struct firmware *fmw, int offset) > +{ > + struct tasdevice_prog *program; > + unsigned int nr_program = 0; useless initialization > + > + for (nr_program = 0; nr_program < tas_fmw->nr_programs; nr_program++) { > + program = &(tas_fmw->programs[nr_program]); > + if (offset + 64 > fmw->size) { > + dev_err(tas_dev->dev, "%s: mpName error\n", > + __func__); > + offset = -1; > + goto out; > + } > + offset += 64; > + > + if (offset + 1 > fmw->size) { > + dev_err(tas_dev->dev, "%s: mnAppMode error\n", > + __func__); > + offset = -1; > + goto out; > + } > + offset++; > + > + if (offset + 1 > fmw->size) { > + dev_err(tas_dev->dev, "%s: mnPDMI2SMode error\n", > + __func__); > + offset = -1; > + goto out; > + } > + offset++; > + if (offset + 1 > fmw->size) { > + dev_err(tas_dev->dev, "%s: mnISnsPD error\n", > + __func__); > + offset = -1; > + goto out; > + } > + offset++; > + if (offset + 1 > fmw->size) { > + dev_err(tas_dev->dev, "%s: mnVSnsPD error\n", > + __func__); > + offset = -1; > + goto out; > + } > + offset++; > + //skip 3-byte reserved > + offset += 3; > + if (offset + 1 > fmw->size) { > + dev_err(tas_dev->dev, "%s: mnPowerLDG error\n", > + __func__); > + offset = -1; > + goto out; > + } > + offset++; > + > + offset = fw_parse_data_kernel(tas_fmw, &(program->dev_data), > + fmw, offset); > + if (offset < 0) > + goto out; > + } > +out: > + return offset; > +} > + > +static int fw_parse_configuration_data_kernel( > + struct tasdevice_priv *tas_dev, > + struct tasdevice_fw *tas_fmw, const struct firmware *fmw, int offset) > +{ > + const unsigned char *data = fmw->data; > + unsigned int nr_configs; > + struct tasdevice_config *config; > + > + for (nr_configs = 0; nr_configs < tas_fmw->nr_configurations; > + nr_configs++) { one line? > + config = > + &(tas_fmw->configs[nr_configs]); one line? > + if (offset + 64 > fmw->size) { > + dev_err(tas_dev->dev, "%s: mpName error\n", > + __func__); one line? > +static int fw_parse_variable_header_kernel( > + struct tasdevice_priv *tas_dev, const struct firmware *fmw, int offset) > +{ > + struct tasdevice_fw *tas_fmw = tas_dev->fmw; > + struct tasdevice_dspfw_hdr *fw_hdr = &(tas_fmw->fw_hdr); > + const unsigned char *buf = fmw->data; > + struct tasdevice_prog *program; > + struct tasdevice_config *config; > + unsigned int nr_program = 0, nr_configs = 0; useless inits. > + unsigned short max_confs = TASDEVICE_MAXCONFIG_NUM_KERNEL; init also looks useless? > + > + if (offset + 2 > fmw->size) { > + dev_err(tas_dev->dev, "%s: File Size error\n", __func__); > + offset = -1; > + goto out; > + } > + fw_hdr->device_family = SMS_HTONS(buf[offset], buf[offset + 1]); > + if (fw_hdr->device_family != 0) { > + dev_err(tas_dev->dev, "ERROR:%s:not TAS device\n", __func__); > + offset = -1; > + goto out; > + } > + offset += 2; > + if (offset + 2 > fmw->size) { > + dev_err(tas_dev->dev, "%s: mnDevice error\n", __func__); > + offset = -1; > + goto out; > + } > + fw_hdr->device = SMS_HTONS(buf[offset], buf[offset + 1]); > + if (fw_hdr->device >= TASDEVICE_DSP_TAS_MAX_DEVICE || > + fw_hdr->device == 6) { > + dev_err(tas_dev->dev, "ERROR:%s: not support device %d\n", > + __func__, fw_hdr->device); > + offset = -1; > + goto out; > + } > + offset += 2; > + fw_hdr->ndev = deviceNumber[fw_hdr->device]; > + > + if (fw_hdr->ndev != tas_dev->ndev) { > + dev_err(tas_dev->dev, > + "%s: ndev(%u) in dspbin dismatch ndev(%u) in DTS\n", > + __func__, fw_hdr->ndev, tas_dev->ndev); > + offset = -1; > + goto out; > + } > + > + if (offset + 4 > fmw->size) { > + dev_err(tas_dev->dev, "%s: mnPrograms error\n", __func__); > + offset = -1; > + goto out; > + } > + tas_fmw->nr_programs = SMS_HTONL(buf[offset], buf[offset + 1], > + buf[offset + 2], buf[offset + 3]); > + offset += 4; > + > + if (tas_fmw->nr_programs == 0 || tas_fmw->nr_programs > > + TASDEVICE_MAXPROGRAM_NUM_KERNEL) { > + dev_err(tas_dev->dev, "%s: mnPrograms is invalid\n", __func__); > + offset = -1; > + goto out; > + } > + > + if (offset + 4 * TASDEVICE_MAXPROGRAM_NUM_KERNEL > fmw->size) { > + dev_err(tas_dev->dev, "%s: mpPrograms error\n", __func__); > + offset = -1; > + goto out; > + } > + > + tas_fmw->programs = > + kcalloc(tas_fmw->nr_programs, > + sizeof(struct tasdevice_prog), GFP_KERNEL); indentation is off. > + if (tas_fmw->programs == NULL) { > + dev_err(tas_dev->dev, "%s: mpPrograms memory failed!\n", > + __func__); > + offset = -1; > + goto out; > + } > + > + for (nr_program = 0; nr_program < tas_fmw->nr_programs; nr_program++) { > + program = &(tas_fmw->programs[nr_program]); > + program->prog_size = SMS_HTONL(buf[offset], buf[offset + 1], > + buf[offset + 2], buf[offset + 3]); > + offset += 4; > + } > + offset += (4 * (5 - nr_program)); > + > + if (offset + 4 > fmw->size) { > + dev_err(tas_dev->dev, "%s: mnConfigurations error\n", > + __func__); > + offset = -1; > + goto out; > + } > + tas_fmw->nr_configurations = SMS_HTONL(buf[offset], buf[offset + 1], > + buf[offset + 2], buf[offset + 3]); > + offset += 4; > + max_confs = (fw_hdr->ndev >= 4) ? > + TASDEVICE_MAXCONFIG_NUM_KERNEL_MULTIPLE_AMPS : > + TASDEVICE_MAXCONFIG_NUM_KERNEL; > + if (tas_fmw->nr_configurations == 0 || > + tas_fmw->nr_configurations > max_confs) { > + dev_err(tas_dev->dev, "%s: mnConfigurations is invalid\n", > + __func__); > + offset = -1; > + goto out; > + } > + > + if (offset + 4 * max_confs > fmw->size) { > + dev_err(tas_dev->dev, "%s: mpConfigurations error\n", > + __func__); > + offset = -1; > + goto out; > + } > + > + tas_fmw->configs = kcalloc(tas_fmw->nr_configurations, > + sizeof(struct tasdevice_config), GFP_KERNEL); > + if (tas_fmw->configs == NULL) { > + offset = -1; > + goto out; > + } > + > + for (nr_configs = 0; nr_configs < tas_fmw->nr_programs; > + nr_configs++) { > + config = > + &(tas_fmw->configs[nr_configs]); > + config->cfg_size = SMS_HTONL(buf[offset], > + buf[offset + 1], buf[offset + 2], buf[offset + 3]); > + offset += 4; > + } > + > + offset += (4 * (max_confs - nr_configs)); > +out: > + return offset; > +} > + > +static int tasdevice_load_block_kernel( > + struct tasdevice_priv *tasdevice, struct tasdev_blk *block) > +{ > + int ret = 0; > + > + unsigned char *data = block->data; > + unsigned int i = 0, length = 0; init for i is useless, init for length is required. don't mix those cases. > + const unsigned int blk_size = block->blk_size; > + unsigned char dev_idx = 0; > + struct tasdevice_dspfw_hdr *fw_hdr = &(tasdevice->fmw->fw_hdr); > + struct tasdevice_fw_fixed_hdr *fw_fixed_hdr = &(fw_hdr->fixed_hdr); > + > + if (fw_fixed_hdr->ppcver >= PPC3_VERSION) { > + switch (block->type) { > + case MAIN_ALL_DEVICES_1X: > + dev_idx = 0|0x80; > + break; > + case MAIN_DEVICE_A_1X: > + dev_idx = 1|0x80; > + break; > + case COEFF_DEVICE_A_1X: > + case PRE_DEVICE_A_1X: > + dev_idx = 1|0xC0; > + break; > + case MAIN_DEVICE_B_1X: > + dev_idx = 2|0x80; > + break; > + case COEFF_DEVICE_B_1X: > + case PRE_DEVICE_B_1X: > + dev_idx = 2|0xC0; > + break; > + case MAIN_DEVICE_C_1X: > + dev_idx = 3|0x80; > + break; > + case COEFF_DEVICE_C_1X: > + case PRE_DEVICE_C_1X: > + dev_idx = 3|0xC0; > + break; > + case MAIN_DEVICE_D_1X: > + dev_idx = 4|0x80; > + break; > + case COEFF_DEVICE_D_1X: > + case PRE_DEVICE_D_1X: > + dev_idx = 4|0xC0; > + break; > + default: > + dev_info(tasdevice->dev, > + "%s: load block: Other Type = 0x%02x\n", > + __func__, block->type); > + break; > + } > + } else { > + switch (block->type) { > + case MAIN_ALL_DEVICES: > + dev_idx = 0|0x80; > + break; > + case MAIN_DEVICE_A: > + dev_idx = 1|0x80; > + break; > + case COEFF_DEVICE_A: > + case PRE_DEVICE_A: > + dev_idx = 1|0xC0; > + break; > + case MAIN_DEVICE_B: > + dev_idx = 2|0x80; > + break; > + case COEFF_DEVICE_B: > + case PRE_DEVICE_B: > + dev_idx = 2|0xC0; > + break; > + case MAIN_DEVICE_C: > + dev_idx = 3|0x80; > + break; > + case COEFF_DEVICE_C: > + case PRE_DEVICE_C: > + dev_idx = 3|0xC0; > + break; > + case MAIN_DEVICE_D: > + dev_idx = 4|0x80; > + break; > + case COEFF_DEVICE_D: > + case PRE_DEVICE_D: > + dev_idx = 4|0xC0; > + break; > + default: > + dev_info(tasdevice->dev, > + "%s: load block: Other Type = 0x%02x\n", > + __func__, block->type); > + break; > + } > + } > + > + for (i = 0; i < block->nr_subblocks; i++) { > + int rc = tasdevice_process_block(tasdevice, data + length, > + dev_idx, blk_size - length); > + if (rc < 0) { > + dev_err(tasdevice->dev, > + "%s: ERROR:%u %u sublock write error\n", > + __func__, length, blk_size); > + break; this will not return an error. should you use 'ret' instead of 'rc' here, or... > + } > + length += (unsigned int)rc; > + if (blk_size < length) { > + dev_err(tasdevice->dev, > + "%s: ERROR:%u %u out of bounary\n", typo: boundary. use a spell-checker! > + __func__, length, blk_size); > + break; > + } > + } > + > + return ret; ... return 0 and remove ret? > +} > + > +static int fw_parse_variable_header_git(struct tasdevice_priv > + *tas_dev, const struct firmware *fmw, int offset) > +{ > + const unsigned char *buf = fmw->data; > + struct tasdevice_fw *tas_fmw = tas_dev->fmw; > + struct tasdevice_dspfw_hdr *fw_hdr = &(tas_fmw->fw_hdr); > + int i = strlen((char *)&buf[offset]); > + > + i++; 'i' is not a great name for a length variable, best used for loops... > + > + if (offset + i > fmw->size) { > + dev_err(tas_dev->dev, "%s: File Size error\n", __func__); > + offset = -1; > + goto out; > + } > + > + offset += i; > + > + if (offset + 4 > fmw->size) { > + dev_err(tas_dev->dev, "%s: File Size error\n", __func__); > + offset = -1; > + goto out; > + } > + fw_hdr->device_family = SMS_HTONL(buf[offset], buf[offset + 1], > + buf[offset + 2], buf[offset + 3]); > + if (fw_hdr->device_family != 0) { > + dev_err(tas_dev->dev, "ERROR:%s: not TAS device\n", __func__); > + offset = -1; > + goto out; > + } > + offset += 4; > + if (offset + 4 > fmw->size) { > + dev_err(tas_dev->dev, "%s: File Size error\n", __func__); > + offset = -1; > + goto out; > + } > + fw_hdr->device = SMS_HTONL(buf[offset], buf[offset + 1], > + buf[offset + 2], buf[offset + 3]); > + if (fw_hdr->device >= TASDEVICE_DSP_TAS_MAX_DEVICE || > + fw_hdr->device == 6) { > + dev_err(tas_dev->dev, "ERROR:%s: not support device %d\n", > + __func__, fw_hdr->device); > + offset = -1; > + goto out; > + } > + offset += 4; > + fw_hdr->ndev = deviceNumber[fw_hdr->device]; > + if (fw_hdr->ndev != tas_dev->ndev) { > + dev_err(tas_dev->dev, > + "%s: ndev(%u) in dspbin dismatch ndev(%u) in DTS\n", > + __func__, fw_hdr->ndev, > + tas_dev->ndev); > + offset = -1; > + } > + > +out: > + return offset; > +} > + > +static int fw_parse_variable_hdr_cal(struct tasdevice_priv *tas_dev, > + struct tasdevice_fw *tas_fmw, const struct firmware *fmw, int offset) > +{ > + const unsigned char *buf = fmw->data; > + struct tasdevice_dspfw_hdr *fw_hdr = &(tas_fmw->fw_hdr); > + int i = strlen((char *)&buf[offset]); > + > + i++; same here. > + > + if (offset + i > fmw->size) { > + dev_err(tas_dev->dev, "%s: File Size error\n", __func__); > + offset = -1; > + goto out; > + } > + > + offset += i; > + > + if (offset + 4 > fmw->size) { > + dev_err(tas_dev->dev, "%s: mnDeviceFamily error\n", __func__); > + offset = -1; > + goto out; > + } > + fw_hdr->device_family = SMS_HTONL(buf[offset], buf[offset + 1], > + buf[offset + 2], buf[offset + 3]); > + if (fw_hdr->device_family != 0) { > + dev_err(tas_dev->dev, "ERROR:%s: not TAS device\n", __func__); > + offset = -1; > + goto out; > + } > + offset += 4; > + if (offset + 4 > fmw->size) { > + dev_err(tas_dev->dev, "%s: mnDevice error\n", __func__); > + offset = -1; > + goto out; > + } > + fw_hdr->device = SMS_HTONL(buf[offset], buf[offset + 1], > + buf[offset + 2], buf[offset + 3]); > + if (fw_hdr->device >= TASDEVICE_DSP_TAS_MAX_DEVICE || > + fw_hdr->device == 6) { > + dev_err(tas_dev->dev, "ERROR:%s: not support device %d\n", > + __func__, fw_hdr->device); > + offset = -1; > + goto out; > + } > + offset += 4; > + fw_hdr->ndev = deviceNumber[fw_hdr->device]; > + if (fw_hdr->ndev != 1) { > + dev_err(tas_dev->dev, > + "%s: calbin must be 1, but currently ndev(%u)\n", > + __func__, fw_hdr->ndev); > + offset = -1; > + } > + > +out: > + return offset; > +} > + > +static inline void tas2781_clear_calfirmware(struct tasdevice_fw > + *tas_fmw) > +{ > + int i = 0; useless init > + unsigned int blks = 0; also useless > + > + if (tas_fmw->calibrations) { > + struct tasdevice_calibration *calibration; > + > + for (i = 0; i < tas_fmw->nr_calibrations; i++) { > + calibration = &(tas_fmw->calibrations[i]); > + if (calibration) { > + struct tasdevice_data *im = > + &(calibration->dev_data); > + > + if (im->dev_blks) { > + struct tasdev_blk *block; > + > + for (blks = 0; blks < im->nr_blk; > + blks++) { > + block = &(im->dev_blks[blks]); > + kfree(block->data); > + } > + kfree(im->dev_blks); > + } > + } > + } > + kfree(tas_fmw->calibrations); > + } > + kfree(tas_fmw); > +} > + > +static int fw_parse_block_data(struct tasdevice_fw *tas_fmw, > + struct tasdev_blk *block, const struct firmware *fmw, int offset) > +{ > + unsigned char *data = (unsigned char *)fmw->data; > + int n; > + > + if (offset + 4 > fmw->size) { > + dev_err(tas_fmw->dev, "%s: mnType error\n", __func__); > + offset = -1; > + goto out; > + } > + block->type = SMS_HTONL(data[offset], data[offset + 1], > + data[offset + 2], data[offset + 3]); > + offset += 4; > + > + if (tas_fmw->fw_hdr.fixed_hdr.drv_ver >= > + PPC_DRIVER_CRCCHK) { > + if (offset + 1 > fmw->size) { > + dev_err(tas_fmw->dev, "%s: mbPChkSumPresent error\n", > + __func__); one line? same for all the code in this file. > + offset = -1; > + goto out; > + } > + block->is_pchksum_present = data[offset]; > + offset++; > + > + if (offset + 1 > fmw->size) { > + dev_err(tas_fmw->dev, "%s: mnPChkSum error\n", > + __func__); > + offset = -1; > + goto out; > + } > + block->pchksum = data[offset]; > + offset++; > + > + if (offset + 1 > fmw->size) { > + dev_err(tas_fmw->dev, "%s: mbYChkSumPresent error\n", > + __func__); > + offset = -1; > + goto out; > + } > + block->is_ychksum_present = data[offset]; > + offset++; > + > + if (offset + 1 > fmw->size) { > + dev_err(tas_fmw->dev, "%s: mnYChkSum error\n", > + __func__); > + offset = -1; > + goto out; > + } > + block->ychksum = data[offset]; > + offset++; > + } else { > + block->is_pchksum_present = 0; > + block->is_ychksum_present = 0; > + } > + if (offset + 4 > fmw->size) { > + dev_err(tas_fmw->dev, "%s: mnCommands error\n", __func__); > + offset = -1; > + goto out; > + } > + block->nr_cmds = SMS_HTONL(data[offset], data[offset + 1], > + data[offset + 2], data[offset + 3]); > + offset += 4; > + > + n = block->nr_cmds * 4; > + if (offset + n > fmw->size) { > + dev_err(tas_fmw->dev, > + "%s: File Size(%lu) error offset = %d n = %d\n", > + __func__, (unsigned long)fmw->size, offset, n); > + offset = -1; > + goto out; > + } > + block->data = kmemdup(&data[offset], n, GFP_KERNEL); > + if (block->data == NULL) { > + offset = -1; > + goto out; > + } > + offset += n; > +out: > + return offset; > +} > + > +static int fw_parse_data(struct tasdevice_fw *tas_fmw, > + struct tasdevice_data *img_data, const struct firmware *fmw, > + int offset) > +{ > + const unsigned char *data = (unsigned char *)fmw->data; > + int n = 0; useless init. Also consider reverse xmas-tree order to make the code nicer. > + unsigned int nr_block; > + > + if (offset + 64 > fmw->size) { > + dev_err(tas_fmw->dev, "%s: mpName error\n", __func__); > + n = -1; > + goto out; > + } > + memcpy(img_data->name, &data[offset], 64); > + offset += 64; > + > + n = strlen((char *)&data[offset]); > + n++; > + if (offset + n > fmw->size) { > + dev_err(tas_fmw->dev, "%s: mpDescription error\n", __func__); > + offset = -1; > + goto out; > + } > + offset += n; > + > + if (offset + 2 > fmw->size) { > + dev_err(tas_fmw->dev, "%s: mnBlocks error\n", __func__); > + offset = -1; > + goto out; > + } > + img_data->nr_blk = SMS_HTONS(data[offset], data[offset + 1]); > + offset += 2; > + > + img_data->dev_blks = > + kcalloc(img_data->nr_blk, sizeof(struct tasdev_blk), > + GFP_KERNEL); > + if (img_data->dev_blks == NULL) { > + dev_err(tas_fmw->dev, "%s: FW memory failed!\n", __func__); > + goto out; > + } > + for (nr_block = 0; nr_block < img_data->nr_blk; nr_block++) { > + offset = fw_parse_block_data(tas_fmw, > + &(img_data->dev_blks[nr_block]), fmw, offset); > + if (offset < 0) { > + offset = -1; > + goto out; > + } > + } > +out: > + return offset; > +} > + > +static int fw_parse_calibration_data(struct tasdevice_priv *tas_dev, > + struct tasdevice_fw *tas_fmw, const struct firmware *fmw, int offset) > +{ > + unsigned char *data = (unsigned char *)fmw->data; > + unsigned int n = 0; > + unsigned int nr_calibration = 0; useless inits > + struct tasdevice_calibration *calibration = NULL; useless as well > + > + if (offset + 2 > fmw->size) { > + dev_err(tas_dev->dev, "%s: mnCalibrations error\n", __func__); > + offset = -1; > + goto out; > + } > + tas_fmw->nr_calibrations = SMS_HTONS(data[offset], data[offset + 1]); > + offset += 2; > + > + if (tas_fmw->nr_calibrations != 1) { > + dev_err(tas_dev->dev, > + "%s: only support one calibraiton(%d)!\n", > + __func__, tas_fmw->nr_calibrations); > + goto out; > + } > + > + tas_fmw->calibrations = > + kcalloc(tas_fmw->nr_calibrations, > + sizeof(struct tasdevice_calibration), GFP_KERNEL); > + if (tas_fmw->calibrations == NULL) { > + dev_err(tas_dev->dev, "%s: mpCalibrations memory failed!\n", > + __func__); > + offset = -1; > + goto out; > + } > + for (nr_calibration = 0; nr_calibration < tas_fmw->nr_calibrations; > + nr_calibration++) { > + if (offset + 64 > fmw->size) { > + dev_err(tas_dev->dev, "%s: mpCalibrations error\n", > + __func__); > + offset = -1; > + goto out; > + } > + calibration = &(tas_fmw->calibrations[nr_calibration]); > + offset += 64; > + > + n = strlen((char *)&data[offset]); > + n++; > + if (offset + n > fmw->size) { > + dev_err(tas_dev->dev, "%s: mpDescription error\n", > + __func__); > + offset = -1; > + goto out; > + } > + offset += n; > + > + if (offset + 1 > fmw->size) { > + dev_err(tas_dev->dev, > + "%s: mnProgram error, offset = %d\n", __func__, > + offset); > + offset = -1; > + goto out; > + } > + offset++; > + > + if (offset + 1 > fmw->size) { > + dev_err(tas_dev->dev, > + "%s: mnConfiguration error, offset = %d\n", > + __func__, > + offset); > + offset = -1; > + goto out; > + } > + offset++; > + > + offset = fw_parse_data(tas_fmw, &(calibration->dev_data), fmw, > + offset); > + if (offset < 0) > + goto out; > + } > + > +out: > + > + return offset; > +} > + > +static int fw_parse_program_data(struct tasdevice_priv *tas_dev, > + struct tasdevice_fw *tas_fmw, const struct firmware *fmw, int offset) > +{ > + struct tasdevice_prog *program; > + unsigned char *buf = (unsigned char *)fmw->data; > + int nr_program = 0; > + > + if (offset + 2 > fmw->size) { > + dev_err(tas_dev->dev, "%s: File Size error\n", __func__); > + offset = -1; > + goto out; > + } > + tas_fmw->nr_programs = SMS_HTONS(buf[offset], buf[offset + 1]); > + offset += 2; > + > + if (tas_fmw->nr_programs == 0) { if (!tas_fmw->nr_programs) > + dev_err(tas_dev->dev, "%s: mnPrograms is null, maybe calbin\n", > + __func__); > + //Do not "offset = -1;", because of calbin Comment is not clear, and C comments are preferred. > + goto out; > + } > + > + tas_fmw->programs = > + kcalloc(tas_fmw->nr_programs, sizeof(struct tasdevice_prog), > + GFP_KERNEL); > + if (tas_fmw->programs == NULL) { > + offset = -1; > + goto out; > + } > + for (nr_program = 0; nr_program < tas_fmw->nr_programs; nr_program++) { > + int n = 0; useless init > + > + program = &(tas_fmw->programs[nr_program]); > + if (offset + 64 > fmw->size) { > + dev_err(tas_dev->dev, "%s: mpName error\n", __func__); > + offset = -1; > + goto out; > + } > + offset += 64; > + > + n = strlen((char *)&buf[offset]); > + n++; > + if (offset + n > fmw->size) { > + dev_err(tas_dev->dev, "%s: mpDescription error\n", > + __func__); > + offset = -1; > + goto out; > + } > + > + offset += n; > + > + if (offset + 1 > fmw->size) { > + dev_err(tas_dev->dev, "%s: mnAppMode error\n", > + __func__); > + offset = -1; > + goto out; > + } > + offset++; > + > + if (offset + 1 > fmw->size) { > + dev_err(tas_dev->dev, "%s: mnPDMI2SMode error\n", > + __func__); > + offset = -1; > + goto out; > + } > + offset++; > + if (offset + 1 > fmw->size) { > + dev_err(tas_dev->dev, "%s: mnISnsPD error\n", > + __func__); > + offset = -1; > + goto out; > + } > + offset++; > + if (offset + 1 > fmw->size) { > + dev_err(tas_dev->dev, "%s: mnVSnsPD error\n", > + __func__); > + offset = -1; > + goto out; > + } > + offset++; > + if (offset + 1 > fmw->size) { > + dev_err(tas_dev->dev, "%s: mnPowerLDG error\n", > + __func__); > + offset = -1; > + goto out; > + } > + offset++; > + > + offset = fw_parse_data(tas_fmw, &(program->dev_data), fmw, > + offset); > + if (offset < 0) > + goto out; > + } > +out: > + return offset; > +} > + > +static int fw_parse_configuration_data( > + struct tasdevice_priv *tas_dev, > + struct tasdevice_fw *tas_fmw, > + const struct firmware *fmw, int offset) > +{ > + unsigned char *data = (unsigned char *)fmw->data; > + int n; > + unsigned int n_configs; > + struct tasdevice_config *config; > + > + if (offset + 2 > fmw->size) { > + dev_err(tas_dev->dev, "%s: File Size error\n", __func__); > + offset = -1; > + goto out; > + } > + tas_fmw->nr_configurations = SMS_HTONS(data[offset], data[offset + 1]); > + offset += 2; > + > + if (tas_fmw->nr_configurations == 0) { > + dev_err(tas_dev->dev, "%s: Configurations is zero\n", > + __func__); > + //Do not "offset = -1;", because of calbin > + goto out; > + } > + tas_fmw->configs = > + kcalloc(tas_fmw->nr_configurations, > + sizeof(struct tasdevice_config), GFP_KERNEL); > + > + for (n_configs = 0; n_configs < tas_fmw->nr_configurations; > + n_configs++) { > + config = > + &(tas_fmw->configs[n_configs]); > + if (offset + 64 > fmw->size) { > + dev_err(tas_dev->dev, "%s: File Size error\n", > + __func__); > + offset = -1; > + goto out; > + } > + memcpy(config->name, &data[offset], 64); > + offset += 64; > + > + n = strlen((char *)&data[offset]); > + n++; > + if (offset + n > fmw->size) { > + dev_err(tas_dev->dev, "%s: mpDescription error\n", > + __func__); > + offset = -1; > + goto out; > + } > + > + offset += n; > + if (offset + 2 > fmw->size) { > + dev_err(tas_dev->dev, > + "%s: mnDevice_orientation error\n", > + __func__); > + offset = -1; > + goto out; > + } > + offset += 2; > + > + if (offset + 1 > fmw->size) { > + dev_err(tas_dev->dev, "%s: mProgram error\n", > + __func__); > + offset = -1; > + goto out; > + } > + offset++; > + > + if (offset + 4 > fmw->size) { > + dev_err(tas_dev->dev, "%s: mnSamplingRate error\n", > + __func__); > + offset = -1; > + goto out; > + } > + offset += 4; > + > + if (offset + 1 > fmw->size) { > + dev_err(tas_dev->dev, "%s: mnPLLSrc error\n", > + __func__); > + offset = -1; > + goto out; > + } > + offset++; > + > + if (offset + 4 > fmw->size) { > + dev_err(tas_dev->dev, "%s: mnPLLSrcRate error\n", > + __func__); > + offset = -1; > + goto out; > + } > + offset += 4; > + > + if (offset + 2 > fmw->size) { > + dev_err(tas_dev->dev, "%s: mnFsRate error\n", > + __func__); > + offset = -1; > + goto out; > + } > + offset += 2; > + > + offset = fw_parse_data(tas_fmw, &(config->dev_data), > + fmw, offset); > + if (offset < 0) > + goto out; > + } > +out: > + return offset; > +} > + > +static int fw_parse_header(struct tasdevice_priv *tas_dev, > + struct tasdevice_fw *tas_fmw, const struct firmware *fmw, int offset) > +{ > + struct tasdevice_dspfw_hdr *fw_hdr = &(tas_fmw->fw_hdr); > + struct tasdevice_fw_fixed_hdr *fw_fixed_hdr = &(fw_hdr->fixed_hdr); > + const unsigned char *buf = (unsigned char *)fmw->data; > + const unsigned char magic_number[] = { 0x35, 0x35, 0x35, 0x32 }; > + > + if (offset + 4 > fmw->size) { > + dev_err(tas_dev->dev, "%s: File Size error\n", __func__); > + offset = -EINVAL; consistency: before you returned -1, now it's -EINVAL? -1 should probably be avoided. > + goto out; > + } > + if (memcmp(&buf[offset], magic_number, 4)) { > + dev_err(tas_dev->dev, "%s: Magic number doesn't match", > + __func__); > + offset = -EINVAL; > + goto out; > + } > + offset += 4; > + if (offset + 4 > fmw->size) { > + dev_err(tas_dev->dev, "%s: mnFWSize error\n", __func__); > + offset = -1; > + goto out; > + } > + fw_fixed_hdr->fwsize = SMS_HTONL(buf[offset], > + buf[offset + 1], buf[offset + 2], buf[offset + 3]); > + offset += 4; > + if (offset + 4 > fmw->size) { > + dev_err(tas_dev->dev, "%s: File Size error\n", __func__); > + offset = -1; > + goto out; > + } > + if (fw_fixed_hdr->fwsize != fmw->size) { > + dev_err(tas_dev->dev, "File size not match, %lu %u", > + (unsigned long)fmw->size, fw_fixed_hdr->fwsize); > + offset = -1; > + goto out; > + } > + if (offset + 4 > fmw->size) { > + dev_err(tas_dev->dev, "%s: mnChecksum error\n", __func__); > + offset = -1; > + goto out; > + } > + offset += 4; > + if (offset + 4 > fmw->size) { > + dev_err(tas_dev->dev, "%s: mnPPCVersion error\n", __func__); > + offset = -1; > + goto out; > + } > + fw_fixed_hdr->ppcver = SMS_HTONL(buf[offset], > + buf[offset + 1], buf[offset + 2], buf[offset + 3]); > + offset += 4; > + if (offset + 4 > fmw->size) { > + dev_err(tas_dev->dev, "%s: mnFWVersion error\n", __func__); > + offset = -1; > + goto out; > + } > + offset += 4; > + if (offset + 4 > fmw->size) { > + dev_err(tas_dev->dev, "%s: mnDriverVersion error\n", __func__); > + offset = -1; > + goto out; > + } > + fw_fixed_hdr->drv_ver = SMS_HTONL(buf[offset], > + buf[offset + 1], buf[offset + 2], buf[offset + 3]); > + offset += 4; > + if (offset + 4 > fmw->size) { > + dev_err(tas_dev->dev, "%s: mnTimeStamp error\n", __func__); > + offset = -1; > + goto out; > + } > + offset += 4; > + if (offset + 64 > fmw->size) { > + dev_err(tas_dev->dev, "%s: mpDDCName error\n", __func__); > + offset = -1; > + goto out; > + } > + offset += 64; > + > + out: > + return offset; > +} > + > +static int check_inpage_yram(struct tas_crc *cd, unsigned char book, > + unsigned char page, unsigned char reg, unsigned char len) > +{ > + int ret = 0; > + > + if (book == TAS2781_YRAM_BOOK1) { > + if (page == TAS2781_YRAM1_PAGE) { > + if (reg >= TAS2781_YRAM1_START_REG) { > + cd->offset = reg; > + cd->len = len; > + ret = 1; > + } else if ((reg + len) > TAS2781_YRAM1_START_REG) { > + cd->offset = TAS2781_YRAM1_START_REG; > + cd->len = > + len - (TAS2781_YRAM1_START_REG - reg); > + ret = 1; > + } else > + ret = 0; > + } else if (page == TAS2781_YRAM3_PAGE) { > + if (reg > TAS2781_YRAM3_END_REG) { > + ret = 0; > + } else if (reg >= TAS2781_YRAM3_START_REG) { > + if ((reg + len) > TAS2781_YRAM3_END_REG) { > + cd->offset = reg; > + cd->len = > + TAS2781_YRAM3_END_REG - reg + 1; > + ret = 1; > + } else { > + cd->offset = reg; > + cd->len = len; > + ret = 1; > + } > + } else { > + if ((reg + (len-1)) < > + TAS2781_YRAM3_START_REG) > + ret = 0; > + else { > + cd->offset = > + TAS2781_YRAM3_START_REG; > + cd->len = > + len - (TAS2781_YRAM3_START_REG - reg); > + ret = 1; > + } > + } > + } > + } else if (book == > + TAS2781_YRAM_BOOK2) { > + if (page == TAS2781_YRAM5_PAGE) { > + if (reg > TAS2781_YRAM5_END_REG) { > + ret = 0; > + } else if (reg >= TAS2781_YRAM5_START_REG) { > + if ((reg + len) > TAS2781_YRAM5_END_REG) { > + cd->offset = reg; > + cd->len = > + TAS2781_YRAM5_END_REG - reg + 1; > + ret = 1; > + } else { > + cd->offset = reg; > + cd->len = len; > + ret = 1; > + } > + } else { > + if ((reg + (len-1)) < > + TAS2781_YRAM5_START_REG) > + ret = 0; > + else { > + cd->offset = > + TAS2781_YRAM5_START_REG; > + cd->len = > + len - (TAS2781_YRAM5_START_REG - reg); > + ret = 1; > + } > + } > + } > + } else > + ret = 0; > + > + return ret; and here you return a positive value on success and zero on error. Usually we use the opposite convention. > +} > + > +static int check_inblock_yram(struct tas_crc *cd, unsigned char book, > + unsigned char page, unsigned char reg, unsigned char len) > +{ > + int ret = 0; > + > + if (book == TAS2781_YRAM_BOOK1) { > + if (page < TAS2781_YRAM2_START_PAGE) > + ret = 0; > + else if (page <= TAS2781_YRAM2_END_PAGE) { > + if (reg > TAS2781_YRAM2_END_REG) > + ret = 0; > + else if (reg >= TAS2781_YRAM2_START_REG) { > + cd->offset = reg; > + cd->len = len; > + ret = 1; > + } else { > + if ((reg + (len-1)) < > + TAS2781_YRAM2_START_REG) > + ret = 0; > + else { > + cd->offset = > + TAS2781_YRAM2_START_REG; > + cd->len = > + reg + len - TAS2781_YRAM2_START_REG; > + ret = 1; > + } > + } > + } else > + ret = 0; > + } else if (book == > + TAS2781_YRAM_BOOK2) { > + if (page < TAS2781_YRAM4_START_PAGE) > + ret = 0; > + else if (page <= TAS2781_YRAM4_END_PAGE) { > + if (reg > TAS2781_YRAM2_END_REG) > + ret = 0; > + else if (reg >= TAS2781_YRAM2_START_REG) { > + cd->offset = reg; > + cd->len = len; > + ret = 1; > + } else { > + if ((reg + (len-1)) > + < TAS2781_YRAM2_START_REG) > + ret = 0; > + else { > + cd->offset = > + TAS2781_YRAM2_START_REG; > + cd->len = > + reg + len - TAS2781_YRAM2_START_REG; > + ret = 1; > + } > + } > + } else > + ret = 0; > + } else > + ret = 0; > + > + return ret; > +} same here, error/sucess are inverted? > + > +static int check_yram(struct tas_crc *cd, unsigned char book, > + unsigned char page, unsigned char reg, unsigned char len) > +{ > + int ret = 0; useless init > + > + ret = check_inpage_yram(cd, book, page, reg, len); > + if (ret == 0) > + ret = check_inblock_yram(cd, book, > + page, reg, len); > + > + return ret; > +} > + > +static int do_singlereg_checksum(struct tasdevice_priv *tasdevice, > + enum channel chl, unsigned char book, unsigned char page, > + unsigned char reg, unsigned char val) > +{ > + int ret = 0; useless init > + struct tas_crc crc_data; > + unsigned int nData1 = 0; useless init > + > + if ((book == TASDEVICE_BOOK_ID(TAS2781_SA_COEFF_SWAP_REG)) > + && (page == TASDEVICE_PAGE_ID(TAS2781_SA_COEFF_SWAP_REG)) > + && (reg >= TASDEVICE_PAGE_REG(TAS2781_SA_COEFF_SWAP_REG)) > + && (reg <= (TASDEVICE_PAGE_REG( > + TAS2781_SA_COEFF_SWAP_REG) + 4))) { > + /*DSP swap command, pass */ > + ret = 0; > + goto end; > + } > + > + ret = check_yram(&crc_data, book, page, reg, 1); > + if (ret == 1) { > + ret = tasdevice_dev_read(tasdevice, chl, > + TASDEVICE_REG(book, page, reg), &nData1); > + if (ret < 0) > + goto end; > + > + if (nData1 != val) { > + dev_err(tasdevice->dev, > + "B[0x%x]P[0x%x]R[0x%x] W[0x%x], R[0x%x]\n", > + book, page, reg, > + val, nData1); > + ret = -EAGAIN; > + tasdevice->tasdevice[chl].err_code |= > + ERROR_YRAM_CRCCHK; > + ret = -EAGAIN; > + goto end; > + } > + > + ret = crc8(tasdevice->crc8_lkp_tbl, &val, 1, 0); > + } > + > +end: > + return ret; > +} > + > +static int do_multireg_checksum(struct tasdevice_priv *tasdevice, > + enum channel chn, unsigned char book, unsigned char page, > + unsigned char reg, unsigned int len) > +{ > + int ret = 0, i = 0; i does not need to be initialized. > + unsigned char crc_chksum = 0; > + unsigned char nBuf1[128] = {0}; > + struct tas_crc crc_data; > + > + if ((reg + len-1) > 127) { > + ret = -EINVAL; > + dev_err(tasdevice->dev, "firmware error\n"); > + goto end; > + } > + > + if ((book == TASDEVICE_BOOK_ID(TAS2781_SA_COEFF_SWAP_REG)) > + && (page == TASDEVICE_PAGE_ID(TAS2781_SA_COEFF_SWAP_REG)) > + && (reg == TASDEVICE_PAGE_REG(TAS2781_SA_COEFF_SWAP_REG)) > + && (len == 4)) { > + /*DSP swap command, pass */ /* DSP swap > + ret = 0; > + goto end; > + } > + > + ret = check_yram(&crc_data, book, page, reg, len); > + if (ret == 1) { > + if (len == 1) { > + dev_err(tasdevice->dev, "firmware error\n"); > + ret = -EINVAL; > + goto end; > + } else { > + ret = tasdevice_dev_bulk_read(tasdevice, chn, > + TASDEVICE_REG(book, page, crc_data.offset), > + nBuf1, crc_data.len); > + if (ret < 0) > + goto end; > + > + for (i = 0; i < crc_data.len; i++) { > + if ((book == TASDEVICE_BOOK_ID( > + TAS2781_SA_COEFF_SWAP_REG)) > + && (page == TASDEVICE_PAGE_ID( > + TAS2781_SA_COEFF_SWAP_REG)) > + && ((i + crc_data.offset) > + >= TASDEVICE_PAGE_REG( > + TAS2781_SA_COEFF_SWAP_REG)) > + && ((i + crc_data.offset) > + <= (TASDEVICE_PAGE_REG( > + TAS2781_SA_COEFF_SWAP_REG) > + + 4))) { > + /*DSP swap command, bypass */ > + continue; > + } else > + crc_chksum += > + crc8(tasdevice->crc8_lkp_tbl, > + &nBuf1[i], 1, 0); > + } > + > + ret = crc_chksum; > + } > + } > + > +end: > + return ret; > +} > + > +static int tasdevice_load_block(struct tasdevice_priv *tas_dev, > + struct tasdev_blk *block) > +{ > + int ret = 0; > + unsigned int nr_cmds = 0; > + unsigned char book = 0; > + unsigned char page = 0; > + unsigned char offset = 0; > + unsigned char val = 0; > + unsigned int len = 0; > + unsigned int sleep_dur = 0; > + unsigned char crc_chksum = 0; > + unsigned int nr_value = 0; > + int nr_retry = 6; > + unsigned char *data = block->data; > + int chn = 0, chnend = 0; I would guess most inits are not needed, except when they are... > + > + switch (block->type) { > + case MAIN_ALL_DEVICES: > + chn = 0; > + chnend = tas_dev->ndev; > + break; > + case MAIN_DEVICE_A: > + case COEFF_DEVICE_A: > + case PRE_DEVICE_A: > + chn = 0; > + chnend = 1; > + break; > + case MAIN_DEVICE_B: > + case COEFF_DEVICE_B: > + case PRE_DEVICE_B: > + chn = 1; > + chnend = 2; > + break; > + case MAIN_DEVICE_C: > + case COEFF_DEVICE_C: > + case PRE_DEVICE_C: > + chn = 2; > + chnend = 3; > + break; > + case MAIN_DEVICE_D: > + case COEFF_DEVICE_D: > + case PRE_DEVICE_D: > + chn = 3; > + chnend = 4; > + break; > + default: > + dev_dbg(tas_dev->dev, "load block: Other Type = 0x%02x\n", > + block->type); > + break; > + } > + > + for (; chn < chnend; chn++) { > + if (tas_dev->tasdevice[chn].is_loading == false) > + continue; > +start: > + if (block->is_pchksum_present) { > + ret = tasdevice_dev_write(tas_dev, chn, > + TASDEVICE_I2CChecksum, 0); > + if (ret < 0) > + goto end; > + } > + > + if (block->is_ychksum_present) > + crc_chksum = 0; > + > + nr_cmds = 0; > + > + while (nr_cmds < block->nr_cmds) { > + data = block->data + nr_cmds * 4; > + > + book = data[0]; > + page = data[1]; > + offset = data[2]; > + val = data[3]; > + > + nr_cmds++; > + > + if (offset <= 0x7F) { > + ret = tasdevice_dev_write(tas_dev, chn, > + TASDEVICE_REG(book, page, offset), > + val); > + if (ret < 0) > + goto end; > + if (block->is_ychksum_present) { > + ret = do_singlereg_checksum(tas_dev, > + chn, book, page, offset, > + val); > + if (ret < 0) > + goto check; > + crc_chksum += (unsigned char)ret; > + } > + continue; > + } > + if (offset == 0x81) { > + sleep_dur = (book << 8) + page; > + msleep(sleep_dur); > + continue; > + } > + if (offset == 0x85) { > + data += 4; > + len = (book << 8) + page; > + book = data[0]; > + page = data[1]; > + offset = data[2]; > + if (len > 1) { > + ret = tasdevice_dev_bulk_write( > + tas_dev, chn, > + TASDEVICE_REG(book, page, > + offset), data + 3, len); > + if (ret < 0) > + goto end; > + if (!block->is_ychksum_present) > + goto next; > + ret = do_multireg_checksum( > + tas_dev, chn, book, > + page, offset, > + len); > + if (ret < 0) > + goto check; > + crc_chksum += > + (unsigned char)ret; > + } else { > + ret = tasdevice_dev_write(tas_dev, > + chn, > + TASDEVICE_REG(book, page, > + offset), > + data[3]); > + if (ret < 0) > + goto end; > + if (!block->is_ychksum_present) > + goto next; > + ret = do_singlereg_checksum( > + tas_dev, chn, book, > + page, offset, > + data[3]); > + if (ret < 0) > + goto check; > + crc_chksum += > + (unsigned char)ret; > + } > +next: > + nr_cmds++; > + if (len >= 2) > + nr_cmds += ((len - 2) / 4) + 1; > + } > + } > + if (block->is_pchksum_present) { > + ret = tasdevice_dev_read(tas_dev, chn, > + TASDEVICE_I2CChecksum, &nr_value); > + if (ret < 0) { > + dev_err(tas_dev->dev, "%s: Channel %d\n", > + __func__, chn); > + goto check; > + } > + if ((nr_value & 0xff) != block->pchksum) { > + dev_err(tas_dev->dev, > + "Block PChkSum Channel %d ", chn); > + dev_err(tas_dev->dev, > + "FW = 0x%x, Reg = 0x%x\n", > + block->pchksum, > + (nr_value & 0xff)); > + ret = -EAGAIN; > + tas_dev->tasdevice[chn].err_code |= > + ERROR_PRAM_CRCCHK; > + goto check; > + } > + ret = 0; > + tas_dev->tasdevice[chn].err_code &= > + ~ERROR_PRAM_CRCCHK; > + } > + > + if (block->is_ychksum_present) { > + //TBD, open it when FW ready > + dev_err(tas_dev->dev, > + "Block YChkSum: FW = 0x%x, YCRC = 0x%x\n", > + block->ychksum, crc_chksum); > + > + tas_dev->tasdevice[chn].err_code &= > + ~ERROR_YRAM_CRCCHK; > + ret = 0; > + } > +check: > + if (ret == -EAGAIN) { > + nr_retry--; > + if (nr_retry > 0) > + goto start; > + else { > + if ((block->type == MAIN_ALL_DEVICES) > + || (block->type == MAIN_DEVICE_A) > + || (block->type == MAIN_DEVICE_B) > + || (block->type == MAIN_DEVICE_C) > + || (block->type == MAIN_DEVICE_D)) > + tas_dev->tasdevice[chn].cur_prog = -1; > + else > + tas_dev->tasdevice[chn].cur_conf = -1; > + nr_retry = 6; > + } > + } > + } > +end: > + if (ret < 0) > + dev_err(tas_dev->dev, "Block (%d) load error\n", block->type); > + > + return ret; > +} > + > + > +static int tasdevice_load_data(struct tasdevice_priv *tas_dev, > + struct tasdevice_data *dev_data) > +{ > + int ret = 0; required init > + unsigned int nr_block = 0; > + struct tasdev_blk *block = NULL; useless inits > + > + for (nr_block = 0; nr_block < dev_data->nr_blk; nr_block++) { > + block = &(dev_data->dev_blks[nr_block]); > + ret = tas_dev->tasdevice_load_block(tas_dev, block); > + if (ret < 0) > + break; > + } > + > + return ret; > +} > + > +static int tasdevice_load_calibrated_data( > + struct tasdevice_priv *tas_dev, struct tasdevice_data *dev_data) > +{ > + int ret = 0; > + unsigned int nr_block = 0; > + struct tasdev_blk *block = NULL; > + > + for (nr_block = 0; nr_block < dev_data->nr_blk; nr_block++) { > + block = &(dev_data->dev_blks[nr_block]); > + ret = tasdevice_load_block(tas_dev, block); > + if (ret < 0) > + break; > + } > + > + return ret; > +} > + > +int tas2781_load_calibration(void *context, > + char *file_name, enum channel i) > +{ > + int ret = 0, offset = 0; > + struct firmware fmw; > + const struct firmware *fw_entry = NULL; > + struct tasdevice_priv *tas_dev = (struct tasdevice_priv *)context; > + struct tasdevice *tasdev = &(tas_dev->tasdevice[i]); > + struct tasdevice_fw *tas_fmw = NULL; > + > + ret = request_firmware(&fw_entry, file_name, tas_dev->dev); > + if (!ret) { > + if (!fw_entry->size) { > + dev_err(tas_dev->dev, > + "%s: file read error: size = %lu\n", > + __func__, (unsigned long)fw_entry->size); > + goto out; > + } > + fmw.size = fw_entry->size; > + fmw.data = fw_entry->data; > + } else { > + dev_info(tas_dev->dev, > + "%s: Request firmware %s failed\n", > + __func__, file_name); > + goto out; > + } > + > + tas_fmw = tasdev->cali_data_fmw = kcalloc(1, > + sizeof(struct tasdevice_fw), GFP_KERNEL); > + if (tasdev->cali_data_fmw == NULL) { > + dev_err(tas_dev->dev, "%s: FW memory failed!\n", __func__); > + ret = -1; > + goto out; > + } > + tas_fmw->dev = tas_dev->dev; > + offset = fw_parse_header(tas_dev, tas_fmw, &fmw, offset); > + if (offset == -1) { > + dev_err(tas_dev->dev, "%s: fw_parse_header EXIT!\n", __func__); > + goto out; > + } > + offset = fw_parse_variable_hdr_cal(tas_dev, tas_fmw, &fmw, offset); > + if (offset == -1) { > + dev_err(tas_dev->dev, > + "%s: fw_parse_variable_header_cal EXIT!\n", __func__); > + goto out; > + } > + offset = fw_parse_program_data(tas_dev, tas_fmw, &fmw, offset); > + if (offset == -1) { > + dev_err(tas_dev->dev, "%s: fw_parse_program_data EXIT!\n", > + __func__); > + goto out; > + } > + offset = fw_parse_configuration_data(tas_dev, tas_fmw, &fmw, > + offset); > + if (offset == -1) { > + dev_err(tas_dev->dev, > + "%s: fw_parse_configuration_data EXIT!\n", __func__); > + goto out; > + } > + offset = fw_parse_calibration_data(tas_dev, > + tas_fmw, &fmw, offset); > + if (offset == -1) { > + dev_err(tas_dev->dev, "%s: fw_parse_calibration_data EXIT!\n", > + __func__); > + goto out; > + } > + tasdev->is_calibrated_data_loaded = true; > +out: > + if (fw_entry) { > + release_firmware(fw_entry); > + fw_entry = NULL; > + } > + return ret; > +} > + > +int tasdevice_dspfw_ready(const struct firmware *fmw, void *context) > +{ > + struct tasdevice_priv *tas_dev = (struct tasdevice_priv *) context; > + struct tasdevice_fw *tas_fmw = NULL; > + struct tasdevice_fw_fixed_hdr *fw_fixed_hdr = NULL; > + int offset = 0, ret = 0; > + > + if (!fmw || !fmw->data) { > + dev_err(tas_dev->dev, "%s: Failed to read firmware %s\n", > + __func__, tas_dev->coef_binaryname); > + ret = -1; > + goto out; > + } > + > + tas_dev->fmw = kcalloc(1, > + sizeof(struct tasdevice_fw), GFP_KERNEL); > + if (tas_dev->fmw == NULL) { > + ret = -1; > + goto out; > + } > + tas_fmw = tas_dev->fmw; > + tas_fmw->dev = tas_dev->dev; > + offset = fw_parse_header(tas_dev, tas_fmw, fmw, offset); > + > + if (offset == -1) > + goto out; > + fw_fixed_hdr = &(tas_fmw->fw_hdr.fixed_hdr); > + switch (fw_fixed_hdr->drv_ver) { > + case 0x301: > + case 0x302: > + case 0x502: > + tas_dev->fw_parse_variable_header = > + fw_parse_variable_header_kernel; > + tas_dev->fw_parse_program_data = > + fw_parse_program_data_kernel; > + tas_dev->fw_parse_configuration_data = > + fw_parse_configuration_data_kernel; > + tas_dev->tasdevice_load_block = > + tasdevice_load_block_kernel; > + break; > + case 0x202: > + case 0x400: > + tas_dev->fw_parse_variable_header = > + fw_parse_variable_header_git; > + tas_dev->fw_parse_program_data = > + fw_parse_program_data; > + tas_dev->fw_parse_configuration_data = > + fw_parse_configuration_data; > + tas_dev->tasdevice_load_block = > + tasdevice_load_block; > + break; > + default: > + if (fw_fixed_hdr->drv_ver == 0x100) { > + if (fw_fixed_hdr->ppcver >= PPC3_VERSION) { > + tas_dev->fw_parse_variable_header = > + fw_parse_variable_header_kernel; > + tas_dev->fw_parse_program_data = > + fw_parse_program_data_kernel; > + tas_dev->fw_parse_configuration_data = > + fw_parse_configuration_data_kernel; > + tas_dev->tasdevice_load_block = > + tasdevice_load_block_kernel; > + tas_dev->fw_parse_calibration_data = NULL; > + } else { > + switch (fw_fixed_hdr->ppcver) { > + case 0x00: > + tas_dev->fw_parse_variable_header = > + fw_parse_variable_header_git; > + tas_dev->fw_parse_program_data = > + fw_parse_program_data; > + tas_dev->fw_parse_configuration_data = > + fw_parse_configuration_data; > + tas_dev->fw_parse_calibration_data = > + fw_parse_calibration_data; > + tas_dev->tasdevice_load_block = > + tasdevice_load_block; > + break; > + default: > + dev_err(tas_dev->dev, > + "%s: PPCVersion must be 0x0 or 0x%02x", > + __func__, PPC3_VERSION); > + dev_err(tas_dev->dev, " Current:0x%02x\n", > + fw_fixed_hdr->ppcver); > + offset = -1; > + break; > + } > + } > + } else { > + dev_err(tas_dev->dev, > + "%s: DriverVersion must be 0x0, 0x230 or above 0x230 ", > + __func__); > + dev_err(tas_dev->dev, "current is 0x%02x\n", > + fw_fixed_hdr->drv_ver); > + offset = -1; > + } > + break; > + } > + > + offset = tas_dev->fw_parse_variable_header(tas_dev, fmw, offset); > + if (offset == -1) > + goto out; > + > + offset = tas_dev->fw_parse_program_data(tas_dev, tas_fmw, fmw, > + offset); > + if (offset < 0) { > + ret = -1; > + goto out; > + } > + offset = tas_dev->fw_parse_configuration_data(tas_dev, > + tas_fmw, fmw, offset); > + if (offset < 0) > + ret = -1; > + > +out: > + return ret; > +} > + > +void tasdevice_calbin_remove(void *context) > +{ > + struct tasdevice_priv *tas_dev = (struct tasdevice_priv *) context; > + struct tasdevice *tasdev = NULL; > + int i = 0; last two inits are useless > + > + if (tas_dev) { > + for (i = 0; i < tas_dev->ndev; i++) { > + tasdev = &(tas_dev->tasdevice[i]); > + if (tasdev->cali_data_fmw) { > + tas2781_clear_calfirmware( > + tasdev->cali_data_fmw); > + tasdev->cali_data_fmw = NULL; > + } > + } > + } > +} > + > +void tasdevice_dsp_remove(void *context) > +{ > + struct tasdevice_priv *tas_dev = (struct tasdevice_priv *) context; > + int i; > + struct tasdev_blk *blk; > + unsigned int nr_blk; > + > + if (tas_dev->fmw) { > + struct tasdevice_fw *tas_fmw = tas_dev->fmw; > + > + if (tas_fmw->programs) { > + struct tasdevice_prog *program; > + > + for (i = 0; i < tas_fmw->nr_programs; i++) { > + program = &(tas_fmw->programs[i]); > + if (program) { > + struct tasdevice_data > + *im = &(program->dev_data); > + > + if (!im->dev_blks) > + continue; > + > + for (nr_blk = 0; nr_blk < im->nr_blk; > + nr_blk++) { > + blk = > + &(im->dev_blks[nr_blk]); > + kfree(blk->data); indentation is awful. > + } > + kfree(im->dev_blks); > + } > + } > + kfree(tas_fmw->programs); > + } > + > + if (tas_fmw->configs) { > + struct tasdevice_config *config; > + > + for (i = 0; i < tas_fmw->nr_configurations; i++) { > + config = &(tas_fmw->configs[i]); > + if (config) { > + struct tasdevice_data > + *im = &(config->dev_data); > + > + if (!im->dev_blks) > + continue; > + for (nr_blk = 0; nr_blk < im->nr_blk; > + nr_blk++) { > + blk = > + &(im->dev_blks[nr_blk]); > + kfree(blk->data); > + } > + kfree(im->dev_blks); > + } > + } > + kfree(tas_fmw->configs); > + } > + kfree(tas_fmw); > + tas_dev->fmw = NULL; > + } > +} > + > +static int tas2781_set_calibration(void *context, enum channel i, > + int n_calibration) > +{ > + struct tasdevice_priv *tas_dev = (struct tasdevice_priv *) context; > + int ret = 0; > + struct tasdevice *tasdevice = &(tas_dev->tasdevice[i]); > + struct tasdevice_fw *cal_fmw = tasdevice->cali_data_fmw; > + > + if ((!tas_dev->fmw->programs) too many parentheses > + || (!tas_dev->fmw->configs)) { > + dev_err(tas_dev->dev, "%s, Firmware not loaded\n\r", __func__); > + ret = 0; > + goto out; > + } > + > + if (n_calibration == 0xFF || (n_calibration == 0x100 > + && tasdevice->is_calibrated_data_loaded == false)) { > + if (cal_fmw) { > + tasdevice->is_calibrated_data_loaded = false; > + tas2781_clear_calfirmware(cal_fmw); > + cal_fmw = NULL; > + } > + > + scnprintf(tas_dev->cal_binaryname[i], 64, "%s_cal_0x%02x.bin", > + tas_dev->dev_name, tas_dev->tasdevice[i].dev_addr); > + ret = tas2781_load_calibration(tas_dev, > + tas_dev->cal_binaryname[i], i); > + if (ret != 0) { > + dev_err(tas_dev->dev, > + "%s: load %s error, no-side effect\n", > + __func__, tas_dev->cal_binaryname[i]); > + ret = 0; > + } > + } > + tasdevice->is_loading = true; > + tasdevice->is_loaderr = false; > + > + if (cal_fmw) { > + struct tasdevice_calibration *calibration = > + cal_fmw->calibrations; > + > + if (calibration) > + tasdevice_load_calibrated_data(tas_dev, > + &(calibration->dev_data)); > + } else > + dev_err(tas_dev->dev, > + "%s: No calibrated data for device %d\n", __func__, i); > + > +out: > + return ret; > +} > + > +int tasdevice_select_tuningprm_cfg(void *context, int prm_no, > + int cfg_no, int rca_conf_no) > +{ > + struct tasdevice_priv *tas_dev = (struct tasdevice_priv *) context; > + struct tasdevice_rca *rca = &(tas_dev->rcabin); > + struct tasdevice_config_info **cfg_info = rca->cfg_info; > + struct tasdevice_fw *tas_fmw = tas_dev->fmw; > + struct tasdevice_config *conf = NULL; > + struct tasdevice_prog *program = NULL; > + int i = 0; > + int status = 0; > + int prog_status = 0; check inits please. > + > + if (tas_fmw == NULL) { if (!tas_fmw) > + dev_err(tas_dev->dev, "%s: Firmware is NULL\n", __func__); > + goto out; > + } > + > + if (cfg_no >= tas_fmw->nr_configurations) { > + dev_err(tas_dev->dev, > + "%s: cfg(%d) is not in range of conf %u\n", > + __func__, cfg_no, tas_fmw->nr_configurations); > + goto out; > + } > + > + if (prm_no >= tas_fmw->nr_programs) { > + dev_err(tas_dev->dev, > + "%s: prm(%d) is not in range of Programs %u\n", > + __func__, prm_no, tas_fmw->nr_programs); > + goto out; > + } > + > + if (rca_conf_no > rca->ncfgs || rca_conf_no <= 0 || > + cfg_info == NULL) { > + dev_err(tas_dev->dev, > + "conf_no:%d should be in range from 0 to %u\n", > + rca_conf_no, rca->ncfgs-1); > + goto out; > + } > + > + conf = &(tas_fmw->configs[cfg_no]); > + for (i = 0; i < tas_dev->ndev; i++) { > + if (cfg_info[rca_conf_no]->active_dev & (1 << i)) { > + if (tas_dev->tasdevice[i].cur_prog != prm_no) { > + tas_dev->tasdevice[i].cur_conf = -1; > + tas_dev->tasdevice[i].is_loading = true; > + prog_status++; > + } > + } else > + tas_dev->tasdevice[i].is_loading = false; > + tas_dev->tasdevice[i].is_loaderr = false; > + } > + > + if (prog_status) { > + program = &(tas_fmw->programs[prm_no]); > + tasdevice_load_data(tas_dev, &(program->dev_data)); > + for (i = 0; i < tas_dev->ndev; i++) { > + if (tas_dev->tasdevice[i].is_loaderr == true) > + continue; > + else if (tas_dev->tasdevice[i].is_loaderr == false > + && tas_dev->tasdevice[i].is_loading == true) { > + struct tasdevice_fw *cal_fmw = > + tas_dev->tasdevice[i].cali_data_fmw; > + > + if (cal_fmw) { > + struct tasdevice_calibration > + *calibration = > + cal_fmw->calibrations; > + > + if (calibration) > + tasdevice_load_calibrated_data( > + tas_dev, > + &(calibration->dev_data)); > + } > + tas_dev->tasdevice[i].cur_prog = prm_no; > + } > + } > + } > + > + if (tas_dev->is_glb_calibrated_data_loaded == false) { > + for (i = 0; i < tas_dev->ndev; i++) > + tas2781_set_calibration(tas_dev, i, 0x100); > + tas_dev->is_glb_calibrated_data_loaded = true; > + /* No wise to reload calibrationdata everytime, > + * this code will work once even if calibrated > + * data still failed to be got > + */ > + } > + > + for (i = 0; i < tas_dev->ndev; i++) { > + if (tas_dev->tasdevice[i].cur_conf != cfg_no > + && (cfg_info[rca_conf_no]->active_dev & (1 << i)) > + && (tas_dev->tasdevice[i].is_loaderr == false)) { > + status++; > + tas_dev->tasdevice[i].is_loading = true; > + } else > + tas_dev->tasdevice[i].is_loading = false; > + } > + > + if (status) { > + status = 0; > + tasdevice_load_data(tas_dev, &(conf->dev_data)); > + for (i = 0; i < tas_dev->ndev; i++) { > + if (tas_dev->tasdevice[i].is_loaderr == true) { > + status |= 1 << (i + 4); > + continue; > + } else if (tas_dev->tasdevice[i].is_loaderr == false > + && tas_dev->tasdevice[i].is_loading == true) > + tas_dev->tasdevice[i].cur_conf = cfg_no; > + } > + } else > + dev_err(tas_dev->dev, > + "%s: No device is in active in conf %d\n", > + __func__, rca_conf_no); > + > + status |= cfg_info[rca_conf_no]->active_dev; > +out: > + return prog_status; > diff --git a/sound/soc/codecs/tas2781-i2c.c b/sound/soc/codecs/tas2781-i2c.c > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include alphabetical order? > + > +int tasdevice_process_block(void *context, > + unsigned char *data, unsigned char dev_idx, int sublocksize) > +{ > + struct tasdevice_priv *tas_dev = > + (struct tasdevice_priv *)context; > + unsigned char subblk_typ = data[1]; > + int subblk_offset = 2; > + int chn = 0, chnend = 0; > + int rc = 0; > + int blktyp = dev_idx & 0xC0, idx = dev_idx & 0x3F; separate lines, this is awful to read. > + bool is_err = false; > + > + if (idx) { > + chn = idx-1; > + chnend = idx; > + } else { > + if (tas_dev->set_global_mode) { > + chn = tas_dev->ndev; > + chnend = tas_dev->ndev + 1; > + } else { > + chn = 0; > + chnend = tas_dev->ndev; > + } > + } > + > + for (; chn < chnend; chn++) { > + if (tas_dev->set_global_mode == NULL && !tas_dev->set_global_mode > + tas_dev->tasdevice[chn].is_loading == false) > + continue; > + > + is_err = false; > + subblk_offset = 2; > + switch (subblk_typ) { > + case TASDEVICE_CMD_SING_W: { > + int i = 0; > + unsigned short len = SMS_HTONS(data[2], data[3]); > + > + subblk_offset += 2; > + if (subblk_offset + 4 * len > sublocksize) { > + dev_err(tas_dev->dev, > + "process_block: Out of bounary\n"); > + is_err = true; > + break; > + } > + > + for (i = 0; i < len; i++) { > + rc = tasdevice_dev_write(tas_dev, chn, > + TASDEVICE_REG(data[subblk_offset], > + data[subblk_offset + 1], > + data[subblk_offset + 2]), > + data[subblk_offset + 3]); > + if (rc < 0) { > + is_err = true; > + dev_err(tas_dev->dev, > + "process_block: single write error\n"); > + } > + subblk_offset += 4; > + } > + } > + break; > + case TASDEVICE_CMD_BURST: { > + unsigned short len = > + SMS_HTONS(data[2], data[3]); > + > + subblk_offset += 2; > + if (subblk_offset + 4 + len > sublocksize) { > + dev_err(tas_dev->dev, > + "%s: BST Out of bounary\n", __func__); > + is_err = true; > + break; > + } > + if (len % 4) { > + dev_err(tas_dev->dev, > + "%s:Bst-len(%u)not div by 4\n", > + __func__, len); > + break; > + } > + > + rc = tasdevice_dev_bulk_write(tas_dev, chn, > + TASDEVICE_REG(data[subblk_offset], > + data[subblk_offset + 1], > + data[subblk_offset + 2]), > + &(data[subblk_offset + 4]), > + len); > + if (rc < 0) { > + is_err = true; > + dev_err(tas_dev->dev, > + "%s: bulk_write error = %d\n", > + __func__, rc); > + } > + subblk_offset += (len + 4); > + } > + break; > + case TASDEVICE_CMD_DELAY: { > + unsigned short delay_time = 0; > + > + if (subblk_offset + 2 > sublocksize) { > + dev_err(tas_dev->dev, > + "%s: deley Out of bounary\n", > + __func__); > + is_err = true; > + break; > + } > + delay_time = SMS_HTONS(data[2], data[3]); > + usleep_range(delay_time*1000, delay_time*1000); > + subblk_offset += 2; > + } > + break; > + case TASDEVICE_CMD_FIELD_W: > + if (subblk_offset + 6 > sublocksize) { > + dev_err(tas_dev->dev, > + "%s: bit write Out of bounary\n", > + __func__); > + is_err = true; > + break; > + } > + rc = tasdevice_dev_update_bits(tas_dev, chn, > + TASDEVICE_REG(data[subblk_offset + 2], > + data[subblk_offset + 3], > + data[subblk_offset + 4]), > + data[subblk_offset + 1], > + data[subblk_offset + 5]); > + if (rc < 0) { > + is_err = true; > + dev_err(tas_dev->dev, > + "%s: update_bits error = %d\n", > + __func__, rc); > + } > + subblk_offset += 6; > + break; > + default: > + break; > + }; > + if (is_err == true && blktyp != 0) { > + if (blktyp == 0x80) { > + tas_dev->tasdevice[chn].cur_prog = -1; > + tas_dev->tasdevice[chn].cur_conf = -1; > + } else > + tas_dev->tasdevice[chn].cur_conf = -1; > + } > + } > + return subblk_offset; > +} > + > +static void tasdevice_select_cfg_blk(void *pContext, int conf_no, > + unsigned char block_type) > +{ > + struct tasdevice_priv *tas_dev = > + (struct tasdevice_priv *) pContext; > + struct tasdevice_rca *rca = &(tas_dev->rcabin); > + struct tasdevice_config_info **cfg_info = rca->cfg_info; > + int j = 0, k = 0, chn = 0, chnend = 0; inits? > + > + if (conf_no >= rca->ncfgs || conf_no < 0 || NULL == cfg_info) { > + dev_err(tas_dev->dev, > + "conf_no should be not more than %u\n", > + rca->ncfgs); > + goto out; > + } > + > + for (j = 0; j < (int)cfg_info[conf_no]->real_nblocks; j++) { > + unsigned int length = 0, rc = 0; > + > + if (block_type > 5 || block_type < 2) { > + dev_err(tas_dev->dev, > + "ERROR!!!block_type should be in range from 2 to 5\n"); > + goto out; > + } > + if (block_type != cfg_info[conf_no]->blk_data[j]->block_type) > + continue; > + > + for (k = 0; k < (int)cfg_info[conf_no]->blk_data[j] > + ->n_subblks; k++) { > + if (cfg_info[conf_no]->blk_data[j]->dev_idx) { > + chn = > + cfg_info[conf_no]->blk_data[j]->dev_idx > + - 1; > + chnend = > + cfg_info[conf_no]->blk_data[j]->dev_idx; > + } else { > + chn = 0; > + chnend = tas_dev->ndev; > + } > + for (; chn < chnend; chn++) > + tas_dev->tasdevice[chn].is_loading = true; > + > + rc = tasdevice_process_block(tas_dev, > + cfg_info[conf_no]->blk_data[j]->regdata + > + length, > + cfg_info[conf_no]->blk_data[j]->dev_idx, > + cfg_info[conf_no]->blk_data[j]->block_size - > + length); > + length += rc; > + if (cfg_info[conf_no]->blk_data[j]->block_size < > + length) { > + dev_err(tas_dev->dev, > + "%s: ERROR:%u %u out of bounary\n", > + __func__, length, > + cfg_info[conf_no]->blk_data[j] > + ->block_size); > + break; > + } > + } > + if (length != cfg_info[conf_no]->blk_data[j]->block_size) > + dev_err(tas_dev->dev, > + "%s: %u %u size is not same\n", > + __func__, length, > + cfg_info[conf_no]->blk_data[j]->block_size); > + > + } > + > +out: > + return; > +} > + > +static struct tasdevice_config_info *tasdevice_add_config( > + void *context, unsigned char *config_data, > + unsigned int config_size) > +{ > + struct tasdevice_priv *tas_dev = > + (struct tasdevice_priv *)context; > + struct tasdevice_config_info *cfg_info = NULL; > + int config_offset = 0, i = 0; inits? > + > + cfg_info = kzalloc( > + sizeof(struct tasdevice_config_info), GFP_KERNEL); > + if (!cfg_info) > + goto out; > + > + if (tas_dev->rcabin.fw_hdr.binary_version_num >= 0x105) { > + if (config_offset + 64 > (int)config_size) { > + dev_err(tas_dev->dev, > + "add config: Out of bounary\n"); > + goto out; > + } > + config_offset += 64; > + } > + > + if (config_offset + 4 > (int)config_size) { > + dev_err(tas_dev->dev, > + "add config: Out of bounary\n"); > + goto out; > + } > + cfg_info->nblocks = > + SMS_HTONL(config_data[config_offset], > + config_data[config_offset + 1], > + config_data[config_offset + 2], config_data[config_offset + 3]); > + config_offset += 4; > + > + cfg_info->blk_data = kcalloc( > + cfg_info->nblocks, sizeof(struct tasdev_blk_data *), > + GFP_KERNEL); > + if (!cfg_info->blk_data) > + goto out; > + > + cfg_info->real_nblocks = 0; > + for (i = 0; i < (int)cfg_info->nblocks; i++) { > + if (config_offset + 12 > config_size) { > + dev_err(tas_dev->dev, > + "add config: Out of bounary: i = %d nblocks = %u!\n", > + i, cfg_info->nblocks); > + break; > + } > + cfg_info->blk_data[i] = kzalloc( > + sizeof(struct tasdev_blk_data), GFP_KERNEL); > + if (!cfg_info->blk_data[i]) > + break; > + > + cfg_info->blk_data[i]->dev_idx = config_data[config_offset]; > + config_offset++; > + > + cfg_info->blk_data[i]->block_type = config_data[config_offset]; > + config_offset++; > + > + if (cfg_info->blk_data[i]->block_type == > + TASDEVICE_BIN_BLK_PRE_POWER_UP) { > + if (cfg_info->blk_data[i]->dev_idx == 0) { > + cfg_info->active_dev = 1; > + } else { > + cfg_info->active_dev = > + 1 << > + (cfg_info->blk_data[i]->dev_idx - 1); > + } > + } > + cfg_info->blk_data[i]->yram_checksum = > + SMS_HTONS(config_data[config_offset], > + config_data[config_offset + 1]); > + config_offset += 2; > + cfg_info->blk_data[i]->block_size = > + SMS_HTONL(config_data[config_offset], > + config_data[config_offset + 1], > + config_data[config_offset + 2], > + config_data[config_offset + 3]); > + config_offset += 4; > + > + cfg_info->blk_data[i]->n_subblks = > + SMS_HTONL(config_data[config_offset], > + config_data[config_offset + 1], > + config_data[config_offset + 2], > + config_data[config_offset + 3]); > + > + config_offset += 4; > + cfg_info->blk_data[i]->regdata = kzalloc( > + cfg_info->blk_data[i]->block_size, GFP_KERNEL); > + if (cfg_info->blk_data[i]->regdata == 0) > + goto out; > + > + if (config_offset + cfg_info->blk_data[i]->block_size > + > config_size) { > + dev_err(tas_dev->dev, > + "%s: block_size Out of bounary: i = %d blks = %u!\n", > + __func__, i, cfg_info->nblocks); > + break; > + } > + memcpy(cfg_info->blk_data[i]->regdata, > + &config_data[config_offset], > + cfg_info->blk_data[i]->block_size); > + config_offset += cfg_info->blk_data[i]->block_size; > + cfg_info->real_nblocks += 1; > + } > +out: > + return cfg_info; > +} > + > +// tas2781 contain book and page two-level regmap, especially book switching > +// will set two registers, one is BXXPXXR00, the other is B0P0R7F, after > +// switching to the correct book, then leverage the mechanism for paging to > +// access the register. Custom control is primarily for regmap booking, > +// paging depends on internal regmap mechanism C comments? > +static int tas2781_digital_getvol(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) > +{ > + struct snd_soc_component *codec > + = snd_soc_kcontrol_component(kcontrol); > + struct tasdevice_priv *tas_dev = > + snd_soc_component_get_drvdata(codec); > + struct soc_mixer_control *mc = > + (struct soc_mixer_control *)kcontrol->private_value; > + int val; > + unsigned int invert = mc->invert; > + int max = mc->max; > + int ret = 0; > + > + /* Read the primary device as the whole */ > + ret = tasdevice_dev_read(tas_dev, 0, mc->reg, &val); > + if (ret) { > + dev_err(tas_dev->dev, > + "%s, get digital vol error\n", > + __func__); > + goto out; > + } > + > + if (val > max) > + val = max; > + if (invert) > + val = max - val; > + if (val < 0) > + val = 0; > + ucontrol->value.integer.value[0] = val; > + > +out: > + return ret; > +} > + > +static int tas2781_digital_putvol(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) > +{ > + struct snd_soc_component *codec > + = snd_soc_kcontrol_component(kcontrol); > + struct tasdevice_priv *tas_dev = > + snd_soc_component_get_drvdata(codec); > + struct soc_mixer_control *mc = > + (struct soc_mixer_control *)kcontrol->private_value; > + int val; > + unsigned int invert = mc->invert; > + int max = mc->max; > + int i, ret = 0; > + > + val = ucontrol->value.integer.value[0]; > + if (val > max) > + val = max; > + if (invert) > + val = max - val; > + if (val < 0) > + val = 0; > + > + if (tas_dev->set_global_mode != NULL) { > + ret = tasdevice_dev_write(tas_dev, tas_dev->ndev, > + mc->reg, (unsigned int)val); > + if (ret) > + dev_err(tas_dev->dev, > + "%s, set digital vol error in global mode\n", > + __func__); indentation is awful > + } else { > + for (i = 0; i < tas_dev->ndev; i++) { > + ret = tasdevice_dev_write(tas_dev, i, > + mc->reg, (unsigned int)val); > + if (ret) > + dev_err(tas_dev->dev, > + "%s, set digital vol error in device %d\n", > + __func__, i); > + } > + } > + > + return 1; is this correct in case of errors? > +} > + > +static int tas2781_amp_getvol(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) > +{ > + struct snd_soc_component *codec > + = snd_soc_kcontrol_component(kcontrol); > + struct tasdevice_priv *tas_dev = > + snd_soc_component_get_drvdata(codec); > + struct soc_mixer_control *mc = > + (struct soc_mixer_control *)kcontrol->private_value; > + int val; > + unsigned char mask = 0; > + int max = mc->max; > + int ret = 0; > + unsigned int invert = mc->invert; > + > + /* Read the primary device */ > + ret = tasdevice_dev_read(tas_dev, 0, mc->reg, &val); > + if (ret) { > + dev_err(tas_dev->dev, > + "%s, get AMP vol error\n", > + __func__); > + goto out; > + } > + > + mask = (1 << fls(mc->max)) - 1; > + mask <<= mc->shift; > + val = (val & mask) >> mc->shift; > + if (val > max) > + val = max; > + if (invert) > + val = max - val; > + if (val < 0) > + val = 0; > + ucontrol->value.integer.value[0] = val; > + > +out: > + return ret; > +} > + > +static int tas2781_amp_putvol(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) > +{ > + struct snd_soc_component *codec > + = snd_soc_kcontrol_component(kcontrol); > + struct tasdevice_priv *tas_dev = > + snd_soc_component_get_drvdata(codec); > + struct soc_mixer_control *mc = > + (struct soc_mixer_control *)kcontrol->private_value; > + int val; > + int i, ret = 0; > + unsigned char mask = 0; > + int max = mc->max; > + unsigned int invert = mc->invert; > + > + mask = (1 << fls(mc->max)) - 1; > + mask <<= mc->shift; > + val = ucontrol->value.integer.value[0]; > + if (val > max) > + val = max; > + if (invert) > + val = max - val; > + if (val < 0) > + val = 0; > + for (i = 0; i < tas_dev->ndev; i++) { > + ret = tasdevice_dev_update_bits(tas_dev, i, > + mc->reg, mask, (unsigned int)(val << mc->shift)); > + if (ret) > + dev_err(tas_dev->dev, > + "%s, set AMP vol error in device %d\n", > + __func__, i); > + } same is this correct to return 1 even for errors? > + > + return 1; > +} > +static void tasdevice_rca_ready(const struct firmware *fmw, > + void *context) > +{ > + struct tasdevice_priv *tas_dev = > + (struct tasdevice_priv *) context; > + struct tasdevice_rca *rca = NULL; > + struct tasdevice_rca_hdr *fw_hdr = NULL; > + struct tasdevice_config_info **cfg_info = NULL; > + const struct firmware *fw_entry = NULL; > + unsigned char *buf = NULL; > + int offset = 0, i = 0; > + unsigned int total_config_sz = 0; > + int ret = 0; check inits. > + > + mutex_lock(&tas_dev->codec_lock); > + rca = &(tas_dev->rcabin); > + fw_hdr = &(rca->fw_hdr); > + if (!fmw || !fmw->data) { > + dev_err(tas_dev->dev, > + "Failed to read %s, no side - effect on driver running\n", > + tas_dev->rca_binaryname); > + ret = -1; > + goto out; > + } > + buf = (unsigned char *)fmw->data; > + > + fw_hdr->img_sz = SMS_HTONL(buf[offset], buf[offset + 1], > + buf[offset + 2], buf[offset + 3]); > + offset += 4; > + if (fw_hdr->img_sz != fmw->size) { > + dev_err(tas_dev->dev, > + "File size not match, %d %u", (int)fmw->size, > + fw_hdr->img_sz); > + ret = -1; > + goto out; > + } > + > + fw_hdr->checksum = SMS_HTONL(buf[offset], buf[offset + 1], > + buf[offset + 2], buf[offset + 3]); > + offset += 4; > + fw_hdr->binary_version_num = SMS_HTONL(buf[offset], > + buf[offset + 1], buf[offset + 2], buf[offset + 3]); > + if (fw_hdr->binary_version_num < 0x103) { > + dev_err(tas_dev->dev, > + "File version 0x%04x is too low", > + fw_hdr->binary_version_num); > + ret = -1; > + goto out; > + } > + offset += 4; > + fw_hdr->drv_fw_version = SMS_HTONL(buf[offset], buf[offset + 1], > + buf[offset + 2], buf[offset + 3]); > + offset += 8; > + fw_hdr->plat_type = buf[offset]; > + offset += 1; > + fw_hdr->dev_family = buf[offset]; > + offset += 1; > + fw_hdr->reserve = buf[offset]; > + offset += 1; > + fw_hdr->ndev = buf[offset]; > + offset += 1; > + if (fw_hdr->ndev != tas_dev->ndev) { > + dev_err(tas_dev->dev, > + "ndev(%u) from rcabin and ndev(%u) from DTS does not match\n", > + fw_hdr->ndev, > + tas_dev->ndev); > + ret = -1; > + goto out; > + } > + if (offset + TASDEVICE_DEVICE_SUM > fw_hdr->img_sz) { > + dev_err(tas_dev->dev, > + "rca_ready: Out of bounary!\n"); > + ret = -1; > + goto out; > + } > + > + for (i = 0; i < TASDEVICE_DEVICE_SUM; i++, offset++) > + fw_hdr->devs[i] = buf[offset]; > + > + fw_hdr->nconfig = SMS_HTONL(buf[offset], buf[offset + 1], > + buf[offset + 2], buf[offset + 3]); > + offset += 4; > + > + for (i = 0; i < TASDEVICE_CONFIG_SUM; i++) { > + fw_hdr->config_size[i] = SMS_HTONL(buf[offset], > + buf[offset + 1], buf[offset + 2], buf[offset + 3]); > + offset += 4; > + total_config_sz += fw_hdr->config_size[i]; > + } > + > + if (fw_hdr->img_sz - total_config_sz != (unsigned int)offset) { > + dev_err(tas_dev->dev, "Bin file error!\n"); > + ret = -1; > + goto out; > + } > + cfg_info = kcalloc(fw_hdr->nconfig, > + sizeof(struct tasdevice_config_info *), > + GFP_KERNEL); > + > + if (!cfg_info) { > + ret = -1; > + goto out; > + } > + rca->cfg_info = cfg_info; > + rca->ncfgs = 0; > + for (i = 0; i < (int)fw_hdr->nconfig; i++) { > + cfg_info[i] = tasdevice_add_config(context, &buf[offset], > + fw_hdr->config_size[i]); > + if (!cfg_info[i]) { > + ret = -1; > + break; > + } > + offset += (int)fw_hdr->config_size[i]; > + rca->ncfgs += 1; > + } > + tasdevice_create_controls(tas_dev); > + > + tasdevice_dsp_remove(tas_dev); > + tasdevice_calbin_remove(tas_dev); > + tas_dev->fw_state = TASDEVICE_DSP_FW_PENDING; > + scnprintf(tas_dev->coef_binaryname, 64, "%s_coef.bin", > + tas_dev->dev_name); > + ret = request_firmware(&fw_entry, tas_dev->coef_binaryname, > + tas_dev->dev); > + if (!ret) { > + ret = tasdevice_dspfw_ready(fw_entry, tas_dev); > + release_firmware(fw_entry); > + fw_entry = NULL; > + } else { > + tas_dev->fw_state = TASDEVICE_DSP_FW_FAIL; > + dev_err(tas_dev->dev, "%s: load %s error\n", __func__, > + tas_dev->coef_binaryname); > + goto out; > + } > + tasdevice_dsp_create_control(tas_dev); > + > + tas_dev->fw_state = TASDEVICE_DSP_FW_ALL_OK; > + tas_dev->is_glb_calibrated_data_loaded = true; > + for (i = 0; i < tas_dev->ndev; i++) { > + scnprintf(tas_dev->cal_binaryname[i], 64, "%s_cal_0x%02x.bin", > + tas_dev->dev_name, tas_dev->tasdevice[i].dev_addr); > + ret = tas2781_load_calibration(tas_dev, > + tas_dev->cal_binaryname[i], i); > + if (ret != 0) { > + dev_err(tas_dev->dev, > + "%s: load %s error, no-side effect\n", > + __func__, > + tas_dev->cal_binaryname[i]); > + ret = 0; > + tas_dev->is_glb_calibrated_data_loaded = false; > + } > + } > + > +out: > + mutex_unlock(&tas_dev->codec_lock); > + if (fmw) > + release_firmware(fmw); > +} > + > +static void tasdevice_config_info_remove(void *context) > +{ > + struct tasdevice_priv *tas_dev = > + (struct tasdevice_priv *) context; > + struct tasdevice_rca *rca = &(tas_dev->rcabin); > + struct tasdevice_config_info **cfg_info = rca->cfg_info; > + int i = 0, j = 0; inits > + > + mutex_lock(&tas_dev->dev_lock); > + if (cfg_info) { > + for (i = 0; i < rca->ncfgs; i++) { > + if (cfg_info[i]) { > + for (j = 0; j < (int)cfg_info[i]->real_nblocks; > + j++) { > + kfree( > + cfg_info[i]->blk_data[j]->regdata); > + kfree(cfg_info[i]->blk_data[j]); > + } > + kfree(cfg_info[i]->blk_data); > + kfree(cfg_info[i]); > + } > + } > + kfree(cfg_info); > + } > + mutex_unlock(&tas_dev->dev_lock); > +} > + > +static void tasdevice_tuning_switch( > + struct tasdevice_priv *tas_dev, int state) > +{ > + struct tasdevice_fw *tas_fmw = tas_dev->fmw; > + int profile_cfg_id = 0; > + int is_set_glb_mode = 0; > + > + if (state == 0) { > + if (tas_fmw) { > + if (tas_dev->cur_prog >= tas_fmw->nr_programs) > + /*bypass all in rca is profile id 0*/ > + profile_cfg_id = RCA_CONFIGID_BYPASS_ALL; > + else { > + /*dsp mode or tuning mode*/ > + profile_cfg_id = > + tas_dev->rcabin.profile_cfg_id; > + > + is_set_glb_mode = > + tasdevice_select_tuningprm_cfg(tas_dev, > + tas_dev->cur_prog, > + tas_dev->cur_conf, > + profile_cfg_id); > + if (tas_dev->set_global_mode != NULL) > + tas_dev->set_global_mode(tas_dev); > + } > + } else > + profile_cfg_id = RCA_CONFIGID_BYPASS_ALL; > + > + tasdevice_select_cfg_blk(tas_dev, profile_cfg_id, > + TASDEVICE_BIN_BLK_PRE_POWER_UP); > + } else > + tasdevice_select_cfg_blk(tas_dev, > + tas_dev->rcabin.profile_cfg_id, > + TASDEVICE_BIN_BLK_PRE_SHUTDOWN); > +} > + > +static int tasdevice_dapm_event(struct snd_soc_dapm_widget *w, > + struct snd_kcontrol *kcontrol, int event) > +{ > + struct snd_soc_component *codec = snd_soc_dapm_to_component(w->dapm); > + struct tasdevice_priv *tas_dev = snd_soc_component_get_drvdata(codec); > + int state = 1; > + > + /* Codec Lock Hold */ > + mutex_lock(&tas_dev->codec_lock); > + if (event == SND_SOC_DAPM_PRE_PMD) > + state = 0; state = (event == SND_SOC_DAPM_PRE_PMD) ? 0 : 1; > + tasdevice_tuning_switch(tas_dev, state); > + /* Codec Lock Release*/ > + mutex_unlock(&tas_dev->codec_lock); > + > + return 0; > +} > + > + > +static void tas2781_reset(struct tasdevice_priv *tas_dev) > +{ > + int ret = 0; > + int i = 0; > + > + for (; i < tas_dev->ndev; i++) { > + ret = tasdevice_dev_write(tas_dev, i, > + TAS2871_REG_SWRESET, > + TAS2871_REG_SWRESET_RESET); > + if (ret < 0) { > + dev_err(tas_dev->dev, "%s: chn %d reset fail, %d\n", > + __func__, i, ret); > + continue; > + } > + } > + usleep_range(1000, 1050); > +} > +static int tasdevice_parse_dt(struct tasdevice_priv *tas_dev) > +{ > + struct device_node *np = tas_dev->dev->of_node; > + int rc = 0, i = 0, ndev = 0; inits? > + unsigned int dev_addrs[max_chn]; > + struct i2c_client *client = (struct i2c_client *)tas_dev->client; > + > + ndev = of_property_read_variable_u32_array(np, "ti,audio-slots", > + dev_addrs, 0, ARRAY_SIZE(dev_addrs)); > + if (ndev <= 0) { > + ndev = 1; > + dev_addrs[0] = client->addr; > + } > + > + for (i = 0; i < ndev; i++) > + tas_dev->tasdevice[i].dev_addr = dev_addrs[i]; > + > + if (ndev > 1) { > + rc = of_property_read_u32(np, "ti,broadcast-addr", > + &(tas_dev->glb_addr.dev_addr)); > + if (rc) { > + dev_err(tas_dev->dev, > + "Looking up node %s failed %d\n", > + np->full_name, rc); > + tas_dev->glb_addr.dev_addr = 0; > + } > + } > + > + tas_dev->ndev = ndev; > + > + tas_dev->rst_gpio = of_get_named_gpio(np, "reset-gpios", i); > + if (gpio_is_valid(tas_dev->rst_gpio)) { > + rc = gpio_request(tas_dev->rst_gpio, "reset"); > + if (!rc) > + gpio_direction_output(tas_dev->rst_gpio, 1); > + } > + > + strcpy(tas_dev->dev_name, tasdevice_id[tas_dev->chip_id].name); > + > + tas_dev->irq_info.irq_gpio = of_get_named_gpio(np, > + "interrupts", 0); > + if (gpio_is_valid(tas_dev->irq_info.irq_gpio)) { > + rc = gpio_request(tas_dev->irq_info.irq_gpio, > + "AUDEV-IRQ"); > + if (!rc) { > + gpio_direction_input( > + tas_dev->irq_info.irq_gpio); > + > + tas_dev->irq_info.irq = > + gpio_to_irq(tas_dev->irq_info.irq_gpio); > + } else > + dev_err(tas_dev->dev, > + "%s: GPIO %d request error\n", > + __func__, > + tas_dev->irq_info.irq_gpio); > + } else > + dev_err(tas_dev->dev, > + "Looking up irq-gpio property in node %s failed %d\n", > + np->full_name, > + tas_dev->irq_info.irq_gpio); > + > + return 0; > +} > +static void tas2781_set_global_mode(struct tasdevice_priv *tas_dev) > +{ > + int i = 0; > + int ret = 0; > + > + for (; i < tas_dev->ndev; i++) { > + ret = tasdevice_dev_update_bits(tas_dev, i, > + TAS2871_MISC_CFG2, TAS2871_GLOBAL_ADDR_MASK, > + TAS2871_GLOBAL_ADDR_ENABLE); > + if (ret < 0) { > + dev_err(tas_dev->dev, > + "%s: chn %d set global fail, %d\n", > + __func__, i, ret); > + continue; > + } > + } > +} > + > +static int tasdevice_init(struct tasdevice_priv *tas_dev) > +{ > + int ret = 0, i = 0; inits? > + > + tas_dev->cur_prog = -1; > + tas_dev->cur_conf = -1; > + > + for (i = 0; i < tas_dev->ndev; i++) { > + tas_dev->tasdevice[i].cur_book = -1; > + tas_dev->tasdevice[i].cur_prog = -1; > + tas_dev->tasdevice[i].cur_conf = -1; > + } > + mutex_init(&tas_dev->dev_lock); > + if (tas_dev->glb_addr.dev_addr != 0 > + && tas_dev->glb_addr.dev_addr < 0x7F) > + tas_dev->set_global_mode = tas2781_set_global_mode; > + dev_set_drvdata(tas_dev->dev, tas_dev); > + > + mutex_init(&tas_dev->codec_lock); > + ret = devm_snd_soc_register_component(tas_dev->dev, > + &soc_codec_driver_tasdevice, > + tasdevice_dai_driver, ARRAY_SIZE(tasdevice_dai_driver)); > + if (ret) { > + dev_err(tas_dev->dev, "%s: codec register error:0x%08x\n", > + __func__, ret); > + goto out; > + } > + > +out: > + return ret; > +} > + > +static void tasdevice_remove(struct tasdevice_priv *tas_dev) > +{ > + snd_soc_unregister_component(tas_dev->dev); no. if you use devm_snd_soc_register_component, then don't manually remove this component > + if (gpio_is_valid(tas_dev->rst_gpio)) > + gpio_free(tas_dev->rst_gpio); > + > + if (gpio_is_valid(tas_dev->irq_info.irq_gpio)) > + gpio_free(tas_dev->irq_info.irq_gpio); > + > + mutex_destroy(&tas_dev->dev_lock); > + mutex_destroy(&tas_dev->codec_lock); > +} > + > +static int tasdevice_i2c_probe(struct i2c_client *i2c) > +{ > + struct tasdevice_priv *tas_dev = NULL; > + int ret = 0; > + const struct i2c_device_id *id = i2c_match_id(tasdevice_id, i2c); > + > + tas_dev = devm_kzalloc(&i2c->dev, sizeof(*tas_dev), GFP_KERNEL); > + if (!tas_dev) { > + ret = -ENOMEM; > + goto out; > + } > + tas_dev->dev = &i2c->dev; > + tas_dev->client = (void *)i2c; > + tas_dev->chip_id = id->driver_data; > + > + if (i2c->dev.of_node) > + ret = tasdevice_parse_dt(tas_dev); > + else { > + dev_err(tas_dev->dev, "No DTS info\n"); > + goto out; > + } > + > + tas_dev->regmap = devm_regmap_init_i2c(i2c, > + &tasdevice_regmap); > + if (IS_ERR(tas_dev->regmap)) { > + ret = PTR_ERR(tas_dev->regmap); > + dev_err(&i2c->dev, "Failed to allocate register map: %d\n", > + ret); > + goto out; > + } > + ret = tasdevice_init(tas_dev); > + > +out: > + if (ret < 0 && tas_dev != NULL) > + tasdevice_remove(tas_dev); if you use devm then there's nothing to remove? > + return ret; > + > +} > + > +static void tasdevice_i2c_remove(struct i2c_client *client) > +{ > + struct tasdevice_priv *tas_dev = i2c_get_clientdata(client); > + > + if (tas_dev) can this not be always true? > + tasdevice_remove(tas_dev); same, is this needed? > +#define SMARTAMP_MODULE_NAME "tas2781" > +#define TASDEVICE_RETRY_COUNT 3 > +#define TASDEVICE_ERROR_FAILED -2 can't you use an existing error code? > + > +#define TASDEVICE_RATES (SNDRV_PCM_RATE_44100 |\ > + SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_96000 |\ > + SNDRV_PCM_RATE_88200) > +#define TASDEVICE_MAX_CHANNELS 8 > + > +#define TASDEVICE_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | \ > + SNDRV_PCM_FMTBIT_S24_LE | \ > + SNDRV_PCM_FMTBIT_S32_LE) > + > +/*PAGE Control Register (available in page0 of each book) */ > +#define TASDEVICE_PAGE_SELECT 0x00 > +#define TASDEVICE_BOOKCTL_PAGE 0x00 > +#define TASDEVICE_BOOKCTL_REG 127 > +#define TASDEVICE_BOOK_ID(reg) (reg / (256 * 128)) > +#define TASDEVICE_PAGE_ID(reg) ((reg % (256 * 128)) / 128) > +#define TASDEVICE_PAGE_REG(reg) ((reg % (256 * 128)) % 128) > +#define TASDEVICE_PGRG(reg) ((reg % (256 * 128))) > +#define TASDEVICE_REG(book, page, reg) (((book * 256 * 128) + \ > + (page * 128)) + reg) > + > + /*Software Reset */ intentation and spaces in all this file? > +#define TAS2871_REG_SWRESET TASDEVICE_REG(0x0, 0X0, 0x02) > +#define TAS2871_REG_SWRESET_RESET BIT(0) > + > + /* Enable Global addresses */ > +#define TAS2871_MISC_CFG2 TASDEVICE_REG(0x0, 0X0, 0x07) > +#define TAS2871_GLOBAL_ADDR_MASK BIT(1) > +#define TAS2871_GLOBAL_ADDR_ENABLE BIT(1) > + > + /*I2C Checksum */ > +#define TASDEVICE_I2CChecksum TASDEVICE_REG(0x0, 0x0, 0x7E) > + > + /* Volume control */ > +#define TAS2781_DVC_LVL TASDEVICE_REG(0x0, 0x0, 0x1A) > +#define TAS2781_AMP_LEVEL TASDEVICE_REG(0x0, 0x0, 0x03) > +#define TAS2781_AMP_LEVEL_MASK GENMASK(5, 1) > + > +#define TASDEVICE_CMD_SING_W 0x1 > +#define TASDEVICE_CMD_BURST 0x2 > +#define TASDEVICE_CMD_DELAY 0x3 > +#define TASDEVICE_CMD_FIELD_W 0x4 > +struct tasdevice_priv { > + struct device *dev; > + void *client; > + struct regmap *regmap; > + struct mutex codec_lock; > + struct mutex dev_lock; need comments on what they protect. checkpatch.pl would tell you that. > + int rst_gpio; > + struct tasdevice tasdevice[max_chn]; > + struct tasdevice_fw *fmw; > + struct tasdevice_rca rcabin; > + struct tasdevice_irqinfo irq_info; > + struct tas_control tas_ctrl; > + struct global_addr glb_addr; > + int cur_prog; > + int cur_conf; > + unsigned int chip_id; > + void (*set_global_mode)(struct tasdevice_priv *tas_dev); > + int (*fw_parse_variable_header)(struct tasdevice_priv *tas_dev, > + const struct firmware *fmw, int offset); > + int (*fw_parse_program_data)(struct tasdevice_priv *tas_dev, > + struct tasdevice_fw *tas_fmw, > + const struct firmware *fmw, int offset); > + int (*fw_parse_configuration_data)(struct tasdevice_priv *tas_dev, > + struct tasdevice_fw *tas_fmw, > + const struct firmware *fmw, int offset); > + int (*tasdevice_load_block)(struct tasdevice_priv *tas_dev, > + struct tasdev_blk *pBlock); > + int (*fw_parse_calibration_data)(struct tasdevice_priv *tas_dev, > + struct tasdevice_fw *tas_fmw, > + const struct firmware *fmw, int offset); > + int fw_state; > + unsigned int magic_num; > + unsigned char ndev; > + unsigned char dev_name[32]; > + unsigned char rca_binaryname[64]; > + unsigned char coef_binaryname[64]; > + unsigned char cal_binaryname[max_chn][64]; > + unsigned char crc8_lkp_tbl[CRC8_TABLE_SIZE]; > + void *codec; > + unsigned int sysclk; > + bool is_glb_calibrated_data_loaded; maybe reorganize, this looks like a dumping ground of lots of different things.