[PATCH 0/5] platform/x86: dell: Add new dell-wmi-ddv driver

[PATCH 0/5] platform/x86: dell: Add new dell-wmi-ddv driver

[Armin Wolf]

This patch series adds a new driver for a WMI interface found in
many newer Dell machines. This interface allows to read battery
properties like temperature and the ePPID (Dell-specific), while
also providing fan and thermal sensor information.

The interface does support multiple batteries which are indetified
by an "index", which appears to be the batteries ACPI UID. Since
the interface also appears to omit any bounts checking of the
index, the ACPI battery hook mechanism is used to discover batteries.

Since the information returned when querying fan/thermal sensor
information is currently unknown, a debugfs entry is created to
allow for easier reverse engineering. The interface is likely
to be replaced by a proper hwmon interface in the future.

Since the driver can potentially be instantiated mutplie times,
the ACPI battery hook mechanism had to be extended.

The first two patches allow that battery hooks are not unloaded
if they return an error when a battery was added, so that they
can safely return -ENODEV.

The next two patches extend the battery hook mechanism to better
support drivers which can be instantiated mupltible times.

The last patch finally adds the new driver. It was called
dell-wmi-ddv since the interface is called "DDV" by Dell software,
likely meaning "Dell Data Vault".

The driver was tested, together with the changes made to the
ACPI battery driver, on a Dell Inspiron 3505. Other drivers
already using the battery hook mechanism where changed as well,
but could only be compile-tested due to missing hardware.

Armin Wolf (5):
  ACPI: battery: Do not unload battery hooks on single error
  ACPI: battery: Simplify battery_hook_unregister()
  ACPI: battery: Allow battery hooks to be registered multiple times.
  ACPI: battery: Allow for passing data to battery hooks.
  platform/x86: dell: Add new dell-wmi-ddv driver

 .../ABI/testing/debugfs-dell-wmi-ddv          |  21 +
 .../ABI/testing/sysfs-platform-dell-wmi-ddv   |  16 +
 MAINTAINERS                                   |   7 +
 drivers/acpi/battery.c                        |  59 ++-
 drivers/platform/x86/asus-wmi.c               |  20 +-
 drivers/platform/x86/dell/Kconfig             |  13 +
 drivers/platform/x86/dell/Makefile            |   1 +
 drivers/platform/x86/dell/dell-wmi-ddv.c      | 365 ++++++++++++++++++
 drivers/platform/x86/huawei-wmi.c             |  15 +-
 drivers/platform/x86/lg-laptop.c              |  14 +-
 drivers/platform/x86/system76_acpi.c          |  22 +-
 drivers/platform/x86/thinkpad_acpi.c          |  15 +-
 drivers/platform/x86/wmi.c                    |   1 +
 include/acpi/battery.h                        |  12 +-
 14 files changed, 504 insertions(+), 77 deletions(-)
 create mode 100644 Documentation/ABI/testing/debugfs-dell-wmi-ddv
 create mode 100644 Documentation/ABI/testing/sysfs-platform-dell-wmi-ddv
 create mode 100644 drivers/platform/x86/dell/dell-wmi-ddv.c

--
2.30.2

[PATCH 1/5] ACPI: battery: Do not unload battery hooks on single error

[Armin Wolf]

Currently, battery hooks are being unloaded if they return
an error when adding a single battery.
This however also causes the removal of successfully added
hooks if they return -ENODEV for a single unsupported
battery.

Do not unload battery hooks in such cases since the hook
handles any cleanup actions.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/acpi/battery.c | 24 +++---------------------
 1 file changed, 3 insertions(+), 21 deletions(-)

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 306513fec1e1..e59c261c7c59 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -724,20 +724,10 @@ void battery_hook_register(struct acpi_battery_hook *hook)
 	 * its attributes.
 	 */
 	list_for_each_entry(battery, &acpi_battery_list, list) {
-		if (hook->add_battery(battery->bat)) {
-			/*
-			 * If a add-battery returns non-zero,
-			 * the registration of the extension has failed,
-			 * and we will not add it to the list of loaded
-			 * hooks.
-			 */
-			pr_err("extension failed to load: %s", hook->name);
-			__battery_hook_unregister(hook, 0);
-			goto end;
-		}
+		hook->add_battery(battery->bat);
 	}
 	pr_info("new extension: %s\n", hook->name);
-end:
+
 	mutex_unlock(&hook_mutex);
 }
 EXPORT_SYMBOL_GPL(battery_hook_register);
@@ -762,15 +752,7 @@ static void battery_hook_add_battery(struct acpi_battery *battery)
 	 * during the battery module initialization.
 	 */
 	list_for_each_entry_safe(hook_node, tmp, &battery_hook_list, list) {
-		if (hook_node->add_battery(battery->bat)) {
-			/*
-			 * The notification of the extensions has failed, to
-			 * prevent further errors we will unload the extension.
-			 */
-			pr_err("error in extension, unloading: %s",
-					hook_node->name);
-			__battery_hook_unregister(hook_node, 0);
-		}
+		hook_node->add_battery(battery->bat);
 	}
 	mutex_unlock(&hook_mutex);
 }
--
2.30.2

Re: [PATCH 1/5] ACPI: battery: Do not unload battery hooks on single error

[Hans de Goede]

Hi,

On 9/12/22 13:53, Armin Wolf wrote:
> Currently, battery hooks are being unloaded if they return
> an error when adding a single battery.
> This however also causes the removal of successfully added
> hooks if they return -ENODEV for a single unsupported
> battery.
> 
> Do not unload battery hooks in such cases since the hook
> handles any cleanup actions.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Maybe instead of removing all error checking, allow -ENODEV
and behave as before when the error is not -ENODEV ?

Otherwise we should probably make the add / remove callbacks
void to indicate that any errors are ignored.

Rafael, do you have any opinion on this?

Regards,

Hans

> ---
>  drivers/acpi/battery.c | 24 +++---------------------
>  1 file changed, 3 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> index 306513fec1e1..e59c261c7c59 100644
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -724,20 +724,10 @@ void battery_hook_register(struct acpi_battery_hook *hook)
>  	 * its attributes.
>  	 */
>  	list_for_each_entry(battery, &acpi_battery_list, list) {
> -		if (hook->add_battery(battery->bat)) {
> -			/*
> -			 * If a add-battery returns non-zero,
> -			 * the registration of the extension has failed,
> -			 * and we will not add it to the list of loaded
> -			 * hooks.
> -			 */
> -			pr_err("extension failed to load: %s", hook->name);
> -			__battery_hook_unregister(hook, 0);
> -			goto end;
> -		}
> +		hook->add_battery(battery->bat);
>  	}
>  	pr_info("new extension: %s\n", hook->name);
> -end:
> +
>  	mutex_unlock(&hook_mutex);
>  }
>  EXPORT_SYMBOL_GPL(battery_hook_register);
> @@ -762,15 +752,7 @@ static void battery_hook_add_battery(struct acpi_battery *battery)
>  	 * during the battery module initialization.
>  	 */
>  	list_for_each_entry_safe(hook_node, tmp, &battery_hook_list, list) {
> -		if (hook_node->add_battery(battery->bat)) {
> -			/*
> -			 * The notification of the extensions has failed, to
> -			 * prevent further errors we will unload the extension.
> -			 */
> -			pr_err("error in extension, unloading: %s",
> -					hook_node->name);
> -			__battery_hook_unregister(hook_node, 0);
> -		}
> +		hook_node->add_battery(battery->bat);
>  	}
>  	mutex_unlock(&hook_mutex);
>  }
> --
> 2.30.2
> 

Re: [PATCH 1/5] ACPI: battery: Do not unload battery hooks on single error

[Rafael J. Wysocki]

On Mon, Sep 19, 2022 at 12:42 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 9/12/22 13:53, Armin Wolf wrote:
> > Currently, battery hooks are being unloaded if they return
> > an error when adding a single battery.
> > This however also causes the removal of successfully added
> > hooks if they return -ENODEV for a single unsupported
> > battery.
> >
> > Do not unload battery hooks in such cases since the hook
> > handles any cleanup actions.
> >
> > Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>
> Maybe instead of removing all error checking, allow -ENODEV
> and behave as before when the error is not -ENODEV ?
>
> Otherwise we should probably make the add / remove callbacks
> void to indicate that any errors are ignored.
>
> Rafael, do you have any opinion on this?

IMV this is not a completely safe change, because things may simply
not work in the cases in which an error is returned.

It would be somewhat better to use a special error code to indicate
"no support" (eg. -ENOTSUPP) and ignore that one only.

> > ---
> >  drivers/acpi/battery.c | 24 +++---------------------
> >  1 file changed, 3 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> > index 306513fec1e1..e59c261c7c59 100644
> > --- a/drivers/acpi/battery.c
> > +++ b/drivers/acpi/battery.c
> > @@ -724,20 +724,10 @@ void battery_hook_register(struct acpi_battery_hook *hook)
> >        * its attributes.
> >        */
> >       list_for_each_entry(battery, &acpi_battery_list, list) {
> > -             if (hook->add_battery(battery->bat)) {
> > -                     /*
> > -                      * If a add-battery returns non-zero,
> > -                      * the registration of the extension has failed,
> > -                      * and we will not add it to the list of loaded
> > -                      * hooks.
> > -                      */
> > -                     pr_err("extension failed to load: %s", hook->name);
> > -                     __battery_hook_unregister(hook, 0);
> > -                     goto end;
> > -             }
> > +             hook->add_battery(battery->bat);
> >       }
> >       pr_info("new extension: %s\n", hook->name);
> > -end:
> > +
> >       mutex_unlock(&hook_mutex);
> >  }
> >  EXPORT_SYMBOL_GPL(battery_hook_register);
> > @@ -762,15 +752,7 @@ static void battery_hook_add_battery(struct acpi_battery *battery)
> >        * during the battery module initialization.
> >        */
> >       list_for_each_entry_safe(hook_node, tmp, &battery_hook_list, list) {
> > -             if (hook_node->add_battery(battery->bat)) {
> > -                     /*
> > -                      * The notification of the extensions has failed, to
> > -                      * prevent further errors we will unload the extension.
> > -                      */
> > -                     pr_err("error in extension, unloading: %s",
> > -                                     hook_node->name);
> > -                     __battery_hook_unregister(hook_node, 0);
> > -             }
> > +             hook_node->add_battery(battery->bat);
> >       }
> >       mutex_unlock(&hook_mutex);
> >  }
> > --
> > 2.30.2
> >
>

Re: [PATCH 1/5] ACPI: battery: Do not unload battery hooks on single error

[Armin Wolf]

Am 19.09.22 um 18:27 schrieb Rafael J. Wysocki:

> On Mon, Sep 19, 2022 at 12:42 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 9/12/22 13:53, Armin Wolf wrote:
>>> Currently, battery hooks are being unloaded if they return
>>> an error when adding a single battery.
>>> This however also causes the removal of successfully added
>>> hooks if they return -ENODEV for a single unsupported
>>> battery.
>>>
>>> Do not unload battery hooks in such cases since the hook
>>> handles any cleanup actions.
>>>
>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> Maybe instead of removing all error checking, allow -ENODEV
>> and behave as before when the error is not -ENODEV ?
>>
>> Otherwise we should probably make the add / remove callbacks
>> void to indicate that any errors are ignored.
>>
>> Rafael, do you have any opinion on this?
> IMV this is not a completely safe change, because things may simply
> not work in the cases in which an error is returned.
>
> It would be somewhat better to use a special error code to indicate
> "no support" (eg. -ENOTSUPP) and ignore that one only.

I would favor -ENODEV then, since it is already used by quiet a few drivers
to indicate a unsupported battery.

Armin Wolf

>>> ---
>>>   drivers/acpi/battery.c | 24 +++---------------------
>>>   1 file changed, 3 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
>>> index 306513fec1e1..e59c261c7c59 100644
>>> --- a/drivers/acpi/battery.c
>>> +++ b/drivers/acpi/battery.c
>>> @@ -724,20 +724,10 @@ void battery_hook_register(struct acpi_battery_hook *hook)
>>>         * its attributes.
>>>         */
>>>        list_for_each_entry(battery, &acpi_battery_list, list) {
>>> -             if (hook->add_battery(battery->bat)) {
>>> -                     /*
>>> -                      * If a add-battery returns non-zero,
>>> -                      * the registration of the extension has failed,
>>> -                      * and we will not add it to the list of loaded
>>> -                      * hooks.
>>> -                      */
>>> -                     pr_err("extension failed to load: %s", hook->name);
>>> -                     __battery_hook_unregister(hook, 0);
>>> -                     goto end;
>>> -             }
>>> +             hook->add_battery(battery->bat);
>>>        }
>>>        pr_info("new extension: %s\n", hook->name);
>>> -end:
>>> +
>>>        mutex_unlock(&hook_mutex);
>>>   }
>>>   EXPORT_SYMBOL_GPL(battery_hook_register);
>>> @@ -762,15 +752,7 @@ static void battery_hook_add_battery(struct acpi_battery *battery)
>>>         * during the battery module initialization.
>>>         */
>>>        list_for_each_entry_safe(hook_node, tmp, &battery_hook_list, list) {
>>> -             if (hook_node->add_battery(battery->bat)) {
>>> -                     /*
>>> -                      * The notification of the extensions has failed, to
>>> -                      * prevent further errors we will unload the extension.
>>> -                      */
>>> -                     pr_err("error in extension, unloading: %s",
>>> -                                     hook_node->name);
>>> -                     __battery_hook_unregister(hook_node, 0);
>>> -             }
>>> +             hook_node->add_battery(battery->bat);
>>>        }
>>>        mutex_unlock(&hook_mutex);
>>>   }
>>> --
>>> 2.30.2
>>>

Re: [PATCH 1/5] ACPI: battery: Do not unload battery hooks on single error

[Armin Wolf]

Am 19.09.22 um 21:12 schrieb Armin Wolf:

> Am 19.09.22 um 18:27 schrieb Rafael J. Wysocki:
>
>> On Mon, Sep 19, 2022 at 12:42 PM Hans de Goede <hdegoede@redhat.com> 
>> wrote:
>>> Hi,
>>>
>>> On 9/12/22 13:53, Armin Wolf wrote:
>>>> Currently, battery hooks are being unloaded if they return
>>>> an error when adding a single battery.
>>>> This however also causes the removal of successfully added
>>>> hooks if they return -ENODEV for a single unsupported
>>>> battery.
>>>>
>>>> Do not unload battery hooks in such cases since the hook
>>>> handles any cleanup actions.
>>>>
>>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>>> Maybe instead of removing all error checking, allow -ENODEV
>>> and behave as before when the error is not -ENODEV ?
>>>
>>> Otherwise we should probably make the add / remove callbacks
>>> void to indicate that any errors are ignored.
>>>
>>> Rafael, do you have any opinion on this?
>> IMV this is not a completely safe change, because things may simply
>> not work in the cases in which an error is returned.
>>
>> It would be somewhat better to use a special error code to indicate
>> "no support" (eg. -ENOTSUPP) and ignore that one only.
>
> I would favor -ENODEV then, since it is already used by quiet a few 
> drivers
> to indicate a unsupported battery.
>
> Armin Wolf
>
While checking all instances where the battery hook mechanism is currently used,
i found out that all but a single battery hook return -ENODEV for all errors they
encounter, the exception being the huawei-wmi driver.

I do not know the reason for this, but i fear unloading the extension on for
example -ENOTSUP will result in similar behavior by hooks wanting to avoid being
unloaded on harmless errors.

However, i agree that when ignoring all errors, battery extensions which provide
similar attributes may currently delete each others attributes.

Any idea on how to solve this?

Armin Wolf

