[1/3] Device tree binding documentation for gpio-switch
diff mbox

Message ID 1449250275-23435-2-git-send-email-martyn.welch@collabora.co.uk
State New, archived
Headers show

Commit Message

Martyn Welch Dec. 4, 2015, 5:31 p.m. UTC
This patch adds documentation for the gpio-switch binding. This binding
provides a mechanism to bind named links to gpio, with the primary
purpose of enabling standardised access to switches that might be standard
across a group of devices but implemented differently on each device.

Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
---
 .../devicetree/bindings/misc/gpio-switch.txt       | 47 ++++++++++++++++++++++
 1 file changed, 47 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/gpio-switch.txt

Comments

Rob Herring Dec. 7, 2015, 5:37 p.m. UTC | #1
+Linus W

On Fri, Dec 04, 2015 at 05:31:13PM +0000, Martyn Welch wrote:
> This patch adds documentation for the gpio-switch binding. This binding
> provides a mechanism to bind named links to gpio, with the primary
> purpose of enabling standardised access to switches that might be standard
> across a group of devices but implemented differently on each device.

This is good and what I suggested, but it now makes me wonder if switch 
is generic enough. This boils down to needing to expose single gpio 
lines to userspace with a defined function/use. IIRC, there's been some 
discussion about this before along with improving the userspace 
interface for GPIO in general. So I'd like to get Linus' thoughts on 
this.


> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
> ---
>  .../devicetree/bindings/misc/gpio-switch.txt       | 47 ++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/misc/gpio-switch.txt
> 
> diff --git a/Documentation/devicetree/bindings/misc/gpio-switch.txt b/Documentation/devicetree/bindings/misc/gpio-switch.txt
> new file mode 100644
> index 0000000..13528bd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/gpio-switch.txt
> @@ -0,0 +1,47 @@
> +Device-Tree bindings for gpio attached switches.
> +
> +This provides a mechanism to provide a named link to specified gpios. This can
> +be useful in instances such as when theres a need to monitor a switch, which is
> +common across a family of devices, but attached to different gpios and even
> +implemented in different ways on differnet devices.
> +
> +Required properties:
> +	- compatible = "gpio-switch";
> +
> +Each signal is represented as a sub-node of "gpio-switch". The naming of
> +sub-nodes is arbitrary.
> +
> +Required sub-node properties:
> +
> +	- label: Name to be given to gpio switch.
> +	- gpios: OF device-tree gpio specification.
> +
> +Optional sub-node properties:
> +
> +	- read-only: Boolean flag to mark the gpio as read-only, i.e. the line
> +	  should not be driven by the host.

In terms a a switch use, allowing driving it would be an override of the 
switch. Is that the idea here?

> +
> +Example nodes:
> +
> +        gpio-switch {
> +                compatible = "gpio-switch";

Both from a binding and driver perspective, there is no point in 
grouping these. Each node can simply have this compatible string.

> +
> +                write-protect {
> +                        label = "write-protect";
> +                        gpios = <&gpx3 0 GPIO_ACTIVE_LOW>;
> +                        read-only;
> +                };
> +
> +                developer-switch {
> +                        label = "developer-switch";
> +                        gpios = <&gpx1 3 GPIO_ACTIVE_HIGH>;
> +                        read-only;
> +                };
> +
> +                recovery-switch {
> +                        label = "recovery-switch";
> +                        gpios = <&gpx0 7 GPIO_ACTIVE_LOW>;
> +                        read-only;
> +                };
> +        };
> +
> -- 
> 2.1.4
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Martyn Welch Dec. 7, 2015, 9:10 p.m. UTC | #2
On 07/12/15 17:37, Rob Herring wrote:
> +Linus W
>
> On Fri, Dec 04, 2015 at 05:31:13PM +0000, Martyn Welch wrote:
>> This patch adds documentation for the gpio-switch binding. This binding
>> provides a mechanism to bind named links to gpio, with the primary
>> purpose of enabling standardised access to switches that might be standard
>> across a group of devices but implemented differently on each device.
>
> This is good and what I suggested, but it now makes me wonder if switch
> is generic enough. This boils down to needing to expose single gpio
> lines to userspace with a defined function/use. IIRC, there's been some
> discussion about this before along with improving the userspace
> interface for GPIO in general. So I'd like to get Linus' thoughts on
> this.
>

No problem. Rename gpio-signal?

