[PATCH 0/4] davinci-mcasp: fix tdm_slots and CBM/CFS

[PATCH 0/4] davinci-mcasp: fix tdm_slots and CBM/CFS

[Ben Gardiner]

This patch series is comprised of three bugfixes and one cleanup that 
were performed during prototyping of McASP operation in codec clock-
master frame-slave mode.

First we noticed that the check of the number of tdm slots requested
by platform data was always returning true -- unrelated to CMB/CFS
mode.

Then a cleanup: the PDIR values set are currently based on magic 
numbers and there are  available bitfield definitions. Not strictly
needed but it makes the changes introduced in the last patch simpler
to read.

It was found that the hardware parameters assigned when 
codec clock-master frame-slave is requested were incorrect. This
change is required for correct CBM/CFS operation.

Finally, the direction of the pins is corrected to reflect the
implications of codec clock-master frame-slave -- i.e. mcasp clock-
input frame-output. This change is also required for correct operation.

The combination was tested with a logic analyzer and the hrtimer pwm
device from Bill Gatliff's PWM framework [1] on a da850evm with
hardware modifications to access the McASP lines.

[1] http://article.gmane.org/gmane.linux.kernel.embedded/3486/

Ben Gardiner (4):
  davinci-mcasp: correct tdm_slots limit
  davinci-mcasp: use bitfield definitions for PDIR
  davinci-mcasp: fix _CBM_CFS hw_params
  davinci-mcasp: fix _CBM_CFS pin directions

 sound/soc/davinci/davinci-mcasp.c |   19 ++++++++++++-------
 1 files changed, 12 insertions(+), 7 deletions(-)

[PATCH 1/4] davinci-mcasp: correct tdm_slots limit

[Ben Gardiner]

The current check for the number of tdm-slots specified by platform data is
always true (x >= 2 || x <= 32); therefore the else branch that warns of an
incorrect number of slots can never be taken.

Check that the number of tdm slots specified by platform data is between 2
and 32, inclusive.

Signed-off-by: Ben Gardiner <bengardiner-ScDXFp4xN3PN+rMO2ozGnw@public.gmane.org>
Reviewed-by: James Nuss <jamesnuss-ScDXFp4xN3PN+rMO2ozGnw@public.gmane.org>
---
 sound/soc/davinci/davinci-mcasp.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c
index fb55d2c..e595756 100644
--- a/sound/soc/davinci/davinci-mcasp.c
+++ b/sound/soc/davinci/davinci-mcasp.c
@@ -644,7 +644,7 @@ static void davinci_hw_param(struct davinci_audio_dev *dev, int stream)
 		mcasp_set_reg(dev->base + DAVINCI_MCASP_TXTDM_REG, mask);
 		mcasp_set_bits(dev->base + DAVINCI_MCASP_TXFMT_REG, TXORD);
 
-		if ((dev->tdm_slots >= 2) || (dev->tdm_slots <= 32))
+		if ((dev->tdm_slots >= 2) && (dev->tdm_slots <= 32))
 			mcasp_mod_bits(dev->base + DAVINCI_MCASP_TXFMCTL_REG,
 					FSXMOD(dev->tdm_slots), FSXMOD(0x1FF));
 		else
@@ -660,7 +660,7 @@ static void davinci_hw_param(struct davinci_audio_dev *dev, int stream)
 				AHCLKRE);
 		mcasp_set_reg(dev->base + DAVINCI_MCASP_RXTDM_REG, mask);
 
-		if ((dev->tdm_slots >= 2) || (dev->tdm_slots <= 32))
+		if ((dev->tdm_slots >= 2) && (dev->tdm_slots <= 32))
 			mcasp_mod_bits(dev->base + DAVINCI_MCASP_RXFMCTL_REG,
 					FSRMOD(dev->tdm_slots), FSRMOD(0x1FF));
 		else
-- 
1.7.1

