| Message ID | 20201026154606.10409-1-hdegoede@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] HID: i2c-hid: Put ACPI enumerated devices in D3 on shutdown | expand |
> On Oct 26, 2020, at 23:46, Hans de Goede <hdegoede@redhat.com> wrote: > > The i2c-hid driver would quietly fail to probe the i2c-hid sensor-hub > with an ACPI device-id of SMO91D0 every other boot. > > Specifically, the i2c_smbus_read_byte() "Make sure there is something at > this address" check would fail every other boot. > > It seems that the BIOS does not properly reset/power-cycle the device > leaving it in a confused state where it refuses to respond to i2c-xfers. > On boots where probing the device failed, the driver-core puts the device > in D3 after the probe-failure, which causes the probe to succeed the next > boot. > > Putting the device in D3 from the shutdown-handler fixes the sensors not > working every other boot. > > This has been tested on both a Lenovo Miix 2-10 and a Dell Venue 8 Pro 5830 > both of which use an i2c-hid sensor-hub with an ACPI id of SMO91D0. > > Note that it is safe to call acpi_device_set_power() with a NULL pointer > as first argument, so on none ACPI enumerated devices this change is a > no-op. > > Cc: Kai-Heng Feng <kai.heng.feng@canonical.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Acked-by: Kai-Heng Feng <kai.heng.feng@canonical.com> And I do wonder if we should do this for all ACPI devices... Kai-Heng > --- > Changes in v2: > -Rebase on 5.10-rc1 > --- > drivers/hid/i2c-hid/i2c-hid-core.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c > index 786e3e9af1c9..aeff1ffb0c8b 100644 > --- a/drivers/hid/i2c-hid/i2c-hid-core.c > +++ b/drivers/hid/i2c-hid/i2c-hid-core.c > @@ -943,6 +943,11 @@ static void i2c_hid_acpi_enable_wakeup(struct device *dev) > } > } > > +static void i2c_hid_acpi_shutdown(struct device *dev) > +{ > + acpi_device_set_power(ACPI_COMPANION(dev), ACPI_STATE_D3_COLD); > +} > + > static const struct acpi_device_id i2c_hid_acpi_match[] = { > {"ACPI0C50", 0 }, > {"PNP0C50", 0 }, > @@ -959,6 +964,8 @@ static inline int i2c_hid_acpi_pdata(struct i2c_client *client, > static inline void i2c_hid_acpi_fix_up_power(struct device *dev) {} > > static inline void i2c_hid_acpi_enable_wakeup(struct device *dev) {} > + > +static inline void i2c_hid_acpi_shutdown(struct device *dev) {} > #endif > > #ifdef CONFIG_OF > @@ -1175,6 +1182,8 @@ static void i2c_hid_shutdown(struct i2c_client *client) > > i2c_hid_set_power(client, I2C_HID_PWR_SLEEP); > free_irq(client->irq, ihid); > + > + i2c_hid_acpi_shutdown(&client->dev); > } > > #ifdef CONFIG_PM_SLEEP > -- > 2.28.0 >
Hi, On 10/26/20 5:27 PM, Kai-Heng Feng wrote: > > >> On Oct 26, 2020, at 23:46, Hans de Goede <hdegoede@redhat.com> wrote: >> >> The i2c-hid driver would quietly fail to probe the i2c-hid sensor-hub >> with an ACPI device-id of SMO91D0 every other boot. >> >> Specifically, the i2c_smbus_read_byte() "Make sure there is something at >> this address" check would fail every other boot. >> >> It seems that the BIOS does not properly reset/power-cycle the device >> leaving it in a confused state where it refuses to respond to i2c-xfers. >> On boots where probing the device failed, the driver-core puts the device >> in D3 after the probe-failure, which causes the probe to succeed the next >> boot. >> >> Putting the device in D3 from the shutdown-handler fixes the sensors not >> working every other boot. >> >> This has been tested on both a Lenovo Miix 2-10 and a Dell Venue 8 Pro 5830 >> both of which use an i2c-hid sensor-hub with an ACPI id of SMO91D0. >> >> Note that it is safe to call acpi_device_set_power() with a NULL pointer >> as first argument, so on none ACPI enumerated devices this change is a >> no-op. >> >> Cc: Kai-Heng Feng <kai.heng.feng@canonical.com> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > Acked-by: Kai-Heng Feng <kai.heng.feng@canonical.com> Thank you. > And I do wonder if we should do this for all ACPI devices... So do it with a power-domain shutdown handler in drivers/acpi/device_pm.c ? Interesting suggestion... Regards, Hans > > Kai-Heng > >> --- >> Changes in v2: >> -Rebase on 5.10-rc1 >> --- >> drivers/hid/i2c-hid/i2c-hid-core.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c >> index 786e3e9af1c9..aeff1ffb0c8b 100644 >> --- a/drivers/hid/i2c-hid/i2c-hid-core.c >> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c >> @@ -943,6 +943,11 @@ static void i2c_hid_acpi_enable_wakeup(struct device *dev) >> } >> } >> >> +static void i2c_hid_acpi_shutdown(struct device *dev) >> +{ >> + acpi_device_set_power(ACPI_COMPANION(dev), ACPI_STATE_D3_COLD); >> +} >> + >> static const struct acpi_device_id i2c_hid_acpi_match[] = { >> {"ACPI0C50", 0 }, >> {"PNP0C50", 0 }, >> @@ -959,6 +964,8 @@ static inline int i2c_hid_acpi_pdata(struct i2c_client *client, >> static inline void i2c_hid_acpi_fix_up_power(struct device *dev) {} >> >> static inline void i2c_hid_acpi_enable_wakeup(struct device *dev) {} >> + >> +static inline void i2c_hid_acpi_shutdown(struct device *dev) {} >> #endif >> >> #ifdef CONFIG_OF >> @@ -1175,6 +1182,8 @@ static void i2c_hid_shutdown(struct i2c_client *client) >> >> i2c_hid_set_power(client, I2C_HID_PWR_SLEEP); >> free_irq(client->irq, ihid); >> + >> + i2c_hid_acpi_shutdown(&client->dev); >> } >> >> #ifdef CONFIG_PM_SLEEP >> -- >> 2.28.0 >> >
On Mon, 26 Oct 2020, Hans de Goede wrote: > The i2c-hid driver would quietly fail to probe the i2c-hid sensor-hub > with an ACPI device-id of SMO91D0 every other boot. > > Specifically, the i2c_smbus_read_byte() "Make sure there is something at > this address" check would fail every other boot. > > It seems that the BIOS does not properly reset/power-cycle the device > leaving it in a confused state where it refuses to respond to i2c-xfers. > On boots where probing the device failed, the driver-core puts the device > in D3 after the probe-failure, which causes the probe to succeed the next > boot. > > Putting the device in D3 from the shutdown-handler fixes the sensors not > working every other boot. > > This has been tested on both a Lenovo Miix 2-10 and a Dell Venue 8 Pro 5830 > both of which use an i2c-hid sensor-hub with an ACPI id of SMO91D0. > > Note that it is safe to call acpi_device_set_power() with a NULL pointer > as first argument, so on none ACPI enumerated devices this change is a > no-op. > > Cc: Kai-Heng Feng <kai.heng.feng@canonical.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Applied, thanks. -- Jiri Kosina SUSE Labs
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index 786e3e9af1c9..aeff1ffb0c8b 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -943,6 +943,11 @@ static void i2c_hid_acpi_enable_wakeup(struct device *dev) } } +static void i2c_hid_acpi_shutdown(struct device *dev) +{ + acpi_device_set_power(ACPI_COMPANION(dev), ACPI_STATE_D3_COLD); +} + static const struct acpi_device_id i2c_hid_acpi_match[] = { {"ACPI0C50", 0 }, {"PNP0C50", 0 }, @@ -959,6 +964,8 @@ static inline int i2c_hid_acpi_pdata(struct i2c_client *client, static inline void i2c_hid_acpi_fix_up_power(struct device *dev) {} static inline void i2c_hid_acpi_enable_wakeup(struct device *dev) {} + +static inline void i2c_hid_acpi_shutdown(struct device *dev) {} #endif #ifdef CONFIG_OF @@ -1175,6 +1182,8 @@ static void i2c_hid_shutdown(struct i2c_client *client) i2c_hid_set_power(client, I2C_HID_PWR_SLEEP); free_irq(client->irq, ihid); + + i2c_hid_acpi_shutdown(&client->dev); } #ifdef CONFIG_PM_SLEEP
The i2c-hid driver would quietly fail to probe the i2c-hid sensor-hub with an ACPI device-id of SMO91D0 every other boot. Specifically, the i2c_smbus_read_byte() "Make sure there is something at this address" check would fail every other boot. It seems that the BIOS does not properly reset/power-cycle the device leaving it in a confused state where it refuses to respond to i2c-xfers. On boots where probing the device failed, the driver-core puts the device in D3 after the probe-failure, which causes the probe to succeed the next boot. Putting the device in D3 from the shutdown-handler fixes the sensors not working every other boot. This has been tested on both a Lenovo Miix 2-10 and a Dell Venue 8 Pro 5830 both of which use an i2c-hid sensor-hub with an ACPI id of SMO91D0. Note that it is safe to call acpi_device_set_power() with a NULL pointer as first argument, so on none ACPI enumerated devices this change is a no-op. Cc: Kai-Heng Feng <kai.heng.feng@canonical.com> Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Changes in v2: -Rebase on 5.10-rc1 --- drivers/hid/i2c-hid/i2c-hid-core.c | 9 +++++++++ 1 file changed, 9 insertions(+)