Received-SPF: pass (google.com: domain of linux-kernel+bounces-196162-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Thu, 30 May 2024 20:36:07 -0500
From: Rob Herring <robh@kernel.org>
To: wangshuaijie@awinic.com
Cc: dmitry.torokhov@gmail.com, krzk+dt@kernel.org, conor+dt@kernel.org,
	jeff@labundy.com, linux-input@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	liweilei@awinic.com, kangjiajun@awinic.com
Subject: Re: [PATCH V1 1/5] dt-bindings: input: Add YAML to Awinic sar sensor.
Message-ID: <20240531013607.GA3665090-robh@kernel.org>
References: <20240529130608.783624-1-wangshuaijie@awinic.com>
 <20240529130608.783624-2-wangshuaijie@awinic.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240529130608.783624-2-wangshuaijie@awinic.com>

On Wed, May 29, 2024 at 01:06:04PM +0000, wangshuaijie@awinic.com wrote:
> From: shuaijie wang <wangshuaijie@awinic.com>
> 
> Add the awinic,aw_sar.yaml file to adapt to the awinic sar sensor driver.
> 
> Signed-off-by: shuaijie wang <wangshuaijie@awinic.com>
> ---
>  .../bindings/input/awinic,aw_sar.yaml         | 110 ++++++++++++++++++
>  1 file changed, 110 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/awinic,aw_sar.yaml
> 
> diff --git a/Documentation/devicetree/bindings/input/awinic,aw_sar.yaml b/Documentation/devicetree/bindings/input/awinic,aw_sar.yaml
> new file mode 100644
> index 000000000000..ed4ec29c9b4d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/awinic,aw_sar.yaml
> @@ -0,0 +1,110 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/awinic,aw_sar.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Awinic sar sensor driver family
> +
> +maintainers:
> +  - Shuaijie Wang <wangshuaijie@awinic.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - awinic,aw_aw96103
> +      - awinic,aw_aw96105
> +      - awinic,aw_aw96303
> +      - awinic,aw_aw96305
> +      - awinic,aw_aw96308
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  sar-num:

Custom properties need vendor prefix.

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      set the index of the sar sensor.

What is 'sar'? It's never defined.

How is the index determined? We generally don't do indexes in DT unless 
there is some correlation to the h/w.

> +
> +  vcc0-supply:
> +    description:
> +      Optional regulator for chip, 1.7V-3.6V.
> +
> +  channel_use_flag:

vendor prefix needed plus use '-' rather than '_'. Here and elsewhere.

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      The flag of channels used.
> +      Configure according to the specific chip channel used.
> +      Bit[31:0] Each bit represents a channel.

So a mask rather than a flag.

Up to 32 channels possible? If not, add constraints.

> +      If the customer uses ch0 and ch2, then channel_use_flag=<0x05>
> +
> +  aw_sar,update_fw:
> +    type: boolean
> +    description:
> +      Choose if you want to update the firmware.

DT is mostly fixed. So someone would want to update the firmware every 
time?

> +
> +  aw_sar,monitor_esd:
> +    type: boolean
> +    description:
> +      Choose if you want to monitor ESD.
> +
> +  aw_sar,pin_set_inter_pull-up:
> +    type: boolean
> +    description:
> +      Choose if you want to set the interrupt pin to internal pull-up.
> +
> +  aw_sar,using_pm_ops:
> +    type: boolean
> +    description:
> +      Choose if you want to use suspend and resume related function.

OS configuration. Doesn't belong in DT.

> +
> +  aw_sar,use_plug_cail:
> +    type: boolean
> +    description:
> +      Choose If you want to perform calibration when plugging and unplugging the charger.
> +
> +  start-mode:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      When connecting to aw963xx, select the location where the firmware starts.
> +      set 0 if start in rom.
> +      set 1 if start in ram

Looks like constraints.

> +
> +  irq-mux:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      set csx as irq pin. config this field when connect to aw96308/aw96305BFOR

Constraints? Can you imply this based on the compatible?

> +
> +required:
> +  - compatible
> +  - reg
> +  - sar-num
> +  - interrupts
> +  - channel_use_flag
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        awinic_sar@12 {
> +            compatible = "awinic,aw_sar";
> +            reg = <0x12>;
> +            sar-num = < 0 >;
> +            interrupt-parent = < &tlmm >;
> +            interrupts = <72 0>;
> +            //vcc0-supply = <&pm660l_l4>;

Why commented?

> +            channel_use_flag = <0xff>;
> +            aw_sar,update_fw;
> +            //aw_sar,monitor_esd;
> +            start-mode = < 1 >;
> +            irq-mux = < 2 >;
> +        };
> +    };
> -- 
> 2.45.1
>