Re: [PATCH 1/8] clk: Add clk_bulk_alloc functions

2018-03-15 Thread Stephen Boyd 
Quoting Marek Szyprowski (2018-02-20 01:36:03)
> Hi Robin,
> 
> On 2018-02-19 17:29, Robin Murphy wrote:
> >
> > Seeing how every subsequent patch ends up with the roughly this same 
> > stanza:
> >
> > x = devm_clk_bulk_alloc(dev, num, names);
> > if (IS_ERR(x)
> >     return PTR_ERR(x);
> > ret = devm_clk_bulk_get(dev, x, num);
> >
> > I wonder if it might be better to simply implement:
> >
> > int devm_clk_bulk_alloc_get(dev, , num, names)
> >
> > that does the whole lot in one go, and let drivers that want to do 
> > more fiddly things continue to open-code the allocation.
> >
> > But perhaps that's an abstraction too far... I'm not all that familiar 
> > with the lie of the land here.
> 
> Hmmm. This patchset clearly shows, that it would be much simpler if we
> get rid of clk_bulk_data structure at all and let clk_bulk_* functions
> to operate on struct clk *array[]. Typically the list of clock names
> is already in some kind of array (taken from driver data or statically
> embedded into driver), so there is little benefit from duplicating it
> in clk_bulk_data. Sadly, I missed clk_bulk_* api discussion and maybe
> there are other benefits from this approach.
> 
> If not, I suggest simplifying clk_bulk_* api by dropping clk_bulk_data
> structure and switching to clock ptr array:
> 
> int clk_bulk_get(struct device *dev, int num_clock, struct clk *clocks[],
>               const char *clk_names[]);
> int clk_bulk_prepare(int num_clks, struct clk *clks[]);
> int clk_bulk_enable(int num_clks, struct clk *clks[]);
> ...
> 

If you have an array of pointers to names of clks then we can put the
struct clk pointers adjacent to them at the same time. I suppose the
problem there is that some drivers want to mark that array of pointers
to names as const. And then we can't update the clk pointers next to
them.

This is the same design that regulators has done so that's why it's
written like this for clks. Honestly, we're talking about a handful of
pointers here so I'm not sure it really matters much. Just duplicate the
pointer and be done if you want to mark the array of names as const or
have your const 'setup' structure point to the bulk_data array that you
define statically non-const somewhere globally.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/8] clk: Add clk_bulk_alloc functions

2018-02-20 Thread Robin Murphy 

Hi Marek,

On 20/02/18 09:36, Marek Szyprowski wrote:

Hi Robin,

On 2018-02-19 17:29, Robin Murphy wrote:

Hi Maciej,

On 19/02/18 15:43, Maciej Purski wrote:

When a driver is going to use clk_bulk_get() function, it has to
initialize an array of clk_bulk_data, by filling its id fields.

Add a new function to the core, which dynamically allocates
clk_bulk_data array and fills its id fields. Add clk_bulk_free()
function, which frees the array allocated by clk_bulk_alloc() function.
Add a managed version of clk_bulk_alloc().


Seeing how every subsequent patch ends up with the roughly this same 
stanza:


