linux-gpio.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V4 2/2] gpio: inverter: document the inverter bindings
@ 2019-06-28  9:30 Harish Jenny K N
  2019-07-04  5:01 ` Harish Jenny K N
                   ` (2 more replies)
  0 siblings, 3 replies; 29+ messages in thread
From: Harish Jenny K N @ 2019-06-28  9:30 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski, Rob Herring, Mark Rutland
  Cc: devicetree, linux-gpio, Harish Jenny K N, Balasubramani Vivekanandan

Document the device tree binding for the inverter gpio
controller to configure the polarity of the gpio pins
used by the consumers.

Signed-off-by: Harish Jenny K N <harish_kandiga@mentor.com>
---
 .../devicetree/bindings/gpio/gpio-inverter.txt     | 29 ++++++++++++++++++++++
 1 file changed, 29 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-inverter.txt

diff --git a/Documentation/devicetree/bindings/gpio/gpio-inverter.txt b/Documentation/devicetree/bindings/gpio/gpio-inverter.txt
new file mode 100644
index 0000000..8bb6b2e
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-inverter.txt
@@ -0,0 +1,29 @@
+GPIO-INVERTER
+======
+This binding defines the gpio-inverter. The gpio-inverter is a driver that
+allows to properly describe the gpio polarities on the hardware.
+
+Please refer to gpio.txt for generic information regarding GPIO bindings.
+
+Required properties:
+- compatible : "gpio-inverter".
+- gpio-controller: Marks the port as GPIO controller.
+- #gpio-cells: One. This is the pin number.
+- inverted-gpios: Array of GPIO pins required from consumers, whose polarity
+  has to be inverted in the driver.
+Note: gpio flag should be set as GPIO_ACTIVE_HIGH. Using GPIO_ACTICE_LOW will
+cause double inversion.
+
+Optional properties:
+- gpio-line-names: Refer to gpio.txt for details regarding this property.
+
+Example:
+
+gpio_inv: gpio-inv {
+	compatible = "gpio-inverter";
+	gpio-controller;
+	#gpio-cells = <1>;
+	inverted-gpios = <&gpio5 24 GPIO_ACTIVE_HIGH>,
+	<&gpio7 0 GPIO_ACTIVE_HIGH>, <&gpio7 1 GPIO_ACTIVE_HIGH>;
+	gpio-line-names = "JTAG_DNL_EN", "lvds-pwrdwn", "lcd-on";
+};
--
2.7.4


^ permalink raw reply related	[flat|nested] 29+ messages in thread

* Re: [PATCH V4 2/2] gpio: inverter: document the inverter bindings
  2019-06-28  9:30 [PATCH V4 2/2] gpio: inverter: document the inverter bindings Harish Jenny K N
@ 2019-07-04  5:01 ` Harish Jenny K N
  2019-07-08 22:36 ` Rob Herring
  2019-09-25 16:51 ` Eugeniu Rosca
  2 siblings, 0 replies; 29+ messages in thread
From: Harish Jenny K N @ 2019-07-04  5:01 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski, Rob Herring, Mark Rutland
  Cc: devicetree, linux-gpio, Balasubramani Vivekanandan

Hi,

On 28/06/19 3:00 PM, Harish Jenny K N wrote:
> Document the device tree binding for the inverter gpio
> controller to configure the polarity of the gpio pins
> used by the consumers.
>
> Signed-off-by: Harish Jenny K N <harish_kandiga@mentor.com>
> ---
>  .../devicetree/bindings/gpio/gpio-inverter.txt     | 29 ++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-inverter.txt
>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-inverter.txt b/Documentation/devicetree/bindings/gpio/gpio-inverter.txt
> new file mode 100644
> index 0000000..8bb6b2e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-inverter.txt
> @@ -0,0 +1,29 @@
> +GPIO-INVERTER
> +======
> +This binding defines the gpio-inverter. The gpio-inverter is a driver that
> +allows to properly describe the gpio polarities on the hardware.
> +
> +Please refer to gpio.txt for generic information regarding GPIO bindings.
> +
> +Required properties:
> +- compatible : "gpio-inverter".
> +- gpio-controller: Marks the port as GPIO controller.
> +- #gpio-cells: One. This is the pin number.
> +- inverted-gpios: Array of GPIO pins required from consumers, whose polarity
> +  has to be inverted in the driver.
> +Note: gpio flag should be set as GPIO_ACTIVE_HIGH. Using GPIO_ACTICE_LOW will
> +cause double inversion.
> +
> +Optional properties:
> +- gpio-line-names: Refer to gpio.txt for details regarding this property.
> +
> +Example:
> +
> +gpio_inv: gpio-inv {
> +	compatible = "gpio-inverter";
> +	gpio-controller;
> +	#gpio-cells = <1>;
> +	inverted-gpios = <&gpio5 24 GPIO_ACTIVE_HIGH>,
> +	<&gpio7 0 GPIO_ACTIVE_HIGH>, <&gpio7 1 GPIO_ACTIVE_HIGH>;
> +	gpio-line-names = "JTAG_DNL_EN", "lvds-pwrdwn", "lcd-on";
> +};
> --
> 2.7.4
>

Can anyone of DT people please review this ?

Just to let you know that Linus Walleij has reviewed the gpio inverter driver and needs some review from the DT people before he applies it.

Thanks in advance.


Best Regards,

Harish Jenny K N




^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH V4 2/2] gpio: inverter: document the inverter bindings
  2019-06-28  9:30 [PATCH V4 2/2] gpio: inverter: document the inverter bindings Harish Jenny K N
  2019-07-04  5:01 ` Harish Jenny K N
@ 2019-07-08 22:36 ` Rob Herring
  2019-07-09  5:25   ` Harish Jenny K N
  2019-09-25 16:51 ` Eugeniu Rosca
  2 siblings, 1 reply; 29+ messages in thread
From: Rob Herring @ 2019-07-08 22:36 UTC (permalink / raw)
  To: Harish Jenny K N
  Cc: Linus Walleij, Bartosz Golaszewski, Mark Rutland, devicetree,
	open list:GPIO SUBSYSTEM, Balasubramani Vivekanandan

On Fri, Jun 28, 2019 at 3:31 AM Harish Jenny K N
<harish_kandiga@mentor.com> wrote:
>
> Document the device tree binding for the inverter gpio
> controller to configure the polarity of the gpio pins
> used by the consumers.
>
> Signed-off-by: Harish Jenny K N <harish_kandiga@mentor.com>
> ---
>  .../devicetree/bindings/gpio/gpio-inverter.txt     | 29 ++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-inverter.txt
>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-inverter.txt b/Documentation/devicetree/bindings/gpio/gpio-inverter.txt
> new file mode 100644
> index 0000000..8bb6b2e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-inverter.txt
> @@ -0,0 +1,29 @@
> +GPIO-INVERTER
> +======
> +This binding defines the gpio-inverter. The gpio-inverter is a driver that
> +allows to properly describe the gpio polarities on the hardware.

I don't understand. Please explain this in terms of the hardware, not a driver.

Rob

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH V4 2/2] gpio: inverter: document the inverter bindings
  2019-07-08 22:36 ` Rob Herring
@ 2019-07-09  5:25   ` Harish Jenny K N
  2019-07-09 16:08     ` Rob Herring
  0 siblings, 1 reply; 29+ messages in thread
From: Harish Jenny K N @ 2019-07-09  5:25 UTC (permalink / raw)
  To: Rob Herring
  Cc: Linus Walleij, Bartosz Golaszewski, Mark Rutland, devicetree,
	open list:GPIO SUBSYSTEM, Balasubramani Vivekanandan

Hi Rob,


On 09/07/19 4:06 AM, Rob Herring wrote:
> On Fri, Jun 28, 2019 at 3:31 AM Harish Jenny K N
> <harish_kandiga@mentor.com> wrote:
>> Document the device tree binding for the inverter gpio
>> controller to configure the polarity of the gpio pins
>> used by the consumers.
>>
>> Signed-off-by: Harish Jenny K N <harish_kandiga@mentor.com>
>> ---
>>  .../devicetree/bindings/gpio/gpio-inverter.txt     | 29 ++++++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-inverter.txt
>>
>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-inverter.txt b/Documentation/devicetree/bindings/gpio/gpio-inverter.txt
>> new file mode 100644
>> index 0000000..8bb6b2e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpio/gpio-inverter.txt
>> @@ -0,0 +1,29 @@
>> +GPIO-INVERTER
>> +======
>> +This binding defines the gpio-inverter. The gpio-inverter is a driver that
>> +allows to properly describe the gpio polarities on the hardware.
> I don't understand. Please explain this in terms of the hardware, not a driver.


gpio inverters can be used on different hardware to alter the polarity of gpio chips.
The polarity of pins can change from hardware to hardware with the use of inverters.
This device tree binding models gpio inverters in the device tree to properly describe the hardware.

Please let me know if this is enough and needs to be updated in the documentation patch.


I am sorry I did not include device tree list in the original discussion ( i.e first version of the patch
https://www.spinics.net/lists/linux-gpio/msg39681.html).


Thanks.


Best Regards,

Harish Jenny K N



^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH V4 2/2] gpio: inverter: document the inverter bindings
  2019-07-09  5:25   ` Harish Jenny K N
@ 2019-07-09 16:08     ` Rob Herring
  2019-07-10  8:28       ` Harish Jenny K N
  0 siblings, 1 reply; 29+ messages in thread
From: Rob Herring @ 2019-07-09 16:08 UTC (permalink / raw)
  To: Harish Jenny K N
  Cc: Linus Walleij, Bartosz Golaszewski, Mark Rutland, devicetree,
	open list:GPIO SUBSYSTEM, Balasubramani Vivekanandan

On Mon, Jul 8, 2019 at 11:25 PM Harish Jenny K N
<harish_kandiga@mentor.com> wrote:
>
> Hi Rob,
>
>
> On 09/07/19 4:06 AM, Rob Herring wrote:
> > On Fri, Jun 28, 2019 at 3:31 AM Harish Jenny K N
> > <harish_kandiga@mentor.com> wrote:
> >> Document the device tree binding for the inverter gpio
> >> controller to configure the polarity of the gpio pins
> >> used by the consumers.
> >>
> >> Signed-off-by: Harish Jenny K N <harish_kandiga@mentor.com>
> >> ---
> >>  .../devicetree/bindings/gpio/gpio-inverter.txt     | 29 ++++++++++++++++++++++
> >>  1 file changed, 29 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-inverter.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/gpio/gpio-inverter.txt b/Documentation/devicetree/bindings/gpio/gpio-inverter.txt
> >> new file mode 100644
> >> index 0000000..8bb6b2e
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/gpio/gpio-inverter.txt
> >> @@ -0,0 +1,29 @@
> >> +GPIO-INVERTER
> >> +======
> >> +This binding defines the gpio-inverter. The gpio-inverter is a driver that
> >> +allows to properly describe the gpio polarities on the hardware.
> > I don't understand. Please explain this in terms of the hardware, not a driver.
>
>
> gpio inverters can be used on different hardware to alter the polarity of gpio chips.
> The polarity of pins can change from hardware to hardware with the use of inverters.

Yes, I know what an inverter is.

> This device tree binding models gpio inverters in the device tree to properly describe the hardware.

We already define the active state of GPIOs in the consumers. If
there's an inverter in the middle, the consumer active state is simply
inverted. I don't agree that that is a hack as Linus said without some
reasoning why an inverter needs to be modeled in DT. Anything about
what 'userspace' needs is not a reason. That's a Linux thing that has
little to do with hardware description.

Rob

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH V4 2/2] gpio: inverter: document the inverter bindings
  2019-07-09 16:08     ` Rob Herring
@ 2019-07-10  8:28       ` Harish Jenny K N
  2019-07-17 13:51         ` Harish Jenny K N
  2019-08-05 11:15         ` Linus Walleij
  0 siblings, 2 replies; 29+ messages in thread
From: Harish Jenny K N @ 2019-07-10  8:28 UTC (permalink / raw)
  To: Rob Herring
  Cc: Linus Walleij, Bartosz Golaszewski, Mark Rutland, devicetree,
	open list:GPIO SUBSYSTEM, Balasubramani Vivekanandan

Hi,

On 09/07/19 9:38 PM, Rob Herring wrote:
> On Mon, Jul 8, 2019 at 11:25 PM Harish Jenny K N
> <harish_kandiga@mentor.com> wrote:
>> Hi Rob,
>>
>>
>> On 09/07/19 4:06 AM, Rob Herring wrote:
>>> On Fri, Jun 28, 2019 at 3:31 AM Harish Jenny K N
>>> <harish_kandiga@mentor.com> wrote:
>>>> Document the device tree binding for the inverter gpio
>>>> controller to configure the polarity of the gpio pins
>>>> used by the consumers.
>>>>
>>>> Signed-off-by: Harish Jenny K N <harish_kandiga@mentor.com>
>>>> ---
>>>>  .../devicetree/bindings/gpio/gpio-inverter.txt     | 29 ++++++++++++++++++++++
>>>>  1 file changed, 29 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-inverter.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-inverter.txt b/Documentation/devicetree/bindings/gpio/gpio-inverter.txt
>>>> new file mode 100644
>>>> index 0000000..8bb6b2e
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/gpio/gpio-inverter.txt
>>>> @@ -0,0 +1,29 @@
>>>> +GPIO-INVERTER
>>>> +======
>>>> +This binding defines the gpio-inverter. The gpio-inverter is a driver that
>>>> +allows to properly describe the gpio polarities on the hardware.
>>> I don't understand. Please explain this in terms of the hardware, not a driver.
>>
>> gpio inverters can be used on different hardware to alter the polarity of gpio chips.
>> The polarity of pins can change from hardware to hardware with the use of inverters.
> Yes, I know what an inverter is.
>
>> This device tree binding models gpio inverters in the device tree to properly describe the hardware.
> We already define the active state of GPIOs in the consumers. If
> there's an inverter in the middle, the consumer active state is simply
> inverted. I don't agree that that is a hack as Linus said without some
> reasoning why an inverter needs to be modeled in DT. Anything about
> what 'userspace' needs is not a reason. That's a Linux thing that has
> little to do with hardware description.


Yes we are talking about the hardware level inversions here. The usecase is for those without the gpio consumer driver. The usecase started with the concept of allowing an abstraction of the underlying hardware for the userland controlling program such that this program does not care whether the GPIO lines are inverted or not physically. In other words, a single userland controlling program can work unmodified across a variety of hardware platforms with the device tree mapping the logical to physical relationship of the GPIO hardware.
I totally understand anything about what 'userspace' needs is not a reason, but this is not restricted to userspace alone as kernel drivers may need this just as much. Also we are just modelling/describing the hardware state in the device tree.

Just to mention that Linus Walleij had proposed this inverter model to describe the hardware and the gpio inverter driver is developed based on comments/review from him.

Also my sincere request to Linus Walleij to please let his opinion know on this.

Thanks,

Best Regards,
Harish Jenny K N




^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH V4 2/2] gpio: inverter: document the inverter bindings
  2019-07-10  8:28       ` Harish Jenny K N
