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/bee0e7762ad2c6025b9f5245c040fcc36ef2b...
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