| Message ID | 20211128190031.405620-5-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [1/5] platform/x86: wmi: Replace read_takes_no_args with a flags field | expand |
On Sun, Nov 28, 2021 at 08:00:31PM +0100, Hans de Goede wrote: > On the back of the device there is a hall sensor connected to the > "INT33FF:02" GPIO controller pin 18, which gets triggered when the > device is fully folded into tablet-mode (when the back of the display > touched the back of the keyboard). > > Use this to disable both the touch-keyboard and the digitizer when > the tablet is fully folded into tablet-mode. Wonderful. I knew that this device would still bring surprises...I didn't find this sensor in my investigation :) In your opinion, is it possible to use this sensor to correct of the custom hinge angle sensor readings from the Intel sensor hub? The hinge angle sensor uses accelerometers to calculate the angle and cannot reliably distinquish between 0 and 360 degrees. I used the lid switch status in a my patch for iio-sensors-proxy but this sensor looks like a yet another candidate. The lid switch is more generic soultion, of course. > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/platform/x86/lenovo-yogabook-wmi.c | 71 +++++++++++++++++++++- > 1 file changed, 70 insertions(+), 1 deletion(-) > > diff --git a/drivers/platform/x86/lenovo-yogabook-wmi.c b/drivers/platform/x86/lenovo-yogabook-wmi.c > index ecd624d9108f..65dd1ed97762 100644 > --- a/drivers/platform/x86/lenovo-yogabook-wmi.c > +++ b/drivers/platform/x86/lenovo-yogabook-wmi.c > @@ -3,6 +3,9 @@ > > #include <linux/acpi.h> > #include <linux/devm-helpers.h> > +#include <linux/gpio/consumer.h> > +#include <linux/gpio/machine.h> > +#include <linux/interrupt.h> > #include <linux/module.h> > #include <linux/leds.h> > #include <linux/wmi.h> > @@ -22,6 +25,7 @@ enum { > YB_KBD_IS_ON, > YB_DIGITIZER_IS_ON, > YB_DIGITIZER_MODE, > + YB_TABLET_MODE, > YB_SUSPENDED, > }; > > @@ -31,6 +35,8 @@ struct yogabook_wmi { > struct acpi_device *dig_adev; > struct device *kbd_dev; > struct device *dig_dev; > + struct gpio_desc *backside_hall_gpio; > + int backside_hall_irq; > struct work_struct work; > struct led_classdev kbd_bl_led; > unsigned long flags; > @@ -109,7 +115,10 @@ static void yogabook_wmi_work(struct work_struct *work) > if (test_bit(YB_SUSPENDED, &data->flags)) > return; > > - if (test_bit(YB_DIGITIZER_MODE, &data->flags)) { > + if (test_bit(YB_TABLET_MODE, &data->flags)) { > + kbd_on = false; > + digitizer_on = false; > + } else if (test_bit(YB_DIGITIZER_MODE, &data->flags)) { > digitizer_on = true; > kbd_on = false; > } else { > @@ -171,6 +180,20 @@ static void yogabook_wmi_notify(struct wmi_device *wdev, union acpi_object *dumm > schedule_work(&data->work); > } > > +static irqreturn_t yogabook_backside_hall_irq(int irq, void *_data) > +{ > + struct yogabook_wmi *data = _data; > + > + if (gpiod_get_value(data->backside_hall_gpio)) > + set_bit(YB_TABLET_MODE, &data->flags); > + else > + clear_bit(YB_TABLET_MODE, &data->flags); > + > + schedule_work(&data->work); > + > + return IRQ_HANDLED; > +} > + > static enum led_brightness kbd_brightness_get(struct led_classdev *cdev) > { > struct yogabook_wmi *data = > @@ -197,6 +220,19 @@ static int kbd_brightness_set(struct led_classdev *cdev, > return yogabook_wmi_set_kbd_backlight(wdev, data->brightness); > } > > +static struct gpiod_lookup_table yogabook_wmi_gpios = { > + .dev_id = "243FEC1D-1963-41C1-8100-06A9D82A94B4", > + .table = { > + GPIO_LOOKUP("INT33FF:02", 18, "backside_hall_sw", GPIO_ACTIVE_LOW), > + {} > + }, > +}; > + > +static void yogabook_wmi_rm_gpio_lookup(void *unused) > +{ > + gpiod_remove_lookup_table(&yogabook_wmi_gpios); > +} > + > static int yogabook_wmi_probe(struct wmi_device *wdev, const void *context) > { > struct yogabook_wmi *data; > @@ -242,6 +278,36 @@ static int yogabook_wmi_probe(struct wmi_device *wdev, const void *context) > goto error_put_devs; > } > > + gpiod_add_lookup_table(&yogabook_wmi_gpios); > + > + r = devm_add_action_or_reset(&wdev->dev, yogabook_wmi_rm_gpio_lookup, NULL); > + if (r) > + goto error_put_devs; > + > + data->backside_hall_gpio = > + devm_gpiod_get(&wdev->dev, "backside_hall_sw", GPIOD_IN); > + if (IS_ERR(data->backside_hall_gpio)) { > + r = PTR_ERR(data->backside_hall_gpio); > + dev_err_probe(&wdev->dev, r, "Getting backside_hall_sw GPIO\n"); > + goto error_put_devs; > + } > + > + r = gpiod_to_irq(data->backside_hall_gpio); > + if (r < 0) { > + dev_err_probe(&wdev->dev, r, "Getting backside_hall_sw IRQ\n"); > + goto error_put_devs; > + } > + data->backside_hall_irq = r; > + > + r = devm_request_irq(&wdev->dev, data->backside_hall_irq, > + yogabook_backside_hall_irq, > + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING, > + "backside_hall_sw", data); > + if (r) { > + dev_err_probe(&wdev->dev, r, "Requesting backside_hall_sw IRQ\n"); > + goto error_put_devs; > + } > + > schedule_work(&data->work); > > data->kbd_bl_led.name = "ybwmi::kbd_backlight"; > @@ -307,6 +373,9 @@ __maybe_unused int yogabook_wmi_resume(struct device *dev) > > clear_bit(YB_SUSPENDED, &data->flags); > > + /* Check for YB_TABLET_MODE changes made during suspend */ > + schedule_work(&data->work); > + > return 0; > } > > -- > 2.33.1 >
Hi, On 11/28/21 22:09, Yauhen Kharuzhy wrote: > On Sun, Nov 28, 2021 at 08:00:31PM +0100, Hans de Goede wrote: >> On the back of the device there is a hall sensor connected to the >> "INT33FF:02" GPIO controller pin 18, which gets triggered when the >> device is fully folded into tablet-mode (when the back of the display >> touched the back of the keyboard). >> >> Use this to disable both the touch-keyboard and the digitizer when >> the tablet is fully folded into tablet-mode. > > Wonderful. I knew that this device would still bring surprises...I > didn't find this sensor in my investigation :) > > In your opinion, is it possible to use this sensor to correct of the custom > hinge angle sensor readings from the Intel sensor hub? The hinge angle sensor > uses accelerometers to calculate the angle and cannot reliably > distinquish between 0 and 360 degrees. I used the lid switch status in a my > patch for iio-sensors-proxy but this sensor looks like a yet another candidate. > The lid switch is more generic soultion, of course. Right, there are quite a few yoga / 360° hinges devices which have 2 accelerometers, one in each half. So any solutions in iio-sensors-proxy really should be generic. I did think about this before, because I'm interested in the iio-sensor-proxy solution for this in general and my thoughts on this are that it is fine to make iio-sensor-proxy send SW_TABLET_MODE=1 for any angle below 5° (and above 185°). The idea being that if the lid is almost closed in laptop mode then the device is not being interacted with anyways and then SW_TABLET_MODE=1 does not matter. In practice what SW_TABLET_MODE=1 does is: 1. Enable auto-rotation of the display, does not matter if the display is almost fully closed, since clearly the user is not looking at it. 2. Enable automatic pop-up of on-screen-keyboard when selecting text input-fields, again this does not matter since with the lid almost closed the user is not likely to select any text-input fields. 3. Suppress *built-in* keyboard and touchpad events to avoid accidental pressed on the keyboard / touchpad to register, and again with the lid almost fully closed the user is unlikely to actually want to use the touchpad/keyboard so the false-positive SW_TABLET_MODE=1 reporting for angles below 5° should not be an issue. Using the lid switch is an interesting idea. But I believe that the angle accuracy won't be all that great, so we would need to check the lid-switch for any angle below say 5° then and in many cases the lid-switch will not report 1 until the lid is very close to being fully closed, so then their will still be a window where we do a false-positive reporting of SW_TABLET_MODE=1 . At which point we might just as well live with the false-positive reporting of SW_TABLET_MODE=1 and not bother with the lid-switch. At least that is my 2 cents. Feel free to copy and paste parts of this email to a big comment in the code explaining why angles below 5° report SW_TABLET_MODE=1, assuming you implement that idea. Regards, Hans > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/platform/x86/lenovo-yogabook-wmi.c | 71 +++++++++++++++++++++- >> 1 file changed, 70 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/platform/x86/lenovo-yogabook-wmi.c b/drivers/platform/x86/lenovo-yogabook-wmi.c >> index ecd624d9108f..65dd1ed97762 100644 >> --- a/drivers/platform/x86/lenovo-yogabook-wmi.c >> +++ b/drivers/platform/x86/lenovo-yogabook-wmi.c >> @@ -3,6 +3,9 @@ >> >> #include <linux/acpi.h> >> #include <linux/devm-helpers.h> >> +#include <linux/gpio/consumer.h> >> +#include <linux/gpio/machine.h> >> +#include <linux/interrupt.h> >> #include <linux/module.h> >> #include <linux/leds.h> >> #include <linux/wmi.h> >> @@ -22,6 +25,7 @@ enum { >> YB_KBD_IS_ON, >> YB_DIGITIZER_IS_ON, >> YB_DIGITIZER_MODE, >> + YB_TABLET_MODE, >> YB_SUSPENDED, >> }; >> >> @@ -31,6 +35,8 @@ struct yogabook_wmi { >> struct acpi_device *dig_adev; >> struct device *kbd_dev; >> struct device *dig_dev; >> + struct gpio_desc *backside_hall_gpio; >> + int backside_hall_irq; >> struct work_struct work; >> struct led_classdev kbd_bl_led; >> unsigned long flags; >> @@ -109,7 +115,10 @@ static void yogabook_wmi_work(struct work_struct *work) >> if (test_bit(YB_SUSPENDED, &data->flags)) >> return; >> >> - if (test_bit(YB_DIGITIZER_MODE, &data->flags)) { >> + if (test_bit(YB_TABLET_MODE, &data->flags)) { >> + kbd_on = false; >> + digitizer_on = false; >> + } else if (test_bit(YB_DIGITIZER_MODE, &data->flags)) { >> digitizer_on = true; >> kbd_on = false; >> } else { >> @@ -171,6 +180,20 @@ static void yogabook_wmi_notify(struct wmi_device *wdev, union acpi_object *dumm >> schedule_work(&data->work); >> } >> >> +static irqreturn_t yogabook_backside_hall_irq(int irq, void *_data) >> +{ >> + struct yogabook_wmi *data = _data; >> + >> + if (gpiod_get_value(data->backside_hall_gpio)) >> + set_bit(YB_TABLET_MODE, &data->flags); >> + else >> + clear_bit(YB_TABLET_MODE, &data->flags); >> + >> + schedule_work(&data->work); >> + >> + return IRQ_HANDLED; >> +} >> + >> static enum led_brightness kbd_brightness_get(struct led_classdev *cdev) >> { >> struct yogabook_wmi *data = >> @@ -197,6 +220,19 @@ static int kbd_brightness_set(struct led_classdev *cdev, >> return yogabook_wmi_set_kbd_backlight(wdev, data->brightness); >> } >> >> +static struct gpiod_lookup_table yogabook_wmi_gpios = { >> + .dev_id = "243FEC1D-1963-41C1-8100-06A9D82A94B4", >> + .table = { >> + GPIO_LOOKUP("INT33FF:02", 18, "backside_hall_sw", GPIO_ACTIVE_LOW), >> + {} >> + }, >> +}; >> + >> +static void yogabook_wmi_rm_gpio_lookup(void *unused) >> +{ >> + gpiod_remove_lookup_table(&yogabook_wmi_gpios); >> +} >> + >> static int yogabook_wmi_probe(struct wmi_device *wdev, const void *context) >> { >> struct yogabook_wmi *data; >> @@ -242,6 +278,36 @@ static int yogabook_wmi_probe(struct wmi_device *wdev, const void *context) >> goto error_put_devs; >> } >> >> + gpiod_add_lookup_table(&yogabook_wmi_gpios); >> + >> + r = devm_add_action_or_reset(&wdev->dev, yogabook_wmi_rm_gpio_lookup, NULL); >> + if (r) >> + goto error_put_devs; >> + >> + data->backside_hall_gpio = >> + devm_gpiod_get(&wdev->dev, "backside_hall_sw", GPIOD_IN); >> + if (IS_ERR(data->backside_hall_gpio)) { >> + r = PTR_ERR(data->backside_hall_gpio); >> + dev_err_probe(&wdev->dev, r, "Getting backside_hall_sw GPIO\n"); >> + goto error_put_devs; >> + } >> + >> + r = gpiod_to_irq(data->backside_hall_gpio); >> + if (r < 0) { >> + dev_err_probe(&wdev->dev, r, "Getting backside_hall_sw IRQ\n"); >> + goto error_put_devs; >> + } >> + data->backside_hall_irq = r; >> + >> + r = devm_request_irq(&wdev->dev, data->backside_hall_irq, >> + yogabook_backside_hall_irq, >> + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING, >> + "backside_hall_sw", data); >> + if (r) { >> + dev_err_probe(&wdev->dev, r, "Requesting backside_hall_sw IRQ\n"); >> + goto error_put_devs; >> + } >> + >> schedule_work(&data->work); >> >> data->kbd_bl_led.name = "ybwmi::kbd_backlight"; >> @@ -307,6 +373,9 @@ __maybe_unused int yogabook_wmi_resume(struct device *dev) >> >> clear_bit(YB_SUSPENDED, &data->flags); >> >> + /* Check for YB_TABLET_MODE changes made during suspend */ >> + schedule_work(&data->work); >> + >> return 0; >> } >> >> -- >> 2.33.1 >> >
On Sun, Nov 28, 2021 at 9:00 PM Hans de Goede <hdegoede@redhat.com> wrote: > > On the back of the device there is a hall sensor connected to the Hall (it's name) > "INT33FF:02" GPIO controller pin 18, which gets triggered when the > device is fully folded into tablet-mode (when the back of the display > touched the back of the keyboard). touches? > Use this to disable both the touch-keyboard and the digitizer when > the tablet is fully folded into tablet-mode. ... > +static irqreturn_t yogabook_backside_hall_irq(int irq, void *_data) > +{ > + struct yogabook_wmi *data = _data; > + if (gpiod_get_value(data->backside_hall_gpio)) > + set_bit(YB_TABLET_MODE, &data->flags); > + else > + clear_bit(YB_TABLET_MODE, &data->flags); assign_bit()? > + schedule_work(&data->work); > + > + return IRQ_HANDLED; > +} ... > + data->backside_hall_gpio = > + devm_gpiod_get(&wdev->dev, "backside_hall_sw", GPIOD_IN); > + if (IS_ERR(data->backside_hall_gpio)) { > + r = PTR_ERR(data->backside_hall_gpio); > + dev_err_probe(&wdev->dev, r, "Getting backside_hall_sw GPIO\n"); Can be r = dev_err_probe(...); but I think you did that on purpose to make lines shorter. > + goto error_put_devs; > + }
Hi, On 11/29/21 10:28, Andy Shevchenko wrote: > On Sun, Nov 28, 2021 at 9:00 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> On the back of the device there is a hall sensor connected to the > > Hall (it's name) Ack, will fix. > >> "INT33FF:02" GPIO controller pin 18, which gets triggered when the >> device is fully folded into tablet-mode (when the back of the display >> touched the back of the keyboard). > > touches? Ack, will fix. > >> Use this to disable both the touch-keyboard and the digitizer when >> the tablet is fully folded into tablet-mode. > > ... > >> +static irqreturn_t yogabook_backside_hall_irq(int irq, void *_data) >> +{ >> + struct yogabook_wmi *data = _data; > >> + if (gpiod_get_value(data->backside_hall_gpio)) >> + set_bit(YB_TABLET_MODE, &data->flags); >> + else >> + clear_bit(YB_TABLET_MODE, &data->flags); > > assign_bit()? Interesting I did not know about that one. I see that it expands to exactly the same code though and I find the current version more readable, so I'm going to keep this as is. > >> + schedule_work(&data->work); >> + >> + return IRQ_HANDLED; >> +} > > ... > >> + data->backside_hall_gpio = >> + devm_gpiod_get(&wdev->dev, "backside_hall_sw", GPIOD_IN); >> + if (IS_ERR(data->backside_hall_gpio)) { > >> + r = PTR_ERR(data->backside_hall_gpio); >> + dev_err_probe(&wdev->dev, r, "Getting backside_hall_sw GPIO\n"); > > Can be > > r = dev_err_probe(...); > > but I think you did that on purpose to make lines shorter. Right, I did this this way to not make the lines too long. Regards, Hans > >> + goto error_put_devs; >> + } >
diff --git a/drivers/platform/x86/lenovo-yogabook-wmi.c b/drivers/platform/x86/lenovo-yogabook-wmi.c index ecd624d9108f..65dd1ed97762 100644 --- a/drivers/platform/x86/lenovo-yogabook-wmi.c +++ b/drivers/platform/x86/lenovo-yogabook-wmi.c @@ -3,6 +3,9 @@ #include <linux/acpi.h> #include <linux/devm-helpers.h> +#include <linux/gpio/consumer.h> +#include <linux/gpio/machine.h> +#include <linux/interrupt.h> #include <linux/module.h> #include <linux/leds.h> #include <linux/wmi.h> @@ -22,6 +25,7 @@ enum { YB_KBD_IS_ON, YB_DIGITIZER_IS_ON, YB_DIGITIZER_MODE, + YB_TABLET_MODE, YB_SUSPENDED, }; @@ -31,6 +35,8 @@ struct yogabook_wmi { struct acpi_device *dig_adev; struct device *kbd_dev; struct device *dig_dev; + struct gpio_desc *backside_hall_gpio; + int backside_hall_irq; struct work_struct work; struct led_classdev kbd_bl_led; unsigned long flags; @@ -109,7 +115,10 @@ static void yogabook_wmi_work(struct work_struct *work) if (test_bit(YB_SUSPENDED, &data->flags)) return; - if (test_bit(YB_DIGITIZER_MODE, &data->flags)) { + if (test_bit(YB_TABLET_MODE, &data->flags)) { + kbd_on = false; + digitizer_on = false; + } else if (test_bit(YB_DIGITIZER_MODE, &data->flags)) { digitizer_on = true; kbd_on = false; } else { @@ -171,6 +180,20 @@ static void yogabook_wmi_notify(struct wmi_device *wdev, union acpi_object *dumm schedule_work(&data->work); } +static irqreturn_t yogabook_backside_hall_irq(int irq, void *_data) +{ + struct yogabook_wmi *data = _data; + + if (gpiod_get_value(data->backside_hall_gpio)) + set_bit(YB_TABLET_MODE, &data->flags); + else + clear_bit(YB_TABLET_MODE, &data->flags); + + schedule_work(&data->work); + + return IRQ_HANDLED; +} + static enum led_brightness kbd_brightness_get(struct led_classdev *cdev) { struct yogabook_wmi *data = @@ -197,6 +220,19 @@ static int kbd_brightness_set(struct led_classdev *cdev, return yogabook_wmi_set_kbd_backlight(wdev, data->brightness); } +static struct gpiod_lookup_table yogabook_wmi_gpios = { + .dev_id = "243FEC1D-1963-41C1-8100-06A9D82A94B4", + .table = { + GPIO_LOOKUP("INT33FF:02", 18, "backside_hall_sw", GPIO_ACTIVE_LOW), + {} + }, +}; + +static void yogabook_wmi_rm_gpio_lookup(void *unused) +{ + gpiod_remove_lookup_table(&yogabook_wmi_gpios); +} + static int yogabook_wmi_probe(struct wmi_device *wdev, const void *context) { struct yogabook_wmi *data; @@ -242,6 +278,36 @@ static int yogabook_wmi_probe(struct wmi_device *wdev, const void *context) goto error_put_devs; } + gpiod_add_lookup_table(&yogabook_wmi_gpios); + + r = devm_add_action_or_reset(&wdev->dev, yogabook_wmi_rm_gpio_lookup, NULL); + if (r) + goto error_put_devs; + + data->backside_hall_gpio = + devm_gpiod_get(&wdev->dev, "backside_hall_sw", GPIOD_IN); + if (IS_ERR(data->backside_hall_gpio)) { + r = PTR_ERR(data->backside_hall_gpio); + dev_err_probe(&wdev->dev, r, "Getting backside_hall_sw GPIO\n"); + goto error_put_devs; + } + + r = gpiod_to_irq(data->backside_hall_gpio); + if (r < 0) { + dev_err_probe(&wdev->dev, r, "Getting backside_hall_sw IRQ\n"); + goto error_put_devs; + } + data->backside_hall_irq = r; + + r = devm_request_irq(&wdev->dev, data->backside_hall_irq, + yogabook_backside_hall_irq, + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING, + "backside_hall_sw", data); + if (r) { + dev_err_probe(&wdev->dev, r, "Requesting backside_hall_sw IRQ\n"); + goto error_put_devs; + } + schedule_work(&data->work); data->kbd_bl_led.name = "ybwmi::kbd_backlight"; @@ -307,6 +373,9 @@ __maybe_unused int yogabook_wmi_resume(struct device *dev) clear_bit(YB_SUSPENDED, &data->flags); + /* Check for YB_TABLET_MODE changes made during suspend */ + schedule_work(&data->work); + return 0; }
On the back of the device there is a hall sensor connected to the "INT33FF:02" GPIO controller pin 18, which gets triggered when the device is fully folded into tablet-mode (when the back of the display touched the back of the keyboard). Use this to disable both the touch-keyboard and the digitizer when the tablet is fully folded into tablet-mode. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/platform/x86/lenovo-yogabook-wmi.c | 71 +++++++++++++++++++++- 1 file changed, 70 insertions(+), 1 deletion(-)