Re: [PATCH v4 07/11] ASoC: jz4740-i2s: Make the PLL clock name SoC-specific

[Paul Cercueil]

Hi Aidan,

Le sam. 22 oct. 2022 à 18:15:05 +0100, Aidan MacDonald 
<aidanmacdonald.0x0@xxxxxxxxx> a écrit :
> Zhou Yanjie <zhouyu@xxxxxxxxxxxxxx> writes:
>
> >  Hi Paul,
> >
> >  On 2022/7/13 下午11:07, Paul Cercueil wrote:
> > >  Hi Zhou,
> > >
> > >  Le mer., juil. 13 2022 at 22:33:44 +0800, Zhou Yanjie 
> > > <zhouyu@xxxxxxxxxxxxxx>
> > >  a écrit :
> > > >  Hi Aidan,
> > > >
> > > >  On 2022/7/9 上午12:02, Aidan MacDonald wrote:
> > > > >  @@ -400,6 +402,7 @@ static const struct i2s_soc_info 
> > > > > jz4740_i2s_soc_info =
> > > > >  {
> > > > >        .field_tx_fifo_thresh    = REG_FIELD(JZ_REG_AIC_CONF, 8, 
> > > > > 11),
> > > > >        .field_i2sdiv_capture    = REG_FIELD(JZ_REG_AIC_CLK_DIV, 
> > > > > 0, 3),
> > > > >        .field_i2sdiv_playback    = REG_FIELD(JZ_REG_AIC_CLK_DIV, 
> > > > > 0, 3),
> > > > >  +    .pll_clk_name        = "pll half",
> > > > >        .shared_fifo_flush    = true,
> > > > >    };
> > > >
> > > >
> > > >  Since JZ4760, according to the description of the I2SCDR register,
> > > >  Ingenic SoCs no longer use PLL/2 clock, but directly use PLL 
> > > > clock,
> > > >  so it seems also inappropriate to use "pll half" for these SoCs.
> > >
> > >  The device tree passes the clock as "pll half". So the driver 
> > > should use this
> > >  name as well...
> >
> >
> >  I see...
> >
> >  It seems that the device tree of JZ4770 has used "pll half" already,
> >  but there is no "pll half" used anywhere in the device tree of 
> > JZ4780,
> >  maybe we can keep the pll_clk_name of JZ4770 as "pll half", and 
> > change
> >  the pll_clk_name of JZ4780 to a more reasonable name.
> >
> >
> >  Thanks and best regards!
>
> Actually, the clock names in the DT are meaningless. The clk_get() 
> call
> matches only the clock's name in the CGU driver. So in fact the driver
> is "broken" for jz4780. It seems jz4770 doesn't work correctly either,
> it has no "pll half", and three possible parents for its "i2s" clock.

That's not true. The clock names are matched via DT.

Only in the case where a corresponding clock cannot be found via DT 
will it search for the clock name among the clock providers. I believe 
this is a legacy mechanism and you absolutely shouldn't rely on it.

-Paul

> Since the driver only supports the internal codec, which requires the
> "ext" clock, there isn't a problem in practice.
>
> I'm just going to drop this patch and leave .set_sysclk() alone for 
> now.
> I think a better approach is to have the DT define an array of parent
> clocks for .set_sysclk()'s use, instead of hardcoding parents in the
> driver. If the parent array is missing the driver can default to using
> "ext" so existing DTs will work.
>
> Regards,
> Aidan