x = devm_clk_bulk_alloc(dev, num, names);
if (IS_ERR(x)
    return PTR_ERR(x);
ret = devm_clk_bulk_get(dev, x, num);

I wonder if it might be better to simply implement:

int devm_clk_bulk_alloc_get(dev, , num, names)

that does the whole lot in one go, and let drivers that want to do 
more fiddly things continue to open-code the allocation.


But perhaps that's an abstraction too far... I'm not all that familiar 
with the lie of the land here.


Hmmm. This patchset clearly shows, that it would be much simpler if we
get rid of clk_bulk_data structure at all and let clk_bulk_* functions
to operate on struct clk *array[]. Typically the list of clock names
is already in some kind of array (taken from driver data or statically
embedded into driver), so there is little benefit from duplicating it
in clk_bulk_data. Sadly, I missed clk_bulk_* api discussion and maybe
there are other benefits from this approach.

If not, I suggest simplifying clk_bulk_* api by dropping clk_bulk_data
structure and switching to clock ptr array:

int clk_bulk_get(struct device *dev, int num_clock, struct clk *clocks[],
              const char *clk_names[]);
int clk_bulk_prepare(int num_clks, struct clk *clks[]);
int clk_bulk_enable(int num_clks, struct clk *clks[]);
...


Yes, that's certainly a possibility; if on the other hand there are 
desirable reasons for the encapsulation (personally, I do think it's 
quite neat), then maybe num_clocks should get pushed down into 
clk_bulk_data as well - then with dedicated alloc/free functions as 
proposed here it could become a simple opaque cookie as far as callers 
are concerned.


I also haven't looked into the origins of the bulk API design, though; 
I've just been familiarising myself from the perspective of reviewing 
general clk API usage in drivers.


Robin.


Signed-off-by: Maciej Purski 
---
  drivers/clk/clk-bulk.c   | 16 
  drivers/clk/clk-devres.c | 37 +---
  include/linux/clk.h  | 64 


  3 files changed, 113 insertions(+), 4 deletions(-)



[...]
@@ -598,6 +645,23 @@ struct clk *clk_get_sys(const char *dev_id, 
const char *con_id);

    #else /* !CONFIG_HAVE_CLK */
  +static inline struct clk_bulk_data *clk_bulk_alloc(int num_clks,
+   const char **clk_ids)
+{
+    return NULL;


Either way, is it intentional not returning an ERR_PTR() value in this 
case? Since NULL will pass an IS_ERR() check, it seems a bit fragile 
for an allocation call to apparently succeed when the whole API is 
configured out (and I believe introducing new uses of IS_ERR_OR_NULL() 
is in general strongly discouraged.)


Robin.


+}
+
+static inline struct clk_bulk_data *devm_clk_bulk_alloc(struct 
device *dev,

+    int num_clks,
+    const char **clk_ids)
+{
+    return NULL;
+}
+
+static inline void clk_bulk_free(struct clk_bulk_data *clks)
+{
+}
+
  static inline struct clk *clk_get(struct device *dev, const char *id)
  {
  return NULL;



Best regards

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/8] clk: Add clk_bulk_alloc functions

2018-02-20 Thread Marek Szyprowski 

Hi Robin,

On 2018-02-19 17:29, Robin Murphy wrote:

Hi Maciej,

On 19/02/18 15:43, Maciej Purski wrote:

When a driver is going to use clk_bulk_get() function, it has to
initialize an array of clk_bulk_data, by filling its id fields.

Add a new function to the core, which dynamically allocates
clk_bulk_data array and fills its id fields. Add clk_bulk_free()
function, which frees the array allocated by clk_bulk_alloc() function.
Add a managed version of clk_bulk_alloc().


Seeing how every subsequent patch ends up with the roughly this same 
stanza:


x = devm_clk_bulk_alloc(dev, num, names);
if (IS_ERR(x)
    return PTR_ERR(x);
ret = devm_clk_bulk_get(dev, x, num);

I wonder if it might be better to simply implement:

int devm_clk_bulk_alloc_get(dev, , num, names)

that does the whole lot in one go, and let drivers that want to do 
more fiddly things continue to open-code the allocation.


But perhaps that's an abstraction too far... I'm not all that familiar 
with the lie of the land here.


Hmmm. This patchset clearly shows, that it would be much simpler if we
get rid of clk_bulk_data structure at all and let clk_bulk_* functions
to operate on struct clk *array[]. Typically the list of clock names
is already in some kind of array (taken from driver data or statically
embedded into driver), so there is little benefit from duplicating it
in clk_bulk_data. Sadly, I missed clk_bulk_* api discussion and maybe
there are other benefits from this approach.

If not, I suggest simplifying clk_bulk_* api by dropping clk_bulk_data
structure and switching to clock ptr array:

int clk_bulk_get(struct device *dev, int num_clock, struct clk *clocks[],
             const char *clk_names[]);
int clk_bulk_prepare(int num_clks, struct clk *clks[]);
int clk_bulk_enable(int num_clks, struct clk *clks[]);
...






Signed-off-by: Maciej Purski 
---
  drivers/clk/clk-bulk.c   | 16 
  drivers/clk/clk-devres.c | 37 +---
  include/linux/clk.h  | 64 


  3 files changed, 113 insertions(+), 4 deletions(-)



[...]
@@ -598,6 +645,23 @@ struct clk *clk_get_sys(const char *dev_id, 
const char *con_id);

    #else /* !CONFIG_HAVE_CLK */
  +static inline struct clk_bulk_data *clk_bulk_alloc(int num_clks,
+   const char **clk_ids)
+{
+    return NULL;


Either way, is it intentional not returning an ERR_PTR() value in this 
case? Since NULL will pass an IS_ERR() check, it seems a bit fragile 
for an allocation call to apparently succeed when the whole API is 
configured out (and I believe introducing new uses of IS_ERR_OR_NULL() 
is in general strongly discouraged.)


Robin.


+}
+
+static inline struct clk_bulk_data *devm_clk_bulk_alloc(struct 
device *dev,

+    int num_clks,
+    const char **clk_ids)
+{
+    return NULL;
+}
+
+static inline void clk_bulk_free(struct clk_bulk_data *clks)
+{
+}
+
  static inline struct clk *clk_get(struct device *dev, const char *id)
  {
  return NULL;



Best regards
--
Marek Szyprowski, PhD
Samsung R Institute Poland

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/8] clk: Add clk_bulk_alloc functions

2018-02-19 Thread Emil Velikov 
HI Maciej,

Just sharing a couple of fly-by ideas - please don't read too much into them.

On 19 February 2018 at 15:43, Maciej Purski  wrote:
> When a driver is going to use clk_bulk_get() function, it has to
> initialize an array of clk_bulk_data, by filling its id fields.
>
> Add a new function to the core, which dynamically allocates
> clk_bulk_data array and fills its id fields. Add clk_bulk_free()
> function, which frees the array allocated by clk_bulk_alloc() function.
> Add a managed version of clk_bulk_alloc().
>
Most places use a small fixed number of struct clk pointers.
Using devres + kalloc to allocate 1-4 pointers feels a bit strange.

Quick grep shows over 150 instances that could be updated to use the new API.
Adding a cocci script to simplify the transition would be a good idea.

> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -15,6 +15,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
The extra header declaration should not be needed. One should be able
to forward declare any undefined structs.

HTH
Emil
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/8] clk: Add clk_bulk_alloc functions

2018-02-19 Thread Robin Murphy 

Hi Maciej,

On 19/02/18 15:43, Maciej Purski wrote:

When a driver is going to use clk_bulk_get() function, it has to
initialize an array of clk_bulk_data, by filling its id fields.

Add a new function to the core, which dynamically allocates
clk_bulk_data array and fills its id fields. Add clk_bulk_free()
function, which frees the array allocated by clk_bulk_alloc() function.
Add a managed version of clk_bulk_alloc().


Seeing how every subsequent patch ends up with the roughly this same stanza:

x = devm_clk_bulk_alloc(dev, num, names);
if (IS_ERR(x)
return PTR_ERR(x);
ret = devm_clk_bulk_get(dev, x, num);

I wonder if it might be better to simply implement:

int devm_clk_bulk_alloc_get(dev, , num, names)

that does the whole lot in one go, and let drivers that want to do more 
fiddly things continue to open-code the allocation.


But perhaps that's an abstraction too far... I'm not all that familiar 
with the lie of the land here.



Signed-off-by: Maciej Purski 
---
  drivers/clk/clk-bulk.c   | 16 
  drivers/clk/clk-devres.c | 37 +---
  include/linux/clk.h  | 64 
  3 files changed, 113 insertions(+), 4 deletions(-)



[...]

@@ -598,6 +645,23 @@ struct clk *clk_get_sys(const char *dev_id, const char 
*con_id);
  
  #else /* !CONFIG_HAVE_CLK */
  
+static inline struct clk_bulk_data *clk_bulk_alloc(int num_clks,

+  const char **clk_ids)
+{
+   return NULL;


Either way, is it intentional not returning an ERR_PTR() value in this 
case? Since NULL will pass an IS_ERR() check, it seems a bit fragile for 
an allocation call to apparently succeed when the whole API is 
configured out (and I believe introducing new uses of IS_ERR_OR_NULL() 
is in general strongly discouraged.)


Robin.


+}
+
+static inline struct clk_bulk_data *devm_clk_bulk_alloc(struct device *dev,
+   int num_clks,
+   const char **clk_ids)
+{
+   return NULL;
+}
+
+static inline void clk_bulk_free(struct clk_bulk_data *clks)
+{
+}
+
  static inline struct clk *clk_get(struct device *dev, const char *id)
  {
return NULL;


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/8] clk: Add clk_bulk_alloc functions

2018-02-19 Thread Maciej Purski 
When a driver is going to use clk_bulk_get() function, it has to
initialize an array of clk_bulk_data, by filling its id fields.

Add a new function to the core, which dynamically allocates
clk_bulk_data array and fills its id fields. Add clk_bulk_free()
function, which frees the array allocated by clk_bulk_alloc() function.
Add a managed version of clk_bulk_alloc().

Signed-off-by: Maciej Purski 
---
 drivers/clk/clk-bulk.c   | 16 
 drivers/clk/clk-devres.c | 37 +---
 include/linux/clk.h  | 64 
 3 files changed, 113 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/clk-bulk.c b/drivers/clk/clk-bulk.c
index 4c10456..2f16941 100644
--- a/drivers/clk/clk-bulk.c
+++ b/drivers/clk/clk-bulk.c
@@ -19,6 +19,22 @@
 #include 
 #include 
 #include 
+#include 
+
+struct clk_bulk_data *clk_bulk_alloc(int num_clocks, const char *const 
*clk_ids)
+{
+   struct clk_bulk_data *ptr;
+   int i;
+
+   ptr = kcalloc(num_clocks, sizeof(*ptr), GFP_KERNEL);
+   if (!ptr)
+   return ERR_PTR(-ENOMEM);
+
+   for (i = 0; i < num_clocks; i++)
+   ptr[i].id = clk_ids[i];
+
+   return ptr;
+}
 
 void clk_bulk_put(int num_clks, struct clk_bulk_data *clks)
 {
diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index d854e26..2115b97 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -9,6 +9,39 @@
 #include 
 #include 
 
+struct clk_bulk_devres {
+   struct clk_bulk_data *clks;
+   int num_clks;
+};
+
+static void devm_clk_alloc_release(struct device *dev, void *res)
+{
+   struct clk_bulk_devres *devres = res;
+
+   clk_bulk_free(devres->clks);
+}
+
+struct clk_bulk_data *devm_clk_bulk_alloc(struct device *dev, int num_clks,
+ const char *const *clk_ids)
+{
+   struct clk_bulk_data **ptr, *clk_bulk;
+
+   ptr = devres_alloc(devm_clk_alloc_release,
+  num_clks * sizeof(*ptr), GFP_KERNEL);
+   if (!ptr)
+   return ERR_PTR(-ENOMEM);
+
+   clk_bulk = clk_bulk_alloc(num_clks, clk_ids);
+   if (clk_bulk) {
+   *ptr = clk_bulk;
+   devres_add(dev, ptr);
+   } else {
+   devres_free(ptr);
+   }
+
+   return clk_bulk;
+}
+
 static void devm_clk_release(struct device *dev, void *res)
 {
clk_put(*(struct clk **)res);
@@ -34,10 +67,6 @@ struct clk *devm_clk_get(struct device *dev, const char *id)
 }
 EXPORT_SYMBOL(devm_clk_get);
 
-struct clk_bulk_devres {
-   struct clk_bulk_data *clks;
-   int num_clks;
-};
 
 static void devm_clk_bulk_release(struct device *dev, void *res)
 {
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 4c4ef9f..7d66f41 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -15,6 +15,7 @@
 #include 
 #include 
 #include 
+#include 
 
 struct device;
 struct clk;
@@ -240,6 +241,52 @@ static inline void clk_bulk_unprepare(int num_clks, struct 
clk_bulk_data *clks)
 #endif
 
 #ifdef CONFIG_HAVE_CLK
+
+/**
+ * clk_bulk_alloc - allocates an array of clk_bulk_data and fills their
+ * id field
+ * @num_clks: number of clk_bulk_data
+ * @clk_ids: array of clock consumer ID's
+ *
+ * This function allows drivers to dynamically create an array of clk_bulk_data
+ * and fill their id field in one operation. If successful, it allows calling
+ * clk_bulk_get on the pointer returned by this function.
+ *
+ * Returns a pointer to a clk_bulk_data array, or valid IS_ERR() condition
+ * containing errno.
+ */
+struct clk_bulk_data *clk_bulk_alloc(int num_clks, const char *const *clk_ids);
+
+/**
+ * devm_clk_bulk_alloc - allocates an array of clk_bulk_data and fills their
+ *  id field
+ * @dev: device for clock "consumer"
+ * @num_clks: number of clk_bulk_data
+ * @clk_ids: array of clock consumer ID's
+ *
+ * This function allows drivers to dynamically create an array of clk_bulk_data
+ * and fill their id field in one operation with management, the array will
+ * automatically be freed when the device is unbound. If successful, it allows
+ * calling clk_bulk_get on the pointer returned by this function.
+ *
+ * Returns a pointer to a clk_bulk_data array, or valid IS_ERR() condition
+ * containing errno.
+ */
+struct clk_bulk_data *devm_clk_bulk_alloc(struct device *dev, int num_clks,
+ const char * const *clk_ids);
+
+/**
+ * clk_bulk_free - frees the array of clk_bulk_data
+ * @clks: pointer to clk_bulk_data array
+ *
+ * This function frees the array allocated by clk_bulk_data. It must be called
+ * when all clks are freed.
+ */
+static inline void clk_bulk_free(struct clk_bulk_data *clks)
+{
+   kfree(clks);
+}
+
 /**
  * clk_get - lookup and obtain a reference to a clock producer.
  * @dev: device for clock "consumer"
@@ -598,6 +645,23 @@ struct clk *clk_get_sys(const char