[PATCH] ASoC:kirkwood: Don't raise an error when no DAI format

[Jean-Francois Moine]

The two DAIs of the kirkwood controller have a unique PCM format.

The simple-card sets the audio hardware definitions of all CPU DAIs.
The PCM format is defined only when it is present in the DT.

This patch prevents the controller to raise an error when
the DT audio card definition by the simple card contains the PCM
format of one CPU DAI only.

Signed-off-by: Jean-Francois Moine <***@free.fr>
---
sound/soc/kirkwood/kirkwood-i2s.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
index 0704cd6..26d5f85 100644
--- a/sound/soc/kirkwood/kirkwood-i2s.c
+++ b/sound/soc/kirkwood/kirkwood-i2s.c
@@ -45,6 +45,8 @@ static int kirkwood_i2s_set_fmt(struct snd_soc_dai *cpu_dai,
unsigned long value;

switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
+ case 0:
+ return 0; /* already done (simple-card) */
case SND_SOC_DAIFMT_RIGHT_J:
mask = KIRKWOOD_I2S_CTL_RJ;
break;

[Russell King - ARM Linux]

Post by Jean-Francois Moine
The two DAIs of the kirkwood controller have a unique PCM format.
The simple-card sets the audio hardware definitions of all CPU DAIs.
The PCM format is defined only when it is present in the DT.
This patch prevents the controller to raise an error when
the DT audio card definition by the simple card contains the PCM
format of one CPU DAI only.

I think this is a silly idea - why should every driver have additional code
to detect when it's called to do thing. Why doesn't the simple card code
always pass the required format?

Looking at other drivers, no one else does this; they all appear to require
the proper format to be specified.

What some drivers do (eg, omap-mcbsp.c) is to block set_fmt when the
DAI is already in use - setting a flag "configured" in hw_params, and
clearing it in shutdown. Maybe following this will solve your problem.

In any case, random drivers doing stuff differently without reason is
really not a good idea.

[Jean-Francois Moine]

On Sun, 28 Sep 2014 16:12:07 +0100

Post by Jean-Francois Moine
The two DAIs of the kirkwood controller have a unique PCM format.
=20
The simple-card sets the audio hardware definitions of all CPU DAIs=

=2E

Post by Jean-Francois Moine
The PCM format is defined only when it is present in the DT.
=20
This patch prevents the controller to raise an error when
the DT audio card definition by the simple card contains the PCM
format of one CPU DAI only.

=20
I think this is a silly idea - why should every driver have additiona=

l code

to detect when it's called to do thing. Why doesn't the simple card =

code

always pass the required format?
=20
Looking at other drivers, no one else does this; they all appear to r=

equire

the proper format to be specified.
=20
What some drivers do (eg, omap-mcbsp.c) is to block set_fmt when the
DAI is already in use - setting a flag "configured" in hw_params, and
clearing it in shutdown. Maybe following this will solve your proble=

m.

=20
In any case, random drivers doing stuff differently without reason is
really not a good idea.

As the simple card is done, the audio hardware definitions of the
platform and all the CPU DAIS are always set in the audio controller.

When the PCM format is globally defined (platform), the function
set_fmt() is called for all CPU DAIs (then, twice for the kirkwood
controller) with the same value. The DT is:

sound {
compatible =3D "simple-audio-card";
simple-audio-card,name =3D "Cubox Audio";
simple-audio-card,format =3D "i2s";

simple-audio-card,dai-***@0 { /* S/PDIF - HDMI & S/PDIF */
...
};
simple-audio-card,dai-***@1 { /* I2S - HDMI */
...
};
};

The PCM format may also be defined per DAI link. The following DT works
the same as setting globally the format, i.e. there are two calls to
set_fmt() with 'i2s':

sound {
compatible =3D "simple-audio-card";
simple-audio-card,name =3D "Cubox Audio";

simple-audio-card,dai-***@0 { /* S/PDIF - HDMI & S/PDIF */
format =3D "i2s";
...
};
simple-audio-card,dai-***@1 { /* I2S - HDMI */
format =3D "i2s";
...
};
};

The problem appears when the format is defined in only one DAI link:

sound {
compatible =3D "simple-audio-card";
simple-audio-card,name =3D "Cubox Audio";

simple-audio-card,dai-***@0 { /* S/PDIF - HDMI & S/PDIF */
format =3D "i2s";
...
};
simple-audio-card,dai-***@1 { /* I2S - HDMI */
/* no format */
...
};
};

Then, audio does not work.

--=20
Ken ar c'henta=C3=B1 | ** Breizh ha Linux atav! **
Jef | http://moinejf.free.fr/

[Mark Brown]

Post by Jean-Francois Moine

Post by Russell King - ARM Linux

Post by Jean-Francois Moine
This patch prevents the controller to raise an error when
the DT audio card definition by the simple card contains the PCM
format of one CPU DAI only.

I think this is a silly idea - why should every driver have additional code
to detect when it's called to do thing. Why doesn't the simple card code
always pass the required format?
Looking at other drivers, no one else does this; they all appear to require
the proper format to be specified.
What some drivers do (eg, omap-mcbsp.c) is to block set_fmt when the
DAI is already in use - setting a flag "configured" in hw_params, and
clearing it in shutdown. Maybe following this will solve your problem.
In any case, random drivers doing stuff differently without reason is
really not a good idea.

As the simple card is done, the audio hardware definitions of the
platform and all the CPU DAIS are always set in the audio controller.
When the PCM format is globally defined (platform), the function
set_fmt() is called for all CPU DAIs (then, twice for the kirkwood

...

Post by Jean-Francois Moine
The PCM format may also be defined per DAI link. The following DT works
the same as setting globally the format, i.e. there are two calls to
Then, audio does not work.

So this is still a generic rather than driver issue and should be solved
outside of the driver - exactly the same issue is going to apply to any
other device with a similar shared configuration. We could either say
that the DT is buggy here or we could say that the generic card ought to
assume that if only one link specifies a format then it should use that
format for other links if possible (since clearly the user doesn't care
what it chooses).

Please also try to use subject lines matching the style for the
subsystem. You've been working with upstream for a while, you really
ought to be familiar with this sort of basic process stuff.

[Jean-Francois Moine]

On Tue, 30 Sep 2014 19:19:10 +0100

Post by Mark Brown

Post by Jean-Francois Moine

Post by Russell King - ARM Linux

Post by Jean-Francois Moine
This patch prevents the controller to raise an error when
the DT audio card definition by the simple card contains the PCM
format of one CPU DAI only.

I think this is a silly idea - why should every driver have additional code
to detect when it's called to do thing. Why doesn't the simple card code
always pass the required format?
Looking at other drivers, no one else does this; they all appear to require
the proper format to be specified.
What some drivers do (eg, omap-mcbsp.c) is to block set_fmt when the
DAI is already in use - setting a flag "configured" in hw_params, and
clearing it in shutdown. Maybe following this will solve your problem.
In any case, random drivers doing stuff differently without reason is
really not a good idea.

As the simple card is done, the audio hardware definitions of the
platform and all the CPU DAIS are always set in the audio controller.
When the PCM format is globally defined (platform), the function
set_fmt() is called for all CPU DAIs (then, twice for the kirkwood

...

Post by Jean-Francois Moine
The PCM format may also be defined per DAI link. The following DT works
the same as setting globally the format, i.e. there are two calls to
Then, audio does not work.

So this is still a generic rather than driver issue and should be solved
outside of the driver - exactly the same issue is going to apply to any
other device with a similar shared configuration. We could either say
that the DT is buggy here or we could say that the generic card ought to
assume that if only one link specifies a format then it should use that
format for other links if possible (since clearly the user doesn't care
what it chooses).

The 'daifmt' of the callback function set_fmt (snd_soc_dai_set_fmt)
contains 4 parts:
- the physical format,
- the clock gating,
- the signal inversions,
- the master clock definitions.

As the value '0' is not used in the physical format nor in the master
clock, this value would have say 'don't change', permitting the
parameters to be set independently.

But, as you and Russell say 'no', I will let the format definition at
the platform level in the DT. Setting twice the hardware registers is
not a problem.

Thanks.

Post by Mark Brown
Please also try to use subject lines matching the style for the
subsystem. You've been working with upstream for a while, you really
ought to be familiar with this sort of basic process stuff.

Sorry.