Re: [PATCH 10/22] ASoC: rsnd: Add DMA support infrastructure for RZ/G3E

[Kuninori Morimoto]

Hi John

> > I think it include many features in 1 patch.
> > You should separate it into each features.
> 
> Agreed, I'll split this into separate patches. One for
> RZ/G3E DMA address support and one for audmac-pp clock/reset
> management. What do you think of this approach ?

Thank you

> > rsnd_dma_probe() is common fucntion.
> > Is above possible to keep compatible with other SoCs ?
> 
> Other SoCs do not need or specify these clock/reset in DTS and
> the fact I use optional APIs makes it compatible with these other
> SoCs. I'm wondering if you meant something else here?

OK, nice to know

> > And, we are already using "audmacpp".
> 
> What do you mean by the above ?

Naming. It is reg name but we are already using "audmapp".
Using same naming is less confuse, I think.

> Regarding rsnd_dma_probe(), I intentionally used
> devm_clk_get_optional_enabled() and
> devm_reset_control_get_optional_exclusive_deasserted() - these
> return NULL when the clock/reset is not present in the device tree,
> so they are fully transparent to existing SoCs (R-Car Gen2/3/4).
> 
> Adding per-SoC branches in rsnd_dma_probe() would duplicate the
> common DMAC setup logic. I believe keeping this in the common
> path is the cleaner approach, but I'm happy to discuss if you
> see a specific concern beyond compatibility.

It's definitely too early to remake it.

But please add /* for RZ/G3E */ etc on top of comment.
We already have /* for GenX */ comment there.

And, you added the code *before* /* for Gen4 */ comment,
but it should be *after* that ?
It is handling priv->xxx_audmac_pp, so maybe before audmapp_end: ?

before audmapp_end: it is for dmac->xxx handling part
after  audmapp_end: it is for priv->xxx handling part, etc.

And I like priv->audmac_pp_xxx instead of prix->xxx_audmac_pp for peram naming.
Because it is easy to find them by normal grep.

Thank you for your help !!

Best regards
---
Kuninori Morimoto