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

From: James Ogletree <James.Ogletree@cirrus.com>
To: Lee Jones <lee@kernel.org>
Cc: James Ogletree <james.ogletree@opensource.cirrus.com>,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Fred Treven <Fred.Treven@cirrus.com>,
	Ben Bright <Ben.Bright@cirrus.com>,
	"linux-input@vger.kernel.org" <linux-input@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v4 3/4] mfd: cs40l50: Add support for CS40L50 core driver
Date: Tue, 24 Oct 2023 01:14:41 +0000	[thread overview]
Message-ID: <DC6C1E91-BC15-47E5-8D94-E468188F58C1@cirrus.com> (raw)
In-Reply-To: <20231023092046.GA8909@google.com>

Any comment that went un-replied will be adopted in the next version.

> On Oct 23, 2023, at 4:20 AM, Lee Jones <lee@kernel.org> wrote:
> 
> On Fri, 20 Oct 2023, James Ogletree wrote:
> 
>>>> +static const struct mfd_cell cs40l50_devs[] = {
>>>> + {
>>>> + .name = "cs40l50-vibra",
>>>> + },
>>> 
>>> 
>>> Where are the other devices?  Without them, it's not an MFD.
>> 
>> The driver will need to support I2S streaming to the device at some point
>> in the future, for which a codec driver will be added. I thought it better to
>> submit this as an MFD driver now, rather than as an Input driver, so as
>> not to have to move everything later.
>> 
>> Should I add the “cs40l50-codec” mfd_cell now, even though it does not
>> exist yet?
> 
> What is your timeline for this to be authored?
> 
> Does the device function well without it?

Without the codec component, one minor feature will be missing, but
the device will have no issues functioning.

The current timeline, as best as I can see it, is 3-6 months.

> 
>>>> +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?
> 
> 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

Understood. Is it possible that “error” is a recent adoption?
Regardless, I will go ahead and use “err” for the MFD driver.

> 
>>>> +static irqreturn_t cs40l50_process_mbox(int irq, void *data)
>>>> +{
>>>> + struct cs40l50_private *cs40l50 = data;
>>>> + int error = 0;
>>>> + u32 val;
>>>> +
>>>> + mutex_lock(&cs40l50->lock);
>>>> +
>>>> + while (!cs40l50_mailbox_read_next(cs40l50, &val)) {
>>>> + switch (val) {
>>>> + case 0:
>>>> + mutex_unlock(&cs40l50->lock);
>>>> + dev_dbg(cs40l50->dev, "Reached end of queue\n");
>>>> + return IRQ_HANDLED;
>>>> + case CS40L50_MBOX_HAPTIC_TRIGGER_GPIO:
>>>> + dev_dbg(cs40l50->dev, "Mailbox: TRIGGER_GPIO\n");
>>> 
>>> These all appear to be no-ops?
>> 
>> Correct.
> 
> Then why do the exist?

In my judgment it alerts the user or developer of an important
error, and in other cases it gives them insight that is useful for
understanding firmware behavior. Is this kind of visibility not
typical? Regardless, I will move it out of MFD for V5.

> 
>>>> + case CS40L50_MBOX_RUNTIME_SHORT:
>>>> + dev_err(cs40l50->dev, "Runtime short detected\n");
>>>> + error = cs40l50_error_release(cs40l50);
>>>> + if (error)
>>>> + goto out_mutex;
>>>> + break;
>>>> + default:
>>>> + dev_err(cs40l50->dev, "Payload %#X not recognized\n", val);
>>>> + error = -EINVAL;
>>>> + goto out_mutex;
>>>> + }
>>>> + }
>>>> +
>>>> + error = -EIO;
>>>> +
>>>> +out_mutex:
>>>> + mutex_unlock(&cs40l50->lock);
>>>> +
>>>> + return IRQ_RETVAL(!error);
>>>> +}
>>> 
>>> 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.

Roger that. Mailbox functionality will move out of MFD for V5.

> 
>>>> +struct cs40l50_irq {
>>>> + const char *name;
>>>> + int irq;
>>>> + irqreturn_t (*handler)(int irq, void *data);
>>>> +};
>>>> +
>>>> +struct cs40l50_private {
>>>> + struct device *dev;
>>>> + struct regmap *regmap;
>>>> + struct cs_dsp dsp;
>>>> + struct mutex lock;
>>>> + struct gpio_desc *reset_gpio;
>>>> + struct regmap_irq_chip_data *irq_data;
>>>> + struct input_dev *input;
>>> 
>>> Where is this used?
>>> 
>>>> + const struct firmware *wmfw;
>>> 
>>> Or this.
>>> 
>>>> + struct cs_hap haptics;
>>> 
>>> Or this?
>>> 
>>>> + u32 devid;
>>>> + u32 revid;
>>> 
>>> Are these used after they're set?
>> 
>> These are all used in the input driver, patch 4/4 of this series. If
>> this is not acceptable in some way, I will change it per your
>> suggestions.
> 
> Do they need to be shared with other devices?
> 
> If not, they should live where they are used.

devid and revid are shared, but the others are not. I will move them to
their proper home in V5.

Best,
James