>
>> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
>> ---
>>   .../devicetree/bindings/misc/gpio-switch.txt       | 47 ++++++++++++++++++++++
>>   1 file changed, 47 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/misc/gpio-switch.txt
>>
>> diff --git a/Documentation/devicetree/bindings/misc/gpio-switch.txt b/Documentation/devicetree/bindings/misc/gpio-switch.txt
>> new file mode 100644
>> index 0000000..13528bd
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/misc/gpio-switch.txt
>> @@ -0,0 +1,47 @@
>> +Device-Tree bindings for gpio attached switches.
>> +
>> +This provides a mechanism to provide a named link to specified gpios. This can
>> +be useful in instances such as when theres a need to monitor a switch, which is
>> +common across a family of devices, but attached to different gpios and even
>> +implemented in different ways on differnet devices.
>> +
>> +Required properties:
>> +	- compatible = "gpio-switch";
>> +
>> +Each signal is represented as a sub-node of "gpio-switch". The naming of
>> +sub-nodes is arbitrary.
>> +
>> +Required sub-node properties:
>> +
>> +	- label: Name to be given to gpio switch.
>> +	- gpios: OF device-tree gpio specification.
>> +
>> +Optional sub-node properties:
>> +
>> +	- read-only: Boolean flag to mark the gpio as read-only, i.e. the line
>> +	  should not be driven by the host.
>
> In terms a a switch use, allowing driving it would be an override of the
> switch. Is that the idea here?
>

Yeah - since it had become a lot more generic and a lot of 
switches/signals would probably be implemented with a pull-up resistor 
of something like that, it seemed to make sense to allow them to be 
driven as well.

>> +
>> +Example nodes:
>> +
>> +        gpio-switch {
>> +                compatible = "gpio-switch";
>
> Both from a binding and driver perspective, there is no point in
> grouping these. Each node can simply have this compatible string.
>

True. I did it this way as this is how gpio-keys is implemented. OK, 
that has one optional parameter (autorepeat) for the block and this has 
none. Though I can also see that these probably have less in common than 
the individual lines used in gpio-keys.

>> +
>> +                write-protect {
>> +                        label = "write-protect";
>> +                        gpios = <&gpx3 0 GPIO_ACTIVE_LOW>;
>> +                        read-only;
>> +                };
>> +
>> +                developer-switch {
>> +                        label = "developer-switch";
>> +                        gpios = <&gpx1 3 GPIO_ACTIVE_HIGH>;
>> +                        read-only;
>> +                };
>> +
>> +                recovery-switch {
>> +                        label = "recovery-switch";
>> +                        gpios = <&gpx0 7 GPIO_ACTIVE_LOW>;
>> +                        read-only;
>> +                };
>> +        };
>> +
>> --
>> 2.1.4
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Dec. 11, 2015, 12:39 p.m. UTC | #3
On Fri, Dec 4, 2015 at 6:31 PM, Martyn Welch
<martyn.welch@collabora.co.uk> wrote:

> This patch adds documentation for the gpio-switch binding. This binding
> provides a mechanism to bind named links to gpio, with the primary
> purpose of enabling standardised access to switches that might be standard
> across a group of devices but implemented differently on each device.
>
> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>

As mentioned in the comment to the second patch, this solves the
following generic problem:

Expose a GPIO line to userspace using a specific name

That means basically naming GPIO lines and marking them as
"not used by the operating system".

This is something that has been proposed before, and postponed
because the kernel lacks the right infrastructure.

Markus Pargmann also did a series that add initial values to
hogs, which is the inverse usecase of this, where you want to
*output* something by default, then maybe also make it available
to userspace.

So what we need to see here is a patch series that does all of these
things:

- Name lines

- Sets them to initial values

- Mark them as read-only

- Mark them as "not used by the operating system" so that they
  can be default-exported to userspace.

Making *USE* of this naming etc inside the Linux kernel