>>>> ---
>>>>   drivers/acpi/battery.c | 24 +++---------------------
>>>>   1 file changed, 3 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
>>>> index 306513fec1e1..e59c261c7c59 100644
>>>> --- a/drivers/acpi/battery.c
>>>> +++ b/drivers/acpi/battery.c
>>>> @@ -724,20 +724,10 @@ void battery_hook_register(struct 
>>>> acpi_battery_hook *hook)
>>>>         * its attributes.
>>>>         */
>>>>        list_for_each_entry(battery, &acpi_battery_list, list) {
>>>> -             if (hook->add_battery(battery->bat)) {
>>>> -                     /*
>>>> -                      * If a add-battery returns non-zero,
>>>> -                      * the registration of the extension has failed,
>>>> -                      * and we will not add it to the list of loaded
>>>> -                      * hooks.
>>>> -                      */
>>>> -                     pr_err("extension failed to load: %s", 
>>>> hook->name);
>>>> -                     __battery_hook_unregister(hook, 0);
>>>> -                     goto end;
>>>> -             }
>>>> +             hook->add_battery(battery->bat);
>>>>        }
>>>>        pr_info("new extension: %s\n", hook->name);
>>>> -end:
>>>> +
>>>>        mutex_unlock(&hook_mutex);
>>>>   }
>>>>   EXPORT_SYMBOL_GPL(battery_hook_register);
>>>> @@ -762,15 +752,7 @@ static void battery_hook_add_battery(struct 
>>>> acpi_battery *battery)
>>>>         * during the battery module initialization.
>>>>         */
>>>>        list_for_each_entry_safe(hook_node, tmp, &battery_hook_list, 
>>>> list) {
>>>> -             if (hook_node->add_battery(battery->bat)) {
>>>> -                     /*
>>>> -                      * The notification of the extensions has 
>>>> failed, to
>>>> -                      * prevent further errors we will unload the 
>>>> extension.
>>>> -                      */
>>>> -                     pr_err("error in extension, unloading: %s",
>>>> -                                     hook_node->name);
>>>> -                     __battery_hook_unregister(hook_node, 0);
>>>> -             }
>>>> +             hook_node->add_battery(battery->bat);
>>>>        }
>>>>        mutex_unlock(&hook_mutex);
>>>>   }
>>>> -- 
>>>> 2.30.2
>>>>

Re: [PATCH 1/5] ACPI: battery: Do not unload battery hooks on single error

[Hans de Goede]

Hi,

On 9/19/22 22:35, Armin Wolf wrote:
> Am 19.09.22 um 21:12 schrieb Armin Wolf:
> 
>> Am 19.09.22 um 18:27 schrieb Rafael J. Wysocki:
>>
>>> On Mon, Sep 19, 2022 at 12:42 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>> Hi,
>>>>
>>>> On 9/12/22 13:53, Armin Wolf wrote:
>>>>> Currently, battery hooks are being unloaded if they return
>>>>> an error when adding a single battery.
>>>>> This however also causes the removal of successfully added
>>>>> hooks if they return -ENODEV for a single unsupported
>>>>> battery.
>>>>>
>>>>> Do not unload battery hooks in such cases since the hook
>>>>> handles any cleanup actions.
>>>>>
>>>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>>>> Maybe instead of removing all error checking, allow -ENODEV
>>>> and behave as before when the error is not -ENODEV ?
>>>>
>>>> Otherwise we should probably make the add / remove callbacks
>>>> void to indicate that any errors are ignored.
>>>>
>>>> Rafael, do you have any opinion on this?
>>> IMV this is not a completely safe change, because things may simply
>>> not work in the cases in which an error is returned.
>>>
>>> It would be somewhat better to use a special error code to indicate
>>> "no support" (eg. -ENOTSUPP) and ignore that one only.
>>
>> I would favor -ENODEV then, since it is already used by quiet a few drivers
>> to indicate a unsupported battery.
>>
>> Armin Wolf
>>
> While checking all instances where the battery hook mechanism is currently used,
> i found out that all but a single battery hook return -ENODEV for all errors they
> encounter, the exception being the huawei-wmi driver.

Right, so this means that using -ENODEV to not automatically unload the
extension on error will result in a behavior change for those drivers,
with possibly unwanted side-effects.

As such I believe that using -ENOTSUP for the case where the extension
does not work for 1 battery but should still be used for the other
batter{y|ies} would be better as this preserves the existing behavior
for existing drivers.

> I do not know the reason for this, but i fear unloading the extension on for
> example -ENOTSUP will result in similar behavior by hooks wanting to avoid being
> unloaded on harmless errors.

I am not sure what you are trying to say here. The whole idea is
to add new behavior for -ENOTSUP to allow drivers to opt out of
getting their extension unregistered when they return this.

Although I wonder why not just have extensions return 0 when
they don't want to register any sysfs attr and that not considered
an error. If it is not considered an error the hook can just
return 0, which would not require any ACPI battery code changes
at all. So maybe just returning 0 is the easiest (which is
also often the best) answer here?

> However, i agree that when ignoring all errors, battery extensions which provide
> similar attributes may currently delete each others attributes.

AFAIK there are no cases where more then 1 extension driver gets loaded,
since all the extension drivers are tied to a specific vendor's interfaces
so we won't e.g. see the thinkpad_acpi driver load on the same laptop as
where toshiba_acpi also loads.

IOW I think you are trying to solve a problem which does not exist here.

Regards,

Hans




> 
> Any idea on how to solve this?
> 
> Armin Wolf
> 
>>>>> ---
>>>>>   drivers/acpi/battery.c | 24 +++---------------------
>>>>>   1 file changed, 3 insertions(+), 21 deletions(-)
>>>>>
>>>>> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
>>>>> index 306513fec1e1..e59c261c7c59 100644
>>>>> --- a/drivers/acpi/battery.c
>>>>> +++ b/drivers/acpi/battery.c
>>>>> @@ -724,20 +724,10 @@ void battery_hook_register(struct acpi_battery_hook *hook)
>>>>>         * its attributes.
>>>>>         */
>>>>>        list_for_each_entry(battery, &acpi_battery_list, list) {
>>>>> -             if (hook->add_battery(battery->bat)) {
>>>>> -                     /*
>>>>> -                      * If a add-battery returns non-zero,
>>>>> -                      * the registration of the extension has failed,
>>>>> -                      * and we will not add it to the list of loaded
>>>>> -                      * hooks.
>>>>> -                      */
>>>>> -                     pr_err("extension failed to load: %s", hook->name);
>>>>> -                     __battery_hook_unregister(hook, 0);
>>>>> -                     goto end;
>>>>> -             }
>>>>> +             hook->add_battery(battery->bat);
>>>>>        }
>>>>>        pr_info("new extension: %s\n", hook->name);
>>>>> -end:
>>>>> +
>>>>>        mutex_unlock(&hook_mutex);
>>>>>   }
>>>>>   EXPORT_SYMBOL_GPL(battery_hook_register);
>>>>> @@ -762,15 +752,7 @@ static void battery_hook_add_battery(struct acpi_battery *battery)
>>>>>         * during the battery module initialization.
>>>>>         */
>>>>>        list_for_each_entry_safe(hook_node, tmp, &battery_hook_list, list) {
>>>>> -             if (hook_node->add_battery(battery->bat)) {
>>>>> -                     /*
>>>>> -                      * The notification of the extensions has failed, to
>>>>> -                      * prevent further errors we will unload the extension.
>>>>> -                      */
>>>>> -                     pr_err("error in extension, unloading: %s",
>>>>> -                                     hook_node->name);
>>>>> -                     __battery_hook_unregister(hook_node, 0);
>>>>> -             }
>>>>> +             hook_node->add_battery(battery->bat);
>>>>>        }
>>>>>        mutex_unlock(&hook_mutex);
>>>>>   }
>>>>> -- 
>>>>> 2.30.2
>>>>>

Re: [PATCH 1/5] ACPI: battery: Do not unload battery hooks on single error

[Armin Wolf]

Am 27.09.22 um 16:29 schrieb Hans de Goede:

> Hi,
>
> On 9/19/22 22:35, Armin Wolf wrote:
>> Am 19.09.22 um 21:12 schrieb Armin Wolf:
>>
>>> Am 19.09.22 um 18:27 schrieb Rafael J. Wysocki:
>>>
>>>> On Mon, Sep 19, 2022 at 12:42 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>>> Hi,
>>>>>
>>>>> On 9/12/22 13:53, Armin Wolf wrote:
>>>>>> Currently, battery hooks are being unloaded if they return
>>>>>> an error when adding a single battery.
>>>>>> This however also causes the removal of successfully added
>>>>>> hooks if they return -ENODEV for a single unsupported
>>>>>> battery.
>>>>>>
>>>>>> Do not unload battery hooks in such cases since the hook
>>>>>> handles any cleanup actions.
>>>>>>
>>>>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>>>>> Maybe instead of removing all error checking, allow -ENODEV
>>>>> and behave as before when the error is not -ENODEV ?
>>>>>
>>>>> Otherwise we should probably make the add / remove callbacks
>>>>> void to indicate that any errors are ignored.
>>>>>
>>>>> Rafael, do you have any opinion on this?
>>>> IMV this is not a completely safe change, because things may simply
>>>> not work in the cases in which an error is returned.
>>>>
>>>> It would be somewhat better to use a special error code to indicate
>>>> "no support" (eg. -ENOTSUPP) and ignore that one only.
>>> I would favor -ENODEV then, since it is already used by quiet a few drivers
>>> to indicate a unsupported battery.
>>>
>>> Armin Wolf
>>>
>> While checking all instances where the battery hook mechanism is currently used,
>> i found out that all but a single battery hook return -ENODEV for all errors they
>> encounter, the exception being the huawei-wmi driver.
> Right, so this means that using -ENODEV to not automatically unload the
> extension on error will result in a behavior change for those drivers,
> with possibly unwanted side-effects.
>
> As such I believe that using -ENOTSUP for the case where the extension
> does not work for 1 battery but should still be used for the other
> batter{y|ies} would be better as this preserves the existing behavior
> for existing drivers.
>
>> I do not know the reason for this, but i fear unloading the extension on for
>> example -ENOTSUP will result in similar behavior by hooks wanting to avoid being
>> unloaded on harmless errors.
> I am not sure what you are trying to say here. The whole idea is
> to add new behavior for -ENOTSUP to allow drivers to opt out of
> getting their extension unregistered when they return this.
>
> Although I wonder why not just have extensions return 0 when
> they don't want to register any sysfs attr and that not considered
> an error. If it is not considered an error the hook can just
> return 0, which would not require any ACPI battery code changes
> at all. So maybe just returning 0 is the easiest (which is
> also often the best) answer here?

I agree, i will send v2 soon.

Armin Wolf

>> However, i agree that when ignoring all errors, battery extensions which provide
>> similar attributes may currently delete each others attributes.
> AFAIK there are no cases where more then 1 extension driver gets loaded,
> since all the extension drivers are tied to a specific vendor's interfaces
> so we won't e.g. see the thinkpad_acpi driver load on the same laptop as
> where toshiba_acpi also loads.
>
> IOW I think you are trying to solve a problem which does not exist here.
>
> Regards,
>
> Hans
>
>
>
>
>> Any idea on how to solve this?
>>
>> Armin Wolf
>>
>>>>>> ---
>>>>>>    drivers/acpi/battery.c | 24 +++---------------------
>>>>>>    1 file changed, 3 insertions(+), 21 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
>>>>>> index 306513fec1e1..e59c261c7c59 100644
>>>>>> --- a/drivers/acpi/battery.c
>>>>>> +++ b/drivers/acpi/battery.c
>>>>>> @@ -724,20 +724,10 @@ void battery_hook_register(struct acpi_battery_hook *hook)
>>>>>>          * its attributes.
>>>>>>          */
>>>>>>         list_for_each_entry(battery, &acpi_battery_list, list) {
>>>>>> -             if (hook->add_battery(battery->bat)) {
>>>>>> -                     /*
>>>>>> -                      * If a add-battery returns non-zero,
>>>>>> -                      * the registration of the extension has failed,
>>>>>> -                      * and we will not add it to the list of loaded
>>>>>> -                      * hooks.
>>>>>> -                      */
>>>>>> -                     pr_err("extension failed to load: %s", hook->name);
>>>>>> -                     __battery_hook_unregister(hook, 0);
>>>>>> -                     goto end;
>>>>>> -             }
>>>>>> +             hook->add_battery(battery->bat);
>>>>>>         }
>>>>>>         pr_info("new extension: %s\n", hook->name);
>>>>>> -end:
>>>>>> +
>>>>>>         mutex_unlock(&hook_mutex);
>>>>>>    }
>>>>>>    EXPORT_SYMBOL_GPL(battery_hook_register);
>>>>>> @@ -762,15 +752,7 @@ static void battery_hook_add_battery(struct acpi_battery *battery)
>>>>>>          * during the battery module initialization.
>>>>>>          */
>>>>>>         list_for_each_entry_safe(hook_node, tmp, &battery_hook_list, list) {
>>>>>> -             if (hook_node->add_battery(battery->bat)) {
>>>>>> -                     /*
>>>>>> -                      * The notification of the extensions has failed, to
>>>>>> -                      * prevent further errors we will unload the extension.
>>>>>> -                      */
>>>>>> -                     pr_err("error in extension, unloading: %s",
>>>>>> -                                     hook_node->name);
>>>>>> -                     __battery_hook_unregister(hook_node, 0);
>>>>>> -             }
>>>>>> +             hook_node->add_battery(battery->bat);
>>>>>>         }
>>>>>>         mutex_unlock(&hook_mutex);
>>>>>>    }
>>>>>> --
>>>>>> 2.30.2
>>>>>>

[PATCH 2/5] ACPI: battery: Simplify battery_hook_unregister()

[Armin Wolf]

Since __battery_hook_unregister() is always called
with lock set to 1, remove the unneeded conditionals
and merge it with battery_hook_unregister().

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/acpi/battery.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index e59c261c7c59..4aea65f3d8c3 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -686,27 +686,22 @@ static LIST_HEAD(acpi_battery_list);
 static LIST_HEAD(battery_hook_list);
 static DEFINE_MUTEX(hook_mutex);

-static void __battery_hook_unregister(struct acpi_battery_hook *hook, int lock)
+void battery_hook_unregister(struct acpi_battery_hook *hook)
 {
 	struct acpi_battery *battery;
 	/*
 	 * In order to remove a hook, we first need to
 	 * de-register all the batteries that are registered.
 	 */
-	if (lock)
-		mutex_lock(&hook_mutex);
+	mutex_lock(&hook_mutex);
+
 	list_for_each_entry(battery, &acpi_battery_list, list) {
 		hook->remove_battery(battery->bat);
 	}
 	list_del(&hook->list);
-	if (lock)
-		mutex_unlock(&hook_mutex);
-	pr_info("extension unregistered: %s\n", hook->name);
-}

-void battery_hook_unregister(struct acpi_battery_hook *hook)
-{
-	__battery_hook_unregister(hook, 1);
+	mutex_unlock(&hook_mutex);
+	pr_info("extension unregistered: %s\n", hook->name);
 }
 EXPORT_SYMBOL_GPL(battery_hook_unregister);

@@ -784,7 +779,7 @@ static void __exit battery_hook_exit(void)
 	 * need to remove the hooks.
 	 */
 	list_for_each_entry_safe(hook, ptr, &battery_hook_list, list) {
-		__battery_hook_unregister(hook, 1);
+		battery_hook_unregister(hook);
 	}
 	mutex_destroy(&hook_mutex);
 }
--
2.30.2

Re: [PATCH 2/5] ACPI: battery: Simplify battery_hook_unregister()

[Hans de Goede]

Hi,

On 9/12/22 13:53, Armin Wolf wrote:
> Since __battery_hook_unregister() is always called
> with lock set to 1, remove the unneeded conditionals
> and merge it with battery_hook_unregister().
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
>  drivers/acpi/battery.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> index e59c261c7c59..4aea65f3d8c3 100644
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -686,27 +686,22 @@ static LIST_HEAD(acpi_battery_list);
>  static LIST_HEAD(battery_hook_list);
>  static DEFINE_MUTEX(hook_mutex);
> 
> -static void __battery_hook_unregister(struct acpi_battery_hook *hook, int lock)
> +void battery_hook_unregister(struct acpi_battery_hook *hook)
>  {
>  	struct acpi_battery *battery;
>  	/*
>  	 * In order to remove a hook, we first need to
>  	 * de-register all the batteries that are registered.
>  	 */
> -	if (lock)
> -		mutex_lock(&hook_mutex);
> +	mutex_lock(&hook_mutex);
> +
>  	list_for_each_entry(battery, &acpi_battery_list, list) {
>  		hook->remove_battery(battery->bat);
>  	}
>  	list_del(&hook->list);
> -	if (lock)
> -		mutex_unlock(&hook_mutex);
> -	pr_info("extension unregistered: %s\n", hook->name);
> -}
> 
> -void battery_hook_unregister(struct acpi_battery_hook *hook)
> -{
> -	__battery_hook_unregister(hook, 1);
> +	mutex_unlock(&hook_mutex);
> +	pr_info("extension unregistered: %s\n", hook->name);
>  }
>  EXPORT_SYMBOL_GPL(battery_hook_unregister);
> 
> @@ -784,7 +779,7 @@ static void __exit battery_hook_exit(void)
>  	 * need to remove the hooks.
>  	 */
>  	list_for_each_entry_safe(hook, ptr, &battery_hook_list, list) {
> -		__battery_hook_unregister(hook, 1);
> +		battery_hook_unregister(hook);
>  	}
>  	mutex_destroy(&hook_mutex);
>  }
> --
> 2.30.2
> 

