Re: [PATCH v1 7/8] ALSA: hda/tas2781: Add tas2781 SPI-based driver

[Andy Shevchenko]

On Tue, Mar 26, 2024 at 09:09:04AM +0800, Baojun Xu wrote:
> Firmware binary load lib code for tas2781 spi driver.

..

> +// tas2781_spi_fwlib.c -- TASDEVICE firmware support

Please, remove file names from the files. This is just a burden: in case the
file gets renamed, often people forgot to update its contents.

Same applies to all such cases.

..

> +#define ERROR_PRAM_CRCCHK			0x0000000
> +#define ERROR_YRAM_CRCCHK			0x0000001
> +#define	PPC_DRIVER_CRCCHK			0x00000200

Stray TAB after define.

..

> +	/* In most projects are many audio cases, such as music, handfree,
> +	 * receiver, games, audio-to-haptics, PMIC record, bypass mode,
> +	 * portrait, landscape, etc. Even in multiple audios, one or
> +	 * two of the chips will work for the special case, such as
> +	 * ultrasonic application. In order to support these variable-numbers
> +	 * of audio cases, flexible configs have been introduced in the
> +	 * dsp firmware.

DSP

> +	 */

/*
 * The correct style of the multi-line comments
 * outside of net subsystem is depicted here. Please,
 * follow and update all the comments accordingly.
 */

..

> +	cfg_info = kzalloc(sizeof(struct tasdevice_config_info), GFP_KERNEL);

sizeof(*cfg_info)
Same applies to all similar cases.


> +	if (!cfg_info) {
> +		*status = -ENOMEM;
> +		goto out;
> +	}

..

> +	if (tas_priv->rcabin.fw_hdr.binary_version_num >= 0x105) {
> +		if (config_offset + 64 > (int)config_size) {

Explicit casting and to signed (sic!) is prone to mistakes. Can you refactor
to get rid of the casting?

> +			*status = -EINVAL;
> +			dev_err(tas_priv->dev, "add conf: Out of boundary\n");
> +			goto out;
> +		}
> +		config_offset += 64;
> +	}

..

> +	if (config_offset + 4 > (int)config_size) {

Ditto.

> +		*status = -EINVAL;
> +		dev_err(tas_priv->dev, "add config: Out of boundary\n");
> +		goto out;
> +	}

..

> +	/* convert data[offset], data[offset + 1], data[offset + 2] and
> +	 * data[offset + 3] into host
> +	 */

See above about comment style.

..

> +	cfg_info->nblocks =
> +		be32_to_cpup((__be32 *)&config_data[config_offset]);

Castings to bitwise types is something that should not happen.
So, this looks like homegrown version of get_unaligned_be32().

..

> +	bk_da = cfg_info->blk_data = kcalloc(cfg_info->nblocks,
> +		sizeof(struct tasdev_blk_data *), GFP_KERNEL);

sizeof(*bk_da) ?

> +	if (!bk_da) {
> +		*status = -ENOMEM;
> +		goto out;
> +	}

..

> +		if (bk_da[i]->block_type == TASDEVICE_BIN_BLK_PRE_POWER_UP) {
> +			if (bk_da[i]->dev_idx == 0)
> +				cfg_info->active_dev =
> +					(1 << tas_priv->ndev) - 1;
> +			else
> +				cfg_info->active_dev |= 1 <<
> +					(bk_da[i]->dev_idx - 1);

Use BIT()

> +

Stray blank line.

> +		}

..

> +		bk_da[i]->yram_checksum =
> +			be16_to_cpup((__be16 *)&config_data[config_offset]);
> +		config_offset += 2;
> +		bk_da[i]->block_size =
> +			be32_to_cpup((__be32 *)&config_data[config_offset]);
> +		config_offset += 4;
> +
> +		bk_da[i]->n_subblks =
> +			be32_to_cpup((__be32 *)&config_data[config_offset]);

get_unaligned_beXX() in all cases, beyond and above.

..

> +out:

Useless label, return directly.

> +	return cfg_info;

This also applies to many places in the code.

..

So, I have stopped here as I believe you have already enough material to rework
the series. I believe with my comments addressed you may shrink the code base
by ~5%.

Also next version probably needs a cover letter to explain a bit
what is this for and why you split patches in the unusual way and how you
suggested to get them working in between (to pass bisectability test).

-- 
With Best Regards,
Andy Shevchenko