Re: [PATCH 09/22] ASoC: rsnd: Add RZ/G3E SoC probing and register map

[Kuninori Morimoto]

Hi John

Thank you for the patch

> RZ/G3E audio subsystem has a different register layout compared to
> R-Car Gen2/Gen3/Gen4, as described below:
> 
> - Different base address organization (SCU, ADG, SSIU, SSI as
>   separate regions accessed by name)
> - Additional registers: AUDIO_CLK_SEL3, SSI_MODE3, SSI_CONTROL2
> - Different register offsets within each region
> 
> Add RZ/G3E SoC's audio subsystem register layouts and probe support.
> 
> Signed-off-by: John Madieu <john.madieu.xa@xxxxxxxxxxxxxx>
> ---
(snip)
> +	static const struct rsnd_regmap_field_conf conf_adg[] = {
> +		RSND_GEN_S_REG(BRRA,			0x00),
> +		RSND_GEN_S_REG(BRRB,			0x04),
> +		RSND_GEN_S_REG(BRGCKR,			0x08),
> +		RSND_GEN_S_REG(AUDIO_CLK_SEL0,		0x0c),
> +		RSND_GEN_S_REG(AUDIO_CLK_SEL1,		0x10),
> +		RSND_GEN_S_REG(AUDIO_CLK_SEL2,		0x14),
> +		RSND_GEN_S_REG(AUDIO_CLK_SEL3,		0x18),
> +		RSND_GEN_S_REG(DIV_EN,			0x30),
> +		RSND_GEN_S_REG(SRCIN_TIMSEL0,		0x34),
> +		RSND_GEN_S_REG(SRCIN_TIMSEL1,		0x38),
> +		RSND_GEN_S_REG(SRCIN_TIMSEL2,		0x3c),
> +		RSND_GEN_S_REG(SRCIN_TIMSEL3,		0x40),
> +		RSND_GEN_S_REG(SRCIN_TIMSEL4,		0x44),
> +		RSND_GEN_S_REG(SRCOUT_TIMSEL0,		0x48),
> +		RSND_GEN_S_REG(SRCOUT_TIMSEL1,		0x4c),
> +		RSND_GEN_S_REG(SRCOUT_TIMSEL2,		0x50),
> +		RSND_GEN_S_REG(SRCOUT_TIMSEL3,		0x54),
> +		RSND_GEN_S_REG(SRCOUT_TIMSEL4,		0x58),
> +		RSND_GEN_S_REG(CMDOUT_TIMSEL,		0x5c),
(snip)
> +	ret = rsnd_gen_regmap_init(priv, 10, RSND_RZG3E_ADG,
> +				   "adg", conf_adg);

I don't think you need 10 ADG.

And it can be 1 line here.

> --- a/sound/soc/renesas/rcar/rsnd.h
> +++ b/sound/soc/renesas/rcar/rsnd.h
> @@ -26,6 +26,11 @@
>  #define RSND_BASE_SSIU	2
>  #define RSND_BASE_SCU	3	// for Gen2/Gen3
>  #define RSND_BASE_SDMC	3	// for Gen4	reuse
> +
> +#define RSND_RZG3E_SCU		0
> +#define RSND_RZG3E_ADG		1
> +#define RSND_RZG3E_SSIU		2
> +#define RSND_RZG3E_SSI		3
>  #define RSND_BASE_MAX	4

You can reuse existing RSND_BASE_xxx

>  	AUDIO_CLK_SEL2,
> +	AUDIO_CLK_SEL3,
>  
>  	/* SSIU */
>  	SSI_MODE,
>  	SSI_MODE0,
>  	SSI_MODE1,
>  	SSI_MODE2,
> +	SSI_MODE3,
>  	SSI_CONTROL,
> +	SSI_CONTROL2,
>  	SSI_CTRL,
>  	SSI_BUSIF0_MODE,
>  	SSI_BUSIF1_MODE,

Do you really use these reg ?

> @@ -627,6 +635,7 @@ struct rsnd_priv {
>  #define RSND_GEN2	(2 << 0)
>  #define RSND_GEN3	(3 << 0)
>  #define RSND_GEN4	(4 << 0)
> +#define RSND_RZG3E	(5 << 0)
>  #define RSND_SOC_MASK	(0xFF << 4)
>  #define RSND_SOC_E	(1 << 4) /* E1/E2/E3 */
(snip)
> +#define rsnd_is_rzg3e(priv)	(((priv)->flags & RSND_GEN_MASK) == RSND_RZG3E)

(5 << 0) will be used for Gen5.
There is not detail rule yet, but I think we want to keep (x << 0) and
(x << 4) for R-Car.

Maybe you can use (xx << 8) and (xx << 12) for RZ ?
Something like this

	#define RSND_RZ_MASK	(0xFF << 8)
	#define RSND_RZ1	(1 << 8)
	#define RSND_RZ2	(2 << 8)
	#define RSND_RZ3	(3 << 8)

	#define RSND_RZG3E	(1 << 12)

	#define rsnd_is_rzg3e(priv)	(((priv)->flags & RSND_RZ_MASK) == (RSND_RZ3 | RSND_RZG3E))


Thank you for your help !!

Best regards
---
Kuninori Morimoto