Re: [PATCH V4 08/11] clk: imx: imx8qxp-lpcg: add parsing clocks from device tree

[Stephen Boyd <sboyd@kernel.org>]

Quoting Dong Aisheng (2019-08-20 04:13:22)
> Add parsing clocks from device tree.

Please describe some more here.

> diff --git a/drivers/clk/imx/clk-imx8qxp-lpcg.c b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> index c0aff7c..90326e5 100644
> --- a/drivers/clk/imx/clk-imx8qxp-lpcg.c
> +++ b/drivers/clk/imx/clk-imx8qxp-lpcg.c
> @@ -157,6 +158,101 @@ static const struct imx8qxp_ss_lpcg imx8qxp_ss_lsio = {
>         .num_max = IMX_LSIO_LPCG_CLK_END,
>  };
>  
> +#define IMX_LPCG_MAX_CLKS      8
> +
> +static struct clk_hw *imx_lpcg_of_clk_src_get(struct of_phandle_args *clkspec,
> +                                             void *data)
> +{
> +       struct clk_hw_onecell_data *hw_data = data;
> +       unsigned int idx = clkspec->args[0] / 4;
> +
> +       if (idx >= hw_data->num) {
> +               pr_err("%s: invalid index %u\n", __func__, idx);
> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       return hw_data->hws[idx];
> +}
> +
> +static int imx_lpcg_parse_clks_from_dt(struct platform_device *pdev,
> +                                      struct device_node *np)
> +{
> +       const char *output_names[IMX_LPCG_MAX_CLKS];
> +       const char *parent_names[IMX_LPCG_MAX_CLKS];
> +       unsigned int bit_offset[IMX_LPCG_MAX_CLKS];
> +       struct clk_hw_onecell_data *clk_data;
> +       struct clk_hw **clk_hws;
> +       struct resource *res;
> +       void __iomem *base;
> +       int count;
> +       int idx;
> +       int ret;
> +       int i;
> +
> +       if (!of_device_is_compatible(np, "fsl,imx8qxp-lpcg"))
> +               return -EINVAL;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       base = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(base))
> +               return PTR_ERR(base);
> +
> +       count = of_property_count_u32_elems(np, "clock-indices");
> +       if (count < 0) {
> +               dev_err(&pdev->dev, "failed to count clocks\n");
> +               return -EINVAL;
> +       }

Is 'count' expected to be equal to IMX_LPCG_MAX_CLKS? Because later on
in this function we set the num of clks to the MAX instead of the count
from clock-indices.

> +
> +       clk_data = devm_kzalloc(&pdev->dev, struct_size(clk_data, hws, IMX_LPCG_MAX_CLKS),

This line is too long.

> +                               GFP_KERNEL);
> +       if (!clk_data)
> +               return -ENOMEM;
> +
> +       clk_data->num = IMX_LPCG_MAX_CLKS;
> +       clk_hws = clk_data->hws;
> +
> +       ret = of_property_read_u32_array(np, "clock-indices", bit_offset,
> +                                        count);
> +       if (ret < 0) {
> +               dev_err(&pdev->dev, "failed to read clocks bit-offset\n");

This isn't called bit-offset anymore.

> +               return -EINVAL;
> +       }
> +
> +       ret = of_clk_parent_fill(np, parent_names, count);
> +       if (ret != count) {
> +               dev_err(&pdev->dev, "failed to get clock parent names\n");
> +               return -EINVAL;

return count?

> +       }
> +
> +       ret = of_property_read_string_array(np, "clock-output-names",
> +                                           output_names, count);
> +       if (ret != count) {
> +               dev_err(&pdev->dev, "failed to read clock-output-names\n");
> +               return -EINVAL;
> +       }
> +
> +       for (i = 0; i < count; i++) {
> +               idx = bit_offset[i] / 4;
> +               if (idx > IMX_LPCG_MAX_CLKS) {
> +                       dev_warn(&pdev->dev, "invalid bit offset of clock %d\n",
> +                                i);
> +                       return -EINVAL;
> +               }
> +
> +               clk_hws[idx] = imx_clk_lpcg_scu(output_names[i],
> +                                               parent_names[i], 0, base,
> +                                               bit_offset[i], false);
> +               if (IS_ERR(clk_hws[idx])) {
> +                       dev_warn(&pdev->dev, "failed to register clock %d\n",
> +                                idx);
> +                       return -EINVAL;
> +               }
> +       }
> +
> +       return devm_of_clk_add_hw_provider(&pdev->dev, imx_lpcg_of_clk_src_get,
> +                                          clk_data);

If this fails does imx_clk_lpcg_scu() need to be unwound and unregister
clks?

> +}
> +