[PATCH 1/4] davinci-mcasp: correct tdm_slots limit

[Ben Gardiner]

The current check for the number of tdm-slots specified by platform data is
always true (x >= 2 || x <= 32); therefore the else branch that warns of an
incorrect number of slots can never be taken.

Check that the number of tdm slots specified by platform data is between 2
and 32, inclusive.

Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca>
Reviewed-by: James Nuss <jamesnuss@nanometrics.ca>
---
 sound/soc/davinci/davinci-mcasp.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c
index fb55d2c..e595756 100644
--- a/sound/soc/davinci/davinci-mcasp.c
+++ b/sound/soc/davinci/davinci-mcasp.c
@@ -644,7 +644,7 @@ static void davinci_hw_param(struct davinci_audio_dev *dev, int stream)
 		mcasp_set_reg(dev->base + DAVINCI_MCASP_TXTDM_REG, mask);
 		mcasp_set_bits(dev->base + DAVINCI_MCASP_TXFMT_REG, TXORD);
 
-		if ((dev->tdm_slots >= 2) || (dev->tdm_slots <= 32))
+		if ((dev->tdm_slots >= 2) && (dev->tdm_slots <= 32))
 			mcasp_mod_bits(dev->base + DAVINCI_MCASP_TXFMCTL_REG,
 					FSXMOD(dev->tdm_slots), FSXMOD(0x1FF));
 		else
@@ -660,7 +660,7 @@ static void davinci_hw_param(struct davinci_audio_dev *dev, int stream)
 				AHCLKRE);
 		mcasp_set_reg(dev->base + DAVINCI_MCASP_RXTDM_REG, mask);
 
-		if ((dev->tdm_slots >= 2) || (dev->tdm_slots <= 32))
+		if ((dev->tdm_slots >= 2) && (dev->tdm_slots <= 32))
 			mcasp_mod_bits(dev->base + DAVINCI_MCASP_RXFMCTL_REG,
 					FSRMOD(dev->tdm_slots), FSRMOD(0x1FF));
 		else
-- 
1.7.1

Re: [PATCH 1/4] davinci-mcasp: correct tdm_slots limit

[Mark Brown]

On Thu, Apr 21, 2011 at 02:19:01PM -0400, Ben Gardiner wrote:
> The current check for the number of tdm-slots specified by platform data is
> always true (x >= 2 || x <= 32); therefore the else branch that warns of an
> incorrect number of slots can never be taken.

Applied all of these.  Please always try to ensure that your commit logs
are consistent with the rest of the subsystem so they don't need to be
rewritten.

Re: [PATCH 1/4] davinci-mcasp: correct tdm_slots limit

[Mark Brown]

On Thu, Apr 21, 2011 at 02:19:01PM -0400, Ben Gardiner wrote:
> The current check for the number of tdm-slots specified by platform data is
> always true (x >= 2 || x <= 32); therefore the else branch that warns of an
> incorrect number of slots can never be taken.

Applied all of these.  Please always try to ensure that your commit logs
are consistent with the rest of the subsystem so they don't need to be
rewritten.

Re: [PATCH 1/4] davinci-mcasp: correct tdm_slots limit

[Ben Gardiner]

On Tue, Apr 26, 2011 at 6:46 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Thu, Apr 21, 2011 at 02:19:01PM -0400, Ben Gardiner wrote:
>> The current check for the number of tdm-slots specified by platform data is
>> always true (x >= 2 || x <= 32); therefore the else branch that warns of an
>> incorrect number of slots can never be taken.
>
> Applied all of these.  Please always try to ensure that your commit logs
> are consistent with the rest of the subsystem so they don't need to be
> rewritten.

