[PATCH 0/3] Introduce new optional property to mark port as write only

[PATCH 0/3] Introduce new optional property to mark port as write only

[Niall Leonard via B4 Submission Endpoint]

Some electronics do not allow the data regsister to be read.
Reading the register can corrupt the output. This makes it
impossible to read the last data written to the port.
The existing shadow data register 'bgpio_data' can be used to allow
the last written value to be returned by the read operation in this
scenario.
This is enabled for a particular port using a new flag and a new
device tree property.

Signed-off-by: Niall Leonard <nl250060@ncr.com>
---
Niall Leonard (3):
      gpio: dt-bindings: add new property to wd,mbl-gpio bindings
      gpio: Add new flag BGPIOF_NO_INPUT
      gpio: mmio: Use new flag BGPIOF_NO_INPUT

 .../devicetree/bindings/gpio/wd,mbl-gpio.txt          |  1 +
 drivers/gpio/gpio-mmio.c                              | 19 +++++++++++++++++--
 include/linux/gpio/driver.h                           |  1 +
 3 files changed, 19 insertions(+), 2 deletions(-)
---
base-commit: 1b929c02afd37871d5afb9d498426f83432e71c2
change-id: 20230126-gpio-mmio-fix-1a69d03ec9e7

Best regards,
-- 
Niall Leonard <nl250060@ncr.com>

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

[Niall Leonard via B4 Submission Endpoint]

From: Niall Leonard <nl250060@ncr.com>

