[PATCH 0/6] Second set of fixes for ux500 ASoC drivers

[PATCH 0/6] Second set of fixes for ux500 ASoC drivers

[Fabio Baltieri]

Hi,

These are some others fixes for various small issues I found while
testing the ux500 ASoC driver on -next.

Patch 1 adds some missing declarations for AD controls that were causing
some weird behaviour in alsamixer, as the default state was outside the
declared range.

Patch 2 fixes a kernel crash when opening and closing the audio device
without sending any data.

Patch 3 drops pinctrl code altogether from the driver.  The actual
implementation is buggy as the pins are only registered to the playback
interfaces, which gives a bunch of warnings during kernel startup and
also kills the capture interface by setting the shared pins to hi-z mode
even if that's still active.  As putting those pins in high-z is not
really needed and was removed from the internal STE driver anyway, I'm
just dropping that code form here as well.  In parallel, I'm sending a
pinctrl patch to declare those pin as a hog.

Patches 4 to 6 fixes some weirdness with time slot usage.  After this
series the driver seems to work fine for both capture and playback
interface (tested on a snowball v11).

Thanks,
Fabio


Fabio Baltieri (6):
  ASoC: ab8500-codec: Add missing ad_to_slot definitions
  ASoC: ux500: Do not clear state if already idle
  ASoC: ux500: Drop pinctrl sleep support
  ASoC: ux500: Update tx tdm slots configuration
  ASoC: ux500: Swap even/odd AD slot definitions
  ASoC: ux500: Use the first two AD slots for capture

 sound/soc/codecs/ab8500-codec.c | 39 +++++++++++++++------------
 sound/soc/codecs/ab8500-codec.h | 36 ++++++++++++-------------
 sound/soc/ux500/mop500_ab8500.c |  4 +--
 sound/soc/ux500/ux500_msp_i2s.c | 58 +++--------------------------------------
 sound/soc/ux500/ux500_msp_i2s.h |  6 -----
 5 files changed, 46 insertions(+), 97 deletions(-)

-- 
1.8.2

[PATCH 1/6] ASoC: ab8500-codec: Add missing ad_to_slot definitions

[Fabio Baltieri]

According to the AB8500 user manual AD to Slot register multiplexer
accept values from 0 to 15 where:

   0 to  7 corresponds to AD_OUTx slots
   8 to 11 corresponds to zero output
  12 to 15 sets the output in tristate mode

Update enum_ad_to_slot_map array to reflect this definition.

This also allows alsamixer to properly display the default
configuration, as all controls are set to tristate (=12) at reset.

Signed-off-by: Fabio Baltieri <fabio.baltieri@linaro.org>
---
 sound/soc/codecs/ab8500-codec.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/sound/soc/codecs/ab8500-codec.c b/sound/soc/codecs/ab8500-codec.c
index a153b16..3126cac 100644
--- a/sound/soc/codecs/ab8500-codec.c
+++ b/sound/soc/codecs/ab8500-codec.c
@@ -1496,6 +1496,12 @@ static const char * const enum_ad_to_slot_map[] = {"AD_OUT1",
 					"AD_OUT7",
 					"AD_OUT8",
 					"zeroes",
+					"zeroes",
+					"zeroes",
+					"zeroes",
+					"tristate",
+					"tristate",
+					"tristate",
 					"tristate"};
 static SOC_ENUM_SINGLE_DECL(soc_enum_adslot0map,
 			AB8500_ADSLOTSEL1, AB8500_ADSLOTSELX_EVEN_SHIFT,
-- 
1.8.2

Re: [PATCH 1/6] ASoC: ab8500-codec: Add missing ad_to_slot definitions

[Lee Jones]

I'm sure this is just me, but:

>    0 to  7 corresponds to AD_OUTx slots
> char * const enum_ad_to_slot_map[] = {"AD_OUT1",    0
                                                      1..5 ???
>  					"AD_OUT7",    6
>  					"AD_OUT8",    7
>    8 to 11 corresponds to zero output
>  					"zeroes",     8
> +					"zeroes",     9
> +					"zeroes",     10
> +					"zeroes",     11
>   12 to 15 sets the output in tristate mode
> +					"tristate",   12
> +					"tristate",   13
> +					"tristate",   14
>  					"tristate"};  15


-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/6] ASoC: ab8500-codec: Add missing ad_to_slot definitions

[Fabio Baltieri]

On Wed, May 08, 2013 at 08:53:28AM +0100, Lee Jones wrote:
> I'm sure this is just me, but:
> 
> >    0 to  7 corresponds to AD_OUTx slots
> > char * const enum_ad_to_slot_map[] = {"AD_OUT1",    0
>                                                       1..5 ???
> >  					"AD_OUT7",    6
> >  					"AD_OUT8",    7

I'm a bit confused... the first one was from the diff header, while OUT7 and
OUT8 are context lines, so 1 to 5 are already there but not shown in the
patch.  The final array is like:

static const char * const enum_ad_to_slot_map[] = {"AD_OUT1",
                                        "AD_OUT2",
                                        "AD_OUT3",
                                        "AD_OUT4",
                                        "AD_OUT5",
                                        "AD_OUT6",
                                        "AD_OUT7",
                                        "AD_OUT8",
                                        "zeroes",
                                        "zeroes",
                                        "zeroes",
                                        "zeroes",
                                        "tristate",
                                        "tristate",
                                        "tristate",
                                        "tristate"};

Is it ok?

Fabio

> >    8 to 11 corresponds to zero output
> >  					"zeroes",     8
> > +					"zeroes",     9
> > +					"zeroes",     10
> > +					"zeroes",     11
> >   12 to 15 sets the output in tristate mode
> > +					"tristate",   12
> > +					"tristate",   13
> > +					"tristate",   14
> >  					"tristate"};  15
> 
> 
> -- 
> Lee Jones
> Linaro ST-Ericsson Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

-- 
Fabio Baltieri

Re: [PATCH 1/6] ASoC: ab8500-codec: Add missing ad_to_slot definitions

[Lee Jones]

On Wed, 08 May 2013, Fabio Baltieri wrote:

> On Wed, May 08, 2013 at 08:53:28AM +0100, Lee Jones wrote:
> > I'm sure this is just me, but:
> > 
> > >    0 to  7 corresponds to AD_OUTx slots
> > > char * const enum_ad_to_slot_map[] = {"AD_OUT1",    0
> >                                                       1..5 ???
> > >  					"AD_OUT7",    6
> > >  					"AD_OUT8",    7
> 
> I'm a bit confused... the first one was from the diff header, while OUT7 and
> OUT8 are context lines, so 1 to 5 are already there but not shown in the
> patch.  The final array is like:
> 
> static const char * const enum_ad_to_slot_map[] = {"AD_OUT1",
>                                         "AD_OUT2",
>                                         "AD_OUT3",
>                                         "AD_OUT4",
>                                         "AD_OUT5",
>                                         "AD_OUT6",
>                                         "AD_OUT7",
>                                         "AD_OUT8",
>                                         "zeroes",
>                                         "zeroes",
>                                         "zeroes",
>                                         "zeroes",
>                                         "tristate",
>                                         "tristate",
>                                         "tristate",
>                                         "tristate"};
> 
> Is it ok?

Yes, I see that now.

Acked-by: Lee Jones <lee.jones@linaro.org>

> > >    8 to 11 corresponds to zero output
> > >  					"zeroes",     8
> > > +					"zeroes",     9
> > > +					"zeroes",     10
> > > +					"zeroes",     11
> > >   12 to 15 sets the output in tristate mode
> > > +					"tristate",   12
> > > +					"tristate",   13
> > > +					"tristate",   14
> > >  					"tristate"};  15
> > 
> > 
> 

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/6] ASoC: ab8500-codec: Add missing ad_to_slot definitions

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 188 bytes --]

On Wed, May 08, 2013 at 09:14:16AM +0200, Fabio Baltieri wrote:
> According to the AB8500 user manual AD to Slot register multiplexer
> accept values from 0 to 15 where:

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 2/6] ASoC: ux500: Do not clear state if already idle

[Fabio Baltieri]

As enable_msp gets called only after some audio data has been received,
if the userspace closes the device before sending any data it causes
ux500_msp_i2s_close to clear device state even if it was not previously
initialized.

This in turns leads to some non necessary but harmless writel, but also
to decrementing the pinctrl usage counter (pinctrl_rxtx_ref) below zero.

To prevent this from happening add a condition to skip register and
pinctrl clear if current msp state is already MSP_STATE_IDLE.

Signed-off-by: Fabio Baltieri <fabio.baltieri@linaro.org>
---
 sound/soc/ux500/ux500_msp_i2s.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/ux500/ux500_msp_i2s.c b/sound/soc/ux500/ux500_msp_i2s.c
index a26c6bf..964cfd6 100644
--- a/sound/soc/ux500/ux500_msp_i2s.c
+++ b/sound/soc/ux500/ux500_msp_i2s.c
@@ -638,7 +638,7 @@ int ux500_msp_i2s_close(struct ux500_msp *msp, unsigned int dir)
 	dev_dbg(msp->dev, "%s: Enter (dir = 0x%01x).\n", __func__, dir);
 
 	status = disable_msp(msp, dir);