[PATCH 3/5] ACPI: battery: Allow battery hooks to be registered multiple times.

[Armin Wolf]

Registering multiple instances of a battery hook is beneficial
for drivers which can be instantiated multiple times. Until now,
this would mean that such a driver would have to implement some
logic to manage battery hooks.

Extend the battery hook handling instead.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/acpi/battery.c               | 21 ++++++++++++++++-----
 drivers/platform/x86/asus-wmi.c      | 15 ++++++---------
 drivers/platform/x86/huawei-wmi.c    | 11 +++++++----
 drivers/platform/x86/lg-laptop.c     | 10 ++++++----
 drivers/platform/x86/system76_acpi.c | 18 ++++++++++--------
 drivers/platform/x86/thinkpad_acpi.c | 11 +++++++----
 include/acpi/battery.h               | 11 ++++++++---
 7 files changed, 60 insertions(+), 37 deletions(-)

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 4aea65f3d8c3..15bb5d868a56 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -696,19 +696,28 @@ void battery_hook_unregister(struct acpi_battery_hook *hook)
 	mutex_lock(&hook_mutex);

 	list_for_each_entry(battery, &acpi_battery_list, list) {
-		hook->remove_battery(battery->bat);
+		hook->ops->remove_battery(battery->bat);
 	}
 	list_del(&hook->list);

 	mutex_unlock(&hook_mutex);
 	pr_info("extension unregistered: %s\n", hook->name);
+	kfree(hook);
 }
 EXPORT_SYMBOL_GPL(battery_hook_unregister);

-void battery_hook_register(struct acpi_battery_hook *hook)
+struct acpi_battery_hook *battery_hook_register(const char *name,
+						const struct acpi_battery_hook_ops *ops)
 {
+	struct acpi_battery_hook *hook = kzalloc(sizeof(*hook), GFP_KERNEL);
 	struct acpi_battery *battery;

+	if (!hook)
+		return ERR_PTR(-ENOMEM);
+
+	hook->name = name;
+	hook->ops = ops;
+
 	mutex_lock(&hook_mutex);
 	INIT_LIST_HEAD(&hook->list);
 	list_add(&hook->list, &battery_hook_list);
@@ -719,11 +728,13 @@ void battery_hook_register(struct acpi_battery_hook *hook)
 	 * its attributes.
 	 */
 	list_for_each_entry(battery, &acpi_battery_list, list) {
-		hook->add_battery(battery->bat);
+		hook->ops->add_battery(battery->bat);
 	}
 	pr_info("new extension: %s\n", hook->name);

 	mutex_unlock(&hook_mutex);
+
+	return hook;
 }
 EXPORT_SYMBOL_GPL(battery_hook_register);

@@ -747,7 +758,7 @@ static void battery_hook_add_battery(struct acpi_battery *battery)
 	 * during the battery module initialization.
 	 */
 	list_for_each_entry_safe(hook_node, tmp, &battery_hook_list, list) {
-		hook_node->add_battery(battery->bat);
+		hook_node->ops->add_battery(battery->bat);
 	}
 	mutex_unlock(&hook_mutex);
 }
@@ -762,7 +773,7 @@ static void battery_hook_remove_battery(struct acpi_battery *battery)
 	 * custom attributes from the battery.
 	 */
 	list_for_each_entry(hook, &battery_hook_list, list) {
-		hook->remove_battery(battery->bat);
+		hook->ops->remove_battery(battery->bat);
 	}
 	/* Then, just remove the battery from the list */
 	list_del(&battery->list);
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index d95170b7dba0..37d8649418f4 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -252,8 +252,8 @@ struct asus_wmi {
 	struct platform_profile_handler platform_profile_handler;
 	bool platform_profile_support;

-	// The RSOC controls the maximum charging percentage.
-	bool battery_rsoc_available;
+	// The RSOC battery hook controls the maximum charging percentage.
+	struct acpi_battery_hook *hook;

 	bool panel_overdrive_available;

@@ -916,25 +916,22 @@ static int asus_wmi_battery_remove(struct power_supply *battery)
 	return 0;
 }

-static struct acpi_battery_hook battery_hook = {
+static const struct acpi_battery_hook_ops battery_hook_ops = {
 	.add_battery = asus_wmi_battery_add,
 	.remove_battery = asus_wmi_battery_remove,
-	.name = "ASUS Battery Extension",
 };

 static void asus_wmi_battery_init(struct asus_wmi *asus)
 {
-	asus->battery_rsoc_available = false;
 	if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_RSOC)) {
-		asus->battery_rsoc_available = true;
-		battery_hook_register(&battery_hook);
+		asus->hook = battery_hook_register("ASUS Battery Extension", &battery_hook_ops);
 	}
 }

 static void asus_wmi_battery_exit(struct asus_wmi *asus)
 {
-	if (asus->battery_rsoc_available)
-		battery_hook_unregister(&battery_hook);
+	if (!IS_ERR_OR_NULL(asus->hook))
+		battery_hook_unregister(asus->hook);
 }

 /* LEDs ***********************************************************************/
diff --git a/drivers/platform/x86/huawei-wmi.c b/drivers/platform/x86/huawei-wmi.c
index eac3e6b4ea11..6fd02b25a97b 100644
--- a/drivers/platform/x86/huawei-wmi.c
+++ b/drivers/platform/x86/huawei-wmi.c
@@ -62,6 +62,7 @@ struct huawei_wmi {
 	bool battery_available;
 	bool fn_lock_available;

+	struct acpi_battery_hook *hook;
 	struct huawei_wmi_debug debug;
 	struct input_dev *idev[2];
 	struct led_classdev cdev;
@@ -491,10 +492,9 @@ static int huawei_wmi_battery_remove(struct power_supply *battery)
 	return 0;
 }

-static struct acpi_battery_hook huawei_wmi_battery_hook = {
+static const struct acpi_battery_hook_ops huawei_wmi_battery_hook_ops = {
 	.add_battery = huawei_wmi_battery_add,
 	.remove_battery = huawei_wmi_battery_remove,
-	.name = "Huawei Battery Extension"
 };

 static void huawei_wmi_battery_setup(struct device *dev)
@@ -507,7 +507,8 @@ static void huawei_wmi_battery_setup(struct device *dev)
 		return;
 	}

-	battery_hook_register(&huawei_wmi_battery_hook);
+	huawei->hook = battery_hook_register("Huawei Battery Extension",
+					     &huawei_wmi_battery_hook_ops);
 	device_create_file(dev, &dev_attr_charge_control_thresholds);
 }

@@ -516,7 +517,9 @@ static void huawei_wmi_battery_exit(struct device *dev)
 	struct huawei_wmi *huawei = dev_get_drvdata(dev);

 	if (huawei->battery_available) {
-		battery_hook_unregister(&huawei_wmi_battery_hook);
+		if (!IS_ERR(huawei->hook))
+			battery_hook_unregister(huawei->hook);
+
 		device_remove_file(dev, &dev_attr_charge_control_thresholds);
 	}
 }
diff --git a/drivers/platform/x86/lg-laptop.c b/drivers/platform/x86/lg-laptop.c
index 332868b140ed..d8a61a07313e 100644
--- a/drivers/platform/x86/lg-laptop.c
+++ b/drivers/platform/x86/lg-laptop.c
@@ -73,6 +73,7 @@ static u32 inited;
 #define INIT_SPARSE_KEYMAP      0x80

 static int battery_limit_use_wmbb;
+static struct acpi_battery_hook *hook;
 static struct led_classdev kbd_backlight;
 static enum led_brightness get_kbd_backlight_level(void);

@@ -562,10 +563,9 @@ static int lg_battery_remove(struct power_supply *battery)
 	return 0;
 }

