Received-SPF: pass (google.com: domain of linux-kernel+bounces-202049-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: wangshuaijie@awinic.com
To: robh@kernel.org
Cc: conor+dt@kernel.org,
	devicetree@vger.kernel.org,
	dmitry.torokhov@gmail.com,
	jeff@labundy.com,
	kangjiajun@awinic.com,
	krzk+dt@kernel.org,
	linux-input@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	liweilei@awinic.com,
	wangshuaijie@awinic.com
Subject: Re: [PATCH V1 1/5] dt-bindings: input: Add YAML to Awinic sar sensor.
Date: Wed,  5 Jun 2024 09:02:06 +0000
Message-ID: <20240605090206.156859-1-wangshuaijie@awinic.com>
In-Reply-To: <20240531013607.GA3665090-robh@kernel.org>
References: <20240531013607.GA3665090-robh@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit

On Thu, 30 May 2024 20:36:07 -0500, robh@kernel.org wrote:
>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.

The patch for version v2 will fix this issue.

>
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description:
>> +      set the index of the sar sensor.
>
>What is 'sar'? It's never defined.

SAR(Specific Absorption Rate), The patch for version v2 will include this explanation.

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

Perhaps using "label" is more appropriate, as it represents the order of the SAR sensor.
The patch for version v2 will fix this issue.

>
>> +
>> +  vcc0-supply:
>> +    description:
>> +      Optional regulator for chip, 1.7V-3.6V.
>> +
>> +  channel_use_flag:
>
>vendor prefix needed plus use '-' rather than '_'. Here and elsewhere.

The patch for version v2 will fix this issue.

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

The patch for version v2 will fix this issue.

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

In certain situations, if the program in the chip's ROM does not meet the needs,
it is necessary to reload new firmware into the SRAM after each power-on,
and then use the new firmware.

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

Perhaps this name is not quite appropriate. It mainly determines whether to
put the chip into sleep mode in the PM, and there are other operations in
the PM that are not controlled by this option.

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

Only the AW963XX series chips need to configure this option.
I will redescribe it in the V2 version of the patch.

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

Yes, only AW96308 and AW96305BFOR need to configure this option.
I will redescribe it in the V2 version of the patch.

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

This is a non-essential configuration,
and I will remove it in the v2 version of the patch.

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

--
Thanks,

Wang Shuaijie