Linux-GPIO Archive on lore.kernel.org
 help / color / 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
  2019-07-08 22:36 ` Rob Herring
  0 siblings, 2 replies; 13+ 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	[flat|nested] 13+ 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
  1 sibling, 0 replies; 13+ 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] 13+ 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
  1 sibling, 1 reply; 13+ 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] 13+ 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; 13+ 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] 13+ 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; 13+ 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] 13+ 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; 13+ 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] 13+ 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; 13+ 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] 13+ 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; 13+ 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] 13+ 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; 13+ 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] 13+ 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; 13+ 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] 13+ 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; 13+ 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] 13+ 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
  0 siblings, 0 replies; 13+ 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] 13+ 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; 13+ 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	[flat|nested] 13+ messages in thread

end of thread, back to index

Thread overview: 13+ 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
  -- 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

Linux-GPIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-gpio/0 linux-gpio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-gpio linux-gpio/ https://lore.kernel.org/linux-gpio \
		linux-gpio@vger.kernel.org linux-gpio@archiver.kernel.org
	public-inbox-index linux-gpio


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-gpio


AGPL code for this site: git clone https://public-inbox.org/ public-inbox