[PATCH v1 00/12] HID: cp2112: Cleanups and refactorings

[PATCH v1 00/12] HID: cp2112: Cleanups and refactorings

[Andy Shevchenko]

After I updated GPIO library for the case Benjamin has with CP2112,
I have a brief look into the CP2112 driver itself.

From GPIO perspective it has two main (maitenance) issues:
- usage of ->to_irq() with IRQ chip present;
- having IRQ chip not immutable.

Besides that there are plenty small cleanups here and there.
Hence this series.

Compile tested only.

Andy Shevchenko (12):
  lib/string_choices: Add str_write_read() helper
  HID: cp2112: Use str_write_read() and str_read_write()
  HID: cp2112: Make irq_chip immutable
  HID: cp2112: Switch to for_each_set_bit() to simplify the code
  HID: cp2112: Don't call ->to_irq() explicitly
  HID: cp2112: Remove dead code
  HID: cp2112: Define maximum GPIO constant and use it
  HID: cp2112: Define all GPIO mask and use it
  HID: cp2112: Use BIT() in GPIO setter and getter
  HID: cp2112: Use sysfs_emit() to instead of scnprintf()
  HID: cp2112: Convert to DEVICE_ATTR_RW()
  HID: cp2112: Use octal permissions

 drivers/hid/hid-cp2112.c       | 169 +++++++++++----------------------
 include/linux/string_choices.h |   1 +
 2 files changed, 58 insertions(+), 112 deletions(-)

-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 01/12] lib/string_choices: Add str_write_read() helper

[Andy Shevchenko]

Add an inversed variant of str_read_write(), i.e. str_write_read().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/string_choices.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/string_choices.h b/include/linux/string_choices.h
index 48120222b9b2..3c1091941eb8 100644
--- a/include/linux/string_choices.h
+++ b/include/linux/string_choices.h
@@ -30,6 +30,7 @@ static inline const char *str_read_write(bool v)
 {
 	return v ? "read" : "write";
 }
+#define str_write_read(v)		str_read_write(!(v))
 
 static inline const char *str_on_off(bool v)
 {
-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 02/12] HID: cp2112: Use str_write_read() and str_read_write()

[Andy Shevchenko]

Use str_write_read() and str_read_write() from string_choices.h.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/hid/hid-cp2112.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 27cadadda7c9..37ccf4714ad1 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -24,6 +24,7 @@
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/nls.h>
+#include <linux/string_choices.h>
 #include <linux/usb/ch9.h>
 #include "hid-ids.h"
 
@@ -532,15 +533,13 @@ static int cp2112_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	hid_dbg(hdev, "I2C %d messages\n", num);
 
 	if (num == 1) {
+		hid_dbg(hdev, "I2C %s %#04x len %d\n",
+			str_read_write(msgs->flags & I2C_M_RD), msgs->addr, msgs->len);
 		if (msgs->flags & I2C_M_RD) {
-			hid_dbg(hdev, "I2C read %#04x len %d\n",
-				msgs->addr, msgs->len);
 			read_length = msgs->len;
 			read_buf = msgs->buf;
 			count = cp2112_read_req(buf, msgs->addr, msgs->len);
 		} else {
-			hid_dbg(hdev, "I2C write %#04x len %d\n",
-				msgs->addr, msgs->len);
 			count = cp2112_i2c_write_req(buf, msgs->addr,
 						     msgs->buf, msgs->len);
 		}
@@ -648,7 +647,7 @@ static int cp2112_xfer(struct i2c_adapter *adap, u16 addr,
 	int ret;
 
 	hid_dbg(hdev, "%s addr 0x%x flags 0x%x cmd 0x%x size %d\n",
-		read_write == I2C_SMBUS_WRITE ? "write" : "read",
+		str_write_read(read_write == I2C_SMBUS_WRITE),
 		addr, flags, command, size);
 
 	switch (size) {
-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 03/12] HID: cp2112: Make irq_chip immutable

[Andy Shevchenko]

Since recently, the kernel is nagging about mutable irq_chips:

   "not an immutable chip, please consider fixing it!"

Drop the unneeded copy, flag it as IRQCHIP_IMMUTABLE, add the new
helper functions and call the appropriate gpiolib functions.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/hid/hid-cp2112.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 37ccf4714ad1..51399b231d36 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -164,7 +164,6 @@ struct cp2112_device {
 	atomic_t read_avail;
 	atomic_t xfer_avail;
 	struct gpio_chip gc;
-	struct irq_chip irq;
 	u8 *in_out_buffer;
 	struct mutex lock;
 
@@ -1079,16 +1078,20 @@ static void cp2112_gpio_irq_mask(struct irq_data *d)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct cp2112_device *dev = gpiochip_get_data(gc);
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
 
-	__clear_bit(d->hwirq, &dev->irq_mask);
+	__clear_bit(hwirq, &dev->irq_mask);
+	gpiochip_disable_irq(gc, hwirq);
 }
 
 static void cp2112_gpio_irq_unmask(struct irq_data *d)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct cp2112_device *dev = gpiochip_get_data(gc);
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
 
-	__set_bit(d->hwirq, &dev->irq_mask);
+	gpiochip_enable_irq(gc, hwirq);
+	__set_bit(hwirq, &dev->irq_mask);
 }
 
 static void cp2112_gpio_poll_callback(struct work_struct *work)
@@ -1174,6 +1177,7 @@ static void cp2112_gpio_irq_shutdown(struct irq_data *d)
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct cp2112_device *dev = gpiochip_get_data(gc);
 
+	cp2112_gpio_irq_mask(d);
 	cancel_delayed_work_sync(&dev->gpio_poll_worker);
 }
 
@@ -1227,6 +1231,18 @@ static int __maybe_unused cp2112_allocate_irq(struct cp2112_device *dev,
 	return ret;
 }
 
