Hi Pierre-Louis,
Thank you for your review.
On Mon, 2023-12-04 at 18:05 -0600, Pierre-Louis Bossart wrote:
> >
> > sound/soc/codecs/tas2562.c | 2 +-
>
> are tas2562 and tas2563 (from commit subject) the same?
>
No, tas2563 is register compatible with tas2562.
> >
> > +#include <linux/slab.h>
> > +#include <linux/delay.h>
>
> nit-pick: alphabetical order?
>
Good idea, I'll fix it and the others in the next version.
> > +
> > +#include <sound/tas2562.h>
> > +#include <sound/tas25xx-dsp.h>
> > +
> > +
> > +static void tas25xx_process_fw_delay(struct tas25xx_cmd *cmd)
> > +{
> > + mdelay(cpu_to_be16(cmd->hdr.length));
>
> is this the length of the header, or the duration of the delay?
>
> Someone will get it wrong with this naming...
>
I think this is because of the format.
If cmd_type is TAS25XX_CMD_DELAY, then hdr.length is the length of the
delay.
At least I think so, based on
https://lore.kernel.org/lkml/6d6aaed3-dac8-e1ec-436c-9b04273df2b3@xxxxxx/T/
https://github.com/torvalds/linux/blob/bee0e7762ad2c6025b9f5245c040fcc36ef2bde8/sound/soc/codecs/tas2781-fmwlib.c#L769
But I don't see any TAS25XX_CMD_DELAY command in the firmware I'm
using.
> >
> > + prog_num = cpu_to_be32(fw_data->hdr->num_programs);
> > + config_num = cpu_to_be32(fw_data->hdr->num_configs);
> > + dev_info(dev, "Firmware loaded: programs %d, configs %d\n",
> > + prog_num, config_num);
> > +
For me:
release_firmware(fw) is missing from here
> > + return fw_data;
> > +
> > +err_prog:
> > + devm_kfree(dev, fw_data->prog_data);
> > +err_hdr:
> > + devm_kfree(dev, fw_data->hdr);
> > +err_data:
> > + devm_kfree(dev, fw_data);
> > +err_fw:
> > + release_firmware(fw);
> > +
> > + return NULL;
> > +}
> >
Regards,
Gergo
