> 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.
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.
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.
>
>> + 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.
I will also incorporate the rest of your review comments not mentioned here in V4.
Thank you for the thorough review.
Regards,
James Ogletree
