diff mbox series

[V2,1/5] dt-bindings: input: Add YAML to Awinic sar sensor.

Message ID 20240605091143.163789-2-wangshuaijie@awinic.com (mailing list archive)
State New
Headers show
Series Add support for Awinic SAR sensor. | expand

Commit Message

wangshuaijie@awinic.com June 5, 2024, 9:11 a.m. UTC
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         | 125 ++++++++++++++++++
 1 file changed, 125 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/awinic,aw_sar.yaml

Comments

Krzysztof Kozlowski June 5, 2024, 10:20 a.m. UTC | #1
On 05/06/2024 11:11, 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.

Subject: drop final stops. From all your patches.

> 
> Signed-off-by: shuaijie wang <wangshuaijie@awinic.com>
> ---

No changelog, so nothing improved?

>  .../bindings/input/awinic,aw_sar.yaml         | 125 ++++++++++++++++++

No underscores, but rather awinic,aw96103.yaml

>  1 file changed, 125 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..2560ef09d3d0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/awinic,aw_sar.yaml
> @@ -0,0 +1,125 @@
> +# 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

driver as Linux driver or some other hardware meaning? If first, then
drop and describe hardware.


> +
> +maintainers:
> +  - Shuaijie Wang <wangshuaijie@awinic.com>

Missing description. You already got question about meaning of sar and
indeed nothing improved.

> +
> +properties:
> +  compatible:
> +    enum:
> +      - awinic,aw96103
> +      - awinic,aw96105
> +      - awinic,aw96303
> +      - awinic,aw96305
> +      - awinic,aw96308
> +
> +  reg:
> +    maxItems: 1
> +
> +  irq-gpio:
> +    maxItems: 1
> +
> +  awinic,sar-label:

label is a string, not number.

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Set the label of the SAR(Specific Absorption Rate) sensor.
> +      It is set to 0 if one awinic sar chip is used.
> +      If two awinic sar chips are used, awinic,sar-label in the first
> +      awinic-sar should be set to 0 and awinic,sar-label in the second
> +      awinic-sar should be set to 1.

Sorry, no instance indexing. Drop.

> +
> +

No need for two line breaks.

> +  awinic,regulator-power-supply:
> +    description:
> +      Choose if you want to use a regulator to power the chip. Then the
> +      vccX-supply has to be set.
> +
> +  vcc0-supply:
> +    description:
> +      Optional regulator for chip, 1.7V-3.6V.
> +      If two awinic sar chips are used, the first regulator
> +      should set the ID to vcc0-supply and the second regulator
> +      should set the ID to vcc1-supply.
> +
> +  awinic,channel-use-mask:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      The mask of channels used.
> +      Configure according to the specific chip channel used.
> +      Bit[31:0] Each bit represents a channel.
> +      If the customer uses ch0 and ch2, then channel_use_mask=<0x05>
> +      For a 3-channel chip, the maximum value is 0x07;
> +      For a 5-channel chip, the maximum value is 0x1F;
> +      For a 8-channel chip, the maximum value is 0xFF;
> +
> +  awinic,update-fw:
> +    type: boolean
> +    description:
> +      Choose if you want to update the firmware.

Not much improve in explanation or rationale. Why do you want to update
FW every time? Explain this in property description.

I mostly skipped the rest, because it does not look like you addresses
previous feedback.

...
> +
> +required:
> +  - compatible
> +  - reg
> +  - irq-gpio
> +  - awinic,sar-label
> +  - awinic,channel-use-mask
> +
> +unevaluatedProperties: false

Missing some ref, like dai-common or component... or this is supposed to
be additionalProperties: false instead.

> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        awinic-sar@12 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


I still have no clue what is sar and there is no description in this
binding.

> +            compatible = "awinic,aw96308";
> +            reg = <0x12>;
> +            irq-gpio = <&tlmm 72 0>;

Use proper defines.

> +            awinic,sar-label = < 0 >;

Do not introduce different coding style. Drop spaces. See DTS coding style.



Best regards,
Krzysztof
wangshuaijie@awinic.com July 12, 2024, 9:47 a.m. UTC | #2
Hi Krzysztof,

