| Message ID | 20231212-aspire1-ec-v2-1-ca495ea0a7ac@trvn.ru |
|---|---|
| State | Superseded |
| Headers | show |
| Series | power: supply: Acer Aspire 1 embedded controller | expand |
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
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
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 + + 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>; + }; + }; + }; + }; + };
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(+)