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

[James Ogletree <James.Ogletree@xxxxxxxxxx>]

Hi Lee,

Thanks for the review.

> On Mar 1, 2024, at 3:17 AM, Lee Jones <lee@xxxxxxxxxx> wrote:
> 
> On Wed, 21 Feb 2024, 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>
>> ---
>> MAINTAINERS                 |   2 +
>> drivers/mfd/Kconfig         |  30 ++
>> drivers/mfd/Makefile        |   4 +
>> drivers/mfd/cs40l50-core.c  | 531 ++++++++++++++++++++++++++++++++++++
>> drivers/mfd/cs40l50-i2c.c   |  69 +++++
>> drivers/mfd/cs40l50-spi.c   |  69 +++++
>> include/linux/mfd/cs40l50.h | 142 ++++++++++
>> 7 files changed, 847 insertions(+)
>> create mode 100644 drivers/mfd/cs40l50-core.c
>> create mode 100644 drivers/mfd/cs40l50-i2c.c
>> create mode 100644 drivers/mfd/cs40l50-spi.c
>> create mode 100644 include/linux/mfd/cs40l50.h
> 
> Mostly fine, just a few nits.
> 
> Assumingly this needs to go in via one tree (usually MFD).
> 
> I can't do so until the other maintainers have provided Acks.
> 

Yes, understood.

>> +static const struct cs_dsp_region cs40l50_dsp_regions[] = {
>> + { .type = WMFW_HALO_PM_PACKED, .base = CS40L50_PMEM_0 },
>> + { .type = WMFW_HALO_XM_PACKED, .base = CS40L50_XMEM_PACKED_0 },
>> + { .type = WMFW_HALO_YM_PACKED, .base = CS40L50_YMEM_PACKED_0 },
>> + { .type = WMFW_ADSP2_XM, .base = CS40L50_XMEM_UNPACKED24_0 },
>> + { .type = WMFW_ADSP2_YM, .base = CS40L50_YMEM_UNPACKED24_0 },
>> +};
>> +
>> +static void cs40l50_dsp_remove(void *data)
>> +{
>> + cs_dsp_remove((struct cs_dsp *)data);
> 
> Is the cast required?
> 
> Where is this function?

Seems that the cast is redundant, so I will remove.

The function cs_dsp_remove() is exported from linux/firmware/cirrus/cs_dsp.h.

> 
>> +}
>> +
>> +static const struct cs_dsp_client_ops cs40l50_client_ops;
> 
> What's this for?  Where is it used?
> 
> In general, I'm not a fan of empty struct definitions like this.

>From the same cs_dsp module as mentioned above, it is a structure containing
callbacks that gives the client driver an opportunity to respond to certain events
during the DSP's lifecycle. It just so happens that this driver does not need to do
anything. However, no struct definition will result in a null pointer dereference by
cs_dsp when it checks for the callbacks.

Best,

James