[PATCH] ASoC: core: Configure pin muxing via pinctrl when registering a DAI

[PATCH] ASoC: core: Configure pin muxing via pinctrl when registering a DAI

[Peter Ujfalusi]

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:
-- 
1.7.12

Re: [PATCH] ASoC: core: Configure pin muxing via pinctrl when registering a DAI

[Mark Brown]

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.

Re: [PATCH] ASoC: core: Configure pin muxing via pinctrl when registering a DAI

[Peter Ujfalusi]

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.

-- 
Péter

Re: [PATCH] ASoC: core: Configure pin muxing via pinctrl when registering a DAI

[Peter Ujfalusi]

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.
> 


-- 
Péter

Re: [PATCH] ASoC: core: Configure pin muxing via pinctrl when registering a DAI

[Mark Brown]

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.

Re: [PATCH] ASoC: core: Configure pin muxing via pinctrl when registering a DAI

[Stephen Warren]

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.

Re: [PATCH] ASoC: core: Configure pin muxing via pinctrl when registering a DAI

[Mark Brown]

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.

Re: [PATCH] ASoC: core: Configure pin muxing via pinctrl when registering a DAI

[Stephen Warren]

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?

Re: [PATCH] ASoC: core: Configure pin muxing via pinctrl when registering a DAI

[Linus Walleij]

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

Re: [PATCH] ASoC: core: Configure pin muxing via pinctrl when registering a DAI

[Mark Brown]

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).

Re: [PATCH] ASoC: core: Configure pin muxing via pinctrl when registering a DAI

[Linus Walleij]

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

Re: [PATCH] ASoC: core: Configure pin muxing via pinctrl when registering a DAI

[Mark Brown]

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.

Re: [PATCH] ASoC: core: Configure pin muxing via pinctrl when registering a DAI

[Peter Ujfalusi]

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.
> 


-- 
Péter

Re: [PATCH] ASoC: core: Configure pin muxing via pinctrl when registering a DAI

[Linus Walleij]

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

Re: [PATCH] ASoC: core: Configure pin muxing via pinctrl when registering a DAI

[Mark Brown]

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...

Re: [PATCH] ASoC: core: Configure pin muxing via pinctrl when registering a DAI

[Linus Walleij]

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

Re: [PATCH] ASoC: core: Configure pin muxing via pinctrl when registering a DAI

[Mark Brown]

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.

Re: [PATCH] ASoC: core: Configure pin muxing via pinctrl when registering a DAI

[Stephen Warren]

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?

Re: [PATCH] ASoC: core: Configure pin muxing via pinctrl when registering a DAI

[Stephen Warren]

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?

Re: [PATCH] ASoC: core: Configure pin muxing via pinctrl when registering a DAI

[Mark Brown]

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.

Re: [PATCH] ASoC: core: Configure pin muxing via pinctrl when registering a DAI

[Linus Walleij]

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

Re: [PATCH] ASoC: core: Configure pin muxing via pinctrl when registering a DAI

[Mark Brown]

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.

Re: [PATCH] ASoC: core: Configure pin muxing via pinctrl when registering a DAI

[Stephen Warren]

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.

Re: [PATCH] ASoC: core: Configure pin muxing via pinctrl when registering a DAI

[Mark Brown]

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?