Re: [PATCH v6 3/5] mfd: cs40l50: Add support for CS40L50 core driver

[Charles Keepax <ckeepax@xxxxxxxxxxxxxxxxxxxxx>]

On Wed, Feb 07, 2024 at 12:36:10AM +0000, James Ogletree wrote:
> Introduce support for Cirrus Logic Device CS40L50: a
> haptic driver with waveform memory, integrated DSP,
> and closed-loop algorithms.
> 
> The MFD component registers and initializes the device.
> 
> Signed-off-by: James Ogletree <jogletre@xxxxxxxxxxxxxxxxxxxxx>
> ---
> +static struct cs_dsp_wseq cs40l50_wseqs[] = {
> +	{ "PM_PWR_ON_SEQ" },
> +};
> +
> +static int cs40l50_configure_dsp(struct cs40l50 *cs40l50)
> +{
> +	u32 nwaves;
> +	int err, i;
> +
> +	if (cs40l50->bin) {
> +		/* Log number of effects if wavetable was loaded */
> +		err = regmap_read(cs40l50->regmap, CS40L50_NUM_WAVES, &nwaves);
> +		if (err)
> +			return err;
> +
> +		dev_info(cs40l50->dev, "Loaded with %u effects\n", nwaves);
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(cs40l50_wseqs); i++) {
> +		mutex_lock(&cs40l50->dsp.pwr_lock);
> +		cs40l50_wseqs[i].ctl = cs_dsp_get_ctl(&cs40l50->dsp,
> +						      cs40l50_wseqs[i].name,
> +						      WMFW_ADSP2_XM,
> +						      CS40L50_PM_ALGO_ID);
> +		mutex_unlock(&cs40l50->dsp.pwr_lock);
> +		if (!cs40l50_wseqs[i].ctl) {
> +			dev_err(cs40l50->dev, "No control: %s\n", cs40l50_wseqs[i].name);
> +			return -ENOENT;
> +		}
> +	}

In general this is a bad pattern to use, don't have global
structs that hold anything other than const data. Here if a
system with two cs40l50's was made they would end up over writing
each others ctl pointers. If the data is modified by the driver
put it in a private struct that is allocated during probe.

Thanks,
Charles