@ 2019-07-17 13:51         ` Harish Jenny K N
  2019-07-29 11:07           ` Harish Jenny K N
  2019-08-05 11:15         ` Linus Walleij
  1 sibling, 1 reply; 29+ messages in thread
From: Harish Jenny K N @ 2019-07-17 13:51 UTC (permalink / raw)
  To: Rob Herring, Linus Walleij
  Cc: Bartosz Golaszewski, Mark Rutland, devicetree,
	open list:GPIO SUBSYSTEM, Balasubramani Vivekanandan

Hi Linus,

On 10/07/19 1:58 PM, Harish Jenny K N wrote:
> Hi,
>
> On 09/07/19 9:38 PM, Rob Herring wrote:
>> On Mon, Jul 8, 2019 at 11:25 PM Harish Jenny K N
>> <harish_kandiga@mentor.com> wrote:
>>> Hi Rob,
>>>
>>>
>>> On 09/07/19 4:06 AM, Rob Herring wrote:
>>>> On Fri, Jun 28, 2019 at 3:31 AM Harish Jenny K N
>>>> <harish_kandiga@mentor.com> wrote:
>>>>> Document the device tree binding for the inverter gpio
>>>>> controller to configure the polarity of the gpio pins
>>>>> used by the consumers.
>>>>>
>>>>> Signed-off-by: Harish Jenny K N <harish_kandiga@mentor.com>
>>>>> ---
>>>>>  .../devicetree/bindings/gpio/gpio-inverter.txt     | 29 ++++++++++++++++++++++
>>>>>  1 file changed, 29 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-inverter.txt
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-inverter.txt b/Documentation/devicetree/bindings/gpio/gpio-inverter.txt
>>>>> new file mode 100644
>>>>> index 0000000..8bb6b2e
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/gpio/gpio-inverter.txt
>>>>> @@ -0,0 +1,29 @@
>>>>> +GPIO-INVERTER
>>>>> +======
>>>>> +This binding defines the gpio-inverter. The gpio-inverter is a driver that
>>>>> +allows to properly describe the gpio polarities on the hardware.
>>>> I don't understand. Please explain this in terms of the hardware, not a driver.
>>> gpio inverters can be used on different hardware to alter the polarity of gpio chips.
>>> The polarity of pins can change from hardware to hardware with the use of inverters.
>> Yes, I know what an inverter is.
>>
>>> This device tree binding models gpio inverters in the device tree to properly describe the hardware.
>> We already define the active state of GPIOs in the consumers. If
>> there's an inverter in the middle, the consumer active state is simply
>> inverted. I don't agree that that is a hack as Linus said without some
>> reasoning why an inverter needs to be modeled in DT. Anything about
>> what 'userspace' needs is not a reason. That's a Linux thing that has
>> little to do with hardware description.
>
> Yes we are talking about the hardware level inversions here. The usecase is for those without the gpio consumer driver. The usecase started with the concept of allowing an abstraction of the underlying hardware for the userland controlling program such that this program does not care whether the GPIO lines are inverted or not physically. In other words, a single userland controlling program can work unmodified across a variety of hardware platforms with the device tree mapping the logical to physical relationship of the GPIO hardware.
> I totally understand anything about what 'userspace' needs is not a reason, but this is not restricted to userspace alone as kernel drivers may need this just as much. Also we are just modelling/describing the hardware state in the device tree.
>
> Just to mention that Linus Walleij had proposed this inverter model to describe the hardware and the gpio inverter driver is developed based on comments/review from him.
>
> Also my sincere request to Linus Walleij to please let his opinion know on this.
>
> Thanks,
>
> Best Regards,
> Harish Jenny K N


Can you please give your opinion on this.


Thanks.


Best Regards,

Harish Jenny K N

>


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH V4 2/2] gpio: inverter: document the inverter bindings
  2019-07-17 13:51         ` Harish Jenny K N
@ 2019-07-29 11:07           ` Harish Jenny K N
  0 siblings, 0 replies; 29+ messages in thread
From: Harish Jenny K N @ 2019-07-29 11:07 UTC (permalink / raw)
  To: Rob Herring, Linus Walleij
  Cc: Bartosz Golaszewski, Mark Rutland, devicetree,
	open list:GPIO SUBSYSTEM, Balasubramani Vivekanandan

Hi Linus,

On 17/07/19 7:21 PM, Harish Jenny K N wrote:
> Hi Linus,
>
> On 10/07/19 1:58 PM, Harish Jenny K N wrote:
>> Hi,
>>
>> On 09/07/19 9:38 PM, Rob Herring wrote:
>>> On Mon, Jul 8, 2019 at 11:25 PM Harish Jenny K N
>>> <harish_kandiga@mentor.com> wrote:
>>>> Hi Rob,
>>>>
>>>>
>>>> On 09/07/19 4:06 AM, Rob Herring wrote:
>>>>> On Fri, Jun 28, 2019 at 3:31 AM Harish Jenny K N
>>>>> <harish_kandiga@mentor.com> wrote:
>>>>>> Document the device tree binding for the inverter gpio
>>>>>> controller to configure the polarity of the gpio pins
>>>>>> used by the consumers.
>>>>>>
>>>>>> Signed-off-by: Harish Jenny K N <harish_kandiga@mentor.com>
>>>>>> ---
>>>>>>  .../devicetree/bindings/gpio/gpio-inverter.txt     | 29 ++++++++++++++++++++++
>>>>>>  1 file changed, 29 insertions(+)
>>>>>>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-inverter.txt
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-inverter.txt b/Documentation/devicetree/bindings/gpio/gpio-inverter.txt
>>>>>> new file mode 100644
>>>>>> index 0000000..8bb6b2e
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/gpio/gpio-inverter.txt
>>>>>> @@ -0,0 +1,29 @@
>>>>>> +GPIO-INVERTER
>>>>>> +======
>>>>>> +This binding defines the gpio-inverter. The gpio-inverter is a driver that
>>>>>> +allows to properly describe the gpio polarities on the hardware.
>>>>> I don't understand. Please explain this in terms of the hardware, not a driver.
>>>> gpio inverters can be used on different hardware to alter the polarity of gpio chips.
>>>> The polarity of pins can change from hardware to hardware with the use of inverters.
>>> Yes, I know what an inverter is.
>>>
>>>> This device tree binding models gpio inverters in the device tree to properly describe the hardware.
>>> We already define the active state of GPIOs in the consumers. If
>>> there's an inverter in the middle, the consumer active state is simply
>>> inverted. I don't agree that that is a hack as Linus said without some
>>> reasoning why an inverter needs to be modeled in DT. Anything about
>>> what 'userspace' needs is not a reason. That's a Linux thing that has
>>> little to do with hardware description.
>> Yes we are talking about the hardware level inversions here. The usecase is for those without the gpio consumer driver. The usecase started with the concept of allowing an abstraction of the underlying hardware for the userland controlling program such that this program does not care whether the GPIO lines are inverted or not physically. In other words, a single userland controlling program can work unmodified across a variety of hardware platforms with the device tree mapping the logical to physical relationship of the GPIO hardware.
>> I totally understand anything about what 'userspace' needs is not a reason, but this is not restricted to userspace alone as kernel drivers may need this just as much. Also we are just modelling/describing the hardware state in the device tree.
>>
>> Just to mention that Linus Walleij had proposed this inverter model to describe the hardware and the gpio inverter driver is developed based on comments/review from him.
>>
>> Also my sincere request to Linus Walleij to please let his opinion know on this.
>>
>> Thanks,
>>
>> Best Regards,
>> Harish Jenny K N
>
> Can you please give your opinion on this.
>
>
> Thanks.
>
>
> Best Regards,
>
> Harish Jenny K N
>

sorry for the repeated mail. can you please give your opinion on this ?

Thanks.

Harish



^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH V4 2/2] gpio: inverter: document the inverter bindings
  2019-07-10  8:28       ` Harish Jenny K N
  2019-07-17 13:51         ` Harish Jenny K N
@ 2019-08-05 11:15         ` Linus Walleij
  2019-08-09 14:08           ` Rob Herring
  1 sibling, 1 reply; 29+ messages in thread
From: Linus Walleij @ 2019-08-05 11:15 UTC (permalink / raw)
  To: Harish Jenny K N
  Cc: Rob Herring, Bartosz Golaszewski, Mark Rutland,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list:GPIO SUBSYSTEM, Balasubramani Vivekanandan

On Wed, Jul 10, 2019 at 10:28 AM Harish Jenny K N
<harish_kandiga@mentor.com> wrote:
> On 09/07/19 9:38 PM, Rob Herring wrote:

> >> This device tree binding models gpio inverters in the device tree to properly describe the hardware.
> >
> > We already define the active state of GPIOs in the consumers. If
> > there's an inverter in the middle, the consumer active state is simply
> > inverted. I don't agree that that is a hack as Linus said without some
> > reasoning why an inverter needs to be modeled in DT. Anything about
> > what 'userspace' needs is not a reason. That's a Linux thing that has
> > little to do with hardware description.

There is some level of ambition here which is inherently a bit fuzzy
around the edges. ("How long is the coast of Britain?" comes to mind.)

Surely the intention of device tree is not to recreate the schematic
in all detail. What we want is a model of the hardware that will
suffice for the operating system usecases.

But sometimes the DTS files will become confusing: why is this
component using GPIO_ACTIVE_LOW when another system
doesn't have that flag? If there is an explicit inverter, the
DTS gets more readable for a human.

But arguable that is case for adding inverters as syntactic
sugar in the DTS compiler instead...

> Yes we are talking about the hardware level inversions here.
> The usecase is for those without the gpio consumer driver.
> The usecase started with the concept of allowing an abstraction
> of the underlying hardware for the userland controlling program
> such that this program does not care whether the GPIO lines
> are inverted or not physically. In other words, a single userland
> controlling program can work unmodified across a variety of
> hardware platforms with the device tree mapping the logical
> to physical relationship of the GPIO hardware.
> I totally understand anything about what 'userspace' needs is
> not a reason, but this is not restricted to userspace alone as
> kernel drivers may need this just as much. Also we are
> just modelling/describing the hardware state in the device tree.

The kernel also has a need to model inverters and it has come
up from time to time, but I don't remember these instances
right off the top of my head.

I am not sure userspace needs are of zero concerns either.

Sure, for anything reimplementing what I have listed in
Documentation/driver-api/gpio/drivers-on-gpio.rst
it is just abuse of the ABI, but things like industrial control
systems and other one-offs have this need to run the
same binary unmodified for measuring the trigger level
of water in some tank or so, they can't create kernel
drivers for that kind of stuff.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH V4 2/2] gpio: inverter: document the inverter bindings
  2019-08-05 11:15         ` Linus Walleij
@ 2019-08-09 14:08           ` Rob Herring
  2019-08-10  8:51             ` Linus Walleij
  0 siblings, 1 reply; 29+ messages in thread
From: Rob Herring @ 2019-08-09 14:08 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Harish Jenny K N, Bartosz Golaszewski, Mark Rutland,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list:GPIO SUBSYSTEM, Balasubramani Vivekanandan

On Mon, Aug 5, 2019 at 5:15 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Wed, Jul 10, 2019 at 10:28 AM Harish Jenny K N
> <harish_kandiga@mentor.com> wrote:
> > On 09/07/19 9:38 PM, Rob Herring wrote:
>
> > >> This device tree binding models gpio inverters in the device tree to properly describe the hardware.
> > >
> > > We already define the active state of GPIOs in the consumers. If
> > > there's an inverter in the middle, the consumer active state is simply
> > > inverted. I don't agree that that is a hack as Linus said without some
> > > reasoning why an inverter needs to be modeled in DT. Anything about
> > > what 'userspace' needs is not a reason. That's a Linux thing that has
> > > little to do with hardware description.
>
> There is some level of ambition here which is inherently a bit fuzzy
> around the edges. ("How long is the coast of Britain?" comes to mind.)
>
> Surely the intention of device tree is not to recreate the schematic
> in all detail. What we want is a model of the hardware that will
> suffice for the operating system usecases.
>
> But sometimes the DTS files will become confusing: why is this
> component using GPIO_ACTIVE_LOW when another system
> doesn't have that flag? If there is an explicit inverter, the
> DTS gets more readable for a human.
>
> But arguable that is case for adding inverters as syntactic
> sugar in the DTS compiler instead...

If you really want something more explicit, then add a new GPIO
'inverted' flag. Then a device can always have the same HIGH/LOW flag.
That also solves the abstract it for userspace problem.

> > Yes we are talking about the hardware level inversions here.
> > The usecase is for those without the gpio consumer driver.
> > The usecase started with the concept of allowing an abstraction
> > of the underlying hardware for the userland controlling program
> > such that this program does not care whether the GPIO lines
> > are inverted or not physically. In other words, a single userland
> > controlling program can work unmodified across a variety of
> > hardware platforms with the device tree mapping the logical
> > to physical relationship of the GPIO hardware.
> > I totally understand anything about what 'userspace' needs is
> > not a reason, but this is not restricted to userspace alone as
> > kernel drivers may need this just as much. Also we are
> > just modelling/describing the hardware state in the device tree.
>
> The kernel also has a need to model inverters and it has come
> up from time to time, but I don't remember these instances
> right off the top of my head.

The only thing I can think of is an inverter needing its power supply
turned on. Seems a bit silly to have such fine grained control, but
who knows.

> I am not sure userspace needs are of zero concerns either.

No, but kernel vs. userspace is all a black box from a DT perspective
and not a distinction that we can design bindings around.

> Sure, for anything reimplementing what I have listed in
> Documentation/driver-api/gpio/drivers-on-gpio.rst
> it is just abuse of the ABI, but things like industrial control
> systems and other one-offs have this need to run the
> same binary unmodified for measuring the trigger level
> of water in some tank or so, they can't create kernel
> drivers for that kind of stuff.

The userspace interface already passes the flags for the gpio lines,
why can't a userspace program honor them? You can't have it both ways:
low level GPIO access and abstracted to not care about the details.

Rob

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH V4 2/2] gpio: inverter: document the inverter bindings
  2019-08-09 14:08           ` Rob Herring
@ 2019-08-10  8:51             ` Linus Walleij
  2019-08-19  9:36               ` Harish Jenny K N
  0 siblings, 1 reply; 29+ messages in thread
From: Linus Walleij @ 2019-08-10  8:51 UTC (permalink / raw)
  To: Rob Herring
  Cc: Harish Jenny K N, Bartosz Golaszewski, Mark Rutland,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list:GPIO SUBSYSTEM, Balasubramani Vivekanandan

On Fri, Aug 9, 2019 at 4:08 PM Rob Herring <robh+dt@kernel.org> wrote:
> On Mon, Aug 5, 2019 at 5:15 AM Linus Walleij <linus.walleij@linaro.org> wrote:

> > There is some level of ambition here which is inherently a bit fuzzy
> > around the edges. ("How long is the coast of Britain?" comes to mind.)
> >
> > Surely the intention of device tree is not to recreate the schematic
> > in all detail. What we want is a model of the hardware that will
> > suffice for the operating system usecases.
> >
> > But sometimes the DTS files will become confusing: why is this
> > component using GPIO_ACTIVE_LOW when another system
> > doesn't have that flag? If there is an explicit inverter, the
> > DTS gets more readable for a human.
> >
> > But arguable that is case for adding inverters as syntactic
> > sugar in the DTS compiler instead...
>
> If you really want something more explicit, then add a new GPIO
> 'inverted' flag. Then a device can always have the same HIGH/LOW flag.
> That also solves the abstract it for userspace problem.

I think there are some intricate ontologies at work here.

Consider this example: a GPIO is controlling a chip select
regulator, say Acme Foo. The chip select
has a pin named CSN. We know from convention that the
"N" at the end of that pin name means "negative" i.e. active
low, and that is how the electronics engineers think about
that chip select line: it activates the IC when
the line goes low.

The regulator subsystem and I think all subsystems in the
Linux kernel say the consumer pin should be named and
tagged after the datsheet of the regulator.

So it has for example:

foo {
    compatible = "acme,foo";
    cs-gpios = <&gpio0 6 GPIO_ACTIVE_LOW>;
};

(It would be inappropriate to name it "csn-gpios" since
we have an established flag for active low. But it is another
of these syntactic choices where people likely do mistakes.)

I think it would be appropriate for the DT binding to say
that this flag must always be GPIO_ACTIVE_LOW since
the bindings are seen from the component point of view,
and thus this is always active low.

It would even be reasonable for a yaml schema to enfore
this, if it could. It is defined as active low after all.

Now if someone adds an inverter on that line between
gpio0 and Acme Foo it looks like this:

foo {
    compatible = "acme,foo";
    cs-gpios = <&gpio0 6 GPIO_ACTIVE_HIGH>;
};

And now we get cognitive dissonance or whatever I should
call it: someone reading this DTS sheet and the data
sheet for the component Acme Foo to troubleshoot
this will be confused: this component has CS active
low and still it is specified as active high? Unless they
also look at the schematic or the board and find the
inverter things are pretty muddy and they will likely curse
and solve the situation with the usual trial-and-error,
inserting some random cursewords as a comment.

With an intermediate inverter node, the cs-gpios
can go back to GPIO_ACTIVE_LOW and follow
the bindings:

inv0: inverter {
    compatible = "gpio-inverter";
    gpio-controller;
    #gpio-cells = <1>;
    inverted-gpios = <&gpio0 6 GPIO_ACTIVE_HIGH>;
};

foo {
    compatible = "acme,foo";
    cs-gpios = <&inv0 0 GPIO_ACTIVE_LOW>;
};

And now Acme Foo bindings can keep enforcing cs-gpios
to always be tagged GPIO_ACTIVE_LOW.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH V4 2/2] gpio: inverter: document the inverter bindings
  2019-08-10  8:51             ` Linus Walleij
