[PATCH 0/3] ASoC: rsnd: add D3 support

[PATCH 0/3] ASoC: rsnd: add D3 support

[Kuninori Morimoto]

Hi Mark, Rob

These adds R-Car D3 support for rsnd driver.
[1/3] is tidyup patch for dt-bindings (not only for D3).
[2/3], [3/3] are for R-Car D3.

Kuninori Morimoto (3):
  ASoC: dt-bindings: renesas: rsnd: tidyup properties
  ASoC: rsnd: tidyup loop on rsnd_adg_clk_query()
  ASoC: rsnd: add null CLOCKIN support

 .../bindings/sound/renesas,rsnd.yaml          | 10 ++++-
 sound/soc/sh/rcar/adg.c                       | 37 ++++++++++++++++---
 2 files changed, 41 insertions(+), 6 deletions(-)

-- 
2.25.1

[PATCH 1/3] ASoC: dt-bindings: renesas: rsnd: tidyup properties

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

1) resets/reset-names needs minItems
2) It can use ports, not only port
3) It is not using audio-graph properties

Without this patch, we will get warnings

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 .../devicetree/bindings/sound/renesas,rsnd.yaml        | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/sound/renesas,rsnd.yaml b/Documentation/devicetree/bindings/sound/renesas,rsnd.yaml
index 605de3a5847f..ee936d1aa724 100644
--- a/Documentation/devicetree/bindings/sound/renesas,rsnd.yaml
+++ b/Documentation/devicetree/bindings/sound/renesas,rsnd.yaml
@@ -86,9 +86,11 @@ properties:
   power-domains: true
 
   resets:
+    minItems: 1
     maxItems: 11
 
   reset-names:
+    minItems: 1
     maxItems: 11
 
   clocks:
@@ -110,6 +112,13 @@ properties:
         - pattern: '^dvc\.[0-1]$'
         - pattern: '^clk_(a|b|c|i)$'
 
+  ports:
+    $ref: /schemas/graph.yaml#/properties/ports
+    properties:
+      port(@[0-9a-f]+)?:
+        $ref: audio-graph-port.yaml#
+        unevaluatedProperties: false
+
   port:
     $ref: audio-graph-port.yaml#
     unevaluatedProperties: false
@@ -257,7 +266,6 @@ required:
   - "#sound-dai-cells"
 
 allOf:
-  - $ref: audio-graph.yaml#
   - if:
       properties:
         compatible:
-- 
2.25.1

[PATCH 2/3] ASoC: rsnd: tidyup loop on rsnd_adg_clk_query()

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

