Re: [PATCH 2/2] ASoC: axentia: tse850: add ASoC driver for the Axentia TSE-850

2016-11-09 Thread Peter Rosin 
On 2016-11-09 17:27, Peter Rosin wrote:
> On 2016-11-09 14:38, Mark Brown wrote:
>> On Tue, Nov 08, 2016 at 05:20:57PM +0100, Peter Rosin wrote:
>>> +   struct snd_soc_pcm_runtime *rtd = substream->private_data;
>>> +   struct device *dev = rtd->dev;
>>> +   struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
>>> +   int dir = substream->stream != SNDRV_PCM_STREAM_PLAYBACK;
>>> +   int div_id = dir ? ATMEL_SSC_RCMR_PERIOD : ATMEL_SSC_TCMR_PERIOD;
>>> +   int period = snd_soc_params_to_frame_size(params) / 2 - 1;
>>
>> Please write the logic out as normal if statements for legibility.  It's
>> a bit concerning that we even need this function, it looks like pretty
>> basic stuff that I'd expect the CPU DAI to just be doing - why can't
>> this be the default behaviour of the CPU DAI?
> 
> I don't know and obviously don't have all the relevant HW to test
> changes. Do you want me to attempt such a change anyway?
> Adding Cc: Nicolas Ferre

Something like this, perhaps?

Cheers,
Peter


diff --git a/sound/soc/atmel/atmel_ssc_dai.c b/sound/soc/atmel/atmel_ssc_dai.c
index 16e459aedffe..059b0b63bd51 100644
--- a/sound/soc/atmel/atmel_ssc_dai.c
+++ b/sound/soc/atmel/atmel_ssc_dai.c
@@ -380,6 +380,7 @@ static void atmel_ssc_shutdown(struct snd_pcm_substream 
*substream,
ssc_writel(ssc_p->ssc->regs, CR, SSC_BIT(CR_SWRST));
/* Clear the SSC dividers */
ssc_p->cmr_div = ssc_p->tcmr_period = ssc_p->rcmr_period = 0;
+   ssc_p->forced_divider = 0;
}
spin_unlock_irq(_p->lock);
 
