* [PATCH 2/5] HID: mcp2221: Allow IO to start during probe
From: Hamish Martin @ 2023-10-25 3:55 UTC (permalink / raw)
To: gupt21, jikos, benjamin.tissoires, Enrik.Berkhan, sven.zuehlsdorf
Cc: linux-i2c, linux-input, Hamish Martin
In-Reply-To: <20231025035514.3450123-1-hamish.martin@alliedtelesis.co.nz>
During the probe we add an I2C adapter and as soon as we add that adapter
it may be used for a transfer (e.g via the code in i2cdetect()).
Those transfers are not able to complete and time out. This is because the
HID raw_event callback (mcp2221_raw_event) will not be invoked until the
HID device's 'driver_input_lock' is marked up at the completion of the
probe in hid_device_probe(). This starves the driver of the responses it
is waiting for.
In order to allow the I2C transfers to complete while we are still in the
probe, start the IO once we have completed init of the HID device.
This issue seems to have been seen before and a patch was submitted but
it seems it was never accepted. See:
https://lore.kernel.org/all/20221103222714.21566-3-Enrik.Berkhan@inka.de/
Signed-off-by: Hamish Martin <hamish.martin@alliedtelesis.co.nz>
---
drivers/hid/hid-mcp2221.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
index b95f31cf0fa2..aef0785c91cc 100644
--- a/drivers/hid/hid-mcp2221.c
+++ b/drivers/hid/hid-mcp2221.c
@@ -1142,6 +1142,8 @@ static int mcp2221_probe(struct hid_device *hdev,
if (ret)
return ret;
+ hid_device_io_start(hdev);
+
/* Set I2C bus clock diviser */
if (i2c_clk_freq > 400)
i2c_clk_freq = 400;
--
2.42.0
^ permalink raw reply related
* [PATCH 1/5] HID: mcp2221: Set driver data before I2C adapter add
From: Hamish Martin @ 2023-10-25 3:55 UTC (permalink / raw)
To: gupt21, jikos, benjamin.tissoires, Enrik.Berkhan, sven.zuehlsdorf
Cc: linux-i2c, linux-input, Hamish Martin
In-Reply-To: <20231025035514.3450123-1-hamish.martin@alliedtelesis.co.nz>
The process of adding an I2C adapter can invoke I2C accesses on that new
adapter (see i2c_detect()).
Ensure we have set the adapter's driver data to avoid null pointer
dereferences in the xfer functions during the adapter add.
This has been noted in the past and the same fix proposed but not
completed. See:
https://lore.kernel.org/lkml/ef597e73-ed71-168e-52af-0d19b03734ac@vigem.de/
Signed-off-by: Hamish Martin <hamish.martin@alliedtelesis.co.nz>
---
drivers/hid/hid-mcp2221.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
index 72883e0ce757..b95f31cf0fa2 100644
--- a/drivers/hid/hid-mcp2221.c
+++ b/drivers/hid/hid-mcp2221.c
@@ -1157,12 +1157,12 @@ static int mcp2221_probe(struct hid_device *hdev,
snprintf(mcp->adapter.name, sizeof(mcp->adapter.name),
"MCP2221 usb-i2c bridge");
+ i2c_set_adapdata(&mcp->adapter, mcp);
ret = devm_i2c_add_adapter(&hdev->dev, &mcp->adapter);
if (ret) {
hid_err(hdev, "can't add usb-i2c adapter: %d\n", ret);
return ret;
}
- i2c_set_adapdata(&mcp->adapter, mcp);
#if IS_REACHABLE(CONFIG_GPIOLIB)
/* Setup GPIO chip */
--
2.42.0
^ permalink raw reply related
* [PATCH 0/5] MCP2221 Improvements
From: Hamish Martin @ 2023-10-25 3:55 UTC (permalink / raw)
To: gupt21, jikos, benjamin.tissoires, Enrik.Berkhan, sven.zuehlsdorf
Cc: linux-i2c, linux-input, Hamish Martin
Recently I've been prototyping a new system which uses a MCP2221 chip as
the I2C bus. During that development a few issues were found that form
the patches in this series.
The first two are resolving long standing issues that have both been
reported before, patches submitted, but it appears never accepted.
The ACPI patch resolves an issue in my x86_64 system.
The final two address fundamental issues with the driver that have not
worked correctly from the beginning it seems.
Please review and let me know what you think.
Thanks,
Hamish Martin
Hamish Martin (5):
HID: mcp2221: Set driver data before I2C adapter add
HID: mcp2221: Allow IO to start during probe
HID: mcp2221: Set ACPI companion
HID: mcp2221: Don't set bus speed on every transfer
HID: mcp2221: Handle reads greater than 60 bytes
drivers/hid/hid-mcp2221.c | 76 +++++++++++++++++++++++++++------------
1 file changed, 53 insertions(+), 23 deletions(-)
--
2.42.0
^ permalink raw reply
* Re: [PATCH v4 3/4] mfd: cs40l50: Add support for CS40L50 core driver
From: Jeff LaBundy @ 2023-10-25 3:20 UTC (permalink / raw)
To: James Ogletree
Cc: James Ogletree, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Lee Jones, Fred Treven, Ben Bright, linux-input@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <20231018175726.3879955-4-james.ogletree@opensource.cirrus.com>
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 <james.ogletree@cirrus.com>
>
> 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 <james.ogletree@cirrus.com>
> ---
> 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: patches@opensource.cirrus.com
> 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 <simont@opensource.cirrus.com>
> 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. <james.ogletree@cirrus.com>");
> +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. <james.ogletree@cirrus.com>");
> +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. <james.ogletree@cirrus.com>");
> +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
^ permalink raw reply
* Re: [PATCH v4 4/4] Input: cs40l50 - Add support for the CS40L50 haptic driver
From: Jeff LaBundy @ 2023-10-25 3:03 UTC (permalink / raw)
To: James Ogletree
Cc: James Ogletree, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Lee Jones, Fred Treven, Ben Bright, linux-input, devicetree,
linux-kernel
In-Reply-To: <20231018175726.3879955-5-james.ogletree@opensource.cirrus.com>
Hi James,
On Wed, Oct 18, 2023 at 05:57:25PM +0000, James Ogletree wrote:
> From: James Ogletree <james.ogletree@cirrus.com>
>
> Introduce support for Cirrus Logic Device CS40L50: a
> haptic driver with waveform memory, integrated DSP,
> and closed-loop algorithms.
>
> The input driver provides the interface for control of
> haptic effects through the device.
>
> Signed-off-by: James Ogletree <james.ogletree@cirrus.com>
> ---
> MAINTAINERS | 1 +
> drivers/input/misc/Kconfig | 10 +
> drivers/input/misc/Makefile | 1 +
> drivers/input/misc/cs40l50-vibra.c | 353 +++++++++++++++++++++++++++++
> 4 files changed, 365 insertions(+)
> create mode 100644 drivers/input/misc/cs40l50-vibra.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 08e1e9695d49..24a00d8e5c1c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4971,6 +4971,7 @@ L: patches@opensource.cirrus.com
> S: Supported
> F: Documentation/devicetree/bindings/input/cirrus,cs40l50.yaml
> F: drivers/input/misc/cirrus*
> +F: drivers/input/misc/cs40l*
> F: drivers/mfd/cs40l*
> F: include/linux/input/cirrus*
> F: include/linux/mfd/cs40l*
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index 9f088900f863..938090648126 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -129,6 +129,16 @@ config INPUT_BMA150
> To compile this driver as a module, choose M here: the
> module will be called bma150.
>
> +config INPUT_CS40L50_VIBRA
> + tristate "CS40L50 Haptic Driver support"
> + depends on MFD_CS40L50_CORE
> + help
> + Say Y here to enable support for Cirrus Logic's CS40L50
> + haptic driver.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called cs40l50-vibra.
> +
> 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 6abefc41037b..6b653ed2124f 100644
> --- a/drivers/input/misc/Makefile
> +++ b/drivers/input/misc/Makefile
> @@ -27,6 +27,7 @@ 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_VIBRA) += cs40l50-vibra.o cirrus_haptics.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-vibra.c b/drivers/input/misc/cs40l50-vibra.c
> new file mode 100644
> index 000000000000..3b3e4cb10de0
> --- /dev/null
> +++ b/drivers/input/misc/cs40l50-vibra.c
> @@ -0,0 +1,353 @@
> +// 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/firmware/cirrus/wmfw.h>
> +#include <linux/mfd/cs40l50.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +
> +static int cs40l50_add(struct input_dev *dev,
> + struct ff_effect *effect,
> + struct ff_effect *old)
> +{
> + struct cs40l50_private *cs40l50 = input_get_drvdata(dev);
> + u32 len = effect->u.periodic.custom_len;
> +
> + if (effect->type != FF_PERIODIC || effect->u.periodic.waveform != FF_CUSTOM) {
> + dev_err(cs40l50->dev, "Type (%#X) or waveform (%#X) unsupported\n",
> + effect->type, effect->u.periodic.waveform);
> + return -EINVAL;
> + }
> +
> + memcpy(&cs40l50->haptics.add_effect, effect, sizeof(struct ff_effect));
> +
> + cs40l50->haptics.add_effect.u.periodic.custom_data = kcalloc(len,
> + sizeof(s16),
> + GFP_KERNEL);
> + if (!cs40l50->haptics.add_effect.u.periodic.custom_data)
> + return -ENOMEM;
> +
> + if (copy_from_user(cs40l50->haptics.add_effect.u.periodic.custom_data,
> + effect->u.periodic.custom_data, sizeof(s16) * len)) {
> + cs40l50->haptics.add_error = -EFAULT;
> + goto out_free;
> + }
> +
> + queue_work(cs40l50->haptics.vibe_wq, &cs40l50->haptics.add_work);
> + flush_work(&cs40l50->haptics.add_work);
> +
> +out_free:
> + kfree(cs40l50->haptics.add_effect.u.periodic.custom_data);
> + cs40l50->haptics.add_effect.u.periodic.custom_data = NULL;
> +
> + return cs40l50->haptics.add_error;
> +}
> +
> +static int cs40l50_playback(struct input_dev *dev, int effect_id, int val)
> +{
> + struct cs40l50_private *cs40l50 = input_get_drvdata(dev);
> +
> + if (val > 0) {
> + cs40l50->haptics.start_effect = &dev->ff->effects[effect_id];
> + queue_work(cs40l50->haptics.vibe_wq,
> + &cs40l50->haptics.vibe_start_work);
> + } else {
> + queue_work(cs40l50->haptics.vibe_wq,
> + &cs40l50->haptics.vibe_stop_work);
> + }
> +
> + return 0;
> +}
> +
> +static int cs40l50_erase(struct input_dev *dev, int effect_id)
> +{
> + struct cs40l50_private *cs40l50 = input_get_drvdata(dev);
> +
> + cs40l50->haptics.erase_effect = &dev->ff->effects[effect_id];
> +
> + queue_work(cs40l50->haptics.vibe_wq, &cs40l50->haptics.erase_work);
> + flush_work(&cs40l50->haptics.erase_work);
> +
> + return cs40l50->haptics.erase_error;
> +}
> +
> +static const struct reg_sequence cs40l50_int_vamp_seq[] = {
> + { CS40L50_BST_LPMODE_SEL, CS40L50_DCM_LOW_POWER },
> + { CS40L50_BLOCK_ENABLES2, CS40L50_OVERTEMP_WARN },
> +};
> +
> +static const struct reg_sequence cs40l50_irq_mask_seq[] = {
> + { CS40L50_IRQ1_MASK_2, CS40L50_IRQ_MASK_2_OVERRIDE },
> + { CS40L50_IRQ1_MASK_20, CS40L50_IRQ_MASK_20_OVERRIDE },
> +};
> +
> +static int cs40l50_hw_init(struct cs40l50_private *cs40l50)
> +{
> + int error;
> +
> + error = regmap_multi_reg_write(cs40l50->regmap,
> + cs40l50_int_vamp_seq,
> + ARRAY_SIZE(cs40l50_int_vamp_seq));
> + if (error)
> + return error;
> +
> + error = cs_hap_pseq_multi_write(&cs40l50->haptics,
> + cs40l50_int_vamp_seq,
> + ARRAY_SIZE(cs40l50_int_vamp_seq),
> + false, PSEQ_OP_WRITE_FULL);
> + if (error)
> + return error;
> +
> + error = regmap_multi_reg_write(cs40l50->regmap, cs40l50_irq_mask_seq,
> + ARRAY_SIZE(cs40l50_irq_mask_seq));
> + if (error)
> + return error;
> +
> + return cs_hap_pseq_multi_write(&cs40l50->haptics, cs40l50_irq_mask_seq,
> + ARRAY_SIZE(cs40l50_irq_mask_seq), false,
> + PSEQ_OP_WRITE_FULL);
> +}
> +
> +static const struct cs_dsp_client_ops cs40l50_cs_dsp_client_ops;
> +
> +static const struct cs_dsp_region cs40l50_dsp_regions[] = {
> + {
> + .type = WMFW_HALO_PM_PACKED,
> + .base = CS40L50_DSP1_PMEM_0
> + },
> + {
> + .type = WMFW_HALO_XM_PACKED,
> + .base = CS40L50_DSP1_XMEM_PACKED_0
> + },
> + {
> + .type = WMFW_HALO_YM_PACKED,
> + .base = CS40L50_DSP1_YMEM_PACKED_0
> + },
> + {
> + .type = WMFW_ADSP2_XM,
> + .base = CS40L50_DSP1_XMEM_UNPACKED24_0
> + },
> + {
> + .type = WMFW_ADSP2_YM,
> + .base = CS40L50_DSP1_YMEM_UNPACKED24_0
> + },
> +};
> +
> +static int cs40l50_cs_dsp_init(struct cs40l50_private *cs40l50)
> +{
> + cs40l50->dsp.num = 1;
> + cs40l50->dsp.type = WMFW_HALO;
> + cs40l50->dsp.dev = cs40l50->dev;
> + cs40l50->dsp.regmap = cs40l50->regmap;
> + cs40l50->dsp.base = CS40L50_CORE_BASE;
> + cs40l50->dsp.base_sysinfo = CS40L50_SYS_INFO_ID;
> + cs40l50->dsp.mem = cs40l50_dsp_regions;
> + cs40l50->dsp.num_mems = ARRAY_SIZE(cs40l50_dsp_regions);
> + cs40l50->dsp.no_core_startstop = true;
> + cs40l50->dsp.client_ops = &cs40l50_cs_dsp_client_ops;
> +
> + return cs_dsp_halo_init(&cs40l50->dsp);
> +}
> +
> +static struct cs_hap_bank cs40l50_banks[] = {
> + {
> + .bank = WVFRM_BANK_RAM,
> + .base_index = CS40L50_RAM_BANK_INDEX_START,
> + .max_index = CS40L50_RAM_BANK_INDEX_START,
> + },
> + {
> + .bank = WVFRM_BANK_ROM,
> + .base_index = CS40L50_ROM_BANK_INDEX_START,
> + .max_index = CS40L50_ROM_BANK_INDEX_END,
> + },
> + {
> + .bank = WVFRM_BANK_OWT,
> + .base_index = CS40L50_RTH_INDEX_START,
> + .max_index = CS40L50_RTH_INDEX_END,
> + },
> +};
These structs describe the DSP, and hence the silicon; they are not
specific to the input/FF device. Presumably the DSP could run algorithms
that support only the I2S streaming case as well (e.g. A2H); therefore,
these seem more appropriately placed in the MFD.
> +
> +static int cs40l50_cs_hap_init(struct cs40l50_private *cs40l50)
> +{
> + cs40l50->haptics.regmap = cs40l50->regmap;
> + cs40l50->haptics.dev = cs40l50->dev;
> + cs40l50->haptics.banks = cs40l50_banks;
> + cs40l50->haptics.dsp.gpio_base_reg = CS40L50_GPIO_BASE;
> + cs40l50->haptics.dsp.owt_base_reg = CS40L50_OWT_BASE;
> + cs40l50->haptics.dsp.owt_offset_reg = CS40L50_OWT_NEXT;
> + cs40l50->haptics.dsp.owt_size_reg = CS40L50_OWT_SIZE;
> + cs40l50->haptics.dsp.mailbox_reg = CS40L50_DSP_MBOX;
> + cs40l50->haptics.dsp.pseq_reg = CS40L50_POWER_ON_SEQ;
> + cs40l50->haptics.dsp.push_owt_cmd = CS40L50_OWT_PUSH;
> + cs40l50->haptics.dsp.delete_owt_cmd = CS40L50_OWT_DELETE;
> + cs40l50->haptics.dsp.stop_cmd = CS40L50_STOP_PLAYBACK;
> + cs40l50->haptics.dsp.pseq_size = CS40L50_PSEQ_SIZE;
> + cs40l50->haptics.runtime_pm = true;
> +
> + return cs_hap_init(&cs40l50->haptics);
> +}
> +
> +static void cs40l50_upload_wt(const struct firmware *bin, void *context)
> +{
> + struct cs40l50_private *cs40l50 = context;
> + u32 nwaves;
> +
> + mutex_lock(&cs40l50->lock);
> +
> + if (cs40l50->wmfw) {
> + if (regmap_write(cs40l50->regmap, CS40L50_CCM_CORE_CONTROL,
> + CS40L50_CLOCK_DISABLE))
> + goto err_mutex;
> +
> + if (regmap_write(cs40l50->regmap, CS40L50_RAM_INIT,
> + CS40L50_RAM_INIT_FLAG))
> + goto err_mutex;
> +
> + if (regmap_write(cs40l50->regmap, CS40L50_PWRMGT_CTL,
> + CS40L50_MEM_RDY_HW))
> + goto err_mutex;
> + }
> +
> + cs_dsp_power_up(&cs40l50->dsp, cs40l50->wmfw, "cs40l50.wmfw",
> + bin, "cs40l50.bin", "cs40l50");
> +
> + if (cs40l50->wmfw) {
> + if (regmap_write(cs40l50->regmap, CS40L50_CCM_CORE_CONTROL,
> + CS40L50_CLOCK_ENABLE))
> + goto err_mutex;
> + }
> +
> + if (regmap_read(cs40l50->regmap, CS40L50_NUM_OF_WAVES, &nwaves))
> + goto err_mutex;
> +
> + cs40l50->haptics.banks[WVFRM_BANK_RAM].max_index += (nwaves - 1);
> +
> +err_mutex:
> + mutex_unlock(&cs40l50->lock);
> + release_firmware(bin);
> + release_firmware(cs40l50->wmfw);
> +}
> +
> +static void cs40l50_upload_patch(const struct firmware *wmfw, void *context)
> +{
> + struct cs40l50_private *cs40l50 = context;
> +
> + cs40l50->wmfw = wmfw;
> +
> + if (request_firmware_nowait(THIS_MODULE, FW_ACTION_UEVENT, CS40L50_WT,
> + cs40l50->dev, GFP_KERNEL,
> + cs40l50, cs40l50_upload_wt))
> + release_firmware(cs40l50->wmfw);
> +}
> +
> +static int cs40l50_input_init(struct cs40l50_private *cs40l50)
> +{
> + int error;
> +
> + cs40l50->input = devm_input_allocate_device(cs40l50->dev);
> + if (!cs40l50->input)
> + return -ENOMEM;
> +
> + cs40l50->input->id.product = cs40l50->devid & 0xFFFF;
> + cs40l50->input->id.version = cs40l50->revid;
> + cs40l50->input->name = "cs40l50_vibra";
> +
> + input_set_drvdata(cs40l50->input, cs40l50);
> + input_set_capability(cs40l50->input, EV_FF, FF_PERIODIC);
> + input_set_capability(cs40l50->input, EV_FF, FF_CUSTOM);
> +
> + error = input_ff_create(cs40l50->input, FF_MAX_EFFECTS);
> + if (error)
> + return error;
> +
> + cs40l50->input->ff->upload = cs40l50_add;
> + cs40l50->input->ff->playback = cs40l50_playback;
> + cs40l50->input->ff->erase = cs40l50_erase;
> +
> + INIT_LIST_HEAD(&cs40l50->haptics.effect_head);
> +
> + error = input_register_device(cs40l50->input);
> + if (error)
> + goto err_free_dev;
> +
> + return cs40l50_cs_hap_init(cs40l50);
> +
> +err_free_dev:
> + input_free_device(cs40l50->input);
> + return error;
> +}
> +static int cs40l50_vibra_probe(struct platform_device *pdev)
> +{
> + struct cs40l50_private *cs40l50 = dev_get_drvdata(pdev->dev.parent);
> + int error;
> +
> + error = cs40l50_input_init(cs40l50);
> + if (error)
> + return error;
> +
> + error = cs40l50_hw_init(cs40l50);
> + if (error)
> + goto err_input;
> +
> + error = cs40l50_cs_dsp_init(cs40l50);
> + if (error)
> + goto err_input;
> +
> + error = request_firmware_nowait(THIS_MODULE, FW_ACTION_UEVENT,
> + CS40L50_FW, cs40l50->dev,
> + GFP_KERNEL, cs40l50,
> + cs40l50_upload_patch);
> + if (error)
> + goto err_dsp;
> +
> + return 0;
> +
> +err_dsp:
> + cs_dsp_remove(&cs40l50->dsp);
> +err_input:
> + input_unregister_device(cs40l50->input);
> + cs_hap_remove(&cs40l50->haptics);
> +
> + return error;
> +}
> +
> +static int cs40l50_vibra_remove(struct platform_device *pdev)
> +{
> + struct cs40l50_private *cs40l50 = dev_get_drvdata(pdev->dev.parent);
> +
> + input_unregister_device(cs40l50->input);
> + cs_hap_remove(&cs40l50->haptics);
> +
> + if (cs40l50->dsp.booted)
> + cs_dsp_power_down(&cs40l50->dsp);
> + if (&cs40l50->dsp)
> + cs_dsp_remove(&cs40l50->dsp);
> +
> + return 0;
> +}
> +
> +static const struct platform_device_id cs40l50_id_vibra[] = {
> + {"cs40l50-vibra", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(platform, cs40l50_id_vibra);
> +
> +static struct platform_driver cs40l50_vibra_driver = {
> + .probe = cs40l50_vibra_probe,
> + .remove = cs40l50_vibra_remove,
> + .id_table = cs40l50_id_vibra,
> + .driver = {
> + .name = "cs40l50-vibra",
> + },
> +};
> +module_platform_driver(cs40l50_vibra_driver);
> +
> +MODULE_DESCRIPTION("CS40L50 Advanced Haptic Driver");
> +MODULE_AUTHOR("James Ogletree, Cirrus Logic Inc. <james.ogletree@cirrus.com>");
> +MODULE_LICENSE("GPL");
> --
> 2.25.1
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v4 2/4] Input: cs40l50 - Add cirrus haptics base support
From: Jeff LaBundy @ 2023-10-25 2:04 UTC (permalink / raw)
To: James Ogletree
Cc: James Ogletree, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Lee Jones, Fred Treven, Ben Bright, linux-input, devicetree,
linux-kernel
In-Reply-To: <20231018175726.3879955-3-james.ogletree@opensource.cirrus.com>
Hi James,
Excellent work as always!
On Wed, Oct 18, 2023 at 05:57:23PM +0000, James Ogletree wrote:
> From: James Ogletree <james.ogletree@cirrus.com>
>
> Introduce the cirrus haptics library which factors out
> common haptics operations used by Cirrus Logic Input
> drivers.
>
> Signed-off-by: James Ogletree <james.ogletree@cirrus.com>
> ---
> MAINTAINERS | 2 +
> drivers/input/misc/cirrus_haptics.c | 586 +++++++++++++++++++++++++++
> include/linux/input/cirrus_haptics.h | 121 ++++++
> 3 files changed, 709 insertions(+)
> create mode 100644 drivers/input/misc/cirrus_haptics.c
> create mode 100644 include/linux/input/cirrus_haptics.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 28f0ca9324b3..57daf77bf550 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4970,6 +4970,8 @@ M: Ben Bright <ben.bright@cirrus.com>
> L: patches@opensource.cirrus.com
> S: Supported
> F: Documentation/devicetree/bindings/input/cirrus,cs40l50.yaml
> +F: drivers/input/misc/cirrus*
> +F: include/linux/input/cirrus*
>
> CIRRUS LOGIC DSP FIRMWARE DRIVER
> M: Simon Trimmer <simont@opensource.cirrus.com>
> diff --git a/drivers/input/misc/cirrus_haptics.c b/drivers/input/misc/cirrus_haptics.c
> new file mode 100644
> index 000000000000..7e539cd45167
> --- /dev/null
> +++ b/drivers/input/misc/cirrus_haptics.c
> @@ -0,0 +1,586 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Helper functions for dealing with wavetable
> + * formats and DSP interfaces used by Cirrus
> + * haptic drivers.
> + *
> + * Copyright 2023 Cirrus Logic, Inc.
> + */
> +
> +#include <linux/firmware/cirrus/cs_dsp.h>
> +#include <linux/input.h>
> +#include <linux/input/cirrus_haptics.h>
> +#include <linux/pm_runtime.h>
> +
> +static int cs_hap_pseq_init(struct cs_hap *haptics)
> +{
> + struct cs_hap_pseq_op *op;
> + int error, i, num_words;
> + u8 operation;
> + u32 *words;
> +
> + if (!haptics->dsp.pseq_size || !haptics->dsp.pseq_reg)
> + return 0;
> +
> + INIT_LIST_HEAD(&haptics->pseq_head);
Anything that allocates or initializes an element that is normally held
in a driver's private data, like a list head or mutex, belongs in probe()
in my opinion. It's less of an issue here, but for more complex cases
where we may set something up in probe() and tear it down in remove(),
the driver is easier to maintain if helper functions such as cs_hap_pseq_init()
only manipulate or organize the data, rather than absorb additional work.
> +
> + words = kcalloc(haptics->dsp.pseq_size, sizeof(u32), GFP_KERNEL);
> + if (!words)
> + return -ENOMEM;
> +
> + error = regmap_bulk_read(haptics->regmap, haptics->dsp.pseq_reg,
> + words, haptics->dsp.pseq_size);
> + if (error)
> + goto err_free;
> +
> + for (i = 0; i < haptics->dsp.pseq_size; i += num_words) {
> + operation = FIELD_GET(PSEQ_OP_MASK, words[i]);
> + switch (operation) {
> + case PSEQ_OP_END:
> + case PSEQ_OP_WRITE_UNLOCK:
> + num_words = PSEQ_OP_END_WORDS;
> + break;
> + case PSEQ_OP_WRITE_ADDR8:
> + case PSEQ_OP_WRITE_H16:
> + case PSEQ_OP_WRITE_L16:
> + num_words = PSEQ_OP_WRITE_X16_WORDS;
> + break;
> + case PSEQ_OP_WRITE_FULL:
> + num_words = PSEQ_OP_WRITE_FULL_WORDS;
> + break;
> + default:
> + error = -EINVAL;
> + dev_err(haptics->dev, "Unsupported op: %u\n", operation);
> + goto err_free;
> + }
> +
> + op = devm_kzalloc(haptics->dev, sizeof(*op), GFP_KERNEL);
> + if (!op) {
> + error = -ENOMEM;
> + goto err_free;
> + }
> +
> + op->size = num_words * sizeof(u32);
> + memcpy(op->words, &words[i], op->size);
> + op->offset = i * sizeof(u32);
> + op->operation = operation;
> + list_add(&op->list, &haptics->pseq_head);
> +
> + if (operation == PSEQ_OP_END)
> + break;
> + }
> +
> + if (operation != PSEQ_OP_END)
> + error = -ENOENT;
> +
> +err_free:
> + kfree(words);
> +
> + return error;
> +}
My first thought as I reviewed this patch was that this and the lot
of pseq-related functions are not necessarily related to haptics, but
rather CS40L50 register access and housekeeping in general.
I seem to recall on L25 and friends that the the power-on sequencer,
i.e. PSEQ, is more or less a "tape recorder" of sorts in DSP memory
that can play back a series of address/data pairs when the device
comes out of hibernation, and any registers written during runtime
must also be mirrored to the PSEQ for "playback" later. Is that still
the case here?
Assuming so, these functions seem like they belong in the MFD, not
an input-specific library, because they will presumably be used by
the codec driver as well, since that driver will write registers to
set BCLK/LRCK ratio, etc.
Therefore, I think it makes more sense for these functions to move to
the MFD, which can then export them for use by the input/FF and codec
children.
This leaves cirrus_haptics.* with only a few functions related to
starting and stopping work, which seem specific enough to just live
in cs40l50-vibra.c. Presumably many of those could be re-used by
the L30 down the road, but in that case I think we'd be looking to
actually re-use the L50 driver and simply add a compatible string
for L30.
I recall L30 has some overhead that L50 does not, which may make
it hairy for cs40l50-vibra.c to support both. But in that case,
you could always fork a cs40l30-vibra.c with its own compatible
string, then have the L50 MFD selectively load the correct child
based on device ID. To summarize, we should have:
* drivers/mfd/cs40l50-core.c: MFD cell definition, device discovery,
IRQ handling, exported PSEQ functions, etc.
* sound/soc/codecs/cs40l50.c: codec driver, uses PSEQ library from
the MFD.
* drivers/input/misc/cs40l50-vibra.c: input/FF driver, start/stop
work, also uses PSEQ library from the MFD.
And down the road, depending on complexity, maybe we also have:
* drivers/input/misc/cs40l30-vibra.c: another input/FF driver that
includes other functionality that didn't really fit in cs40l50-vibra.c;
also uses PSEQ library from, and is loaded by, the original L50 MFD.
If this driver duplicates small bits of cs40l50-vibra.c, it's not the
end of the world.
All of these files would #include include/linux/mfd/cs40l50.h. And
finally, cirrus_haptics.* simply go away. Same idea, just slightly
more scalable, and closer to common design patterns.
> +
> +static int cs_hap_pseq_find_end(struct cs_hap *haptics,
> + struct cs_hap_pseq_op **op_end)
> +{
> + u8 operation = PSEQ_OP_WRITE_FULL;
> + struct cs_hap_pseq_op *op;
> +
> + list_for_each_entry(op, &haptics->pseq_head, list) {
> + operation = op->operation;
> + if (operation == PSEQ_OP_END)
> + break;
> + }
> +
> + if (operation != PSEQ_OP_END) {
> + dev_err(haptics->dev, "Missing PSEQ list terminator\n");
> + return -ENOENT;
> + }
> +
> + *op_end = op;
> +
> + return 0;
> +}
> +
> +static struct cs_hap_pseq_op *cs_hap_pseq_find_op(struct cs_hap_pseq_op *match_op,
> + struct list_head *pseq_head)
> +{
> + struct cs_hap_pseq_op *op;
> +
> + list_for_each_entry(op, pseq_head, list) {
> + if (op->operation == PSEQ_OP_END)
> + break;
Nit: a line break here makes this easier to read IMO.
> + if (op->operation != match_op->operation ||
> + op->words[0] != match_op->words[0])
> + continue;
And here.
> + switch (op->operation) {
> + case PSEQ_OP_WRITE_FULL:
> + if (FIELD_GET(GENMASK(23, 8), op->words[1]) ==
> + FIELD_GET(GENMASK(23, 8), match_op->words[1]))
> + return op;
> + break;
> + case PSEQ_OP_WRITE_H16:
> + case PSEQ_OP_WRITE_L16:
> + if (FIELD_GET(GENMASK(23, 16), op->words[1]) ==
> + FIELD_GET(GENMASK(23, 16), match_op->words[1]))
> + return op;
> + break;
> + default:
> + break;
> + }
> + }
> +
> + return NULL;
> +}
> +
> +int cs_hap_pseq_write(struct cs_hap *haptics, u32 addr,
> + u32 data, bool update, u8 op_code)
> +{
> + struct cs_hap_pseq_op *op, *op_end, *op_new;
> + struct cs_dsp_chunk ch;
> + u32 pseq_bytes;
> + int error;
> +
> + op_new = devm_kzalloc(haptics->dev, sizeof(*op_new), GFP_KERNEL);
> + if (!op_new)
> + return -ENOMEM;
> +
> + op_new->operation = op_code;
> +
> + ch = cs_dsp_chunk((void *) op_new->words,
> + PSEQ_OP_WRITE_FULL_WORDS * sizeof(u32));
> + cs_dsp_chunk_write(&ch, 8, op_code);
> + switch (op_code) {
> + case PSEQ_OP_WRITE_FULL:
> + cs_dsp_chunk_write(&ch, 32, addr);
> + cs_dsp_chunk_write(&ch, 32, data);
> + break;
> + case PSEQ_OP_WRITE_L16:
> + case PSEQ_OP_WRITE_H16:
> + cs_dsp_chunk_write(&ch, 24, addr);
> + cs_dsp_chunk_write(&ch, 16, data);
> + break;
> + default:
> + error = -EINVAL;
> + goto op_new_free;
> + }
> +
> + op_new->size = cs_dsp_chunk_bytes(&ch);
> +
> + if (update) {
> + op = cs_hap_pseq_find_op(op_new, &haptics->pseq_head);
> + if (!op) {
> + error = -EINVAL;
> + goto op_new_free;
> + }
> + }
It seems we are relying on the developer to remember if he or she has
already written 'addr' using a previous call to cs_hap_pseq_write(),
then set the update flag accordingly; is that accurate?
If so, that is a high risk for bugs to be introduced as the driver is
maintained. Can we not search for an existing address/data entry upon
any call to cs_hap_pseq_write() using cs_hap_pseq_find_op(), and add
or replace a new address/data entry automatically?
> +
> + error = cs_hap_pseq_find_end(haptics, &op_end);
> + if (error)
> + goto op_new_free;
> +
> + pseq_bytes = haptics->dsp.pseq_size * sizeof(u32);
> +
> + if (pseq_bytes - op_end->offset < op_new->size) {
> + error = -ENOMEM;
> + goto op_new_free;
> + }
> +
> + if (update) {
> + op_new->offset = op->offset;
> + } else {
> + op_new->offset = op_end->offset;
> + op_end->offset += op_new->size;
> + }
> +
> + error = regmap_raw_write(haptics->regmap, haptics->dsp.pseq_reg +
> + op_new->offset, op_new->words, op_new->size);
> + if (error)
> + goto op_new_free;
> +
> + if (update) {
> + list_replace(&op->list, &op_new->list);
> + } else {
> + error = regmap_raw_write(haptics->regmap, haptics->dsp.pseq_reg +
> + op_end->offset, op_end->words,
> + op_end->size);
> + if (error)
> + goto op_new_free;
> +
> + list_add(&op_new->list, &haptics->pseq_head);
> + }
> +
> + return 0;
> +
> +op_new_free:
> + devm_kfree(haptics->dev, op_new);
> +
> + return error;
> +}
> +EXPORT_SYMBOL_GPL(cs_hap_pseq_write);
> +
> +int cs_hap_pseq_multi_write(struct cs_hap *haptics,
> + const struct reg_sequence *reg_seq,
> + int num_regs, bool update, u8 op_code)
> +{
> + int error, i;
> +
> + for (i = 0; i < num_regs; i++) {
> + error = cs_hap_pseq_write(haptics, reg_seq[i].reg,
> + reg_seq[i].def, update, op_code);
> + if (error)
> + return error;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(cs_hap_pseq_multi_write);
> +
> +static struct cs_hap_effect *cs_hap_find_effect(int id,
> + struct list_head *effect_head)
> +{
> + struct cs_hap_effect *effect;
> +
> + list_for_each_entry(effect, effect_head, list)
> + if (effect->id == id)
> + return effect;
> +
> + return NULL;
> +}
> +
> +static int cs_hap_effect_bank_set(struct cs_hap *haptics,
> + struct cs_hap_effect *effect,
> + struct ff_periodic_effect add_effect)
> +{
> + s16 bank = add_effect.custom_data[0] & 0xffffu;
> + unsigned int len = add_effect.custom_len;
> +
> + if (bank >= WVFRM_BANK_NUM) {
> + dev_err(haptics->dev, "Invalid waveform bank: %d\n", bank);
> + return -EINVAL;
> + }
> +
> + effect->bank = len > CUSTOM_DATA_SIZE ? WVFRM_BANK_OWT : bank;
> +
> + return 0;
> +}
> +
> +static int cs_hap_effect_mapping_set(struct cs_hap *haptics, u16 button,
> + struct cs_hap_effect *effect)
> +{
> + u32 gpio_num, gpio_edge;
> +
> + if (button) {
> + gpio_num = FIELD_GET(BTN_NUM_MASK, button);
> + gpio_edge = FIELD_GET(BTN_EDGE_MASK, button);
> + effect->mapping = haptics->dsp.gpio_base_reg +
> + (gpio_num * 8) - gpio_edge;
> +
> + return regmap_write(haptics->regmap, effect->mapping, button);
> + }
> +
> + effect->mapping = GPIO_MAPPING_INVALID;
> +
> + return 0;
> +}
> +
> +static int cs_hap_effect_index_set(struct cs_hap *haptics,
> + struct cs_hap_effect *effect,
> + struct ff_periodic_effect add_effect)
> +{
> + struct cs_hap_effect *owt_effect;
> + u32 base_index, max_index;
> +
> + base_index = haptics->banks[effect->bank].base_index;
> + max_index = haptics->banks[effect->bank].max_index;
> +
> + effect->index = base_index;
> +
> + switch (effect->bank) {
> + case WVFRM_BANK_OWT:
> + list_for_each_entry(owt_effect, &haptics->effect_head, list)
> + if (owt_effect->bank == WVFRM_BANK_OWT)
> + effect->index++;
> + break;
> + case WVFRM_BANK_ROM:
> + case WVFRM_BANK_RAM:
> + effect->index += add_effect.custom_data[1] & 0xffffu;
> + break;
> + default:
> + dev_err(haptics->dev, "Bank not supported: %d\n", effect->bank);
> + return -EINVAL;
> + }
> +
> + if (effect->index > max_index || effect->index < base_index) {
> + dev_err(haptics->dev, "Index out of bounds: %u\n", effect->index);
> + return -ENOSPC;
> + }
> +
> + return 0;
> +}
> +
> +static int cs_hap_upload_pwle(struct cs_hap *haptics,
> + struct cs_hap_effect *effect,
> + struct ff_periodic_effect add_effect)
> +{
> + u32 len, wt_offset, wt_size_words;
> + struct cs_hap_pwle_header header;
> + u8 *out_data;
> + int error;
> +
> + error = regmap_read(haptics->regmap, haptics->dsp.owt_offset_reg,
> + &wt_offset);
> + if (error)
> + return error;
> +
> + error = regmap_read(haptics->regmap, haptics->dsp.owt_size_reg,
> + &wt_size_words);
> + if (error)
> + return error;
> +
> + len = 2 * add_effect.custom_len;
> +
> + if ((wt_size_words * sizeof(u32)) < OWT_HEADER_SIZE + len)
> + return -ENOSPC;
> +
> + out_data = kzalloc(OWT_HEADER_SIZE + len, GFP_KERNEL);
> + if (!out_data)
> + return -ENOMEM;
> +
> + header.type = add_effect.custom_data[0] == PCM_ID ? OWT_TYPE_PCM :
> + OWT_TYPE_PWLE;
> + header.offset = OWT_HEADER_SIZE / sizeof(u32);
> + header.data_words = len / sizeof(u32);
> +
> + memcpy(out_data, &header, sizeof(header));
> + memcpy(out_data + OWT_HEADER_SIZE, add_effect.custom_data, len);
> +
> + error = regmap_bulk_write(haptics->regmap, haptics->dsp.owt_base_reg +
> + (wt_offset * sizeof(u32)), out_data,
> + OWT_HEADER_SIZE + len);
> + if (error)
> + goto err_free;
> +
> + error = regmap_write(haptics->regmap, haptics->dsp.mailbox_reg,
> + haptics->dsp.push_owt_cmd);
> +
> +err_free:
> + kfree(out_data);
> +
> + return error;
> +}
> +
> +static void cs_hap_add_worker(struct work_struct *work)
> +{
> + struct cs_hap *haptics = container_of(work, struct cs_hap,
> + add_work);
> + struct ff_effect add_effect = haptics->add_effect;
> + bool is_new = false;
> + struct cs_hap_effect *effect;
> + int error;
> +
> + if (haptics->runtime_pm) {
> + error = pm_runtime_resume_and_get(haptics->dev);
> + if (error < 0) {
> + haptics->add_error = error;
> + return;
> + }
> + }
> +
> + mutex_lock(&haptics->lock);
> +
> + effect = cs_hap_find_effect(add_effect.id, &haptics->effect_head);
> + if (!effect) {
> + effect = kzalloc(sizeof(*effect), GFP_KERNEL);
> + if (!effect) {
> + error = -ENOMEM;
> + goto err_mutex;
> + }
> + effect->id = add_effect.id;
> + is_new = true;
> + }
> +
> + error = cs_hap_effect_bank_set(haptics, effect, add_effect.u.periodic);
> + if (error)
> + goto err_free;
> +
> + error = cs_hap_effect_index_set(haptics, effect, add_effect.u.periodic);
> + if (error)
> + goto err_free;
> +
> + error = cs_hap_effect_mapping_set(haptics, add_effect.trigger.button,
> + effect);
> + if (error)
> + goto err_free;
> +
> + if (effect->bank == WVFRM_BANK_OWT)
> + error = cs_hap_upload_pwle(haptics, effect,
> + add_effect.u.periodic);
> +
> +err_free:
> + if (is_new) {
> + if (error)
> + kfree(effect);
> + else
> + list_add(&effect->list, &haptics->effect_head);
> + }
> +
> +err_mutex:
> + mutex_unlock(&haptics->lock);
> +
> + if (haptics->runtime_pm) {
> + pm_runtime_mark_last_busy(haptics->dev);
> + pm_runtime_put_autosuspend(haptics->dev);
> + }
> +
> + haptics->add_error = error;
> +}
> +
> +static void cs_hap_erase_worker(struct work_struct *work)
> +{
> + struct cs_hap *haptics = container_of(work, struct cs_hap,
> + erase_work);
> + int error = 0;
> + struct cs_hap_effect *owt_effect, *erase_effect;
> +
> + if (haptics->runtime_pm) {
> + error = pm_runtime_resume_and_get(haptics->dev);
> + if (error < 0) {
> + haptics->erase_error = error;
> + return;
> + }
> + }
> +
> + mutex_lock(&haptics->lock);
> +
> + erase_effect = cs_hap_find_effect(haptics->erase_effect->id,
> + &haptics->effect_head);
> + if (!erase_effect) {
> + dev_err(haptics->dev, "Effect to erase does not exist\n");
> + error = -EINVAL;
> + goto err_mutex;
> + }
> +
> + if (erase_effect->mapping != GPIO_MAPPING_INVALID) {
> + error = regmap_write(haptics->regmap, erase_effect->mapping,
> + GPIO_DISABLE);
> + if (error)
> + goto err_mutex;
> + }
> +
> + if (erase_effect->bank == WVFRM_BANK_OWT) {
> + error = regmap_write(haptics->regmap, haptics->dsp.mailbox_reg,
> + haptics->dsp.delete_owt_cmd |
> + erase_effect->index);
> + if (error)
> + goto err_mutex;
> +
> + list_for_each_entry(owt_effect, &haptics->effect_head, list)
> + if (owt_effect->bank == WVFRM_BANK_OWT &&
> + owt_effect->index > erase_effect->index)
> + owt_effect->index--;
> + }
> +
> + list_del(&erase_effect->list);
> + kfree(erase_effect);
> +
> +err_mutex:
> + mutex_unlock(&haptics->lock);
> +
> + if (haptics->runtime_pm) {
> + pm_runtime_mark_last_busy(haptics->dev);
> + pm_runtime_put_autosuspend(haptics->dev);
> + }
> +
> + haptics->erase_error = error;
> +}
> +
> +static void cs_hap_vibe_start_worker(struct work_struct *work)
> +{
> + struct cs_hap *haptics = container_of(work, struct cs_hap,
> + vibe_start_work);
> + struct cs_hap_effect *effect;
> + int error;
> +
> + if (haptics->runtime_pm) {
> + error = pm_runtime_resume_and_get(haptics->dev);
> + if (error < 0) {
> + haptics->start_error = error;
> + return;
> + }
> + }
> +
> + mutex_lock(&haptics->lock);
> +
> + effect = cs_hap_find_effect(haptics->start_effect->id,
> + &haptics->effect_head);
> + if (effect) {
> + error = regmap_write(haptics->regmap, haptics->dsp.mailbox_reg,
> + effect->index);
> + } else {
> + dev_err(haptics->dev, "Effect to start does not exist\n");
> + error = -EINVAL;
> + }
> +
> + mutex_unlock(&haptics->lock);
> +
> + if (haptics->runtime_pm) {
> + pm_runtime_mark_last_busy(haptics->dev);
> + pm_runtime_put_autosuspend(haptics->dev);
> + }
> +
> + haptics->start_error = error;
> +}
> +
> +static void cs_hap_vibe_stop_worker(struct work_struct *work)
> +{
> + struct cs_hap *haptics = container_of(work, struct cs_hap,
> + vibe_stop_work);
> + int error;
> +
> + if (haptics->runtime_pm) {
> + error = pm_runtime_resume_and_get(haptics->dev);
> + if (error < 0) {
> + haptics->stop_error = error;
> + return;
> + }
> + }
> +
> + mutex_lock(&haptics->lock);
> + error = regmap_write(haptics->regmap, haptics->dsp.mailbox_reg,
> + haptics->dsp.stop_cmd);
> + mutex_unlock(&haptics->lock);
> +
> + if (haptics->runtime_pm) {
> + pm_runtime_mark_last_busy(haptics->dev);
> + pm_runtime_put_autosuspend(haptics->dev);
> + }
> +
> + haptics->stop_error = error;
This seems like another argument for not separating the input/FF child
from the meat of the driver; it just seems messy to pass around error
codes within a driver's private data like this.
That being said, where are start_error and stop_error used? I didn't
see them in the input/FF child. We should only introduce code that has
at least one user.
> +}
> +
> +int cs_hap_init(struct cs_hap *haptics)
> +{
> + haptics->vibe_wq = alloc_ordered_workqueue("vibe_wq", 0);
> + if (!haptics->vibe_wq)
> + return -ENOMEM;
> +
> + mutex_init(&haptics->lock);
> +
> + INIT_WORK(&haptics->vibe_start_work, cs_hap_vibe_start_worker);
> + INIT_WORK(&haptics->vibe_stop_work, cs_hap_vibe_stop_worker);
> + INIT_WORK(&haptics->erase_work, cs_hap_erase_worker);
> + INIT_WORK(&haptics->add_work, cs_hap_add_worker);
> +
> + return cs_hap_pseq_init(haptics);
> +}
> +EXPORT_SYMBOL_GPL(cs_hap_init);
> +
> +void cs_hap_remove(struct cs_hap *haptics)
> +{
> + flush_workqueue(haptics->vibe_wq);
> + destroy_workqueue(haptics->vibe_wq);
> +}
> +EXPORT_SYMBOL_GPL(cs_hap_remove);
> +
> +MODULE_DESCRIPTION("Cirrus Logic Haptics Support");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/input/cirrus_haptics.h b/include/linux/input/cirrus_haptics.h
> new file mode 100644
> index 000000000000..42f6afed7944
> --- /dev/null
> +++ b/include/linux/input/cirrus_haptics.h
> @@ -0,0 +1,121 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Helper functions for dealing with wavetable
> + * formats and DSP interfaces used by Cirrus
> + * haptic drivers.
> + *
> + * Copyright 2023 Cirrus Logic, Inc.
> + */
> +
> +#ifndef __CIRRUS_HAPTICS_H
> +#define __CIRRUS_HAPTICS_H
> +
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +
> +/* Power-on write sequencer */
> +#define PSEQ_OP_MASK GENMASK(23, 16)
> +#define PSEQ_OP_SHIFT 16
> +#define PSEQ_OP_WRITE_FULL_WORDS 3
> +#define PSEQ_OP_WRITE_X16_WORDS 2
> +#define PSEQ_OP_END_WORDS 1
> +#define PSEQ_OP_WRITE_FULL 0x00
> +#define PSEQ_OP_WRITE_ADDR8 0x02
> +#define PSEQ_OP_WRITE_L16 0x04
> +#define PSEQ_OP_WRITE_H16 0x05
> +#define PSEQ_OP_WRITE_UNLOCK 0xFD
> +#define PSEQ_OP_END 0xFF
> +
> +/* Open wavetable */
> +#define OWT_HEADER_SIZE 12
> +#define OWT_TYPE_PCM 8
> +#define OWT_TYPE_PWLE 12
> +#define PCM_ID 0x0
> +#define CUSTOM_DATA_SIZE 2
> +
> +/* GPIO */
> +#define BTN_NUM_MASK GENMASK(14, 12)
> +#define BTN_EDGE_MASK BIT(15)
> +#define GPIO_MAPPING_INVALID 0
> +#define GPIO_DISABLE 0x1FF
> +
> +enum cs_hap_bank_type {
> + WVFRM_BANK_RAM,
> + WVFRM_BANK_ROM,
> + WVFRM_BANK_OWT,
> + WVFRM_BANK_NUM,
> +};
> +
> +struct cs_hap_pseq_op {
> + struct list_head list;
> + u32 words[3];
> + u16 offset;
> + u8 operation;
> + u8 size;
> +};
> +
> +struct cs_hap_effect {
> + enum cs_hap_bank_type bank;
> + struct list_head list;
> + u32 mapping;
> + u32 index;
> + int id;
> +};
> +
> +struct cs_hap_pwle_header {
> + u32 type;
> + u32 data_words;
> + u32 offset;
> +} __packed;
> +
> +struct cs_hap_bank {
> + enum cs_hap_bank_type bank;
> + u32 base_index;
> + u32 max_index;
> +};
> +
> +struct cs_hap_dsp {
> + u32 gpio_base_reg;
> + u32 owt_offset_reg;
> + u32 owt_size_reg;
> + u32 owt_base_reg;
> + u32 mailbox_reg;
> + u32 pseq_reg;
> + u32 push_owt_cmd;
> + u32 delete_owt_cmd;
> + u32 stop_cmd;
> + u32 pseq_size;
> +};
> +
> +struct cs_hap {
> + struct regmap *regmap;
> + struct mutex lock;
> + struct device *dev;
> + struct list_head pseq_head;
> + struct cs_hap_bank *banks;
> + struct cs_hap_dsp dsp;
> + struct workqueue_struct *vibe_wq;
> + struct work_struct vibe_start_work;
> + struct work_struct vibe_stop_work;
> + struct work_struct erase_work;
> + struct work_struct add_work;
> + struct ff_effect *start_effect;
> + struct ff_effect *erase_effect;
> + struct ff_effect add_effect;
> + struct list_head effect_head;
> + int erase_error;
> + int start_error;
> + int stop_error;
> + int add_error;
> + bool runtime_pm;
> +};
> +
> +int cs_hap_pseq_write(struct cs_hap *haptics, u32 addr,
> + u32 data, bool update, u8 op_code);
> +int cs_hap_pseq_multi_write(struct cs_hap *haptics,
> + const struct reg_sequence *reg_seq,
> + int num_regs, bool update, u8 op_code);
> +int cs_hap_init(struct cs_hap *haptics);
> +void cs_hap_remove(struct cs_hap *haptics);
> +
> +#endif
> --
> 2.25.1
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* [PATCH v2] Input: cyttsp5 - improve error handling and remove regmap
From: James Hilliard @ 2023-10-25 1:39 UTC (permalink / raw)
To: linux-input; +Cc: James Hilliard, Linus Walleij, Dmitry Torokhov, linux-kernel
The vendor cyttsp5 driver does not use regmap for i2c support, it
would appear this is due to regmap not providing sufficient levels
of control to handle various error conditions that may be present
under some configuration/firmware variants.
To improve reliability lets refactor the cyttsp5 i2c interface to
function more like the vendor driver and implement some of the error
handling retry/recovery techniques present there.
As part of this rather than assuming the device is in bootloader mode
we should first check that the device is in bootloader and only
attempt to launch the app if it actually is in the bootloader.
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
Changes v1 -> v2:
- remove unused reg variable
---
drivers/input/touchscreen/cyttsp5.c | 257 ++++++++++++++++++----------
1 file changed, 168 insertions(+), 89 deletions(-)
diff --git a/drivers/input/touchscreen/cyttsp5.c b/drivers/input/touchscreen/cyttsp5.c
index db5a885ecd72..c17c740220f6 100644
--- a/drivers/input/touchscreen/cyttsp5.c
+++ b/drivers/input/touchscreen/cyttsp5.c
@@ -20,15 +20,16 @@
#include <linux/i2c.h>
#include <linux/mod_devicetable.h>
#include <linux/module.h>
-#include <linux/regmap.h>
#include <asm/unaligned.h>
#define CYTTSP5_NAME "cyttsp5"
#define CY_I2C_DATA_SIZE (2 * 256)
#define HID_VERSION 0x0100
#define CY_MAX_INPUT 512
+#define CY_PIP_1P7_EMPTY_BUF 0xFF00
#define CYTTSP5_PREALLOCATED_CMD_BUFFER 32
#define CY_BITS_PER_BTN 1
+#define CY_CORE_STARTUP_RETRY_COUNT 10
#define CY_NUM_BTN_EVENT_ID GENMASK(CY_BITS_PER_BTN - 1, 0)
#define MAX_AREA 255
@@ -67,6 +68,7 @@
#define HID_BTN_REPORT_ID 0x3
#define HID_APP_RESPONSE_REPORT_ID 0x1F
#define HID_APP_OUTPUT_REPORT_ID 0x2F
+#define HID_BL_REPORT_ID 0xFF
#define HID_BL_RESPONSE_REPORT_ID 0x30
#define HID_BL_OUTPUT_REPORT_ID 0x40
#define HID_RESPONSE_REPORT_ID 0xF0
@@ -205,7 +207,6 @@ struct cyttsp5 {
struct input_dev *input;
char phys[NAME_MAX];
int num_prv_rec;
- struct regmap *regmap;
struct touchscreen_properties prop;
struct regulator *vdd;
};
@@ -218,49 +219,65 @@ struct cyttsp5 {
*/
static int cyttsp5_read(struct cyttsp5 *ts, u8 *buf, u32 max)
{
- int error;
+ struct i2c_client *client = to_i2c_client(ts->dev);
+ struct i2c_msg msgs[2];
+ u8 msg_count = 1;
+ int rc;
u32 size;
- u8 temp[2];
- /* Read the frame to retrieve the size */
- error = regmap_bulk_read(ts->regmap, HID_INPUT_REG, temp, sizeof(temp));
- if (error)
- return error;
+ if (!buf)
+ return -EINVAL;
- size = get_unaligned_le16(temp);
- if (!size || size == 2)
+ msgs[0].addr = client->addr;
+ msgs[0].flags = (client->flags & I2C_M_TEN) | I2C_M_RD;
+ msgs[0].len = 2;
+ msgs[0].buf = buf;
+ rc = i2c_transfer(client->adapter, msgs, msg_count);
+ if (rc < 0 || rc != msg_count)
+ return (rc < 0) ? rc : -EIO;
+
+ size = get_unaligned_le16(&buf[0]);
+ /*
+ * Before PIP 1.7, empty buffer is 0x0002
+ * From PIP 1.7, empty buffer is 0xFFXX
+ */
+ if (!size || size == 2 || size >= CY_PIP_1P7_EMPTY_BUF)
return 0;
if (size > max)
return -EINVAL;
- /* Get the real value */
- return regmap_bulk_read(ts->regmap, HID_INPUT_REG, buf, size);
+ rc = i2c_master_recv(client, buf, size);
+
+ return (rc < 0) ? rc : rc != (int)size ? -EIO : 0;
}
static int cyttsp5_write(struct cyttsp5 *ts, unsigned int reg, u8 *data,
size_t size)
{
- u8 cmd[HID_OUTPUT_MAX_CMD_SIZE];
+ u8 cmd[HID_OUTPUT_MAX_CMD_SIZE + 2];
+ struct i2c_client *client = to_i2c_client(ts->dev);
+ struct i2c_msg msgs[2];
+ u8 msg_count = 1;
+ int rc;
- if (size + 1 > HID_OUTPUT_MAX_CMD_SIZE)
+ if (size > HID_OUTPUT_MAX_CMD_SIZE + 2)
return -E2BIG;
- /* High bytes of register address needed as first byte of cmd */
- cmd[0] = (reg >> 8) & 0xFF;
-
/* Copy the rest of the data */
if (data)
- memcpy(&cmd[1], data, size);
+ memcpy(&cmd[0], data, size);
- /*
- * The hardware wants to receive a frame with the address register
- * contained in the first two bytes. As the regmap_write function
- * add the register adresse in the frame, we use the low byte as
- * first frame byte for the address register and the first
- * data byte is the high register + left of the cmd to send
- */
- return regmap_bulk_write(ts->regmap, reg & 0xFF, cmd, size + 1);
+ msgs[0].addr = client->addr;
+ msgs[0].flags = client->flags & I2C_M_TEN;
+ msgs[0].len = size;
+ msgs[0].buf = cmd;
+ rc = i2c_transfer(client->adapter, msgs, msg_count);
+
+ if (rc < 0 || rc != msg_count)
+ return (rc < 0) ? rc : -EIO;
+
+ return 0;
}
static void cyttsp5_get_touch_axis(int *axis, int size, int max, u8 *xy_data,
@@ -535,22 +552,29 @@ static int cyttsp5_get_sysinfo_regs(struct cyttsp5 *ts)
scd->len_x = get_unaligned_le16(&scd_dev->len_x);
scd->len_y = get_unaligned_le16(&scd_dev->len_y);
+ if (scd_dev->max_num_of_tch_per_refresh_cycle == 0)
+ return -EINVAL;
+
return 0;
}
static int cyttsp5_hid_output_get_sysinfo(struct cyttsp5 *ts)
{
int rc;
- u8 cmd[HID_OUTPUT_GET_SYSINFO_SIZE];
+ u8 cmd[HID_OUTPUT_GET_SYSINFO_SIZE + 2];
+
+ /* Set Output register */
+ memcpy(&cmd[0], &ts->hid_desc.output_register,
+ sizeof(ts->hid_desc.output_register));
/* HI bytes of Output register address */
- put_unaligned_le16(HID_OUTPUT_GET_SYSINFO_SIZE, cmd);
- cmd[2] = HID_APP_OUTPUT_REPORT_ID;
- cmd[3] = 0x0; /* Reserved */
- cmd[4] = HID_OUTPUT_GET_SYSINFO;
+ put_unaligned_le16(HID_OUTPUT_GET_SYSINFO_SIZE, &cmd[2]);
+ cmd[4] = HID_APP_OUTPUT_REPORT_ID;
+ cmd[5] = 0x0; /* Reserved */
+ cmd[6] = HID_OUTPUT_GET_SYSINFO;
rc = cyttsp5_write(ts, HID_OUTPUT_REG, cmd,
- HID_OUTPUT_GET_SYSINFO_SIZE);
+ HID_OUTPUT_GET_SYSINFO_SIZE + 2);
if (rc) {
dev_err(ts->dev, "Failed to write command %d", rc);
return rc;
@@ -559,7 +583,7 @@ static int cyttsp5_hid_output_get_sysinfo(struct cyttsp5 *ts)
rc = wait_for_completion_interruptible_timeout(&ts->cmd_done,
msecs_to_jiffies(CY_HID_OUTPUT_GET_SYSINFO_TIMEOUT_MS));
if (rc <= 0) {
- dev_err(ts->dev, "HID output cmd execution timed out\n");
+ dev_err(ts->dev, "HID output get sysinfo cmd execution timed out\n");
rc = -ETIMEDOUT;
return rc;
}
@@ -610,21 +634,25 @@ static int cyttsp5_power_control(struct cyttsp5 *ts, bool on)
static int cyttsp5_hid_output_bl_launch_app(struct cyttsp5 *ts)
{
int rc;
- u8 cmd[HID_OUTPUT_BL_LAUNCH_APP_SIZE];
+ u8 cmd[HID_OUTPUT_BL_LAUNCH_APP_SIZE + 2];
u16 crc;
- put_unaligned_le16(HID_OUTPUT_BL_LAUNCH_APP_SIZE, cmd);
- cmd[2] = HID_BL_OUTPUT_REPORT_ID;
- cmd[3] = 0x0; /* Reserved */
- cmd[4] = HID_OUTPUT_BL_SOP;
- cmd[5] = HID_OUTPUT_BL_LAUNCH_APP;
- put_unaligned_le16(0x00, &cmd[6]);
- crc = crc_itu_t(0xFFFF, &cmd[4], 4);
- put_unaligned_le16(crc, &cmd[8]);
- cmd[10] = HID_OUTPUT_BL_EOP;
+ /* Set Output register */
+ memcpy(&cmd[0], &ts->hid_desc.output_register,
+ sizeof(ts->hid_desc.output_register));
+
+ put_unaligned_le16(HID_OUTPUT_BL_LAUNCH_APP_SIZE, &cmd[2]);
+ cmd[4] = HID_BL_OUTPUT_REPORT_ID;
+ cmd[5] = 0x0; /* Reserved */
+ cmd[6] = HID_OUTPUT_BL_SOP;
+ cmd[7] = HID_OUTPUT_BL_LAUNCH_APP;
+ put_unaligned_le16(0x00, &cmd[8]);
+ crc = crc_itu_t(0xFFFF, &cmd[6], 4);
+ put_unaligned_le16(crc, &cmd[10]);
+ cmd[12] = HID_OUTPUT_BL_EOP;
rc = cyttsp5_write(ts, HID_OUTPUT_REG, cmd,
- HID_OUTPUT_BL_LAUNCH_APP_SIZE);
+ HID_OUTPUT_BL_LAUNCH_APP_SIZE + 2);
if (rc) {
dev_err(ts->dev, "Failed to write command %d", rc);
return rc;
@@ -633,7 +661,7 @@ static int cyttsp5_hid_output_bl_launch_app(struct cyttsp5 *ts)
rc = wait_for_completion_interruptible_timeout(&ts->cmd_done,
msecs_to_jiffies(CY_HID_OUTPUT_TIMEOUT_MS));
if (rc <= 0) {
- dev_err(ts->dev, "HID output cmd execution timed out\n");
+ dev_err(ts->dev, "HID output bl launch app cmd execution timed out\n");
rc = -ETIMEDOUT;
return rc;
}
@@ -651,9 +679,12 @@ static int cyttsp5_get_hid_descriptor(struct cyttsp5 *ts,
struct cyttsp5_hid_desc *desc)
{
struct device *dev = ts->dev;
+ u8 cmd[2] = { 0 };
int rc;
- rc = cyttsp5_write(ts, HID_DESC_REG, NULL, 0);
+ put_unaligned_le16(HID_DESC_REG, cmd);
+
+ rc = cyttsp5_write(ts, HID_DESC_REG, cmd, 2);
if (rc) {
dev_err(dev, "Failed to get HID descriptor, rc=%d\n", rc);
return rc;
@@ -708,7 +739,8 @@ static irqreturn_t cyttsp5_handle_irq(int irq, void *handle)
if (size == 0) {
/* reset */
report_id = 0;
- size = 2;
+ } else if (size == 2 || size >= CY_PIP_1P7_EMPTY_BUF) {
+ return IRQ_HANDLED;
} else {
report_id = ts->input_buf[2];
}
@@ -733,19 +765,38 @@ static irqreturn_t cyttsp5_handle_irq(int irq, void *handle)
return IRQ_HANDLED;
}
+static int cyttsp5_deassert_read(struct cyttsp5 *ts, u8 *buf, int size)
+{
+ struct i2c_client *client = to_i2c_client(ts->dev);
+ int rc;
+
+ if (!buf || !size || size > CY_I2C_DATA_SIZE)
+ return -EINVAL;
+
+ rc = i2c_master_recv(client, buf, size);
+
+ return (rc < 0) ? rc : rc != size ? -EIO : 0;
+}
+
static int cyttsp5_deassert_int(struct cyttsp5 *ts)
{
u16 size;
- u8 buf[2];
+ u8 retry = 3;
int error;
- error = regmap_bulk_read(ts->regmap, HID_INPUT_REG, buf, sizeof(buf));
- if (error < 0)
- return error;
+ do {
+ error = cyttsp5_deassert_read(ts, ts->input_buf, 2);
+ if (error < 0)
+ return error;
- size = get_unaligned_le16(&buf[0]);
- if (size == 2 || size == 0)
- return 0;
+ size = get_unaligned_le16(&ts->input_buf[0]);
+ if (size == 2 || size == 0 || size >= CY_PIP_1P7_EMPTY_BUF)
+ return 0;
+
+ error = cyttsp5_deassert_read(ts, ts->input_buf, size);
+ if (error < 0)
+ return error;
+ } while (retry--);
return -EINVAL;
}
@@ -774,39 +825,65 @@ static int cyttsp5_fill_all_touch(struct cyttsp5 *ts)
static int cyttsp5_startup(struct cyttsp5 *ts)
{
+ int retry = CY_CORE_STARTUP_RETRY_COUNT;
int error;
+reset:
error = cyttsp5_deassert_int(ts);
if (error) {
dev_err(ts->dev, "Error on deassert int r=%d\n", error);
- return -ENODEV;
+ }
+
+ error = cyttsp5_get_hid_descriptor(ts, &ts->hid_desc);
+ if (error < 0) {
+ dev_err(ts->dev, "Error on getting HID descriptor r=%d\n", error);
+ if (retry--)
+ goto reset;
+ return error;
}
/*
* Launch the application as the device starts in bootloader mode
* because of a power-on-reset
*/
- error = cyttsp5_hid_output_bl_launch_app(ts);
- if (error < 0) {
- dev_err(ts->dev, "Error on launch app r=%d\n", error);
- return error;
- }
+ if (ts->hid_desc.packet_id == HID_BL_REPORT_ID) {
+ error = cyttsp5_hid_output_bl_launch_app(ts);
+ if (error < 0) {
+ dev_err(ts->dev, "Error on launch app r=%d\n", error);
+ if (retry--)
+ goto reset;
+ return error;
+ }
- error = cyttsp5_get_hid_descriptor(ts, &ts->hid_desc);
- if (error < 0) {
- dev_err(ts->dev, "Error on getting HID descriptor r=%d\n", error);
- return error;
+ error = cyttsp5_get_hid_descriptor(ts, &ts->hid_desc);
+ if (error < 0) {
+ dev_err(ts->dev, "Error on getting HID descriptor r=%d\n", error);
+ if (retry--)
+ goto reset;
+ return error;
+ }
+
+ if (ts->hid_desc.packet_id == HID_BL_REPORT_ID) {
+ dev_err(ts->dev, "Error on launch app still in bootloader\n");
+ if (retry--)
+ goto reset;
+ return -EPROTO;
+ }
}
error = cyttsp5_fill_all_touch(ts);
if (error < 0) {
dev_err(ts->dev, "Error on report descriptor r=%d\n", error);
+ if (retry--)
+ goto reset;
return error;
}
error = cyttsp5_hid_output_get_sysinfo(ts);
if (error) {
dev_err(ts->dev, "Error on getting sysinfo r=%d\n", error);
+ if (retry--)
+ goto reset;
return error;
}
@@ -820,8 +897,7 @@ static void cyttsp5_cleanup(void *data)
regulator_disable(ts->vdd);
}
-static int cyttsp5_probe(struct device *dev, struct regmap *regmap, int irq,
- const char *name)
+static struct cyttsp5 *cyttsp5_probe(struct device *dev, int irq, const char *name)
{
struct cyttsp5 *ts;
struct cyttsp5_sysinfo *si;
@@ -829,10 +905,9 @@ static int cyttsp5_probe(struct device *dev, struct regmap *regmap, int irq,
ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
if (!ts)
- return -ENOMEM;
+ return ERR_PTR(-ENOMEM);
/* Initialize device info */
- ts->regmap = regmap;
ts->dev = dev;
si = &ts->sysinfo;
dev_set_drvdata(dev, ts);
@@ -843,21 +918,21 @@ static int cyttsp5_probe(struct device *dev, struct regmap *regmap, int irq,
ts->vdd = devm_regulator_get(dev, "vdd");
if (IS_ERR(ts->vdd)) {
error = PTR_ERR(ts->vdd);
- return error;
+ return ERR_PTR(error);
}
error = devm_add_action_or_reset(dev, cyttsp5_cleanup, ts);
if (error)
- return error;
+ return ERR_PTR(error);
error = regulator_enable(ts->vdd);
if (error)
- return error;
+ return ERR_PTR(error);
ts->input = devm_input_allocate_device(dev);
if (!ts->input) {
dev_err(dev, "Error, failed to allocate input device\n");
- return -ENODEV;
+ return ERR_PTR(-ENODEV);
}
ts->input->name = "cyttsp5";
@@ -870,7 +945,7 @@ static int cyttsp5_probe(struct device *dev, struct regmap *regmap, int irq,
if (IS_ERR(ts->reset_gpio)) {
error = PTR_ERR(ts->reset_gpio);
dev_err(dev, "Failed to request reset gpio, error %d\n", error);
- return error;
+ return ERR_PTR(error);
}
gpiod_set_value_cansleep(ts->reset_gpio, 0);
@@ -878,22 +953,22 @@ static int cyttsp5_probe(struct device *dev, struct regmap *regmap, int irq,
msleep(20);
error = devm_request_threaded_irq(dev, irq, NULL, cyttsp5_handle_irq,
- IRQF_ONESHOT, name, ts);
+ IRQF_TRIGGER_LOW | IRQF_ONESHOT, name, ts);
if (error) {
dev_err(dev, "unable to request IRQ\n");
- return error;
+ return ERR_PTR(error);
}
error = cyttsp5_startup(ts);
if (error) {
dev_err(ts->dev, "Fail initial startup r=%d\n", error);
- return error;
+ return ERR_PTR(error);
}
error = cyttsp5_parse_dt_key_code(dev);
if (error < 0) {
dev_err(ts->dev, "Error while parsing dts %d\n", error);
- return error;
+ return ERR_PTR(error);
}
touchscreen_parse_properties(ts->input, true, &ts->prop);
@@ -902,25 +977,29 @@ static int cyttsp5_probe(struct device *dev, struct regmap *regmap, int irq,
for (i = 0; i < si->num_btns; i++)
__set_bit(si->key_code[i], ts->input->keybit);
- return cyttsp5_setup_input_device(dev);
+ error = cyttsp5_setup_input_device(dev);
+ if (error < 0)
+ return ERR_PTR(error);
+
+ return ts;
}
static int cyttsp5_i2c_probe(struct i2c_client *client)
{
- struct regmap *regmap;
- static const struct regmap_config config = {
- .reg_bits = 8,
- .val_bits = 8,
- };
+ struct cyttsp5 *ts;
- regmap = devm_regmap_init_i2c(client, &config);
- if (IS_ERR(regmap)) {
- dev_err(&client->dev, "regmap allocation failed: %ld\n",
- PTR_ERR(regmap));
- return PTR_ERR(regmap);
+ if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+ dev_err(&client->dev, "I2C functionality not Supported\n");
+ return -EIO;
}
- return cyttsp5_probe(&client->dev, regmap, client->irq, client->name);
+ ts = cyttsp5_probe(&client->dev, client->irq, client->name);
+
+ if (IS_ERR(ts))
+ return PTR_ERR(ts);
+
+ i2c_set_clientdata(client, ts);
+ return 0;
}
static const struct of_device_id cyttsp5_of_match[] = {
--
2.34.1
^ permalink raw reply related
* [PATCH v3 2/2] Input: add Himax HX852x(ES) touchscreen driver
From: Stephan Gerhold @ 2023-10-24 18:35 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Henrik Rydberg,
linux-input, devicetree, linux-kernel, Jeff LaBundy,
Christophe JAILLET, Jonathan Albrieux, Stephan Gerhold
In-Reply-To: <20231024-hx852x-v3-0-a1890d3a81e9@gerhold.net>
From: Jonathan Albrieux <jonathan.albrieux@gmail.com>
Add a simple driver for the Himax HX852x(ES) touch panel controller,
with support for multi-touch and capacitive touch keys.
The driver is somewhat based on sample code from Himax. However, that
code was so extremely confusing that we spent a significant amount of
time just trying to understand the packet format and register commands.
In this driver they are described with clean structs and defines rather
than lots of magic numbers and offset calculations.
Signed-off-by: Jonathan Albrieux <jonathan.albrieux@gmail.com>
Co-developed-by: Stephan Gerhold <stephan@gerhold.net>
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
MAINTAINERS | 7 +
drivers/input/touchscreen/Kconfig | 10 +
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/himax_hx852x.c | 500 +++++++++++++++++++++++++++++++
4 files changed, 518 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 4cc6bf79fdd8..c0004b25b524 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9264,6 +9264,13 @@ S: Maintained
F: Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml
F: drivers/input/touchscreen/himax_hx83112b.c
+HIMAX HX852X TOUCHSCREEN DRIVER
+M: Stephan Gerhold <stephan@gerhold.net>
+L: linux-input@vger.kernel.org
+S: Maintained
+F: Documentation/devicetree/bindings/input/touchscreen/himax,hx852es.yaml
+F: drivers/input/touchscreen/himax_hx852x.c
+
HIPPI
M: Jes Sorensen <jes@trained-monkey.org>
L: linux-hippi@sunsite.dk
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index e3e2324547b9..8e5667ae5dab 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -427,6 +427,16 @@ config TOUCHSCREEN_HIDEEP
To compile this driver as a module, choose M here : the
module will be called hideep_ts.
+config TOUCHSCREEN_HIMAX_HX852X
+ tristate "Himax HX852x(ES) touchscreen"
+ depends on I2C
+ help
+ Say Y here if you have a Himax HX852x(ES) touchscreen.
+ If unsure, say N.
+
+ To compile this driver as a module, choose M here: the module
+ will be called himax_hx852x.
+
config TOUCHSCREEN_HYCON_HY46XX
tristate "Hycon hy46xx touchscreen support"
depends on I2C
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 62bd24f3ac8e..f42a87faa86c 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -48,6 +48,7 @@ obj-$(CONFIG_TOUCHSCREEN_EXC3000) += exc3000.o
obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
+obj-$(CONFIG_TOUCHSCREEN_HIMAX_HX852X) += himax_hx852x.o
obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
obj-$(CONFIG_TOUCHSCREEN_ILITEK) += ilitek_ts_i2c.o
diff --git a/drivers/input/touchscreen/himax_hx852x.c b/drivers/input/touchscreen/himax_hx852x.c
new file mode 100644
index 000000000000..6aa39f02829d
--- /dev/null
+++ b/drivers/input/touchscreen/himax_hx852x.c
@@ -0,0 +1,500 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Himax HX852x(ES) Touchscreen Driver
+ * Copyright (c) 2020-2023 Stephan Gerhold <stephan@gerhold.net>
+ * Copyright (c) 2020 Jonathan Albrieux <jonathan.albrieux@gmail.com>
+ *
+ * Based on the Himax Android Driver Sample Code Ver 0.3 for HMX852xES chipset:
+ * Copyright (c) 2014 Himax Corporation.
+ */
+
+#include <asm/unaligned.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/input.h>
+#include <linux/input/mt.h>
+#include <linux/input/touchscreen.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regulator/consumer.h>
+
+#define HX852X_COORD_SIZE(fingers) ((fingers) * sizeof(struct hx852x_coord))
+#define HX852X_WIDTH_SIZE(fingers) ALIGN(fingers, 4)
+#define HX852X_BUF_SIZE(fingers) (HX852X_COORD_SIZE(fingers) + \
+ HX852X_WIDTH_SIZE(fingers) + \
+ sizeof(struct hx852x_touch_info))
+
+#define HX852X_MAX_FINGERS 12
+#define HX852X_MAX_KEY_COUNT 4
+#define HX852X_MAX_BUF_SIZE HX852X_BUF_SIZE(HX852X_MAX_FINGERS)
+
+#define HX852X_TS_SLEEP_IN 0x80
+#define HX852X_TS_SLEEP_OUT 0x81
+#define HX852X_TS_SENSE_OFF 0x82
+#define HX852X_TS_SENSE_ON 0x83
+#define HX852X_READ_ONE_EVENT 0x85
+#define HX852X_READ_ALL_EVENTS 0x86
+#define HX852X_READ_LATEST_EVENT 0x87
+#define HX852X_CLEAR_EVENT_STACK 0x88
+
+#define HX852X_REG_SRAM_SWITCH 0x8c
+#define HX852X_REG_SRAM_ADDR 0x8b
+#define HX852X_REG_FLASH_RPLACE 0x5a
+
+#define HX852X_SRAM_SWITCH_TEST_MODE 0x14
+#define HX852X_SRAM_ADDR_CONFIG 0x7000
+
+struct hx852x {
+ struct i2c_client *client;
+ struct input_dev *input_dev;
+ struct touchscreen_properties props;
+ struct gpio_desc *reset_gpiod;
+ struct regulator_bulk_data supplies[2];
+ unsigned int max_fingers;
+ unsigned int keycount;
+ unsigned int keycodes[HX852X_MAX_KEY_COUNT];
+};
+
+struct hx852x_config {
+ u8 rx_num;
+ u8 tx_num;
+ u8 max_pt;
+ u8 padding1[3];
+ __be16 x_res;
+ __be16 y_res;
+ u8 padding2[2];
+} __packed __aligned(4);
+
+struct hx852x_coord {
+ __be16 x;
+ __be16 y;
+} __packed __aligned(4);
+
+struct hx852x_touch_info {
+ u8 finger_num;
+ __le16 finger_pressed;
+ u8 padding;
+} __packed __aligned(4);
+
+static int hx852x_i2c_read(struct hx852x *hx, u8 cmd, void *data, u16 len)
+{
+ struct i2c_client *client = hx->client;
+ int ret;
+
+ struct i2c_msg msg[] = {
+ {
+ .addr = client->addr,
+ .flags = 0,
+ .len = 1,
+ .buf = &cmd,
+ },
+ {
+ .addr = client->addr,
+ .flags = I2C_M_RD,
+ .len = len,
+ .buf = data,
+ },
+ };
+
+ ret = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
+ if (ret != ARRAY_SIZE(msg)) {
+ dev_err(&client->dev, "failed to read %#x: %d\n", cmd, ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int hx852x_power_on(struct hx852x *hx)
+{
+ struct device *dev = &hx->client->dev;
+ int error;
+
+ error = regulator_bulk_enable(ARRAY_SIZE(hx->supplies), hx->supplies);
+ if (error) {
+ dev_err(dev, "failed to enable regulators: %d\n", error);
+ return error;
+ }
+
+ gpiod_set_value_cansleep(hx->reset_gpiod, 1);
+ msleep(20);
+ gpiod_set_value_cansleep(hx->reset_gpiod, 0);
+ msleep(20);
+
+ return 0;
+}
+
+static int hx852x_start(struct hx852x *hx)
+{
+ struct device *dev = &hx->client->dev;
+ int error;
+
+ error = i2c_smbus_write_byte(hx->client, HX852X_TS_SLEEP_OUT);
+ if (error) {
+ dev_err(dev, "failed to send TS_SLEEP_OUT: %d\n", error);
+ return error;
+ }
+ msleep(30);
+
+ error = i2c_smbus_write_byte(hx->client, HX852X_TS_SENSE_ON);
+ if (error) {
+ dev_err(dev, "failed to send TS_SENSE_ON: %d\n", error);
+ return error;
+ }
+ msleep(20);
+
+ return 0;
+}
+
+static int hx852x_stop(struct hx852x *hx)
+{
+ struct device *dev = &hx->client->dev;
+ int error;
+
+ error = i2c_smbus_write_byte(hx->client, HX852X_TS_SENSE_OFF);
+ if (error) {
+ dev_err(dev, "failed to send TS_SENSE_OFF: %d\n", error);
+ return error;
+ }
+ msleep(20);
+
+ error = i2c_smbus_write_byte(hx->client, HX852X_TS_SLEEP_IN);
+ if (error) {
+ dev_err(dev, "failed to send TS_SLEEP_IN: %d\n", error);
+ return error;
+ }
+ msleep(30);
+
+ return 0;
+}
+
+static int hx852x_power_off(struct hx852x *hx)
+{
+ struct device *dev = &hx->client->dev;
+ int error;
+
+ error = regulator_bulk_disable(ARRAY_SIZE(hx->supplies), hx->supplies);
+ if (error) {
+ dev_err(dev, "failed to disable regulators: %d\n", error);
+ return error;
+ }
+
+ return 0;
+}
+
+static int hx852x_read_config(struct hx852x *hx)
+{
+ struct device *dev = &hx->client->dev;
+ struct hx852x_config conf;
+ int x_res, y_res;
+ int error;
+
+ error = hx852x_power_on(hx);
+ if (error)
+ return error;
+
+ /* Sensing must be turned on briefly to load the config */
+ error = hx852x_start(hx);
+ if (error)
+ goto err_power_off;
+
+ error = hx852x_stop(hx);
+ if (error)
+ goto err_power_off;
+
+ error = i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH,
+ HX852X_SRAM_SWITCH_TEST_MODE);
+ if (error)
+ goto err_power_off;
+
+ error = i2c_smbus_write_word_data(hx->client, HX852X_REG_SRAM_ADDR,
+ HX852X_SRAM_ADDR_CONFIG);
+ if (error)
+ goto err_test_mode;
+
+ error = hx852x_i2c_read(hx, HX852X_REG_FLASH_RPLACE, &conf, sizeof(conf));
+ if (error)
+ goto err_test_mode;
+
+ x_res = be16_to_cpu(conf.x_res);
+ y_res = be16_to_cpu(conf.y_res);
+ hx->max_fingers = (conf.max_pt & 0xf0) >> 4;
+ dev_dbg(dev, "x res: %u, y res: %u, max fingers: %u\n",
+ x_res, y_res, hx->max_fingers);
+
+ if (hx->max_fingers > HX852X_MAX_FINGERS) {
+ dev_err(dev, "max supported fingers: %u, found: %u\n",
+ HX852X_MAX_FINGERS, hx->max_fingers);
+ error = -EINVAL;
+ goto err_test_mode;
+ }
+
+ if (x_res && y_res) {
+ input_set_abs_params(hx->input_dev, ABS_MT_POSITION_X, 0, x_res - 1, 0, 0);
+ input_set_abs_params(hx->input_dev, ABS_MT_POSITION_Y, 0, y_res - 1, 0, 0);
+ }
+
+err_test_mode:
+ error = i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH, 0) ? : error;
+err_power_off:
+ return hx852x_power_off(hx) ? : error;
+}
+
+static int hx852x_handle_events(struct hx852x *hx)
+{
+ /*
+ * The event packets have variable size, depending on the amount of
+ * supported fingers (hx->max_fingers). They are laid out as follows:
+ * - struct hx852x_coord[hx->max_fingers]: Coordinates for each finger
+ * - u8[ALIGN(hx->max_fingers, 4)]: Touch width for each finger
+ * with padding for 32-bit alignment
+ * - struct hx852x_touch_info
+ *
+ * Load everything into a 32-bit aligned buffer so the coordinates
+ * can be assigned directly, without using get_unaligned_*().
+ */
+ u8 buf[HX852X_MAX_BUF_SIZE] __aligned(4);
+ struct hx852x_coord *coord = (struct hx852x_coord *)buf;
+ u8 *width = &buf[HX852X_COORD_SIZE(hx->max_fingers)];
+ struct hx852x_touch_info *info = (struct hx852x_touch_info *)
+ &width[HX852X_WIDTH_SIZE(hx->max_fingers)];
+ unsigned long finger_pressed, key_pressed;
+ unsigned int i, x, y, w;
+ int error;
+
+ error = hx852x_i2c_read(hx, HX852X_READ_ALL_EVENTS, buf,
+ HX852X_BUF_SIZE(hx->max_fingers));
+ if (error)
+ return error;
+
+ finger_pressed = get_unaligned_le16(&info->finger_pressed);
+ key_pressed = finger_pressed >> HX852X_MAX_FINGERS;
+
+ /* All bits are set when no touch is detected */
+ if (info->finger_num == 0xff || !(info->finger_num & 0x0f))
+ finger_pressed = 0;
+ if (key_pressed == 0xf)
+ key_pressed = 0;
+
+ for_each_set_bit(i, &finger_pressed, hx->max_fingers) {
+ x = be16_to_cpu(coord[i].x);
+ y = be16_to_cpu(coord[i].y);
+ w = width[i];
+
+ input_mt_slot(hx->input_dev, i);
+ input_mt_report_slot_state(hx->input_dev, MT_TOOL_FINGER, 1);
+ touchscreen_report_pos(hx->input_dev, &hx->props, x, y, true);
+ input_report_abs(hx->input_dev, ABS_MT_TOUCH_MAJOR, w);
+ }
+ input_mt_sync_frame(hx->input_dev);
+
+ for (i = 0; i < hx->keycount; i++)
+ input_report_key(hx->input_dev, hx->keycodes[i], key_pressed & BIT(i));
+
+ input_sync(hx->input_dev);
+ return 0;
+}
+
+static irqreturn_t hx852x_interrupt(int irq, void *ptr)
+{
+ struct hx852x *hx = ptr;
+ int error;
+
+ error = hx852x_handle_events(hx);
+ if (error) {
+ dev_err_ratelimited(&hx->client->dev, "failed to handle events: %d\n", error);
+ return IRQ_NONE;
+ }
+
+ return IRQ_HANDLED;
+}
+
+static int hx852x_input_open(struct input_dev *dev)
+{
+ struct hx852x *hx = input_get_drvdata(dev);
+ int error;
+
+ error = hx852x_power_on(hx);
+ if (error)
+ return error;
+
+ error = hx852x_start(hx);
+ if (error) {
+ hx852x_power_off(hx);
+ return error;
+ }
+
+ enable_irq(hx->client->irq);
+ return 0;
+}
+
+static void hx852x_input_close(struct input_dev *dev)
+{
+ struct hx852x *hx = input_get_drvdata(dev);
+
+ hx852x_stop(hx);
+ disable_irq(hx->client->irq);
+ hx852x_power_off(hx);
+}
+
+static int hx852x_parse_properties(struct hx852x *hx)
+{
+ struct device *dev = &hx->client->dev;
+ int error, count;
+
+ count = device_property_count_u32(dev, "linux,keycodes");
+ if (count == -EINVAL) {
+ /* Property does not exist, keycodes are optional */
+ return 0;
+ } else if (count < 0) {
+ dev_err(dev, "Failed to read linux,keycodes: %d\n", count);
+ return count;
+ } else if (count > HX852X_MAX_KEY_COUNT) {
+ dev_err(dev, "max supported keys: %u, found: %u\n",
+ HX852X_MAX_KEY_COUNT, hx->keycount);
+ return -EINVAL;
+ }
+ hx->keycount = count;
+
+ error = device_property_read_u32_array(dev, "linux,keycodes",
+ hx->keycodes, hx->keycount);
+ if (error) {
+ dev_err(dev, "failed to read linux,keycodes: %d\n", error);
+ return error;
+ }
+
+ return 0;
+}
+
+static int hx852x_probe(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ struct hx852x *hx;
+ int error, i;
+
+ if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C |
+ I2C_FUNC_SMBUS_WRITE_BYTE |
+ I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
+ I2C_FUNC_SMBUS_WRITE_WORD_DATA)) {
+ dev_err(dev, "not all required i2c functionality supported\n");
+ return -ENXIO;
+ }
+
+ hx = devm_kzalloc(dev, sizeof(*hx), GFP_KERNEL);
+ if (!hx)
+ return -ENOMEM;
+
+ hx->client = client;
+ hx->input_dev = devm_input_allocate_device(dev);
+ if (!hx->input_dev)
+ return -ENOMEM;
+
+ hx->input_dev->name = "Himax HX852x";
+ hx->input_dev->id.bustype = BUS_I2C;
+ hx->input_dev->open = hx852x_input_open;
+ hx->input_dev->close = hx852x_input_close;
+
+ i2c_set_clientdata(client, hx);
+ input_set_drvdata(hx->input_dev, hx);
+
+ hx->supplies[0].supply = "vcca";
+ hx->supplies[1].supply = "vccd";
+ error = devm_regulator_bulk_get(dev, ARRAY_SIZE(hx->supplies), hx->supplies);
+ if (error)
+ return dev_err_probe(dev, error, "failed to get regulators\n");
+
+ hx->reset_gpiod = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+ if (IS_ERR(hx->reset_gpiod))
+ return dev_err_probe(dev, PTR_ERR(hx->reset_gpiod),
+ "failed to get reset gpio\n");
+
+ error = devm_request_threaded_irq(dev, client->irq, NULL, hx852x_interrupt,
+ IRQF_ONESHOT | IRQF_NO_AUTOEN, NULL, hx);
+ if (error)
+ return dev_err_probe(dev, error, "failed to request irq %d", client->irq);
+
+ error = hx852x_read_config(hx);
+ if (error)
+ return error;
+
+ input_set_capability(hx->input_dev, EV_ABS, ABS_MT_POSITION_X);
+ input_set_capability(hx->input_dev, EV_ABS, ABS_MT_POSITION_Y);
+ input_set_abs_params(hx->input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
+
+ touchscreen_parse_properties(hx->input_dev, true, &hx->props);
+ error = hx852x_parse_properties(hx);
+ if (error)
+ return error;
+
+ hx->input_dev->keycode = hx->keycodes;
+ hx->input_dev->keycodemax = hx->keycount;
+ hx->input_dev->keycodesize = sizeof(hx->keycodes[0]);
+ for (i = 0; i < hx->keycount; i++)
+ input_set_capability(hx->input_dev, EV_KEY, hx->keycodes[i]);
+
+ error = input_mt_init_slots(hx->input_dev, hx->max_fingers,
+ INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
+ if (error)
+ return dev_err_probe(dev, error, "failed to init MT slots\n");
+
+ error = input_register_device(hx->input_dev);
+ if (error)
+ return dev_err_probe(dev, error, "failed to register input device\n");
+
+ return 0;
+}
+
+static int hx852x_suspend(struct device *dev)
+{
+ struct hx852x *hx = dev_get_drvdata(dev);
+ int error = 0;
+
+ mutex_lock(&hx->input_dev->mutex);
+ if (input_device_enabled(hx->input_dev))
+ error = hx852x_stop(hx);
+ mutex_unlock(&hx->input_dev->mutex);
+
+ return error;
+}
+
+static int hx852x_resume(struct device *dev)
+{
+ struct hx852x *hx = dev_get_drvdata(dev);
+ int error = 0;
+
+ mutex_lock(&hx->input_dev->mutex);
+ if (input_device_enabled(hx->input_dev))
+ error = hx852x_start(hx);
+ mutex_unlock(&hx->input_dev->mutex);
+
+ return error;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(hx852x_pm_ops, hx852x_suspend, hx852x_resume);
+
+#ifdef CONFIG_OF
+static const struct of_device_id hx852x_of_match[] = {
+ { .compatible = "himax,hx852es" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, hx852x_of_match);
+#endif
+
+static struct i2c_driver hx852x_driver = {
+ .probe = hx852x_probe,
+ .driver = {
+ .name = "himax_hx852x",
+ .pm = pm_sleep_ptr(&hx852x_pm_ops),
+ .of_match_table = of_match_ptr(hx852x_of_match),
+ },
+};
+module_i2c_driver(hx852x_driver);
+
+MODULE_DESCRIPTION("Himax HX852x(ES) Touchscreen Driver");
+MODULE_AUTHOR("Jonathan Albrieux <jonathan.albrieux@gmail.com>");
+MODULE_AUTHOR("Stephan Gerhold <stephan@gerhold.net>");
+MODULE_LICENSE("GPL");
--
2.42.0
^ permalink raw reply related
* [PATCH v3 1/2] dt-bindings: input: touchscreen: document Himax HX852x(ES)
From: Stephan Gerhold @ 2023-10-24 18:35 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Henrik Rydberg,
linux-input, devicetree, linux-kernel, Jeff LaBundy,
Christophe JAILLET, Jonathan Albrieux, Stephan Gerhold,
Krzysztof Kozlowski
In-Reply-To: <20231024-hx852x-v3-0-a1890d3a81e9@gerhold.net>
Himax HX852x(ES) is a touch panel controller with optional support
for capacitive touch keys.
Unfortunately, the model naming is quite unclear and confusing. There
seems to be a distinction between models (e.g. HX8526) and the "series"
suffix (e.g. -A, -B, -C, -D, -E, -ES). But this doesn't seem to be
applied very consistently because e.g. HX8527-E(44) actually seems to
belong to the -ES series.
The compatible consists of the actual part number followed by the
"series" as fallback compatible. Typically only the latter will be
interesting for drivers as there is no relevant difference on the
driver side.
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
.../bindings/input/touchscreen/himax,hx852es.yaml | 81 ++++++++++++++++++++++
1 file changed, 81 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/himax,hx852es.yaml b/Documentation/devicetree/bindings/input/touchscreen/himax,hx852es.yaml
new file mode 100644
index 000000000000..40a60880111d
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/himax,hx852es.yaml
@@ -0,0 +1,81 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/touchscreen/himax,hx852es.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Himax HX852x(ES) touch panel controller
+
+maintainers:
+ - Stephan Gerhold <stephan@gerhold.net>
+
+allOf:
+ - $ref: touchscreen.yaml#
+
+properties:
+ compatible:
+ items:
+ - enum:
+ - himax,hx8525e
+ - himax,hx8526e
+ - himax,hx8527e
+ - const: himax,hx852es
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+ description: Touch Screen Interrupt (TSIX), active low
+
+ reset-gpios:
+ maxItems: 1
+ description: External Reset (XRES), active low
+
+ vcca-supply:
+ description: Analog power supply (VCCA)
+
+ vccd-supply:
+ description: Digital power supply (VCCD)
+
+ touchscreen-inverted-x: true
+ touchscreen-inverted-y: true
+ touchscreen-size-x: true
+ touchscreen-size-y: true
+ touchscreen-swapped-x-y: true
+
+ linux,keycodes:
+ minItems: 1
+ maxItems: 4
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - reset-gpios
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/input/input.h>
+ #include <dt-bindings/interrupt-controller/irq.h>
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ touchscreen@48 {
+ compatible = "himax,hx8527e", "himax,hx852es";
+ reg = <0x48>;
+ interrupt-parent = <&tlmm>;
+ interrupts = <13 IRQ_TYPE_LEVEL_LOW>;
+ reset-gpios = <&tlmm 12 GPIO_ACTIVE_LOW>;
+ vcca-supply = <®_ts_vcca>;
+ vccd-supply = <&pm8916_l6>;
+ linux,keycodes = <KEY_BACK KEY_HOMEPAGE KEY_APPSELECT>;
+ };
+ };
+
+...
--
2.42.0
^ permalink raw reply related
* [PATCH v3 0/2] Input: add Himax HX852x(ES) touchscreen driver
From: Stephan Gerhold @ 2023-10-24 18:35 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Henrik Rydberg,
linux-input, devicetree, linux-kernel, Jeff LaBundy,
Christophe JAILLET, Jonathan Albrieux, Stephan Gerhold,
Krzysztof Kozlowski
Add DT schema and driver for the Himax HX852x(ES) touch panel
controller, with support for multi-touch and capacitive touch keys.
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
Changes in v3:
- Fix device_property_count_u32() error handling (Jeff)
- Properly handle errors in hx852x_suspend (Jeff)
- Simplify error handling in hx852x_read_config() (Jeff)
- Close i2c_msg array with trailing comma (Jeff)
- Clean up error handling in hx852x_power_off()
- Link to v2: https://lore.kernel.org/r/20230930-hx852x-v2-0-c5821947b225@gerhold.net
Changes in v2:
- dt-bindings: Swap required:/additionalProperties: (Krzysztof)
- Use dev_err_ratelimited() for error in IRQ thread (Christophe)
- Use dev_err_probe() consistently (Christophe)
- Improve error handling of hx852x_power_off()/hx852x_stop() (Jeff)
- Add linux/of.h and linux/mod_devicetable.h include (Jeff)
- Fix %d -> %u in some format strings (Jeff)
- Fix other small comments from Jeff
- Link to v1: https://lore.kernel.org/r/20230913-hx852x-v1-0-9c1ebff536eb@gerhold.net
---
Jonathan Albrieux (1):
Input: add Himax HX852x(ES) touchscreen driver
Stephan Gerhold (1):
dt-bindings: input: touchscreen: document Himax HX852x(ES)
.../bindings/input/touchscreen/himax,hx852es.yaml | 81 ++++
MAINTAINERS | 7 +
drivers/input/touchscreen/Kconfig | 10 +
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/himax_hx852x.c | 500 +++++++++++++++++++++
5 files changed, 599 insertions(+)
---
change-id: 20230816-hx852x-3490d2773322
Best regards,
--
Stephan Gerhold <stephan@gerhold.net>
^ permalink raw reply
* Re: [PATCH v2 2/2] Input: add Himax HX852x(ES) touchscreen driver
From: Stephan Gerhold @ 2023-10-24 18:35 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, linux-input, devicetree, linux-kernel,
Christophe JAILLET, Jonathan Albrieux
In-Reply-To: <ZTcPrC+0K1unNPIv@nixie71>
Hi Jeff,
On Mon, Oct 23, 2023 at 07:28:28PM -0500, Jeff LaBundy wrote:
> On Sun, Oct 22, 2023 at 08:49:13PM +0200, Stephan Gerhold wrote:
> > > > +static int hx852x_read_config(struct hx852x *hx)
> > > > +{
> > > > + struct device *dev = &hx->client->dev;
> > > > + struct hx852x_config conf;
> > > > + int x_res, y_res;
> > > > + int error;
> > > > +
> > > > + error = hx852x_power_on(hx);
> > > > + if (error)
> > > > + return error;
> > > > +
> > > > + /* Sensing must be turned on briefly to load the config */
> > > > + error = hx852x_start(hx);
> > > > + if (error)
> > > > + goto err_power_off;
> > > > +
> > > > + error = hx852x_stop(hx);
> > > > + if (error)
> > > > + goto err_power_off;
> > > > +
> > > > + error = i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH,
> > > > + HX852X_SRAM_SWITCH_TEST_MODE);
> > > > + if (error)
> > > > + goto err_power_off;
> > > > +
> > > > + error = i2c_smbus_write_word_data(hx->client, HX852X_REG_SRAM_ADDR,
> > > > + HX852X_SRAM_ADDR_CONFIG);
> > > > + if (error)
> > > > + goto err_test_mode;
> > > > +
> > > > + error = hx852x_i2c_read(hx, HX852X_REG_FLASH_RPLACE, &conf, sizeof(conf));
> > > > + if (error)
> > > > + goto err_test_mode;
> > > > +
> > > > + x_res = be16_to_cpu(conf.x_res);
> > > > + y_res = be16_to_cpu(conf.y_res);
> > > > + hx->max_fingers = (conf.max_pt & 0xf0) >> 4;
> > > > + dev_dbg(dev, "x res: %u, y res: %u, max fingers: %u\n",
> > > > + x_res, y_res, hx->max_fingers);
> > > > +
> > > > + if (hx->max_fingers > HX852X_MAX_FINGERS) {
> > > > + dev_err(dev, "max supported fingers: %u, found: %u\n",
> > > > + HX852X_MAX_FINGERS, hx->max_fingers);
> > > > + error = -EINVAL;
> > > > + goto err_test_mode;
> > > > + }
> > > > +
> > > > + if (x_res && y_res) {
> > > > + input_set_abs_params(hx->input_dev, ABS_MT_POSITION_X, 0, x_res - 1, 0, 0);
> > > > + input_set_abs_params(hx->input_dev, ABS_MT_POSITION_Y, 0, y_res - 1, 0, 0);
> > > > + }
> > > > +
> > > > + error = i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH, 0);
> > > > + if (error)
> > > > + goto err_power_off;
> > > > +
> > > > + return hx852x_power_off(hx);
> > > > +
> > > > +err_test_mode:
> > > > + i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH, 0);
> > > > +err_power_off:
> > > > + hx852x_power_off(hx);
> > > > + return error;
> > >
> > > Your new version is an improvement, but maybe we can remove duplicate
> > > code by introducing some helper variables:
> > >
> > > int error, error2 = 0, error3;
> > >
> > > /* ... */
> > >
> > > err_test_mode:
> > > error2 = i2c_smbus_write_byte_data(...);
> > >
> > > err_power_off:
> > > error3 = hx852x_power_off(...);
> > >
> > > if (error)
> > > return error;
> > >
> > > return error2 ? : error3;
> > >
> > > In this case we achieve our goal of attempting to return the device to a
> > > safe state in both passing and failing cases. In the event of multiple
> > > errors, we return the first to occur.
> > >
> >
> > Right, this would work as well. Personally I think my solution is
> > slightly easier to read though. In your version my eyes somewhat
> > "stumble" over the multiple "error" variables and then about the purpose
> > of the "? : " construction. This is probably highly subjective. :-)
>
> Agreed, my suggestion is a bit unwieldy, and prone to uninitialized
> variable bugs. However, I feel that duplicate code, especially side
> by side like this, is also confusing and prone to bugs in case the
> sequence must be updated in the future. As a compromise, how about
> something closer to my first idea:
>
> err_test_mode:
> error = i2c_smbus_write_byte_data(...) ? : error;
>
> err_power_off:
> return hx852x_power_off(...) ? : error;
>
> This is nice and compact, and ensures that errors returned by the two
> functions are reported no matter the flow. The only functional change
> is that the _last_ error takes priority; but in practice this does not
> really matter. Normally if one I2C write fails, all subsequent writes
> will fail with the same return code until the hardware is recovered
> somehow.
>
> For the corner case where the code jumps to exit_test_mode with error
> equal to -EINVAL, and i2c_smbus_write_byte_data() then fails and changes
> error to something like -EIO, I don't think we care. After the HW issue
> is fixed and all I2C writes succeed, the developer will then see that
> the number of contacts reported by the FW is invalid anyway :)
>
> Side note: the '? :' syntax is just a shortcut that sets error equal
> to the return value of i2c_smbus_write_byte_data() if true (failure)
> without having to declare a temporary variable. If false (no failure),
> error is assigned to itself. It is perfectly legal.
>
Thanks a lot for your detailed review and explanations!
In v3 I have changed the code to your suggestion above and also
addressed your other comments in the initial reply. :)
Stephan
^ permalink raw reply
* Re: [PATCH v5 2/2] input: joystick: driver for Adafruit Seesaw Gamepad
From: Thomas Weißschuh @ 2023-10-24 15:56 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Anshul Dalal, linux-input, devicetree, Dmitry Torokhov,
Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Thomas Weißschuh, Shuah Khan, linux-kernel-mentees,
linux-kernel
In-Reply-To: <ZTbklpRhpMIPey2j@nixie71>
Hi Jeff,
Oct 23, 2023 23:24:55 Jeff LaBundy <jeff@labundy.com>:
> On Mon, Oct 23, 2023 at 07:55:52AM +0200, Thomas Weißschuh wrote:
>
> [...]
>
>>>> + err = i2c_master_send(client, write_buf, sizeof(write_buf));
>>>> + if (err < 0)
>>>> + return err;
>>>
>>> You correctly return err (or rather, ret) for negative values, but you should also
>>> check that ret matches the size of the data sent. For 0 <= ret < sizeof(writebuf),
>>> return -EIO.
>>
>> The driver did this originally.
>> I then requested it to be removed as this case
>> can never happen.
>> i2c_master_send will either return size of(writebuf) or an error.
>
> Great catch; indeed you are correct. Apologies for having missed this
> in the change log; this is good to know in the future.
I guess it would make sense to also adapt the
function documentation to be more explicit
about this invariant.
No need to complicate every caller unnecessarily.
I can send a patch somewhere next week, but
if you want to send one I'll be happy to review it.
> That being said, it's a moot point IMO; this driver seems like a good
> candidate for regmap. If regmap cannot be made to work here for some
> reason, then I'd like to at least see some wrapper functions to avoid
> duplicate code and manual assignments to a buffer.
Ack.
Thomas
^ permalink raw reply
* Re: [PATCH v4 3/4] mfd: cs40l50: Add support for CS40L50 core driver
From: Lee Jones @ 2023-10-24 15:47 UTC (permalink / raw)
To: Jeff LaBundy
Cc: James Ogletree, James Ogletree, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Fred Treven, Ben Bright,
linux-input@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <ZTcZIMbrFEhz+rm4@nixie71>
On Mon, 23 Oct 2023, Jeff LaBundy wrote:
> I understand that no customer would ever build the to-be-added codec
> driver _without_ the input driver, but the MFD must be generic enough
> to support this case. Would a codec-only implementation use f0 and ReDC
> estimation? If so, then these functions _do_ belong in the MFD, albeit
> with some comments to explain their nature.
I'm not going to be able to accept a single-function device into the
multi-function devices subsystem. Please submit both once the codec is
ready.
> > > >> + 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.
> >
> > It just seems odd to create something and not tear it down.
>
> mutex_init() is not creating anything; the mutex struct is allocated as
> part of the driver's private data, which is de-allocated as part of device
> managed resources being freed when the module is unloaded.
>
> mutex_destroy() is a NOP unless CONFIG_DEBUG_MUTEXES is set, and there are
> roughly 4x fewer instances of it than mutex_init() in mainline. I recommend
> not to add mutex_destroy() because it adds unnecessary tear-down paths and
> remove() callbacks that wouldn't otherwise have to exist.
Fair enough.
--
Lee Jones [李琼斯]
^ permalink raw reply
* Re:Re: [PATCH] HID: fix a crash in hid_debug_events_release
From: be286 @ 2023-10-24 9:49 UTC (permalink / raw)
To: Rahul Rameshbabu; +Cc: jikos, benjamin.tissoires, linux-input, linux-kernel
In-Reply-To: <87a5s9mldo.fsf@protonmail.com>
Hi Rahul,
When hid_debug_events_release() was being called, in most case,
hid_device_release() finish already, the memory of list->hdev
freed by hid_device_release(), if list->hdev memory
reallocate by others, and it's modified, zeroed, then
list->hdev->debug_list_lock occasioned crash come out.
The runing order:
[ 258.201069] CPU: 0 PID: 203 Comm: kworker/0:2 Not tainted 5.10.110 #255
[ 258.201073] Hardware name: Rockchip RK3588 EVB4 LP4 V10 Board (DT)
[ 258.201086] Workqueue: usb_hub_wq hub_event
[ 258.201092] Call trace:
[ 258.201100] dump_backtrace+0x0/0x1f8
[ 258.201105] show_stack+0x1c/0x2c
[ 258.201112] dump_stack_lvl+0xd8/0x12c
[ 258.201116] dump_stack+0x1c/0x60
[ 258.201122] hid_device_release+0x94/0xb4
[ 258.201127] device_release+0x38/0x94
[ 258.201133] kobject_cleanup+0xd0/0x174
[ 258.201137] kobject_put+0x68/0xa8
[ 258.201143] put_device+0x1c/0x2c
[ 258.201146] hid_destroy_device+0x60/0x74
[ 258.201153] usbhid_disconnect+0x5c/0x90
[ 258.201157] usb_unbind_interface+0xc4/0x268
[ 258.201162] device_release_driver_internal+0x184/0x25c
[ 258.201165] device_release_driver+0x1c/0x2c
[ 258.201169] bus_remove_device+0xdc/0x138
[ 258.201173] device_del+0x1d0/0x3d8
[ 258.201177] usb_disable_device+0x108/0x228
[ 258.201181] usb_disconnect+0xf4/0x304
[ 258.201184] usb_disconnect+0xdc/0x304
[ 258.201188] hub_port_connect+0x88/0xa24
[ 258.201191] hub_port_connect_change+0x2d8/0x4cc
[ 258.201195] port_event+0x550/0x614
[ 258.201198] hub_event+0x12c/0x494
[ 258.201203] process_one_work+0x1f4/0x490
[ 258.201207] worker_thread+0x278/0x4dc
[ 258.201211] kthread+0x130/0x338
[ 258.201215] ret_from_fork+0x10/0x30
[ 259.925641] CPU: 1 PID: 2354 Comm: hidt_bridge Not tainted 5.10.110 #255
[ 259.925652] Hardware name: Rockchip RK3588 EVB4 LP4 V10 Board (DT)
[ 259.925656] Call trace:
[ 259.925671] dump_backtrace+0x0/0x1f8
[ 259.925676] show_stack+0x1c/0x2c
[ 259.925685] dump_stack_lvl+0xd8/0x12c
[ 259.925689] dump_stack+0x1c/0x60
[ 259.925697] hid_debug_events_release+0x24/0x134
[ 259.925704] full_proxy_release+0x50/0xbc
[ 259.925709] __fput+0xdc/0x238
[ 259.925714] ____fput+0x14/0x24
[ 259.925720] task_work_run+0x90/0x148
[ 259.925725] do_exit+0x1bc/0x8a4
[ 259.925729] do_group_exit+0x8c/0xa4
[ 259.925734] get_signal+0x468/0x744
[ 259.925739] do_signal+0x84/0x280
[ 259.925743] do_notify_resume+0xd8/0x228
[ 259.925747] work_pending+0xc/0x3f0
The crash:
[ 120.728477][ T4396] kernel BUG at lib/list_debug.c:53!
[ 120.728505][ T4396] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
[ 120.739806][ T4396] Modules linked in: bcmdhd dhd_static_buf 8822cu pcie_mhi r8168
[ 120.747386][ T4396] CPU: 1 PID: 4396 Comm: hidt_bridge Not tainted 5.10.110 #257
[ 120.754771][ T4396] Hardware name: Rockchip RK3588 EVB4 LP4 V10 Board (DT)
[ 120.761643][ T4396] pstate: 60400089 (nZCv daIf +PAN -UAO -TCO BTYPE=--)
[ 120.768338][ T4396] pc : __list_del_entry_valid+0x98/0xac
[ 120.773730][ T4396] lr : __list_del_entry_valid+0x98/0xac
[ 120.779120][ T4396] sp : ffffffc01e62bb60
[ 120.783126][ T4396] x29: ffffffc01e62bb60 x28: ffffff818ce3a200
[ 120.789126][ T4396] x27: 0000000000000009 x26: 0000000000980000
[ 120.795126][ T4396] x25: ffffffc012431000 x24: ffffff802c6d4e00
[ 120.801125][ T4396] x23: ffffff8005c66f00 x22: ffffffc01183b5b8
[ 120.807125][ T4396] x21: ffffff819df2f100 x20: 0000000000000000
[ 120.813124][ T4396] x19: ffffff802c3f0700 x18: ffffffc01d2cd058
[ 120.819124][ T4396] x17: 0000000000000000 x16: 0000000000000000
[ 120.825124][ T4396] x15: 0000000000000004 x14: 0000000000003fff
[ 120.831123][ T4396] x13: ffffffc012085588 x12: 0000000000000003
[ 120.837123][ T4396] x11: 00000000ffffbfff x10: 0000000000000003
[ 120.843123][ T4396] x9 : 455103d46b329300 x8 : 455103d46b329300
[ 120.849124][ T4396] x7 : 74707572726f6320 x6 : ffffffc0124b8cb5
[ 120.855124][ T4396] x5 : ffffffffffffffff x4 : 0000000000000000
[ 120.861123][ T4396] x3 : ffffffc011cf4f90 x2 : ffffff81fee7b948
[ 120.867122][ T4396] x1 : ffffffc011cf4f90 x0 : 0000000000000054
[ 120.873122][ T4396] Call trace:
[ 120.876259][ T4396] __list_del_entry_valid+0x98/0xac
[ 120.881304][ T4396] hid_debug_events_release+0x48/0x12c
[ 120.886617][ T4396] full_proxy_release+0x50/0xbc
[ 120.891323][ T4396] __fput+0xdc/0x238
[ 120.895075][ T4396] ____fput+0x14/0x24
[ 120.898911][ T4396] task_work_run+0x90/0x148
[ 120.903268][ T4396] do_exit+0x1bc/0x8a4
[ 120.907193][ T4396] do_group_exit+0x8c/0xa4
[ 120.911458][ T4396] get_signal+0x468/0x744
[ 120.915643][ T4396] do_signal+0x84/0x280
[ 120.919650][ T4396] do_notify_resume+0xd0/0x218
[ 120.924262][ T4396] work_pending+0xc/0x3f0
At 2023-10-23 23:03:35, "Rahul Rameshbabu" <sergeantsagara@protonmail.com> wrote:
>Hi Charles,
>
>On Mon, 23 Oct, 2023 17:35:00 +0800 "Charles Yi" <be286@163.com> wrote:
>> hid_debug_events_release() access released memory by
>> hid_device_release(). This is fixed by the patch.
>>
>
>A couple of things here. Can you add a Fixes: tag?
>
>https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
>
>Before any v2 however, it would be nice to understand where this issue
>is coming from. I am wondering if it's really a core issue or rather an
>issue with a higher level device specific driver making use of the hid
>subsystem. I am having a hard time seeing how this issue occurs
>currently. A stack trace in a follow-up email to this one pertaining to
>the crash would be helpful. If you are resolving a syzbot report, a link
>to that report would suffice.
>
>This patch doesn't make a lot of sense to me as-is because
>hid_debug_events_release is about release resources related to hid debug
>events (at least from my current understanding). It should not be
>free-ing the underlying hid device/instance itself.
>
>> Signed-off-by: Charles Yi <be286@163.com>
>> ---
>> drivers/hid/hid-core.c | 12 ++++++++++--
>> drivers/hid/hid-debug.c | 3 +++
>> include/linux/hid.h | 3 +++
>> 3 files changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index 8992e3c1e769..e0181218ad85 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -702,15 +702,22 @@ static void hid_close_report(struct hid_device *device)
>> * Free a device structure, all reports, and all fields.
>> */
>>
>> -static void hid_device_release(struct device *dev)
>> +void hiddev_free(struct kref *ref)
>> {
>> - struct hid_device *hid = to_hid_device(dev);
>> + struct hid_device *hid = container_of(ref, struct hid_device, ref);
>>
>> hid_close_report(hid);
>> kfree(hid->dev_rdesc);
>> kfree(hid);
>> }
>>
>> +static void hid_device_release(struct device *dev)
>> +{
>> + struct hid_device *hid = to_hid_device(dev);
>> +
>> + kref_put(&hid->ref, hiddev_free);
>> +}
>> +
>> /*
>> * Fetch a report description item from the data stream. We support long
>> * items, though they are not used yet.
>> @@ -2846,6 +2853,7 @@ struct hid_device *hid_allocate_device(void)
>> spin_lock_init(&hdev->debug_list_lock);
>> sema_init(&hdev->driver_input_lock, 1);
>> mutex_init(&hdev->ll_open_lock);
>> + kref_init(&hdev->ref);
>>
>> hid_bpf_device_init(hdev);
>>
>> diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
>> index e7ef1ea107c9..7dd83ec74f8a 100644
>> --- a/drivers/hid/hid-debug.c
>> +++ b/drivers/hid/hid-debug.c
>> @@ -1135,6 +1135,7 @@ static int hid_debug_events_open(struct inode *inode, struct file *file)
>> goto out;
>> }
>> list->hdev = (struct hid_device *) inode->i_private;
>> + kref_get(&list->hdev->ref);
>> file->private_data = list;
>> mutex_init(&list->read_mutex);
>>
>> @@ -1227,6 +1228,8 @@ static int hid_debug_events_release(struct inode *inode, struct file *file)
>> list_del(&list->node);
>> spin_unlock_irqrestore(&list->hdev->debug_list_lock, flags);
>> kfifo_free(&list->hid_debug_fifo);
>> +
>> + kref_put(&list->hdev->ref, hiddev_free);
>> kfree(list);
>>
>> return 0;
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index 964ca1f15e3f..3b08a2957229 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -679,6 +679,7 @@ struct hid_device { /* device report descriptor */
>> struct list_head debug_list;
>> spinlock_t debug_list_lock;
>> wait_queue_head_t debug_wait;
>> + struct kref ref;
>>
>> unsigned int id; /* system unique id */
>>
>> @@ -687,6 +688,8 @@ struct hid_device { /* device report descriptor */
>> #endif /* CONFIG_BPF */
>> };
>>
>> +void hiddev_free(struct kref *ref);
>> +
>> #define to_hid_device(pdev) \
>> container_of(pdev, struct hid_device, dev)
>
>--
>Thank you for the patch,
>
>Rahul Rameshbabu
^ permalink raw reply
* Re: [PATCH] hid: lenovo: Resend all settings on reset_resume for compact keyboards
From: Martin Kepplinger @ 2023-10-24 8:55 UTC (permalink / raw)
To: jikos, benjamin.tissoires, jm, linux-kernel
Cc: linux-input, stable, mail, hdegoede, iam
In-Reply-To: <20231002150914.22101-1-martink@posteo.de>
Am Montag, dem 02.10.2023 um 15:09 +0000 schrieb Martin Kepplinger:
> From: Jamie Lentin <jm@lentin.co.uk>
>
> The USB Compact Keyboard variant requires a reset_resume function to
> restore keyboard configuration after a suspend in some situations.
> Move
> configuration normally done on probe to lenovo_features_set_cptkbd(),
> then
> recycle this for use on reset_resume.
>
> Without, the keyboard and driver would end up in an inconsistent
> state,
> breaking middle-button scrolling amongst other problems, and
> twiddling
> sysfs values wouldn't help as the middle-button mode won't be set
> until
> the driver is reloaded.
>
> Tested on a USB and Bluetooth Thinkpad Compact Keyboard.
>
> CC: stable@vger.kernel.org
> Fixes: 94eefa271323 ("HID: lenovo: Use native middle-button mode for
> compact keyboards")
> Signed-off-by: Jamie Lentin <jm@lentin.co.uk>
> Signed-off-by: Martin Kepplinger <martink@posteo.de>
This is sitting over 3 weeks and I simply add Bernhard and Hans who
wrote big parts of the driver. Maybe more review can help with queuing
this bugfix up? (scrolling and function keys are currently broken after
resuming)
I basically sent Jamie's patch because I have the hardware:
https://lore.kernel.org/all/20231002150914.22101-1-martink@posteo.de/
thanks,
martin
> ---
> drivers/hid/hid-lenovo.c | 50 +++++++++++++++++++++++++++-----------
> --
> 1 file changed, 34 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
> index 44763c0da444..614320bff39f 100644
> --- a/drivers/hid/hid-lenovo.c
> +++ b/drivers/hid/hid-lenovo.c
> @@ -521,6 +521,19 @@ static void lenovo_features_set_cptkbd(struct
> hid_device *hdev)
> int ret;
> struct lenovo_drvdata *cptkbd_data = hid_get_drvdata(hdev);
>
> + /*
> + * Tell the keyboard a driver understands it, and turn F7,
> F9, F11 into
> + * regular keys
> + */
> + ret = lenovo_send_cmd_cptkbd(hdev, 0x01, 0x03);
> + if (ret)
> + hid_warn(hdev, "Failed to switch F7/9/11 mode: %d\n",
> ret);
> +
> + /* Switch middle button to native mode */
> + ret = lenovo_send_cmd_cptkbd(hdev, 0x09, 0x01);
> + if (ret)
> + hid_warn(hdev, "Failed to switch middle button:
> %d\n", ret);
> +
> ret = lenovo_send_cmd_cptkbd(hdev, 0x05, cptkbd_data-
> >fn_lock);
> if (ret)
> hid_err(hdev, "Fn-lock setting failed: %d\n", ret);
> @@ -1126,22 +1139,6 @@ static int lenovo_probe_cptkbd(struct
> hid_device *hdev)
> }
> hid_set_drvdata(hdev, cptkbd_data);
>
> - /*
> - * Tell the keyboard a driver understands it, and turn F7,
> F9, F11 into
> - * regular keys (Compact only)
> - */
> - if (hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD ||
> - hdev->product == USB_DEVICE_ID_LENOVO_CBTKBD) {
> - ret = lenovo_send_cmd_cptkbd(hdev, 0x01, 0x03);
> - if (ret)
> - hid_warn(hdev, "Failed to switch F7/9/11
> mode: %d\n", ret);
> - }
> -
> - /* Switch middle button to native mode */
> - ret = lenovo_send_cmd_cptkbd(hdev, 0x09, 0x01);
> - if (ret)
> - hid_warn(hdev, "Failed to switch middle button:
> %d\n", ret);
> -
> /* Set keyboard settings to known state */
> cptkbd_data->middlebutton_state = 0;
> cptkbd_data->fn_lock = true;
> @@ -1264,6 +1261,24 @@ static int lenovo_probe(struct hid_device
> *hdev,
> return ret;
> }
>
> +#ifdef CONFIG_PM
> +static int lenovo_reset_resume(struct hid_device *hdev)
> +{
> + switch (hdev->product) {
> + case USB_DEVICE_ID_LENOVO_CUSBKBD:
> + case USB_DEVICE_ID_LENOVO_TPIIUSBKBD:
> + if (hdev->type == HID_TYPE_USBMOUSE)
> + lenovo_features_set_cptkbd(hdev);
> +
> + break;
> + default:
> + break;
> + }
> +
> + return 0;
> +}
> +#endif
> +
> static void lenovo_remove_tpkbd(struct hid_device *hdev)
> {
> struct lenovo_drvdata *data_pointer = hid_get_drvdata(hdev);
> @@ -1380,6 +1395,9 @@ static struct hid_driver lenovo_driver = {
> .raw_event = lenovo_raw_event,
> .event = lenovo_event,
> .report_fixup = lenovo_report_fixup,
> +#ifdef CONFIG_PM
> + .reset_resume = lenovo_reset_resume,
> +#endif
> };
> module_hid_driver(lenovo_driver);
>
^ permalink raw reply
* Wycena paneli fotowoltaicznych
From: Kamil Lasek @ 2023-10-24 8:10 UTC (permalink / raw)
To: linux-input
Dzień dobry,
dostrzegam możliwość współpracy z Państwa firmą.
Świadczymy kompleksową obsługę inwestycji w fotowoltaikę, która obniża koszty energii elektrycznej.
Czy są Państwo zainteresowani weryfikacją wstępnych propozycji?
Pozdrawiam,
Kamil Lasek
^ permalink raw reply
* Re: [PATCH v4 3/4] mfd: cs40l50: Add support for CS40L50 core driver
From: James Ogletree @ 2023-10-24 1:30 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Lee Jones, James Ogletree, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Fred Treven, Ben Bright,
linux-input@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <ZTcZIMbrFEhz+rm4@nixie71>
> On Oct 23, 2023, at 8:08 PM, Jeff LaBundy <jeff@labundy.com> wrote:
>
>>>> 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.
>
> I think this is just a misnomer; the code uses the terms "mailbox" and
> "mbox" throughout because the relevant registers are named as such in
> the datasheet.
>
> Please correct me James, but I believe the datasheet uses this language
> because both the host and the part itself have write access, meaning the
> part may write a status code to the register after the host writes that
> same register. There is no relation to IPC or the mbox subsystem.
>
> That being said, some of the functions currently placed in this MFD,
> namely those related to haptic motor properties (e.g. f0 and ReDC), do
> seem more appropriate for the input/FF child device. My understanding
> is that those functions serve only momentary haptic click effects and
> not the I2S streaming case; please let me know if I have misunderstood.
>
> I understand that no customer would ever build the to-be-added codec
> driver _without_ the input driver, but the MFD must be generic enough
> to support this case. Would a codec-only implementation use f0 and ReDC
> estimation? If so, then these functions _do_ belong in the MFD, albeit
> with some comments to explain their nature.
Thank you for the clarifications, Jeff, and you are correct on all counts.
I see that I spoke before having a good enough grasp on the mailbox
framework. As regards the codec-only use case, they would not be used.
So those functions do belong in the input driver.
^ permalink raw reply
* Re: [PATCH v4 3/4] mfd: cs40l50: Add support for CS40L50 core driver
From: James Ogletree @ 2023-10-24 1:14 UTC (permalink / raw)
To: Lee Jones
Cc: James Ogletree, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Fred Treven, Ben Bright, linux-input@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
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
^ permalink raw reply
* Re: [PATCH v4 3/4] mfd: cs40l50: Add support for CS40L50 core driver
From: Jeff LaBundy @ 2023-10-24 1:08 UTC (permalink / raw)
To: Lee Jones
Cc: James Ogletree, James Ogletree, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Fred Treven, Ben Bright,
linux-input@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20231023092046.GA8909@google.com>
Hi Lee and James,
On Mon, Oct 23, 2023 at 10:20:46AM +0100, Lee Jones wrote:
[...]
> > >> +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?
FWIW, this is not an uncommon outcome for submissions that span multiple
subsystems.
>
> 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
Right, the history here is that this driver started its life in input,
where submitters have historically been asked to use 'error' for return
variables that return either zero or a negative error code. Since this
driver is no longer in input, this can easily be changed.
[...]
> > > 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.
I think this is just a misnomer; the code uses the terms "mailbox" and
"mbox" throughout because the relevant registers are named as such in
the datasheet.
Please correct me James, but I believe the datasheet uses this language
because both the host and the part itself have write access, meaning the
part may write a status code to the register after the host writes that
same register. There is no relation to IPC or the mbox subsystem.
That being said, some of the functions currently placed in this MFD,
namely those related to haptic motor properties (e.g. f0 and ReDC), do
seem more appropriate for the input/FF child device. My understanding
is that those functions serve only momentary haptic click effects and
not the I2S streaming case; please let me know if I have misunderstood.
I understand that no customer would ever build the to-be-added codec
driver _without_ the input driver, but the MFD must be generic enough
to support this case. Would a codec-only implementation use f0 and ReDC
estimation? If so, then these functions _do_ belong in the MFD, albeit
with some comments to explain their nature.
[...]
> > >> +{
> > >> + 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.
>
> It just seems odd to create something and not tear it down.
mutex_init() is not creating anything; the mutex struct is allocated as
part of the driver's private data, which is de-allocated as part of device
managed resources being freed when the module is unloaded.
mutex_destroy() is a NOP unless CONFIG_DEBUG_MUTEXES is set, and there are
roughly 4x fewer instances of it than mutex_init() in mainline. I recommend
not to add mutex_destroy() because it adds unnecessary tear-down paths and
remove() callbacks that wouldn't otherwise have to exist.
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v2 2/2] Input: add Himax HX852x(ES) touchscreen driver
From: Jeff LaBundy @ 2023-10-24 0:28 UTC (permalink / raw)
To: Stephan Gerhold
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, linux-input, devicetree, linux-kernel,
Christophe JAILLET, Jonathan Albrieux
In-Reply-To: <ZTVuqW7oU5BmPzTS@gerhold.net>
Hi Stephan,
On Sun, Oct 22, 2023 at 08:49:13PM +0200, Stephan Gerhold wrote:
> > > +static int hx852x_read_config(struct hx852x *hx)
> > > +{
> > > + struct device *dev = &hx->client->dev;
> > > + struct hx852x_config conf;
> > > + int x_res, y_res;
> > > + int error;
> > > +
> > > + error = hx852x_power_on(hx);
> > > + if (error)
> > > + return error;
> > > +
> > > + /* Sensing must be turned on briefly to load the config */
> > > + error = hx852x_start(hx);
> > > + if (error)
> > > + goto err_power_off;
> > > +
> > > + error = hx852x_stop(hx);
> > > + if (error)
> > > + goto err_power_off;
> > > +
> > > + error = i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH,
> > > + HX852X_SRAM_SWITCH_TEST_MODE);
> > > + if (error)
> > > + goto err_power_off;
> > > +
> > > + error = i2c_smbus_write_word_data(hx->client, HX852X_REG_SRAM_ADDR,
> > > + HX852X_SRAM_ADDR_CONFIG);
> > > + if (error)
> > > + goto err_test_mode;
> > > +
> > > + error = hx852x_i2c_read(hx, HX852X_REG_FLASH_RPLACE, &conf, sizeof(conf));
> > > + if (error)
> > > + goto err_test_mode;
> > > +
> > > + x_res = be16_to_cpu(conf.x_res);
> > > + y_res = be16_to_cpu(conf.y_res);
> > > + hx->max_fingers = (conf.max_pt & 0xf0) >> 4;
> > > + dev_dbg(dev, "x res: %u, y res: %u, max fingers: %u\n",
> > > + x_res, y_res, hx->max_fingers);
> > > +
> > > + if (hx->max_fingers > HX852X_MAX_FINGERS) {
> > > + dev_err(dev, "max supported fingers: %u, found: %u\n",
> > > + HX852X_MAX_FINGERS, hx->max_fingers);
> > > + error = -EINVAL;
> > > + goto err_test_mode;
> > > + }
> > > +
> > > + if (x_res && y_res) {
> > > + input_set_abs_params(hx->input_dev, ABS_MT_POSITION_X, 0, x_res - 1, 0, 0);
> > > + input_set_abs_params(hx->input_dev, ABS_MT_POSITION_Y, 0, y_res - 1, 0, 0);
> > > + }
> > > +
> > > + error = i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH, 0);
> > > + if (error)
> > > + goto err_power_off;
> > > +
> > > + return hx852x_power_off(hx);
> > > +
> > > +err_test_mode:
> > > + i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH, 0);
> > > +err_power_off:
> > > + hx852x_power_off(hx);
> > > + return error;
> >
> > Your new version is an improvement, but maybe we can remove duplicate
> > code by introducing some helper variables:
> >
> > int error, error2 = 0, error3;
> >
> > /* ... */
> >
> > err_test_mode:
> > error2 = i2c_smbus_write_byte_data(...);
> >
> > err_power_off:
> > error3 = hx852x_power_off(...);
> >
> > if (error)
> > return error;
> >
> > return error2 ? : error3;
> >
> > In this case we achieve our goal of attempting to return the device to a
> > safe state in both passing and failing cases. In the event of multiple
> > errors, we return the first to occur.
> >
>
> Right, this would work as well. Personally I think my solution is
> slightly easier to read though. In your version my eyes somewhat
> "stumble" over the multiple "error" variables and then about the purpose
> of the "? : " construction. This is probably highly subjective. :-)
Agreed, my suggestion is a bit unwieldy, and prone to uninitialized
variable bugs. However, I feel that duplicate code, especially side
by side like this, is also confusing and prone to bugs in case the
sequence must be updated in the future. As a compromise, how about
something closer to my first idea:
err_test_mode:
error = i2c_smbus_write_byte_data(...) ? : error;
err_power_off:
return hx852x_power_off(...) ? : error;
This is nice and compact, and ensures that errors returned by the two
functions are reported no matter the flow. The only functional change
is that the _last_ error takes priority; but in practice this does not
really matter. Normally if one I2C write fails, all subsequent writes
will fail with the same return code until the hardware is recovered
somehow.
For the corner case where the code jumps to exit_test_mode with error
equal to -EINVAL, and i2c_smbus_write_byte_data() then fails and changes
error to something like -EIO, I don't think we care. After the HW issue
is fixed and all I2C writes succeed, the developer will then see that
the number of contacts reported by the FW is invalid anyway :)
Side note: the '? :' syntax is just a shortcut that sets error equal
to the return value of i2c_smbus_write_byte_data() if true (failure)
without having to declare a temporary variable. If false (no failure),
error is assigned to itself. It is perfectly legal.
> Do you mind if keep this as-is? I don't have a strong opinion about
> this, but a slight preference for my current solution.
I think any of these three solutions gets the job done; please choose
the one you feel is easiest to read and maintain.
> > > +}
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v5 2/2] input: joystick: driver for Adafruit Seesaw Gamepad
From: Jeff LaBundy @ 2023-10-23 21:24 UTC (permalink / raw)
To: Thomas Weißschuh
Cc: Anshul Dalal, linux-input, devicetree, Dmitry Torokhov,
Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Thomas Weißschuh, Shuah Khan, linux-kernel-mentees,
linux-kernel
In-Reply-To: <00d2fcbc-3fd8-477d-8df1-afec20b458b6@t-8ch.de>
Hi Thomas,
On Mon, Oct 23, 2023 at 07:55:52AM +0200, Thomas Weißschuh wrote:
[...]
> >> + err = i2c_master_send(client, write_buf, sizeof(write_buf));
> >> + if (err < 0)
> >> + return err;
> >
> > You correctly return err (or rather, ret) for negative values, but you should also
> > check that ret matches the size of the data sent. For 0 <= ret < sizeof(writebuf),
> > return -EIO.
>
> The driver did this originally.
> I then requested it to be removed as this case
> can never happen.
> i2c_master_send will either return size of(writebuf) or an error.
Great catch; indeed you are correct. Apologies for having missed this
in the change log; this is good to know in the future.
That being said, it's a moot point IMO; this driver seems like a good
candidate for regmap. If regmap cannot be made to work here for some
reason, then I'd like to at least see some wrapper functions to avoid
duplicate code and manual assignments to a buffer.
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v5 1/2] dt-bindings: input: bindings for Adafruit Seesaw Gamepad
From: Jeff LaBundy @ 2023-10-23 18:53 UTC (permalink / raw)
To: Anshul Dalal
Cc: linux-input, devicetree, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Thomas Weißschuh,
Shuah Khan, linux-kernel-mentees, linux-kernel, Conor Dooley,
Krzysztof Kozlowski
In-Reply-To: <7ef752b4-915b-4f9d-8425-79df8195656b@gmail.com>
Hi Anshul,
Thank you for this additional information.
On Mon, Oct 23, 2023 at 05:28:10PM +0530, Anshul Dalal wrote:
> Hello Jeff,
>
> On 10/23/23 05:17, Jeff LaBundy wrote:
> > Hi Anshul,
> >
> > On Tue, Oct 17, 2023 at 09:13:44AM +0530, Anshul Dalal wrote:
> >> Adds bindings for the Adafruit Seesaw Gamepad.
> >>
> >> The gamepad functions as an i2c device with the default address of 0x50
> >> and has an IRQ pin that can be enabled in the driver to allow for a rising
> >> edge trigger on each button press or joystick movement.
> >>
> >> Product page:
> >> https://www.adafruit.com/product/5743
> >> Arduino driver:
> >> https://github.com/adafruit/Adafruit_Seesaw
> >>
> >> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> >> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >> Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
> >
> > Perhaps this ship has sailed, but is there any reason this simple device
> > cannot be added to Documentation/devicetree/bindings/trivial-devices.yaml
> > as opposed to having its own binding?
> >
> > It has no vendor-specific properties, and the only properties are the
> > standard properties already understood by the I2C core. In case I have
> > misunderstood, please let me know.
> >
>
> The driver currently implements only a subset of the functionality in
> the Adafruit Seesaw specification. I eventually plan on adding adding
> full support for the Seesaw framework in the form of a driver for the
> atsamd09 seesaw breakout board:
> https://learn.adafruit.com/adafruit-seesaw-atsamd09-breakout
>
> Then I think it would be better for this driver to use the newly exposed
> seesaw APIs by the atsamd09 driver instead of relying on kernel's i2c APIs.
The underlying functions used to implement I2C communication are orthogonal
to the binding. Whether you use the kernel's core I2C support, regmap, or
your own wrappers built on top of either have no bearing on whether or not
a binding is necessary.
The binding is used to define device tree properties that describe the
hardware and its constraints. Classic examples are things such as clock
frequency, regulator voltage, etc. Drivers often translate device tree
properties into register settings.
In the case of this device, the only thing the driver needs to know about
the hardware are its compatible string and I2C client address, both of
which are already supported in the common trivial devices binding [1].
> I would also like to add support for the provided interrupt pin later
> down the line which is documented in the binding along with description
> of the non-standard action button layout.
The trivial devices binding includes interrupts as well; please see [1].
My opinion is that the device's own documentation is responsible for
describing the product and anything unique about its physical layout.
> Above were my reasons for going for a standalone binding, please let me
> know if you disagree.
I don't see any need for a binding for this device because it has no vendor-
specific properties, and the only properties it does have are covered by
existing infrastructure.
My feedback is that this patch can be replaced with at most a two-line patch
to [1]. This is just my $.02; it is ultimately up to the maintainers. The
existing binding, albeit unnecessary, is nicely written either way :)
Kind regards,
Jeff LaBundy
[1] Documentation/devicetree/bindings/trivial-devices.yaml
^ permalink raw reply
* Re: [PATCH v10 0/4] Input: add initial support for Goodix Berlin touchscreen IC
From: Neil Armstrong @ 2023-10-23 15:18 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bastien Nocera,
Hans de Goede, Henrik Rydberg, Jeff LaBundy, linux-arm-msm,
devicetree, linux-kernel, Rob Herring
In-Reply-To: <20231023-topic-goodix-berlin-upstream-initial-v10-0-59066cf1f56f@linaro.org>
Hi,
Please ignore this cover letter, gmail's SMTP returned an error while sending it
but still sent it....
The properly sent patchset can be found at:
https://lore.kernel.org/all/20231023-topic-goodix-berlin-upstream-initial-v10-0-88eec2e51c0b@linaro.org/
Neil
On 23/10/2023 17:03, Neil Armstrong wrote:
> These touchscreen ICs support SPI, I2C and I3C interface, up to
> 10 finger touch, stylus and gestures events.
>
> This initial driver is derived from the Goodix goodix_ts_berlin
> available at [1] and [2] and only supports the GT9916 IC
> present on the Qualcomm SM8550 MTP & QRD touch panel.
>
> The current implementation only supports BerlinD, aka GT9916.
>
> Support for advanced features like:
> - Firmware & config update
> - Stylus events
> - Gestures events
> - Previous revisions support (BerlinA or BerlinB)
> is not included in current version.
>
> The current support will work with currently flashed firmware
> and config, and bail out if firmware or config aren't flashed yet.
>
> [1] https://github.com/goodix/goodix_ts_berlin
> [2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers
>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
> Changes in v10:
> - Fix according to Dmitry's review:
> - move goodix_berlin_get_ic_info() afe_data to heap
> - merge the goodix_berlin_parse_finger() loops and skip invalid fingers instead of returning
> - remove unwanted goodix_berlin_touch_handler() "static" for buffer
> - only call goodix_berlin_request_handle_reset() if gpio was provided
> - use "error = func(); if(error) return error;" instead of "return func()" when function handles multiple error cases
> - Link to v9: https://lore.kernel.org/r/20231021-topic-goodix-berlin-upstream-initial-v9-0-13fb4e887156@linaro.org
>
> Changes in v9:
> - Rebased on next-20231020
> - Link to v8: https://lore.kernel.org/r/20231003-topic-goodix-berlin-upstream-initial-v8-0-171606102ed6@linaro.org
>
> Changes in v8:
> - Add missing bitfield.h include in core
> - Link to v7: https://lore.kernel.org/r/20231002-topic-goodix-berlin-upstream-initial-v7-0-792fb91f5e88@linaro.org
>
> Changes in v7:
> - rebased on v6.6-rc3
> - Link to v6: https://lore.kernel.org/r/20230912-topic-goodix-berlin-upstream-initial-v6-0-b4ecfa49fb9d@linaro.org
>
> Changes in v6:
> - rebased on v6.6-rc1
> - changed commit message prefix to match the other Input commits
> - Link to v5: https://lore.kernel.org/r/20230801-topic-goodix-berlin-upstream-initial-v5-0-079252935593@linaro.org
>
> Changes in v5:
> - rebased on next-20230801
> - Link to v4: https://lore.kernel.org/r/20230606-topic-goodix-berlin-upstream-initial-v4-0-0947c489be17@linaro.org
>
> Changes in v4:
> - Core updates:
> - drop kconfig depends, deps will be handled by _SPI and _I2C
> - change power_on() error labels
> - print errors on all dev_err() prints
> - remove useless default variable initialization
> - switch irq touch checksum error to dev_err()
> - add Jeff's review tag
> - I2C changes
> - change REGMAP_I2C Kconfig from depends to select
> - add Jeff's review tag
> - SPI changes
> - add select REGMAP to Kconfig
> - added GOODIX_BERLIN_ prefix to defines
> - switched from ret to error
> - add Jeff's review tag
> - Link to v3: https://lore.kernel.org/r/20230606-topic-goodix-berlin-upstream-initial-v3-0-f0577cead709@linaro.org
>
> Changes in v3:
> - Another guge cleanups after Jeff's review:
> - appended goodix_berlin_ before all defines
> - removed some unused defines
> - removed retries on most of read functions, can be added back later
> - added __le to ic_info structures
> - reworked and simplified irq handling, dropped enum and ts_event structs
> - added struct for touch data
> - simplified and cleaned goodix_berlin_check_checksum & goodix_berlin_is_dummy_data
> - moved touch_data_addr to the end of the main code_data
> - reworked probe to get_irq last and right before setip input device
> - cleaned probe by removing the "cd->dev"
> - added short paragraph to justify new driver for berlin devices
> - defined all offsets & masks
> - Added bindings review tag
> - Link to v2: https://lore.kernel.org/r/20230606-topic-goodix-berlin-upstream-initial-v2-0-26bc8fe1e90e@linaro.org
>
> Changes in v2:
> - Huge cleanups after Jeff's review:
> - switch to error instead of ret
> - drop dummy vendor/product ids
> - drop unused defined/enums
> - drop unused ic_info and only keep needes values
> - cleanup namings and use goodix_berlin_ everywhere
> - fix regulator setup
> - fix default variables value when assigned afterwars
> - removed indirections
> - dropped debugfs
> - cleaned input_dev setup
> - dropped _remove()
> - sync'ed i2c and spi drivers
> - fixed yaml bindings
> - Link to v1: https://lore.kernel.org/r/20230606-topic-goodix-berlin-upstream-initial-v1-0-4a0741b8aefd@linaro.org
>
> ---
> Neil Armstrong (4):
> dt-bindings: input: document Goodix Berlin Touchscreen IC
> Input: add core support for Goodix Berlin Touchscreen IC
> Input: goodix-berlin - add I2C support for Goodix Berlin Touchscreen IC
> Input: goodix-berlin - add SPI support for Goodix Berlin Touchscreen IC
>
> .../bindings/input/touchscreen/goodix,gt9916.yaml | 95 ++++
> drivers/input/touchscreen/Kconfig | 31 ++
> drivers/input/touchscreen/Makefile | 3 +
> drivers/input/touchscreen/goodix_berlin.h | 159 ++++++
> drivers/input/touchscreen/goodix_berlin_core.c | 594 +++++++++++++++++++++
> drivers/input/touchscreen/goodix_berlin_i2c.c | 74 +++
> drivers/input/touchscreen/goodix_berlin_spi.c | 177 ++++++
> 7 files changed, 1133 insertions(+)
> ---
> base-commit: 2030579113a1b1b5bfd7ff24c0852847836d8fd1
> change-id: 20230606-topic-goodix-berlin-upstream-initial-ba97e8ec8f4c
>
> Best regards,
^ permalink raw reply
* [PATCH v10 4/4] Input: goodix-berlin - add SPI support for Goodix Berlin Touchscreen IC
From: Neil Armstrong @ 2023-10-23 15:03 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bastien Nocera,
Hans de Goede, Henrik Rydberg, Jeff LaBundy, linux-arm-msm,
devicetree, linux-kernel, Neil Armstrong
In-Reply-To: <20231023-topic-goodix-berlin-upstream-initial-v10-0-88eec2e51c0b@linaro.org>
Add initial support for the new Goodix "Berlin" touchscreen ICs
over the SPI interface.
The driver doesn't use the regmap_spi code since the SPI messages
needs to be prefixed, thus this custom regmap code.
This initial driver is derived from the Goodix goodix_ts_berlin
available at [1] and [2] and only supports the GT9916 IC
present on the Qualcomm SM8550 MTP & QRD touch panel.
The current implementation only supports BerlinD, aka GT9916.
[1] https://github.com/goodix/goodix_ts_berlin
[2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
drivers/input/touchscreen/Kconfig | 14 ++
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/goodix_berlin_spi.c | 177 ++++++++++++++++++++++++++
3 files changed, 192 insertions(+)
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index cc7b88118158..c821fe3ee794 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -433,6 +433,20 @@ config TOUCHSCREEN_GOODIX_BERLIN_I2C
To compile this driver as a module, choose M here: the
module will be called goodix_berlin_i2c.
+config TOUCHSCREEN_GOODIX_BERLIN_SPI
+ tristate "Goodix Berlin SPI touchscreen"
+ depends on SPI_MASTER
+ select REGMAP
+ select TOUCHSCREEN_GOODIX_BERLIN_CORE
+ help
+ Say Y here if you have a Goodix Berlin IC connected to
+ your system via SPI.
+
+ If unsure, say N.
+
+ To compile this driver as a module, choose M here: the
+ module will be called goodix_berlin_spi.
+
config TOUCHSCREEN_HIDEEP
tristate "HiDeep Touch IC"
depends on I2C
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 7ef677cf7a10..a81cb5aa21a5 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -49,6 +49,7 @@ obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_CORE) += goodix_berlin_core.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_I2C) += goodix_berlin_i2c.o
+obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_SPI) += goodix_berlin_spi.o
obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
diff --git a/drivers/input/touchscreen/goodix_berlin_spi.c b/drivers/input/touchscreen/goodix_berlin_spi.c
new file mode 100644
index 000000000000..502b143e9e05
--- /dev/null
+++ b/drivers/input/touchscreen/goodix_berlin_spi.c
@@ -0,0 +1,177 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Goodix Berlin Touchscreen Driver
+ *
+ * Copyright (C) 2020 - 2021 Goodix, Inc.
+ * Copyright (C) 2023 Linaro Ltd.
+ *
+ * Based on goodix_ts_berlin driver.
+ */
+#include <asm/unaligned.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/spi/spi.h>
+
+#include "goodix_berlin.h"
+
+#define GOODIX_BERLIN_SPI_TRANS_PREFIX_LEN 1
+#define GOODIX_BERLIN_REGISTER_WIDTH 4
+#define GOODIX_BERLIN_SPI_READ_DUMMY_LEN 3
+#define GOODIX_BERLIN_SPI_READ_PREFIX_LEN (GOODIX_BERLIN_SPI_TRANS_PREFIX_LEN + \
+ GOODIX_BERLIN_REGISTER_WIDTH + \
+ GOODIX_BERLIN_SPI_READ_DUMMY_LEN)
+#define GOODIX_BERLIN_SPI_WRITE_PREFIX_LEN (GOODIX_BERLIN_SPI_TRANS_PREFIX_LEN + \
+ GOODIX_BERLIN_REGISTER_WIDTH)
+
+#define GOODIX_BERLIN_SPI_WRITE_FLAG 0xF0
+#define GOODIX_BERLIN_SPI_READ_FLAG 0xF1
+
+static int goodix_berlin_spi_read(void *context, const void *reg_buf,
+ size_t reg_size, void *val_buf,
+ size_t val_size)
+{
+ struct spi_device *spi = context;
+ struct spi_transfer xfers;
+ struct spi_message spi_msg;
+ const u32 *reg = reg_buf; /* reg is stored as native u32 at start of buffer */
+ u8 *buf;
+ int error;
+
+ if (reg_size != GOODIX_BERLIN_REGISTER_WIDTH)
+ return -EINVAL;
+
+ buf = kzalloc(GOODIX_BERLIN_SPI_READ_PREFIX_LEN + val_size, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ spi_message_init(&spi_msg);
+ memset(&xfers, 0, sizeof(xfers));
+
+ /* buffer format: 0xF1 + addr(4bytes) + dummy(3bytes) + data */
+ buf[0] = GOODIX_BERLIN_SPI_READ_FLAG;
+ put_unaligned_be32(*reg, buf + GOODIX_BERLIN_SPI_TRANS_PREFIX_LEN);
+ memset(buf + GOODIX_BERLIN_SPI_TRANS_PREFIX_LEN + GOODIX_BERLIN_REGISTER_WIDTH,
+ 0xff, GOODIX_BERLIN_SPI_READ_DUMMY_LEN);
+
+ xfers.tx_buf = buf;
+ xfers.rx_buf = buf;
+ xfers.len = GOODIX_BERLIN_SPI_READ_PREFIX_LEN + val_size;
+ xfers.cs_change = 0;
+ spi_message_add_tail(&xfers, &spi_msg);
+
+ error = spi_sync(spi, &spi_msg);
+ if (error < 0)
+ dev_err(&spi->dev, "spi transfer error, %d", error);
+ else
+ memcpy(val_buf, buf + GOODIX_BERLIN_SPI_READ_PREFIX_LEN, val_size);
+
+ kfree(buf);
+ return error;
+}
+
+static int goodix_berlin_spi_write(void *context, const void *data,
+ size_t count)
+{
+ unsigned int len = count - GOODIX_BERLIN_REGISTER_WIDTH;
+ struct spi_device *spi = context;
+ struct spi_transfer xfers;
+ struct spi_message spi_msg;
+ const u32 *reg = data; /* reg is stored as native u32 at start of buffer */
+ u8 *buf;
+ int error;
+
+ buf = kzalloc(GOODIX_BERLIN_SPI_WRITE_PREFIX_LEN + len, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ spi_message_init(&spi_msg);
+ memset(&xfers, 0, sizeof(xfers));
+
+ buf[0] = GOODIX_BERLIN_SPI_WRITE_FLAG;
+ put_unaligned_be32(*reg, buf + GOODIX_BERLIN_SPI_TRANS_PREFIX_LEN);
+ memcpy(buf + GOODIX_BERLIN_SPI_WRITE_PREFIX_LEN,
+ data + GOODIX_BERLIN_REGISTER_WIDTH, len);
+
+ xfers.tx_buf = buf;
+ xfers.len = GOODIX_BERLIN_SPI_WRITE_PREFIX_LEN + len;
+ xfers.cs_change = 0;
+ spi_message_add_tail(&xfers, &spi_msg);
+
+ error = spi_sync(spi, &spi_msg);
+ if (error < 0)
+ dev_err(&spi->dev, "spi transfer error, %d", error);
+
+ kfree(buf);
+ return error;
+}
+
+static const struct regmap_config goodix_berlin_spi_regmap_conf = {
+ .reg_bits = 32,
+ .val_bits = 8,
+ .read = goodix_berlin_spi_read,
+ .write = goodix_berlin_spi_write,
+};
+
+/* vendor & product left unassigned here, should probably be updated from fw info */
+static const struct input_id goodix_berlin_spi_input_id = {
+ .bustype = BUS_SPI,
+};
+
+static int goodix_berlin_spi_probe(struct spi_device *spi)
+{
+ struct regmap_config regmap_config;
+ struct regmap *regmap;
+ size_t max_size;
+ int error = 0;
+
+ spi->mode = SPI_MODE_0;
+ spi->bits_per_word = 8;
+ error = spi_setup(spi);
+ if (error)
+ return error;
+
+ max_size = spi_max_transfer_size(spi);
+
+ regmap_config = goodix_berlin_spi_regmap_conf;
+ regmap_config.max_raw_read = max_size - GOODIX_BERLIN_SPI_READ_PREFIX_LEN;
+ regmap_config.max_raw_write = max_size - GOODIX_BERLIN_SPI_WRITE_PREFIX_LEN;
+
+ regmap = devm_regmap_init(&spi->dev, NULL, spi, ®map_config);
+ if (IS_ERR(regmap))
+ return PTR_ERR(regmap);
+
+ error = goodix_berlin_probe(&spi->dev, spi->irq,
+ &goodix_berlin_spi_input_id, regmap);
+ if (error)
+ return error;
+
+ return 0;
+}
+
+static const struct spi_device_id goodix_berlin_spi_ids[] = {
+ { "gt9916" },
+ { },
+};
+MODULE_DEVICE_TABLE(spi, goodix_berlin_spi_ids);
+
+static const struct of_device_id goodix_berlin_spi_of_match[] = {
+ { .compatible = "goodix,gt9916", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, goodix_berlin_spi_of_match);
+
+static struct spi_driver goodix_berlin_spi_driver = {
+ .driver = {
+ .name = "goodix-berlin-spi",
+ .of_match_table = goodix_berlin_spi_of_match,
+ .pm = pm_sleep_ptr(&goodix_berlin_pm_ops),
+ },
+ .probe = goodix_berlin_spi_probe,
+ .id_table = goodix_berlin_spi_ids,
+};
+module_spi_driver(goodix_berlin_spi_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Goodix Berlin SPI Touchscreen driver");
+MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
--
2.34.1
^ permalink raw reply related
* Re: [PATCH] HID: fix a crash in hid_debug_events_release
From: Rahul Rameshbabu @ 2023-10-23 15:03 UTC (permalink / raw)
To: Charles Yi; +Cc: jikos, benjamin.tissoires, linux-input, linux-kernel
In-Reply-To: <20231023093500.1391443-1-be286@163.com>
Hi Charles,
On Mon, 23 Oct, 2023 17:35:00 +0800 "Charles Yi" <be286@163.com> wrote:
> hid_debug_events_release() access released memory by
> hid_device_release(). This is fixed by the patch.
>
A couple of things here. Can you add a Fixes: tag?
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
Before any v2 however, it would be nice to understand where this issue
is coming from. I am wondering if it's really a core issue or rather an
issue with a higher level device specific driver making use of the hid
subsystem. I am having a hard time seeing how this issue occurs
currently. A stack trace in a follow-up email to this one pertaining to
the crash would be helpful. If you are resolving a syzbot report, a link
to that report would suffice.
This patch doesn't make a lot of sense to me as-is because
hid_debug_events_release is about release resources related to hid debug
events (at least from my current understanding). It should not be
free-ing the underlying hid device/instance itself.
> Signed-off-by: Charles Yi <be286@163.com>
> ---
> drivers/hid/hid-core.c | 12 ++++++++++--
> drivers/hid/hid-debug.c | 3 +++
> include/linux/hid.h | 3 +++
> 3 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 8992e3c1e769..e0181218ad85 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -702,15 +702,22 @@ static void hid_close_report(struct hid_device *device)
> * Free a device structure, all reports, and all fields.
> */
>
> -static void hid_device_release(struct device *dev)
> +void hiddev_free(struct kref *ref)
> {
> - struct hid_device *hid = to_hid_device(dev);
> + struct hid_device *hid = container_of(ref, struct hid_device, ref);
>
> hid_close_report(hid);
> kfree(hid->dev_rdesc);
> kfree(hid);
> }
>
> +static void hid_device_release(struct device *dev)
> +{
> + struct hid_device *hid = to_hid_device(dev);
> +
> + kref_put(&hid->ref, hiddev_free);
> +}
> +
> /*
> * Fetch a report description item from the data stream. We support long
> * items, though they are not used yet.
> @@ -2846,6 +2853,7 @@ struct hid_device *hid_allocate_device(void)
> spin_lock_init(&hdev->debug_list_lock);
> sema_init(&hdev->driver_input_lock, 1);
> mutex_init(&hdev->ll_open_lock);
> + kref_init(&hdev->ref);
>
> hid_bpf_device_init(hdev);
>
> diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
> index e7ef1ea107c9..7dd83ec74f8a 100644
> --- a/drivers/hid/hid-debug.c
> +++ b/drivers/hid/hid-debug.c
> @@ -1135,6 +1135,7 @@ static int hid_debug_events_open(struct inode *inode, struct file *file)
> goto out;
> }
> list->hdev = (struct hid_device *) inode->i_private;
> + kref_get(&list->hdev->ref);
> file->private_data = list;
> mutex_init(&list->read_mutex);
>
> @@ -1227,6 +1228,8 @@ static int hid_debug_events_release(struct inode *inode, struct file *file)
> list_del(&list->node);
> spin_unlock_irqrestore(&list->hdev->debug_list_lock, flags);
> kfifo_free(&list->hid_debug_fifo);
> +
> + kref_put(&list->hdev->ref, hiddev_free);
> kfree(list);
>
> return 0;
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 964ca1f15e3f..3b08a2957229 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -679,6 +679,7 @@ struct hid_device { /* device report descriptor */
> struct list_head debug_list;
> spinlock_t debug_list_lock;
> wait_queue_head_t debug_wait;
> + struct kref ref;
>
> unsigned int id; /* system unique id */
>
> @@ -687,6 +688,8 @@ struct hid_device { /* device report descriptor */
> #endif /* CONFIG_BPF */
> };
>
> +void hiddev_free(struct kref *ref);
> +
> #define to_hid_device(pdev) \
> container_of(pdev, struct hid_device, dev)
--
Thank you for the patch,
Rahul Rameshbabu
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox