[patch 0/5] kirkwood openrd client audio support - v3

[patch 0/5] kirkwood openrd client audio support - v3

[apatard at mandriva.com]

Hi,

It's a new try to get merged kirkwood openrd client support.

The changes are :
- set window configuration according to dma address
- namespaced all cs42l51 #define
- modified cs42l51_dac_names array according to what proposed Mark Brown
- use SOC_DOUBLE_TLV for mic boost

Arnaud

[patch 1/5] orion/kirkwood: add audio functions.

[apatard at mandriva.com]

An embedded and charset-unspecified text was scrubbed...
Name: kirkwood_add_audio_dev.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100525/626920ff/attachment.el>

[patch 2/5] openrd-client: initialise audio

[apatard at mandriva.com]

An embedded and charset-unspecified text was scrubbed...
Name: openrd_init_audio_dev.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100525/84a8254e/attachment.el>

[patch 3/5] cs42l51: add asoc driver

[apatard at mandriva.com]

An embedded and charset-unspecified text was scrubbed...
Name: cs42l51_support.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100525/371166ee/attachment.el>

[patch 4/5] orion/kirkwood: Add i2s support

[apatard at mandriva.com]

An embedded and charset-unspecified text was scrubbed...
Name: kirkwood_i2s.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100525/0c7dcc63/attachment.el>

[patch 5/5] kirkwood: Add audio support to openrd client platforms

[apatard at mandriva.com]

An embedded and charset-unspecified text was scrubbed...
Name: openrd_audio.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100525/d4538c73/attachment.el>

[patch 3/5] cs42l51: add asoc driver

[Mark Brown]

On Tue, May 25, 2010 at 02:22:39PM +0200, apatard at mandriva.com wrote:

> +	/* route microphone */
> +	ret = snd_soc_write(codec, CS42L51_ADC_INPUT, 0xF0);
> +	if (ret < 0)
> +		goto error_alloc;

As I said last time this really should be visible to userspace rather
than fixed function in the driver.

> +#define DAPM_EVENT_PRE_POST_PMD	(SND_SOC_DAPM_PRE_PMD | SND_SOC_DAPM_POST_PMD)

As I said last time this should be in soc-dapm.h rather than in the
driver.

> +	SND_SOC_DAPM_DAC_E("Left DAC", "Left HiFi Playback",
> +		CS42L51_POWER_CTL1, 5, 1,
> +		cs42l51_pdn_event, DAPM_EVENT_PRE_POST_PMD),
> +	SND_SOC_DAPM_DAC_E("Right DAC", "Right HiFi Playback",
> +		CS42L51_POWER_CTL1, 6, 1,
> +		cs42l51_pdn_event, DAPM_EVENT_PRE_POST_PMD),

Your event here uses snd_soc_write() so for a stereo enable/disable
you'll do extra writes.  If you changed it to use snd_soc_update_bits()
then these would be suppressed.

> +	switch (format & SND_SOC_DAIFMT_MASTER_MASK) {
> +	case SND_SOC_DAIFMT_CBM_CFM:
> +		cs42l51->func = MODE_MASTER;
> +		break;
> +	default:
> +	case SND_SOC_DAIFMT_CBS_CFS:
> +		cs42l51->func = MODE_SLAVE_AUTO;
> +		break;

Is the device really capable of automatically coping with mixed master
buses automatically, or should the default be to return an error?

> +	dev_info(&pdev->dev, "CS42L51 ALSA SoC Codec\n");

I really would prefer to remove this.

> +struct cs42l51_setup_data {
> +	int i2c_bus;
> +	unsigned short i2c_address;
> +};

This is unused and could be removed.

[patch 0/5] kirkwood openrd client audio support - v3

[Mark Brown]

On Tue, May 25, 2010 at 02:22:36PM +0200, apatard at mandriva.com wrote:

> It's a new try to get merged kirkwood openrd client support.

> The changes are :
> - set window configuration according to dma address
> - namespaced all cs42l51 #define
> - modified cs42l51_dac_names array according to what proposed Mark Brown
> - use SOC_DOUBLE_TLV for mic boost

This looks mostly OK to me - I had a few comments on the CODEC driver
but they're fairly minor.  It'd be good to get some input from the
Kirkwood guys, particularly with regard to the merge path here.

[patch 4/5] orion/kirkwood: Add i2s support

[Nicolas Pitre]

On Tue, 25 May 2010, apatard at mandriva.com wrote:

> This patch enables support for the i2s controller available on orion/kirkwood platforms
> 
> Signed-off-by: Arnaud Patard <apatard@mandriva.com>

The actual Orion processor family has no I2S/audio support.  It would be 
better if all the audio related stuff was renamed to "kirkwood" and 
"KIRKWOOD_*" instead.

> Index: sound-2.6/sound/soc/orion/Kconfig
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ sound-2.6/sound/soc/orion/Kconfig	2010-05-25 10:31:24.601080937 +0200
> @@ -0,0 +1,11 @@
> +config SND_ORION_SOC
> +	tristate "SoC Audio for the Marvell Orion/Kirkwood chip"
> +	depends on ARCH_KIRKWOOD || ARCH_ARCH_ORION5X

ARCH_ARCH_ORION5X should probably be ARCH_ORION5X.  But again, this is 
wrong to add ARCH_ORION5X anyway.


Nicolas

[patch 0/5] kirkwood openrd client audio support - v3

[Nicolas Pitre]

On Wed, 26 May 2010, Mark Brown wrote:

> On Tue, May 25, 2010 at 02:22:36PM +0200, apatard at mandriva.com wrote:
> 
> > It's a new try to get merged kirkwood openrd client support.
> 
> > The changes are :
> > - set window configuration according to dma address
> > - namespaced all cs42l51 #define
> > - modified cs42l51_dac_names array according to what proposed Mark Brown
> > - use SOC_DOUBLE_TLV for mic boost
> 
> This looks mostly OK to me - I had a few comments on the CODEC driver
> but they're fairly minor.  It'd be good to get some input from the
> Kirkwood guys, particularly with regard to the merge path here.

I don't mind if you wish to carry it through the ASOC tree.  The bulk of 
it is ASOC related anyway.  However I think that "orion" names should be 
"kirkwood" throughout instead, as pure Orion chips have no audio 
interfaces.


Nicolas

[patch 0/5] kirkwood openrd client audio support - v3

[Mark Brown]

On Tue, May 25, 2010 at 09:10:54PM -0400, Nicolas Pitre wrote:

> I don't mind if you wish to carry it through the ASOC tree.  The bulk of 
> it is ASOC related anyway.  However I think that "orion" names should be 
> "kirkwood" throughout instead, as pure Orion chips have no audio 
> interfaces.

Can I add an ack from you for the patches with the name change, then?

[patch 0/5] kirkwood openrd client audio support - v3

[Nicolas Pitre]

On Tue, 25 May 2010, Mark Brown wrote:

> On Tue, May 25, 2010 at 09:10:54PM -0400, Nicolas Pitre wrote:
> 
> > I don't mind if you wish to carry it through the ASOC tree.  The bulk of 
> > it is ASOC related anyway.  However I think that "orion" names should be 
> > "kirkwood" throughout instead, as pure Orion chips have no audio 
> > interfaces.
> 
> Can I add an ack from you for the patches with the name change, then?

I'd like to see it first.


Nicolas

[patch 4/5] orion/kirkwood: Add i2s support

[saeed bishara]

On Wed, May 26, 2010 at 3:31 AM, Nicolas Pitre <nico@fluxnic.net> wrote:
> On Tue, 25 May 2010, apatard at mandriva.com wrote:
>
>> This patch enables support for the i2s controller available on orion/kirkwood platforms
>>
>> Signed-off-by: Arnaud Patard <apatard@mandriva.com>
>
> The actual Orion processor family has no I2S/audio support. ?It would be
> better if all the audio related stuff was renamed to "kirkwood" and
> "KIRKWOOD_*" instead.
This i2s was first included in orion90L (6183).

saeed

[patch 3/5] cs42l51: add asoc driver

[Arnaud Patard]

Mark Brown <broonie@opensource.wolfsonmicro.com> writes:

Hi,

> On Tue, May 25, 2010 at 02:22:39PM +0200, apatard at mandriva.com wrote:
>
>> +	/* route microphone */
>> +	ret = snd_soc_write(codec, CS42L51_ADC_INPUT, 0xF0);
>> +	if (ret < 0)
>> +		goto error_alloc;
>
> As I said last time this really should be visible to userspace rather
> than fixed function in the driver.
>

It is visible userspace. Look at the DAPM widgets
cs42l51_adcl_mux_controls/cs42l51_adcr_mux_controls. It's there to set a
nice default.


>> +#define DAPM_EVENT_PRE_POST_PMD	(SND_SOC_DAPM_PRE_PMD | SND_SOC_DAPM_POST_PMD)
>
> As I said last time this should be in soc-dapm.h rather than in the
> driver.
>

You told me it was not so important as I'm the only user so I have been
lazy and didn't sent a patch for that. Of course, if it's really
important, please say it and I'll be happy to send a patch to
soc-dapm.h.


>> +	SND_SOC_DAPM_DAC_E("Left DAC", "Left HiFi Playback",
>> +		CS42L51_POWER_CTL1, 5, 1,
>> +		cs42l51_pdn_event, DAPM_EVENT_PRE_POST_PMD),
>> +	SND_SOC_DAPM_DAC_E("Right DAC", "Right HiFi Playback",
>> +		CS42L51_POWER_CTL1, 6, 1,
>> +		cs42l51_pdn_event, DAPM_EVENT_PRE_POST_PMD),
>
> Your event here uses snd_soc_write() so for a stereo enable/disable
> you'll do extra writes.  If you changed it to use snd_soc_update_bits()
> then these would be suppressed.
>

ok. I didn't know about snd_soc_update_bits()


>> +	switch (format & SND_SOC_DAIFMT_MASTER_MASK) {
>> +	case SND_SOC_DAIFMT_CBM_CFM:
>> +		cs42l51->func = MODE_MASTER;
>> +		break;
>> +	default:
>> +	case SND_SOC_DAIFMT_CBS_CFS:
>> +		cs42l51->func = MODE_SLAVE_AUTO;
>> +		break;
>
> Is the device really capable of automatically coping with mixed master
> buses automatically,

hm... seeing such question means it may be yet an other bad naming. The
_AUTO part is refering to the speed mode of the codec. It's to set the
AUTO bit of the CS42L51_MIC_POWER_CTL reg and avoid most of the
clocks configurations.

Quote from the specs:
"Enables the auto-detect circuitry for detecting the speed mode of the
CODEC when operating as a slave.
When AUTO is enabled, the MCLK/LRCK ratio must be implemented according
to Table 3 on page 39. The SPEED[1:0] bits are ignored when this bit is
enabled. Speed is determined by the MCLK/LRCK ratio."

>  or should the default be to return an error?

I wanted to make sure things were working even with not supported
setting but it looks like it was not a good idea otherwise you wouldn't
ask. Will fix to return an error.

>
>> +	dev_info(&pdev->dev, "CS42L51 ALSA SoC Codec\n");
>
> I really would prefer to remove this.

I'd like to keep it. Being able to tell from logs if the codec driver
has been loaded or not is really nice for debugging purpose.

>
>> +struct cs42l51_setup_data {
>> +	int i2c_bus;
>> +	unsigned short i2c_address;
>> +};
>
> This is unused and could be removed.

ok.

Thanks,
Arnaud

[patch 4/5] orion/kirkwood: Add i2s support

[Arnaud Patard]

Nicolas Pitre <nico@fluxnic.net> writes:

Hi,

> On Tue, 25 May 2010, apatard at mandriva.com wrote:
>
>> This patch enables support for the i2s controller available on orion/kirkwood platforms
>> 
>> Signed-off-by: Arnaud Patard <apatard@mandriva.com>
>
> The actual Orion processor family has no I2S/audio support.  It would be 
> better if all the audio related stuff was renamed to "kirkwood" and 
> "KIRKWOOD_*" instead.

I've been told there was some orion with audio. As I've never looked at
the orion family in details, I assumed this is right and I've changed the
naming accordingly. I can change again but it would be nice to have an
agreement here. Who's/what's right ? orion can or can not have i2s audio ?


Arnaud

[patch 4/5] orion/kirkwood: Add i2s support

[Nicolas Pitre]

On Wed, 26 May 2010, saeed bishara wrote:

> On Wed, May 26, 2010 at 3:31 AM, Nicolas Pitre <nico@fluxnic.net> wrote:
> > On Tue, 25 May 2010, apatard at mandriva.com wrote:
> >
> >> This patch enables support for the i2s controller available on orion/kirkwood platforms
> >>
> >> Signed-off-by: Arnaud Patard <apatard@mandriva.com>
> >
> > The actual Orion processor family has no I2S/audio support. ?It would be
> > better if all the audio related stuff was renamed to "kirkwood" and
> > "KIRKWOOD_*" instead.
> This i2s was first included in orion90L (6183).

This has no presence in the mainline source tree, nor any hardware that 
people may care about at this point.  So it is probably best to refer to 
it as being kirkwood related more than orion related.


Nicolas

[patch 4/5] orion/kirkwood: Add i2s support

[Nicolas Pitre]

On Wed, 26 May 2010, Arnaud Patard wrote:

> Nicolas Pitre <nico@fluxnic.net> writes:
> 
> Hi,
> 
> > On Tue, 25 May 2010, apatard at mandriva.com wrote:
> >
> >> This patch enables support for the i2s controller available on orion/kirkwood platforms
> >> 
> >> Signed-off-by: Arnaud Patard <apatard@mandriva.com>
> >
> > The actual Orion processor family has no I2S/audio support.  It would be 
> > better if all the audio related stuff was renamed to "kirkwood" and 
> > "KIRKWOOD_*" instead.
> 
> I've been told there was some orion with audio. As I've never looked at
> the orion family in details, I assumed this is right and I've changed the
> naming accordingly.

Most Orion flavours don't.  And the one that does is not supported in 
mainline (most probably because hardware is not widely 
available/interesting).

> I can change again but it would be nice to have an
> agreement here. Who's/what's right ? orion can or can not have i2s audio ?

Given that Kirkwood is the first (and perhaps the only one) SOC to make 
use of this in mainline Linux, I think it is less confusing to name it 
"kirkwood".  That won't prevent the Orion90L from using Kirkwood I2S if 
that ever happens.


Nicolas

[alsa-devel] [patch 3/5] cs42l51: add asoc driver

[Mark Brown]

On Wed, May 26, 2010 at 10:20:33AM +0200, Arnaud Patard wrote:
> Mark Brown <broonie@opensource.wolfsonmicro.com> writes:
> > On Tue, May 25, 2010 at 02:22:39PM +0200, apatard at mandriva.com wrote:

> >> +	/* route microphone */
> >> +	ret = snd_soc_write(codec, CS42L51_ADC_INPUT, 0xF0);
> >> +	if (ret < 0)
> >> +		goto error_alloc;

> > As I said last time this really should be visible to userspace rather
> > than fixed function in the driver.

> It is visible userspace. Look at the DAPM widgets
> cs42l51_adcl_mux_controls/cs42l51_adcr_mux_controls. It's there to set a
> nice default.

As discussed last time you should use the hardware defaults.  This
configuration may not be deisrable on some other boards so best to
assume that the hardware default will at worst be non-harmful.

> >> +#define DAPM_EVENT_PRE_POST_PMD	(SND_SOC_DAPM_PRE_PMD | SND_SOC_DAPM_POST_PMD)

> > As I said last time this should be in soc-dapm.h rather than in the
> > driver.

> You told me it was not so important as I'm the only user so I have been
> lazy and didn't sent a patch for that. Of course, if it's really
> important, please say it and I'll be happy to send a patch to
> soc-dapm.h.

It wasn't a reason to reject the driver in and of itself but it is
disappointing to see that it wasn't fixed when the patch was respun.

> >> +	switch (format & SND_SOC_DAIFMT_MASTER_MASK) {
> >> +	case SND_SOC_DAIFMT_CBM_CFM:
> >> +		cs42l51->func = MODE_MASTER;
> >> +		break;
> >> +	default:
> >> +	case SND_SOC_DAIFMT_CBS_CFS:
> >> +		cs42l51->func = MODE_SLAVE_AUTO;
> >> +		break;

> > Is the device really capable of automatically coping with mixed master
> > buses automatically,

> hm... seeing such question means it may be yet an other bad naming. The
> _AUTO part is refering to the speed mode of the codec. It's to set the
> AUTO bit of the CS42L51_MIC_POWER_CTL reg and avoid most of the
> clocks configurations.

...

> >  or should the default be to return an error?

> I wanted to make sure things were working even with not supported
> setting but it looks like it was not a good idea otherwise you wouldn't
> ask. Will fix to return an error.

If you accept the value then it looks like the setting is supposed to
work, which it obviously won't do if the device is not able to support
it.  This won't help anyone trying to debug a broken clocking setup.

> >> +	dev_info(&pdev->dev, "CS42L51 ALSA SoC Codec\n");

> > I really would prefer to remove this.

> I'd like to keep it. Being able to tell from logs if the codec driver
> has been loaded or not is really nice for debugging purpose.

ASoC already generates debug logs for you when the driver is registered
with the core, which is the critical bit here.  There's no need to
clutter up the console with non-information like this, it just slows
things down.