Hi James,

On Wed, Aug 16, 2023 at 09:02:26PM +0000, James Ogletree wrote:

quoted

On Aug 10, 2023, at 11:07 PM, Jeff LaBundy [off-list ref] wrote:

On Wed, Aug 09, 2023 at 07:10:28PM +0000, James Ogletree wrote:

quoted

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.

quoted

quoted

+ 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