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

From: Conor Dooley <conor@kernel.org>
To: Nikita Travkin <nikita@trvn.ru>
Cc: Sebastian Reichel <sre@kernel.org>,
	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>,
	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: Tue, 12 Dec 2023 17:24:13 +0000	[thread overview]
Message-ID: <20231212-wasp-overcoat-cf2bd1383670@spud> (raw)
In-Reply-To: <20231212-aspire1-ec-v2-1-ca495ea0a7ac@trvn.ru>

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

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
> 

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