Thank you very much for your valuable suggestions. I will fix the related issues in V3.

On Wed, 5 Jun 2024 12:20:31, krzk@kernel.org wrote:
>On 05/06/2024 11:11, 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.
>
>Subject: drop final stops. From all your patches.
>

The patch for V3 will fix this issue.

>> 
>> Signed-off-by: shuaijie wang <wangshuaijie@awinic.com>
>> ---
>
>No changelog, so nothing improved?
>

The patch for V3 will fix this issue.
I will include a changelog in the next version.

>>  .../bindings/input/awinic,aw_sar.yaml         | 125 ++++++++++++++++++
>
>No underscores, but rather awinic,aw96103.yaml

I will change the name to awinic,aw96xxx.yaml in the next version.

>
>>  1 file changed, 125 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..2560ef09d3d0
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/input/awinic,aw_sar.yaml
>> @@ -0,0 +1,125 @@
>> +# 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
>
>driver as Linux driver or some other hardware meaning? If first, then
>drop and describe hardware.
>
>

The patch for V3 will fix this issue.
(title: Awinic's AW96XXX capacitive proximity sensor)

>> +
>> +maintainers:
>> +  - Shuaijie Wang <wangshuaijie@awinic.com>
>
>Missing description. You already got question about meaning of sar and
>indeed nothing improved.
>

The patch for V3 will fix this issue.

>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - awinic,aw96103
>> +      - awinic,aw96105
>> +      - awinic,aw96303
>> +      - awinic,aw96305
>> +      - awinic,aw96308
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  irq-gpio:
>> +    maxItems: 1
>> +
>> +  awinic,sar-label:
>
>label is a string, not number.
>
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description:
>> +      Set the label of the SAR(Specific Absorption Rate) sensor.
>> +      It is set to 0 if one awinic sar chip is used.
>> +      If two awinic sar chips are used, awinic,sar-label in the first
>> +      awinic-sar should be set to 0 and awinic,sar-label in the second
>> +      awinic-sar should be set to 1.
>
>Sorry, no instance indexing. Drop.
>

I will modify the relevant descriptions in the V3.

>> +
>> +
>
>No need for two line breaks.
>
>> +  awinic,regulator-power-supply:
>> +    description:
>> +      Choose if you want to use a regulator to power the chip. Then the
>> +      vccX-supply has to be set.
>> +
>> +  vcc0-supply:
>> +    description:
>> +      Optional regulator for chip, 1.7V-3.6V.
>> +      If two awinic sar chips are used, the first regulator
>> +      should set the ID to vcc0-supply and the second regulator
>> +      should set the ID to vcc1-supply.
>> +
>> +  awinic,channel-use-mask:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description:
>> +      The mask of channels used.
>> +      Configure according to the specific chip channel used.
>> +      Bit[31:0] Each bit represents a channel.
>> +      If the customer uses ch0 and ch2, then channel_use_mask=<0x05>
>> +      For a 3-channel chip, the maximum value is 0x07;
>> +      For a 5-channel chip, the maximum value is 0x1F;
>> +      For a 8-channel chip, the maximum value is 0xFF;
>> +
>> +  awinic,update-fw:
>> +    type: boolean
>> +    description:
>> +      Choose if you want to update the firmware.
>
>Not much improve in explanation or rationale. Why do you want to update
>FW every time? Explain this in property description.
>

In the next version, I will remove the relevant feature.

>I mostly skipped the rest, because it does not look like you addresses
>previous feedback.
>
>...
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - irq-gpio
>> +  - awinic,sar-label
>> +  - awinic,channel-use-mask
>> +
>> +unevaluatedProperties: false
>
>Missing some ref, like dai-common or component... or this is supposed to
>be additionalProperties: false instead.
>

In the next version, I will change it to additionalProperties: 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 {
>
>Node names should be generic. See also an explanation and list of
>examples (not exhaustive) in DT specification:
>https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
>

In the next version, I will change it to proximity.

>
>I still have no clue what is sar and there is no description in this
>binding.
>

I will add relevant descriptions in the next version.

The specific absorption rate (SAR) is a metric that measures
the degree of absorption of electromagnetic radiation emitted by wireless
devices, such as mobile phones and tablets, by human tissue.

In mobile phone applications, the proximity sensor is primarily used to detect
the proximity of the human body to the phone. When the phone approaches the human body,
it will actively reduce the transmit power of the antenna to keep the SAR within a safe
range. Therefore, we also refer to the proximity sensor as a SAR sensor.

>> +            compatible = "awinic,aw96308";
>> +            reg = <0x12>;
>> +            irq-gpio = <&tlmm 72 0>;
>
>Use proper defines.
>

In the next version, I will change it to interrupts.

>> +            awinic,sar-label = < 0 >;
>
>Do not introduce different coding style. Drop spaces. See DTS coding style.
>

The patch for V3 will fix this issue.

>
>
>Best regards,
>Krzysztof

Kind regards,
Wang Shuaijie
diff mbox series

Patch

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..2560ef09d3d0
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/awinic,aw_sar.yaml
@@ -0,0 +1,125 @@ 
+# 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,aw96103
+      - awinic,aw96105
+      - awinic,aw96303
+      - awinic,aw96305
+      - awinic,aw96308
+
+  reg:
+    maxItems: 1
+
+  irq-gpio:
+    maxItems: 1
+
+  awinic,sar-label:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Set the label of the SAR(Specific Absorption Rate) sensor.
+      It is set to 0 if one awinic sar chip is used.
+      If two awinic sar chips are used, awinic,sar-label in the first
+      awinic-sar should be set to 0 and awinic,sar-label in the second
+      awinic-sar should be set to 1.
+
+
+  awinic,regulator-power-supply:
+    description:
+      Choose if you want to use a regulator to power the chip. Then the
+      vccX-supply has to be set.
+
+  vcc0-supply:
+    description:
+      Optional regulator for chip, 1.7V-3.6V.
+      If two awinic sar chips are used, the first regulator
+      should set the ID to vcc0-supply and the second regulator
+      should set the ID to vcc1-supply.
+
+  awinic,channel-use-mask:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      The mask of channels used.
+      Configure according to the specific chip channel used.
+      Bit[31:0] Each bit represents a channel.
+      If the customer uses ch0 and ch2, then channel_use_mask=<0x05>
+      For a 3-channel chip, the maximum value is 0x07;
+      For a 5-channel chip, the maximum value is 0x1F;
+      For a 8-channel chip, the maximum value is 0xFF;
+
+  awinic,update-fw:
+    type: boolean
+    description:
+      Choose if you want to update the firmware.
+
+  awinic,monitor-esd:
+    type: boolean
+    description:
+      Choose if you want to monitor ESD.
+
+  awinic,pin-set-inter-pull-up:
+    type: boolean
+    description:
+      Choose if you want to set the interrupt pin to internal pull-up.
+
+  awinic,using-pm-ops:
+    type: boolean
+    description:
+      Choose if you want to change the chip mode on suppend and resume.
+
+  awinic,use-plug-cail:
+    type: boolean
+    description:
+      Choose If you want to perform calibration when plugging and unplugging the charger.
+
+  awinic,start-mode:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      You only need to set this configuration item if you are using AW963XX.
+      When connecting to AW963XX, select the location where the firmware starts.
+      Set 0 if start in rom.
+      Set 1 if start in ram
+
+  awinic,irq-mux:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      You only need to set this configuration item if you are using AW96308 adn AW96305BFOR.
+      If CS2 is used as the interrupt pin, this item should be set to 2.
+      If CS5 is used as the interrupt pin, this item should be set to 5.
+
+required:
+  - compatible
+  - reg
+  - irq-gpio
+  - awinic,sar-label
+  - awinic,channel-use-mask
+
+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,aw96308";
+            reg = <0x12>;
+            irq-gpio = <&tlmm 72 0>;
+            awinic,sar-label = < 0 >;
+            awinic,channel-use-mask = <0xff>;
+            awinic,start-mode = < 0 >;
+            awinic,irq-mux = < 2 >;
+        };
+    };