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

[Nikita Travkin <nikita@trvn.ru>]

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?

> [...]
> 
>>  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