* [PATCH v2] HID: i2c-hid: Put ACPI enumerated devices in D3 on shutdown
@ 2020-10-26 15:46 Hans de Goede
2020-10-26 16:27 ` Kai-Heng Feng
2020-10-29 11:50 ` Jiri Kosina
0 siblings, 2 replies; 4+ messages in thread
From: Hans de Goede @ 2020-10-26 15:46 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires; +Cc: Hans de Goede, Kai-Heng Feng, linux-input
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(+)
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] HID: i2c-hid: Put ACPI enumerated devices in D3 on shutdown
2020-10-26 15:46 [PATCH v2] HID: i2c-hid: Put ACPI enumerated devices in D3 on shutdown Hans de Goede
@ 2020-10-26 16:27 ` Kai-Heng Feng
2020-10-26 16:30 ` Hans de Goede
2020-10-29 11:50 ` Jiri Kosina
1 sibling, 1 reply; 4+ messages in thread
From: Kai-Heng Feng @ 2020-10-26 16:27 UTC (permalink / raw)
To: Hans de Goede; +Cc: Jiri Kosina, Benjamin Tissoires, linux-input
> 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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] HID: i2c-hid: Put ACPI enumerated devices in D3 on shutdown
2020-10-26 16:27 ` Kai-Heng Feng
@ 2020-10-26 16:30 ` Hans de Goede
0 siblings, 0 replies; 4+ messages in thread
From: Hans de Goede @ 2020-10-26 16:30 UTC (permalink / raw)
To: Kai-Heng Feng; +Cc: Jiri Kosina, Benjamin Tissoires, linux-input
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
>>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] HID: i2c-hid: Put ACPI enumerated devices in D3 on shutdown
2020-10-26 15:46 [PATCH v2] HID: i2c-hid: Put ACPI enumerated devices in D3 on shutdown Hans de Goede
2020-10-26 16:27 ` Kai-Heng Feng
@ 2020-10-29 11:50 ` Jiri Kosina
1 sibling, 0 replies; 4+ messages in thread
From: Jiri Kosina @ 2020-10-29 11:50 UTC (permalink / raw)
To: Hans de Goede; +Cc: Benjamin Tissoires, Kai-Heng Feng, linux-input
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-10-29 11:50 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-26 15:46 [PATCH v2] HID: i2c-hid: Put ACPI enumerated devices in D3 on shutdown Hans de Goede
2020-10-26 16:27 ` Kai-Heng Feng
2020-10-26 16:30 ` Hans de Goede
2020-10-29 11:50 ` Jiri Kosina
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).