Re: [PATCH v2 1/3] dt-bindings: power: supply: Add Acer Aspire 1 EC

From: Rob Herring <robh@kernel.org>
To: Nikita Travkin <nikita@trvn.ru>
Cc: Sebastian Reichel <sre@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>,
	linux-pm@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH v2 1/3] dt-bindings: power: supply: Add Acer Aspire 1 EC
Date: Thu, 22 Feb 2024 21:52:27 -0700	[thread overview]
Message-ID: <20240223045227.GA4017491-robh@kernel.org> (raw)
In-Reply-To: <207edefe4e8eac9679cd8966d28820cd@trvn.ru>

On Fri, Dec 15, 2023 at 10:29:22AM +0500, Nikita Travkin wrote:
> Rob Herring писал(а) 15.12.2023 03:02:
> > On Tue, Dec 12, 2023 at 05:49:09PM +0500, Nikita Travkin wrote:
> >> Add binding for the EC found in the Acer Aspire 1 laptop.
> >>
> >> Signed-off-by: Nikita Travkin <nikita@trvn.ru>
> >> ---
> >>  .../bindings/power/supply/acer,aspire1-ec.yaml     | 67 ++++++++++++++++++++++
> >>  1 file changed, 67 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/power/supply/acer,aspire1-ec.yaml b/Documentation/devicetree/bindings/power/supply/acer,aspire1-ec.yaml
> >> new file mode 100644
> >> index 000000000000..1fbf1272a00f
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/power/supply/acer,aspire1-ec.yaml
> >> @@ -0,0 +1,67 @@
> >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/power/supply/acer,aspire1-ec.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: Acer Aspire 1 Embedded Controller
> >> +
> >> +maintainers:
> >> +  - Nikita Travkin <nikita@trvn.ru>
> >> +
> >> +description:
> >> +  The Acer Aspire 1 laptop uses an embedded controller to control battery
> >> +  and charging as well as to provide a set of misc features such as the
> >> +  laptop lid status and HPD events for the USB Type-C DP alt mode.
> >> +
> >> +properties:
> >> +  compatible:
> >> +    const: acer,aspire1-ec
> >> +
> >> +  reg:
> >> +    const: 0x76
> >> +
> >> +  interrupts:
> >> +    maxItems: 1
> >> +
> >> +  acer,media-keys-on-top:
> >> +    description: Configure the keyboard layout to use media features of
> >> +      the fn row when the fn key is not pressed. The firmware may choose
> >> +      to add this property when user selects the fn mode in the firmware
> >> +      setup utility.
> >> +    type: boolean
> > 
> > Besides the naming, this isn't really a property of the EC, but really 
> > part of the keyboard layout. It seems you just stuck it here because 
> > this is part of the specific device.
> > 
> 
> The EC on this device is also a keyboard controller, but the keyboard
> part has a dedicated i2c bus with hid-over-i2c. Since this is the
> "management" bus of the same device, I decided that it fits here.

So there's also a hid-over-i2c DT node? Then why wouldn't you put this 
there?

> 
> > It is also hardly a feature unique to this device. I'm typing this from 
> > a device with the exact same thing (M1 Macbook Pro). Actually, all 3 
> > laptops I have in front of me have the same thing. The other 2 have 
> > a Fnlock (Fn+ESC) though.  On the M1, it's just a module param which I 
> > set as persistent. Though I now wonder if the Fnlock could be 
> > implemented on it too. Being able to switch whenever I want would be 
> > nice. That would probably have to be in Linux where as these other 
> > laptops probably implement this in their EC/firmware?
> > 
> > What I'm getting at is controlling changing this in firmware is not a 
> > great experience and this should all be common.
> > 
> 
> You may be right, however my goal here is to support the original
> firmware feature that is lost when we use DT.
> 
> This is a WoA laptop with UEFI/ACPI and, as usual for "Windows"
> machines, there is a setting in the firmware setup utility ("bios") to
> set the fn behavior. But it works by setting an ACPI value, and for
> Snapdragon devices we can't use that now.
> 
> Long term I want to have a EFI driver that would automatically
> detect/load DT and my plan is to handle things like this (and i.e. mac
> address, different touchpad vendor, etc) there. Thus I'm adding this
> property already, as an equivalent of that weird acpi bit that original
> firmware sets.
> 
> If we only provide a module param, the "intended by OEM" way of setting
> the fn mode will be broken, and one would need to know how to write a
> magic special config file to set a kernel module param. I think it's not
> the best UX. (and just adds to the silly "arm/dt bad, x86/uefi/acpi
> "just works"" argument many people sadly have)

But it always works, it is just a question of what is the default mode 
and I, as a user, want to decide that, not the OEM. And I want to change 
it at run-time, not reboot into BIOS to change it.

I wasn't suggesting you do a module param either. That's still specific 
to the module. Something like a sysfs file would be nice:

echo 1 >  /sys/class/input/input1/fnlock

> If you think I shouldn't use DT to pass this info, feel free to say so.
> I will drop this property and see if there is something else I can do
> to still support this without relying on Linux cooperation.

Not saying no to being in DT, but if it is, it should be a common 
property because it is a common thing on all laptops.

Rob