@@ -429,10 +430,12 @@ static int atmel_ssc_set_dai_clkdiv(struct snd_soc_dai 
*cpu_dai,
break;
 
case ATMEL_SSC_TCMR_PERIOD:
+   ssc_p->forced_divider |= BIT(ATMEL_SSC_TCMR_PERIOD);
ssc_p->tcmr_period = div;
break;
 
case ATMEL_SSC_RCMR_PERIOD:
+   ssc_p->forced_divider |= BIT(ATMEL_SSC_RCMR_PERIOD);
ssc_p->rcmr_period = div;
break;
 
@@ -459,6 +462,8 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream 
*substream,
u32 tfmr, rfmr, tcmr, rcmr;
int ret;
int fslen, fslen_ext;
+   u32 tcmr_period;
+   u32 rcmr_period;
 
/*
 * Currently, there is only one set of dma params for
@@ -470,6 +475,13 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream 
*substream,
else
dir = 1;
 
+   tcmr_period = ssc_p->tcmr_period;
+   if (!(ssc_p->forced_divider & BIT(ATMEL_SSC_TCMR_PERIOD)))
+   tcmr_period = snd_soc_params_to_frame_size(params) / 2 - 1;
+   rcmr_period = ssc_p->rcmr_period;
+   if (!(ssc_p->forced_divider & BIT(ATMEL_SSC_RCMR_PERIOD)))
+   rcmr_period = snd_soc_params_to_frame_size(params) / 2 - 1;
+
dma_params = ssc_p->dma_params[dir];
 
channels = params_channels(params);
@@ -524,7 +536,7 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream 
*substream,
fslen_ext = (bits - 1) / 16;
fslen = (bits - 1) % 16;
 
-   rcmr =SSC_BF(RCMR_PERIOD, ssc_p->rcmr_period)
+   rcmr =SSC_BF(RCMR_PERIOD, rcmr_period)
| SSC_BF(RCMR_STTDLY, START_DELAY)
| SSC_BF(RCMR_START, SSC_START_FALLING_RF)
| SSC_BF(RCMR_CKI, SSC_CKI_RISING)
@@ -540,7 +552,7 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream 
*substream,
| SSC_BF(RFMR_LOOP, 0)
| SSC_BF(RFMR_DATLEN, (bits - 1));
 
-   tcmr =SSC_BF(TCMR_PERIOD, ssc_p->tcmr_period)
+   tcmr =SSC_BF(TCMR_PERIOD, tcmr_period)
| SSC_BF(TCMR_STTDLY, START_DELAY)
| SSC_BF(TCMR_START, SSC_START_FALLING_RF)
| SSC_BF(TCMR_CKI, SSC_CKI_FALLING)
@@ -606,7 +618,7 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream 
*substream,
fslen_ext = (bits - 1) / 16;
fslen = (bits - 1) % 16;
 
-   rcmr =SSC_BF(RCMR_PERIOD, ssc_p->rcmr_period)
+   rcmr =SSC_BF(RCMR_PERIOD, rcmr_period)
| SSC_BF(RCMR_STTDLY, START_DELAY)
| SSC_BF(RCMR_START, SSC_START_FALLING_RF)
| SSC_BF(RCMR_CKI, SSC_CKI_RISING)
@@ -623,7 +635,7 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream 
*substream,
| SSC_BF(RFMR_LOOP, 0)
| SSC_BF(RFMR_DATLEN, (bits - 1));
 
-   tcmr =SSC_BF(TCMR_PERIOD, ssc_p->tcmr_period)
+   tcmr =SSC_BF(TCMR_PERIOD, tcmr_period)
| SSC_BF(TCMR_STTDLY, START_DELAY)
| SSC_BF(TCMR_START, SSC_START_FALLING_RF)
| SSC_BF(TCMR_CKI, SSC_CKI_FALLING)
@@ -650,7 +662,7 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream 
*substream,

Re: [PATCH 2/2] ASoC: axentia: tse850: add ASoC driver for the Axentia TSE-850

2016-11-09 Thread Peter Rosin 
On 2016-11-09 17:27, Peter Rosin wrote:
> On 2016-11-09 14:38, Mark Brown wrote:
>> On Tue, Nov 08, 2016 at 05:20:57PM +0100, Peter Rosin wrote:
>>> +   struct snd_soc_pcm_runtime *rtd = substream->private_data;
>>> +   struct device *dev = rtd->dev;
>>> +   struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
>>> +   int dir = substream->stream != SNDRV_PCM_STREAM_PLAYBACK;
>>> +   int div_id = dir ? ATMEL_SSC_RCMR_PERIOD : ATMEL_SSC_TCMR_PERIOD;
>>> +   int period = snd_soc_params_to_frame_size(params) / 2 - 1;
>>
>> Please write the logic out as normal if statements for legibility.  It's
>> a bit concerning that we even need this function, it looks like pretty
>> basic stuff that I'd expect the CPU DAI to just be doing - why can't
>> this be the default behaviour of the CPU DAI?
> 
> I don't know and obviously don't have all the relevant HW to test
> changes. Do you want me to attempt such a change anyway?
> Adding Cc: Nicolas Ferre

Something like this, perhaps?

Cheers,
Peter


diff --git a/sound/soc/atmel/atmel_ssc_dai.c b/sound/soc/atmel/atmel_ssc_dai.c
index 16e459aedffe..059b0b63bd51 100644
--- a/sound/soc/atmel/atmel_ssc_dai.c
+++ b/sound/soc/atmel/atmel_ssc_dai.c
@@ -380,6 +380,7 @@ static void atmel_ssc_shutdown(struct snd_pcm_substream 
*substream,
ssc_writel(ssc_p->ssc->regs, CR, SSC_BIT(CR_SWRST));
/* Clear the SSC dividers */
ssc_p->cmr_div = ssc_p->tcmr_period = ssc_p->rcmr_period = 0;
+   ssc_p->forced_divider = 0;
}
spin_unlock_irq(_p->lock);
 
