| Message ID | 20160927002650.4316-1-stefan@agner.ch |
|---|---|
| State | New |
| Headers | show |
Hi Stefan, On 09/27/2016 03:26 AM, Stefan Agner wrote: > If a GPIO gets freed after selecting a new pinctrl configuration > the driver should not change pinctrl anymore. Otherwise this will > likely lead to a unusable pin configuration for > the newly selected > pinctrl. > > Signed-off-by: Stefan Agner <stefan@agner.ch> > --- > This turned out to be problematic when using the I2C GPIO bus recovery >functionality. After muxing back to I2C, the GPIO is being freed, which > cased I2C to stop working completely. IMHO this recent "i.MX I2C GPIO bus recovery" feature is kind of a hack, for example I believe it breaks I2C bus driver initialization on i.MX31 boards, where today there is no pinctrl driver at all. IMHO something like I've partially described in the recent "Requesting as a GPIO a pin already used through pinctrl" topic should be done here. Could you consider to add another pinctrl-1 group with alternative GPIO line mux/config settings to an i2c controller device node and apply it, when you need a bus recovery? You may find references how this kind of dynamic pinctrl management is done within mmc/sd subsystem. By the way did I miss a patch, which falls back to mux settings on .gpio_disable_free call for non-Vybrid platforms? -- With best wishes, Vladimir -- 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 2016-09-27 05:12, Vladimir Zapolskiy wrote: > Hi Stefan, > > On 09/27/2016 03:26 AM, Stefan Agner wrote: >> If a GPIO gets freed after selecting a new pinctrl configuration >> the driver should not change pinctrl anymore. Otherwise this will >> likely lead to a unusable pin configuration for > the newly selected >> pinctrl. >> >> Signed-off-by: Stefan Agner <stefan@agner.ch> >> --- >> This turned out to be problematic when using the I2C GPIO bus recovery >>functionality. After muxing back to I2C, the GPIO is being freed, which >> cased I2C to stop working completely. > > IMHO this recent "i.MX I2C GPIO bus recovery" feature is kind of a hack, > for example I believe it breaks I2C bus driver initialization on i.MX31 > boards, where today there is no pinctrl driver at all. This has been addressed by Li Yang's patch, already in the next branch: https://lkml.org/lkml/2016/9/12/1161 > > IMHO something like I've partially described in the recent "Requesting as > a GPIO a pin already used through pinctrl" topic should be done here. > Could you consider to add another pinctrl-1 group with alternative GPIO > line mux/config settings to an i2c controller device node and apply it, > when you need a bus recovery? You may find references how this kind of > dynamic pinctrl management is done within mmc/sd subsystem. I don't quite understand, that is already the case. This is what device tree looks like to get the I2C recovery functionality: &i2c1 { pinctrl-names = "default", "gpio"; pinctrl-0 = <&pinctrl_i2c1>; pinctrl-1 = <&pinctrl_i2c1_gpio>; scl-gpios = <&gpio1 6 GPIO_ACTIVE_HIGH>; sda-gpios = <&gpio1 7 GPIO_ACTIVE_HIGH>; status = "okay"; }; > > By the way did I miss a patch, which falls back to mux settings on > .gpio_disable_free call for non-Vybrid platforms? Currently only Vybrid makes use of the .gpio_request_enable... and so should .gpio_disable_free then. -- Stefan -- 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
Hi Stefan, On 09/27/2016 07:37 PM, Stefan Agner wrote: > On 2016-09-27 05:12, Vladimir Zapolskiy wrote: >> Hi Stefan, >> >> On 09/27/2016 03:26 AM, Stefan Agner wrote: >>> If a GPIO gets freed after selecting a new pinctrl configuration >>> the driver should not change pinctrl anymore. Otherwise this will >>> likely lead to a unusable pin configuration for > the newly selected >>> pinctrl. >>> >>> Signed-off-by: Stefan Agner <stefan@agner.ch> >>> --- >>> This turned out to be problematic when using the I2C GPIO bus recovery >>> functionality. After muxing back to I2C, the GPIO is being freed, which >>> cased I2C to stop working completely. >> >> IMHO this recent "i.MX I2C GPIO bus recovery" feature is kind of a hack, >> for example I believe it breaks I2C bus driver initialization on i.MX31 >> boards, where today there is no pinctrl driver at all. > > This has been addressed by Li Yang's patch, already in the next branch: > https://lkml.org/lkml/2016/9/12/1161 Nice to know about it, thank you for the link. >> >> IMHO something like I've partially described in the recent "Requesting as >> a GPIO a pin already used through pinctrl" topic should be done here. >> Could you consider to add another pinctrl-1 group with alternative GPIO >> line mux/config settings to an i2c controller device node and apply it, >> when you need a bus recovery? You may find references how this kind of >> dynamic pinctrl management is done within mmc/sd subsystem. > > I don't quite understand, that is already the case. This is what device > tree looks like to get the I2C recovery functionality: > > &i2c1 { > pinctrl-names = "default", "gpio"; > pinctrl-0 = <&pinctrl_i2c1>; > pinctrl-1 = <&pinctrl_i2c1_gpio>; > scl-gpios = <&gpio1 6 GPIO_ACTIVE_HIGH>; > sda-gpios = <&gpio1 7 GPIO_ACTIVE_HIGH>; > status = "okay"; > }; Great, then why do you experience a problem you've described? >>> After muxing back to I2C, the GPIO is being freed, which cased I2C >>> to stop working completely. Release GPIO firstly, then mux back to I2C, that's the correct sequence and I believe it obsoletes this change. >> >> By the way did I miss a patch, which falls back to mux settings on >> .gpio_disable_free call for non-Vybrid platforms? > > Currently only Vybrid makes use of the .gpio_request_enable... and so > should .gpio_disable_free then. > So, I guess this is a change with a runtime difference for Vybrid only. I find that it was initially done wrong that a number of Vybrid specific hooks were added to the shared pinctrl-imx.c, in my opinion it is better to make needed abstractions and move all code around SHARE_MUX_CONF_REG to pinctrl-vf610.c: ./pinctrl-imx.c:216: if (info->flags & SHARE_MUX_CONF_REG) { ./pinctrl-imx.c:317: if (!(info->flags & SHARE_MUX_CONF_REG)) ./pinctrl-imx.c:357: if (!(info->flags & SHARE_MUX_CONF_REG)) ./pinctrl-imx.c:382: if (!(info->flags & SHARE_MUX_CONF_REG)) ./pinctrl-imx.c:425: if (info->flags & SHARE_MUX_CONF_REG) ./pinctrl-imx.c:450: if (info->flags & SHARE_MUX_CONF_REG) { ./pinctrl-imx.c:534: if (info->flags & SHARE_MUX_CONF_REG) ./pinctrl-imx.c:575: if (info->flags & SHARE_MUX_CONF_REG) { Nevertheless this is not directly related to the change. -- With best wishes, Vladimir -- 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 2016-09-27 11:17, Vladimir Zapolskiy wrote: > Hi Stefan, > > On 09/27/2016 07:37 PM, Stefan Agner wrote: >> On 2016-09-27 05:12, Vladimir Zapolskiy wrote: >>> Hi Stefan, >>> >>> On 09/27/2016 03:26 AM, Stefan Agner wrote: >>>> If a GPIO gets freed after selecting a new pinctrl configuration >>>> the driver should not change pinctrl anymore. Otherwise this will >>>> likely lead to a unusable pin configuration for > the newly selected >>>> pinctrl. >>>> >>>> Signed-off-by: Stefan Agner <stefan@agner.ch> >>>> --- >>>> This turned out to be problematic when using the I2C GPIO bus recovery >>>> functionality. After muxing back to I2C, the GPIO is being freed, which >>>> cased I2C to stop working completely. >>> >>> IMHO this recent "i.MX I2C GPIO bus recovery" feature is kind of a hack, >>> for example I believe it breaks I2C bus driver initialization on i.MX31 >>> boards, where today there is no pinctrl driver at all. >> >> This has been addressed by Li Yang's patch, already in the next branch: >> https://lkml.org/lkml/2016/9/12/1161 > > Nice to know about it, thank you for the link. > >>> >>> IMHO something like I've partially described in the recent "Requesting as >>> a GPIO a pin already used through pinctrl" topic should be done here. >>> Could you consider to add another pinctrl-1 group with alternative GPIO >>> line mux/config settings to an i2c controller device node and apply it, >>> when you need a bus recovery? You may find references how this kind of >>> dynamic pinctrl management is done within mmc/sd subsystem. >> >> I don't quite understand, that is already the case. This is what device >> tree looks like to get the I2C recovery functionality: >> >> &i2c1 { >> pinctrl-names = "default", "gpio"; >> pinctrl-0 = <&pinctrl_i2c1>; >> pinctrl-1 = <&pinctrl_i2c1_gpio>; >> scl-gpios = <&gpio1 6 GPIO_ACTIVE_HIGH>; >> sda-gpios = <&gpio1 7 GPIO_ACTIVE_HIGH>; >> status = "okay"; >> }; > > Great, then why do you experience a problem you've described? > >>>> After muxing back to I2C, the GPIO is being freed, which cased I2C >>>> to stop working completely. > > Release GPIO firstly, then mux back to I2C, that's the correct sequence > and I believe it obsoletes this change. > Added Viresh Kumar to the discussion, he implemented the I2C recovery functions. Yes, reordering the pinctrl/gpio_free calls would fix the problem too. However, I guess there is no explicit rule to that ("request/free GPIOs only when they are muxed as GPIO"), so I think of it that the issue is actually in the pinctrl driver. On top of that it is not entirely trivial to reorder the calls the way i2c_generic_gpio_recovery and i2c_generic_recovery are set up right now. >>> >>> By the way did I miss a patch, which falls back to mux settings on >>> .gpio_disable_free call for non-Vybrid platforms? >> >> Currently only Vybrid makes use of the .gpio_request_enable... and so >> should .gpio_disable_free then. >> > > So, I guess this is a change with a runtime difference for Vybrid only. That is correct. > > I find that it was initially done wrong that a number of Vybrid specific > hooks were added to the shared pinctrl-imx.c, in my opinion it is better > to make needed abstractions and move all code around SHARE_MUX_CONF_REG > to pinctrl-vf610.c: > > ./pinctrl-imx.c:216: if (info->flags & SHARE_MUX_CONF_REG) { > ./pinctrl-imx.c:317: if (!(info->flags & SHARE_MUX_CONF_REG)) > ./pinctrl-imx.c:357: if (!(info->flags & SHARE_MUX_CONF_REG)) > ./pinctrl-imx.c:382: if (!(info->flags & SHARE_MUX_CONF_REG)) > ./pinctrl-imx.c:425: if (info->flags & SHARE_MUX_CONF_REG) > ./pinctrl-imx.c:450: if (info->flags & SHARE_MUX_CONF_REG) { > ./pinctrl-imx.c:534: if (info->flags & SHARE_MUX_CONF_REG) > ./pinctrl-imx.c:575: if (info->flags & SHARE_MUX_CONF_REG) { > > Nevertheless this is not directly related to the change. Yeah I was really on the fence there. Despite the same name, the IOMUXC on Vybrid is quite a bit different than on i.MX. The problem is it is not easily possible to factor out the SoC specific stuff into pinctrl-vf610.c I would have basically had to add tons of callbacks or re-implement lots of functions in pinctrl-vf610.c. Maybe it would have probably been better to basically implement Vybrids IOMUXC as its own pinctrl driver. -- Stefan -- 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 09/27/2016 10:34 PM, Stefan Agner wrote: > On 2016-09-27 11:17, Vladimir Zapolskiy wrote: >> Hi Stefan, >> >> On 09/27/2016 07:37 PM, Stefan Agner wrote: >>> On 2016-09-27 05:12, Vladimir Zapolskiy wrote: >>>> Hi Stefan, >>>> >>>> On 09/27/2016 03:26 AM, Stefan Agner wrote: >>>>> If a GPIO gets freed after selecting a new pinctrl configuration >>>>> the driver should not change pinctrl anymore. Otherwise this will >>>>> likely lead to a unusable pin configuration for > the newly selected >>>>> pinctrl. >>>>> >>>>> Signed-off-by: Stefan Agner <stefan@agner.ch> >>>>> --- >>>>> This turned out to be problematic when using the I2C GPIO bus recovery >>>>> functionality. After muxing back to I2C, the GPIO is being freed, which >>>>> cased I2C to stop working completely. >>>> >>>> IMHO this recent "i.MX I2C GPIO bus recovery" feature is kind of a hack, >>>> for example I believe it breaks I2C bus driver initialization on i.MX31 >>>> boards, where today there is no pinctrl driver at all. >>> >>> This has been addressed by Li Yang's patch, already in the next branch: >>> https://lkml.org/lkml/2016/9/12/1161 >> >> Nice to know about it, thank you for the link. >> >>>> >>>> IMHO something like I've partially described in the recent "Requesting as >>>> a GPIO a pin already used through pinctrl" topic should be done here. >>>> Could you consider to add another pinctrl-1 group with alternative GPIO >>>> line mux/config settings to an i2c controller device node and apply it, >>>> when you need a bus recovery? You may find references how this kind of >>>> dynamic pinctrl management is done within mmc/sd subsystem. >>> >>> I don't quite understand, that is already the case. This is what device >>> tree looks like to get the I2C recovery functionality: >>> >>> &i2c1 { >>> pinctrl-names = "default", "gpio"; >>> pinctrl-0 = <&pinctrl_i2c1>; >>> pinctrl-1 = <&pinctrl_i2c1_gpio>; >>> scl-gpios = <&gpio1 6 GPIO_ACTIVE_HIGH>; >>> sda-gpios = <&gpio1 7 GPIO_ACTIVE_HIGH>; >>> status = "okay"; >>> }; >> >> Great, then why do you experience a problem you've described? >> >>>>> After muxing back to I2C, the GPIO is being freed, which cased I2C >>>>> to stop working completely. >> >> Release GPIO firstly, then mux back to I2C, that's the correct sequence >> and I believe it obsoletes this change. >> > > Added Viresh Kumar to the discussion, he implemented the I2C recovery > functions. > > Yes, reordering the pinctrl/gpio_free calls would fix the problem too. > > However, I guess there is no explicit rule to that ("request/free GPIOs > only when they are muxed as GPIO"), so I think of it that the issue is > actually in the pinctrl driver. there is a rule applied to strict type pinctrl controllers (and from the code I see that Vybrid IOMUXC is a strict type pinctrl, the existense of the problem under discussion testifies that it is strict also) that pads shall not be requested by GPIO or another controller, when they are in use by some controller. In your case you have I2C and GPIO controllers competing for a pad, please manage the resource correctly, request should be done only after release. > On top of that it is not entirely trivial to reorder the calls the way > i2c_generic_gpio_recovery and i2c_generic_recovery are set up right now. > Better to fix it earlier than later. >>>> >>>> By the way did I miss a patch, which falls back to mux settings on >>>> .gpio_disable_free call for non-Vybrid platforms? >>> >>> Currently only Vybrid makes use of the .gpio_request_enable... and so >>> should .gpio_disable_free then. >>> >> >> So, I guess this is a change with a runtime difference for Vybrid only. > > That is correct. > >> >> I find that it was initially done wrong that a number of Vybrid specific >> hooks were added to the shared pinctrl-imx.c, in my opinion it is better >> to make needed abstractions and move all code around SHARE_MUX_CONF_REG >> to pinctrl-vf610.c: >> >> ./pinctrl-imx.c:216: if (info->flags & SHARE_MUX_CONF_REG) { >> ./pinctrl-imx.c:317: if (!(info->flags & SHARE_MUX_CONF_REG)) >> ./pinctrl-imx.c:357: if (!(info->flags & SHARE_MUX_CONF_REG)) >> ./pinctrl-imx.c:382: if (!(info->flags & SHARE_MUX_CONF_REG)) >> ./pinctrl-imx.c:425: if (info->flags & SHARE_MUX_CONF_REG) >> ./pinctrl-imx.c:450: if (info->flags & SHARE_MUX_CONF_REG) { >> ./pinctrl-imx.c:534: if (info->flags & SHARE_MUX_CONF_REG) >> ./pinctrl-imx.c:575: if (info->flags & SHARE_MUX_CONF_REG) { >> >> Nevertheless this is not directly related to the change. > > Yeah I was really on the fence there. Despite the same name, the IOMUXC > on Vybrid is quite a bit different than on i.MX. The problem is it is > not easily possible to factor out the SoC specific stuff into > pinctrl-vf610.c I would have basically had to add tons of callbacks or > re-implement lots of functions in pinctrl-vf610.c. Maybe it would have > probably been better to basically implement Vybrids IOMUXC as its own > pinctrl driver. -- With best wishes, Vladimir -- 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 27-09-16, 12:34, Stefan Agner wrote: > Added Viresh Kumar to the discussion, he implemented the I2C recovery > functions. > > Yes, reordering the pinctrl/gpio_free calls would fix the problem too. > > However, I guess there is no explicit rule to that ("request/free GPIOs > only when they are muxed as GPIO"), so I think of it that the issue is > actually in the pinctrl driver. > > On top of that it is not entirely trivial to reorder the calls the way > i2c_generic_gpio_recovery and i2c_generic_recovery are set up right now. AFAICT, these routines don't touch the muxing part at all. Perhaps it is done internally by the GPIO calls. Can you please elaborate the exact change you are hinting towards here ?
On 2016-09-27 19:00, Viresh Kumar wrote: > On 27-09-16, 12:34, Stefan Agner wrote: >> Added Viresh Kumar to the discussion, he implemented the I2C recovery >> functions. >> >> Yes, reordering the pinctrl/gpio_free calls would fix the problem too. >> >> However, I guess there is no explicit rule to that ("request/free GPIOs >> only when they are muxed as GPIO"), so I think of it that the issue is >> actually in the pinctrl driver. >> >> On top of that it is not entirely trivial to reorder the calls the way >> i2c_generic_gpio_recovery and i2c_generic_recovery are set up right now. > > AFAICT, these routines don't touch the muxing part at all. Perhaps it is done > internally by the GPIO calls. Can you please elaborate the exact change you are > hinting towards here ? The i.MX I2C driver touches the pinctrl in its prepare/unprepare callbacks. So, on a i.MX or Vybrid, the call chain looks like this: i2c_generic_gpio_recovery -> i2c_get_gpios_for_recovery -> gpio_request_one -> i2c_generic_recovery -> prepare_recovery (i2c_imx_prepare_recovery) -> pinctrl_select_state [gpio] -> unprepare_recovery (i2c_imx_unprepare_recovery) -> pinctrl_select_state [default] -> i2c_put_gpios_for_recovery -> gpio_free And for the pinctrl/GPIO driver of Vybrid this is actually a problem because gpio_free disables the output driver of the pad, and when that happens after the (I2C) default pinctrl state gets selected the pad is no longer active. -- Stefan -- 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 27-09-16, 20:38, Stefan Agner wrote: > The i.MX I2C driver touches the pinctrl in its prepare/unprepare > callbacks. > > So, on a i.MX or Vybrid, the call chain looks like this: > > i2c_generic_gpio_recovery > -> i2c_get_gpios_for_recovery > -> gpio_request_one > -> i2c_generic_recovery > -> prepare_recovery (i2c_imx_prepare_recovery) > -> pinctrl_select_state [gpio] Why is this done here? And not in gpio_request_one? > -> unprepare_recovery (i2c_imx_unprepare_recovery) > -> pinctrl_select_state [default] > -> i2c_put_gpios_for_recovery > -> gpio_free > > > And for the pinctrl/GPIO driver of Vybrid this is actually a problem > because gpio_free disables the output driver of the pad, and when that > happens after the (I2C) default pinctrl state gets selected the pad is > no longer active. > > -- > Stefan
On 2016-09-27 21:14, Viresh Kumar wrote: > On 27-09-16, 20:38, Stefan Agner wrote: >> The i.MX I2C driver touches the pinctrl in its prepare/unprepare >> callbacks. >> >> So, on a i.MX or Vybrid, the call chain looks like this: >> >> i2c_generic_gpio_recovery >> -> i2c_get_gpios_for_recovery >> -> gpio_request_one >> -> i2c_generic_recovery >> -> prepare_recovery (i2c_imx_prepare_recovery) >> -> pinctrl_select_state [gpio] > > Why is this done here? And not in gpio_request_one? > You need to differentiate between Vybrid and i.MX: Vybrid muxes a pin to GPIO on gpio_request_one (via .gpio_request_enable callback) i.MX does not mux a pin as GPIO on its own, but needs to be muxed explicitly. That has been always the case... I don't know what behavior is right, it is just "different"... In Vybrid I did it that way because I knew that was the behavior of another SoC I worked on (namely a Tegra)... And I had to touch the pinctrl register anyway (using gpio_set_direction, to set the direction). I guess in the end it boils down to one question: Is the GPIO and pinctrl API ment to be orthogonal? If so, then we probably should select the GPIO via pinctrl in the i.MX I2C driver but mux the pin in gpio_request_one similar as we do it on Vybrid... But that would be rather invasive change... @Shawn, Linus Walleij, others, what is your take on that? >> -> unprepare_recovery (i2c_imx_unprepare_recovery) >> -> pinctrl_select_state [default] >> -> i2c_put_gpios_for_recovery >> -> gpio_free Currently free does not restore the last pinmux, so if the API's are meant to be completely orthogonal we need to store the pinmux in gpio_request_one so we can restore it in gpio_free. -- Stefan -- 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 29-09-16, 09:33, Stefan Agner wrote: > You need to differentiate between Vybrid and i.MX: > > Vybrid muxes a pin to GPIO on gpio_request_one (via .gpio_request_enable > callback) > i.MX does not mux a pin as GPIO on its own, but needs to be muxed > explicitly. That has been always the case... > > I don't know what behavior is right, it is just "different"... Hmm, I think What Vybrid and Tegra have done is better, but it would be better to get inputs from Linus, which you already asked for :)
On Thu, Sep 29, 2016 at 6:33 PM, Stefan Agner <stefan@agner.ch> wrote: > On 2016-09-27 21:14, Viresh Kumar wrote: >> On 27-09-16, 20:38, Stefan Agner wrote: >>> The i.MX I2C driver touches the pinctrl in its prepare/unprepare >>> callbacks. >>> >>> So, on a i.MX or Vybrid, the call chain looks like this: >>> >>> i2c_generic_gpio_recovery >>> -> i2c_get_gpios_for_recovery >>> -> gpio_request_one >>> -> i2c_generic_recovery >>> -> prepare_recovery (i2c_imx_prepare_recovery) >>> -> pinctrl_select_state [gpio] >> >> Why is this done here? And not in gpio_request_one? >> > > You need to differentiate between Vybrid and i.MX: > > Vybrid muxes a pin to GPIO on gpio_request_one (via .gpio_request_enable > callback) > i.MX does not mux a pin as GPIO on its own, but needs to be muxed > explicitly. That has been always the case... > > I don't know what behavior is right, it is just "different"... Exactly. It would have been nice if we had defined clear semantics on how this should work when creating the pin control subsystem, had we been able to agree. We didn't so it's up to each driver and system to deal with this in whatever way they like. Whoever is interested in stringency may drive it. > In Vybrid I did it that way because I knew that was the behavior of > another SoC I worked on (namely a Tegra)... And I had to touch the > pinctrl register anyway (using gpio_set_direction, to set the > direction). Yes and that is one school of pin controlling. > I guess in the end it boils down to one question: Is the GPIO and > pinctrl API ment to be orthogonal? Both and. The platforms implementing the following in their struct pinmux_ops: .gpio_request_enable() .gpio_disable_free() .gpio_set_direction() AND have a corresponding GPIO driver calling gpiochip_add_pin_range() or gpiochip_add_pingroup_range() so we can cross reference pins and GPIO lines, AND do something like call pinctrl_request_gpio() and pinctrl_free_gpio() equivalent to the below: if (of_property_read_bool(dev->of_node, "gpio-ranges")) { chip->gc.request = gpiochip_generic_request; chip->gc.free = gpiochip_generic_free; } And potentially also pinctrl_gpio_direction_input() and pinctrl_gpio_direction_output() will use the corresponing pin controller as back end in their GPIO set-up when dealing with request/free and optionally also direction. This is cooperative and NOT orthogonal. On the other hand: a pin control driver implementing none of the above functions will be orthogonal, and in these cases they just have to mux the line into GPIO mode on their own using tables, DT, hogs, whatever. > If so, then we probably should select > the GPIO via pinctrl in the i.MX I2C driver but mux the pin in > gpio_request_one similar as we do it on Vybrid... But that would be > rather invasive change... > > @Shawn, Linus Walleij, others, what is your take on that? You can choose. Do everything muxwise in the pin controller or do it all as a back-end per above. Notice that the above approach has "holes": we can only set direction. Any other pin configuration (drive strength, push/pull or open drain, bias etc) still need to be set up from pin control. I had some ideas of how to deal with that by adding yet another cross call but never get around to fixing it. >>> -> unprepare_recovery (i2c_imx_unprepare_recovery) >>> -> pinctrl_select_state [default] >>> -> i2c_put_gpios_for_recovery >>> -> gpio_free > > Currently free does not restore the last pinmux, so if the API's are > meant to be completely orthogonal we need to store the pinmux in > gpio_request_one so we can restore it in gpio_free. I don't understand this but it sounds like some frankenstein solution. If you can't get the two subsystems orthogonal for your usecases then rewrite your driver as per above implementing pin control as a back-end to GPIO. 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 Tue, Sep 27, 2016 at 2:26 AM, Stefan Agner <stefan@agner.ch> wrote: > + /* Only change pad configuration if pad is still a GPIO */ > + if (reg & (0x7 << 20)) > + return; > + > + /* Clear IBE/OBE/PUE to disable the pin (Hi-Z) */ > reg &= ~0x7; > writel(reg, ipctl->base + pin_reg->mux_reg); Please #define the magic 7 and 20 above so we see what is going on if you choose this approach. 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
diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c index 4761320..61cfa95 100644 --- a/drivers/pinctrl/freescale/pinctrl-imx.c +++ b/drivers/pinctrl/freescale/pinctrl-imx.c @@ -361,8 +361,13 @@ static void imx_pmx_gpio_disable_free(struct pinctrl_dev *pctldev, if (pin_reg->mux_reg == -1) return; - /* Clear IBE/OBE/PUE to disable the pin (Hi-Z) */ reg = readl(ipctl->base + pin_reg->mux_reg); + + /* Only change pad configuration if pad is still a GPIO */ + if (reg & (0x7 << 20)) + return; + + /* Clear IBE/OBE/PUE to disable the pin (Hi-Z) */ reg &= ~0x7; writel(reg, ipctl->base + pin_reg->mux_reg); }
If a GPIO gets freed after selecting a new pinctrl configuration the driver should not change pinctrl anymore. Otherwise this will likely lead to a unusable pin configuration for the newly selected pinctrl. Signed-off-by: Stefan Agner <stefan@agner.ch> --- This turned out to be problematic when using the I2C GPIO bus recovery functionality. After muxing back to I2C, the GPIO is being freed, which cased I2C to stop working completely. -- Stefan drivers/pinctrl/freescale/pinctrl-imx.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)