[alsa-devel] [PATCH 0/6] Second set of fixes for ux500 ASoC drivers
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(-)
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,
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", 78 to 11 corresponds to zero output "zeroes", 8
"zeroes", 9"zeroes", 10"zeroes", 11"tristate", 12"tristate", 13"tristate", 14 "tristate"}; 15
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"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
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", 7I'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"tristate", 12"tristate", 13"tristate", 14 "tristate"}; 15
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) &
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
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) &
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.
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.
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
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.
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.
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.
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.
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.
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.
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
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 {
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 {
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 {
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 {
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
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)?
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
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...
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
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?
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
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
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.
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.
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.
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
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?
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
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
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
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);
/* 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
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.
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.
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
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.
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
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
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
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:
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);
/* AD_OUT3 -> slot 0, AD_OUT2 -> slot 1 */
/* AD_OUT1 -> slot 0, AD_OUT2 -> slot 1 */
AB8500_ADSLOTSELX_AD_OUT3_TO_SLOT_EVEN |
AB8500_ADSLOTSELX_AD_OUT1_TO_SLOT_EVEN | AB8500_ADSLOTSELX_AD_OUT2_TO_SLOT_ODD);
Acked-by: Lee Jones lee.jones@linaro.org
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.
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.
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.
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
participants (4)
-
Fabio Baltieri -
Lee Jones -
Linus Walleij -
Mark Brown