@@ -429,10 +430,12 @@ static int atmel_ssc_set_dai_clkdiv(struct snd_soc_dai 
*cpu_dai,
break;
 
case ATMEL_SSC_TCMR_PERIOD:
+   ssc_p->forced_divider |= BIT(ATMEL_SSC_TCMR_PERIOD);
ssc_p->tcmr_period = div;
break;
 
case ATMEL_SSC_RCMR_PERIOD:
+   ssc_p->forced_divider |= BIT(ATMEL_SSC_RCMR_PERIOD);
ssc_p->rcmr_period = div;
break;
 
@@ -459,6 +462,8 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream 
*substream,
u32 tfmr, rfmr, tcmr, rcmr;
int ret;
int fslen, fslen_ext;
+   u32 tcmr_period;
+   u32 rcmr_period;
 
/*
 * Currently, there is only one set of dma params for
@@ -470,6 +475,13 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream 
*substream,
else
dir = 1;
 
+   tcmr_period = ssc_p->tcmr_period;
+   if (!(ssc_p->forced_divider & BIT(ATMEL_SSC_TCMR_PERIOD)))
+   tcmr_period = snd_soc_params_to_frame_size(params) / 2 - 1;
+   rcmr_period = ssc_p->rcmr_period;
+   if (!(ssc_p->forced_divider & BIT(ATMEL_SSC_RCMR_PERIOD)))
+   rcmr_period = snd_soc_params_to_frame_size(params) / 2 - 1;
+
dma_params = ssc_p->dma_params[dir];
 
channels = params_channels(params);
@@ -524,7 +536,7 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream 
*substream,
fslen_ext = (bits - 1) / 16;
fslen = (bits - 1) % 16;
 
-   rcmr =SSC_BF(RCMR_PERIOD, ssc_p->rcmr_period)
+   rcmr =SSC_BF(RCMR_PERIOD, rcmr_period)
| SSC_BF(RCMR_STTDLY, START_DELAY)
| SSC_BF(RCMR_START, SSC_START_FALLING_RF)
| SSC_BF(RCMR_CKI, SSC_CKI_RISING)
@@ -540,7 +552,7 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream 
*substream,
| SSC_BF(RFMR_LOOP, 0)
| SSC_BF(RFMR_DATLEN, (bits - 1));
 
-   tcmr =SSC_BF(TCMR_PERIOD, ssc_p->tcmr_period)
+   tcmr =SSC_BF(TCMR_PERIOD, tcmr_period)
| SSC_BF(TCMR_STTDLY, START_DELAY)
| SSC_BF(TCMR_START, SSC_START_FALLING_RF)
| SSC_BF(TCMR_CKI, SSC_CKI_FALLING)
@@ -606,7 +618,7 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream 
*substream,
fslen_ext = (bits - 1) / 16;
fslen = (bits - 1) % 16;
 
-   rcmr =SSC_BF(RCMR_PERIOD, ssc_p->rcmr_period)
+   rcmr =SSC_BF(RCMR_PERIOD, rcmr_period)
| SSC_BF(RCMR_STTDLY, START_DELAY)
| SSC_BF(RCMR_START, SSC_START_FALLING_RF)
| SSC_BF(RCMR_CKI, SSC_CKI_RISING)
@@ -623,7 +635,7 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream 
*substream,
| SSC_BF(RFMR_LOOP, 0)
| SSC_BF(RFMR_DATLEN, (bits - 1));
 
-   tcmr =SSC_BF(TCMR_PERIOD, ssc_p->tcmr_period)
+   tcmr =SSC_BF(TCMR_PERIOD, tcmr_period)
| SSC_BF(TCMR_STTDLY, START_DELAY)
| SSC_BF(TCMR_START, SSC_START_FALLING_RF)
| SSC_BF(TCMR_CKI, SSC_CKI_FALLING)
@@ -650,7 +662,7 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream 
*substream,

Re: [PATCH 2/2] ASoC: axentia: tse850: add ASoC driver for the Axentia TSE-850

2016-11-09 Thread Peter Rosin 
On 2016-11-09 14:38, Mark Brown wrote:
> On Tue, Nov 08, 2016 at 05:20:57PM +0100, Peter Rosin wrote:
> 
>> +++ b/sound/soc/axentia/Kconfig
>> @@ -0,0 +1,10 @@
>> +config SND_SOC_AXENTIA_TSE850_PCM5142
>> +tristate "ASoC driver for the Axentia TSE-850"
>> +depends on ARCH_AT91 && OF
>> +select ATMEL_SSC
>> +select SND_ATMEL_SOC
>> +select SND_ATMEL_SOC_SSC_DMA
>> +select SND_SOC_PCM512x_I2C
>> +help
>> +  Say Y if you want to add support for the ASoC driver for the
>> +  Axentia TSE-850 with a PCM5142 codec.
> 
> This just looks like a normal machine driver for an Atmel system which
> would usually go in the atemel directory - why is a new directory being
> created?

I thought atmel in this context meant that Atmel made the board, not that
the board was based on an Atmel cpu.

I'll move it for v2.

>> +static int tse850_get_mux2(struct snd_kcontrol *kctrl,
>> +   struct snd_ctl_elem_value *ucontrol)
>> +{
>> +struct snd_soc_dapm_context *dapm = snd_soc_dapm_kcontrol_dapm(kctrl);
>> +struct snd_soc_card *card = dapm->card;
>> +struct tse850_priv *tse850 = snd_soc_card_get_drvdata(card);
>> +int ret;
>> +
>> +ret = gpiod_get_value(tse850->loop2);
>> +if (ret < 0)
>> +return ret;
> 
> We can't reliably read the value of output GPIOs (though in practice the
> majority do support it) so it'd be better practice to use a state
> variable to remember what we set.  I'd also expect this to use the
> _cansleep() GPIO calls as it's not in a context where sleeping would be
> a problem.

Ok, I'll add _cansleep and cached values for v2.

>> +int tse850_get_ana(struct snd_kcontrol *kctrl,
>> +   struct snd_ctl_elem_value *ucontrol)
>> +{
>> +struct snd_soc_dapm_context *dapm = snd_soc_dapm_kcontrol_dapm(kctrl);
>> +struct snd_soc_card *card = dapm->card;
>> +struct tse850_priv *tse850 = snd_soc_card_get_drvdata(card);
>> +int ret;
>> +
>> +ret = regulator_get_voltage(tse850->ana);
>> +if (ret < 0)
>> +return ret;
>> +
>> +if (ret < 1100)
>> +ret = 1100;
>> +else if (ret > 2000)
>> +ret = 2000;
> 
> This needs some comments...

Ok, I'll add some words...

>> +struct snd_soc_pcm_runtime *rtd = substream->private_data;
>> +struct device *dev = rtd->dev;
>> +struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
>> +int dir = substream->stream != SNDRV_PCM_STREAM_PLAYBACK;
>> +int div_id = dir ? ATMEL_SSC_RCMR_PERIOD : ATMEL_SSC_TCMR_PERIOD;
>> +int period = snd_soc_params_to_frame_size(params) / 2 - 1;
> 
> Please write the logic out as normal if statements for legibility.  It's
> a bit concerning that we even need this function, it looks like pretty
> basic stuff that I'd expect the CPU DAI to just be doing - why can't
> this be the default behaviour of the CPU DAI?

I don't know and obviously don't have all the relevant HW to test
changes. Do you want me to attempt such a change anyway?
Adding Cc: Nicolas Ferre

>> +static int tse850_init(struct snd_soc_pcm_runtime *rtd)
>> +{
>> +struct snd_soc_dapm_context *dapm = >card->dapm;
>> +
>> +return snd_soc_dapm_add_routes(dapm, tse850_intercon,
>> +   ARRAY_SIZE(tse850_intercon));
> 
> Set this up in the card data structure rather than open coding the call,
> you can register DAPM routes there too.

Right.

Thanks for looking!

Cheers,
Peter

Re: [PATCH 2/2] ASoC: axentia: tse850: add ASoC driver for the Axentia TSE-850

2016-11-09 Thread Peter Rosin 
On 2016-11-09 14:38, Mark Brown wrote:
> On Tue, Nov 08, 2016 at 05:20:57PM +0100, Peter Rosin wrote:
> 
>> +++ b/sound/soc/axentia/Kconfig
>> @@ -0,0 +1,10 @@
>> +config SND_SOC_AXENTIA_TSE850_PCM5142
>> +tristate "ASoC driver for the Axentia TSE-850"
>> +depends on ARCH_AT91 && OF
>> +select ATMEL_SSC
>> +select SND_ATMEL_SOC
>> +select SND_ATMEL_SOC_SSC_DMA
>> +select SND_SOC_PCM512x_I2C
>> +help
>> +  Say Y if you want to add support for the ASoC driver for the
>> +  Axentia TSE-850 with a PCM5142 codec.
> 
> This just looks like a normal machine driver for an Atmel system which
> would usually go in the atemel directory - why is a new directory being
> created?

I thought atmel in this context meant that Atmel made the board, not that
the board was based on an Atmel cpu.

I'll move it for v2.

>> +static int tse850_get_mux2(struct snd_kcontrol *kctrl,
>> +   struct snd_ctl_elem_value *ucontrol)
>> +{
>> +struct snd_soc_dapm_context *dapm = snd_soc_dapm_kcontrol_dapm(kctrl);
>> +struct snd_soc_card *card = dapm->card;
>> +struct tse850_priv *tse850 = snd_soc_card_get_drvdata(card);
>> +int ret;
>> +
>> +ret = gpiod_get_value(tse850->loop2);
>> +if (ret < 0)
>> +return ret;
> 
> We can't reliably read the value of output GPIOs (though in practice the
> majority do support it) so it'd be better practice to use a state
> variable to remember what we set.  I'd also expect this to use the
> _cansleep() GPIO calls as it's not in a context where sleeping would be
> a problem.

Ok, I'll add _cansleep and cached values for v2.

>> +int tse850_get_ana(struct snd_kcontrol *kctrl,
>> +   struct snd_ctl_elem_value *ucontrol)
>> +{
>> +struct snd_soc_dapm_context *dapm = snd_soc_dapm_kcontrol_dapm(kctrl);
>> +struct snd_soc_card *card = dapm->card;
>> +struct tse850_priv *tse850 = snd_soc_card_get_drvdata(card);
>> +int ret;
>> +
>> +ret = regulator_get_voltage(tse850->ana);
>> +if (ret < 0)
>> +return ret;
>> +
>> +if (ret < 1100)
>> +ret = 1100;
>> +else if (ret > 2000)
>> +ret = 2000;
> 
> This needs some comments...

Ok, I'll add some words...

>> +struct snd_soc_pcm_runtime *rtd = substream->private_data;
>> +struct device *dev = rtd->dev;
>> +struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
>> +int dir = substream->stream != SNDRV_PCM_STREAM_PLAYBACK;
>> +int div_id = dir ? ATMEL_SSC_RCMR_PERIOD : ATMEL_SSC_TCMR_PERIOD;
>> +int period = snd_soc_params_to_frame_size(params) / 2 - 1;
> 
> Please write the logic out as normal if statements for legibility.  It's
> a bit concerning that we even need this function, it looks like pretty
> basic stuff that I'd expect the CPU DAI to just be doing - why can't
> this be the default behaviour of the CPU DAI?

I don't know and obviously don't have all the relevant HW to test
changes. Do you want me to attempt such a change anyway?
Adding Cc: Nicolas Ferre

>> +static int tse850_init(struct snd_soc_pcm_runtime *rtd)
>> +{
>> +struct snd_soc_dapm_context *dapm = >card->dapm;
>> +
>> +return snd_soc_dapm_add_routes(dapm, tse850_intercon,
>> +   ARRAY_SIZE(tse850_intercon));
> 
> Set this up in the card data structure rather than open coding the call,
> you can register DAPM routes there too.

Right.

Thanks for looking!

Cheers,
Peter

Re: [PATCH 2/2] ASoC: axentia: tse850: add ASoC driver for the Axentia TSE-850

2016-11-09 Thread Mark Brown 
On Tue, Nov 08, 2016 at 05:20:57PM +0100, Peter Rosin wrote:

> +++ b/sound/soc/axentia/Kconfig
> @@ -0,0 +1,10 @@
> +config SND_SOC_AXENTIA_TSE850_PCM5142
> + tristate "ASoC driver for the Axentia TSE-850"
> + depends on ARCH_AT91 && OF
> + select ATMEL_SSC
> + select SND_ATMEL_SOC
> + select SND_ATMEL_SOC_SSC_DMA
> + select SND_SOC_PCM512x_I2C
> + help
> +   Say Y if you want to add support for the ASoC driver for the
> +   Axentia TSE-850 with a PCM5142 codec.

This just looks like a normal machine driver for an Atmel system which
would usually go in the atemel directory - why is a new directory being
created?

> +static int tse850_get_mux2(struct snd_kcontrol *kctrl,
> +struct snd_ctl_elem_value *ucontrol)
> +{
> + struct snd_soc_dapm_context *dapm = snd_soc_dapm_kcontrol_dapm(kctrl);
> + struct snd_soc_card *card = dapm->card;
> + struct tse850_priv *tse850 = snd_soc_card_get_drvdata(card);
> + int ret;
> +
> + ret = gpiod_get_value(tse850->loop2);
> + if (ret < 0)
> + return ret;

We can't reliably read the value of output GPIOs (though in practice the
majority do support it) so it'd be better practice to use a state
variable to remember what we set.  I'd also expect this to use the
_cansleep() GPIO calls as it's not in a context where sleeping would be
a problem.

> +int tse850_get_ana(struct snd_kcontrol *kctrl,
> +struct snd_ctl_elem_value *ucontrol)
> +{
> + struct snd_soc_dapm_context *dapm = snd_soc_dapm_kcontrol_dapm(kctrl);
> + struct snd_soc_card *card = dapm->card;
> + struct tse850_priv *tse850 = snd_soc_card_get_drvdata(card);
> + int ret;
> +
> + ret = regulator_get_voltage(tse850->ana);
> + if (ret < 0)
> + return ret;
> +
> + if (ret < 1100)
> + ret = 1100;
> + else if (ret > 2000)
> + ret = 2000;

This needs some comments...

> + struct snd_soc_pcm_runtime *rtd = substream->private_data;
> + struct device *dev = rtd->dev;
> + struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> + int dir = substream->stream != SNDRV_PCM_STREAM_PLAYBACK;
> + int div_id = dir ? ATMEL_SSC_RCMR_PERIOD : ATMEL_SSC_TCMR_PERIOD;
> + int period = snd_soc_params_to_frame_size(params) / 2 - 1;

Please write the logic out as normal if statements for legibility.  It's
a bit concerning that we even need this function, it looks like pretty
basic stuff that I'd expect the CPU DAI to just be doing - why can't
this be the default behaviour of the CPU DAI?

> +static int tse850_init(struct snd_soc_pcm_runtime *rtd)
> +{
> + struct snd_soc_dapm_context *dapm = >card->dapm;
> +
> + return snd_soc_dapm_add_routes(dapm, tse850_intercon,
> +ARRAY_SIZE(tse850_intercon));

Set this up in the card data structure rather than open coding the call,
you can register DAPM routes there too.


signature.asc
Description: PGP signature

Re: [PATCH 2/2] ASoC: axentia: tse850: add ASoC driver for the Axentia TSE-850

2016-11-09 Thread Mark Brown 
On Tue, Nov 08, 2016 at 05:20:57PM +0100, Peter Rosin wrote:

> +++ b/sound/soc/axentia/Kconfig
> @@ -0,0 +1,10 @@
> +config SND_SOC_AXENTIA_TSE850_PCM5142
> + tristate "ASoC driver for the Axentia TSE-850"
> + depends on ARCH_AT91 && OF
> + select ATMEL_SSC
> + select SND_ATMEL_SOC
> + select SND_ATMEL_SOC_SSC_DMA
> + select SND_SOC_PCM512x_I2C
> + help
> +   Say Y if you want to add support for the ASoC driver for the
> +   Axentia TSE-850 with a PCM5142 codec.

This just looks like a normal machine driver for an Atmel system which
would usually go in the atemel directory - why is a new directory being
created?

> +static int tse850_get_mux2(struct snd_kcontrol *kctrl,
> +struct snd_ctl_elem_value *ucontrol)
> +{
> + struct snd_soc_dapm_context *dapm = snd_soc_dapm_kcontrol_dapm(kctrl);
> + struct snd_soc_card *card = dapm->card;
> + struct tse850_priv *tse850 = snd_soc_card_get_drvdata(card);
> + int ret;
> +
> + ret = gpiod_get_value(tse850->loop2);
> + if (ret < 0)
> + return ret;

We can't reliably read the value of output GPIOs (though in practice the
majority do support it) so it'd be better practice to use a state
variable to remember what we set.  I'd also expect this to use the
_cansleep() GPIO calls as it's not in a context where sleeping would be
a problem.

> +int tse850_get_ana(struct snd_kcontrol *kctrl,
> +struct snd_ctl_elem_value *ucontrol)
> +{
> + struct snd_soc_dapm_context *dapm = snd_soc_dapm_kcontrol_dapm(kctrl);
> + struct snd_soc_card *card = dapm->card;
> + struct tse850_priv *tse850 = snd_soc_card_get_drvdata(card);
> + int ret;
> +
> + ret = regulator_get_voltage(tse850->ana);
> + if (ret < 0)
> + return ret;
> +
> + if (ret < 1100)
> + ret = 1100;
> + else if (ret > 2000)
> + ret = 2000;

This needs some comments...

> + struct snd_soc_pcm_runtime *rtd = substream->private_data;
> + struct device *dev = rtd->dev;
> + struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> + int dir = substream->stream != SNDRV_PCM_STREAM_PLAYBACK;
> + int div_id = dir ? ATMEL_SSC_RCMR_PERIOD : ATMEL_SSC_TCMR_PERIOD;
> + int period = snd_soc_params_to_frame_size(params) / 2 - 1;

Please write the logic out as normal if statements for legibility.  It's
a bit concerning that we even need this function, it looks like pretty
basic stuff that I'd expect the CPU DAI to just be doing - why can't
this be the default behaviour of the CPU DAI?

> +static int tse850_init(struct snd_soc_pcm_runtime *rtd)
> +{
> + struct snd_soc_dapm_context *dapm = >card->dapm;
> +
> + return snd_soc_dapm_add_routes(dapm, tse850_intercon,
> +ARRAY_SIZE(tse850_intercon));

Set this up in the card data structure rather than open coding the call,
you can register DAPM routes there too.


signature.asc
Description: PGP signature

[PATCH 2/2] ASoC: axentia: tse850: add ASoC driver for the Axentia TSE-850

2016-11-08 Thread Peter Rosin 
The TSE-850 is an FM Transmitter Station Equipment, designed to generate
baseband signals for FM, mainly the DARC subcarrier, but other signals
are also possible.

Signed-off-by: Peter Rosin 
---
 MAINTAINERS|   1 +
 sound/soc/Kconfig  |   1 +
 sound/soc/Makefile |   1 +
 sound/soc/axentia/Kconfig  |  10 +
 sound/soc/axentia/Makefile |   3 +
 sound/soc/axentia/tse850-pcm5142.c | 504 +
 6 files changed, 520 insertions(+)
 create mode 100644 sound/soc/axentia/Kconfig
 create mode 100644 sound/soc/axentia/Makefile
 create mode 100644 sound/soc/axentia/tse850-pcm5142.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 4f2ebf3ab51a..e966dfa79680 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2323,6 +2323,7 @@ M:Peter Rosin 
 L: alsa-de...@alsa-project.org (moderated for non-subscribers)
 S: Maintained
 F: Documentation/devicetree/bindings/sound/axentia,*
+F: sound/soc/axentia/
 
 AZ6007 DVB DRIVER
 M: Mauro Carvalho Chehab 
diff --git a/sound/soc/Kconfig b/sound/soc/Kconfig
index 182d92efc7c8..5e2f9e58710b 100644
--- a/sound/soc/Kconfig
+++ b/sound/soc/Kconfig
@@ -41,6 +41,7 @@ source "sound/soc/adi/Kconfig"
 source "sound/soc/amd/Kconfig"
 source "sound/soc/atmel/Kconfig"
 source "sound/soc/au1x/Kconfig"
+source "sound/soc/axentia/Kconfig"
 source "sound/soc/bcm/Kconfig"
 source "sound/soc/blackfin/Kconfig"
 source "sound/soc/cirrus/Kconfig"
diff --git a/sound/soc/Makefile b/sound/soc/Makefile
index 9a30f21d16ee..dcdd41358e2a 100644
--- a/sound/soc/Makefile
+++ b/sound/soc/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_SND_SOC) += adi/
 obj-$(CONFIG_SND_SOC)  += amd/
 obj-$(CONFIG_SND_SOC)  += atmel/
 obj-$(CONFIG_SND_SOC)  += au1x/
+obj-$(CONFIG_SND_SOC)  += axentia/
 obj-$(CONFIG_SND_SOC)  += bcm/
 obj-$(CONFIG_SND_SOC)  += blackfin/
 obj-$(CONFIG_SND_SOC)  += cirrus/
diff --git a/sound/soc/axentia/Kconfig b/sound/soc/axentia/Kconfig
new file mode 100644
index ..7c760edd4bed
--- /dev/null
+++ b/sound/soc/axentia/Kconfig
@@ -0,0 +1,10 @@
+config SND_SOC_AXENTIA_TSE850_PCM5142
+   tristate "ASoC driver for the Axentia TSE-850"
+   depends on ARCH_AT91 && OF
+   select ATMEL_SSC
+   select SND_ATMEL_SOC
+   select SND_ATMEL_SOC_SSC_DMA
+   select SND_SOC_PCM512x_I2C
+   help
+ Say Y if you want to add support for the ASoC driver for the
+ Axentia TSE-850 with a PCM5142 codec.
diff --git a/sound/soc/axentia/Makefile b/sound/soc/axentia/Makefile
new file mode 100644
index ..d4ef52915809
--- /dev/null
+++ b/sound/soc/axentia/Makefile
@@ -0,0 +1,3 @@
+snd-soc-tse850-pcm5142-objs := tse850-pcm5142.o
+
+obj-$(CONFIG_SND_SOC_AXENTIA_TSE850_PCM5142) += snd-soc-tse850-pcm5142.o
diff --git a/sound/soc/axentia/tse850-pcm5142.c 
b/sound/soc/axentia/tse850-pcm5142.c
new file mode 100644
index ..1865f1eea554
--- /dev/null
+++ b/sound/soc/axentia/tse850-pcm5142.c
@@ -0,0 +1,504 @@
+/*
+ * TSE-850 audio - ASoC driver for the Axentia TSE-850 with a PCM5142 codec
+ *
+ * Copyright (C) 2016 Axentia Technologies AB
+ *
+ * Author: Peter Rosin 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+/*
+ *   loop1 relays
+ *   IN1 +---o  ++  o---+ OUT1
+ *\/
+ * +  +
+ * |   /  |
+ * +--o  +--. |
+ * |  add   | |
+ * |V |
+ * |  .---.   |
+ *   DAC +--->|Sum|---+
+ * |  '---'   |
+ * |  |
+ * +  +
+ *
+ *   IN2 +---o--++--o---+ OUT2
+ *   loop2 relays
+ *
+ * The 'loop1' gpio pin controlls two relays, which are either in loop
+ * position, meaning that input and output are directly connected, or
+ * they are in mixer position, meaning that the signal is passed through
+ * the 'Sum' mixer. Similarly for 'loop2'.
+ *
+ * In the above, the 'loop1' relays are inactive, thus feeding IN1 to the
+ * mixer (if 'add' is active) and feeding the mixer output to OUT1. The
+ * 'loop2' relays are active, short-cutting the TSE-850 from channel 2.
+ * IN1, IN2, OUT1 and OUT2 are TSE-850 connectors and DAC is the PCB name
+ * of the (filtered) output from the PCM5142 codec.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+
+#include "../atmel/atmel_ssc_dai.h"
+
+struct tse850_priv {
+   int ssc_id;
+
+   struct gpio_desc *add;
+   struct gpio_desc *loop1;
+   struct gpio_desc *loop2;
+
+   struct regulator *ana;
+};
+
+static int tse850_get_mux1(struct snd_kcontrol *kctrl,
+

[PATCH 2/2] ASoC: axentia: tse850: add ASoC driver for the Axentia TSE-850

2016-11-08 Thread Peter Rosin 
The TSE-850 is an FM Transmitter Station Equipment, designed to generate
baseband signals for FM, mainly the DARC subcarrier, but other signals
are also possible.

Signed-off-by: Peter Rosin 
---
 MAINTAINERS|   1 +
 sound/soc/Kconfig  |   1 +
 sound/soc/Makefile |   1 +
 sound/soc/axentia/Kconfig  |  10 +
 sound/soc/axentia/Makefile |   3 +
 sound/soc/axentia/tse850-pcm5142.c | 504 +
 6 files changed, 520 insertions(+)
 create mode 100644 sound/soc/axentia/Kconfig
 create mode 100644 sound/soc/axentia/Makefile
 create mode 100644 sound/soc/axentia/tse850-pcm5142.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 4f2ebf3ab51a..e966dfa79680 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2323,6 +2323,7 @@ M:Peter Rosin 
 L: alsa-de...@alsa-project.org (moderated for non-subscribers)
 S: Maintained
 F: Documentation/devicetree/bindings/sound/axentia,*
+F: sound/soc/axentia/
 
 AZ6007 DVB DRIVER
 M: Mauro Carvalho Chehab 
diff --git a/sound/soc/Kconfig b/sound/soc/Kconfig
index 182d92efc7c8..5e2f9e58710b 100644
--- a/sound/soc/Kconfig
+++ b/sound/soc/Kconfig
@@ -41,6 +41,7 @@ source "sound/soc/adi/Kconfig"
 source "sound/soc/amd/Kconfig"
 source "sound/soc/atmel/Kconfig"
 source "sound/soc/au1x/Kconfig"
+source "sound/soc/axentia/Kconfig"
 source "sound/soc/bcm/Kconfig"
 source "sound/soc/blackfin/Kconfig"
 source "sound/soc/cirrus/Kconfig"
diff --git a/sound/soc/Makefile b/sound/soc/Makefile
index 9a30f21d16ee..dcdd41358e2a 100644
--- a/sound/soc/Makefile
+++ b/sound/soc/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_SND_SOC) += adi/
 obj-$(CONFIG_SND_SOC)  += amd/
 obj-$(CONFIG_SND_SOC)  += atmel/
 obj-$(CONFIG_SND_SOC)  += au1x/
+obj-$(CONFIG_SND_SOC)  += axentia/
 obj-$(CONFIG_SND_SOC)  += bcm/
 obj-$(CONFIG_SND_SOC)  += blackfin/
 obj-$(CONFIG_SND_SOC)  += cirrus/
diff --git a/sound/soc/axentia/Kconfig b/sound/soc/axentia/Kconfig
new file mode 100644
index ..7c760edd4bed
--- /dev/null
+++ b/sound/soc/axentia/Kconfig
@@ -0,0 +1,10 @@
+config SND_SOC_AXENTIA_TSE850_PCM5142
+   tristate "ASoC driver for the Axentia TSE-850"
+   depends on ARCH_AT91 && OF
+   select ATMEL_SSC
+   select SND_ATMEL_SOC
+   select SND_ATMEL_SOC_SSC_DMA
+   select SND_SOC_PCM512x_I2C
+   help
+ Say Y if you want to add support for the ASoC driver for the
+ Axentia TSE-850 with a PCM5142 codec.
diff --git a/sound/soc/axentia/Makefile b/sound/soc/axentia/Makefile
new file mode 100644
index ..d4ef52915809
--- /dev/null
+++ b/sound/soc/axentia/Makefile
@@ -0,0 +1,3 @@
+snd-soc-tse850-pcm5142-objs := tse850-pcm5142.o
+
+obj-$(CONFIG_SND_SOC_AXENTIA_TSE850_PCM5142) += snd-soc-tse850-pcm5142.o
diff --git a/sound/soc/axentia/tse850-pcm5142.c 
b/sound/soc/axentia/tse850-pcm5142.c
new file mode 100644
index ..1865f1eea554
--- /dev/null
+++ b/sound/soc/axentia/tse850-pcm5142.c
@@ -0,0 +1,504 @@
+/*
+ * TSE-850 audio - ASoC driver for the Axentia TSE-850 with a PCM5142 codec
+ *
+ * Copyright (C) 2016 Axentia Technologies AB
+ *
+ * Author: Peter Rosin 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+/*
+ *   loop1 relays
+ *   IN1 +---o  ++  o---+ OUT1
+ *\/
+ * +  +
+ * |   /  |
+ * +--o  +--. |
+ * |  add   | |
+ * |V |
+ * |  .---.   |
+ *   DAC +--->|Sum|---+
+ * |  '---'   |
+ * |  |
+ * +  +
+ *
+ *   IN2 +---o--++--o---+ OUT2
+ *   loop2 relays
+ *
+ * The 'loop1' gpio pin controlls two relays, which are either in loop
+ * position, meaning that input and output are directly connected, or
+ * they are in mixer position, meaning that the signal is passed through
+ * the 'Sum' mixer. Similarly for 'loop2'.
+ *
+ * In the above, the 'loop1' relays are inactive, thus feeding IN1 to the
+ * mixer (if 'add' is active) and feeding the mixer output to OUT1. The
+ * 'loop2' relays are active, short-cutting the TSE-850 from channel 2.
+ * IN1, IN2, OUT1 and OUT2 are TSE-850 connectors and DAC is the PCB name
+ * of the (filtered) output from the PCM5142 codec.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+
+#include "../atmel/atmel_ssc_dai.h"
+
+struct tse850_priv {
+   int ssc_id;
+
+   struct gpio_desc *add;
+   struct gpio_desc *loop1;
+   struct gpio_desc *loop2;
+
+   struct regulator *ana;
+};
+
+static int tse850_get_mux1(struct snd_kcontrol *kctrl,
+  struct snd_ctl_elem_value *ucontrol)
+{
+   struct