Date: Sun, 28 Jan 2024 11:22:25 +0500
From: Nikita Travkin <nikita@...n.ru>
To: Rob Herring <robh@...nel.org>
Cc: Sebastian Reichel <sre@...nel.org>, Krzysztof Kozlowski
 <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>,
 cros-qcom-dts-watchers@...omium.org, Andy Gross <agross@...nel.org>, Bjorn
 Andersson <andersson@...nel.org>, Konrad Dybcio <konrad.dybcio@...aro.org>,
 linux-pm@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v2 1/3] dt-bindings: power: supply: Add Acer Aspire 1 EC

Nikita Travkin писал(а) 15.12.2023 10:29:
> 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@...n.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@...n.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.
> 
>> 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)
> 
> 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.
> 

Hi Rob, Conor,

I'd still appreciate hearing your opinion before proceeding with this.

Nikita

> Looking forward to your opinion,
> Nikita
> 
>> Rob

Powered by blists - more mailing lists