| Message ID | 20170929101503.6769-1-ckeepax@opensource.cirrus.com |
|---|---|
| Headers | show |
| Series | Add support for muxing individual pins | expand |
On Fri, Sep 29, 2017 at 12:14 PM, Charles Keepax <ckeepax@opensource.cirrus.com> wrote: > This series add support for muxing individual pins within > pin mux, rather than just whole groups. Mainly, I had two > motivations here, one to avoid the need to add loads of groups > containing individual pins and hardware that actually has some > internal concept of groups of pins, and disambiguating that from > individual pin muxing. I have marked it as RFC to just get > peoples opinions at this stage, although it should be pretty well > tested. Sorry about the amount of files touched in patch 2 it > would be possible to drop it from the chain although it leaves > the field rather inaccurately named. > > Also I have left all the existing code paths parsing all mux > options as groups from DT, and added a new helper to unlock the > pin based functionality this should ease the transition across. There is currently a driver in the pin control subsystem that handles individual pins and that is pinctrl-single.c. The driver is deployed for single pins muxed by a single register, and if this infrastructure is to be deployed it must be applied also in pinctrl-single. We cannot have several ways of doing the same thing, that way lies madness. So you need Tony Lindgren's review and direction on this patch series. I see the problem you are setting out to solve. I too have ran into the situation (on systems such as Qualcomm's) where single-pin groups are more rule than exception. It would be good to alleviate this and handle it in the core somehow. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 09, 2017 at 11:10:34PM +0200, Linus Walleij wrote: > On Fri, Sep 29, 2017 at 12:14 PM, Charles Keepax > <ckeepax@opensource.cirrus.com> wrote: > > > This series add support for muxing individual pins within > > pin mux, rather than just whole groups. Mainly, I had two > > motivations here, one to avoid the need to add loads of groups > > containing individual pins and hardware that actually has some > > internal concept of groups of pins, and disambiguating that from > > individual pin muxing. I have marked it as RFC to just get > > peoples opinions at this stage, although it should be pretty well > > tested. Sorry about the amount of files touched in patch 2 it > > would be possible to drop it from the chain although it leaves > > the field rather inaccurately named. > > > > Also I have left all the existing code paths parsing all mux > > options as groups from DT, and added a new helper to unlock the > > pin based functionality this should ease the transition across. > > There is currently a driver in the pin control subsystem that > handles individual pins and that is pinctrl-single.c. > > The driver is deployed for single pins muxed by a single > register, and if this infrastructure is to be deployed it must > be applied also in pinctrl-single. We cannot have several ways > of doing the same thing, that way lies madness. > Apologies for the oversight I will have a look at that and try to update the series as appropriate. I am travelling for the next couple of weeks so there might be a small lag in getting an updated version out. > So you need Tony Lindgren's review and direction on this > patch series. > Any thoughts would be greatly appreciated. > I see the problem you are setting out to solve. I too have ran > into the situation (on systems such as Qualcomm's) where > single-pin groups are more rule than exception. It would be > good to alleviate this and handle it in the core somehow. Thanks, Charles -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 09, 2017 at 11:10:34PM +0200, Linus Walleij wrote: > On Fri, Sep 29, 2017 at 12:14 PM, Charles Keepax > <ckeepax@opensource.cirrus.com> wrote: > > This series add support for muxing individual pins within > > pin mux, rather than just whole groups. Mainly, I had two > > motivations here, one to avoid the need to add loads of groups > > containing individual pins and hardware that actually has some > > internal concept of groups of pins, and disambiguating that from > > individual pin muxing. I have marked it as RFC to just get > > peoples opinions at this stage, although it should be pretty well > > tested. Sorry about the amount of files touched in patch 2 it > > would be possible to drop it from the chain although it leaves > > the field rather inaccurately named. > > > > Also I have left all the existing code paths parsing all mux > > options as groups from DT, and added a new helper to unlock the > > pin based functionality this should ease the transition across. > There is currently a driver in the pin control subsystem that > handles individual pins and that is pinctrl-single.c. > > The driver is deployed for single pins muxed by a single > register, and if this infrastructure is to be deployed it must > be applied also in pinctrl-single. We cannot have several ways > of doing the same thing, that way lies madness. > > So you need Tony Lindgren's review and direction on this > patch series. Apologies for the delay on this one, I got some what snowed under with other tasks. Please let me know if you would rather I just resent the series to refresh everyones memory. But I have finally managed to get some time to look over the pinctrl-single stuff. Naively one could convert the pinctrl-single stuff over to use the patches I proposed creating one large group for the driver and then mux each pin individually from within that. However I am not really sure it would make sense. From the implementation so far the pinctrl-single stuff appears to target systems where there isn't really a concept of groups. Each pin is just a completely separate entry and you can only configure things one pin at a time. In that case it almost makes more sense to model each pin as an individual group such that it is clearly distinct from the others. My thinking had been more along the lines of you perhaps have a group that represents an I2S port but you can also individually assign each of those pins as a GPIO when not in use as the I2S port. Alternatively one could perhaps look at expanding the pinctrl-single stuff to have some notion of groups as well. So you could define sub groups of the pins that can be set as a block. Allowing users to configure either groups or individual pins. I guess my main question is what is the intention of the pinctrl-single code, is this something that should be seeing expansion to handle groups as well or is it firmly for the unrelated pins cases? Thanks, Charles -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Dec 8, 2017 at 3:29 PM, Charles Keepax <ckeepax@opensource.cirrus.com> wrote: > (...) I have finally > managed to get some time to look over the pinctrl-single stuff. > > Naively one could convert the pinctrl-single stuff over to use > the patches I proposed creating one large group for the driver > and then mux each pin individually from within that. However I > am not really sure it would make sense. From the implementation > so far the pinctrl-single stuff appears to target systems where > there isn't really a concept of groups. Each pin is just a > completely separate entry and you can only configure things one > pin at a time. In that case it almost makes more sense to model > each pin as an individual group such that it is clearly distinct > from the others. My thinking had been more along the lines of you > perhaps have a group that represents an I2S port but you can also > individually assign each of those pins as a GPIO when not in use > as the I2S port. So then I toss the qcom driver into the game instead :) If you look at drivers/pinctrl/qcom/* e.g. pinctrl-ipq4019.c or essentially any of the subdrivers, you find exactly this scenario. I am concerned that if we add infrastructure for this, it needs to have more than one user. Qualcomm does fit your description above I think. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Charles Keepax <ckeepax@opensource.cirrus.com> [171208 14:31]: > From the implementation so far the pinctrl-single stuff appears to > target systems where there isn't really a concept of groups. Each > pin is just a completely separate entry and you can only configure > things one pin at a time. In that case it almost makes more sense > to model each pin as an individual group such that it is clearly > distinct from the others. Maybe check again or else I don't follow you :) The pinctrl groups are created dynamically with pinctrl-single based on how the pins are grouped in the dts file: mmc1_pins: pinmux_mmc1_pins { pinctrl-single,pins = < OMAP4_IOPAD(0x0e2, PIN_INPUT_PULLUP | MUX_MODE0) OMAP4_IOPAD(0x0e4, PIN_INPUT_PULLUP | MUX_MODE0) OMAP4_IOPAD(0x0e6, PIN_INPUT_PULLUP | MUX_MODE0) OMAP4_IOPAD(0x0e8, PIN_INPUT_PULLUP | MUX_MODE0) OMAP4_IOPAD(0x0ea, PIN_INPUT_PULLUP | MUX_MODE0) OMAP4_IOPAD(0x0ec, PIN_INPUT_PULLUP | MUX_MODE0) >; }; Then on a booted system we have the following under /sys/kernel/debug/pinctrl/4a100040.pinmux/pingroups: group: pinmux_mmc1_pins pin 81 (PIN81) pin 82 (PIN82) pin 83 (PIN83) pin 84 (PIN84) pin 85 (PIN85) pin 86 (PIN86) Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Dec 08, 2017 at 08:28:18AM -0800, Tony Lindgren wrote: > * Charles Keepax <ckeepax@opensource.cirrus.com> [171208 14:31]: > > From the implementation so far the pinctrl-single stuff appears to > > target systems where there isn't really a concept of groups. Each > > pin is just a completely separate entry and you can only configure > > things one pin at a time. In that case it almost makes more sense > > to model each pin as an individual group such that it is clearly > > distinct from the others. > > Maybe check again or else I don't follow you :) > > The pinctrl groups are created dynamically with pinctrl-single > based on how the pins are grouped in the dts file: > > mmc1_pins: pinmux_mmc1_pins { > pinctrl-single,pins = < > OMAP4_IOPAD(0x0e2, PIN_INPUT_PULLUP | MUX_MODE0) > OMAP4_IOPAD(0x0e4, PIN_INPUT_PULLUP | MUX_MODE0) > OMAP4_IOPAD(0x0e6, PIN_INPUT_PULLUP | MUX_MODE0) > OMAP4_IOPAD(0x0e8, PIN_INPUT_PULLUP | MUX_MODE0) > OMAP4_IOPAD(0x0ea, PIN_INPUT_PULLUP | MUX_MODE0) > OMAP4_IOPAD(0x0ec, PIN_INPUT_PULLUP | MUX_MODE0) > >; > }; > > Then on a booted system we have the following under > /sys/kernel/debug/pinctrl/4a100040.pinmux/pingroups: > > group: pinmux_mmc1_pins > pin 81 (PIN81) > pin 82 (PIN82) > pin 83 (PIN83) > pin 84 (PIN84) > pin 85 (PIN85) > pin 86 (PIN86) > Hmm... apologies, I suspect this is me that needs to review the code some more. So this is actually creating a group per user rather than per controller. Although I guess my original point still stands that I am not clear how this would benefit from my patches to allow muxing of individual pins within a group. Since I guess each user will just get a group created for the pins they use regardless. Or am I missing something here too? Thanks, Charles -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Dec 08, 2017 at 03:40:49PM +0100, Linus Walleij wrote: > On Fri, Dec 8, 2017 at 3:29 PM, Charles Keepax > <ckeepax@opensource.cirrus.com> wrote: > > > (...) I have finally > > managed to get some time to look over the pinctrl-single stuff. > > > > Naively one could convert the pinctrl-single stuff over to use > > the patches I proposed creating one large group for the driver > > and then mux each pin individually from within that. However I > > am not really sure it would make sense. From the implementation > > so far the pinctrl-single stuff appears to target systems where > > there isn't really a concept of groups. Each pin is just a > > completely separate entry and you can only configure things one > > pin at a time. In that case it almost makes more sense to model > > each pin as an individual group such that it is clearly distinct > > from the others. My thinking had been more along the lines of you > > perhaps have a group that represents an I2S port but you can also > > individually assign each of those pins as a GPIO when not in use > > as the I2S port. > > So then I toss the qcom driver into the game instead :) > > If you look at drivers/pinctrl/qcom/* e.g. pinctrl-ipq4019.c or > essentially any of the subdrivers, you find exactly this scenario. > > I am concerned that if we add infrastructure for this, it needs > to have more than one user. Qualcomm does fit your description > above I think. > Yeah I could certainly have a hunt through for other users that would make good candidates to update. The QC driver certainly looks like it would be capable of muxing individual pins, although it looks like it might not let you mux an individual GPIO at the moment, need to dig into that more. I guess maybe a bigger question is do we think this is a problem worth solving or should really just be adding a group for each pin I want to be able to mux? Thanks, Charles -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Charles Keepax <ckeepax@opensource.cirrus.com> [171208 17:19]: > On Fri, Dec 08, 2017 at 08:28:18AM -0800, Tony Lindgren wrote: > > * Charles Keepax <ckeepax@opensource.cirrus.com> [171208 14:31]: > > > From the implementation so far the pinctrl-single stuff appears to > > > target systems where there isn't really a concept of groups. Each > > > pin is just a completely separate entry and you can only configure > > > things one pin at a time. In that case it almost makes more sense > > > to model each pin as an individual group such that it is clearly > > > distinct from the others. > > > > Maybe check again or else I don't follow you :) > > > > The pinctrl groups are created dynamically with pinctrl-single > > based on how the pins are grouped in the dts file: > > > > mmc1_pins: pinmux_mmc1_pins { > > pinctrl-single,pins = < > > OMAP4_IOPAD(0x0e2, PIN_INPUT_PULLUP | MUX_MODE0) > > OMAP4_IOPAD(0x0e4, PIN_INPUT_PULLUP | MUX_MODE0) > > OMAP4_IOPAD(0x0e6, PIN_INPUT_PULLUP | MUX_MODE0) > > OMAP4_IOPAD(0x0e8, PIN_INPUT_PULLUP | MUX_MODE0) > > OMAP4_IOPAD(0x0ea, PIN_INPUT_PULLUP | MUX_MODE0) > > OMAP4_IOPAD(0x0ec, PIN_INPUT_PULLUP | MUX_MODE0) > > >; > > }; > > > > Then on a booted system we have the following under > > /sys/kernel/debug/pinctrl/4a100040.pinmux/pingroups: > > > > group: pinmux_mmc1_pins > > pin 81 (PIN81) > > pin 82 (PIN82) > > pin 83 (PIN83) > > pin 84 (PIN84) > > pin 85 (PIN85) > > pin 86 (PIN86) > > > > Hmm... apologies, I suspect this is me that needs to review the > code some more. So this is actually creating a group per user > rather than per controller. > > Although I guess my original point still stands that I am not > clear how this would benefit from my patches to allow muxing of > individual pins within a group. Since I guess each user will just > get a group created for the pins they use regardless. Or am I > missing something here too? Yeah it seems pinctrl-single does not need anything specific for muxing individual pins within a group. You can already do that quite easily by creating multiple groups in the dts for the device. Let's assume you need to dynamically remux MMC dat1 line between GPIO and MMC mode (similar to what we do on am335x for dat1 wake-up interrupts). In that case you would just modify the grouping example above with: mmc1_dat1_pin: pinmux_mmc1_dat1_pin { pinctrl-single,pins = < OMAP4_IOPAD(0x0e8, PIN_INPUT_PULLUP | MUX_MODE0) >; }; mmc1_pins: pinmux_mmc1_pins { pinctrl-single,pins = < OMAP4_IOPAD(0x0e2, PIN_INPUT_PULLUP | MUX_MODE0) OMAP4_IOPAD(0x0e4, PIN_INPUT_PULLUP | MUX_MODE0) OMAP4_IOPAD(0x0e6, PIN_INPUT_PULLUP | MUX_MODE0) OMAP4_IOPAD(0x0ea, PIN_INPUT_PULLUP | MUX_MODE0) OMAP4_IOPAD(0x0ec, PIN_INPUT_PULLUP | MUX_MODE0) >; }; And that allows you to dynamically change dat1 pin only. Not sure if that helps in your case though, just trying to clarify how it currently works :) Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri 08 Dec 09:22 PST 2017, Charles Keepax wrote: > On Fri, Dec 08, 2017 at 03:40:49PM +0100, Linus Walleij wrote: > > On Fri, Dec 8, 2017 at 3:29 PM, Charles Keepax > > <ckeepax@opensource.cirrus.com> wrote: > > > > > (...) I have finally > > > managed to get some time to look over the pinctrl-single stuff. > > > > > > Naively one could convert the pinctrl-single stuff over to use > > > the patches I proposed creating one large group for the driver > > > and then mux each pin individually from within that. However I > > > am not really sure it would make sense. From the implementation > > > so far the pinctrl-single stuff appears to target systems where > > > there isn't really a concept of groups. Each pin is just a > > > completely separate entry and you can only configure things one > > > pin at a time. In that case it almost makes more sense to model > > > each pin as an individual group such that it is clearly distinct > > > from the others. My thinking had been more along the lines of you > > > perhaps have a group that represents an I2S port but you can also > > > individually assign each of those pins as a GPIO when not in use > > > as the I2S port. > > > > So then I toss the qcom driver into the game instead :) > > > > If you look at drivers/pinctrl/qcom/* e.g. pinctrl-ipq4019.c or > > essentially any of the subdrivers, you find exactly this scenario. > > > > I am concerned that if we add infrastructure for this, it needs > > to have more than one user. Qualcomm does fit your description > > above I think. > > > > Yeah I could certainly have a hunt through for other users that > would make good candidates to update. The QC driver certainly > looks like it would be capable of muxing individual pins, > although it looks like it might not let you mux an individual > GPIO at the moment, need to dig into that more. > In the Qualcomm hardware we have X GPIO pins and some number of special purpose pins. GPIO pins can be muxed and controlled individually and the special purpose pins (e.g. SDCC) are controlled as a group. I expected to describe both of these types as "pinctrl groups", but after struggling with this I realized that as the control is on pin-granularity (for the GPIOs) this is what should be represented as a group in pinctl/pinmux/pinconf. So each GPIO pin can be controlled/muxed individually and as such is represented as a group, special groups are describes as one group (matching the single set of registers). To configure a "logical group" (e.g. a UART), we list each pin (technically group), specify the same function and the necessary configuration options. So if you hardware supports configuring individual pins within a group my suggestion is that you should rework the driver to make "group" match "configurable thing" and then tie together logical groups when configuring your system. PS. A side effect of this is that e.g. UART is a function of 4 pins, but by specifying the pinmux/pinconf of only two of them I have a 2 pin UART, without having to describe this setup in the pinctrl driver. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html