-	if (msp->dir_busy == 0) {
+	if (msp->dir_busy == 0 && msp->msp_state != MSP_STATE_IDLE) {
 		/* disable sample rate and frame generators */
 		msp->msp_state = MSP_STATE_IDLE;
 		writel((readl(msp->registers + MSP_GCR) &
-- 
1.8.2

Re: [PATCH 2/6] ASoC: ux500: Do not clear state if already idle

[Lee Jones]

On Wed, 08 May 2013, Fabio Baltieri wrote:

> As enable_msp gets called only after some audio data has been received,
> if the userspace closes the device before sending any data it causes
> ux500_msp_i2s_close to clear device state even if it was not previously
> initialized.
> 
> This in turns leads to some non necessary but harmless writel, but also
   Singular ^

> to decrementing the pinctrl usage counter (pinctrl_rxtx_ref) below zero.
> 
> To prevent this from happening add a condition to skip register and
> pinctrl clear if current msp state is already MSP_STATE_IDLE.
> 
> Signed-off-by: Fabio Baltieri <fabio.baltieri@linaro.org>
> ---
>  sound/soc/ux500/ux500_msp_i2s.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sound/soc/ux500/ux500_msp_i2s.c b/sound/soc/ux500/ux500_msp_i2s.c
> index a26c6bf..964cfd6 100644
> --- a/sound/soc/ux500/ux500_msp_i2s.c
> +++ b/sound/soc/ux500/ux500_msp_i2s.c
> @@ -638,7 +638,7 @@ int ux500_msp_i2s_close(struct ux500_msp *msp, unsigned int dir)
>  	dev_dbg(msp->dev, "%s: Enter (dir = 0x%01x).\n", __func__, dir);
>  
>  	status = disable_msp(msp, dir);
> -	if (msp->dir_busy == 0) {
> +	if (msp->dir_busy == 0 && msp->msp_state != MSP_STATE_IDLE) {
>  		/* disable sample rate and frame generators */
>  		msp->msp_state = MSP_STATE_IDLE;
>  		writel((readl(msp->registers + MSP_GCR) &

Code looks good though:

Acked-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v2 2/6] ASoC: ux500: Do not clear state if already idle

[Fabio Baltieri]

As enable_msp gets called only after some audio data has been received,
if the userspace closes the device before sending any data it causes
ux500_msp_i2s_close to clear device state even if it was not previously
initialized.

This in turn leads to some non necessary but harmless writel, but also
to decrementing the pinctrl usage counter (pinctrl_rxtx_ref) below zero.

To prevent this from happening add a condition to skip register and
pinctrl clear if current msp state is already MSP_STATE_IDLE.

Acked-by: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Fabio Baltieri <fabio.baltieri@linaro.org>
---

Changes from v1:
- fix grammatical error in commit message

Thanks,
Fabio

 sound/soc/ux500/ux500_msp_i2s.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/ux500/ux500_msp_i2s.c b/sound/soc/ux500/ux500_msp_i2s.c
index a26c6bf..964cfd6 100644
--- a/sound/soc/ux500/ux500_msp_i2s.c
+++ b/sound/soc/ux500/ux500_msp_i2s.c
@@ -638,7 +638,7 @@ int ux500_msp_i2s_close(struct ux500_msp *msp, unsigned int dir)
 	dev_dbg(msp->dev, "%s: Enter (dir = 0x%01x).\n", __func__, dir);
 
 	status = disable_msp(msp, dir);
-	if (msp->dir_busy == 0) {
+	if (msp->dir_busy == 0 && msp->msp_state != MSP_STATE_IDLE) {
 		/* disable sample rate and frame generators */
 		msp->msp_state = MSP_STATE_IDLE;
 		writel((readl(msp->registers + MSP_GCR) &
-- 
1.8.2

Re: [PATCH v2 2/6] ASoC: ux500: Do not clear state if already idle

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 560 bytes --]

On Wed, May 08, 2013 at 10:39:14AM +0200, Fabio Baltieri wrote:
> As enable_msp gets called only after some audio data has been received,
> if the userspace closes the device before sending any data it causes
> ux500_msp_i2s_close to clear device state even if it was not previously
> initialized.

Ugh, please don't do stuff like this - you're posting an individual
revision of a patch buried in the middle of a thread.  This just makes
things hard to follow and error prone.  Repost the patch series or wait
until what can be applied is applied then repost.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 2/6] ASoC: ux500: Do not clear state if already idle

[Lee Jones]

On Wed, 08 May 2013, Mark Brown wrote:

> On Wed, May 08, 2013 at 10:39:14AM +0200, Fabio Baltieri wrote:
> > As enable_msp gets called only after some audio data has been received,
> > if the userspace closes the device before sending any data it causes
> > ux500_msp_i2s_close to clear device state even if it was not previously
> > initialized.
> 
> Ugh, please don't do stuff like this - you're posting an individual
> revision of a patch buried in the middle of a thread.  This just makes
> things hard to follow and error prone.  Repost the patch series

It's so much more convenient to do it this way. Re-sending entire
patch-sets for small fixups is clumsy and annoying at best. Creating
much more churn than is actually required. Sending patches again
signally i.e. not as a reply to the original [PATCH x/x], would be
even more prone to error.

Personally, I like to get the niggles and fixups out of the way using
this method, then send the entire patch-set again, complete with all
of the reaped Acks once there are significant fixes or when I believe
it to be finished and ready for applying.

Surely most people have their mail setup as threaded? Then the
time-line and subsequent patch versions are very easy to follow aren't
they? I get a nice trace like this:

> <date> Fabio Baltieri    (  0) ├>[PATCH 2/6] ASoC: ux500: <snip>
> <date> To Fabio Baltieri (  0) │└>
> <date> Fabio Baltieri    (  0) │ └>[PATCH v2 2/6] ASoC:   <snip>

... or even better would be to reply to the original one, then
subsequent versions won't be "buried in the thread" per say:

> <date> Fabio Baltieri    (  0) ├>[PATCH 2/6] ASoC: ux500: <snip>
> <date> Fabio Baltieri    (  0) │ └>[PATCH v2 2/6] ASoC:   <snip>
> <date> To Fabio Baltieri (  0) │->

> or wait until what can be applied is applied then repost.

Taking patches out-of-order, or 'willy-nilly', is asking for trouble.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 2/6] ASoC: ux500: Do not clear state if already idle

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2479 bytes --]

On Wed, May 08, 2013 at 12:04:51PM +0100, Lee Jones wrote:
> On Wed, 08 May 2013, Mark Brown wrote:

> > Ugh, please don't do stuff like this - you're posting an individual
> > revision of a patch buried in the middle of a thread.  This just makes
> > things hard to follow and error prone.  Repost the patch series

> It's so much more convenient to do it this way. Re-sending entire
> patch-sets for small fixups is clumsy and annoying at best. Creating
> even more prone to error.

Then consider what happens as soon as you get more than one update to
the patch, or there's any meaningful discussion on the patches - picking
out which of many versions in bifurcating threads.  You shouldn't even
assume that followups to the patches are being read, for example if it's
clear that there's revision required and it's all device specific
discussion rather than framework stuff I'll often just stop reading the
thread and wait for the respost.

> much more churn than is actually required. Sending patches again
> signally i.e. not as a reply to the original [PATCH x/x], would be

Of course, yes - that's a bad idea.

> Surely most people have their mail setup as threaded? Then the
> time-line and subsequent patch versions are very easy to follow aren't
> they? I get a nice trace like this:

> > <date> Fabio Baltieri    (  0) ├>[PATCH 2/6] ASoC: ux500: <snip>
> > <date> To Fabio Baltieri (  0) │└>
> > <date> Fabio Baltieri    (  0) │ └>[PATCH v2 2/6] ASoC:   <snip>

> ... or even better would be to reply to the original one, then
> subsequent versions won't be "buried in the thread" per say:

> > <date> Fabio Baltieri    (  0) ├>[PATCH 2/6] ASoC: ux500: <snip>
> > <date> Fabio Baltieri    (  0) │ └>[PATCH v2 2/6] ASoC:   <snip>
> > <date> To Fabio Baltieri (  0) │->

This doesn't work nearly so well once you start getting meaningful
discussion, multiple branches on the thread and indentation can make it
hard to spot where the latest patch is and it's still more effort to
find the latest version.

> > or wait until what can be applied is applied then repost.

> Taking patches out-of-order, or 'willy-nilly', is asking for trouble.

We've been through this repeatedly.  If your early patches won't work
without the later patches then you need to improve your early patches so
they stand alone.  Asking people to re-review an enormous patch series
because of a change at the end isn't helpful

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 2/6] ASoC: ux500: Do not clear state if already idle

[Lee Jones]

On Wed, 08 May 2013, Mark Brown wrote:

> On Wed, May 08, 2013 at 12:04:51PM +0100, Lee Jones wrote:
> > On Wed, 08 May 2013, Mark Brown wrote:
> 
> > > Ugh, please don't do stuff like this - you're posting an individual
> > > revision of a patch buried in the middle of a thread.  This just makes
> > > things hard to follow and error prone.  Repost the patch series
> 
> > It's so much more convenient to do it this way. Re-sending entire
> > patch-sets for small fixups is clumsy and annoying at best. Creating
> > even more prone to error.
> 
> Then consider what happens as soon as you get more than one update to
> the patch, or there's any meaningful discussion on the patches - picking
> out which of many versions in bifurcating threads.  You shouldn't even
> assume that followups to the patches are being read, for example if it's
> clear that there's revision required and it's all device specific
> discussion rather than framework stuff I'll often just stop reading the
> thread and wait for the respost.

<snip>

> > Surely most people have their mail setup as threaded? Then the
> > time-line and subsequent patch versions are very easy to follow aren't
> > they? I get a nice trace like this:

<snip>

> This doesn't work nearly so well once you start getting meaningful
> discussion, multiple branches on the thread and indentation can make it
> hard to spot where the latest patch is and it's still more effort to
> find the latest version.

Yes, of course one still has to use common sense. If this happens then
I'd say a re-post would be the obvious thing to do. I'm speaking more
about situations such as this, where the discussion is trivial and the
fixup, less so.

> > > or wait until what can be applied is applied then repost.
> 
> > Taking patches out-of-order, or 'willy-nilly', is asking for trouble.
> 
> We've been through this repeatedly.  If your early patches won't work
> without the later patches then you need to improve your early patches so
> they stand alone.

It's never okay for early patches to rely on later ones and yes, all
patches should be as orthogonal as possible. But it is okay for later
patches to rely on earlier ones.

Besides, I was more referencing the massively increased effort
imparted to the developer by applying patches in an arbitrary order.
Forcing the developer to rearranging and rebase the patch-set causing
unnecessary merge conflicts. It's better if the maintainer takes the
patch-set in the order it was written to prevent unnecessary (which is
the key word here) such things.

> Asking people to re-review an enormous patch series
> because of a change at the end isn't helpful

I completely agree. Hence why I reap Acks and apply them on next
submission to show the maintainer what they have reviewed and accepted
already. 

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 2/6] ASoC: ux500: Do not clear state if already idle

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 654 bytes --]

On Wed, May 08, 2013 at 01:03:26PM +0100, Lee Jones wrote:

> Besides, I was more referencing the massively increased effort
> imparted to the developer by applying patches in an arbitrary order.
> Forcing the developer to rearranging and rebase the patch-set causing
> unnecessary merge conflicts. It's better if the maintainer takes the
> patch-set in the order it was written to prevent unnecessary (which is
> the key word here) such things.

Meh, rebase takes care of all this stuff for you and you really need to
be rebasing anyway to take account of changes sent by other people.  The
problem you were having was that you weren't rebasing at all.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 2/6] ASoC: ux500: Do not clear state if already idle

[Lee Jones]

On Wed, 08 May 2013, Mark Brown wrote:

> On Wed, May 08, 2013 at 01:03:26PM +0100, Lee Jones wrote:
> 
> > Besides, I was more referencing the massively increased effort
> > imparted to the developer by applying patches in an arbitrary order.
> > Forcing the developer to rearranging and rebase the patch-set causing
> > unnecessary merge conflicts. It's better if the maintainer takes the
> > patch-set in the order it was written to prevent unnecessary (which is
> > the key word here) such things.
> 
> Meh, rebase takes care of all this stuff for you and you really need to
> be rebasing anyway to take account of changes sent by other people.

> The problem you were having was that you weren't rebasing at all.

Eh? That's just plain wrong.

Anyway, I'm not talking about any particular incident/session/period.

I'm saying, from experience, from the developer side, that if a
reviewer goes though a patch-set taking the ones s/he likes leaving
the rest behind, there are bound to be merge conflicts and semantic
issues which the developer will then have to resolve. Stuff that I
believe is added, unnecessary burden which would be easily avoided if
the set is firstly reviewed and _then_ applied after the Acks have
been awarded.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 2/6] ASoC: ux500: Do not clear state if already idle

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1729 bytes --]

On Wed, May 08, 2013 at 02:05:54PM +0100, Lee Jones wrote:

> I'm saying, from experience, from the developer side, that if a
> reviewer goes though a patch-set taking the ones s/he likes leaving
> the rest behind, there are bound to be merge conflicts and semantic
> issues which the developer will then have to resolve. Stuff that I
> believe is added, unnecessary burden which would be easily avoided if
> the set is firstly reviewed and _then_ applied after the Acks have
> been awarded.

So, you have to assume a bit of taste on the part of the people applying
the patches here.  If you're seeing merge conflicts that's probably an
issue, but then it's very surprising that the patches would apply
successfully in the first place since the conflicts generally come up
when the patches are applied too.

The other thing to bear in mind here is that a patch series which is
"here's a bunch of random changes to this driver" isn't the same as a
patch series which builds something up through a series of changes.
This series is a good example of the former - there's a few related
things but really there's no visible relationship between most of the
changes except that they happen to be for the same driver and sent at
the same time.

The big downsides of not applying patches are that it takes longer to
get the benefit of the bits that are good out to people and the big
increase in reviewer fatigue from having to re-read the same patches
over and over again.  This is one of the major ways problematic code
gets in, reviewers eyes glaze over and they just start missing things.
There's also the fact that serieses often end up having separate bugfix
and development components which need to be routed differently anyway.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 2/6] ASoC: ux500: Do not clear state if already idle

[Lee Jones]

On Wed, 08 May 2013, Mark Brown wrote:

> On Wed, May 08, 2013 at 02:05:54PM +0100, Lee Jones wrote:
> 
> > I'm saying, from experience, from the developer side, that if a
> > reviewer goes though a patch-set taking the ones s/he likes leaving
> > the rest behind, there are bound to be merge conflicts and semantic
> > issues which the developer will then have to resolve. Stuff that I
> > believe is added, unnecessary burden which would be easily avoided if
> > the set is firstly reviewed and _then_ applied after the Acks have
> > been awarded.
> 
> So, you have to assume a bit of taste on the part of the people applying
> the patches here.  If you're seeing merge conflicts that's probably an
> issue, but then it's very surprising that the patches would apply
> successfully in the first place since the conflicts generally come up
> when the patches are applied too.
> 
> The other thing to bear in mind here is that a patch series which is
> "here's a bunch of random changes to this driver" isn't the same as a
> patch series which builds something up through a series of changes.
> This series is a good example of the former - there's a few related
> things but really there's no visible relationship between most of the
> changes except that they happen to be for the same driver and sent at
> the same time.
> 
> The big downsides of not applying patches are that it takes longer to
> get the benefit of the bits that are good out to people and the big
> increase in reviewer fatigue from having to re-read the same patches
> over and over again.  This is one of the major ways problematic code
> gets in, reviewers eyes glaze over and they just start missing things.
> There's also the fact that serieses often end up having separate bugfix
> and development components which need to be routed differently anyway.

I understand your points and certainly sympathise with a great many of
them. I also understand the difference between random changes, which
aren't related in any systematic way and changes which are build upon
each other. It was the latter type to which I was referring to having
issues with.

Bringing this back on subject, whilst trying not to drag this out for
longer than we have to. I think replying to one's first patch with a
subsequent version has its merits. And as long as the thread hasn't
become too overgrown I see it as a useful tool to obtain an Ack or
further comment easily and without churn overhead. This also aids in
your quest to save maintainers from reviewing the same code
repeatedly, as once the Ack is in place it's easy to see which patches
are in need of further review and which ones can be skipped.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 2/6] ASoC: ux500: Do not clear state if already idle

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 762 bytes --]

On Wed, May 08, 2013 at 03:06:18PM +0100, Lee Jones wrote:

> Bringing this back on subject, whilst trying not to drag this out for
> longer than we have to. I think replying to one's first patch with a
> subsequent version has its merits. And as long as the thread hasn't
> become too overgrown I see it as a useful tool to obtain an Ack or
> further comment easily and without churn overhead. This also aids in
> your quest to save maintainers from reviewing the same code
> repeatedly, as once the Ack is in place it's easy to see which patches
> are in need of further review and which ones can be skipped.

If you want to do that what often works better is to post an incremental
diff rather than the full patch to show exactly what's been done in the
fix.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 2/6] ASoC: ux500: Do not clear state if already idle

[Fabio Baltieri]

On Wed, May 08, 2013 at 11:34:01AM +0100, Mark Brown wrote:
> On Wed, May 08, 2013 at 10:39:14AM +0200, Fabio Baltieri wrote:
> > As enable_msp gets called only after some audio data has been received,
> > if the userspace closes the device before sending any data it causes
> > ux500_msp_i2s_close to clear device state even if it was not previously
> > initialized.
> 
> Ugh, please don't do stuff like this - you're posting an individual
> revision of a patch buried in the middle of a thread.  This just makes
> things hard to follow and error prone.  Repost the patch series or wait
> until what can be applied is applied then repost.

Ok, I'll just repost all the not applied patches after the review.

Anything on the patch itself?

Fabio

