Re: [PATCH v3 2/3] power: supply: Add Acer Aspire 1 embedded controller driver

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

Hi,

+Ilpo (fellow pdx86 maintainer)

On 2/23/24 15:32, Nikita Travkin wrote:
> Sebastian Reichel писал(а) 22.02.2024 04:41:
>> Hi,
>>
>> On Tue, Feb 20, 2024 at 04:57:13PM +0500, Nikita Travkin wrote:
>>> Acer Aspire 1 is a Snapdragon 7c based laptop. It uses an embedded
>>> controller to control the charging and battery management, as well as to
>>> perform a set of misc functions.
>>>
>>> Unfortunately, while all this functionality is implemented in ACPI, it's
>>> currently not possible to use ACPI to boot Linux on such Qualcomm
>>> devices. To allow Linux to still support the features provided by EC,
>>> this driver reimplments the relevant ACPI parts. This allows us to boot
>>> the laptop with Device Tree and retain all the features.
>>>
>>> Signed-off-by: Nikita Travkin <nikita@trvn.ru>
>>> ---
>>>  drivers/power/supply/Kconfig           |  14 +
>>>  drivers/power/supply/Makefile          |   1 +
>>>  drivers/power/supply/acer-aspire1-ec.c | 453 +++++++++++++++++++++++++++++++++
>>
>> I think this belongs into drivers/platform, as it handles all bits of
>> the EC.
>>
> 
> Hm, I initially submitted it to power/supply following the c630 driver,
> but I think you're right... Though I'm not sure where in platform/ I'd
> put this driver... (+CC Hans)
> 
> Seems like most of the things live in platform/x86 but there is no i.e.
> platform/arm64...
> 
> Hans, (as a maintainer for most things in platform/) what do you think
> would be the best place to put this (and at least two more I'd expect)
> driver in inside platform/? And can we handle it through the
> platform-driver-x86 list?

I guess that adding a drivers/platform/aarch64 map for this makes
sense, with some comments in the Makefile and in the Kconfig
help explaining that this is for PC/laptop style EC drivers,
which combine multiple logical functions in one, only!

Assuming that we are only going to use this for such EC drivers,
using the platform-driver-x86 mailinglist for this makes sense
since that is where are the people are with knowledge of e.g.
userspace APIs for various typical EC functionalities.

It might even make sense to also use:

git://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git

As git tree for this and send pull-reqs to Linus for this
together with the other pdx86 for the same reasons.

I would be open to that as long as this is strictly limited to
EC (like) drivers.

Ilpo, what do you think about this ?

Regards,

Hans








> 
>> [...]
>>
>>>  3 files changed, 468 insertions(+)
>>>
>>> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
>>> index 3e31375491d5..e91a3acecb41 100644
>>> --- a/drivers/power/supply/Kconfig
>>> +++ b/drivers/power/supply/Kconfig
>>> @@ -985,4 +985,18 @@ config FUEL_GAUGE_MM8013
>>>  	  the state of charge, temperature, cycle count, actual and design
>>>  	  capacity, etc.
>>>
>>> +config EC_ACER_ASPIRE1
>>> +	tristate "Acer Aspire 1 Emedded Controller driver"
>>> +	depends on I2C
>>> +	depends on DRM
>>> +	help
>>> +	  Say Y here to enable the EC driver for the (Snapdragon-based)
>>> +	  Acer Aspire 1 laptop. The EC handles battery and charging
>>> +	  monitoring as well as some misc functions like the lid sensor
>>> +	  and USB Type-C DP HPD events.
>>> +
>>> +	  This driver provides battery and AC status support for the mentioned
>>
>> I did not see any AC status bits?
>>
> 
> I was referring to whatever ACPI spec calls "AC Adapter" but I guess
> I should have used the word "charger" instead... Will reword this.
> 
>>> [...]
>>
>>> +	case POWER_SUPPLY_PROP_PRESENT:
>>> +		val->intval = 1;
>>
>> You have an unused ASPIRE_EC_FG_FLAG_PRESENT, that looks like it
>> should be used here?
>>
> 
> Oh, you're right! I think I initially didn't have this property and
> added it like this as a reaction to that upower change that made it
> consider everything not explicitly present as absent.
> 
> I've just checked what is reported after unplugging the battery and
> seems like the flag (as well as everything else) is gone. Will change
> the driver to read the flag.
> 
> Thanks for your review!
> Nikita
> 
>>> [...]
>>
>> Otherwise the power-supply bits LGTM.
>>
>> -- Sebastian
>