Re: [PATCH v3 2/2] Input: cs40l50 - Initial support for Cirrus Logic CS40L50

[Jeff LaBundy]

Hi James,

On Wed, Aug 16, 2023 at 09:02:26PM +0000, James Ogletree wrote:
> 
> 
> > On Aug 10, 2023, at 11:07 PM, Jeff LaBundy <jeff@xxxxxxxxxxx> wrote:
> > 
> > On Wed, Aug 09, 2023 at 07:10:28PM +0000, James Ogletree wrote:
> >> Introduce support for Cirrus Logic Device CS40L50: a
> >> haptics driver with waveform memory DSP and closed-loop
> >> algorithms.
> > 
> > From my extremely naive point of view, some of the code that follows
> > bares resemblance to the recently reviewed L26. My assumption is that
> > these devices are different enough in nature to warrant completely
> > different drivers; is that accurate?
> > 
> > One reason for asking is because the L26 driver included a cornucopia
> > of power-management overhead, yet we see none of that here. Assuming
> > both L50 and L26 are built around the same Halo DSP, why is there such
> > a fundamental difference between the two in terms of power management?
> > 
> > To that end, how does this driver handle hibernation? Is hibernation
> > not supported, or do we simply defer to the DSP? In the case of the
> > latter, why is L50 given this freedom but not L26?
> 
> One key difference is that L50’s Halo Core DSP is self-booting; the firmware
> is burned in and no firmware download is required. On L26, firmware
> downloading is compulsory. This differentiates dealing with the DSP in the
> two drivers, because the L50 driver does not need to do a look up every
> time it reads or writes to a firmware control. The registers are all static.

Interesting stuff; thanks for sharing that background information.

> 
> Minor reasons are that they have different power supply configurations that
> require different register settings, they have errata differences, and a different set
> of exposed features (L50 being much more simplistic). I think taken cumulatively
> these differences warrant separate drivers. Though, I will take Charles’
> recommendation to factor out the similarities into a shared library that both L50
> and L26 can use.
> 
> Let me know whether you disagree on the above points or have followup
> questions.

Makes sense to me.

> 
> With respect to power management, I did not think that that there was any merit
> in itself in maintaining equality with L26’s approach, and I was inclined to accept
> your reasoning for using retry logic over the runtime PM facilities (not that the
> latter way is incorrect).
> 
> Regarding the need for I2S streaming support, signs point to maybe. I will
> migrate this driver to MFD for V4.

MFD seems like the right (i.e. scalable) solution if A2H is on the roadmap.
In early L25 days, very early adopters simply asked for a sysfs control
exposed from the core (then LED !!) driver to enable/disable I2S streaming
mode, but this got hairy in case customers needed to control BCLK/Fs ratio,
bit depth, etc. during runtime.

>From my naive point of view, maybe the solution looks something like this:

* drivers/mfd/cs40l50-i2c.c: I2C client
* drivers/mfd/cs40l50-spi.c: SPI client
* drivers/mfd/cs40l50-core.c: common tasks such as FW loading, OTP management, etc;
  perhaps L26 can leverage it as well
* drivers/input/misc/cs40l50-vibra.c: FF-specific support (name as you like, I just
  picked what seems to be most common)
* sound/soc/codecs/cs40l50-codec.c: codec-specific support

In case I have misunderstood, please let me know.

> 
> > 
> >> + return cs40l50_dsp_write(cs40l50, CS40L50_BST_LPMODE_SEL, CS40L50_DCM_LOW_POWER);
> >> +}
> >> +
> >> +static int cs40l50_patch_firmware(struct cs40l50_private *cs40l50)
> >> +{
> >> + const struct firmware *bin = NULL, *wmfw = NULL;
> >> + int error = 0;
> >> +
> >> + if (request_firmware(&bin, "cs40l50.bin", cs40l50->dev))
> >> + dev_info(cs40l50->dev, "Could not request wavetable file: %d\n", error);
> >> +
> >> + if (request_firmware(&wmfw, "cs40l50.wmfw", cs40l50->dev))
> >> + dev_info(cs40l50->dev, "Could not request firmware patch file: %d\n", error);
> >> +
> >> + if (wmfw) {
> > 
> > It is a much more common design pattern to bail if request_firmware() returns
> > an error, than to proceed and check against the FW pointer remaining NULL.
> > 
> > Is this done because cs_dsp_power_up() must be called, with or without a wmfw
> > or coefficient file being available?
> 
> I don’t think that cs_dsp_power_up() must be called in the case of non-existent .wmfw
> and .bin files, so I will take your suggestion and optimize this function. I will also switch
> to asynchronous firmware requesting for V4.

Since L50 can work out of the box without FW, another option is to follow the
approach used by some codec drivers and wait to load FW until the first request
to stream I2S, and/or trigger an FF effect in this case. By that point, rootfs
has been available for some time and request_firmware() will succeed.

This is advantageous because even though request_firmware_nowait() can solve
the deadlock problem for FW loaded at probe, it could still return -ENOENT right
away on some platforms. The disadvantage is that the first effect would be very
delayed due to the overhead of transferring several kB of FW over I2C for the
first time. That's OK for audio applications, but not haptics where delay normally
must be within single-digit ms.

Ultimately, I think the right approach is to look for FW from probe as you have
done, but use request_firmware_nowait(). However, I recommend structuring the
driver such that rip-up would be minimal in case you had to retry FW loading from
the first FF trigger, to support a customer whose rootfs is not available at probe.
In such a case, the customer could work around the first-time latency problem by
triggering a dummy effect early in their boot (e.g. init.rc).

> 
> I will also incorporate the rest of your review comments not mentioned here in V4.
> Thank you for the thorough review.

Thank you for the productive discussion!

> 
> Regards,
> James Ogletree

Kind regards,
Jeff LaBundy