-- 
Fabio Baltieri

Re: [PATCH v2 2/6] ASoC: ux500: Do not clear state if already idle

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 130 bytes --]

On Wed, May 08, 2013 at 02:04:49PM +0200, Fabio Baltieri wrote:

> Anything on the patch itself?

I didn't really look at it TBH.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 3/6] ASoC: ux500: Drop pinctrl sleep support

[Fabio Baltieri]

Drop pinctrl default/sleep state switching code, as it was breaking the
capture interface by putting the I2S pins in hi-z mode regardless of its
usage status, and not giving any real benefit.

Pinctrl default mode configuration is already managed automatically by a
specific pinctrl hog.

Signed-off-by: Fabio Baltieri <fabio.baltieri@linaro.org>
---
 sound/soc/ux500/ux500_msp_i2s.c | 56 ++---------------------------------------
 sound/soc/ux500/ux500_msp_i2s.h |  6 -----
 2 files changed, 2 insertions(+), 60 deletions(-)

diff --git a/sound/soc/ux500/ux500_msp_i2s.c b/sound/soc/ux500/ux500_msp_i2s.c
index 964cfd6..8512c78 100644
--- a/sound/soc/ux500/ux500_msp_i2s.c
+++ b/sound/soc/ux500/ux500_msp_i2s.c
@@ -15,7 +15,6 @@
 
 #include <linux/module.h>
 #include <linux/platform_device.h>
-#include <linux/pinctrl/consumer.h>
 #include <linux/delay.h>
 #include <linux/slab.h>
 #include <linux/io.h>
@@ -28,9 +27,6 @@
 
 #include "ux500_msp_i2s.h"
 
