Hi Lee, Krzysztof and Fred,
On Wed, Feb 12, 2025 at 03:50:50PM +0000, Lee Jones wrote:
> On Tue, 11 Feb 2025, Fred Treven wrote:
>
> > On 2/5/25 04:34, Krzysztof Kozlowski wrote:
> > > On 05/02/2025 00:18, Fred Treven wrote:
> > > > Introduce support for Cirrus Logic Device CS40L26:
> > > > A boosted haptic driver with integrated DSP and
> > > > waveform memory with advanced closed loop algorithms
> > > > and LRA protection.
[...]
> > > > +int cs40l26_set_pll_loop(struct cs40l26 *cs40l26, const u32 pll_loop)
> > > > +{
> > > > + int i;
> > > > +
> > > > + /* Retry in case DSP is hibernating */
> > > > + for (i = 0; i < CS40L26_PLL_NUM_SET_ATTEMPTS; i++) {
> > > > + if (!regmap_update_bits(cs40l26->regmap, CS40L26_REFCLK_INPUT,
> > > > + CS40L26_PLL_REFCLK_LOOP_MASK,
> > > > + pll_loop << CS40L26_PLL_REFCLK_LOOP_SHIFT))
> > > > + break;
> > > > + }
> > > > +
> > > > + if (i == CS40L26_PLL_NUM_SET_ATTEMPTS) {
> > > > + dev_err(cs40l26->dev, "Failed to configure PLL\n");
> > > > + return -ETIMEDOUT;
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(cs40l26_set_pll_loop);
> > > > +
> > >
> > > This looks way past simple MFD driver. Not only this - entire file. You
> > > configure there quite a lot and for example setting PLLs is not job for
> > > MFD. This should be placed in appropriate subsystem.
> > >
> > I disagree here because the configuration being done in this file
> > is essential to the core operation of the part. For instance,
> > setting the PLL to open-loop here is required to prevent any
> > external interference (e.g. GPIO events) from interrupting
> > the part while loading firmware.
> >
> > The other hardware configuration being done here is required for
> > both the Input and ASoC operations of the part.
> >
> > Lastly, these need to be done in order and independently of which
> > child driver (ASoC or input) the user adds. If this is moved
> > to cs40l26-vibra.c (the input driver), for instance,
> > and that module is then not added, it will disturb the
> > required setup for use by the ASoC driver.
> >
> > I would really like to get Lee's opinion here because it does not
> > make sense to me why this is inappropriate when the configuration
> > done in the core MFD driver is required for use by all of its
> > children.
>
> FWIW, I agree with Krzysztof.
>
> There's a bunch of functionality in here that should be exported out to
> leaf drivers which should reside in their associated subsystems. From
> just a quick glance that looks to include, but not necessary limited
> to; IRQs, GPIOs and PLLs (Clocks).
>
> MFD has been used for a dumping ground under the premise of "core
> functionality" before. Tolerance for those arguments are now fairly
> low.
I think all three of you are right here. MFD should not be a "kitchen
sink", but I'm also cautious to call this particular series one such
offender. Let's consider how this hardware is logically organized,
keeping in mind what I personally consider a core tenet of MFD, which
is that each child should serve a purpose in the absence of all others.
This device is fundamentally a haptic chip; like many others in input,
it has a low-latency GPIO trigger. As far as I can tell, the chip itself
is the only consumer of this GPIO; there is no practical scenario where
another chip would access it.
What makes this chip is unique is that it has an I2S port; from that
perspective, it's also a DAC with a motor instead of a speaker. Any
stimulus that drives the motor (e.g. external GPIO trigger, FF ioctl,
audio-like stream, etc.) is therefore capable of asserting an interrupt
(e.g. hardware short), which is why the parent (i.e. MFD) seems like
the most logical place to handle this work. This chip would never be an
interrupt-parent to anything else on the board, so an irqchip driver
does not seem useful here.
I do see a lot of GPIO-related ISRs, but it looks like we can simply drop
these; all they seem to do is print a debug message, and perform some
housekeeping to track the edge polarity so as to print the correct debug
message. If I'm mistaken and the driver really does need to perform some
critical tasks in response to GPIO edges, then I do agree a gpiochip
driver seems useful here, albeit an extremely limited one.
I agree there is a case here for a lightweight clock driver representing
the device's internal PLL output, but it looks like all the children need
to do here is one regmap call. I'm guessing the reason this regmap call
got put in a wrapper in the MFD is to avoid duplicating a small retry
loop in every child driver. Simply replicating this in only two children
seems like less code then a whole clock driver solely for that purpose;
alternatively, maybe there is some way for any child setting the PLL via
regmap to better understand whether the device is asleep and may NAK.
Maybe v2 can start small and do the following:
1. Remove what can be considered "customer bring-up" code (e.g. liberal
use of dev_dbg); this may get rid of all GPIO-related noise.
2. Consider whether regmap alone can handle anything related to IRQs and
the PLL.
3. Consider whether anything in cs40l26_dsp_*() is better suited for the
cs_dsp driver in firmware. A lot of this work seems suitable for any HALO
DSP based device, even if CS40L26 is the only user for now. Maybe some of
it can be consolidated with CS40L50?
With the series pared down in this way, it will hopefully become clearer
how much remaining functionality, if any, should live as a separate stub
or leaf driver.
>
> --
> Lee Jones [李琼斯]
Kind regards,
Jeff LaBundy
