diff mbox series

[6/9] ASoC: fsl: switch to use snd_soc_daifmt_parse_format/clock_provider()

Message ID 87wnr5ciyz.wl-kuninori.morimoto.gx@renesas.com (mailing list archive)
State Superseded
Headers show
Series ASoC: tidyup snd_soc_of_parse_daifmt() | expand

Commit Message

Kuninori Morimoto June 8, 2021, 12:12 a.m. UTC
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(-)

Comments

Jerome Brunet June 8, 2021, 7:50 a.m. UTC | #1
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;
> -		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));

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 */
Mark Brown June 8, 2021, 12:34 p.m. UTC | #2
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.
Kuninori Morimoto June 8, 2021, 11:50 p.m. UTC | #3
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
diff mbox series

Patch

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 */