-/* MSP1/3 Tx/Rx usage protection */
-static DEFINE_SPINLOCK(msp_rxtx_lock);
-
  /* Protocol desciptors */
 static const struct msp_protdesc prot_descs[] = {
 	{ /* I2S */
@@ -358,24 +354,8 @@ static int configure_multichannel(struct ux500_msp *msp,
 
 static int enable_msp(struct ux500_msp *msp, struct ux500_msp_config *config)
 {
-	int status = 0, retval = 0;
+	int status = 0;
 	u32 reg_val_DMACR, reg_val_GCR;
-	unsigned long flags;
-
-	/* Check msp state whether in RUN or CONFIGURED Mode */
-	if (msp->msp_state == MSP_STATE_IDLE) {
-		spin_lock_irqsave(&msp_rxtx_lock, flags);
-		if (msp->pinctrl_rxtx_ref == 0 &&
-			!(IS_ERR(msp->pinctrl_p) || IS_ERR(msp->pinctrl_def))) {
-			retval = pinctrl_select_state(msp->pinctrl_p,
-						msp->pinctrl_def);
-			if (retval)
-				pr_err("could not set MSP defstate\n");
-		}
-		if (!retval)
-			msp->pinctrl_rxtx_ref++;
-		spin_unlock_irqrestore(&msp_rxtx_lock, flags);
-	}
 
 	/* Configure msp with protocol dependent settings */
 	configure_protocol(msp, config);
@@ -632,8 +612,7 @@ int ux500_msp_i2s_trigger(struct ux500_msp *msp, int cmd, int direction)
 
 int ux500_msp_i2s_close(struct ux500_msp *msp, unsigned int dir)
 {
-	int status = 0, retval = 0;
-	unsigned long flags;
+	int status = 0;
 
 	dev_dbg(msp->dev, "%s: Enter (dir = 0x%01x).\n", __func__, dir);
 
@@ -645,18 +624,6 @@ int ux500_msp_i2s_close(struct ux500_msp *msp, unsigned int dir)
 			       (~(FRAME_GEN_ENABLE | SRG_ENABLE))),
 			      msp->registers + MSP_GCR);
 
-		spin_lock_irqsave(&msp_rxtx_lock, flags);
-		WARN_ON(!msp->pinctrl_rxtx_ref);
-		msp->pinctrl_rxtx_ref--;
-		if (msp->pinctrl_rxtx_ref == 0 &&
-			!(IS_ERR(msp->pinctrl_p) || IS_ERR(msp->pinctrl_sleep))) {
-			retval = pinctrl_select_state(msp->pinctrl_p,
-						msp->pinctrl_sleep);
-			if (retval)
-				pr_err("could not set MSP sleepstate\n");
-		}
-		spin_unlock_irqrestore(&msp_rxtx_lock, flags);
-
 		writel(0, msp->registers + MSP_GCR);
 		writel(0, msp->registers + MSP_TCF);
 		writel(0, msp->registers + MSP_RCF);
@@ -745,25 +712,6 @@ int ux500_msp_i2s_init_msp(struct platform_device *pdev,
 	dev_dbg(&pdev->dev, "I2S device-name: '%s'\n", i2s_cont->name);
 	msp->i2s_cont = i2s_cont;
 
-	msp->pinctrl_p = pinctrl_get(msp->dev);
-	if (IS_ERR(msp->pinctrl_p))
-		dev_err(&pdev->dev, "could not get MSP pinctrl\n");
-	else {
-		msp->pinctrl_def = pinctrl_lookup_state(msp->pinctrl_p,
-						PINCTRL_STATE_DEFAULT);
-		if (IS_ERR(msp->pinctrl_def)) {
-			dev_err(&pdev->dev,
-				"could not get MSP defstate (%li)\n",
-				PTR_ERR(msp->pinctrl_def));
-		}
-		msp->pinctrl_sleep = pinctrl_lookup_state(msp->pinctrl_p,
-						PINCTRL_STATE_SLEEP);
-		if (IS_ERR(msp->pinctrl_sleep))
-			dev_err(&pdev->dev,
-				"could not get MSP idlestate (%li)\n",
-				PTR_ERR(msp->pinctrl_def));
-	}
-
 	return 0;
 }
 
diff --git a/sound/soc/ux500/ux500_msp_i2s.h b/sound/soc/ux500/ux500_msp_i2s.h
index 1311c0d..1ce336f 100644
--- a/sound/soc/ux500/ux500_msp_i2s.h
+++ b/sound/soc/ux500/ux500_msp_i2s.h
@@ -530,12 +530,6 @@ struct ux500_msp {
 	int loopback_enable;
 	u32 backup_regs[MAX_MSP_BACKUP_REGS];
 	unsigned int f_bitclk;
-	/* Pin modes */
-	struct pinctrl *pinctrl_p;
-	struct pinctrl_state *pinctrl_def;
-	struct pinctrl_state *pinctrl_sleep;
-	/* Reference Count */
-	int pinctrl_rxtx_ref;
 };
 
 struct ux500_msp_dma_params {
-- 
1.8.2

Re: [PATCH 3/6] ASoC: ux500: Drop pinctrl sleep support

[Lee Jones]

On Wed, 08 May 2013, Fabio Baltieri wrote:

> Drop pinctrl default/sleep state switching code, as it was breaking the
> capture interface by putting the I2S pins in hi-z mode regardless of its
> usage status, and not giving any real benefit.
> 
> Pinctrl default mode configuration is already managed automatically by a
> specific pinctrl hog.

I'm sure we should support pinctrl though shouldn't we?

Is there no way of fixing the implementation instead of ripping it out?

> Signed-off-by: Fabio Baltieri <fabio.baltieri@linaro.org>
> ---
>  sound/soc/ux500/ux500_msp_i2s.c | 56 ++---------------------------------------
>  sound/soc/ux500/ux500_msp_i2s.h |  6 -----
>  2 files changed, 2 insertions(+), 60 deletions(-)
> 
> diff --git a/sound/soc/ux500/ux500_msp_i2s.c b/sound/soc/ux500/ux500_msp_i2s.c
> index 964cfd6..8512c78 100644
> --- a/sound/soc/ux500/ux500_msp_i2s.c
> +++ b/sound/soc/ux500/ux500_msp_i2s.c
> @@ -15,7 +15,6 @@
>  
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> -#include <linux/pinctrl/consumer.h>
>  #include <linux/delay.h>
>  #include <linux/slab.h>
>  #include <linux/io.h>
> @@ -28,9 +27,6 @@
>  
>  #include "ux500_msp_i2s.h"
>  
> -/* MSP1/3 Tx/Rx usage protection */
> -static DEFINE_SPINLOCK(msp_rxtx_lock);
> -
>   /* Protocol desciptors */
>  static const struct msp_protdesc prot_descs[] = {
>  	{ /* I2S */
> @@ -358,24 +354,8 @@ static int configure_multichannel(struct ux500_msp *msp,
>  
>  static int enable_msp(struct ux500_msp *msp, struct ux500_msp_config *config)
>  {
> -	int status = 0, retval = 0;
> +	int status = 0;
>  	u32 reg_val_DMACR, reg_val_GCR;
> -	unsigned long flags;
> -
> -	/* Check msp state whether in RUN or CONFIGURED Mode */
> -	if (msp->msp_state == MSP_STATE_IDLE) {
> -		spin_lock_irqsave(&msp_rxtx_lock, flags);
> -		if (msp->pinctrl_rxtx_ref == 0 &&
> -			!(IS_ERR(msp->pinctrl_p) || IS_ERR(msp->pinctrl_def))) {
> -			retval = pinctrl_select_state(msp->pinctrl_p,
> -						msp->pinctrl_def);
> -			if (retval)
> -				pr_err("could not set MSP defstate\n");
> -		}
> -		if (!retval)
> -			msp->pinctrl_rxtx_ref++;
> -		spin_unlock_irqrestore(&msp_rxtx_lock, flags);
> -	}
>  
>  	/* Configure msp with protocol dependent settings */
>  	configure_protocol(msp, config);
> @@ -632,8 +612,7 @@ int ux500_msp_i2s_trigger(struct ux500_msp *msp, int cmd, int direction)
>  
>  int ux500_msp_i2s_close(struct ux500_msp *msp, unsigned int dir)
>  {
> -	int status = 0, retval = 0;
> -	unsigned long flags;
> +	int status = 0;
>  
>  	dev_dbg(msp->dev, "%s: Enter (dir = 0x%01x).\n", __func__, dir);
>  
> @@ -645,18 +624,6 @@ int ux500_msp_i2s_close(struct ux500_msp *msp, unsigned int dir)
>  			       (~(FRAME_GEN_ENABLE | SRG_ENABLE))),
>  			      msp->registers + MSP_GCR);
>  
> -		spin_lock_irqsave(&msp_rxtx_lock, flags);
> -		WARN_ON(!msp->pinctrl_rxtx_ref);
> -		msp->pinctrl_rxtx_ref--;
> -		if (msp->pinctrl_rxtx_ref == 0 &&
> -			!(IS_ERR(msp->pinctrl_p) || IS_ERR(msp->pinctrl_sleep))) {
> -			retval = pinctrl_select_state(msp->pinctrl_p,
> -						msp->pinctrl_sleep);
> -			if (retval)
> -				pr_err("could not set MSP sleepstate\n");
> -		}
> -		spin_unlock_irqrestore(&msp_rxtx_lock, flags);
> -
>  		writel(0, msp->registers + MSP_GCR);
>  		writel(0, msp->registers + MSP_TCF);
>  		writel(0, msp->registers + MSP_RCF);
> @@ -745,25 +712,6 @@ int ux500_msp_i2s_init_msp(struct platform_device *pdev,
>  	dev_dbg(&pdev->dev, "I2S device-name: '%s'\n", i2s_cont->name);
>  	msp->i2s_cont = i2s_cont;
>  
> -	msp->pinctrl_p = pinctrl_get(msp->dev);
> -	if (IS_ERR(msp->pinctrl_p))
> -		dev_err(&pdev->dev, "could not get MSP pinctrl\n");
> -	else {
> -		msp->pinctrl_def = pinctrl_lookup_state(msp->pinctrl_p,
> -						PINCTRL_STATE_DEFAULT);
> -		if (IS_ERR(msp->pinctrl_def)) {
> -			dev_err(&pdev->dev,
> -				"could not get MSP defstate (%li)\n",
> -				PTR_ERR(msp->pinctrl_def));
> -		}
> -		msp->pinctrl_sleep = pinctrl_lookup_state(msp->pinctrl_p,
> -						PINCTRL_STATE_SLEEP);
> -		if (IS_ERR(msp->pinctrl_sleep))
> -			dev_err(&pdev->dev,
> -				"could not get MSP idlestate (%li)\n",
> -				PTR_ERR(msp->pinctrl_def));
> -	}
> -
>  	return 0;
>  }
>  
> diff --git a/sound/soc/ux500/ux500_msp_i2s.h b/sound/soc/ux500/ux500_msp_i2s.h
> index 1311c0d..1ce336f 100644
> --- a/sound/soc/ux500/ux500_msp_i2s.h
> +++ b/sound/soc/ux500/ux500_msp_i2s.h
> @@ -530,12 +530,6 @@ struct ux500_msp {
>  	int loopback_enable;
>  	u32 backup_regs[MAX_MSP_BACKUP_REGS];
>  	unsigned int f_bitclk;
> -	/* Pin modes */
> -	struct pinctrl *pinctrl_p;
> -	struct pinctrl_state *pinctrl_def;
> -	struct pinctrl_state *pinctrl_sleep;
> -	/* Reference Count */
> -	int pinctrl_rxtx_ref;
>  };
>  
>  struct ux500_msp_dma_params {

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 3/6] ASoC: ux500: Drop pinctrl sleep support

[Fabio Baltieri]

On Wed, May 08, 2013 at 09:07:08AM +0100, Lee Jones wrote:
> On Wed, 08 May 2013, Fabio Baltieri wrote:
> 
> > Drop pinctrl default/sleep state switching code, as it was breaking the
> > capture interface by putting the I2S pins in hi-z mode regardless of its
> > usage status, and not giving any real benefit.
> > 
> > Pinctrl default mode configuration is already managed automatically by a
> > specific pinctrl hog.
> 
> I'm sure we should support pinctrl though shouldn't we?
> 
> Is there no way of fixing the implementation instead of ripping it out?

Yes, but requesting the default pinctrl configuration should be enough,
and as those pins are shared with multiple device ids, a "hog"
configuration should be the cleanest.

Actually I asked Linus an opinion before doing this, so maybe he can ack
this patch or suggest a better way of doing this, such as declaring the
same pins for multiple device ids, but I'm not sure that would work as
expected.

Thanks,
Fabio

> > Signed-off-by: Fabio Baltieri <fabio.baltieri@linaro.org>
> > ---
> >  sound/soc/ux500/ux500_msp_i2s.c | 56 ++---------------------------------------
> >  sound/soc/ux500/ux500_msp_i2s.h |  6 -----
> >  2 files changed, 2 insertions(+), 60 deletions(-)
> > 
> > diff --git a/sound/soc/ux500/ux500_msp_i2s.c b/sound/soc/ux500/ux500_msp_i2s.c
> > index 964cfd6..8512c78 100644
> > --- a/sound/soc/ux500/ux500_msp_i2s.c
> > +++ b/sound/soc/ux500/ux500_msp_i2s.c
> > @@ -15,7 +15,6 @@
> >  
> >  #include <linux/module.h>
> >  #include <linux/platform_device.h>
> > -#include <linux/pinctrl/consumer.h>
> >  #include <linux/delay.h>
> >  #include <linux/slab.h>
> >  #include <linux/io.h>
> > @@ -28,9 +27,6 @@
> >  
> >  #include "ux500_msp_i2s.h"
> >  
> > -/* MSP1/3 Tx/Rx usage protection */
> > -static DEFINE_SPINLOCK(msp_rxtx_lock);
> > -
> >   /* Protocol desciptors */
> >  static const struct msp_protdesc prot_descs[] = {
> >  	{ /* I2S */
> > @@ -358,24 +354,8 @@ static int configure_multichannel(struct ux500_msp *msp,
> >  
> >  static int enable_msp(struct ux500_msp *msp, struct ux500_msp_config *config)
> >  {
> > -	int status = 0, retval = 0;
> > +	int status = 0;
> >  	u32 reg_val_DMACR, reg_val_GCR;
> > -	unsigned long flags;
> > -
> > -	/* Check msp state whether in RUN or CONFIGURED Mode */
> > -	if (msp->msp_state == MSP_STATE_IDLE) {
> > -		spin_lock_irqsave(&msp_rxtx_lock, flags);
> > -		if (msp->pinctrl_rxtx_ref == 0 &&
> > -			!(IS_ERR(msp->pinctrl_p) || IS_ERR(msp->pinctrl_def))) {
> > -			retval = pinctrl_select_state(msp->pinctrl_p,
> > -						msp->pinctrl_def);
> > -			if (retval)
> > -				pr_err("could not set MSP defstate\n");
> > -		}
> > -		if (!retval)
> > -			msp->pinctrl_rxtx_ref++;
> > -		spin_unlock_irqrestore(&msp_rxtx_lock, flags);
> > -	}
> >  
> >  	/* Configure msp with protocol dependent settings */
> >  	configure_protocol(msp, config);
> > @@ -632,8 +612,7 @@ int ux500_msp_i2s_trigger(struct ux500_msp *msp, int cmd, int direction)
> >  
> >  int ux500_msp_i2s_close(struct ux500_msp *msp, unsigned int dir)
> >  {
> > -	int status = 0, retval = 0;
> > -	unsigned long flags;
> > +	int status = 0;
> >  
> >  	dev_dbg(msp->dev, "%s: Enter (dir = 0x%01x).\n", __func__, dir);
> >  
> > @@ -645,18 +624,6 @@ int ux500_msp_i2s_close(struct ux500_msp *msp, unsigned int dir)
> >  			       (~(FRAME_GEN_ENABLE | SRG_ENABLE))),
> >  			      msp->registers + MSP_GCR);
> >  
> > -		spin_lock_irqsave(&msp_rxtx_lock, flags);
> > -		WARN_ON(!msp->pinctrl_rxtx_ref);
> > -		msp->pinctrl_rxtx_ref--;
> > -		if (msp->pinctrl_rxtx_ref == 0 &&
> > -			!(IS_ERR(msp->pinctrl_p) || IS_ERR(msp->pinctrl_sleep))) {
> > -			retval = pinctrl_select_state(msp->pinctrl_p,
> > -						msp->pinctrl_sleep);
> > -			if (retval)
> > -				pr_err("could not set MSP sleepstate\n");
> > -		}
> > -		spin_unlock_irqrestore(&msp_rxtx_lock, flags);
> > -
> >  		writel(0, msp->registers + MSP_GCR);
> >  		writel(0, msp->registers + MSP_TCF);
> >  		writel(0, msp->registers + MSP_RCF);
> > @@ -745,25 +712,6 @@ int ux500_msp_i2s_init_msp(struct platform_device *pdev,
> >  	dev_dbg(&pdev->dev, "I2S device-name: '%s'\n", i2s_cont->name);
> >  	msp->i2s_cont = i2s_cont;
> >  
> > -	msp->pinctrl_p = pinctrl_get(msp->dev);
> > -	if (IS_ERR(msp->pinctrl_p))
> > -		dev_err(&pdev->dev, "could not get MSP pinctrl\n");
> > -	else {
> > -		msp->pinctrl_def = pinctrl_lookup_state(msp->pinctrl_p,
> > -						PINCTRL_STATE_DEFAULT);
> > -		if (IS_ERR(msp->pinctrl_def)) {
> > -			dev_err(&pdev->dev,
> > -				"could not get MSP defstate (%li)\n",
> > -				PTR_ERR(msp->pinctrl_def));
> > -		}
> > -		msp->pinctrl_sleep = pinctrl_lookup_state(msp->pinctrl_p,
> > -						PINCTRL_STATE_SLEEP);
> > -		if (IS_ERR(msp->pinctrl_sleep))
> > -			dev_err(&pdev->dev,
> > -				"could not get MSP idlestate (%li)\n",
> > -				PTR_ERR(msp->pinctrl_def));
> > -	}
> > -
> >  	return 0;
> >  }
> >  
> > diff --git a/sound/soc/ux500/ux500_msp_i2s.h b/sound/soc/ux500/ux500_msp_i2s.h
> > index 1311c0d..1ce336f 100644
> > --- a/sound/soc/ux500/ux500_msp_i2s.h
> > +++ b/sound/soc/ux500/ux500_msp_i2s.h
> > @@ -530,12 +530,6 @@ struct ux500_msp {
> >  	int loopback_enable;
> >  	u32 backup_regs[MAX_MSP_BACKUP_REGS];
> >  	unsigned int f_bitclk;
> > -	/* Pin modes */
> > -	struct pinctrl *pinctrl_p;
> > -	struct pinctrl_state *pinctrl_def;
> > -	struct pinctrl_state *pinctrl_sleep;
> > -	/* Reference Count */
> > -	int pinctrl_rxtx_ref;
> >  };
> >  
> >  struct ux500_msp_dma_params {

-- 
Fabio Baltieri

Re: [PATCH 3/6] ASoC: ux500: Drop pinctrl sleep support

[Lee Jones]

On Wed, 08 May 2013, Fabio Baltieri wrote:

> On Wed, May 08, 2013 at 09:07:08AM +0100, Lee Jones wrote:
> > On Wed, 08 May 2013, Fabio Baltieri wrote:
> > 
> > > Drop pinctrl default/sleep state switching code, as it was breaking the
> > > capture interface by putting the I2S pins in hi-z mode regardless of its
> > > usage status, and not giving any real benefit.
> > > 
> > > Pinctrl default mode configuration is already managed automatically by a
> > > specific pinctrl hog.
> > 
> > I'm sure we should support pinctrl though shouldn't we?
> > 
> > Is there no way of fixing the implementation instead of ripping it out?
> 
> Yes, but requesting the default pinctrl configuration should be enough,
> and as those pins are shared with multiple device ids, a "hog"
> configuration should be the cleanest.
> 
> Actually I asked Linus an opinion before doing this, so maybe he can ack
> this patch or suggest a better way of doing this, such as declaring the
> same pins for multiple device ids, but I'm not sure that would work as
> expected.

Linus is on vacation at the moment, but I agree he should have the
final say on this. Better wait until he returns.

> > > Signed-off-by: Fabio Baltieri <fabio.baltieri@linaro.org>
> > > ---
> > >  sound/soc/ux500/ux500_msp_i2s.c | 56 ++---------------------------------------
> > >  sound/soc/ux500/ux500_msp_i2s.h |  6 -----
> > >  2 files changed, 2 insertions(+), 60 deletions(-)
> > > 
> > > diff --git a/sound/soc/ux500/ux500_msp_i2s.c b/sound/soc/ux500/ux500_msp_i2s.c
> > > index 964cfd6..8512c78 100644
> > > --- a/sound/soc/ux500/ux500_msp_i2s.c
> > > +++ b/sound/soc/ux500/ux500_msp_i2s.c
> > > @@ -15,7 +15,6 @@
> > >  
> > >  #include <linux/module.h>
> > >  #include <linux/platform_device.h>
> > > -#include <linux/pinctrl/consumer.h>
> > >  #include <linux/delay.h>
> > >  #include <linux/slab.h>
> > >  #include <linux/io.h>
> > > @@ -28,9 +27,6 @@
> > >  
> > >  #include "ux500_msp_i2s.h"
> > >  
> > > -/* MSP1/3 Tx/Rx usage protection */
> > > -static DEFINE_SPINLOCK(msp_rxtx_lock);
> > > -
> > >   /* Protocol desciptors */
> > >  static const struct msp_protdesc prot_descs[] = {
> > >  	{ /* I2S */
> > > @@ -358,24 +354,8 @@ static int configure_multichannel(struct ux500_msp *msp,
> > >  
> > >  static int enable_msp(struct ux500_msp *msp, struct ux500_msp_config *config)
> > >  {
> > > -	int status = 0, retval = 0;
> > > +	int status = 0;
> > >  	u32 reg_val_DMACR, reg_val_GCR;
> > > -	unsigned long flags;
> > > -
> > > -	/* Check msp state whether in RUN or CONFIGURED Mode */
> > > -	if (msp->msp_state == MSP_STATE_IDLE) {
> > > -		spin_lock_irqsave(&msp_rxtx_lock, flags);
> > > -		if (msp->pinctrl_rxtx_ref == 0 &&
> > > -			!(IS_ERR(msp->pinctrl_p) || IS_ERR(msp->pinctrl_def))) {
> > > -			retval = pinctrl_select_state(msp->pinctrl_p,
> > > -						msp->pinctrl_def);
> > > -			if (retval)
> > > -				pr_err("could not set MSP defstate\n");
> > > -		}
> > > -		if (!retval)
> > > -			msp->pinctrl_rxtx_ref++;
> > > -		spin_unlock_irqrestore(&msp_rxtx_lock, flags);
> > > -	}
> > >  
> > >  	/* Configure msp with protocol dependent settings */
> > >  	configure_protocol(msp, config);
> > > @@ -632,8 +612,7 @@ int ux500_msp_i2s_trigger(struct ux500_msp *msp, int cmd, int direction)
> > >  
> > >  int ux500_msp_i2s_close(struct ux500_msp *msp, unsigned int dir)
> > >  {
> > > -	int status = 0, retval = 0;
> > > -	unsigned long flags;
> > > +	int status = 0;
> > >  
> > >  	dev_dbg(msp->dev, "%s: Enter (dir = 0x%01x).\n", __func__, dir);
> > >  
> > > @@ -645,18 +624,6 @@ int ux500_msp_i2s_close(struct ux500_msp *msp, unsigned int dir)
> > >  			       (~(FRAME_GEN_ENABLE | SRG_ENABLE))),
> > >  			      msp->registers + MSP_GCR);
> > >  
> > > -		spin_lock_irqsave(&msp_rxtx_lock, flags);
> > > -		WARN_ON(!msp->pinctrl_rxtx_ref);
> > > -		msp->pinctrl_rxtx_ref--;
> > > -		if (msp->pinctrl_rxtx_ref == 0 &&
> > > -			!(IS_ERR(msp->pinctrl_p) || IS_ERR(msp->pinctrl_sleep))) {
> > > -			retval = pinctrl_select_state(msp->pinctrl_p,
> > > -						msp->pinctrl_sleep);
> > > -			if (retval)
> > > -				pr_err("could not set MSP sleepstate\n");
> > > -		}
> > > -		spin_unlock_irqrestore(&msp_rxtx_lock, flags);
> > > -
> > >  		writel(0, msp->registers + MSP_GCR);
> > >  		writel(0, msp->registers + MSP_TCF);
> > >  		writel(0, msp->registers + MSP_RCF);
> > > @@ -745,25 +712,6 @@ int ux500_msp_i2s_init_msp(struct platform_device *pdev,
> > >  	dev_dbg(&pdev->dev, "I2S device-name: '%s'\n", i2s_cont->name);
> > >  	msp->i2s_cont = i2s_cont;
> > >  
> > > -	msp->pinctrl_p = pinctrl_get(msp->dev);
> > > -	if (IS_ERR(msp->pinctrl_p))
> > > -		dev_err(&pdev->dev, "could not get MSP pinctrl\n");
> > > -	else {
> > > -		msp->pinctrl_def = pinctrl_lookup_state(msp->pinctrl_p,
> > > -						PINCTRL_STATE_DEFAULT);
> > > -		if (IS_ERR(msp->pinctrl_def)) {
> > > -			dev_err(&pdev->dev,
> > > -				"could not get MSP defstate (%li)\n",
> > > -				PTR_ERR(msp->pinctrl_def));
> > > -		}
> > > -		msp->pinctrl_sleep = pinctrl_lookup_state(msp->pinctrl_p,
> > > -						PINCTRL_STATE_SLEEP);
> > > -		if (IS_ERR(msp->pinctrl_sleep))
> > > -			dev_err(&pdev->dev,
> > > -				"could not get MSP idlestate (%li)\n",
> > > -				PTR_ERR(msp->pinctrl_def));
> > > -	}
> > > -
> > >  	return 0;
> > >  }
> > >  
> > > diff --git a/sound/soc/ux500/ux500_msp_i2s.h b/sound/soc/ux500/ux500_msp_i2s.h
> > > index 1311c0d..1ce336f 100644
> > > --- a/sound/soc/ux500/ux500_msp_i2s.h
> > > +++ b/sound/soc/ux500/ux500_msp_i2s.h
> > > @@ -530,12 +530,6 @@ struct ux500_msp {
> > >  	int loopback_enable;
> > >  	u32 backup_regs[MAX_MSP_BACKUP_REGS];
> > >  	unsigned int f_bitclk;
> > > -	/* Pin modes */
> > > -	struct pinctrl *pinctrl_p;
> > > -	struct pinctrl_state *pinctrl_def;
> > > -	struct pinctrl_state *pinctrl_sleep;
> > > -	/* Reference Count */
> > > -	int pinctrl_rxtx_ref;
> > >  };
> > >  
> > >  struct ux500_msp_dma_params {
> 

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 3/6] ASoC: ux500: Drop pinctrl sleep support

[Fabio Baltieri]

On Wed, May 08, 2013 at 09:48:46AM +0100, Lee Jones wrote:
> On Wed, 08 May 2013, Fabio Baltieri wrote:
> 
> > On Wed, May 08, 2013 at 09:07:08AM +0100, Lee Jones wrote:
> > > On Wed, 08 May 2013, Fabio Baltieri wrote:
> > > 
> > > > Drop pinctrl default/sleep state switching code, as it was breaking the
> > > > capture interface by putting the I2S pins in hi-z mode regardless of its
> > > > usage status, and not giving any real benefit.
> > > > 
> > > > Pinctrl default mode configuration is already managed automatically by a
> > > > specific pinctrl hog.
> > > 
> > > I'm sure we should support pinctrl though shouldn't we?
> > > 
> > > Is there no way of fixing the implementation instead of ripping it out?
> > 
> > Yes, but requesting the default pinctrl configuration should be enough,
> > and as those pins are shared with multiple device ids, a "hog"
> > configuration should be the cleanest.
> > 
> > Actually I asked Linus an opinion before doing this, so maybe he can ack
> > this patch or suggest a better way of doing this, such as declaring the
> > same pins for multiple device ids, but I'm not sure that would work as
> > expected.
> 
> Linus is on vacation at the moment, but I agree he should have the
> final say on this. Better wait until he returns.

Sounds good, I'll send the pinctrl patch in the meantime.  There should
be no dependency issues regardless of merging order so I'll keep that
one on its own.

Fabio

-- 
Fabio Baltieri

Re: [PATCH 3/6] ASoC: ux500: Drop pinctrl sleep support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 634 bytes --]

On Wed, May 08, 2013 at 09:14:18AM +0200, Fabio Baltieri wrote:
> Drop pinctrl default/sleep state switching code, as it was breaking the
> capture interface by putting the I2S pins in hi-z mode regardless of its
> usage status, and not giving any real benefit.
> 
> Pinctrl default mode configuration is already managed automatically by a
> specific pinctrl hog.

I tend to agree with Lee that this looks like a bad approach - there's a
whole bunch of other code in there which I'd guess is probably equally
broken but only the pinctrl code is being removed.  Why not just fix it
(or better yet simplify all this stuff)?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/6] ASoC: ux500: Drop pinctrl sleep support

[Fabio Baltieri]

On Wed, May 08, 2013 at 11:51:24AM +0100, Mark Brown wrote:
> On Wed, May 08, 2013 at 09:14:18AM +0200, Fabio Baltieri wrote:
> > Drop pinctrl default/sleep state switching code, as it was breaking the
> > capture interface by putting the I2S pins in hi-z mode regardless of its
> > usage status, and not giving any real benefit.
> > 
> > Pinctrl default mode configuration is already managed automatically by a
> > specific pinctrl hog.
> 
> I tend to agree with Lee that this looks like a bad approach - there's a
> whole bunch of other code in there which I'd guess is probably equally
> broken but only the pinctrl code is being removed.  Why not just fix it
> (or better yet simplify all this stuff)?

Sorry I did not get the subject, are you referring to other broken code
in the ux500 driver or to pinctrl and shared pins in generals?

Fabio

-- 
Fabio Baltieri

Re: [PATCH 3/6] ASoC: ux500: Drop pinctrl sleep support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 840 bytes --]

On Wed, May 08, 2013 at 01:42:16PM +0200, Fabio Baltieri wrote:
> On Wed, May 08, 2013 at 11:51:24AM +0100, Mark Brown wrote:

> > I tend to agree with Lee that this looks like a bad approach - there's a
> > whole bunch of other code in there which I'd guess is probably equally
> > broken but only the pinctrl code is being removed.  Why not just fix it
> > (or better yet simplify all this stuff)?

> Sorry I did not get the subject, are you referring to other broken code
> in the ux500 driver or to pinctrl and shared pins in generals?

I'm saying that if functions like enable_msp() don't work reliably then
removing some but not all of their functionality isn't an obviously good
approach to fixing that.  Why does the other functionality work well but
not this bit?  It sounds like there's some reference counting bug here
is all...

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/6] ASoC: ux500: Drop pinctrl sleep support

[Fabio Baltieri]

On Wed, May 08, 2013 at 01:32:25PM +0100, Mark Brown wrote:
> On Wed, May 08, 2013 at 01:42:16PM +0200, Fabio Baltieri wrote:
> > On Wed, May 08, 2013 at 11:51:24AM +0100, Mark Brown wrote:
> 
> > > I tend to agree with Lee that this looks like a bad approach - there's a
> > > whole bunch of other code in there which I'd guess is probably equally
> > > broken but only the pinctrl code is being removed.  Why not just fix it
> > > (or better yet simplify all this stuff)?
> 
> > Sorry I did not get the subject, are you referring to other broken code
> > in the ux500 driver or to pinctrl and shared pins in generals?
> 
> I'm saying that if functions like enable_msp() don't work reliably then
> removing some but not all of their functionality isn't an obviously good
> approach to fixing that.  Why does the other functionality work well but
> not this bit?  It sounds like there's some reference counting bug here
> is all...

Yes, it started as a reference counting bug, due to the actual counter
not being shared between ux500-msp-i2s instances.

That said, the actual fork of this driver deployed by STE internally
does not handle I2S pin sleep state, and I was not able to find any
other ASoC driver that does that, which seems reasonable to me as I
can't come up with a reason to put those pins in hi-z anyway.

If I understood the problem correctly you do that when you want to cut
power completely to some peripherals to avoid spurious current paths,
and that should not be the case for the audio codec, especially in this
case where it's part of a big multifuntion IC.

So this is why I'm proposing to drop that code altogheter rather than
fix it.

Fabio

-- 
Fabio Baltieri

Re: [PATCH 3/6] ASoC: ux500: Drop pinctrl sleep support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1444 bytes --]

On Wed, May 08, 2013 at 03:10:20PM +0200, Fabio Baltieri wrote:
> On Wed, May 08, 2013 at 01:32:25PM +0100, Mark Brown wrote:

> > I'm saying that if functions like enable_msp() don't work reliably then
> > removing some but not all of their functionality isn't an obviously good
> > approach to fixing that.  Why does the other functionality work well but
> > not this bit?  It sounds like there's some reference counting bug here
> > is all...

> Yes, it started as a reference counting bug, due to the actual counter
> not being shared between ux500-msp-i2s instances.

> That said, the actual fork of this driver deployed by STE internally
> does not handle I2S pin sleep state, and I was not able to find any
> other ASoC driver that does that, which seems reasonable to me as I
> can't come up with a reason to put those pins in hi-z anyway.

But why does the rest of the code work well if the reference counting is
wrong, it's in the middle of a big block of code?  This all smells like
this change is papering over a specific symptom of some underlying issue
- if that's not the case then it needs to be clearer why.

> If I understood the problem correctly you do that when you want to cut
> power completely to some peripherals to avoid spurious current paths,
> and that should not be the case for the audio codec, especially in this
> case where it's part of a big multifuntion IC.

Being a MFD should have nothing to do with this?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/6] ASoC: ux500: Drop pinctrl sleep support

[Fabio Baltieri]

On Wed, May 08, 2013 at 02:54:13PM +0100, Mark Brown wrote:
> On Wed, May 08, 2013 at 03:10:20PM +0200, Fabio Baltieri wrote:
> > On Wed, May 08, 2013 at 01:32:25PM +0100, Mark Brown wrote:
> 
> > > I'm saying that if functions like enable_msp() don't work reliably then
> > > removing some but not all of their functionality isn't an obviously good
> > > approach to fixing that.  Why does the other functionality work well but
> > > not this bit?  It sounds like there's some reference counting bug here
> > > is all...
> 
> > Yes, it started as a reference counting bug, due to the actual counter
> > not being shared between ux500-msp-i2s instances.
> 
> > That said, the actual fork of this driver deployed by STE internally
> > does not handle I2S pin sleep state, and I was not able to find any
> > other ASoC driver that does that, which seems reasonable to me as I
> > can't come up with a reason to put those pins in hi-z anyway.
> 
> But why does the rest of the code work well if the reference counting is
> wrong, it's in the middle of a big block of code?  This all smells like
> this change is papering over a specific symptom of some underlying issue
> - if that's not the case then it needs to be clearer why.

Well, the counting by itself is not wrong, it's just that the same pins
are used by both driver instances (ux500-msp-i2s.1 and ux500-msp-i2s.3)
but the actual counter is specific of each instance
(msp->pinctrl_rxtx_ref, if I'm not mistaken msp is different between the
capture and playback interfaces).

Also, right now the pins are only associated with the first instance,
as in:

DB8500_PIN("GPIO33_AF2", out_lo_slpm_nowkup, "ux500-msp-i2s.1"),

which does not seems to be correct as these are used also by
ux500-msp-i2s.3, hence why I sent the patch to set the pin as a hog.

> > If I understood the problem correctly you do that when you want to cut
> > power completely to some peripherals to avoid spurious current paths,
> > and that should not be the case for the audio codec, especially in this
> > case where it's part of a big multifuntion IC.
> 
> Being a MFD should have nothing to do with this?

Ok, what I'm trying to say is that the codec used in this platform
should be able to handle sleep modes without requiring any
reconfiguration of the digital interface on the SoC side.  In support of
this the fact that the STE fork of the driver does not do that, and the
same goes for all other ASoC drivers currently in mainline.

Fabio

-- 
Fabio Baltieri

Re: [PATCH 3/6] ASoC: ux500: Drop pinctrl sleep support

[Fabio Baltieri]

On Wed, May 08, 2013 at 04:17:23PM +0200, Fabio Baltieri wrote:
> > > If I understood the problem correctly you do that when you want to cut
> > > power completely to some peripherals to avoid spurious current paths,
> > > and that should not be the case for the audio codec, especially in this
> > > case where it's part of a big multifuntion IC.
> > 
> > Being a MFD should have nothing to do with this?
> 
> Ok, what I'm trying to say is that the codec used in this platform
> should be able to handle sleep modes without requiring any
> reconfiguration of the digital interface on the SoC side.  In support of
> this the fact that the STE fork of the driver does not do that, and the
> same goes for all other ASoC drivers currently in mainline.

And by the way, if the current code is *really* setting the digital
audio bus pins in hi-z mode (without any pull-up/down/keeper) as it
claims, this is not just usless, it's plain wrong.  The bus should never
be left floating on both sides, right?

Fabio

-- 
Fabio Baltieri

Re: [PATCH 3/6] ASoC: ux500: Drop pinctrl sleep support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 758 bytes --]

On Wed, May 08, 2013 at 04:27:34PM +0200, Fabio Baltieri wrote:
> On Wed, May 08, 2013 at 04:17:23PM +0200, Fabio Baltieri wrote:

> > Ok, what I'm trying to say is that the codec used in this platform
> > should be able to handle sleep modes without requiring any
> > reconfiguration of the digital interface on the SoC side.  In support of
> > this the fact that the STE fork of the driver does not do that, and the
> > same goes for all other ASoC drivers currently in mainline.

> And by the way, if the current code is *really* setting the digital
> audio bus pins in hi-z mode (without any pull-up/down/keeper) as it
> claims, this is not just usless, it's plain wrong.  The bus should never
> be left floating on both sides, right?

Probably not, no.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/6] ASoC: ux500: Drop pinctrl sleep support

[Lee Jones]

On Wed, 08 May 2013, Mark Brown wrote:

> On Wed, May 08, 2013 at 04:27:34PM +0200, Fabio Baltieri wrote:
> > On Wed, May 08, 2013 at 04:17:23PM +0200, Fabio Baltieri wrote:
> 
> > > Ok, what I'm trying to say is that the codec used in this platform
> > > should be able to handle sleep modes without requiring any
> > > reconfiguration of the digital interface on the SoC side.  In support of
> > > this the fact that the STE fork of the driver does not do that, and the
> > > same goes for all other ASoC drivers currently in mainline.
> 
> > And by the way, if the current code is *really* setting the digital
> > audio bus pins in hi-z mode (without any pull-up/down/keeper) as it
> > claims, this is not just usless, it's plain wrong.  The bus should never
> > be left floating on both sides, right?
> 
> Probably not, no.

Why don't we wait and see what LinusW says?

If anyone would know, it's him.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 3/6] ASoC: ux500: Drop pinctrl sleep support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 189 bytes --]

On Wed, May 08, 2013 at 04:07:29PM +0100, Lee Jones wrote:

> Why don't we wait and see what LinusW says?

> If anyone would know, it's him.

This stuff is all kind of off the point now...

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/6] ASoC: ux500: Drop pinctrl sleep support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1758 bytes --]

On Wed, May 08, 2013 at 04:17:23PM +0200, Fabio Baltieri wrote:
> On Wed, May 08, 2013 at 02:54:13PM +0100, Mark Brown wrote:

> > But why does the rest of the code work well if the reference counting is
> > wrong, it's in the middle of a big block of code?  This all smells like
> > this change is papering over a specific symptom of some underlying issue
> > - if that's not the case then it needs to be clearer why.

> Well, the counting by itself is not wrong, it's just that the same pins
> are used by both driver instances (ux500-msp-i2s.1 and ux500-msp-i2s.3)
> but the actual counter is specific of each instance
> (msp->pinctrl_rxtx_ref, if I'm not mistaken msp is different between the
> capture and playback interfaces).

So if these pins are being shared between the instances then surely
there are other interdependencies that need to be taken care of Is that
happening?  For example if the clocks are shared then is the code
currently stopping the interfaces being configured with incompatible
sample rates or word sizes?

It seems like the code is definitely buggy here but is this really all
that's buggy or is it just the symptom you happen to have seen?

> Ok, what I'm trying to say is that the codec used in this platform
> should be able to handle sleep modes without requiring any
> reconfiguration of the digital interface on the SoC side.  In support of
> this the fact that the STE fork of the driver does not do that, and the
> same goes for all other ASoC drivers currently in mainline.

Well, most things would probably be able to get some small benefit from
doing this, it's just that we've not had any real support for managing
pin muxing - one of the goals of adding pinctrl is to make it easier to
deploy this sort of thing.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/6] ASoC: ux500: Drop pinctrl sleep support

[Fabio Baltieri]

On Wed, May 08, 2013 at 03:29:38PM +0100, Mark Brown wrote:
> On Wed, May 08, 2013 at 04:17:23PM +0200, Fabio Baltieri wrote:
> > On Wed, May 08, 2013 at 02:54:13PM +0100, Mark Brown wrote:
> 
> > > But why does the rest of the code work well if the reference counting is
> > > wrong, it's in the middle of a big block of code?  This all smells like
> > > this change is papering over a specific symptom of some underlying issue
> > > - if that's not the case then it needs to be clearer why.
> 
> > Well, the counting by itself is not wrong, it's just that the same pins
> > are used by both driver instances (ux500-msp-i2s.1 and ux500-msp-i2s.3)
> > but the actual counter is specific of each instance
> > (msp->pinctrl_rxtx_ref, if I'm not mistaken msp is different between the
> > capture and playback interfaces).
> 
> So if these pins are being shared between the instances then surely
> there are other interdependencies that need to be taken care of Is that
> happening?  For example if the clocks are shared then is the code
> currently stopping the interfaces being configured with incompatible
> sample rates or word sizes?

That's an interesting point.  Mixed format, rate and channel counts
seems to work just fine, but that's probably because the supported rate
and format is locked by the codec to 48k + S16_LE.  This is in line with
what is specified on the ab8500 datasheet:

---
The AB8500 audio module exchanges audio data through the two digital
interfaces at a fixed 48 kHz rate in different formats with up to 8
channels per interface.
---

> It seems like the code is definitely buggy here but is this really all
> that's buggy or is it just the symptom you happen to have seen?

For the pinctrl code drop I still think that it should go, but really
I leave the last word to LinusW as suggested.

For the format mix, my guess is that you are probably right and this
thing may just fall apart if used on another codec.

On the other side, I don't think anybody is using the ux500 with
anything else than the ab8500, so I don't even know how would I test and
fix this.

> > Ok, what I'm trying to say is that the codec used in this platform
> > should be able to handle sleep modes without requiring any
> > reconfiguration of the digital interface on the SoC side.  In support of
> > this the fact that the STE fork of the driver does not do that, and the
> > same goes for all other ASoC drivers currently in mainline.
> 
> Well, most things would probably be able to get some small benefit from
> doing this, it's just that we've not had any real support for managing
> pin muxing - one of the goals of adding pinctrl is to make it easier to
> deploy this sort of thing.

Pin muxing is handled transparently.  The real problem is sleep mode
switching, but that's really not needed if the codec has a proper low
power mode.

Fabio

-- 
Fabio Baltieri

Re: [PATCH 3/6] ASoC: ux500: Drop pinctrl sleep support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1930 bytes --]

On Wed, May 08, 2013 at 05:48:05PM +0200, Fabio Baltieri wrote:
> On Wed, May 08, 2013 at 03:29:38PM +0100, Mark Brown wrote:

> > So if these pins are being shared between the instances then surely
> > there are other interdependencies that need to be taken care of Is that
> > happening?  For example if the clocks are shared then is the code
> > currently stopping the interfaces being configured with incompatible
> > sample rates or word sizes?

> That's an interesting point.  Mixed format, rate and channel counts
> seems to work just fine, but that's probably because the supported rate
> and format is locked by the codec to 48k + S16_LE.  This is in line with
> what is specified on the ab8500 datasheet:
> 
> ---
> The AB8500 audio module exchanges audio data through the two digital
> interfaces at a fixed 48 kHz rate in different formats with up to 8
> channels per interface.
> ---

That sounds like there's still some potential problems with the channel
count, though?

> On the other side, I don't think anybody is using the ux500 with
> anything else than the ab8500, so I don't even know how would I test and
> fix this.

The fix should simply be a case of setting constraints when opening
additional interfaces based on the configuration of the primary
interface.  You can test this sort of thing by simply verifying that it
doesn't break the existing cases.

> > Well, most things would probably be able to get some small benefit from
> > doing this, it's just that we've not had any real support for managing
> > pin muxing - one of the goals of adding pinctrl is to make it easier to
> > deploy this sort of thing.

> Pin muxing is handled transparently.  The real problem is sleep mode
> switching, but that's really not needed if the codec has a proper low
> power mode.

I'm not following that at all I'm afraid - I don't understand the
relevance of the CODEC?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/6] ASoC: ux500: Drop pinctrl sleep support

[Fabio Baltieri]

On Thu, May 09, 2013 at 10:41:03AM +0100, Mark Brown wrote:
> On Wed, May 08, 2013 at 05:48:05PM +0200, Fabio Baltieri wrote:
> > On Wed, May 08, 2013 at 03:29:38PM +0100, Mark Brown wrote:
> 
> > > So if these pins are being shared between the instances then surely
> > > there are other interdependencies that need to be taken care of Is that
> > > happening?  For example if the clocks are shared then is the code
> > > currently stopping the interfaces being configured with incompatible
> > > sample rates or word sizes?
> 
> > That's an interesting point.  Mixed format, rate and channel counts
> > seems to work just fine, but that's probably because the supported rate
> > and format is locked by the codec to 48k + S16_LE.  This is in line with
> > what is specified on the ab8500 datasheet:
> > 
> > ---
> > The AB8500 audio module exchanges audio data through the two digital
> > interfaces at a fixed 48 kHz rate in different formats with up to 8
> > channels per interface.
> > ---
> 
> That sounds like there's still some potential problems with the channel
> count, though?

Ok I'll try to figure out how to properly handle this. Right now the
timeslot mapping is exposed to the userspace with a series of dapm
widgets at codec level, and the same bits are set in the tdm api
depending on the channel counts.

> > On the other side, I don't think anybody is using the ux500 with
> > anything else than the ab8500, so I don't even know how would I test and
> > fix this.
> 
> The fix should simply be a case of setting constraints when opening
> additional interfaces based on the configuration of the primary
> interface.  You can test this sort of thing by simply verifying that it
> doesn't break the existing cases.

Ok.

> > > Well, most things would probably be able to get some small benefit from
> > > doing this, it's just that we've not had any real support for managing
> > > pin muxing - one of the goals of adding pinctrl is to make it easier to
> > > deploy this sort of thing.
> 
> > Pin muxing is handled transparently.  The real problem is sleep mode
> > switching, but that's really not needed if the codec has a proper low
> > power mode.
> 
> I'm not following that at all I'm afraid - I don't understand the
> relevance of the CODEC?

My understanding is that ASoC may not need explicit pinctrl support
because default pin assignments is already handled at pinctrl framework
level, and pinctrl sleep mode may not be necessary at all, unless
someone else can come out with a good reason for that.

Fabio

-- 
Fabio Baltieri

Re: [PATCH 3/6] ASoC: ux500: Drop pinctrl sleep support

[Linus Walleij]

On Wed, May 8, 2013 at 5:48 PM, Fabio Baltieri
<fabio.baltieri@linaro.org> wrote:
> On Wed, May 08, 2013 at 03:29:38PM +0100, Mark Brown wrote:

>> It seems like the code is definitely buggy here but is this really all
>> that's buggy or is it just the symptom you happen to have seen?
>
> For the pinctrl code drop I still think that it should go, but really
> I leave the last word to LinusW as suggested.

Kill it then.

At the time the driver was upstreamed, it was using the old
nmk_config_pins() interface to switch the pins.

All I did was convert them to use pinctrl. See commit
08d98fe0e81cd9424ef2451ed13afe91a9a26f9f

If nobody knows why it was doing that in the first place, let's
just kill it.

Yours,
Linus Walleij

[PATCH 4/6] ASoC: ux500: Update tx tdm slots configuration

[Fabio Baltieri]

Update ab8500-codec and mop500_ab8500 tx slot configuration to reflect
the actual one used by STE.  Also update a wrong comment in the process.

Signed-off-by: Fabio Baltieri <fabio.baltieri@linaro.org>
---
 sound/soc/codecs/ab8500-codec.c | 20 ++++++++++----------
 sound/soc/ux500/mop500_ab8500.c |  4 ++--
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/sound/soc/codecs/ab8500-codec.c b/sound/soc/codecs/ab8500-codec.c
index 3126cac..7c026d4 100644
--- a/sound/soc/codecs/ab8500-codec.c
+++ b/sound/soc/codecs/ab8500-codec.c
@@ -2300,18 +2300,18 @@ static int ab8500_codec_set_dai_tdm_slot(struct snd_soc_dai *dai,
 	case 0:
 		break;
 	case 1:
-		/* Slot 9 -> DA_IN1 & DA_IN3 */
-		snd_soc_update_bits(codec, AB8500_DASLOTCONF1, mask, 11);
-		snd_soc_update_bits(codec, AB8500_DASLOTCONF3, mask, 11);
-		snd_soc_update_bits(codec, AB8500_DASLOTCONF2, mask, 11);
-		snd_soc_update_bits(codec, AB8500_DASLOTCONF4, mask, 11);
+		/* Slot 8 -> DA_IN1 to DA_IN4 */
+		snd_soc_update_bits(codec, AB8500_DASLOTCONF1, mask, 8);
+		snd_soc_update_bits(codec, AB8500_DASLOTCONF3, mask, 8);
+		snd_soc_update_bits(codec, AB8500_DASLOTCONF2, mask, 8);
+		snd_soc_update_bits(codec, AB8500_DASLOTCONF4, mask, 8);
 		break;
 	case 2:
-		/* Slot 9 -> DA_IN1 & DA_IN3, Slot 11 -> DA_IN2 & DA_IN4 */
-		snd_soc_update_bits(codec, AB8500_DASLOTCONF1, mask, 9);
-		snd_soc_update_bits(codec, AB8500_DASLOTCONF3, mask, 9);
-		snd_soc_update_bits(codec, AB8500_DASLOTCONF2, mask, 11);
-		snd_soc_update_bits(codec, AB8500_DASLOTCONF4, mask, 11);
+		/* Slot 8 -> DA_IN1 & DA_IN3, Slot 9 -> DA_IN2 & DA_IN4 */
+		snd_soc_update_bits(codec, AB8500_DASLOTCONF1, mask, 8);
+		snd_soc_update_bits(codec, AB8500_DASLOTCONF3, mask, 8);
+		snd_soc_update_bits(codec, AB8500_DASLOTCONF2, mask, 9);
+		snd_soc_update_bits(codec, AB8500_DASLOTCONF4, mask, 9);
 
 		break;
 	case 8:
diff --git a/sound/soc/ux500/mop500_ab8500.c b/sound/soc/ux500/mop500_ab8500.c
index 4606194..44899f7 100644
--- a/sound/soc/ux500/mop500_ab8500.c
+++ b/sound/soc/ux500/mop500_ab8500.c
@@ -28,8 +28,8 @@
 #include "ux500_msp_dai.h"
 #include "../codecs/ab8500-codec.h"
 
-#define TX_SLOT_MONO	0x0008
-#define TX_SLOT_STEREO	0x000a
+#define TX_SLOT_MONO	0x0001
+#define TX_SLOT_STEREO	0x0003
 #define RX_SLOT_MONO	0x0001
 #define RX_SLOT_STEREO	0x0003
 #define TX_SLOT_8CH	0x00FF
-- 
1.8.2

Re: [PATCH 4/6] ASoC: ux500: Update tx tdm slots configuration

[Lee Jones]

On Wed, 08 May 2013, Fabio Baltieri wrote:

> Update ab8500-codec and mop500_ab8500 tx slot configuration to reflect
> the actual one used by STE.  Also update a wrong comment in the process.
> 
> Signed-off-by: Fabio Baltieri <fabio.baltieri@linaro.org>
> ---
>  sound/soc/codecs/ab8500-codec.c | 20 ++++++++++----------
>  sound/soc/ux500/mop500_ab8500.c |  4 ++--
>  2 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/sound/soc/codecs/ab8500-codec.c b/sound/soc/codecs/ab8500-codec.c
> index 3126cac..7c026d4 100644
> --- a/sound/soc/codecs/ab8500-codec.c
> +++ b/sound/soc/codecs/ab8500-codec.c
> @@ -2300,18 +2300,18 @@ static int ab8500_codec_set_dai_tdm_slot(struct snd_soc_dai *dai,
>  	case 0:
>  		break;
>  	case 1:
> -		/* Slot 9 -> DA_IN1 & DA_IN3 */
> -		snd_soc_update_bits(codec, AB8500_DASLOTCONF1, mask, 11);
> -		snd_soc_update_bits(codec, AB8500_DASLOTCONF3, mask, 11);
> -		snd_soc_update_bits(codec, AB8500_DASLOTCONF2, mask, 11);
> -		snd_soc_update_bits(codec, AB8500_DASLOTCONF4, mask, 11);
> +		/* Slot 8 -> DA_IN1 to DA_IN4 */
> +		snd_soc_update_bits(codec, AB8500_DASLOTCONF1, mask, 8);
> +		snd_soc_update_bits(codec, AB8500_DASLOTCONF3, mask, 8);
> +		snd_soc_update_bits(codec, AB8500_DASLOTCONF2, mask, 8);
> +		snd_soc_update_bits(codec, AB8500_DASLOTCONF4, mask, 8);
>  		break;
>  	case 2:
> -		/* Slot 9 -> DA_IN1 & DA_IN3, Slot 11 -> DA_IN2 & DA_IN4 */
> -		snd_soc_update_bits(codec, AB8500_DASLOTCONF1, mask, 9);
> -		snd_soc_update_bits(codec, AB8500_DASLOTCONF3, mask, 9);
> -		snd_soc_update_bits(codec, AB8500_DASLOTCONF2, mask, 11);
> -		snd_soc_update_bits(codec, AB8500_DASLOTCONF4, mask, 11);
> +		/* Slot 8 -> DA_IN1 & DA_IN3, Slot 9 -> DA_IN2 & DA_IN4 */
> +		snd_soc_update_bits(codec, AB8500_DASLOTCONF1, mask, 8);
> +		snd_soc_update_bits(codec, AB8500_DASLOTCONF3, mask, 8);
> +		snd_soc_update_bits(codec, AB8500_DASLOTCONF2, mask, 9);
> +		snd_soc_update_bits(codec, AB8500_DASLOTCONF4, mask, 9);
>  
>  		break;
>  	case 8:
> diff --git a/sound/soc/ux500/mop500_ab8500.c b/sound/soc/ux500/mop500_ab8500.c
> index 4606194..44899f7 100644
> --- a/sound/soc/ux500/mop500_ab8500.c
> +++ b/sound/soc/ux500/mop500_ab8500.c
> @@ -28,8 +28,8 @@
>  #include "ux500_msp_dai.h"
>  #include "../codecs/ab8500-codec.h"
>  
> -#define TX_SLOT_MONO	0x0008
> -#define TX_SLOT_STEREO	0x000a
> +#define TX_SLOT_MONO	0x0001
> +#define TX_SLOT_STEREO	0x0003
>  #define RX_SLOT_MONO	0x0001
>  #define RX_SLOT_STEREO	0x0003
>  #define TX_SLOT_8CH	0x00FF

Looks fine: Acked-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 4/6] ASoC: ux500: Update tx tdm slots configuration

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 577 bytes --]

On Wed, May 08, 2013 at 09:14:19AM +0200, Fabio Baltieri wrote:
> Update ab8500-codec and mop500_ab8500 tx slot configuration to reflect
> the actual one used by STE.  Also update a wrong comment in the process.

This seems wrong, the individual chip drivers should just be doing
whatever they're being told by the machine driver.  Sounds like there's
two fixes needed here - one is to change the TDM API so that the chip
drivers are just implementing configuration supplied by the machine
driver and the other is to change the configuration being done to
whatever is desired.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 4/6] ASoC: ux500: Update tx tdm slots configuration

