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
