Provide a managed devm_clk_bulk* wrapper to get and enable all
bulk clocks in order to simplify drivers that keeps all clocks
enabled for the time of driver operation.
Suggested-by: Marek Szyprowski <[email protected]>
Reviewed-by: Alim Akhtar <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
Signed-off-by: Shradha Todi <[email protected]>
---
drivers/clk/clk-devres.c | 40 ++++++++++++++++++++++++++++++++++++++++
include/linux/clk.h | 22 ++++++++++++++++++++++
2 files changed, 62 insertions(+)
diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index 4fb4fd4b06bd..cbbd2cc339c3 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -182,6 +182,46 @@ int __must_check devm_clk_bulk_get_all(struct device *dev,
}
EXPORT_SYMBOL_GPL(devm_clk_bulk_get_all);
+static void devm_clk_bulk_release_all_enable(struct device *dev, void *res)
+{
+ struct clk_bulk_devres *devres = res;
+
+ clk_bulk_disable_unprepare(devres->num_clks, devres->clks);
+ clk_bulk_put_all(devres->num_clks, devres->clks);
+}
+
+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) {
+ *clks = devres->clks;
+ devres->num_clks = ret;
+ } else {
+ devres_free(devres);
+ return ret;
+ }
+
+ 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);
+
static int devm_clk_match(struct device *dev, void *res, void *data)
{
struct clk **c = res;
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 1ef013324237..a8e6b045b848 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -438,6 +438,22 @@ int __must_check devm_clk_bulk_get_optional(struct device *dev, int num_clks,
int __must_check devm_clk_bulk_get_all(struct device *dev,
struct clk_bulk_data **clks);
+/**
+ * devm_clk_bulk_get_all_enable - Get and enable all clocks of the consumer (managed)
+ * @dev: device for clock "consumer"
+ * @clks: pointer to the clk_bulk_data table of consumer
+ *
+ * Returns success (0) or negative errno.
+ *
+ * This helper function allows drivers to get all clocks of the
+ * consumer and enables them in one operation with management.
+ * The clks will automatically be disabled and freed when the device
+ * is unbound.
+ */
+
+int __must_check devm_clk_bulk_get_all_enable(struct device *dev,
+ struct clk_bulk_data **clks);
+
/**
* devm_clk_get - lookup and obtain a managed reference to a clock producer.
* @dev: device for clock "consumer"
@@ -960,6 +976,12 @@ static inline int __must_check devm_clk_bulk_get_all(struct device *dev,
return 0;
}
+static inline int __must_check devm_clk_bulk_get_all_enable(struct device *dev,
+ struct clk_bulk_data **clks)
+{
+ return 0;
+}
+
static inline struct clk *devm_get_clk_from_child(struct device *dev,
struct device_node *np, const char *con_id)
{
--
2.17.1
Quoting Shradha Todi (2024-02-20 00:40:45)
> Provide a managed devm_clk_bulk* wrapper to get and enable all
> bulk clocks in order to simplify drivers that keeps all clocks
> enabled for the time of driver operation.
>
> Suggested-by: Marek Szyprowski <[email protected]>
> Reviewed-by: Alim Akhtar <[email protected]>
> Reviewed-by: Manivannan Sadhasivam <[email protected]>
> Signed-off-by: Shradha Todi <[email protected]>
> ---
Applied to clk-next
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.
> + *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.
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);
> +
> -----Original Message-----
> From: Dan Carpenter <[email protected]>
> Sent: 05 March 2024 14:20
> To: Shradha Todi <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected]; linux-samsung-
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]
> 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);
> > +
Quoting Shradha Todi (2024-03-06 04:13:03)
> >
> > 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.
Maybe we shouldn't even return the clks to the caller. Do you have any
use for the clk pointers?
> -----Original Message-----
> From: Stephen Boyd <[email protected]>
> Sent: 09 March 2024 06:21
> To: 'Dan Carpenter' <[email protected]>; Shradha Todi
> <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected]; linux-samsung-
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]
> Subject: RE: [PATCH v6 1/2] clk: Provide managed helper to get and enable bulk
> clocks
>
> Quoting Shradha Todi (2024-03-06 04:13:03)
> > >
> > > 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.
>
> Maybe we shouldn't even return the clks to the caller. Do you have any use for
> the clk pointers?
The intention to return the clk pointers was in the case where caller wants to
manipulate a particular clock in certain conditions. They can obtain the clock pointer
and use clk_set_parent, clk_set_rate on those particular clocks.
But I understand that in that case users can use existing clk_bulk_get_all() API.
So, should I go ahead and send v7?
Quoting Shradha Todi (2024-03-15 04:34:44)
> >
> > Quoting Shradha Todi (2024-03-06 04:13:03)
> > > >
> > > > 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.
> >
> > Maybe we shouldn't even return the clks to the caller. Do you have any use for
> > the clk pointers?
>
> The intention to return the clk pointers was in the case where caller wants to
> manipulate a particular clock in certain conditions. They can obtain the clock pointer
> and use clk_set_parent, clk_set_rate on those particular clocks.
> But I understand that in that case users can use existing clk_bulk_get_all() API.
> So, should I go ahead and send v7?
>
No, I think this is fine.