Added optional "no-input" property

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.
 
 Examples:
 	gpio0: gpio0@e0000000 {

-- 
2.34.1

[PATCH 2/3] gpio: Add new flag BGPIOF_NO_INPUT

[Niall Leonard via B4 Submission Endpoint]

From: Niall Leonard <nl250060@ncr.com>

Signed-off-by: Niall Leonard <nl250060@ncr.com>
---
 include/linux/gpio/driver.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 44783fc16125..e8e57310e3b8 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -682,6 +682,7 @@ int bgpio_init(struct gpio_chip *gc, struct device *dev,
 #define BGPIOF_READ_OUTPUT_REG_SET	BIT(4) /* reg_set stores output value */
 #define BGPIOF_NO_OUTPUT		BIT(5) /* only input */
 #define BGPIOF_NO_SET_ON_INPUT		BIT(6)
+#define BGPIOF_NO_INPUT				BIT(7)	/* only output */
 
 int gpiochip_irq_map(struct irq_domain *d, unsigned int irq,
 		     irq_hw_number_t hwirq);

-- 
2.34.1

[PATCH 3/3] gpio: mmio: Use new flag BGPIOF_NO_INPUT

[Niall Leonard via B4 Submission Endpoint]

From: Niall Leonard <nl250060@ncr.com>

Use the existing shadow data register 'bgpio_data' to allow
the last written value to be returned by the read operation
when BGPIOF_NO_INPUT flag is set.

Signed-off-by: Niall Leonard <nl250060@ncr.com>
---
 drivers/gpio/gpio-mmio.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c
index d9dff3dc92ae..7125bd8caaa4 100644
--- a/drivers/gpio/gpio-mmio.c
+++ b/drivers/gpio/gpio-mmio.c
@@ -164,6 +164,11 @@ static int bgpio_get_set_multiple(struct gpio_chip *gc, unsigned long *mask,
 	return 0;
 }
 
+static int bgpio_get_shadow(struct gpio_chip *gc, unsigned int gpio)
+{
+	return !!(gc->bgpio_data & bgpio_line2mask(gc, gpio));
+}
+
 static int bgpio_get(struct gpio_chip *gc, unsigned int gpio)
 {
 	return !!(gc->read_reg(gc->reg_dat) & bgpio_line2mask(gc, gpio));
@@ -526,7 +531,10 @@ static int bgpio_setup_io(struct gpio_chip *gc,
 		 * reading each line individually in that fringe case.
 		 */
 	} else {
-		gc->get = bgpio_get;
+		if (flags & BGPIOF_NO_INPUT)
+			gc->get = bgpio_get_shadow;
+		else
+			gc->get = bgpio_get;
 		if (gc->be_bits)
 			gc->get_multiple = bgpio_get_multiple_be;
 		else
@@ -630,7 +638,11 @@ int bgpio_init(struct gpio_chip *gc, struct device *dev,
 	if (ret)
 		return ret;
 
-	gc->bgpio_data = gc->read_reg(gc->reg_dat);
+	if (flags & BGPIOF_NO_INPUT)
+		gc->bgpio_data = 0;
+	else
+		gc->bgpio_data = gc->read_reg(gc->reg_dat);
+
 	if (gc->set == bgpio_set_set &&
 			!(flags & BGPIOF_UNREADABLE_REG_SET))
 		gc->bgpio_data = gc->read_reg(gc->reg_set);
@@ -711,6 +723,9 @@ static struct bgpio_pdata *bgpio_parse_dt(struct platform_device *pdev,
 	if (of_property_read_bool(pdev->dev.of_node, "no-output"))
 		*flags |= BGPIOF_NO_OUTPUT;
 
+	if (of_property_read_bool(pdev->dev.of_node, "no-input"))
+		*flags |= BGPIOF_NO_INPUT;
+
 	return pdata;
 }
 #else

-- 
2.34.1

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

[Krzysztof Kozlowski]

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?

Anyway, please convert the bindings to DT schema first (see
writing-schema and example-schema).
Documentation/devicetree/bindings/writing-schema.rst

Best regards,
Krzysztof

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

[Leonard, Niall]

> -----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. That is essentially the change I have 
submitted.
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
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.

What's your view ?

> Best regards,
> Krzysztof

Re: [PATCH 3/3] gpio: mmio: Use new flag BGPIOF_NO_INPUT

[Linus Walleij]

On Thu, Jan 26, 2023 at 11:18 AM Niall Leonard via B4 Submission
Endpoint <devnull+nl250060.ncr.com@kernel.org> wrote:

> From: Niall Leonard <nl250060@ncr.com>
>
> Use the existing shadow data register 'bgpio_data' to allow
> the last written value to be returned by the read operation
> when BGPIOF_NO_INPUT flag is set.
>
> Signed-off-by: Niall Leonard <nl250060@ncr.com>

Weird hardware, but given these restrictions it makes perfect sense
to have this.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 2/3] gpio: Add new flag BGPIOF_NO_INPUT

[Linus Walleij]

On Thu, Jan 26, 2023 at 11:18 AM Niall Leonard via B4 Submission
Endpoint <devnull+nl250060.ncr.com@kernel.org> wrote:

> From: Niall Leonard <nl250060@ncr.com>
>
> Signed-off-by: Niall Leonard <nl250060@ncr.com>

I would squash this with the other patch adding it to the MMIO driver
per the principle one patch = one technical step.

Anyway:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

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

[Linus Walleij]

Hi Niall,

thanks for your patch!

On Thu, Jan 26, 2023 at 11:18 AM Niall Leonard via B4 Submission
Endpoint <devnull+nl250060.ncr.com@kernel.org> wrote:

>  Optional properties:
>         - no-output: GPIOs are read-only.
> +       - no-input: GPIOs are write-only. Read is via a shadow register.

"Shadow register" is unclear technical lingo.

Just write "GPIO output registers are write-only"

DT bindings are OS neutral, the fact that Linux and other OS:es need to
cache ("shadow") this value is an implementation detail.

Yours,
Linus Walleij

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

[Krzysztof Kozlowski]

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?

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

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

> 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

Re: [PATCH 3/3] gpio: mmio: Use new flag BGPIOF_NO_INPUT

[Krzysztof Kozlowski]

On 26/01/2023 11:17, Niall Leonard via B4 Submission Endpoint wrote:
> From: Niall Leonard <nl250060@ncr.com>
> 
> Use the existing shadow data register 'bgpio_data' to allow
> the last written value to be returned by the read operation
> when BGPIOF_NO_INPUT flag is set.
> 

(...)

>  	if (gc->set == bgpio_set_set &&
>  			!(flags & BGPIOF_UNREADABLE_REG_SET))
>  		gc->bgpio_data = gc->read_reg(gc->reg_set);
> @@ -711,6 +723,9 @@ static struct bgpio_pdata *bgpio_parse_dt(struct platform_device *pdev,
>  	if (of_property_read_bool(pdev->dev.of_node, "no-output"))
>  		*flags |= BGPIOF_NO_OUTPUT;
>  
> +	if (of_property_read_bool(pdev->dev.of_node, "no-input"))

As pointed, this brings undocumented property to two other bindings.
This needs to be fixed.

Best regards,
Krzysztof

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

[Leonard, Niall]

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
> 

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

[Rob Herring]

On Mon, Jan 30, 2023 at 01:20:55PM +0000, Leonard, Niall wrote:
> 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. 

This part makes sense as you explained the h/w.

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

Not quite relevant to the DT binding being a Linux detail.

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

A property for a board level quirk is appropriate. You just need to 
explain that in the commit message rather than stating what the diff 
already tells us.

Rob

Re: [PATCH 2/3] gpio: Add new flag BGPIOF_NO_INPUT

[Leonard, Niall]

On 27/01/2023 12:54, Linus Walleij wrote:
> *External Message* - Use caution before opening links or attachments
> 
> On Thu, Jan 26, 2023 at 11:18 AM Niall Leonard via B4 Submission
> Endpoint <devnull+nl250060.ncr.com@kernel.org> wrote:
> 
>> From: Niall Leonard <nl250060@ncr.com>
>>
>> Signed-off-by: Niall Leonard <nl250060@ncr.com>
> 
> I would squash this with the other patch adding it to the MMIO driver
> per the principle one patch = one technical step.
> 
> Anyway:
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Yours,
> Linus Walleij
Thanks for reviewing.
I will do as you suggest in the next version.

Regards,
Niall Leonard

Re: [PATCH 3/3] gpio: mmio: Use new flag BGPIOF_NO_INPUT

[Leonard, Niall]

On 29/01/2023 16:01, Krzysztof Kozlowski wrote:
> *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>
>>
>> Use the existing shadow data register 'bgpio_data' to allow
>> the last written value to be returned by the read operation
>> when BGPIOF_NO_INPUT flag is set.
>>
> 
> (...)
> 
>>   	if (gc->set == bgpio_set_set &&
>>   			!(flags & BGPIOF_UNREADABLE_REG_SET))
>>   		gc->bgpio_data = gc->read_reg(gc->reg_set);
>> @@ -711,6 +723,9 @@ static struct bgpio_pdata *bgpio_parse_dt(struct platform_device *pdev,
>>   	if (of_property_read_bool(pdev->dev.of_node, "no-output"))
>>   		*flags |= BGPIOF_NO_OUTPUT;
>>   
>> +	if (of_property_read_bool(pdev->dev.of_node, "no-input"))
> 
> As pointed, this brings undocumented property to two other bindings.
> This needs to be fixed.
> 
> Best regards,
> Krzysztof
> 
Thanks for reviewing.
I will update the other 2 bindings in the next version.

Regards,
Niall Leonard

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

[Leonard, Niall]

On 30/01/2023 18:37, Rob Herring wrote:
> *External Message* - Use caution before opening links or attachments
> 
> On Mon, Jan 30, 2023 at 01:20:55PM +0000, Leonard, Niall wrote:
>> 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.
> 
> This part makes sense as you explained the h/w.
> 
>> 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.
> 
> Not quite relevant to the DT binding being a Linux detail.
> 
>>
>> 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.
> 
> A property for a board level quirk is appropriate. You just need to
> explain that in the commit message rather than stating what the diff
> already tells us.
> 
> Rob
Thanks for reviewing.
I will update the description in the patch introduction to indicate this 
a board level quirk and the reasoning behind it.

Regards,
Niall Leonard