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
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 <[email protected]>
---
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
It's cleaner to use for_each_set_bit() than open coding it.
Signed-off-by: Andy Shevchenko <[email protected]>
---
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
Use str_write_read() and str_read_write() from string_choices.h.
Signed-off-by: Andy Shevchenko <[email protected]>
---
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
Instead of custom wrapper, use DEVICE_tATTR_RW() directly.
Signed-off-by: Andy Shevchenko <[email protected]>
---
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
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 <[email protected]>
---
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
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
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
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
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
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
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
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
> > <[email protected]> wrote:
> > > On Mon, Aug 21, 2023 at 11:35 AM Andy Shevchenko
> > > <[email protected]> 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
>
>