Re: [PATCH v2 3/3] ALSA: hda/tas2781: Firmware load for tas2781 driver for SPI

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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