Re: [PATCH 1/3] gpio: dt-bindings: add new property to wd,mbl-gpio bindings

["Leonard, Niall" <Niall.Leonard@ncr.com>]

On 29/01/2023 15:59, Krzysztof Kozlowski wrote:
> *External Message* - Use caution before opening links or attachments
> 
> On 27/01/2023 12:39, Leonard, Niall wrote:
>>
>>
>>> -----Original Message-----
>>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>> Sent: 26 January 2023 12:29
>>> To: Leonard, Niall <Niall.Leonard@ncr.com>; Linus Walleij
>>> <linus.walleij@linaro.org>; Bartosz Golaszewski <brgl@bgdev.pl>; Rob
>>> Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
>>> <krzysztof.kozlowski+dt@linaro.org>
>>> Cc: linux-gpio@vger.kernel.org; devicetree@vger.kernel.org; linux-
>>> kernel@vger.kernel.org
>>> Subject: Re: [PATCH 1/3] gpio: dt-bindings: add new property to wd,mbl-gpio
>>> bindings
>>>
>>> *External Message* - Use caution before opening links or attachments
>>>
>>> On 26/01/2023 11:17, Niall Leonard via B4 Submission Endpoint wrote:
>>>> From: Niall Leonard <nl250060@ncr.com>
>>>
>>> Subject: missing "wd,mbl-gpio:" prefix.
>>>
>>> Subject: drop second/last, redundant "bindings". The "dt-bindings"
>>> prefix is already stating that these are bindings.
>>>
>>>>
>>>> Added optional "no-input" property
>>>
>>> Missing full stop.
>>>
>>>>
>>>> Signed-off-by: Niall Leonard <nl250060@ncr.com>
>>>> ---
>>>>   Documentation/devicetree/bindings/gpio/wd,mbl-gpio.txt | 1 +
>>>>   1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/gpio/wd,mbl-gpio.txt
>>>> b/Documentation/devicetree/bindings/gpio/wd,mbl-gpio.txt
>>>> index 038c3a6a1f4d..9405f9dad522 100644
>>>> --- a/Documentation/devicetree/bindings/gpio/wd,mbl-gpio.txt
>>>> +++ b/Documentation/devicetree/bindings/gpio/wd,mbl-gpio.txt
>>>> @@ -18,6 +18,7 @@ Required properties:
>>>>
>>>>   Optional properties:
>>>>   	- no-output: GPIOs are read-only.
>>>> +	- no-input: GPIOs are write-only. Read is via a shadow register.
>>>
>>> Why this property is needed? Why driver cannot always use shadow
>>> register?
>>>
>> The shadow register is currently only used during the write operation. It is not available during the read operation.
> 
> You just wrote above that reading is via shadow register, so how can it
> not be available for reads? Again, why you cannot always read via shadow
> register and need to make a property? You mean that for other GPIOs
> there is no shadow register at all?
> 
The existing read method does not use the shadow register.

static int bgpio_get(struct gpio_chip *gc, unsigned int gpio)
{
	return !!(gc->read_reg(gc->reg_dat) & bgpio_line2mask(gc, gpio));
}

> What changes between one board and another that justifies this property?

I have a couple of boards where the electronics engineer decided to only 
use the chip select line, so no read/write signal is connected. This 
means that reading the address activates the chip select and drives the 
contents of the data bus to the port. For example is someone reads the 
file /sys/kernel/debug/gpio this corrupts the port. So I have had to add 
this property to avoid that situation.

If you are strongly against this then just reject it and I will look 
after it myself. I thought there may be others who would find this 
change useful.

> 
>> That is essentially the change I have
>> submitted.
> 
> This does not answer me. I am asking why this change is justified in
> terms of Devicetree.
> 
How else would you suggest it was done ? I followed the existing pattern 
used previously for the "no-output" property.


>> An alternative approach would have been to develop an entire new gpio driver similar to the 74xx driver, but I felt this approach was better.
>>
>>> Anyway, please convert the bindings to DT schema first (see writing-schema
>>> and example-schema).
>>> Documentation/devicetree/bindings/writing-schema.rst
>>>
>> The bindings for this driver are duplicated in a few files even though they use the same driver.
>> i.e. wd,mbl-gpio.txt, ni,169445-nand-gpio.txt, brcm,bcm6345-gpio.yaml
> 
> So your changes here affect several bindings but you adjust only one?
> This won't work.
> 
>> I don't know why these multiple bindings exist. It would perhaps make sense to remove these duplicate binding documentation files and replace with a single one for "basic-mmio-gpio". I happened to pick ". wd,mbl-gpio.txt", but I could have just as easily chosen one of the other 2.
> 
> We usually keep same hardware in the same bindings. This might or might
> not map to same Linux driver (drivers are independent). All this
> hardware looks like having the same interface and same properties, so
> having one binding makes sense.
> 
> Best regards,
> Krzysztof
>