Re: [PATCH v4] HID: i2c-hid: add polling mode based on connected GPIO chip's pin status

[Coiby Xu <coiby.xu@gmail.com>]

Hi Greg and Barnabás,

On Wed, Dec 09, 2020 at 04:44:35PM +0100, Greg KH wrote:
>On Wed, Dec 09, 2020 at 03:38:11PM +0000, Barnabás Pőcze wrote:
>> 2020. december 9., szerda 8:00 keltezéssel, Greg KH írta:
>>
>> > On Tue, Dec 08, 2020 at 09:59:20PM +0000, Barnabás Pőcze wrote:
>> >
>> > > 2020.  november 25., szerda 16:07 keltezéssel, Greg KH írta:
>> > >
>> > > > [...]
>> > > >
>> > > > > +static u8 polling_mode;
>> > > > > +module_param(polling_mode, byte, 0444);
>> > > > > +MODULE_PARM_DESC(polling_mode, "How to poll (default=0) - 0 disabled; 1 based on GPIO pin's status");
>> > > >
>> > > > Module parameters are for the 1990's, they are global and horrible to
>> > > > try to work with. You should provide something on a per-device basis,
>> > > > as what happens if your system requires different things here for
>> > > > different devices? You set this for all devices :(
>> > > > [...]

Thank you for pointing out that.

>> > >
>> > > Hi
>> > > do you think something like what the usbcore has would be better?
>> > > A module parameter like "quirks=<vendor-id>:<product-id>:<flags>[,<vendor-id>:<product-id>:<flags>]*"?
>> >
>> > Not really, that's just for debugging, and asking users to test
>> > something, not for a final solution to anything.
>>

This patch is not only for debugging. The primary reason is as a
fallback solution to save the touchpad. The mentioned touchpads will
be fixed by Linux 5.11 which means the enthusiastic Linux users have to
wait for ~10 months to get their touchpads fixed.

>> My understanding is that this polling mode option is by no means intended
>> as a final solution, it's purely for debugging/fallback:
>>
>> "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."
>>
>> What would you suggest instead of the module parameter?
>
>a debugfs file?  That means it's only for debugging and you have to be
>root to change the value.
>

Thank you for the helpful discussion and the suggestions!

If we can answer the following two questions, it could help decide
what's the next move.

1. How many machines have two or more I2C HID devices?

For the laptops this patch try to fix, they only have one I2C HID
device, i.e., the touchpad. If it's the case with most machines
running Linux, then we don't need to support per-HID-I2C-device
setting and module parameter is the most user-friendly since the user
doesn't even need to know the <vendor-id>/<product-id> pair.

2. How many I2C HID devices like touchpads could be saved by this
patch in the future?

If polling could potentially save lots of I2C hid devices, we are more
motivated to make it easier to use.


--
Best regards,
Coiby