| Message ID | 20201021134931.462560-1-coiby.xu@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | [v3] HID: i2c-hid: add polling mode based on connected GPIO chip's pin status | expand |
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(&irq_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(&client->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, &ihid->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)) { > + dev_warn(&client->dev, > + "Failed to get GPIO Interrupt Assertion Level, could not enable polling mode for %s", > + client->name); > + return -EINVAL; > + } > + > + ihid->polling_thread = kthread_create(i2c_hid_polling_thread, ihid, > + "I2C HID polling thread"); > + > + if (!IS_ERR(ihid->polling_thread)) { > + pr_info("I2C HID polling thread created"); > + wake_up_process(ihid->polling_thread); > + return 0; > + } > + > + return PTR_ERR(ihid->polling_thread); I would personally rewrite this parts as ``` if (IS_ERR(...)) { dev_err(...); return PTR_ERR(...); } .... return 0; ``` > +} > [...] Regards, Barnabás Pőcze
Hi, On Thu, Oct 22, 2020 at 02:22:51PM +0000, 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(&irq_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(&data->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(&client->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, &ihid->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. > According to the doc of kthread_create_on_node, @threadfn() can either call do_exit() directly if it is a * standalone thread for which no one will call kthread_stop(), or * return when 'kthread_should_stop()' is true (which means * kthread_stop() has been called). do_exit is not necessary. Thank you for raising up this issue and looking at other examples for me! > >> + 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)) { >> + dev_warn(&client->dev, >> + "Failed to get GPIO Interrupt Assertion Level, could not enable polling mode for %s", >> + client->name); >> + return -EINVAL; >> + } >> + >> + ihid->polling_thread = kthread_create(i2c_hid_polling_thread, ihid, >> + "I2C HID polling thread"); >> + >> + if (!IS_ERR(ihid->polling_thread)) { >> + pr_info("I2C HID polling thread created"); >> + wake_up_process(ihid->polling_thread); >> + return 0; >> + } >> + >> + return PTR_ERR(ihid->polling_thread); > >I would personally rewrite this parts as > >``` >if (IS_ERR(...)) { > dev_err(...); > return PTR_ERR(...); >} >.... >return 0; >``` Thank you! This style is consistent with other functions in this file. > > >> +} >> [...] > > >Regards, >Barnabás Pőcze -- Best regards, Coiby
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(&irq_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(&data->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(&client->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
On Sun, Nov 22, 2020 at 01:33:01PM +0000, 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(&irq_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(&data->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(&client->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
> [...] > >> >> +static int get_gpio_pin_state(struct irq_desc *irq_desc) > >> >> +{ > >> >> + struct gpio_chip *gc = irq_data_get_irq_chip_data(&irq_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(&data->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
On Mon, Nov 23, 2020 at 04:32:40PM +0000, 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(&irq_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(&data->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
2020. november 25., szerda 11:57 keltezéssel, Coiby Xu írta: > On Mon, Nov 23, 2020 at 04:32:40PM +0000, 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(&irq_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(&data->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
On Wed, Nov 25, 2020 at 12:39:02PM +0000, 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 +0000, 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(&irq_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(&data->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
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 <linux/hid.h> #include <linux/mutex.h> #include <linux/acpi.h> +#include <linux/kthread.h> +#include <linux/gpio/driver.h> #include <linux/of.h> #include <linux/regulator/consumer.h> @@ -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; + bool irq_wake_enabled; struct mutex reset_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(&irq_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(&client->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, &ihid->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); + } + + do_exit(0); + 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)) { + dev_warn(&client->dev, + "Failed to get GPIO Interrupt Assertion Level, could not enable polling mode for %s", + client->name); + return -EINVAL; + } + + ihid->polling_thread = kthread_create(i2c_hid_polling_thread, ihid, + "I2C HID polling thread"); + + if (!IS_ERR(ihid->polling_thread)) { + pr_info("I2C HID polling thread created"); + wake_up_process(ihid->polling_thread); + return 0; + } + + return PTR_ERR(ihid->polling_thread); +} + static int i2c_hid_init_irq(struct i2c_client *client) { struct i2c_hid *ihid = i2c_get_clientdata(client); @@ -1007,6 +1117,15 @@ static void i2c_hid_fwnode_probe(struct i2c_client *client, pdata->post_power_delay_ms = val; } +static void free_irq_or_stop_polling(struct i2c_client *client, + struct i2c_hid *ihid) +{ + if (polling_mode != I2C_HID_POLLING_DISABLED) + kthread_stop(ihid->polling_thread); + else + free_irq(client->irq, ihid); +} + static int i2c_hid_probe(struct i2c_client *client, const struct i2c_device_id *dev_id) { @@ -1102,7 +1221,11 @@ static int i2c_hid_probe(struct i2c_client *client, if (ret < 0) goto err_regulator; - ret = i2c_hid_init_irq(client); + if (polling_mode != I2C_HID_POLLING_DISABLED) + ret = i2c_hid_init_polling(ihid); + else + ret = i2c_hid_init_irq(client); + if (ret < 0) goto err_regulator; @@ -1141,7 +1264,7 @@ static int i2c_hid_probe(struct i2c_client *client, hid_destroy_device(hid); err_irq: - free_irq(client->irq, ihid); + free_irq_or_stop_polling(client, ihid); err_regulator: regulator_bulk_disable(ARRAY_SIZE(ihid->pdata.supplies), @@ -1158,7 +1281,7 @@ static int i2c_hid_remove(struct i2c_client *client) hid = ihid->hid; hid_destroy_device(hid); - free_irq(client->irq, ihid); + free_irq_or_stop_polling(client, ihid); if (ihid->bufsize) i2c_hid_free_buffers(ihid); @@ -1174,7 +1297,7 @@ static void i2c_hid_shutdown(struct i2c_client *client) struct i2c_hid *ihid = i2c_get_clientdata(client); i2c_hid_set_power(client, I2C_HID_PWR_SLEEP); - free_irq(client->irq, ihid); + free_irq_or_stop_polling(client, ihid); } #ifdef CONFIG_PM_SLEEP @@ -1195,15 +1318,16 @@ static int i2c_hid_suspend(struct device *dev) /* Save some power */ i2c_hid_set_power(client, I2C_HID_PWR_SLEEP); - disable_irq(client->irq); + if (polling_mode == I2C_HID_POLLING_DISABLED) + disable_irq(client->irq); - if (device_may_wakeup(&client->dev)) { + if (device_may_wakeup(&client->dev) && polling_mode == I2C_HID_POLLING_DISABLED) { wake_status = enable_irq_wake(client->irq); if (!wake_status) ihid->irq_wake_enabled = true; else hid_warn(hid, "Failed to enable irq wake: %d\n", - wake_status); + wake_status); } else { regulator_bulk_disable(ARRAY_SIZE(ihid->pdata.supplies), ihid->pdata.supplies); @@ -1220,7 +1344,7 @@ static int i2c_hid_resume(struct device *dev) struct hid_device *hid = ihid->hid; int wake_status; - if (!device_may_wakeup(&client->dev)) { + if (!device_may_wakeup(&client->dev) || polling_mode != I2C_HID_POLLING_DISABLED) { ret = regulator_bulk_enable(ARRAY_SIZE(ihid->pdata.supplies), ihid->pdata.supplies); if (ret) @@ -1237,7 +1361,8 @@ static int i2c_hid_resume(struct device *dev) wake_status); } - enable_irq(client->irq); + if (polling_mode == I2C_HID_POLLING_DISABLED) + enable_irq(client->irq); /* Instead of resetting device, simply powers the device on. This * solves "incomplete reports" on Raydium devices 2386:3118 and
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: <stable@vger.kernel.org> Link: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1887190 Signed-off-by: Coiby Xu <coiby.xu@gmail.com> --- drivers/hid/i2c-hid/i2c-hid-core.c | 145 +++++++++++++++++++++++++++-- 1 file changed, 135 insertions(+), 10 deletions(-) -- 2.28.0