Hi James,
Just a few trailing comments on top of Lee's feedback.
On Wed, Oct 18, 2023 at 05:57:24PM +0000, James Ogletree wrote:
> From: James Ogletree <[email protected]>
>
> Introduce support for Cirrus Logic Device CS40L50: a
> haptic driver with waveform memory, integrated DSP,
> and closed-loop algorithms.
>
> The MFD component registers and initializes the device.
>
> Signed-off-by: James Ogletree <[email protected]>
> ---
> MAINTAINERS | 2 +
> drivers/mfd/Kconfig | 30 +++
> drivers/mfd/Makefile | 4 +
> drivers/mfd/cs40l50-core.c | 443 ++++++++++++++++++++++++++++++++++++
> drivers/mfd/cs40l50-i2c.c | 69 ++++++
> drivers/mfd/cs40l50-spi.c | 68 ++++++
> include/linux/mfd/cs40l50.h | 198 ++++++++++++++++
> 7 files changed, 814 insertions(+)
> create mode 100644 drivers/mfd/cs40l50-core.c
> create mode 100644 drivers/mfd/cs40l50-i2c.c
> create mode 100644 drivers/mfd/cs40l50-spi.c
> create mode 100644 include/linux/mfd/cs40l50.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 57daf77bf550..08e1e9695d49 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4971,7 +4971,9 @@ L: [email protected]
> S: Supported
> F: Documentation/devicetree/bindings/input/cirrus,cs40l50.yaml
> F: drivers/input/misc/cirrus*
> +F: drivers/mfd/cs40l*
> F: include/linux/input/cirrus*
> +F: include/linux/mfd/cs40l*
>
> CIRRUS LOGIC DSP FIRMWARE DRIVER
> M: Simon Trimmer <[email protected]>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 8b93856de432..a133d04a7859 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -2187,6 +2187,36 @@ config MCP_UCB1200_TS
>
> endmenu
>
> +config MFD_CS40L50_CORE
> + tristate
> + select MFD_CORE
> + select CS_DSP
> + select REGMAP_IRQ
> +
> +config MFD_CS40L50_I2C
> + tristate "Cirrus Logic CS40L50 (I2C)"
> + select REGMAP_I2C
> + select MFD_CS40L50_CORE
> + depends on I2C
> + help
> + Select this to support the Cirrus Logic CS40L50 Haptic
> + Driver over I2C.
> +
> + This driver can be built as a module. If built as a module it will be
> + called "cs40l50-i2c".
> +
> +config MFD_CS40L50_SPI
> + tristate "Cirrus Logic CS40L50 (SPI)"
> + select REGMAP_SPI
> + select MFD_CS40L50_CORE
> + depends on SPI
> + help
> + Select this to support the Cirrus Logic CS40L50 Haptic
> + Driver over SPI.
> +
> + This driver can be built as a module. If built as a module it will be
> + called "cs40l50-spi".
> +
> config MFD_VEXPRESS_SYSREG
> tristate "Versatile Express System Registers"
> depends on VEXPRESS_CONFIG && GPIOLIB
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 7ed3ef4a698c..3b1a43b3acaf 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -95,6 +95,10 @@ obj-$(CONFIG_MFD_MADERA) += madera.o
> obj-$(CONFIG_MFD_MADERA_I2C) += madera-i2c.o
> obj-$(CONFIG_MFD_MADERA_SPI) += madera-spi.o
>
> +obj-$(CONFIG_MFD_CS40L50_CORE) += cs40l50-core.o
> +obj-$(CONFIG_MFD_CS40L50_I2C) += cs40l50-i2c.o
> +obj-$(CONFIG_MFD_CS40L50_SPI) += cs40l50-spi.o
> +
> obj-$(CONFIG_TPS6105X) += tps6105x.o
> obj-$(CONFIG_TPS65010) += tps65010.o
> obj-$(CONFIG_TPS6507X) += tps6507x.o
> diff --git a/drivers/mfd/cs40l50-core.c b/drivers/mfd/cs40l50-core.c
> new file mode 100644
> index 000000000000..f1eadd80203a
> --- /dev/null
> +++ b/drivers/mfd/cs40l50-core.c
> @@ -0,0 +1,443 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CS40L50 Advanced Haptic Driver with waveform memory,
> + * integrated DSP, and closed-loop algorithms
> + *
> + * Copyright 2023 Cirrus Logic, Inc.
> + *
> + */
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/cs40l50.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
> +
> +static const struct mfd_cell cs40l50_devs[] = {
> + {
> + .name = "cs40l50-vibra",
> + },
> +};
I also recommend including the codec driver, especially because it will
force you to think what core components belong in the MFD (e.g. PSEQ
housekeeping as per the other thread).
If L50 shares a die with an audio amp that has its own codec driver,
finding a way to make it work as the codec child of this MFD would be
a beautiful solution, since presumably that audio amp has to manage the
PSEQ as well?
I'm sure lining up the audio and haptic amps is a lot messier than it
sounds in real life; maybe something to think about for next gen though.
For now, even an extremely simple codec driver should suffice.
> +
> +const struct regmap_config cs40l50_regmap = {
> + .reg_bits = 32,
> + .reg_stride = 4,
> + .val_bits = 32,
> + .reg_format_endian = REGMAP_ENDIAN_BIG,
> + .val_format_endian = REGMAP_ENDIAN_BIG,
> +};
> +EXPORT_SYMBOL_GPL(cs40l50_regmap);
> +
> +static struct regulator_bulk_data cs40l50_supplies[] = {
> + {
> + .supply = "vp",
> + },
> + {
> + .supply = "vio",
> + },
> +};
> +
> +static int cs40l50_handle_f0_est_done(struct cs40l50_private *cs40l50)
> +{
> + u32 f_zero;
> + int error;
> +
> + error = regmap_read(cs40l50->regmap, CS40L50_F0_ESTIMATION, &f_zero);
> + if (error)
> + return error;
> +
> + return regmap_write(cs40l50->regmap, CS40L50_F0_STORED, f_zero);
> +}
> +
> +static int cs40l50_handle_redc_est_done(struct cs40l50_private *cs40l50)
> +{
> + int error, fractional, integer, stored;
> + u32 redc;
> +
> + error = regmap_read(cs40l50->regmap, CS40L50_RE_EST_STATUS, &redc);
> + if (error)
> + return error;
> +
> + error = regmap_write(cs40l50->regmap, CS40L50_REDC_ESTIMATION, redc);
> + if (error)
> + return error;
> +
> + /* Convert from Q8.15 to (Q7.17 * 29/240) */
> + integer = ((redc >> 15) & 0xFF) << 17;
> + fractional = (redc & 0x7FFF) * 4;
> + stored = (integer | fractional) * 29 / 240;
> +
> + return regmap_write(cs40l50->regmap, CS40L50_REDC_STORED, stored);
> +}
> +
> +static int cs40l50_error_release(struct cs40l50_private *cs40l50)
> +{
> + int error;
> +
> + error = regmap_write(cs40l50->regmap, CS40L50_ERR_RLS,
> + CS40L50_GLOBAL_ERR_RLS);
> + if (error)
> + return error;
> +
> + return regmap_write(cs40l50->regmap, CS40L50_ERR_RLS, 0);
> +}
> +
> +static int cs40l50_mailbox_read_next(struct cs40l50_private *cs40l50, u32 *val)
> +{
> + u32 rd_ptr, wt_ptr;
> + int error;
> +
> + error = regmap_read(cs40l50->regmap, CS40L50_MBOX_QUEUE_WT, &wt_ptr);
> + if (error)
> + return error;
> +
> + error = regmap_read(cs40l50->regmap, CS40L50_MBOX_QUEUE_RD, &rd_ptr);
> + if (error)
> + return error;
> +
> + if (wt_ptr == rd_ptr) {
> + *val = 0;
> + return 0;
> + }
> +
> + error = regmap_read(cs40l50->regmap, rd_ptr, val);
> + if (error)
> + return error;
> +
> + rd_ptr += sizeof(u32);
> + if (rd_ptr > CS40L50_MBOX_QUEUE_END)
> + rd_ptr = CS40L50_MBOX_QUEUE_BASE;
> +
> + return regmap_write(cs40l50->regmap, CS40L50_MBOX_QUEUE_RD, rd_ptr);
> +}
> +
> +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);
Interleaving the mutex through the switch statement like this is dangerous;
it kind of looks like a zipper. It makes the code difficult to follow and
can lead to bugs in case the switch statement grows over time.
In general, we want to lock as little code as possible; maybe the mutex
needs to move inside the helper functions called from here.
> + 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");
> + break;
> + case CS40L50_MBOX_HAPTIC_TRIGGER_MBOX:
> + dev_dbg(cs40l50->dev, "Mailbox: TRIGGER_MBOX\n");
> + break;
> + case CS40L50_MBOX_HAPTIC_TRIGGER_I2S:
> + dev_dbg(cs40l50->dev, "Mailbox: TRIGGER_I2S\n");
> + break;
> + case CS40L50_MBOX_HAPTIC_COMPLETE_MBOX:
> + dev_dbg(cs40l50->dev, "Mailbox: COMPLETE_MBOX\n");
> + break;
> + case CS40L50_MBOX_HAPTIC_COMPLETE_GPIO:
> + dev_dbg(cs40l50->dev, "Mailbox: COMPLETE_GPIO\n");
> + break;
> + case CS40L50_MBOX_HAPTIC_COMPLETE_I2S:
> + dev_dbg(cs40l50->dev, "Mailbox: COMPLETE_I2S\n");
> + break;
> + case CS40L50_MBOX_F0_EST_START:
> + dev_dbg(cs40l50->dev, "Mailbox: F0_EST_START\n");
> + break;
> + case CS40L50_MBOX_F0_EST_DONE:
> + dev_dbg(cs40l50->dev, "Mailbox: F0_EST_DONE\n");
> + error = cs40l50_handle_f0_est_done(cs40l50);
> + if (error)
> + goto out_mutex;
> + break;
> + case CS40L50_MBOX_REDC_EST_START:
> + dev_dbg(cs40l50->dev, "Mailbox: REDC_EST_START\n");
> + break;
> + case CS40L50_MBOX_REDC_EST_DONE:
> + dev_dbg(cs40l50->dev, "Mailbox: REDC_EST_DONE\n");
> + error = cs40l50_handle_redc_est_done(cs40l50);
> + if (error)
> + goto out_mutex;
> + break;
> + case CS40L50_MBOX_LE_EST_START:
> + dev_dbg(cs40l50->dev, "Mailbox: LE_EST_START\n");
> + break;
> + case CS40L50_MBOX_LE_EST_DONE:
> + dev_dbg(cs40l50->dev, "Mailbox: LE_EST_DONE\n");
> + break;
> + case CS40L50_MBOX_AWAKE:
> + dev_dbg(cs40l50->dev, "Mailbox: AWAKE\n");
> + break;
> + case CS40L50_MBOX_INIT:
> + dev_dbg(cs40l50->dev, "Mailbox: INIT\n");
> + break;
> + case CS40L50_MBOX_ACK:
> + dev_dbg(cs40l50->dev, "Mailbox: ACK\n");
> + break;
> + case CS40L50_MBOX_ERR_EVENT_UNMAPPED:
> + dev_err(cs40l50->dev, "Unmapped event\n");
> + break;
> + case CS40L50_MBOX_ERR_EVENT_MODIFY:
> + dev_err(cs40l50->dev, "Failed to modify event index\n");
> + break;
> + case CS40L50_MBOX_ERR_NULLPTR:
> + dev_err(cs40l50->dev, "Null pointer\n");
> + break;
> + case CS40L50_MBOX_ERR_BRAKING:
> + dev_err(cs40l50->dev, "Braking not in progress\n");
> + break;
> + case CS40L50_MBOX_ERR_INVAL_SRC:
> + dev_err(cs40l50->dev, "Suspend/resume invalid source\n");
> + break;
> + case CS40L50_MBOX_ERR_ENABLE_RANGE:
> + dev_err(cs40l50->dev, "GPIO enable out of range\n");
> + break;
> + case CS40L50_MBOX_ERR_GPIO_UNMAPPED:
> + dev_err(cs40l50->dev, "GPIO not mapped\n");
> + break;
> + case CS40L50_MBOX_ERR_ISR_RANGE:
> + dev_err(cs40l50->dev, "GPIO ISR out of range\n");
> + break;
> + case CS40L50_MBOX_PERMANENT_SHORT:
> + dev_crit(cs40l50->dev, "Permanent short detected\n");
> + break;
> + 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;
> + }
> + }
I don't think we need such a large chunk of code just to translate the mailbox
codes to human-readable strings; leave that to the datasheet. Just print the
numeric value.
> +
> + error = -EIO;
> +
> +out_mutex:
> + mutex_unlock(&cs40l50->lock);
> +
> + return IRQ_RETVAL(!error);
> +}
> +
> +static irqreturn_t cs40l50_error(int irq, void *data);
> +
> +static const struct cs40l50_irq cs40l50_irqs[] = {
> + CS40L50_IRQ(AMP_SHORT, "Amp short", error),
> + CS40L50_IRQ(VIRT2_MBOX, "Mailbox", process_mbox),
> + CS40L50_IRQ(TEMP_ERR, "Overtemperature", error),
> + CS40L50_IRQ(BST_UVP, "Boost undervoltage", error),
> + CS40L50_IRQ(BST_SHORT, "Boost short", error),
> + CS40L50_IRQ(BST_ILIMIT, "Boost current limit", error),
> + CS40L50_IRQ(UVLO_VDDBATT, "Boost UVLO", error),
> + CS40L50_IRQ(GLOBAL_ERROR, "Global", error),
> +};
> +
> +static irqreturn_t cs40l50_error(int irq, void *data)
> +{
> + struct cs40l50_private *cs40l50 = data;
> +
> + dev_err(cs40l50->dev, "%s error\n", cs40l50_irqs[irq].name);
> +
> + return IRQ_RETVAL(!cs40l50_error_release(cs40l50));
> +}
> +
> +static const struct regmap_irq cs40l50_reg_irqs[] = {
> + CS40L50_REG_IRQ(IRQ1_INT_1, AMP_SHORT),
> + CS40L50_REG_IRQ(IRQ1_INT_2, VIRT2_MBOX),
> + CS40L50_REG_IRQ(IRQ1_INT_8, TEMP_ERR),
> + CS40L50_REG_IRQ(IRQ1_INT_9, BST_UVP),
> + CS40L50_REG_IRQ(IRQ1_INT_9, BST_SHORT),
> + CS40L50_REG_IRQ(IRQ1_INT_9, BST_ILIMIT),
> + CS40L50_REG_IRQ(IRQ1_INT_10, UVLO_VDDBATT),
> + CS40L50_REG_IRQ(IRQ1_INT_18, GLOBAL_ERROR),
> +};
> +
> +static struct regmap_irq_chip cs40l50_irq_chip = {
> + .name = "CS40L50 IRQ Controller",
> +
> + .status_base = CS40L50_IRQ1_INT_1,
> + .mask_base = CS40L50_IRQ1_MASK_1,
> + .ack_base = CS40L50_IRQ1_INT_1,
> + .num_regs = 22,
> +
> + .irqs = cs40l50_reg_irqs,
> + .num_irqs = ARRAY_SIZE(cs40l50_reg_irqs),
> +
> + .runtime_pm = true,
> +};
> +
> +static int cs40l50_irq_init(struct cs40l50_private *cs40l50)
> +{
> + struct device *dev = cs40l50->dev;
> + int error, i, irq;
> +
> + error = devm_regmap_add_irq_chip(dev, cs40l50->regmap, cs40l50->irq,
> + IRQF_ONESHOT | IRQF_SHARED, 0,
> + &cs40l50_irq_chip, &cs40l50->irq_data);
> + if (error)
> + return error;
> +
> + for (i = 0; i < ARRAY_SIZE(cs40l50_irqs); i++) {
> + irq = regmap_irq_get_virq(cs40l50->irq_data, cs40l50_irqs[i].irq);
> + if (irq < 0) {
> + dev_err(dev, "Failed getting %s\n", cs40l50_irqs[i].name);
> + return irq;
> + }
> +
> + error = devm_request_threaded_irq(dev, irq, NULL,
> + cs40l50_irqs[i].handler,
> + IRQF_ONESHOT | IRQF_SHARED,
> + cs40l50_irqs[i].name, cs40l50);
> + if (error) {
> + dev_err(dev, "Failed requesting %s\n", cs40l50_irqs[i].name);
> + return error;
> + }
> + }
This is kind of an uncommon design pattern; if anyone reads /proc/interrupts
on their system, it's going to be full of L50 interrupts. Normally we declare
a single IRQ, read the status register(s) from inside its handler and then
act accordingly.
What is the motivation for having one handler per interrupt status bit? If
multiple bits are set at once, does the register get read multiple times and
if so, does doing so clear any pending status? Or are the status registers
write-to-clear instead of read-to-clear?
> +
> + return 0;
> +}
> +
> +static int cs40l50_part_num_resolve(struct cs40l50_private *cs40l50)
> +{
> + struct device *dev = cs40l50->dev;
> + int error;
> +
> + error = regmap_read(cs40l50->regmap, CS40L50_DEVID, &cs40l50->devid);
> + if (error)
> + return error;
> +
> + if (cs40l50->devid != CS40L50_DEVID_A) {
> + dev_err(dev, "Invalid device ID: %#010X\n", cs40l50->devid);
> + return -EINVAL;
> + }
> +
> + error = regmap_read(cs40l50->regmap, CS40L50_REVID, &cs40l50->revid);
> + if (error)
> + return error;
> +
> + if (cs40l50->revid != CS40L50_REVID_B0) {
> + dev_err(dev, "Invalid revision: %#04X\n", cs40l50->revid);
> + return -EINVAL;
> + }
I recommend this be '<' and not '!=' so that the driver isn't immediately
broken if a backwards-compatible metal fix hits the market (e.g. B1).
> +
> + dev_info(dev, "Cirrus Logic CS40L50 revision %02X\n", cs40l50->revid);
> +
> + return 0;
> +}
> +
> +int cs40l50_probe(struct cs40l50_private *cs40l50)
> +{
> + struct device *dev = cs40l50->dev;
> + int error;
> +
> + mutex_init(&cs40l50->lock);
> +
> + cs40l50->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(cs40l50->reset_gpio))
> + return dev_err_probe(dev, PTR_ERR(cs40l50->reset_gpio),
> + "Failed getting reset GPIO\n");
> +
> + error = devm_regulator_bulk_get(dev, ARRAY_SIZE(cs40l50_supplies),
> + cs40l50_supplies);
> + if (error)
> + goto err_reset;
You shouldn't have to reset the device here; by initializing the GPIO as
GPIOD_OUT_HIGH, it is already asserted, which is required while the rails
are in an unknown state.
If GPIOD_OUT_HIGH is 1.8 V and not GND on your board, then the polarity
specified in your dts is backwards. gpiod is a logical API, not a physical
API. HIGH/1 = asserted (GND for active low when configured properly in
dts); LOW/0 = deasserted. Please check the flags in 'interrupts'.
> +
> + error = regulator_bulk_enable(ARRAY_SIZE(cs40l50_supplies),
> + cs40l50_supplies);
> + if (error)
> + goto err_reset;
And here.
> +
> + usleep_range(CS40L50_CP_READY_US, CS40L50_CP_READY_US + 100);
> +
> + gpiod_set_value_cansleep(cs40l50->reset_gpio, 1);
This confirms your dts is backwards. To drive an active-low reset high using
the gpiod API, you must write zero here. Fixing this will allow you to get
rid of the goto label.
> +
> + usleep_range(CS40L50_CP_READY_US, CS40L50_CP_READY_US + 1000);
> +
> + pm_runtime_set_autosuspend_delay(cs40l50->dev, CS40L50_AUTOSUSPEND_MS);
> + pm_runtime_use_autosuspend(cs40l50->dev);
> + pm_runtime_set_active(cs40l50->dev);
> + pm_runtime_get_noresume(cs40l50->dev);
> + devm_pm_runtime_enable(cs40l50->dev);
> +
> + error = cs40l50_part_num_resolve(cs40l50);
> + if (error)
> + goto err_supplies;
> +
> + error = cs40l50_irq_init(cs40l50);
> + if (error)
> + goto err_supplies;
> +
> + error = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, cs40l50_devs,
> + ARRAY_SIZE(cs40l50_devs), NULL, 0, NULL);
> + if (error)
> + goto err_supplies;
> +
> + pm_runtime_mark_last_busy(cs40l50->dev);
> + pm_runtime_put_autosuspend(cs40l50->dev);
> +
> + return 0;
> +
> +err_supplies:
> + regulator_bulk_disable(ARRAY_SIZE(cs40l50_supplies), cs40l50_supplies);
I recommend moving the disable call to a devm_add_action_or_reset callback;
this will save you the trouble of having to disable the regulators in the
error path or a remove callback, which can go away.
> +err_reset:
> + gpiod_set_value_cansleep(cs40l50->reset_gpio, 0);
This won't be necessary once you fix the polarity in your dts.
> +
> + return error;
> +}
> +EXPORT_SYMBOL_GPL(cs40l50_probe);
> +
> +int cs40l50_remove(struct cs40l50_private *cs40l50)
> +{
> + regulator_bulk_disable(ARRAY_SIZE(cs40l50_supplies), cs40l50_supplies);
> + gpiod_set_value_cansleep(cs40l50->reset_gpio, 1);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(cs40l50_remove);
> +
> +static int cs40l50_runtime_suspend(struct device *dev)
> +{
> + struct cs40l50_private *cs40l50 = dev_get_drvdata(dev);
> +
> + return regmap_write(cs40l50->regmap, CS40L50_DSP_MBOX, CS40L50_ALLOW_HIBER);
> +}
> +
> +static int cs40l50_runtime_resume(struct device *dev)
> +{
> + struct cs40l50_private *cs40l50 = dev_get_drvdata(dev);
> + int error, i;
> + u32 val;
> +
> + /* Device NAKs when exiting hibernation, so optionally retry here. */
> + for (i = 0; i < CS40L50_DSP_TIMEOUT_COUNT; i++) {
> + error = regmap_write(cs40l50->regmap, CS40L50_DSP_MBOX,
> + CS40L50_PREVENT_HIBER);
> + if (!error)
> + break;
> +
> + usleep_range(CS40L50_DSP_POLL_US, CS40L50_DSP_POLL_US + 100);
> + }
> +
> + for (; i < CS40L50_DSP_TIMEOUT_COUNT; i++) {
> + error = regmap_read(cs40l50->regmap, CS40L50_DSP_MBOX, &val);
> + if (!error && val == 0)
> + return 0;
> +
> + usleep_range(CS40L50_DSP_POLL_US, CS40L50_DSP_POLL_US + 100);
> + }
> +
> + return error ? error : -ETIMEDOUT;
This is a style preference, but it's perfectly legal to write 'return error ? : ...
> +}
> +
> +EXPORT_GPL_DEV_PM_OPS(cs40l50_pm_ops) = {
> + RUNTIME_PM_OPS(cs40l50_runtime_suspend, cs40l50_runtime_resume, NULL)
> +};
> +
> +MODULE_DESCRIPTION("CS40L50 Advanced Haptic Driver");
> +MODULE_AUTHOR("James Ogletree, Cirrus Logic Inc. <[email protected]>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/mfd/cs40l50-i2c.c b/drivers/mfd/cs40l50-i2c.c
> new file mode 100644
> index 000000000000..be1b130eb94b
> --- /dev/null
> +++ b/drivers/mfd/cs40l50-i2c.c
> @@ -0,0 +1,69 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CS40L50 I2C Driver
> + *
> + * Copyright 2023 Cirrus Logic, Inc.
> + *
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/mfd/cs40l50.h>
> +
> +static int cs40l50_i2c_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct device *dev = &client->dev;
> + struct cs40l50_private *cs40l50;
> +
> + cs40l50 = devm_kzalloc(dev, sizeof(*cs40l50), GFP_KERNEL);
> + if (!cs40l50)
> + return -ENOMEM;
> +
> + i2c_set_clientdata(client, cs40l50);
> +
> + cs40l50->regmap = devm_regmap_init_i2c(client, &cs40l50_regmap);
> + if (IS_ERR(cs40l50->regmap))
> + return dev_err_probe(cs40l50->dev, PTR_ERR(cs40l50->regmap),
> + "Failed to initialize register map\n");
> +
> + cs40l50->dev = dev;
> + cs40l50->irq = client->irq;
> +
> + return cs40l50_probe(cs40l50);
> +}
> +
> +static void cs40l50_i2c_remove(struct i2c_client *client)
> +{
> + struct cs40l50_private *cs40l50 = i2c_get_clientdata(client);
> +
> + cs40l50_remove(cs40l50);
> +}
> +
> +static const struct i2c_device_id cs40l50_id_i2c[] = {
> + {"cs40l50", 0},
> + {}
> +};
> +MODULE_DEVICE_TABLE(i2c, cs40l50_id_i2c);
> +
> +static const struct of_device_id cs40l50_of_match[] = {
> + { .compatible = "cirrus,cs40l50" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, cs40l50_of_match);
> +
> +static struct i2c_driver cs40l50_i2c_driver = {
> + .driver = {
> + .name = "cs40l50",
> + .of_match_table = cs40l50_of_match,
> + .pm = pm_ptr(&cs40l50_pm_ops),
> + },
> + .id_table = cs40l50_id_i2c,
> + .probe = cs40l50_i2c_probe,
> + .remove = cs40l50_i2c_remove,
> +};
> +
> +module_i2c_driver(cs40l50_i2c_driver);
> +
> +MODULE_DESCRIPTION("CS40L50 I2C Driver");
> +MODULE_AUTHOR("James Ogletree, Cirrus Logic Inc. <[email protected]>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/mfd/cs40l50-spi.c b/drivers/mfd/cs40l50-spi.c
> new file mode 100644
> index 000000000000..8311d48efedf
> --- /dev/null
> +++ b/drivers/mfd/cs40l50-spi.c
> @@ -0,0 +1,68 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CS40L50 SPI Driver
> + *
> + * Copyright 2023 Cirrus Logic, Inc.
> + *
> + */
> +
> +#include <linux/input/cs40l50.h>
> +#include <linux/mfd/spi.h>
> +
> +static int cs40l50_spi_probe(struct spi_device *spi)
> +{
> + struct cs40l50_private *cs40l50;
> + struct device *dev = &spi->dev;
> +
> + cs40l50 = devm_kzalloc(dev, sizeof(*cs40l50), GFP_KERNEL);
> + if (!cs40l50)
> + return -ENOMEM;
> +
> + spi_set_drvdata(spi, cs40l50);
> +
> + cs40l50->regmap = devm_regmap_init_spi(spi, &cs40l50_regmap);
> + if (IS_ERR(cs40l50->regmap))
> + return dev_err_probe(cs40l50->dev, PTR_ERR(cs40l50->regmap),
> + "Failed to initialize register map\n");
> +
> + cs40l50->dev = dev;
> + cs40l50->irq = spi->irq;
> +
> + return cs40l50_probe(cs40l50);
> +}
> +
> +static void cs40l50_spi_remove(struct spi_device *spi)
> +{
> + struct cs40l50_private *cs40l50 = spi_get_drvdata(spi);
> +
> + cs40l50_remove(cs40l50);
> +}
> +
> +static const struct spi_device_id cs40l50_id_spi[] = {
> + {"cs40l50", 0},
> + {}
> +};
> +MODULE_DEVICE_TABLE(spi, cs40l50_id_spi);
> +
> +static const struct of_device_id cs40l50_of_match[] = {
> + { .compatible = "cirrus,cs40l50" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, cs40l50_of_match);
> +
> +static struct spi_driver cs40l50_spi_driver = {
> + .driver = {
> + .name = "cs40l50",
> + .of_match_table = cs40l50_of_match,
> + .pm = pm_ptr(&cs40l50_pm_ops),
> + },
> + .id_table = cs40l50_id_spi,
> + .probe = cs40l50_spi_probe,
> + .remove = cs40l50_spi_remove,
> +};
> +
> +module_spi_driver(cs40l50_spi_driver);
> +
> +MODULE_DESCRIPTION("CS40L50 SPI Driver");
> +MODULE_AUTHOR("James Ogletree, Cirrus Logic Inc. <[email protected]>");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/cs40l50.h b/include/linux/mfd/cs40l50.h
> new file mode 100644
> index 000000000000..c625a999a5ae
> --- /dev/null
> +++ b/include/linux/mfd/cs40l50.h
> @@ -0,0 +1,198 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + *
> + * CS40L50 Advanced Haptic Driver with waveform memory,
> + * integrated DSP, and closed-loop algorithms
> + *
> + * Copyright 2023 Cirrus Logic, Inc.
> + *
> + */
> +
> +#ifndef __CS40L50_H__
> +#define __CS40L50_H__
> +
> +#include <linux/firmware/cirrus/cs_dsp.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/input.h>
> +#include <linux/input/cirrus_haptics.h>
> +#include <linux/interrupt.h>
> +#include <linux/pm.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +
> +/* Power Supply Configuration */
> +#define CS40L50_BLOCK_ENABLES2 0x201C
> +#define CS40L50_ERR_RLS 0x2034
> +#define CS40L50_PWRMGT_CTL 0x2900
> +#define CS40L50_BST_LPMODE_SEL 0x3810
> +#define CS40L50_DCM_LOW_POWER 0x1
> +#define CS40L50_OVERTEMP_WARN 0x4000010
> +
> +/* Interrupts */
> +#define CS40L50_IRQ1_INT_1 0xE010
> +#define CS40L50_IRQ1_INT_2 0xE014
> +#define CS40L50_IRQ1_INT_8 0xE02C
> +#define CS40L50_IRQ1_INT_9 0xE030
> +#define CS40L50_IRQ1_INT_10 0xE034
> +#define CS40L50_IRQ1_INT_18 0xE054
> +#define CS40L50_IRQ1_MASK_1 0xE090
> +#define CS40L50_IRQ1_MASK_2 0xE094
> +#define CS40L50_IRQ1_MASK_20 0xE0DC
> +#define CS40L50_IRQ_MASK_2_OVERRIDE 0xFFDF7FFF
> +#define CS40L50_IRQ_MASK_20_OVERRIDE 0x15C01000
> +#define CS40L50_AMP_SHORT_MASK BIT(31)
> +#define CS40L50_VIRT2_MBOX_MASK BIT(21)
> +#define CS40L50_TEMP_ERR_MASK BIT(31)
> +#define CS40L50_BST_UVP_MASK BIT(6)
> +#define CS40L50_BST_SHORT_MASK BIT(7)
> +#define CS40L50_BST_ILIMIT_MASK BIT(8)
> +#define CS40L50_UVLO_VDDBATT_MASK BIT(16)
> +#define CS40L50_GLOBAL_ERROR_MASK BIT(15)
> +#define CS40L50_GLOBAL_ERR_RLS BIT(11)
> +#define CS40L50_IRQ(_irq, _name, _hand) \
> + { \
> + .irq = CS40L50_ ## _irq ## _IRQ,\
> + .name = _name, \
> + .handler = cs40l50_ ## _hand, \
> + }
> +#define CS40L50_REG_IRQ(_reg, _irq) \
> + [CS40L50_ ## _irq ## _IRQ] = { \
> + .reg_offset = (CS40L50_ ## _reg) - CS40L50_IRQ1_INT_1, \
> + .mask = CS40L50_ ## _irq ## _MASK \
> + }
> +
> +/* Mailbox Inbound Commands */
> +#define CS40L50_RAM_BANK_INDEX_START 0x1000000
> +#define CS40L50_RTH_INDEX_START 0x1400000
> +#define CS40L50_RTH_INDEX_END 0x1400001
> +#define CS40L50_ROM_BANK_INDEX_START 0x1800000
> +#define CS40L50_ROM_BANK_INDEX_END 0x180001A
> +#define CS40L50_PREVENT_HIBER 0x2000003
> +#define CS40L50_ALLOW_HIBER 0x2000004
> +#define CS40L50_OWT_PUSH 0x3000008
> +#define CS40L50_STOP_PLAYBACK 0x5000000
> +#define CS40L50_OWT_DELETE 0xD000000
> +
> +/* Mailbox Outbound Commands */
> +#define CS40L50_MBOX_QUEUE_BASE 0x11004
> +#define CS40L50_MBOX_QUEUE_END 0x1101C
> +#define CS40L50_DSP_MBOX 0x11020
> +#define CS40L50_MBOX_QUEUE_WT 0x28042C8
> +#define CS40L50_MBOX_QUEUE_RD 0x28042CC
> +#define CS40L50_MBOX_HAPTIC_COMPLETE_MBOX 0x1000000
> +#define CS40L50_MBOX_HAPTIC_COMPLETE_GPIO 0x1000001
> +#define CS40L50_MBOX_HAPTIC_COMPLETE_I2S 0x1000002
> +#define CS40L50_MBOX_HAPTIC_TRIGGER_MBOX 0x1000010
> +#define CS40L50_MBOX_HAPTIC_TRIGGER_GPIO 0x1000011
> +#define CS40L50_MBOX_HAPTIC_TRIGGER_I2S 0x1000012
> +#define CS40L50_MBOX_INIT 0x2000000
> +#define CS40L50_MBOX_AWAKE 0x2000002
> +#define CS40L50_MBOX_F0_EST_START 0x7000011
> +#define CS40L50_MBOX_F0_EST_DONE 0x7000021
> +#define CS40L50_MBOX_REDC_EST_START 0x7000012
> +#define CS40L50_MBOX_REDC_EST_DONE 0x7000022
> +#define CS40L50_MBOX_LE_EST_START 0x7000014
> +#define CS40L50_MBOX_LE_EST_DONE 0x7000024
> +#define CS40L50_MBOX_ACK 0xA000000
> +#define CS40L50_MBOX_PANIC 0xC000000
> +#define CS40L50_MBOX_WATERMARK 0xD000000
> +#define CS40L50_MBOX_ERR_EVENT_UNMAPPED 0xC0004B3
> +#define CS40L50_MBOX_ERR_EVENT_MODIFY 0xC0004B4
> +#define CS40L50_MBOX_ERR_NULLPTR 0xC0006A5
> +#define CS40L50_MBOX_ERR_BRAKING 0xC0006A8
> +#define CS40L50_MBOX_ERR_INVAL_SRC 0xC0006AC
> +#define CS40L50_MBOX_ERR_ENABLE_RANGE 0xC000836
> +#define CS40L50_MBOX_ERR_GPIO_UNMAPPED 0xC000837
> +#define CS40L50_MBOX_ERR_ISR_RANGE 0xC000838
> +#define CS40L50_MBOX_PERMANENT_SHORT 0xC000C1C
> +#define CS40L50_MBOX_RUNTIME_SHORT 0xC000C1D
> +
> +/* DSP */
> +#define CS40L50_DSP1_XMEM_PACKED_0 0x2000000
> +#define CS40L50_DSP1_XMEM_UNPACKED32_0 0x2400000
> +#define CS40L50_SYS_INFO_ID 0x25E0000
> +#define CS40L50_DSP1_XMEM_UNPACKED24_0 0x2800000
> +#define CS40L50_RAM_INIT 0x28021DC
> +#define CS40L50_POWER_ON_SEQ 0x2804320
> +#define CS40L50_OWT_BASE 0x2805C34
> +#define CS40L50_NUM_OF_WAVES 0x280CB4C
> +#define CS40L50_CORE_BASE 0x2B80000
> +#define CS40L50_CCM_CORE_CONTROL 0x2BC1000
> +#define CS40L50_DSP1_YMEM_PACKED_0 0x2C00000
> +#define CS40L50_DSP1_YMEM_UNPACKED32_0 0x3000000
> +#define CS40L50_DSP1_YMEM_UNPACKED24_0 0x3400000
> +#define CS40L50_DSP1_PMEM_0 0x3800000
> +#define CS40L50_DSP1_PMEM_5114 0x3804FE8
> +#define CS40L50_MEM_RDY_HW 0x2
> +#define CS40L50_RAM_INIT_FLAG 0x1
> +#define CS40L50_CLOCK_DISABLE 0x80
> +#define CS40L50_CLOCK_ENABLE 0x281
> +#define CS40L50_DSP_POLL_US 1000
> +#define CS40L50_DSP_TIMEOUT_COUNT 100
> +#define CS40L50_CP_READY_US 2200
> +#define CS40L50_PSEQ_SIZE 200
> +#define CS40L50_AUTOSUSPEND_MS 2000
> +
> +/* Firmware */
> +#define CS40L50_FW "cs40l50.wmfw"
> +#define CS40L50_WT "cs40l50.bin"
> +
> +/* Calibration */
> +#define CS40L50_REDC_ESTIMATION 0x2802F7C
> +#define CS40L50_F0_ESTIMATION 0x2802F84
> +#define CS40L50_F0_STORED 0x2805C00
> +#define CS40L50_REDC_STORED 0x2805C04
> +#define CS40L50_RE_EST_STATUS 0x3401B40
> +
> +/* Open wavetable */
> +#define CS40L50_OWT_SIZE 0x2805C38
> +#define CS40L50_OWT_NEXT 0x2805C3C
> +#define CS40L50_NUM_OF_OWT_WAVES 0x2805C40
> +
> +/* GPIO */
> +#define CS40L50_GPIO_BASE 0x2804140
> +
> +/* Device */
> +#define CS40L50_DEVID 0x0
> +#define CS40L50_REVID 0x4
> +#define CS40L50_DEVID_A 0x40A50
> +#define CS40L50_REVID_B0 0xB0
> +
> +enum cs40l50_irq_list {
> + CS40L50_GLOBAL_ERROR_IRQ,
> + CS40L50_UVLO_VDDBATT_IRQ,
> + CS40L50_BST_ILIMIT_IRQ,
> + CS40L50_BST_SHORT_IRQ,
> + CS40L50_BST_UVP_IRQ,
> + CS40L50_TEMP_ERR_IRQ,
> + CS40L50_VIRT2_MBOX_IRQ,
> + CS40L50_AMP_SHORT_IRQ,
> +};
> +
> +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;
> + const struct firmware *wmfw;
> + struct cs_hap haptics;
> + u32 devid;
> + u32 revid;
> + int irq;
> +};
> +
> +int cs40l50_probe(struct cs40l50_private *cs40l50);
> +int cs40l50_remove(struct cs40l50_private *cs40l50);
> +
> +extern const struct regmap_config cs40l50_regmap;
> +extern const struct dev_pm_ops cs40l50_pm_ops;
> +
> +#endif /* __CS40L50_H__ */
> --
> 2.25.1
>
Kind regards,
Jeff LaBundy
On Tue, 24 Oct 2023, Jeff LaBundy wrote:
> Hi James,
>
> Just a few trailing comments on top of Lee's feedback.
>
> On Wed, Oct 18, 2023 at 05:57:24PM +0000, James Ogletree wrote:
> > From: James Ogletree <[email protected]>
> >
> > Introduce support for Cirrus Logic Device CS40L50: a
> > haptic driver with waveform memory, integrated DSP,
> > and closed-loop algorithms.
> >
> > The MFD component registers and initializes the device.
> >
> > Signed-off-by: James Ogletree <[email protected]>
> > ---
> > MAINTAINERS | 2 +
> > drivers/mfd/Kconfig | 30 +++
> > drivers/mfd/Makefile | 4 +
> > drivers/mfd/cs40l50-core.c | 443 ++++++++++++++++++++++++++++++++++++
> > drivers/mfd/cs40l50-i2c.c | 69 ++++++
> > drivers/mfd/cs40l50-spi.c | 68 ++++++
> > include/linux/mfd/cs40l50.h | 198 ++++++++++++++++
> > 7 files changed, 814 insertions(+)
> > create mode 100644 drivers/mfd/cs40l50-core.c
> > create mode 100644 drivers/mfd/cs40l50-i2c.c
> > create mode 100644 drivers/mfd/cs40l50-spi.c
> > create mode 100644 include/linux/mfd/cs40l50.h
Just popping along to say; these are excellent comments Jeff.
Thank you for your review.
--
Lee Jones [李琼斯]