+static const struct irq_chip cp2112_gpio_irqchip = {
+	.name = "cp2112-gpio",
+	.irq_startup = cp2112_gpio_irq_startup,
+	.irq_shutdown = cp2112_gpio_irq_shutdown,
+	.irq_ack = cp2112_gpio_irq_ack,
+	.irq_mask = cp2112_gpio_irq_mask,
+	.irq_unmask = cp2112_gpio_irq_unmask,
+	.irq_set_type = cp2112_gpio_irq_type,
+	.flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_IMMUTABLE,
+	GPIOCHIP_IRQ_RESOURCE_HELPERS,
+};
+
 static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
 {
 	struct cp2112_device *dev;
@@ -1336,17 +1352,8 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	dev->gc.can_sleep		= 1;
 	dev->gc.parent			= &hdev->dev;
 
-	dev->irq.name = "cp2112-gpio";
-	dev->irq.irq_startup = cp2112_gpio_irq_startup;
-	dev->irq.irq_shutdown = cp2112_gpio_irq_shutdown;
-	dev->irq.irq_ack = cp2112_gpio_irq_ack;
-	dev->irq.irq_mask = cp2112_gpio_irq_mask;
-	dev->irq.irq_unmask = cp2112_gpio_irq_unmask;
-	dev->irq.irq_set_type = cp2112_gpio_irq_type;
-	dev->irq.flags = IRQCHIP_MASK_ON_SUSPEND;
-
 	girq = &dev->gc.irq;
-	girq->chip = &dev->irq;
+	gpio_irq_chip_set_chip(girq, &cp2112_gpio_irqchip);
 	/* The event comes from the outside so no parent handler */
 	girq->parent_handler = NULL;
 	girq->num_parents = 0;
-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 04/12] HID: cp2112: Switch to for_each_set_bit() to simplify the code

[Andy Shevchenko]

It's cleaner to use for_each_set_bit() than open coding it.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/hid/hid-cp2112.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 51399b231d36..fb4548feb0c8 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -16,6 +16,7 @@
  *   https://www.silabs.com/documents/public/application-notes/an495-cp2112-interface-specification.pdf
  */
 
+#include <linux/bitops.h>
 #include <linux/gpio/consumer.h>
 #include <linux/gpio/machine.h>
 #include <linux/gpio/driver.h>
@@ -1100,7 +1101,6 @@ static void cp2112_gpio_poll_callback(struct work_struct *work)
 						 gpio_poll_worker.work);
 	struct irq_data *d;
 	u8 gpio_mask;
-	u8 virqs = (u8)dev->irq_mask;
 	u32 irq_type;
 	int irq, virq, ret;
 
@@ -1111,11 +1111,7 @@ static void cp2112_gpio_poll_callback(struct work_struct *work)
 		goto exit;
 
 	gpio_mask = ret;
