Message-ID: <20240712094701.2393968-1-wangshuaijie@awinic.com>
Date: Fri, 12 Jul 2024 09:47:01 +0000
From: wangshuaijie@...nic.com
To: krzk@...nel.org
Cc: conor+dt@...nel.org,
	devicetree@...r.kernel.org,
	dmitry.torokhov@...il.com,
	jeff@...undy.com,
	kangjiajun@...nic.com,
	krzk+dt@...nel.org,
	linux-input@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	liweilei@...nic.com,
	robh@...nel.org,
	wangshuaijie@...nic.com
Subject: Re: [PATCH V2 1/5] dt-bindings: input: Add YAML to Awinic sar sensor

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@...nel.org wrote:
>On 05/06/2024 11:11, wangshuaijie@...nic.com wrote:
>> From: shuaijie wang <wangshuaijie@...nic.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@...nic.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@...nic.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

Powered by blists - more mailing lists