Received-SPF: pass (google.com: best guess record for domain of linux-crypto-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190617160339.29179-1-andrew.smirnov@gmail.com>
 <20190617160339.29179-5-andrew.smirnov@gmail.com> <VI1PR04MB50556DE0057E1B5802E0DAA1EEEB0@VI1PR04MB5055.eurprd04.prod.outlook.com>
In-Reply-To: <VI1PR04MB50556DE0057E1B5802E0DAA1EEEB0@VI1PR04MB5055.eurprd04.prod.outlook.com>
From:   Andrey Smirnov <andrew.smirnov@gmail.com>
Date:   Mon, 17 Jun 2019 17:21:01 -0700
Message-ID: <CAHQ1cqFLhj0N+qKFTYDjmCzAm5CaDeuvMM4ZWN+9CQNr0orXTA@mail.gmail.com>
Subject: Re: [PATCH v3 4/5] crypto: caam - simplfy clock initialization
To:     Leonard Crestez <leonard.crestez@nxp.com>
Cc:     "linux-crypto@vger.kernel.org" <linux-crypto@vger.kernel.org>,
        Horia Geanta <horia.geanta@nxp.com>,
        Chris Spencer <christopher.spencer@sea.co.uk>,
        Cory Tusar <cory.tusar@zii.aero>,
        Chris Healy <cphealy@gmail.com>,
        Lucas Stach <l.stach@pengutronix.de>,
        Aymen Sghaier <aymen.sghaier@nxp.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-crypto-owner@vger.kernel.org
Precedence: bulk

On Mon, Jun 17, 2019 at 12:48 PM Leonard Crestez
<leonard.crestez@nxp.com> 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