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

From: Sebastian Reichel <sebastian.reichel@collabora.com>
To: Nikita Travkin <nikita@trvn.ru>
Cc: Rob Herring <robh+dt@kernel.org>,
	 Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	 cros-qcom-dts-watchers@chromium.org,
	Andy Gross <agross@kernel.org>,
	 Bjorn Andersson <andersson@kernel.org>,
	Konrad Dybcio <konrad.dybcio@linaro.org>,
	 Rob Herring <robh@kernel.org>,
	linux-pm@vger.kernel.org, devicetree@vger.kernel.org,
	 linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH v3 2/3] power: supply: Add Acer Aspire 1 embedded controller driver
Date: Thu, 22 Feb 2024 00:41:34 +0100	[thread overview]
Message-ID: <qoidm5wujjbeoc2hlraky26wuwmuaxi2atyl6ehovhvffdbfeh@g5gunqdei45m> (raw)
In-Reply-To: <20240220-aspire1-ec-v3-2-02cb139a4931@trvn.ru>

[-- Attachment #1: Type: text/plain, Size: 2058 bytes --]

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.

[...]

>  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?

> [...]

> +	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?

> [...]

Otherwise the power-supply bits LGTM.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]