[PATCH v6 1/2] clk: Provide managed helper to get and enable bulk clocks
Shradha Todi
shradha.t at samsung.com
Wed Mar 6 04:13:03 PST 2024
> -----Original Message-----
> From: Dan Carpenter <dan.carpenter at linaro.org>
> Sent: 05 March 2024 14:20
> To: Shradha Todi <shradha.t at samsung.com>
> Cc: linux-clk at vger.kernel.org; linux-kernel at vger.kernel.org; linux-
> pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-samsung-
> soc at vger.kernel.org; mturquette at baylibre.com; sboyd at kernel.org;
> jingoohan1 at gmail.com; lpieralisi at kernel.org; kw at linux.com; robh at kernel.org;
> bhelgaas at google.com; krzysztof.kozlowski at linaro.org;
> alim.akhtar at samsung.com; linux at armlinux.org.uk;
> m.szyprowski at samsung.com; manivannan.sadhasivam at linaro.org;
> pankaj.dubey at samsung.com; gost.dev at samsung.com
> Subject: Re: [PATCH v6 1/2] clk: Provide managed helper to get and enable bulk
> clocks
>
> On Tue, Feb 20, 2024 at 02:10:45PM +0530, Shradha Todi wrote:
> > +int __must_check devm_clk_bulk_get_all_enable(struct device *dev,
> > + struct clk_bulk_data **clks) {
> > + struct clk_bulk_devres *devres;
> > + int ret;
> > +
> > + devres = devres_alloc(devm_clk_bulk_release_all_enable,
> > + sizeof(*devres), GFP_KERNEL);
> > + if (!devres)
> > + return -ENOMEM;
> > +
> > + ret = clk_bulk_get_all(dev, &devres->clks);
> > + if (ret > 0) {
>
> I feel like this should be >= instead of >. There aren't any callers of this
function
> yet so we can't see what's in *clks at the start but it's easy to imagine a
situation
> where it's bad data.
>
Reference for this piece of code has been taken from devm_clk_bulk_get_all()
which
has multiple callers, so it's safe. If we make this >=, it will hold on to the
devres node
even though there are no clocks.
> > + *clks = devres->clks;
> > + devres->num_clks = ret;
> > + } else {
> > + devres_free(devres);
> > + return ret;
>
> When clk_bulk_get_all() returns zero then we return success here.
>
Yes, we are returning success in case there are no clocks as well. In case there
are no
clocks defined in the DT-node, then it is assumed that the driver does not need
any
clock manipulation for driver operation. So the intention here is to continue
without
throwing error.
> regards,
> dan carpenter
>
> > + }
> > +
> > + ret = clk_bulk_prepare_enable(devres->num_clks, *clks);
> > + if (!ret) {
> > + devres_add(dev, devres);
> > + } else {
> > + clk_bulk_put_all(devres->num_clks, devres->clks);
> > + devres_free(devres);
> > + }
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(devm_clk_bulk_get_all_enable);
> > +
More information about the linux-arm-kernel
mailing list