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
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(-)
From: Mark Brown <broonie@kernel.org> Date: 2021-05-24 12:00:49
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
Hi Morimoto-san,
On Mon, May 24, 2021 at 8:12 AM Kuninori Morimoto
[off-list ref] 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
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?
@@ -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
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
Hi Morimoto-san,
On Wed, May 26, 2021 at 12:48 AM Kuninori Morimoto
[off-list ref] wrote:
quoted
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?
quoted
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.
quoted
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
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
Hi Morimoto-san,
On Thu, May 27, 2021 at 12:06 AM Kuninori Morimoto
[off-list ref] wrote:
quoted
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.
quoted
#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
From: Mark Brown <broonie@kernel.org> Date: 2021-05-27 09:49:15
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.