[PATCH 0/9] ASoC: tidyup snd_soc_of_parse_daifmt()
Hi Mark
I want to add new audio-graph-card2 sound card driver, and this is last part of necessary soc-core cleanup for it.
Current some drivers are using DT, and Then, snd_soc_of_parse_daifmt() parses daifmt, but bitclock/frame provider parsing part is one of headache, because we are assuming below both cases.
A) node { bitclock-master; frame-master; ... };
B) link { bitclock-master = <&xxx>; frame-master = <&xxx>; ... };
The original was style A), and style B) was added later.
snd_soc_of_parse_daifmt() parses A) style as original style, and user need to update to B) style for clock_provider part if needed. In such case, user need to re-parse it, like below.
daifmt = snd_soc_of_parse_daifmt(..., &bitclkmaster, &framemaster); daifmt &= ~SND_SOC_DAIFMT_MASTER_MASK;
if (codec == bitclkmaster) daifmt |= (codec == framemaster) ? SND_SOC_DAIFMT_CBM_CFM : SND_SOC_DAIFMT_CBM_CFS; else daifmt |= (codec == framemaster) ? SND_SOC_DAIFMT_CBS_CFM : SND_SOC_DAIFMT_CBS_CFS;
This patch-set adds new functions, and handle these more simply. Unfortunately, there are too many use-case, do it by 1 function was implessible.
style A) bit_frame = snd_soc_daifmt_parse_clock_provider(); daifmt = snd_soc_daifmt_parse_format(...) | /* format part */ snd_soc_daifmt_clock_provider_pickup(bit_frame); /* clock part */
style B) snd_soc_daifmt_parse_clock_provider(..., &bit, &frame); daifmt = snd_soc_daifmt_parse_format(...) | /* format part */ snd_soc_daifmt_clock_provider_pickup( /* clock part */ ((codec == bit) << 4) + (codec == frame));
Kuninori Morimoto (9): ASoC: soc-core: don't use discriminatory terms on snd_soc_runtime_get_dai_fmt() ASoC: soc-core: add snd_soc_daifmt_clock_provider_pickup() ASoC: soc-core: add snd_soc_daifmt_clock_provider_fliped() ASoC: soc-core: add snd_soc_daifmt_parse_format/clock_provider() ASoC: atmel: switch to use snd_soc_daifmt_parse_format/clock_provider() ASoC: fsl: switch to use snd_soc_daifmt_parse_format/clock_provider() ASoC: meson: switch to use snd_soc_daifmt_parse_format/clock_provider() ASoC: simple-card-utils: switch to use snd_soc_daifmt_parse_format/clock_provider() ASoC: soc-core: remove snd_soc_of_parse_daifmt()
include/sound/soc.h | 13 +++- sound/soc/atmel/mikroe-proto.c | 18 ++--- sound/soc/fsl/fsl-asoc-card.c | 16 +--- sound/soc/generic/simple-card-utils.c | 19 ++--- sound/soc/meson/meson-card-utils.c | 15 ++-- sound/soc/soc-core.c | 103 ++++++++++++++++---------- 6 files changed, 99 insertions(+), 85 deletions(-)
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
snd_soc_runtime_get_dai_fmt() is using discriminatory terms. This patch fixup it.
Fixes: ba9e82a1c891 ("ASoC: soc-core: add snd_soc_runtime_get_dai_fmt()") Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/soc-core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 4daa9b22b33c..44e65f984a5c 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1211,8 +1211,8 @@ static void snd_soc_runtime_get_dai_fmt(struct snd_soc_pcm_runtime *rtd) mask |= SND_SOC_DAIFMT_CLOCK_MASK; if (!(dai_link->dai_fmt & SND_SOC_DAIFMT_INV_MASK)) mask |= SND_SOC_DAIFMT_INV_MASK; - if (!(dai_link->dai_fmt & SND_SOC_DAIFMT_MASTER_MASK)) - mask |= SND_SOC_DAIFMT_MASTER_MASK; + if (!(dai_link->dai_fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK)) + mask |= SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK;
dai_link->dai_fmt |= (dai_fmt & mask); }
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
This patch adds snd_soc_daifmt_clock_provider_pickup() function to judge clock/frame master. This is prepare for snd_soc_of_parse_daifmt() cleanup.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- include/sound/soc.h | 1 + sound/soc/soc-core.c | 33 +++++++++++++++++++-------------- 2 files changed, 20 insertions(+), 14 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h index e746da996351..e852cfbaf572 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -1232,6 +1232,7 @@ void snd_soc_of_parse_audio_prefix(struct snd_soc_card *card, int snd_soc_of_parse_audio_routing(struct snd_soc_card *card, const char *propname); int snd_soc_of_parse_aux_devs(struct snd_soc_card *card, const char *propname); +unsigned int snd_soc_daifmt_clock_provider_pickup(unsigned int bit_frame); unsigned int snd_soc_of_parse_daifmt(struct device_node *np, const char *prefix, struct device_node **bitclkmaster, diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 44e65f984a5c..2ce73bf77c05 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -3017,6 +3017,24 @@ int snd_soc_of_parse_aux_devs(struct snd_soc_card *card, const char *propname) } EXPORT_SYMBOL_GPL(snd_soc_of_parse_aux_devs);
+unsigned int snd_soc_daifmt_clock_provider_pickup(unsigned int bit_frame) +{ + /* Codec base */ + switch (bit_frame) { + case 0x11: + return SND_SOC_DAIFMT_CBP_CFP; + case 0x10: + return SND_SOC_DAIFMT_CBP_CFC; + case 0x01: + return SND_SOC_DAIFMT_CBC_CFP; + default: + return SND_SOC_DAIFMT_CBC_CFC; + } + + return 0; +} +EXPORT_SYMBOL_GPL(snd_soc_daifmt_clock_provider_pickup); + unsigned int snd_soc_of_parse_daifmt(struct device_node *np, const char *prefix, struct device_node **bitclkmaster, @@ -3115,20 +3133,7 @@ unsigned int snd_soc_of_parse_daifmt(struct device_node *np, if (frame && framemaster) *framemaster = of_parse_phandle(np, prop, 0);
- switch ((bit << 4) + frame) { - case 0x11: - format |= SND_SOC_DAIFMT_CBM_CFM; - break; - case 0x10: - format |= SND_SOC_DAIFMT_CBM_CFS; - break; - case 0x01: - format |= SND_SOC_DAIFMT_CBS_CFM; - break; - default: - format |= SND_SOC_DAIFMT_CBS_CFS; - break; - } + format |= snd_soc_daifmt_clock_provider_pickup((bit << 4) + frame);
return format; }
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
Sometimes we want to get CLOCK_PROVIDER fliped dai_fmt. This patch adds new snd_soc_daifmt_clock_provider_fliped() for it.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- include/sound/soc.h | 2 ++ sound/soc/soc-core.c | 40 +++++++++++++++++++++++++--------------- 2 files changed, 27 insertions(+), 15 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h index e852cfbaf572..0f25d4be92ae 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -1232,6 +1232,8 @@ void snd_soc_of_parse_audio_prefix(struct snd_soc_card *card, int snd_soc_of_parse_audio_routing(struct snd_soc_card *card, const char *propname); int snd_soc_of_parse_aux_devs(struct snd_soc_card *card, const char *propname); + +unsigned int snd_soc_daifmt_clock_provider_fliped(unsigned int dai_fmt); unsigned int snd_soc_daifmt_clock_provider_pickup(unsigned int bit_frame); unsigned int snd_soc_of_parse_daifmt(struct device_node *np, const char *prefix, diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 2ce73bf77c05..fd5a3b60aeb3 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1249,21 +1249,8 @@ int snd_soc_runtime_set_dai_fmt(struct snd_soc_pcm_runtime *rtd, * Flip the polarity for the "CPU" end of a CODEC<->CODEC link * the component which has non_legacy_dai_naming is Codec */ - inv_dai_fmt = dai_fmt & ~SND_SOC_DAIFMT_MASTER_MASK; - switch (dai_fmt & SND_SOC_DAIFMT_MASTER_MASK) { - case SND_SOC_DAIFMT_CBM_CFM: - inv_dai_fmt |= SND_SOC_DAIFMT_CBS_CFS; - break; - case SND_SOC_DAIFMT_CBM_CFS: - inv_dai_fmt |= SND_SOC_DAIFMT_CBS_CFM; - break; - case SND_SOC_DAIFMT_CBS_CFM: - inv_dai_fmt |= SND_SOC_DAIFMT_CBM_CFS; - break; - case SND_SOC_DAIFMT_CBS_CFS: - inv_dai_fmt |= SND_SOC_DAIFMT_CBM_CFM; - break; - } + inv_dai_fmt = snd_soc_daifmt_clock_provider_fliped(dai_fmt); + for_each_rtd_cpu_dais(rtd, i, cpu_dai) { unsigned int fmt = dai_fmt;
@@ -3017,6 +3004,29 @@ int snd_soc_of_parse_aux_devs(struct snd_soc_card *card, const char *propname) } EXPORT_SYMBOL_GPL(snd_soc_of_parse_aux_devs);
+unsigned int snd_soc_daifmt_clock_provider_fliped(unsigned int dai_fmt) +{ + unsigned int inv_dai_fmt = dai_fmt & ~SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK; + + switch (dai_fmt & SND_SOC_DAIFMT_CLOCK_PROVIDER_MASK) { + case SND_SOC_DAIFMT_CBP_CFP: + inv_dai_fmt |= SND_SOC_DAIFMT_CBC_CFC; + break; + case SND_SOC_DAIFMT_CBP_CFC: + inv_dai_fmt |= SND_SOC_DAIFMT_CBC_CFP; + break; + case SND_SOC_DAIFMT_CBC_CFP: + inv_dai_fmt |= SND_SOC_DAIFMT_CBP_CFC; + break; + case SND_SOC_DAIFMT_CBC_CFC: + inv_dai_fmt |= SND_SOC_DAIFMT_CBP_CFP; + break; + } + + return inv_dai_fmt; +} +EXPORT_SYMBOL_GPL(snd_soc_daifmt_clock_provider_fliped); + unsigned int snd_soc_daifmt_clock_provider_pickup(unsigned int bit_frame) { /* Codec base */
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
snd_soc_of_parse_daifmt() parses daifmt, but bitclock/frame provider parsing part is one of headache, because we are assuming below both cases.
A) node { bitclock-master; frame-master; ... };
B) link { bitclock-master = <&xxx>; frame-master = <&xxx>; ... };
The original was style A), and style B) was added later by commit b3ca11ff59bc ("ASoC: simple-card: Move dai-link level properties away from dai subnodes").
snd_soc_of_parse_daifmt() parses A) style as original style, and user need to update to B) style for clock_provider part if needed. In such case, user need to re-parse it, like below.
daifmt = snd_soc_of_parse_daifmt(..., &bitclkmaster, &framemaster); daifmt &= ~SND_SOC_DAIFMT_MASTER_MASK;
if (codec == bitclkmaster) daifmt |= (codec == framemaster) ? SND_SOC_DAIFMT_CBM_CFM : SND_SOC_DAIFMT_CBM_CFS; else daifmt |= (codec == framemaster) ? SND_SOC_DAIFMT_CBS_CFM : SND_SOC_DAIFMT_CBS_CFS;
This patch adds new functions which separetes snd_soc_of_parse_daifmt() helper function into parsing format part (= snd_soc_daifmt_parse_format()), and parsing clock_provider part (= snd_soc_daifmt_parse_clock_provider()). User can use snd_soc_daifmt_clock_provider_pickup/fliped() helper function with it.
style A) bit_frame = snd_soc_daifmt_parse_clock_provider(); daifmt = snd_soc_daifmt_parse_format(...) | /* format part */ snd_soc_daifmt_clock_provider_pickup(bit_frame); /* clock part */
style B) snd_soc_daifmt_parse_clock_provider(..., &bit, &frame); daifmt = snd_soc_daifmt_parse_format(...) | /* format part */ snd_soc_daifmt_clock_provider_pickup( /* clock part */ ((codec == bit) << 4) + (codec == frame));
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- include/sound/soc.h | 6 +++ sound/soc/soc-core.c | 114 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 120 insertions(+)
diff --git a/include/sound/soc.h b/include/sound/soc.h index 0f25d4be92ae..f402b259a255 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -1235,6 +1235,12 @@ int snd_soc_of_parse_aux_devs(struct snd_soc_card *card, const char *propname);
unsigned int snd_soc_daifmt_clock_provider_fliped(unsigned int dai_fmt); unsigned int snd_soc_daifmt_clock_provider_pickup(unsigned int bit_frame); +unsigned int snd_soc_daifmt_parse_format(struct device_node *np, + const char *prefix); +unsigned int snd_soc_daifmt_parse_clock_provider(struct device_node *np, + const char *prefix, + struct device_node **bitclkmaster, + struct device_node **framemaster); unsigned int snd_soc_of_parse_daifmt(struct device_node *np, const char *prefix, struct device_node **bitclkmaster, diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index fd5a3b60aeb3..a9cb39c3d8c5 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -3045,6 +3045,120 @@ unsigned int snd_soc_daifmt_clock_provider_pickup(unsigned int bit_frame) } EXPORT_SYMBOL_GPL(snd_soc_daifmt_clock_provider_pickup);
+unsigned int snd_soc_daifmt_parse_format(struct device_node *np, + const char *prefix) +{ + int ret, i; + char prop[128]; + unsigned int format = 0; + int bit, frame; + const char *str; + struct { + char *name; + unsigned int val; + } of_fmt_table[] = { + { "i2s", SND_SOC_DAIFMT_I2S }, + { "right_j", SND_SOC_DAIFMT_RIGHT_J }, + { "left_j", SND_SOC_DAIFMT_LEFT_J }, + { "dsp_a", SND_SOC_DAIFMT_DSP_A }, + { "dsp_b", SND_SOC_DAIFMT_DSP_B }, + { "ac97", SND_SOC_DAIFMT_AC97 }, + { "pdm", SND_SOC_DAIFMT_PDM}, + { "msb", SND_SOC_DAIFMT_MSB }, + { "lsb", SND_SOC_DAIFMT_LSB }, + }; + + if (!prefix) + prefix = ""; + + /* + * check "dai-format = xxx" + * or "[prefix]format = xxx" + * SND_SOC_DAIFMT_FORMAT_MASK area + */ + ret = of_property_read_string(np, "dai-format", &str); + if (ret < 0) { + snprintf(prop, sizeof(prop), "%sformat", prefix); + ret = of_property_read_string(np, prop, &str); + } + if (ret == 0) { + for (i = 0; i < ARRAY_SIZE(of_fmt_table); i++) { + if (strcmp(str, of_fmt_table[i].name) == 0) { + format |= of_fmt_table[i].val; + break; + } + } + } + + /* + * check "[prefix]continuous-clock" + * SND_SOC_DAIFMT_CLOCK_MASK area + */ + snprintf(prop, sizeof(prop), "%scontinuous-clock", prefix); + if (of_property_read_bool(np, prop)) + format |= SND_SOC_DAIFMT_CONT; + else + format |= SND_SOC_DAIFMT_GATED; + + /* + * check "[prefix]bitclock-inversion" + * check "[prefix]frame-inversion" + * SND_SOC_DAIFMT_INV_MASK area + */ + snprintf(prop, sizeof(prop), "%sbitclock-inversion", prefix); + bit = !!of_get_property(np, prop, NULL); + + snprintf(prop, sizeof(prop), "%sframe-inversion", prefix); + frame = !!of_get_property(np, prop, NULL); + + switch ((bit << 4) + frame) { + case 0x11: + format |= SND_SOC_DAIFMT_IB_IF; + break; + case 0x10: + format |= SND_SOC_DAIFMT_IB_NF; + break; + case 0x01: + format |= SND_SOC_DAIFMT_NB_IF; + break; + default: + /* SND_SOC_DAIFMT_NB_NF is default */ + break; + } + + return format; +} +EXPORT_SYMBOL_GPL(snd_soc_daifmt_parse_format); + +unsigned int snd_soc_daifmt_parse_clock_provider(struct device_node *np, + const char *prefix, + struct device_node **bitclkmaster, + struct device_node **framemaster) +{ + char prop[128]; + unsigned int bit, frame; + + if (!prefix) + prefix = ""; + + /* + * check "[prefix]bitclock-master" + * check "[prefix]frame-master" + */ + snprintf(prop, sizeof(prop), "%sbitclock-master", prefix); + bit = !!of_get_property(np, prop, NULL); + if (bit && bitclkmaster) + *bitclkmaster = of_parse_phandle(np, prop, 0); + + snprintf(prop, sizeof(prop), "%sframe-master", prefix); + frame = !!of_get_property(np, prop, NULL); + if (frame && framemaster) + *framemaster = of_parse_phandle(np, prop, 0); + + return (bit << 4) + frame; +} +EXPORT_SYMBOL_GPL(snd_soc_daifmt_parse_clock_provider); + unsigned int snd_soc_of_parse_daifmt(struct device_node *np, const char *prefix, struct device_node **bitclkmaster,
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
This patch switch to use snd_soc_daifmt_parse_format/clock_provider() from snd_soc_of_parse_daifmt().
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/atmel/mikroe-proto.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/sound/soc/atmel/mikroe-proto.c b/sound/soc/atmel/mikroe-proto.c index f9a85fd01b79..eeb2effed1e4 100644 --- a/sound/soc/atmel/mikroe-proto.c +++ b/sound/soc/atmel/mikroe-proto.c @@ -69,6 +69,7 @@ static int snd_proto_probe(struct platform_device *pdev) struct device_node *bitclkmaster = NULL; struct device_node *framemaster = NULL; unsigned int dai_fmt; + unsigned int bit_frame; int ret = 0;
if (!np) { @@ -120,19 +121,18 @@ static int snd_proto_probe(struct platform_device *pdev) dai->cpus->of_node = cpu_np; dai->platforms->of_node = cpu_np;
- dai_fmt = snd_soc_of_parse_daifmt(np, NULL, - &bitclkmaster, &framemaster); + bit_frame = snd_soc_daifmt_parse_clock_provider(np, NULL, &bitclkmaster, &framemaster); if (bitclkmaster != framemaster) { dev_err(&pdev->dev, "Must be the same bitclock and frame master\n"); return -EINVAL; } - if (bitclkmaster) { - dai_fmt &= ~SND_SOC_DAIFMT_MASTER_MASK; - if (codec_np == bitclkmaster) - dai_fmt |= SND_SOC_DAIFMT_CBM_CFM; - else - dai_fmt |= SND_SOC_DAIFMT_CBS_CFS; - } + if (bitclkmaster) + bit_frame = ((codec_np == bitclkmaster) << 4) + + (codec_np == framemaster); + + dai_fmt = snd_soc_daifmt_parse_format(np, NULL) | + snd_soc_daifmt_clock_provider_pickup(bit_frame); + of_node_put(bitclkmaster); of_node_put(framemaster); dai->dai_fmt = dai_fmt;
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
This patch switch to use snd_soc_daifmt_parse_format/clock_provider() from snd_soc_of_parse_daifmt().
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/fsl/fsl-asoc-card.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/sound/soc/fsl/fsl-asoc-card.c b/sound/soc/fsl/fsl-asoc-card.c index c62bfd1c3ac7..6a6f098da0dc 100644 --- a/sound/soc/fsl/fsl-asoc-card.c +++ b/sound/soc/fsl/fsl-asoc-card.c @@ -540,7 +540,6 @@ static int fsl_asoc_card_probe(struct platform_device *pdev) struct device *codec_dev = NULL; const char *codec_dai_name; const char *codec_dev_name; - unsigned int daifmt; u32 width; int ret;
@@ -684,19 +683,12 @@ static int fsl_asoc_card_probe(struct platform_device *pdev) }
/* Format info from DT is optional. */ - daifmt = snd_soc_of_parse_daifmt(np, NULL, - &bitclkmaster, &framemaster); - daifmt &= ~SND_SOC_DAIFMT_MASTER_MASK; + snd_soc_daifmt_parse_clock_provider(np, NULL, &bitclkmaster, &framemaster); if (bitclkmaster || framemaster) { - if (codec_np == bitclkmaster) - daifmt |= (codec_np == framemaster) ? - SND_SOC_DAIFMT_CBM_CFM : SND_SOC_DAIFMT_CBM_CFS; - else - daifmt |= (codec_np == framemaster) ? - SND_SOC_DAIFMT_CBS_CFM : SND_SOC_DAIFMT_CBS_CFS; - /* Override dai_fmt with value from DT */ - priv->dai_fmt = daifmt; + priv->dai_fmt = snd_soc_daifmt_parse_format(np, NULL) | + snd_soc_daifmt_clock_provider_pickup(((codec_np == bitclkmaster) << 4) + + (codec_np == framemaster)); }
/* Change direction according to format */
On Tue 08 Jun 2021 at 02:12, Kuninori Morimoto kuninori.morimoto.gx@renesas.com wrote:
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
This patch switch to use snd_soc_daifmt_parse_format/clock_provider() from snd_soc_of_parse_daifmt().
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
sound/soc/fsl/fsl-asoc-card.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/sound/soc/fsl/fsl-asoc-card.c b/sound/soc/fsl/fsl-asoc-card.c index c62bfd1c3ac7..6a6f098da0dc 100644 --- a/sound/soc/fsl/fsl-asoc-card.c +++ b/sound/soc/fsl/fsl-asoc-card.c @@ -540,7 +540,6 @@ static int fsl_asoc_card_probe(struct platform_device *pdev) struct device *codec_dev = NULL; const char *codec_dai_name; const char *codec_dev_name;
- unsigned int daifmt; u32 width; int ret;
@@ -684,19 +683,12 @@ static int fsl_asoc_card_probe(struct platform_device *pdev) }
/* Format info from DT is optional. */
- daifmt = snd_soc_of_parse_daifmt(np, NULL,
&bitclkmaster, &framemaster);- daifmt &= ~SND_SOC_DAIFMT_MASTER_MASK;
- snd_soc_daifmt_parse_clock_provider(np, NULL, &bitclkmaster, &framemaster); if (bitclkmaster || framemaster) {
if (codec_np == bitclkmaster)daifmt |= (codec_np == framemaster) ?SND_SOC_DAIFMT_CBM_CFM : SND_SOC_DAIFMT_CBM_CFS;elsedaifmt |= (codec_np == framemaster) ?SND_SOC_DAIFMT_CBS_CFM : SND_SOC_DAIFMT_CBS_CFS;- /* Override dai_fmt with value from DT */
priv->dai_fmt = daifmt;
priv->dai_fmt = snd_soc_daifmt_parse_format(np, NULL) |snd_soc_daifmt_clock_provider_pickup(((codec_np == bitclkmaster) << 4) +(codec_np == framemaster));
Hi,
I understand you are trying to fold some code but I'm not sure about this snd_soc_daifmt_clock_provider_pickup().
Instead of repeating the if clause around DAIFMT (which is a bit verbose but fairly easy to understand and maintain) there is now the calculation of a made up constant (which is rather opaque as it is), which later translate to the same type of test around DAIFMT.
I'd be in favor of dropping the snd_soc_daifmt_clock_provider_pickup() part for the sake or readability. This apply to the rest of the series, not just fsl.
The rest looks good, Thx Kuninori.
}
/* Change direction according to format */
On Tue, Jun 08, 2021 at 09:50:52AM +0200, Jerome Brunet wrote:
I understand you are trying to fold some code but I'm not sure about this snd_soc_daifmt_clock_provider_pickup().
Instead of repeating the if clause around DAIFMT (which is a bit verbose but fairly easy to understand and maintain) there is now the calculation of a made up constant (which is rather opaque as it is), which later translate to the same type of test around DAIFMT.
I'd be in favor of dropping the snd_soc_daifmt_clock_provider_pickup() part for the sake or readability. This apply to the rest of the series, not just fsl.
Yeah, I'm also just finding the _pickup() name unclear and can't really immediately think how to make it clearer - I think the bit being factored out needs to be at least as complex/unclear as the interface being introduced to do so.
Hi Jerome, Mark
Thank you for your feedback
I understand you are trying to fold some code but I'm not sure about this snd_soc_daifmt_clock_provider_pickup().
Instead of repeating the if clause around DAIFMT (which is a bit verbose but fairly easy to understand and maintain) there is now the calculation of a made up constant (which is rather opaque as it is), which later translate to the same type of test around DAIFMT.
I'd be in favor of dropping the snd_soc_daifmt_clock_provider_pickup() part for the sake or readability. This apply to the rest of the series, not just fsl.
Yeah, I'm also just finding the _pickup() name unclear and can't really immediately think how to make it clearer - I think the bit being factored out needs to be at least as complex/unclear as the interface being introduced to do so.
Because there are many kind of use case, it was difficult to do this thing by 1 function. And indeed the naming was unclear.
Main purpose of this patch-set was that I want to have clock_provider related functions. But indeed it was a little overkill to force it to use it to existing drivers.
I will fixup it in v2
Thank you for your help !!
Best regards --- Kuninori Morimoto
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
This patch switch to use snd_soc_daifmt_parse_format/clock_provider() from snd_soc_of_parse_daifmt().
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/meson/meson-card-utils.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/sound/soc/meson/meson-card-utils.c b/sound/soc/meson/meson-card-utils.c index 300ac8be46ef..60959e2c71b8 100644 --- a/sound/soc/meson/meson-card-utils.c +++ b/sound/soc/meson/meson-card-utils.c @@ -119,18 +119,13 @@ unsigned int meson_card_parse_daifmt(struct device_node *node, struct device_node *framemaster = NULL; unsigned int daifmt;
- daifmt = snd_soc_of_parse_daifmt(node, "", - &bitclkmaster, &framemaster); - daifmt &= ~SND_SOC_DAIFMT_MASTER_MASK; + snd_soc_daifmt_parse_clock_provider(node, "", &bitclkmaster, &framemaster);
/* If no master is provided, default to cpu master */ - if (!bitclkmaster || bitclkmaster == cpu_node) { - daifmt |= (!framemaster || framemaster == cpu_node) ? - SND_SOC_DAIFMT_CBS_CFS : SND_SOC_DAIFMT_CBS_CFM; - } else { - daifmt |= (!framemaster || framemaster == cpu_node) ? - SND_SOC_DAIFMT_CBM_CFS : SND_SOC_DAIFMT_CBM_CFM; - } + daifmt = snd_soc_daifmt_parse_format(node, "") | + snd_soc_daifmt_clock_provider_pickup( + ((bitclkmaster && bitclkmaster != cpu_node) << 4) + + (framemaster && framemaster != cpu_node));
of_node_put(bitclkmaster); of_node_put(framemaster);
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
This patch switch to use snd_soc_daifmt_parse_format/clock_provider() from snd_soc_of_parse_daifmt().
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/generic/simple-card-utils.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c index fa1247f0dda1..de23e6e7f3f3 100644 --- a/sound/soc/generic/simple-card-utils.c +++ b/sound/soc/generic/simple-card-utils.c @@ -60,10 +60,9 @@ int asoc_simple_parse_daifmt(struct device *dev, struct device_node *bitclkmaster = NULL; struct device_node *framemaster = NULL; unsigned int daifmt; + unsigned int bit_frame;
- daifmt = snd_soc_of_parse_daifmt(node, prefix, - &bitclkmaster, &framemaster); - daifmt &= ~SND_SOC_DAIFMT_MASTER_MASK; + snd_soc_daifmt_parse_clock_provider(node, prefix, &bitclkmaster, &framemaster);
if (!bitclkmaster && !framemaster) { /* @@ -73,17 +72,15 @@ int asoc_simple_parse_daifmt(struct device *dev, */ dev_dbg(dev, "Revert to legacy daifmt parsing\n");
- daifmt = snd_soc_of_parse_daifmt(codec, NULL, NULL, NULL) | - (daifmt & ~SND_SOC_DAIFMT_CLOCK_MASK); + bit_frame = snd_soc_daifmt_parse_clock_provider(codec, NULL, NULL, NULL); } else { - if (codec == bitclkmaster) - daifmt |= (codec == framemaster) ? - SND_SOC_DAIFMT_CBM_CFM : SND_SOC_DAIFMT_CBM_CFS; - else - daifmt |= (codec == framemaster) ? - SND_SOC_DAIFMT_CBS_CFM : SND_SOC_DAIFMT_CBS_CFS; + bit_frame = ((codec == bitclkmaster) << 4) + + (codec == framemaster); }
+ daifmt = snd_soc_daifmt_parse_format(node, prefix) | + snd_soc_daifmt_clock_provider_pickup(bit_frame); + of_node_put(bitclkmaster); of_node_put(framemaster);
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
No driver is using snd_soc_of_parse_daifmt(). This patch removes it.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- include/sound/soc.h | 4 -- sound/soc/soc-core.c | 104 ------------------------------------------- 2 files changed, 108 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h index f402b259a255..75ad899fd84f 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -1241,10 +1241,6 @@ unsigned int snd_soc_daifmt_parse_clock_provider(struct device_node *np, const char *prefix, struct device_node **bitclkmaster, struct device_node **framemaster); -unsigned int snd_soc_of_parse_daifmt(struct device_node *np, - const char *prefix, - struct device_node **bitclkmaster, - struct device_node **framemaster); int snd_soc_get_dai_id(struct device_node *ep); int snd_soc_get_dai_name(const struct of_phandle_args *args, const char **dai_name); diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index a9cb39c3d8c5..3009a315a4a6 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -3159,110 +3159,6 @@ unsigned int snd_soc_daifmt_parse_clock_provider(struct device_node *np, } EXPORT_SYMBOL_GPL(snd_soc_daifmt_parse_clock_provider);
-unsigned int snd_soc_of_parse_daifmt(struct device_node *np, - const char *prefix, - struct device_node **bitclkmaster, - struct device_node **framemaster) -{ - int ret, i; - char prop[128]; - unsigned int format = 0; - int bit, frame; - const char *str; - struct { - char *name; - unsigned int val; - } of_fmt_table[] = { - { "i2s", SND_SOC_DAIFMT_I2S }, - { "right_j", SND_SOC_DAIFMT_RIGHT_J }, - { "left_j", SND_SOC_DAIFMT_LEFT_J }, - { "dsp_a", SND_SOC_DAIFMT_DSP_A }, - { "dsp_b", SND_SOC_DAIFMT_DSP_B }, - { "ac97", SND_SOC_DAIFMT_AC97 }, - { "pdm", SND_SOC_DAIFMT_PDM}, - { "msb", SND_SOC_DAIFMT_MSB }, - { "lsb", SND_SOC_DAIFMT_LSB }, - }; - - if (!prefix) - prefix = ""; - - /* - * check "dai-format = xxx" - * or "[prefix]format = xxx" - * SND_SOC_DAIFMT_FORMAT_MASK area - */ - ret = of_property_read_string(np, "dai-format", &str); - if (ret < 0) { - snprintf(prop, sizeof(prop), "%sformat", prefix); - ret = of_property_read_string(np, prop, &str); - } - if (ret == 0) { - for (i = 0; i < ARRAY_SIZE(of_fmt_table); i++) { - if (strcmp(str, of_fmt_table[i].name) == 0) { - format |= of_fmt_table[i].val; - break; - } - } - } - - /* - * check "[prefix]continuous-clock" - * SND_SOC_DAIFMT_CLOCK_MASK area - */ - snprintf(prop, sizeof(prop), "%scontinuous-clock", prefix); - if (of_property_read_bool(np, prop)) - format |= SND_SOC_DAIFMT_CONT; - else - format |= SND_SOC_DAIFMT_GATED; - - /* - * check "[prefix]bitclock-inversion" - * check "[prefix]frame-inversion" - * SND_SOC_DAIFMT_INV_MASK area - */ - snprintf(prop, sizeof(prop), "%sbitclock-inversion", prefix); - bit = !!of_get_property(np, prop, NULL); - - snprintf(prop, sizeof(prop), "%sframe-inversion", prefix); - frame = !!of_get_property(np, prop, NULL); - - switch ((bit << 4) + frame) { - case 0x11: - format |= SND_SOC_DAIFMT_IB_IF; - break; - case 0x10: - format |= SND_SOC_DAIFMT_IB_NF; - break; - case 0x01: - format |= SND_SOC_DAIFMT_NB_IF; - break; - default: - /* SND_SOC_DAIFMT_NB_NF is default */ - break; - } - - /* - * check "[prefix]bitclock-master" - * check "[prefix]frame-master" - * SND_SOC_DAIFMT_MASTER_MASK area - */ - snprintf(prop, sizeof(prop), "%sbitclock-master", prefix); - bit = !!of_get_property(np, prop, NULL); - if (bit && bitclkmaster) - *bitclkmaster = of_parse_phandle(np, prop, 0); - - snprintf(prop, sizeof(prop), "%sframe-master", prefix); - frame = !!of_get_property(np, prop, NULL); - if (frame && framemaster) - *framemaster = of_parse_phandle(np, prop, 0); - - format |= snd_soc_daifmt_clock_provider_pickup((bit << 4) + frame); - - return format; -} -EXPORT_SYMBOL_GPL(snd_soc_of_parse_daifmt); - int snd_soc_get_dai_id(struct device_node *ep) { struct snd_soc_component *component;
On 08 Jun 2021 09:11:17 +0900, Kuninori Morimoto wrote:
I want to add new audio-graph-card2 sound card driver, and this is last part of necessary soc-core cleanup for it.
Current some drivers are using DT, and Then, snd_soc_of_parse_daifmt() parses daifmt, but bitclock/frame provider parsing part is one of headache, because we are assuming below both cases.
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/9] ASoC: soc-core: don't use discriminatory terms on snd_soc_runtime_get_dai_fmt() commit: 640eac4c849d6390f272862ba8db14f28d423670
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
participants (3)
-
Jerome Brunet -
Kuninori Morimoto -
Mark Brown