| Message ID | 1362587021-32762-3-git-send-email-thomas.petazzoni@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 06, 2013 at 05:23:40PM +0100, Thomas Petazzoni wrote: > The OpenBlocks A6 .dts file was using a long list of pinmux > descriptors to select each GPIO of the external GPIO connector and the > internal DIP switch, for no apparent reason. This commit factors those > GPIO pins into two descriptors: one for the external GPIO connector > and one for the internal DIP switch. Hi Thomas There is no need to pinmux gpio pins at all. The pinctrl driver does it when the gpio driver requests the pins. This stems from an error i made. I also didn't know this and added hogs for gpio pins as i converted some boards. Others have then just cut/paste my error..... Andrew > > As an added bonus, this commit also adds comments that says what those > GPIOs are used for. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > --- > arch/arm/boot/dts/kirkwood-openblocks_a6.dts | 74 +++++--------------------- > 1 file changed, 14 insertions(+), 60 deletions(-) > > diff --git a/arch/arm/boot/dts/kirkwood-openblocks_a6.dts b/arch/arm/boot/dts/kirkwood-openblocks_a6.dts > index 087681d..0488d6a 100644 > --- a/arch/arm/boot/dts/kirkwood-openblocks_a6.dts > +++ b/arch/arm/boot/dts/kirkwood-openblocks_a6.dts > @@ -85,12 +85,7 @@ > }; > > pinctrl: pinctrl@10000 { > - pinctrl-0 = <&pmx_dip_sw0 &pmx_dip_sw1 > - &pmx_dip_sw2 &pmx_dip_sw3 > - &pmx_gpio_0 &pmx_gpio_1 > - &pmx_gpio_2 &pmx_gpio_3 > - &pmx_gpio_4 &pmx_gpio_5 > - &pmx_gpio_6 &pmx_gpio_7>; > + pinctrl-0 = <&pmx_dip_sw &pmx_ext_gpios>; > pinctrl-names = "default"; > > pmx_uart0: pmx-uart0 { > @@ -110,63 +105,22 @@ > marvell,function = "sysrst"; > }; > > - pmx_dip_sw0: pmx-dip-sw0 { > - marvell,pins = "mpp20"; > + /* > + * Four input GPIOs connected to the 4-way > + * DIP-switch inside the device. > + */ > + pmx_dip_sw: pmx-dip-sw { > + marvell,pins = "mpp20", "mpp21", "mpp22", "mpp23"; > marvell,function = "gpio"; > }; > > - pmx_dip_sw1: pmx-dip-sw1 { > - marvell,pins = "mpp21"; > - marvell,function = "gpio"; > - }; > - > - pmx_dip_sw2: pmx-dip-sw2 { > - marvell,pins = "mpp22"; > - marvell,function = "gpio"; > - }; > - > - pmx_dip_sw3: pmx-dip-sw3 { > - marvell,pins = "mpp23"; > - marvell,function = "gpio"; > - }; > - > - pmx_gpio_0: pmx-gpio-0 { > - marvell,pins = "mpp24"; > - marvell,function = "gpio"; > - }; > - > - pmx_gpio_1: pmx-gpio-1 { > - marvell,pins = "mpp25"; > - marvell,function = "gpio"; > - }; > - > - pmx_gpio_2: pmx-gpio-2 { > - marvell,pins = "mpp26"; > - marvell,function = "gpio"; > - }; > - > - pmx_gpio_3: pmx-gpio-3 { > - marvell,pins = "mpp27"; > - marvell,function = "gpio"; > - }; > - > - pmx_gpio_4: pmx-gpio-4 { > - marvell,pins = "mpp28"; > - marvell,function = "gpio"; > - }; > - > - pmx_gpio_5: pmx-gpio-5 { > - marvell,pins = "mpp29"; > - marvell,function = "gpio"; > - }; > - > - pmx_gpio_6: pmx-gpio-6 { > - marvell,pins = "mpp30"; > - marvell,function = "gpio"; > - }; > - > - pmx_gpio_7: pmx-gpio-7 { > - marvell,pins = "mpp31"; > + /* > + * 8 GPIOs available through the Hirose 16 > + * pins header at the back of the device. > + */ > + pmx_ext_gpios: pmx-ext-gpios { > + marvell,pins = "mpp24", "mpp25", "mpp26", "mpp27", > + "mpp28", "mpp29", "mpp30", "mpp31"; > marvell,function = "gpio"; > }; > > -- > 1.7.9.5 >
Dear Andrew Lunn, On Wed, 6 Mar 2013 19:16:48 +0100, Andrew Lunn wrote: > There is no need to pinmux gpio pins at all. The pinctrl driver does > it when the gpio driver requests the pins. Right, that's true. I knew about this, but since the pinmux configuration was made explicit for those GPIOs, I thought the original author(s) of this .dts did this intentionally. For example, it somewhat documents the 8 GPIOs that are available in the bottom connector of the OpenBlocks A6. They are not tied to anyway particular device, so they wouldn't appear anywhere in the DT. Ditto for the DIP switch GPIOs. Do you also want those to be removed from the DT? Thanks, Thomas
On Wed, Mar 06, 2013 at 08:16:46PM +0100, Thomas Petazzoni wrote: > Dear Andrew Lunn, > > On Wed, 6 Mar 2013 19:16:48 +0100, Andrew Lunn wrote: > > > There is no need to pinmux gpio pins at all. The pinctrl driver does > > it when the gpio driver requests the pins. > > Right, that's true. I knew about this, but since the pinmux > configuration was made explicit for those GPIOs, I thought the original > author(s) of this .dts did this intentionally. For example, it somewhat > documents the 8 GPIOs that are available in the bottom connector of the > OpenBlocks A6. They are not tied to anyway particular device, so they > wouldn't appear anywhere in the DT. Ditto for the DIP switch GPIOs. Do > you also want those to be removed from the DT? Hi Thomas Good point. They are not required, but don't harm. So yes, leave them there for documentation purposes. Andrew
Dear Andrew Lunn, On Thu, 7 Mar 2013 07:22:55 +0100, Andrew Lunn wrote: > > Right, that's true. I knew about this, but since the pinmux > > configuration was made explicit for those GPIOs, I thought the original > > author(s) of this .dts did this intentionally. For example, it somewhat > > documents the 8 GPIOs that are available in the bottom connector of the > > OpenBlocks A6. They are not tied to anyway particular device, so they > > wouldn't appear anywhere in the DT. Ditto for the DIP switch GPIOs. Do > > you also want those to be removed from the DT? > > Hi Thomas > > Good point. They are not required, but don't harm. So yes, leave them > there for documentation purposes. So I keep the GPIO muxing for the external GPIOs and the DIP switch GPIOs, but the one that are "claimed" by another device (like gpio-leds or gpio-keys), I should remove the muxing? I'm fine with that, just want to confirm the choice. Thanks! Thomas
> So I keep the GPIO muxing for the external GPIOs and the DIP switch > GPIOs, but the one that are "claimed" by another device (like gpio-leds > or gpio-keys), I should remove the muxing? I'm fine with that, just > want to confirm the choice. Hi Thomas, Yes, i'm happy with that. Andrew
Dear Andrew Lunn, On Wed, 6 Mar 2013 19:16:48 +0100, Andrew Lunn wrote: > On Wed, Mar 06, 2013 at 05:23:40PM +0100, Thomas Petazzoni wrote: > > The OpenBlocks A6 .dts file was using a long list of pinmux > > descriptors to select each GPIO of the external GPIO connector and the > > internal DIP switch, for no apparent reason. This commit factors those > > GPIO pins into two descriptors: one for the external GPIO connector > > and one for the internal DIP switch. > > Hi Thomas > > There is no need to pinmux gpio pins at all. The pinctrl driver does > it when the gpio driver requests the pins. > > This stems from an error i made. I also didn't know this and added > hogs for gpio pins as i converted some boards. Others have then just > cut/paste my error..... I was looking at this today to send an updated version of those patches. However, it turns out that having the pinctrl-0 property to associate pinmux configurations to a device has an advantage: pinctrl knows about this association. If you keep the pinctrl-0 property in gpio-leds and gpio-keys, then /sys/kernel/debug/pinctrl/f1010000.pinctrl/pinmux-pins looks like this: ====================================================================== pin 38 (PIN38): gpio_keys.2 mvebu-gpio:38 function gpio group mpp38 pin 39 (PIN39): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 40 (PIN40): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 41 (PIN41): gpio-leds.1 mvebu-gpio:41 function gpio group mpp41 pin 42 (PIN42): gpio-leds.1 mvebu-gpio:42 function gpio group mpp42 pin 43 (PIN43): gpio-leds.1 mvebu-gpio:43 function gpio group mpp43 ====================================================================== On the other hand, if you remove the pinctrl-0 property, the same file looks like this: ====================================================================== pin 38 (PIN38): (MUX UNCLAIMED) mvebu-gpio:38 pin 39 (PIN39): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 40 (PIN40): (MUX UNCLAIMED) (GPIO UNCLAIMED) pin 41 (PIN41): (MUX UNCLAIMED) mvebu-gpio:41 pin 42 (PIN42): (MUX UNCLAIMED) mvebu-gpio:42 pin 43 (PIN43): (MUX UNCLAIMED) mvebu-gpio:43 ====================================================================== So you no longer know by what device the pin is claimed. It is not horrible of course, but I find it quite nice to be able to see which pin is used by what device. What do you think? Thomas
On Fri, Mar 22, 2013 at 04:44:40PM +0100, Thomas Petazzoni wrote: > Dear Andrew Lunn, > > On Wed, 6 Mar 2013 19:16:48 +0100, Andrew Lunn wrote: > > On Wed, Mar 06, 2013 at 05:23:40PM +0100, Thomas Petazzoni wrote: > > > The OpenBlocks A6 .dts file was using a long list of pinmux > > > descriptors to select each GPIO of the external GPIO connector and the > > > internal DIP switch, for no apparent reason. This commit factors those > > > GPIO pins into two descriptors: one for the external GPIO connector > > > and one for the internal DIP switch. > > > > Hi Thomas > > > > There is no need to pinmux gpio pins at all. The pinctrl driver does > > it when the gpio driver requests the pins. > > > > This stems from an error i made. I also didn't know this and added > > hogs for gpio pins as i converted some boards. Others have then just > > cut/paste my error..... > > I was looking at this today to send an updated version of those > patches. However, it turns out that having the pinctrl-0 property to > associate pinmux configurations to a device has an advantage: pinctrl > knows about this association. > > If you keep the pinctrl-0 property in gpio-leds and gpio-keys, > then /sys/kernel/debug/pinctrl/f1010000.pinctrl/pinmux-pins looks like > this: > > ====================================================================== > pin 38 (PIN38): gpio_keys.2 mvebu-gpio:38 function gpio group mpp38 > pin 39 (PIN39): (MUX UNCLAIMED) (GPIO UNCLAIMED) > pin 40 (PIN40): (MUX UNCLAIMED) (GPIO UNCLAIMED) > pin 41 (PIN41): gpio-leds.1 mvebu-gpio:41 function gpio group mpp41 > pin 42 (PIN42): gpio-leds.1 mvebu-gpio:42 function gpio group mpp42 > pin 43 (PIN43): gpio-leds.1 mvebu-gpio:43 function gpio group mpp43 > ====================================================================== > > On the other hand, if you remove the pinctrl-0 property, the same file > looks like this: > > ====================================================================== > pin 38 (PIN38): (MUX UNCLAIMED) mvebu-gpio:38 > pin 39 (PIN39): (MUX UNCLAIMED) (GPIO UNCLAIMED) > pin 40 (PIN40): (MUX UNCLAIMED) (GPIO UNCLAIMED) > pin 41 (PIN41): (MUX UNCLAIMED) mvebu-gpio:41 > pin 42 (PIN42): (MUX UNCLAIMED) mvebu-gpio:42 > pin 43 (PIN43): (MUX UNCLAIMED) mvebu-gpio:43 > ====================================================================== > > So you no longer know by what device the pin is claimed. It is not > horrible of course, but I find it quite nice to be able to see which > pin is used by what device. > > What do you think? Hi Thomas I think the first is more informative. Please use that. Thanks Andrew
diff --git a/arch/arm/boot/dts/kirkwood-openblocks_a6.dts b/arch/arm/boot/dts/kirkwood-openblocks_a6.dts index 087681d..0488d6a 100644 --- a/arch/arm/boot/dts/kirkwood-openblocks_a6.dts +++ b/arch/arm/boot/dts/kirkwood-openblocks_a6.dts @@ -85,12 +85,7 @@ }; pinctrl: pinctrl@10000 { - pinctrl-0 = <&pmx_dip_sw0 &pmx_dip_sw1 - &pmx_dip_sw2 &pmx_dip_sw3 - &pmx_gpio_0 &pmx_gpio_1 - &pmx_gpio_2 &pmx_gpio_3 - &pmx_gpio_4 &pmx_gpio_5 - &pmx_gpio_6 &pmx_gpio_7>; + pinctrl-0 = <&pmx_dip_sw &pmx_ext_gpios>; pinctrl-names = "default"; pmx_uart0: pmx-uart0 { @@ -110,63 +105,22 @@ marvell,function = "sysrst"; }; - pmx_dip_sw0: pmx-dip-sw0 { - marvell,pins = "mpp20"; + /* + * Four input GPIOs connected to the 4-way + * DIP-switch inside the device. + */ + pmx_dip_sw: pmx-dip-sw { + marvell,pins = "mpp20", "mpp21", "mpp22", "mpp23"; marvell,function = "gpio"; }; - pmx_dip_sw1: pmx-dip-sw1 { - marvell,pins = "mpp21"; - marvell,function = "gpio"; - }; - - pmx_dip_sw2: pmx-dip-sw2 { - marvell,pins = "mpp22"; - marvell,function = "gpio"; - }; - - pmx_dip_sw3: pmx-dip-sw3 { - marvell,pins = "mpp23"; - marvell,function = "gpio"; - }; - - pmx_gpio_0: pmx-gpio-0 { - marvell,pins = "mpp24"; - marvell,function = "gpio"; - }; - - pmx_gpio_1: pmx-gpio-1 { - marvell,pins = "mpp25"; - marvell,function = "gpio"; - }; - - pmx_gpio_2: pmx-gpio-2 { - marvell,pins = "mpp26"; - marvell,function = "gpio"; - }; - - pmx_gpio_3: pmx-gpio-3 { - marvell,pins = "mpp27"; - marvell,function = "gpio"; - }; - - pmx_gpio_4: pmx-gpio-4 { - marvell,pins = "mpp28"; - marvell,function = "gpio"; - }; - - pmx_gpio_5: pmx-gpio-5 { - marvell,pins = "mpp29"; - marvell,function = "gpio"; - }; - - pmx_gpio_6: pmx-gpio-6 { - marvell,pins = "mpp30"; - marvell,function = "gpio"; - }; - - pmx_gpio_7: pmx-gpio-7 { - marvell,pins = "mpp31"; + /* + * 8 GPIOs available through the Hirose 16 + * pins header at the back of the device. + */ + pmx_ext_gpios: pmx-ext-gpios { + marvell,pins = "mpp24", "mpp25", "mpp26", "mpp27", + "mpp28", "mpp29", "mpp30", "mpp31"; marvell,function = "gpio"; };
The OpenBlocks A6 .dts file was using a long list of pinmux descriptors to select each GPIO of the external GPIO connector and the internal DIP switch, for no apparent reason. This commit factors those GPIO pins into two descriptors: one for the external GPIO connector and one for the internal DIP switch. As an added bonus, this commit also adds comments that says what those GPIOs are used for. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> --- arch/arm/boot/dts/kirkwood-openblocks_a6.dts | 74 +++++--------------------- 1 file changed, 14 insertions(+), 60 deletions(-)