@ 2019-08-19  9:36               ` Harish Jenny K N
  2019-08-27  7:47                 ` Harish Jenny K N
  0 siblings, 1 reply; 29+ messages in thread
From: Harish Jenny K N @ 2019-08-19  9:36 UTC (permalink / raw)
  To: Linus Walleij, Rob Herring
  Cc: Bartosz Golaszewski, Mark Rutland,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list:GPIO SUBSYSTEM, Balasubramani Vivekanandan

Hi Rob,


On 10/08/19 2:21 PM, Linus Walleij wrote:
> On Fri, Aug 9, 2019 at 4:08 PM Rob Herring <robh+dt@kernel.org> wrote:
>> On Mon, Aug 5, 2019 at 5:15 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>>> There is some level of ambition here which is inherently a bit fuzzy
>>> around the edges. ("How long is the coast of Britain?" comes to mind.)
>>>
>>> Surely the intention of device tree is not to recreate the schematic
>>> in all detail. What we want is a model of the hardware that will
>>> suffice for the operating system usecases.
>>>
>>> But sometimes the DTS files will become confusing: why is this
>>> component using GPIO_ACTIVE_LOW when another system
>>> doesn't have that flag? If there is an explicit inverter, the
>>> DTS gets more readable for a human.
>>>
>>> But arguable that is case for adding inverters as syntactic
>>> sugar in the DTS compiler instead...
>> If you really want something more explicit, then add a new GPIO
>> 'inverted' flag. Then a device can always have the same HIGH/LOW flag.
>> That also solves the abstract it for userspace problem.
> I think there are some intricate ontologies at work here.
>
> Consider this example: a GPIO is controlling a chip select
> regulator, say Acme Foo. The chip select
> has a pin named CSN. We know from convention that the
> "N" at the end of that pin name means "negative" i.e. active
> low, and that is how the electronics engineers think about
> that chip select line: it activates the IC when
> the line goes low.
>
> The regulator subsystem and I think all subsystems in the
> Linux kernel say the consumer pin should be named and
> tagged after the datsheet of the regulator.
>
> So it has for example:
>
> foo {
>     compatible = "acme,foo";
>     cs-gpios = <&gpio0 6 GPIO_ACTIVE_LOW>;
> };
>
> (It would be inappropriate to name it "csn-gpios" since
> we have an established flag for active low. But it is another
> of these syntactic choices where people likely do mistakes.)
>
> I think it would be appropriate for the DT binding to say
> that this flag must always be GPIO_ACTIVE_LOW since
> the bindings are seen from the component point of view,
> and thus this is always active low.
>
> It would even be reasonable for a yaml schema to enfore
> this, if it could. It is defined as active low after all.
>
> Now if someone adds an inverter on that line between
> gpio0 and Acme Foo it looks like this:
>
> foo {
>     compatible = "acme,foo";
>     cs-gpios = <&gpio0 6 GPIO_ACTIVE_HIGH>;
> };
>
> And now we get cognitive dissonance or whatever I should
> call it: someone reading this DTS sheet and the data
> sheet for the component Acme Foo to troubleshoot
> this will be confused: this component has CS active
> low and still it is specified as active high? Unless they
> also look at the schematic or the board and find the
> inverter things are pretty muddy and they will likely curse
> and solve the situation with the usual trial-and-error,
> inserting some random cursewords as a comment.
>
> With an intermediate inverter node, the cs-gpios
> can go back to GPIO_ACTIVE_LOW and follow
> the bindings:
>
> inv0: inverter {
>     compatible = "gpio-inverter";
>     gpio-controller;
>     #gpio-cells = <1>;
>     inverted-gpios = <&gpio0 6 GPIO_ACTIVE_HIGH>;
> };
>
> foo {
>     compatible = "acme,foo";
>     cs-gpios = <&inv0 0 GPIO_ACTIVE_LOW>;
> };
>
> And now Acme Foo bindings can keep enforcing cs-gpios
> to always be tagged GPIO_ACTIVE_LOW.


Can you please review/let us know your opinion on this ? I think the idea here is to also isolate the changes to a separate consumer driver and avoid getting inversions inside gpiolib.


Thanks.


Regards,

Harish Jenny K N



^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH V4 2/2] gpio: inverter: document the inverter bindings
  2019-08-19  9:36               ` Harish Jenny K N
@ 2019-08-27  7:47                 ` Harish Jenny K N
  2019-08-30  5:21                   ` Harish Jenny K N
  0 siblings, 1 reply; 29+ messages in thread
From: Harish Jenny K N @ 2019-08-27  7:47 UTC (permalink / raw)
  To: Linus Walleij, Rob Herring
  Cc: Bartosz Golaszewski, Mark Rutland,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list:GPIO SUBSYSTEM, Balasubramani Vivekanandan

Hi Rob,


On 19/08/19 3:06 PM, Harish Jenny K N wrote:
> Hi Rob,
>
>
> On 10/08/19 2:21 PM, Linus Walleij wrote:
>> On Fri, Aug 9, 2019 at 4:08 PM Rob Herring <robh+dt@kernel.org> wrote:
>>> On Mon, Aug 5, 2019 at 5:15 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>>>> There is some level of ambition here which is inherently a bit fuzzy
>>>> around the edges. ("How long is the coast of Britain?" comes to mind.)
>>>>
>>>> Surely the intention of device tree is not to recreate the schematic
>>>> in all detail. What we want is a model of the hardware that will
>>>> suffice for the operating system usecases.
>>>>
>>>> But sometimes the DTS files will become confusing: why is this
>>>> component using GPIO_ACTIVE_LOW when another system
>>>> doesn't have that flag? If there is an explicit inverter, the
>>>> DTS gets more readable for a human.
>>>>
>>>> But arguable that is case for adding inverters as syntactic
>>>> sugar in the DTS compiler instead...
>>> If you really want something more explicit, then add a new GPIO
>>> 'inverted' flag. Then a device can always have the same HIGH/LOW flag.
>>> That also solves the abstract it for userspace problem.
>> I think there are some intricate ontologies at work here.
>>
>> Consider this example: a GPIO is controlling a chip select
>> regulator, say Acme Foo. The chip select
>> has a pin named CSN. We know from convention that the
>> "N" at the end of that pin name means "negative" i.e. active
>> low, and that is how the electronics engineers think about
>> that chip select line: it activates the IC when
>> the line goes low.
>>
>> The regulator subsystem and I think all subsystems in the
>> Linux kernel say the consumer pin should be named and
>> tagged after the datsheet of the regulator.
>>
>> So it has for example:
>>
>> foo {
>>     compatible = "acme,foo";
>>     cs-gpios = <&gpio0 6 GPIO_ACTIVE_LOW>;
>> };
>>
>> (It would be inappropriate to name it "csn-gpios" since
>> we have an established flag for active low. But it is another
>> of these syntactic choices where people likely do mistakes.)
>>
>> I think it would be appropriate for the DT binding to say
>> that this flag must always be GPIO_ACTIVE_LOW since
>> the bindings are seen from the component point of view,
>> and thus this is always active low.
>>
>> It would even be reasonable for a yaml schema to enfore
>> this, if it could. It is defined as active low after all.
>>
>> Now if someone adds an inverter on that line between
>> gpio0 and Acme Foo it looks like this:
>>
>> foo {
>>     compatible = "acme,foo";
>>     cs-gpios = <&gpio0 6 GPIO_ACTIVE_HIGH>;
>> };
>>
>> And now we get cognitive dissonance or whatever I should
>> call it: someone reading this DTS sheet and the data
>> sheet for the component Acme Foo to troubleshoot
>> this will be confused: this component has CS active
>> low and still it is specified as active high? Unless they
>> also look at the schematic or the board and find the
>> inverter things are pretty muddy and they will likely curse
>> and solve the situation with the usual trial-and-error,
>> inserting some random cursewords as a comment.
>>
>> With an intermediate inverter node, the cs-gpios
>> can go back to GPIO_ACTIVE_LOW and follow
>> the bindings:
>>
>> inv0: inverter {
>>     compatible = "gpio-inverter";
>>     gpio-controller;
>>     #gpio-cells = <1>;
>>     inverted-gpios = <&gpio0 6 GPIO_ACTIVE_HIGH>;
>> };
>>
>> foo {
>>     compatible = "acme,foo";
>>     cs-gpios = <&inv0 0 GPIO_ACTIVE_LOW>;
>> };
>>
>> And now Acme Foo bindings can keep enforcing cs-gpios
>> to always be tagged GPIO_ACTIVE_LOW.
>
> Can you please review/let us know your opinion on this ? I think the idea here is to also isolate the changes to a separate consumer driver and avoid getting inversions inside gpiolib.
>
>
> Thanks.
>
>
> Regards,
>
> Harish Jenny K N
>

Can you please comment on this ?


Thanks,

Harish Jenny K N



^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH V4 2/2] gpio: inverter: document the inverter bindings
  2019-08-27  7:47                 ` Harish Jenny K N
@ 2019-08-30  5:21                   ` Harish Jenny K N
  2019-09-04  4:58                     ` Harish Jenny K N
  0 siblings, 1 reply; 29+ messages in thread
From: Harish Jenny K N @ 2019-08-30  5:21 UTC (permalink / raw)
  To: Linus Walleij, Rob Herring
  Cc: Bartosz Golaszewski, Mark Rutland,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list:GPIO SUBSYSTEM, Balasubramani Vivekanandan

Hi Rob,


On 27/08/19 1:17 PM, Harish Jenny K N wrote:
> Hi Rob,
>
>
> On 19/08/19 3:06 PM, Harish Jenny K N wrote:
>> Hi Rob,
>>
>>
>> On 10/08/19 2:21 PM, Linus Walleij wrote:
>>> On Fri, Aug 9, 2019 at 4:08 PM Rob Herring <robh+dt@kernel.org> wrote:
>>>> On Mon, Aug 5, 2019 at 5:15 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>>>>> There is some level of ambition here which is inherently a bit fuzzy
>>>>> around the edges. ("How long is the coast of Britain?" comes to mind.)
>>>>>
>>>>> Surely the intention of device tree is not to recreate the schematic
>>>>> in all detail. What we want is a model of the hardware that will
>>>>> suffice for the operating system usecases.
>>>>>
>>>>> But sometimes the DTS files will become confusing: why is this
>>>>> component using GPIO_ACTIVE_LOW when another system
>>>>> doesn't have that flag? If there is an explicit inverter, the
>>>>> DTS gets more readable for a human.
>>>>>
>>>>> But arguable that is case for adding inverters as syntactic
>>>>> sugar in the DTS compiler instead...
>>>> If you really want something more explicit, then add a new GPIO
>>>> 'inverted' flag. Then a device can always have the same HIGH/LOW flag.
>>>> That also solves the abstract it for userspace problem.
>>> I think there are some intricate ontologies at work here.
>>>
>>> Consider this example: a GPIO is controlling a chip select
>>> regulator, say Acme Foo. The chip select
>>> has a pin named CSN. We know from convention that the
>>> "N" at the end of that pin name means "negative" i.e. active
>>> low, and that is how the electronics engineers think about
>>> that chip select line: it activates the IC when
>>> the line goes low.
>>>
>>> The regulator subsystem and I think all subsystems in the
>>> Linux kernel say the consumer pin should be named and
>>> tagged after the datsheet of the regulator.
>>>
>>> So it has for example:
>>>
>>> foo {
>>>     compatible = "acme,foo";
>>>     cs-gpios = <&gpio0 6 GPIO_ACTIVE_LOW>;
>>> };
>>>
>>> (It would be inappropriate to name it "csn-gpios" since
>>> we have an established flag for active low. But it is another
>>> of these syntactic choices where people likely do mistakes.)
>>>
>>> I think it would be appropriate for the DT binding to say
>>> that this flag must always be GPIO_ACTIVE_LOW since
>>> the bindings are seen from the component point of view,
>>> and thus this is always active low.
>>>
>>> It would even be reasonable for a yaml schema to enfore
>>> this, if it could. It is defined as active low after all.
>>>
>>> Now if someone adds an inverter on that line between
>>> gpio0 and Acme Foo it looks like this:
>>>
>>> foo {
>>>     compatible = "acme,foo";
>>>     cs-gpios = <&gpio0 6 GPIO_ACTIVE_HIGH>;
>>> };
>>>
>>> And now we get cognitive dissonance or whatever I should
>>> call it: someone reading this DTS sheet and the data
>>> sheet for the component Acme Foo to troubleshoot
>>> this will be confused: this component has CS active
>>> low and still it is specified as active high? Unless they
>>> also look at the schematic or the board and find the
>>> inverter things are pretty muddy and they will likely curse
>>> and solve the situation with the usual trial-and-error,
>>> inserting some random cursewords as a comment.
>>>
>>> With an intermediate inverter node, the cs-gpios
>>> can go back to GPIO_ACTIVE_LOW and follow
>>> the bindings:
>>>
>>> inv0: inverter {
>>>     compatible = "gpio-inverter";
>>>     gpio-controller;
>>>     #gpio-cells = <1>;
>>>     inverted-gpios = <&gpio0 6 GPIO_ACTIVE_HIGH>;
>>> };
>>>
>>> foo {
>>>     compatible = "acme,foo";
>>>     cs-gpios = <&inv0 0 GPIO_ACTIVE_LOW>;
>>> };
>>>
>>> And now Acme Foo bindings can keep enforcing cs-gpios
>>> to always be tagged GPIO_ACTIVE_LOW.
>> Can you please review/let us know your opinion on this ? I think the idea here is to also isolate the changes to a separate consumer driver and avoid getting inversions inside gpiolib.
>>
>>
>> Thanks.
>>
>>
>> Regards,
>>
>> Harish Jenny K N
>>
> Can you please comment on this ?
>
>
> Thanks,
>
> Harish Jenny K N
>

Friendly Reminder.

can we please finalize this ?

Linus has also mentioned in another patchset "[PATCH v2] Input: tsc2007 - use GPIO descriptor" that