commit 06e8f5c842f2d ("ASoC: rsnd: don't call clk_get_rate() under
atomic context") used saved clk_rate, thus for_each_rsnd_clk()
is no longer needed. This patch fixes it.

Fixes: 06e8f5c842f2d ("ASoC: rsnd: don't call clk_get_rate() under atomic context")
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/soc/sh/rcar/adg.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/sound/soc/sh/rcar/adg.c b/sound/soc/sh/rcar/adg.c
index f7773c41085b..a0b5bd5a7464 100644
--- a/sound/soc/sh/rcar/adg.c
+++ b/sound/soc/sh/rcar/adg.c
@@ -290,7 +290,6 @@ static void rsnd_adg_set_ssi_clk(struct rsnd_mod *ssi_mod, u32 val)
 int rsnd_adg_clk_query(struct rsnd_priv *priv, unsigned int rate)
 {
 	struct rsnd_adg *adg = rsnd_priv_to_adg(priv);
-	struct clk *clk;
 	int i;
 	int sel_table[] = {
 		[CLKA] = 0x1,
@@ -303,10 +302,9 @@ int rsnd_adg_clk_query(struct rsnd_priv *priv, unsigned int rate)
 	 * find suitable clock from
 	 * AUDIO_CLKA/AUDIO_CLKB/AUDIO_CLKC/AUDIO_CLKI.
 	 */
-	for_each_rsnd_clk(clk, adg, i) {
+	for (i = 0; i < CLKMAX; i++)
 		if (rate == adg->clk_rate[i])
 			return sel_table[i];
-	}
 
 	/*
 	 * find divided clock from BRGA/BRGB
-- 
2.25.1

[PATCH 3/3] ASoC: rsnd: add null CLOCKIN support

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Some Renesas SoC doesn't have full CLOCKIN.
This patch add null_clk, and accepts it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/soc/sh/rcar/adg.c | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/sound/soc/sh/rcar/adg.c b/sound/soc/sh/rcar/adg.c
index a0b5bd5a7464..134549b16e0a 100644
--- a/sound/soc/sh/rcar/adg.c
+++ b/sound/soc/sh/rcar/adg.c
@@ -3,8 +3,8 @@
 // Helper routines for R-Car sound ADG.
 //
 //  Copyright (C) 2013  Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
-
 #include <linux/clk-provider.h>
+#include <linux/clkdev.h>
 #include "rsnd.h"
 
 #define CLKA	0
@@ -389,6 +389,30 @@ void rsnd_adg_clk_control(struct rsnd_priv *priv, int enable)
 	}
 }
 
+#define NULL_CLK "rsnd_adg_null"
+static struct clk *rsnd_adg_null_clk_get(struct rsnd_priv *priv)
+{
+	static struct clk_hw *hw;
+	struct device *dev = rsnd_priv_to_dev(priv);
+
+	if (!hw) {
+		struct clk_hw *_hw;
+		int ret;
+
+		_hw = clk_hw_register_fixed_rate_with_accuracy(dev, NULL_CLK, NULL, 0, 0, 0);
+		if (IS_ERR(_hw))
+			return NULL;
+
+		ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_simple_get, _hw);
+		if (ret < 0)
+			clk_hw_unregister_fixed_rate(_hw);
+
+		hw = _hw;
+	}
+
+	return clk_hw_get_clk(hw, NULL_CLK);
+}
+
 static void rsnd_adg_get_clkin(struct rsnd_priv *priv,
 			       struct rsnd_adg *adg)
 {
@@ -398,7 +422,12 @@ static void rsnd_adg_get_clkin(struct rsnd_priv *priv,
 	for (i = 0; i < CLKMAX; i++) {
 		struct clk *clk = devm_clk_get(dev, clk_name[i]);
 
-		adg->clk[i] = IS_ERR(clk) ? NULL : clk;
+		if (IS_ERR(clk))
+			clk = rsnd_adg_null_clk_get(priv);
+		if (IS_ERR(clk))
+			dev_err(dev, "no adg clock (%s)\n", clk_name[i]);
+
+		adg->clk[i] = clk;
 	}
 }
 
-- 
2.25.1

Re: [PATCH 0/3] ASoC: rsnd: add D3 support

[Mark Brown]

On 24 May 2021 15:11:29 +0900, Kuninori Morimoto wrote:
> These adds R-Car D3 support for rsnd driver.
> [1/3] is tidyup patch for dt-bindings (not only for D3).
> [2/3], [3/3] are for R-Car D3.
> 
> Kuninori Morimoto (3):
>   ASoC: dt-bindings: renesas: rsnd: tidyup properties
>   ASoC: rsnd: tidyup loop on rsnd_adg_clk_query()
>   ASoC: rsnd: add null CLOCKIN support
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/3] ASoC: dt-bindings: renesas: rsnd: tidyup properties
      commit: 17c2d247ddd231199e682b0a7fda42fe46c2c07b
[2/3] ASoC: rsnd: tidyup loop on rsnd_adg_clk_query()
      commit: cf9d5c6619fadfc41cf8f5154cb990cc38e3da85
[3/3] ASoC: rsnd: add null CLOCKIN support
      commit: d6956a7dde6fbf843da117f8b69cc512101fdea2

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

Re: [PATCH 3/3] ASoC: rsnd: add null CLOCKIN support

[Geert Uytterhoeven]

Hi Morimoto-san,

On Mon, May 24, 2021 at 8:12 AM Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>
> Some Renesas SoC doesn't have full CLOCKIN.
> This patch add null_clk, and accepts it.
>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Thanks for your patch, which is now commit d6956a7dde6fbf84 ("ASoC:
rsnd: add null CLOCKIN support") in asoc/for-next.

]> --- a/sound/soc/sh/rcar/adg.c
> +++ b/sound/soc/sh/rcar/adg.c
> @@ -389,6 +389,30 @@ void rsnd_adg_clk_control(struct rsnd_priv *priv, int enable)
>         }
>  }
>
> +#define NULL_CLK "rsnd_adg_null"
> +static struct clk *rsnd_adg_null_clk_get(struct rsnd_priv *priv)
> +{
> +       static struct clk_hw *hw;
> +       struct device *dev = rsnd_priv_to_dev(priv);
> +
> +       if (!hw) {
> +               struct clk_hw *_hw;
> +               int ret;
> +
> +               _hw = clk_hw_register_fixed_rate_with_accuracy(dev, NULL_CLK, NULL, 0, 0, 0);
> +               if (IS_ERR(_hw))
> +                       return NULL;
> +
> +               ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_simple_get, _hw);

I'm not such a big fan of creating dummy clocks.
And what if a future SoC lacks two CLOCKIN pins? Then you'll try to
register a second dummy clock with the same name, which will fail,
presumably?

> +               if (ret < 0)
> +                       clk_hw_unregister_fixed_rate(_hw);
> +
> +               hw = _hw;
> +       }
> +
> +       return clk_hw_get_clk(hw, NULL_CLK);
> +}
> +
>  static void rsnd_adg_get_clkin(struct rsnd_priv *priv,
>                                struct rsnd_adg *adg)
>  {
> @@ -398,7 +422,12 @@ static void rsnd_adg_get_clkin(struct rsnd_priv *priv,
>         for (i = 0; i < CLKMAX; i++) {
>                 struct clk *clk = devm_clk_get(dev, clk_name[i]);
>
> -               adg->clk[i] = IS_ERR(clk) ? NULL : clk;
> +               if (IS_ERR(clk))
> +                       clk = rsnd_adg_null_clk_get(priv);

This should only be done when the clock does not exist, not in case
of other errors (e.g. -EPROBE_DEFER, which isn't handled yet)?

As devm_clk_get_optional() already checks for existence, you could use:

    struct clk *clk = devm_clk_get_optional(dev, clk_name[i]);
    if (!clk)
            clk = rsnd_adg_null_clk_get(priv);

But in light of the above (avoiding dummy clocks), it might be more
robust to make sure all code can handle adg->clk[i] = NULL?

> +               if (IS_ERR(clk))
> +                       dev_err(dev, "no adg clock (%s)\n", clk_name[i]);
> +
> +               adg->clk[i] = clk;
>         }
>  }

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 3/3] ASoC: rsnd: add null CLOCKIN support

[Kuninori Morimoto]

Hi Geert

> I'm not such a big fan of creating dummy clocks.
> And what if a future SoC lacks two CLOCKIN pins? Then you'll try to
> register a second dummy clock with the same name, which will fail,
> presumably?

I think current code will reuse same null_clk for these.

> This should only be done when the clock does not exist, not in case
> of other errors (e.g. -EPROBE_DEFER, which isn't handled yet)?
> 
> As devm_clk_get_optional() already checks for existence, you could use:
> 
>     struct clk *clk = devm_clk_get_optional(dev, clk_name[i]);
>     if (!clk)
>             clk = rsnd_adg_null_clk_get(priv);

Ah, indeed.
Thanks. I will fix it.

> But in light of the above (avoiding dummy clocks), it might be more
> robust to make sure all code can handle adg->clk[i] = NULL?

The reason why I don't use adg->clk[i] = NULL is it is using this macro

	#define for_each_rsnd_clk(pos, adg, i)		\
		for (i = 0;				\
		     (i < CLKMAX) &&			\
		     ((pos) = adg->clk[i]);		\
		     i++)

The loop will stop at (A) if it was

	adg->clk[0] = audio_clk_a;
	adg->clk[1] = audio_clk_b;
(A)	adg->clk[2] = NULL
	adg->clk[3] = audio_clk_i;

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [PATCH 3/3] ASoC: rsnd: add null CLOCKIN support

[Geert Uytterhoeven]

Hi Morimoto-san,

On Wed, May 26, 2021 at 12:48 AM Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
> > I'm not such a big fan of creating dummy clocks.
> > And what if a future SoC lacks two CLOCKIN pins? Then you'll try to
> > register a second dummy clock with the same name, which will fail,
> > presumably?
>
> I think current code will reuse same null_clk for these.

Oh right, I missed the static clk_hw pointer.
What if you unload the snd-soc-rcar.ko module?

> > This should only be done when the clock does not exist, not in case
> > of other errors (e.g. -EPROBE_DEFER, which isn't handled yet)?
> >
> > As devm_clk_get_optional() already checks for existence, you could use:
> >
> >     struct clk *clk = devm_clk_get_optional(dev, clk_name[i]);
> >     if (!clk)
> >             clk = rsnd_adg_null_clk_get(priv);
>
> Ah, indeed.
> Thanks. I will fix it.
>
> > But in light of the above (avoiding dummy clocks), it might be more
> > robust to make sure all code can handle adg->clk[i] = NULL?
>
> The reason why I don't use adg->clk[i] = NULL is it is using this macro
>
>         #define for_each_rsnd_clk(pos, adg, i)          \
>                 for (i = 0;                             \
>                      (i < CLKMAX) &&                    \
>                      ((pos) = adg->clk[i]);             \
>                      i++)
>
> The loop will stop at (A) if it was
>
>         adg->clk[0] = audio_clk_a;
>         adg->clk[1] = audio_clk_b;
> (A)     adg->clk[2] = NULL
>         adg->clk[3] = audio_clk_i;

If you use this macro everywhere, that is easily handled by the
following variant:

    #define for_each_rsnd_clk(pos, adg, i)          \
            for (i = 0; (pos) = adg->clk[i], i < CLKMAX; i++)            \
                    if (pos) {                      \
                            continue;               \
                    } else

There are several existing examples of such a construct.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 3/3] ASoC: rsnd: add null CLOCKIN support

[Kuninori Morimoto]

Hi Geert

> Oh right, I missed the static clk_hw pointer.
> What if you unload the snd-soc-rcar.ko module?

Hmm.. indeed.
It needs something..
Thank you for poining it.

>     #define for_each_rsnd_clk(pos, adg, i)          \
>             for (i = 0; (pos) = adg->clk[i], i < CLKMAX; i++)            \
>                     if (pos) {                      \
>                             continue;               \
>                     } else

Wow!! I didn't know this technique.
Indeed it can use NULL pointer.

But, I want to avoid "if (pos) else" code as much as possible,
and keep simple code.
It can handle all clk case without thinking it if it has null_clk.

Why you don't want null_clk ??

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [PATCH 3/3] ASoC: rsnd: add null CLOCKIN support

[Geert Uytterhoeven]

Hi Morimoto-san,

On Thu, May 27, 2021 at 12:06 AM Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
> > Oh right, I missed the static clk_hw pointer.
> > What if you unload the snd-soc-rcar.ko module?
>
> Hmm.. indeed.
> It needs something..
> Thank you for poining it.
>
> >     #define for_each_rsnd_clk(pos, adg, i)          \
> >             for (i = 0; (pos) = adg->clk[i], i < CLKMAX; i++)            \
> >                     if (pos) {                      \
> >                             continue;               \
> >                     } else
>
> Wow!! I didn't know this technique.
> Indeed it can use NULL pointer.
>
> But, I want to avoid "if (pos) else" code as much as possible,
> and keep simple code.
> It can handle all clk case without thinking it if it has null_clk.
>
> Why you don't want null_clk ??

It adds a dummy object, which needs to be cleaned up.  Basically you
are trading a simple NULL pointer check for a zero clock rate check
deeper inside the driver, with the additional burden of needing to
take care of the dummy clock's life cycle.

Note that most clk_*() calls happily operate on a NULL pointer, and
just return success.  This includes clk_get_rate(), which returns
a zero rate.

Mark might have a different view, though, due to his experience with
dummy regulators?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 3/3] ASoC: rsnd: add null CLOCKIN support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 794 bytes --]

On Thu, May 27, 2021 at 09:30:38AM +0200, Geert Uytterhoeven wrote:

> It adds a dummy object, which needs to be cleaned up.  Basically you
> are trading a simple NULL pointer check for a zero clock rate check
> deeper inside the driver, with the additional burden of needing to
> take care of the dummy clock's life cycle.

> Note that most clk_*() calls happily operate on a NULL pointer, and
> just return success.  This includes clk_get_rate(), which returns
> a zero rate.

> Mark might have a different view, though, due to his experience with
> dummy regulators?

Not particularly TBH.  The regulator API doesn't accept NULL
pointers due to constant issues with people just ignoring errors
especially around trying to decide that devices don't need power,
it'd just make all that worse.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]