Re: [PATCH] clk, clk-si5341: Support multiple input ports

[Stephen Boyd <sboyd@kernel.org>]

Quoting Mike Looijmans (2019-12-05 03:57:34)
> The Si5341 and Si5340 have multiple input clock options. So far, the driver
> only supported the XTAL input, this adds support for the three external
> clock inputs as well.
> 
> If the clock chip is't programmed at boot, the driver will default to the

isn't

> XTAL input as before. If there is no "xtal" clock input available, it will
> pick the first connected input (e.g. "in0") as the input clock for the PLL.
> One can use clock-assigned-parents to select a particular clock as input.
> 
> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>

> diff --git a/drivers/clk/clk-si5341.c b/drivers/clk/clk-si5341.c
> index 6e780c2a9e6b..f7dba7698083 100644
> --- a/drivers/clk/clk-si5341.c
> +++ b/drivers/clk/clk-si5341.c
> @@ -4,7 +4,6 @@
>   * Copyright (C) 2019 Topic Embedded Products
>   * Author: Mike Looijmans <mike.looijmans@topic.nl>
>   */
> -
>  #include <linux/clk.h>
>  #include <linux/clk-provider.h>
>  #include <linux/delay.h>

I think we can do without this hunk.

> @@ -390,7 +410,112 @@ static unsigned long si5341_clk_recalc_rate(struct clk_hw *hw,
>         return (unsigned long)res;
>  }
>  
> +static int si5341_clk_get_selected_input(struct clk_si5341 *data)
> +{
> +       int err;
> +       u32 val;
> +
> +       err = regmap_read(data->regmap, SI5341_IN_SEL, &val);
> +       if (err < 0)
> +               return err;
> +
> +       return (val & SI5341_IN_SEL_MASK) >> SI5341_IN_SEL_SHIFT;
> +}
> +
> +static unsigned char si5341_clk_get_parent(struct clk_hw *hw)

Please use u8 for now.

> +{
> +       struct clk_si5341 *data = to_clk_si5341(hw);
> +       int res = si5341_clk_get_selected_input(data);
> +
> +       if (res < 0)
> +               return 0; /* Apparently we cannot report errors */

For now this is the case. I'll rekick the series to convert this API to
a function that returns clk_hw pointers.
 
> +
> +       return res;
> +}
> +
[...]
> @@ -985,7 +1110,8 @@ static const struct regmap_range si5341_regmap_volatile_range[] = {
>         regmap_reg_range(0x000C, 0x0012), /* Status */
>         regmap_reg_range(0x001C, 0x001E), /* reset, finc/fdec */
>         regmap_reg_range(0x00E2, 0x00FE), /* NVM, interrupts, device ready */
> -       /* Update bits for synth config */
> +       /* Update bits for P divider and synth config */
> +       regmap_reg_range(SI5341_PX_UPD, SI5341_PX_UPD),
>         regmap_reg_range(SI5341_SYNTH_N_UPD(0), SI5341_SYNTH_N_UPD(0)),
>         regmap_reg_range(SI5341_SYNTH_N_UPD(1), SI5341_SYNTH_N_UPD(1)),
>         regmap_reg_range(SI5341_SYNTH_N_UPD(2), SI5341_SYNTH_N_UPD(2)),
> @@ -1122,6 +1248,7 @@ static int si5341_initialize_pll(struct clk_si5341 *data)
>         struct device_node *np = data->i2c_client->dev.of_node;
>         u32 m_num = 0;
>         u32 m_den = 0;
> +       int sel;
>  
>         if (of_property_read_u32(np, "silabs,pll-m-num", &m_num)) {
>                 dev_err(&data->i2c_client->dev,
> @@ -1135,7 +1262,11 @@ static int si5341_initialize_pll(struct clk_si5341 *data)
>         if (!m_num || !m_den) {
>                 dev_err(&data->i2c_client->dev,
>                         "PLL configuration invalid, assume 14GHz\n");
> -               m_den = clk_get_rate(data->pxtal) / 10;
> +               sel = si5341_clk_get_selected_input(data);
> +               if (sel < 0)
> +                       return sel;
> +
> +               m_den = clk_get_rate(data->input_clk[sel]) / 10;
>                 m_num = 1400000000;
>         }
>  
> @@ -1143,11 +1274,52 @@ static int si5341_initialize_pll(struct clk_si5341 *data)
>                         SI5341_PLL_M_NUM, m_num, m_den);
>  }
>  
> +static int si5341_clk_select_active_input(struct clk_si5341 *data)
> +{
> +       int res;
> +       int err;
> +       int i;
> +
> +       res = si5341_clk_get_selected_input(data);
> +       if (res < 0)
> +               return res;
> +
> +       /* If the current register setting is invalid, pick the first input */
> +       if (!data->input_clk[res]) {
> +               dev_dbg(&data->i2c_client->dev,
> +                       "Input %d not connected, rerouting\n", res);
> +               res = -ENODEV;
> +               for (i = 0; i < SI5341_NUM_INPUTS; ++i) {
> +                       if (data->input_clk[i]) {
> +                               res = i;
> +                               break;
> +                       }
> +               }
> +               if (res < 0) {

What if res is == SI5341_NUM_INPUTS?

> +                       dev_err(&data->i2c_client->dev,
> +                               "No clock input available\n");
> +                       return res;
> +               }
> +       }
> +
> +       /* Make sure the selected clock is also enabled and routed */
> +       err = si5341_clk_reparent(data, res);
> +       if (err < 0)
> +               return err;
> +
> +       err = clk_prepare_enable(data->input_clk[res]);

Is it possible to do this setup and configuration stuff when the clk is
prepared by something? Maybe I've asked this before but I'd prefer that
this driver is a clk provider and not a clk consumer. If some default
setup needs to be done, preferably do that via direct register writes or
by calling the clk_ops functions directly instead of going through the
framework, preferably before registering the clks to the framework.

> +       if (err < 0)
> +               return err;
> +
> +       return res;
> +}
> +