Thanks, Mark, for taking the patches anyways (and Liam for the Ack's)
-- Sorry I forgot the 'ASoC' tag (I noticed this patch was committed
as 049cfaa ASoC: davinci-mcasp: correct tdm_slots limit).

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca

Re: [PATCH 1/4] davinci-mcasp: correct tdm_slots limit

[Ben Gardiner]

On Tue, Apr 26, 2011 at 6:46 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Thu, Apr 21, 2011 at 02:19:01PM -0400, Ben Gardiner wrote:
>> The current check for the number of tdm-slots specified by platform data is
>> always true (x >= 2 || x <= 32); therefore the else branch that warns of an
>> incorrect number of slots can never be taken.
>
> Applied all of these.  Please always try to ensure that your commit logs
> are consistent with the rest of the subsystem so they don't need to be
> rewritten.

Thanks, Mark, for taking the patches anyways (and Liam for the Ack's)
-- Sorry I forgot the 'ASoC' tag (I noticed this patch was committed
as 049cfaa ASoC: davinci-mcasp: correct tdm_slots limit).

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca

[PATCH 2/4] davinci-mcasp: use bitfield definitions for PDIR

[Ben Gardiner]

The current driver creates value for set/clr of PDIR using (x<<26) instead
of the #defines that are convieniently made available.

Update the driver to use the bitfield definitions of PDIR. There is no
functional change introduced by this patch.

Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca>
Reviewed-by: James Nuss <jamesnuss@nanometrics.ca>
---
 sound/soc/davinci/davinci-mcasp.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c
index e595756..1aa24a1 100644
--- a/sound/soc/davinci/davinci-mcasp.c
+++ b/sound/soc/davinci/davinci-mcasp.c
@@ -434,7 +434,8 @@ static int davinci_mcasp_set_dai_fmt(struct snd_soc_dai *cpu_dai,
 		mcasp_set_bits(base + DAVINCI_MCASP_ACLKRCTL_REG, ACLKRE);
 		mcasp_set_bits(base + DAVINCI_MCASP_RXFMCTL_REG, AFSRE);
 
-		mcasp_set_bits(base + DAVINCI_MCASP_PDIR_REG, (0x7 << 26));
+		mcasp_set_bits(base + DAVINCI_MCASP_PDIR_REG,
+				ACLKX | AHCLKX | AFSX);
 		break;
 	case SND_SOC_DAIFMT_CBM_CFS:
 		/* codec is clock master and frame slave */
@@ -444,7 +445,8 @@ static int davinci_mcasp_set_dai_fmt(struct snd_soc_dai *cpu_dai,
 		mcasp_set_bits(base + DAVINCI_MCASP_ACLKRCTL_REG, ACLKRE);
 		mcasp_set_bits(base + DAVINCI_MCASP_RXFMCTL_REG, AFSRE);
 
-		mcasp_set_bits(base + DAVINCI_MCASP_PDIR_REG, (0x2d << 26));
+		mcasp_set_bits(base + DAVINCI_MCASP_PDIR_REG,
+				ACLKX | AFSX | ACLKR | AFSR);
 		break;
 	case SND_SOC_DAIFMT_CBM_CFM:
 		/* codec is clock and frame master */
@@ -454,7 +456,8 @@ static int davinci_mcasp_set_dai_fmt(struct snd_soc_dai *cpu_dai,
 		mcasp_clr_bits(base + DAVINCI_MCASP_ACLKRCTL_REG, ACLKRE);
 		mcasp_clr_bits(base + DAVINCI_MCASP_RXFMCTL_REG, AFSRE);
 
-		mcasp_clr_bits(base + DAVINCI_MCASP_PDIR_REG, (0x3f << 26));
+		mcasp_clr_bits(base + DAVINCI_MCASP_PDIR_REG,
+				ACLKX | AHCLKX | AFSX | ACLKR | AHCLKR | AFSR);
 		break;
 
 	default:
-- 
1.7.1

[PATCH 3/4] davinci-mcasp: fix _CBM_CFS hw_params

[Ben Gardiner]

The current davinci_mcasp_set_dai_fmt() sets bits ACLKXE and ACLKRE (CLKXM
and CLKRM as they are reffered to in SPRUFM1 [1]) for codec clock-slave/
frame-slave mode (_CBS_CFS) which selects internally generated bit-clock and
frame-sync signals; however, it does the same thing again for codec
clock-master/frame-slave mode (_CBM_CFS) in the very next case statement which
is incorrectly selecting internally generated bit-clocks in this mode.

For codec clock-master/frame-slave mode (_CBM_CFS), clear bits ACLKXE and
ACLKRE to select externally-generated bit-clocks.

[1] http://www.ti.com/litv/pdf/sprufm1

Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca>
Reviewed-by: James Nuss <jamesnuss@nanometrics.ca>
---
 sound/soc/davinci/davinci-mcasp.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c
index 1aa24a1..8004643 100644
--- a/sound/soc/davinci/davinci-mcasp.c
+++ b/sound/soc/davinci/davinci-mcasp.c
@@ -439,10 +439,10 @@ static int davinci_mcasp_set_dai_fmt(struct snd_soc_dai *cpu_dai,
 		break;
 	case SND_SOC_DAIFMT_CBM_CFS:
 		/* codec is clock master and frame slave */
-		mcasp_set_bits(base + DAVINCI_MCASP_ACLKXCTL_REG, ACLKXE);
+		mcasp_clr_bits(base + DAVINCI_MCASP_ACLKXCTL_REG, ACLKXE);
 		mcasp_set_bits(base + DAVINCI_MCASP_TXFMCTL_REG, AFSXE);
 
-		mcasp_set_bits(base + DAVINCI_MCASP_ACLKRCTL_REG, ACLKRE);
+		mcasp_clr_bits(base + DAVINCI_MCASP_ACLKRCTL_REG, ACLKRE);
 		mcasp_set_bits(base + DAVINCI_MCASP_RXFMCTL_REG, AFSRE);
 
 		mcasp_set_bits(base + DAVINCI_MCASP_PDIR_REG,
-- 
1.7.1

[PATCH 3/4] davinci-mcasp: fix _CBM_CFS hw_params

[Ben Gardiner]

The current davinci_mcasp_set_dai_fmt() sets bits ACLKXE and ACLKRE (CLKXM
and CLKRM as they are reffered to in SPRUFM1 [1]) for codec clock-slave/
frame-slave mode (_CBS_CFS) which selects internally generated bit-clock and
frame-sync signals; however, it does the same thing again for codec
clock-master/frame-slave mode (_CBM_CFS) in the very next case statement which
is incorrectly selecting internally generated bit-clocks in this mode.

For codec clock-master/frame-slave mode (_CBM_CFS), clear bits ACLKXE and
ACLKRE to select externally-generated bit-clocks.

[1] http://www.ti.com/litv/pdf/sprufm1

Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca>
Reviewed-by: James Nuss <jamesnuss@nanometrics.ca>
---
 sound/soc/davinci/davinci-mcasp.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c
index 1aa24a1..8004643 100644
--- a/sound/soc/davinci/davinci-mcasp.c
+++ b/sound/soc/davinci/davinci-mcasp.c
@@ -439,10 +439,10 @@ static int davinci_mcasp_set_dai_fmt(struct snd_soc_dai *cpu_dai,
 		break;
 	case SND_SOC_DAIFMT_CBM_CFS:
 		/* codec is clock master and frame slave */
-		mcasp_set_bits(base + DAVINCI_MCASP_ACLKXCTL_REG, ACLKXE);
+		mcasp_clr_bits(base + DAVINCI_MCASP_ACLKXCTL_REG, ACLKXE);
 		mcasp_set_bits(base + DAVINCI_MCASP_TXFMCTL_REG, AFSXE);
 
-		mcasp_set_bits(base + DAVINCI_MCASP_ACLKRCTL_REG, ACLKRE);
+		mcasp_clr_bits(base + DAVINCI_MCASP_ACLKRCTL_REG, ACLKRE);
 		mcasp_set_bits(base + DAVINCI_MCASP_RXFMCTL_REG, AFSRE);
 
 		mcasp_set_bits(base + DAVINCI_MCASP_PDIR_REG,
-- 
1.7.1

[PATCH 4/4] davinci-mcasp: fix _CBM_CFS pin directions

[Ben Gardiner]

The current davinci_mcasp_set_dai_fmt() sets bits ACLKX and ACLKR in the PDIR
register for the codec clock-master/frame-slave mode; however, this results in
the ACLKX and ACLKR pins being outputs according to SPRUFM1 [1]  which
conflicts with "codec is clock master."

Similarly to the previous patch in this series, "fix _CBM_CFS hw_params" --
For codec clock-master/frame-slave mode (_CMB_CFS), clear bits ACLKX and ACLKR
in the PDIR register to set the pins as inputs and hence allow externally
sourced bit-clocks.

[1] http://www.ti.com/litv/pdf/sprufm1

Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca>
Reviewed-by: James Nuss <jamesnuss@nanometrics.ca>
---
 sound/soc/davinci/davinci-mcasp.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c
index 8004643..0a3d891 100644
--- a/sound/soc/davinci/davinci-mcasp.c
+++ b/sound/soc/davinci/davinci-mcasp.c
@@ -445,8 +445,10 @@ static int davinci_mcasp_set_dai_fmt(struct snd_soc_dai *cpu_dai,
 		mcasp_clr_bits(base + DAVINCI_MCASP_ACLKRCTL_REG, ACLKRE);
 		mcasp_set_bits(base + DAVINCI_MCASP_RXFMCTL_REG, AFSRE);
 
+		mcasp_clr_bits(base + DAVINCI_MCASP_PDIR_REG,
+				ACLKX | ACLKR);
 		mcasp_set_bits(base + DAVINCI_MCASP_PDIR_REG,
-				ACLKX | AFSX | ACLKR | AFSR);
+				AFSX | AFSR);
 		break;
 	case SND_SOC_DAIFMT_CBM_CFM:
 		/* codec is clock and frame master */
-- 
1.7.1

[PATCH 4/4] davinci-mcasp: fix _CBM_CFS pin directions

[Ben Gardiner]

The current davinci_mcasp_set_dai_fmt() sets bits ACLKX and ACLKR in the PDIR
register for the codec clock-master/frame-slave mode; however, this results in
the ACLKX and ACLKR pins being outputs according to SPRUFM1 [1]  which
conflicts with "codec is clock master."

Similarly to the previous patch in this series, "fix _CBM_CFS hw_params" --
For codec clock-master/frame-slave mode (_CMB_CFS), clear bits ACLKX and ACLKR
in the PDIR register to set the pins as inputs and hence allow externally
sourced bit-clocks.

[1] http://www.ti.com/litv/pdf/sprufm1

Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca>
Reviewed-by: James Nuss <jamesnuss@nanometrics.ca>
---
 sound/soc/davinci/davinci-mcasp.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c
index 8004643..0a3d891 100644
--- a/sound/soc/davinci/davinci-mcasp.c
+++ b/sound/soc/davinci/davinci-mcasp.c
@@ -445,8 +445,10 @@ static int davinci_mcasp_set_dai_fmt(struct snd_soc_dai *cpu_dai,
 		mcasp_clr_bits(base + DAVINCI_MCASP_ACLKRCTL_REG, ACLKRE);
 		mcasp_set_bits(base + DAVINCI_MCASP_RXFMCTL_REG, AFSRE);
 
+		mcasp_clr_bits(base + DAVINCI_MCASP_PDIR_REG,
+				ACLKX | ACLKR);
 		mcasp_set_bits(base + DAVINCI_MCASP_PDIR_REG,
-				ACLKX | AFSX | ACLKR | AFSR);
+				AFSX | AFSR);
 		break;
 	case SND_SOC_DAIFMT_CBM_CFM:
 		/* codec is clock and frame master */
-- 
1.7.1

Re: [PATCH 0/4] davinci-mcasp: fix tdm_slots and CBM/CFS

[Liam Girdwood]

On Thu, 2011-04-21 at 14:19 -0400, Ben Gardiner wrote:
> This patch series is comprised of three bugfixes and one cleanup that 
> were performed during prototyping of McASP operation in codec clock-
> master frame-slave mode.
> 
> First we noticed that the check of the number of tdm slots requested
> by platform data was always returning true -- unrelated to CMB/CFS
> mode.
> 
> Then a cleanup: the PDIR values set are currently based on magic 
> numbers and there are  available bitfield definitions. Not strictly
> needed but it makes the changes introduced in the last patch simpler
> to read.
> 
> It was found that the hardware parameters assigned when 
> codec clock-master frame-slave is requested were incorrect. This
> change is required for correct CBM/CFS operation.
> 
> Finally, the direction of the pins is corrected to reflect the
> implications of codec clock-master frame-slave -- i.e. mcasp clock-
> input frame-output. This change is also required for correct operation.
> 
> The combination was tested with a logic analyzer and the hrtimer pwm
> device from Bill Gatliff's PWM framework [1] on a da850evm with
> hardware modifications to access the McASP lines.
> 
> [1] http://article.gmane.org/gmane.linux.kernel.embedded/3486/
> 
> Ben Gardiner (4):
>   davinci-mcasp: correct tdm_slots limit
>   davinci-mcasp: use bitfield definitions for PDIR
>   davinci-mcasp: fix _CBM_CFS hw_params
>   davinci-mcasp: fix _CBM_CFS pin directions
> 
>  sound/soc/davinci/davinci-mcasp.c |   19 ++++++++++++-------
>  1 files changed, 12 insertions(+), 7 deletions(-)
> 

All

Acked-by: Liam Girdwood <lrg@ti.com>

Re: [PATCH 0/4] davinci-mcasp: fix tdm_slots and CBM/CFS

[Liam Girdwood]

On Thu, 2011-04-21 at 14:19 -0400, Ben Gardiner wrote:
> This patch series is comprised of three bugfixes and one cleanup that 
> were performed during prototyping of McASP operation in codec clock-
> master frame-slave mode.
> 
> First we noticed that the check of the number of tdm slots requested
> by platform data was always returning true -- unrelated to CMB/CFS
> mode.
> 
> Then a cleanup: the PDIR values set are currently based on magic 
> numbers and there are  available bitfield definitions. Not strictly
> needed but it makes the changes introduced in the last patch simpler
> to read.
> 
> It was found that the hardware parameters assigned when 
> codec clock-master frame-slave is requested were incorrect. This
> change is required for correct CBM/CFS operation.
> 
> Finally, the direction of the pins is corrected to reflect the
> implications of codec clock-master frame-slave -- i.e. mcasp clock-
> input frame-output. This change is also required for correct operation.
> 
> The combination was tested with a logic analyzer and the hrtimer pwm
> device from Bill Gatliff's PWM framework [1] on a da850evm with
> hardware modifications to access the McASP lines.
> 
> [1] http://article.gmane.org/gmane.linux.kernel.embedded/3486/
> 
> Ben Gardiner (4):
>   davinci-mcasp: correct tdm_slots limit
>   davinci-mcasp: use bitfield definitions for PDIR
>   davinci-mcasp: fix _CBM_CFS hw_params
>   davinci-mcasp: fix _CBM_CFS pin directions
> 
>  sound/soc/davinci/davinci-mcasp.c |   19 ++++++++++++-------
>  1 files changed, 12 insertions(+), 7 deletions(-)
> 

All

Acked-by: Liam Girdwood <lrg@ti.com>