Re: [PATCH 4/6] clk: stm32f4: Add I2S clock

[Gabriel Fernandez]

On 11/07/2016 03:14 PM, Daniel Thompson wrote:
> On 07/11/16 13:05, gabriel.fernandez@xxxxxx wrote:
> > From: Gabriel Fernandez <gabriel.fernandez@xxxxxx>
> >
> > This patch introduces I2S clock for stm32f4 soc.
> > The I2S clock could be derived from an external clock or from pll-i2s
> >
> > Signed-off-by: Gabriel Fernandez <gabriel.fernandez@xxxxxx>
> > ---
> >  drivers/clk/clk-stm32f4.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/clk/clk-stm32f4.c b/drivers/clk/clk-stm32f4.c
> > index 5fa5d51..b7cb359 100644
> > --- a/drivers/clk/clk-stm32f4.c
> > +++ b/drivers/clk/clk-stm32f4.c
> > @@ -216,6 +216,7 @@ enum {
> >      SYSTICK, FCLK, CLK_LSI, CLK_LSE, CLK_HSE_RTC, CLK_RTC,
> >      PLL_VCO_I2S, PLL_VCO_SAI,
> >      CLK_LCD,
> > +    CLK_I2S,
>
> Sorry, this has just clicked and it applies to most of the other 
> patches, but adding things to this list effectively extends the clock 
> bindings (i.e. the list of valid "other" clocks access with a primary 
> index of 1).
>
> This list if a list of "arbitrary" constants by which DT periphericals 
> can be linked to specific clocks.
>
> So...
>
>  1)  If a clock is introduced here we should update the clock binding
>      documentations.
>
>  2)  If no peripheral can connect to the clock (because it is internal
>      to the clock gen logic and peripherals must connect to the gated
>      version) it should not be included in this enum.
>
>  3)  I failed to mention this when the four undocumented clocks
>      (LSI, LSE, HSE_RTC and RTC) were added.
>
>  4)  I *should* have added a comment explaining the above to the code.
ok i agree

> >      END_PRIMARY_CLK
> >  };
> >
> > @@ -967,6 +968,8 @@ static struct clk_hw *stm32_register_cclk(struct 
> > device *dev, const char *name,
> >
> >  static const char *sdmux_parents[2] = { "pll48", "sys" };
> >
> > +static const char *i2s_parents[2] = { "plli2s-r", NULL };
> > +
> >  struct stm32f4_clk_data {
> >      const struct stm32f4_gate_data *gates_data;
> >      const u64 *gates_map;
> > @@ -1005,7 +1008,7 @@ struct stm32f4_clk_data {
> >
> >  static void __init stm32f4_rcc_init(struct device_node *np)
> >  {
> > -    const char *hse_clk;
> > +    const char *hse_clk, *i2s_in_clk;
> >      int n;
> >      const struct of_device_id *match;
> >      const struct stm32f4_clk_data *data;
> > @@ -1038,6 +1041,7 @@ static void __init stm32f4_rcc_init(struct 
> > device_node *np)
> >      stm32f4_gate_map = data->gates_map;
> >
> >      hse_clk = of_clk_get_parent_name(np, 0);
> > +    i2s_in_clk = of_clk_get_parent_name(np, 1);
>
> Again this looks like a change to the DT bindings.
ok

> Also does the code work if i2s_in_clk is NULL or as you hoping to get 
> away with a not-backwards compatible change?
yes it works if i2s_in_clk is NULL.

BR
Gabriel

> Daniel.
>
>
> >      clk_register_fixed_rate_with_accuracy(NULL, "hsi", NULL, 0,
> >              16000000, 160000);
> > @@ -1053,6 +1057,12 @@ static void __init stm32f4_rcc_init(struct 
> > device_node *np)
> >      clks[PLL_VCO_SAI] = stm32f4_rcc_register_pll(pllsrc,
> >              &data->pll_data[2], &stm32f4_clk_lock);
> >
> > +    i2s_parents[1] = i2s_in_clk;
> > +
> > +    clks[CLK_I2S] =    clk_hw_register_mux_table(NULL, "i2s",
> > +                i2s_parents, ARRAY_SIZE(i2s_parents), 0,
> > +                base + STM32F4_RCC_CFGR, 23, 1, 0, NULL,
> > +                &stm32f4_clk_lock);
> >      sys_parents[1] = hse_clk;
> >      clk_register_mux_table(
> >          NULL, "sys", sys_parents, ARRAY_SIZE(sys_parents), 0,