Re: [PATCH v3] HID: i2c-hid: add polling mode based on connected GPIO chip's pin status
On Wed, Nov 25, 2020 at 12:39:02PM +, Barnabás Pőcze wrote:
2020. november 25., szerda 11:57 keltezéssel, Coiby Xu írta:
On Mon, Nov 23, 2020 at 04:32:40PM +, Barnabás Pőcze wrote:
>> [...]
>> >> >> +static int get_gpio_pin_state(struct irq_desc *irq_desc)
>> >> >> +{
>> >> >> +struct gpio_chip *gc =
irq_data_get_irq_chip_data(_desc->irq_data);
>> >> >> +
>> >> >> +return gc->get(gc, irq_desc->irq_data.hwirq);
>> >> >> +}
>> >> [...]
>> >> >> +ssize_t status = get_gpio_pin_state(irq_desc);
>> >> >
>> >> >`get_gpio_pin_state()` returns an `int`, so I am not sure why `ssize_t`
is used here.
>> >> >
>> >>
>> >> I used `ssize_t` because I found gpiolib-sysfs.c uses `ssize_t`
>> >>
>> >> // drivers/gpio/gpiolib-sysfs.c
>> >> static ssize_t value_show(struct device *dev,
>> >> struct device_attribute *attr, char *buf)
>> >> {
>> >> struct gpiod_data *data = dev_get_drvdata(dev);
>> >> struct gpio_desc *desc = data->desc;
>> >> ssize_t status;
>> >>
>> >> mutex_lock(>mutex);
>> >>
>> >> status = gpiod_get_value_cansleep(desc);
>> >> ...
>> >> return status;
>> >> }
>> >>
>> >> According to the book Advanced Programming in the UNIX Environment by
>> >> W. Richard Stevens,
>> >> With the 1990 POSIX.1 standard, the primitive system data type
>> >> ssize_t was introduced to provide the signed return value...
>> >>
>> >> So ssize_t is fairly common, for example, the read and write syscall
>> >> return a value of type ssize_t. But I haven't found out why ssize_t is
>> >> better int.
>> >> >
>> >
>> >Sorry if I wasn't clear, what prompted me to ask that question is the
following:
>> >`gc->get()` returns `int`, `get_gpio_pin_state()` returns `int`, yet you
still
>> >save the return value of `get_gpio_pin_state()` into a variable with type
>> >`ssize_t` for no apparent reason. In the example you cited, `ssize_t` is
used
>> >because the show() callback of a sysfs attribute must return `ssize_t`, but
here,
>> >`interrupt_line_active()` returns `bool`, so I don't see any advantage over
a
>> >plain `int`. Anyways, I believe either one is fine, I just found it odd.
>> >
>> I don't understand why "the show() callback of a sysfs attribute
>> must return `ssize_t`" instead of int. Do you think the rationale
>> behind it is the same for this case? If yes, using "ssize_t" for
>> status could be justified.
>> [...]
>
>Because it was decided that way, `ssize_t` is a better choice for that purpose
>than plain `int`. You can see it in include/linux/device.h, that both the
>show() and store() methods must return `ssize_t`.
>
Could you explain why `ssize_t` is a better choice? AFAIU, ssize_t
is used because we can return negative value to indicate an error.
ssize_t: "Signed integer type used for a count of bytes or an error
indication."[1]
And POSIX mandates that the return type of read() and write() be `ssize_t`,
so it makes sense to keep a similar interface in the kernel since show() and
store()
are called as a direct result of the user using the read() and write() system
calls, respectively.
If
we use ssize_t here, it's a reminder that reading a GPIO pin's status
could fail. And ssize_t reminds us it's a operation similar to read
or write. So ssize_t is better than int here. And maybe it's the same
reason why "it was decided that way".
[...]
I believe it's more appropriate to use ssize_t when it's about a "count of
elements",
but the GPIO pin state is a single boolean value (or an error indication), which
is returned as an `int`. Since it's returned as an `int` - I'm arguing that -
there is no reason to use `ssize_t` here. Anyways, both `ssize_t` and `int`
work fine
in this case.
So value_show in gpiolib-sysfs.c is a kind of being forced to use
ssize_t. I'll use int instead to avoid confusion in v4. Thank you for
the explanation!
[1]:
https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_12
Regards,
Barnabás Pőcze
--
Best regards,
Coiby
Re: [PATCH v3] HID: i2c-hid: add polling mode based on connected GPIO chip's pin status
2020. november 25., szerda 11:57 keltezéssel, Coiby Xu írta:
> On Mon, Nov 23, 2020 at 04:32:40PM +, Barnabás Pőcze wrote:
> >> [...]
> >> >> >> +static int get_gpio_pin_state(struct irq_desc *irq_desc)
> >> >> >> +{
> >> >> >> +struct gpio_chip *gc =
> >> >> >> irq_data_get_irq_chip_data(_desc->irq_data);
> >> >> >> +
> >> >> >> +return gc->get(gc, irq_desc->irq_data.hwirq);
> >> >> >> +}
> >> >> [...]
> >> >> >> +ssize_t status = get_gpio_pin_state(irq_desc);
> >> >> >
> >> >> >`get_gpio_pin_state()` returns an `int`, so I am not sure why
> >> >> >`ssize_t` is used here.
> >> >> >
> >> >>
> >> >> I used `ssize_t` because I found gpiolib-sysfs.c uses `ssize_t`
> >> >>
> >> >> // drivers/gpio/gpiolib-sysfs.c
> >> >> static ssize_t value_show(struct device *dev,
> >> >> struct device_attribute *attr, char *buf)
> >> >> {
> >> >> struct gpiod_data *data = dev_get_drvdata(dev);
> >> >> struct gpio_desc *desc = data->desc;
> >> >> ssize_t status;
> >> >>
> >> >> mutex_lock(>mutex);
> >> >>
> >> >> status = gpiod_get_value_cansleep(desc);
> >> >> ...
> >> >> return status;
> >> >> }
> >> >>
> >> >> According to the book Advanced Programming in the UNIX Environment by
> >> >> W. Richard Stevens,
> >> >> With the 1990 POSIX.1 standard, the primitive system data type
> >> >> ssize_t was introduced to provide the signed return value...
> >> >>
> >> >> So ssize_t is fairly common, for example, the read and write syscall
> >> >> return a value of type ssize_t. But I haven't found out why ssize_t is
> >> >> better int.
> >> >> >
> >> >
> >> >Sorry if I wasn't clear, what prompted me to ask that question is the
> >> >following:
> >> >`gc->get()` returns `int`, `get_gpio_pin_state()` returns `int`, yet you
> >> >still
> >> >save the return value of `get_gpio_pin_state()` into a variable with type
> >> >`ssize_t` for no apparent reason. In the example you cited, `ssize_t` is
> >> >used
> >> >because the show() callback of a sysfs attribute must return `ssize_t`,
> >> >but here,
> >> >`interrupt_line_active()` returns `bool`, so I don't see any advantage
> >> >over a
> >> >plain `int`. Anyways, I believe either one is fine, I just found it odd.
> >> >
> >> I don't understand why "the show() callback of a sysfs attribute
> >> must return `ssize_t`" instead of int. Do you think the rationale
> >> behind it is the same for this case? If yes, using "ssize_t" for
> >> status could be justified.
> >> [...]
> >
> >Because it was decided that way, `ssize_t` is a better choice for that
> >purpose
> >than plain `int`. You can see it in include/linux/device.h, that both the
> >show() and store() methods must return `ssize_t`.
> >
>
> Could you explain why `ssize_t` is a better choice? AFAIU, ssize_t
> is used because we can return negative value to indicate an error.
ssize_t: "Signed integer type used for a count of bytes or an error
indication."[1]
And POSIX mandates that the return type of read() and write() be `ssize_t`,
so it makes sense to keep a similar interface in the kernel since show() and
store()
are called as a direct result of the user using the read() and write() system
calls, respectively.
> If
> we use ssize_t here, it's a reminder that reading a GPIO pin's status
> could fail. And ssize_t reminds us it's a operation similar to read
> or write. So ssize_t is better than int here. And maybe it's the same
> reason why "it was decided that way".
> [...]
I believe it's more appropriate to use ssize_t when it's about a "count of
elements",
but the GPIO pin state is a single boolean value (or an error indication), which
is returned as an `int`. Since it's returned as an `int` - I'm arguing that -
there is no reason to use `ssize_t` here. Anyways, both `ssize_t` and `int`
work fine
in this case.
[1]:
https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_12
Regards,
Barnabás Pőcze
Re: [PATCH v3] HID: i2c-hid: add polling mode based on connected GPIO chip's pin status
On Mon, Nov 23, 2020 at 04:32:40PM +, Barnabás Pőcze wrote:
[...]
>> >> +static int get_gpio_pin_state(struct irq_desc *irq_desc)
>> >> +{
>> >> + struct gpio_chip *gc =
irq_data_get_irq_chip_data(_desc->irq_data);
>> >> +
>> >> + return gc->get(gc, irq_desc->irq_data.hwirq);
>> >> +}
>> [...]
>> >> + ssize_t status = get_gpio_pin_state(irq_desc);
>> >
>> >`get_gpio_pin_state()` returns an `int`, so I am not sure why `ssize_t` is
used here.
>> >
>>
>> I used `ssize_t` because I found gpiolib-sysfs.c uses `ssize_t`
>>
>> // drivers/gpio/gpiolib-sysfs.c
>> static ssize_t value_show(struct device *dev,
>>struct device_attribute *attr, char *buf)
>> {
>>struct gpiod_data *data = dev_get_drvdata(dev);
>>struct gpio_desc *desc = data->desc;
>>ssize_t status;
>>
>>mutex_lock(>mutex);
>>
>>status = gpiod_get_value_cansleep(desc);
>> ...
>>return status;
>> }
>>
>> According to the book Advanced Programming in the UNIX Environment by
>> W. Richard Stevens,
>> With the 1990 POSIX.1 standard, the primitive system data type
>> ssize_t was introduced to provide the signed return value...
>>
>> So ssize_t is fairly common, for example, the read and write syscall
>> return a value of type ssize_t. But I haven't found out why ssize_t is
>> better int.
>> >
>
>Sorry if I wasn't clear, what prompted me to ask that question is the
following:
>`gc->get()` returns `int`, `get_gpio_pin_state()` returns `int`, yet you still
>save the return value of `get_gpio_pin_state()` into a variable with type
>`ssize_t` for no apparent reason. In the example you cited, `ssize_t` is used
>because the show() callback of a sysfs attribute must return `ssize_t`, but
here,
>`interrupt_line_active()` returns `bool`, so I don't see any advantage over a
>plain `int`. Anyways, I believe either one is fine, I just found it odd.
>
I don't understand why "the show() callback of a sysfs attribute
must return `ssize_t`" instead of int. Do you think the rationale
behind it is the same for this case? If yes, using "ssize_t" for
status could be justified.
[...]
Because it was decided that way, `ssize_t` is a better choice for that purpose
than plain `int`. You can see it in include/linux/device.h, that both the
show() and store() methods must return `ssize_t`.
Could you explain why `ssize_t` is a better choice? AFAIU, ssize_t
is used because we can return negative value to indicate an error. If
we use ssize_t here, it's a reminder that reading a GPIO pin's status
could fail. And ssize_t reminds us it's a operation similar to read
or write. So ssize_t is better than int here. And maybe it's the same
reason why "it was decided that way".
What I'm arguing here, is that there is no reason to use `ssize_t` in this case.
Because `get_gpio_pin_state()` returns `int`. So when you do
```
ssize_t status = get_gpio_pin_state(...);
```
then the return value of `get_gpio_pin_state()` (which is an `int`), will be
converted to an `ssize_t`, and saved into `status`. I'm arguing that that is
unnecessary and a plain `int` would work perfectly well in this case. Anyways,
both work fine, I just found the unnecessary use of `ssize_t` here odd.
Regards,
Barnabás Pőcze
--
Best regards,
Coiby
Re: [PATCH v3] HID: i2c-hid: add polling mode based on connected GPIO chip's pin status
> [...]
> >> >> +static int get_gpio_pin_state(struct irq_desc *irq_desc)
> >> >> +{
> >> >> + struct gpio_chip *gc =
> >> >> irq_data_get_irq_chip_data(_desc->irq_data);
> >> >> +
> >> >> + return gc->get(gc, irq_desc->irq_data.hwirq);
> >> >> +}
> >> [...]
> >> >> + ssize_t status = get_gpio_pin_state(irq_desc);
> >> >
> >> >`get_gpio_pin_state()` returns an `int`, so I am not sure why `ssize_t`
> >> >is used here.
> >> >
> >>
> >> I used `ssize_t` because I found gpiolib-sysfs.c uses `ssize_t`
> >>
> >> // drivers/gpio/gpiolib-sysfs.c
> >> static ssize_t value_show(struct device *dev,
> >>struct device_attribute *attr, char *buf)
> >> {
> >>struct gpiod_data *data = dev_get_drvdata(dev);
> >>struct gpio_desc *desc = data->desc;
> >>ssize_t status;
> >>
> >>mutex_lock(>mutex);
> >>
> >>status = gpiod_get_value_cansleep(desc);
> >> ...
> >>return status;
> >> }
> >>
> >> According to the book Advanced Programming in the UNIX Environment by
> >> W. Richard Stevens,
> >> With the 1990 POSIX.1 standard, the primitive system data type
> >> ssize_t was introduced to provide the signed return value...
> >>
> >> So ssize_t is fairly common, for example, the read and write syscall
> >> return a value of type ssize_t. But I haven't found out why ssize_t is
> >> better int.
> >> >
> >
> >Sorry if I wasn't clear, what prompted me to ask that question is the
> >following:
> >`gc->get()` returns `int`, `get_gpio_pin_state()` returns `int`, yet you
> >still
> >save the return value of `get_gpio_pin_state()` into a variable with type
> >`ssize_t` for no apparent reason. In the example you cited, `ssize_t` is used
> >because the show() callback of a sysfs attribute must return `ssize_t`, but
> >here,
> >`interrupt_line_active()` returns `bool`, so I don't see any advantage over a
> >plain `int`. Anyways, I believe either one is fine, I just found it odd.
> >
> I don't understand why "the show() callback of a sysfs attribute
> must return `ssize_t`" instead of int. Do you think the rationale
> behind it is the same for this case? If yes, using "ssize_t" for
> status could be justified.
> [...]
Because it was decided that way, `ssize_t` is a better choice for that purpose
than plain `int`. You can see it in include/linux/device.h, that both the
show() and store() methods must return `ssize_t`.
What I'm arguing here, is that there is no reason to use `ssize_t` in this case.
Because `get_gpio_pin_state()` returns `int`. So when you do
```
ssize_t status = get_gpio_pin_state(...);
```
then the return value of `get_gpio_pin_state()` (which is an `int`), will be
converted to an `ssize_t`, and saved into `status`. I'm arguing that that is
unnecessary and a plain `int` would work perfectly well in this case. Anyways,
both work fine, I just found the unnecessary use of `ssize_t` here odd.
Regards,
Barnabás Pőcze
Re: [PATCH v3] HID: i2c-hid: add polling mode based on connected GPIO chip's pin status
On Sun, Nov 22, 2020 at 01:33:01PM +, Barnabás Pőcze wrote:
Hi
2020. november 22., vasárnap 11:15 keltezéssel, Coiby Xu írta:
[...]
>> +static int get_gpio_pin_state(struct irq_desc *irq_desc)
>> +{
>> + struct gpio_chip *gc =
irq_data_get_irq_chip_data(_desc->irq_data);
>> +
>> + return gc->get(gc, irq_desc->irq_data.hwirq);
>> +}
[...]
>> + ssize_t status = get_gpio_pin_state(irq_desc);
>
>`get_gpio_pin_state()` returns an `int`, so I am not sure why `ssize_t` is
used here.
>
I used `ssize_t` because I found gpiolib-sysfs.c uses `ssize_t`
// drivers/gpio/gpiolib-sysfs.c
static ssize_t value_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct gpiod_data *data = dev_get_drvdata(dev);
struct gpio_desc *desc = data->desc;
ssize_t status;
mutex_lock(>mutex);
status = gpiod_get_value_cansleep(desc);
...
return status;
}
According to the book Advanced Programming in the UNIX Environment by
W. Richard Stevens,
With the 1990 POSIX.1 standard, the primitive system data type
ssize_t was introduced to provide the signed return value...
So ssize_t is fairly common, for example, the read and write syscall
return a value of type ssize_t. But I haven't found out why ssize_t is
better int.
>
Sorry if I wasn't clear, what prompted me to ask that question is the following:
`gc->get()` returns `int`, `get_gpio_pin_state()` returns `int`, yet you still
save the return value of `get_gpio_pin_state()` into a variable with type
`ssize_t` for no apparent reason. In the example you cited, `ssize_t` is used
because the show() callback of a sysfs attribute must return `ssize_t`, but
here,
`interrupt_line_active()` returns `bool`, so I don't see any advantage over a
plain `int`. Anyways, I believe either one is fine, I just found it odd.
I don't understand why "the show() callback of a sysfs attribute
must return `ssize_t`" instead of int. Do you think the rationale
behind it is the same for this case? If yes, using "ssize_t" for
status could be justified.
>> +
>> + if (status < 0) {
>> + dev_warn(>dev,
>> + "Failed to get GPIO Interrupt line status for %s",
>> + client->name);
>
>I think it's possible that the kernel message buffer is flooded with these
>messages, which is not optimal in my opinion.
>
Thank you! Replaced with dev_dbg in v4.
[...]
Have you looked at `dev_{warn,dbg,...}_ratelimited()`?
Thank you for pointing me to these functions!
Regards,
Barnabás Pőcze
--
Best regards,
Coiby
Re: [PATCH v3] HID: i2c-hid: add polling mode based on connected GPIO chip's pin status
Hi
2020. november 22., vasárnap 11:15 keltezéssel, Coiby Xu írta:
> [...]
> >> +static int get_gpio_pin_state(struct irq_desc *irq_desc)
> >> +{
> >> + struct gpio_chip *gc = irq_data_get_irq_chip_data(_desc->irq_data);
> >> +
> >> + return gc->get(gc, irq_desc->irq_data.hwirq);
> >> +}
> [...]
> >> + ssize_t status = get_gpio_pin_state(irq_desc);
> >
> >`get_gpio_pin_state()` returns an `int`, so I am not sure why `ssize_t` is
> >used here.
> >
>
> I used `ssize_t` because I found gpiolib-sysfs.c uses `ssize_t`
>
> // drivers/gpio/gpiolib-sysfs.c
> static ssize_t value_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> struct gpiod_data *data = dev_get_drvdata(dev);
> struct gpio_desc *desc = data->desc;
> ssize_t status;
>
> mutex_lock(>mutex);
>
> status = gpiod_get_value_cansleep(desc);
> ...
> return status;
> }
>
> According to the book Advanced Programming in the UNIX Environment by
> W. Richard Stevens,
> With the 1990 POSIX.1 standard, the primitive system data type
> ssize_t was introduced to provide the signed return value...
>
> So ssize_t is fairly common, for example, the read and write syscall
> return a value of type ssize_t. But I haven't found out why ssize_t is
> better int.
> >
Sorry if I wasn't clear, what prompted me to ask that question is the following:
`gc->get()` returns `int`, `get_gpio_pin_state()` returns `int`, yet you still
save the return value of `get_gpio_pin_state()` into a variable with type
`ssize_t` for no apparent reason. In the example you cited, `ssize_t` is used
because the show() callback of a sysfs attribute must return `ssize_t`, but
here,
`interrupt_line_active()` returns `bool`, so I don't see any advantage over a
plain `int`. Anyways, I believe either one is fine, I just found it odd.
> >> +
> >> + if (status < 0) {
> >> + dev_warn(>dev,
> >> + "Failed to get GPIO Interrupt line status for %s",
> >> + client->name);
> >
> >I think it's possible that the kernel message buffer is flooded with these
> >messages, which is not optimal in my opinion.
> >
> Thank you! Replaced with dev_dbg in v4.
> [...]
Have you looked at `dev_{warn,dbg,...}_ratelimited()`?
Regards,
Barnabás Pőcze
Re: [PATCH v3] HID: i2c-hid: add polling mode based on connected GPIO chip's pin status
Hi,
On Thu, Oct 22, 2020 at 02:22:51PM +, Barnabás Pőcze wrote:
Hi,
I think this looks a lot better than the first version, the issues around
suspend/resume are sorted out as far as I can see. However, I still have a
couple
comments, mainly minor ones.
Thank you for reviewing this patch!
[...]
+/* polling mode */
+#define I2C_HID_POLLING_DISABLED 0
+#define I2C_HID_POLLING_GPIO_PIN 1
+#define I2C_HID_POLLING_INTERVAL_ACTIVE_US 4000
+#define I2C_HID_POLLING_INTERVAL_IDLE_MS 10
+
+static u8 polling_mode;
+module_param(polling_mode, byte, 0444);
+MODULE_PARM_DESC(polling_mode, "How to poll - 0 disabled; 1 based on GPIO pin's
status");
+
Minor thing, but maybe the default value should be documented in the parameter
description?
+static unsigned int polling_interval_active_us =
I2C_HID_POLLING_INTERVAL_ACTIVE_US;
+module_param(polling_interval_active_us, uint, 0644);
+MODULE_PARM_DESC(polling_interval_active_us,
+"Poll every {polling_interval_active_us} us when the touchpad is
active. Default to 4000 us");
+
+static unsigned int polling_interval_idle_ms =
I2C_HID_POLLING_INTERVAL_IDLE_MS;
Since these two parameters are mostly read, I think the `__read_mostly`
attribute (linux/cache.h) is justified here.
+module_param(polling_interval_idle_ms, uint, 0644);
+MODULE_PARM_DESC(polling_interval_idle_ms,
+"Poll every {polling_interval_idle_ms} ms when the touchpad is
idle. Default to 10 ms");
This is minor stylistic thing; as far as I see, the prevalent pattern is to put
the default value at the end, in parenthesis:
E.g. "some parameter description (default=X)" or "... (default: X)" or
something similar
Maybe __stringify() (linux/stringify.h) could be used here and for the previous
module parameter?
E.g. "... (default=" __stringify(I2C_HID_POLLING_INTERVAL_IDLE_MS) ")"
Thank you for the above three suggestions! Will be applied in v4.
[...]
+static int get_gpio_pin_state(struct irq_desc *irq_desc)
+{
+ struct gpio_chip *gc = irq_data_get_irq_chip_data(_desc->irq_data);
+
+ return gc->get(gc, irq_desc->irq_data.hwirq);
+}
+
+static bool interrupt_line_active(struct i2c_client *client)
+{
+ unsigned long trigger_type = irq_get_trigger_type(client->irq);
Can the trigger type change? Because if not, then I think it'd be better to
store
the value somewhere and not query it every time.
The irq trigger type is obtained from ACPI so I don't think it won't
change.
+ struct irq_desc *irq_desc = irq_to_desc(client->irq);
Same here.
Thank you for the reminding!
+ ssize_t status = get_gpio_pin_state(irq_desc);
`get_gpio_pin_state()` returns an `int`, so I am not sure why `ssize_t` is used
here.
I used `ssize_t` because I found gpiolib-sysfs.c uses `ssize_t`
// drivers/gpio/gpiolib-sysfs.c
static ssize_t value_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct gpiod_data *data = dev_get_drvdata(dev);
struct gpio_desc *desc = data->desc;
ssize_t status;
mutex_lock(>mutex);
status = gpiod_get_value_cansleep(desc);
...
return status;
}
According to the book Advanced Programming in the UNIX Environment by
W. Richard Stevens,
With the 1990 POSIX.1 standard, the primitive system data type
ssize_t was introduced to provide the signed return value...
So ssize_t is fairly common, for example, the read and write syscall
return a value of type ssize_t. But I haven't found out why ssize_t is
better int.
+
+ if (status < 0) {
+ dev_warn(>dev,
+"Failed to get GPIO Interrupt line status for %s",
+client->name);
I think it's possible that the kernel message buffer is flooded with these
messages, which is not optimal in my opinion.
Thank you! Replaced with dev_dbg in v4.
+ return false;
+ }
+ /*
+* According to Windows Precsiontion Touchpad's specs
+*
https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-precision-touchpad-device-bus-connectivity,
+* GPIO Interrupt Assertion Leve could be either ActiveLow or
+* ActiveHigh.
+*/
+ if (trigger_type & IRQF_TRIGGER_LOW)
+ return !status;
+
+ return status;
+}
+
+static int i2c_hid_polling_thread(void *i2c_hid)
+{
+ struct i2c_hid *ihid = i2c_hid;
+ struct i2c_client *client = ihid->client;
+ unsigned int polling_interval_idle;
+
+ while (1) {
+ if (kthread_should_stop())
+ break;
I think this should be `while (!kthread_should_stop())`.
This simplifies the code. Thank you!
+
+ while (interrupt_line_active(client) &&
+ !test_bit(I2C_HID_READ_PENDING, >flags) &&
+ !kthread_should_stop()) {
+
Re: [PATCH v3] HID: i2c-hid: add polling mode based on connected GPIO chip's pin status
Hi,
I think this looks a lot better than the first version, the issues around
suspend/resume are sorted out as far as I can see. However, I still have a
couple
comments, mainly minor ones.
> [...]
> +/* polling mode */
> +#define I2C_HID_POLLING_DISABLED 0
> +#define I2C_HID_POLLING_GPIO_PIN 1
> +#define I2C_HID_POLLING_INTERVAL_ACTIVE_US 4000
> +#define I2C_HID_POLLING_INTERVAL_IDLE_MS 10
> +
> +static u8 polling_mode;
> +module_param(polling_mode, byte, 0444);
> +MODULE_PARM_DESC(polling_mode, "How to poll - 0 disabled; 1 based on GPIO
> pin's status");
> +
Minor thing, but maybe the default value should be documented in the parameter
description?
> +static unsigned int polling_interval_active_us =
> I2C_HID_POLLING_INTERVAL_ACTIVE_US;
> +module_param(polling_interval_active_us, uint, 0644);
> +MODULE_PARM_DESC(polling_interval_active_us,
> + "Poll every {polling_interval_active_us} us when the touchpad
> is active. Default to 4000 us");
> +
> +static unsigned int polling_interval_idle_ms =
> I2C_HID_POLLING_INTERVAL_IDLE_MS;
Since these two parameters are mostly read, I think the `__read_mostly`
attribute (linux/cache.h) is justified here.
> +module_param(polling_interval_idle_ms, uint, 0644);
> +MODULE_PARM_DESC(polling_interval_idle_ms,
> + "Poll every {polling_interval_idle_ms} ms when the touchpad is
> idle. Default to 10 ms");
This is minor stylistic thing; as far as I see, the prevalent pattern is to put
the default value at the end, in parenthesis:
E.g. "some parameter description (default=X)" or "... (default: X)" or
something similar
Maybe __stringify() (linux/stringify.h) could be used here and for the previous
module parameter?
E.g. "... (default=" __stringify(I2C_HID_POLLING_INTERVAL_IDLE_MS) ")"
> [...]
> +static int get_gpio_pin_state(struct irq_desc *irq_desc)
> +{
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(_desc->irq_data);
> +
> + return gc->get(gc, irq_desc->irq_data.hwirq);
> +}
> +
> +static bool interrupt_line_active(struct i2c_client *client)
> +{
> + unsigned long trigger_type = irq_get_trigger_type(client->irq);
Can the trigger type change? Because if not, then I think it'd be better to
store
the value somewhere and not query it every time.
> + struct irq_desc *irq_desc = irq_to_desc(client->irq);
Same here.
> + ssize_t status = get_gpio_pin_state(irq_desc);
`get_gpio_pin_state()` returns an `int`, so I am not sure why `ssize_t` is used
here.
> +
> + if (status < 0) {
> + dev_warn(>dev,
> + "Failed to get GPIO Interrupt line status for %s",
> + client->name);
I think it's possible that the kernel message buffer is flooded with these
messages, which is not optimal in my opinion.
> + return false;
> + }
> + /*
> + * According to Windows Precsiontion Touchpad's specs
> + *
> https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-precision-touchpad-device-bus-connectivity,
> + * GPIO Interrupt Assertion Leve could be either ActiveLow or
> + * ActiveHigh.
> + */
> + if (trigger_type & IRQF_TRIGGER_LOW)
> + return !status;
> +
> + return status;
> +}
> +
> +static int i2c_hid_polling_thread(void *i2c_hid)
> +{
> + struct i2c_hid *ihid = i2c_hid;
> + struct i2c_client *client = ihid->client;
> + unsigned int polling_interval_idle;
> +
> + while (1) {
> + if (kthread_should_stop())
> + break;
I think this should be `while (!kthread_should_stop())`.
> +
> + while (interrupt_line_active(client) &&
> +!test_bit(I2C_HID_READ_PENDING, >flags) &&
> +!kthread_should_stop()) {
> + i2c_hid_get_input(ihid);
> + usleep_range(polling_interval_active_us,
> + polling_interval_active_us + 100);
> + }
> + /*
> + * re-calculate polling_interval_idle
> + * so the module parameters polling_interval_idle_ms can be
> + * changed dynamically through sysfs as
> polling_interval_active_us
> + */
> + polling_interval_idle = polling_interval_idle_ms * 1000;
> + usleep_range(polling_interval_idle,
> + polling_interval_idle + 1000);
I don't quite understand why you use an extra variable here. I'm assuming
you want to "save" a multiplication? I believe the compiler will optimize it
to a single read, and single multiplication regardless whether you use a
"temporary"
variable or not.
> + }
> +
> + do_exit(0);
Looking at other examples, I don't think `do_exit()` is necessary.
> + return 0;
> +}
> +
> +static int i2c_hid_init_polling(struct i2c_hid *ihid)
> +{
> + struct i2c_client *client = ihid->client;
> +
> + if (!irq_get_trigger_type(client->irq)) {
[PATCH v3] HID: i2c-hid: add polling mode based on connected GPIO chip's pin status
For a broken touchpad, it may take several months or longer to be fixed.
Polling mode could be a fallback solution for enthusiastic Linux users
when they have a new laptop. It also acts like a debugging feature. If
polling mode works for a broken touchpad, we can almost be certain
the root cause is related to the interrupt or power setting.
When polling mode is enabled, an I2C device can't wake up the suspended
system since enable/disable_irq_wake is invalid for polling mode.
Three module parameters are added to i2c-hid,
- polling_mode: by default set to 0, i.e., polling is disabled
- polling_interval_idle_ms: the polling internal when the touchpad
is idle, default to 10ms
- polling_interval_active_us: the polling internal when the touchpad
is active, default to 4000us
User can change the last two runtime polling parameter by writing to
/sys/module/i2c_hid/parameters/polling_interval_{idle_ms,active_us}.
Cc:
Link: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1887190
Signed-off-by: Coiby Xu
---
drivers/hid/i2c-hid/i2c-hid-core.c | 145 +++--
1 file changed, 135 insertions(+), 10 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c
b/drivers/hid/i2c-hid/i2c-hid-core.c
index 786e3e9af1c9..e44237562068 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -36,6 +36,8 @@
#include
#include
#include
+#include
+#include
#include
#include
@@ -60,6 +62,25 @@
#define I2C_HID_PWR_ON 0x00
#define I2C_HID_PWR_SLEEP 0x01
+/* polling mode */
+#define I2C_HID_POLLING_DISABLED 0
+#define I2C_HID_POLLING_GPIO_PIN 1
+#define I2C_HID_POLLING_INTERVAL_ACTIVE_US 4000
+#define I2C_HID_POLLING_INTERVAL_IDLE_MS 10
+
+static u8 polling_mode;
+module_param(polling_mode, byte, 0444);
+MODULE_PARM_DESC(polling_mode, "How to poll - 0 disabled; 1 based on GPIO
pin's status");
+
+static unsigned int polling_interval_active_us =
I2C_HID_POLLING_INTERVAL_ACTIVE_US;
+module_param(polling_interval_active_us, uint, 0644);
+MODULE_PARM_DESC(polling_interval_active_us,
+"Poll every {polling_interval_active_us} us when the touchpad
is active. Default to 4000 us");
+
+static unsigned int polling_interval_idle_ms =
I2C_HID_POLLING_INTERVAL_IDLE_MS;
+module_param(polling_interval_idle_ms, uint, 0644);
+MODULE_PARM_DESC(polling_interval_idle_ms,
+"Poll every {polling_interval_idle_ms} ms when the touchpad is
idle. Default to 10 ms");
/* debug option */
static bool debug;
module_param(debug, bool, 0444);
@@ -158,6 +179,8 @@ struct i2c_hid {
struct i2c_hid_platform_data pdata;
+ struct task_struct *polling_thread;
+
boolirq_wake_enabled;
struct mutexreset_lock;
};
@@ -772,7 +795,9 @@ static int i2c_hid_start(struct hid_device *hid)
i2c_hid_free_buffers(ihid);
ret = i2c_hid_alloc_buffers(ihid, bufsize);
- enable_irq(client->irq);
+
+ if (polling_mode == I2C_HID_POLLING_DISABLED)
+ enable_irq(client->irq);
if (ret)
return ret;
@@ -814,6 +839,91 @@ struct hid_ll_driver i2c_hid_ll_driver = {
};
EXPORT_SYMBOL_GPL(i2c_hid_ll_driver);
+static int get_gpio_pin_state(struct irq_desc *irq_desc)
+{
+ struct gpio_chip *gc = irq_data_get_irq_chip_data(_desc->irq_data);
+
+ return gc->get(gc, irq_desc->irq_data.hwirq);
+}
+
+static bool interrupt_line_active(struct i2c_client *client)
+{
+ unsigned long trigger_type = irq_get_trigger_type(client->irq);
+ struct irq_desc *irq_desc = irq_to_desc(client->irq);
+ ssize_t status = get_gpio_pin_state(irq_desc);
+
+ if (status < 0) {
+ dev_warn(>dev,
+"Failed to get GPIO Interrupt line status for %s",
+client->name);
+ return false;
+ }
+ /*
+* According to Windows Precsiontion Touchpad's specs
+*
https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/windows-precision-touchpad-device-bus-connectivity,
+* GPIO Interrupt Assertion Leve could be either ActiveLow or
+* ActiveHigh.
+*/
+ if (trigger_type & IRQF_TRIGGER_LOW)
+ return !status;
+
+ return status;
+}
+
+static int i2c_hid_polling_thread(void *i2c_hid)
+{
+ struct i2c_hid *ihid = i2c_hid;
+ struct i2c_client *client = ihid->client;
+ unsigned int polling_interval_idle;
+
+ while (1) {
+ if (kthread_should_stop())
+ break;
+
+ while (interrupt_line_active(client) &&
+ !test_bit(I2C_HID_READ_PENDING, >flags) &&
+ !kthread_should_stop()) {
+ i2c_hid_get_input(ihid);
+ usleep_range(polling_interval_active_us,
+