-static struct acpi_battery_hook battery_hook = {
+static const struct acpi_battery_hook_ops battery_hook_ops = {
 	.add_battery = lg_battery_add,
 	.remove_battery = lg_battery_remove,
-	.name = "LG Battery Extension",
 };

 static struct attribute *dev_attributes[] = {
@@ -750,7 +750,7 @@ static int acpi_add(struct acpi_device *device)
 	led_classdev_register(&pf_device->dev, &tpad_led);

 	wmi_input_setup();
-	battery_hook_register(&battery_hook);
+	hook = battery_hook_register("LG Battery Extension", &battery_hook_ops);

 	return 0;

@@ -768,7 +768,9 @@ static int acpi_remove(struct acpi_device *device)
 	led_classdev_unregister(&tpad_led);
 	led_classdev_unregister(&kbd_backlight);

-	battery_hook_unregister(&battery_hook);
+	if (!IS_ERR(hook))
+		battery_hook_unregister(hook);
+
 	wmi_input_destroy();
 	platform_device_unregister(pf_device);
 	pf_device = NULL;
diff --git a/drivers/platform/x86/system76_acpi.c b/drivers/platform/x86/system76_acpi.c
index 958df41ad509..1ec22db32bd0 100644
--- a/drivers/platform/x86/system76_acpi.c
+++ b/drivers/platform/x86/system76_acpi.c
@@ -26,6 +26,7 @@

 struct system76_data {
 	struct acpi_device *acpi_dev;
+	struct acpi_battery_hook *hook;
 	struct led_classdev ap_led;
 	struct led_classdev kb_led;
 	enum led_brightness kb_brightness;
@@ -272,20 +273,21 @@ static int system76_battery_remove(struct power_supply *battery)
 	return 0;
 }

-static struct acpi_battery_hook system76_battery_hook = {
+static const struct acpi_battery_hook_ops system76_battery_hook_ops = {
 	.add_battery = system76_battery_add,
 	.remove_battery = system76_battery_remove,
-	.name = "System76 Battery Extension",
 };

-static void system76_battery_init(void)
+static void system76_battery_init(struct system76_data *data)
 {
-	battery_hook_register(&system76_battery_hook);
+	data->hook = battery_hook_register("System76 Battery Extension",
+					   &system76_battery_hook_ops);
 }

-static void system76_battery_exit(void)
+static void system76_battery_exit(struct system76_data *data)
 {
-	battery_hook_unregister(&system76_battery_hook);
+	if (!IS_ERR(data->hook))
+		battery_hook_unregister(data->hook);
 }

 // Get the airplane mode LED brightness
@@ -730,7 +732,7 @@ static int system76_add(struct acpi_device *acpi_dev)
 		if (err)
 			goto error;

-		system76_battery_init();
+		system76_battery_init(data);
 	}

 	return 0;
@@ -751,7 +753,7 @@ static int system76_remove(struct acpi_device *acpi_dev)
 	data = acpi_driver_data(acpi_dev);

 	if (data->has_open_ec) {
-		system76_battery_exit();
+		system76_battery_exit(data);
 		kfree(data->nfan);
 		kfree(data->ntmp);
 	}
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 8fbe21ebcc52..8adafc3c31fa 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -9407,6 +9407,7 @@ struct tpacpi_battery_data {

 struct tpacpi_battery_driver_data {
 	struct tpacpi_battery_data batteries[3];
+	struct acpi_battery_hook *hook;
 	int individual_addressing;
 };

@@ -9914,10 +9915,9 @@ static int tpacpi_battery_remove(struct power_supply *battery)
 	return 0;
 }

-static struct acpi_battery_hook battery_hook = {
+static const struct acpi_battery_hook_ops battery_hook_ops = {
 	.add_battery = tpacpi_battery_add,
 	.remove_battery = tpacpi_battery_remove,
-	.name = "ThinkPad Battery Extension",
 };

 /* Subdriver init/exit */
@@ -9943,13 +9943,16 @@ static int __init tpacpi_battery_init(struct ibm_init_struct *ibm)
 					battery_quirk_table,
 					ARRAY_SIZE(battery_quirk_table));

-	battery_hook_register(&battery_hook);
+	battery_info.hook = battery_hook_register("ThinkPad Battery Extension",
+						  &battery_hook_ops);
+
 	return 0;
 }

 static void tpacpi_battery_exit(void)
 {
-	battery_hook_unregister(&battery_hook);
+	if (!IS_ERR(battery_info.hook))
+		battery_hook_unregister(battery_info.hook);
 }

 static struct ibm_struct battery_driver_data = {
diff --git a/include/acpi/battery.h b/include/acpi/battery.h
index b8d56b702c7a..b3c81abada1e 100644
--- a/include/acpi/battery.h
+++ b/include/acpi/battery.h
@@ -10,14 +10,19 @@
 #define ACPI_BATTERY_NOTIFY_INFO	0x81
 #define ACPI_BATTERY_NOTIFY_THRESHOLD   0x82

-struct acpi_battery_hook {
-	const char *name;
+struct acpi_battery_hook_ops {
 	int (*add_battery)(struct power_supply *battery);
 	int (*remove_battery)(struct power_supply *battery);
+};
+
+struct acpi_battery_hook {
+	const char *name;
+	const struct acpi_battery_hook_ops *ops;
 	struct list_head list;
 };

-void battery_hook_register(struct acpi_battery_hook *hook);
+struct acpi_battery_hook *battery_hook_register(const char *name,
+						const struct acpi_battery_hook_ops *hook);
 void battery_hook_unregister(struct acpi_battery_hook *hook);

 #endif
--
2.30.2

Re: [PATCH 3/5] ACPI: battery: Allow battery hooks to be registered multiple times.

[Barnabás Pőcze]

Hi

2022. szeptember 12., hétfő 14:53 keltezéssel, Armin Wolf írta:

> Registering multiple instances of a battery hook is beneficial
> for drivers which can be instantiated multiple times. Until now,
> this would mean that such a driver would have to implement some
> logic to manage battery hooks.
> 
> Extend the battery hook handling instead.

I think this is already possible by embedding the acpi_battery_hook
object inside the driver's device specific data object, no?

Regards,
Barnabás Pőcze


> [...]

Re: [PATCH 3/5] ACPI: battery: Allow battery hooks to be registered multiple times.

[Armin Wolf]

Am 12.09.22 um 18:42 schrieb Barnabás Pőcze:

> Hi
>
> 2022. szeptember 12., hétfő 14:53 keltezéssel, Armin Wolf írta:
>
>> Registering multiple instances of a battery hook is beneficial
>> for drivers which can be instantiated multiple times. Until now,
>> this would mean that such a driver would have to implement some
>> logic to manage battery hooks.
>>
>> Extend the battery hook handling instead.
> I think this is already possible by embedding the acpi_battery_hook
> object inside the driver's device specific data object, no?
>
> Regards,
> Barnabás Pőcze
>
>
Yes, it indeed is. However afaik it is not possible to pass instance-specific
data to such an embedded battery hook. It could be possible by passing the
battery hook as an argument to add_battery()/remove_battery() and using container_of(),
but in my opinion this would be too much of a quick hack.

>> [...]

Re: [PATCH 3/5] ACPI: battery: Allow battery hooks to be registered multiple times.

[Hans de Goede]

Hi,

On 9/12/22 18:29, Armin Wolf wrote:
> Am 12.09.22 um 18:42 schrieb Barnabás Pőcze:
> 
>> Hi
>>
>> 2022. szeptember 12., hétfő 14:53 keltezéssel, Armin Wolf írta:
>>
>>> Registering multiple instances of a battery hook is beneficial
>>> for drivers which can be instantiated multiple times. Until now,
>>> this would mean that such a driver would have to implement some
>>> logic to manage battery hooks.
>>>
>>> Extend the battery hook handling instead.
>> I think this is already possible by embedding the acpi_battery_hook
>> object inside the driver's device specific data object, no?
>>
>> Regards,
>> Barnabás Pőcze
>>
>>
> Yes, it indeed is. However afaik it is not possible to pass instance-specific
> data to such an embedded battery hook. It could be possible by passing the
> battery hook as an argument to add_battery()/remove_battery() and using container_of(),
> but in my opinion this would be too much of a quick hack.

Actually thinking more about this (after my reviewed-by of 4/5) I believe
that leaving the lifetime management of the struct acpi_battery_hook hook
in the caller and then modifying 4/4 to pass the hook to the callback,
so that the callback can indeed do container_of to get its top-level
driver-data struct would be a better/cleaner solution then this patch +
patch 4/5 .

Doing things this way is quite normal in the kernel and doing a single
large alloc is better then a bunch of small allocs. In this case it does
not really matter, but if we do things like this over all drivers
eventually all the small mallocs add up.

Doing things this way would also reduce the amount of churn in this
series a bit.

Regards,

Hans

Re: [PATCH 3/5] ACPI: battery: Allow battery hooks to be registered multiple times.

[Barnabás Pőcze]

Hi

2022. szeptember 12., hétfő 19:29 keltezéssel, Armin Wolf <W_Armin@gmx.de> írta:

> Am 12.09.22 um 18:42 schrieb Barnabás Pőcze:
> 
> > Hi
> > 
> > 2022. szeptember 12., hétfő 14:53 keltezéssel, Armin Wolf írta:
> > 
> > > Registering multiple instances of a battery hook is beneficial
> > > for drivers which can be instantiated multiple times. Until now,
> > > this would mean that such a driver would have to implement some
> > > logic to manage battery hooks.
> > > 
> > > Extend the battery hook handling instead.
> > > I think this is already possible by embedding the acpi_battery_hook
> > > object inside the driver's device specific data object, no?
> > 
> > Regards,
> > Barnabás Pőcze
> 
> Yes, it indeed is. However afaik it is not possible to pass instance-specific
> data to such an embedded battery hook. It could be possible by passing the
> battery hook as an argument to add_battery()/remove_battery() and using container_of(),
> but in my opinion this would be too much of a quick hack.

Good point about the instance-specific data. However, regarding the second point,
I am with Hans. I do not really think it is that big of a hack. It is inheritance.


Regards,
Barnabás Pőcze

[PATCH 4/5] ACPI: battery: Allow for passing data to battery hooks.

[Armin Wolf]

Since a driver may register multiple instances of a
battery hook, passing data to each instance is
convenient.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/acpi/battery.c               | 11 ++++++-----
 drivers/platform/x86/asus-wmi.c      |  7 ++++---
 drivers/platform/x86/huawei-wmi.c    |  6 +++---
 drivers/platform/x86/lg-laptop.c     |  6 +++---
 drivers/platform/x86/system76_acpi.c |  6 +++---
 drivers/platform/x86/thinkpad_acpi.c |  6 +++---
 include/acpi/battery.h               |  7 ++++---
 7 files changed, 26 insertions(+), 23 deletions(-)

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 15bb5d868a56..396a7324216c 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -696,7 +696,7 @@ void battery_hook_unregister(struct acpi_battery_hook *hook)
 	mutex_lock(&hook_mutex);

 	list_for_each_entry(battery, &acpi_battery_list, list) {
-		hook->ops->remove_battery(battery->bat);
+		hook->ops->remove_battery(hook->data, battery->bat);
 	}
 	list_del(&hook->list);

@@ -706,7 +706,7 @@ void battery_hook_unregister(struct acpi_battery_hook *hook)
 }
 EXPORT_SYMBOL_GPL(battery_hook_unregister);

-struct acpi_battery_hook *battery_hook_register(const char *name,
+struct acpi_battery_hook *battery_hook_register(const char *name, void *data,
 						const struct acpi_battery_hook_ops *ops)
 {
 	struct acpi_battery_hook *hook = kzalloc(sizeof(*hook), GFP_KERNEL);
@@ -716,6 +716,7 @@ struct acpi_battery_hook *battery_hook_register(const char *name,
 		return ERR_PTR(-ENOMEM);

 	hook->name = name;
+	hook->data = data;
 	hook->ops = ops;

 	mutex_lock(&hook_mutex);
@@ -728,7 +729,7 @@ struct acpi_battery_hook *battery_hook_register(const char *name,
 	 * its attributes.
 	 */
 	list_for_each_entry(battery, &acpi_battery_list, list) {
-		hook->ops->add_battery(battery->bat);
+		hook->ops->add_battery(hook->data, battery->bat);
 	}
 	pr_info("new extension: %s\n", hook->name);

@@ -758,7 +759,7 @@ static void battery_hook_add_battery(struct acpi_battery *battery)
 	 * during the battery module initialization.
 	 */
 	list_for_each_entry_safe(hook_node, tmp, &battery_hook_list, list) {
-		hook_node->ops->add_battery(battery->bat);
+		hook_node->ops->add_battery(hook_node->data, battery->bat);
 	}
 	mutex_unlock(&hook_mutex);
 }
@@ -773,7 +774,7 @@ static void battery_hook_remove_battery(struct acpi_battery *battery)
 	 * custom attributes from the battery.
 	 */
 	list_for_each_entry(hook, &battery_hook_list, list) {
-		hook->ops->remove_battery(battery->bat);
+		hook->ops->remove_battery(hook->data, battery->bat);
 	}
 	/* Then, just remove the battery from the list */
 	list_del(&battery->list);
diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 37d8649418f4..18afcf38931f 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -882,7 +882,7 @@ static ssize_t charge_control_end_threshold_show(struct device *device,

 static DEVICE_ATTR_RW(charge_control_end_threshold);

-static int asus_wmi_battery_add(struct power_supply *battery)
+static int asus_wmi_battery_add(void *data, struct power_supply *battery)
 {
 	/* The WMI method does not provide a way to specific a battery, so we
 	 * just assume it is the first battery.
@@ -909,7 +909,7 @@ static int asus_wmi_battery_add(struct power_supply *battery)
 	return 0;
 }

-static int asus_wmi_battery_remove(struct power_supply *battery)
+static int asus_wmi_battery_remove(void *data, struct power_supply *battery)
 {
 	device_remove_file(&battery->dev,
 			   &dev_attr_charge_control_end_threshold);
@@ -924,7 +924,8 @@ static const struct acpi_battery_hook_ops battery_hook_ops = {
 static void asus_wmi_battery_init(struct asus_wmi *asus)
 {
 	if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_RSOC)) {
-		asus->hook = battery_hook_register("ASUS Battery Extension", &battery_hook_ops);
+		asus->hook = battery_hook_register("ASUS Battery Extension", NULL,
+						   &battery_hook_ops);
 	}
 }

diff --git a/drivers/platform/x86/huawei-wmi.c b/drivers/platform/x86/huawei-wmi.c
index 6fd02b25a97b..f23806299c1a 100644
--- a/drivers/platform/x86/huawei-wmi.c
+++ b/drivers/platform/x86/huawei-wmi.c
@@ -469,7 +469,7 @@ static DEVICE_ATTR_RW(charge_control_start_threshold);
 static DEVICE_ATTR_RW(charge_control_end_threshold);
 static DEVICE_ATTR_RW(charge_control_thresholds);

-static int huawei_wmi_battery_add(struct power_supply *battery)
+static int huawei_wmi_battery_add(void *data, struct power_supply *battery)
 {
 	int err = 0;

@@ -484,7 +484,7 @@ static int huawei_wmi_battery_add(struct power_supply *battery)
 	return err;
 }

-static int huawei_wmi_battery_remove(struct power_supply *battery)
+static int huawei_wmi_battery_remove(void *data, struct power_supply *battery)
 {
 	device_remove_file(&battery->dev, &dev_attr_charge_control_start_threshold);
 	device_remove_file(&battery->dev, &dev_attr_charge_control_end_threshold);
@@ -507,7 +507,7 @@ static void huawei_wmi_battery_setup(struct device *dev)
 		return;
 	}

-	huawei->hook = battery_hook_register("Huawei Battery Extension",
+	huawei->hook = battery_hook_register("Huawei Battery Extension", NULL,
 					     &huawei_wmi_battery_hook_ops);
 	device_create_file(dev, &dev_attr_charge_control_thresholds);
 }
diff --git a/drivers/platform/x86/lg-laptop.c b/drivers/platform/x86/lg-laptop.c
index d8a61a07313e..f1abb1924150 100644
--- a/drivers/platform/x86/lg-laptop.c
+++ b/drivers/platform/x86/lg-laptop.c
@@ -547,7 +547,7 @@ static DEVICE_ATTR_RW(fn_lock);
 static DEVICE_ATTR_RW(charge_control_end_threshold);
 static DEVICE_ATTR_RW(battery_care_limit);

-static int lg_battery_add(struct power_supply *battery)
+static int lg_battery_add(void *data, struct power_supply *battery)
 {
 	if (device_create_file(&battery->dev,
 			       &dev_attr_charge_control_end_threshold))
@@ -556,7 +556,7 @@ static int lg_battery_add(struct power_supply *battery)
 	return 0;
 }

-static int lg_battery_remove(struct power_supply *battery)
+static int lg_battery_remove(void *data, struct power_supply *battery)
 {
 	device_remove_file(&battery->dev,
 			   &dev_attr_charge_control_end_threshold);
@@ -750,7 +750,7 @@ static int acpi_add(struct acpi_device *device)
 	led_classdev_register(&pf_device->dev, &tpad_led);

 	wmi_input_setup();
-	hook = battery_hook_register("LG Battery Extension", &battery_hook_ops);
+	hook = battery_hook_register("LG Battery Extension", NULL, &battery_hook_ops);

 	return 0;

diff --git a/drivers/platform/x86/system76_acpi.c b/drivers/platform/x86/system76_acpi.c
index 1ec22db32bd0..9414b9491806 100644
--- a/drivers/platform/x86/system76_acpi.c
+++ b/drivers/platform/x86/system76_acpi.c
@@ -255,7 +255,7 @@ static struct attribute *system76_battery_attrs[] = {

 ATTRIBUTE_GROUPS(system76_battery);

-static int system76_battery_add(struct power_supply *battery)
+static int system76_battery_add(void *data, struct power_supply *battery)
 {
 	// System76 EC only supports 1 battery
 	if (strcmp(battery->desc->name, "BAT0") != 0)
@@ -267,7 +267,7 @@ static int system76_battery_add(struct power_supply *battery)
 	return 0;
 }

-static int system76_battery_remove(struct power_supply *battery)
+static int system76_battery_remove(void *data, struct power_supply *battery)
 {
 	device_remove_groups(&battery->dev, system76_battery_groups);
 	return 0;
@@ -280,7 +280,7 @@ static const struct acpi_battery_hook_ops system76_battery_hook_ops = {

 static void system76_battery_init(struct system76_data *data)
 {
-	data->hook = battery_hook_register("System76 Battery Extension",
+	data->hook = battery_hook_register("System76 Battery Extension", NULL,
 					   &system76_battery_hook_ops);
 }

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 8adafc3c31fa..6008d88e0727 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -9898,7 +9898,7 @@ ATTRIBUTE_GROUPS(tpacpi_battery);

 /* ACPI battery hooking */

-static int tpacpi_battery_add(struct power_supply *battery)
+static int tpacpi_battery_add(void *data, struct power_supply *battery)
 {
 	int batteryid = tpacpi_battery_get_id(battery->desc->name);

@@ -9909,7 +9909,7 @@ static int tpacpi_battery_add(struct power_supply *battery)
 	return 0;
 }

-static int tpacpi_battery_remove(struct power_supply *battery)
+static int tpacpi_battery_remove(void *data, struct power_supply *battery)
 {
 	device_remove_groups(&battery->dev, tpacpi_battery_groups);
 	return 0;
@@ -9943,7 +9943,7 @@ static int __init tpacpi_battery_init(struct ibm_init_struct *ibm)
 					battery_quirk_table,
 					ARRAY_SIZE(battery_quirk_table));

-	battery_info.hook = battery_hook_register("ThinkPad Battery Extension",
+	battery_info.hook = battery_hook_register("ThinkPad Battery Extension", NULL,
 						  &battery_hook_ops);

 	return 0;
diff --git a/include/acpi/battery.h b/include/acpi/battery.h
index b3c81abada1e..cca401b793b2 100644
--- a/include/acpi/battery.h
+++ b/include/acpi/battery.h
@@ -11,17 +11,18 @@
 #define ACPI_BATTERY_NOTIFY_THRESHOLD   0x82

 struct acpi_battery_hook_ops {
-	int (*add_battery)(struct power_supply *battery);
-	int (*remove_battery)(struct power_supply *battery);
+	int (*add_battery)(void *data, struct power_supply *battery);
+	int (*remove_battery)(void *data, struct power_supply *battery);
 };

 struct acpi_battery_hook {
 	const char *name;
 	const struct acpi_battery_hook_ops *ops;
+	void *data;
 	struct list_head list;
 };

-struct acpi_battery_hook *battery_hook_register(const char *name,
+struct acpi_battery_hook *battery_hook_register(const char *name, void *data,
 						const struct acpi_battery_hook_ops *hook);
 void battery_hook_unregister(struct acpi_battery_hook *hook);

--
2.30.2

Re: [PATCH 4/5] ACPI: battery: Allow for passing data to battery hooks.

[Hans de Goede]

Hi,

On 9/12/22 13:53, Armin Wolf wrote:
> Since a driver may register multiple instances of a
> battery hook, passing data to each instance is
> convenient.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
>  drivers/acpi/battery.c               | 11 ++++++-----
>  drivers/platform/x86/asus-wmi.c      |  7 ++++---
>  drivers/platform/x86/huawei-wmi.c    |  6 +++---
>  drivers/platform/x86/lg-laptop.c     |  6 +++---
>  drivers/platform/x86/system76_acpi.c |  6 +++---
>  drivers/platform/x86/thinkpad_acpi.c |  6 +++---
>  include/acpi/battery.h               |  7 ++++---
>  7 files changed, 26 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> index 15bb5d868a56..396a7324216c 100644
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -696,7 +696,7 @@ void battery_hook_unregister(struct acpi_battery_hook *hook)
>  	mutex_lock(&hook_mutex);
> 
>  	list_for_each_entry(battery, &acpi_battery_list, list) {
> -		hook->ops->remove_battery(battery->bat);
> +		hook->ops->remove_battery(hook->data, battery->bat);
>  	}
>  	list_del(&hook->list);
> 
> @@ -706,7 +706,7 @@ void battery_hook_unregister(struct acpi_battery_hook *hook)
>  }
>  EXPORT_SYMBOL_GPL(battery_hook_unregister);
> 
> -struct acpi_battery_hook *battery_hook_register(const char *name,
> +struct acpi_battery_hook *battery_hook_register(const char *name, void *data,
>  						const struct acpi_battery_hook_ops *ops)
>  {
>  	struct acpi_battery_hook *hook = kzalloc(sizeof(*hook), GFP_KERNEL);
> @@ -716,6 +716,7 @@ struct acpi_battery_hook *battery_hook_register(const char *name,
>  		return ERR_PTR(-ENOMEM);
> 
>  	hook->name = name;
> +	hook->data = data;
>  	hook->ops = ops;
> 
>  	mutex_lock(&hook_mutex);
> @@ -728,7 +729,7 @@ struct acpi_battery_hook *battery_hook_register(const char *name,
>  	 * its attributes.
>  	 */
>  	list_for_each_entry(battery, &acpi_battery_list, list) {
> -		hook->ops->add_battery(battery->bat);
> +		hook->ops->add_battery(hook->data, battery->bat);
>  	}
>  	pr_info("new extension: %s\n", hook->name);
> 
> @@ -758,7 +759,7 @@ static void battery_hook_add_battery(struct acpi_battery *battery)
>  	 * during the battery module initialization.
>  	 */
>  	list_for_each_entry_safe(hook_node, tmp, &battery_hook_list, list) {
> -		hook_node->ops->add_battery(battery->bat);
> +		hook_node->ops->add_battery(hook_node->data, battery->bat);
>  	}
>  	mutex_unlock(&hook_mutex);
>  }
> @@ -773,7 +774,7 @@ static void battery_hook_remove_battery(struct acpi_battery *battery)
>  	 * custom attributes from the battery.
>  	 */
>  	list_for_each_entry(hook, &battery_hook_list, list) {
> -		hook->ops->remove_battery(battery->bat);
> +		hook->ops->remove_battery(hook->data, battery->bat);
>  	}
>  	/* Then, just remove the battery from the list */
>  	list_del(&battery->list);
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 37d8649418f4..18afcf38931f 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -882,7 +882,7 @@ static ssize_t charge_control_end_threshold_show(struct device *device,
> 
>  static DEVICE_ATTR_RW(charge_control_end_threshold);
> 
> -static int asus_wmi_battery_add(struct power_supply *battery)
> +static int asus_wmi_battery_add(void *data, struct power_supply *battery)
>  {
>  	/* The WMI method does not provide a way to specific a battery, so we
>  	 * just assume it is the first battery.
> @@ -909,7 +909,7 @@ static int asus_wmi_battery_add(struct power_supply *battery)
>  	return 0;
>  }
> 
> -static int asus_wmi_battery_remove(struct power_supply *battery)
> +static int asus_wmi_battery_remove(void *data, struct power_supply *battery)
>  {
>  	device_remove_file(&battery->dev,
>  			   &dev_attr_charge_control_end_threshold);
> @@ -924,7 +924,8 @@ static const struct acpi_battery_hook_ops battery_hook_ops = {
>  static void asus_wmi_battery_init(struct asus_wmi *asus)
>  {
>  	if (asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_RSOC)) {
> -		asus->hook = battery_hook_register("ASUS Battery Extension", &battery_hook_ops);
> +		asus->hook = battery_hook_register("ASUS Battery Extension", NULL,
> +						   &battery_hook_ops);
>  	}
>  }
> 
> diff --git a/drivers/platform/x86/huawei-wmi.c b/drivers/platform/x86/huawei-wmi.c
> index 6fd02b25a97b..f23806299c1a 100644
> --- a/drivers/platform/x86/huawei-wmi.c
> +++ b/drivers/platform/x86/huawei-wmi.c
> @@ -469,7 +469,7 @@ static DEVICE_ATTR_RW(charge_control_start_threshold);
>  static DEVICE_ATTR_RW(charge_control_end_threshold);
>  static DEVICE_ATTR_RW(charge_control_thresholds);
> 
> -static int huawei_wmi_battery_add(struct power_supply *battery)
> +static int huawei_wmi_battery_add(void *data, struct power_supply *battery)
>  {
>  	int err = 0;
> 
> @@ -484,7 +484,7 @@ static int huawei_wmi_battery_add(struct power_supply *battery)
>  	return err;
>  }
> 
> -static int huawei_wmi_battery_remove(struct power_supply *battery)
> +static int huawei_wmi_battery_remove(void *data, struct power_supply *battery)
>  {
>  	device_remove_file(&battery->dev, &dev_attr_charge_control_start_threshold);
>  	device_remove_file(&battery->dev, &dev_attr_charge_control_end_threshold);
> @@ -507,7 +507,7 @@ static void huawei_wmi_battery_setup(struct device *dev)
>  		return;
>  	}
> 
> -	huawei->hook = battery_hook_register("Huawei Battery Extension",
> +	huawei->hook = battery_hook_register("Huawei Battery Extension", NULL,
>  					     &huawei_wmi_battery_hook_ops);
>  	device_create_file(dev, &dev_attr_charge_control_thresholds);
>  }
> diff --git a/drivers/platform/x86/lg-laptop.c b/drivers/platform/x86/lg-laptop.c
> index d8a61a07313e..f1abb1924150 100644
> --- a/drivers/platform/x86/lg-laptop.c
> +++ b/drivers/platform/x86/lg-laptop.c
> @@ -547,7 +547,7 @@ static DEVICE_ATTR_RW(fn_lock);
>  static DEVICE_ATTR_RW(charge_control_end_threshold);
>  static DEVICE_ATTR_RW(battery_care_limit);
> 
> -static int lg_battery_add(struct power_supply *battery)
> +static int lg_battery_add(void *data, struct power_supply *battery)
>  {
>  	if (device_create_file(&battery->dev,
>  			       &dev_attr_charge_control_end_threshold))
> @@ -556,7 +556,7 @@ static int lg_battery_add(struct power_supply *battery)
>  	return 0;
>  }
> 
> -static int lg_battery_remove(struct power_supply *battery)
> +static int lg_battery_remove(void *data, struct power_supply *battery)
>  {
>  	device_remove_file(&battery->dev,
>  			   &dev_attr_charge_control_end_threshold);
> @@ -750,7 +750,7 @@ static int acpi_add(struct acpi_device *device)
>  	led_classdev_register(&pf_device->dev, &tpad_led);
> 
>  	wmi_input_setup();
> -	hook = battery_hook_register("LG Battery Extension", &battery_hook_ops);
> +	hook = battery_hook_register("LG Battery Extension", NULL, &battery_hook_ops);
> 
>  	return 0;
> 
> diff --git a/drivers/platform/x86/system76_acpi.c b/drivers/platform/x86/system76_acpi.c
> index 1ec22db32bd0..9414b9491806 100644
> --- a/drivers/platform/x86/system76_acpi.c
> +++ b/drivers/platform/x86/system76_acpi.c
> @@ -255,7 +255,7 @@ static struct attribute *system76_battery_attrs[] = {
> 
>  ATTRIBUTE_GROUPS(system76_battery);
> 
> -static int system76_battery_add(struct power_supply *battery)
> +static int system76_battery_add(void *data, struct power_supply *battery)
>  {
>  	// System76 EC only supports 1 battery
>  	if (strcmp(battery->desc->name, "BAT0") != 0)
> @@ -267,7 +267,7 @@ static int system76_battery_add(struct power_supply *battery)
>  	return 0;
>  }
> 
> -static int system76_battery_remove(struct power_supply *battery)
> +static int system76_battery_remove(void *data, struct power_supply *battery)
>  {
>  	device_remove_groups(&battery->dev, system76_battery_groups);
>  	return 0;
> @@ -280,7 +280,7 @@ static const struct acpi_battery_hook_ops system76_battery_hook_ops = {
> 
>  static void system76_battery_init(struct system76_data *data)
>  {
> -	data->hook = battery_hook_register("System76 Battery Extension",
> +	data->hook = battery_hook_register("System76 Battery Extension", NULL,
>  					   &system76_battery_hook_ops);
>  }
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index 8adafc3c31fa..6008d88e0727 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -9898,7 +9898,7 @@ ATTRIBUTE_GROUPS(tpacpi_battery);
> 
>  /* ACPI battery hooking */
> 
> -static int tpacpi_battery_add(struct power_supply *battery)
> +static int tpacpi_battery_add(void *data, struct power_supply *battery)
>  {
>  	int batteryid = tpacpi_battery_get_id(battery->desc->name);
> 
> @@ -9909,7 +9909,7 @@ static int tpacpi_battery_add(struct power_supply *battery)
>  	return 0;
>  }
> 
> -static int tpacpi_battery_remove(struct power_supply *battery)
> +static int tpacpi_battery_remove(void *data, struct power_supply *battery)
>  {
>  	device_remove_groups(&battery->dev, tpacpi_battery_groups);
>  	return 0;
> @@ -9943,7 +9943,7 @@ static int __init tpacpi_battery_init(struct ibm_init_struct *ibm)
>  					battery_quirk_table,
>  					ARRAY_SIZE(battery_quirk_table));
> 
> -	battery_info.hook = battery_hook_register("ThinkPad Battery Extension",
> +	battery_info.hook = battery_hook_register("ThinkPad Battery Extension", NULL,
>  						  &battery_hook_ops);
> 
>  	return 0;
> diff --git a/include/acpi/battery.h b/include/acpi/battery.h
> index b3c81abada1e..cca401b793b2 100644
> --- a/include/acpi/battery.h
> +++ b/include/acpi/battery.h
> @@ -11,17 +11,18 @@
>  #define ACPI_BATTERY_NOTIFY_THRESHOLD   0x82
> 
>  struct acpi_battery_hook_ops {
> -	int (*add_battery)(struct power_supply *battery);
> -	int (*remove_battery)(struct power_supply *battery);
> +	int (*add_battery)(void *data, struct power_supply *battery);
> +	int (*remove_battery)(void *data, struct power_supply *battery);
>  };
> 
>  struct acpi_battery_hook {
>  	const char *name;
>  	const struct acpi_battery_hook_ops *ops;
> +	void *data;
>  	struct list_head list;
>  };
> 
> -struct acpi_battery_hook *battery_hook_register(const char *name,
> +struct acpi_battery_hook *battery_hook_register(const char *name, void *data,
>  						const struct acpi_battery_hook_ops *hook);
>  void battery_hook_unregister(struct acpi_battery_hook *hook);
> 
> --
> 2.30.2
> 

[PATCH 5/5] platform/x86: dell: Add new dell-wmi-ddv driver

[Armin Wolf]

The dell-wmi-ddv driver adds support for reading
the current temperature and ePPID of ACPI batteries
on supported Dell machines.

Since the WMI interface used by this driver does not
do any input validation and thus cannot be used for probing,
the driver depends on the ACPI battery extension machanism
to discover batteries.

The driver also supports a debugfs interface for retrieving
buffers containing fan and thermal sensor information.
Since the meaing of the content of those buffers is currently
unknown, the interface is meant for reverse-engineering and
will likely be replaced with an hwmon interface once the
meaning has been understood.

The driver was tested on a Dell Inspiron 3505.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 .../ABI/testing/debugfs-dell-wmi-ddv          |  21 +
 .../ABI/testing/sysfs-platform-dell-wmi-ddv   |  16 +
 MAINTAINERS                                   |   7 +
 drivers/platform/x86/dell/Kconfig             |  13 +
 drivers/platform/x86/dell/Makefile            |   1 +
 drivers/platform/x86/dell/dell-wmi-ddv.c      | 365 ++++++++++++++++++
 drivers/platform/x86/wmi.c                    |   1 +
 7 files changed, 424 insertions(+)
 create mode 100644 Documentation/ABI/testing/debugfs-dell-wmi-ddv
 create mode 100644 Documentation/ABI/testing/sysfs-platform-dell-wmi-ddv
 create mode 100644 drivers/platform/x86/dell/dell-wmi-ddv.c

diff --git a/Documentation/ABI/testing/debugfs-dell-wmi-ddv b/Documentation/ABI/testing/debugfs-dell-wmi-ddv
new file mode 100644
index 000000000000..fbcc5d6f7388
--- /dev/null
+++ b/Documentation/ABI/testing/debugfs-dell-wmi-ddv
@@ -0,0 +1,21 @@
+What:		/sys/kernel/debug/dell-wmi-ddv-<wmi_device_name>/fan_sensor_information
+Date:		September 2022
+KernelVersion:	6.1
+Contact:	Armin Wolf <W_Armin@gmx.de>
+Description:
+		This file contains the contents of the fan sensor information buffer,
+		which contains fan sensor entries and a terminating character (0xFF).
+
+		Each fan sensor entry consists of three bytes with an unknown meaning,
+		interested people may use this file for reverse-engineering.
+
+What:		/sys/kernel/debug/dell-wmi-ddv-<wmi_device_name>/thermal_sensor_information
+Date:		September 2022
+KernelVersion:	6.1
+Contact:	Armin Wolf <W_Armin@gmx.de>
+Description:
+		This file contains the contents of the thermal sensor information buffer,
+		which contains thermal sensor entries and a terminating character (0xFF).
+
+		Each thermal sensor entry consists of five bytes with an unknown meaning,
+		interested people may use this file for reverse-engineering.
diff --git a/Documentation/ABI/testing/sysfs-platform-dell-wmi-ddv b/Documentation/ABI/testing/sysfs-platform-dell-wmi-ddv
new file mode 100644
index 000000000000..98e0b8399d13
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform-dell-wmi-ddv
@@ -0,0 +1,16 @@
+What:		/sys/class/power_supply/<battery_name>/temp
+Date:		September 2022
+KernelVersion:	6.1
+Contact:	Armin Wolf <W_Armin@gmx.de>
+Description:
+		Reports the current ACPI battery temperature on supported Dell machines.
+
+		Values are represented in 1/10 Degrees Celsius.
+
+What:		/sys/class/power_supply/<battery_name>/eppid
+Date:		September 2022
+KernelVersion:	6.1
+Contact:	Armin Wolf <W_Armin@gmx.de>
+Description:
+		Reports the Dell ePPID (electronic Dell Piece Part Identification)
+		of the ACPI battery.
diff --git a/MAINTAINERS b/MAINTAINERS
index 6bb894ea4a77..d9fd4c9eebbc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5821,6 +5821,13 @@ L:	Dell.Client.Kernel@dell.com
 S:	Maintained
 F:	drivers/platform/x86/dell/dell-wmi-descriptor.c

+DELL WMI DDV DRIVER
+M:	Armin Wolf <W_Armin@gmx.de>
+S:	Maintained
+F:	Documentation/ABI/testing/debugfs-dell-wmi-ddv
+F:	Documentation/ABI/testing/sysfs-platform-dell-wmi-ddv
+F:	drivers/platform/x86/dell/dell-wmi-ddv.c
+
 DELL WMI SYSMAN DRIVER
 M:	Divya Bharathi <divya.bharathi@dell.com>
 M:	Prasanth Ksr <prasanth.ksr@dell.com>
diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
index 25421e061c47..209e63e347e2 100644
--- a/drivers/platform/x86/dell/Kconfig
+++ b/drivers/platform/x86/dell/Kconfig
@@ -189,6 +189,19 @@ config DELL_WMI_DESCRIPTOR
 	default n
 	depends on ACPI_WMI

+config DELL_WMI_DDV
+	tristate "Dell WMI sensors Support"
+	default m
+	depends on ACPI_BATTERY
+	depends on ACPI_WMI
+	help
+	  This option adds support for WMI-based sensors like
+	  battery temperature sensors found on some Dell notebooks.
+	  It also supports reading of the batteries ePPID.
+
+	  To compile this drivers as a module, choose M here: the module will
+	  be called dell-wmi-ddv.
+
 config DELL_WMI_LED
 	tristate "External LED on Dell Business Netbooks"
 	default m
diff --git a/drivers/platform/x86/dell/Makefile b/drivers/platform/x86/dell/Makefile
index ddba1df71e80..1b8942426622 100644
--- a/drivers/platform/x86/dell/Makefile
+++ b/drivers/platform/x86/dell/Makefile
@@ -19,5 +19,6 @@ dell-wmi-objs				:= dell-wmi-base.o
 dell-wmi-$(CONFIG_DELL_WMI_PRIVACY)	+= dell-wmi-privacy.o
 obj-$(CONFIG_DELL_WMI_AIO)		+= dell-wmi-aio.o
 obj-$(CONFIG_DELL_WMI_DESCRIPTOR)	+= dell-wmi-descriptor.o
+obj-$(CONFIG_DELL_WMI_DDV)		+= dell-wmi-ddv.o
 obj-$(CONFIG_DELL_WMI_LED)		+= dell-wmi-led.o
 obj-$(CONFIG_DELL_WMI_SYSMAN)		+= dell-wmi-sysman/
diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
new file mode 100644
index 000000000000..6eef298f13eb
--- /dev/null
+++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
@@ -0,0 +1,365 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * dell-wmi-ddv.c -- Linux driver for WMI sensor information on Dell notebooks.
+ *
+ * Copyright (C) 2022 Armin Wolf <W_Armin@gmx.de>
+ */
+
+#define pr_format(fmt) KBUILD_MODNAME ": " fmt
+
+#include <acpi/battery.h>
+#include <linux/acpi.h>
+#include <linux/debugfs.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/kstrtox.h>
+#include <linux/math.h>
+#include <linux/module.h>
+#include <linux/limits.h>
+#include <linux/power_supply.h>
+#include <linux/seq_file.h>
+#include <linux/sysfs.h>
+#include <linux/wmi.h>
+
+#define DRIVER_NAME	"dell-wmi-ddv"
+
+#define DELL_DDV_SUPPORTED_INTERFACE 2
+#define DELL_DDV_GUID	"8A42EA14-4F2A-FD45-6422-0087F7A7E608"
+
+enum dell_ddv_method {
+	DELL_DDV_BATTERY_DESIGN_CAPACITY	= 0x01,
+	DELL_DDV_BATTERY_FULL_CHARGE_CAPACITY	= 0x02,
+	DELL_DDV_BATTERY_MANUFACTURE_NAME	= 0x03,
+	DELL_DDV_BATTERY_MANUFACTURE_DATE	= 0x04,
+	DELL_DDV_BATTERY_SERIAL_NUMBER		= 0x05,
+	DELL_DDV_BATTERY_CHEMISTRY_VALUE	= 0x06,
+	DELL_DDV_BATTERY_TEMPERATURE		= 0x07,
+	DELL_DDV_BATTERY_CURRENT		= 0x08,
+	DELL_DDV_BATTERY_VOLTAGE		= 0x09,
+	DELL_DDV_BATTERY_MANUFACTURER_ACCESS	= 0x0A,
+	DELL_DDV_BATTERY_RELATIVE_CHARGE_STATE	= 0x0B,
+	DELL_DDV_BATTERY_CYCLE_COUNT		= 0x0C,
+	DELL_DDV_BATTERY_EPPID			= 0x0D,
+	DELL_DDV_BATTERY_RAW_ANALYTICS_START	= 0x0E,
+	DELL_DDV_BATTERY_RAW_ANALYTICS		= 0x0F,
+	DELL_DDV_BATTERY_DESIGN_VOLTAGE		= 0x10,
+
+	DELL_DDV_INTERFACE_VERSION		= 0x12,
+
+	DELL_DDV_FAN_SENSOR_INFORMATION		= 0x20,
+	DELL_DDV_THERMAL_SENSOR_INFORMATION	= 0x22,
+};
+
+struct dell_wmi_ddv_data {
+	struct device_attribute temp_attr, eppid_attr;
+	struct wmi_device *wdev;
+};
+
+static int dell_wmi_ddv_query_type(struct wmi_device *wdev, enum dell_ddv_method method, u32 arg,
+				   union acpi_object **result, acpi_object_type type)
+{
+	struct acpi_buffer out = { ACPI_ALLOCATE_BUFFER, NULL };
+	const struct acpi_buffer in = {
+		.length = sizeof(arg),
+		.pointer = &arg,
+	};
+	union acpi_object *obj;
+	acpi_status ret;
+
+	ret = wmidev_evaluate_method(wdev, 0x0, method, &in, &out);
+	if (ACPI_FAILURE(ret))
+		return -EIO;
+
+	obj = out.pointer;
+	if (!obj)
+		return -ENODATA;
+
+	if (obj->type != type) {
+		kfree(obj);
+		return -EIO;
+	}
+
+	*result = obj;
+
+	return 0;
+}
+
+static int dell_wmi_ddv_query_integer(struct wmi_device *wdev, enum dell_ddv_method method,
+				      u32 arg, u32 *res)
+{
+	union acpi_object *obj;
+	int ret;
+
+	ret = dell_wmi_ddv_query_type(wdev, method, arg, &obj, ACPI_TYPE_INTEGER);
+	if (ret < 0)
+		return ret;
+
+	if (obj->integer.value <= U32_MAX)
+		*res = (u32)obj->integer.value;
+	else
+		ret = -ERANGE;
+
+	kfree(obj);
+
+	return ret;
+}
+
+static int dell_wmi_ddv_query_buffer(struct wmi_device *wdev, enum dell_ddv_method method,
+				     u32 arg, union acpi_object **result)
+{
+	union acpi_object *obj;
+	u64 buffer_size;
+	int ret;
+
+	ret = dell_wmi_ddv_query_type(wdev, method, arg, &obj, ACPI_TYPE_PACKAGE);
+	if (ret < 0)
+		return ret;
+
+	if (obj->package.count != 2)
+		goto err_free;
+
+	if (obj->package.elements[0].type != ACPI_TYPE_INTEGER)
+		goto err_free;
+
+	buffer_size = obj->package.elements[0].integer.value;
+
+	if (obj->package.elements[1].type != ACPI_TYPE_BUFFER)
+		goto err_free;
+
+	if (buffer_size != obj->package.elements[1].buffer.length) {
+		dev_warn(&wdev->dev,
+			 FW_WARN "ACPI buffer size (%llu) does not match WMI buffer size (%d)\n",
+			 buffer_size, obj->package.elements[1].buffer.length);
+
+		goto err_free;
+	}
+
+	*result = obj;
+
+	return 0;
+
+err_free:
+	kfree(obj);
+
+	return -EIO;
+}
+
+static int dell_wmi_ddv_query_string(struct wmi_device *wdev, enum dell_ddv_method method,
+				     u32 arg, union acpi_object **result)
+{
+	return dell_wmi_ddv_query_type(wdev, method, arg, result, ACPI_TYPE_STRING);
+}
+
+static int dell_wmi_ddv_battery_index(struct acpi_device *acpi_dev, u32 *index)
+{
+	const char *uid_str = acpi_device_uid(acpi_dev);
+
+	if (!uid_str)
+		return -ENODEV;
+
+	return kstrtou32(uid_str, 10, index);
+}
+
+static ssize_t temp_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct dell_wmi_ddv_data *data = container_of(attr, struct dell_wmi_ddv_data, temp_attr);
+	u32 index, value;
+	int ret;
+
+	ret = dell_wmi_ddv_battery_index(to_acpi_device(dev->parent), &index);
+	if (ret < 0)
+		return ret;
+
+	ret = dell_wmi_ddv_query_integer(data->wdev, DELL_DDV_BATTERY_TEMPERATURE, index, &value);
+	if (ret < 0)
+		return ret;
+
+	return sysfs_emit(buf, "%d\n", DIV_ROUND_CLOSEST(value, 10));
+}
+
+static ssize_t eppid_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct dell_wmi_ddv_data *data = container_of(attr, struct dell_wmi_ddv_data, eppid_attr);
+	union acpi_object *obj;
+	u32 index;
+	int ret;
+
+	ret = dell_wmi_ddv_battery_index(to_acpi_device(dev->parent), &index);
+	if (ret < 0)
+		return ret;
+
+	ret = dell_wmi_ddv_query_string(data->wdev, DELL_DDV_BATTERY_EPPID, index, &obj);
+	if (ret < 0)
+		return ret;
+
+	ret = sysfs_emit(buf, "%s\n", obj->string.pointer);
+
+	kfree(obj);
+
+	return ret;
+}
+
+static int dell_wmi_ddv_add_battery(void *drvdata, struct power_supply *battery)
+{
+	struct dell_wmi_ddv_data *data = drvdata;
+	u32 index;
+	int ret;
+
+	ret = dell_wmi_ddv_battery_index(to_acpi_device(battery->dev.parent), &index);
+	if (ret < 0)
+		return ret;
+
+	ret = device_create_file(&battery->dev, &data->temp_attr);
+	if (ret < 0)
+		return ret;
+
+	ret = device_create_file(&battery->dev, &data->eppid_attr);
+	if (ret < 0) {
+		device_remove_file(&battery->dev, &data->temp_attr);
+
+		return ret;
+	}
+
+	return 0;
+}
+
+static int dell_wmi_ddv_remove_battery(void *drvdata, struct power_supply *battery)
+{
+	struct dell_wmi_ddv_data *data = drvdata;
+
+	device_remove_file(&battery->dev, &data->temp_attr);
+	device_remove_file(&battery->dev, &data->eppid_attr);
+
+	return 0;
+}
+
+static const struct acpi_battery_hook_ops dell_wmi_ddv_battery_hook_ops = {
+	.add_battery = dell_wmi_ddv_add_battery,
+	.remove_battery = dell_wmi_ddv_remove_battery,
+};
+
+static void dell_wmi_ddv_battery_remove(void *data)
+{
+	struct acpi_battery_hook *hook = data;
+
+	battery_hook_unregister(hook);
+}
+
+static int dell_wmi_ddv_battery_add(struct dell_wmi_ddv_data *data)
+{
+	struct acpi_battery_hook *hook;
+
+	sysfs_attr_init(&data->temp_attr.attr);
+	data->temp_attr.attr.name = "temp";
+	data->temp_attr.attr.mode = 0444;
+	data->temp_attr.show = temp_show;
+
+	sysfs_attr_init(&data->eppid_attr.attr);
+	data->eppid_attr.attr.name = "eppid";
+	data->eppid_attr.attr.mode = 0444;
+	data->eppid_attr.show = eppid_show;
+
+	hook = battery_hook_register("DELL Battery Extension", data,
+				     &dell_wmi_ddv_battery_hook_ops);
+	if (IS_ERR(hook))
+		return PTR_ERR(hook);
+
+	return devm_add_action_or_reset(&data->wdev->dev, dell_wmi_ddv_battery_remove, hook);
+}
+
+static int dell_wmi_ddv_buffer_read(struct seq_file *seq, enum dell_ddv_method method)
+{
+	struct device *dev = seq->private;
+	struct dell_wmi_ddv_data *data = dev_get_drvdata(dev);
+	union acpi_object *obj;
+	union acpi_object buf;
+	int ret;
+
+	ret = dell_wmi_ddv_query_buffer(data->wdev, method, 0, &obj);
+	if (ret < 0)
+		return ret;
+
+	buf = obj->package.elements[1];
+	ret = seq_write(seq, buf.buffer.pointer, buf.buffer.length);
+	kfree(obj);
+
+	return ret;
+}
+
+static int dell_wmi_ddv_fan_read(struct seq_file *seq, void *offset)
+{
+	return dell_wmi_ddv_buffer_read(seq, DELL_DDV_FAN_SENSOR_INFORMATION);
+}
+
+static int dell_wmi_ddv_temp_read(struct seq_file *seq, void *offset)
+{
+	return dell_wmi_ddv_buffer_read(seq, DELL_DDV_THERMAL_SENSOR_INFORMATION);
+}
+
+static void dell_wmi_ddv_debugfs_remove(void *data)
+{
+	struct dentry *entry = data;
+
+	debugfs_remove(entry);
+}
+
+static void dell_wmi_ddv_debugfs_init(struct wmi_device *wdev)
+{
+	struct dentry *entry;
+	char name[64];
+
+	scnprintf(name, ARRAY_SIZE(name), "%s-%s", DRIVER_NAME, dev_name(&wdev->dev));
+	entry = debugfs_create_dir(name, NULL);
+
+	debugfs_create_devm_seqfile(&wdev->dev, "fan_sensor_information", entry,
+				    dell_wmi_ddv_fan_read);
+	debugfs_create_devm_seqfile(&wdev->dev, "thermal_sensor_information", entry,
+				    dell_wmi_ddv_temp_read);
+
+	devm_add_action_or_reset(&wdev->dev, dell_wmi_ddv_debugfs_remove, entry);
+}
+
+static int dell_wmi_ddv_probe(struct wmi_device *wdev, const void *context)
+{
+	struct dell_wmi_ddv_data *data;
+	u32 version;
+	int ret;
+
+	ret = dell_wmi_ddv_query_integer(wdev, DELL_DDV_INTERFACE_VERSION, 0, &version);
+	if (ret < 0)
+		return ret;
+
+	dev_dbg(&wdev->dev, "WMI interface version: %d\n", version);
+	if (version != DELL_DDV_SUPPORTED_INTERFACE)
+		return -ENODEV;
+
+	data = devm_kzalloc(&wdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	dev_set_drvdata(&wdev->dev, data);
+	data->wdev = wdev;
+
+	dell_wmi_ddv_debugfs_init(wdev);
+
+	return dell_wmi_ddv_battery_add(data);
+}
+
+static const struct wmi_device_id dell_wmi_ddv_id_table[] = {
+	{ DELL_DDV_GUID, NULL },
+	{ }
+};
+MODULE_DEVICE_TABLE(wmi, dell_wmi_ddv_id_table);
+
+static struct wmi_driver dell_wmi_ddv_driver = {
+	.driver = {
+		.name = DRIVER_NAME,
+	},
+	.id_table = dell_wmi_ddv_id_table,
+	.probe = dell_wmi_ddv_probe,
+};
+module_wmi_driver(dell_wmi_ddv_driver);
+
+MODULE_AUTHOR("Armin Wolf <W_Armin@gmx.de>");
+MODULE_DESCRIPTION("Dell WMI sensor driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index aff23309b5d3..f307d8c5c6c3 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -108,6 +108,7 @@ MODULE_DEVICE_TABLE(acpi, wmi_device_ids);
 /* allow duplicate GUIDs as these device drivers use struct wmi_driver */
 static const char * const allow_duplicates[] = {
 	"05901221-D566-11D1-B2F0-00A0C9062910",	/* wmi-bmof */
+	"8A42EA14-4F2A-FD45-6422-0087F7A7E608",	/* dell-wmi-ddv */
 	NULL
 };

--
2.30.2

Re: [PATCH 5/5] platform/x86: dell: Add new dell-wmi-ddv driver

[Randy Dunlap]

Hi--

On 9/12/22 05:53, Armin Wolf wrote:
> diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
> index 25421e061c47..209e63e347e2 100644
> --- a/drivers/platform/x86/dell/Kconfig
> +++ b/drivers/platform/x86/dell/Kconfig
> @@ -189,6 +189,19 @@ config DELL_WMI_DESCRIPTOR
>  	default n
>  	depends on ACPI_WMI
> 
> +config DELL_WMI_DDV
> +	tristate "Dell WMI sensors Support"
> +	default m

You should (try to) justify default m, otherwise just
don't have a default for it.

> +	depends on ACPI_BATTERY
> +	depends on ACPI_WMI
> +	help
> +	  This option adds support for WMI-based sensors like
> +	  battery temperature sensors found on some Dell notebooks.
> +	  It also supports reading of the batteries ePPID.
> +
> +	  To compile this drivers as a module, choose M here: the module will
> +	  be called dell-wmi-ddv.

-- 
~Randy

Re: [PATCH 5/5] platform/x86: dell: Add new dell-wmi-ddv driver

[Armin Wolf]

Am 12.09.22 um 23:56 schrieb Randy Dunlap:

> Hi--
>
> On 9/12/22 05:53, Armin Wolf wrote:
>> diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
>> index 25421e061c47..209e63e347e2 100644
>> --- a/drivers/platform/x86/dell/Kconfig
>> +++ b/drivers/platform/x86/dell/Kconfig
>> @@ -189,6 +189,19 @@ config DELL_WMI_DESCRIPTOR
>>   	default n
>>   	depends on ACPI_WMI
>>
>> +config DELL_WMI_DDV
>> +	tristate "Dell WMI sensors Support"
>> +	default m
> You should (try to) justify default m, otherwise just
> don't have a default for it.

I have chosen default m since many other Dell platform drivers are being
default m. Since this driver is not essential for normal operation,
i will drop default m then.

Armin Wolf

>> +	depends on ACPI_BATTERY
>> +	depends on ACPI_WMI
>> +	help
>> +	  This option adds support for WMI-based sensors like
>> +	  battery temperature sensors found on some Dell notebooks.
>> +	  It also supports reading of the batteries ePPID.
>> +
>> +	  To compile this drivers as a module, choose M here: the module will
>> +	  be called dell-wmi-ddv.

RE: [PATCH 5/5] platform/x86: dell: Add new dell-wmi-ddv driver

[Limonciello, Mario]

[Public]



> -----Original Message-----
> From: Armin Wolf <W_Armin@gmx.de>
> Sent: Tuesday, September 13, 2022 09:41
> To: Randy Dunlap <rdunlap@infradead.org>; hdegoede@redhat.com;
> markgross@kernel.org
> Cc: rafael@kernel.org; lenb@kernel.org; hmh@hmh.eng.br;
> matan@svgalib.org; corentin.chary@gmail.com; jeremy@system76.com;
> productdev@system76.com; platform-driver-x86@vger.kernel.org; linux-
> acpi@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 5/5] platform/x86: dell: Add new dell-wmi-ddv driver
> 
> Am 12.09.22 um 23:56 schrieb Randy Dunlap:
> 
> > Hi--
> >
> > On 9/12/22 05:53, Armin Wolf wrote:
> >> diff --git a/drivers/platform/x86/dell/Kconfig
> b/drivers/platform/x86/dell/Kconfig
> >> index 25421e061c47..209e63e347e2 100644
> >> --- a/drivers/platform/x86/dell/Kconfig
> >> +++ b/drivers/platform/x86/dell/Kconfig
> >> @@ -189,6 +189,19 @@ config DELL_WMI_DESCRIPTOR
> >>   	default n
> >>   	depends on ACPI_WMI
> >>
> >> +config DELL_WMI_DDV
> >> +	tristate "Dell WMI sensors Support"
> >> +	default m
> > You should (try to) justify default m, otherwise just
> > don't have a default for it.
> 
> I have chosen default m since many other Dell platform drivers are being
> default m. Since this driver is not essential for normal operation,
> i will drop default m then.

Actually Dell drivers directory are a bit unique in this regard.  There is a special
top level boolean.  I would suggest to keep it as is.

Take a look at:
menuconfig X86_PLATFORM_DRIVERS_DELL

> 
> Armin Wolf
> 
> >> +	depends on ACPI_BATTERY
> >> +	depends on ACPI_WMI
> >> +	help
> >> +	  This option adds support for WMI-based sensors like
> >> +	  battery temperature sensors found on some Dell notebooks.
> >> +	  It also supports reading of the batteries ePPID.
> >> +
> >> +	  To compile this drivers as a module, choose M here: the module will
> >> +	  be called dell-wmi-ddv.

Re: [PATCH 5/5] platform/x86: dell: Add new dell-wmi-ddv driver

[Armin Wolf]

Am 13.09.22 um 18:08 schrieb Limonciello, Mario:

> [Public]
>
>
>
>> -----Original Message-----
>> From: Armin Wolf <W_Armin@gmx.de>
>> Sent: Tuesday, September 13, 2022 09:41
>> To: Randy Dunlap <rdunlap@infradead.org>; hdegoede@redhat.com;
>> markgross@kernel.org
>> Cc: rafael@kernel.org; lenb@kernel.org; hmh@hmh.eng.br;
>> matan@svgalib.org; corentin.chary@gmail.com; jeremy@system76.com;
>> productdev@system76.com; platform-driver-x86@vger.kernel.org; linux-
>> acpi@vger.kernel.org; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH 5/5] platform/x86: dell: Add new dell-wmi-ddv driver
>>
>> Am 12.09.22 um 23:56 schrieb Randy Dunlap:
>>
>>> Hi--
>>>
>>> On 9/12/22 05:53, Armin Wolf wrote:
>>>> diff --git a/drivers/platform/x86/dell/Kconfig
>> b/drivers/platform/x86/dell/Kconfig
>>>> index 25421e061c47..209e63e347e2 100644
>>>> --- a/drivers/platform/x86/dell/Kconfig
>>>> +++ b/drivers/platform/x86/dell/Kconfig
>>>> @@ -189,6 +189,19 @@ config DELL_WMI_DESCRIPTOR
>>>>    	default n
>>>>    	depends on ACPI_WMI
>>>>
>>>> +config DELL_WMI_DDV
>>>> +	tristate "Dell WMI sensors Support"
>>>> +	default m
>>> You should (try to) justify default m, otherwise just
>>> don't have a default for it.
>> I have chosen default m since many other Dell platform drivers are being
>> default m. Since this driver is not essential for normal operation,
>> i will drop default m then.
> Actually Dell drivers directory are a bit unique in this regard.  There is a special
> top level boolean.  I would suggest to keep it as is.
>
> Take a look at:
> menuconfig X86_PLATFORM_DRIVERS_DELL

Ok.

Armin Wolf

>> Armin Wolf
>>
>>>> +	depends on ACPI_BATTERY
>>>> +	depends on ACPI_WMI
>>>> +	help
>>>> +	  This option adds support for WMI-based sensors like
>>>> +	  battery temperature sensors found on some Dell notebooks.
>>>> +	  It also supports reading of the batteries ePPID.
>>>> +
>>>> +	  To compile this drivers as a module, choose M here: the module will
>>>> +	  be called dell-wmi-ddv.

Re: [PATCH 5/5] platform/x86: dell: Add new dell-wmi-ddv driver

[Randy Dunlap]

On 9/13/22 09:08, Limonciello, Mario wrote:
> [Public]
> 
> 
> 
>> -----Original Message-----
>> From: Armin Wolf <W_Armin@gmx.de>
>> Sent: Tuesday, September 13, 2022 09:41
>> To: Randy Dunlap <rdunlap@infradead.org>; hdegoede@redhat.com;
>> markgross@kernel.org
>> Cc: rafael@kernel.org; lenb@kernel.org; hmh@hmh.eng.br;
>> matan@svgalib.org; corentin.chary@gmail.com; jeremy@system76.com;
>> productdev@system76.com; platform-driver-x86@vger.kernel.org; linux-
>> acpi@vger.kernel.org; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH 5/5] platform/x86: dell: Add new dell-wmi-ddv driver
>>
>> Am 12.09.22 um 23:56 schrieb Randy Dunlap:
>>
>>> Hi--
>>>
>>> On 9/12/22 05:53, Armin Wolf wrote:
>>>> diff --git a/drivers/platform/x86/dell/Kconfig
>> b/drivers/platform/x86/dell/Kconfig
>>>> index 25421e061c47..209e63e347e2 100644
>>>> --- a/drivers/platform/x86/dell/Kconfig
>>>> +++ b/drivers/platform/x86/dell/Kconfig
>>>> @@ -189,6 +189,19 @@ config DELL_WMI_DESCRIPTOR
>>>>   	default n
>>>>   	depends on ACPI_WMI
>>>>
>>>> +config DELL_WMI_DDV
>>>> +	tristate "Dell WMI sensors Support"
>>>> +	default m
>>> You should (try to) justify default m, otherwise just
>>> don't have a default for it.
>>
>> I have chosen default m since many other Dell platform drivers are being
>> default m. Since this driver is not essential for normal operation,
>> i will drop default m then.
> 
> Actually Dell drivers directory are a bit unique in this regard.  There is a special
> top level boolean.  I would suggest to keep it as is.
> 
> Take a look at:
> menuconfig X86_PLATFORM_DRIVERS_DELL
> 

So all of those "default m" and "default y" drivers are *needed*
as opposed to desirable?

>>
>> Armin Wolf
>>
>>>> +	depends on ACPI_BATTERY
>>>> +	depends on ACPI_WMI
>>>> +	help
>>>> +	  This option adds support for WMI-based sensors like
>>>> +	  battery temperature sensors found on some Dell notebooks.
>>>> +	  It also supports reading of the batteries ePPID.
>>>> +
>>>> +	  To compile this drivers as a module, choose M here: the module will
>>>> +	  be called dell-wmi-ddv.

thanks.
-- 
~Randy

Re: [PATCH 5/5] platform/x86: dell: Add new dell-wmi-ddv driver

[Limonciello, Mario]

On 9/13/2022 13:27, Randy Dunlap wrote:
> 
> 
> On 9/13/22 09:08, Limonciello, Mario wrote:
>> [Public]
>>
>>
>>
>>> -----Original Message-----
>>> From: Armin Wolf <W_Armin@gmx.de>
>>> Sent: Tuesday, September 13, 2022 09:41
>>> To: Randy Dunlap <rdunlap@infradead.org>; hdegoede@redhat.com;
>>> markgross@kernel.org
>>> Cc: rafael@kernel.org; lenb@kernel.org; hmh@hmh.eng.br;
>>> matan@svgalib.org; corentin.chary@gmail.com; jeremy@system76.com;
>>> productdev@system76.com; platform-driver-x86@vger.kernel.org; linux-
>>> acpi@vger.kernel.org; linux-kernel@vger.kernel.org
>>> Subject: Re: [PATCH 5/5] platform/x86: dell: Add new dell-wmi-ddv driver
>>>
>>> Am 12.09.22 um 23:56 schrieb Randy Dunlap:
>>>
>>>> Hi--
>>>>
>>>> On 9/12/22 05:53, Armin Wolf wrote:
>>>>> diff --git a/drivers/platform/x86/dell/Kconfig
>>> b/drivers/platform/x86/dell/Kconfig
>>>>> index 25421e061c47..209e63e347e2 100644
>>>>> --- a/drivers/platform/x86/dell/Kconfig
>>>>> +++ b/drivers/platform/x86/dell/Kconfig
>>>>> @@ -189,6 +189,19 @@ config DELL_WMI_DESCRIPTOR
>>>>>    	default n
>>>>>    	depends on ACPI_WMI
>>>>>
>>>>> +config DELL_WMI_DDV
>>>>> +	tristate "Dell WMI sensors Support"
>>>>> +	default m
>>>> You should (try to) justify default m, otherwise just
>>>> don't have a default for it.
>>>
>>> I have chosen default m since many other Dell platform drivers are being
>>> default m. Since this driver is not essential for normal operation,
>>> i will drop default m then.
>>
>> Actually Dell drivers directory are a bit unique in this regard.  There is a special
>> top level boolean.  I would suggest to keep it as is.
>>
>> Take a look at:
>> menuconfig X86_PLATFORM_DRIVERS_DELL
>>
> 
> So all of those "default m" and "default y" drivers are *needed*
> as opposed to desirable?
> 

It was supposed to be a convenience option, it's first introduced in 
f1e1ea516721d1.

So if you have a Dell laptop you set the one option and then get 
defaults for all those modules.

>>>
>>> Armin Wolf
>>>
>>>>> +	depends on ACPI_BATTERY
>>>>> +	depends on ACPI_WMI
>>>>> +	help
>>>>> +	  This option adds support for WMI-based sensors like
>>>>> +	  battery temperature sensors found on some Dell notebooks.
>>>>> +	  It also supports reading of the batteries ePPID.
>>>>> +
>>>>> +	  To compile this drivers as a module, choose M here: the module will
>>>>> +	  be called dell-wmi-ddv.
> 
> thanks.

Re: [PATCH 5/5] platform/x86: dell: Add new dell-wmi-ddv driver

[Randy Dunlap]

On 9/13/22 11:30, Limonciello, Mario wrote:
> On 9/13/2022 13:27, Randy Dunlap wrote:
>>
>>
>> On 9/13/22 09:08, Limonciello, Mario wrote:
>>> [Public]
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Armin Wolf <W_Armin@gmx.de>
>>>> Sent: Tuesday, September 13, 2022 09:41
>>>> To: Randy Dunlap <rdunlap@infradead.org>; hdegoede@redhat.com;
>>>> markgross@kernel.org
>>>> Cc: rafael@kernel.org; lenb@kernel.org; hmh@hmh.eng.br;
>>>> matan@svgalib.org; corentin.chary@gmail.com; jeremy@system76.com;
>>>> productdev@system76.com; platform-driver-x86@vger.kernel.org; linux-
>>>> acpi@vger.kernel.org; linux-kernel@vger.kernel.org
>>>> Subject: Re: [PATCH 5/5] platform/x86: dell: Add new dell-wmi-ddv driver
>>>>
>>>> Am 12.09.22 um 23:56 schrieb Randy Dunlap:
>>>>
>>>>> Hi--
>>>>>
>>>>> On 9/12/22 05:53, Armin Wolf wrote:
>>>>>> diff --git a/drivers/platform/x86/dell/Kconfig
>>>> b/drivers/platform/x86/dell/Kconfig
>>>>>> index 25421e061c47..209e63e347e2 100644
>>>>>> --- a/drivers/platform/x86/dell/Kconfig
>>>>>> +++ b/drivers/platform/x86/dell/Kconfig
>>>>>> @@ -189,6 +189,19 @@ config DELL_WMI_DESCRIPTOR
>>>>>>        default n
>>>>>>        depends on ACPI_WMI
>>>>>>
>>>>>> +config DELL_WMI_DDV
>>>>>> +    tristate "Dell WMI sensors Support"
>>>>>> +    default m
>>>>> You should (try to) justify default m, otherwise just
>>>>> don't have a default for it.
>>>>
>>>> I have chosen default m since many other Dell platform drivers are being
>>>> default m. Since this driver is not essential for normal operation,
>>>> i will drop default m then.
>>>
>>> Actually Dell drivers directory are a bit unique in this regard.  There is a special
>>> top level boolean.  I would suggest to keep it as is.
>>>
>>> Take a look at:
>>> menuconfig X86_PLATFORM_DRIVERS_DELL
>>>
>>
>> So all of those "default m" and "default y" drivers are *needed*
>> as opposed to desirable?
>>
> 
> It was supposed to be a convenience option, it's first introduced in f1e1ea516721d1.
> 
> So if you have a Dell laptop you set the one option and then get defaults for all those modules.

oh well. whatever.

thanks.

>>>>
>>>> Armin Wolf
>>>>
>>>>>> +    depends on ACPI_BATTERY
>>>>>> +    depends on ACPI_WMI
>>>>>> +    help
>>>>>> +      This option adds support for WMI-based sensors like
>>>>>> +      battery temperature sensors found on some Dell notebooks.
>>>>>> +      It also supports reading of the batteries ePPID.
>>>>>> +
>>>>>> +      To compile this drivers as a module, choose M here: the module will
>>>>>> +      be called dell-wmi-ddv.
>>
>> thanks.
> 

-- 
~Randy

Re: [PATCH 5/5] platform/x86: dell: Add new dell-wmi-ddv driver

[Hans de Goede]

Hi,

On 9/12/22 13:53, Armin Wolf wrote:
> The dell-wmi-ddv driver adds support for reading
> the current temperature and ePPID of ACPI batteries
> on supported Dell machines.
> 
> Since the WMI interface used by this driver does not
> do any input validation and thus cannot be used for probing,
> the driver depends on the ACPI battery extension machanism
> to discover batteries.
> 
> The driver also supports a debugfs interface for retrieving
> buffers containing fan and thermal sensor information.
> Since the meaing of the content of those buffers is currently
> unknown, the interface is meant for reverse-engineering and
> will likely be replaced with an hwmon interface once the
> meaning has been understood.
> 
> The driver was tested on a Dell Inspiron 3505.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  .../ABI/testing/debugfs-dell-wmi-ddv          |  21 +
>  .../ABI/testing/sysfs-platform-dell-wmi-ddv   |  16 +
>  MAINTAINERS                                   |   7 +
>  drivers/platform/x86/dell/Kconfig             |  13 +
>  drivers/platform/x86/dell/Makefile            |   1 +
>  drivers/platform/x86/dell/dell-wmi-ddv.c      | 365 ++++++++++++++++++
>  drivers/platform/x86/wmi.c                    |   1 +
>  7 files changed, 424 insertions(+)
>  create mode 100644 Documentation/ABI/testing/debugfs-dell-wmi-ddv
>  create mode 100644 Documentation/ABI/testing/sysfs-platform-dell-wmi-ddv
>  create mode 100644 drivers/platform/x86/dell/dell-wmi-ddv.c
> 
> diff --git a/Documentation/ABI/testing/debugfs-dell-wmi-ddv b/Documentation/ABI/testing/debugfs-dell-wmi-ddv
> new file mode 100644
> index 000000000000..fbcc5d6f7388
> --- /dev/null
> +++ b/Documentation/ABI/testing/debugfs-dell-wmi-ddv
> @@ -0,0 +1,21 @@
> +What:		/sys/kernel/debug/dell-wmi-ddv-<wmi_device_name>/fan_sensor_information
> +Date:		September 2022
> +KernelVersion:	6.1
> +Contact:	Armin Wolf <W_Armin@gmx.de>
> +Description:
> +		This file contains the contents of the fan sensor information buffer,
> +		which contains fan sensor entries and a terminating character (0xFF).
> +
> +		Each fan sensor entry consists of three bytes with an unknown meaning,
> +		interested people may use this file for reverse-engineering.
> +
> +What:		/sys/kernel/debug/dell-wmi-ddv-<wmi_device_name>/thermal_sensor_information
> +Date:		September 2022
> +KernelVersion:	6.1
> +Contact:	Armin Wolf <W_Armin@gmx.de>
> +Description:
> +		This file contains the contents of the thermal sensor information buffer,
> +		which contains thermal sensor entries and a terminating character (0xFF).
> +
> +		Each thermal sensor entry consists of five bytes with an unknown meaning,
> +		interested people may use this file for reverse-engineering.
> diff --git a/Documentation/ABI/testing/sysfs-platform-dell-wmi-ddv b/Documentation/ABI/testing/sysfs-platform-dell-wmi-ddv
> new file mode 100644
> index 000000000000..98e0b8399d13
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-platform-dell-wmi-ddv
> @@ -0,0 +1,16 @@
> +What:		/sys/class/power_supply/<battery_name>/temp
> +Date:		September 2022
> +KernelVersion:	6.1
> +Contact:	Armin Wolf <W_Armin@gmx.de>
> +Description:
> +		Reports the current ACPI battery temperature on supported Dell machines.
> +
> +		Values are represented in 1/10 Degrees Celsius.

This is a standard power_supply class attribute which is already documented in:
Documentation/ABI/testing/sysfs-class-power

so no need to document this here, please drop it.

Other then that this patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> +
> +What:		/sys/class/power_supply/<battery_name>/eppid
> +Date:		September 2022
> +KernelVersion:	6.1
> +Contact:	Armin Wolf <W_Armin@gmx.de>
> +Description:
> +		Reports the Dell ePPID (electronic Dell Piece Part Identification)
> +		of the ACPI battery.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6bb894ea4a77..d9fd4c9eebbc 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5821,6 +5821,13 @@ L:	Dell.Client.Kernel@dell.com
>  S:	Maintained
>  F:	drivers/platform/x86/dell/dell-wmi-descriptor.c
> 
> +DELL WMI DDV DRIVER
> +M:	Armin Wolf <W_Armin@gmx.de>
> +S:	Maintained
> +F:	Documentation/ABI/testing/debugfs-dell-wmi-ddv
> +F:	Documentation/ABI/testing/sysfs-platform-dell-wmi-ddv
> +F:	drivers/platform/x86/dell/dell-wmi-ddv.c
> +
>  DELL WMI SYSMAN DRIVER
>  M:	Divya Bharathi <divya.bharathi@dell.com>
>  M:	Prasanth Ksr <prasanth.ksr@dell.com>
> diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
> index 25421e061c47..209e63e347e2 100644
> --- a/drivers/platform/x86/dell/Kconfig
> +++ b/drivers/platform/x86/dell/Kconfig
> @@ -189,6 +189,19 @@ config DELL_WMI_DESCRIPTOR
>  	default n
>  	depends on ACPI_WMI
> 
> +config DELL_WMI_DDV
> +	tristate "Dell WMI sensors Support"
> +	default m
> +	depends on ACPI_BATTERY
> +	depends on ACPI_WMI
> +	help
> +	  This option adds support for WMI-based sensors like
> +	  battery temperature sensors found on some Dell notebooks.
> +	  It also supports reading of the batteries ePPID.
> +
> +	  To compile this drivers as a module, choose M here: the module will
> +	  be called dell-wmi-ddv.
> +
>  config DELL_WMI_LED
>  	tristate "External LED on Dell Business Netbooks"
>  	default m
> diff --git a/drivers/platform/x86/dell/Makefile b/drivers/platform/x86/dell/Makefile
> index ddba1df71e80..1b8942426622 100644
> --- a/drivers/platform/x86/dell/Makefile
> +++ b/drivers/platform/x86/dell/Makefile
> @@ -19,5 +19,6 @@ dell-wmi-objs				:= dell-wmi-base.o
>  dell-wmi-$(CONFIG_DELL_WMI_PRIVACY)	+= dell-wmi-privacy.o
>  obj-$(CONFIG_DELL_WMI_AIO)		+= dell-wmi-aio.o
>  obj-$(CONFIG_DELL_WMI_DESCRIPTOR)	+= dell-wmi-descriptor.o
> +obj-$(CONFIG_DELL_WMI_DDV)		+= dell-wmi-ddv.o
>  obj-$(CONFIG_DELL_WMI_LED)		+= dell-wmi-led.o
>  obj-$(CONFIG_DELL_WMI_SYSMAN)		+= dell-wmi-sysman/
> diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
> new file mode 100644
> index 000000000000..6eef298f13eb
> --- /dev/null
> +++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
> @@ -0,0 +1,365 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * dell-wmi-ddv.c -- Linux driver for WMI sensor information on Dell notebooks.
> + *
> + * Copyright (C) 2022 Armin Wolf <W_Armin@gmx.de>
> + */
> +
> +#define pr_format(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <acpi/battery.h>
> +#include <linux/acpi.h>
> +#include <linux/debugfs.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/kstrtox.h>
> +#include <linux/math.h>
> +#include <linux/module.h>
> +#include <linux/limits.h>
> +#include <linux/power_supply.h>
> +#include <linux/seq_file.h>
> +#include <linux/sysfs.h>
> +#include <linux/wmi.h>
> +
> +#define DRIVER_NAME	"dell-wmi-ddv"
> +
> +#define DELL_DDV_SUPPORTED_INTERFACE 2
> +#define DELL_DDV_GUID	"8A42EA14-4F2A-FD45-6422-0087F7A7E608"
> +
> +enum dell_ddv_method {
> +	DELL_DDV_BATTERY_DESIGN_CAPACITY	= 0x01,
> +	DELL_DDV_BATTERY_FULL_CHARGE_CAPACITY	= 0x02,
> +	DELL_DDV_BATTERY_MANUFACTURE_NAME	= 0x03,
> +	DELL_DDV_BATTERY_MANUFACTURE_DATE	= 0x04,
> +	DELL_DDV_BATTERY_SERIAL_NUMBER		= 0x05,
> +	DELL_DDV_BATTERY_CHEMISTRY_VALUE	= 0x06,
> +	DELL_DDV_BATTERY_TEMPERATURE		= 0x07,
> +	DELL_DDV_BATTERY_CURRENT		= 0x08,
> +	DELL_DDV_BATTERY_VOLTAGE		= 0x09,
> +	DELL_DDV_BATTERY_MANUFACTURER_ACCESS	= 0x0A,
> +	DELL_DDV_BATTERY_RELATIVE_CHARGE_STATE	= 0x0B,
> +	DELL_DDV_BATTERY_CYCLE_COUNT		= 0x0C,
> +	DELL_DDV_BATTERY_EPPID			= 0x0D,
> +	DELL_DDV_BATTERY_RAW_ANALYTICS_START	= 0x0E,
> +	DELL_DDV_BATTERY_RAW_ANALYTICS		= 0x0F,
> +	DELL_DDV_BATTERY_DESIGN_VOLTAGE		= 0x10,
> +
> +	DELL_DDV_INTERFACE_VERSION		= 0x12,
> +
> +	DELL_DDV_FAN_SENSOR_INFORMATION		= 0x20,
> +	DELL_DDV_THERMAL_SENSOR_INFORMATION	= 0x22,
> +};
> +
> +struct dell_wmi_ddv_data {
> +	struct device_attribute temp_attr, eppid_attr;
> +	struct wmi_device *wdev;
> +};
> +
> +static int dell_wmi_ddv_query_type(struct wmi_device *wdev, enum dell_ddv_method method, u32 arg,
> +				   union acpi_object **result, acpi_object_type type)
> +{
> +	struct acpi_buffer out = { ACPI_ALLOCATE_BUFFER, NULL };
> +	const struct acpi_buffer in = {
> +		.length = sizeof(arg),
> +		.pointer = &arg,
> +	};
> +	union acpi_object *obj;
> +	acpi_status ret;
> +
> +	ret = wmidev_evaluate_method(wdev, 0x0, method, &in, &out);
> +	if (ACPI_FAILURE(ret))
> +		return -EIO;
> +
> +	obj = out.pointer;
> +	if (!obj)
> +		return -ENODATA;
> +
> +	if (obj->type != type) {
> +		kfree(obj);
> +		return -EIO;
> +	}
> +
> +	*result = obj;
> +
> +	return 0;
> +}
> +
> +static int dell_wmi_ddv_query_integer(struct wmi_device *wdev, enum dell_ddv_method method,
> +				      u32 arg, u32 *res)
> +{
> +	union acpi_object *obj;
> +	int ret;
> +
> +	ret = dell_wmi_ddv_query_type(wdev, method, arg, &obj, ACPI_TYPE_INTEGER);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (obj->integer.value <= U32_MAX)
> +		*res = (u32)obj->integer.value;
> +	else
> +		ret = -ERANGE;
> +
> +	kfree(obj);
> +
> +	return ret;
> +}
> +
> +static int dell_wmi_ddv_query_buffer(struct wmi_device *wdev, enum dell_ddv_method method,
> +				     u32 arg, union acpi_object **result)
> +{
> +	union acpi_object *obj;
> +	u64 buffer_size;
> +	int ret;
> +
> +	ret = dell_wmi_ddv_query_type(wdev, method, arg, &obj, ACPI_TYPE_PACKAGE);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (obj->package.count != 2)
> +		goto err_free;
> +
> +	if (obj->package.elements[0].type != ACPI_TYPE_INTEGER)
> +		goto err_free;
> +
> +	buffer_size = obj->package.elements[0].integer.value;
> +
> +	if (obj->package.elements[1].type != ACPI_TYPE_BUFFER)
> +		goto err_free;
> +
> +	if (buffer_size != obj->package.elements[1].buffer.length) {
> +		dev_warn(&wdev->dev,
> +			 FW_WARN "ACPI buffer size (%llu) does not match WMI buffer size (%d)\n",
> +			 buffer_size, obj->package.elements[1].buffer.length);
> +
> +		goto err_free;
> +	}
> +
> +	*result = obj;
> +
> +	return 0;
> +
> +err_free:
> +	kfree(obj);
> +
> +	return -EIO;
> +}
> +
> +static int dell_wmi_ddv_query_string(struct wmi_device *wdev, enum dell_ddv_method method,
> +				     u32 arg, union acpi_object **result)
> +{
> +	return dell_wmi_ddv_query_type(wdev, method, arg, result, ACPI_TYPE_STRING);
> +}
> +
> +static int dell_wmi_ddv_battery_index(struct acpi_device *acpi_dev, u32 *index)
> +{
> +	const char *uid_str = acpi_device_uid(acpi_dev);
> +
> +	if (!uid_str)
> +		return -ENODEV;
> +
> +	return kstrtou32(uid_str, 10, index);
> +}
> +
> +static ssize_t temp_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct dell_wmi_ddv_data *data = container_of(attr, struct dell_wmi_ddv_data, temp_attr);
> +	u32 index, value;
> +	int ret;
> +
> +	ret = dell_wmi_ddv_battery_index(to_acpi_device(dev->parent), &index);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = dell_wmi_ddv_query_integer(data->wdev, DELL_DDV_BATTERY_TEMPERATURE, index, &value);
> +	if (ret < 0)
> +		return ret;
> +
> +	return sysfs_emit(buf, "%d\n", DIV_ROUND_CLOSEST(value, 10));
> +}
> +
> +static ssize_t eppid_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct dell_wmi_ddv_data *data = container_of(attr, struct dell_wmi_ddv_data, eppid_attr);
> +	union acpi_object *obj;
> +	u32 index;
> +	int ret;
> +
> +	ret = dell_wmi_ddv_battery_index(to_acpi_device(dev->parent), &index);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = dell_wmi_ddv_query_string(data->wdev, DELL_DDV_BATTERY_EPPID, index, &obj);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = sysfs_emit(buf, "%s\n", obj->string.pointer);
> +
> +	kfree(obj);
> +
> +	return ret;
> +}
> +
> +static int dell_wmi_ddv_add_battery(void *drvdata, struct power_supply *battery)
> +{
> +	struct dell_wmi_ddv_data *data = drvdata;
> +	u32 index;
> +	int ret;
> +
> +	ret = dell_wmi_ddv_battery_index(to_acpi_device(battery->dev.parent), &index);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = device_create_file(&battery->dev, &data->temp_attr);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = device_create_file(&battery->dev, &data->eppid_attr);
> +	if (ret < 0) {
> +		device_remove_file(&battery->dev, &data->temp_attr);
> +
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int dell_wmi_ddv_remove_battery(void *drvdata, struct power_supply *battery)
> +{
> +	struct dell_wmi_ddv_data *data = drvdata;
> +
> +	device_remove_file(&battery->dev, &data->temp_attr);
> +	device_remove_file(&battery->dev, &data->eppid_attr);
> +
> +	return 0;
> +}
> +
> +static const struct acpi_battery_hook_ops dell_wmi_ddv_battery_hook_ops = {
> +	.add_battery = dell_wmi_ddv_add_battery,
> +	.remove_battery = dell_wmi_ddv_remove_battery,
> +};
> +
> +static void dell_wmi_ddv_battery_remove(void *data)
> +{
> +	struct acpi_battery_hook *hook = data;
> +
> +	battery_hook_unregister(hook);
> +}
> +
> +static int dell_wmi_ddv_battery_add(struct dell_wmi_ddv_data *data)
> +{
> +	struct acpi_battery_hook *hook;
> +
> +	sysfs_attr_init(&data->temp_attr.attr);
> +	data->temp_attr.attr.name = "temp";
> +	data->temp_attr.attr.mode = 0444;
> +	data->temp_attr.show = temp_show;
> +
> +	sysfs_attr_init(&data->eppid_attr.attr);
> +	data->eppid_attr.attr.name = "eppid";
> +	data->eppid_attr.attr.mode = 0444;
> +	data->eppid_attr.show = eppid_show;
> +
> +	hook = battery_hook_register("DELL Battery Extension", data,
> +				     &dell_wmi_ddv_battery_hook_ops);
> +	if (IS_ERR(hook))
> +		return PTR_ERR(hook);
> +
> +	return devm_add_action_or_reset(&data->wdev->dev, dell_wmi_ddv_battery_remove, hook);
> +}
> +
> +static int dell_wmi_ddv_buffer_read(struct seq_file *seq, enum dell_ddv_method method)
> +{
> +	struct device *dev = seq->private;
> +	struct dell_wmi_ddv_data *data = dev_get_drvdata(dev);
> +	union acpi_object *obj;
> +	union acpi_object buf;
> +	int ret;
> +
> +	ret = dell_wmi_ddv_query_buffer(data->wdev, method, 0, &obj);
> +	if (ret < 0)
> +		return ret;
> +
> +	buf = obj->package.elements[1];
> +	ret = seq_write(seq, buf.buffer.pointer, buf.buffer.length);
> +	kfree(obj);
> +
> +	return ret;
> +}
> +
> +static int dell_wmi_ddv_fan_read(struct seq_file *seq, void *offset)
> +{
> +	return dell_wmi_ddv_buffer_read(seq, DELL_DDV_FAN_SENSOR_INFORMATION);
> +}
> +
> +static int dell_wmi_ddv_temp_read(struct seq_file *seq, void *offset)
> +{
> +	return dell_wmi_ddv_buffer_read(seq, DELL_DDV_THERMAL_SENSOR_INFORMATION);
> +}
> +
> +static void dell_wmi_ddv_debugfs_remove(void *data)
> +{
> +	struct dentry *entry = data;
> +
> +	debugfs_remove(entry);
> +}
> +
> +static void dell_wmi_ddv_debugfs_init(struct wmi_device *wdev)
> +{
> +	struct dentry *entry;
> +	char name[64];
> +
> +	scnprintf(name, ARRAY_SIZE(name), "%s-%s", DRIVER_NAME, dev_name(&wdev->dev));
> +	entry = debugfs_create_dir(name, NULL);
> +
> +	debugfs_create_devm_seqfile(&wdev->dev, "fan_sensor_information", entry,
> +				    dell_wmi_ddv_fan_read);
> +	debugfs_create_devm_seqfile(&wdev->dev, "thermal_sensor_information", entry,
> +				    dell_wmi_ddv_temp_read);
> +
> +	devm_add_action_or_reset(&wdev->dev, dell_wmi_ddv_debugfs_remove, entry);
> +}
> +
> +static int dell_wmi_ddv_probe(struct wmi_device *wdev, const void *context)
> +{
> +	struct dell_wmi_ddv_data *data;
> +	u32 version;
> +	int ret;
> +
> +	ret = dell_wmi_ddv_query_integer(wdev, DELL_DDV_INTERFACE_VERSION, 0, &version);
> +	if (ret < 0)
> +		return ret;
> +
> +	dev_dbg(&wdev->dev, "WMI interface version: %d\n", version);
> +	if (version != DELL_DDV_SUPPORTED_INTERFACE)
> +		return -ENODEV;
> +
> +	data = devm_kzalloc(&wdev->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(&wdev->dev, data);
> +	data->wdev = wdev;
> +
> +	dell_wmi_ddv_debugfs_init(wdev);
> +
> +	return dell_wmi_ddv_battery_add(data);
> +}
> +
> +static const struct wmi_device_id dell_wmi_ddv_id_table[] = {
> +	{ DELL_DDV_GUID, NULL },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(wmi, dell_wmi_ddv_id_table);
> +
> +static struct wmi_driver dell_wmi_ddv_driver = {
> +	.driver = {
> +		.name = DRIVER_NAME,
> +	},
> +	.id_table = dell_wmi_ddv_id_table,
> +	.probe = dell_wmi_ddv_probe,
> +};
> +module_wmi_driver(dell_wmi_ddv_driver);
> +
> +MODULE_AUTHOR("Armin Wolf <W_Armin@gmx.de>");
> +MODULE_DESCRIPTION("Dell WMI sensor driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> index aff23309b5d3..f307d8c5c6c3 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -108,6 +108,7 @@ MODULE_DEVICE_TABLE(acpi, wmi_device_ids);
>  /* allow duplicate GUIDs as these device drivers use struct wmi_driver */
>  static const char * const allow_duplicates[] = {
>  	"05901221-D566-11D1-B2F0-00A0C9062910",	/* wmi-bmof */
> +	"8A42EA14-4F2A-FD45-6422-0087F7A7E608",	/* dell-wmi-ddv */
>  	NULL
>  };
> 
> --
> 2.30.2
>