Re: [PATCH v3 4/5] crypto: caam - simplfy clock initialization

[Andrey Smirnov]

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