[Lee Jones]

On Wed, 08 May 2013, Mark Brown wrote:

> On Wed, May 08, 2013 at 09:14:19AM +0200, Fabio Baltieri wrote:
> > Update ab8500-codec and mop500_ab8500 tx slot configuration to reflect
> > the actual one used by STE.  Also update a wrong comment in the process.
> 
> This seems wrong, the individual chip drivers should just be doing
> whatever they're being told by the machine driver.  Sounds like there's
> two fixes needed here - one is to change the TDM API so that the chip
> drivers are just implementing configuration supplied by the machine
> driver and the other is to change the configuration being done to
> whatever is desired.

Do you mean that the original implementation is incorrect, or that
this patch is doing the wrong thing? I think this patch is a bugfix
rather than a opportunity to refactor the driver.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 4/6] ASoC: ux500: Update tx tdm slots configuration

[Fabio Baltieri]

On Wed, May 08, 2013 at 12:11:10PM +0100, Lee Jones wrote:
> On Wed, 08 May 2013, Mark Brown wrote:
> 
> > On Wed, May 08, 2013 at 09:14:19AM +0200, Fabio Baltieri wrote:
> > > Update ab8500-codec and mop500_ab8500 tx slot configuration to reflect
> > > the actual one used by STE.  Also update a wrong comment in the process.
> > 
> > This seems wrong, the individual chip drivers should just be doing
> > whatever they're being told by the machine driver.  Sounds like there's
> > two fixes needed here - one is to change the TDM API so that the chip
> > drivers are just implementing configuration supplied by the machine
> > driver and the other is to change the configuration being done to
> > whatever is desired.
> 
> Do you mean that the original implementation is incorrect, or that
> this patch is doing the wrong thing? I think this patch is a bugfix
> rather than a opportunity to refactor the driver.