he is in favor of introducing explicit inverters in device tree.


Please consider this and let us know your inputs.



Thanks,

Harish Jenny K N



^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH V4 2/2] gpio: inverter: document the inverter bindings
  2019-08-30  5:21                   ` Harish Jenny K N
@ 2019-09-04  4:58                     ` Harish Jenny K N
  2019-09-10  7:47                       ` Rob Herring
  0 siblings, 1 reply; 29+ messages in thread
From: Harish Jenny K N @ 2019-09-04  4:58 UTC (permalink / raw)
  To: Linus Walleij, Rob Herring, robh
  Cc: Bartosz Golaszewski, Mark Rutland,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list:GPIO SUBSYSTEM, Balasubramani Vivekanandan

Hi Rob, Hi Linus,


On 30/08/19 10:51 AM, Harish Jenny K N wrote:
> Hi Rob,
>
>
> On 27/08/19 1:17 PM, Harish Jenny K N wrote:
>> Hi Rob,
>>
>>
>> On 19/08/19 3:06 PM, Harish Jenny K N wrote:
>>> Hi Rob,
>>>
>>>
>>> On 10/08/19 2:21 PM, Linus Walleij wrote:
>>>> On Fri, Aug 9, 2019 at 4:08 PM Rob Herring <robh+dt@kernel.org> wrote:
>>>>> On Mon, Aug 5, 2019 at 5:15 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>>>>>> There is some level of ambition here which is inherently a bit fuzzy
>>>>>> around the edges. ("How long is the coast of Britain?" comes to mind.)
>>>>>>
>>>>>> Surely the intention of device tree is not to recreate the schematic
>>>>>> in all detail. What we want is a model of the hardware that will
>>>>>> suffice for the operating system usecases.
>>>>>>
>>>>>> But sometimes the DTS files will become confusing: why is this
>>>>>> component using GPIO_ACTIVE_LOW when another system
>>>>>> doesn't have that flag? If there is an explicit inverter, the
>>>>>> DTS gets more readable for a human.
>>>>>>
>>>>>> But arguable that is case for adding inverters as syntactic
>>>>>> sugar in the DTS compiler instead...
>>>>> If you really want something more explicit, then add a new GPIO
>>>>> 'inverted' flag. Then a device can always have the same HIGH/LOW flag.
>>>>> That also solves the abstract it for userspace problem.
>>>> I think there are some intricate ontologies at work here.
>>>>
>>>> Consider this example: a GPIO is controlling a chip select
>>>> regulator, say Acme Foo. The chip select
>>>> has a pin named CSN. We know from convention that the
>>>> "N" at the end of that pin name means "negative" i.e. active
>>>> low, and that is how the electronics engineers think about
>>>> that chip select line: it activates the IC when
>>>> the line goes low.
>>>>
>>>> The regulator subsystem and I think all subsystems in the
>>>> Linux kernel say the consumer pin should be named and
>>>> tagged after the datsheet of the regulator.
>>>>
>>>> So it has for example:
>>>>
>>>> foo {
>>>>     compatible = "acme,foo";
>>>>     cs-gpios = <&gpio0 6 GPIO_ACTIVE_LOW>;
>>>> };
>>>>
>>>> (It would be inappropriate to name it "csn-gpios" since
>>>> we have an established flag for active low. But it is another
>>>> of these syntactic choices where people likely do mistakes.)
>>>>
>>>> I think it would be appropriate for the DT binding to say
>>>> that this flag must always be GPIO_ACTIVE_LOW since
>>>> the bindings are seen from the component point of view,
>>>> and thus this is always active low.
>>>>
>>>> It would even be reasonable for a yaml schema to enfore
>>>> this, if it could. It is defined as active low after all.
>>>>
>>>> Now if someone adds an inverter on that line between
>>>> gpio0 and Acme Foo it looks like this:
>>>>
>>>> foo {
>>>>     compatible = "acme,foo";
>>>>     cs-gpios = <&gpio0 6 GPIO_ACTIVE_HIGH>;
>>>> };
>>>>
>>>> And now we get cognitive dissonance or whatever I should
>>>> call it: someone reading this DTS sheet and the data
>>>> sheet for the component Acme Foo to troubleshoot
>>>> this will be confused: this component has CS active
>>>> low and still it is specified as active high? Unless they
>>>> also look at the schematic or the board and find the
>>>> inverter things are pretty muddy and they will likely curse
>>>> and solve the situation with the usual trial-and-error,
>>>> inserting some random cursewords as a comment.
>>>>
>>>> With an intermediate inverter node, the cs-gpios
>>>> can go back to GPIO_ACTIVE_LOW and follow
>>>> the bindings:
>>>>
>>>> inv0: inverter {
>>>>     compatible = "gpio-inverter";
>>>>     gpio-controller;
>>>>     #gpio-cells = <1>;
>>>>     inverted-gpios = <&gpio0 6 GPIO_ACTIVE_HIGH>;
>>>> };
>>>>
>>>> foo {
>>>>     compatible = "acme,foo";
>>>>     cs-gpios = <&inv0 0 GPIO_ACTIVE_LOW>;
>>>> };
>>>>
>>>> And now Acme Foo bindings can keep enforcing cs-gpios
>>>> to always be tagged GPIO_ACTIVE_LOW.
>>> Can you please review/let us know your opinion on this ? I think the idea here is to also isolate the changes to a separate consumer driver and avoid getting inversions inside gpiolib.
>>>
>>>
>>> Thanks.
>>>
>>>
>>> Regards,
>>>
>>> Harish Jenny K N
>>>
>> Can you please comment on this ?
>>
>>
>> Thanks,
>>
>> Harish Jenny K N
>>
> Friendly Reminder.
>
> can we please finalize this ?
>
> Linus has also mentioned in another patchset "[PATCH v2] Input: tsc2007 - use GPIO descriptor" that
>
> he is in favor of introducing explicit inverters in device tree.
>
>
> Please consider this and let us know your inputs.
>
>
>
> Thanks,
>
> Harish Jenny K N
>

Can we please finalize this ?


Sorry for the repeated emails.

Am I missing something here ? I am not getting replies.



Thanks,

Harish Jenny K N




^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH V4 2/2] gpio: inverter: document the inverter bindings
  2019-09-04  4:58                     ` Harish Jenny K N
@ 2019-09-10  7:47                       ` Rob Herring
  2019-09-11 12:52                         ` Harish Jenny K N
  0 siblings, 1 reply; 29+ messages in thread
From: Rob Herring @ 2019-09-10  7:47 UTC (permalink / raw)
  To: Harish Jenny K N
  Cc: Linus Walleij, Bartosz Golaszewski, Mark Rutland,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list:GPIO SUBSYSTEM, Balasubramani Vivekanandan

On Wed, Sep 4, 2019 at 5:58 AM Harish Jenny K N
<harish_kandiga@mentor.com> wrote:
>
> Hi Rob, Hi Linus,
>
>
> On 30/08/19 10:51 AM, Harish Jenny K N wrote:
> > Hi Rob,
> >
> >
> > On 27/08/19 1:17 PM, Harish Jenny K N wrote:
> >> Hi Rob,
> >>
> >>
> >> On 19/08/19 3:06 PM, Harish Jenny K N wrote:
> >>> Hi Rob,
> >>>
> >>>
> >>> On 10/08/19 2:21 PM, Linus Walleij wrote:
> >>>> On Fri, Aug 9, 2019 at 4:08 PM Rob Herring <robh+dt@kernel.org> wrote:
> >>>>> On Mon, Aug 5, 2019 at 5:15 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> >>>>>> There is some level of ambition here which is inherently a bit fuzzy
> >>>>>> around the edges. ("How long is the coast of Britain?" comes to mind.)
> >>>>>>
> >>>>>> Surely the intention of device tree is not to recreate the schematic
> >>>>>> in all detail. What we want is a model of the hardware that will
> >>>>>> suffice for the operating system usecases.
> >>>>>>
> >>>>>> But sometimes the DTS files will become confusing: why is this
> >>>>>> component using GPIO_ACTIVE_LOW when another system
> >>>>>> doesn't have that flag? If there is an explicit inverter, the
> >>>>>> DTS gets more readable for a human.
> >>>>>>
> >>>>>> But arguable that is case for adding inverters as syntactic
> >>>>>> sugar in the DTS compiler instead...
> >>>>> If you really want something more explicit, then add a new GPIO
> >>>>> 'inverted' flag. Then a device can always have the same HIGH/LOW flag.
> >>>>> That also solves the abstract it for userspace problem.
> >>>> I think there are some intricate ontologies at work here.
> >>>>
> >>>> Consider this example: a GPIO is controlling a chip select
> >>>> regulator, say Acme Foo. The chip select
> >>>> has a pin named CSN. We know from convention that the
> >>>> "N" at the end of that pin name means "negative" i.e. active
> >>>> low, and that is how the electronics engineers think about
> >>>> that chip select line: it activates the IC when
> >>>> the line goes low.
> >>>>
> >>>> The regulator subsystem and I think all subsystems in the
> >>>> Linux kernel say the consumer pin should be named and
> >>>> tagged after the datsheet of the regulator.
> >>>>
> >>>> So it has for example:
> >>>>
> >>>> foo {
> >>>>     compatible = "acme,foo";
> >>>>     cs-gpios = <&gpio0 6 GPIO_ACTIVE_LOW>;
> >>>> };
> >>>>
> >>>> (It would be inappropriate to name it "csn-gpios" since
> >>>> we have an established flag for active low. But it is another
> >>>> of these syntactic choices where people likely do mistakes.)
> >>>>
> >>>> I think it would be appropriate for the DT binding to say
> >>>> that this flag must always be GPIO_ACTIVE_LOW since
> >>>> the bindings are seen from the component point of view,
> >>>> and thus this is always active low.
> >>>>
> >>>> It would even be reasonable for a yaml schema to enfore
> >>>> this, if it could. It is defined as active low after all.
> >>>>
> >>>> Now if someone adds an inverter on that line between
> >>>> gpio0 and Acme Foo it looks like this:
> >>>>
> >>>> foo {
> >>>>     compatible = "acme,foo";
> >>>>     cs-gpios = <&gpio0 6 GPIO_ACTIVE_HIGH>;
> >>>> };
> >>>>
> >>>> And now we get cognitive dissonance or whatever I should
> >>>> call it: someone reading this DTS sheet and the data
> >>>> sheet for the component Acme Foo to troubleshoot
> >>>> this will be confused: this component has CS active
> >>>> low and still it is specified as active high? Unless they
> >>>> also look at the schematic or the board and find the
> >>>> inverter things are pretty muddy and they will likely curse
> >>>> and solve the situation with the usual trial-and-error,
> >>>> inserting some random cursewords as a comment.
> >>>>
> >>>> With an intermediate inverter node, the cs-gpios
> >>>> can go back to GPIO_ACTIVE_LOW and follow
> >>>> the bindings:
> >>>>
> >>>> inv0: inverter {
> >>>>     compatible = "gpio-inverter";
> >>>>     gpio-controller;
> >>>>     #gpio-cells = <1>;
> >>>>     inverted-gpios = <&gpio0 6 GPIO_ACTIVE_HIGH>;
> >>>> };
> >>>>
> >>>> foo {
> >>>>     compatible = "acme,foo";
> >>>>     cs-gpios = <&inv0 0 GPIO_ACTIVE_LOW>;
> >>>> };
> >>>>
> >>>> And now Acme Foo bindings can keep enforcing cs-gpios
> >>>> to always be tagged GPIO_ACTIVE_LOW.
> >>> Can you please review/let us know your opinion on this ? I think the idea here is to also isolate the changes to a separate consumer driver and avoid getting inversions inside gpiolib.
> >>>
> >>>
> >>> Thanks.
> >>>
> >>>
> >>> Regards,
> >>>
> >>> Harish Jenny K N
> >>>
> >> Can you please comment on this ?
> >>
> >>
> >> Thanks,
> >>
> >> Harish Jenny K N
> >>
> > Friendly Reminder.
> >
> > can we please finalize this ?

I think I have made my position clear and don't really have more to
add. I'm simply not convinced of the need for this. An inverter is not
a GPIO controller. You can't set/get or do any control. It is already
possible to invert GPIO lines in DT by changing the flags and it has
been this way for decades.

Rob

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH V4 2/2] gpio: inverter: document the inverter bindings
  2019-09-10  7:47                       ` Rob Herring
@ 2019-09-11 12:52                         ` Harish Jenny K N
  0 siblings, 0 replies; 29+ messages in thread
From: Harish Jenny K N @ 2019-09-11 12:52 UTC (permalink / raw)
  To: Rob Herring, Linus Walleij
  Cc: Bartosz Golaszewski, Mark Rutland,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list:GPIO SUBSYSTEM, Balasubramani Vivekanandan

Hi Linus,

