| Message ID | 1350922139-3693-6-git-send-email-haojian.zhuang@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/22/2012 10:08 AM, Haojian Zhuang wrote: > Add comments with pinconf & gpio range in the document of > pinctrl-single. > > Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com> > --- > .../devicetree/bindings/pinctrl/pinctrl-single.txt | 52 ++++++++++++++++++++ > arch/arm/boot/dts/pxa910.dtsi | 1 - > 2 files changed, 52 insertions(+), 1 deletions(-) > > diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt > index 2c81e45..6da2f13 100644 > --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt > +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt > @@ -17,6 +17,36 @@ Optional properties: > - pinctrl-single,bit-per-mux : boolean to indicate that one register controls > more than one pin > > +- pinctrl-single,gpio-ranges : gpio range list > + > +- pinctrl-single,gpio : array with gpio range start, size & register > + offset > + > +- pinctrl-single,gpio-func : gpio function value in the pinmux register Some more explanation is needed here; some questions/comments: 1) Looking at the example, pinctrl-single,gpio-ranges is a property within the main pinctrl node, whereas pinctrl-single,gpio and pinctrl-single,gpio-func are properties within some other node. There's no explanation of this in the binding description itself, only in the example. Related to this, the documentation for pinctrl-single,gpio-ranges doesn't say what it's a list of; it needs to say that it's a list of phandles. 2) pinctrl-single,gpio is listed as optional. Presumably it's not; every GPIO range node must have this property? 3) Why is pinctrl-single,gpio-func optional? Presumably you always need to program the pinmux HW to select the GPIO function. Yet, the driver code in an earlier patch seems to deliberately do nothing if this property is missing. Shouldn't the DT parsing return an error instead? 4) I'm a little confused re: the data model. Is the idea that if pinctrl-single,gpio-ranges is specified, then the node describes a combined pin controller and GPIO HW? Are the pin IDs of the pin controller expected to match the pin IDs of the GPIO HW? I'm left wondering exactly which numbering space the values in pinctrl-single,gpio are; do they describe the pin controller IDs that this GPIO range describes, or do they describe the GPIO IDs that this range describes and attempt to map them back to pin controller IDs? Similarly, I'm not sure why there's a register offset here rather than say a pin controller pin ID number. Shouldn't the property be a list of <pin-controller-pin-ID GPIO-controller-GPIO-ID number-of-GPIOs> > +- pinctrl-single,power-source-mask : mask of setting power source in > + the pinmux register > + > +- pinctrl-single,power-source : value of setting power source field > + in the pinmux register > + > +- pinctrl-single,bias-mask : mask of setting bias value in the pinmux > + register > + > +- pinctrl-single,bias-disable : value of disabling bias in the pinmux > + register > + > +- pinctrl-single,bias-pull-down : value of setting bias pull down in > + the pinmux register > + > +- pinctrl-single,bias-pull-up : value of setting bias pull up in the > + pinmux register > + > +- pinctrl-single,bias : value of setting bias in the pinmux register > + > +- pinctrl-single,input-schmitt-mask : mask of setting input schmitt > + in the pinmux register I suppose it's OK that a generic pin controller binding would use the generic pin configuration config options. I'm still not convinced that the semantics of generic pin control make sense. Maybe if they're just arbitrary names for SoC-specific things it's fine though. Do these patches expose /all/ generic pin configuration options? It doesn't seem worth exposing only some of them and ignoring others. > +/* third controller instance for pins in gpio domain */ > +pmx_gpio: pinmux@d401e000 { > + compatible = "pinctrl-single"; > + reg = <0xd401e000 0x0330>; > + #address-cells = <1>; > + #size-cells = <0>; #gpio-cells would be needed here for a GPIO controller. > diff --git a/arch/arm/boot/dts/pxa910.dtsi b/arch/arm/boot/dts/pxa910.dtsi > - pinctrl-single,gpio-mask = <7>; I assume that's a mistake; the line shouldn't be removed in this documentation patch?
On Tue, Oct 23, 2012 at 6:44 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 10/22/2012 10:08 AM, Haojian Zhuang wrote: >> Add comments with pinconf & gpio range in the document of >> pinctrl-single. >> >> Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com> >> --- >> .../devicetree/bindings/pinctrl/pinctrl-single.txt | 52 ++++++++++++++++++++ >> arch/arm/boot/dts/pxa910.dtsi | 1 - >> 2 files changed, 52 insertions(+), 1 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt >> index 2c81e45..6da2f13 100644 >> --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt >> +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt >> @@ -17,6 +17,36 @@ Optional properties: >> - pinctrl-single,bit-per-mux : boolean to indicate that one register controls >> more than one pin >> >> +- pinctrl-single,gpio-ranges : gpio range list >> + >> +- pinctrl-single,gpio : array with gpio range start, size & register >> + offset >> + >> +- pinctrl-single,gpio-func : gpio function value in the pinmux register > > Some more explanation is needed here; some questions/comments: > > 1) Looking at the example, pinctrl-single,gpio-ranges is a property > within the main pinctrl node, whereas pinctrl-single,gpio and > pinctrl-single,gpio-func are properties within some other node. There's > no explanation of this in the binding description itself, only in the > example. Related to this, the documentation for > pinctrl-single,gpio-ranges doesn't say what it's a list of; it needs to > say that it's a list of phandles. > I'll add more comments. > 2) pinctrl-single,gpio is listed as optional. Presumably it's not; every > GPIO range node must have this property? > Yes, they must be included in GPIO range node. But if GPIO feature isn't supported in the pinctrl device, pinctrl-single,gpio is still optional. I'll add more comments on this. > 3) Why is pinctrl-single,gpio-func optional? Presumably you always need > to program the pinmux HW to select the GPIO function. Yet, the driver > code in an earlier patch seems to deliberately do nothing if this > property is missing. Shouldn't the DT parsing return an error instead? > pinctrl-single,gpio-func is optional for above reason. > 4) I'm a little confused re: the data model. Is the idea that if > pinctrl-single,gpio-ranges is specified, then the node describes a > combined pin controller and GPIO HW? Are the pin IDs of the pin > controller expected to match the pin IDs of the GPIO HW? GPIO function is enabled by setting proper field in mux field of pinmux register. > I'm left wondering exactly which numbering space the values in > pinctrl-single,gpio are; do they describe the pin controller IDs that > this GPIO range describes, or do they describe the GPIO IDs that this > range describes and attempt to map them back to pin controller IDs? > Similarly, I'm not sure why there's a register offset here rather than > say a pin controller pin ID number. Shouldn't the property be a list of > <pin-controller-pin-ID GPIO-controller-GPIO-ID number-of-GPIOs> > Whatever it's pin ID or offset of pinmux register, they're nearly same. We can calculate offset from pin ID, vice visa. If pin ID is more acceptable by you and Tony, I can change it later. >> +- pinctrl-single,power-source-mask : mask of setting power source in >> + the pinmux register >> + >> +- pinctrl-single,power-source : value of setting power source field >> + in the pinmux register >> + >> +- pinctrl-single,bias-mask : mask of setting bias value in the pinmux >> + register >> + >> +- pinctrl-single,bias-disable : value of disabling bias in the pinmux >> + register >> + >> +- pinctrl-single,bias-pull-down : value of setting bias pull down in >> + the pinmux register >> + >> +- pinctrl-single,bias-pull-up : value of setting bias pull up in the >> + pinmux register >> + >> +- pinctrl-single,bias : value of setting bias in the pinmux register >> + >> +- pinctrl-single,input-schmitt-mask : mask of setting input schmitt >> + in the pinmux register > > I suppose it's OK that a generic pin controller binding would use the > generic pin configuration config options. I'm still not convinced that > the semantics of generic pin control make sense. Maybe if they're just > arbitrary names for SoC-specific things it's fine though. > > Do these patches expose /all/ generic pin configuration options? It > doesn't seem worth exposing only some of them and ignoring others. > I believe general pinconf can't support all cases in different silicons. And we still have some common features that could be covered in general pinconf. So we need a structure to support both pinconf & specific pinconf. >> +/* third controller instance for pins in gpio domain */ >> +pmx_gpio: pinmux@d401e000 { >> + compatible = "pinctrl-single"; >> + reg = <0xd401e000 0x0330>; >> + #address-cells = <1>; >> + #size-cells = <0>; > > #gpio-cells would be needed here for a GPIO controller. > >> diff --git a/arch/arm/boot/dts/pxa910.dtsi b/arch/arm/boot/dts/pxa910.dtsi > >> - pinctrl-single,gpio-mask = <7>; > > I assume that's a mistake; the line shouldn't be removed in this > documentation patch? > Yes, this part should be included in the previous patch. I'll fix it.
On 10/31/2012 10:58 AM, Haojian Zhuang wrote: > On Tue, Oct 23, 2012 at 6:44 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 10/22/2012 10:08 AM, Haojian Zhuang wrote: >>> Add comments with pinconf & gpio range in the document of >>> pinctrl-single. ... >>> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt ... >>> @@ -17,6 +17,36 @@ Optional properties: >>> - pinctrl-single,bit-per-mux : boolean to indicate that one register controls >>> more than one pin >>> >>> +- pinctrl-single,gpio-ranges : gpio range list >>> + >>> +- pinctrl-single,gpio : array with gpio range start, size & register >>> + offset >>> + >>> +- pinctrl-single,gpio-func : gpio function value in the pinmux register ... >> 2) pinctrl-single,gpio is listed as optional. Presumably it's not; every >> GPIO range node must have this property? >> > Yes, they must be included in GPIO range node. But if GPIO feature > isn't supported in the pinctrl device, pinctrl-single,gpio is still optional. > I'll add more comments on this. > >> 3) Why is pinctrl-single,gpio-func optional? Presumably you always need >> to program the pinmux HW to select the GPIO function. Yet, the driver >> code in an earlier patch seems to deliberately do nothing if this >> property is missing. Shouldn't the DT parsing return an error instead? >> > pinctrl-single,gpio-func is optional for above reason. Presumably the node that contains the pinctrl-single,gpio-func property is optional, but once you have such a node, pinctrl-single,gpio-func is required? >>> +- pinctrl-single,power-source-mask : mask of setting power source in >>> + the pinmux register >>> + >>> +- pinctrl-single,power-source : value of setting power source field >>> + in the pinmux register ... >> I suppose it's OK that a generic pin controller binding would use the >> generic pin configuration config options. I'm still not convinced that >> the semantics of generic pin control make sense. Maybe if they're just >> arbitrary names for SoC-specific things it's fine though. >> >> Do these patches expose /all/ generic pin configuration options? It >> doesn't seem worth exposing only some of them and ignoring others. > > I believe general pinconf can't support all cases in different silicons. I tend to agree. > And we still have some common features that could be covered in general > pinconf. So we need a structure to support both pinconf & specific pinconf. But that tends to imply that adding support for generic pinconf into the pinctrl-simple driver isnt' actually going to be useful for anyone. If pinctrl-single only supports some part of your HW, how can you use it? Or, do you intend to somehow make pinctrl-single support both the common generic pinconf stuff, and somehow be extensible to support any SoC-specific pin config fields?
On Wed, Oct 31, 2012 at 11:26 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 10/31/2012 10:58 AM, Haojian Zhuang wrote: >> On Tue, Oct 23, 2012 at 6:44 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >>> 3) Why is pinctrl-single,gpio-func optional? Presumably you always need >>> to program the pinmux HW to select the GPIO function. Yet, the driver >>> code in an earlier patch seems to deliberately do nothing if this >>> property is missing. Shouldn't the DT parsing return an error instead? >>> >> pinctrl-single,gpio-func is optional for above reason. > > Presumably the node that contains the pinctrl-single,gpio-func property > is optional, but once you have such a node, pinctrl-single,gpio-func is > required? > Yes, gpio-func is must required if the phandle of gpio range is defined. I'll add such comments in document. >>> I suppose it's OK that a generic pin controller binding would use the >>> generic pin configuration config options. I'm still not convinced that >>> the semantics of generic pin control make sense. Maybe if they're just >>> arbitrary names for SoC-specific things it's fine though. >>> >>> Do these patches expose /all/ generic pin configuration options? It >>> doesn't seem worth exposing only some of them and ignoring others. >> >> I believe general pinconf can't support all cases in different silicons. > > I tend to agree. > >> And we still have some common features that could be covered in general >> pinconf. So we need a structure to support both pinconf & specific pinconf. > > But that tends to imply that adding support for generic pinconf into the > pinctrl-simple driver isnt' actually going to be useful for anyone. If > pinctrl-single only supports some part of your HW, how can you use it? > Or, do you intend to somehow make pinctrl-single support both the common > generic pinconf stuff, and somehow be extensible to support any > SoC-specific pin config fields? I'm intend to make pinctrl-single to support both the common generic pinconf stuff and any SoC-specific pinconf. I'll try to handle it. But they won't be included in V3.
* Haojian Zhuang <haojian.zhuang@gmail.com> [121031 15:53]: > On Wed, Oct 31, 2012 at 11:26 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > > > > But that tends to imply that adding support for generic pinconf into the > > pinctrl-simple driver isnt' actually going to be useful for anyone. If > > pinctrl-single only supports some part of your HW, how can you use it? > > Or, do you intend to somehow make pinctrl-single support both the common > > generic pinconf stuff, and somehow be extensible to support any > > SoC-specific pin config fields? > > I'm intend to make pinctrl-single to support both the common generic pinconf > stuff and any SoC-specific pinconf. I'll try to handle it. But they > won't be included > in V3. The easiest way to support generic pinconf in pinctrl-single.c is to set up a separate driver instance for the register ranges that need it. Otherwise things can get messy. Regards, Tony
diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt index 2c81e45..6da2f13 100644 --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt @@ -17,6 +17,36 @@ Optional properties: - pinctrl-single,bit-per-mux : boolean to indicate that one register controls more than one pin +- pinctrl-single,gpio-ranges : gpio range list + +- pinctrl-single,gpio : array with gpio range start, size & register + offset + +- pinctrl-single,gpio-func : gpio function value in the pinmux register + +- pinctrl-single,power-source-mask : mask of setting power source in + the pinmux register + +- pinctrl-single,power-source : value of setting power source field + in the pinmux register + +- pinctrl-single,bias-mask : mask of setting bias value in the pinmux + register + +- pinctrl-single,bias-disable : value of disabling bias in the pinmux + register + +- pinctrl-single,bias-pull-down : value of setting bias pull down in + the pinmux register + +- pinctrl-single,bias-pull-up : value of setting bias pull up in the + pinmux register + +- pinctrl-single,bias : value of setting bias in the pinmux register + +- pinctrl-single,input-schmitt-mask : mask of setting input schmitt + in the pinmux register + This driver assumes that there is only one register for each pin (unless the pinctrl-single,bit-per-mux is set), and uses the common pinctrl bindings as specified in the pinctrl-bindings.txt document in this directory. @@ -76,6 +106,28 @@ control_devconf0: pinmux@48002274 { pinctrl-single,function-mask = <0x5F>; }; +/* third controller instance for pins in gpio domain */ +pmx_gpio: pinmux@d401e000 { + compatible = "pinctrl-single"; + reg = <0xd401e000 0x0330>; + #address-cells = <1>; + #size-cells = <0>; + pinctrl-single,register-width = <32>; + pinctrl-single,function-mask = <7>; + pinctrl-single,gpio-ranges = <&gpiorange0 &gpiorange1>; +}; + +gpiorange0: gpiorange@d401e0dc { + pinctrl-single,gpio = <0 55 0x0dc>; + pinctrl-single,gpio-func = <0>; +}; + +gpiorange1: gpiorange@d401e2f0 { + pinctrl-single,gpio = <55 5 0x2f0>; + pinctrl-single,gpio-func = <1>; +}; + + /* board specific .dts file */ &pmx_core { diff --git a/arch/arm/boot/dts/pxa910.dtsi b/arch/arm/boot/dts/pxa910.dtsi index cf807e8..a11a582 100644 --- a/arch/arm/boot/dts/pxa910.dtsi +++ b/arch/arm/boot/dts/pxa910.dtsi @@ -61,7 +61,6 @@ #size-cells = <0>; pinctrl-single,register-width = <32>; pinctrl-single,function-mask = <7>; - pinctrl-single,gpio-mask = <7>; pinctrl-single,gpio-ranges = <&gpiorange0 &gpiorange1 &gpiorange2 &gpiorange3 &gpiorange4 &gpiorange5
Add comments with pinconf & gpio range in the document of pinctrl-single. Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com> --- .../devicetree/bindings/pinctrl/pinctrl-single.txt | 52 ++++++++++++++++++++ arch/arm/boot/dts/pxa910.dtsi | 1 - 2 files changed, 52 insertions(+), 1 deletions(-)