That's correct, the idea for this whole series was just to fix the
existing codebase so that it can be enabled and used for regression
testing, rather than mixing up bug fixes and code cleanups.

Fabio

-- 
Fabio Baltieri

Re: [PATCH 4/6] ASoC: ux500: Update tx tdm slots configuration

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1320 bytes --]

On Wed, May 08, 2013 at 12:11:10PM +0100, Lee Jones wrote:
> On Wed, 08 May 2013, Mark Brown wrote:

> > On Wed, May 08, 2013 at 09:14:19AM +0200, Fabio Baltieri wrote:
> > > Update ab8500-codec and mop500_ab8500 tx slot configuration to reflect
> > > the actual one used by STE.  Also update a wrong comment in the process.

> > This seems wrong, the individual chip drivers should just be doing
> > whatever they're being told by the machine driver.  Sounds like there's
> > two fixes needed here - one is to change the TDM API so that the chip
> > drivers are just implementing configuration supplied by the machine
> > driver and the other is to change the configuration being done to
> > whatever is desired.

> Do you mean that the original implementation is incorrect, or that
> this patch is doing the wrong thing? I think this patch is a bugfix
> rather than a opportunity to refactor the driver.

I mean that the original implementation is incorrect and this is just
continuing the problem - the reason that we don't want to have this
stuff hard coded in the device drivers is that we should be doing this
sort of configuration in the machine drivers so only the relevant
systems are affected by configuration updates.  Putting this in the
drivers tends to lead to a series of configuration changes like this.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 4/6] ASoC: ux500: Update tx tdm slots configuration

