* Re: [PATCH v2 1/2] dt-bindings: input: touchscreen: Add ilitek 9882T touchscreen chip
From: Rob Herring @ 2023-06-09 22:03 UTC (permalink / raw)
To: cong yang
Cc: Conor Dooley, krzysztof.kozlowski+dt, conor+dt, dmitry.torokhov,
jikos, benjamin.tissoires, dianders, hsinyi, linux-input,
devicetree, linux-kernel
In-Reply-To: <CAHwB_NK_j1SJ1BBkVqafFM_+fWSyvwjCpMmHQxjLjnz_KHR=KA@mail.gmail.com>
On Tue, Jun 06, 2023 at 10:06:05AM +0800, cong yang wrote:
> Hi,Conor,
>
> On Mon, Jun 5, 2023 at 6:20 PM Conor Dooley <conor@kernel.org> wrote:
> >
> > Hey Cong Yang,
> >
> > On Mon, Jun 05, 2023 at 02:05:23PM +0800, Cong Yang wrote:
> > > Add an ilitek touch screen chip ili9882t.
> >
> > Could you add a comment here mentioning the relationship between these
> > chips?
>
> Okay, I will add in V3 version.
>
> > On Mon, Jun 05, 2023 at 02:05:23PM +0800, Cong Yang wrote:
> >
> > > Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
> > > ---
> > > .../bindings/input/elan,ekth6915.yaml | 23 ++++++++++++++++---
> > > 1 file changed, 20 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
> > > index 05e6f2df604c..f0e7ffdce605 100644
> > > --- a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
> > > +++ b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
> > > @@ -15,11 +15,14 @@ description:
> > >
> > > properties:
> > > compatible:
> > > - items:
> > > - - const: elan,ekth6915
> > > + enum:
> > > + - elan,ekth6915
> > > + - ilitek,ili9882t
> > >
> > > reg:
> > > - const: 0x10
> > > + enum:
> > > + - 0x10
> > > + - 0x41
> >
> > Is 0x10 only valid for the elan,ekth6915 & 0x41 for the ilitek one?
> > If so, please add some enforcement of the values based on the
> > compatible.
>
> I don't think 0x10 is the only address for ekth6915,(nor is 0x41 the
> only address for ili9882t). It depends on the hardware design.
I'd just drop the values as we don't typically enforce 'reg' values.
Rob
^ permalink raw reply
* čestitam
From: Alice Walton @ 2023-06-08 17:18 UTC (permalink / raw)
To: linux-input
čestitam,
Vaša e-pošta osvojila je 5.000.000,00 USD od strane Alice
Walton 2023 humanitarnog programa podrške donacijama.
Kontaktirajte nas za više pojedinosti o tome kako možete
zatražiti svoj donacijski fond.
Hvala i Bog blagoslovio
Zaklada Alice Walton
=========================================
Congratulation,
Your email has won $5,000,000.00 USD by Alice Walton 2023
humanitarian donation support program. Contact us for more
details on how you can claim your donation fund.
Thanks And God Bless
Alice Walton Foundation
^ permalink raw reply
* Re: [BUG: 6.3 kernel] Logitech Trackball M575 misidentified
From: Xose Vazquez Perez @ 2023-06-10 22:46 UTC (permalink / raw)
To: Bastien Nocera, Linux regressions mailing list, linux-input
Cc: Filipe Laíns, Jiri Kosina, Benjamin Tissoires, LKML
In-Reply-To: <8308180826ba9a5478bf568396034b8dc7fb6e72.camel@hadess.net>
On 5/31/23 10:50, Bastien Nocera wrote:
> The device name problem is tracked in 217330, which I filed.
>
> A bisection would definitely help me if you have time, otherwise I'll
> get to it, but it's probably not going to be before a couple of weeks.
>
> You can also test this patch on top of the latest kernel tree:
> https://patchwork.kernel.org/project/linux-input/patch/20230531082428.21763-1-hadess@hadess.net/
> although I don't expect it to make a difference.
It was fixed in 6.3.7.
Thank you.
^ permalink raw reply
* [PATCH] hid-mcp2200 added driver for MCP2200 GPIOs
From: Johannes Roith @ 2023-06-11 14:48 UTC (permalink / raw)
To: jikos; +Cc: linux-kernel, linux-input, Johannes Roith
Added a gpiochip compatible driver to control the 8 GPIOs of the MCP2200
by using the HID interface.
Using GPIOs with alternative functions (GP0<->SSPND, GP1<->USBCFG,
GP6<->RXLED, GP7<->TXLED) will reset the functions, if set (unset by
default).
The driver was tested while also using the UART of the chip. Setting
and reading the GPIOs has no effect on the UART communication. However,
a reset is triggered after the CONFIGURE command. If the GPIO Direction
is constantly changed, this will affect the communication at low baud
rates. This is a hardware problem of the MCP2200 and is not caused by
the driver.
Signed-off-by: Johannes Roith <johannes@gnu-linux.rocks>
---
drivers/hid/Kconfig | 10 +
drivers/hid/Makefile | 1 +
drivers/hid/hid-mcp2200.c | 421 ++++++++++++++++++++++++++++++++++++++
3 files changed, 432 insertions(+)
create mode 100644 drivers/hid/hid-mcp2200.c
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 4ce012f83253..02115dcc6b0b 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -1283,6 +1283,16 @@ config HID_MCP2221
To compile this driver as a module, choose M here: the module
will be called hid-mcp2221.ko.
+config HID_MCP2200
+ tristate "Microchip MCP2200 HID USB-to-GPIO bridge"
+ depends on USB_HID
+ imply GPIOLIB
+ help
+ Provides GPIO functionality over USB-HID through MCP2200 device.
+
+ To compile this driver as a module, choose M here: the module
+ will be called hid-mcp2200.ko.
+
config HID_KUNIT_TEST
tristate "KUnit tests for HID" if !KUNIT_ALL_TESTS
depends on KUNIT=y
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 5d37cacbde33..d593fb982f7d 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -79,6 +79,7 @@ obj-$(CONFIG_HID_MACALLY) += hid-macally.o
obj-$(CONFIG_HID_MAGICMOUSE) += hid-magicmouse.o
obj-$(CONFIG_HID_MALTRON) += hid-maltron.o
obj-$(CONFIG_HID_MCP2221) += hid-mcp2221.o
+obj-$(CONFIG_HID_MCP2200) += hid-mcp2200.o
obj-$(CONFIG_HID_MAYFLASH) += hid-mf.o
obj-$(CONFIG_HID_MEGAWORLD_FF) += hid-megaworld.o
obj-$(CONFIG_HID_MICROSOFT) += hid-microsoft.o
diff --git a/drivers/hid/hid-mcp2200.c b/drivers/hid/hid-mcp2200.c
new file mode 100644
index 000000000000..600f3f3e073b
--- /dev/null
+++ b/drivers/hid/hid-mcp2200.c
@@ -0,0 +1,421 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * MCP2200 - Microchip USB to GPIO bridge
+ *
+ * Copyright (c) 2023, Johannes Roith <johannes@gnu-linux.rocks>
+ *
+ * Datasheet: https://ww1.microchip.com/downloads/en/DeviceDoc/22228A.pdf
+ * App Note for HID: https://ww1.microchip.com/downloads/en/DeviceDoc/93066A.pdf
+ */
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/bitfield.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/hid.h>
+#include <linux/hidraw.h>
+#include <linux/gpio/driver.h>
+#include "hid-ids.h"
+
+/* Commands codes in a raw output report */
+enum {
+ SET_CLEAR_OUTPUTS = 0x08,
+ CONFIGURE = 0x10,
+ READ_EE = 0x20,
+ WRITE_EE = 0x40,
+ READ_ALL = 0x80
+};
+
+/* MCP GPIO direction encoding */
+enum MCP_IO_DIR {
+ MCP2200_DIR_OUT = 0x00,
+ MCP2200_DIR_IN = 0x01,
+};
+
+/* Altternative pin assignments */
+enum {
+ TXLED = 2,
+ RXLED = 3,
+ USBCFG = 6,
+ SSPND = 7,
+
+};
+
+#define MCP_NGPIO 8
+
+/* CMD to set or clear a GPIO output */
+struct mcp_set_clear_outputs {
+ u8 cmd;
+ u8 dummys1[10];
+ u8 set_bmap;
+ u8 clear_bmap;
+ u8 dummys2[3];
+} __packed;
+
+/* CMD to configure the IOs */
+struct mcp_configure {
+ u8 cmd;
+ u8 dummys1[3];
+ u8 io_bmap;
+ u8 config_alt_pins;
+ u8 io_default_val_bmap;
+ u8 config_alt_options;
+ u8 baud_h;
+ u8 baud_l;
+ u8 dummys2[6];
+} __packed;
+
+/* CMD to read all parameters */
+struct mcp_read_all {
+ u8 cmd;
+ u8 dummys[15];
+} __packed;
+
+/* Response to the read all cmd */
+struct mcp_read_all_resp {
+ u8 cmd;
+ u8 eep_addr;
+ u8 dummy;
+ u8 eep_val;
+ u8 io_bmap;
+ u8 config_alt_pins;
+ u8 io_default_val_bmap;
+ u8 config_alt_options;
+ u8 baud_h;
+ u8 baud_l;
+ u8 io_port_val_bmap;
+ u8 dummys[5];
+} __packed;
+
+struct mcp2200 {
+ struct hid_device *hdev;
+ struct mutex lock;
+ struct completion wait_in_report;
+ u8 gpio_dir;
+ u8 gpio_val;
+ u8 gpio_inval;
+ u8 baud_h;
+ u8 baud_l;
+ u8 config_alt_pins;
+ u8 gpio_reset_val;
+ u8 config_alt_options;
+ int status;
+ struct gpio_chip gc;
+};
+
+/* this executes the READ_ALL cmd */
+static int mcp_cmd_read_all(struct mcp2200 *mcp)
+{
+ struct mcp_read_all *read_all;
+ int len, t;
+
+ reinit_completion(&mcp->wait_in_report);
+ mutex_lock(&mcp->lock);
+
+ read_all = kzalloc(sizeof(struct mcp_read_all), GFP_KERNEL);
+ if (!read_all)
+ return -ENOMEM;
+
+ read_all->cmd = READ_ALL;
+ len = hid_hw_output_report(mcp->hdev, (u8 *) read_all,
+ sizeof(struct mcp_read_all));
+
+ if (len != sizeof(struct mcp_read_all))
+ return -EINVAL;
+
+ kfree(read_all);
+ mutex_unlock(&mcp->lock);
+ t = wait_for_completion_timeout(&mcp->wait_in_report, msecs_to_jiffies(4000));
+ if (!t)
+ return -ETIMEDOUT;
+
+ /* return status, negative value if wrong response was received */
+ return mcp->status;
+}
+
+static void mcp_set_multiple(struct gpio_chip *gc, unsigned long *mask,
+ unsigned long *bits)
+{
+ struct mcp2200 *mcp = gpiochip_get_data(gc);
+ u8 value;
+ int status;
+ struct mcp_set_clear_outputs *cmd;
+
+ cmd = kzalloc(sizeof(struct mcp_set_clear_outputs), GFP_KERNEL);
+ if (!cmd)
+ return;
+
+ mutex_lock(&mcp->lock);
+
+ value = mcp->gpio_val & ~*mask;
+ value |= (*mask & *bits);
+
+ cmd->cmd = SET_CLEAR_OUTPUTS;
+ cmd->set_bmap = value;
+ cmd->clear_bmap = ~(value);
+
+ status = hid_hw_output_report(mcp->hdev, (u8 *) cmd,
+ sizeof(struct mcp_set_clear_outputs));
+ if (status == sizeof(struct mcp_set_clear_outputs))
+ mcp->gpio_val = value;
+
+ kfree(cmd);
+ mutex_unlock(&mcp->lock);
+}
+
+static void mcp_set(struct gpio_chip *gc, unsigned int gpio_nr, int value)
+{
+ unsigned long mask = (1 << gpio_nr);
+ unsigned long bmap_value = (value<<gpio_nr);
+
+ mcp_set_multiple(gc, &mask, &bmap_value);
+}
+
+static int mcp_get_multiple(struct gpio_chip *gc, unsigned long *mask,
+ unsigned long *bits)
+{
+ u32 val;
+ struct mcp2200 *mcp = gpiochip_get_data(gc);
+ int status;
+
+ status = mcp_cmd_read_all(mcp);
+ if (status != 0)
+ return status;
+
+ val = mcp->gpio_inval;
+ *bits = (val & *mask);
+ return 0;
+}
+
+static int mcp_get(struct gpio_chip *gc, unsigned int gpio_nr)
+{
+ unsigned long mask = 0, bits = 0;
+
+ mask = (1 << gpio_nr);
+ mcp_get_multiple(gc, &mask, &bits);
+ return (bits > 0) ? 1 : 0;
+}
+
+static int mcp_get_direction(struct gpio_chip *gc, unsigned int gpio_nr)
+{
+ struct mcp2200 *mcp = gpiochip_get_data(gc);
+
+ return (mcp->gpio_dir & (MCP2200_DIR_IN << gpio_nr))
+ ? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT;
+}
+
+static int mcp_set_direction(struct gpio_chip *gc, unsigned int gpio_nr,
+ enum MCP_IO_DIR io_direction)
+{
+ struct mcp2200 *mcp = gpiochip_get_data(gc);
+ struct mcp_configure *conf;
+ int status;
+ /* after the configure cmd we will need to set the outputs again */
+ unsigned long mask = ~(mcp->gpio_dir); /* only set outputs */
+ unsigned long bits = mcp->gpio_val;
+ /* Offsets of alternative pins in config_alt_pins, 0 is not used */
+ u8 alt_pin_conf[8] = {SSPND, USBCFG, 0, 0, 0, 0, RXLED, TXLED};
+ u8 config_alt_pins = mcp->config_alt_pins;
+
+ /* Read in the reset baudrate first, we need it later */
+ status = mcp_cmd_read_all(mcp);
+ if (status != 0)
+ return status;
+
+ conf = kzalloc(sizeof(struct mcp_configure), GFP_KERNEL);
+ if (!conf)
+ return -ENOMEM;
+ mutex_lock(&mcp->lock);
+
+ /* configure will reset the chip! */
+ conf->cmd = CONFIGURE;
+ conf->io_bmap = (mcp->gpio_dir & ~(1 << gpio_nr))
+ | (io_direction << gpio_nr);
+ /* Don't overwrite the reset parameters */
+ conf->baud_h = mcp->baud_h;
+ conf->baud_l = mcp->baud_l;
+ conf->config_alt_options = mcp->config_alt_options;
+ conf->io_default_val_bmap = mcp->gpio_reset_val;
+ /* Adjust alt. func if necessary */
+ if (alt_pin_conf[gpio_nr])
+ config_alt_pins &= ~(1 << alt_pin_conf[gpio_nr]);
+ conf->config_alt_pins = config_alt_pins;
+
+ status = hid_hw_output_report(mcp->hdev, (u8 *) conf,
+ sizeof(struct mcp_set_clear_outputs));
+ if (status == sizeof(struct mcp_set_clear_outputs)) {
+ mcp->gpio_dir &= ~(1 << gpio_nr);
+ mcp->config_alt_pins = config_alt_pins;
+ } else {
+ return -EIO;
+ }
+
+ kfree(conf);
+ mutex_unlock(&mcp->lock);
+
+ /* Configure CMD will clear all IOs -> rewrite them */
+ mcp_set_multiple(gc, &mask, &bits);
+ return 0;
+}
+
+static int mcp_direction_input(struct gpio_chip *gc, unsigned int gpio_nr)
+{
+ return mcp_set_direction(gc, gpio_nr, MCP2200_DIR_IN);
+}
+
+static int mcp_direction_output(struct gpio_chip *gc, unsigned int gpio_nr,
+ int value)
+{
+ int ret;
+ unsigned long mask, bmap_value;
+
+ mask = (1 << gpio_nr);
+ bmap_value = (value << gpio_nr);
+
+ ret = mcp_set_direction(gc, gpio_nr, MCP2200_DIR_OUT);
+ if (ret == 0)
+ mcp_set_multiple(gc, &mask, &bmap_value);
+ return ret;
+}
+
+
+static const struct gpio_chip template_chip = {
+ .label = "mcp2200",
+ .owner = THIS_MODULE,
+ .get_direction = mcp_get_direction,
+ .direction_input = mcp_direction_input,
+ .direction_output = mcp_direction_output,
+ .set = mcp_set,
+ .set_multiple = mcp_set_multiple,
+ .get = mcp_get,
+ .get_multiple = mcp_get_multiple,
+ .base = -1,
+ .ngpio = MCP_NGPIO,
+ .can_sleep = true,
+};
+
+/*
+ * MCP2200 uses interrupt endpoint for input reports. This function
+ * is called by HID layer when it receives i/p report from mcp2200,
+ * which is actually a response to the previously sent command.
+ */
+static int mcp2200_raw_event(struct hid_device *hdev, struct hid_report *report,
+ u8 *data, int size)
+{
+ struct mcp2200 *mcp = hid_get_drvdata(hdev);
+ struct mcp_read_all_resp *all_resp;
+
+ switch (data[0]) {
+ case READ_ALL:
+ all_resp = (struct mcp_read_all_resp *) data;
+ mcp->status = 0;
+ mcp->gpio_inval = all_resp->io_port_val_bmap;
+ mcp->baud_h = all_resp->baud_h;
+ mcp->baud_l = all_resp->baud_l;
+ mcp->gpio_reset_val = all_resp->io_default_val_bmap;
+ mcp->config_alt_pins = all_resp->config_alt_pins;
+ mcp->config_alt_options = all_resp->config_alt_options;
+ break;
+ default:
+ mcp->status = -EIO;
+ break;
+ }
+
+ complete(&mcp->wait_in_report);
+ return 1;
+}
+
+static void mcp2200_hid_unregister(void *ptr)
+{
+ struct hid_device *hdev = ptr;
+
+ hid_hw_close(hdev);
+ hid_hw_stop(hdev);
+}
+
+static int mcp2200_probe(struct hid_device *hdev, const struct hid_device_id *id)
+{
+ int ret;
+ struct mcp2200 *mcp;
+
+ mcp = devm_kzalloc(&hdev->dev, sizeof(*mcp), GFP_KERNEL);
+ if (!mcp)
+ return -ENOMEM;
+
+ ret = hid_parse(hdev);
+ if (ret) {
+ hid_err(hdev, "can't parse reports\n");
+ return ret;
+ }
+
+ /*
+ * This driver uses the .raw_event callback and therefore does not need any
+ * HID_CONNECT_xxx flags.
+ */
+ ret = hid_hw_start(hdev, 0);
+ if (ret) {
+ hid_err(hdev, "can't start hardware\n");
+ return ret;
+ }
+
+ hid_info(hdev, "USB HID v%x.%02x Device [%s] on %s\n", hdev->version >> 8,
+ hdev->version & 0xff, hdev->name, hdev->phys);
+
+ ret = hid_hw_open(hdev);
+ if (ret) {
+ hid_err(hdev, "can't open device\n");
+ hid_hw_stop(hdev);
+ return ret;
+ }
+
+ mutex_init(&mcp->lock);
+ init_completion(&mcp->wait_in_report);
+ hid_set_drvdata(hdev, mcp);
+ mcp->hdev = hdev;
+
+ ret = devm_add_action_or_reset(&hdev->dev, mcp2200_hid_unregister, hdev);
+ if (ret)
+ return ret;
+
+ mcp->gc = template_chip;
+ mcp->gc.parent = &hdev->dev;
+
+ ret = gpiochip_add_data(&mcp->gc, mcp);
+ if (ret < 0) {
+ dev_err(&hdev->dev, "Unable to register gpiochip\n");
+ hid_hw_stop(hdev);
+ return ret;
+ }
+
+ return 0;
+}
+
+static void mcp2200_remove(struct hid_device *hdev)
+{
+ struct mcp2200 *mcp;
+
+ mcp = hid_get_drvdata(hdev);
+ gpiochip_remove(&mcp->gc);
+}
+
+static const struct hid_device_id mcp2200_devices[] = {
+ { HID_USB_DEVICE(USB_VENDOR_ID_MICROCHIP, USB_DEVICE_ID_MCP2200) },
+ { }
+};
+MODULE_DEVICE_TABLE(hid, mcp2200_devices);
+
+static struct hid_driver mcp2200_driver = {
+ .name = "mcp2200",
+ .id_table = mcp2200_devices,
+ .probe = mcp2200_probe,
+ .remove = mcp2200_remove,
+ .raw_event = mcp2200_raw_event,
+};
+
+/* Register with HID core */
+module_hid_driver(mcp2200_driver);
+
+MODULE_AUTHOR("Johannes Roith <johannes@gnu-linux.rocks>");
+MODULE_DESCRIPTION("MCP2200 Microchip HID USB to GPIO bridge");
+MODULE_LICENSE("GPL");
--
2.41.0
^ permalink raw reply related
* [PATCH 1/2] hid-mcp2200 added driver for MCP2200 GPIOs
From: Johannes Roith @ 2023-06-11 16:48 UTC (permalink / raw)
To: jikos; +Cc: linux-input, linux-kernel, Johannes Roith
Added a gpiochip compatible driver to control the 8 GPIOs of the MCP2200
by using the HID interface.
Using GPIOs with alternative functions (GP0<->SSPND, GP1<->USBCFG,
GP6<->RXLED, GP7<->TXLED) will reset the functions, if set (unset by
default).
The driver was tested while also using the UART of the chip. Setting
and reading the GPIOs has no effect on the UART communication. However,
a reset is triggered after the CONFIGURE command. If the GPIO Direction
is constantly changed, this will affect the communication at low baud
rates. This is a hardware problem of the MCP2200 and is not caused by
the driver.
Signed-off-by: Johannes Roith <johannes@gnu-linux.rocks>
create mode 100644 drivers/hid/hid-mcp2200.c
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 4ce012f83253..02115dcc6b0b 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -1283,6 +1283,16 @@ config HID_MCP2221
To compile this driver as a module, choose M here: the module
will be called hid-mcp2221.ko.
+config HID_MCP2200
+ tristate "Microchip MCP2200 HID USB-to-GPIO bridge"
+ depends on USB_HID
+ imply GPIOLIB
+ help
+ Provides GPIO functionality over USB-HID through MCP2200 device.
+
+ To compile this driver as a module, choose M here: the module
+ will be called hid-mcp2200.ko.
+
config HID_KUNIT_TEST
tristate "KUnit tests for HID" if !KUNIT_ALL_TESTS
depends on KUNIT=y
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 5d37cacbde33..d593fb982f7d 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -79,6 +79,7 @@ obj-$(CONFIG_HID_MACALLY) += hid-macally.o
obj-$(CONFIG_HID_MAGICMOUSE) += hid-magicmouse.o
obj-$(CONFIG_HID_MALTRON) += hid-maltron.o
obj-$(CONFIG_HID_MCP2221) += hid-mcp2221.o
+obj-$(CONFIG_HID_MCP2200) += hid-mcp2200.o
obj-$(CONFIG_HID_MAYFLASH) += hid-mf.o
obj-$(CONFIG_HID_MEGAWORLD_FF) += hid-megaworld.o
obj-$(CONFIG_HID_MICROSOFT) += hid-microsoft.o
diff --git a/drivers/hid/hid-mcp2200.c b/drivers/hid/hid-mcp2200.c
new file mode 100644
index 000000000000..600f3f3e073b
--- /dev/null
+++ b/drivers/hid/hid-mcp2200.c
@@ -0,0 +1,421 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * MCP2200 - Microchip USB to GPIO bridge
+ *
+ * Copyright (c) 2023, Johannes Roith <johannes@gnu-linux.rocks>
+ *
+ * Datasheet: https://ww1.microchip.com/downloads/en/DeviceDoc/22228A.pdf
+ * App Note for HID: https://ww1.microchip.com/downloads/en/DeviceDoc/93066A.pdf
+ */
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/bitfield.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/hid.h>
+#include <linux/hidraw.h>
+#include <linux/gpio/driver.h>
+#include "hid-ids.h"
+
+/* Commands codes in a raw output report */
+enum {
+ SET_CLEAR_OUTPUTS = 0x08,
+ CONFIGURE = 0x10,
+ READ_EE = 0x20,
+ WRITE_EE = 0x40,
+ READ_ALL = 0x80
+};
+
+/* MCP GPIO direction encoding */
+enum MCP_IO_DIR {
+ MCP2200_DIR_OUT = 0x00,
+ MCP2200_DIR_IN = 0x01,
+};
+
+/* Altternative pin assignments */
+enum {
+ TXLED = 2,
+ RXLED = 3,
+ USBCFG = 6,
+ SSPND = 7,
+
+};
+
+#define MCP_NGPIO 8
+
+/* CMD to set or clear a GPIO output */
+struct mcp_set_clear_outputs {
+ u8 cmd;
+ u8 dummys1[10];
+ u8 set_bmap;
+ u8 clear_bmap;
+ u8 dummys2[3];
+} __packed;
+
+/* CMD to configure the IOs */
+struct mcp_configure {
+ u8 cmd;
+ u8 dummys1[3];
+ u8 io_bmap;
+ u8 config_alt_pins;
+ u8 io_default_val_bmap;
+ u8 config_alt_options;
+ u8 baud_h;
+ u8 baud_l;
+ u8 dummys2[6];
+} __packed;
+
+/* CMD to read all parameters */
+struct mcp_read_all {
+ u8 cmd;
+ u8 dummys[15];
+} __packed;
+
+/* Response to the read all cmd */
+struct mcp_read_all_resp {
+ u8 cmd;
+ u8 eep_addr;
+ u8 dummy;
+ u8 eep_val;
+ u8 io_bmap;
+ u8 config_alt_pins;
+ u8 io_default_val_bmap;
+ u8 config_alt_options;
+ u8 baud_h;
+ u8 baud_l;
+ u8 io_port_val_bmap;
+ u8 dummys[5];
+} __packed;
+
+struct mcp2200 {
+ struct hid_device *hdev;
+ struct mutex lock;
+ struct completion wait_in_report;
+ u8 gpio_dir;
+ u8 gpio_val;
+ u8 gpio_inval;
+ u8 baud_h;
+ u8 baud_l;
+ u8 config_alt_pins;
+ u8 gpio_reset_val;
+ u8 config_alt_options;
+ int status;
+ struct gpio_chip gc;
+};
+
+/* this executes the READ_ALL cmd */
+static int mcp_cmd_read_all(struct mcp2200 *mcp)
+{
+ struct mcp_read_all *read_all;
+ int len, t;
+
+ reinit_completion(&mcp->wait_in_report);
+ mutex_lock(&mcp->lock);
+
+ read_all = kzalloc(sizeof(struct mcp_read_all), GFP_KERNEL);
+ if (!read_all)
+ return -ENOMEM;
+
+ read_all->cmd = READ_ALL;
+ len = hid_hw_output_report(mcp->hdev, (u8 *) read_all,
+ sizeof(struct mcp_read_all));
+
+ if (len != sizeof(struct mcp_read_all))
+ return -EINVAL;
+
+ kfree(read_all);
+ mutex_unlock(&mcp->lock);
+ t = wait_for_completion_timeout(&mcp->wait_in_report, msecs_to_jiffies(4000));
+ if (!t)
+ return -ETIMEDOUT;
+
+ /* return status, negative value if wrong response was received */
+ return mcp->status;
+}
+
+static void mcp_set_multiple(struct gpio_chip *gc, unsigned long *mask,
+ unsigned long *bits)
+{
+ struct mcp2200 *mcp = gpiochip_get_data(gc);
+ u8 value;
+ int status;
+ struct mcp_set_clear_outputs *cmd;
+
+ cmd = kzalloc(sizeof(struct mcp_set_clear_outputs), GFP_KERNEL);
+ if (!cmd)
+ return;
+
+ mutex_lock(&mcp->lock);
+
+ value = mcp->gpio_val & ~*mask;
+ value |= (*mask & *bits);
+
+ cmd->cmd = SET_CLEAR_OUTPUTS;
+ cmd->set_bmap = value;
+ cmd->clear_bmap = ~(value);
+
+ status = hid_hw_output_report(mcp->hdev, (u8 *) cmd,
+ sizeof(struct mcp_set_clear_outputs));
+ if (status == sizeof(struct mcp_set_clear_outputs))
+ mcp->gpio_val = value;
+
+ kfree(cmd);
+ mutex_unlock(&mcp->lock);
+}
+
+static void mcp_set(struct gpio_chip *gc, unsigned int gpio_nr, int value)
+{
+ unsigned long mask = (1 << gpio_nr);
+ unsigned long bmap_value = (value<<gpio_nr);
+
+ mcp_set_multiple(gc, &mask, &bmap_value);
+}
+
+static int mcp_get_multiple(struct gpio_chip *gc, unsigned long *mask,
+ unsigned long *bits)
+{
+ u32 val;
+ struct mcp2200 *mcp = gpiochip_get_data(gc);
+ int status;
+
+ status = mcp_cmd_read_all(mcp);
+ if (status != 0)
+ return status;
+
+ val = mcp->gpio_inval;
+ *bits = (val & *mask);
+ return 0;
+}
+
+static int mcp_get(struct gpio_chip *gc, unsigned int gpio_nr)
+{
+ unsigned long mask = 0, bits = 0;
+
+ mask = (1 << gpio_nr);
+ mcp_get_multiple(gc, &mask, &bits);
+ return (bits > 0) ? 1 : 0;
+}
+
+static int mcp_get_direction(struct gpio_chip *gc, unsigned int gpio_nr)
+{
+ struct mcp2200 *mcp = gpiochip_get_data(gc);
+
+ return (mcp->gpio_dir & (MCP2200_DIR_IN << gpio_nr))
+ ? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT;
+}
+
+static int mcp_set_direction(struct gpio_chip *gc, unsigned int gpio_nr,
+ enum MCP_IO_DIR io_direction)
+{
+ struct mcp2200 *mcp = gpiochip_get_data(gc);
+ struct mcp_configure *conf;
+ int status;
+ /* after the configure cmd we will need to set the outputs again */
+ unsigned long mask = ~(mcp->gpio_dir); /* only set outputs */
+ unsigned long bits = mcp->gpio_val;
+ /* Offsets of alternative pins in config_alt_pins, 0 is not used */
+ u8 alt_pin_conf[8] = {SSPND, USBCFG, 0, 0, 0, 0, RXLED, TXLED};
+ u8 config_alt_pins = mcp->config_alt_pins;
+
+ /* Read in the reset baudrate first, we need it later */
+ status = mcp_cmd_read_all(mcp);
+ if (status != 0)
+ return status;
+
+ conf = kzalloc(sizeof(struct mcp_configure), GFP_KERNEL);
+ if (!conf)
+ return -ENOMEM;
+ mutex_lock(&mcp->lock);
+
+ /* configure will reset the chip! */
+ conf->cmd = CONFIGURE;
+ conf->io_bmap = (mcp->gpio_dir & ~(1 << gpio_nr))
+ | (io_direction << gpio_nr);
+ /* Don't overwrite the reset parameters */
+ conf->baud_h = mcp->baud_h;
+ conf->baud_l = mcp->baud_l;
+ conf->config_alt_options = mcp->config_alt_options;
+ conf->io_default_val_bmap = mcp->gpio_reset_val;
+ /* Adjust alt. func if necessary */
+ if (alt_pin_conf[gpio_nr])
+ config_alt_pins &= ~(1 << alt_pin_conf[gpio_nr]);
+ conf->config_alt_pins = config_alt_pins;
+
+ status = hid_hw_output_report(mcp->hdev, (u8 *) conf,
+ sizeof(struct mcp_set_clear_outputs));
+ if (status == sizeof(struct mcp_set_clear_outputs)) {
+ mcp->gpio_dir &= ~(1 << gpio_nr);
+ mcp->config_alt_pins = config_alt_pins;
+ } else {
+ return -EIO;
+ }
+
+ kfree(conf);
+ mutex_unlock(&mcp->lock);
+
+ /* Configure CMD will clear all IOs -> rewrite them */
+ mcp_set_multiple(gc, &mask, &bits);
+ return 0;
+}
+
+static int mcp_direction_input(struct gpio_chip *gc, unsigned int gpio_nr)
+{
+ return mcp_set_direction(gc, gpio_nr, MCP2200_DIR_IN);
+}
+
+static int mcp_direction_output(struct gpio_chip *gc, unsigned int gpio_nr,
+ int value)
+{
+ int ret;
+ unsigned long mask, bmap_value;
+
+ mask = (1 << gpio_nr);
+ bmap_value = (value << gpio_nr);
+
+ ret = mcp_set_direction(gc, gpio_nr, MCP2200_DIR_OUT);
+ if (ret == 0)
+ mcp_set_multiple(gc, &mask, &bmap_value);
+ return ret;
+}
+
+
+static const struct gpio_chip template_chip = {
+ .label = "mcp2200",
+ .owner = THIS_MODULE,
+ .get_direction = mcp_get_direction,
+ .direction_input = mcp_direction_input,
+ .direction_output = mcp_direction_output,
+ .set = mcp_set,
+ .set_multiple = mcp_set_multiple,
+ .get = mcp_get,
+ .get_multiple = mcp_get_multiple,
+ .base = -1,
+ .ngpio = MCP_NGPIO,
+ .can_sleep = true,
+};
+
+/*
+ * MCP2200 uses interrupt endpoint for input reports. This function
+ * is called by HID layer when it receives i/p report from mcp2200,
+ * which is actually a response to the previously sent command.
+ */
+static int mcp2200_raw_event(struct hid_device *hdev, struct hid_report *report,
+ u8 *data, int size)
+{
+ struct mcp2200 *mcp = hid_get_drvdata(hdev);
+ struct mcp_read_all_resp *all_resp;
+
+ switch (data[0]) {
+ case READ_ALL:
+ all_resp = (struct mcp_read_all_resp *) data;
+ mcp->status = 0;
+ mcp->gpio_inval = all_resp->io_port_val_bmap;
+ mcp->baud_h = all_resp->baud_h;
+ mcp->baud_l = all_resp->baud_l;
+ mcp->gpio_reset_val = all_resp->io_default_val_bmap;
+ mcp->config_alt_pins = all_resp->config_alt_pins;
+ mcp->config_alt_options = all_resp->config_alt_options;
+ break;
+ default:
+ mcp->status = -EIO;
+ break;
+ }
+
+ complete(&mcp->wait_in_report);
+ return 1;
+}
+
+static void mcp2200_hid_unregister(void *ptr)
+{
+ struct hid_device *hdev = ptr;
+
+ hid_hw_close(hdev);
+ hid_hw_stop(hdev);
+}
+
+static int mcp2200_probe(struct hid_device *hdev, const struct hid_device_id *id)
+{
+ int ret;
+ struct mcp2200 *mcp;
+
+ mcp = devm_kzalloc(&hdev->dev, sizeof(*mcp), GFP_KERNEL);
+ if (!mcp)
+ return -ENOMEM;
+
+ ret = hid_parse(hdev);
+ if (ret) {
+ hid_err(hdev, "can't parse reports\n");
+ return ret;
+ }
+
+ /*
+ * This driver uses the .raw_event callback and therefore does not need any
+ * HID_CONNECT_xxx flags.
+ */
+ ret = hid_hw_start(hdev, 0);
+ if (ret) {
+ hid_err(hdev, "can't start hardware\n");
+ return ret;
+ }
+
+ hid_info(hdev, "USB HID v%x.%02x Device [%s] on %s\n", hdev->version >> 8,
+ hdev->version & 0xff, hdev->name, hdev->phys);
+
+ ret = hid_hw_open(hdev);
+ if (ret) {
+ hid_err(hdev, "can't open device\n");
+ hid_hw_stop(hdev);
+ return ret;
+ }
+
+ mutex_init(&mcp->lock);
+ init_completion(&mcp->wait_in_report);
+ hid_set_drvdata(hdev, mcp);
+ mcp->hdev = hdev;
+
+ ret = devm_add_action_or_reset(&hdev->dev, mcp2200_hid_unregister, hdev);
+ if (ret)
+ return ret;
+
+ mcp->gc = template_chip;
+ mcp->gc.parent = &hdev->dev;
+
+ ret = gpiochip_add_data(&mcp->gc, mcp);
+ if (ret < 0) {
+ dev_err(&hdev->dev, "Unable to register gpiochip\n");
+ hid_hw_stop(hdev);
+ return ret;
+ }
+
+ return 0;
+}
+
+static void mcp2200_remove(struct hid_device *hdev)
+{
+ struct mcp2200 *mcp;
+
+ mcp = hid_get_drvdata(hdev);
+ gpiochip_remove(&mcp->gc);
+}
+
+static const struct hid_device_id mcp2200_devices[] = {
+ { HID_USB_DEVICE(USB_VENDOR_ID_MICROCHIP, USB_DEVICE_ID_MCP2200) },
+ { }
+};
+MODULE_DEVICE_TABLE(hid, mcp2200_devices);
+
+static struct hid_driver mcp2200_driver = {
+ .name = "mcp2200",
+ .id_table = mcp2200_devices,
+ .probe = mcp2200_probe,
+ .remove = mcp2200_remove,
+ .raw_event = mcp2200_raw_event,
+};
+
+/* Register with HID core */
+module_hid_driver(mcp2200_driver);
+
+MODULE_AUTHOR("Johannes Roith <johannes@gnu-linux.rocks>");
+MODULE_DESCRIPTION("MCP2200 Microchip HID USB to GPIO bridge");
+MODULE_LICENSE("GPL");
--
2.41.0
^ permalink raw reply related
* [PATCH 2/2] hid-mcp2200 - updated hid-id.h
From: Johannes Roith @ 2023-06-11 16:48 UTC (permalink / raw)
To: jikos; +Cc: linux-input, linux-kernel, Johannes Roith
In-Reply-To: <20230611164811.1388-1-johannes@gnu-linux.rocks>
Added Product ID for Microchip MCP2200 so the module can be compiled
correctly.
Signed-off-by: Johannes Roith <johannes@gnu-linux.rocks>
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 5d29abac2300..b48cb8224e0d 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -912,6 +912,7 @@
#define USB_DEVICE_ID_PICK16F1454_V2 0xf2f7
#define USB_DEVICE_ID_LUXAFOR 0xf372
#define USB_DEVICE_ID_MCP2221 0x00dd
+#define USB_DEVICE_ID_MCP2221 0x00df
#define USB_VENDOR_ID_MICROSOFT 0x045e
#define USB_DEVICE_ID_SIDEWINDER_GV 0x003b
--
2.41.0
^ permalink raw reply related
* Re: [PATCH] hid-mcp2200 added driver for MCP2200 GPIOs
From: kernel test robot @ 2023-06-11 18:22 UTC (permalink / raw)
To: Johannes Roith, jikos
Cc: oe-kbuild-all, linux-kernel, linux-input, Johannes Roith
In-Reply-To: <20230611144831.41238-1-johannes@gnu-linux.rocks>
Hi Johannes,
kernel test robot noticed the following build errors:
[auto build test ERROR on v6.4-rc5]
[also build test ERROR on linus/master next-20230609]
[cannot apply to hid/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Johannes-Roith/hid-mcp2200-added-driver-for-MCP2200-GPIOs/20230611-225658
base: v6.4-rc5
patch link: https://lore.kernel.org/r/20230611144831.41238-1-johannes%40gnu-linux.rocks
patch subject: [PATCH] hid-mcp2200 added driver for MCP2200 GPIOs
config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20230612/202306120229.FSzdnoEl-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build):
git checkout v6.4-rc5
b4 shazam https://lore.kernel.org/r/20230611144831.41238-1-johannes@gnu-linux.rocks
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=i386 olddefconfig
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306120229.FSzdnoEl-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from drivers/hid/hid-mcp2200.c:16:
>> drivers/hid/hid-mcp2200.c:403:51: error: 'USB_DEVICE_ID_MCP2200' undeclared here (not in a function); did you mean 'USB_DEVICE_ID_MCP2221'?
403 | { HID_USB_DEVICE(USB_VENDOR_ID_MICROCHIP, USB_DEVICE_ID_MCP2200) },
| ^~~~~~~~~~~~~~~~~~~~~
include/linux/hid.h:715:54: note: in definition of macro 'HID_USB_DEVICE'
715 | .bus = BUS_USB, .vendor = (ven), .product = (prod)
| ^~~~
vim +403 drivers/hid/hid-mcp2200.c
401
402 static const struct hid_device_id mcp2200_devices[] = {
> 403 { HID_USB_DEVICE(USB_VENDOR_ID_MICROCHIP, USB_DEVICE_ID_MCP2200) },
404 { }
405 };
406 MODULE_DEVICE_TABLE(hid, mcp2200_devices);
407
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH] hid-mcp2200 added driver for MCP2200 GPIOs
From: kernel test robot @ 2023-06-11 18:43 UTC (permalink / raw)
To: Johannes Roith, jikos
Cc: oe-kbuild-all, linux-kernel, linux-input, Johannes Roith
In-Reply-To: <20230611144831.41238-1-johannes@gnu-linux.rocks>
Hi Johannes,
kernel test robot noticed the following build errors:
[auto build test ERROR on v6.4-rc5]
[also build test ERROR on linus/master next-20230609]
[cannot apply to hid/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Johannes-Roith/hid-mcp2200-added-driver-for-MCP2200-GPIOs/20230611-225658
base: v6.4-rc5
patch link: https://lore.kernel.org/r/20230611144831.41238-1-johannes%40gnu-linux.rocks
patch subject: [PATCH] hid-mcp2200 added driver for MCP2200 GPIOs
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20230612/202306120210.lbnILDe6-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build):
git checkout v6.4-rc5
b4 shazam https://lore.kernel.org/r/20230611144831.41238-1-johannes@gnu-linux.rocks
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 olddefconfig
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306120210.lbnILDe6-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from drivers/hid/hid-mcp2200.c:16:
>> drivers/hid/hid-mcp2200.c:403:51: error: 'USB_DEVICE_ID_MCP2200' undeclared here (not in a function); did you mean 'USB_DEVICE_ID_MCP2221'?
403 | { HID_USB_DEVICE(USB_VENDOR_ID_MICROCHIP, USB_DEVICE_ID_MCP2200) },
| ^~~~~~~~~~~~~~~~~~~~~
include/linux/hid.h:715:54: note: in definition of macro 'HID_USB_DEVICE'
715 | .bus = BUS_USB, .vendor = (ven), .product = (prod)
| ^~~~
vim +403 drivers/hid/hid-mcp2200.c
401
402 static const struct hid_device_id mcp2200_devices[] = {
> 403 { HID_USB_DEVICE(USB_VENDOR_ID_MICROCHIP, USB_DEVICE_ID_MCP2200) },
404 { }
405 };
406 MODULE_DEVICE_TABLE(hid, mcp2200_devices);
407
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH 2/2] hid-mcp2200 - updated hid-id.h
From: kernel test robot @ 2023-06-11 18:43 UTC (permalink / raw)
To: Johannes Roith, jikos
Cc: llvm, oe-kbuild-all, linux-input, linux-kernel, Johannes Roith
In-Reply-To: <20230611164811.1388-2-johannes@gnu-linux.rocks>
Hi Johannes,
kernel test robot noticed the following build warnings:
[auto build test WARNING on v6.4-rc5]
[also build test WARNING on linus/master next-20230609]
[cannot apply to hid/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Johannes-Roith/hid-mcp2200-updated-hid-id-h/20230612-005025
base: v6.4-rc5
patch link: https://lore.kernel.org/r/20230611164811.1388-2-johannes%40gnu-linux.rocks
patch subject: [PATCH 2/2] hid-mcp2200 - updated hid-id.h
config: um-x86_64_defconfig (https://download.01.org/0day-ci/archive/20230612/202306120220.hHmBq7QJ-lkp@intel.com/config)
compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project.git 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
reproduce (this is a W=1 build):
mkdir -p ~/bin
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install um cross compiling tool for clang build
# apt-get install binutils-um-linux-gnu
git checkout v6.4-rc5
b4 shazam https://lore.kernel.org/r/20230611164811.1388-2-johannes@gnu-linux.rocks
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=um olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=um SHELL=/bin/bash drivers/
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306120220.hHmBq7QJ-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from drivers/hid/hid-core.c:31:
In file included from include/linux/hid.h:29:
In file included from include/linux/hid_bpf.h:6:
In file included from include/linux/bpf.h:31:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/um/include/asm/hardirq.h:5:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/um/include/asm/io.h:24:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
^
In file included from drivers/hid/hid-core.c:31:
In file included from include/linux/hid.h:29:
In file included from include/linux/hid_bpf.h:6:
In file included from include/linux/bpf.h:31:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/um/include/asm/hardirq.h:5:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/um/include/asm/io.h:24:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
^
In file included from drivers/hid/hid-core.c:31:
In file included from include/linux/hid.h:29:
In file included from include/linux/hid_bpf.h:6:
In file included from include/linux/bpf.h:31:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/um/include/asm/hardirq.h:5:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/um/include/asm/io.h:24:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:692:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:700:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:708:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:717:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:726:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:735:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
In file included from drivers/hid/hid-core.c:36:
>> drivers/hid/hid-ids.h:915:9: warning: 'USB_DEVICE_ID_MCP2221' macro redefined [-Wmacro-redefined]
#define USB_DEVICE_ID_MCP2221 0x00df
^
drivers/hid/hid-ids.h:914:9: note: previous definition is here
#define USB_DEVICE_ID_MCP2221 0x00dd
^
13 warnings generated.
vim +/USB_DEVICE_ID_MCP2221 +915 drivers/hid/hid-ids.h
905
906 #define USB_VENDOR_ID_MICROCHIP 0x04d8
907 #define USB_DEVICE_ID_PICKIT1 0x0032
908 #define USB_DEVICE_ID_PICKIT2 0x0033
909 #define USB_DEVICE_ID_PICOLCD 0xc002
910 #define USB_DEVICE_ID_PICOLCD_BOOTLOADER 0xf002
911 #define USB_DEVICE_ID_PICK16F1454 0x0042
912 #define USB_DEVICE_ID_PICK16F1454_V2 0xf2f7
913 #define USB_DEVICE_ID_LUXAFOR 0xf372
914 #define USB_DEVICE_ID_MCP2221 0x00dd
> 915 #define USB_DEVICE_ID_MCP2221 0x00df
916
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH 1/2] hid-mcp2200 added driver for MCP2200 GPIOs
From: kernel test robot @ 2023-06-11 18:43 UTC (permalink / raw)
To: Johannes Roith, jikos
Cc: llvm, oe-kbuild-all, linux-input, linux-kernel, Johannes Roith
In-Reply-To: <20230611164811.1388-1-johannes@gnu-linux.rocks>
Hi Johannes,
kernel test robot noticed the following build errors:
[auto build test ERROR on v6.4-rc5]
[also build test ERROR on linus/master next-20230609]
[cannot apply to hid/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Johannes-Roith/hid-mcp2200-updated-hid-id-h/20230612-005025
base: v6.4-rc5
patch link: https://lore.kernel.org/r/20230611164811.1388-1-johannes%40gnu-linux.rocks
patch subject: [PATCH 1/2] hid-mcp2200 added driver for MCP2200 GPIOs
config: x86_64-buildonly-randconfig-r003-20230612 (https://download.01.org/0day-ci/archive/20230612/202306120239.maB0RzPe-lkp@intel.com/config)
compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project.git 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
reproduce (this is a W=1 build):
mkdir -p ~/bin
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout v6.4-rc5
b4 shazam https://lore.kernel.org/r/20230611164811.1388-1-johannes@gnu-linux.rocks
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=x86_64 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306120239.maB0RzPe-lkp@intel.com/
All errors (new ones prefixed by >>):
>> drivers/hid/hid-mcp2200.c:403:44: error: use of undeclared identifier 'USB_DEVICE_ID_MCP2200'
{ HID_USB_DEVICE(USB_VENDOR_ID_MICROCHIP, USB_DEVICE_ID_MCP2200) },
^
1 error generated.
vim +/USB_DEVICE_ID_MCP2200 +403 drivers/hid/hid-mcp2200.c
401
402 static const struct hid_device_id mcp2200_devices[] = {
> 403 { HID_USB_DEVICE(USB_VENDOR_ID_MICROCHIP, USB_DEVICE_ID_MCP2200) },
404 { }
405 };
406 MODULE_DEVICE_TABLE(hid, mcp2200_devices);
407
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH 2/2] hid-mcp2200 - updated hid-id.h
From: kernel test robot @ 2023-06-11 18:53 UTC (permalink / raw)
To: Johannes Roith, jikos
Cc: oe-kbuild-all, linux-input, linux-kernel, Johannes Roith
In-Reply-To: <20230611164811.1388-2-johannes@gnu-linux.rocks>
Hi Johannes,
kernel test robot noticed the following build warnings:
[auto build test WARNING on v6.4-rc5]
[also build test WARNING on linus/master next-20230609]
[cannot apply to hid/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Johannes-Roith/hid-mcp2200-updated-hid-id-h/20230612-005025
base: v6.4-rc5
patch link: https://lore.kernel.org/r/20230611164811.1388-2-johannes%40gnu-linux.rocks
patch subject: [PATCH 2/2] hid-mcp2200 - updated hid-id.h
config: parisc-defconfig (https://download.01.org/0day-ci/archive/20230612/202306120204.6ROntFhJ-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 12.3.0
reproduce (this is a W=1 build):
mkdir -p ~/bin
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout v6.4-rc5
b4 shazam https://lore.kernel.org/r/20230611164811.1388-2-johannes@gnu-linux.rocks
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=parisc olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=parisc SHELL=/bin/bash drivers/
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306120204.6ROntFhJ-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from drivers/hid/hid-core.c:36:
>> drivers/hid/hid-ids.h:915: warning: "USB_DEVICE_ID_MCP2221" redefined
915 | #define USB_DEVICE_ID_MCP2221 0x00df
|
drivers/hid/hid-ids.h:914: note: this is the location of the previous definition
914 | #define USB_DEVICE_ID_MCP2221 0x00dd
|
vim +/USB_DEVICE_ID_MCP2221 +915 drivers/hid/hid-ids.h
905
906 #define USB_VENDOR_ID_MICROCHIP 0x04d8
907 #define USB_DEVICE_ID_PICKIT1 0x0032
908 #define USB_DEVICE_ID_PICKIT2 0x0033
909 #define USB_DEVICE_ID_PICOLCD 0xc002
910 #define USB_DEVICE_ID_PICOLCD_BOOTLOADER 0xf002
911 #define USB_DEVICE_ID_PICK16F1454 0x0042
912 #define USB_DEVICE_ID_PICK16F1454_V2 0xf2f7
913 #define USB_DEVICE_ID_LUXAFOR 0xf372
914 #define USB_DEVICE_ID_MCP2221 0x00dd
> 915 #define USB_DEVICE_ID_MCP2221 0x00df
916
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH 1/2] hid-mcp2200 added driver for MCP2200 GPIOs
From: Christophe JAILLET @ 2023-06-11 19:02 UTC (permalink / raw)
To: Johannes Roith, jikos; +Cc: linux-input, linux-kernel
In-Reply-To: <20230611164811.1388-1-johannes@gnu-linux.rocks>
Le 11/06/2023 à 18:48, Johannes Roith a écrit :
> Added a gpiochip compatible driver to control the 8 GPIOs of the MCP2200
> by using the HID interface.
>
> Using GPIOs with alternative functions (GP0<->SSPND, GP1<->USBCFG,
> GP6<->RXLED, GP7<->TXLED) will reset the functions, if set (unset by
> default).
>
> The driver was tested while also using the UART of the chip. Setting
> and reading the GPIOs has no effect on the UART communication. However,
> a reset is triggered after the CONFIGURE command. If the GPIO Direction
> is constantly changed, this will affect the communication at low baud
> rates. This is a hardware problem of the MCP2200 and is not caused by
> the driver.
>
> Signed-off-by: Johannes Roith <johannes@gnu-linux.rocks>
Hi,
a few nits below, should it help the review.
[...]
> --- /dev/null
> +++ b/drivers/hid/hid-mcp2200.c
> @@ -0,0 +1,421 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * MCP2200 - Microchip USB to GPIO bridge
> + *
> + * Copyright (c) 2023, Johannes Roith <johannes@gnu-linux.rocks>
> + *
> + * Datasheet: https://ww1.microchip.com/downloads/en/DeviceDoc/22228A.pdf
> + * App Note for HID: https://ww1.microchip.com/downloads/en/DeviceDoc/93066A.pdf
> + */
> +#include <linux/module.h>
> +#include <linux/err.h>
> +#include <linux/mutex.h>
> +#include <linux/bitfield.h>
Is this include needed?
> +#include <linux/completion.h>
> +#include <linux/delay.h>
> +#include <linux/hid.h>
> +#include <linux/hidraw.h>
> +#include <linux/gpio/driver.h>
Many maintainers prefer alphabetic order for includes.
> +#include "hid-ids.h"
> +
> +/* Commands codes in a raw output report */
> +enum {
> + SET_CLEAR_OUTPUTS = 0x08,
> + CONFIGURE = 0x10,
> + READ_EE = 0x20,
> + WRITE_EE = 0x40,
> + READ_ALL = 0x80
> +};
Does some
#define xxx BIT(n)
would make more sense than this enum?
> +
> +/* MCP GPIO direction encoding */
> +enum MCP_IO_DIR {
> + MCP2200_DIR_OUT = 0x00,
> + MCP2200_DIR_IN = 0x01,
> +};
> +
> +/* Altternative pin assignments */
> +enum {
> + TXLED = 2,
> + RXLED = 3,
> + USBCFG = 6,
> + SSPND = 7,
> +
Uneeded new line.
> +};
> +
> +#define MCP_NGPIO 8
> +
[...]
> +/* this executes the READ_ALL cmd */
> +static int mcp_cmd_read_all(struct mcp2200 *mcp)
> +{
> + struct mcp_read_all *read_all;
> + int len, t;
> +
> + reinit_completion(&mcp->wait_in_report);
> + mutex_lock(&mcp->lock);
> +
> + read_all = kzalloc(sizeof(struct mcp_read_all), GFP_KERNEL);
> + if (!read_all)
> + return -ENOMEM;
Allocation could be deone before the lock.
> +
> + read_all->cmd = READ_ALL;
> + len = hid_hw_output_report(mcp->hdev, (u8 *) read_all,
> + sizeof(struct mcp_read_all));
> +
> + if (len != sizeof(struct mcp_read_all))
kfree(read_all); ?
(or move the call just below before the test)
> + return -EINVAL;
> +
> + kfree(read_all);
> + mutex_unlock(&mcp->lock);
Mutex unlock could be done before kfree() or even before the "if
(len..." a few lines above.
> + t = wait_for_completion_timeout(&mcp->wait_in_report, msecs_to_jiffies(4000));
> + if (!t)
> + return -ETIMEDOUT;
> +
> + /* return status, negative value if wrong response was received */
> + return mcp->status;
> +}
> +
> +static void mcp_set_multiple(struct gpio_chip *gc, unsigned long *mask,
> + unsigned long *bits)
> +{
> + struct mcp2200 *mcp = gpiochip_get_data(gc);
> + u8 value;
> + int status;
> + struct mcp_set_clear_outputs *cmd;
> +
> + cmd = kzalloc(sizeof(struct mcp_set_clear_outputs), GFP_KERNEL);
> + if (!cmd)
> + return;
> +
> + mutex_lock(&mcp->lock);
> +
> + value = mcp->gpio_val & ~*mask;
> + value |= (*mask & *bits);
> +
> + cmd->cmd = SET_CLEAR_OUTPUTS;
> + cmd->set_bmap = value;
> + cmd->clear_bmap = ~(value);
> +
> + status = hid_hw_output_report(mcp->hdev, (u8 *) cmd,
> + sizeof(struct mcp_set_clear_outputs));
> + if (status == sizeof(struct mcp_set_clear_outputs))
> + mcp->gpio_val = value;
> +
> + kfree(cmd);
> + mutex_unlock(&mcp->lock);
Mutex unlock could be done before kfree().
> +}
> +
> +static void mcp_set(struct gpio_chip *gc, unsigned int gpio_nr, int value)
> +{
> + unsigned long mask = (1 << gpio_nr);
Uneeded ()
Does using BIT makes sense here?
> + unsigned long bmap_value = (value<<gpio_nr);
Uneeded () and missing spaces aoud <<
> +
> + mcp_set_multiple(gc, &mask, &bmap_value);
> +}
> +
> +static int mcp_get_multiple(struct gpio_chip *gc, unsigned long *mask,
> + unsigned long *bits)
> +{
> + u32 val;
> + struct mcp2200 *mcp = gpiochip_get_data(gc);
> + int status;
> +
> + status = mcp_cmd_read_all(mcp);
> + if (status != 0)
> + return status;
> +
> + val = mcp->gpio_inval;
> + *bits = (val & *mask);
> + return 0;
> +}
> +
> +static int mcp_get(struct gpio_chip *gc, unsigned int gpio_nr)
> +{
> + unsigned long mask = 0, bits = 0;
No need to init long (and maybe bits)
> +
> + mask = (1 << gpio_nr);
Uneeded ()
Does using BIT makes sense here?
> + mcp_get_multiple(gc, &mask, &bits);
> + return (bits > 0) ? 1 : 0;
> +}
> +
> +static int mcp_get_direction(struct gpio_chip *gc, unsigned int gpio_nr)
> +{
> + struct mcp2200 *mcp = gpiochip_get_data(gc);
> +
> + return (mcp->gpio_dir & (MCP2200_DIR_IN << gpio_nr))
> + ? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT;
> +}
> +
> +static int mcp_set_direction(struct gpio_chip *gc, unsigned int gpio_nr,
> + enum MCP_IO_DIR io_direction)
> +{
> + struct mcp2200 *mcp = gpiochip_get_data(gc);
> + struct mcp_configure *conf;
> + int status;
> + /* after the configure cmd we will need to set the outputs again */
> + unsigned long mask = ~(mcp->gpio_dir); /* only set outputs */
> + unsigned long bits = mcp->gpio_val;
> + /* Offsets of alternative pins in config_alt_pins, 0 is not used */
> + u8 alt_pin_conf[8] = {SSPND, USBCFG, 0, 0, 0, 0, RXLED, TXLED};
> + u8 config_alt_pins = mcp->config_alt_pins;
> +
> + /* Read in the reset baudrate first, we need it later */
> + status = mcp_cmd_read_all(mcp);
> + if (status != 0)
> + return status;
> +
> + conf = kzalloc(sizeof(struct mcp_configure), GFP_KERNEL);
> + if (!conf)
> + return -ENOMEM;
> + mutex_lock(&mcp->lock);
> +
> + /* configure will reset the chip! */
> + conf->cmd = CONFIGURE;
> + conf->io_bmap = (mcp->gpio_dir & ~(1 << gpio_nr))
> + | (io_direction << gpio_nr);
> + /* Don't overwrite the reset parameters */
> + conf->baud_h = mcp->baud_h;
> + conf->baud_l = mcp->baud_l;
> + conf->config_alt_options = mcp->config_alt_options;
> + conf->io_default_val_bmap = mcp->gpio_reset_val;
> + /* Adjust alt. func if necessary */
> + if (alt_pin_conf[gpio_nr])
> + config_alt_pins &= ~(1 << alt_pin_conf[gpio_nr]);
> + conf->config_alt_pins = config_alt_pins;
> +
> + status = hid_hw_output_report(mcp->hdev, (u8 *) conf,
> + sizeof(struct mcp_set_clear_outputs));
> + if (status == sizeof(struct mcp_set_clear_outputs)) {
> + mcp->gpio_dir &= ~(1 << gpio_nr);
> + mcp->config_alt_pins = config_alt_pins;
> + } else {
> + return -EIO;
> + }
> +
> + kfree(conf);
> + mutex_unlock(&mcp->lock);
Mutex unlock could be done before kfree().
> +
> + /* Configure CMD will clear all IOs -> rewrite them */
> + mcp_set_multiple(gc, &mask, &bits);
> + return 0;
> +}
> +
> +static int mcp_direction_input(struct gpio_chip *gc, unsigned int gpio_nr)
> +{
> + return mcp_set_direction(gc, gpio_nr, MCP2200_DIR_IN);
> +}
> +
> +static int mcp_direction_output(struct gpio_chip *gc, unsigned int gpio_nr,
> + int value)
> +{
> + int ret;
> + unsigned long mask, bmap_value;
> +
> + mask = (1 << gpio_nr);
Uneeded ()
Does using BIT makes sense here?
> + bmap_value = (value << gpio_nr);
Uneeded ()
> +
> + ret = mcp_set_direction(gc, gpio_nr, MCP2200_DIR_OUT);
> + if (ret == 0)
> + mcp_set_multiple(gc, &mask, &bmap_value);
> + return ret;
> +}
> +
> +
No need for 2 new lines.
[...]
> +static int mcp2200_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{
> + int ret;
> + struct mcp2200 *mcp;
> +
> + mcp = devm_kzalloc(&hdev->dev, sizeof(*mcp), GFP_KERNEL);
> + if (!mcp)
> + return -ENOMEM;
> +
> + ret = hid_parse(hdev);
> + if (ret) {
> + hid_err(hdev, "can't parse reports\n");
> + return ret;
> + }
> +
> + /*
> + * This driver uses the .raw_event callback and therefore does not need any
> + * HID_CONNECT_xxx flags.
> + */
> + ret = hid_hw_start(hdev, 0);
> + if (ret) {
> + hid_err(hdev, "can't start hardware\n");
> + return ret;
> + }
> +
> + hid_info(hdev, "USB HID v%x.%02x Device [%s] on %s\n", hdev->version >> 8,
> + hdev->version & 0xff, hdev->name, hdev->phys);
> +
> + ret = hid_hw_open(hdev);
> + if (ret) {
> + hid_err(hdev, "can't open device\n");
> + hid_hw_stop(hdev);
> + return ret;
> + }
> +
> + mutex_init(&mcp->lock);
> + init_completion(&mcp->wait_in_report);
> + hid_set_drvdata(hdev, mcp);
> + mcp->hdev = hdev;
> +
> + ret = devm_add_action_or_reset(&hdev->dev, mcp2200_hid_unregister, hdev);
> + if (ret)
> + return ret;
> +
> + mcp->gc = template_chip;
> + mcp->gc.parent = &hdev->dev;
> +
> + ret = gpiochip_add_data(&mcp->gc, mcp);
devm_gpiochip_add_data() and no .remove function?
> + if (ret < 0) {
> + dev_err(&hdev->dev, "Unable to register gpiochip\n");
hid_err() to be consistent?
> + hid_hw_stop(hdev);
hid_hw_stop() would be called twice. Once here and once because of the
devm_add_action_or_reset() above.
Just my 2c,
CJ
> + return ret;
> + }
> +
> + return 0;
> +}
[...]
^ permalink raw reply
* Re: [PATCH 2/2] hid-mcp2200 - updated hid-id.h
From: Christophe JAILLET @ 2023-06-11 19:13 UTC (permalink / raw)
To: Johannes Roith, jikos; +Cc: linux-input, linux-kernel
In-Reply-To: <20230611164811.1388-2-johannes@gnu-linux.rocks>
Le 11/06/2023 à 18:48, Johannes Roith a écrit :
> Added Product ID for Microchip MCP2200 so the module can be compiled
> correctly.
>
> Signed-off-by: Johannes Roith <johannes@gnu-linux.rocks>
>
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 5d29abac2300..b48cb8224e0d 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -912,6 +912,7 @@
> #define USB_DEVICE_ID_PICK16F1454_V2 0xf2f7
> #define USB_DEVICE_ID_LUXAFOR 0xf372
> #define USB_DEVICE_ID_MCP2221 0x00dd
> +#define USB_DEVICE_ID_MCP2221 0x00df
2 times USB_DEVICE_ID_MCP2221 with different values?
USB_DEVICE_ID_MCP2200?
Also this one should be merged with patch 1/2 or sent before the other
one, otherwise 1/2 can't compile.
CJ
>
> #define USB_VENDOR_ID_MICROSOFT 0x045e
> #define USB_DEVICE_ID_SIDEWINDER_GV 0x003b
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: input: iqs7222: Add properties for Azoteq IQS7222D
From: Jeff LaBundy @ 2023-06-12 0:29 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Krzysztof Kozlowski, dmitry.torokhov, robh+dt, linux-input,
devicetree, krzysztof.kozlowski+dt, conor+dt
In-Reply-To: <1bdaee8a-b327-d97f-a96a-25367e34cdad@linaro.org>
Hi Krzysztof,
On Fri, Jun 02, 2023 at 09:21:00AM +0200, Krzysztof Kozlowski wrote:
> On 01/06/2023 03:23, Jeff LaBundy wrote:
> >>>
> >>> reg:
> >>> maxItems: 1
> >>> @@ -173,6 +174,148 @@ properties:
> >>> maximum: 3000
> >>> description: Specifies the report rate (in ms) during ultra-low-power mode.
> >>>
> >>> + touchscreen-size-x: true
> >>> + touchscreen-size-y: true
> >>> + touchscreen-inverted-x: true
> >>> + touchscreen-inverted-y: true
> >>> + touchscreen-swapped-x-y: true
> >>
> >> Why? Aren't they coming from common schema?
> >
> > Yes, but because additionalProperties is set to false here, we must explicitly
> > include the subset of properties from the common schema that are allowed for
> > this particular instance. I counted over a dozen other bindings doing the same.
> >
> > In case I have misunderstood, please let me know.
>
> If you are listing now most of touchscreen properties, it is a sign you
> should use just unevaluatedProperties: false (instead
> additionalProperties) and then no need for any of these here.
At present, touchscreen.yaml offers 17 properties. I am only replicating 5
of them, so this binding may not be such a case for this idea. That said, I
tried your suggestion locally.
In the present implementation (additionalProperties: false), the tooling
rightfully presents an error if the example includes a property that is not
explicitly offered (e.g. 'touchscreen-x-plate-ohms').
If I change the binding to use "unevaluatedProperties: false", the tooling
accepts an example which includes this same property that is not supported
by this particular hardware. This seems like a regression.
In my humble opinion, the present implementation is optimal. It is for that
reason we see multiple bindings with this same block of inherited properties.
I will leave this unchanged; please let me know in case I have misunderstood.
>
> >
> >>
> >>> +
> >>> + trackpad:
> >>> + type: object
> >>> + description: Represents all channels associated with the trackpad.
> >>> +
> >>> + properties:
> >>> + azoteq,channel-select:
> >>> + $ref: /schemas/types.yaml#/definitions/uint32-array
> >>> + minItems: 1
> >>> + maxItems: 12
> >>> + items:
> >>> + minimum: 0
> >>> + maximum: 13
> >>> + description:
> >>> + Specifies the order of the channels that participate in the trackpad.
> >>> + Specify 255 to omit a given channel for the purpose of mapping a non-
> >>> + rectangular trackpad.
> >>> +
> >>> + azoteq,num-rows:
> >>> + $ref: /schemas/types.yaml#/definitions/uint32
> >>> + minimum: 1
> >>> + maximum: 12
> >>> + description: Specifies the number of rows that comprise the trackpad.
> >>> +
> >>> + azoteq,num-cols:
> >>> + $ref: /schemas/types.yaml#/definitions/uint32
> >>> + minimum: 1
> >>> + maximum: 12
> >>> + description: Specifies the number of columns that comprise the trackpad.
> >>> +
> >>> + azoteq,top-speed:
> >>> + $ref: /schemas/types.yaml#/definitions/uint32
> >>> + multipleOf: 4
> >>> + minimum: 0
> >>> + maximum: 1020
> >>> + description:
> >>> + Specifies the speed of movement after which coordinate filtering is
> >>> + no longer applied.
> >>
> >> Units?
> >
> > This is a ratiometric, i.e. unitless value that represents a hardware filter
> > coefficient. It already exists in this binding prior to this patch under the
> > slider-0/1 node and is simply re-used here.
>
> Then mention the ratio (e.g. "speed of movement expressed as ratio
> of..."). Description said "speed" and we usually measure speed in very
> specific units.
ACK; this is a good call-out. Please note that the existing binding uses the
same language elsewhere; I will mirror the updated language to the existing
instances in a separate patch.
>
> >
> >>
> >>> +
> >>> + azoteq,bottom-speed:
> >>> + $ref: /schemas/types.yaml#/definitions/uint32
> >>> + minimum: 0
> >>> + maximum: 255
> >>> + description:
> >>> + Specifies the speed of movement after which coordinate filtering is
> >>> + linearly reduced.
> >>
> >> Units?
> >
> > Same here.
> >
> >>
> >>> +
> >>> + azoteq,use-prox:
> >>> + type: boolean
> >>> + description:
> >>> + Directs the trackpad to respond to the proximity states of the se-
> >>> + lected channels instead of their corresponding touch states. Note
> >>
> >> Don't split the words.
> >
> > ACK.
> >
> >>
> >>> + the trackpad cannot report granular coordinates during a state of
> >>> + proximity.
> >>> +
> >>> + patternProperties:
> >>> + "^azoteq,lower-cal-(x|y)$":
> >>> + $ref: /schemas/types.yaml#/definitions/uint32
> >>> + minimum: 0
> >>> + maximum: 255
> >>> + description: Specifies the trackpad's lower starting points.
> >>
> >> Why would you need this property? Why does this represent hardware property?
> >
> > This property and its cousin below define the physical boundaries of the
> > touch surface. They are typically used to mask areas that cannot elicit
> > an electrical response due to manufacturing tolerances or the presence of
> > an overlay. For that reason, they descend directly from properties of the
> > hardware.
> >
> > Similar properties already exist in this binding for the slider case; this
> > device simply extends the functionality to a second dimenstion.
>
> OK
>
> >
> >>
> >>> +
> >>> + "^azoteq,upper-cal-(x|y)$":
> >>> + $ref: /schemas/types.yaml#/definitions/uint32
> >>> + minimum: 0
> >>> + maximum: 255
> >>> + description: Specifies the trackpad's upper starting points.
> >>> +
> >>> + "^event-(press|tap|(swipe|flick)-(x|y)-(pos|neg))$":
> >>> + type: object
> >>> + $ref: input.yaml#
> >>> + description:
> >>> + Represents a press or gesture event reported by the trackpad. Specify
> >>> + 'linux,code' under the press event to report absolute coordinates.
> >>> +
> >>> + properties:
> >>> + linux,code: true
> >>> +
> >>> + azoteq,gesture-angle-tighten:
> >>> + type: boolean
> >>> + description:
> >>> + Limits the tangent of the gesture angle to 0.5 (axial gestures
> >>> + only). If specified in one direction, the effect is applied in
> >>> + either direction.
> >>> +
> >>> + azoteq,gesture-max-ms:
> >>> + multipleOf: 16
> >>> + minimum: 0
> >>> + maximum: 4080
> >>> + description:
> >>> + Specifies the length of time (in ms) within which a tap, swipe
> >>> + or flick gesture must be completed in order to be acknowledged
> >>> + by the device. The number specified for any one swipe or flick
> >>> + gesture applies to all other swipe or flick gestures.
> >>> +
> >>> + azoteq,gesture-min-ms:
> >>> + multipleOf: 16
> >>> + minimum: 0
> >>> + maximum: 4080
> >>> + description:
> >>> + Specifies the length of time (in ms) for which a tap gesture must
> >>> + be held in order to be acknowledged by the device.
> >>> +
> >>> + azoteq,gesture-dist:
> >>> + $ref: /schemas/types.yaml#/definitions/uint32
> >>> + minimum: 0
> >>> + maximum: 65535
> >>> + description:
> >>> + Specifies the distance across which a tap, swipe or flick gesture
> >>> + must travel in order to be acknowledged by the device. The number
> >>> + specified for any one swipe or flick gesture applies to all other
> >>> + swipe or flick gestures.
> >>> +
> >>> + azoteq,gpio-select:
> >>> + $ref: /schemas/types.yaml#/definitions/uint32-array
> >>> + minItems: 1
> >>> + maxItems: 3
> >>> + items:
> >>> + minimum: 0
> >>> + maximum: 2
> >>> + description: |
> >>> + Specifies one or more GPIO mapped to the event as follows:
> >>> + 0: GPIO0
> >>> + 1: GPIO3
> >>> + 2: GPIO4
> >>> +
> >>> + Note that although multiple events can be mapped to a single
> >>> + GPIO, they must all be of the same type (proximity, touch or
> >>> + trackpad gesture).
> >>> +
> >>> + additionalProperties: false
> >>> +
> >>> + required:
> >>> + - azoteq,channel-select
> >>> +
> >>> + additionalProperties: false
> >>> +
> >>> patternProperties:
> >>> "^cycle-[0-9]$":
> >>> type: object
> >>> @@ -288,6 +431,10 @@ patternProperties:
> >>> Activates the reference channel in response to proximity events
> >>> instead of touch events.
> >>>
> >>> + azoteq,counts-filt-enable:
> >>> + type: boolean
> >>> + description: Applies counts filtering to the channel.
> >>> +
> >>> azoteq,ati-band:
> >>> $ref: /schemas/types.yaml#/definitions/uint32
> >>> enum: [0, 1, 2, 3]
> >>> @@ -432,12 +579,12 @@ patternProperties:
> >>> description: |
> >>> Specifies one or more GPIO mapped to the event as follows:
> >>> 0: GPIO0
> >>> - 1: GPIO3 (IQS7222C only)
> >>> - 2: GPIO4 (IQS7222C only)
> >>> + 1: GPIO3
> >>> + 2: GPIO4
> >>
> >> Why changing this? Is it valid for IQS7222A?
> >
> > It's not, only for 'C' and now 'D'. However, the restriction for 'A' is already
> > conveyed in an if/then schema in the original binding. So rather than updating
> > this text to say "(IQS7222C and IQS7222D only)", I opted to drop the open-coded
> > text and rely on the existing schema.
>
> OK
>
> >
>
>
> Best regards,
> Krzysztof
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: input: Add bindings for Azoteq IQS7210A/7211A/E
From: Jeff LaBundy @ 2023-06-12 1:06 UTC (permalink / raw)
To: Rob Herring
Cc: dmitry.torokhov, linux-input, devicetree, krzysztof.kozlowski+dt,
conor+dt
In-Reply-To: <20230609142538.GA878396-robh@kernel.org>
Hi Rob,
On Fri, Jun 09, 2023 at 08:25:38AM -0600, Rob Herring wrote:
> On Mon, May 29, 2023 at 07:33:47PM -0500, Jeff LaBundy wrote:
> > Add bindings for the Azoteq IQS7210A/7211A/E family of trackpad/
> > touchscreen controllers.
> >
> > Signed-off-by: Jeff LaBundy <jeff@labundy.com>
> > ---
> > Changes in v2:
> > - Renamed 'azoteq,default-comms' to 'azoteq,forced-comms-default' and redefined
> > 0, 1 and 2 as unspecified, 0 and 1, respectively
> > - Defined ATI upon its first occurrence
> > - Redefined 'azoteq,gesture-angle' in units of degrees
> > - Declared 'azoteq,rx-enable' to depend upon 'azoteq,tx-enable' within the
> > 'trackpad' node
> >
> > Hi Rob,
> >
> > I attempted to reference existing properties from a common binding [1] as per
> > your feedback in [2], however 'make DT_CHECKER_FLAGS=-m dt_binding_check' fails
> > with the message 'Vendor specific properties must have a type and description
> > unless they have a defined, common suffix.'
>
> Is that because you have differing constraints in each case?
In the failing example [2], I have started with a simple boolean that carries
nothing but a type and description. From the new azoteq,common.yaml:
properties:
[...]
azoteq,use-prox:
type: boolean
description: foo
And from the first consumer:
patternProperties:
"^hall-switch-(north|south)$":
type: object
allOf:
- $ref: input.yaml#
- $ref: azoteq,common.yaml#
description: bar
properties:
linux,code: true
azoteq,use-prox: true
However, the tooling presents the following:
CHKDT Documentation/devicetree/bindings/processed-schema.json
/home/jlabundy/work/linux/Documentation/devicetree/bindings/input/iqs62x-keys.yaml: patternProperties:^hall-switch-(north|south)$:properties:azoteq,use-prox: True is not of type 'object'
hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
[...]
I am committed to addressing your feedback; to help me do so, can you help me
identify what I've done wrong, and/or point me to an example that successfully
passes dt_binding_check?
>
> > This seems related to the discussion in [3], where you warned that the tooling
> > cannot yet deduce that vendor-specific properties have already been typed in an
> > externally $ref'd binding. The only other example of a common vendor schema is
> > [4], but in that case the common properties are defined under arbitraily named
> > pinmux config nodes. As such, they are part of 'additionalProperties' instead of
> > 'properties' and hence exempt from this particular validation.
> >
> > Please let me know if I am mistaken (surprise!), in which case I will continue
> > on this path and send a v3. Otherwise, I would like to suggest that the review
> > moves forward under the premise that I will happily consolidate these bindings
> > once the tooling supports this idea.
> >
> > Kind regards,
> > Jeff LaBundy
[2] https://github.com/jlabundy/linux/tree/azoteq-common
Kind regards,
Jeff LaBundy
^ permalink raw reply
* [PATCH V2] Input: synaptics-rmi4- Add a new feature for Forepad.
From: Marge Yang @ 2023-06-12 2:26 UTC (permalink / raw)
To: dmitry.torokhov, linux-input, linux-kernel, marge.yang
Cc: david.chiu, derek.cheng, sam.tsai, vincent.huang
Forcepad devices will use F21, for click simulation
due to lack of a metal button, so we add F21 support
to make forcepad support click function.
Signed-off-by: Marge Yang <marge.yang@tw.synaptics.com>
---
drivers/input/rmi4/Kconfig | 8 +++
drivers/input/rmi4/Makefile | 1 +
drivers/input/rmi4/rmi_bus.c | 3 +
drivers/input/rmi4/rmi_driver.h | 5 +-
drivers/input/rmi4/rmi_f21.c | 110 ++++++++++++++++++++++++++++++++
5 files changed, 126 insertions(+), 1 deletion(-)
create mode 100644 drivers/input/rmi4/rmi_f21.c
diff --git a/drivers/input/rmi4/Kconfig b/drivers/input/rmi4/Kconfig
index c0163b983ce6..37e2ba4918fd 100644
--- a/drivers/input/rmi4/Kconfig
+++ b/drivers/input/rmi4/Kconfig
@@ -108,6 +108,14 @@ config RMI4_F3A
Function 3A provides GPIO support for RMI4 devices. This includes
support for buttons on TouchPads and ClickPads.
+config RMI4_F21
+ bool "RMI4 Function 21 (PRESSURE)"
+ help
+ Say Y here if you want to add support for RMI4 function 21.
+
+ Function 21 provides buttons/pressure for RMI4 devices. This includes
+ support for buttons/pressure on PressurePad.
+
config RMI4_F54
bool "RMI4 Function 54 (Analog diagnostics)"
depends on VIDEO_DEV=y || (RMI4_CORE=m && VIDEO_DEV=m)
diff --git a/drivers/input/rmi4/Makefile b/drivers/input/rmi4/Makefile
index 02f14c846861..ec4f08513d8b 100644
--- a/drivers/input/rmi4/Makefile
+++ b/drivers/input/rmi4/Makefile
@@ -11,6 +11,7 @@ rmi_core-$(CONFIG_RMI4_F12) += rmi_f12.o
rmi_core-$(CONFIG_RMI4_F30) += rmi_f30.o
rmi_core-$(CONFIG_RMI4_F34) += rmi_f34.o rmi_f34v7.o
rmi_core-$(CONFIG_RMI4_F3A) += rmi_f3a.o
+rmi_core-$(CONFIG_RMI4_F21) += rmi_f21.o
rmi_core-$(CONFIG_RMI4_F54) += rmi_f54.o
rmi_core-$(CONFIG_RMI4_F55) += rmi_f55.o
diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
index f2e093b0b998..e12033aa7517 100644
--- a/drivers/input/rmi4/rmi_bus.c
+++ b/drivers/input/rmi4/rmi_bus.c
@@ -369,6 +369,9 @@ static struct rmi_function_handler *fn_handlers[] = {
#ifdef CONFIG_RMI4_F3A
&rmi_f3a_handler,
#endif
+#ifdef CONFIG_RMI4_F21
+ &rmi_f21_handler,
+#endif
#ifdef CONFIG_RMI4_F54
&rmi_f54_handler,
#endif
diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
index 1c6c6086c0e5..57f258371bab 100644
--- a/drivers/input/rmi4/rmi_driver.h
+++ b/drivers/input/rmi4/rmi_driver.h
@@ -114,7 +114,9 @@ static inline int rmi_f03_overwrite_button(struct rmi_function *fn,
}
static inline void rmi_f03_commit_buttons(struct rmi_function *fn) {}
#endif
-
+#ifdef CONFIG_RMI4_F21
+int rmi_f21_report_pressure(struct rmi_function *fn, int i);
+#endif
#ifdef CONFIG_RMI4_F34
int rmi_f34_create_sysfs(struct rmi_device *rmi_dev);
void rmi_f34_remove_sysfs(struct rmi_device *rmi_dev);
@@ -136,6 +138,7 @@ extern struct rmi_function_handler rmi_f12_handler;
extern struct rmi_function_handler rmi_f30_handler;
extern struct rmi_function_handler rmi_f34_handler;
extern struct rmi_function_handler rmi_f3a_handler;
+extern struct rmi_function_handler rmi_f21_handler;
extern struct rmi_function_handler rmi_f54_handler;
extern struct rmi_function_handler rmi_f55_handler;
#endif
diff --git a/drivers/input/rmi4/rmi_f21.c b/drivers/input/rmi4/rmi_f21.c
new file mode 100644
index 000000000000..5657bf4a2026
--- /dev/null
+++ b/drivers/input/rmi4/rmi_f21.c
@@ -0,0 +1,110 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2012-2020 Synaptics Incorporated
+ */
+
+#include <linux/kernel.h>
+#include <linux/rmi.h>
+#include <linux/input.h>
+#include <linux/slab.h>
+#include "rmi_driver.h"
+
+#define RMI_F21_FORCE_CLICK_OFFSET 8
+#define RMI_f21_FORCE_CLICK 0x01
+#define RMI_f21_DATA_REGS_MAX_SIZE 1
+#define RMI_f21_FORCEPAD_BUTTON_COUNT 1
+
+struct f21_data {
+ /* Query Data */
+ u8 data_regs[RMI_f21_DATA_REGS_MAX_SIZE];
+ struct input_dev *input;
+ u16 key_code;
+};
+
+static irqreturn_t rmi_f21_attention(int irq, void *ctx)
+{
+ struct rmi_function *fn = ctx;
+ struct f21_data *f21 = dev_get_drvdata(&fn->dev);
+ int error;
+
+ error = rmi_read_block(fn->rmi_dev,
+ fn->fd.data_base_addr+RMI_F21_FORCE_CLICK_OFFSET,
+ f21->data_regs, 1);
+ if (error) {
+ dev_err(&fn->dev,
+ "%s: Failed to read f21 data registers: %d\n",
+ __func__, error);
+ return IRQ_RETVAL(error);
+ }
+
+ if (!!(f21->data_regs[0] & RMI_f21_FORCE_CLICK))
+ input_report_key(f21->input, f21->key_code, true);
+ else
+ input_report_key(f21->input, f21->key_code, false);
+ return IRQ_HANDLED;
+}
+
+static int rmi_f21_config(struct rmi_function *fn)
+{
+ struct f21_data *f21 = dev_get_drvdata(&fn->dev);
+ struct rmi_driver *drv = fn->rmi_dev->driver;
+
+ if (!f21)
+ return 0;
+
+ drv->set_irq_bits(fn->rmi_dev, fn->irq_mask);
+
+ return 0;
+}
+
+static int rmi_f21_initialize(struct rmi_function *fn, struct f21_data *f21)
+{
+ struct input_dev *input = f21->input;
+ unsigned int button = BTN_LEFT;
+
+ f21->key_code = button;
+ input_set_capability(input, EV_KEY, f21->key_code);
+ input->keycode = &(f21->key_code);
+ input->keycodesize = sizeof(f21->key_code);
+ input->keycodemax = RMI_f21_FORCEPAD_BUTTON_COUNT;
+
+ __set_bit(INPUT_PROP_BUTTONPAD, input->propbit);
+
+ return 0;
+}
+
+static int rmi_f21_probe(struct rmi_function *fn)
+{
+ struct rmi_device *rmi_dev = fn->rmi_dev;
+ struct rmi_driver_data *drv_data = dev_get_drvdata(&rmi_dev->dev);
+ struct f21_data *f21;
+ int error;
+
+ if (!drv_data->input) {
+ dev_info(&fn->dev, "f21: no input device found, ignoring\n");
+ return -ENXIO;
+ }
+
+ f21 = devm_kzalloc(&fn->dev, sizeof(*f21), GFP_KERNEL);
+ if (!f21)
+ return -ENOMEM;
+
+ f21->input = drv_data->input;
+
+ error = rmi_f21_initialize(fn, f21);
+ if (error)
+ return error;
+
+ dev_set_drvdata(&fn->dev, f21);
+ return 0;
+}
+
+struct rmi_function_handler rmi_f21_handler = {
+ .driver = {
+ .name = "rmi4_f21",
+ },
+ .func = 0x21,
+ .probe = rmi_f21_probe,
+ .config = rmi_f21_config,
+ .attention = rmi_f21_attention,
+};
--
2.25.1
^ permalink raw reply related
* Re: [PATCH RFC 3/4] input: touchscreen: add I2C support for Goodix Berlin Touchscreen IC
From: Jeff LaBundy @ 2023-06-12 3:09 UTC (permalink / raw)
To: Neil Armstrong
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bastien Nocera, Hans de Goede, Henrik Rydberg, linux-input,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20230606-topic-goodix-berlin-upstream-initial-v1-3-4a0741b8aefd@linaro.org>
Hi Neil,
Excellent work; just a few nits on this particular instance.
On Tue, Jun 06, 2023 at 04:31:58PM +0200, Neil Armstrong wrote:
> Add initial support for the new Goodix "Berlin" touchscreen ICs
> over the I2C interface.
>
> 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
>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
> drivers/input/touchscreen/Kconfig | 14 +++++
> drivers/input/touchscreen/Makefile | 1 +
> drivers/input/touchscreen/goodix_berlin_i2c.c | 76 +++++++++++++++++++++++++++
> 3 files changed, 91 insertions(+)
>
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 1a6f6f6da991..da6d5d75c42d 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -421,6 +421,20 @@ config TOUCHSCREEN_GOODIX_BERLIN_CORE
> depends on REGMAP
> tristate
>
> +config TOUCHSCREEN_GOODIX_BERLIN_I2C
> + tristate "Goodix Berlin I2C touchscreen"
> + depends on I2C
> + depends on REGMAP_I2C
As you already depend upon REGMAP, I think you can simply select REGMAP_I2C
here instead of depending upon it as well.
> + select TOUCHSCREEN_GOODIX_BERLIN_CORE
> + help
> + Say Y here if you have the a touchscreen connected to your
> + system using the Goodix Berlin IC connection via I2C.
This language was a bit of a tongue-twister; perhaps it is better to say
"...if you have a Goodix Berlin IC connected to your system via I2C."
> +
> + If unsure, say N.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called goodix_berlin_i2c.
> +
> config TOUCHSCREEN_HIDEEP
> tristate "HiDeep Touch IC"
> depends on I2C
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index 29cdb042e104..921a2da0c2be 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_GOODIX_BERLIN_CORE) += goodix_berlin_core.o
> +obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_I2C) += goodix_berlin_i2c.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_i2c.c b/drivers/input/touchscreen/goodix_berlin_i2c.c
> new file mode 100644
> index 000000000000..fc32b8077287
> --- /dev/null
> +++ b/drivers/input/touchscreen/goodix_berlin_i2c.c
> @@ -0,0 +1,76 @@
> +// 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 <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/regmap.h>
> +#include <asm/unaligned.h>
This last include does not seem to provide anything.
> +
> +#include "goodix_berlin.h"
> +
> +#define I2C_MAX_TRANSFER_SIZE 256
> +
> +static const struct regmap_config goodix_berlin_i2c_regmap_conf = {
> + .reg_bits = 32,
> + .val_bits = 8,
> + .max_raw_read = I2C_MAX_TRANSFER_SIZE,
> + .max_raw_write = I2C_MAX_TRANSFER_SIZE,
> +};
> +
> +static const struct input_id goodix_berlin_i2c_input_id = {
> + .bustype = BUS_I2C,
> + .vendor = 0x0416,
> + .product = 0x1001,
> +};
> +
> +static int goodix_berlin_i2c_probe(struct i2c_client *client)
> +{
> + struct regmap *map;
'regmap' tends to be more common than simply 'map'.
> +
> + map = devm_regmap_init_i2c(client, &goodix_berlin_i2c_regmap_conf);
> + if (IS_ERR(map))
> + return PTR_ERR(map);
> +
> + return goodix_berlin_probe(&client->dev, client->irq,
> + &goodix_berlin_i2c_input_id, map);
> +}
> +
> +static void goodix_berlin_i2c_remove(struct i2c_client *client)
> +{
> + goodix_berlin_remove(&client->dev);
> +}
> +
> +static const struct i2c_device_id goodix_berlin_i2c_id[] = {
> + { "gt9916", 0 },
> + { }
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, goodix_berlin_i2c_id);
> +
> +static const struct of_device_id goodix_berlin_i2c_of_match[] = {
> + { .compatible = "goodix,gt9916", },
> + { },
No need for a trailing comma following the sentinel.
> +};
> +
> +static struct i2c_driver goodix_berlin_i2c_driver = {
> + .driver = {
> + .name = "goodix-berlin-i2c",
> + .of_match_table = goodix_berlin_i2c_of_match,
> + .pm = pm_sleep_ptr(&goodix_berlin_pm_ops),
> + },
> + .probe = goodix_berlin_i2c_probe,
> + .remove = goodix_berlin_i2c_remove,
> + .id_table = goodix_berlin_i2c_id,
> +};
> +module_i2c_driver(goodix_berlin_i2c_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Goodix Berlin I2C Touchscreen driver");
> +MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
>
> --
> 2.34.1
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH RFC 4/4] input: touchscreen: add SPI support for Goodix Berlin Touchscreen IC
From: Jeff LaBundy @ 2023-06-12 3:31 UTC (permalink / raw)
To: Neil Armstrong
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bastien Nocera, Hans de Goede, Henrik Rydberg, linux-input,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20230606-topic-goodix-berlin-upstream-initial-v1-4-4a0741b8aefd@linaro.org>
Hi Neil,
On Tue, Jun 06, 2023 at 04:31:59PM +0200, Neil Armstrong wrote:
> 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
>
> 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 | 183 ++++++++++++++++++++++++++
> 3 files changed, 198 insertions(+)
>
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index da6d5d75c42d..ffe0c0a4cd15 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -435,6 +435,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
> + depends on REGMAP
As TOUCHSCREEN_GOODIX_BERLIN_CORE already depends on REGMAP; is this
line necessary?
I was about to ask why not to select REGMAP_SPI; thank you for the
additional information in the commit message.
> + select TOUCHSCREEN_GOODIX_BERLIN_CORE
> + help
> + Say Y here if you have the a touchscreen connected to your
> + system using the Goodix Berlin IC connection via SPI.
Same comment here with regard to diction.
> +
> + 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 921a2da0c2be..29524e8a83db 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..0f4f650fdf3f
> --- /dev/null
> +++ b/drivers/input/touchscreen/goodix_berlin_spi.c
> @@ -0,0 +1,183 @@
> +// 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 <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/spi/spi.h>
> +#include <linux/regmap.h>
Please alphabetize these to aid readability.
> +#include <asm/unaligned.h>
> +
> +#include "goodix_berlin.h"
> +
> +#define SPI_TRANS_PREFIX_LEN 1
> +#define REGISTER_WIDTH 4
> +#define SPI_READ_DUMMY_LEN 3
> +#define SPI_READ_PREFIX_LEN (SPI_TRANS_PREFIX_LEN + REGISTER_WIDTH + SPI_READ_DUMMY_LEN)
> +#define SPI_WRITE_PREFIX_LEN (SPI_TRANS_PREFIX_LEN + REGISTER_WIDTH)
> +
> +#define SPI_WRITE_FLAG 0xF0
> +#define 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 = NULL;
> + int ret = 0;
No need to initialize these, only to forcibly assign them later.
> +
> + if (reg_size != REGISTER_WIDTH)
> + return -EINVAL;
> +
> + buf = kzalloc(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] = SPI_READ_FLAG;
> + put_unaligned_be32(*reg, buf + SPI_TRANS_PREFIX_LEN);
> + memset(buf + SPI_TRANS_PREFIX_LEN + REGISTER_WIDTH, 0xff,
> + SPI_READ_DUMMY_LEN);
> +
> + xfers.tx_buf = buf;
> + xfers.rx_buf = buf;
> + xfers.len = SPI_READ_PREFIX_LEN + val_size;
> + xfers.cs_change = 0;
> + spi_message_add_tail(&xfers, &spi_msg);
> +
> + ret = spi_sync(spi, &spi_msg);
> + if (ret < 0) {
> + dev_err(&spi->dev, "transfer error:%d", ret);
> + goto exit;
> + }
My comment is purely idiomatic, but this seems cleaner:
ret = ...
if (ret)
dev_err(...);
else
memcpy(...);
kfree(...);
return ret;
> +
> + memcpy(val_buf, buf + SPI_READ_PREFIX_LEN, val_size);
> +exit:
> + kfree(buf);
> + return ret;
> +}
> +
> +static int goodix_berlin_spi_write(void *context, const void *data,
> + size_t count)
> +{
> + unsigned int len = count - 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 = NULL;
> + int ret = 0;
Same comment here with regard to initialization.
> +
> + buf = kzalloc(SPI_WRITE_PREFIX_LEN + len, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + spi_message_init(&spi_msg);
> + memset(&xfers, 0, sizeof(xfers));
> +
> + buf[0] = SPI_WRITE_FLAG;
> + put_unaligned_be32(*reg, buf + SPI_TRANS_PREFIX_LEN);
> + memcpy(buf + SPI_WRITE_PREFIX_LEN, data + REGISTER_WIDTH, len);
> +
> + xfers.tx_buf = buf;
> + xfers.len = SPI_WRITE_PREFIX_LEN + len;
> + xfers.cs_change = 0;
> + spi_message_add_tail(&xfers, &spi_msg);
> +
> + ret = spi_sync(spi, &spi_msg);
> + if (ret < 0)
> + dev_err(&spi->dev, "transfer error:%d", ret);
> +
> + kfree(buf);
> + return ret;
> +}
> +
> +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,
> +};
> +
> +static const struct input_id goodix_berlin_spi_input_id = {
> + .bustype = BUS_SPI,
> + .vendor = 0x0416,
> + .product = 0x1001,
After having seen these in the I2C counterpart; consider defining them
in goodix_berlin.h.
> +};
> +
> +static int goodix_berlin_spi_probe(struct spi_device *spi)
> +{
> + struct regmap_config *cfg;
regmap_config
> + struct regmap *map;
regmap
(see more examples in MFD where such dual-mode devices are common).
> + size_t max_size;
> + int ret = 0;
> +
> + cfg = devm_kmemdup(&spi->dev, &goodix_berlin_spi_regmap_conf,
> + sizeof(*cfg), GFP_KERNEL);
> + if (!cfg)
> + return -ENOMEM;
> +
> + spi->mode = SPI_MODE_0;
> + spi->bits_per_word = 8;
> + ret = spi_setup(spi);
> + if (ret)
> + return ret;
> +
> + max_size = spi_max_transfer_size(spi);
> + cfg->max_raw_read = max_size - SPI_READ_PREFIX_LEN;
> + cfg->max_raw_write = max_size - SPI_WRITE_PREFIX_LEN;
> +
> + map = devm_regmap_init(&spi->dev, NULL, spi, cfg);
> + if (IS_ERR(map))
> + return PTR_ERR(map);
> +
> + return goodix_berlin_probe(&spi->dev, spi->irq,
> + &goodix_berlin_spi_input_id, map);
> +}
> +
> +static void goodix_berlin_spi_remove(struct spi_device *spi)
> +{
> + goodix_berlin_remove(&spi->dev);
> +}
> +
> +static const struct of_device_id goodix_berlin_spi_of_match[] = {
> + {
> + .compatible = "goodix,gt9916",
> + },
This format is different than its I2C counterpart.
> + { },
Nit: same comment with regards to trailing commas.
> +};
> +MODULE_DEVICE_TABLE(of, goodix_berlin_spi_of_match);
> +
> +static const struct spi_device_id goodix_berlin_spi_ids[] = {
> + { "gt9916" },
> + { },
And here.
> +};
> +MODULE_DEVICE_TABLE(spi, goodix_berlin_spi_ids);
> +
> +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),
> + },
> + .id_table = goodix_berlin_spi_ids,
> + .probe = goodix_berlin_spi_probe,
> + .remove = goodix_berlin_spi_remove,
> +};
> +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
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH RFC 2/4] input: touchscreen: add core support for Goodix Berlin Touchscreen IC
From: Jeff LaBundy @ 2023-06-12 4:45 UTC (permalink / raw)
To: Neil Armstrong
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bastien Nocera, Hans de Goede, Henrik Rydberg, linux-input,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20230606-topic-goodix-berlin-upstream-initial-v1-2-4a0741b8aefd@linaro.org>
Hi Neil,
On Tue, Jun 06, 2023 at 04:31:57PM +0200, Neil Armstrong wrote:
> Add initial support for the new Goodix "Berlin" touchscreen ICs.
>
> 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>
> ---
> drivers/input/touchscreen/Kconfig | 5 +
> drivers/input/touchscreen/Makefile | 1 +
> drivers/input/touchscreen/goodix_berlin.h | 228 ++++++
> drivers/input/touchscreen/goodix_berlin_core.c | 935 +++++++++++++++++++++++++
> 4 files changed, 1169 insertions(+)
>
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index c2cbd332af1d..1a6f6f6da991 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -416,6 +416,11 @@ config TOUCHSCREEN_GOODIX
> To compile this driver as a module, choose M here: the
> module will be called goodix.
>
> +config TOUCHSCREEN_GOODIX_BERLIN_CORE
> + depends on GPIOLIB || COMPILE_TEST
> + depends on REGMAP
> + tristate
> +
> config TOUCHSCREEN_HIDEEP
> tristate "HiDeep Touch IC"
> depends on I2C
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index 159cd5136fdb..29cdb042e104 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -47,6 +47,7 @@ obj-$(CONFIG_TOUCHSCREEN_EGALAX_SERIAL) += egalax_ts_serial.o
> obj-$(CONFIG_TOUCHSCREEN_EXC3000) += exc3000.o
> 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_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.h b/drivers/input/touchscreen/goodix_berlin.h
> new file mode 100644
> index 000000000000..83f16201c354
> --- /dev/null
> +++ b/drivers/input/touchscreen/goodix_berlin.h
> @@ -0,0 +1,228 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Goodix Touchscreen Driver
> + * Copyright (C) 2020 - 2021 Goodix, Inc.
> + * Copyright (C) 2023 Linaro Ltd.
> + *
> + * Based on goodix_berlin_berlin driver.
> + */
> +
> +#ifndef __GOODIX_BERLIN_H_
> +#define __GOODIX_BERLIN_H_
> +
> +#include <linux/input.h>
> +#include <linux/of_gpio.h>
> +#include <linux/input/touchscreen.h>
> +#include <linux/regulator/consumer.h>
> +
> +#define GOODIX_MAX_TOUCH 10
> +#define GOODIX_MAX_FRAMEDATA_LEN 1700
> +
> +#define GOODIX_NORMAL_RESET_DELAY_MS 100
> +#define GOODIX_HOLD_CPU_RESET_DELAY_MS 5
> +
> +enum CHECKSUM_MODE {
> + CHECKSUM_MODE_U8_LE,
> + CHECKSUM_MODE_U16_LE,
> +};
These don't seem to be used anywhere.
Understanding that vendor drivers are typically saddled with lots of cruft
as a result of having supported many different ICs and customers, please
prune the driver for these kinds of things for v1.
> +
> +#define MAX_SCAN_FREQ_NUM 8
> +#define MAX_SCAN_RATE_NUM 8
> +#define MAX_FREQ_NUM_STYLUS 8
> +
> +#define IRQ_EVENT_HEAD_LEN 8
> +#define BYTES_PER_POINT 8
> +#define COOR_DATA_CHECKSUM_SIZE 2
> +
> +#define GOODIX_TOUCH_EVENT BIT(7)
> +#define GOODIX_REQUEST_EVENT BIT(6)
> +#define GOODIX_GESTURE_EVENT BIT(5)
> +
> +#define POINT_TYPE_STYLUS_HOVER 0x01
> +#define POINT_TYPE_STYLUS 0x03
> +
> +#define DEV_CONFIRM_VAL 0xAA
> +#define BOOTOPTION_ADDR 0x10000
> +#define FW_VERSION_INFO_ADDR 0x10014
> +
> +#define GOODIX_IC_INFO_MAX_LEN 1024
> +#define GOODIX_IC_INFO_ADDR 0x10070
> +
> +enum brl_request_code {
> + BRL_REQUEST_CODE_CONFIG = 1,
> + BRL_REQUEST_CODE_REF_ERR = 2,
> + BRL_REQUEST_CODE_RESET = 3,
> + BRL_REQUEST_CODE_CLOCK = 4,
> +};
> +
> +struct goodix_fw_version {
> + u8 rom_pid[6]; /* rom PID */
This comment is obvious and noisy; there are similar instances below.
> + u8 rom_vid[3]; /* Mask VID */
> + u8 rom_vid_reserved;
> + u8 patch_pid[8]; /* Patch PID */
> + u8 patch_vid[4]; /* Patch VID */
> + u8 patch_vid_reserved;
> + u8 sensor_id;
> + u8 reserved[2];
> + u16 checksum;
> +} __packed;
> +
> +struct goodix_ic_info_version {
> + u8 info_customer_id;
> + u8 info_version_id;
> + u8 ic_die_id;
> + u8 ic_version_id;
> + u32 config_id;
> + u8 config_version;
> + u8 frame_data_customer_id;
> + u8 frame_data_version_id;
> + u8 touch_data_customer_id;
> + u8 touch_data_version_id;
> + u8 reserved[3];
> +} __packed;
> +
> +struct goodix_ic_info_feature { /* feature info*/
> + u16 freqhop_feature;
> + u16 calibration_feature;
> + u16 gesture_feature;
> + u16 side_touch_feature;
> + u16 stylus_feature;
> +} __packed;
> +
> +struct goodix_ic_info_param { /* param */
> + u8 drv_num;
> + u8 sen_num;
> + u8 button_num;
> + u8 force_num;
> + u8 active_scan_rate_num;
> + u16 active_scan_rate[MAX_SCAN_RATE_NUM];
> + u8 mutual_freq_num;
> + u16 mutual_freq[MAX_SCAN_FREQ_NUM];
> + u8 self_tx_freq_num;
> + u16 self_tx_freq[MAX_SCAN_FREQ_NUM];
> + u8 self_rx_freq_num;
> + u16 self_rx_freq[MAX_SCAN_FREQ_NUM];
> + u8 stylus_freq_num;
> + u16 stylus_freq[MAX_FREQ_NUM_STYLUS];
> +} __packed;
> +
> +struct goodix_ic_info_misc { /* other data */
> + u32 cmd_addr;
> + u16 cmd_max_len;
> + u32 cmd_reply_addr;
> + u16 cmd_reply_len;
> + u32 fw_state_addr;
> + u16 fw_state_len;
> + u32 fw_buffer_addr;
> + u16 fw_buffer_max_len;
> + u32 frame_data_addr;
> + u16 frame_data_head_len;
> + u16 fw_attr_len;
> + u16 fw_log_len;
> + u8 pack_max_num;
> + u8 pack_compress_version;
> + u16 stylus_struct_len;
> + u16 mutual_struct_len;
> + u16 self_struct_len;
> + u16 noise_struct_len;
> + u32 touch_data_addr;
> + u16 touch_data_head_len;
> + u16 point_struct_len;
> + u16 reserved1;
> + u16 reserved2;
> + u32 mutual_rawdata_addr;
> + u32 mutual_diffdata_addr;
> + u32 mutual_refdata_addr;
> + u32 self_rawdata_addr;
> + u32 self_diffdata_addr;
> + u32 self_refdata_addr;
> + u32 iq_rawdata_addr;
> + u32 iq_refdata_addr;
> + u32 im_rawdata_addr;
> + u16 im_readata_len;
> + u32 noise_rawdata_addr;
> + u16 noise_rawdata_len;
> + u32 stylus_rawdata_addr;
> + u16 stylus_rawdata_len;
> + u32 noise_data_addr;
> + u32 esd_addr;
> +} __packed;
> +
> +struct goodix_ic_info {
> + u16 length;
> + struct goodix_ic_info_version version;
> + struct goodix_ic_info_feature feature;
> + struct goodix_ic_info_param parm;
> + struct goodix_ic_info_misc misc;
> +} __packed;
> +
> +/* interrupt event type */
> +enum ts_event_type {
> + EVENT_INVALID,
> + EVENT_TOUCH, /* finger touch event */
> + EVENT_REQUEST,
> +};
Please consistently namespace all of these, i.e. goodix_berlin_...
> +
> +enum ts_request_type {
> + REQUEST_TYPE_RESET,
> +};
And here.
> +
> +enum touch_point_status {
> + TS_NONE,
> + TS_RELEASE,
> + TS_TOUCH,
> +};
> +
> +/* coordinate package */
> +struct goodix_berlin_coords {
> + int status; /* NONE, RELEASE, TOUCH */
enum touch_point_status status; (without comment, and namespace fixed)
> + unsigned int x, y, w, p;
> +};
> +
> +/* touch event data */
> +struct goodix_touch_data {
> + int touch_num;
> + struct goodix_berlin_coords coords[GOODIX_MAX_TOUCH];
> +};
> +
> +/* touch event struct */
> +struct goodix_berlin_event {
> + enum ts_event_type event_type;
> + u8 request_code; /* represent the request type */
> + struct goodix_touch_data touch_data;
> +};
> +
> +struct goodix_berlin_core {
> + struct device *dev;
> + struct regmap *regmap;
It is just my opinion, but I find these newlines to be unnecessary.
> +
> + struct regulator *avdd;
> + struct regulator *iovdd;
> + struct gpio_desc *reset_gpio;
> +
> + struct touchscreen_properties props;
> +
> + struct goodix_fw_version fw_version;
> + struct goodix_ic_info ic_info;
> +
> + struct input_dev *input_dev;
> + struct goodix_berlin_event ts_event;
> +
> + int irq;
> +
> + struct dentry *debugfs_root;
> +};
> +
> +u32 goodix_append_checksum(u8 *data, int len, int mode);
> +int checksum_cmp(const u8 *data, int size, int mode);
> +int is_risk_data(const u8 *data, int size);
> +u32 goodix_get_file_config_id(u8 *ic_config);
> +void goodix_rotate_abcd2cbad(int tx, int rx, s16 *data);
> +
> +int goodix_berlin_probe(struct device *dev, int irq, const struct input_id *id,
> + struct regmap *regmap);
> +int goodix_berlin_remove(struct device *dev);
> +
> +extern const struct dev_pm_ops goodix_berlin_pm_ops;
> +
> +#endif
> diff --git a/drivers/input/touchscreen/goodix_berlin_core.c b/drivers/input/touchscreen/goodix_berlin_core.c
> new file mode 100644
> index 000000000000..0719690a5bd3
> --- /dev/null
> +++ b/drivers/input/touchscreen/goodix_berlin_core.c
> @@ -0,0 +1,935 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Goodix Touchscreen Driver
> + * Copyright (C) 2020 - 2021 Goodix, Inc.
> + * Copyright (C) 2023 Linaro Ltd.
> + *
> + * Based on goodix_ts_berlin driver.
> + */
> +#include <linux/regmap.h>
> +#include <linux/input/mt.h>
> +#include <linux/input/touchscreen.h>
> +#include <linux/debugfs.h>
Please sort these alphabetically.
> +
> +#include "goodix_berlin.h"
> +
> +/*
> + * Goodix "Berlin" Touchscreen ID driver
> + *
> + * Currently only handles Multitouch events with already
> + * programmed firmware and "config" for "Revision D" Berlin IC.
> + *
> + * Support is missing for:
> + * - ESD Management
> + * - Firmware update/flashing
> + * - "Config" update/flashing
> + * - PEN Events
What is "PEN"; is it "stylus" as written elsewhere?
> + * - Gesture Events
> + * - Support for older revisions (A, B & C)
> + */
> +
> +static bool goodix_berlin_check_checksum(const u8 *data, int size)
> +{
> + u32 cal_checksum = 0;
> + u32 r_checksum = 0;
> + u32 i;
> +
> + if (size < COOR_DATA_CHECKSUM_SIZE)
> + return false;
> +
> + for (i = 0; i < size - COOR_DATA_CHECKSUM_SIZE; i++)
> + cal_checksum += data[i];
> +
> + r_checksum += data[i++];
> + r_checksum += (data[i] << 8);
> +
> + return (cal_checksum & 0xFFFF) == r_checksum;
> +}
> +
> +static bool goodix_berlin_is_risk_data(struct goodix_berlin_core *cd,
> + const u8 *data, int size)
> +{
> + int zero_count = 0;
> + int ff_count = 0;
> + int i;
> +
> + for (i = 0; i < size; i++) {
> + if (data[i] == 0)
> + zero_count++;
> + else if (data[i] == 0xff)
> + ff_count++;
> + }
> + if (zero_count == size || ff_count == size) {
> + dev_warn(cd->dev, "warning data is all %s\n",
> + zero_count == size ? "zero" : "0xff");
> + return true;
> + }
> +
> + return false;
> +}
> +
> +static int goodix_berlin_dev_confirm(struct goodix_berlin_core *cd)
> +{
> + u8 tx_buf[8] = { 0 };
> + u8 rx_buf[8] = { 0 };
> + int retry = 3;
> + int ret = 0;
No need to initialize 'ret' here.
Please note however that in input, return values which can either return
zero (success) or an error code are typically called 'error' since we are
quite literally evaluating an error code. Therefore:
int error;
memset(...);
while (...) {
error = ...;
if (error)
return error;
[...]
}
> +
> + memset(tx_buf, DEV_CONFIRM_VAL, sizeof(tx_buf));
> + while (retry--) {
> + ret = regmap_raw_write(cd->regmap, BOOTOPTION_ADDR, tx_buf,
> + sizeof(tx_buf));
> + if (ret < 0)
> + return ret;
> + ret = regmap_raw_read(cd->regmap, BOOTOPTION_ADDR, rx_buf,
> + sizeof(rx_buf));
> + if (ret < 0)
> + return ret;
> + if (!memcmp(tx_buf, rx_buf, sizeof(tx_buf)))
> + break;
> + usleep_range(5000, 5100);
> + }
> +
> + if (retry < 0) {
> + ret = -EINVAL;
> + dev_err(cd->dev, "device confirm failed, rx_buf: %*ph\n", 8, rx_buf);
> + }
> +
> + return ret;
> +}
> +
> +static int goodix_berlin_power_on(struct goodix_berlin_core *cd, bool on)
> +{
> + int ret = 0;
> +
> + if (on) {
> + ret = regulator_enable(cd->iovdd);
> + if (ret < 0) {
> + dev_err(cd->dev, "Failed to enable iovdd: %d\n", ret);
> + goto power_off;
This initially struck me as an imbalance; can you let me know in case I have
misunderstood? If we fail here, we will disable both regulators despite only
have attempted to enable the first.
Also, I do not believe failing to enable a regulator is sufficient evidence
to subsequently disable it.
> + }
> +
Let us have a comment to explain why 3 ms is special.
> + usleep_range(3000, 3100);
> +
> + ret = regulator_enable(cd->avdd);
> + if (ret < 0) {
> + dev_err(cd->dev, "Failed to enable avdd: %d\n", ret);
> + goto power_off;
> + }
> +
> + usleep_range(15000, 15100);
And here.
> +
> + gpiod_set_value(cd->reset_gpio, 0);
> +
> + usleep_range(4000, 4100);
> +
> + ret = goodix_berlin_dev_confirm(cd);
> + if (ret < 0)
> + goto power_off;
> +
> + msleep(GOODIX_NORMAL_RESET_DELAY_MS);
> +
> + return 0;
> + }
> +
> +power_off:
> + gpiod_set_value(cd->reset_gpio, 1);
> + regulator_disable(cd->iovdd);
> + regulator_disable(cd->avdd);
> + return ret;
> +}
> +
> +static int goodix_berlin_do_suspend(struct goodix_berlin_core *cd)
> +{
> + if (goodix_berlin_power_on(cd, 0))
> + dev_err(cd->dev, "failed power off\n");
> +
> + return 0;
> +}
> +
> +static int goodix_berlin_do_resume(struct goodix_berlin_core *cd)
> +{
> + int ret = 0;
> +
> + ret = goodix_berlin_power_on(cd, 1);
> + if (ret) {
> + dev_err(cd->dev, "failed power on\n");
> + return ret;
> + }
> +
> + return ret;
> +}
> +
> +static int goodix_berlin_reset(struct goodix_berlin_core *cd, int delay)
> +{
> + gpiod_set_value(cd->reset_gpio, 1);
> + usleep_range(2000, 2100);
> + gpiod_set_value(cd->reset_gpio, 0);
> +
> + if (delay < 20)
> + usleep_range(delay * 1000, delay * 1000 + 100);
> + else
> + msleep(delay);
This seems to duplicate fsleep. That being said, why is this logic necessary
at all? This function seems to only be called once; there is no need to make
a decision based on the delay...you already know the value.
> +
> + return 0;
> +}
> +
> +static int goodix_berlin_irq_enable(struct goodix_berlin_core *cd, bool enable)
> +{
> + if (enable)
> + enable_irq(cd->irq);
> + else
> + disable_irq(cd->irq);
Why to wrap this logic? While I agree doing so is common for vendor drivers,
that does not make it correct.
> +
> + return 0;
> +}
> +
> +static int goodix_berlin_read_version(struct goodix_berlin_core *cd,
> + struct goodix_fw_version *version)
> +{
> + u8 buf[sizeof(struct goodix_fw_version)] = { 0 };
Same comment here with regard to initialization of immediately overwritten
variables.
> + int ret, i;
> +
> + for (i = 0; i < 2; i++) {
> + ret = regmap_raw_read(cd->regmap, FW_VERSION_INFO_ADDR, buf, sizeof(buf));
> + if (ret) {
> + dev_dbg(cd->dev, "read fw version: %d, retry %d\n", ret, i);
> + usleep_range(5000, 5100);
> + continue;
> + }
> +
> + if (goodix_berlin_check_checksum(buf, sizeof(buf)))
> + break;
> +
> + dev_dbg(cd->dev, "invalid fw version: checksum error\n");
> +
> + ret = -EINVAL;
> + usleep_range(10000, 11000);
Let us optimize this logic here; no need to sleep for 10 ms only to fail
upon the last try.
> + }
> +
> + if (ret) {
> + dev_err(cd->dev, "failed to get fw version");
> + return ret;
> + }
> +
> + memcpy(version, buf, sizeof(*version));
> +
> + return 0;
> +}
> +
> +static int goodix_berlin_fw_version_show(struct seq_file *s, void *unused)
> +{
> + struct goodix_berlin_core *cd = s->private;
> + struct goodix_fw_version *version = &cd->fw_version;
> + u8 temp_pid[9] = { 0 };
And here, with regard to initialization.
> +
> + memcpy(temp_pid, version->rom_pid, sizeof(version->rom_pid));
> + temp_pid[sizeof(version->rom_pid)] = '\0';
> +
> + seq_printf(s, "rom_pid: %s\n", temp_pid);
> + seq_printf(s, "rom_vid: %*ph\n",
> + (int)sizeof(version->rom_vid),
> + version->rom_vid);
> +
> + memcpy(temp_pid, version->patch_pid, sizeof(version->patch_pid));
> + temp_pid[sizeof(version->patch_pid)] = '\0';
> +
> + seq_printf(s, "patch_pid: %s\n", temp_pid);
> + seq_printf(s, "patch_vid: %*ph\n",
> + (int)sizeof(version->patch_vid),
> + version->patch_vid);
> + seq_printf(s, "sensor_id: %d\n",
> + version->sensor_id);
> +
> + return 0;
> +}
> +
> +DEFINE_SHOW_ATTRIBUTE(goodix_berlin_fw_version);
> +
> +static int goodix_berlin_convert_ic_info(struct goodix_berlin_core *cd,
> + struct goodix_ic_info *info,
> + const u8 *data)
> +{
> + int i;
> + struct goodix_ic_info_version *version = &info->version;
> + struct goodix_ic_info_feature *feature = &info->feature;
> + struct goodix_ic_info_param *parm = &info->parm;
> + struct goodix_ic_info_misc *misc = &info->misc;
> +
> + info->length = le16_to_cpup((__le16 *)data);
> +
> + data += 2;
> + memcpy(version, data, sizeof(*version));
> + version->config_id = le32_to_cpu(version->config_id);
> +
> + data += sizeof(struct goodix_ic_info_version);
> + memcpy(feature, data, sizeof(*feature));
> + feature->freqhop_feature = le16_to_cpu(feature->freqhop_feature);
> + feature->calibration_feature =
> + le16_to_cpu(feature->calibration_feature);
> + feature->gesture_feature = le16_to_cpu(feature->gesture_feature);
> + feature->side_touch_feature = le16_to_cpu(feature->side_touch_feature);
> + feature->stylus_feature = le16_to_cpu(feature->stylus_feature);
> +
> + data += sizeof(struct goodix_ic_info_feature);
> + parm->drv_num = *(data++);
> + parm->sen_num = *(data++);
> + parm->button_num = *(data++);
> + parm->force_num = *(data++);
> + parm->active_scan_rate_num = *(data++);
> +
> + if (parm->active_scan_rate_num > MAX_SCAN_RATE_NUM) {
> + dev_err(cd->dev, "invalid scan rate num %d > %d\n",
> + parm->active_scan_rate_num, MAX_SCAN_RATE_NUM);
> + return -EINVAL;
> + }
> +
> + for (i = 0; i < parm->active_scan_rate_num; i++)
> + parm->active_scan_rate[i] =
> + le16_to_cpup((__le16 *)(data + i * 2));
> +
> + data += parm->active_scan_rate_num * 2;
> + parm->mutual_freq_num = *(data++);
> +
> + if (parm->mutual_freq_num > MAX_SCAN_FREQ_NUM) {
> + dev_err(cd->dev, "invalid mntual freq num %d > %d\n",
> + parm->mutual_freq_num, MAX_SCAN_FREQ_NUM);
> + return -EINVAL;
> + }
> +
> + for (i = 0; i < parm->mutual_freq_num; i++)
> + parm->mutual_freq[i] = le16_to_cpup((__le16 *)(data + i * 2));
> +
> + data += parm->mutual_freq_num * 2;
> + parm->self_tx_freq_num = *(data++);
> +
> + if (parm->self_tx_freq_num > MAX_SCAN_FREQ_NUM) {
> + dev_err(cd->dev, "invalid tx freq num %d > %d\n",
> + parm->self_tx_freq_num, MAX_SCAN_FREQ_NUM);
> + return -EINVAL;
> + }
> +
> + for (i = 0; i < parm->self_tx_freq_num; i++)
> + parm->self_tx_freq[i] = le16_to_cpup((__le16 *)(data + i * 2));
> +
> + data += parm->self_tx_freq_num * 2;
> + parm->self_rx_freq_num = *(data++);
> +
> + if (parm->self_rx_freq_num > MAX_SCAN_FREQ_NUM) {
> + dev_err(cd->dev, "invalid rx freq num %d > %d\n",
> + parm->self_rx_freq_num, MAX_SCAN_FREQ_NUM);
> + return -EINVAL;
> + }
> +
> + for (i = 0; i < parm->self_rx_freq_num; i++)
> + parm->self_rx_freq[i] = le16_to_cpup((__le16 *)(data + i * 2));
> +
> + data += parm->self_rx_freq_num * 2;
> + parm->stylus_freq_num = *(data++);
> +
> + if (parm->stylus_freq_num > MAX_FREQ_NUM_STYLUS) {
> + dev_err(cd->dev, "invalid stylus freq num %d > %d\n",
> + parm->stylus_freq_num, MAX_FREQ_NUM_STYLUS);
> + return -EINVAL;
> + }
> +
> + for (i = 0; i < parm->stylus_freq_num; i++)
> + parm->stylus_freq[i] = le16_to_cpup((__le16 *)(data + i * 2));
> +
> + data += parm->stylus_freq_num * 2;
> + memcpy(misc, data, sizeof(*misc));
> + misc->cmd_addr = le32_to_cpu(misc->cmd_addr);
> + misc->cmd_max_len = le16_to_cpu(misc->cmd_max_len);
> + misc->cmd_reply_addr = le32_to_cpu(misc->cmd_reply_addr);
> + misc->cmd_reply_len = le16_to_cpu(misc->cmd_reply_len);
> + misc->fw_state_addr = le32_to_cpu(misc->fw_state_addr);
> + misc->fw_state_len = le16_to_cpu(misc->fw_state_len);
> + misc->fw_buffer_addr = le32_to_cpu(misc->fw_buffer_addr);
> + misc->fw_buffer_max_len = le16_to_cpu(misc->fw_buffer_max_len);
> + misc->frame_data_addr = le32_to_cpu(misc->frame_data_addr);
> + misc->frame_data_head_len = le16_to_cpu(misc->frame_data_head_len);
> +
> + misc->fw_attr_len = le16_to_cpu(misc->fw_attr_len);
> + misc->fw_log_len = le16_to_cpu(misc->fw_log_len);
> + misc->stylus_struct_len = le16_to_cpu(misc->stylus_struct_len);
> + misc->mutual_struct_len = le16_to_cpu(misc->mutual_struct_len);
> + misc->self_struct_len = le16_to_cpu(misc->self_struct_len);
> + misc->noise_struct_len = le16_to_cpu(misc->noise_struct_len);
> + misc->touch_data_addr = le32_to_cpu(misc->touch_data_addr);
> + misc->touch_data_head_len = le16_to_cpu(misc->touch_data_head_len);
> + misc->point_struct_len = le16_to_cpu(misc->point_struct_len);
> + misc->mutual_rawdata_addr = le32_to_cpu(misc->mutual_rawdata_addr);
> + misc->mutual_diffdata_addr = le32_to_cpu(misc->mutual_diffdata_addr);
> + misc->mutual_refdata_addr = le32_to_cpu(misc->mutual_refdata_addr);
> + misc->self_rawdata_addr = le32_to_cpu(misc->self_rawdata_addr);
> + misc->self_diffdata_addr = le32_to_cpu(misc->self_diffdata_addr);
> + misc->self_refdata_addr = le32_to_cpu(misc->self_refdata_addr);
> + misc->iq_rawdata_addr = le32_to_cpu(misc->iq_rawdata_addr);
> + misc->iq_refdata_addr = le32_to_cpu(misc->iq_refdata_addr);
> + misc->im_rawdata_addr = le32_to_cpu(misc->im_rawdata_addr);
> + misc->im_readata_len = le16_to_cpu(misc->im_readata_len);
> + misc->noise_rawdata_addr = le32_to_cpu(misc->noise_rawdata_addr);
> + misc->noise_rawdata_len = le16_to_cpu(misc->noise_rawdata_len);
> + misc->stylus_rawdata_addr = le32_to_cpu(misc->stylus_rawdata_addr);
> + misc->stylus_rawdata_len = le16_to_cpu(misc->stylus_rawdata_len);
> + misc->noise_data_addr = le32_to_cpu(misc->noise_data_addr);
> + misc->esd_addr = le32_to_cpu(misc->esd_addr);
What is all of this stuff for? The beginning of the driver explicitly states
that ESD recovery is not supported; please consider stripping all of this if
its only purpose is to support noise in the logs or sysfs attributes that we
cannot reasonably react to.
> +
> + return 0;
> +}
> +
> +/* Convert to debugfs */
This comment is unnecessary.
> +static int goodix_berlin_ic_info_show(struct seq_file *s, void *unused)
> +{
> + struct goodix_berlin_core *cd = s->private;
> + struct goodix_ic_info_version *version = &cd->ic_info.version;
> + struct goodix_ic_info_feature *feature = &cd->ic_info.feature;
> + struct goodix_ic_info_param *parm = &cd->ic_info.parm;
> + struct goodix_ic_info_misc *misc = &cd->ic_info.misc;
> +
> + seq_printf(s, "ic_info_length: %d\n", cd->ic_info.length);
> + seq_printf(s, "info_customer_id: 0x%01X\n",
> + version->info_customer_id);
> + seq_printf(s, "info_version_id: 0x%01X\n",
> + version->info_version_id);
> + seq_printf(s, "ic_die_id: 0x%01X\n",
> + version->ic_die_id);
> + seq_printf(s, "ic_version_id: 0x%01X\n",
> + version->ic_version_id);
> + seq_printf(s, "config_id: 0x%4X\n",
> + version->config_id);
> + seq_printf(s, "config_version: 0x%01X\n",
> + version->config_version);
> + seq_printf(s, "frame_data_customer_id: 0x%01X\n",
> + version->frame_data_customer_id);
> + seq_printf(s, "frame_data_version_id: 0x%01X\n",
> + version->frame_data_version_id);
> + seq_printf(s, "touch_data_customer_id: 0x%01X\n",
> + version->touch_data_customer_id);
> + seq_printf(s, "touch_data_version_id: 0x%01X\n",
> + version->touch_data_version_id);
> + seq_printf(s, "freqhop_feature: 0x%04X\n",
> + feature->freqhop_feature);
> + seq_printf(s, "calibration_feature: 0x%04X\n",
> + feature->calibration_feature);
> + seq_printf(s, "gesture_feature: 0x%04X\n",
> + feature->gesture_feature);
> + seq_printf(s, "side_touch_feature: 0x%04X\n",
> + feature->side_touch_feature);
> + seq_printf(s, "stylus_feature: 0x%04X\n",
> + feature->stylus_feature);
> + seq_printf(s, "Drv*Sen,Button,Force num: %d x %d, %d, %d\n",
> + parm->drv_num, parm->sen_num, parm->button_num, parm->force_num);
> + seq_printf(s, "Cmd: 0x%04X, %d\n",
> + misc->cmd_addr, misc->cmd_max_len);
> + seq_printf(s, "Cmd-Reply: 0x%04X, %d\n",
> + misc->cmd_reply_addr, misc->cmd_reply_len);
> + seq_printf(s, "FW-State: 0x%04X, %d\n",
> + misc->fw_state_addr, misc->fw_state_len);
> + seq_printf(s, "FW-Buffer: 0x%04X, %d\n",
> + misc->fw_buffer_addr, misc->fw_buffer_max_len);
> + seq_printf(s, "Touch-Data: 0x%04X, %d\n",
> + misc->touch_data_addr, misc->touch_data_head_len);
> + seq_printf(s, "point_struct_len: %d\n",
> + misc->point_struct_len);
> + seq_printf(s, "mutual_rawdata_addr: 0x%04X\n",
> + misc->mutual_rawdata_addr);
> + seq_printf(s, "mutual_diffdata_addr: 0x%04X\n",
> + misc->mutual_diffdata_addr);
> + seq_printf(s, "self_rawdata_addr: 0x%04X\n",
> + misc->self_rawdata_addr);
> + seq_printf(s, "self_diffdata_addr: 0x%04X\n",
> + misc->self_diffdata_addr);
> + seq_printf(s, "stylus_rawdata_addr: 0x%04X, %d\n",
> + misc->stylus_rawdata_addr, misc->stylus_rawdata_len);
> + seq_printf(s, "esd_addr: 0x%04X\n",
> + misc->esd_addr);
> +
> + return 0;
> +}
> +
> +DEFINE_SHOW_ATTRIBUTE(goodix_berlin_ic_info);
> +
> +static int goodix_berlin_get_ic_info(struct goodix_berlin_core *cd,
> + struct goodix_ic_info *ic_info)
> +{
> + int ret, i;
> + u16 length = 0;
> + u32 ic_addr;
> + u8 afe_data[GOODIX_IC_INFO_MAX_LEN] = { 0 };
> +
> + ic_addr = GOODIX_IC_INFO_ADDR;
> +
> + for (i = 0; i < 3; i++) {
> + ret = regmap_raw_read(cd->regmap, ic_addr, (u8 *)&length, sizeof(length));
> + if (ret) {
> + dev_info(cd->dev, "failed get ic info length, %d\n", ret);
> + usleep_range(5000, 5100);
> + continue;
> + }
> +
> + length = le16_to_cpu(length);
> + if (length >= GOODIX_IC_INFO_MAX_LEN) {
> + dev_info(cd->dev, "invalid ic info length %d, retry %d\n", length, i);
> + continue;
> + }
> +
> + ret = regmap_raw_read(cd->regmap, ic_addr, afe_data, length);
> + if (ret) {
> + dev_info(cd->dev, "failed get ic info data, %d\n", ret);
> + usleep_range(5000, 5100);
> + continue;
> + }
> +
> + /* judge whether the data is valid */
> + if (goodix_berlin_is_risk_data(cd, (const uint8_t *)afe_data,
> + length)) {
> + dev_info(cd->dev, "fw info data invalid\n");
> + usleep_range(5000, 5100);
> + continue;
> + }
> +
> + if (!goodix_berlin_check_checksum((const uint8_t *)afe_data, length)) {
> + dev_info(cd->dev, "fw info checksum error\n");
> + usleep_range(5000, 5100);
> + continue;
> + }
> +
> + break;
> + }
> + if (i == 3) {
> + dev_err(cd->dev, "failed get ic info\n");
> + return -EINVAL;
> + }
> +
> + ret = goodix_berlin_convert_ic_info(cd, ic_info, afe_data);
> + if (ret) {
> + dev_err(cd->dev, "error converting ic info\n");
> + return ret;
> + }
> +
> + /* check some key info */
> + if (!ic_info->misc.cmd_addr || !ic_info->misc.fw_buffer_addr ||
> + !ic_info->misc.touch_data_addr) {
> + dev_err(cd->dev, "cmd_addr fw_buf_addr and touch_data_addr is null\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int goodix_berlin_after_event_handler(struct goodix_berlin_core *cd)
> +{
> + struct goodix_ic_info_misc *misc = &cd->ic_info.misc;
> + u8 sync_clean = 0;
> +
> + return regmap_raw_write(cd->regmap, misc->touch_data_addr, &sync_clean, 1);
> +}
> +
> +static void goodix_berlin_parse_finger(struct goodix_berlin_core *cd,
> + struct goodix_touch_data *touch_data,
> + u8 *buf, int touch_num)
> +{
> + unsigned int id = 0, x = 0, y = 0, w = 0;
> + u8 *coor_data;
> + int i;
> +
> + coor_data = &buf[IRQ_EVENT_HEAD_LEN];
> +
> + for (i = 0; i < touch_num; i++) {
> + id = (coor_data[0] >> 4) & 0x0F;
> +
> + if (id >= GOODIX_MAX_TOUCH) {
> + dev_warn(cd->dev, "invalid finger id %d\n", id);
> +
> + touch_data->touch_num = 0;
> + return;
> + }
> +
> + x = le16_to_cpup((__le16 *)(coor_data + 2));
> + y = le16_to_cpup((__le16 *)(coor_data + 4));
> + w = le16_to_cpup((__le16 *)(coor_data + 6));
> +
> + touch_data->coords[id].status = TS_TOUCH;
> + touch_data->coords[id].x = x;
> + touch_data->coords[id].y = y;
> + touch_data->coords[id].w = w;
> +
> + coor_data += BYTES_PER_POINT;
> + }
> +
> + touch_data->touch_num = touch_num;
> +}
> +
> +static int goodix_berlin_touch_handler(struct goodix_berlin_core *cd,
> + u8 *pre_buf, u32 pre_buf_len)
> +{
> + static u8 buffer[IRQ_EVENT_HEAD_LEN + BYTES_PER_POINT * GOODIX_MAX_TOUCH + 2];
> + struct goodix_touch_data *touch_data = &cd->ts_event.touch_data;
> + struct goodix_ic_info_misc *misc = &cd->ic_info.misc;
> + u8 point_type = 0;
> + u8 touch_num = 0;
> + int ret = 0;
> +
> + /* clean event buffer */
> + memset(&cd->ts_event, 0, sizeof(cd->ts_event));
> +
> + /* copy pre-data to buffer */
> + memcpy(buffer, pre_buf, pre_buf_len);
> +
> + touch_num = buffer[2] & 0x0F;
> +
> + if (touch_num > GOODIX_MAX_TOUCH) {
> + dev_warn(cd->dev, "invalid touch num %d\n", touch_num);
> + return -EINVAL;
> + }
> +
> + /* read more data if more than 2 touch events */
> + if (unlikely(touch_num > 2)) {
> + ret = regmap_raw_read(cd->regmap,
> + misc->touch_data_addr + pre_buf_len,
> + &buffer[pre_buf_len],
> + (touch_num - 2) * BYTES_PER_POINT);
> + if (ret) {
> + dev_err(cd->dev, "failed get touch data\n");
> + return ret;
> + }
> + }
> +
> + /* read done */
> + goodix_berlin_after_event_handler(cd);
> +
> + if (touch_num == 0)
> + return 0;
> +
> + point_type = buffer[IRQ_EVENT_HEAD_LEN] & 0x0F;
> +
> + /* TOFIX: Not handled */
Same here; this comment is unnecessary as the print makes it obvious.
> + if (point_type == POINT_TYPE_STYLUS || point_type == POINT_TYPE_STYLUS_HOVER) {
> + dev_warn_once(cd->dev, "Stylus event type not handled\n");
> + return 0;
> + }
> +
> + if (!goodix_berlin_check_checksum(&buffer[IRQ_EVENT_HEAD_LEN],
> + touch_num * BYTES_PER_POINT + 2)) {
> + dev_dbg(cd->dev, "touch data checksum error\n");
> + dev_dbg(cd->dev, "data: %*ph\n",
> + touch_num * BYTES_PER_POINT + 2, &buffer[IRQ_EVENT_HEAD_LEN]);
> + return -EINVAL;
> + }
> +
> + cd->ts_event.event_type = EVENT_TOUCH;
> + goodix_berlin_parse_finger(cd, touch_data, buffer, touch_num);
> +
> + return 0;
> +}
> +
> +static int goodix_berlin_event_handler(struct goodix_berlin_core *cd)
> +{
> + struct goodix_ic_info_misc *misc = &cd->ic_info.misc;
> + int pre_read_len;
> + u8 pre_buf[32];
> + u8 event_status;
> + int ret;
> +
> + pre_read_len = IRQ_EVENT_HEAD_LEN + BYTES_PER_POINT * 2 +
> + COOR_DATA_CHECKSUM_SIZE;
> +
> + ret = regmap_raw_read(cd->regmap, misc->touch_data_addr, pre_buf,
> + pre_read_len);
> + if (ret) {
> + dev_err(cd->dev, "failed get event head data\n");
> + return ret;
> + }
> +
> + if (pre_buf[0] == 0x00)
> + return -EINVAL;
> +
> + if (!goodix_berlin_check_checksum(pre_buf, IRQ_EVENT_HEAD_LEN)) {
> + dev_warn(cd->dev, "touch head checksum err : %*ph\n",
> + IRQ_EVENT_HEAD_LEN, pre_buf);
> + return -EINVAL;
> + }
> +
> + event_status = pre_buf[0];
> + if (event_status & GOODIX_TOUCH_EVENT)
> + return goodix_berlin_touch_handler(cd, pre_buf, pre_read_len);
> +
> + if (event_status & GOODIX_REQUEST_EVENT) {
> + cd->ts_event.event_type = EVENT_REQUEST;
> + if (pre_buf[2] == BRL_REQUEST_CODE_RESET)
> + cd->ts_event.request_code = REQUEST_TYPE_RESET;
> + else
> + dev_warn(cd->dev, "unsupported request code 0x%x\n", pre_buf[2]);
> + }
> +
> + goodix_berlin_after_event_handler(cd);
> +
> + return 0;
> +}
> +
> +static void goodix_berlin_report_finger(struct goodix_berlin_core *cd)
> +{
> + struct goodix_touch_data *touch_data = &cd->ts_event.touch_data;
> + int i;
> +
> + mutex_lock(&cd->input_dev->mutex);
> +
> + for (i = 0; i < GOODIX_MAX_TOUCH; i++) {
> + if (touch_data->coords[i].status == TS_TOUCH) {
> + dev_dbg(cd->dev, "report: id[%d], x %d, y %d, w %d\n", i,
> + touch_data->coords[i].x,
> + touch_data->coords[i].y,
> + touch_data->coords[i].w);
> +
> + input_mt_slot(cd->input_dev, i);
> + input_mt_report_slot_state(cd->input_dev,
> + MT_TOOL_FINGER, true);
> + touchscreen_report_pos(cd->input_dev, &cd->props,
> + touch_data->coords[i].x,
> + touch_data->coords[i].y, true);
> + input_report_abs(cd->input_dev, ABS_MT_TOUCH_MAJOR,
> + touch_data->coords[i].w);
> + } else {
> + input_mt_slot(cd->input_dev, i);
> + input_mt_report_slot_state(cd->input_dev,
> + MT_TOOL_FINGER, false);
> + }
> + }
> +
> + input_mt_sync_frame(cd->input_dev);
> + input_sync(cd->input_dev);
> +
> + mutex_unlock(&cd->input_dev->mutex);
> +}
> +
> +static int goodix_berlin_request_handle(struct goodix_berlin_core *cd)
> +{
> + /* TOFIX: Handle more request codes */
> + if (cd->ts_event.request_code != REQUEST_TYPE_RESET) {
> + dev_info(cd->dev, "can't handle request type 0x%x\n",
> + cd->ts_event.request_code);
> + return -EINVAL;
> + }
> +
> + return goodix_berlin_reset(cd, GOODIX_NORMAL_RESET_DELAY_MS);
This is confusing; can you add some comments here? Under what conditions
would we bail and reset the device? Surely this is unlikely?
> +}
> +
> +static irqreturn_t goodix_berlin_threadirq_func(int irq, void *data)
> +{
> + struct goodix_berlin_core *cd = data;
> + int ret;
> +
> + ret = goodix_berlin_event_handler(cd);
> + if (likely(!ret)) {
> + switch (cd->ts_event.event_type) {
> + case EVENT_TOUCH:
> + goodix_berlin_report_finger(cd);
> + break;
> +
> + case EVENT_REQUEST:
> + goodix_berlin_request_handle(cd);
> + break;
> +
> + /* TOFIX: Handle more request types */
> + case EVENT_INVALID:
> + break;
> + }
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int goodix_berlin_input_dev_config(struct goodix_berlin_core *cd,
> + const struct input_id *id)
> +{
> + struct input_dev *input_dev = devm_input_allocate_device(cd->dev);
Please break this assignment into its own line.
> +
> + if (!input_dev)
> + return -ENOMEM;
> +
> + cd->input_dev = input_dev;
> + input_set_drvdata(input_dev, cd);
> +
> + input_dev->name = "Goodix Berlin Capacitive TouchScreen";
> + input_dev->phys = "input/ts";
> + input_dev->dev.parent = cd->dev;
> +
> + memcpy(&input_dev->id, id, sizeof(*id));
> +
> + /* set input parameters */
> + input_set_abs_params(cd->input_dev, ABS_MT_POSITION_X, 0, SZ_64K - 1, 0, 0);
> + input_set_abs_params(cd->input_dev, ABS_MT_POSITION_Y, 0, SZ_64K - 1, 0, 0);
> + input_set_abs_params(cd->input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
> +
> + /* Override with DT */
This comment is unnecessary; furthermore touchscreen_parse_properties() is not
limited to OF.
> + touchscreen_parse_properties(cd->input_dev, true, &cd->props);
> +
> + input_mt_init_slots(cd->input_dev, GOODIX_MAX_TOUCH, INPUT_MT_DIRECT);
Please check this function's return value and bail if negative.
> +
> + return input_register_device(cd->input_dev);
> +}
> +
> +static void goodix_berlin_release_connects(struct goodix_berlin_core *cd)
> +{
> + struct input_dev *input_dev = cd->input_dev;
> + int i;
> +
> + mutex_lock(&input_dev->mutex);
> +
> + for (i = 0; i < GOODIX_MAX_TOUCH; i++) {
> + input_mt_slot(input_dev, i);
> + input_mt_report_slot_state(input_dev, MT_TOOL_FINGER, false);
> + }
> + input_mt_sync_frame(input_dev);
> + input_sync(input_dev);
> +
> + mutex_unlock(&input_dev->mutex);
> +}
> +
> +static int goodix_berlin_suspend(struct goodix_berlin_core *cd)
> +{
> + /* disable irq */
> + goodix_berlin_irq_enable(cd, false);
> +
> + /* enter sleep mode or power off */
> + goodix_berlin_do_suspend(cd);
> +
> + goodix_berlin_release_connects(cd);
For my own understanding, why is this operation necessary? Do you expect
a platform to suspend if the user's fingers remain on the panel? I also
find it confusing that multiple entry points can call input_mt_sync_frame()
or input_sync(); typically these are limited to a common handler.
> +
> + return 0;
> +}
> +
> +static int goodix_berlin_resume(struct goodix_berlin_core *cd)
> +{
> + goodix_berlin_irq_enable(cd, false);
> +
> + /* reset device or power on*/
> + goodix_berlin_do_resume(cd);
> +
> + /* enable irq */
> + goodix_berlin_irq_enable(cd, true);
> +
> + return 0;
> +}
> +
> +int goodix_berlin_pm_suspend(struct device *dev)
> +{
> + struct goodix_berlin_core *cd = dev_get_drvdata(dev);
> +
> + return goodix_berlin_suspend(cd);
> +}
> +
> +int goodix_berlin_pm_resume(struct device *dev)
> +{
> + struct goodix_berlin_core *cd = dev_get_drvdata(dev);
> +
> + return goodix_berlin_resume(cd);
> +}
> +
> +EXPORT_GPL_SIMPLE_DEV_PM_OPS(goodix_berlin_pm_ops,
> + goodix_berlin_pm_suspend,
> + goodix_berlin_pm_resume);
> +
> +int goodix_berlin_probe(struct device *dev, int irq, const struct input_id *id,
> + struct regmap *regmap)
> +{
> + struct goodix_berlin_core *cd;
> + int ret;
> +
> + if (irq <= 0)
> + return -EINVAL;
> +
> + cd = devm_kzalloc(dev, sizeof(*cd), GFP_KERNEL);
> + if (!cd)
> + return -ENOMEM;
> +
> + cd->dev = dev;
> + cd->regmap = regmap;
> + cd->irq = irq;
> +
> + cd->reset_gpio = devm_gpiod_get(cd->dev, "reset", GPIOF_OUT_INIT_HIGH);
> + if (IS_ERR(cd->reset_gpio))
> + return dev_err_probe(cd->dev, PTR_ERR(cd->reset_gpio),
> + "Failed to request reset gpio\n");
In my experience, the reset GPIO is the first to go in case the board designer
runs out of GPIO. Can it be optional?
> +
> + cd->avdd = devm_regulator_get(cd->dev, "avdd");
> + if (IS_ERR(cd->avdd))
> + return dev_err_probe(cd->dev, PTR_ERR(cd->avdd),
> + "Failed to request avdd regulator\n");
> +
> + cd->iovdd = devm_regulator_get(cd->dev, "iovdd");
> + if (IS_ERR(cd->iovdd))
> + return dev_err_probe(cd->dev, PTR_ERR(cd->iovdd),
> + "Failed to request iovdd regulator\n");
> +
> + ret = goodix_berlin_input_dev_config(cd, id);
> + if (ret < 0) {
> + dev_err(cd->dev, "failed set input device");
> + return ret;
> + }
> +
> + ret = devm_request_threaded_irq(dev, irq, NULL,
> + goodix_berlin_threadirq_func,
> + IRQF_ONESHOT, "goodix-berlin", cd);
> + if (ret) {
> + dev_err(dev, "request threaded irq failed: %d\n", ret);
> + return ret;
> + }
> +
> + dev_set_drvdata(dev, cd);
> +
> + ret = goodix_berlin_power_on(cd, true);
This seems like something that can be posted as an device-managed action
as opposed to having subsequent goto statements from here on.
> + if (ret) {
> + dev_err(cd->dev, "failed power on");
> + goto err_out;
> + }
> +
> + ret = goodix_berlin_read_version(cd, &cd->fw_version);
> + if (ret < 0) {
> + dev_err(cd->dev, "failed to get version info");
> + goto err_out;
> + }
> +
> + ret = goodix_berlin_get_ic_info(cd, &cd->ic_info);
> + if (ret) {
> + dev_err(cd->dev, "invalid ic info, abort");
> + goto err_out;
> + }
> +
> + cd->debugfs_root = debugfs_create_dir(dev_name(dev), NULL);
> + debugfs_create_file("fw_version", 0444, cd->debugfs_root,
> + cd, &goodix_berlin_fw_version_fops);
> + debugfs_create_file("ic_info", 0444, cd->debugfs_root,
> + cd, &goodix_berlin_ic_info_fops);
We need not explicitly create attributes; consider the dev_groups member
of the device_driver struct instead.
> +
> + dev_info(cd->dev, "Goodix Berlin %s Touchscreen Controller", cd->fw_version.patch_pid);
dev_dbg()
> +
> + return 0;
> +
> +err_out:
> + goodix_berlin_power_on(cd, false);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(goodix_berlin_probe);
> +
> +int goodix_berlin_remove(struct device *dev)
> +{
> + struct goodix_berlin_core *cd = dev_get_drvdata(dev);
> +
> + goodix_berlin_release_connects(cd);
> +
> + goodix_berlin_irq_enable(cd, false);
> +
> + goodix_berlin_power_on(cd, false);
> +
> + debugfs_remove(cd->debugfs_root);
> +
> + return 0;
I suspect much of this is unnecessary if you adopt modern primitives. Once
you wrap regulator disables with device-managed actions, you should be able
to get rid of the remove callback altogether.
> +}
> +EXPORT_SYMBOL_GPL(goodix_berlin_remove);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Goodix Berlin Core Touchscreen driver");
> +MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
>
> --
> 2.34.1
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH] HID: sensor-hub: Allow multi-function sensor devices
From: srinivas pandruvada @ 2023-06-12 4:53 UTC (permalink / raw)
To: Jiri Kosina, Daniel Thompson
Cc: Jonathan Cameron, Benjamin Tissoires, Bjorn Andersson,
linux-input, linux-iio, linux-kernel
In-Reply-To: <nycvar.YFH.7.76.2306091742090.5716@cbobk.fhfr.pm>
On Fri, 2023-06-09 at 17:43 +0200, Jiri Kosina wrote:
> On Sun, 28 May 2023, Daniel Thompson wrote:
>
> > The Lenovo Yoga C630 has a combined keyboard and accelerometer that
> > interfaces via i2c-hid. Currently this laptop either has a working
> > keyboard (if CONFIG_HID_SENSOR_HUB is disabled) or a working
> > accelerometer.
> > only works on kernels. Put another way, most distro kernels enable
> > CONFIG_HID_SENSOR_HUB and therefore cannot work on this device
> > since the
> > keyboard doesn't work!
> >
> > Fix this by providing a richer connect mask during the probe. With
> > this
> > change both keyboard and screen orientation sensors work correctly.
> >
> > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
>
> Srinivas, are you aware of any multi-function device that this patch
> might
> break?
I tried two systems, one with ISH and another with external hub.
Didn't break anything.
I don't have 5+ years old systems, to confirm if it will break
something on older systems with external hub.
Thanks,
Srinivas
>
> Thanks,
>
^ permalink raw reply
* Re: [PATCH] HID: sensor-hub: Allow multi-function sensor devices
From: Jiri Kosina @ 2023-06-12 7:39 UTC (permalink / raw)
To: srinivas pandruvada
Cc: Daniel Thompson, Jonathan Cameron, Benjamin Tissoires,
Bjorn Andersson, linux-input, linux-iio, linux-kernel
In-Reply-To: <51dfdee66713e470d9a925ac5a8f8ae1da644462.camel@linux.intel.com>
On Sun, 11 Jun 2023, srinivas pandruvada wrote:
> > > The Lenovo Yoga C630 has a combined keyboard and accelerometer that
> > > interfaces via i2c-hid. Currently this laptop either has a working
> > > keyboard (if CONFIG_HID_SENSOR_HUB is disabled) or a working
> > > accelerometer. only works on kernels. Put another way, most distro
> > > kernels enable CONFIG_HID_SENSOR_HUB and therefore cannot work on
> > > this device since the keyboard doesn't work!
> > >
> > > Fix this by providing a richer connect mask during the probe. With
> > > this change both keyboard and screen orientation sensors work
> > > correctly.
> > >
> > > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> >
> > Srinivas, are you aware of any multi-function device that this patch
> > might break?
> I tried two systems, one with ISH and another with external hub. Didn't
> break anything. I don't have 5+ years old systems, to confirm if it will
> break something on older systems with external hub.
Thanks a lot. Can I take the above as an equivalent of
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
? :)
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH RFC 2/4] input: touchscreen: add core support for Goodix Berlin Touchscreen IC
From: Neil Armstrong @ 2023-06-12 8:37 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bastien Nocera, Hans de Goede, Henrik Rydberg, linux-input,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <ZIai3KaMmuAbJB3M@nixie71>
Hi Jeff,
On 12/06/2023 06:45, Jeff LaBundy wrote:
> Hi Neil,
>
> On Tue, Jun 06, 2023 at 04:31:57PM +0200, Neil Armstrong wrote:
>> Add initial support for the new Goodix "Berlin" touchscreen ICs.
>>
>> 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>
>> ---
>> drivers/input/touchscreen/Kconfig | 5 +
>> drivers/input/touchscreen/Makefile | 1 +
>> drivers/input/touchscreen/goodix_berlin.h | 228 ++++++
>> drivers/input/touchscreen/goodix_berlin_core.c | 935 +++++++++++++++++++++++++
>> 4 files changed, 1169 insertions(+)
>>
>> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
>> index c2cbd332af1d..1a6f6f6da991 100644
>> --- a/drivers/input/touchscreen/Kconfig
>> +++ b/drivers/input/touchscreen/Kconfig
>> @@ -416,6 +416,11 @@ config TOUCHSCREEN_GOODIX
>> To compile this driver as a module, choose M here: the
>> module will be called goodix.
>>
>> +config TOUCHSCREEN_GOODIX_BERLIN_CORE
>> + depends on GPIOLIB || COMPILE_TEST
>> + depends on REGMAP
>> + tristate
>> +
>> config TOUCHSCREEN_HIDEEP
>> tristate "HiDeep Touch IC"
>> depends on I2C
>> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
>> index 159cd5136fdb..29cdb042e104 100644
>> --- a/drivers/input/touchscreen/Makefile
>> +++ b/drivers/input/touchscreen/Makefile
>> @@ -47,6 +47,7 @@ obj-$(CONFIG_TOUCHSCREEN_EGALAX_SERIAL) += egalax_ts_serial.o
>> obj-$(CONFIG_TOUCHSCREEN_EXC3000) += exc3000.o
>> 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_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.h b/drivers/input/touchscreen/goodix_berlin.h
>> new file mode 100644
>> index 000000000000..83f16201c354
>> --- /dev/null
>> +++ b/drivers/input/touchscreen/goodix_berlin.h
>> @@ -0,0 +1,228 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Goodix Touchscreen Driver
>> + * Copyright (C) 2020 - 2021 Goodix, Inc.
>> + * Copyright (C) 2023 Linaro Ltd.
>> + *
>> + * Based on goodix_berlin_berlin driver.
>> + */
>> +
>> +#ifndef __GOODIX_BERLIN_H_
>> +#define __GOODIX_BERLIN_H_
>> +
>> +#include <linux/input.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/input/touchscreen.h>
>> +#include <linux/regulator/consumer.h>
>> +
>> +#define GOODIX_MAX_TOUCH 10
>> +#define GOODIX_MAX_FRAMEDATA_LEN 1700
>> +
>> +#define GOODIX_NORMAL_RESET_DELAY_MS 100
>> +#define GOODIX_HOLD_CPU_RESET_DELAY_MS 5
>> +
>> +enum CHECKSUM_MODE {
>> + CHECKSUM_MODE_U8_LE,
>> + CHECKSUM_MODE_U16_LE,
>> +};
>
> These don't seem to be used anywhere.
>
> Understanding that vendor drivers are typically saddled with lots of cruft
> as a result of having supported many different ICs and customers, please
> prune the driver for these kinds of things for v1.
Thanks for your very advanced review, I already gone though a very
heavy cleanup of the vendor driver but seems I missed some pieces
and namings. I'll make sure all the remaining unused stuff is pruned
on v2.
>
>> +
>> +#define MAX_SCAN_FREQ_NUM 8
>> +#define MAX_SCAN_RATE_NUM 8
>> +#define MAX_FREQ_NUM_STYLUS 8
>> +
>> +#define IRQ_EVENT_HEAD_LEN 8
>> +#define BYTES_PER_POINT 8
>> +#define COOR_DATA_CHECKSUM_SIZE 2
>> +
>> +#define GOODIX_TOUCH_EVENT BIT(7)
>> +#define GOODIX_REQUEST_EVENT BIT(6)
>> +#define GOODIX_GESTURE_EVENT BIT(5)
>> +
>> +#define POINT_TYPE_STYLUS_HOVER 0x01
>> +#define POINT_TYPE_STYLUS 0x03
>> +
>> +#define DEV_CONFIRM_VAL 0xAA
>> +#define BOOTOPTION_ADDR 0x10000
>> +#define FW_VERSION_INFO_ADDR 0x10014
>> +
>> +#define GOODIX_IC_INFO_MAX_LEN 1024
>> +#define GOODIX_IC_INFO_ADDR 0x10070
>> +
>> +enum brl_request_code {
>> + BRL_REQUEST_CODE_CONFIG = 1,
>> + BRL_REQUEST_CODE_REF_ERR = 2,
>> + BRL_REQUEST_CODE_RESET = 3,
>> + BRL_REQUEST_CODE_CLOCK = 4,
>> +};
>> +
>> +struct goodix_fw_version {
>> + u8 rom_pid[6]; /* rom PID */
>
> This comment is obvious and noisy; there are similar instances below.
>
>> + u8 rom_vid[3]; /* Mask VID */
>> + u8 rom_vid_reserved;
>> + u8 patch_pid[8]; /* Patch PID */
>> + u8 patch_vid[4]; /* Patch VID */
>> + u8 patch_vid_reserved;
>> + u8 sensor_id;
>> + u8 reserved[2];
>> + u16 checksum;
>> +} __packed;
>> +
>> +struct goodix_ic_info_version {
>> + u8 info_customer_id;
>> + u8 info_version_id;
>> + u8 ic_die_id;
>> + u8 ic_version_id;
>> + u32 config_id;
>> + u8 config_version;
>> + u8 frame_data_customer_id;
>> + u8 frame_data_version_id;
>> + u8 touch_data_customer_id;
>> + u8 touch_data_version_id;
>> + u8 reserved[3];
>> +} __packed;
>> +
>> +struct goodix_ic_info_feature { /* feature info*/
>> + u16 freqhop_feature;
>> + u16 calibration_feature;
>> + u16 gesture_feature;
>> + u16 side_touch_feature;
>> + u16 stylus_feature;
>> +} __packed;
>> +
>> +struct goodix_ic_info_param { /* param */
>> + u8 drv_num;
>> + u8 sen_num;
>> + u8 button_num;
>> + u8 force_num;
>> + u8 active_scan_rate_num;
>> + u16 active_scan_rate[MAX_SCAN_RATE_NUM];
>> + u8 mutual_freq_num;
>> + u16 mutual_freq[MAX_SCAN_FREQ_NUM];
>> + u8 self_tx_freq_num;
>> + u16 self_tx_freq[MAX_SCAN_FREQ_NUM];
>> + u8 self_rx_freq_num;
>> + u16 self_rx_freq[MAX_SCAN_FREQ_NUM];
>> + u8 stylus_freq_num;
>> + u16 stylus_freq[MAX_FREQ_NUM_STYLUS];
>> +} __packed;
>> +
>> +struct goodix_ic_info_misc { /* other data */
>> + u32 cmd_addr;
>> + u16 cmd_max_len;
>> + u32 cmd_reply_addr;
>> + u16 cmd_reply_len;
>> + u32 fw_state_addr;
>> + u16 fw_state_len;
>> + u32 fw_buffer_addr;
>> + u16 fw_buffer_max_len;
>> + u32 frame_data_addr;
>> + u16 frame_data_head_len;
>> + u16 fw_attr_len;
>> + u16 fw_log_len;
>> + u8 pack_max_num;
>> + u8 pack_compress_version;
>> + u16 stylus_struct_len;
>> + u16 mutual_struct_len;
>> + u16 self_struct_len;
>> + u16 noise_struct_len;
>> + u32 touch_data_addr;
>> + u16 touch_data_head_len;
>> + u16 point_struct_len;
>> + u16 reserved1;
>> + u16 reserved2;
>> + u32 mutual_rawdata_addr;
>> + u32 mutual_diffdata_addr;
>> + u32 mutual_refdata_addr;
>> + u32 self_rawdata_addr;
>> + u32 self_diffdata_addr;
>> + u32 self_refdata_addr;
>> + u32 iq_rawdata_addr;
>> + u32 iq_refdata_addr;
>> + u32 im_rawdata_addr;
>> + u16 im_readata_len;
>> + u32 noise_rawdata_addr;
>> + u16 noise_rawdata_len;
>> + u32 stylus_rawdata_addr;
>> + u16 stylus_rawdata_len;
>> + u32 noise_data_addr;
>> + u32 esd_addr;
>> +} __packed;
>> +
>> +struct goodix_ic_info {
>> + u16 length;
>> + struct goodix_ic_info_version version;
>> + struct goodix_ic_info_feature feature;
>> + struct goodix_ic_info_param parm;
>> + struct goodix_ic_info_misc misc;
>> +} __packed;
>> +
>> +/* interrupt event type */
>> +enum ts_event_type {
>> + EVENT_INVALID,
>> + EVENT_TOUCH, /* finger touch event */
>> + EVENT_REQUEST,
>> +};
>
> Please consistently namespace all of these, i.e. goodix_berlin_...
>
>> +
>> +enum ts_request_type {
>> + REQUEST_TYPE_RESET,
>> +};
>
> And here.
>
>> +
>> +enum touch_point_status {
>> + TS_NONE,
>> + TS_RELEASE,
>> + TS_TOUCH,
>> +};
>> +
>> +/* coordinate package */
>> +struct goodix_berlin_coords {
>> + int status; /* NONE, RELEASE, TOUCH */
>
> enum touch_point_status status; (without comment, and namespace fixed)
>
>> + unsigned int x, y, w, p;
>> +};
>> +
>> +/* touch event data */
>> +struct goodix_touch_data {
>> + int touch_num;
>> + struct goodix_berlin_coords coords[GOODIX_MAX_TOUCH];
>> +};
>> +
>> +/* touch event struct */
>> +struct goodix_berlin_event {
>> + enum ts_event_type event_type;
>> + u8 request_code; /* represent the request type */
>> + struct goodix_touch_data touch_data;
>> +};
>> +
>> +struct goodix_berlin_core {
>> + struct device *dev;
>> + struct regmap *regmap;
>
> It is just my opinion, but I find these newlines to be unnecessary.
>
>> +
>> + struct regulator *avdd;
>> + struct regulator *iovdd;
>> + struct gpio_desc *reset_gpio;
>> +
>> + struct touchscreen_properties props;
>> +
>> + struct goodix_fw_version fw_version;
>> + struct goodix_ic_info ic_info;
>> +
>> + struct input_dev *input_dev;
>> + struct goodix_berlin_event ts_event;
>> +
>> + int irq;
>> +
>> + struct dentry *debugfs_root;
>> +};
>> +
>> +u32 goodix_append_checksum(u8 *data, int len, int mode);
>> +int checksum_cmp(const u8 *data, int size, int mode);
>> +int is_risk_data(const u8 *data, int size);
>> +u32 goodix_get_file_config_id(u8 *ic_config);
>> +void goodix_rotate_abcd2cbad(int tx, int rx, s16 *data);
>> +
>> +int goodix_berlin_probe(struct device *dev, int irq, const struct input_id *id,
>> + struct regmap *regmap);
>> +int goodix_berlin_remove(struct device *dev);
>> +
>> +extern const struct dev_pm_ops goodix_berlin_pm_ops;
>> +
>> +#endif
>> diff --git a/drivers/input/touchscreen/goodix_berlin_core.c b/drivers/input/touchscreen/goodix_berlin_core.c
>> new file mode 100644
>> index 000000000000..0719690a5bd3
>> --- /dev/null
>> +++ b/drivers/input/touchscreen/goodix_berlin_core.c
>> @@ -0,0 +1,935 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Goodix Touchscreen Driver
>> + * Copyright (C) 2020 - 2021 Goodix, Inc.
>> + * Copyright (C) 2023 Linaro Ltd.
>> + *
>> + * Based on goodix_ts_berlin driver.
>> + */
>> +#include <linux/regmap.h>
>> +#include <linux/input/mt.h>
>> +#include <linux/input/touchscreen.h>
>> +#include <linux/debugfs.h>
>
> Please sort these alphabetically.
>
>> +
>> +#include "goodix_berlin.h"
>> +
>> +/*
>> + * Goodix "Berlin" Touchscreen ID driver
>> + *
>> + * Currently only handles Multitouch events with already
>> + * programmed firmware and "config" for "Revision D" Berlin IC.
>> + *
>> + * Support is missing for:
>> + * - ESD Management
>> + * - Firmware update/flashing
>> + * - "Config" update/flashing
>> + * - PEN Events
>
> What is "PEN"; is it "stylus" as written elsewhere?
Yes, they use both terms in the vendor driver.
>
>> + * - Gesture Events
>> + * - Support for older revisions (A, B & C)
>> + */
>> +
>> +static bool goodix_berlin_check_checksum(const u8 *data, int size)
>> +{
>> + u32 cal_checksum = 0;
>> + u32 r_checksum = 0;
>> + u32 i;
>> +
>> + if (size < COOR_DATA_CHECKSUM_SIZE)
>> + return false;
>> +
>> + for (i = 0; i < size - COOR_DATA_CHECKSUM_SIZE; i++)
>> + cal_checksum += data[i];
>> +
>> + r_checksum += data[i++];
>> + r_checksum += (data[i] << 8);
>> +
>> + return (cal_checksum & 0xFFFF) == r_checksum;
>> +}
>> +
>> +static bool goodix_berlin_is_risk_data(struct goodix_berlin_core *cd,
>> + const u8 *data, int size)
>> +{
>> + int zero_count = 0;
>> + int ff_count = 0;
>> + int i;
>> +
>> + for (i = 0; i < size; i++) {
>> + if (data[i] == 0)
>> + zero_count++;
>> + else if (data[i] == 0xff)
>> + ff_count++;
>> + }
>> + if (zero_count == size || ff_count == size) {
>> + dev_warn(cd->dev, "warning data is all %s\n",
>> + zero_count == size ? "zero" : "0xff");
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
>> +static int goodix_berlin_dev_confirm(struct goodix_berlin_core *cd)
>> +{
>> + u8 tx_buf[8] = { 0 };
>> + u8 rx_buf[8] = { 0 };
>> + int retry = 3;
>> + int ret = 0;
>
> No need to initialize 'ret' here.
>
> Please note however that in input, return values which can either return
> zero (success) or an error code are typically called 'error' since we are
> quite literally evaluating an error code. Therefore:
>
> int error;
>
> memset(...);
> while (...) {
> error = ...;
> if (error)
> return error;
>
> [...]
> }
>
>> +
>> + memset(tx_buf, DEV_CONFIRM_VAL, sizeof(tx_buf));
>> + while (retry--) {
>> + ret = regmap_raw_write(cd->regmap, BOOTOPTION_ADDR, tx_buf,
>> + sizeof(tx_buf));
>> + if (ret < 0)
>> + return ret;
>> + ret = regmap_raw_read(cd->regmap, BOOTOPTION_ADDR, rx_buf,
>> + sizeof(rx_buf));
>> + if (ret < 0)
>> + return ret;
>> + if (!memcmp(tx_buf, rx_buf, sizeof(tx_buf)))
>> + break;
>> + usleep_range(5000, 5100);
>> + }
>> +
>> + if (retry < 0) {
>> + ret = -EINVAL;
>> + dev_err(cd->dev, "device confirm failed, rx_buf: %*ph\n", 8, rx_buf);
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int goodix_berlin_power_on(struct goodix_berlin_core *cd, bool on)
>> +{
>> + int ret = 0;
>> +
>> + if (on) {
>> + ret = regulator_enable(cd->iovdd);
>> + if (ret < 0) {
>> + dev_err(cd->dev, "Failed to enable iovdd: %d\n", ret);
>> + goto power_off;
>
> This initially struck me as an imbalance; can you let me know in case I have
> misunderstood? If we fail here, we will disable both regulators despite only
> have attempted to enable the first.
I problably missed this, I'll fix it.
>
> Also, I do not believe failing to enable a regulator is sufficient evidence
> to subsequently disable it.
>
>> + }
>> +
>
> Let us have a comment to explain why 3 ms is special.
I must say I have no idea why 3ms is needed since I don't
have access to any HW documentation, I'll probbaly write something
like :
/* Vendo waits 3ms here to wait for iovdd to settle */
>
>> + usleep_range(3000, 3100);
>> +
>> + ret = regulator_enable(cd->avdd);
>> + if (ret < 0) {
>> + dev_err(cd->dev, "Failed to enable avdd: %d\n", ret);
>> + goto power_off;
>> + }
>> +
>> + usleep_range(15000, 15100);
>
> And here.
>
>> +
>> + gpiod_set_value(cd->reset_gpio, 0);
>> +
>> + usleep_range(4000, 4100);
>> +
>> + ret = goodix_berlin_dev_confirm(cd);
>> + if (ret < 0)
>> + goto power_off;
>> +
>> + msleep(GOODIX_NORMAL_RESET_DELAY_MS);
>> +
>> + return 0;
>> + }
>> +
>> +power_off:
>> + gpiod_set_value(cd->reset_gpio, 1);
>> + regulator_disable(cd->iovdd);
>> + regulator_disable(cd->avdd);
>> + return ret;
>> +}
>> +
>> +static int goodix_berlin_do_suspend(struct goodix_berlin_core *cd)
>> +{
>> + if (goodix_berlin_power_on(cd, 0))
>> + dev_err(cd->dev, "failed power off\n");
>> +
>> + return 0;
>> +}
>> +
>> +static int goodix_berlin_do_resume(struct goodix_berlin_core *cd)
>> +{
>> + int ret = 0;
>> +
>> + ret = goodix_berlin_power_on(cd, 1);
>> + if (ret) {
>> + dev_err(cd->dev, "failed power on\n");
>> + return ret;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int goodix_berlin_reset(struct goodix_berlin_core *cd, int delay)
>> +{
>> + gpiod_set_value(cd->reset_gpio, 1);
>> + usleep_range(2000, 2100);
>> + gpiod_set_value(cd->reset_gpio, 0);
>> +
>> + if (delay < 20)
>> + usleep_range(delay * 1000, delay * 1000 + 100);
>> + else
>> + msleep(delay);
>
> This seems to duplicate fsleep. That being said, why is this logic necessary
> at all? This function seems to only be called once; there is no need to make
> a decision based on the delay...you already know the value.
Yep I'll inline it.
>
>> +
>> + return 0;
>> +}
>> +
>> +static int goodix_berlin_irq_enable(struct goodix_berlin_core *cd, bool enable)
>> +{
>> + if (enable)
>> + enable_irq(cd->irq);
>> + else
>> + disable_irq(cd->irq);
>
> Why to wrap this logic? While I agree doing so is common for vendor drivers,
> that does not make it correct.
Ack
>
>> +
>> + return 0;
>> +}
>> +
>> +static int goodix_berlin_read_version(struct goodix_berlin_core *cd,
>> + struct goodix_fw_version *version)
>> +{
>> + u8 buf[sizeof(struct goodix_fw_version)] = { 0 };
>
> Same comment here with regard to initialization of immediately overwritten
> variables.
>
>> + int ret, i;
>> +
>> + for (i = 0; i < 2; i++) {
>> + ret = regmap_raw_read(cd->regmap, FW_VERSION_INFO_ADDR, buf, sizeof(buf));
>> + if (ret) {
>> + dev_dbg(cd->dev, "read fw version: %d, retry %d\n", ret, i);
>> + usleep_range(5000, 5100);
>> + continue;
>> + }
>> +
>> + if (goodix_berlin_check_checksum(buf, sizeof(buf)))
>> + break;
>> +
>> + dev_dbg(cd->dev, "invalid fw version: checksum error\n");
>> +
>> + ret = -EINVAL;
>> + usleep_range(10000, 11000);
>
> Let us optimize this logic here; no need to sleep for 10 ms only to fail
> upon the last try.
>
>> + }
>> +
>> + if (ret) {
>> + dev_err(cd->dev, "failed to get fw version");
>> + return ret;
>> + }
>> +
>> + memcpy(version, buf, sizeof(*version));
>> +
>> + return 0;
>> +}
>> +
>> +static int goodix_berlin_fw_version_show(struct seq_file *s, void *unused)
>> +{
>> + struct goodix_berlin_core *cd = s->private;
>> + struct goodix_fw_version *version = &cd->fw_version;
>> + u8 temp_pid[9] = { 0 };
>
> And here, with regard to initialization.
>
>> +
>> + memcpy(temp_pid, version->rom_pid, sizeof(version->rom_pid));
>> + temp_pid[sizeof(version->rom_pid)] = '\0';
>> +
>> + seq_printf(s, "rom_pid: %s\n", temp_pid);
>> + seq_printf(s, "rom_vid: %*ph\n",
>> + (int)sizeof(version->rom_vid),
>> + version->rom_vid);
>> +
>> + memcpy(temp_pid, version->patch_pid, sizeof(version->patch_pid));
>> + temp_pid[sizeof(version->patch_pid)] = '\0';
>> +
>> + seq_printf(s, "patch_pid: %s\n", temp_pid);
>> + seq_printf(s, "patch_vid: %*ph\n",
>> + (int)sizeof(version->patch_vid),
>> + version->patch_vid);
>> + seq_printf(s, "sensor_id: %d\n",
>> + version->sensor_id);
>> +
>> + return 0;
>> +}
>> +
>> +DEFINE_SHOW_ATTRIBUTE(goodix_berlin_fw_version);
>> +
>> +static int goodix_berlin_convert_ic_info(struct goodix_berlin_core *cd,
>> + struct goodix_ic_info *info,
>> + const u8 *data)
>> +{
>> + int i;
>> + struct goodix_ic_info_version *version = &info->version;
>> + struct goodix_ic_info_feature *feature = &info->feature;
>> + struct goodix_ic_info_param *parm = &info->parm;
>> + struct goodix_ic_info_misc *misc = &info->misc;
>> +
>> + info->length = le16_to_cpup((__le16 *)data);
>> +
>> + data += 2;
>> + memcpy(version, data, sizeof(*version));
>> + version->config_id = le32_to_cpu(version->config_id);
>> +
>> + data += sizeof(struct goodix_ic_info_version);
>> + memcpy(feature, data, sizeof(*feature));
>> + feature->freqhop_feature = le16_to_cpu(feature->freqhop_feature);
>> + feature->calibration_feature =
>> + le16_to_cpu(feature->calibration_feature);
>> + feature->gesture_feature = le16_to_cpu(feature->gesture_feature);
>> + feature->side_touch_feature = le16_to_cpu(feature->side_touch_feature);
>> + feature->stylus_feature = le16_to_cpu(feature->stylus_feature);
>> +
>> + data += sizeof(struct goodix_ic_info_feature);
>> + parm->drv_num = *(data++);
>> + parm->sen_num = *(data++);
>> + parm->button_num = *(data++);
>> + parm->force_num = *(data++);
>> + parm->active_scan_rate_num = *(data++);
>> +
>> + if (parm->active_scan_rate_num > MAX_SCAN_RATE_NUM) {
>> + dev_err(cd->dev, "invalid scan rate num %d > %d\n",
>> + parm->active_scan_rate_num, MAX_SCAN_RATE_NUM);
>> + return -EINVAL;
>> + }
>> +
>> + for (i = 0; i < parm->active_scan_rate_num; i++)
>> + parm->active_scan_rate[i] =
>> + le16_to_cpup((__le16 *)(data + i * 2));
>> +
>> + data += parm->active_scan_rate_num * 2;
>> + parm->mutual_freq_num = *(data++);
>> +
>> + if (parm->mutual_freq_num > MAX_SCAN_FREQ_NUM) {
>> + dev_err(cd->dev, "invalid mntual freq num %d > %d\n",
>> + parm->mutual_freq_num, MAX_SCAN_FREQ_NUM);
>> + return -EINVAL;
>> + }
>> +
>> + for (i = 0; i < parm->mutual_freq_num; i++)
>> + parm->mutual_freq[i] = le16_to_cpup((__le16 *)(data + i * 2));
>> +
>> + data += parm->mutual_freq_num * 2;
>> + parm->self_tx_freq_num = *(data++);
>> +
>> + if (parm->self_tx_freq_num > MAX_SCAN_FREQ_NUM) {
>> + dev_err(cd->dev, "invalid tx freq num %d > %d\n",
>> + parm->self_tx_freq_num, MAX_SCAN_FREQ_NUM);
>> + return -EINVAL;
>> + }
>> +
>> + for (i = 0; i < parm->self_tx_freq_num; i++)
>> + parm->self_tx_freq[i] = le16_to_cpup((__le16 *)(data + i * 2));
>> +
>> + data += parm->self_tx_freq_num * 2;
>> + parm->self_rx_freq_num = *(data++);
>> +
>> + if (parm->self_rx_freq_num > MAX_SCAN_FREQ_NUM) {
>> + dev_err(cd->dev, "invalid rx freq num %d > %d\n",
>> + parm->self_rx_freq_num, MAX_SCAN_FREQ_NUM);
>> + return -EINVAL;
>> + }
>> +
>> + for (i = 0; i < parm->self_rx_freq_num; i++)
>> + parm->self_rx_freq[i] = le16_to_cpup((__le16 *)(data + i * 2));
>> +
>> + data += parm->self_rx_freq_num * 2;
>> + parm->stylus_freq_num = *(data++);
>> +
>> + if (parm->stylus_freq_num > MAX_FREQ_NUM_STYLUS) {
>> + dev_err(cd->dev, "invalid stylus freq num %d > %d\n",
>> + parm->stylus_freq_num, MAX_FREQ_NUM_STYLUS);
>> + return -EINVAL;
>> + }
>> +
>> + for (i = 0; i < parm->stylus_freq_num; i++)
>> + parm->stylus_freq[i] = le16_to_cpup((__le16 *)(data + i * 2));
>> +
>> + data += parm->stylus_freq_num * 2;
>> + memcpy(misc, data, sizeof(*misc));
>> + misc->cmd_addr = le32_to_cpu(misc->cmd_addr);
>> + misc->cmd_max_len = le16_to_cpu(misc->cmd_max_len);
>> + misc->cmd_reply_addr = le32_to_cpu(misc->cmd_reply_addr);
>> + misc->cmd_reply_len = le16_to_cpu(misc->cmd_reply_len);
>> + misc->fw_state_addr = le32_to_cpu(misc->fw_state_addr);
>> + misc->fw_state_len = le16_to_cpu(misc->fw_state_len);
>> + misc->fw_buffer_addr = le32_to_cpu(misc->fw_buffer_addr);
>> + misc->fw_buffer_max_len = le16_to_cpu(misc->fw_buffer_max_len);
>> + misc->frame_data_addr = le32_to_cpu(misc->frame_data_addr);
>> + misc->frame_data_head_len = le16_to_cpu(misc->frame_data_head_len);
>> +
>> + misc->fw_attr_len = le16_to_cpu(misc->fw_attr_len);
>> + misc->fw_log_len = le16_to_cpu(misc->fw_log_len);
>> + misc->stylus_struct_len = le16_to_cpu(misc->stylus_struct_len);
>> + misc->mutual_struct_len = le16_to_cpu(misc->mutual_struct_len);
>> + misc->self_struct_len = le16_to_cpu(misc->self_struct_len);
>> + misc->noise_struct_len = le16_to_cpu(misc->noise_struct_len);
>> + misc->touch_data_addr = le32_to_cpu(misc->touch_data_addr);
>> + misc->touch_data_head_len = le16_to_cpu(misc->touch_data_head_len);
>> + misc->point_struct_len = le16_to_cpu(misc->point_struct_len);
>> + misc->mutual_rawdata_addr = le32_to_cpu(misc->mutual_rawdata_addr);
>> + misc->mutual_diffdata_addr = le32_to_cpu(misc->mutual_diffdata_addr);
>> + misc->mutual_refdata_addr = le32_to_cpu(misc->mutual_refdata_addr);
>> + misc->self_rawdata_addr = le32_to_cpu(misc->self_rawdata_addr);
>> + misc->self_diffdata_addr = le32_to_cpu(misc->self_diffdata_addr);
>> + misc->self_refdata_addr = le32_to_cpu(misc->self_refdata_addr);
>> + misc->iq_rawdata_addr = le32_to_cpu(misc->iq_rawdata_addr);
>> + misc->iq_refdata_addr = le32_to_cpu(misc->iq_refdata_addr);
>> + misc->im_rawdata_addr = le32_to_cpu(misc->im_rawdata_addr);
>> + misc->im_readata_len = le16_to_cpu(misc->im_readata_len);
>> + misc->noise_rawdata_addr = le32_to_cpu(misc->noise_rawdata_addr);
>> + misc->noise_rawdata_len = le16_to_cpu(misc->noise_rawdata_len);
>> + misc->stylus_rawdata_addr = le32_to_cpu(misc->stylus_rawdata_addr);
>> + misc->stylus_rawdata_len = le16_to_cpu(misc->stylus_rawdata_len);
>> + misc->noise_data_addr = le32_to_cpu(misc->noise_data_addr);
>> + misc->esd_addr = le32_to_cpu(misc->esd_addr);
>
> What is all of this stuff for? The beginning of the driver explicitly states
> that ESD recovery is not supported; please consider stripping all of this if
> its only purpose is to support noise in the logs or sysfs attributes that we
> cannot reasonably react to.
Those are all the firmware parameters used by the driver, for now only a little
are used, we already read the entire struct so stripping isn't an option otherwise
the values will be shifted.
We only print those over debugfs se it won't fill any logs
>
>> +
>> + return 0;
>> +}
>> +
>> +/* Convert to debugfs */
>
> This comment is unnecessary.
>
>> +static int goodix_berlin_ic_info_show(struct seq_file *s, void *unused)
>> +{
>> + struct goodix_berlin_core *cd = s->private;
>> + struct goodix_ic_info_version *version = &cd->ic_info.version;
>> + struct goodix_ic_info_feature *feature = &cd->ic_info.feature;
>> + struct goodix_ic_info_param *parm = &cd->ic_info.parm;
>> + struct goodix_ic_info_misc *misc = &cd->ic_info.misc;
>> +
>> + seq_printf(s, "ic_info_length: %d\n", cd->ic_info.length);
>> + seq_printf(s, "info_customer_id: 0x%01X\n",
>> + version->info_customer_id);
>> + seq_printf(s, "info_version_id: 0x%01X\n",
>> + version->info_version_id);
>> + seq_printf(s, "ic_die_id: 0x%01X\n",
>> + version->ic_die_id);
>> + seq_printf(s, "ic_version_id: 0x%01X\n",
>> + version->ic_version_id);
>> + seq_printf(s, "config_id: 0x%4X\n",
>> + version->config_id);
>> + seq_printf(s, "config_version: 0x%01X\n",
>> + version->config_version);
>> + seq_printf(s, "frame_data_customer_id: 0x%01X\n",
>> + version->frame_data_customer_id);
>> + seq_printf(s, "frame_data_version_id: 0x%01X\n",
>> + version->frame_data_version_id);
>> + seq_printf(s, "touch_data_customer_id: 0x%01X\n",
>> + version->touch_data_customer_id);
>> + seq_printf(s, "touch_data_version_id: 0x%01X\n",
>> + version->touch_data_version_id);
>> + seq_printf(s, "freqhop_feature: 0x%04X\n",
>> + feature->freqhop_feature);
>> + seq_printf(s, "calibration_feature: 0x%04X\n",
>> + feature->calibration_feature);
>> + seq_printf(s, "gesture_feature: 0x%04X\n",
>> + feature->gesture_feature);
>> + seq_printf(s, "side_touch_feature: 0x%04X\n",
>> + feature->side_touch_feature);
>> + seq_printf(s, "stylus_feature: 0x%04X\n",
>> + feature->stylus_feature);
>> + seq_printf(s, "Drv*Sen,Button,Force num: %d x %d, %d, %d\n",
>> + parm->drv_num, parm->sen_num, parm->button_num, parm->force_num);
>> + seq_printf(s, "Cmd: 0x%04X, %d\n",
>> + misc->cmd_addr, misc->cmd_max_len);
>> + seq_printf(s, "Cmd-Reply: 0x%04X, %d\n",
>> + misc->cmd_reply_addr, misc->cmd_reply_len);
>> + seq_printf(s, "FW-State: 0x%04X, %d\n",
>> + misc->fw_state_addr, misc->fw_state_len);
>> + seq_printf(s, "FW-Buffer: 0x%04X, %d\n",
>> + misc->fw_buffer_addr, misc->fw_buffer_max_len);
>> + seq_printf(s, "Touch-Data: 0x%04X, %d\n",
>> + misc->touch_data_addr, misc->touch_data_head_len);
>> + seq_printf(s, "point_struct_len: %d\n",
>> + misc->point_struct_len);
>> + seq_printf(s, "mutual_rawdata_addr: 0x%04X\n",
>> + misc->mutual_rawdata_addr);
>> + seq_printf(s, "mutual_diffdata_addr: 0x%04X\n",
>> + misc->mutual_diffdata_addr);
>> + seq_printf(s, "self_rawdata_addr: 0x%04X\n",
>> + misc->self_rawdata_addr);
>> + seq_printf(s, "self_diffdata_addr: 0x%04X\n",
>> + misc->self_diffdata_addr);
>> + seq_printf(s, "stylus_rawdata_addr: 0x%04X, %d\n",
>> + misc->stylus_rawdata_addr, misc->stylus_rawdata_len);
>> + seq_printf(s, "esd_addr: 0x%04X\n",
>> + misc->esd_addr);
>> +
>> + return 0;
>> +}
>> +
>> +DEFINE_SHOW_ATTRIBUTE(goodix_berlin_ic_info);
>> +
>> +static int goodix_berlin_get_ic_info(struct goodix_berlin_core *cd,
>> + struct goodix_ic_info *ic_info)
>> +{
>> + int ret, i;
>> + u16 length = 0;
>> + u32 ic_addr;
>> + u8 afe_data[GOODIX_IC_INFO_MAX_LEN] = { 0 };
>> +
>> + ic_addr = GOODIX_IC_INFO_ADDR;
>> +
>> + for (i = 0; i < 3; i++) {
>> + ret = regmap_raw_read(cd->regmap, ic_addr, (u8 *)&length, sizeof(length));
>> + if (ret) {
>> + dev_info(cd->dev, "failed get ic info length, %d\n", ret);
>> + usleep_range(5000, 5100);
>> + continue;
>> + }
>> +
>> + length = le16_to_cpu(length);
>> + if (length >= GOODIX_IC_INFO_MAX_LEN) {
>> + dev_info(cd->dev, "invalid ic info length %d, retry %d\n", length, i);
>> + continue;
>> + }
>> +
>> + ret = regmap_raw_read(cd->regmap, ic_addr, afe_data, length);
>> + if (ret) {
>> + dev_info(cd->dev, "failed get ic info data, %d\n", ret);
>> + usleep_range(5000, 5100);
>> + continue;
>> + }
>> +
>> + /* judge whether the data is valid */
>> + if (goodix_berlin_is_risk_data(cd, (const uint8_t *)afe_data,
>> + length)) {
>> + dev_info(cd->dev, "fw info data invalid\n");
>> + usleep_range(5000, 5100);
>> + continue;
>> + }
>> +
>> + if (!goodix_berlin_check_checksum((const uint8_t *)afe_data, length)) {
>> + dev_info(cd->dev, "fw info checksum error\n");
>> + usleep_range(5000, 5100);
>> + continue;
>> + }
>> +
>> + break;
>> + }
>> + if (i == 3) {
>> + dev_err(cd->dev, "failed get ic info\n");
>> + return -EINVAL;
>> + }
>> +
>> + ret = goodix_berlin_convert_ic_info(cd, ic_info, afe_data);
>> + if (ret) {
>> + dev_err(cd->dev, "error converting ic info\n");
>> + return ret;
>> + }
>> +
>> + /* check some key info */
>> + if (!ic_info->misc.cmd_addr || !ic_info->misc.fw_buffer_addr ||
>> + !ic_info->misc.touch_data_addr) {
>> + dev_err(cd->dev, "cmd_addr fw_buf_addr and touch_data_addr is null\n");
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int goodix_berlin_after_event_handler(struct goodix_berlin_core *cd)
>> +{
>> + struct goodix_ic_info_misc *misc = &cd->ic_info.misc;
>> + u8 sync_clean = 0;
>> +
>> + return regmap_raw_write(cd->regmap, misc->touch_data_addr, &sync_clean, 1);
>> +}
>> +
>> +static void goodix_berlin_parse_finger(struct goodix_berlin_core *cd,
>> + struct goodix_touch_data *touch_data,
>> + u8 *buf, int touch_num)
>> +{
>> + unsigned int id = 0, x = 0, y = 0, w = 0;
>> + u8 *coor_data;
>> + int i;
>> +
>> + coor_data = &buf[IRQ_EVENT_HEAD_LEN];
>> +
>> + for (i = 0; i < touch_num; i++) {
>> + id = (coor_data[0] >> 4) & 0x0F;
>> +
>> + if (id >= GOODIX_MAX_TOUCH) {
>> + dev_warn(cd->dev, "invalid finger id %d\n", id);
>> +
>> + touch_data->touch_num = 0;
>> + return;
>> + }
>> +
>> + x = le16_to_cpup((__le16 *)(coor_data + 2));
>> + y = le16_to_cpup((__le16 *)(coor_data + 4));
>> + w = le16_to_cpup((__le16 *)(coor_data + 6));
>> +
>> + touch_data->coords[id].status = TS_TOUCH;
>> + touch_data->coords[id].x = x;
>> + touch_data->coords[id].y = y;
>> + touch_data->coords[id].w = w;
>> +
>> + coor_data += BYTES_PER_POINT;
>> + }
>> +
>> + touch_data->touch_num = touch_num;
>> +}
>> +
>> +static int goodix_berlin_touch_handler(struct goodix_berlin_core *cd,
>> + u8 *pre_buf, u32 pre_buf_len)
>> +{
>> + static u8 buffer[IRQ_EVENT_HEAD_LEN + BYTES_PER_POINT * GOODIX_MAX_TOUCH + 2];
>> + struct goodix_touch_data *touch_data = &cd->ts_event.touch_data;
>> + struct goodix_ic_info_misc *misc = &cd->ic_info.misc;
>> + u8 point_type = 0;
>> + u8 touch_num = 0;
>> + int ret = 0;
>> +
>> + /* clean event buffer */
>> + memset(&cd->ts_event, 0, sizeof(cd->ts_event));
>> +
>> + /* copy pre-data to buffer */
>> + memcpy(buffer, pre_buf, pre_buf_len);
>> +
>> + touch_num = buffer[2] & 0x0F;
>> +
>> + if (touch_num > GOODIX_MAX_TOUCH) {
>> + dev_warn(cd->dev, "invalid touch num %d\n", touch_num);
>> + return -EINVAL;
>> + }
>> +
>> + /* read more data if more than 2 touch events */
>> + if (unlikely(touch_num > 2)) {
>> + ret = regmap_raw_read(cd->regmap,
>> + misc->touch_data_addr + pre_buf_len,
>> + &buffer[pre_buf_len],
>> + (touch_num - 2) * BYTES_PER_POINT);
>> + if (ret) {
>> + dev_err(cd->dev, "failed get touch data\n");
>> + return ret;
>> + }
>> + }
>> +
>> + /* read done */
>> + goodix_berlin_after_event_handler(cd);
>> +
>> + if (touch_num == 0)
>> + return 0;
>> +
>> + point_type = buffer[IRQ_EVENT_HEAD_LEN] & 0x0F;
>> +
>> + /* TOFIX: Not handled */
>
> Same here; this comment is unnecessary as the print makes it obvious.
>
>> + if (point_type == POINT_TYPE_STYLUS || point_type == POINT_TYPE_STYLUS_HOVER) {
>> + dev_warn_once(cd->dev, "Stylus event type not handled\n");
>> + return 0;
>> + }
>> +
>> + if (!goodix_berlin_check_checksum(&buffer[IRQ_EVENT_HEAD_LEN],
>> + touch_num * BYTES_PER_POINT + 2)) {
>> + dev_dbg(cd->dev, "touch data checksum error\n");
>> + dev_dbg(cd->dev, "data: %*ph\n",
>> + touch_num * BYTES_PER_POINT + 2, &buffer[IRQ_EVENT_HEAD_LEN]);
>> + return -EINVAL;
>> + }
>> +
>> + cd->ts_event.event_type = EVENT_TOUCH;
>> + goodix_berlin_parse_finger(cd, touch_data, buffer, touch_num);
>> +
>> + return 0;
>> +}
>> +
>> +static int goodix_berlin_event_handler(struct goodix_berlin_core *cd)
>> +{
>> + struct goodix_ic_info_misc *misc = &cd->ic_info.misc;
>> + int pre_read_len;
>> + u8 pre_buf[32];
>> + u8 event_status;
>> + int ret;
>> +
>> + pre_read_len = IRQ_EVENT_HEAD_LEN + BYTES_PER_POINT * 2 +
>> + COOR_DATA_CHECKSUM_SIZE;
>> +
>> + ret = regmap_raw_read(cd->regmap, misc->touch_data_addr, pre_buf,
>> + pre_read_len);
>> + if (ret) {
>> + dev_err(cd->dev, "failed get event head data\n");
>> + return ret;
>> + }
>> +
>> + if (pre_buf[0] == 0x00)
>> + return -EINVAL;
>> +
>> + if (!goodix_berlin_check_checksum(pre_buf, IRQ_EVENT_HEAD_LEN)) {
>> + dev_warn(cd->dev, "touch head checksum err : %*ph\n",
>> + IRQ_EVENT_HEAD_LEN, pre_buf);
>> + return -EINVAL;
>> + }
>> +
>> + event_status = pre_buf[0];
>> + if (event_status & GOODIX_TOUCH_EVENT)
>> + return goodix_berlin_touch_handler(cd, pre_buf, pre_read_len);
>> +
>> + if (event_status & GOODIX_REQUEST_EVENT) {
>> + cd->ts_event.event_type = EVENT_REQUEST;
>> + if (pre_buf[2] == BRL_REQUEST_CODE_RESET)
>> + cd->ts_event.request_code = REQUEST_TYPE_RESET;
>> + else
>> + dev_warn(cd->dev, "unsupported request code 0x%x\n", pre_buf[2]);
>> + }
>> +
>> + goodix_berlin_after_event_handler(cd);
>> +
>> + return 0;
>> +}
>> +
>> +static void goodix_berlin_report_finger(struct goodix_berlin_core *cd)
>> +{
>> + struct goodix_touch_data *touch_data = &cd->ts_event.touch_data;
>> + int i;
>> +
>> + mutex_lock(&cd->input_dev->mutex);
>> +
>> + for (i = 0; i < GOODIX_MAX_TOUCH; i++) {
>> + if (touch_data->coords[i].status == TS_TOUCH) {
>> + dev_dbg(cd->dev, "report: id[%d], x %d, y %d, w %d\n", i,
>> + touch_data->coords[i].x,
>> + touch_data->coords[i].y,
>> + touch_data->coords[i].w);
>> +
>> + input_mt_slot(cd->input_dev, i);
>> + input_mt_report_slot_state(cd->input_dev,
>> + MT_TOOL_FINGER, true);
>> + touchscreen_report_pos(cd->input_dev, &cd->props,
>> + touch_data->coords[i].x,
>> + touch_data->coords[i].y, true);
>> + input_report_abs(cd->input_dev, ABS_MT_TOUCH_MAJOR,
>> + touch_data->coords[i].w);
>> + } else {
>> + input_mt_slot(cd->input_dev, i);
>> + input_mt_report_slot_state(cd->input_dev,
>> + MT_TOOL_FINGER, false);
>> + }
>> + }
>> +
>> + input_mt_sync_frame(cd->input_dev);
>> + input_sync(cd->input_dev);
>> +
>> + mutex_unlock(&cd->input_dev->mutex);
>> +}
>> +
>> +static int goodix_berlin_request_handle(struct goodix_berlin_core *cd)
>> +{
>> + /* TOFIX: Handle more request codes */
>> + if (cd->ts_event.request_code != REQUEST_TYPE_RESET) {
>> + dev_info(cd->dev, "can't handle request type 0x%x\n",
>> + cd->ts_event.request_code);
>> + return -EINVAL;
>> + }
>> +
>> + return goodix_berlin_reset(cd, GOODIX_NORMAL_RESET_DELAY_MS);
>
> This is confusing; can you add some comments here? Under what conditions
> would we bail and reset the device? Surely this is unlikely?
I'll add comments, the current goodix.c has the same scheme where the firmwre
can ask for a reset.
>
>> +}
>> +
>> +static irqreturn_t goodix_berlin_threadirq_func(int irq, void *data)
>> +{
>> + struct goodix_berlin_core *cd = data;
>> + int ret;
>> +
>> + ret = goodix_berlin_event_handler(cd);
>> + if (likely(!ret)) {
>> + switch (cd->ts_event.event_type) {
>> + case EVENT_TOUCH:
>> + goodix_berlin_report_finger(cd);
>> + break;
>> +
>> + case EVENT_REQUEST:
>> + goodix_berlin_request_handle(cd);
>> + break;
>> +
>> + /* TOFIX: Handle more request types */
>> + case EVENT_INVALID:
>> + break;
>> + }
>> + }
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static int goodix_berlin_input_dev_config(struct goodix_berlin_core *cd,
>> + const struct input_id *id)
>> +{
>> + struct input_dev *input_dev = devm_input_allocate_device(cd->dev);
>
> Please break this assignment into its own line.
>
>> +
>> + if (!input_dev)
>> + return -ENOMEM;
>> +
>> + cd->input_dev = input_dev;
>> + input_set_drvdata(input_dev, cd);
>> +
>> + input_dev->name = "Goodix Berlin Capacitive TouchScreen";
>> + input_dev->phys = "input/ts";
>> + input_dev->dev.parent = cd->dev;
>> +
>> + memcpy(&input_dev->id, id, sizeof(*id));
>> +
>> + /* set input parameters */
>> + input_set_abs_params(cd->input_dev, ABS_MT_POSITION_X, 0, SZ_64K - 1, 0, 0);
>> + input_set_abs_params(cd->input_dev, ABS_MT_POSITION_Y, 0, SZ_64K - 1, 0, 0);
>> + input_set_abs_params(cd->input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
>> +
>> + /* Override with DT */
>
> This comment is unnecessary; furthermore touchscreen_parse_properties() is not
> limited to OF.
>
>> + touchscreen_parse_properties(cd->input_dev, true, &cd->props);
>> +
>> + input_mt_init_slots(cd->input_dev, GOODIX_MAX_TOUCH, INPUT_MT_DIRECT);
>
> Please check this function's return value and bail if negative.
>
>> +
>> + return input_register_device(cd->input_dev);
>> +}
>> +
>> +static void goodix_berlin_release_connects(struct goodix_berlin_core *cd)
>> +{
>> + struct input_dev *input_dev = cd->input_dev;
>> + int i;
>> +
>> + mutex_lock(&input_dev->mutex);
>> +
>> + for (i = 0; i < GOODIX_MAX_TOUCH; i++) {
>> + input_mt_slot(input_dev, i);
>> + input_mt_report_slot_state(input_dev, MT_TOOL_FINGER, false);
>> + }
>> + input_mt_sync_frame(input_dev);
>> + input_sync(input_dev);
>> +
>> + mutex_unlock(&input_dev->mutex);
>> +}
>> +
>> +static int goodix_berlin_suspend(struct goodix_berlin_core *cd)
>> +{
>> + /* disable irq */
>> + goodix_berlin_irq_enable(cd, false);
>> +
>> + /* enter sleep mode or power off */
>> + goodix_berlin_do_suspend(cd);
>> +
>> + goodix_berlin_release_connects(cd);
>
> For my own understanding, why is this operation necessary? Do you expect
> a platform to suspend if the user's fingers remain on the panel? I also
> find it confusing that multiple entry points can call input_mt_sync_frame()
> or input_sync(); typically these are limited to a common handler.
I don't know either, I can drop it if it's looks fishy.
>
>> +
>> + return 0;
>> +}
>> +
>> +static int goodix_berlin_resume(struct goodix_berlin_core *cd)
>> +{
>> + goodix_berlin_irq_enable(cd, false);
>> +
>> + /* reset device or power on*/
>> + goodix_berlin_do_resume(cd);
>> +
>> + /* enable irq */
>> + goodix_berlin_irq_enable(cd, true);
>> +
>> + return 0;
>> +}
>> +
>> +int goodix_berlin_pm_suspend(struct device *dev)
>> +{
>> + struct goodix_berlin_core *cd = dev_get_drvdata(dev);
>> +
>> + return goodix_berlin_suspend(cd);
>> +}
>> +
>> +int goodix_berlin_pm_resume(struct device *dev)
>> +{
>> + struct goodix_berlin_core *cd = dev_get_drvdata(dev);
>> +
>> + return goodix_berlin_resume(cd);
>> +}
>> +
>> +EXPORT_GPL_SIMPLE_DEV_PM_OPS(goodix_berlin_pm_ops,
>> + goodix_berlin_pm_suspend,
>> + goodix_berlin_pm_resume);
>> +
>> +int goodix_berlin_probe(struct device *dev, int irq, const struct input_id *id,
>> + struct regmap *regmap)
>> +{
>> + struct goodix_berlin_core *cd;
>> + int ret;
>> +
>> + if (irq <= 0)
>> + return -EINVAL;
>> +
>> + cd = devm_kzalloc(dev, sizeof(*cd), GFP_KERNEL);
>> + if (!cd)
>> + return -ENOMEM;
>> +
>> + cd->dev = dev;
>> + cd->regmap = regmap;
>> + cd->irq = irq;
>> +
>> + cd->reset_gpio = devm_gpiod_get(cd->dev, "reset", GPIOF_OUT_INIT_HIGH);
>> + if (IS_ERR(cd->reset_gpio))
>> + return dev_err_probe(cd->dev, PTR_ERR(cd->reset_gpio),
>> + "Failed to request reset gpio\n");
>
> In my experience, the reset GPIO is the first to go in case the board designer
> runs out of GPIO. Can it be optional?
Sure, I need to make it optional in the bindings
>
>> +
>> + cd->avdd = devm_regulator_get(cd->dev, "avdd");
>> + if (IS_ERR(cd->avdd))
>> + return dev_err_probe(cd->dev, PTR_ERR(cd->avdd),
>> + "Failed to request avdd regulator\n");
>> +
>> + cd->iovdd = devm_regulator_get(cd->dev, "iovdd");
>> + if (IS_ERR(cd->iovdd))
>> + return dev_err_probe(cd->dev, PTR_ERR(cd->iovdd),
>> + "Failed to request iovdd regulator\n");
>> +
>> + ret = goodix_berlin_input_dev_config(cd, id);
>> + if (ret < 0) {
>> + dev_err(cd->dev, "failed set input device");
>> + return ret;
>> + }
>> +
>> + ret = devm_request_threaded_irq(dev, irq, NULL,
>> + goodix_berlin_threadirq_func,
>> + IRQF_ONESHOT, "goodix-berlin", cd);
>> + if (ret) {
>> + dev_err(dev, "request threaded irq failed: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + dev_set_drvdata(dev, cd);
>> +
>> + ret = goodix_berlin_power_on(cd, true);
>
> This seems like something that can be posted as an device-managed action
> as opposed to having subsequent goto statements from here on.
>
>> + if (ret) {
>> + dev_err(cd->dev, "failed power on");
>> + goto err_out;
>> + }
>> +
>> + ret = goodix_berlin_read_version(cd, &cd->fw_version);
>> + if (ret < 0) {
>> + dev_err(cd->dev, "failed to get version info");
>> + goto err_out;
>> + }
>> +
>> + ret = goodix_berlin_get_ic_info(cd, &cd->ic_info);
>> + if (ret) {
>> + dev_err(cd->dev, "invalid ic info, abort");
>> + goto err_out;
>> + }
>> +
>> + cd->debugfs_root = debugfs_create_dir(dev_name(dev), NULL);
>> + debugfs_create_file("fw_version", 0444, cd->debugfs_root,
>> + cd, &goodix_berlin_fw_version_fops);
>> + debugfs_create_file("ic_info", 0444, cd->debugfs_root,
>> + cd, &goodix_berlin_ic_info_fops);
>
> We need not explicitly create attributes; consider the dev_groups member
> of the device_driver struct instead.
>
>> +
>> + dev_info(cd->dev, "Goodix Berlin %s Touchscreen Controller", cd->fw_version.patch_pid);
>
> dev_dbg()
>
>> +
>> + return 0;
>> +
>> +err_out:
>> + goodix_berlin_power_on(cd, false);
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(goodix_berlin_probe);
>> +
>> +int goodix_berlin_remove(struct device *dev)
>> +{
>> + struct goodix_berlin_core *cd = dev_get_drvdata(dev);
>> +
>> + goodix_berlin_release_connects(cd);
>> +
>> + goodix_berlin_irq_enable(cd, false);
>> +
>> + goodix_berlin_power_on(cd, false);
>> +
>> + debugfs_remove(cd->debugfs_root);
>> +
>> + return 0;
>
> I suspect much of this is unnecessary if you adopt modern primitives. Once
> you wrap regulator disables with device-managed actions, you should be able
> to get rid of the remove callback altogether.
Ack I'll try to reduce in order to remove this callback.
>
>> +}
>> +EXPORT_SYMBOL_GPL(goodix_berlin_remove);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("Goodix Berlin Core Touchscreen driver");
>> +MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
>>
>> --
>> 2.34.1
>>
>
> Kind regards,
> Jeff LaBundy
^ permalink raw reply
* Tree dumb questions from an occasional
From: Marco Morandini @ 2023-06-12 10:29 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, linux-input
First of all, please bear with me for writing this.
Should this appear email to be a criticism toward any of you,
be assured that this is not the intention. I'm
really grateful to all you guys, who keep improving the kernel
and allows us to use a free operating system.
My questions here below are basically along the lines of
"would it make sense to write this and that?". This is not
because I'd like someone else to do the work for me, but
because I really don't know whether it makes sense or not.
Should be wrong to make such type of questions without
accompanying proofs of concept, then please ignore me, and
I'll go back into my cave.
Some background: I've recently bought a laser digital pointer
from HP. It connects through Bluetooth.
Since it did not work, I made a point to have it working,
even if I'm not a kernel developer, and do not plan to become a
kernel developer.
At the end, this turned out to be a two-line patch, adding
the HID_QUIRK_MULTI_INPUT for such device. Something that
would likely require less than 5 minutes
of a not-so proficient kernel developer.
However, the process for me was much more cumbersome:
I had to navigate a lot o wrong or misleading documentation
in different forums, try to make sense of the kernel documentation,
understand what a HID descriptor is,
understand how to parse it, try to make sense of some
unknown kernel code (mostly unsuccessfully),
try with ebpf, try this and that... you get the idea.
Now, I'm writing because I _think_ I've learned something
in the process, and perhaps it could be useful to share it.
Thus the questions:
1) do you think it would make sense to have some basic documentation
describing what a hid descriptor is, where to download the documents
defining it (https://www.usb.org/ is linked from the docs,
but this is not enough to get started, at least for someone like me),
how to actually read it from the hardware, how to parse it... ?
Very basic things, that, if I'm not wrong, are not currently
covered by the kernel documentation, and that could allow
someone else like me to get started more quickly?
If yes, I can try to write a skeleton for that, but I'm not sure
there will not be errors and/or omissions, thus it would likely need
to be fixed by someone more knowledgeable than me.
2) if I got it right, one can add a quirk like HID_QUIRK_MULTI_INPUT
while loading the usb_hid module, but not while loading the usb_generic
one (that turned out to be the module that manages my HP pointer),
even if the statically defined quirks were moved into their own file.
Would it make sense to add the possibility to
add quirks while loading hid_generic? Is this the right place for
such code? If yes, I can try in my spare time to do this,
even if I'm not sure I'll be able to get it right.
3) always if I get it right, currently it is not possible to inject quirks
using ebpf, but only to modify the HID descriptor.
Is this correct? If yes, do you think it would be feasible and reasonable
to add such a possibility? If yes, I can try in my spare time to do this,
even if I'm not sure I'll be able to get it right.
Apologize again for this long email:
I understand it's full of good intentions but without any
significant contribution :(
Marco
^ permalink raw reply
* Re: [PATCH RFC 4/4] input: touchscreen: add SPI support for Goodix Berlin Touchscreen IC
From: Neil Armstrong @ 2023-06-12 12:01 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bastien Nocera, Hans de Goede, Henrik Rydberg, linux-input,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <ZIaRoTHar/s5yZAh@nixie71>
Hi,
On 12/06/2023 05:31, Jeff LaBundy wrote:
> Hi Neil,
>
> On Tue, Jun 06, 2023 at 04:31:59PM +0200, Neil Armstrong wrote:
>> 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
>>
>> 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 | 183 ++++++++++++++++++++++++++
>> 3 files changed, 198 insertions(+)
>>
>> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
>> index da6d5d75c42d..ffe0c0a4cd15 100644
>> --- a/drivers/input/touchscreen/Kconfig
>> +++ b/drivers/input/touchscreen/Kconfig
>> @@ -435,6 +435,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
>> + depends on REGMAP
>
> As TOUCHSCREEN_GOODIX_BERLIN_CORE already depends on REGMAP; is this
> line necessary?
>
> I was about to ask why not to select REGMAP_SPI; thank you for the
> additional information in the commit message.
>
>> + select TOUCHSCREEN_GOODIX_BERLIN_CORE
>> + help
>> + Say Y here if you have the a touchscreen connected to your
>> + system using the Goodix Berlin IC connection via SPI.
>
> Same comment here with regard to diction.
>
>> +
>> + 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 921a2da0c2be..29524e8a83db 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..0f4f650fdf3f
>> --- /dev/null
>> +++ b/drivers/input/touchscreen/goodix_berlin_spi.c
>> @@ -0,0 +1,183 @@
>> +// 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 <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/spi/spi.h>
>> +#include <linux/regmap.h>
>
> Please alphabetize these to aid readability.
>
>> +#include <asm/unaligned.h>
>> +
>> +#include "goodix_berlin.h"
>> +
>> +#define SPI_TRANS_PREFIX_LEN 1
>> +#define REGISTER_WIDTH 4
>> +#define SPI_READ_DUMMY_LEN 3
>> +#define SPI_READ_PREFIX_LEN (SPI_TRANS_PREFIX_LEN + REGISTER_WIDTH + SPI_READ_DUMMY_LEN)
>> +#define SPI_WRITE_PREFIX_LEN (SPI_TRANS_PREFIX_LEN + REGISTER_WIDTH)
>> +
>> +#define SPI_WRITE_FLAG 0xF0
>> +#define 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 = NULL;
>> + int ret = 0;
>
> No need to initialize these, only to forcibly assign them later.
>
>> +
>> + if (reg_size != REGISTER_WIDTH)
>> + return -EINVAL;
>> +
>> + buf = kzalloc(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] = SPI_READ_FLAG;
>> + put_unaligned_be32(*reg, buf + SPI_TRANS_PREFIX_LEN);
>> + memset(buf + SPI_TRANS_PREFIX_LEN + REGISTER_WIDTH, 0xff,
>> + SPI_READ_DUMMY_LEN);
>> +
>> + xfers.tx_buf = buf;
>> + xfers.rx_buf = buf;
>> + xfers.len = SPI_READ_PREFIX_LEN + val_size;
>> + xfers.cs_change = 0;
>> + spi_message_add_tail(&xfers, &spi_msg);
>> +
>> + ret = spi_sync(spi, &spi_msg);
>> + if (ret < 0) {
>> + dev_err(&spi->dev, "transfer error:%d", ret);
>> + goto exit;
>> + }
>
> My comment is purely idiomatic, but this seems cleaner:
>
> ret = ...
> if (ret)
> dev_err(...);
> else
> memcpy(...);
>
> kfree(...);
> return ret;
>
>> +
>> + memcpy(val_buf, buf + SPI_READ_PREFIX_LEN, val_size);
>> +exit:
>> + kfree(buf);
>> + return ret;
>> +}
>> +
>> +static int goodix_berlin_spi_write(void *context, const void *data,
>> + size_t count)
>> +{
>> + unsigned int len = count - 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 = NULL;
>> + int ret = 0;
>
> Same comment here with regard to initialization.
>
>> +
>> + buf = kzalloc(SPI_WRITE_PREFIX_LEN + len, GFP_KERNEL);
>> + if (!buf)
>> + return -ENOMEM;
>> +
>> + spi_message_init(&spi_msg);
>> + memset(&xfers, 0, sizeof(xfers));
>> +
>> + buf[0] = SPI_WRITE_FLAG;
>> + put_unaligned_be32(*reg, buf + SPI_TRANS_PREFIX_LEN);
>> + memcpy(buf + SPI_WRITE_PREFIX_LEN, data + REGISTER_WIDTH, len);
>> +
>> + xfers.tx_buf = buf;
>> + xfers.len = SPI_WRITE_PREFIX_LEN + len;
>> + xfers.cs_change = 0;
>> + spi_message_add_tail(&xfers, &spi_msg);
>> +
>> + ret = spi_sync(spi, &spi_msg);
>> + if (ret < 0)
>> + dev_err(&spi->dev, "transfer error:%d", ret);
>> +
>> + kfree(buf);
>> + return ret;
>> +}
>> +
>> +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,
>> +};
>> +
>> +static const struct input_id goodix_berlin_spi_input_id = {
>> + .bustype = BUS_SPI,
>> + .vendor = 0x0416,
>> + .product = 0x1001,
>
> After having seen these in the I2C counterpart; consider defining them
> in goodix_berlin.h.
To be honest, I blindly copied it from goodix.c because the vendor
driver puts random values here.
input_dev->id.product = 0xDEAD;
input_dev->id.vendor = 0xBEEF;
So what should I set ?
Neil
>
>> +};
>> +
>> +static int goodix_berlin_spi_probe(struct spi_device *spi)
>> +{
>> + struct regmap_config *cfg;
>
> regmap_config
>
>> + struct regmap *map;
>
> regmap
>
> (see more examples in MFD where such dual-mode devices are common).
>
>> + size_t max_size;
>> + int ret = 0;
>> +
>> + cfg = devm_kmemdup(&spi->dev, &goodix_berlin_spi_regmap_conf,
>> + sizeof(*cfg), GFP_KERNEL);
>> + if (!cfg)
>> + return -ENOMEM;
>> +
>> + spi->mode = SPI_MODE_0;
>> + spi->bits_per_word = 8;
>> + ret = spi_setup(spi);
>> + if (ret)
>> + return ret;
>> +
>> + max_size = spi_max_transfer_size(spi);
>> + cfg->max_raw_read = max_size - SPI_READ_PREFIX_LEN;
>> + cfg->max_raw_write = max_size - SPI_WRITE_PREFIX_LEN;
>> +
>> + map = devm_regmap_init(&spi->dev, NULL, spi, cfg);
>> + if (IS_ERR(map))
>> + return PTR_ERR(map);
>> +
>> + return goodix_berlin_probe(&spi->dev, spi->irq,
>> + &goodix_berlin_spi_input_id, map);
>> +}
>> +
>> +static void goodix_berlin_spi_remove(struct spi_device *spi)
>> +{
>> + goodix_berlin_remove(&spi->dev);
>> +}
>> +
>> +static const struct of_device_id goodix_berlin_spi_of_match[] = {
>> + {
>> + .compatible = "goodix,gt9916",
>> + },
>
> This format is different than its I2C counterpart.
>
>> + { },
>
> Nit: same comment with regards to trailing commas.
>
>> +};
>> +MODULE_DEVICE_TABLE(of, goodix_berlin_spi_of_match);
>> +
>> +static const struct spi_device_id goodix_berlin_spi_ids[] = {
>> + { "gt9916" },
>> + { },
>
> And here.
>
>> +};
>> +MODULE_DEVICE_TABLE(spi, goodix_berlin_spi_ids);
>> +
>> +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),
>> + },
>> + .id_table = goodix_berlin_spi_ids,
>> + .probe = goodix_berlin_spi_probe,
>> + .remove = goodix_berlin_spi_remove,
>> +};
>> +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
>>
>
> Kind regards,
> Jeff LaBundy
^ permalink raw reply
* Re: Tree dumb questions from an occasional
From: Benjamin Tissoires @ 2023-06-12 13:07 UTC (permalink / raw)
To: Marco Morandini; +Cc: Jiri Kosina, linux-input
In-Reply-To: <5673fd15-b3f2-7911-6eea-39e5126f4874@polimi.it>
Hi Marco,
On Mon, Jun 12, 2023 at 12:29 PM Marco Morandini
<marco.morandini@polimi.it> wrote:
>
> First of all, please bear with me for writing this.
>
> Should this appear email to be a criticism toward any of you,
> be assured that this is not the intention. I'm
> really grateful to all you guys, who keep improving the kernel
> and allows us to use a free operating system.
>
> My questions here below are basically along the lines of
> "would it make sense to write this and that?". This is not
> because I'd like someone else to do the work for me, but
> because I really don't know whether it makes sense or not.
> Should be wrong to make such type of questions without
> accompanying proofs of concept, then please ignore me, and
> I'll go back into my cave.
Asking questions is always fine. And given that you already did a lot
of homework, not answering would be rude ;)
>
> Some background: I've recently bought a laser digital pointer
> from HP. It connects through Bluetooth.
> Since it did not work, I made a point to have it working,
> even if I'm not a kernel developer, and do not plan to become a
> kernel developer.
> At the end, this turned out to be a two-line patch, adding
> the HID_QUIRK_MULTI_INPUT for such device. Something that
> would likely require less than 5 minutes
> of a not-so proficient kernel developer.
Yeah, right. This is the kind of situation where it's usually easy
enough to detect with hid-tools[0]. We can record the device on your
machine, then we can replay it locally on ours, and make several
attempts.
>
> However, the process for me was much more cumbersome:
> I had to navigate a lot o wrong or misleading documentation
> in different forums, try to make sense of the kernel documentation,
> understand what a HID descriptor is,
> understand how to parse it, try to make sense of some
> unknown kernel code (mostly unsuccessfully),
> try with ebpf, try this and that... you get the idea.
Heh, you tried hid-bpf :) thanks!
>
> Now, I'm writing because I _think_ I've learned something
> in the process, and perhaps it could be useful to share it.
>
> Thus the questions:
>
> 1) do you think it would make sense to have some basic documentation
> describing what a hid descriptor is, where to download the documents
> defining it (https://www.usb.org/ is linked from the docs,
> but this is not enough to get started, at least for someone like me),
> how to actually read it from the hardware, how to parse it... ?
Yes, very much yes. At least having pointers to various projects that
can read HID descriptors and parse them would be already better.
> Very basic things, that, if I'm not wrong, are not currently
> covered by the kernel documentation, and that could allow
> someone else like me to get started more quickly?
> If yes, I can try to write a skeleton for that, but I'm not sure
> there will not be errors and/or omissions, thus it would likely need
> to be fixed by someone more knowledgeable than me.
Sure. Please write (if you want) your first draft, we can review it
and we can iterate from there. Do not forget to add the linux doc
mailing list in CC in case some people from there want to also add
things.
>
> 2) if I got it right, one can add a quirk like HID_QUIRK_MULTI_INPUT
> while loading the usb_hid module, but not while loading the usb_generic
> one (that turned out to be the module that manages my HP pointer),
> even if the statically defined quirks were moved into their own file.
> Would it make sense to add the possibility to
> add quirks while loading hid_generic? Is this the right place for
> such code? If yes, I can try in my spare time to do this,
> even if I'm not sure I'll be able to get it right.
I'm not 100% sure of what you mean, but currently dynamic quirks can
be added to the *usbhid* module (not usb_hid or usb_generic), which is
the transport layer driver for HID.
This module is responsible for creating a HID device, which can be
handled by HID drivers, hid_generic being one of them.
As the name says, hid_generic is supposed to be generic, and I do not
want to see special cases handled there, because it would not be
generic anymore.
However, other drivers, (hid_multitouch for instance) can have a
.driver_data dynamically set too, which allows for quirking a device.
But the quirk is local to the driver.
Given that HID_QUIRK_MULTI_INPUT is a global HID quirk, it makes sense
IMO to add it at the usbhid level because you are just using the
hid-generic implementation.
Furthermore, if you submit your patch to the LKML, the quirk will
likely end up in driver/hid/hid-quirks.c which is exactly the static
equivalent of the dynamic one from usbhid.
So I don't think having such a new quirk mechanism makes sense.
Furthermore, that quirk mechanism allows for quick and dirty testing
of the impact on the kernel, but a proper submission as a kernel patch
ensures that everybody gets the same fix, so I'd rather not have a
forum that explains in details what to do for a given HID product when
we can just quirk it in the tree and forget.
>
> 3) always if I get it right, currently it is not possible to inject quirks
> using ebpf, but only to modify the HID descriptor.
> Is this correct?
You can also change the events and filter them out if you want, but
yes that's pretty much the extent of HID_BPF nowadays.
> If yes, do you think it would be feasible and reasonable
> to add such a possibility? If yes, I can try in my spare time to do this,
> even if I'm not sure I'll be able to get it right.
Long story short: I would rather not have such bpf call.
The fundamental of HID-BPF is that you take a HID thingy, and you
output another HID thingy. Messing up with the kernel drivers is out
of the scope.
But furthermore, what would be the benefit compared to the already
available dynamic quirks the kernel allows to have? You can simply add
a configuration file to your system to locally have the dynamic quirk
set, and then it gets re-applied on every reboot.
As much as I'd like bpf to be the universal answer, this doesn't seem
to be a good replacement to what we currently have.
HID-BPF seems to be a good answer to replace (some) drivers, but I
don't think it should replace the generic kernel processing. So that's
why I don't think this is a good idea.
>
> Apologize again for this long email:
> I understand it's full of good intentions but without any
> significant contribution :(
No need to apologize. You are actually proposing ideas and your help
to make things better for end-users, which is extremely valuable in
itself :)
Cheers,
Benjamin
[0] https://gitlab.freedesktop.org/libevdev/hid-tools
^ 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