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 [李琼斯]
