On Mon, Jun 17, 2019 at 12:48 PM Leonard Crestez
<leonard.crestez@xxxxxxx> wrote:
>
> On 6/17/2019 7:04 PM, Andrey Smirnov wrote:
> > Simplify clock initialization code by converting it to use clk-bulk,
> > devres and soc_device_match() match table. No functional change
> > intended.
>
> Subject is misspelled.
>
Will fix.
> > +struct clk_bulk_caam {
> > + const struct clk_bulk_data *clks;
> > + int num_clks;
> > +};
>
> clks could be an array[0] at the end to avoid an additional allocation.
>
struct clk_bulk_caam is also used to declare caam_imx*_clk_data
variables, where this approach wouldn't work.
> > +static void disable_clocks(void *private)
> > +{
> > + struct clk_bulk_caam *context = private;
> > +
> > + clk_bulk_disable_unprepare(context->num_clks,
> > + (struct clk_bulk_data *)context->clks);
> > +}
>
> Not sure using devm for this is worthwhile. Maybe someday CAAM clks will
> be enabled dynamically?
>
I don't see a reason why this code can't be changed _if_ and when that
ever happens.
> It would be make sense to reference "clk" instead of "clocks".
>
I am not sure what you reference here. I'd rather we avoid discussing
trivial spelling aspects like this.
> > +static int init_clocks(struct device *dev,
> > + const struct clk_bulk_caam *data)
> > +{
> > + struct clk_bulk_data *clks;
> > + struct clk_bulk_caam *context;
> > + int num_clks;
> > + int ret;
> > +
> > + num_clks = data->num_clks;
> > + clks = devm_kmemdup(dev, data->clks,
> > + data->num_clks * sizeof(data->clks[0]),
> > + GFP_KERNEL);
> > + if (!clks)
> > + return -ENOMEM;
> > +
> > + ret = devm_clk_bulk_get(dev, num_clks, clks);
> > + if (ret) {
> > + dev_err(dev,
> > + "Failed to request all necessary clocks\n");
> > + return ret;
> > + }
> > +
> > + ret = clk_bulk_prepare_enable(num_clks, clks);
> > + if (ret) {
> > + dev_err(dev,
> > + "Failed to prepare/enable all necessary clocks\n");
> > + return ret;
> > + }
> > +
> > + context = devm_kzalloc(dev, sizeof(*context), GFP_KERNEL);
> > + if (!context)
> > + return -ENOMEM;
>
> Aren't clks left enabled if this fails? Can move this allocation higher.
>
Good point, will do.
> > + context->num_clks = num_clks;
> > + context->clks = clks;
> > +
> > + ret = devm_add_action_or_reset(dev, disable_clocks, context);
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
> > +}
>
> > static int caam_probe(struct platform_device *pdev)
> > {
> > int ret, ring, gen_sk, ent_delay = RTSDCTL_ENT_DLY_MIN;
> > u64 caam_id;
> > - static const struct soc_device_attribute imx_soc[] = {
> > - {.family = "Freescale i.MX"},
> > - {},
> > - };
> > + const struct soc_device_attribute *soc_attr;
>
> This "soc_attr" is difficult to understand, maybe rename to something
> like "imx_soc_match"?
Sure, will do in next version.
Thanks,
Andrey Smirnov
