Re: [PATCH 2/2] Input: cs40l50 - Initial support for Cirrus Logic CS40L50

From: Krzysztof Kozlowski
Date: Wed Aug 09 2023 - 02:47:20 EST

Next message: Dmitry Rokosov: "Re: [PATCH v1 1/1] iio: accel: msa311: Use correct header(s) instead of string_helpers.h"
Previous message: Krzysztof Kozlowski: "Re: [PATCH 1/2] dt-bindings: input: cirrus,cs40l50: Support for CS40L50"
In reply to: James Ogletree: "[PATCH 2/2] Input: cs40l50 - Initial support for Cirrus Logic CS40L50"
Next in thread: James Ogletree: "Re: [PATCH 2/2] Input: cs40l50 - Initial support for Cirrus Logic CS40L50"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 08/08/2023 19:25, James Ogletree wrote:
> Introduce support for Cirrus Logic Device CS40L50: a
> haptics driver with waveform memory DSP and closed-loop
> algorithms.
>
> Change-Id: I468e8f726e81072ebea19b2dfbe23ca775fedc97

Please run scripts/checkpatch.pl and fix reported warnings. Some
warnings can be ignored, but the code here looks like it needs a fix.
Feel free to get in touch if the warning is not clear.

> Signed-off-by: James Ogletree <james.ogletree@xxxxxxxxxx>
> ---
> MAINTAINERS | 2 +
> drivers/input/misc/Kconfig | 33 +
> drivers/input/misc/Makefile | 3 +
> drivers/input/misc/cs40l50-i2c.c | 67 ++
> drivers/input/misc/cs40l50-spi.c | 67 ++
> drivers/input/misc/cs40l50.c | 1013 ++++++++++++++++++++++++++++++
> include/linux/input/cs40l50.h | 321 ++++++++++
> 7 files changed, 1506 insertions(+)
> create mode 100644 drivers/input/misc/cs40l50-i2c.c
> create mode 100644 drivers/input/misc/cs40l50-spi.c
> create mode 100644 drivers/input/misc/cs40l50.c
> create mode 100644 include/linux/input/cs40l50.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 016e5ff3b831..f1d913166166 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2089,6 +2089,8 @@ L: patches@xxxxxxxxxxxxxxxxxxxxxx
> S: Supported
> T: git https://github.com/CirrusLogic/linux-drivers.git
> F: Documentation/devicetree/bindings/input/cirrus,cs40l50.yaml
> +F: drivers/input/misc/cs40l*
> +F: include/linux/input/cs40l*
>
> ARM/CLKDEV SUPPORT
> M: Russell King <linux@xxxxxxxxxxxxxxx>
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index c47eecc6f83b..fa0bdd23f87d 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -140,6 +140,39 @@ config INPUT_BMA150
> To compile this driver as a module, choose M here: the
> module will be called bma150.
>
> +config INPUT_CS40L50
> + tristate "Cirrus Logic CS40L50 Haptic Driver support"
> + select CS_DSP
> + select REGMAP_IRQ
> + help
> + Say Y here to enable support for CS40L50 boosted haptic
> + driver.
> +
> + To compile the driver as a module choose M here: the module
> + will be called cs40l50_core.
> +
> +config INPUT_CS40L50_I2C
> + tristate "Support I2C bus connection"
> + depends on INPUT_CS40L50 && I2C
> + select REGMAP_I2C
> + help
> + Say Y here if you have Cirrus Logic's CS40L50 connected
> + to an I2C bus.
> +
> + To compile the driver as a module choose M here: the
> + module will be called cs40l50_i2c
> +
> +config INPUT_CS40L50_SPI
> + tristate "Support SPI bus connection"
> + depends on INPUT_CS40L50 && SPI
> + select REGMAP_SPI
> + help
> + Say Y here if you have Cirrus Logic's CS40L50 connected
> + to a SPI bus.
> +
> + To compile the driver as a module choose M here: the
> + module will be called cs40l50_spi.
> +
> config INPUT_E3X0_BUTTON
> tristate "NI Ettus Research USRP E3xx Button support."
> default n
> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> index 04296a4abe8e..77f34a33c364 100644
> --- a/drivers/input/misc/Makefile
> +++ b/drivers/input/misc/Makefile
> @@ -28,6 +28,9 @@ obj-$(CONFIG_INPUT_CMA3000) += cma3000_d0x.o
> obj-$(CONFIG_INPUT_CMA3000_I2C) += cma3000_d0x_i2c.o
> obj-$(CONFIG_INPUT_COBALT_BTNS) += cobalt_btns.o
> obj-$(CONFIG_INPUT_CPCAP_PWRBUTTON) += cpcap-pwrbutton.o
> +obj-$(CONFIG_INPUT_CS40L50) += cs40l50.o
> +obj-$(CONFIG_INPUT_CS40L50_I2C) += cs40l50-i2c.o
> +obj-$(CONFIG_INPUT_CS40L50_SPI) += cs40l50-spi.o
> obj-$(CONFIG_INPUT_DA7280_HAPTICS) += da7280.o
> obj-$(CONFIG_INPUT_DA9052_ONKEY) += da9052_onkey.o
> obj-$(CONFIG_INPUT_DA9055_ONKEY) += da9055_onkey.o
> diff --git a/drivers/input/misc/cs40l50-i2c.c b/drivers/input/misc/cs40l50-i2c.c
> new file mode 100644
> index 000000000000..bab07debdaa9
> --- /dev/null
> +++ b/drivers/input/misc/cs40l50-i2c.c
> @@ -0,0 +1,67 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CS40L50 Advanced Haptic Driver with Waveform Memory DSP
> + * and Closed-Loop Algorithms
> + *
> + * Copyright 2023 Cirrus Logic, Inc.
> + *
> + */
> +#include <linux/i2c.h>
> +#include <linux/input/cs40l50.h>
> +
> +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 int cs40l50_i2c_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct cs40l50_private *cs40l50;
> +
> + cs40l50 = devm_kzalloc(dev, sizeof(struct cs40l50_private), GFP_KERNEL);