[Fabio Baltieri]

On Wed, May 08, 2013 at 12:01:49PM +0100, Mark Brown wrote:
> On Wed, May 08, 2013 at 09:14:19AM +0200, Fabio Baltieri wrote:
> > Update ab8500-codec and mop500_ab8500 tx slot configuration to reflect
> > the actual one used by STE.  Also update a wrong comment in the process.
> 
> This seems wrong, the individual chip drivers should just be doing
> whatever they're being told by the machine driver.  Sounds like there's
> two fixes needed here - one is to change the TDM API so that the chip
> drivers are just implementing configuration supplied by the machine
> driver and the other is to change the configuration being done to
> whatever is desired.

Ok so, this patch was really just going to slightly align the
configuration with the STE driver.  I'll drop it and just fix the wrong
comment as a trivial patch.

For the reimplementation with channel configuration from machine driver,
I actually went through that, but was not able to find the reason why
there is a slot offset somewhere (example: I request 0x0001 for first
slot on the DAI and that shows up on slot 8 on the codec), so I just
updated the hardcoded value.  I'll try to get some explanation for that.

Fabio

-- 
Fabio Baltieri

[PATCH 5/6] ASoC: ux500: Swap even/odd AD slot definitions

[Fabio Baltieri]

AD slots definitions for ab8500 codec were erroneously swapped between
even and odd channels.  Fix this by swapping the definitions to be
coherent with the channel number.

Signed-off-by: Fabio Baltieri <fabio.baltieri@linaro.org>
---
 sound/soc/codecs/ab8500-codec.h | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/sound/soc/codecs/ab8500-codec.h b/sound/soc/codecs/ab8500-codec.h
index 114f69a..306d0bc 100644
--- a/sound/soc/codecs/ab8500-codec.h
+++ b/sound/soc/codecs/ab8500-codec.h
@@ -348,25 +348,25 @@
 
 /* AB8500_ADSLOTSELX */
 #define AB8500_ADSLOTSELX_AD_OUT1_TO_SLOT_ODD	0x00
-#define AB8500_ADSLOTSELX_AD_OUT2_TO_SLOT_ODD	0x01
-#define AB8500_ADSLOTSELX_AD_OUT3_TO_SLOT_ODD	0x02
-#define AB8500_ADSLOTSELX_AD_OUT4_TO_SLOT_ODD	0x03
-#define AB8500_ADSLOTSELX_AD_OUT5_TO_SLOT_ODD	0x04
-#define AB8500_ADSLOTSELX_AD_OUT6_TO_SLOT_ODD	0x05
-#define AB8500_ADSLOTSELX_AD_OUT7_TO_SLOT_ODD	0x06
-#define AB8500_ADSLOTSELX_AD_OUT8_TO_SLOT_ODD	0x07
-#define AB8500_ADSLOTSELX_ZEROES_TO_SLOT_ODD	0x08
-#define AB8500_ADSLOTSELX_TRISTATE_TO_SLOT_ODD	0x0F
+#define AB8500_ADSLOTSELX_AD_OUT2_TO_SLOT_ODD	0x10
+#define AB8500_ADSLOTSELX_AD_OUT3_TO_SLOT_ODD	0x20
+#define AB8500_ADSLOTSELX_AD_OUT4_TO_SLOT_ODD	0x30
+#define AB8500_ADSLOTSELX_AD_OUT5_TO_SLOT_ODD	0x40
+#define AB8500_ADSLOTSELX_AD_OUT6_TO_SLOT_ODD	0x50
+#define AB8500_ADSLOTSELX_AD_OUT7_TO_SLOT_ODD	0x60
+#define AB8500_ADSLOTSELX_AD_OUT8_TO_SLOT_ODD	0x70
+#define AB8500_ADSLOTSELX_ZEROES_TO_SLOT_ODD	0x80
+#define AB8500_ADSLOTSELX_TRISTATE_TO_SLOT_ODD	0xF0
 #define AB8500_ADSLOTSELX_AD_OUT1_TO_SLOT_EVEN	0x00
-#define AB8500_ADSLOTSELX_AD_OUT2_TO_SLOT_EVEN	0x10
-#define AB8500_ADSLOTSELX_AD_OUT3_TO_SLOT_EVEN	0x20
-#define AB8500_ADSLOTSELX_AD_OUT4_TO_SLOT_EVEN	0x30
-#define AB8500_ADSLOTSELX_AD_OUT5_TO_SLOT_EVEN	0x40
-#define AB8500_ADSLOTSELX_AD_OUT6_TO_SLOT_EVEN	0x50
-#define AB8500_ADSLOTSELX_AD_OUT7_TO_SLOT_EVEN	0x60
-#define AB8500_ADSLOTSELX_AD_OUT8_TO_SLOT_EVEN	0x70
-#define AB8500_ADSLOTSELX_ZEROES_TO_SLOT_EVEN	0x80
-#define AB8500_ADSLOTSELX_TRISTATE_TO_SLOT_EVEN	0xF0
+#define AB8500_ADSLOTSELX_AD_OUT2_TO_SLOT_EVEN	0x01
+#define AB8500_ADSLOTSELX_AD_OUT3_TO_SLOT_EVEN	0x02
+#define AB8500_ADSLOTSELX_AD_OUT4_TO_SLOT_EVEN	0x03
+#define AB8500_ADSLOTSELX_AD_OUT5_TO_SLOT_EVEN	0x04
+#define AB8500_ADSLOTSELX_AD_OUT6_TO_SLOT_EVEN	0x05
+#define AB8500_ADSLOTSELX_AD_OUT7_TO_SLOT_EVEN	0x06
+#define AB8500_ADSLOTSELX_AD_OUT8_TO_SLOT_EVEN	0x07
+#define AB8500_ADSLOTSELX_ZEROES_TO_SLOT_EVEN	0x08
+#define AB8500_ADSLOTSELX_TRISTATE_TO_SLOT_EVEN	0x0F
 #define AB8500_ADSLOTSELX_EVEN_SHIFT		0
 #define AB8500_ADSLOTSELX_ODD_SHIFT		4
 
