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

[Lee Jones <lee@xxxxxxxxxx>]

On Fri, 01 Mar 2024, James Ogletree wrote:

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

Then check for NULL before dereferencing it?

-- 
Lee Jones [李琼斯]