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: Hans de Goede <hdegoede@redhat.com>,
	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,
	 platform-driver-x86@vger.kernel.org
Subject: Re: [PATCH v3 2/3] power: supply: Add Acer Aspire 1 embedded controller driver
Date: Fri, 23 Feb 2024 23:32:34 +0100	[thread overview]
Message-ID: <wr4awfmyip3fe5y4xpv6xajigsvc4qftawonkolrtsamr3pumb@aio2ishg3o7c> (raw)
In-Reply-To: <6e3fb1080c54cfc38dc3c3e79e32a53d@trvn.ru>

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

Hi,

On Fri, Feb 23, 2024 at 08:34:29PM +0500, Nikita Travkin wrote:
> Sebastian Reichel писал(а) 23.02.2024 20:04:
> > On Fri, Feb 23, 2024 at 07:32:17PM +0500, Nikita Travkin wrote:
> >> >> + 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.
> > 
> > But you only register a power-supply device for the battery and not
> > for the AC adapter/charger. When you write "and AC status support" I
> > would have expected something similar to this (that's from ACPI AC
> > adapter driver):
> > 
> > $ cat /sys/class/power_supply/AC/uevent
> > POWER_SUPPLY_NAME=AC
> > POWER_SUPPLY_TYPE=Mains
> > POWER_SUPPLY_ONLINE=1
> 
> Ah, I see... Yeah looking at it one more time, I mistakenly assumed the
> acpi ac code uses the same data fields as the battery but seems like it
> reads the single online flag from a different place. I don't think there
> is really a point on implementing that field since we can still easily
> track the battery charging/discharging status so I will probably omit it
> for now. Will reword the help text to not mention charger/ac adapter.

If you have the information easily available, it's a good plan to
expose it.

Without a charger reporting online status at least the kernel's
power_supply_is_system_supplied() will return false (which is e.g.
used by AMD GPU driver to select power profile).

Generic userspace (i.e. upower) probably behaves similar, since
battery status is not the same as AC connceted. A system might
not charge the battery but still run from AC itself.

-- Sebastian

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