Received-SPF: pass (google.com: domain of linux-kernel+bounces-136958-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Tue, 9 Apr 2024 16:26:52 +0300
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Baojun Xu <baojun.xu@ti.com>
Cc: tiwai@suse.de, robh+dt@kernel.org, lgirdwood@gmail.com, perex@perex.cz,
	pierre-louis.bossart@linux.intel.com, kevin-lu@ti.com,
	shenghao-ding@ti.com, navada@ti.com, alsa-devel@alsa-project.org,
	linux-kernel@vger.kernel.org, liam.r.girdwood@intel.com,
	yung-chuan.liao@linux.intel.com, broonie@kernel.org, soyer@irl.hu
Subject: Re: [PATCH v2 3/3] ALSA: hda/tas2781: Firmware load for tas2781
 driver for SPI
Message-ID: <ZhVCHJrjKDVOP5ji@smile.fi.intel.com>
References: <20240409024816.1180-1-baojun.xu@ti.com>
 <20240409024816.1180-4-baojun.xu@ti.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240409024816.1180-4-baojun.xu@ti.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo

On Tue, Apr 09, 2024 at 10:48:15AM +0800, Baojun Xu wrote:
> Firmware download and parser lib for tas2781, it work for spi
> device with a single firmware binary file.

I believe this also can benefit from comments given against previous patches.

..

> +		im = &(calibration->dev_data);

Unneeded parentheses.

> +
> +		if (!im->dev_blks)
> +			continue;
> +
> +		for (blks = 0; blks < im->nr_blk; blks++) {
> +			block = &(im->dev_blks[blks]);
> +			if (!block)
> +				continue;
> +			kfree(block->data);
> +		}
> +		kfree(im->dev_blks);
> +	}
> +	kfree(tas_fmw->calibrations);
> +out:
> +	kfree(tas_fmw);

It may gain if you use cleanup.h from day 1.

> +}
> +
> +void tasdevice_spi_calbin_remove(void *context)
> +{
> +	struct tasdevice_priv *tas_priv = (struct tasdevice_priv *) context;

Casting is not needed.

> +	struct tasdevice *tasdev;

> +	if (!tas_priv)
> +		return;

How is this not a dead code?

> +	tasdev = &(tas_priv->tasdevice);
> +	if (tasdev->cali_data_fmw) {
> +		tas2781_clear_calfirmware(tasdev->cali_data_fmw);
> +		tasdev->cali_data_fmw = NULL;
> +	}
> +}

..

> +void tasdevice_spi_config_info_remove(void *context)
> +{
> +	struct tasdevice_priv *tas_priv = (struct tasdevice_priv *) context;
> +	struct tasdevice_rca *rca = &(tas_priv->rcabin);
> +	struct tasdevice_config_info **ci = rca->cfg_info;
> +	int i, j;
> +
> +	if (!ci)
> +		return;
> +	for (i = 0; i < rca->ncfgs; i++) {
> +		if (!ci[i])
> +			continue;
> +		if (ci[i]->blk_data) {
> +			for (j = 0; j < (int)ci[i]->real_nblocks; j++) {

Oh, explicit castings should be _rarely_ used. What's the problem with making j
to be the same type as real_nblocks?

> +				if (!ci[i]->blk_data[j])
> +					continue;
> +				kfree(ci[i]->blk_data[j]->regdata);
> +				kfree(ci[i]->blk_data[j]);
> +			}
> +			kfree(ci[i]->blk_data);
> +		}
> +		kfree(ci[i]);
> +	}
> +	kfree(ci);
> +}

..

> +	if (cfg_no >= 0
> +		&& (tas_priv->tasdevice.cur_conf != cfg_no)
> +		&& (cfg_info[rca_conf_no]->active_dev & 1)
> +		&& (tas_priv->tasdevice.is_loaderr == false)) {

This is unparsable. Please, use postfix style and proper indentation.

	if (foo &&
	    bar ...) {
		...stuff...;
	}

> +		status++;
> +		tas_priv->tasdevice.is_loading = true;
> +	} else {
> +		tas_priv->tasdevice.is_loading = false;
> +	}

..

> +	if (state == 0) {
> +		if (tas_priv->cur_prog < tas_fmw->nr_programs) {
> +			/*dsp mode or tuning mode*/
> +			profile_cfg_id = tas_priv->rcabin.profile_cfg_id;
> +			tasdevice_spi_select_tuningprm_cfg(tas_priv,
> +				tas_priv->cur_prog, tas_priv->cur_conf,
> +				profile_cfg_id);
> +		}
> +
> +		tasdevice_spi_select_cfg_blk(tas_priv, profile_cfg_id,
> +			TASDEVICE_BIN_BLK_PRE_POWER_UP);
> +	} else
> +		tasdevice_spi_select_cfg_blk(tas_priv, profile_cfg_id,
> +			TASDEVICE_BIN_BLK_PRE_SHUTDOWN);

Out of a sudden different style (no {} in 'else' branch). Try to be consistent
in style everywhere.

-- 
With Best Regards,
Andy Shevchenko