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

  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(+)
  

Hey,

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.

I'm not a huge fan of the property name/description here.
For the description, I'm not sure from reading this what the default
behaviour is and how the fn row works when the fn key is pressed.
Is the default behaviour that pressing the fn key enables the media keys
and the row by default provides the fn functionality? And then when this
option is set in firmware the behaviour is inverted?

For the name, the "on-top" bit seems a bit weird. I would prefer it to
be reworded in terms of the behavioural change of the fn key. The media
keys are physically at the top of the keyboard whether or not this
option is enabled, hence the "on-top" seeming a bit weird to me.

Thanks,
Conor.

> +    type: boolean
> +
> +  connector:
> +    $ref: /schemas/connector/usb-connector.yaml#
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |+
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        embedded-controller@76 {
> +            compatible = "acer,aspire1-ec";
> +            reg = <0x76>;
> +
> +            interrupts-extended = <&tlmm 30 IRQ_TYPE_LEVEL_LOW>;
> +
> +            connector {
> +                compatible = "usb-c-connector";
> +
> +                port {
> +                    ec_dp_in: endpoint {
> +                        remote-endpoint = <&mdss_dp_out>;
> +                    };
> +                };
> +            };
> +        };
> +    };
> 
> -- 
> 2.43.0
>
  

Conor Dooley писал(а) 12.12.2023 22:24:
> Hey,
> 
> 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.
> 
> I'm not a huge fan of the property name/description here.
> For the description, I'm not sure from reading this what the default
> behaviour is and how the fn row works when the fn key is pressed.
> Is the default behaviour that pressing the fn key enables the media keys
> and the row by default provides the fn functionality? And then when this
> option is set in firmware the behaviour is inverted?
> 

Yes. By default pressing i.e. F11 would create F11, Fn+F11 would be vol+.
With this option set, the default for F11 would be vol+ and Fn+f11 would
be F11.

Perhaps this description would be better?

  Configure the keyboard layout to invert the Fn key. By default the
  function row of the keyboard inputs function keys (i.e F11) when
  Fn is not pressed. With this option set, pressing the key without
  Fn would input media keys (i.e. Vol-Up). Firmware may (...) 

> For the name, the "on-top" bit seems a bit weird. I would prefer it to
> be reworded in terms of the behavioural change of the fn key. The media
> keys are physically at the top of the keyboard whether or not this
> option is enabled, hence the "on-top" seeming a bit weird to me.
> 

I was trying to be unambiguous with this name without making
it too long. The implied meaning of "on-top" was "On the top
keyboard layer" (in the opposition of the "Fn" layer). Perhaps
"acer,media-keys-on-top-layer" would have been better then?

I was trying to define this property via the word "media"
since using "fn" is somewhat ambiguous (fn row key vs F(1..12) key
vs Fn key), and something like `inverted-fn-key" would not reflect
what is the impact on the layout. Using "fn-selects-function-keys"
also felt a bit ambiguous to me.

FWIW I can also invert the default and make it
"fn-selects-media-keys" which would be clear. I haven't yet
written the firmware that would pass this property from the
setup so it shouldn't be a problem from the DT POV, but this
is not the "reset" default for the EC.

Do any of those suggestions sound better to you?

Nikita

> Thanks,
> Conor.
> 
>> +    type: boolean
>> +
>> +  connector:
>> +    $ref: /schemas/connector/usb-connector.yaml#
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |+
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +    i2c {
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        embedded-controller@76 {
>> +            compatible = "acer,aspire1-ec";
>> +            reg = <0x76>;
>> +
>> +            interrupts-extended = <&tlmm 30 IRQ_TYPE_LEVEL_LOW>;
>> +
>> +            connector {
>> +                compatible = "usb-c-connector";
>> +
>> +                port {
>> +                    ec_dp_in: endpoint {
>> +                        remote-endpoint = <&mdss_dp_out>;
>> +                    };
>> +                };
>> +            };
>> +        };
>> +    };
>>
>> --
>> 2.43.0
>>
  

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.

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.

Rob
  

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.

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

Looking forward to your opinion,
Nikita

> Rob
  

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@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.
> 
>> 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
  

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
  

Rob Herring писал(а) 23.02.2024 09:52:
> 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?
> 

Yes indeed, however I wasn't sure of a nice way to make two drivers
interact so I opted to having the prop here instead given I was trying
to reflect "device specific" (in terms of uefi setup -> os code
interaction) property.

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

I see your point. Having a generic way of switching fnlock, attached to
the correct input device would be great. However as I wasn't sure how
exactly that can be implemented in a generic manner, I opted to just
make sure that at least the user manual for this device still "works".

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

Right. Then I think I will drop the property for now and will see if
we can introduce something better and generic later.

Thanks for explaining!
Nikita

> Rob