> +        gpio-switch {
> +                compatible = "gpio-switch";
> +
> +                write-protect {
> +                        label = "write-protect";
> +                        gpios = <&gpx3 0 GPIO_ACTIVE_LOW>;
> +                        read-only;
> +                };

This should not need new structures and nodes like this. It should
be part of Documentation/devicetree/bindings/gpio/gpio.txt
and put directly in the gpiochip node.

Maybe as an extension of the existing hogs, but that has already
been tried.

While we can agree on a device tree binding, the kernel still needs
major refactoring to actually expose named GPIOs to userspace,
and that should be done using the new chardev, not with sysfs
links.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring Dec. 11, 2015, 2:06 p.m. UTC | #4
On Fri, Dec 11, 2015 at 6:39 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Dec 4, 2015 at 6:31 PM, Martyn Welch
> <martyn.welch@collabora.co.uk> wrote:
>
>> This patch adds documentation for the gpio-switch binding. This binding
>> provides a mechanism to bind named links to gpio, with the primary
>> purpose of enabling standardised access to switches that might be standard
>> across a group of devices but implemented differently on each device.
>>
>> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
>
> As mentioned in the comment to the second patch, this solves the
> following generic problem:
>
> Expose a GPIO line to userspace using a specific name
>
> That means basically naming GPIO lines and marking them as
> "not used by the operating system".
>
> This is something that has been proposed before, and postponed
> because the kernel lacks the right infrastructure.

That doesn't necessarily mean we can't define a binding.

> Markus Pargmann also did a series that add initial values to
> hogs, which is the inverse usecase of this, where you want to
> *output* something by default, then maybe also make it available
> to userspace.
>
> So what we need to see here is a patch series that does all of these
> things:
>
> - Name lines
>
> - Sets them to initial values
>
> - Mark them as read-only
>
> - Mark them as "not used by the operating system" so that they
>   can be default-exported to userspace.

No! This should not be a DT property.

Whether I want to control a GPIO in the kernel or userspace is not
known and can change over time. It could simply depend on kernel
config. There is also the case that a GPIO has no connection or kernel
driver until some time later when a DT overlay for an expansion board
is applied.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Dec. 14, 2015, 2:28 p.m. UTC | #5
On Fri, Dec 11, 2015 at 3:06 PM, Rob Herring <robh+dt@kernel.org> wrote:
> On Fri, Dec 11, 2015 at 6:39 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Fri, Dec 4, 2015 at 6:31 PM, Martyn Welch
>> <martyn.welch@collabora.co.uk> wrote:
>>
>>> This patch adds documentation for the gpio-switch binding. This binding
>>> provides a mechanism to bind named links to gpio, with the primary
>>> purpose of enabling standardised access to switches that might be standard
>>> across a group of devices but implemented differently on each device.
>>>
>>> Signed-off-by: Martyn Welch <martyn.welch@collabora.co.uk>
>>
>> As mentioned in the comment to the second patch, this solves the
>> following generic problem:
>>
>> Expose a GPIO line to userspace using a specific name
>>
>> That means basically naming GPIO lines and marking them as
>> "not used by the operating system".
>>
>> This is something that has been proposed before, and postponed
>> because the kernel lacks the right infrastructure.
>
> That doesn't necessarily mean we can't define a binding.

Yeah that's true. just saying that the other stuff was not merged
for this reason, but then we can have a look at Markus' bindings
in parallell, Markus can you repost them to this audience? Only
the bindings I mean.

>> Markus Pargmann also did a series that add initial values to
>> hogs, which is the inverse usecase of this, where you want to
>> *output* something by default, then maybe also make it available
>> to userspace.
>>
>> So what we need to see here is a patch series that does all of these
>> things:
>>
>> - Name lines
>>
>> - Sets them to initial values
>>
>> - Mark them as read-only
>>
>> - Mark them as "not used by the operating system" so that they
>>   can be default-exported to userspace.
>
> No! This should not be a DT property.
>
> Whether I want to control a GPIO in the kernel or userspace is not
> known and can change over time. It could simply depend on kernel
> config. There is also the case that a GPIO has no connection or kernel
> driver until some time later when a DT overlay for an expansion board
> is applied.

That's correct. So from a DT point of view, what really matters is
to give things a name, and the hogs and initvals syntax already
has a structure for this that is in use
(from Documentation/devicetree/bindings/gpio/gpio.txt):

        qe_pio_a: gpio-controller@1400 {
                compatible = "fsl,qe-pario-bank-a", "fsl,qe-pario-bank";
                reg = <0x1400 0x18>;
                gpio-controller;
                #gpio-cells = <2>;

                line_b {
                        gpio-hog;
                        gpios = <6 0>;
                        output-low;
                        line-name = "foo-bar-gpio";
                };
        };

Markus use this also for initial values. That could easily be used in
a budget version like this:

                line_b {
                        /* Just naming */
                        gpios = <6 0>;
                        line-name = "foo-bar-gpio";
                };

That could grow kind of big though. Or maybe not? How many
GPIO lines are actually properly named in a typical system?

The problem is that naming is usually imposed by consumers (they
are the ones who know how the line is used).

And then I do not mean naming it after the pin on the capsule
where it comes out as per the datasheet, but
naming it after the actual use.

Naming it after the hardware specs is something the operating
system can do, in Linux' case by the array char *names; inside the
struct gpio_chip and should not be part of the bindings IMO.

I would rather imagine this is something used in a top-level board
file naming it: "header-2-22" for the 22nd pin on some second header
on my BeagleBone Black or something like that. And those may not
be so vast in numbers so they could be named using this pattern.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring Dec. 14, 2015, 3:45 p.m. UTC | #6
On Mon, Dec 14, 2015 at 8:28 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Dec 11, 2015 at 3:06 PM, Rob Herring <robh+dt@kernel.org> wrote:
>> On Fri, Dec 11, 2015 at 6:39 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>> On Fri, Dec 4, 2015 at 6:31 PM, Martyn Welch
>>> <martyn.welch@collabora.co.uk> wrote:

[...]

>>> Markus Pargmann also did a series that add initial values to
>>> hogs, which is the inverse usecase of this, where you want to
>>> *output* something by default, then maybe also make it available
>>> to userspace.
>>>
>>> So what we need to see here is a patch series that does all of these
>>> things:
>>>
>>> - Name lines
>>>
>>> - Sets them to initial values
>>>
>>> - Mark them as read-only
>>>
>>> - Mark them as "not used by the operating system" so that they
>>>   can be default-exported to userspace.
>>
>> No! This should not be a DT property.
>>
>> Whether I want to control a GPIO in the kernel or userspace is not
>> known and can change over time. It could simply depend on kernel
>> config. There is also the case that a GPIO has no connection or kernel
>> driver until some time later when a DT overlay for an expansion board
>> is applied.
>
> That's correct. So from a DT point of view, what really matters is
> to give things a name, and the hogs and initvals syntax already
> has a structure for this that is in use
> (from Documentation/devicetree/bindings/gpio/gpio.txt):
>
>         qe_pio_a: gpio-controller@1400 {
>                 compatible = "fsl,qe-pario-bank-a", "fsl,qe-pario-bank";
>                 reg = <0x1400 0x18>;
>                 gpio-controller;
>                 #gpio-cells = <2>;
>
>                 line_b {
>                         gpio-hog;
>                         gpios = <6 0>;
>                         output-low;
>                         line-name = "foo-bar-gpio";
>                 };
>         };
>
> Markus use this also for initial values. That could easily be used in
> a budget version like this:
>
>                 line_b {
>                         /* Just naming */
>                         gpios = <6 0>;
>                         line-name = "foo-bar-gpio";
>                 };
>
> That could grow kind of big though. Or maybe not? How many
> GPIO lines are actually properly named in a typical system?

We should limit it to GPIOs with no connection to another node. For
example, I don't want to see a SD card detect in the list as that
should be in the SD host node. However, that is hard to enforce and
can change over time. Removing it would break userspace potentially.
Of course if the kernel starts own a signal that userspace used, then
that potentially breaks userspace regardless of the DT changing. OTOH,
using GPIOs in userspace is kind of use at your own risk.

The only real differences between this and Martyn's proposal are the
location of the nodes and having a compatible string. A compatible
string may be good to have. It indicates type of signal, but not
specific use. Similar to how gpio-key compatible defines the function,
but not what key code, or gpio-led does not say what color LED. A
compatible here could cover switches, mode/id/revision strapping
signals, jumpers, presence detect, etc.

> The problem is that naming is usually imposed by consumers (they
> are the ones who know how the line is used).
>
> And then I do not mean naming it after the pin on the capsule
> where it comes out as per the datasheet, but
> naming it after the actual use.

Right. We need a way to query "I need the GPIO that does X" which
works across boards.

> Naming it after the hardware specs is something the operating
> system can do, in Linux' case by the array char *names; inside the
> struct gpio_chip and should not be part of the bindings IMO.

Agreed. That just came up with STM32 gpio bindings[1].

> I would rather imagine this is something used in a top-level board
> file naming it: "header-2-22" for the 22nd pin on some second header
> on my BeagleBone Black or something like that. And those may not
> be so vast in numbers so they could be named using this pattern.

Exactly. This is one of the cases I care about. However, this is not
really a function, but it is SOC independent at least.

We also have to consider how to handle add-on boards. We probably need
a connector node which can remap connector signals to host signals in
order to have overlays independent of the host board DT. For GPIOs
this is probably a gpio-map property similar to interrupt-map. The
complicated part will be connectors that require pinmux setup of the
host.

Rob

[1] https://lkml.org/lkml/2015/12/11/600
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Markus Pargmann Dec. 15, 2015, 9:09 a.m. UTC | #7
Hi,

On Monday 14 December 2015 09:45:48 Rob Herring wrote:
> On Mon, Dec 14, 2015 at 8:28 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Fri, Dec 11, 2015 at 3:06 PM, Rob Herring <robh+dt@kernel.org> wrote:
> >> On Fri, Dec 11, 2015 at 6:39 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> >>> On Fri, Dec 4, 2015 at 6:31 PM, Martyn Welch
> >>> <martyn.welch@collabora.co.uk> wrote:
> 
> [...]
> 
> >>> Markus Pargmann also did a series that add initial values to
> >>> hogs, which is the inverse usecase of this, where you want to
> >>> *output* something by default, then maybe also make it available
> >>> to userspace.
> >>>
> >>> So what we need to see here is a patch series that does all of these
> >>> things:
> >>>
> >>> - Name lines
> >>>
> >>> - Sets them to initial values
> >>>
> >>> - Mark them as read-only
> >>>
> >>> - Mark them as "not used by the operating system" so that they
> >>>   can be default-exported to userspace.
> >>
> >> No! This should not be a DT property.
> >>
> >> Whether I want to control a GPIO in the kernel or userspace is not
> >> known and can change over time. It could simply depend on kernel
> >> config. There is also the case that a GPIO has no connection or kernel
> >> driver until some time later when a DT overlay for an expansion board
> >> is applied.
> >
> > That's correct. So from a DT point of view, what really matters is
> > to give things a name, and the hogs and initvals syntax already
> > has a structure for this that is in use
> > (from Documentation/devicetree/bindings/gpio/gpio.txt):
> >
> >         qe_pio_a: gpio-controller@1400 {
> >                 compatible = "fsl,qe-pario-bank-a", "fsl,qe-pario-bank";
> >                 reg = <0x1400 0x18>;
> >                 gpio-controller;
> >                 #gpio-cells = <2>;
> >
> >                 line_b {
> >                         gpio-hog;
> >                         gpios = <6 0>;
> >                         output-low;
> >                         line-name = "foo-bar-gpio";
> >                 };
> >         };
> >
> > Markus use this also for initial values. That could easily be used in
> > a budget version like this:
> >
> >                 line_b {
> >                         /* Just naming */
> >                         gpios = <6 0>;
> >                         line-name = "foo-bar-gpio";
> >                 };
> >
> > That could grow kind of big though. Or maybe not? How many
> > GPIO lines are actually properly named in a typical system?
> 
> We should limit it to GPIOs with no connection to another node. For
> example, I don't want to see a SD card detect in the list as that
> should be in the SD host node. However, that is hard to enforce and
> can change over time. Removing it would break userspace potentially.
> Of course if the kernel starts own a signal that userspace used, then
> that potentially breaks userspace regardless of the DT changing. OTOH,
> using GPIOs in userspace is kind of use at your own risk.

I see this a bit differently. I would really like to see the each GPIO having
two different names:
- GPIO label: This is the name of the GPIO line in the schematic for example
- GPIO use (this is the current semantic of the GPIO name): The use of a GPIO,
  e.g. 'sd-card-detect', 'LED', ...

I think it would be good to describe all GPIO labels in gpiochip subnodes as
gpio-hogging introduced it. This would offer a use-independent naming. The use
of the function could be defined in the device node that is using this gpio.

As an example perhaps something like this:

	&gpiochip {
		some_interrupt {
			gpios = <4 0>;
			line-name = "some_interrupt_line";
		};

		line_b {
			gpios = <6 0>;
			line-name = "line-b";
		};
	};

	randomswitch {
		compatible = "gpio-switch";
		gpios = <&gpiochip 4 0>;
		use = "action-trigger";
		read-only;
	};

Also this does seem kind of inconsistent with gpio-hogging and the proposed
gpio-initval. gpio-hogging is defined in subnodes of the gpio chip while
gpio-switches are not. As "gpio-switch" is not really any kind of device it
would perhaps make sense to keep this consistent with gpio-hogging as well and
define it in the same subnodes?
I would be happy about any consistent way.

> 
> The only real differences between this and Martyn's proposal are the
> location of the nodes and having a compatible string. A compatible
> string may be good to have. It indicates type of signal, but not
> specific use. Similar to how gpio-key compatible defines the function,
> but not what key code, or gpio-led does not say what color LED. A
> compatible here could cover switches, mode/id/revision strapping
> signals, jumpers, presence detect, etc.
> 
> > The problem is that naming is usually imposed by consumers (they
> > are the ones who know how the line is used).
> >
> > And then I do not mean naming it after the pin on the capsule
> > where it comes out as per the datasheet, but
> > naming it after the actual use.
> 
> Right. We need a way to query "I need the GPIO that does X" which
> works across boards.
> 
> > Naming it after the hardware specs is something the operating
> > system can do, in Linux' case by the array char *names; inside the
> > struct gpio_chip and should not be part of the bindings IMO.
> 
> Agreed. That just came up with STM32 gpio bindings[1].
> 
> > I would rather imagine this is something used in a top-level board
> > file naming it: "header-2-22" for the 22nd pin on some second header
> > on my BeagleBone Black or something like that. And those may not
> > be so vast in numbers so they could be named using this pattern.
> 
> Exactly. This is one of the cases I care about. However, this is not
> really a function, but it is SOC independent at least.
> 
> We also have to consider how to handle add-on boards. We probably need
> a connector node which can remap connector signals to host signals in
> order to have overlays independent of the host board DT. For GPIOs
> this is probably a gpio-map property similar to interrupt-map. The
> complicated part will be connectors that require pinmux setup of the
> host.

Also what about hotplugging gpio-chips? Is there any mechanism to let the
'gpio-switch' know that the GPIO is not there anymore?

Best Regards,

Markus
Rob Herring March 2, 2016, 4:03 p.m. UTC | #8
Reviving this thread...

On Tue, Dec 15, 2015 at 3:09 AM, Markus Pargmann <mpa@pengutronix.de> wrote:
> Hi,
>
> On Monday 14 December 2015 09:45:48 Rob Herring wrote:
>> On Mon, Dec 14, 2015 at 8:28 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> > On Fri, Dec 11, 2015 at 3:06 PM, Rob Herring <robh+dt@kernel.org> wrote:
>> >> On Fri, Dec 11, 2015 at 6:39 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> >>> On Fri, Dec 4, 2015 at 6:31 PM, Martyn Welch
>> >>> <martyn.welch@collabora.co.uk> wrote:
>>
>> [...]
>>
>> >>> Markus Pargmann also did a series that add initial values to
>> >>> hogs, which is the inverse usecase of this, where you want to
>> >>> *output* something by default, then maybe also make it available
>> >>> to userspace.
>> >>>
>> >>> So what we need to see here is a patch series that does all of these
>> >>> things:
>> >>>
>> >>> - Name lines
>> >>>
>> >>> - Sets them to initial values
>> >>>
>> >>> - Mark them as read-only
>> >>>
>> >>> - Mark them as "not used by the operating system" so that they
>> >>>   can be default-exported to userspace.
>> >>
>> >> No! This should not be a DT property.
>> >>
>> >> Whether I want to control a GPIO in the kernel or userspace is not
>> >> known and can change over time. It could simply depend on kernel
>> >> config. There is also the case that a GPIO has no connection or kernel
>> >> driver until some time later when a DT overlay for an expansion board
>> >> is applied.
>> >
>> > That's correct. So from a DT point of view, what really matters is
>> > to give things a name, and the hogs and initvals syntax already
>> > has a structure for this that is in use
>> > (from Documentation/devicetree/bindings/gpio/gpio.txt):
>> >
>> >         qe_pio_a: gpio-controller@1400 {
>> >                 compatible = "fsl,qe-pario-bank-a", "fsl,qe-pario-bank";
>> >                 reg = <0x1400 0x18>;
>> >                 gpio-controller;
>> >                 #gpio-cells = <2>;
>> >
>> >                 line_b {
>> >                         gpio-hog;
>> >                         gpios = <6 0>;
>> >                         output-low;
>> >                         line-name = "foo-bar-gpio";
>> >                 };
>> >         };
>> >
>> > Markus use this also for initial values. That could easily be used in
>> > a budget version like this:
>> >
>> >                 line_b {
>> >                         /* Just naming */
>> >                         gpios = <6 0>;
>> >                         line-name = "foo-bar-gpio";
>> >                 };
>> >
>> > That could grow kind of big though. Or maybe not? How many
>> > GPIO lines are actually properly named in a typical system?
>>
>> We should limit it to GPIOs with no connection to another node. For
>> example, I don't want to see a SD card detect in the list as that
>> should be in the SD host node. However, that is hard to enforce and
>> can change over time. Removing it would break userspace potentially.
>> Of course if the kernel starts own a signal that userspace used, then
>> that potentially breaks userspace regardless of the DT changing. OTOH,
>> using GPIOs in userspace is kind of use at your own risk.
>
> I see this a bit differently. I would really like to see the each GPIO having
> two different names:

I think we are saying the same thing...

> - GPIO label: This is the name of the GPIO line in the schematic for example

Yes.

> - GPIO use (this is the current semantic of the GPIO name): The use of a GPIO,
>   e.g. 'sd-card-detect', 'LED', ...

This should be determined from the compatible string and/or -gpios
prefix. This is the what the function is and "label" is which one.

> I think it would be good to describe all GPIO labels in gpiochip subnodes as
> gpio-hogging introduced it. This would offer a use-independent naming. The use
> of the function could be defined in the device node that is using this gpio.

I think I agree here. You may have a defined function without any
connection to another node. I also think we should encourage simple
GPIO bindings like gpio-leds to be child nodes. Having them at the
top-level is kind of arbitrary. Of course, allowing for both is
required.

> As an example perhaps something like this:
>
>         &gpiochip {
>                 some_interrupt {
>                         gpios = <4 0>;
>                         line-name = "some_interrupt_line";
>                 };
>
>                 line_b {
>                         gpios = <6 0>;
>                         line-name = "line-b";
>                 };
>         };
>
>         randomswitch {
>                 compatible = "gpio-switch";
>                 gpios = <&gpiochip 4 0>;
>                 use = "action-trigger";
>                 read-only;
>         };
>
> Also this does seem kind of inconsistent with gpio-hogging and the proposed
> gpio-initval. gpio-hogging is defined in subnodes of the gpio chip while
> gpio-switches are not. As "gpio-switch" is not really any kind of device it
> would perhaps make sense to keep this consistent with gpio-hogging as well and
> define it in the same subnodes?
> I would be happy about any consistent way.

Yes, as well as gpio leds, keys, etc. bindings. The key is you would
need to be able to start with something minimal and extend it with
specific compatibles.

[...]

>> We also have to consider how to handle add-on boards. We probably need
>> a connector node which can remap connector signals to host signals in
>> order to have overlays independent of the host board DT. For GPIOs
>> this is probably a gpio-map property similar to interrupt-map. The
>> complicated part will be connectors that require pinmux setup of the
>> host.
>
> Also what about hotplugging gpio-chips? Is there any mechanism to let the
> 'gpio-switch' know that the GPIO is not there anymore?

There are certainly issues around hotplug and GPIOs. If the
gpio-switch device is a child of the gpio controller, then it should
be possible.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Markus Pargmann March 7, 2016, 8:26 a.m. UTC | #9
On Wednesday 02 March 2016 10:03:27 Rob Herring wrote:
> Reviving this thread...
> 
> On Tue, Dec 15, 2015 at 3:09 AM, Markus Pargmann <mpa@pengutronix.de> wrote:
> > Hi,
> >
> > On Monday 14 December 2015 09:45:48 Rob Herring wrote:
> >> On Mon, Dec 14, 2015 at 8:28 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> >> > On Fri, Dec 11, 2015 at 3:06 PM, Rob Herring <robh+dt@kernel.org> wrote:
> >> >> On Fri, Dec 11, 2015 at 6:39 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> >> >>> On Fri, Dec 4, 2015 at 6:31 PM, Martyn Welch
> >> >>> <martyn.welch@collabora.co.uk> wrote:
> >>
> >> [...]
> >>
> >> >>> Markus Pargmann also did a series that add initial values to
> >> >>> hogs, which is the inverse usecase of this, where you want to
> >> >>> *output* something by default, then maybe also make it available
> >> >>> to userspace.
> >> >>>
> >> >>> So what we need to see here is a patch series that does all of these
> >> >>> things:
> >> >>>
> >> >>> - Name lines
> >> >>>
> >> >>> - Sets them to initial values
> >> >>>
> >> >>> - Mark them as read-only
> >> >>>
> >> >>> - Mark them as "not used by the operating system" so that they
> >> >>>   can be default-exported to userspace.
> >> >>
> >> >> No! This should not be a DT property.
> >> >>
> >> >> Whether I want to control a GPIO in the kernel or userspace is not
> >> >> known and can change over time. It could simply depend on kernel
> >> >> config. There is also the case that a GPIO has no connection or kernel
> >> >> driver until some time later when a DT overlay for an expansion board
> >> >> is applied.
> >> >
> >> > That's correct. So from a DT point of view, what really matters is
> >> > to give things a name, and the hogs and initvals syntax already
> >> > has a structure for this that is in use
> >> > (from Documentation/devicetree/bindings/gpio/gpio.txt):
> >> >
> >> >         qe_pio_a: gpio-controller@1400 {
> >> >                 compatible = "fsl,qe-pario-bank-a", "fsl,qe-pario-bank";
> >> >                 reg = <0x1400 0x18>;
> >> >                 gpio-controller;
> >> >                 #gpio-cells = <2>;
> >> >
> >> >                 line_b {
> >> >                         gpio-hog;
> >> >                         gpios = <6 0>;
> >> >                         output-low;
> >> >                         line-name = "foo-bar-gpio";
> >> >                 };
> >> >         };
> >> >
> >> > Markus use this also for initial values. That could easily be used in
> >> > a budget version like this:
> >> >
> >> >                 line_b {
> >> >                         /* Just naming */
> >> >                         gpios = <6 0>;
> >> >                         line-name = "foo-bar-gpio";
> >> >                 };
> >> >
> >> > That could grow kind of big though. Or maybe not? How many
> >> > GPIO lines are actually properly named in a typical system?
> >>
> >> We should limit it to GPIOs with no connection to another node. For
> >> example, I don't want to see a SD card detect in the list as that
> >> should be in the SD host node. However, that is hard to enforce and
> >> can change over time. Removing it would break userspace potentially.
> >> Of course if the kernel starts own a signal that userspace used, then
> >> that potentially breaks userspace regardless of the DT changing. OTOH,
> >> using GPIOs in userspace is kind of use at your own risk.
> >
> > I see this a bit differently. I would really like to see the each GPIO having
> > two different names:
> 
> I think we are saying the same thing...
> 
> > - GPIO label: This is the name of the GPIO line in the schematic for example
> 
> Yes.
> 
> > - GPIO use (this is the current semantic of the GPIO name): The use of a GPIO,
> >   e.g. 'sd-card-detect', 'LED', ...
> 
> This should be determined from the compatible string and/or -gpios
> prefix. This is the what the function is and "label" is which one.
> 
> > I think it would be good to describe all GPIO labels in gpiochip subnodes as
> > gpio-hogging introduced it. This would offer a use-independent naming. The use
> > of the function could be defined in the device node that is using this gpio.
> 
> I think I agree here. You may have a defined function without any
> connection to another node. I also think we should encourage simple
> GPIO bindings like gpio-leds to be child nodes. Having them at the
> top-level is kind of arbitrary. Of course, allowing for both is
> required.
> 
> > As an example perhaps something like this:
> >
> >         &gpiochip {
> >                 some_interrupt {
> >                         gpios = <4 0>;
> >                         line-name = "some_interrupt_line";
> >                 };
> >
> >                 line_b {
> >                         gpios = <6 0>;
> >                         line-name = "line-b";
> >                 };
> >         };
> >
> >         randomswitch {
> >                 compatible = "gpio-switch";
> >                 gpios = <&gpiochip 4 0>;
> >                 use = "action-trigger";
> >                 read-only;
> >         };
> >
> > Also this does seem kind of inconsistent with gpio-hogging and the proposed
> > gpio-initval. gpio-hogging is defined in subnodes of the gpio chip while
> > gpio-switches are not. As "gpio-switch" is not really any kind of device it
> > would perhaps make sense to keep this consistent with gpio-hogging as well and
> > define it in the same subnodes?
> > I would be happy about any consistent way.
> 
> Yes, as well as gpio leds, keys, etc. bindings. The key is you would
> need to be able to start with something minimal and extend it with
> specific compatibles.

So if I understand you right it would be better to handle most
distinctions using compatibles? Something like this?

	&gpiochip {
		some_led {
			compatible = "gpio-leds";
			default-state = "on";
			gpios = <3 0>;
			line-name = "leda";
		};

		some_switch {
			compatible = "gpio-switch", "gpio-initval";
			gpios = <4 0>;
			line-name = "switch1";

			/* This is used by gpio-initval in case gpio-switch is not implemented */
			output-low;
		};

		some_interrupt {
			gpios = <5 0>;
			line-name = "some_interrupt_line";
		};

		line_b {
			gpios = <6 0>;
			line-name = "line-b";
		};
	};

	randomcomponent {
		gpios = <&gpiochip 5 0>;
		read-only;
	};

In this case there is a gpio-led embedded within the gpiochip node. Also
'some_switch' is a gpio-switch and falls back to being gpio-initval.
This way we can replace it at some point with a real driver. But if it
doesn't exist we can at least initialize the GPIO properly.

As you suggested this would open up the possibilities for all
compatibles and drivers for single gpios.

Best Regards,

Markus

> 
> [...]
> 
> >> We also have to consider how to handle add-on boards. We probably need
> >> a connector node which can remap connector signals to host signals in
> >> order to have overlays independent of the host board DT. For GPIOs
> >> this is probably a gpio-map property similar to interrupt-map. The
> >> complicated part will be connectors that require pinmux setup of the
> >> host.
> >
> > Also what about hotplugging gpio-chips? Is there any mechanism to let the
> > 'gpio-switch' know that the GPIO is not there anymore?
> 
> There are certainly issues around hotplug and GPIOs. If the
> gpio-switch device is a child of the gpio controller, then it should
> be possible.
> 
> Rob
>

Patch
diff mbox

diff --git a/Documentation/devicetree/bindings/misc/gpio-switch.txt b/Documentation/devicetree/bindings/misc/gpio-switch.txt
new file mode 100644
index 0000000..13528bd
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/gpio-switch.txt
@@ -0,0 +1,47 @@ 
+Device-Tree bindings for gpio attached switches.
+
+This provides a mechanism to provide a named link to specified gpios. This can
+be useful in instances such as when theres a need to monitor a switch, which is
+common across a family of devices, but attached to different gpios and even
+implemented in different ways on differnet devices.
+
+Required properties:
+	- compatible = "gpio-switch";
+
+Each signal is represented as a sub-node of "gpio-switch". The naming of
+sub-nodes is arbitrary.
+
+Required sub-node properties:
+
+	- label: Name to be given to gpio switch.
+	- gpios: OF device-tree gpio specification.
+
+Optional sub-node properties:
+
+	- read-only: Boolean flag to mark the gpio as read-only, i.e. the line
+	  should not be driven by the host.
+
+Example nodes:
+
+        gpio-switch {
+                compatible = "gpio-switch";
+
+                write-protect {
+                        label = "write-protect";
+                        gpios = <&gpx3 0 GPIO_ACTIVE_LOW>;
+                        read-only;
+                };
+
+                developer-switch {
+                        label = "developer-switch";
+                        gpios = <&gpx1 3 GPIO_ACTIVE_HIGH>;
+                        read-only;
+                };
+
+                recovery-switch {
+                        label = "recovery-switch";
+                        gpios = <&gpx0 7 GPIO_ACTIVE_LOW>;
+                        read-only;
+                };
+        };
+