| Message ID | 1403823587-23404-3-git-send-email-fkan@apm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 26, 2014 at 11:59:46PM +0100, Feng Kan wrote: > Documentation for APM X-Gene SoC GPIO controller DTS binding. > > Signed-off-by: Feng Kan <fkan@apm.com> > Reviewed-by: Alexandre Courbot <acourbot@nvidia.com> > --- > .../devicetree/bindings/gpio/gpio-xgene.txt | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > create mode 100644 Documentation/devicetree/bindings/gpio/gpio-xgene.txt > > diff --git a/Documentation/devicetree/bindings/gpio/gpio-xgene.txt b/Documentation/devicetree/bindings/gpio/gpio-xgene.txt > new file mode 100644 > index 0000000..bd5fd85 > --- /dev/null > +++ b/Documentation/devicetree/bindings/gpio/gpio-xgene.txt > @@ -0,0 +1,20 @@ > +APM X-Gene SoC GPIO controller bindings > + > +This is a gpio controller that is part of the flash controller. > +This gpio controller controls a total of 48 gpios. > + > +Required properties: > +- compatible: "apm,xgene-gpio" for X-Gene GPIO controller > +- reg: Physical base address and size of the controller's registers There is just the one bank? > +- #gpio-cells: Should be two. > + - first cell is the pin number > + - second cell is used to specify optional parameters (unused) Why is there an unused cell? Why not just make this a single cell if the binding defines no valid parameters? > +- gpio-controller: Marks the device node as a GPIO controller. No interrupts? Thanks, Mark. > + > +Example: > + gpio0: gpio0@1701c000 { > + compatible = "apm,xgene-gpio"; > + reg = <0x0 0x1701c000 0x0 0x40>; > + gpio-controller; > + #gpio-cells = <2>; > + }; > -- > 1.9.1 > > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Mon, Jun 30, 2014 at 2:53 PM, Mark Rutland <mark.rutland@arm.com> wrote: > On Thu, Jun 26, 2014 at 11:59:46PM +0100, Feng Kan wrote: >> +- #gpio-cells: Should be two. >> + - first cell is the pin number >> + - second cell is used to specify optional parameters (unused) > > Why is there an unused cell? > > Why not just make this a single cell if the binding defines no valid > parameters? I don't get this either. The only reason would be that this cell should contain flags (such as open collector) the code for using it being inplemented later. If the controller is too primitive to use such flags it should be onecell. Yours, Linus Walleij
On Fri, Jul 4, 2014 at 2:28 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Mon, Jun 30, 2014 at 2:53 PM, Mark Rutland <mark.rutland@arm.com> wrote: >> On Thu, Jun 26, 2014 at 11:59:46PM +0100, Feng Kan wrote: > >>> +- #gpio-cells: Should be two. >>> + - first cell is the pin number >>> + - second cell is used to specify optional parameters (unused) >> >> Why is there an unused cell? >> >> Why not just make this a single cell if the binding defines no valid >> parameters? > > I don't get this either. The only reason would be that this cell > should contain flags (such as open collector) the code for using > it being inplemented later. Yes, open drain configuration and mux via pinctrl was something I planned for later. There was another reason for this as well, part of the gpio code I read was confusing me. So I look through the other gpio documentations and found an example that did this as well. int of_gpio_simple_xlate(struct gpio_chip *gc, const struct of_phandle_args *gpiospec, u32 *flags) { /* * We're discouraging gpio_cells < 2, since that way you'll have to * write your own xlate function (that will have to retrive the GPIO * number and the flags from a single gpio cell -- this is possible, * but not recommended). */ Thanks, and please advise me on what to do next. > > If the controller is too primitive to use such flags it should be onecell. > > Yours, > Linus Walleij
On Mon, Jul 7, 2014 at 8:52 PM, Feng Kan <fkan@apm.com> wrote: > On Fri, Jul 4, 2014 at 2:28 PM, Linus Walleij <linus.walleij@linaro.org> wrote: >> I don't get this either. The only reason would be that this cell >> should contain flags (such as open collector) the code for using >> it being inplemented later. > > Yes, open drain configuration and mux via pinctrl was something I > planned for later. OK but pinctrl is something else. There is also the GPIO OD configuration, as it modifies the behaviour of how to set GPIO lines. (Yes I know the frameworks maybe ought to talk to each other for such things....) So if you plan to do this, add it to the bindings even if you're not adding the code for handling it right now. > There was another reason for this as well, part of the gpio code I read > was confusing me. So I look through the other gpio documentations and > found an example that did this as well. > > int of_gpio_simple_xlate(struct gpio_chip *gc, > const struct of_phandle_args *gpiospec, u32 *flags) > { > /* > * We're discouraging gpio_cells < 2, since that way you'll have to > * write your own xlate function (that will have to retrive the GPIO > * number and the flags from a single gpio cell -- this is possible, > * but not recommended). > */ Hm yeah that's right. This check was added by Anton in 2008. Anton why did we discourage onecell GPIOs? Yours, Linus Walleij
On Mon, Jul 07, 2014 at 11:26:18PM +0200, Linus Walleij wrote: ... > > There was another reason for this as well, part of the gpio code I read > > was confusing me. So I look through the other gpio documentations and > > found an example that did this as well. > > > > int of_gpio_simple_xlate(struct gpio_chip *gc, > > const struct of_phandle_args *gpiospec, u32 *flags) > > { > > /* > > * We're discouraging gpio_cells < 2, since that way you'll have to > > * write your own xlate function (that will have to retrive the GPIO > > * number and the flags from a single gpio cell -- this is possible, > > * but not recommended). > > */ > > Hm yeah that's right. > > This check was added by Anton in 2008. Anton why did we discourage > onecell GPIOs? Yup, the check was there from the very beginning. Think of OF_GPIO_ACTIVE_LOW flag (it is widely used in drivers.) The documentation in Feng's driver says "second cell is used to specify optional parameters (unused)," which is not entirely correct. With the standard xlate call it is used for active-low flag. You can implement active-low flag w/o using the second cell, but it will be ugly. Thanks, Anton
On Mon, Jun 30, 2014 at 5:53 AM, Mark Rutland <mark.rutland@arm.com> wrote: > On Thu, Jun 26, 2014 at 11:59:46PM +0100, Feng Kan wrote: >> Documentation for APM X-Gene SoC GPIO controller DTS binding. >> >> Signed-off-by: Feng Kan <fkan@apm.com> >> Reviewed-by: Alexandre Courbot <acourbot@nvidia.com> >> --- >> .../devicetree/bindings/gpio/gpio-xgene.txt | 20 ++++++++++++++++++++ >> 1 file changed, 20 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/gpio/gpio-xgene.txt >> >> diff --git a/Documentation/devicetree/bindings/gpio/gpio-xgene.txt b/Documentation/devicetree/bindings/gpio/gpio-xgene.txt >> new file mode 100644 >> index 0000000..bd5fd85 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/gpio/gpio-xgene.txt >> @@ -0,0 +1,20 @@ >> +APM X-Gene SoC GPIO controller bindings >> + >> +This is a gpio controller that is part of the flash controller. >> +This gpio controller controls a total of 48 gpios. >> + >> +Required properties: >> +- compatible: "apm,xgene-gpio" for X-Gene GPIO controller >> +- reg: Physical base address and size of the controller's registers > > There is just the one bank? Internally there are three banks. Due to the fact the address space is meshed, It was agreed that we turn this into one simple node. > >> +- #gpio-cells: Should be two. >> + - first cell is the pin number >> + - second cell is used to specify optional parameters (unused) > > Why is there an unused cell? > > Why not just make this a single cell if the binding defines no valid > parameters? I will update documentation to indicate second cell is used for active low and active high setting since that is the behavior of the default of_xlate function. All it is seems to be doing is flipping the value of the gpio. I initially did this because I did not want to use additional attributes and followed the example documentation of number of other gpio driver. > >> +- gpio-controller: Marks the device node as a GPIO controller. > > No interrupts? Yes, no interrupts in this block. > > Thanks, > Mark. > >> + >> +Example: >> + gpio0: gpio0@1701c000 { >> + compatible = "apm,xgene-gpio"; >> + reg = <0x0 0x1701c000 0x0 0x40>; >> + gpio-controller; >> + #gpio-cells = <2>; >> + }; >> -- >> 1.9.1 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe devicetree" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >>
diff --git a/Documentation/devicetree/bindings/gpio/gpio-xgene.txt b/Documentation/devicetree/bindings/gpio/gpio-xgene.txt new file mode 100644 index 0000000..bd5fd85 --- /dev/null +++ b/Documentation/devicetree/bindings/gpio/gpio-xgene.txt @@ -0,0 +1,20 @@ +APM X-Gene SoC GPIO controller bindings + +This is a gpio controller that is part of the flash controller. +This gpio controller controls a total of 48 gpios. + +Required properties: +- compatible: "apm,xgene-gpio" for X-Gene GPIO controller +- reg: Physical base address and size of the controller's registers +- #gpio-cells: Should be two. + - first cell is the pin number + - second cell is used to specify optional parameters (unused) +- gpio-controller: Marks the device node as a GPIO controller. + +Example: + gpio0: gpio0@1701c000 { + compatible = "apm,xgene-gpio"; + reg = <0x0 0x1701c000 0x0 0x40>; + gpio-controller; + #gpio-cells = <2>; + };