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;elsecfg_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).