On 10/09/19 1:17 PM, Rob Herring wrote:
> On Wed, Sep 4, 2019 at 5:58 AM Harish Jenny K N
> <harish_kandiga@mentor.com> wrote:
>> Hi Rob, Hi Linus,
>>
>>
>> On 30/08/19 10:51 AM, Harish Jenny K N wrote:
>>> Hi Rob,
>>>
>>>
>>> On 27/08/19 1:17 PM, Harish Jenny K N wrote:
>>>> Hi Rob,
>>>>
>>>>
>>>> On 19/08/19 3:06 PM, Harish Jenny K N wrote:
>>>>> Hi Rob,
>>>>>
>>>>>
>>>>> On 10/08/19 2:21 PM, Linus Walleij wrote:
>>>>>> On Fri, Aug 9, 2019 at 4:08 PM Rob Herring <robh+dt@kernel.org> wrote:
>>>>>>> On Mon, Aug 5, 2019 at 5:15 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>>>>>>>> There is some level of ambition here which is inherently a bit fuzzy
>>>>>>>> around the edges. ("How long is the coast of Britain?" comes to mind.)
>>>>>>>>
>>>>>>>> Surely the intention of device tree is not to recreate the schematic
>>>>>>>> in all detail. What we want is a model of the hardware that will
>>>>>>>> suffice for the operating system usecases.
>>>>>>>>
>>>>>>>> But sometimes the DTS files will become confusing: why is this
>>>>>>>> component using GPIO_ACTIVE_LOW when another system
>>>>>>>> doesn't have that flag? If there is an explicit inverter, the
>>>>>>>> DTS gets more readable for a human.
>>>>>>>>
>>>>>>>> But arguable that is case for adding inverters as syntactic
>>>>>>>> sugar in the DTS compiler instead...
>>>>>>> If you really want something more explicit, then add a new GPIO
>>>>>>> 'inverted' flag. Then a device can always have the same HIGH/LOW flag.
>>>>>>> That also solves the abstract it for userspace problem.
>>>>>> I think there are some intricate ontologies at work here.
>>>>>>
>>>>>> Consider this example: a GPIO is controlling a chip select
>>>>>> regulator, say Acme Foo. The chip select
>>>>>> has a pin named CSN. We know from convention that the
>>>>>> "N" at the end of that pin name means "negative" i.e. active
>>>>>> low, and that is how the electronics engineers think about
>>>>>> that chip select line: it activates the IC when
>>>>>> the line goes low.
>>>>>>
>>>>>> The regulator subsystem and I think all subsystems in the
>>>>>> Linux kernel say the consumer pin should be named and
>>>>>> tagged after the datsheet of the regulator.
>>>>>>
>>>>>> So it has for example:
>>>>>>
>>>>>> foo {
>>>>>>     compatible = "acme,foo";
>>>>>>     cs-gpios = <&gpio0 6 GPIO_ACTIVE_LOW>;
>>>>>> };
>>>>>>
>>>>>> (It would be inappropriate to name it "csn-gpios" since
>>>>>> we have an established flag for active low. But it is another
>>>>>> of these syntactic choices where people likely do mistakes.)
>>>>>>
>>>>>> I think it would be appropriate for the DT binding to say
>>>>>> that this flag must always be GPIO_ACTIVE_LOW since
>>>>>> the bindings are seen from the component point of view,
>>>>>> and thus this is always active low.
>>>>>>
>>>>>> It would even be reasonable for a yaml schema to enfore
>>>>>> this, if it could. It is defined as active low after all.
>>>>>>
>>>>>> Now if someone adds an inverter on that line between
>>>>>> gpio0 and Acme Foo it looks like this:
>>>>>>
>>>>>> foo {
>>>>>>     compatible = "acme,foo";
>>>>>>     cs-gpios = <&gpio0 6 GPIO_ACTIVE_HIGH>;
>>>>>> };
>>>>>>
>>>>>> And now we get cognitive dissonance or whatever I should
>>>>>> call it: someone reading this DTS sheet and the data
>>>>>> sheet for the component Acme Foo to troubleshoot
>>>>>> this will be confused: this component has CS active
>>>>>> low and still it is specified as active high? Unless they
>>>>>> also look at the schematic or the board and find the
>>>>>> inverter things are pretty muddy and they will likely curse
>>>>>> and solve the situation with the usual trial-and-error,
>>>>>> inserting some random cursewords as a comment.
>>>>>>
>>>>>> With an intermediate inverter node, the cs-gpios
>>>>>> can go back to GPIO_ACTIVE_LOW and follow
>>>>>> the bindings:
>>>>>>
>>>>>> inv0: inverter {
>>>>>>     compatible = "gpio-inverter";
>>>>>>     gpio-controller;
>>>>>>     #gpio-cells = <1>;
>>>>>>     inverted-gpios = <&gpio0 6 GPIO_ACTIVE_HIGH>;
>>>>>> };
>>>>>>
>>>>>> foo {
>>>>>>     compatible = "acme,foo";
>>>>>>     cs-gpios = <&inv0 0 GPIO_ACTIVE_LOW>;
>>>>>> };
>>>>>>
>>>>>> And now Acme Foo bindings can keep enforcing cs-gpios
>>>>>> to always be tagged GPIO_ACTIVE_LOW.
>>>>> Can you please review/let us know your opinion on this ? I think the idea here is to also isolate the changes to a separate consumer driver and avoid getting inversions inside gpiolib.
>>>>>
>>>>>
>>>>> Thanks.
>>>>>
>>>>>
>>>>> Regards,
>>>>>
>>>>> Harish Jenny K N
>>>>>
>>>> Can you please comment on this ?
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> Harish Jenny K N
>>>>
>>> Friendly Reminder.
>>>
>>> can we please finalize this ?
> I think I have made my position clear and don't really have more to
> add. I'm simply not convinced of the need for this. An inverter is not
> a GPIO controller. You can't set/get or do any control. It is already
> possible to invert GPIO lines in DT by changing the flags and it has
> been this way for decades.
>
> Rob


If Rob is fine with adding "inverted" flag in the device tree, can we just go back the initial approach of

defining the polarity on the producer side?

With this we would need something like this in the device tree for any gpiochip controller.

inverted = /bits/ 8 <0 0 0 0 0 0 1 0 0 0 0 0 0 0 0 0 0 0>;


RFC patch sent earlier can be found here. https://www.spinics.net/lists/linux-gpio/msg38815.html ( Note: various terms would need change)

I can send another patchset if you agree.


Please let us know your suggestion.


Thanks,

Harish Jenny K N




^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH V4 2/2] gpio: inverter: document the inverter bindings
  2019-06-28  9:30 [PATCH V4 2/2] gpio: inverter: document the inverter bindings Harish Jenny K N
  2019-07-04  5:01 ` Harish Jenny K N
  2019-07-08 22:36 ` Rob Herring
@ 2019-09-25 16:51 ` Eugeniu Rosca
  2019-09-27  5:52   ` Phil Reid
                     ` (2 more replies)
  2 siblings, 3 replies; 29+ messages in thread
From: Eugeniu Rosca @ 2019-09-25 16:51 UTC (permalink / raw)
  To: Linus Walleij, Harish Jenny K N, Rob Herring, Mark Rutland
  Cc: Bartosz Golaszewski, Balasubramani Vivekanandan,
	Laurent Pinchart, Stephen Warren, Stephen Warren,
	Geert Uytterhoeven, Phil Reid, Enrico Weigelt, linux-gpio,
	devicetree, Eugeniu Rosca, Eugeniu Rosca

Hi All,

I've additionally Cc-ed Laurent and Stephen, since their fruitful
discussion in [1] back in 2014 concluded with a useful documentation
update [2] which is precisely related to the interpretation and usage
of the polarity flag in GPIO specifiers.

I've also Cc-ed those people who have participated in reviewing the
previous patch iterations (Geert, Phil, Enrico).

Before leaving this thread in limbo, I would like to attempt clarifying
what it actually tried to accomplish, one more time.

First of all, it stems from the need to implement a specific customer
requirement. Whether this requirement is sane or not, that's actually
a very important question, but I haven't found much discussion around
it the comments posted so far.

To paraphrase what Harish stated in [3], the customer has a list of GPIO
pins which need to be controlled from userspace. Of course, the customer
can set the polarity of those pins from userspace, as pointed out by
Linus in [4] (thanks!). But, keeping track of GPIO polarity in userspace
is seen like a burden. The customer thinks that the right place for this
HW-specific detail is in device trees. Do you think this preference
is ill-formed?

If we hog a GPIO pin in DTS (which allows specifying its polarity),
userspace no longer has access to that pin. There isn't a way to define
GPIO polarity by means of DTS without affecting userspace access
(can anybody contradict this statement?).

Whether it is obvious or not, the main goal of this series is actually
to provide the possibility of inverting the default ACTIVE_HIGH polarity
for GPIO pin X _via DTS_ while still allowing to operate on that pin
_from userspace_. My two questions are then:
 - I hope it is something sane to desire?
 - If it is sane, how can this be accomplished, if the functionality
   implemented by Harish doesn't pass the community review?

[1] https://marc.info/?l=linux-gpio&m=139204273132477&w=4 ("Correct meaning of the GPIO active low flag")
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=51e8afc1c43c75 ("gpio: document polarity flag best practices")
[3] https://marc.info/?l=linux-gpio&m=155721267517644&w=2 ("[PATCH V1 1/2] gpio: make it possible to set active-state on GPIO lines")
[4] https://marc.info/?l=linux-gpio&m=155713157122847&w=2 ("[PATCH V1 1/2] gpio: make it possible to set active-state on GPIO lines")

-- 
Best Regards,
Eugeniu

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH V4 2/2] gpio: inverter: document the inverter bindings
  2019-09-25 16:51 ` Eugeniu Rosca
@ 2019-09-27  5:52   ` Phil Reid
  2019-09-27  9:07   ` Geert Uytterhoeven
  2019-10-04 19:07   ` Stephen Warren
  2 siblings, 0 replies; 29+ messages in thread
From: Phil Reid @ 2019-09-27  5:52 UTC (permalink / raw)
  To: Eugeniu Rosca, Linus Walleij, Harish Jenny K N, Rob Herring,
	Mark Rutland
  Cc: Bartosz Golaszewski, Balasubramani Vivekanandan,
	Laurent Pinchart, Stephen Warren, Stephen Warren,
	Geert Uytterhoeven, Enrico Weigelt, linux-gpio, devicetree,
	Eugeniu Rosca

On 26/09/2019 00:51, Eugeniu Rosca wrote:
> Hi All,
> 
> I've additionally Cc-ed Laurent and Stephen, since their fruitful
> discussion in [1] back in 2014 concluded with a useful documentation
> update [2] which is precisely related to the interpretation and usage
> of the polarity flag in GPIO specifiers.
> 
> I've also Cc-ed those people who have participated in reviewing the
> previous patch iterations (Geert, Phil, Enrico).
> 
> Before leaving this thread in limbo, I would like to attempt clarifying
> what it actually tried to accomplish, one more time.
> 
> First of all, it stems from the need to implement a specific customer
> requirement. Whether this requirement is sane or not, that's actually
> a very important question, but I haven't found much discussion around
> it the comments posted so far.
> 
> To paraphrase what Harish stated in [3], the customer has a list of GPIO
> pins which need to be controlled from userspace. Of course, the customer
> can set the polarity of those pins from userspace, as pointed out by
> Linus in [4] (thanks!). But, keeping track of GPIO polarity in userspace
> is seen like a burden. The customer thinks that the right place for this
> HW-specific detail is in device trees. Do you think this preference
> is ill-formed?
> 
> If we hog a GPIO pin in DTS (which allows specifying its polarity),
> userspace no longer has access to that pin. There isn't a way to define
> GPIO polarity by means of DTS without affecting userspace access
> (can anybody contradict this statement?).
> 
> Whether it is obvious or not, the main goal of this series is actually
> to provide the possibility of inverting the default ACTIVE_HIGH polarity
> for GPIO pin X _via DTS_ while still allowing to operate on that pin
> _from userspace_. My two questions are then:
>   - I hope it is something sane to desire?
>   - If it is sane, how can this be accomplished, if the functionality
>     implemented by Harish doesn't pass the community review?
> 
> [1] https://marc.info/?l=linux-gpio&m=139204273132477&w=4 ("Correct meaning of the GPIO active low flag")
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=51e8afc1c43c75 ("gpio: document polarity flag best practices")
> [3] https://marc.info/?l=linux-gpio&m=155721267517644&w=2 ("[PATCH V1 1/2] gpio: make it possible to set active-state on GPIO lines")
> [4] https://marc.info/?l=linux-gpio&m=155713157122847&w=2 ("[PATCH V1 1/2] gpio: make it possible to set active-state on GPIO lines")
> 

I still think the concept is very useful.

A related issue for example is that some drivers want to enforce the GPIO consumer to be open drain.
Eg see i2c-imx or i2c-davinci for examples

But suppose I've connected the gpio to the i2c bus via a FET (which is an inverter with open drain output).
We're basically saying I can't model that FET's existence in the dts.
So I can't accurately describe my hardware in the dts to ensure the software does the right thing without the inverter.
That is drive the GPIO output as push / pull.

It's probably good the drivers do enforce this kind of open drain thing to prevent dts error destroying hardware.
But it does prevent some hardware designs from working if we can't model the bits in between.


-- 
Regards
Phil Reid

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH V4 2/2] gpio: inverter: document the inverter bindings
  2019-09-25 16:51 ` Eugeniu Rosca
  2019-09-27  5:52   ` Phil Reid
@ 2019-09-27  9:07   ` Geert Uytterhoeven
  2019-10-05 13:07     ` Eugeniu Rosca
  2019-10-04 19:07   ` Stephen Warren
  2 siblings, 1 reply; 29+ messages in thread
From: Geert Uytterhoeven @ 2019-09-27  9:07 UTC (permalink / raw)
  To: Eugeniu Rosca
  Cc: Linus Walleij, Harish Jenny K N, Rob Herring, Mark Rutland,
	Bartosz Golaszewski, Balasubramani Vivekanandan,
	Laurent Pinchart, Stephen Warren, Stephen Warren, Phil Reid,
	Enrico Weigelt, open list:GPIO SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Eugeniu Rosca

Hi Eugeniu,

On Wed, Sep 25, 2019 at 6:51 PM Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
> I've additionally Cc-ed Laurent and Stephen, since their fruitful
> discussion in [1] back in 2014 concluded with a useful documentation
> update [2] which is precisely related to the interpretation and usage
> of the polarity flag in GPIO specifiers.
>
> I've also Cc-ed those people who have participated in reviewing the
> previous patch iterations (Geert, Phil, Enrico).
>
> Before leaving this thread in limbo, I would like to attempt clarifying
> what it actually tried to accomplish, one more time.
>
> First of all, it stems from the need to implement a specific customer
> requirement. Whether this requirement is sane or not, that's actually
> a very important question, but I haven't found much discussion around
> it the comments posted so far.
>
> To paraphrase what Harish stated in [3], the customer has a list of GPIO
> pins which need to be controlled from userspace. Of course, the customer
> can set the polarity of those pins from userspace, as pointed out by
> Linus in [4] (thanks!). But, keeping track of GPIO polarity in userspace
> is seen like a burden. The customer thinks that the right place for this
> HW-specific detail is in device trees. Do you think this preference
> is ill-formed?
>
> If we hog a GPIO pin in DTS (which allows specifying its polarity),
> userspace no longer has access to that pin. There isn't a way to define
> GPIO polarity by means of DTS without affecting userspace access
> (can anybody contradict this statement?).
>
> Whether it is obvious or not, the main goal of this series is actually
> to provide the possibility of inverting the default ACTIVE_HIGH polarity
> for GPIO pin X _via DTS_ while still allowing to operate on that pin
> _from userspace_. My two questions are then:
>  - I hope it is something sane to desire?
>  - If it is sane, how can this be accomplished, if the functionality
>    implemented by Harish doesn't pass the community review?
>
> [1] https://marc.info/?l=linux-gpio&m=139204273132477&w=4 ("Correct meaning of the GPIO active low flag")
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=51e8afc1c43c75 ("gpio: document polarity flag best practices")
> [3] https://marc.info/?l=linux-gpio&m=155721267517644&w=2 ("[PATCH V1 1/2] gpio: make it possible to set active-state on GPIO lines")
> [4] https://marc.info/?l=linux-gpio&m=155713157122847&w=2 ("[PATCH V1 1/2] gpio: make it possible to set active-state on GPIO lines")

My standard reply would be: describe the device connected to the GPIO(s)
in DT.  The GPIO line polarities are specified in the device's "gpios"
properties.

BTW, can you give an example of what's actually connected to those
GPIOs?
Is it a complex device (the GPIO is only a part of it, it's also hanging
off e.g. an I2C bus)?
Is it something simple (e.g. an LED ("gpio-leds"), relay, or actuator)?

