diff mbox series

[56/56] ASoC: simple-card-utils: Move snd_soc_component_is_codec to be local

Message ID 20220519154318.2153729-57-ckeepax@opensource.cirrus.com (mailing list archive)
State Accepted
Commit 28086d05ada6d03daa886aad0e469854b811311c
Headers show
Series Specify clock provider directly to CPU DAIs | expand

Commit Message

Charles Keepax May 19, 2022, 3:43 p.m. UTC
The helper function snd_soc_component_is_codec is based off the
presence of the non_legacy_dai_naming flag. This isn't super robust
as CPU side components may also specify this flag, and indeed the
kernel already contains a couple that do. After componentisation there
isn't really a totally robust solution to identifying what is a CODEC
driver, without introducing a flag specifically for that purpose, and
really the desirable direction to move in is that the distinction
doesn't matter.

This patch does two things to try to mitigate these problems. Firstly,
now that all the other users of the helper function have been removed,
it makes the helper function local to the driver rather, than being
part of the core. This should help to discourage any new code from
being created that depends on the CODEC driver distinction. Secondly,
it updates the helper function itself to use the endianness flag
rather than the non_legacy_dai_naming flag. The endianness flag is
definitely invalid on a CPU side component, so it a more reliable
indicator that the device is definitely a CODEC. The vast majority of
buses require the CODEC to set the endianness flag, so the number of
corner cases should be fairly minimal. It is worth noting that CODECs
sending audio over SPI, or built into the CPU CODECs are potential
corner cases, however the hope is that in most cases those types of
devices do not consitute a simple audio card.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 include/sound/soc-component.h         | 5 -----
 sound/soc/generic/simple-card-utils.c | 7 ++++++-
 2 files changed, 6 insertions(+), 6 deletions(-)

Comments

Kuninori Morimoto May 20, 2022, 1:02 a.m. UTC | #1
Hi

This is not related to this patch, but...

> -static inline int snd_soc_component_is_codec(struct snd_soc_component *component)
> -{
> -	return component->driver->non_legacy_dai_naming;
> -}
(snip)
> +static inline int asoc_simple_component_is_codec(struct snd_soc_component *component)
> +{
> +	return component->driver->endianness;
> +}

I have added "endianness" "non_legacy_dai_naming" to component
when convert old "Codec style" into current "Component style".
All codec needs to have these 2.

	69941bab7c7aeaa7bf7e84397e294c17f0b7c6df
	("ASoC: snd_soc_component_driver has non_legacy_dai_naming")

	273d778ef38a8861f880e9df5799029dc82bd55d
	("ASoC: snd_soc_component_driver has endianness")

The reason why I didn't use "codec" was that try to keep
original style as much as possible.
But it seems this is good time to use "codec" for it ?
I think the code will be more understandable.

-	.endianness
-	.non_legacy_dai_naming
+	.is_codec

Thank you for your help !!

Best regards
---
Kuninori Morimoto
Charles Keepax May 20, 2022, 10:24 a.m. UTC | #2
On Fri, May 20, 2022 at 01:02:33AM +0000, Kuninori Morimoto wrote:
> 
> Hi
> 
> This is not related to this patch, but...
> 
> > -static inline int snd_soc_component_is_codec(struct snd_soc_component *component)
> > -{
> > -	return component->driver->non_legacy_dai_naming;
> > -}
> (snip)
> > +static inline int asoc_simple_component_is_codec(struct snd_soc_component *component)
> > +{
> > +	return component->driver->endianness;
> > +}
> 
> I have added "endianness" "non_legacy_dai_naming" to component
> when convert old "Codec style" into current "Component style".
> All codec needs to have these 2.
> 
> 	69941bab7c7aeaa7bf7e84397e294c17f0b7c6df
> 	("ASoC: snd_soc_component_driver has non_legacy_dai_naming")
> 
> 	273d778ef38a8861f880e9df5799029dc82bd55d
> 	("ASoC: snd_soc_component_driver has endianness")
> 
> The reason why I didn't use "codec" was that try to keep
> original style as much as possible.
> But it seems this is good time to use "codec" for it ?
> I think the code will be more understandable.
> 
> -	.endianness
> -	.non_legacy_dai_naming
> +	.is_codec

Yeah I considered this but it didn't really feel like the right
way to go to me. Firstly, at this stage we almost certainly need
to keep the endianness and non_legacy_dai_naming flags, there are
corner cases when endianness probably shouldn't be applied to CODECs
(as noted in my endianness series), and there are platform drivers
that use non_legacy_dai_naming.

We could add an is_codec flag along side the other two. But it
means a whole extra flag and means the the core is still requiring
a concept of what is a CODEC driver, which really we want to get
rid of as part of componentisation.

My thinking was that, whilst making this function local to the
driver isn't perfect, simple card will be used with simple CODECs
that are likely to exist on a standard audio bus, and thus have
endianness and the newer graph cards don't require specific
identification of what is a CODEC driver. So it probably works as
a solution for now.

> 
> Thank you for your help !!

Absolutely no problem, thank you for all the work you have done
on this over the years.

Thanks,
Charles
Kuninori Morimoto May 22, 2022, 11:26 p.m. UTC | #3
Hi

> > -	.endianness
> > -	.non_legacy_dai_naming
> > +	.is_codec
> 
> Yeah I considered this but it didn't really feel like the right
> way to go to me. Firstly, at this stage we almost certainly need
> to keep the endianness and non_legacy_dai_naming flags, there are
> corner cases when endianness probably shouldn't be applied to CODECs
> (as noted in my endianness series), and there are platform drivers
> that use non_legacy_dai_naming.

Oops, yes indeed :)
Thank you for pointing it.

> We could add an is_codec flag along side the other two. But it
> means a whole extra flag and means the the core is still requiring
> a concept of what is a CODEC driver, which really we want to get
> rid of as part of componentisation.

Hmm... yes, indeed.

Thank you for your help !!

Best regards
---
Kuninori Morimoto
diff mbox series

Patch

diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h
index 5a764c3099d3e..5c4cfa70b018c 100644
--- a/include/sound/soc-component.h
+++ b/include/sound/soc-component.h
@@ -348,11 +348,6 @@  static inline int snd_soc_component_cache_sync(
 	return regcache_sync(component->regmap);
 }
 
-static inline int snd_soc_component_is_codec(struct snd_soc_component *component)
-{
-	return component->driver->non_legacy_dai_naming;
-}
-
 void snd_soc_component_set_aux(struct snd_soc_component *component,
 			       struct snd_soc_aux_dev *aux);
 int snd_soc_component_init(struct snd_soc_component *component);
diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c
index 539d7f081bd79..50a9827089335 100644
--- a/sound/soc/generic/simple-card-utils.c
+++ b/sound/soc/generic/simple-card-utils.c
@@ -513,6 +513,11 @@  static int asoc_simple_init_dai(struct snd_soc_dai *dai,
 	return 0;
 }
 
+static inline int asoc_simple_component_is_codec(struct snd_soc_component *component)
+{
+	return component->driver->endianness;
+}
+
 static int asoc_simple_init_dai_link_params(struct snd_soc_pcm_runtime *rtd,
 					    struct simple_dai_props *dai_props)
 {
@@ -524,7 +529,7 @@  static int asoc_simple_init_dai_link_params(struct snd_soc_pcm_runtime *rtd,
 
 	/* Only Codecs */
 	for_each_rtd_components(rtd, i, component) {
-		if (!snd_soc_component_is_codec(component))
+		if (!asoc_simple_component_is_codec(component))
 			return 0;
 	}