Re: [PATCH] ASoC: rt1320: reads patch code from firmware file

[Mark Brown <broonie@xxxxxxxxxx>]

On Fri, Sep 13, 2024 at 04:27:33PM +0800, shumingf@xxxxxxxxxxx wrote:

> +/*
> + * The 'patch code' is written to the patch code area.
> + * The patch code area is used for SDCA register expansion flexibility.
> + */
> +static void rt1320_load_mcu_patch(struct rt1320_sdw_priv *rt1320)
> +{
> +	struct sdw_slave *slave = rt1320->sdw_slave;
> +	const struct firmware *patch;
> +	char filename[128];
> +	unsigned int addr, val;
> +	const unsigned int *ptr;
> +	int ret, i;
> +
> +	if (rt1320->version_id <= RT1320_VB)
> +		strscpy(filename, RT1320_VAB_MCU_PATCH, sizeof(filename));
> +	else
> +		strscpy(filename, RT1320_VC_MCU_PATCH, sizeof(filename));
> +
> +	/* load the patch code here */
> +	ret = request_firmware(&patch, filename, &slave->dev);

Why not just

	const char *filename;

	if (rt1320->version_id <= RT1320_VB)
		filename = RT1320_VAB_MCU_PATCH;
	else
		filename = RT1320_VC_MCU_PATCH;

	ret = request_firmware(&patch, filename, &slave->dev);

(ie, what does the copy into the buffer do?)

> +	} else {
> +		ptr = (const unsigned int *)patch->data;
> +		for (i = 0; i < (patch->size / sizeof(unsigned int)); i += 2) {
> +			addr = ptr[i];
> +			val = ptr[i + 1];
> +			regmap_write(rt1320->regmap, addr, val);
> +		}

Shouldn't this be using a specifically sized type in case we build on
some architecture where int is a different size?  We should also
validate that the file is an even number of register/value combinations
to make sure it's not corrupt, checking the value of regmap_write() to
make sure the register numbers are OK would probably be good.

Attachment: signature.asc
Description: PGP signature