Re: [PATCH 1/2] ASoc: tas2563: DSP Firmware loading support

[Gergo Koteles <soyer@irl.hu>]

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@ti.com/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