Next step would be to use the device from Linux.  For that to work, you
need a dedicated driver (for the complex case), or something generic
(for the simple case).
The latter is not unlike e.g. spidev.  Once you have a generic driver,
you can use "driver_override" in sysfs to bind the generic driver to
your device.  See e.g. commit 5039563e7c25eccd ("spi: Add
driver_override SPI device attribute").

Currently we don't have a "generic" driver for GPIOs. We do have the
GPIO chardev interface, which exports a full gpio_chip.
It indeed looks like this "gpio-inverter" could be used as a generic
driver.  But it is limited to GPIOs that are inverted, which rules out
some use cases.

So what about making it more generic, and dropping the "inverter" from
its name, and the "inverted" from the "inverted-gpios" property? After
all the inversion can be specified by the polarity of the GPIO cells in
the "gpios" property, and the GPIO core will take care of it[*]?
Which boils down to adding a simple DT interface to my gpio-aggregator
("[PATCH/RFC v2 0/5] gpio: Add GPIO Aggregator Driver",
 https://lore.kernel.org/lkml/20190911143858.13024-1-geert+renesas@glider.be/).
And now I have realized[*], we probably no longer need the GPIO
Forwarder Helper, as there is no need to add inversion on top.

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH V4 2/2] gpio: inverter: document the inverter bindings
  2019-09-25 16:51 ` Eugeniu Rosca
  2019-09-27  5:52   ` Phil Reid
  2019-09-27  9:07   ` Geert Uytterhoeven
@ 2019-10-04 19:07   ` Stephen Warren
  2019-10-05 17:50     ` Eugeniu Rosca
  2 siblings, 1 reply; 29+ messages in thread
From: Stephen Warren @ 2019-10-04 19:07 UTC (permalink / raw)
  To: Eugeniu Rosca, Linus Walleij, Harish Jenny K N, Rob Herring,
	Mark Rutland
  Cc: Bartosz Golaszewski, Balasubramani Vivekanandan,
	Laurent Pinchart, Stephen Warren, Geert Uytterhoeven, Phil Reid,
	Enrico Weigelt, linux-gpio, devicetree, Eugeniu Rosca

On 9/25/19 10:51 AM, Eugeniu Rosca wrote:
> Hi All,
> 
> I've additionally Cc-ed Laurent and Stephen, since their fruitful
> discussion in [1] back in 2014 concluded with a useful documentation
> update [2] which is precisely related to the interpretation and usage
> of the polarity flag in GPIO specifiers.
> 
> I've also Cc-ed those people who have participated in reviewing the
> previous patch iterations (Geert, Phil, Enrico).
> 
> Before leaving this thread in limbo, I would like to attempt clarifying
> what it actually tried to accomplish, one more time.
> 
> First of all, it stems from the need to implement a specific customer
> requirement. Whether this requirement is sane or not, that's actually
> a very important question, but I haven't found much discussion around
> it the comments posted so far.
> 
> To paraphrase what Harish stated in [3], the customer has a list of GPIO
> pins which need to be controlled from userspace. Of course, the customer
> can set the polarity of those pins from userspace, as pointed out by
> Linus in [4] (thanks!). But, keeping track of GPIO polarity in userspace
> is seen like a burden. The customer thinks that the right place for this
> HW-specific detail is in device trees. Do you think this preference
> is ill-formed?

I think the DT should represent the device that's attached to the GPIOs. 
That way, there's already a clear way to represent the GPIO polarity, as 
described in the document linked by Eugenui in [2] below.

If for some reason that's not possible, then I think keeping track of 
the GPIO polarity in user-space is entirely reasonable, and is the 
correct approach. To claim that tracking GPIO polarity in user-space is 
too much burden, yet to also allow user-space to control GPIOs at all, 
and hence to know exactly which GPIOs must be controlled, is an 
inconsistent assertion.

Put another way: If a piece of user-space SW controls GPIOs, it must 
know which GPIO number to use for each logical purpose. This information 
presumably varies on different platforms, so the SW must have a list of 
GPIO numbers and GPIO controller IDs per platform. Additionally storing 
a polarity bit along with that information seems entirely trivial to me.

Is there some other issue that I'm overlooking?

If the list of GPIO IDs is retrieved from DT by the user-space SW, I 
could see an argument for storing the polarity information in DT along 
with that list of GPIO IDs. However, I don't believe there's any 
standard way of representing "a list of GPIO IDs for user space use" in DT.

> If we hog a GPIO pin in DTS (which allows specifying its polarity),
> userspace no longer has access to that pin. There isn't a way to define
> GPIO polarity by means of DTS without affecting userspace access
> (can anybody contradict this statement?).

GPIO hog doesn't seem like the right approach; its intent is to actively 
configure the GPIO in a fixed state, which is logically incompatible 
with user-space control of the GPIO.

> Whether it is obvious or not, the main goal of this series is actually
> to provide the possibility of inverting the default ACTIVE_HIGH polarity
> for GPIO pin X _via DTS_ while still allowing to operate on that pin
> _from userspace_. My two questions are then:
>   - I hope it is something sane to desire?
>   - If it is sane, how can this be accomplished, if the functionality
>     implemented by Harish doesn't pass the community review?
> 
> [1] https://marc.info/?l=linux-gpio&m=139204273132477&w=4 ("Correct meaning of the GPIO active low flag")
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=51e8afc1c43c75 ("gpio: document polarity flag best practices")
> [3] https://marc.info/?l=linux-gpio&m=155721267517644&w=2 ("[PATCH V1 1/2] gpio: make it possible to set active-state on GPIO lines")
> [4] https://marc.info/?l=linux-gpio&m=155713157122847&w=2 ("[PATCH V1 1/2] gpio: make it possible to set active-state on GPIO lines")
> 


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH V4 2/2] gpio: inverter: document the inverter bindings
  2019-09-27  9:07   ` Geert Uytterhoeven
@ 2019-10-05 13:07     ` Eugeniu Rosca
  2019-10-07  8:18       ` Geert Uytterhoeven
  0 siblings, 1 reply; 29+ messages in thread
From: Eugeniu Rosca @ 2019-10-05 13:07 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Eugeniu Rosca, Linus Walleij, Harish Jenny K N, Rob Herring,
	Mark Rutland, Bartosz Golaszewski, Balasubramani Vivekanandan,
	Laurent Pinchart, Stephen Warren, Stephen Warren, Phil Reid,
	Enrico Weigelt, open list:GPIO SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Eugeniu Rosca

Hi Geert,

On Fri, Sep 27, 2019 at 11:07:20AM +0200, Geert Uytterhoeven wrote:

[..]

> My standard reply would be: describe the device connected to the GPIO(s)
> in DT.  The GPIO line polarities are specified in the device's "gpios"
> properties.
> 
> BTW, can you give an example of what's actually connected to those
> GPIOs?
> Is it a complex device (the GPIO is only a part of it, it's also hanging
> off e.g. an I2C bus)?
> Is it something simple (e.g. an LED ("gpio-leds"), relay, or actuator)?

Since the targeted user of the new feature is not in immediate vicinity,
we expect some delay in getting this information.

> 
> Next step would be to use the device from Linux.  For that to work, you
> need a dedicated driver (for the complex case), or something generic
> (for the simple case).
> The latter is not unlike e.g. spidev.  Once you have a generic driver,
> you can use "driver_override" in sysfs to bind the generic driver to
> your device.  See e.g. commit 5039563e7c25eccd ("spi: Add
> driver_override SPI device attribute").

We have passed your suggestions along. Many thanks.

> Currently we don't have a "generic" driver for GPIOs. We do have the
> GPIO chardev interface, which exports a full gpio_chip.
> It indeed looks like this "gpio-inverter" could be used as a generic
> driver.  But it is limited to GPIOs that are inverted, which rules out
> some use cases.
> 
> So what about making it more generic, and dropping the "inverter" from
> its name, and the "inverted" from the "inverted-gpios" property? After
> all the inversion can be specified by the polarity of the GPIO cells in
> the "gpios" property, and the GPIO core will take care of it[*]?
> Which boils down to adding a simple DT interface to my gpio-aggregator
> ("[PATCH/RFC v2 0/5] gpio: Add GPIO Aggregator Driver",
>  https://lore.kernel.org/lkml/20190911143858.13024-1-geert+renesas@glider.be/).
> And now I have realized[*], we probably no longer need the GPIO
> Forwarder Helper, as there is no need to add inversion on top.

After having a look at the gpio aggregator (and giving it a try on
R-Car3 H3ULCB), here is how I interpret the above comment:

If there is still a compelling reason for having gpio-inverter, then it
probably makes sense to strip it from its "inverter" function (hence,
transforming it into some kind of "repeater") on the basis that the
inverting function is more of a collateral/secondary feature, rather
than its primary one. Just like in the case of gpio aggregator, the
primary function of gpio inverter is to accept a bunch of GPIO lines and
to expose those via a dedicated gpiochip. I hope this is a proper
summary of the first point in your comment. In any case, this is the
understanding I get based on my experiments with both drivers.

What I also infer is that, assuming gpio-inverter will stay (potentially
renamed and stripped of its non-essential inverting function), the gpio
aggregator will need to keep its Forwarder Helper (supposed to act as a
common foundation for both drivers).

The second point which I extract from your comment is that the "gpio
aggregator" could alternatively acquire the role of "gpio-inverter"
(hence superseding it) by adding a "simple DT interface". I actually
tend to like this proposal, since (as said above) both drivers are
essentially doing the same thing, i.e. they cluster a number of gpio
lines and expose this cluster as a new gpiochip (keeping the
reserved/used gpio lines on hold). That looks like a huge overlap in
the functionalities of the two drivers.

The only difference which I see is that "gpio-inverter" is getting its
input from DT and generates the gpiochips at probe time, while
"gpio aggregator" is getting its input from sysfs and generates the
gpiochips at runtime, post-probe.

So, assuming no objections from Harish and other reviewers, I would be
very happy to review and test the DT-based gpio inversion functionality
as part of gpio aggregator. Thanks!

-- 
Best Regards,
Eugeniu

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH V4 2/2] gpio: inverter: document the inverter bindings
  2019-10-04 19:07   ` Stephen Warren
@ 2019-10-05 17:50     ` Eugeniu Rosca
  2019-10-07 15:36       ` Stephen Warren
  0 siblings, 1 reply; 29+ messages in thread
From: Eugeniu Rosca @ 2019-10-05 17:50 UTC (permalink / raw)
  To: Stephen Warren
  Cc: Eugeniu Rosca, Linus Walleij, Harish Jenny K N, Rob Herring,
	Mark Rutland, Bartosz Golaszewski, Balasubramani Vivekanandan,
	Laurent Pinchart, Stephen Warren, Geert Uytterhoeven, Phil Reid,
	Enrico Weigelt, linux-gpio, devicetree, Eugeniu Rosca

Hi Stephen,

On Fri, Oct 04, 2019 at 01:07:23PM -0600, Stephen Warren wrote:

[..]

> I think the DT should represent the device that's attached to the GPIOs.
> That way, there's already a clear way to represent the GPIO polarity, as
> described in the document linked by Eugenui in [2] below.
> 
> If for some reason that's not possible, then I think keeping track of the
> GPIO polarity in user-space is entirely reasonable, and is the correct
> approach. To claim that tracking GPIO polarity in user-space is too much
> burden, yet to also allow user-space to control GPIOs at all, and hence to
> know exactly which GPIOs must be controlled, is an inconsistent assertion.
> 
> Put another way: If a piece of user-space SW controls GPIOs, it must know
> which GPIO number to use for each logical purpose. This information
> presumably varies on different platforms, so the SW must have a list of GPIO
> numbers and GPIO controller IDs per platform. Additionally storing a
> polarity bit along with that information seems entirely trivial to me.
> 
> Is there some other issue that I'm overlooking?

Based on the discussions so far, the user who requested this feature
intends to (in fact already does) "mark" the userspace-relevant gpio
lines via the "gpio-line-names" [5] DT property, implemented by Linus
in v4.7 commit [6]. By keeping track of "gpio line name" both in DT and
in user-space, apparently the user is able to accurately map the
"line name" (visible in userspace) to the corresponding gpio chip/name
and gpio line offset in a "platform/board-independent" way.

Do you think this is unorthodox?

> If the list of GPIO IDs is retrieved from DT by the user-space SW, I could
> see an argument for storing the polarity information in DT along with that
> list of GPIO IDs. However, I don't believe there's any standard way of
> representing "a list of GPIO IDs for user space use" in DT.
> 
> > If we hog a GPIO pin in DTS (which allows specifying its polarity),
> > userspace no longer has access to that pin. There isn't a way to define
> > GPIO polarity by means of DTS without affecting userspace access
> > (can anybody contradict this statement?).
> 
> GPIO hog doesn't seem like the right approach; its intent is to actively
> configure the GPIO in a fixed state, which is logically incompatible with
> user-space control of the GPIO.

Agreed. Thanks for strengthening the idea behind hogging the gpios.

[..]

> > [1] https://marc.info/?l=linux-gpio&m=139204273132477&w=4 ("Correct meaning of the GPIO active low flag")
> > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=51e8afc1c43c75 ("gpio: document polarity flag best practices")
> > [3] https://marc.info/?l=linux-gpio&m=155721267517644&w=2 ("[PATCH V1 1/2] gpio: make it possible to set active-state on GPIO lines")
> > [4] https://marc.info/?l=linux-gpio&m=155713157122847&w=2 ("[PATCH V1 1/2] gpio: make it possible to set active-state on GPIO lines")
[5] https://marc.info/?l=linux-gpio&m=155712945922102&w=2
[6] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fd9c55315db9
    ("gpio: of: make it possible to name GPIO lines")

-- 
Best Regards,
Eugeniu

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH V4 2/2] gpio: inverter: document the inverter bindings
  2019-10-05 13:07     ` Eugeniu Rosca
@ 2019-10-07  8:18       ` Geert Uytterhoeven
  2019-10-11  4:35         ` Harish Jenny K N
  0 siblings, 1 reply; 29+ messages in thread
From: Geert Uytterhoeven @ 2019-10-07  8:18 UTC (permalink / raw)
  To: Eugeniu Rosca
  Cc: Eugeniu Rosca, Linus Walleij, Harish Jenny K N, Rob Herring,
	Mark Rutland, Bartosz Golaszewski, Balasubramani Vivekanandan,
	Laurent Pinchart, Stephen Warren, Stephen Warren, Phil Reid,
	Enrico Weigelt, open list:GPIO SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

Hi Eugeniu,

On Sat, Oct 5, 2019 at 3:08 PM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
> On Fri, Sep 27, 2019 at 11:07:20AM +0200, Geert Uytterhoeven wrote:
> > My standard reply would be: describe the device connected to the GPIO(s)
> > in DT.  The GPIO line polarities are specified in the device's "gpios"
> > properties.

> > Next step would be to use the device from Linux.  For that to work, you
> > need a dedicated driver (for the complex case), or something generic
> > (for the simple case).
> > The latter is not unlike e.g. spidev.  Once you have a generic driver,
> > you can use "driver_override" in sysfs to bind the generic driver to
> > your device.  See e.g. commit 5039563e7c25eccd ("spi: Add
> > driver_override SPI device attribute").
>
> We have passed your suggestions along. Many thanks.
>
> > Currently we don't have a "generic" driver for GPIOs. We do have the
> > GPIO chardev interface, which exports a full gpio_chip.
> > It indeed looks like this "gpio-inverter" could be used as a generic
> > driver.  But it is limited to GPIOs that are inverted, which rules out
> > some use cases.
> >
> > So what about making it more generic, and dropping the "inverter" from
> > its name, and the "inverted" from the "inverted-gpios" property? After
> > all the inversion can be specified by the polarity of the GPIO cells in
> > the "gpios" property, and the GPIO core will take care of it[*]?
> > Which boils down to adding a simple DT interface to my gpio-aggregator
> > ("[PATCH/RFC v2 0/5] gpio: Add GPIO Aggregator Driver",
> >  https://lore.kernel.org/lkml/20190911143858.13024-1-geert+renesas@glider.be/).
> > And now I have realized[*], we probably no longer need the GPIO
> > Forwarder Helper, as there is no need to add inversion on top.
>
> After having a look at the gpio aggregator (and giving it a try on
> R-Car3 H3ULCB), here is how I interpret the above comment:
>
> If there is still a compelling reason for having gpio-inverter, then it
> probably makes sense to strip it from its "inverter" function (hence,
> transforming it into some kind of "repeater") on the basis that the
> inverting function is more of a collateral/secondary feature, rather
> than its primary one. Just like in the case of gpio aggregator, the
> primary function of gpio inverter is to accept a bunch of GPIO lines and
> to expose those via a dedicated gpiochip. I hope this is a proper
> summary of the first point in your comment. In any case, this is the
> understanding I get based on my experiments with both drivers.

Yes, the inverter is basically a "repeater" (or "aggregator", when it has
multiple GPIOs connected), hardcoded to invert.

> What I also infer is that, assuming gpio-inverter will stay (potentially
> renamed and stripped of its non-essential inverting function), the gpio
> aggregator will need to keep its Forwarder Helper (supposed to act as a
> common foundation for both drivers).

What I meant is that if the inverter and aggregator would be combinoed
into a single driver, there would no longer be a need[*] for a separate
helper, and it could be incorporated into the single driver.

[*] The individual helper functions may still be useful for some other
     driver, though.

> The second point which I extract from your comment is that the "gpio
> aggregator" could alternatively acquire the role of "gpio-inverter"
> (hence superseding it) by adding a "simple DT interface". I actually
> tend to like this proposal, since (as said above) both drivers are
> essentially doing the same thing, i.e. they cluster a number of gpio
> lines and expose this cluster as a new gpiochip (keeping the
> reserved/used gpio lines on hold). That looks like a huge overlap in
> the functionalities of the two drivers.

Yes, both drivers are very similar.  The difference lies in how they
acquire the list of GPIO descriptors.

> The only difference which I see is that "gpio-inverter" is getting its
> input from DT and generates the gpiochips at probe time, while
> "gpio aggregator" is getting its input from sysfs and generates the
> gpiochips at runtime, post-probe.

Exactly.

For my virtualization use case, I need to create the list of GPIO
descriptors at run-time, hence the sysfs interface. This is
polarity-agnostic (i.e. the end user needs to care about polarity).

For Harish use case, he needs to describe the list from DT, with
polarity inverted, which can be done by specifying the GPIO_ACTIVE_LOW
flag in the node's"gpios" property.

For your use case, you want to describe the list in DT, with line-names,
and polarity specified.

> So, assuming no objections from Harish and other reviewers, I would be
> very happy to review and test the DT-based gpio inversion functionality
> as part of gpio aggregator. Thanks!

Thanks, adding to my list ;-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH V4 2/2] gpio: inverter: document the inverter bindings
  2019-10-05 17:50     ` Eugeniu Rosca
@ 2019-10-07 15:36       ` Stephen Warren
  0 siblings, 0 replies; 29+ messages in thread
From: Stephen Warren @ 2019-10-07 15:36 UTC (permalink / raw)
  To: Eugeniu Rosca
  Cc: Eugeniu Rosca, Linus Walleij, Harish Jenny K N, Rob Herring,
	Mark Rutland, Bartosz Golaszewski, Balasubramani Vivekanandan,
	Laurent Pinchart, Stephen Warren, Geert Uytterhoeven, Phil Reid,
	Enrico Weigelt, linux-gpio, devicetree

On 10/5/19 11:50 AM, Eugeniu Rosca wrote:
> Hi Stephen,
> 
> On Fri, Oct 04, 2019 at 01:07:23PM -0600, Stephen Warren wrote:
> 
> [..]
> 
>> I think the DT should represent the device that's attached to the GPIOs.
>> That way, there's already a clear way to represent the GPIO polarity, as
>> described in the document linked by Eugenui in [2] below.
>>
>> If for some reason that's not possible, then I think keeping track of the
>> GPIO polarity in user-space is entirely reasonable, and is the correct
>> approach. To claim that tracking GPIO polarity in user-space is too much
>> burden, yet to also allow user-space to control GPIOs at all, and hence to
>> know exactly which GPIOs must be controlled, is an inconsistent assertion.
>>
>> Put another way: If a piece of user-space SW controls GPIOs, it must know
>> which GPIO number to use for each logical purpose. This information
>> presumably varies on different platforms, so the SW must have a list of GPIO
>> numbers and GPIO controller IDs per platform. Additionally storing a
>> polarity bit along with that information seems entirely trivial to me.
>>
>> Is there some other issue that I'm overlooking?
> 
> Based on the discussions so far, the user who requested this feature
> intends to (in fact already does) "mark" the userspace-relevant gpio
> lines via the "gpio-line-names" [5] DT property, implemented by Linus
> in v4.7 commit [6]. By keeping track of "gpio line name" both in DT and
> in user-space, apparently the user is able to accurately map the
> "line name" (visible in userspace) to the corresponding gpio chip/name
> and gpio line offset in a "platform/board-independent" way.
> 
> Do you think this is unorthodox?

No, that sounds reasonable.

Given that, it might be plausible to extend that DT scheme to represent 
more information, but it's still better to represent the actual end 
device (that's connected to the GPIO) in DT, since that also represents 
the semantics.

> [6] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fd9c55315db9

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH V4 2/2] gpio: inverter: document the inverter bindings
  2019-10-07  8:18       ` Geert Uytterhoeven
@ 2019-10-11  4:35         ` Harish Jenny K N
  2019-11-12 11:52           ` Harish Jenny K N
  0 siblings, 1 reply; 29+ messages in thread
From: Harish Jenny K N @ 2019-10-11  4:35 UTC (permalink / raw)
  To: Geert Uytterhoeven, Eugeniu Rosca
  Cc: Eugeniu Rosca, Linus Walleij, Rob Herring, Mark Rutland,
	Bartosz Golaszewski, Balasubramani Vivekanandan,
	Laurent Pinchart, Stephen Warren, Stephen Warren, Phil Reid,
	Enrico Weigelt, open list:GPIO SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

Hi Geert,


On 07/10/19 1:48 PM, Geert Uytterhoeven wrote:
> Hi Eugeniu,
>
> On Sat, Oct 5, 2019 at 3:08 PM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
>> On Fri, Sep 27, 2019 at 11:07:20AM +0200, Geert Uytterhoeven wrote:
>>> My standard reply would be: describe the device connected to the GPIO(s)
>>> in DT.  The GPIO line polarities are specified in the device's "gpios"
>>> properties.
>>> Next step would be to use the device from Linux.  For that to work, you
>>> need a dedicated driver (for the complex case), or something generic
>>> (for the simple case).
>>> The latter is not unlike e.g. spidev.  Once you have a generic driver,
>>> you can use "driver_override" in sysfs to bind the generic driver to
>>> your device.  See e.g. commit 5039563e7c25eccd ("spi: Add
>>> driver_override SPI device attribute").
>> We have passed your suggestions along. Many thanks.
>>
>>> Currently we don't have a "generic" driver for GPIOs. We do have the
>>> GPIO chardev interface, which exports a full gpio_chip.
>>> It indeed looks like this "gpio-inverter" could be used as a generic
>>> driver.  But it is limited to GPIOs that are inverted, which rules out
>>> some use cases.
>>>
>>> So what about making it more generic, and dropping the "inverter" from
>>> its name, and the "inverted" from the "inverted-gpios" property? After
>>> all the inversion can be specified by the polarity of the GPIO cells in
>>> the "gpios" property, and the GPIO core will take care of it[*]?
>>> Which boils down to adding a simple DT interface to my gpio-aggregator
>>> ("[PATCH/RFC v2 0/5] gpio: Add GPIO Aggregator Driver",
>>>  https://lore.kernel.org/lkml/20190911143858.13024-1-geert+renesas@glider.be/).
>>> And now I have realized[*], we probably no longer need the GPIO
>>> Forwarder Helper, as there is no need to add inversion on top.
>> After having a look at the gpio aggregator (and giving it a try on
>> R-Car3 H3ULCB), here is how I interpret the above comment:
>>
>> If there is still a compelling reason for having gpio-inverter, then it
>> probably makes sense to strip it from its "inverter" function (hence,
>> transforming it into some kind of "repeater") on the basis that the
>> inverting function is more of a collateral/secondary feature, rather
>> than its primary one. Just like in the case of gpio aggregator, the
>> primary function of gpio inverter is to accept a bunch of GPIO lines and
>> to expose those via a dedicated gpiochip. I hope this is a proper
>> summary of the first point in your comment. In any case, this is the
>> understanding I get based on my experiments with both drivers.
> Yes, the inverter is basically a "repeater" (or "aggregator", when it has
> multiple GPIOs connected), hardcoded to invert.
>
>> What I also infer is that, assuming gpio-inverter will stay (potentially
>> renamed and stripped of its non-essential inverting function), the gpio
>> aggregator will need to keep its Forwarder Helper (supposed to act as a
>> common foundation for both drivers).
> What I meant is that if the inverter and aggregator would be combinoed
> into a single driver, there would no longer be a need[*] for a separate
> helper, and it could be incorporated into the single driver.
>
> [*] The individual helper functions may still be useful for some other
>      driver, though.


Agree.


>> The second point which I extract from your comment is that the "gpio
>> aggregator" could alternatively acquire the role of "gpio-inverter"
>> (hence superseding it) by adding a "simple DT interface". I actually
>> tend to like this proposal, since (as said above) both drivers are
>> essentially doing the same thing, i.e. they cluster a number of gpio
>> lines and expose this cluster as a new gpiochip (keeping the
>> reserved/used gpio lines on hold). That looks like a huge overlap in
>> the functionalities of the two drivers.
> Yes, both drivers are very similar.  The difference lies in how they
> acquire the list of GPIO descriptors.

Yes. In fact my V2 version of the patch tried to implement the same role as repeater/forwarder albeit with a different naming/intention.

Linus Walleij mentioned that using GPIO_ACTIVE_LOW just to get free inversion inside GPIOLIB was not OK really and this is a hardware description problem and totally different from the implementation problem inside the driver.

Hence we changed the logic to inverter consumer driver doing inversion inside get and set functions.

>
>> The only difference which I see is that "gpio-inverter" is getting its
>> input from DT and generates the gpiochips at probe time, while
>> "gpio aggregator" is getting its input from sysfs and generates the
>> gpiochips at runtime, post-probe.
> Exactly.
>
> For my virtualization use case, I need to create the list of GPIO
> descriptors at run-time, hence the sysfs interface. This is
> polarity-agnostic (i.e. the end user needs to care about polarity).
>
> For Harish use case, he needs to describe the list from DT, with
> polarity inverted, which can be done by specifying the GPIO_ACTIVE_LOW
> flag in the node's"gpios" property.
>
> For your use case, you want to describe the list in DT, with line-names,
> and polarity specified.
>
>> So, assuming no objections from Harish and other reviewers, I would be
>> very happy to review and test the DT-based gpio inversion functionality
>> as part of gpio aggregator. Thanks!


I tested your aggregator driver with the below minor changes in gpio-aggregator (combined with some minor changes in GPIO forwarder) to get devicetree support.


195,196d194
<     int index = 0;
<     int count;
278,295d275
<     count = gpiod_count(dev, NULL);
<     if (count > 0) {
<         while (index < count) {
<             desc = devm_gpiod_get_index(dev, NULL, index, GPIOD_ASIS);
<
<             if (desc == ERR_PTR(-ENOENT))
<                 return -EPROBE_DEFER;
<
<             if (IS_ERR(desc))
<                 return PTR_ERR(desc);
<
<             error = add_gpio(dev, &descs, &n, desc);
<             if (error)
<                 return error;
<             index++;
<         }
<     }
<
316,319d295
< static const struct of_device_id gpio_aggregator_match[] = {
<     { .compatible =    "gpio-aggregator", }, { },
< };
<
326d301
<         .of_match_table = of_match_ptr(gpio_aggregator_match),


This does work and achieve our aim of inverter driver.

Hence no objection from my side to merge the drivers. Please let me know if I need to send you a patch on top of your aggregator patch.

Hoping to get some credits for my work of 5 months effort ! ;)


Best Regards,

Harish Jenny K N



^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH V4 2/2] gpio: inverter: document the inverter bindings
  2019-10-11  4:35         ` Harish Jenny K N
@ 2019-11-12 11:52           ` Harish Jenny K N
  2019-11-12 12:19             ` Geert Uytterhoeven
  0 siblings, 1 reply; 29+ messages in thread
From: Harish Jenny K N @ 2019-11-12 11:52 UTC (permalink / raw)
  To: Geert Uytterhoeven, Eugeniu Rosca
  Cc: Eugeniu Rosca, Linus Walleij, Rob Herring, Mark Rutland,
	Bartosz Golaszewski, Balasubramani Vivekanandan,
	Laurent Pinchart, Stephen Warren, Stephen Warren, Phil Reid,
	Enrico Weigelt, open list:GPIO SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

Hi Geert,


On 11/10/19 10:05 AM, Harish Jenny K N wrote:
> Hi Geert,
>
>
> On 07/10/19 1:48 PM, Geert Uytterhoeven wrote:
>> Hi Eugeniu,
>>
>> On Sat, Oct 5, 2019 at 3:08 PM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
>>> On Fri, Sep 27, 2019 at 11:07:20AM +0200, Geert Uytterhoeven wrote:
>>>> My standard reply would be: describe the device connected to the GPIO(s)
>>>> in DT.  The GPIO line polarities are specified in the device's "gpios"
>>>> properties.
>>>> Next step would be to use the device from Linux.  For that to work, you
>>>> need a dedicated driver (for the complex case), or something generic
>>>> (for the simple case).
>>>> The latter is not unlike e.g. spidev.  Once you have a generic driver,
>>>> you can use "driver_override" in sysfs to bind the generic driver to
>>>> your device.  See e.g. commit 5039563e7c25eccd ("spi: Add
>>>> driver_override SPI device attribute").
>>> We have passed your suggestions along. Many thanks.
>>>
>>>> Currently we don't have a "generic" driver for GPIOs. We do have the
>>>> GPIO chardev interface, which exports a full gpio_chip.
>>>> It indeed looks like this "gpio-inverter" could be used as a generic
>>>> driver.  But it is limited to GPIOs that are inverted, which rules out
>>>> some use cases.
>>>>
>>>> So what about making it more generic, and dropping the "inverter" from
>>>> its name, and the "inverted" from the "inverted-gpios" property? After
>>>> all the inversion can be specified by the polarity of the GPIO cells in
>>>> the "gpios" property, and the GPIO core will take care of it[*]?
>>>> Which boils down to adding a simple DT interface to my gpio-aggregator
>>>> ("[PATCH/RFC v2 0/5] gpio: Add GPIO Aggregator Driver",
>>>>  https://lore.kernel.org/lkml/20190911143858.13024-1-geert+renesas@glider.be/).
>>>> And now I have realized[*], we probably no longer need the GPIO
>>>> Forwarder Helper, as there is no need to add inversion on top.
>>> After having a look at the gpio aggregator (and giving it a try on
>>> R-Car3 H3ULCB), here is how I interpret the above comment:
>>>
>>> If there is still a compelling reason for having gpio-inverter, then it
>>> probably makes sense to strip it from its "inverter" function (hence,
>>> transforming it into some kind of "repeater") on the basis that the
>>> inverting function is more of a collateral/secondary feature, rather
>>> than its primary one. Just like in the case of gpio aggregator, the
>>> primary function of gpio inverter is to accept a bunch of GPIO lines and
>>> to expose those via a dedicated gpiochip. I hope this is a proper
>>> summary of the first point in your comment. In any case, this is the
>>> understanding I get based on my experiments with both drivers.
>> Yes, the inverter is basically a "repeater" (or "aggregator", when it has
>> multiple GPIOs connected), hardcoded to invert.
>>
>>> What I also infer is that, assuming gpio-inverter will stay (potentially
>>> renamed and stripped of its non-essential inverting function), the gpio
>>> aggregator will need to keep its Forwarder Helper (supposed to act as a
>>> common foundation for both drivers).
>> What I meant is that if the inverter and aggregator would be combinoed
>> into a single driver, there would no longer be a need[*] for a separate
>> helper, and it could be incorporated into the single driver.
>>
>> [*] The individual helper functions may still be useful for some other
>>      driver, though.
>
> Agree.
>
>
>>> The second point which I extract from your comment is that the "gpio
>>> aggregator" could alternatively acquire the role of "gpio-inverter"
>>> (hence superseding it) by adding a "simple DT interface". I actually
>>> tend to like this proposal, since (as said above) both drivers are
>>> essentially doing the same thing, i.e. they cluster a number of gpio
>>> lines and expose this cluster as a new gpiochip (keeping the
>>> reserved/used gpio lines on hold). That looks like a huge overlap in
>>> the functionalities of the two drivers.
>> Yes, both drivers are very similar.  The difference lies in how they
>> acquire the list of GPIO descriptors.
> Yes. In fact my V2 version of the patch tried to implement the same role as repeater/forwarder albeit with a different naming/intention.
>
> Linus Walleij mentioned that using GPIO_ACTIVE_LOW just to get free inversion inside GPIOLIB was not OK really and this is a hardware description problem and totally different from the implementation problem inside the driver.
>
> Hence we changed the logic to inverter consumer driver doing inversion inside get and set functions.
>
>>> The only difference which I see is that "gpio-inverter" is getting its
>>> input from DT and generates the gpiochips at probe time, while
>>> "gpio aggregator" is getting its input from sysfs and generates the
>>> gpiochips at runtime, post-probe.
>> Exactly.
>>
>> For my virtualization use case, I need to create the list of GPIO
>> descriptors at run-time, hence the sysfs interface. This is
>> polarity-agnostic (i.e. the end user needs to care about polarity).
>>
>> For Harish use case, he needs to describe the list from DT, with
>> polarity inverted, which can be done by specifying the GPIO_ACTIVE_LOW
>> flag in the node's"gpios" property.
>>
>> For your use case, you want to describe the list in DT, with line-names,
>> and polarity specified.
>>
>>> So, assuming no objections from Harish and other reviewers, I would be
>>> very happy to review and test the DT-based gpio inversion functionality
>>> as part of gpio aggregator. Thanks!
>
> I tested your aggregator driver with the below minor changes in gpio-aggregator (combined with some minor changes in GPIO forwarder) to get devicetree support.
>
>
> 195,196d194
> <     int index = 0;
> <     int count;
> 278,295d275
> <     count = gpiod_count(dev, NULL);
> <     if (count > 0) {
> <         while (index < count) {
> <             desc = devm_gpiod_get_index(dev, NULL, index, GPIOD_ASIS);
> <
> <             if (desc == ERR_PTR(-ENOENT))
> <                 return -EPROBE_DEFER;
> <
> <             if (IS_ERR(desc))
> <                 return PTR_ERR(desc);
> <
> <             error = add_gpio(dev, &descs, &n, desc);
> <             if (error)
> <                 return error;
> <             index++;
> <         }
> <     }
> <
> 316,319d295
> < static const struct of_device_id gpio_aggregator_match[] = {
> <     { .compatible =    "gpio-aggregator", }, { },
> < };
> <
> 326d301
> <         .of_match_table = of_match_ptr(gpio_aggregator_match),
>
>
> This does work and achieve our aim of inverter driver.
>
> Hence no objection from my side to merge the drivers. Please let me know if I need to send you a patch on top of your aggregator patch.
>
> Hoping to get some credits for my work of 5 months effort ! ;)
>
>
> Best Regards,
>
> Harish Jenny K N


Is any attempt being made for the newer version of the aggregator/inverter driver ?


Best Regards,

Harish Jenny K N

>

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH V4 2/2] gpio: inverter: document the inverter bindings
  2019-11-12 11:52           ` Harish Jenny K N
@ 2019-11-12 12:19             ` Geert Uytterhoeven
  0 siblings, 0 replies; 29+ messages in thread
From: Geert Uytterhoeven @ 2019-11-12 12:19 UTC (permalink / raw)
  To: Harish Jenny K N
  Cc: Eugeniu Rosca, Eugeniu Rosca, Linus Walleij, Rob Herring,
	Mark Rutland, Bartosz Golaszewski, Balasubramani Vivekanandan,
	Laurent Pinchart, Stephen Warren, Stephen Warren, Phil Reid,
	Enrico Weigelt, open list:GPIO SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

Hi Harish,

On Tue, Nov 12, 2019 at 12:52 PM Harish Jenny K N
<harish_kandiga@mentor.com> wrote:
> On 11/10/19 10:05 AM, Harish Jenny K N wrote:
> > On 07/10/19 1:48 PM, Geert Uytterhoeven wrote:
> >> On Sat, Oct 5, 2019 at 3:08 PM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
> >>> On Fri, Sep 27, 2019 at 11:07:20AM +0200, Geert Uytterhoeven wrote:
> >>>> My standard reply would be: describe the device connected to the GPIO(s)
> >>>> in DT.  The GPIO line polarities are specified in the device's "gpios"
> >>>> properties.
> >>>> Next step would be to use the device from Linux.  For that to work, you
> >>>> need a dedicated driver (for the complex case), or something generic
> >>>> (for the simple case).
> >>>> The latter is not unlike e.g. spidev.  Once you have a generic driver,
> >>>> you can use "driver_override" in sysfs to bind the generic driver to
> >>>> your device.  See e.g. commit 5039563e7c25eccd ("spi: Add
> >>>> driver_override SPI device attribute").
> >>> We have passed your suggestions along. Many thanks.
> >>>
> >>>> Currently we don't have a "generic" driver for GPIOs. We do have the
> >>>> GPIO chardev interface, which exports a full gpio_chip.
> >>>> It indeed looks like this "gpio-inverter" could be used as a generic
> >>>> driver.  But it is limited to GPIOs that are inverted, which rules out
> >>>> some use cases.
> >>>>
> >>>> So what about making it more generic, and dropping the "inverter" from
> >>>> its name, and the "inverted" from the "inverted-gpios" property? After
> >>>> all the inversion can be specified by the polarity of the GPIO cells in
> >>>> the "gpios" property, and the GPIO core will take care of it[*]?
> >>>> Which boils down to adding a simple DT interface to my gpio-aggregator
> >>>> ("[PATCH/RFC v2 0/5] gpio: Add GPIO Aggregator Driver",
> >>>>  https://lore.kernel.org/lkml/20190911143858.13024-1-geert+renesas@glider.be/).
> >>>> And now I have realized[*], we probably no longer need the GPIO
> >>>> Forwarder Helper, as there is no need to add inversion on top.
> >>> After having a look at the gpio aggregator (and giving it a try on
> >>> R-Car3 H3ULCB), here is how I interpret the above comment:
> >>>
> >>> If there is still a compelling reason for having gpio-inverter, then it
> >>> probably makes sense to strip it from its "inverter" function (hence,
> >>> transforming it into some kind of "repeater") on the basis that the
> >>> inverting function is more of a collateral/secondary feature, rather
> >>> than its primary one. Just like in the case of gpio aggregator, the
> >>> primary function of gpio inverter is to accept a bunch of GPIO lines and
> >>> to expose those via a dedicated gpiochip. I hope this is a proper
> >>> summary of the first point in your comment. In any case, this is the
> >>> understanding I get based on my experiments with both drivers.
> >> Yes, the inverter is basically a "repeater" (or "aggregator", when it has
> >> multiple GPIOs connected), hardcoded to invert.
> >>
> >>> What I also infer is that, assuming gpio-inverter will stay (potentially
> >>> renamed and stripped of its non-essential inverting function), the gpio
> >>> aggregator will need to keep its Forwarder Helper (supposed to act as a
> >>> common foundation for both drivers).
> >> What I meant is that if the inverter and aggregator would be combinoed
> >> into a single driver, there would no longer be a need[*] for a separate
> >> helper, and it could be incorporated into the single driver.
> >>
> >> [*] The individual helper functions may still be useful for some other
> >>      driver, though.
> >
> > Agree.
> >
> >
> >>> The second point which I extract from your comment is that the "gpio
> >>> aggregator" could alternatively acquire the role of "gpio-inverter"
> >>> (hence superseding it) by adding a "simple DT interface". I actually
> >>> tend to like this proposal, since (as said above) both drivers are
> >>> essentially doing the same thing, i.e. they cluster a number of gpio
> >>> lines and expose this cluster as a new gpiochip (keeping the
> >>> reserved/used gpio lines on hold). That looks like a huge overlap in
> >>> the functionalities of the two drivers.
> >> Yes, both drivers are very similar.  The difference lies in how they
> >> acquire the list of GPIO descriptors.
> > Yes. In fact my V2 version of the patch tried to implement the same role as repeater/forwarder albeit with a different naming/intention.
> >
> > Linus Walleij mentioned that using GPIO_ACTIVE_LOW just to get free inversion inside GPIOLIB was not OK really and this is a hardware description problem and totally different from the implementation problem inside the driver.
> >
> > Hence we changed the logic to inverter consumer driver doing inversion inside get and set functions.
> >
> >>> The only difference which I see is that "gpio-inverter" is getting its
> >>> input from DT and generates the gpiochips at probe time, while
> >>> "gpio aggregator" is getting its input from sysfs and generates the
> >>> gpiochips at runtime, post-probe.
> >> Exactly.
> >>
> >> For my virtualization use case, I need to create the list of GPIO
> >> descriptors at run-time, hence the sysfs interface. This is
> >> polarity-agnostic (i.e. the end user needs to care about polarity).
> >>
> >> For Harish use case, he needs to describe the list from DT, with
> >> polarity inverted, which can be done by specifying the GPIO_ACTIVE_LOW
> >> flag in the node's"gpios" property.
> >>
> >> For your use case, you want to describe the list in DT, with line-names,
> >> and polarity specified.
> >>
> >>> So, assuming no objections from Harish and other reviewers, I would be
> >>> very happy to review and test the DT-based gpio inversion functionality
> >>> as part of gpio aggregator. Thanks!
> >
> > I tested your aggregator driver with the below minor changes in gpio-aggregator (combined with some minor changes in GPIO forwarder) to get devicetree support.
> >
> >
> > 195,196d194
> > <     int index = 0;
> > <     int count;
> > 278,295d275
> > <     count = gpiod_count(dev, NULL);
> > <     if (count > 0) {
> > <         while (index < count) {
> > <             desc = devm_gpiod_get_index(dev, NULL, index, GPIOD_ASIS);
> > <
> > <             if (desc == ERR_PTR(-ENOENT))
> > <                 return -EPROBE_DEFER;
> > <
> > <             if (IS_ERR(desc))
> > <                 return PTR_ERR(desc);
> > <
> > <             error = add_gpio(dev, &descs, &n, desc);
> > <             if (error)
> > <                 return error;
> > <             index++;
> > <         }
> > <     }
> > <
> > 316,319d295
> > < static const struct of_device_id gpio_aggregator_match[] = {
> > <     { .compatible =    "gpio-aggregator", }, { },
> > < };
> > <
> > 326d301
> > <         .of_match_table = of_match_ptr(gpio_aggregator_match),
> >
> >
> > This does work and achieve our aim of inverter driver.
> >
> > Hence no objection from my side to merge the drivers. Please let me know if I need to send you a patch on top of your aggregator patch.
> >
> > Hoping to get some credits for my work of 5 months effort ! ;)
> >
> >
> > Best Regards,
> >
> > Harish Jenny K N
>
>
> Is any attempt being made for the newer version of the aggregator/inverter driver ?

It's on my list, and I hope to tackle it soon (later this week, or next week).
Thanks for your patience!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH V4 2/2] gpio: inverter: document the inverter bindings
  2019-06-28  5:20 [PATCH V4 0/2] Add Inverter controller for gpio configuration Harish Jenny K N
@ 2019-06-28  5:20 ` Harish Jenny K N
  0 siblings, 0 replies; 29+ messages in thread
From: Harish Jenny K N @ 2019-06-28  5:20 UTC (permalink / raw)
  To: Linus Walleij, Bartosz Golaszewski
  Cc: linux-gpio, Harish Jenny K N, Balasubramani Vivekanandan

Document the device tree binding for the inverter gpio
controller to configure the polarity of the gpio pins
used by the consumers.

Signed-off-by: Harish Jenny K N <harish_kandiga@mentor.com>
---
 .../devicetree/bindings/gpio/gpio-inverter.txt     | 29 ++++++++++++++++++++++
 1 file changed, 29 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-inverter.txt

diff --git a/Documentation/devicetree/bindings/gpio/gpio-inverter.txt b/Documentation/devicetree/bindings/gpio/gpio-inverter.txt
new file mode 100644
index 0000000..8bb6b2e
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-inverter.txt
@@ -0,0 +1,29 @@
+GPIO-INVERTER
+======
+This binding defines the gpio-inverter. The gpio-inverter is a driver that
+allows to properly describe the gpio polarities on the hardware.
+
+Please refer to gpio.txt for generic information regarding GPIO bindings.
+
+Required properties:
+- compatible : "gpio-inverter".
+- gpio-controller: Marks the port as GPIO controller.
+- #gpio-cells: One. This is the pin number.
+- inverted-gpios: Array of GPIO pins required from consumers, whose polarity
+  has to be inverted in the driver.
+Note: gpio flag should be set as GPIO_ACTIVE_HIGH. Using GPIO_ACTICE_LOW will
+cause double inversion.
+
+Optional properties:
+- gpio-line-names: Refer to gpio.txt for details regarding this property.
+
+Example:
+
+gpio_inv: gpio-inv {
+	compatible = "gpio-inverter";
+	gpio-controller;
+	#gpio-cells = <1>;
+	inverted-gpios = <&gpio5 24 GPIO_ACTIVE_HIGH>,
+	<&gpio7 0 GPIO_ACTIVE_HIGH>, <&gpio7 1 GPIO_ACTIVE_HIGH>;
+	gpio-line-names = "JTAG_DNL_EN", "lvds-pwrdwn", "lcd-on";
+};
--
2.7.4


^ permalink raw reply related	[flat|nested] 29+ messages in thread

end of thread, other threads:[~2019-11-12 12:19 UTC | newest]

Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-28  9:30 [PATCH V4 2/2] gpio: inverter: document the inverter bindings Harish Jenny K N
2019-07-04  5:01 ` Harish Jenny K N
2019-07-08 22:36 ` Rob Herring
2019-07-09  5:25   ` Harish Jenny K N
2019-07-09 16:08     ` Rob Herring
2019-07-10  8:28       ` Harish Jenny K N
2019-07-17 13:51         ` Harish Jenny K N
2019-07-29 11:07           ` Harish Jenny K N
2019-08-05 11:15         ` Linus Walleij
2019-08-09 14:08           ` Rob Herring
2019-08-10  8:51             ` Linus Walleij
2019-08-19  9:36               ` Harish Jenny K N
2019-08-27  7:47                 ` Harish Jenny K N
2019-08-30  5:21                   ` Harish Jenny K N
2019-09-04  4:58                     ` Harish Jenny K N
2019-09-10  7:47                       ` Rob Herring
2019-09-11 12:52                         ` Harish Jenny K N
2019-09-25 16:51 ` Eugeniu Rosca
2019-09-27  5:52   ` Phil Reid
2019-09-27  9:07   ` Geert Uytterhoeven
2019-10-05 13:07     ` Eugeniu Rosca
2019-10-07  8:18       ` Geert Uytterhoeven
2019-10-11  4:35         ` Harish Jenny K N
2019-11-12 11:52           ` Harish Jenny K N
2019-11-12 12:19             ` Geert Uytterhoeven
2019-10-04 19:07   ` Stephen Warren
2019-10-05 17:50     ` Eugeniu Rosca
2019-10-07 15:36       ` Stephen Warren
  -- strict thread matches above, loose matches on Subject: below --
2019-06-28  5:20 [PATCH V4 0/2] Add Inverter controller for gpio configuration Harish Jenny K N
2019-06-28  5:20 ` [PATCH V4 2/2] gpio: inverter: document the inverter bindings Harish Jenny K N

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).