Re: [PATCH 3/4] power: supply: axp288_fuel_gauge: Unregister duplicate ACPI battery supply

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 31-03-17 11:11, Rafael J. Wysocki wrote:
> On Fri, Mar 31, 2017 at 11:08 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 31-03-17 11:05, Rafael J. Wysocki wrote:
>>>
>>> On Fri, Mar 31, 2017 at 11:01 AM, Hans de Goede <hdegoede@redhat.com>
>>> wrote:
>>>
>>> [cut]
>>>
>>>>>> --- a/drivers/power/supply/axp288_fuel_gauge.c
>>>>>> +++ b/drivers/power/supply/axp288_fuel_gauge.c
>>>>>> @@ -26,6 +26,7 @@
>>>>>>  #include <linux/mfd/axp20x.h>
>>>>>>  #include <linux/platform_device.h>
>>>>>>  #include <linux/power_supply.h>
>>>>>> +#include <linux/power/acpi.h>
>>>>>>  #include <linux/iio/consumer.h>
>>>>>>  #include <linux/debugfs.h>
>>>>>>  #include <linux/seq_file.h>
>>>>>> @@ -754,6 +755,8 @@ static int axp288_fuel_gauge_probe(struct
>>>>>> platform_device *pdev)
>>>>>>                 return ret;
>>>>>>         }
>>>>>>
>>>>>> +       acpi_battery_unregister();
>>>>>> +
>>>>>
>>>>>
>>>>>
>>>>> What if the ACPI battery driver is loaded after this has been called
>>>>> already?
>>>>
>>>>
>>>>
>>>> The module exports that symbol so it must be loaded already.
>>>
>>>
>>> But then it may be unloaded manually and loaded again, may it not?
>>
>>
>> Only if you first unload axp288_fuel_gauge.ko otherwise it will
>> have a refcount > 0.
>
> OK
>
> Anyway, I'd prefer blacklists in the battery and ac drivers to be honest.

As I explained in my reply to the discussion around the first patch that
is somewhat hard to do and requires encoding knowledge in those drivers
which really does not belong there:

"The problem is that Intel re-uses HIDs between generations and
for the Whiskey Cove PMIC we want to not use the ACPI battery
and ac drivers on Cherry Trail (where they are known to be
broken) but things are different on Apollo Lake. Yet both
use the same HID for their companion Whiskey Cove PMIC even
though they are 2 completely different revisions of the PMIC
(e.g. one uses i2c the other does not).

The 2 native drivers have code to detect which revision they
are dealing with and exit with -ENODEV if it is not the
revision they were written for, but this means that simple
HID blacklisting will not work. So IMHO the decision to
unregister the  ACPI battery / ac interface really belongs
in the native driver as that has all the nitty gritty detail
needed to make that decision."

Regards,

Hans