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
