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

[John Madieu]

Hi Kuninori,

Thanks for the review.

> -----Original Message-----
> From: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
> Sent: Monday, March 23, 2026 1:47 AM
> To: John Madieu <john.madieu.xa@xxxxxxxxxxxxxx>
> Subject: Re: [PATCH 09/22] ASoC: rsnd: Add RZ/G3E SoC probing and register
> map
> 
> 
> 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.

Agreed. Will update in v2.

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

Indeed. I'll update accordingly  

> 
> >  	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 ?

Yes, they are being used.

> 
> > @@ -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))

I got the idea behind. I'll switch to it in v2.

Regards,
John.

> 
> 
> Thank you for your help !!
> 
> Best regards
> ---
> Kuninori Morimoto