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
