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

[Shuming [范書銘] <shumingf@xxxxxxxxxxx>]

> > + * 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?)

Will fix

> > +	} 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.

Thanks for reviewing. Will update the v2 patch.