-- 
1.8.2

Re: [PATCH 5/6] ASoC: ux500: Swap even/odd AD slot definitions

[Lee Jones]

On Wed, 08 May 2013, Fabio Baltieri wrote:

> AD slots definitions for ab8500 codec were erroneously swapped between
> even and odd channels.  Fix this by swapping the definitions to be
> coherent with the channel number.
> 
> Signed-off-by: Fabio Baltieri <fabio.baltieri@linaro.org>
> ---
>  sound/soc/codecs/ab8500-codec.h | 36 ++++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/sound/soc/codecs/ab8500-codec.h b/sound/soc/codecs/ab8500-codec.h
> index 114f69a..306d0bc 100644
> --- a/sound/soc/codecs/ab8500-codec.h
> +++ b/sound/soc/codecs/ab8500-codec.h
> @@ -348,25 +348,25 @@
>  
>  /* AB8500_ADSLOTSELX */
>  #define AB8500_ADSLOTSELX_AD_OUT1_TO_SLOT_ODD	0x00
> -#define AB8500_ADSLOTSELX_AD_OUT2_TO_SLOT_ODD	0x01
> -#define AB8500_ADSLOTSELX_AD_OUT3_TO_SLOT_ODD	0x02
> -#define AB8500_ADSLOTSELX_AD_OUT4_TO_SLOT_ODD	0x03
> -#define AB8500_ADSLOTSELX_AD_OUT5_TO_SLOT_ODD	0x04
> -#define AB8500_ADSLOTSELX_AD_OUT6_TO_SLOT_ODD	0x05
> -#define AB8500_ADSLOTSELX_AD_OUT7_TO_SLOT_ODD	0x06
> -#define AB8500_ADSLOTSELX_AD_OUT8_TO_SLOT_ODD	0x07
> -#define AB8500_ADSLOTSELX_ZEROES_TO_SLOT_ODD	0x08
> -#define AB8500_ADSLOTSELX_TRISTATE_TO_SLOT_ODD	0x0F
> +#define AB8500_ADSLOTSELX_AD_OUT2_TO_SLOT_ODD	0x10
> +#define AB8500_ADSLOTSELX_AD_OUT3_TO_SLOT_ODD	0x20
> +#define AB8500_ADSLOTSELX_AD_OUT4_TO_SLOT_ODD	0x30
> +#define AB8500_ADSLOTSELX_AD_OUT5_TO_SLOT_ODD	0x40
> +#define AB8500_ADSLOTSELX_AD_OUT6_TO_SLOT_ODD	0x50
> +#define AB8500_ADSLOTSELX_AD_OUT7_TO_SLOT_ODD	0x60
> +#define AB8500_ADSLOTSELX_AD_OUT8_TO_SLOT_ODD	0x70
> +#define AB8500_ADSLOTSELX_ZEROES_TO_SLOT_ODD	0x80
> +#define AB8500_ADSLOTSELX_TRISTATE_TO_SLOT_ODD	0xF0
>  #define AB8500_ADSLOTSELX_AD_OUT1_TO_SLOT_EVEN	0x00
> -#define AB8500_ADSLOTSELX_AD_OUT2_TO_SLOT_EVEN	0x10
> -#define AB8500_ADSLOTSELX_AD_OUT3_TO_SLOT_EVEN	0x20
> -#define AB8500_ADSLOTSELX_AD_OUT4_TO_SLOT_EVEN	0x30
> -#define AB8500_ADSLOTSELX_AD_OUT5_TO_SLOT_EVEN	0x40
> -#define AB8500_ADSLOTSELX_AD_OUT6_TO_SLOT_EVEN	0x50
> -#define AB8500_ADSLOTSELX_AD_OUT7_TO_SLOT_EVEN	0x60
> -#define AB8500_ADSLOTSELX_AD_OUT8_TO_SLOT_EVEN	0x70
> -#define AB8500_ADSLOTSELX_ZEROES_TO_SLOT_EVEN	0x80
> -#define AB8500_ADSLOTSELX_TRISTATE_TO_SLOT_EVEN	0xF0
> +#define AB8500_ADSLOTSELX_AD_OUT2_TO_SLOT_EVEN	0x01
> +#define AB8500_ADSLOTSELX_AD_OUT3_TO_SLOT_EVEN	0x02
> +#define AB8500_ADSLOTSELX_AD_OUT4_TO_SLOT_EVEN	0x03
> +#define AB8500_ADSLOTSELX_AD_OUT5_TO_SLOT_EVEN	0x04
> +#define AB8500_ADSLOTSELX_AD_OUT6_TO_SLOT_EVEN	0x05
> +#define AB8500_ADSLOTSELX_AD_OUT7_TO_SLOT_EVEN	0x06
> +#define AB8500_ADSLOTSELX_AD_OUT8_TO_SLOT_EVEN	0x07
> +#define AB8500_ADSLOTSELX_ZEROES_TO_SLOT_EVEN	0x08
> +#define AB8500_ADSLOTSELX_TRISTATE_TO_SLOT_EVEN	0x0F
>  #define AB8500_ADSLOTSELX_EVEN_SHIFT		0
>  #define AB8500_ADSLOTSELX_ODD_SHIFT		4

Acked-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 6/6] ASoC: ux500: Use the first two AD slots for capture

[Fabio Baltieri]

Set AD_OUT1 and AD_OUT2, corresponding to LINL and LINR pins, as the
default input slots for the capture interfaces.

Signed-off-by: Fabio Baltieri <fabio.baltieri@linaro.org>
---
 sound/soc/codecs/ab8500-codec.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/sound/soc/codecs/ab8500-codec.c b/sound/soc/codecs/ab8500-codec.c
index 7c026d4..9dafd52 100644
--- a/sound/soc/codecs/ab8500-codec.c
+++ b/sound/soc/codecs/ab8500-codec.c
@@ -2334,17 +2334,18 @@ static int ab8500_codec_set_dai_tdm_slot(struct snd_soc_dai *dai,
 	case 0:
 		break;
 	case 1:
-		/* AD_OUT3 -> slot 0 & 1 */
-		snd_soc_update_bits(codec, AB8500_ADSLOTSEL1, AB8500_MASK_ALL,
-				AB8500_ADSLOTSELX_AD_OUT3_TO_SLOT_EVEN |
-				AB8500_ADSLOTSELX_AD_OUT3_TO_SLOT_ODD);
+		/* AD_OUT1 -> slot 0 & 1 */
+		snd_soc_update_bits(codec, AB8500_ADSLOTSEL1,
+				AB8500_MASK_ALL,
+				AB8500_ADSLOTSELX_AD_OUT1_TO_SLOT_EVEN |
+				AB8500_ADSLOTSELX_AD_OUT1_TO_SLOT_ODD);
 		break;
 	case 2:
-		/* AD_OUT3 -> slot 0, AD_OUT2 -> slot 1 */
+		/* AD_OUT1 -> slot 0, AD_OUT2 -> slot 1 */
 		snd_soc_update_bits(codec,
 				AB8500_ADSLOTSEL1,
 				AB8500_MASK_ALL,
-				AB8500_ADSLOTSELX_AD_OUT3_TO_SLOT_EVEN |
+				AB8500_ADSLOTSELX_AD_OUT1_TO_SLOT_EVEN |
 				AB8500_ADSLOTSELX_AD_OUT2_TO_SLOT_ODD);
 		break;
 	case 8:
-- 
1.8.2

Re: [PATCH 6/6] ASoC: ux500: Use the first two AD slots for capture

[Lee Jones]

On Wed, 08 May 2013, Fabio Baltieri wrote:

> Set AD_OUT1 and AD_OUT2, corresponding to LINL and LINR pins, as the
> default input slots for the capture interfaces.
> 
> Signed-off-by: Fabio Baltieri <fabio.baltieri@linaro.org>
> ---
>  sound/soc/codecs/ab8500-codec.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/sound/soc/codecs/ab8500-codec.c b/sound/soc/codecs/ab8500-codec.c
> index 7c026d4..9dafd52 100644
> --- a/sound/soc/codecs/ab8500-codec.c
> +++ b/sound/soc/codecs/ab8500-codec.c
> @@ -2334,17 +2334,18 @@ static int ab8500_codec_set_dai_tdm_slot(struct snd_soc_dai *dai,
>  	case 0:
>  		break;
>  	case 1:
> -		/* AD_OUT3 -> slot 0 & 1 */
> -		snd_soc_update_bits(codec, AB8500_ADSLOTSEL1, AB8500_MASK_ALL,
> -				AB8500_ADSLOTSELX_AD_OUT3_TO_SLOT_EVEN |
> -				AB8500_ADSLOTSELX_AD_OUT3_TO_SLOT_ODD);
> +		/* AD_OUT1 -> slot 0 & 1 */
> +		snd_soc_update_bits(codec, AB8500_ADSLOTSEL1,
> +				AB8500_MASK_ALL,
> +				AB8500_ADSLOTSELX_AD_OUT1_TO_SLOT_EVEN |
> +				AB8500_ADSLOTSELX_AD_OUT1_TO_SLOT_ODD);
>  		break;
>  	case 2:
> -		/* AD_OUT3 -> slot 0, AD_OUT2 -> slot 1 */
> +		/* AD_OUT1 -> slot 0, AD_OUT2 -> slot 1 */
>  		snd_soc_update_bits(codec,
>  				AB8500_ADSLOTSEL1,
>  				AB8500_MASK_ALL,
> -				AB8500_ADSLOTSELX_AD_OUT3_TO_SLOT_EVEN |
> +				AB8500_ADSLOTSELX_AD_OUT1_TO_SLOT_EVEN |
>  				AB8500_ADSLOTSELX_AD_OUT2_TO_SLOT_ODD);
>  		break;
>  	case 8:

Acked-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 6/6] ASoC: ux500: Use the first two AD slots for capture

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 534 bytes --]

On Wed, May 08, 2013 at 09:14:21AM +0200, Fabio Baltieri wrote:
> Set AD_OUT1 and AD_OUT2, corresponding to LINL and LINR pins, as the
> default input slots for the capture interfaces.

If these are routing specific analogue inputs to specific timeslots then
this is routing that should be being exposed to DAPM as muxes, not hard
coded in the middle of TDM slot configuration (which is both obscure and
not what the TDM API is there for).  The TDM API should be purely about
placing the timeslots for the audio interface on the bus.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 6/6] ASoC: ux500: Use the first two AD slots for capture

[Lee Jones]

On Wed, 08 May 2013, Mark Brown wrote:

> On Wed, May 08, 2013 at 09:14:21AM +0200, Fabio Baltieri wrote:
> > Set AD_OUT1 and AD_OUT2, corresponding to LINL and LINR pins, as the
> > default input slots for the capture interfaces.
> 
> If these are routing specific analogue inputs to specific timeslots then
> this is routing that should be being exposed to DAPM as muxes, not hard
> coded in the middle of TDM slot configuration (which is both obscure and
> not what the TDM API is there for).  The TDM API should be purely about
> placing the timeslots for the audio interface on the bus.

Again, I think this patch is a valid bugfix. If the driver requires
refactoring, it should be done by someone else. That's not the purpose
of Fabio's role.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 6/6] ASoC: ux500: Use the first two AD slots for capture

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 696 bytes --]

On Wed, May 08, 2013 at 12:12:53PM +0100, Lee Jones wrote:
> On Wed, 08 May 2013, Mark Brown wrote:

> > If these are routing specific analogue inputs to specific timeslots then
> > this is routing that should be being exposed to DAPM as muxes, not hard
> > coded in the middle of TDM slot configuration (which is both obscure and
> > not what the TDM API is there for).  The TDM API should be purely about
> > placing the timeslots for the audio interface on the bus.

> Again, I think this patch is a valid bugfix. If the driver requires
> refactoring, it should be done by someone else. That's not the purpose
> of Fabio's role.

It's a configuraiton change not a bug fix as far as I can see.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 6/6] ASoC: ux500: Use the first two AD slots for capture

[Fabio Baltieri]

On Wed, May 08, 2013 at 11:56:54AM +0100, Mark Brown wrote:
> On Wed, May 08, 2013 at 09:14:21AM +0200, Fabio Baltieri wrote:
> > Set AD_OUT1 and AD_OUT2, corresponding to LINL and LINR pins, as the
> > default input slots for the capture interfaces.
> 
> If these are routing specific analogue inputs to specific timeslots then
> this is routing that should be being exposed to DAPM as muxes, not hard
> coded in the middle of TDM slot configuration (which is both obscure and
> not what the TDM API is there for).  The TDM API should be purely about
> placing the timeslots for the audio interface on the bus.

Ok, it makes sense.  Again, this patch was wrote as a fix to the
existing codebase to make the driver usable with the actual board
available to the community (the snowball).

Fabio

-- 
Fabio Baltieri