[alsa-devel] [PATCH] ASoC: core: Configure pin muxing via pinctrl when registering a DAI
pinctrl framework now becoming widely available for SoCs to configure pin multiplexing. Instead of adding the same code to all dai drivers the core can take care of this transparently. Do not make too much noise if pinctrl is not provided via DT for the dai but just use dev_info().
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com --- Hi Mark, Liam,
I was about to add the pinctrl support for OMAP dais (McBSP/McPDM/DMIC/HDMI) when I realized that I just kept duplicating the same code all over. It is better to leave this to the core.
Regards, Peter
sound/soc/soc-core.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 9a6daf9..37464c5 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -33,6 +33,7 @@ #include <linux/ctype.h> #include <linux/slab.h> #include <linux/of.h> +#include <linux/pinctrl/consumer.h> #include <sound/ac97_codec.h> #include <sound/core.h> #include <sound/jack.h> @@ -3686,6 +3687,19 @@ static inline char *fmt_multiple_name(struct device *dev, return kstrdup(dai_drv->name, GFP_KERNEL); }
+/* + * Simple function to be used in snd_soc_dai_register_dai/s to set the default + * pinmuxing for the dai. + */ +static inline void snd_soc_configure_dai_pins(struct device *dev) +{ + struct pinctrl *pinctrl; + + pinctrl = devm_pinctrl_get_select_default(dev); + if (IS_ERR(pinctrl)) + dev_info(dev, "pins are not configured for the DAI\n"); +} + /** * snd_soc_register_dai - Register a DAI with the ASoC core * @@ -3734,6 +3748,8 @@ int snd_soc_register_dai(struct device *dev,
mutex_unlock(&client_mutex);
+ snd_soc_configure_dai_pins(dev); + pr_debug("Registered DAI '%s'\n", dai->name);
return 0; @@ -3828,6 +3844,8 @@ int snd_soc_register_dais(struct device *dev, pr_debug("Registered DAI '%s'\n", dai->name); }
+ snd_soc_configure_dai_pins(dev); + return 0;
err:
On Fri, Sep 21, 2012 at 10:54:26AM +0300, Peter Ujfalusi wrote:
pinctrl framework now becoming widely available for SoCs to configure pin multiplexing. Instead of adding the same code to all dai drivers the core can take care of this transparently. Do not make too much noise if pinctrl is not provided via DT for the dai but just use dev_info().
Please fix your word wrapping, I've mentioned this to you before...
+/*
- Simple function to be used in snd_soc_dai_register_dai/s to set the default
- pinmuxing for the dai.
I would be more inclined to do this on card init time, doing it at registration seems wrong as systems typically have interfaces that aren't used and share pins with other functions. By the time we are setting up a card we've got a good idea we actually want to use the IP but at probe time that's not the case.
It also seems bad that we're ignoring errors, does the pinctl API not stub itself out well enough.
On 09/21/2012 02:13 PM, Mark Brown wrote:
On Fri, Sep 21, 2012 at 10:54:26AM +0300, Peter Ujfalusi wrote:
pinctrl framework now becoming widely available for SoCs to configure pin multiplexing. Instead of adding the same code to all dai drivers the core can take care of this transparently. Do not make too much noise if pinctrl is not provided via DT for the dai but just use dev_info().
Please fix your word wrapping, I've mentioned this to you before...
I'll reformat it. BTW: The text is edited so it fits in the 'Commit Message' part of git gui.
+/*
- Simple function to be used in snd_soc_dai_register_dai/s to set the default
- pinmuxing for the dai.
I would be more inclined to do this on card init time, doing it at registration seems wrong as systems typically have interfaces that aren't used and share pins with other functions. By the time we are setting up a card we've got a good idea we actually want to use the IP but at probe time that's not the case.
Make sense. My first candidate to do this is the soc_probe_link_dais() function and to call snd_soc_configure_dai_pins() for both the cpu_dai and codec_dai.
It also seems bad that we're ignoring errors, does the pinctl API not stub itself out well enough.
pinctrl_get_select() returns with a pointer to struct pinctrl. If the platform does not have CONFIG_PINCTRL enabled it will return with NULL. If no pinctrl has been specified for the device it will return with error (-ENODEV). Neither of these cases should be considered as error. We do print out with dev_info() to notify the developer, but having pinctrl mux should not be mandatory.
On 09/21/2012 04:16 PM, Peter Ujfalusi wrote:
I would be more inclined to do this on card init time, doing it at registration seems wrong as systems typically have interfaces that aren't used and share pins with other functions. By the time we are setting up a card we've got a good idea we actually want to use the IP but at probe time that's not the case.
Make sense. My first candidate to do this is the soc_probe_link_dais() function and to call snd_soc_configure_dai_pins() for both the cpu_dai and codec_dai.
If we are going to do the pinctrl mux configuration in core we should also consider the machine driver as well. But they tend to request the needed GPIOs in their probe before registering the card. Same can be true for codec drivers where they might need the mux configured before they request the GPIOs they need.
It might be better to leave this to the drivers?
It also seems bad that we're ignoring errors, does the pinctl API not stub itself out well enough.
pinctrl_get_select() returns with a pointer to struct pinctrl. If the platform does not have CONFIG_PINCTRL enabled it will return with NULL. If no pinctrl has been specified for the device it will return with error (-ENODEV). Neither of these cases should be considered as error. We do print out with dev_info() to notify the developer, but having pinctrl mux should not be mandatory.
On Fri, Sep 21, 2012 at 05:55:11PM +0300, Peter Ujfalusi wrote:
You're doing the odd word wrapping here too and it's not a commit message...
If we are going to do the pinctrl mux configuration in core we should also consider the machine driver as well. But they tend to request the needed GPIOs in their probe before registering the card. Same can be true for codec drivers where they might need the mux configured before they request the GPIOs they need.
I would really expect that the GPIO drivers would do the pinmuxing in the same way that other drivers do.
On 09/21/2012 07:16 AM, Peter Ujfalusi wrote:
On 09/21/2012 02:13 PM, Mark Brown wrote:
On Fri, Sep 21, 2012 at 10:54:26AM +0300, Peter Ujfalusi wrote:
pinctrl framework now becoming widely available for SoCs to configure pin multiplexing. Instead of adding the same code to all dai drivers the core can take care of this transparently. Do not make too much noise if pinctrl is not provided via DT for the dai but just use dev_info().
Please fix your word wrapping, I've mentioned this to you before...
I'll reformat it. BTW: The text is edited so it fits in the 'Commit Message' part of git gui.
+/*
- Simple function to be used in snd_soc_dai_register_dai/s to set the default
- pinmuxing for the dai.
I would be more inclined to do this on card init time, doing it at registration seems wrong as systems typically have interfaces that aren't used and share pins with other functions. By the time we are setting up a card we've got a good idea we actually want to use the IP but at probe time that's not the case.
Make sense. My first candidate to do this is the soc_probe_link_dais() function and to call snd_soc_configure_dai_pins() for both the cpu_dai and codec_dai.
It also seems bad that we're ignoring errors, does the pinctl API not stub itself out well enough.
pinctrl_get_select() returns with a pointer to struct pinctrl. If the platform does not have CONFIG_PINCTRL enabled it will return with NULL. If no pinctrl has been specified for the device it will return with error (-ENODEV). Neither of these cases should be considered as error. We do print out with dev_info() to notify the developer, but having pinctrl mux should not be mandatory.
Indeed - what about a platform like Tegra which has pinctrl enabled, yet doesn't specify any pinctrl configuration for any device other than the pin controller itself? Do we want to force every device into having to specify an empty pin control configuration? If we take this route, we should really have the driver core or platform bus set up the initial pinctrl state, and I believe that approach was explicitly decided against.
On Fri, Sep 21, 2012 at 10:23:50AM -0600, Stephen Warren wrote:
On 09/21/2012 07:16 AM, Peter Ujfalusi wrote:
pinctrl_get_select() returns with a pointer to struct pinctrl. If the platform does not have CONFIG_PINCTRL enabled it will return with NULL. If no pinctrl has been specified for the device it will return with error (-ENODEV). Neither of these cases should be considered as error. We do print out with dev_info() to notify the developer, but having pinctrl mux should not be mandatory.
Indeed - what about a platform like Tegra which has pinctrl enabled, yet doesn't specify any pinctrl configuration for any device other than the pin controller itself? Do we want to force every device into having to specify an empty pin control configuration? If we take this route, we should really have the driver core or platform bus set up the initial pinctrl state, and I believe that approach was explicitly decided against.
Well, the other options are for pinctrl to either not return an error in the no configuration case or change to a void return type. The current behaviour seems a bit pointless since we can't usefully do anything with the errors, we can just factor the error logs into the core and have done with it since it's not possible to treat errors as such.
On 09/22/2012 09:28 AM, Mark Brown wrote:
On Fri, Sep 21, 2012 at 10:23:50AM -0600, Stephen Warren wrote:
On 09/21/2012 07:16 AM, Peter Ujfalusi wrote:
pinctrl_get_select() returns with a pointer to struct pinctrl. If the platform does not have CONFIG_PINCTRL enabled it will return with NULL. If no pinctrl has been specified for the device it will return with error (-ENODEV). Neither of these cases should be considered as error. We do print out with dev_info() to notify the developer, but having pinctrl mux should not be mandatory.
Indeed - what about a platform like Tegra which has pinctrl enabled, yet doesn't specify any pinctrl configuration for any device other than the pin controller itself? Do we want to force every device into having to specify an empty pin control configuration? If we take this route, we should really have the driver core or platform bus set up the initial pinctrl state, and I believe that approach was explicitly decided against.
Well, the other options are for pinctrl to either not return an error in the no configuration case or change to a void return type. The current behaviour seems a bit pointless since we can't usefully do anything with the errors, we can just factor the error logs into the core and have done with it since it's not possible to treat errors as such.
(I'm not 100% sure if everyone agrees on this and I'm not sure if it's been explicitly stated, so perhaps this is my personal opinion. However, the more I think about this, the more it makes sense).
I think the errors shouldn't be ignored at all; it's an error for a driver to explicitly go out and request a pinctrl state if that state is not defined. In turn, this means that no generic code should be going out and requesting a pinctrl state on behalf of the driver; it has no idea if the driver has defined that it needs pinctrl set up for it.
I'll explain this in a slightly different way that will probably make more sense.
For any pinctrl configuration that is static, that configuration should be applied one time as the system boots using the "hog" feature of the pin controller itself, i.e. the pin controller driver should apply that state during its own probe(). Individual drivers should only ever request a pinctrl state if they need to dynamically switch between different states at run-time. This is (a) likely to be fairly rare, and (b) the need is likely to be defined directly by the kind of protocol that the driver/HW is implementing.
So in other words, I'd expect to see an almost complete system-wide "default" pinctrl state defined for the pin controller, and quite possibly no other drivers with pinctrl states defined at all.
Drivers that might switch between pinctrl states are say I2C/SPI/MDIO/... bus muxes that use an SoC's pinctrl module to perform the muxing, or perhaps some kind of high-speed data bus that might need different pin configuration (say biasing/equalization/... stuff rather than muxing) based on the runtime-selected bus clock.
Given all this, that probably means that none of your (Peter's) DAI drivers need to touch pinctrl at all, so this patch isn't needed in any form?
On Sun, Sep 23, 2012 at 5:23 AM, Stephen Warren swarren@wwwdotorg.org wrote:
I think the errors shouldn't be ignored at all; it's an error for a driver to explicitly go out and request a pinctrl state if that state is not defined. In turn, this means that no generic code should be going out and requesting a pinctrl state on behalf of the driver; it has no idea if the driver has defined that it needs pinctrl set up for it.
This makes perfect sense to me, right now atleast.
So the problem with the patch we're discussing is that it's "too far up" in the framework, and handling pinctrl should be done somewhere below.
For any pinctrl configuration that is static, that configuration should be applied one time as the system boots using the "hog" feature of the pin controller itself,
Agreed. Even in the docs luckily :-)
i.e. the pin controller driver should apply that state during its own probe(). Individual drivers should only ever request a pinctrl state if they need to dynamically switch between different states at run-time. This is (a) likely to be fairly rare,
The Ux500 smash this completely by having almost no static configurations at all and needing to switch everything at runtime, tied in with runtime PM. This is how we save some microamps.
Sorry for this, I'm happy for you if Tegra is simpler to handle for low-power :-)
and (b) the need is likely to be defined directly by the kind of protocol that the driver/HW is implementing.
Yes. This is 100% true. It needs to be close to the driver and its subsystem. (I don't know if the patch under disussion is at the right level, seems controversial...)
So in other words, I'd expect to see an almost complete system-wide "default" pinctrl state defined for the pin controller, and quite possibly no other drivers with pinctrl states defined at all.
Hehe not for us :-)
Yours, Linus Walleij
On Mon, Sep 24, 2012 at 11:20:16AM +0200, Linus Walleij wrote:
On Sun, Sep 23, 2012 at 5:23 AM, Stephen Warren swarren@wwwdotorg.org wrote:
I think the errors shouldn't be ignored at all; it's an error for a driver to explicitly go out and request a pinctrl state if that state is not defined. In turn, this means that no generic code should be going out and requesting a pinctrl state on behalf of the driver; it has no idea if the driver has defined that it needs pinctrl set up for it.
This makes perfect sense to me, right now atleast.
So the problem with the patch we're discussing is that it's "too far up" in the framework, and handling pinctrl should be done somewhere below.
Well, the problem here is that people keep wanting to add one shot pinctrl calls in drivers which clearly suggests that it ought to be factored out.
For any pinctrl configuration that is static, that configuration should be applied one time as the system boots using the "hog" feature of the pin controller itself,
Agreed. Even in the docs luckily :-)
Which is one way of factoring out, though it doesn't seem to be universally applied (and I don't immediately see how it scales when the device is used in multiple SoCs some of which need runtime configuration and some of which don't).
On Mon, Sep 24, 2012 at 12:17 PM, Mark Brown broonie@opensource.wolfsonmicro.com wrote:
On Mon, Sep 24, 2012 at 11:20:16AM +0200, Linus Walleij wrote:
So the problem with the patch we're discussing is that it's "too far up" in the framework, and handling pinctrl should be done somewhere below.
Well, the problem here is that people keep wanting to add one shot pinctrl calls in drivers which clearly suggests that it ought to be factored out.
OK so can we find some suitable middle ground here?
Something like drivers could add some boolean flag to "opt-in" for the core to handle this or so?
After all only some MMC drivers call up to mmc_regulator_set_ocr() to set voltages, it's optional but centralized.
I'm not sure I'm familiar enough with ASoC to have a good suggestion on how to do this though...
Yours, Linus Walleij
On Mon, Sep 24, 2012 at 04:37:23PM +0200, Linus Walleij wrote:
On Mon, Sep 24, 2012 at 12:17 PM, Mark Brown
Well, the problem here is that people keep wanting to add one shot pinctrl calls in drivers which clearly suggests that it ought to be factored out.
OK so can we find some suitable middle ground here?
Something like drivers could add some boolean flag to "opt-in" for the core to handle this or so?
Well, I don't know if we need to - it sounds like the platforms that are running around adding these pinctrl calls to all the drivers are doing it wrong and should be fixed.
After all only some MMC drivers call up to mmc_regulator_set_ocr() to set voltages, it's optional but centralized.
This is mostly a historical thing, though - pinctrl is new.
On 09/25/2012 02:11 PM, Mark Brown wrote:
On Mon, Sep 24, 2012 at 04:37:23PM +0200, Linus Walleij wrote:
On Mon, Sep 24, 2012 at 12:17 PM, Mark Brown
Well, the problem here is that people keep wanting to add one shot pinctrl calls in drivers which clearly suggests that it ought to be factored out.
OK so can we find some suitable middle ground here?
Something like drivers could add some boolean flag to "opt-in" for the core to handle this or so?
Well, I don't know if we need to - it sounds like the platforms that are running around adding these pinctrl calls to all the drivers are doing it wrong and should be fixed.
In the scope of audio... I guess it is one thing to use the pinctrl framework to configure the mux for the pins (which was the original idea of the patch). This should be taken care in some centralized place, I agree. But the pinctrl could be used to change the mux runtime. For example in OMAP for most of the audio related pins we need to select MODE0 mux configuration in order to route the signal to the pin. However the same pin can be configured to SAFE_MODE which disconnects the pin from outside. During audio activity we obviously need to configure these pins to MODE0 but when there is not activity we could set them to SAFE_MODE which could result some power savings (preventing leakage, etc).
For this to work it would be ideal to use the pm_runtime as a centralized place to handle clocks, pinctrl, etc... But again, what to do with the cases when this is not needed (the pinctrl part for the devices)?
After all only some MMC drivers call up to mmc_regulator_set_ocr() to set voltages, it's optional but centralized.
This is mostly a historical thing, though - pinctrl is new.
NOTE: can someone relay me the original patch in this thread or cross-post it to linux-kernel since that is where it obviously needs to be discussed? I'm not on alsa-devel... (OK I will subscribe...)
On Tue, Sep 25, 2012 at 1:24 PM, Peter Ujfalusi peter.ujfalusi@ti.com wrote:
But the pinctrl could be used to change the mux runtime. For example in OMAP for most of the audio related pins we need to select MODE0 mux configuration in order to route the signal to the pin. However the same pin can be configured to SAFE_MODE which disconnects the pin from outside. During audio activity we obviously need to configure these pins to MODE0 but when there is not activity we could set them to SAFE_MODE which could result some power savings (preventing leakage, etc).
For this to work it would be ideal to use the pm_runtime as a centralized place to handle clocks, pinctrl, etc...
So this is exactly what we are doing for the ux500 and implementing in most of the AMBA devs.
We even have standard names for it in the pinctrl subsystem - the modes named "default" and "sleep" or maybe "idle" if you also have a separate "sleep" state apart from the relaxed state. (include/linux/pinctrl/pinctrl-state.h).
So we go into "default" when the device is in use and relax it by going into "sleep" whenever we can.
But again, what to do with the cases when this is not needed (the pinctrl part for the devices)?
This is too unspecific for me, sorry... are you thinking about Stephen's preference to do this using hogs or of systems not using pinctrl at all? (They should be safe since not configuring pinctrl means it gets stubbed out.)
Yours, Linus Walleij
On Tue, Sep 25, 2012 at 02:24:23PM +0300, Peter Ujfalusi wrote:
During audio activity we obviously need to configure these pins to MODE0 but when there is not activity we could set them to SAFE_MODE which could result some power savings (preventing leakage, etc).
Right, this is Linus' power saving case. It's going to be applicable in some way to most devices, though the benefits will vary quite a bit.
For this to work it would be ideal to use the pm_runtime as a centralized place to handle clocks, pinctrl, etc... But again, what to do with the cases when this is not needed (the pinctrl part for the devices)?
It's also not massively clear to me that generic pm_runtime code is a good place to put all this stuff - it's too coarse grained for a lot of things. Devices that can be wake sources are an example here, if they are doing that then turning them completely off may break functionality but you can still turn some of the device (possibly a varying selection depending on usage) off. For simple cases it works well and it'd be good to have it but you end up back into ignoring errors...
On Tue, Sep 25, 2012 at 1:43 PM, Mark Brown broonie@opensource.wolfsonmicro.com wrote:
On Tue, Sep 25, 2012 at 02:24:23PM +0300, Peter Ujfalusi wrote:
For this to work it would be ideal to use the pm_runtime as a centralized place to handle clocks, pinctrl, etc... But again, what to do with the cases when this is not needed (the pinctrl part for the devices)?
It's also not massively clear to me that generic pm_runtime code is a good place to put all this stuff - it's too coarse grained for a lot of things.
I tend to agree. If you think of things like the clock handling in drivers/base/power/clock_ops.c that shmobile is relying upon. This is just not working with our platforms because different drivers need different semantics.
In ux500 drivers we use *specific* (as opposed to generic) runtime PM code to e.g. get some hysteresis before a drivers clock and pins (etc) are put to sleep. Pure driver business.
So these are runtime suspend/resume calls in the driver itself.
Devices that can be wake sources are an example here, if they are doing that then turning them completely off may break functionality but you can still turn some of the device (possibly a varying selection depending on usage) off. For simple cases it works well and it'd be good to have it but you end up back into ignoring errors...
Our ux500 "sleep" state on the pins will put them into the apropriate wakeup mode where applicable. Since this may vary across boards it's even machine-specific.
Configuring wakeup sources when going to a state named "sleep" seems intuitive to me atleast.
Then we have this (currently unused) "idle" state, which is intended as a power-saving state without going to sleep. So this is for the case when the system needs to be relaxed on power but not yet put to sleep. On some systems and for some drivers this state and the "sleep" state will likely be the same, but I'm just philosophizing here.
These are just defined as strings in include/linux/pinctrl/pinctrl-state.h with some kind of semantics.
The pinctrl handle can be moved into any state at any time, it's not like it's a state machine or something, so the subsystems and drivers need to know what they are doing.
Yours, Linus Walleij
On Tue, Sep 25, 2012 at 01:56:13PM +0200, Linus Walleij wrote:
In ux500 drivers we use *specific* (as opposed to generic) runtime PM code to e.g. get some hysteresis before a drivers clock and pins (etc) are put to sleep. Pure driver business.
Right, the runtime PM infrastructure is really useful for this stuff.
Devices that can be wake sources are an example here, if they are doing that then turning them completely off may break functionality but you can still turn some of the device (possibly a varying selection depending on usage) off. For simple cases it works well and it'd be good to have it but you end up back into ignoring errors...
Our ux500 "sleep" state on the pins will put them into the apropriate wakeup mode where applicable. Since this may vary across boards it's even machine-specific.
Configuring wakeup sources when going to a state named "sleep" seems intuitive to me atleast.
For some devices the set of wakeup sources vary depending on the current system state - for example, if a cable is plugged into a connector we may have to keep some additional things on to monitor activity on the cable. This isn't the common case, for most things a sleep state works fine, but it does come up.
On 09/25/2012 05:24 AM, Peter Ujfalusi wrote:
On 09/25/2012 02:11 PM, Mark Brown wrote:
On Mon, Sep 24, 2012 at 04:37:23PM +0200, Linus Walleij wrote:
On Mon, Sep 24, 2012 at 12:17 PM, Mark Brown
Well, the problem here is that people keep wanting to add one shot pinctrl calls in drivers which clearly suggests that it ought to be factored out.
OK so can we find some suitable middle ground here?
Something like drivers could add some boolean flag to "opt-in" for the core to handle this or so?
Well, I don't know if we need to - it sounds like the platforms that are running around adding these pinctrl calls to all the drivers are doing it wrong and should be fixed.
In the scope of audio... I guess it is one thing to use the pinctrl framework to configure the mux for the pins (which was the original idea of the patch). This should be taken care in some centralized place, I agree. But the pinctrl could be used to change the mux runtime. For example in OMAP for most of the audio related pins we need to select MODE0 mux configuration in order to route the signal to the pin. However the same pin can be configured to SAFE_MODE which disconnects the pin from outside. During audio activity we obviously need to configure these pins to MODE0 but when there is not activity we could set them to SAFE_MODE which could result some power savings (preventing leakage, etc).
For this to work it would be ideal to use the pm_runtime as a centralized place to handle clocks, pinctrl, etc... But again, what to do with the cases when this is not needed (the pinctrl part for the devices)?
Maybe it can be opt-in, like pm_runtime support at all, e.g. a driver that supports it can:
pm_runtime_enable_pinctrl(dev, pinctrl_state_name);
where if pinctrl_state_name==NULL, it uses PINCTRL_STATE_IDLE. (Making this a parameter would allow for devices that use a more complex set of states where the default state names may not be appropriate). Perhaps we could phase that in if/when needed, by introducing pm_runtime_enable_pinctrl(dev) first, then adding pm_runtime_enable_pinctrl_state(dev, state) later?
Of course, I suppose that only really addresses the case where the IP/driver itself knows if/when to enable this feature - if an I2S IP block is used in 10 different SoCs, and runtime pinctrl is only useful on 5 of them, how does it know when to make that call?
On 09/24/2012 04:17 AM, Mark Brown wrote:
On Mon, Sep 24, 2012 at 11:20:16AM +0200, Linus Walleij wrote:
On Sun, Sep 23, 2012 at 5:23 AM, Stephen Warren swarren@wwwdotorg.org wrote:
I think the errors shouldn't be ignored at all; it's an error for a driver to explicitly go out and request a pinctrl state if that state is not defined. In turn, this means that no generic code should be going out and requesting a pinctrl state on behalf of the driver; it has no idea if the driver has defined that it needs pinctrl set up for it.
This makes perfect sense to me, right now atleast.
So the problem with the patch we're discussing is that it's "too far up" in the framework, and handling pinctrl should be done somewhere below.
Well, the problem here is that people keep wanting to add one shot pinctrl calls in drivers which clearly suggests that it ought to be factored out.
For any pinctrl configuration that is static, that configuration should be applied one time as the system boots using the "hog" feature of the pin controller itself,
Agreed. Even in the docs luckily :-)
Which is one way of factoring out, though it doesn't seem to be universally applied (and I don't immediately see how it scales when the device is used in multiple SoCs some of which need runtime configuration and some of which don't).
Anywhere that particular device is used needs to have a pinctrl state defined. On SoCs that need the pinctrl changed, the pinctrl state would be populated with meaningful data. On SoCs that don't need pinctrl changed, the pinctrl state would be empty; essentially a dummy just to represent the fact that someone thought about the issue and decided nothing was needed.
But I still find it unlikely that this situation will occur; surely there's some specific reason directly related to the device itself, or the protocol it implements, that defines whether it needs dynamic pinctrl, and hence no matter what SoC that device is inserted into, it will either need or not-need dynamic pinctrl. Are there any extant examples to refute this?
On Mon, Sep 24, 2012 at 09:41:22AM -0600, Stephen Warren wrote:
But I still find it unlikely that this situation will occur; surely there's some specific reason directly related to the device itself, or the protocol it implements, that defines whether it needs dynamic pinctrl, and hence no matter what SoC that device is inserted into, it will either need or not-need dynamic pinctrl. Are there any extant examples to refute this?
Linus' example of adjusting the pinctrl state to relax power usage is an obvious one, and one that should be implementable on almost any system. It's really a function of the SoC integration rather than of the device itself (the IP isn't accomplishing anything for itself, it's allowing the SoC to save some power when it doesn't need to do anything with the hardware. Essentially any IP could implement this, but the gains from doing it are likely to vary with SoC.
On Fri, Sep 21, 2012 at 6:23 PM, Stephen Warren swarren@wwwdotorg.org wrote:
On 09/21/2012 07:16 AM, Peter Ujfalusi wrote:
pinctrl_get_select() returns with a pointer to struct pinctrl. If the platform does not have CONFIG_PINCTRL enabled it will return with NULL. If no pinctrl has been specified for the device it will return with error (-ENODEV). Neither of these cases should be considered as error. We do print out with dev_info() to notify the developer, but having pinctrl mux should not be mandatory.
Indeed - what about a platform like Tegra which has pinctrl enabled, yet doesn't specify any pinctrl configuration for any device other than the pin controller itself?
It would be fixed by calling pinctrl_provide_dummies() right?
Not that it's elegant or anything ...
What it all boils down to in the end (IMO) is whether a centralized or decentralized approach to the control handles should be used. The same problem will arise with runtime PM I think.
For example the shmobile will get and gate all it's clocks in a centralized fashion in drivers/base/power/clock_ops.c, and this doesn't work with all other systems so in other drivers you find clk_prepare/enable/disable/unprepare distributed in the drivers.
(I think someone could attempt to do the same thing with regulators with the same centralization failure, we know regulator voltages need to be switched at runtime, take an SD card and centralized regulators breaks apart immediately.)
Some time I thought the level of centralization may be dependent on actual hardware architecture, that the centralized approach would work for some. I'm not so sure anymore...
For advanced pin configuration, where we (in ux500) need to switch pins from default to sleep state at runtime to save power, it doesn't work do do it with hogs, we need to distribute it, because the pinctrl driver doesn't know when the UART wants to go to sleep (which is not always the same as when the system as a whole wants to sleep, incidentally) so in this case the decentralized approach is the only thing that works. We are now merging the same approach to our SPI driver, and plan to continue with I2C etc. We really want to relax the pins for these blocks between individual transfers, sometimes with delayed autosuspend via runtime PM to avoid jittering.
I don't know if this helps anything for this ASoC case though :-/
Yours, Linus Walleij
On Mon, Sep 24, 2012 at 10:34:50AM +0200, Linus Walleij wrote:
(I think someone could attempt to do the same thing with regulators with the same centralization failure, we know regulator voltages need to be switched at runtime, take an SD card and centralized regulators breaks apart immediately.)
It's also not terribly tractable because idle states don't mean power off, between retention and wake sources there's far too many execeptions.
On 09/24/2012 02:34 AM, Linus Walleij wrote:
On Fri, Sep 21, 2012 at 6:23 PM, Stephen Warren swarren@wwwdotorg.org wrote:
On 09/21/2012 07:16 AM, Peter Ujfalusi wrote:
pinctrl_get_select() returns with a pointer to struct pinctrl. If the platform does not have CONFIG_PINCTRL enabled it will return with NULL. If no pinctrl has been specified for the device it will return with error (-ENODEV). Neither of these cases should be considered as error. We do print out with dev_info() to notify the developer, but having pinctrl mux should not be mandatory.
Indeed - what about a platform like Tegra which has pinctrl enabled, yet doesn't specify any pinctrl configuration for any device other than the pin controller itself?
It would be fixed by calling pinctrl_provide_dummies() right?
Not that it's elegant or anything ...
Right - calling pinctrl_provide_dummies() ends up telling the pinctrl core never to return an error even when it "should". I don't think it's appropriate to use that API at all except to help out transitioning a particular SoC to use pinctrl.
On Fri, Sep 21, 2012 at 04:16:26PM +0300, Peter Ujfalusi wrote:
On 09/21/2012 02:13 PM, Mark Brown wrote:
On Fri, Sep 21, 2012 at 10:54:26AM +0300, Peter Ujfalusi wrote:
pinctrl framework now becoming widely available for SoCs to configure pin multiplexing. Instead of adding the same code to all dai drivers the core can take care of this transparently. Do not make too much noise if pinctrl is not provided via DT for the dai but just use dev_info().
Please fix your word wrapping, I've mentioned this to you before...
I'll reformat it. BTW: The text is edited so it fits in the 'Commit Message' part of git gui.
The line length is perfectly fine, it's the fact that you appear to start each sentence on a new line so we end up with line lengths alternating between normal and extremely short within paragraphs - just look at the above. It's doing absolutely nothing for legibility and it's a constant thing.
It also seems bad that we're ignoring errors, does the pinctl API not stub itself out well enough.
pinctrl_get_select() returns with a pointer to struct pinctrl. If the platform does not have CONFIG_PINCTRL enabled it will return with NULL. If no pinctrl has been specified for the device it will return with error (-ENODEV). Neither of these cases should be considered as error. We do print out with dev_info() to notify the developer, but having pinctrl mux should not be mandatory.
So then why is pinctrl returning errors in the first place if the sensible thing to do is just ignore them?
participants (4)
-
Linus Walleij -
Mark Brown -
Peter Ujfalusi -
Stephen Warren