Re: [PATCH 2/3] dt-bindings: iio: proximity: Add Nicera D3-323-AA PIR sensor

[Waqar Hameed <waqar.hameed@axis.com>]

On Tue, May 20, 2025 at 08:36 +0200 Krzysztof Kozlowski <krzk@kernel.org> wrote:

> On 16/05/2025 19:15, Waqar Hameed wrote:
>> On Fri, May 09, 2025 at 17:06 +0200 Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> 
>>> On 09/05/2025 17:03, Waqar Hameed wrote:
>>>> Nicera D3-323-AA is a PIR sensor for human detection. It has support for
>>>> raw data measurements and detection notification. The communication
>>>> protocol is custom made and therefore needs to be GPIO bit banged.
>>>>
>>>> Add devicetree bindings requiring the compatible string and the various
>>>> GPIOs needed for operation. Some of the GPIOs have multiple use-cases
>>>> depending on device state. Describe these thoroughly.
>>>
>>>
>>> Drop redundant parts of description. Describe the hardware. 
>> 
>> I'll reformulate and incorporate some information of the pins and how it
>> is used in the hardware.
>> 
>>> Entire last paragraph is pretty pointless.
>> 
>> I'll remove it then. (Some sub-system maintainers want a description of
>> what the patch does, in imperative form. But I can see why it might not
>> add any value when it comes to dt-bindings.)
>> 
>>>
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: nicera,d3323aa
>>>> +
>>>> +  vdd-gpio:
>>>> +    maxItems: 1
>>>> +    description:
>>>> +      GPIO for supply voltage (1.8 to 5.5 V).
>>>
>>> This is not how pins are represented in the kernel. Either you have here
>>> regulator (supply) or reset gpios. 
>> 
>> I'll change it to `vdd-supply`.
>> 
>>> Plus 'gpio' suffix is not valid, btw.
>> 
>> I actually `grep`ed before writing this to see if there were other
>> dt-bindings with this suffix. Because the GPIO framework supports both
>> `gpio` and `gpios` as suffixes (see `gpio_suffixes[]` in `gpiolib.c`).
>> However, since `-gpios` are clearly in majority, we should go for that.
>
>
> One is deprecated. It is always, always gpios.

Ah, ok! Good to know.

[...]

>>>> +    maxItems: 1
>>>> +    description:
>>>> +      GPIO for clock and detection.
>>>> +      After reset, the device signals with two falling edges on this pin that it
>>>> +      is ready for configuration (within 1.2 s), which the driver listens for as
>>>> +      interrupts.
>>>> +      During configuration, it is used as clock for data reading and writing (on
>>>> +      data-gpio). The driver drives this pin with the frequency of 1 kHz (bit
>>>> +      banging).
>>>> +      After all this, the device is now in operational mode and signals on this
>>>> +      pin for any detections. The driver listens for this as interrupts.
>>>> +
>>>> +  data-gpio:
>>>
>>> There is no such pin.
>> 
>> You mean to change it to `data-gpios`? (There are some using that, e.g.
>> `sensirion,sht15.yaml`).
>
> No, I meant I opened datasheet and found no such pin. Unless you meant
> DO, but then mention in description the actual name of the pin, if you
> are using some more descriptive property name for it.

Got it! Let's do that.

>>>> +    maxItems: 1
>>>> +    description:
>>>> +      GPIO for data reading and writing.
>>>> +      During configuration, configuration data will be written and read back
>>>> +      with bit banging (together with clk-vout-gpio as clock).
>>>> +      After this, during operational mode, the device will output serial data on
>>>> +      this GPIO. However, the driver currently doesn't read this.
>
> BTW, drop all references to driver here and in other places. If driver
> change, you will change binding? If my driver behaves differently, why
> binding would be claiming something else?

Understood, will do that!