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: Fri, 20 Oct 2023 15:39:10 +0000	[thread overview]
Message-ID: <E3224624-7FF4-48F6-BA53-08312B69EF9F@cirrus.com> (raw)
In-Reply-To: <20231019162359.GF2424087@google.com>

Thank you for your thorough review. Anything not replied to below will be
incorporated in the next version.

>> +/*
>> + * CS40L50 Advanced Haptic Driver with waveform memory,
> 
> s/Driver/device/

CS40L50 is a “haptic driver”, like a "motor driver" in a car. It is an
unfortunate name in this context, but it is straight from the datasheet.

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

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

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

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

>> +static irqreturn_t cs40l50_error(int irq, void *data);
> 
> Why is this being forward declared?
> 
>> +static const struct cs40l50_irq cs40l50_irqs[] = {
>> + CS40L50_IRQ(AMP_SHORT, "Amp short", error),
> 
> I assume that last parameter is half of a function name.
> 
> Better to have 2 different structures and do 2 requests I feel.

I think I will combine the two handler functions into one, so as not
to need the struct handler parameter, or the forward declaration.

>> +{
>> + 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.

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

Best,
James