-
-	while (virqs) {
-		virq = ffs(virqs) - 1;
-		virqs &= ~BIT(virq);
-
+	for_each_set_bit(virq, &dev->irq_mask, 8) {
 		if (!dev->gc.to_irq)
 			break;
 
-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 05/12] HID: cp2112: Don't call ->to_irq() explicitly

[Andy Shevchenko]

GPIO library guarantees that ->to_irq() is always exists.
Moreover, it tending to become a nische thingy and has to
not be used in ordinary drivers. Hence, replace that by
irq_find_mapping().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/hid/hid-cp2112.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index fb4548feb0c8..15b626359281 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -1112,10 +1112,9 @@ static void cp2112_gpio_poll_callback(struct work_struct *work)
 
 	gpio_mask = ret;
 	for_each_set_bit(virq, &dev->irq_mask, 8) {
-		if (!dev->gc.to_irq)
-			break;
-
-		irq = dev->gc.to_irq(&dev->gc, virq);
+		irq = irq_find_mapping(dev->gc.irq.domain, virq);
+		if (!irq)
+			continue;
 
 		d = irq_get_irq_data(irq);
 		if (!d)
-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 06/12] HID: cp2112: Remove dead code

[Andy Shevchenko]

Remove cp2112_allocate_irq() and counterparts that seems to be
a dead code from day 1. In case somebody needs it, it can be
retrieved from Git index.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/hid/hid-cp2112.c | 54 ----------------------------------------
 1 file changed, 54 deletions(-)

diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 15b626359281..45cd0d2fd3fd 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -17,8 +17,6 @@
  */
 
 #include <linux/bitops.h>
-#include <linux/gpio/consumer.h>
-#include <linux/gpio/machine.h>
 #include <linux/gpio/driver.h>
 #include <linux/hid.h>
 #include <linux/hidraw.h>
@@ -168,7 +166,6 @@ struct cp2112_device {
 	u8 *in_out_buffer;
 	struct mutex lock;
 
-	struct gpio_desc *desc[8];
 	bool gpio_poll;
 	struct delayed_work gpio_poll_worker;
 	unsigned long irq_mask;
@@ -1181,51 +1178,6 @@ static int cp2112_gpio_irq_type(struct irq_data *d, unsigned int type)
 	return 0;
 }
 
-static int __maybe_unused cp2112_allocate_irq(struct cp2112_device *dev,
-					      int pin)
-{
-	int ret;
-
-	if (dev->desc[pin])
-		return -EINVAL;
-
-	dev->desc[pin] = gpiochip_request_own_desc(&dev->gc, pin,
-						   "HID/I2C:Event",
-						   GPIO_ACTIVE_HIGH,
-						   GPIOD_IN);
-	if (IS_ERR(dev->desc[pin])) {
-		dev_err(dev->gc.parent, "Failed to request GPIO\n");
-		return PTR_ERR(dev->desc[pin]);
-	}
-
-	ret = cp2112_gpio_direction_input(&dev->gc, pin);
-	if (ret < 0) {
-		dev_err(dev->gc.parent, "Failed to set GPIO to input dir\n");
-		goto err_desc;
-	}
-
-	ret = gpiochip_lock_as_irq(&dev->gc, pin);
-	if (ret) {
-		dev_err(dev->gc.parent, "Failed to lock GPIO as interrupt\n");
-		goto err_desc;
-	}
-
-	ret = gpiod_to_irq(dev->desc[pin]);
-	if (ret < 0) {
-		dev_err(dev->gc.parent, "Failed to translate GPIO to IRQ\n");
-		goto err_lock;
-	}
-
-	return ret;
-
-err_lock:
-	gpiochip_unlock_as_irq(&dev->gc, pin);
-err_desc:
-	gpiochip_free_own_desc(dev->desc[pin]);
-	dev->desc[pin] = NULL;
-	return ret;
-}
-
 static const struct irq_chip cp2112_gpio_irqchip = {
 	.name = "cp2112-gpio",
 	.irq_startup = cp2112_gpio_irq_startup,
@@ -1390,7 +1342,6 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
 static void cp2112_remove(struct hid_device *hdev)
 {
 	struct cp2112_device *dev = hid_get_drvdata(hdev);
-	int i;
 
 	sysfs_remove_group(&hdev->dev.kobj, &cp2112_attr_group);
 	i2c_del_adapter(&dev->adap);
@@ -1400,11 +1351,6 @@ static void cp2112_remove(struct hid_device *hdev)
 		cancel_delayed_work_sync(&dev->gpio_poll_worker);
 	}
 
-	for (i = 0; i < ARRAY_SIZE(dev->desc); i++) {
-		gpiochip_unlock_as_irq(&dev->gc, i);
-		gpiochip_free_own_desc(dev->desc[i]);
-	}
-
 	gpiochip_remove(&dev->gc);
 	/* i2c_del_adapter has finished removing all i2c devices from our
 	 * adapter. Well behaved devices should no longer call our cp2112_xfer
-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 07/12] HID: cp2112: Define maximum GPIO constant and use it

[Andy Shevchenko]

Define maximum GPIO constant and use it in the driver.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/hid/hid-cp2112.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 45cd0d2fd3fd..c8c59d70500e 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -31,6 +31,7 @@
 #define CP2112_GPIO_CONFIG_LENGTH		5
 #define CP2112_GPIO_GET_LENGTH			2
 #define CP2112_GPIO_SET_LENGTH			3
+#define CP2112_GPIO_MAX_GPIO			8
 
 enum {
 	CP2112_GPIO_CONFIG		= 0x02,
@@ -1108,7 +1109,7 @@ static void cp2112_gpio_poll_callback(struct work_struct *work)
 		goto exit;
 
 	gpio_mask = ret;
-	for_each_set_bit(virq, &dev->irq_mask, 8) {
+	for_each_set_bit(virq, &dev->irq_mask, CP2112_GPIO_MAX_GPIO) {
 		irq = irq_find_mapping(dev->gc.irq.domain, virq);
 		if (!irq)
 			continue;
@@ -1295,7 +1296,7 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	dev->gc.set			= cp2112_gpio_set;
 	dev->gc.get			= cp2112_gpio_get;
 	dev->gc.base			= -1;
-	dev->gc.ngpio			= 8;
+	dev->gc.ngpio			= CP2112_GPIO_MAX_GPIO;
 	dev->gc.can_sleep		= 1;
 	dev->gc.parent			= &hdev->dev;
 
-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 08/12] HID: cp2112: Define all GPIO mask and use it

[Andy Shevchenko]

Define all GPIO mask and use it in the driver.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/hid/hid-cp2112.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index c8c59d70500e..37ed7fc54770 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -32,6 +32,7 @@
 #define CP2112_GPIO_GET_LENGTH			2
 #define CP2112_GPIO_SET_LENGTH			3
 #define CP2112_GPIO_MAX_GPIO			8
+#define CP2112_GPIO_ALL_GPIO_MASK		GENMASK(7, 0)
 
 enum {
 	CP2112_GPIO_CONFIG		= 0x02,
@@ -173,7 +174,7 @@ struct cp2112_device {
 	u8 gpio_prev_state;
 };
 
-static int gpio_push_pull = 0xFF;
+static int gpio_push_pull = CP2112_GPIO_ALL_GPIO_MASK;
 module_param(gpio_push_pull, int, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(gpio_push_pull, "GPIO push-pull configuration bitmask");
 
@@ -226,7 +227,7 @@ static void cp2112_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
 	mutex_lock(&dev->lock);
 
 	buf[0] = CP2112_GPIO_SET;
-	buf[1] = value ? 0xff : 0;
+	buf[1] = value ? CP2112_GPIO_ALL_GPIO_MASK : 0;
 	buf[2] = 1 << offset;
 
 	ret = hid_hw_raw_request(hdev, CP2112_GPIO_SET, buf,
-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 09/12] HID: cp2112: Use BIT() in GPIO setter and getter

[Andy Shevchenko]

Use BIT() in GPIO setter and getter for the sake of consistency
with GENMASK() usage elsewhere in the driver.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/hid/hid-cp2112.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 37ed7fc54770..2eebb2b19501 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -197,7 +197,7 @@ static int cp2112_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
 		goto exit;
 	}
 
-	buf[1] &= ~(1 << offset);
+	buf[1] &= ~BIT(offset);
 	buf[2] = gpio_push_pull;
 
 	ret = hid_hw_raw_request(hdev, CP2112_GPIO_CONFIG, buf,
@@ -228,7 +228,7 @@ static void cp2112_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
 
 	buf[0] = CP2112_GPIO_SET;
 	buf[1] = value ? CP2112_GPIO_ALL_GPIO_MASK : 0;
-	buf[2] = 1 << offset;
+	buf[2] = BIT(offset);
 
 	ret = hid_hw_raw_request(hdev, CP2112_GPIO_SET, buf,
 				 CP2112_GPIO_SET_LENGTH, HID_FEATURE_REPORT,
-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 10/12] HID: cp2112: Use sysfs_emit() to instead of scnprintf()

[Andy Shevchenko]

Follow the advice of the Documentation/filesystems/sysfs.rst and show()
should only use sysfs_emit() or sysfs_emit_at() when formatting the
value to be returned to user space.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/hid/hid-cp2112.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 2eebb2b19501..1e45f5407d09 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -893,7 +893,7 @@ static ssize_t name##_show(struct device *kdev, \
 	int ret = cp2112_get_usb_config(hdev, &cfg); \
 	if (ret) \
 		return ret; \
-	return scnprintf(buf, PAGE_SIZE, format, ##__VA_ARGS__); \
+	return sysfs_emit(buf, format, ##__VA_ARGS__); \
 } \
 static DEVICE_ATTR_RW(name);
 
-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 11/12] HID: cp2112: Convert to DEVICE_ATTR_RW()

[Andy Shevchenko]

Instead of custom wrapper, use DEVICE_tATTR_RW() directly.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/hid/hid-cp2112.c | 42 ++++++++++++++++++----------------------
 1 file changed, 19 insertions(+), 23 deletions(-)

diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 1e45f5407d09..3c6a3be8fc02 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -944,18 +944,10 @@ CP2112_CONFIG_ATTR(release_version, ({
 
 #undef CP2112_CONFIG_ATTR
 
-struct cp2112_pstring_attribute {
-	struct device_attribute attr;
-	unsigned char report;
-};
-
-static ssize_t pstr_store(struct device *kdev,
-			  struct device_attribute *kattr, const char *buf,
-			  size_t count)
+static ssize_t pstr_store(struct device *kdev, struct device_attribute *kattr,
+			  const char *buf, size_t count, int number)
 {
 	struct hid_device *hdev = to_hid_device(kdev);
-	struct cp2112_pstring_attribute *attr =
-		container_of(kattr, struct cp2112_pstring_attribute, attr);
 	struct cp2112_string_report report;
 	int ret;
 
@@ -963,7 +955,7 @@ static ssize_t pstr_store(struct device *kdev,
 
 	ret = utf8s_to_utf16s(buf, count, UTF16_LITTLE_ENDIAN,
 			      report.string, ARRAY_SIZE(report.string));
-	report.report = attr->report;
+	report.report = number;
 	report.length = ret * sizeof(report.string[0]) + 2;
 	report.type = USB_DT_STRING;
 
@@ -981,17 +973,15 @@ static ssize_t pstr_store(struct device *kdev,
 	return count;
 }
 
-static ssize_t pstr_show(struct device *kdev,
-			 struct device_attribute *kattr, char *buf)
+static ssize_t pstr_show(struct device *kdev, struct device_attribute *kattr,
+			 char *buf, int number)
 {
 	struct hid_device *hdev = to_hid_device(kdev);
-	struct cp2112_pstring_attribute *attr =
-		container_of(kattr, struct cp2112_pstring_attribute, attr);
 	struct cp2112_string_report report;
 	u8 length;
 	int ret;
 
-	ret = cp2112_hid_get(hdev, attr->report, (u8 *)&report.contents,
+	ret = cp2112_hid_get(hdev, number, (u8 *)&report.contents,
 			     sizeof(report.contents), HID_FEATURE_REPORT);
 	if (ret < 3) {
 		hid_err(hdev, "error reading %s string: %d\n", kattr->attr.name,
@@ -1016,10 +1006,16 @@ static ssize_t pstr_show(struct device *kdev,
 }
 
 #define CP2112_PSTR_ATTR(name, _report) \
-static struct cp2112_pstring_attribute dev_attr_##name = { \
-	.attr = __ATTR(name, (S_IWUSR | S_IRUGO), pstr_show, pstr_store), \
-	.report = _report, \
-};
+static ssize_t name##_store(struct device *kdev, struct device_attribute *kattr, \
+			    const char *buf, size_t count) \
+{ \
+	return pstr_store(kdev, kattr, buf, count, _report); \
+} \
+static ssize_t name##_show(struct device *kdev, struct device_attribute *kattr, char *buf) \
+{ \
+	return pstr_show(kdev, kattr, buf, _report); \
+} \
+static DEVICE_ATTR_RW(name);
 
 CP2112_PSTR_ATTR(manufacturer,	CP2112_MANUFACTURER_STRING);
 CP2112_PSTR_ATTR(product,	CP2112_PRODUCT_STRING);
@@ -1034,9 +1030,9 @@ static const struct attribute_group cp2112_attr_group = {
 		&dev_attr_max_power.attr,
 		&dev_attr_power_mode.attr,
 		&dev_attr_release_version.attr,
-		&dev_attr_manufacturer.attr.attr,
-		&dev_attr_product.attr.attr,
-		&dev_attr_serial.attr.attr,
+		&dev_attr_manufacturer.attr,
+		&dev_attr_product.attr,
+		&dev_attr_serial.attr,
 		NULL
 	}
 };
-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 12/12] HID: cp2112: Use octal permissions

[Andy Shevchenko]

Octal permissions are preferred as stated in
Documentation/dev-tools/checkpatch.rst. Replace symbolic permissions
with octal permissions when creating the files.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/hid/hid-cp2112.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 3c6a3be8fc02..adbe8a47cf67 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -175,7 +175,7 @@ struct cp2112_device {
 };
 
 static int gpio_push_pull = CP2112_GPIO_ALL_GPIO_MASK;
-module_param(gpio_push_pull, int, S_IRUGO | S_IWUSR);
+module_param(gpio_push_pull, int, 0644);
 MODULE_PARM_DESC(gpio_push_pull, "GPIO push-pull configuration bitmask");
 
 static int cp2112_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
@@ -1057,7 +1057,7 @@ static void chmod_sysfs_attrs(struct hid_device *hdev)
 	}
 
 	for (attr = cp2112_attr_group.attrs; *attr; ++attr) {
-		umode_t mode = (buf[1] & 1) ? S_IWUSR | S_IRUGO : S_IRUGO;
+		umode_t mode = (buf[1] & 1) ? 0644: 0444;
 		ret = sysfs_chmod_file(&hdev->dev.kobj, *attr, mode);
 		if (ret < 0)
 			hid_err(hdev, "error chmoding sysfs file %s\n",
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v1 00/12] HID: cp2112: Cleanups and refactorings

[Andy Shevchenko]

On Mon, Jul 03, 2023 at 09:52:10PM +0300, Andy Shevchenko wrote:
> After I updated GPIO library for the case Benjamin has with CP2112,
> I have a brief look into the CP2112 driver itself.
> 
> From GPIO perspective it has two main (maitenance) issues:
> - usage of ->to_irq() with IRQ chip present;
> - having IRQ chip not immutable.
> 
> Besides that there are plenty small cleanups here and there.
> Hence this series.

Any comments on this?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 00/12] HID: cp2112: Cleanups and refactorings

[Andy Shevchenko]

On Thu, Jul 27, 2023 at 09:43:29PM +0300, Andy Shevchenko wrote:
> On Mon, Jul 03, 2023 at 09:52:10PM +0300, Andy Shevchenko wrote:
> > After I updated GPIO library for the case Benjamin has with CP2112,
> > I have a brief look into the CP2112 driver itself.
> > 
> > From GPIO perspective it has two main (maitenance) issues:
> > - usage of ->to_irq() with IRQ chip present;
> > - having IRQ chip not immutable.
> > 
> > Besides that there are plenty small cleanups here and there.
> > Hence this series.
> 
> Any comments on this?

Gentle ping^2 for this...

Anything should I do to improve it or is it okay to go as is?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 00/12] HID: cp2112: Cleanups and refactorings

[Jiri Kosina]

On Fri, 4 Aug 2023, Andy Shevchenko wrote:

> On Thu, Jul 27, 2023 at 09:43:29PM +0300, Andy Shevchenko wrote:
> > On Mon, Jul 03, 2023 at 09:52:10PM +0300, Andy Shevchenko wrote:
> > > After I updated GPIO library for the case Benjamin has with CP2112,
> > > I have a brief look into the CP2112 driver itself.
> > > 
> > > From GPIO perspective it has two main (maitenance) issues:
> > > - usage of ->to_irq() with IRQ chip present;
> > > - having IRQ chip not immutable.
> > > 
> > > Besides that there are plenty small cleanups here and there.
> > > Hence this series.
> > 
> > Any comments on this?
> 
> Gentle ping^2 for this...
> 
> Anything should I do to improve it or is it okay to go as is?

I have been off pretty much the whole July. I am now back and slowly 
making my way through everything that accumulated, I will eventually get 
to this.

Thanks for the patience,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v1 00/12] HID: cp2112: Cleanups and refactorings

[Andy Shevchenko]

On Mon, Aug 07, 2023 at 01:19:54PM +0200, Jiri Kosina wrote:
> On Fri, 4 Aug 2023, Andy Shevchenko wrote:
> > On Thu, Jul 27, 2023 at 09:43:29PM +0300, Andy Shevchenko wrote:
> > > On Mon, Jul 03, 2023 at 09:52:10PM +0300, Andy Shevchenko wrote:
> > > > After I updated GPIO library for the case Benjamin has with CP2112,
> > > > I have a brief look into the CP2112 driver itself.
> > > > 
> > > > From GPIO perspective it has two main (maitenance) issues:
> > > > - usage of ->to_irq() with IRQ chip present;
> > > > - having IRQ chip not immutable.
> > > > 
> > > > Besides that there are plenty small cleanups here and there.
> > > > Hence this series.
> > > 
> > > Any comments on this?
> > 
> > Gentle ping^2 for this...
> > 
> > Anything should I do to improve it or is it okay to go as is?
> 
> I have been off pretty much the whole July. I am now back and slowly 
> making my way through everything that accumulated, I will eventually get 
> to this.
> 
> Thanks for the patience,

Ah, okay, no worries and take your time!

I was thinking more on Benjamin's answer as last time he had a hw setup
to test... Not sure what the status of that now and if he has a chance
to test this or busy enough with something else.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 00/12] HID: cp2112: Cleanups and refactorings

[Jiri Kosina]

On Mon, 7 Aug 2023, Andy Shevchenko wrote:

> > > > > After I updated GPIO library for the case Benjamin has with CP2112,
> > > > > I have a brief look into the CP2112 driver itself.
> > > > > 
> > > > > From GPIO perspective it has two main (maitenance) issues:
> > > > > - usage of ->to_irq() with IRQ chip present;
> > > > > - having IRQ chip not immutable.
> > > > > 
> > > > > Besides that there are plenty small cleanups here and there.
> > > > > Hence this series.
> > > > 
> > > > Any comments on this?
> > > 
> > > Gentle ping^2 for this...
> > > 
> > > Anything should I do to improve it or is it okay to go as is?
> > 
> > I have been off pretty much the whole July. I am now back and slowly 
> > making my way through everything that accumulated, I will eventually get 
> > to this.
> > 
> > Thanks for the patience,
> 
> Ah, okay, no worries and take your time!
> 
> I was thinking more on Benjamin's answer as last time he had a hw setup
> to test... Not sure what the status of that now and if he has a chance
> to test this or busy enough with something else.

Ah, that would be of course nice. Benjamin?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v1 00/12] HID: cp2112: Cleanups and refactorings

[Andy Shevchenko]

On Mon, Aug 14, 2023 at 11:28:58AM +0200, Jiri Kosina wrote:
> On Mon, 7 Aug 2023, Andy Shevchenko wrote:

...

> > > > > > After I updated GPIO library for the case Benjamin has with CP2112,
> > > > > > I have a brief look into the CP2112 driver itself.
> > > > > > 
> > > > > > From GPIO perspective it has two main (maitenance) issues:
> > > > > > - usage of ->to_irq() with IRQ chip present;
> > > > > > - having IRQ chip not immutable.
> > > > > > 
> > > > > > Besides that there are plenty small cleanups here and there.
> > > > > > Hence this series.
> > > > > 
> > > > > Any comments on this?
> > > > 
> > > > Gentle ping^2 for this...
> > > > 
> > > > Anything should I do to improve it or is it okay to go as is?
> > > 
> > > I have been off pretty much the whole July. I am now back and slowly 
> > > making my way through everything that accumulated, I will eventually get 
> > > to this.
> > > 
> > > Thanks for the patience,
> > 
> > Ah, okay, no worries and take your time!
> > 
> > I was thinking more on Benjamin's answer as last time he had a hw setup
> > to test... Not sure what the status of that now and if he has a chance
> > to test this or busy enough with something else.
> 
> Ah, that would be of course nice. Benjamin?

Benjamin? It almost full release cycle passed...
I understand if you are busy with something, just tell us.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 00/12] HID: cp2112: Cleanups and refactorings

[Benjamin Tissoires]

On Aug 21 2023, Andy Shevchenko wrote:
> On Mon, Aug 14, 2023 at 11:28:58AM +0200, Jiri Kosina wrote:
> > On Mon, 7 Aug 2023, Andy Shevchenko wrote:
> 
> ...
> 
> > > > > > > After I updated GPIO library for the case Benjamin has with CP2112,
> > > > > > > I have a brief look into the CP2112 driver itself.
> > > > > > > 
> > > > > > > From GPIO perspective it has two main (maitenance) issues:
> > > > > > > - usage of ->to_irq() with IRQ chip present;
> > > > > > > - having IRQ chip not immutable.
> > > > > > > 
> > > > > > > Besides that there are plenty small cleanups here and there.
> > > > > > > Hence this series.
> > > > > > 
> > > > > > Any comments on this?
> > > > > 
> > > > > Gentle ping^2 for this...
> > > > > 
> > > > > Anything should I do to improve it or is it okay to go as is?
> > > > 
> > > > I have been off pretty much the whole July. I am now back and slowly 
> > > > making my way through everything that accumulated, I will eventually get 
> > > > to this.
> > > > 
> > > > Thanks for the patience,
> > > 
> > > Ah, okay, no worries and take your time!
> > > 
> > > I was thinking more on Benjamin's answer as last time he had a hw setup
> > > to test... Not sure what the status of that now and if he has a chance
> > > to test this or busy enough with something else.
> > 
> > Ah, that would be of course nice. Benjamin?
> 
> Benjamin? It almost full release cycle passed...
> I understand if you are busy with something, just tell us.

Sorry for not answering, I was off in August until just now.

I tried you series just before taking time off, but the problem was that
my automation relies on this driver to not be too far from the current
upstream, as I need to patch it to be able to inject a node child in it.

Which is why I was very interested in the ACPI/DT work so that I do not
have to patch the driver.

Long story short, I'm not able to test it right now (and I got quite
some backlog as you can imagine). IIRC the code was fine, so I think we
can just take the series as is, and work on the quirks (if any) later.

Cheers,
Benjamin

Re: [PATCH v1 00/12] HID: cp2112: Cleanups and refactorings

[Andy Shevchenko]

On Mon, Aug 21, 2023 at 10:51:04AM +0200, Benjamin Tissoires wrote:
> On Aug 21 2023, Andy Shevchenko wrote:
> > On Mon, Aug 14, 2023 at 11:28:58AM +0200, Jiri Kosina wrote:
> > > On Mon, 7 Aug 2023, Andy Shevchenko wrote:

...

> > > > > > > > After I updated GPIO library for the case Benjamin has with CP2112,
> > > > > > > > I have a brief look into the CP2112 driver itself.
> > > > > > > > 
> > > > > > > > From GPIO perspective it has two main (maitenance) issues:
> > > > > > > > - usage of ->to_irq() with IRQ chip present;
> > > > > > > > - having IRQ chip not immutable.
> > > > > > > > 
> > > > > > > > Besides that there are plenty small cleanups here and there.
> > > > > > > > Hence this series.
> > > > > > > 
> > > > > > > Any comments on this?
> > > > > > 
> > > > > > Gentle ping^2 for this...
> > > > > > 
> > > > > > Anything should I do to improve it or is it okay to go as is?
> > > > > 
> > > > > I have been off pretty much the whole July. I am now back and slowly 
> > > > > making my way through everything that accumulated, I will eventually get 
> > > > > to this.
> > > > > 
> > > > > Thanks for the patience,
> > > > 
> > > > Ah, okay, no worries and take your time!
> > > > 
> > > > I was thinking more on Benjamin's answer as last time he had a hw setup
> > > > to test... Not sure what the status of that now and if he has a chance
> > > > to test this or busy enough with something else.
> > > 
> > > Ah, that would be of course nice. Benjamin?
> > 
> > Benjamin? It almost full release cycle passed...
> > I understand if you are busy with something, just tell us.
> 
> Sorry for not answering, I was off in August until just now.
> 
> I tried you series just before taking time off, but the problem was that
> my automation relies on this driver to not be too far from the current
> upstream, as I need to patch it to be able to inject a node child in it.
> 
> Which is why I was very interested in the ACPI/DT work so that I do not
> have to patch the driver.
> 
> Long story short, I'm not able to test it right now (and I got quite
> some backlog as you can imagine). IIRC the code was fine, so I think we
> can just take the series as is, and work on the quirks (if any) later.

Thank you!

The thing that might be broken is interrupts handling. If that works,
I'm pretty confident with the rest.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 00/12] HID: cp2112: Cleanups and refactorings

[Andy Shevchenko]

On Mon, Aug 21, 2023 at 12:34:30PM +0300, Andy Shevchenko wrote:
> On Mon, Aug 21, 2023 at 10:51:04AM +0200, Benjamin Tissoires wrote:
> > On Aug 21 2023, Andy Shevchenko wrote:

...

> > Long story short, I'm not able to test it right now (and I got quite
> > some backlog as you can imagine). IIRC the code was fine, so I think we
> > can just take the series as is, and work on the quirks (if any) later.
> 
> Thank you!
> 
> The thing that might be broken is interrupts handling. If that works,
> I'm pretty confident with the rest.

I.o.w. first 5 patches to test is already 98% of guarantee that everything
is fine.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 00/12] HID: cp2112: Cleanups and refactorings

[Benjamin Tissoires]

On Mon, Aug 21, 2023 at 11:35 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Aug 21, 2023 at 12:34:30PM +0300, Andy Shevchenko wrote:
> > On Mon, Aug 21, 2023 at 10:51:04AM +0200, Benjamin Tissoires wrote:
> > > On Aug 21 2023, Andy Shevchenko wrote:
>
> ...
>
> > > Long story short, I'm not able to test it right now (and I got quite
> > > some backlog as you can imagine). IIRC the code was fine, so I think we
> > > can just take the series as is, and work on the quirks (if any) later.
> >
> > Thank you!
> >
> > The thing that might be broken is interrupts handling. If that works,
> > I'm pretty confident with the rest.
>
> I.o.w. first 5 patches to test is already 98% of guarantee that everything
> is fine.

Actually I applied you series locally, and applied Danny's patches on
top, and I could run your series in qemu with the cp2112 as USB
passthrough.

Everything is working fine, so I can take this one just now.

Cheers,
Benjamin

>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v1 00/12] HID: cp2112: Cleanups and refactorings

[Benjamin Tissoires]

On Mon, Aug 21, 2023 at 12:19 PM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Mon, Aug 21, 2023 at 11:35 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Mon, Aug 21, 2023 at 12:34:30PM +0300, Andy Shevchenko wrote:
> > > On Mon, Aug 21, 2023 at 10:51:04AM +0200, Benjamin Tissoires wrote:
> > > > On Aug 21 2023, Andy Shevchenko wrote:
> >
> > ...
> >
> > > > Long story short, I'm not able to test it right now (and I got quite
> > > > some backlog as you can imagine). IIRC the code was fine, so I think we
> > > > can just take the series as is, and work on the quirks (if any) later.
> > >
> > > Thank you!
> > >
> > > The thing that might be broken is interrupts handling. If that works,
> > > I'm pretty confident with the rest.
> >
> > I.o.w. first 5 patches to test is already 98% of guarantee that everything
> > is fine.
>
> Actually I applied you series locally, and applied Danny's patches on
> top, and I could run your series in qemu with the cp2112 as USB
> passthrough.
>
> Everything is working fine, so I can take this one just now.

I've pushed the series to for-6.6/cp2112, but for some reasons, b4
doesn't seem to believe the series is the one you submitted.

Would you mind double checking on your side if everything is good?

https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/log/?h=for-6.6/cp2112

Cheers,
Benjamin

>
> Cheers,
> Benjamin
>
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >

Re: [PATCH v1 00/12] HID: cp2112: Cleanups and refactorings

[Andy Shevchenko]

On Mon, Aug 21, 2023 at 12:19:39PM +0200, Benjamin Tissoires wrote:
> On Mon, Aug 21, 2023 at 11:35 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Aug 21, 2023 at 12:34:30PM +0300, Andy Shevchenko wrote:
> > > On Mon, Aug 21, 2023 at 10:51:04AM +0200, Benjamin Tissoires wrote:
> > > > On Aug 21 2023, Andy Shevchenko wrote:

...

> > > > Long story short, I'm not able to test it right now (and I got quite
> > > > some backlog as you can imagine). IIRC the code was fine, so I think we
> > > > can just take the series as is, and work on the quirks (if any) later.
> > >
> > > Thank you!
> > >
> > > The thing that might be broken is interrupts handling. If that works,
> > > I'm pretty confident with the rest.
> >
> > I.o.w. first 5 patches to test is already 98% of guarantee that everything
> > is fine.
> 
> Actually I applied you series locally, and applied Danny's patches on
> top, and I could run your series in qemu with the cp2112 as USB
> passthrough.
> 
> Everything is working fine, so I can take this one just now.

Thank you! I assume you have some IRQ (like GPIO button) to test with that.
If no, it's easily to describe (in ACPI, see [1]) and use a wire to emulate
the button presses. In that case the /proc/interrupts should show the
different numbers.

[1]: https://github.com/westeri/meta-acpi/blob/master/recipes-bsp/acpi-tables/samples/edison/buttons.asli

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 00/12] HID: cp2112: Cleanups and refactorings

[Andy Shevchenko]

On Mon, Aug 21, 2023 at 12:27:22PM +0200, Benjamin Tissoires wrote:
> On Mon, Aug 21, 2023 at 12:19 PM Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> > On Mon, Aug 21, 2023 at 11:35 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Mon, Aug 21, 2023 at 12:34:30PM +0300, Andy Shevchenko wrote:
> > > > On Mon, Aug 21, 2023 at 10:51:04AM +0200, Benjamin Tissoires wrote:
> > > > > On Aug 21 2023, Andy Shevchenko wrote:

...

> > > > > Long story short, I'm not able to test it right now (and I got quite
> > > > > some backlog as you can imagine). IIRC the code was fine, so I think we
> > > > > can just take the series as is, and work on the quirks (if any) later.
> > > >
> > > > Thank you!
> > > >
> > > > The thing that might be broken is interrupts handling. If that works,
> > > > I'm pretty confident with the rest.
> > >
> > > I.o.w. first 5 patches to test is already 98% of guarantee that everything
> > > is fine.
> >
> > Actually I applied you series locally, and applied Danny's patches on
> > top, and I could run your series in qemu with the cp2112 as USB
> > passthrough.
> >
> > Everything is working fine, so I can take this one just now.
> 
> I've pushed the series to for-6.6/cp2112, but for some reasons, b4
> doesn't seem to believe the series is the one you submitted.
> 
> Would you mind double checking on your side if everything is good?
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/log/?h=for-6.6/cp2112

Everything is fine as far as I can tell.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 00/12] HID: cp2112: Cleanups and refactorings

[Benjamin Tissoires]

On Mon, Aug 21, 2023 at 12:32 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Aug 21, 2023 at 12:19:39PM +0200, Benjamin Tissoires wrote:
> > On Mon, Aug 21, 2023 at 11:35 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Mon, Aug 21, 2023 at 12:34:30PM +0300, Andy Shevchenko wrote:
> > > > On Mon, Aug 21, 2023 at 10:51:04AM +0200, Benjamin Tissoires wrote:
> > > > > On Aug 21 2023, Andy Shevchenko wrote:
>
> ...
>
> > > > > Long story short, I'm not able to test it right now (and I got quite
> > > > > some backlog as you can imagine). IIRC the code was fine, so I think we
> > > > > can just take the series as is, and work on the quirks (if any) later.
> > > >
> > > > Thank you!
> > > >
> > > > The thing that might be broken is interrupts handling. If that works,
> > > > I'm pretty confident with the rest.
> > >
> > > I.o.w. first 5 patches to test is already 98% of guarantee that everything
> > > is fine.
> >
> > Actually I applied you series locally, and applied Danny's patches on
> > top, and I could run your series in qemu with the cp2112 as USB
> > passthrough.
> >
> > Everything is working fine, so I can take this one just now.
>
> Thank you! I assume you have some IRQ (like GPIO button) to test with that.

Yeah, binding a test i2c-hid touchpad on top of hid forces you to use
GPIOs. Otherwise you are polling, and it's not allowed in i2c-hid
anymore IIRC :)

> If no, it's easily to describe (in ACPI, see [1]) and use a wire to emulate
> the button presses. In that case the /proc/interrupts should show the
> different numbers.

Thanks, but again, the GPIO is tested just by checking if the touchpad
can send events when touched.

Now I need to update my CI to rely on danny's patches and a DSDT overwrite :)

Cheers,
Benjamin

>
> [1]: https://github.com/westeri/meta-acpi/blob/master/recipes-bsp/acpi-tables/samples/edison/buttons.asli
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v1 00/12] HID: cp2112: Cleanups and refactorings

[Benjamin Tissoires]

On Aug 21 2023, Andy Shevchenko wrote:
> On Mon, Aug 21, 2023 at 12:27:22PM +0200, Benjamin Tissoires wrote:
> > On Mon, Aug 21, 2023 at 12:19 PM Benjamin Tissoires
> > <benjamin.tissoires@redhat.com> wrote:
> > > On Mon, Aug 21, 2023 at 11:35 AM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Mon, Aug 21, 2023 at 12:34:30PM +0300, Andy Shevchenko wrote:
> > > > > On Mon, Aug 21, 2023 at 10:51:04AM +0200, Benjamin Tissoires wrote:
> > > > > > On Aug 21 2023, Andy Shevchenko wrote:
> 
> ...
> 
> > > > > > Long story short, I'm not able to test it right now (and I got quite
> > > > > > some backlog as you can imagine). IIRC the code was fine, so I think we
> > > > > > can just take the series as is, and work on the quirks (if any) later.
> > > > >
> > > > > Thank you!
> > > > >
> > > > > The thing that might be broken is interrupts handling. If that works,
> > > > > I'm pretty confident with the rest.
> > > >
> > > > I.o.w. first 5 patches to test is already 98% of guarantee that everything
> > > > is fine.
> > >
> > > Actually I applied you series locally, and applied Danny's patches on
> > > top, and I could run your series in qemu with the cp2112 as USB
> > > passthrough.
> > >
> > > Everything is working fine, so I can take this one just now.
> > 
> > I've pushed the series to for-6.6/cp2112, but for some reasons, b4
> > doesn't seem to believe the series is the one you submitted.
> > 
> > Would you mind double checking on your side if everything is good?
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/log/?h=for-6.6/cp2112
> 
> Everything is fine as far as I can tell.

Great, thanks for double checking.

Cheers,
Benjamin

> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH v1 06/12] HID: cp2112: Remove dead code

[Christophe JAILLET]

Le 03/07/2023 à 20:52, Andy Shevchenko a écrit :
> Remove cp2112_allocate_irq() and counterparts that seems to be
> a dead code from day 1. In case somebody needs it, it can be
> retrieved from Git index.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Hi,

for the records, just in case it still makesense to keep this code:

	https://lore.kernel.org/all/CAO-hwJJfncQ3jgtS=HO0atbzrTNOT_rzU66oG2yRTWTSY-L8KA@mail.gmail.com/

CJ

> ---
>   drivers/hid/hid-cp2112.c | 54 ----------------------------------------
>   1 file changed, 54 deletions(-)
> 
> diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
> index 15b626359281..45cd0d2fd3fd 100644
> --- a/drivers/hid/hid-cp2112.c
> +++ b/drivers/hid/hid-cp2112.c
> @@ -17,8 +17,6 @@
>    */
>   
>   #include <linux/bitops.h>
> -#include <linux/gpio/consumer.h>
> -#include <linux/gpio/machine.h>
>   #include <linux/gpio/driver.h>
>   #include <linux/hid.h>
>   #include <linux/hidraw.h>
> @@ -168,7 +166,6 @@ struct cp2112_device {
>   	u8 *in_out_buffer;
>   	struct mutex lock;
>   
> -	struct gpio_desc *desc[8];
>   	bool gpio_poll;
>   	struct delayed_work gpio_poll_worker;
>   	unsigned long irq_mask;
> @@ -1181,51 +1178,6 @@ static int cp2112_gpio_irq_type(struct irq_data *d, unsigned int type)
>   	return 0;
>   }
>   
> -static int __maybe_unused cp2112_allocate_irq(struct cp2112_device *dev,
> -					      int pin)
> -{
> -	int ret;
> -
> -	if (dev->desc[pin])
> -		return -EINVAL;
> -
> -	dev->desc[pin] = gpiochip_request_own_desc(&dev->gc, pin,
> -						   "HID/I2C:Event",
> -						   GPIO_ACTIVE_HIGH,
> -						   GPIOD_IN);
> -	if (IS_ERR(dev->desc[pin])) {
> -		dev_err(dev->gc.parent, "Failed to request GPIO\n");
> -		return PTR_ERR(dev->desc[pin]);
> -	}
> -
> -	ret = cp2112_gpio_direction_input(&dev->gc, pin);
> -	if (ret < 0) {
> -		dev_err(dev->gc.parent, "Failed to set GPIO to input dir\n");
> -		goto err_desc;
> -	}
> -
> -	ret = gpiochip_lock_as_irq(&dev->gc, pin);
> -	if (ret) {
> -		dev_err(dev->gc.parent, "Failed to lock GPIO as interrupt\n");
> -		goto err_desc;
> -	}
> -
> -	ret = gpiod_to_irq(dev->desc[pin]);
> -	if (ret < 0) {
> -		dev_err(dev->gc.parent, "Failed to translate GPIO to IRQ\n");
> -		goto err_lock;
> -	}
> -
> -	return ret;
> -
> -err_lock:
> -	gpiochip_unlock_as_irq(&dev->gc, pin);
> -err_desc:
> -	gpiochip_free_own_desc(dev->desc[pin]);
> -	dev->desc[pin] = NULL;
> -	return ret;
> -}
> -
>   static const struct irq_chip cp2112_gpio_irqchip = {
>   	.name = "cp2112-gpio",
>   	.irq_startup = cp2112_gpio_irq_startup,
> @@ -1390,7 +1342,6 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
>   static void cp2112_remove(struct hid_device *hdev)
>   {
>   	struct cp2112_device *dev = hid_get_drvdata(hdev);
> -	int i;
>   
>   	sysfs_remove_group(&hdev->dev.kobj, &cp2112_attr_group);
>   	i2c_del_adapter(&dev->adap);
> @@ -1400,11 +1351,6 @@ static void cp2112_remove(struct hid_device *hdev)
>   		cancel_delayed_work_sync(&dev->gpio_poll_worker);
>   	}
>   
> -	for (i = 0; i < ARRAY_SIZE(dev->desc); i++) {
> -		gpiochip_unlock_as_irq(&dev->gc, i);
> -		gpiochip_free_own_desc(dev->desc[i]);
> -	}
> -
>   	gpiochip_remove(&dev->gc);
>   	/* i2c_del_adapter has finished removing all i2c devices from our
>   	 * adapter. Well behaved devices should no longer call our cp2112_xfer

Re: [PATCH v1 06/12] HID: cp2112: Remove dead code

[Andy Shevchenko]

On Sat, Aug 26, 2023 at 9:30 PM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
>
> Le 03/07/2023 à 20:52, Andy Shevchenko a écrit :
> > Remove cp2112_allocate_irq() and counterparts that seems to be
> > a dead code from day 1. In case somebody needs it, it can be
> > retrieved from Git index.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> Hi,
>
> for the records, just in case it still makesense to keep this code:
>
>         https://lore.kernel.org/all/CAO-hwJJfncQ3jgtS=HO0atbzrTNOT_rzU66oG2yRTWTSY-L8KA@mail.gmail.com/

It's in the Git index, so we can return it, but the rule of thumb is
that we do not add dead code to the kernel.

-- 
With Best Regards,
Andy Shevchenko