sizeof(*)

> + 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 struct i2c_driver cs40l50_i2c_driver = {
> + .driver = {
> + .name = "cs40l50",
> + .of_match_table = cs40l50_of_match,
> + },
> + .id_table = cs40l50_id_i2c,
> + .probe_new = 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. <james.ogletree@xxxxxxxxxx>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/input/misc/cs40l50-spi.c b/drivers/input/misc/cs40l50-spi.c
> new file mode 100644
> index 000000000000..dbaa185b6bbd
> --- /dev/null
> +++ b/drivers/input/misc/cs40l50-spi.c
> @@ -0,0 +1,67 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CS40L50 Advanced Haptic Driver with Waveform Memory DSP
> + * and Closed-Loop Algorithms
> + *
> + * Copyright 2023 Cirrus Logic, Inc.
> + *
> + */
> +#include <linux/spi/spi.h>
> +#include <linux/input/cs40l50.h>
> +
> +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 int cs40l50_spi_probe(struct spi_device *spi)
> +{
> + struct cs40l50_private *cs40l50;
> + struct device *dev = &spi->dev;
> +
> + cs40l50 = devm_kzalloc(dev, sizeof(struct cs40l50_private), GFP_KERNEL);

sizeof(*)
In all other places as well.

...

> +
> +int cs40l50_probe(struct cs40l50_private *cs40l50)
> +{
> + int error, i, irq;
> + u32 val;
> +
> + mutex_init(&cs40l50->lock);
> +
> + error = devm_regulator_bulk_get(cs40l50->dev, ARRAY_SIZE(cs40l50_supplies),
> + cs40l50_supplies);
> + if (error) {
> + dev_err(cs40l50->dev, "Failed to request supplies\n");

dev_err_probe

> + goto err;

So you call cs40l50_remove() here? Are you sure you tested this path?

> + }
> +
> + error = regulator_bulk_enable(ARRAY_SIZE(cs40l50_supplies), cs40l50_supplies);
> + if (error) {
> + dev_err(cs40l50->dev, "Failed to enable supplies\n");

This for sure is wrong. Why do you disable regulators when enabling them
failed?

> + goto err;
> + }
> +
> + cs40l50->reset_gpio = devm_gpiod_get_optional(cs40l50->dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(cs40l50->reset_gpio)) {
> + error = PTR_ERR(cs40l50->reset_gpio);
> + goto err;
> + }
> +
> + usleep_range(CS40L50_MIN_RESET_PULSE_WIDTH_US, CS40L50_MIN_RESET_PULSE_WIDTH_US + 100);
> +
> + gpiod_set_value_cansleep(cs40l50->reset_gpio, 0);
> +
> + usleep_range(CS40L50_CP_READY_DELAY_US, CS40L50_CP_READY_DELAY_US + 1000);
> +
> + for (i = 0; i < CS40L50_DSP_TIMEOUT_COUNT; i++) {
> + error = cs40l50_dsp_read(cs40l50, CS40L50_DSP1_HALO_STATE, &val);
> + if (!error && val < 0xFFFF && val >= CS40L50_HALO_STATE_BOOT_DONE)
> + break;
> +
> + usleep_range(CS40L50_DSP_POLL_US, CS40L50_DSP_POLL_US + 100);
> + }
> +
> + if (i == CS40L50_DSP_TIMEOUT_COUNT) {
> + dev_err(cs40l50->dev, "Firmware boot failed: %d, halo state = %#x\n", error, val);
> + goto err;
> + }
> +
> + cs40l50->vibe_workqueue = alloc_ordered_workqueue("cs40l50_workqueue", WQ_HIGHPRI);
> + if (!cs40l50->vibe_workqueue) {
> + error = -ENOMEM;
> + goto err;
> + }
> +
> + INIT_WORK(&cs40l50->vibe_start_work, cs40l50_vibe_start_worker);
> + INIT_WORK(&cs40l50->vibe_stop_work, cs40l50_vibe_stop_worker);
> +
> + error = cs40l50_cs_dsp_init(cs40l50);
> + if (error)
> + goto err;
> +
> + error = cs40l50_input_init(cs40l50);
> + if (error)
> + goto err;
> +
> + error = cs40l50_patch_firmware(cs40l50);
> + if (error)
> + goto err;
> +
> + error = cs40l50_pseq_init(cs40l50);
> + if (error)
> + goto err;
> +
> + error = cs40l50_config_bst(cs40l50);
> + if (error)
> + goto err;
> +
> + error = devm_regmap_add_irq_chip(cs40l50->dev, cs40l50->regmap, cs40l50->irq,
> + IRQF_ONESHOT | IRQF_SHARED | IRQF_TRIGGER_LOW, 0, &cs40l50_regmap_irq_chip,
> + &cs40l50->irq_data);
> + if (error) {
> + dev_err(cs40l50->dev, "Failed to register IRQ chip: %d\n", error);
> + goto err;
> + }
> +
> + 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(cs40l50->dev, "Failed to get %s\n", cs40l50_irqs[i].name);
> + error = irq;
> + goto err;
> + }
> +
> + error = devm_request_threaded_irq(cs40l50->dev, irq, NULL, cs40l50_irqs[i].handler,
> + IRQF_ONESHOT | IRQF_SHARED | IRQF_TRIGGER_LOW,
> + cs40l50_irqs[i].name, cs40l50);
> + if (error) {
> + dev_err(cs40l50->dev, "Failed to request IRQ %s: %d\n",
> + cs40l50_irqs[i].name, error);
> + goto err;
> + }
> + }
> +
> + return 0;
> +
> +err:
> + cs40l50_remove(cs40l50);
> +
> + return error;
> +}
> +EXPORT_SYMBOL_GPL(cs40l50_probe);
> +
> +int cs40l50_remove(struct cs40l50_private *cs40l50)
> +{
> + disable_irq(cs40l50->irq);
> +
> + if (cs40l50->dsp.booted)
> + cs_dsp_power_down(&cs40l50->dsp);
> + if (&cs40l50->dsp)
> + cs_dsp_remove(&cs40l50->dsp);
> +
> + if (cs40l50->vibe_workqueue) {
> + flush_workqueue(cs40l50->vibe_workqueue);
> + destroy_workqueue(cs40l50->vibe_workqueue);
> + }
> +
> + gpiod_set_value_cansleep(cs40l50->reset_gpio, 1);
> + regulator_bulk_disable(ARRAY_SIZE(cs40l50_supplies), cs40l50_supplies);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(cs40l50_remove);
> +
> +MODULE_DESCRIPTION("CS40L50 Advanced Haptic Driver");
> +MODULE_AUTHOR("James Ogletree, Cirrus Logic Inc. <james.ogletree@xxxxxxxxxx>");
> +MODULE_LICENSE("GPL");

I don't think this is a module.

Best regards,
Krzysztof

Next message: Dmitry Rokosov: "Re: [PATCH v1 1/1] iio: accel: msa311: Use correct header(s) instead of string_helpers.h"
Previous message: Krzysztof Kozlowski: "Re: [PATCH 1/2] dt-bindings: input: cirrus,cs40l50: Support for CS40L50"
In reply to: James Ogletree: "[PATCH 2/2] Input: cs40l50 - Initial support for Cirrus Logic CS40L50"
Next in thread: James Ogletree: "Re: [PATCH 2/2] Input: cs40l50 - Initial support for Cirrus Logic CS40L50"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]