Re: [PATCH v4 3/4] mfd: cs40l50: Add support for CS40L50 core driver

[Jeff LaBundy]

Hi Lee and James,

On Mon, Oct 23, 2023 at 10:20:46AM +0100, Lee Jones wrote:

[...]

> > >> +static int cs40l50_handle_redc_est_done(struct cs40l50_private *cs40l50)
> > >> +{
> > >> + int error, fractional, integer, stored;
> > > 
> > > err or ret is traditional.
> > 
> > We received feedback to change from “ret” to “error” in the input
> > subsystem, and now the opposite in MFD. I have no problem adopting
> > “err” here, but is it understood that styles will be mixed across
> > components?

FWIW, this is not an uncommon outcome for submissions that span multiple
subsystems.

> 
> That surprises me:
> 
> % git grep "int .*error" | wc -l
> 6152
> 
> vs
> 
> % git grep "int .*err" | grep -v error | wc -l
> 34753
> % git grep "int .*ret" | wc -l  
> 76584

Right, the history here is that this driver started its life in input,
where submitters have historically been asked to use 'error' for return
variables that return either zero or a negative error code. Since this
driver is no longer in input, this can easily be changed.

[...]

> > > Should the last two drivers live in drivers/mailbox?
> > 
> > Adopting the mailbox framework seems like an excessive amount
> > of overhead for our requirements.
> 
> MFD isn't a dumping a ground for miscellaneous functionality.
> 
> MFD requests resources and registers devices.
> 
> Mailbox functionality should live in drivers/mailbox.

I think this is just a misnomer; the code uses the terms "mailbox" and
"mbox" throughout because the relevant registers are named as such in
the datasheet.

Please correct me James, but I believe the datasheet uses this language
because both the host and the part itself have write access, meaning the
part may write a status code to the register after the host writes that
same register. There is no relation to IPC or the mbox subsystem.

That being said, some of the functions currently placed in this MFD,
namely those related to haptic motor properties (e.g. f0 and ReDC), do
seem more appropriate for the input/FF child device. My understanding
is that those functions serve only momentary haptic click effects and
not the I2S streaming case; please let me know if I have misunderstood.

I understand that no customer would ever build the to-be-added codec
driver _without_ the input driver, but the MFD must be generic enough
to support this case. Would a codec-only implementation use f0 and ReDC
estimation? If so, then these functions _do_ belong in the MFD, albeit
with some comments to explain their nature.

[...]

> > >> +{
> > >> + struct device *dev = cs40l50->dev;
> > >> + int error;
> > >> +
> > >> + mutex_init(&cs40l50->lock);
> > > 
> > > Don't you need to destroy this in the error path?
> > 
> > My understanding based on past feedback is that mutex_destroy()
> > is an empty function unless mutex debugging is enabled, and there
> > is no need cleanup the mutex explicitly. I will change this if you
> > disagree with that feedback.
> 
> It just seems odd to create something and not tear it down.

mutex_init() is not creating anything; the mutex struct is allocated as
part of the driver's private data, which is de-allocated as part of device
managed resources being freed when the module is unloaded.

mutex_destroy() is a NOP unless CONFIG_DEBUG_MUTEXES is set, and there are
roughly 4x fewer instances of it than mutex_init() in mainline. I recommend
not to add mutex_destroy() because it adds unnecessary tear-down paths and
remove() callbacks that wouldn't otherwise have to exist.

Kind regards,
Jeff LaBundy