Re: [RFC PATCH 1/3] clk: add clk_bulk_get accessories

[Dong Aisheng]

On Fri, Apr 21, 2017 at 08:16:47PM -0700, Stephen Boyd wrote:
> On 04/12, Dong Aisheng wrote:
> >  
> >  #ifdef CONFIG_HAVE_CLK
> > @@ -230,6 +257,32 @@ static inline void clk_unprepare(struct clk *clk)
> >  struct clk *clk_get(struct device *dev, const char *id);
> >  
> >  /**
> > + * clk_bulk_get - lookup and obtain a number of references to clock 
> > producer.
> > + * @dev: device for clock "consumer"
> > + * @num_clks: the number of clk_bulk_data
> > + * @clks: the clk_bulk_data table of consumer
> > + *
> > + * This helper function allows drivers to get several clk consumers in one
> > + * operation. If any of the clk cannot be acquired then any clks
> > + * that were obtained will be freed before returning to the caller.
> > + *
> > + * Returns 0 if all clocks specified in clk_bulk_data table are obtained
> > + * successfully, or valid IS_ERR() condition containing errno.
> > + * The implementation uses @dev and @clk_bulk_data.id to determine the
> > + * clock consumer, and thereby the clock producer.
> > + * (IOW, @id may be identical strings, but clk_get may return different
> > + * clock producers depending on @dev.) The clock returned is stored in
> 
> This comment is inaccurate. Only one dev is possible with this
> API.
> 

Good catch! Will fix it.

> > + * each @clk_bulk_data.clk field.
> > + *
> > + * Drivers must assume that the clock source is not enabled.
> > + *
> > + * clk_bulk_get should not be called from within interrupt context.
> > + */
> > +
> 
> Drop space.
> 

Got it

> > +int __must_check clk_bulk_get(struct device *dev, int num_clks,
> > + struct clk_bulk_data *clks);
> > +
> > +/**
> >   * devm_clk_get - lookup and obtain a managed reference to a clock 
> > producer.
> >   * @dev: device for clock "consumer"
> >   * @id: clock consumer ID
> > @@ -279,6 +332,20 @@ struct clk *devm_get_clk_from_child(struct device *dev,
> >  int clk_enable(struct clk *clk);
> >  
> >  /**
> > + * clk_bulk_enable - inform the system when the bulk of clock source should
> > + *  be running.
> > + * @num_clks: the number of clk_bulk_data
> > + * @clks: the clk_bulk_data table of consumer
> > + *
> > + * If the clock can not be enabled/disabled all, this should return 
> > success.
> > + *
> > + * May be called from atomic contexts.
> > + *
> > + * Returns success (0) or negative errno.
> > + */
> > +int __must_check clk_bulk_enable(int num_clks, struct clk_bulk_data *clks);
> > +
> > +/**
> >   * clk_disable - inform the system when the clock source is no longer 
> > required.
> >   * @clk: clock source
> >   *
> > @@ -295,6 +362,24 @@ int clk_enable(struct clk *clk);
> >  void clk_disable(struct clk *clk);
> >  
> >  /**
> > + * clk_bulk_disable - inform the system when the bulk of clock source is no
> > + *   longer required.
> > + * @num_clks: the number of clk_bulk_data
> > + * @clks: the clk_bulk_data table of consumer
> > + *
> > + * Inform the system that a bulk of clock source is no longer required by
> > + * a driver and may be shut down.
> > + *
> > + * May be called from atomic contexts.
> > + *
> > + * Implementation detail: if the bulk of clock source is shared between
> 
> I'm not sure "bulk of clock source" is the correct terminology.
> Perhaps "set of clks"?
> 

Good to me, will change.

> > + * multiple drivers, clk_bulk_enable() calls must be balanced by the
> > + * same number of clk_bulk_disable() calls for the clock source to be
> > + * disabled.
> > + */
> > +void clk_bulk_disable(int num_clks, struct clk_bulk_data *clks);
> 
> We can mark clk_bulk_data structure as const here? Probably
> applies in other places as well in this patch.
> 

Sounds good, will change the following API,
clk_bulk_{enable|disable}
clk_bulk_{prepare|unprepare}
except clk_bulk_{get|put} which needs update clk_bulk_date.

Regards
Dong Aisheng

Re: [RFC PATCH 1/3] clk: add clk_bulk_get accessories

[Dong Aisheng]

On Fri, Apr 21, 2017 at 08:16:47PM -0700, Stephen Boyd wrote:
> On 04/12, Dong Aisheng wrote:
> >  
> >  #ifdef CONFIG_HAVE_CLK
> > @@ -230,6 +257,32 @@ static inline void clk_unprepare(struct clk *clk)
> >  struct clk *clk_get(struct device *dev, const char *id);
> >  
> >  /**
> > + * clk_bulk_get - lookup and obtain a number of references to clock 
> > producer.
> > + * @dev: device for clock "consumer"
> > + * @num_clks: the number of clk_bulk_data
> > + * @clks: the clk_bulk_data table of consumer
> > + *
> > + * This helper function allows drivers to get several clk consumers in one
> > + * operation. If any of the clk cannot be acquired then any clks
> > + * that were obtained will be freed before returning to the caller.
> > + *
> > + * Returns 0 if all clocks specified in clk_bulk_data table are obtained
> > + * successfully, or valid IS_ERR() condition containing errno.
> > + * The implementation uses @dev and @clk_bulk_data.id to determine the
> > + * clock consumer, and thereby the clock producer.
> > + * (IOW, @id may be identical strings, but clk_get may return different
> > + * clock producers depending on @dev.) The clock returned is stored in
> 
> This comment is inaccurate. Only one dev is possible with this
> API.
> 

Good catch! Will fix it.

> > + * each @clk_bulk_data.clk field.
> > + *
> > + * Drivers must assume that the clock source is not enabled.
> > + *
> > + * clk_bulk_get should not be called from within interrupt context.
> > + */
> > +
> 
> Drop space.
> 

Got it

> > +int __must_check clk_bulk_get(struct device *dev, int num_clks,
> > + struct clk_bulk_data *clks);
> > +
> > +/**
> >   * devm_clk_get - lookup and obtain a managed reference to a clock 
> > producer.
> >   * @dev: device for clock "consumer"
> >   * @id: clock consumer ID
> > @@ -279,6 +332,20 @@ struct clk *devm_get_clk_from_child(struct device *dev,
> >  int clk_enable(struct clk *clk);
> >  
> >  /**
> > + * clk_bulk_enable - inform the system when the bulk of clock source should
> > + *  be running.
> > + * @num_clks: the number of clk_bulk_data
> > + * @clks: the clk_bulk_data table of consumer
> > + *
> > + * If the clock can not be enabled/disabled all, this should return 
> > success.
> > + *
> > + * May be called from atomic contexts.
> > + *
> > + * Returns success (0) or negative errno.
> > + */
> > +int __must_check clk_bulk_enable(int num_clks, struct clk_bulk_data *clks);
> > +
> > +/**
> >   * clk_disable - inform the system when the clock source is no longer 
> > required.
> >   * @clk: clock source
> >   *
> > @@ -295,6 +362,24 @@ int clk_enable(struct clk *clk);
> >  void clk_disable(struct clk *clk);
> >  
> >  /**
> > + * clk_bulk_disable - inform the system when the bulk of clock source is no
> > + *   longer required.
> > + * @num_clks: the number of clk_bulk_data
> > + * @clks: the clk_bulk_data table of consumer
> > + *
> > + * Inform the system that a bulk of clock source is no longer required by
> > + * a driver and may be shut down.
> > + *
> > + * May be called from atomic contexts.
> > + *
> > + * Implementation detail: if the bulk of clock source is shared between
> 
> I'm not sure "bulk of clock source" is the correct terminology.
> Perhaps "set of clks"?
> 

Good to me, will change.

> > + * multiple drivers, clk_bulk_enable() calls must be balanced by the
> > + * same number of clk_bulk_disable() calls for the clock source to be
> > + * disabled.
> > + */
> > +void clk_bulk_disable(int num_clks, struct clk_bulk_data *clks);
> 
> We can mark clk_bulk_data structure as const here? Probably
> applies in other places as well in this patch.
> 

Sounds good, will change the following API,
clk_bulk_{enable|disable}
clk_bulk_{prepare|unprepare}
except clk_bulk_{get|put} which needs update clk_bulk_date.

Regards
Dong Aisheng

Re: [RFC PATCH 1/3] clk: add clk_bulk_get accessories

[Stephen Boyd]

On 04/12, Dong Aisheng wrote:
>  
>  #ifdef CONFIG_HAVE_CLK
> @@ -230,6 +257,32 @@ static inline void clk_unprepare(struct clk *clk)
>  struct clk *clk_get(struct device *dev, const char *id);
>  
>  /**
> + * clk_bulk_get - lookup and obtain a number of references to clock producer.
> + * @dev: device for clock "consumer"
> + * @num_clks: the number of clk_bulk_data
> + * @clks: the clk_bulk_data table of consumer
> + *
> + * This helper function allows drivers to get several clk consumers in one
> + * operation. If any of the clk cannot be acquired then any clks
> + * that were obtained will be freed before returning to the caller.
> + *
> + * Returns 0 if all clocks specified in clk_bulk_data table are obtained
> + * successfully, or valid IS_ERR() condition containing errno.
> + * The implementation uses @dev and @clk_bulk_data.id to determine the
> + * clock consumer, and thereby the clock producer.
> + * (IOW, @id may be identical strings, but clk_get may return different
> + * clock producers depending on @dev.) The clock returned is stored in

This comment is inaccurate. Only one dev is possible with this
API.

> + * each @clk_bulk_data.clk field.
> + *
> + * Drivers must assume that the clock source is not enabled.
> + *
> + * clk_bulk_get should not be called from within interrupt context.
> + */
> +

Drop space.

> +int __must_check clk_bulk_get(struct device *dev, int num_clks,
> +   struct clk_bulk_data *clks);
> +
> +/**
>   * devm_clk_get - lookup and obtain a managed reference to a clock producer.
>   * @dev: device for clock "consumer"
>   * @id: clock consumer ID
> @@ -279,6 +332,20 @@ struct clk *devm_get_clk_from_child(struct device *dev,
>  int clk_enable(struct clk *clk);
>  
>  /**
> + * clk_bulk_enable - inform the system when the bulk of clock source should
> + *be running.
> + * @num_clks: the number of clk_bulk_data
> + * @clks: the clk_bulk_data table of consumer
> + *
> + * If the clock can not be enabled/disabled all, this should return success.
> + *
> + * May be called from atomic contexts.
> + *
> + * Returns success (0) or negative errno.
> + */
> +int __must_check clk_bulk_enable(int num_clks, struct clk_bulk_data *clks);
> +
> +/**
>   * clk_disable - inform the system when the clock source is no longer 
> required.
>   * @clk: clock source
>   *
> @@ -295,6 +362,24 @@ int clk_enable(struct clk *clk);
>  void clk_disable(struct clk *clk);
>  
>  /**
> + * clk_bulk_disable - inform the system when the bulk of clock source is no
> + * longer required.
> + * @num_clks: the number of clk_bulk_data
> + * @clks: the clk_bulk_data table of consumer
> + *
> + * Inform the system that a bulk of clock source is no longer required by
> + * a driver and may be shut down.
> + *
> + * May be called from atomic contexts.
> + *
> + * Implementation detail: if the bulk of clock source is shared between

I'm not sure "bulk of clock source" is the correct terminology.
Perhaps "set of clks"?

> + * multiple drivers, clk_bulk_enable() calls must be balanced by the
> + * same number of clk_bulk_disable() calls for the clock source to be
> + * disabled.
> + */
> +void clk_bulk_disable(int num_clks, struct clk_bulk_data *clks);

We can mark clk_bulk_data structure as const here? Probably
applies in other places as well in this patch.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RFC PATCH 1/3] clk: add clk_bulk_get accessories

[Stephen Boyd]

On 04/12, Dong Aisheng wrote:
>  
>  #ifdef CONFIG_HAVE_CLK
> @@ -230,6 +257,32 @@ static inline void clk_unprepare(struct clk *clk)
>  struct clk *clk_get(struct device *dev, const char *id);
>  
>  /**
> + * clk_bulk_get - lookup and obtain a number of references to clock producer.
> + * @dev: device for clock "consumer"
> + * @num_clks: the number of clk_bulk_data
> + * @clks: the clk_bulk_data table of consumer
> + *
> + * This helper function allows drivers to get several clk consumers in one
> + * operation. If any of the clk cannot be acquired then any clks
> + * that were obtained will be freed before returning to the caller.
> + *
> + * Returns 0 if all clocks specified in clk_bulk_data table are obtained
> + * successfully, or valid IS_ERR() condition containing errno.
> + * The implementation uses @dev and @clk_bulk_data.id to determine the
> + * clock consumer, and thereby the clock producer.
> + * (IOW, @id may be identical strings, but clk_get may return different
> + * clock producers depending on @dev.) The clock returned is stored in

This comment is inaccurate. Only one dev is possible with this
API.

> + * each @clk_bulk_data.clk field.
> + *
> + * Drivers must assume that the clock source is not enabled.
> + *
> + * clk_bulk_get should not be called from within interrupt context.
> + */
> +

Drop space.

> +int __must_check clk_bulk_get(struct device *dev, int num_clks,
> +   struct clk_bulk_data *clks);
> +
> +/**
>   * devm_clk_get - lookup and obtain a managed reference to a clock producer.
>   * @dev: device for clock "consumer"
>   * @id: clock consumer ID
> @@ -279,6 +332,20 @@ struct clk *devm_get_clk_from_child(struct device *dev,
>  int clk_enable(struct clk *clk);
>  
>  /**
> + * clk_bulk_enable - inform the system when the bulk of clock source should
> + *be running.
> + * @num_clks: the number of clk_bulk_data
> + * @clks: the clk_bulk_data table of consumer
> + *
> + * If the clock can not be enabled/disabled all, this should return success.
> + *
> + * May be called from atomic contexts.
> + *
> + * Returns success (0) or negative errno.
> + */
> +int __must_check clk_bulk_enable(int num_clks, struct clk_bulk_data *clks);
> +
> +/**
>   * clk_disable - inform the system when the clock source is no longer 
> required.
>   * @clk: clock source
>   *
> @@ -295,6 +362,24 @@ int clk_enable(struct clk *clk);
>  void clk_disable(struct clk *clk);
>  
>  /**
> + * clk_bulk_disable - inform the system when the bulk of clock source is no
> + * longer required.
> + * @num_clks: the number of clk_bulk_data
> + * @clks: the clk_bulk_data table of consumer
> + *
> + * Inform the system that a bulk of clock source is no longer required by
> + * a driver and may be shut down.
> + *
> + * May be called from atomic contexts.
> + *
> + * Implementation detail: if the bulk of clock source is shared between

I'm not sure "bulk of clock source" is the correct terminology.
Perhaps "set of clks"?

> + * multiple drivers, clk_bulk_enable() calls must be balanced by the
> + * same number of clk_bulk_disable() calls for the clock source to be
> + * disabled.
> + */
> +void clk_bulk_disable(int num_clks, struct clk_bulk_data *clks);

We can mark clk_bulk_data structure as const here? Probably
applies in other places as well in this patch.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RFC PATCH 1/3] clk: add clk_bulk_get accessories

[Dong Aisheng]

Hi Geert,

On Thu, Apr 13, 2017 at 09:56:31PM +0200, Geert Uytterhoeven wrote:
> On Wed, Apr 12, 2017 at 6:03 AM, Dong Aisheng  wrote:
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -520,6 +520,23 @@ void clk_unprepare(struct clk *clk)
> >  }
> >  EXPORT_SYMBOL_GPL(clk_unprepare);
> >
> > +/**
> > + * clk_bulk_unprepare - undo preparation of a bulk of clock sources
> > + * @num_clks: the number of clk_bulk_data
> > + * @clks: the clk_bulk_data table being ungated
> > + *
> > + * clk_bulk_unprepare may sleep, which differentiates it from 
> > clk_bulk_disable.
> > + * Returns 0 on success, -EERROR otherwise.
> > + */
> > +void clk_bulk_unprepare(int num_clks, struct clk_bulk_data *clks)
> 
> unsigned int num_clks (everywhere)
> 
> > +{
> > +   int i;
> 
> unsigned int i (everywhere)

Any special purpose?

Looks like 'int i' for a loop is widely used in kernel.

Would you please help clarify more?

> > +
> > +   for (i = 0; i < num_clks; i++)
> > +   clk_unprepare(clks[i].clk);
> > +}
> > +EXPORT_SYMBOL_GPL(clk_bulk_unprepare);
> 
> This does mean you have to change your "while (--i >= 0)" loops.

Is that really necessary as i thought the clk_bulk_get/put does not
guarantee any clk operation orders within the bulk?
Should we need add that support?

And currently this does the same thing as bulk regulator.

Regards
Dong Aisheng

Re: [RFC PATCH 1/3] clk: add clk_bulk_get accessories

[Dong Aisheng]

Hi Geert,

On Thu, Apr 13, 2017 at 09:56:31PM +0200, Geert Uytterhoeven wrote:
> On Wed, Apr 12, 2017 at 6:03 AM, Dong Aisheng  wrote:
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -520,6 +520,23 @@ void clk_unprepare(struct clk *clk)
> >  }
> >  EXPORT_SYMBOL_GPL(clk_unprepare);
> >
> > +/**
> > + * clk_bulk_unprepare - undo preparation of a bulk of clock sources
> > + * @num_clks: the number of clk_bulk_data
> > + * @clks: the clk_bulk_data table being ungated
> > + *
> > + * clk_bulk_unprepare may sleep, which differentiates it from 
> > clk_bulk_disable.
> > + * Returns 0 on success, -EERROR otherwise.
> > + */
> > +void clk_bulk_unprepare(int num_clks, struct clk_bulk_data *clks)
> 
> unsigned int num_clks (everywhere)
> 
> > +{
> > +   int i;
> 
> unsigned int i (everywhere)

Any special purpose?

Looks like 'int i' for a loop is widely used in kernel.

Would you please help clarify more?

> > +
> > +   for (i = 0; i < num_clks; i++)
> > +   clk_unprepare(clks[i].clk);
> > +}
> > +EXPORT_SYMBOL_GPL(clk_bulk_unprepare);
> 
> This does mean you have to change your "while (--i >= 0)" loops.

Is that really necessary as i thought the clk_bulk_get/put does not
guarantee any clk operation orders within the bulk?
Should we need add that support?

And currently this does the same thing as bulk regulator.

Regards
Dong Aisheng

Re: [RFC PATCH 1/3] clk: add clk_bulk_get accessories

[Geert Uytterhoeven]

On Tue, Apr 11, 2017 at 7:19 PM, Leonard Crestez
 wrote:
> On Wed, 2017-04-12 at 12:03 +0800, Dong Aisheng wrote:
>> +/**
>> + * clk_bulk_enable - ungate a bulk of clocks
>> + * @num_clks: the number of clk_bulk_data
>> + * @clks: the clk_bulk_data table being ungated
>> + *
>> + * clk_bulk_enable must not sleep
>> + * Returns 0 on success, -EERROR otherwise.
>> + */
>> +int clk_bulk_enable(int num_clks, struct clk_bulk_data *clks)
>> +{
>> +   int ret;
>> +   int i;
>> +
>> +   for (i = 0; i < num_clks; i++) {
>> +   ret = clk_enable(clks[i].clk);
>> +   if (ret) {
>> +   pr_err("Failed to enable clk '%s': %d\n",
>> +   clks[i].id, ret);
>> +   goto err;
>> +   }
>> +   }
>> +
>> +   return 0;
>> +
>> +err:
>> +   while (--i >= 0)
>> +   clk_put(clks[i].clk);
>
> Shouldn't this be clk_disable?
>
> And you can probably use clk_bulk_disable(i, clks) instead

To avoid nasty surprises, clk_bulk_disable() should loop over all clocks
in reverse order.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [RFC PATCH 1/3] clk: add clk_bulk_get accessories

[Geert Uytterhoeven]

On Tue, Apr 11, 2017 at 7:19 PM, Leonard Crestez
 wrote:
> On Wed, 2017-04-12 at 12:03 +0800, Dong Aisheng wrote:
>> +/**
>> + * clk_bulk_enable - ungate a bulk of clocks
>> + * @num_clks: the number of clk_bulk_data
>> + * @clks: the clk_bulk_data table being ungated
>> + *
>> + * clk_bulk_enable must not sleep
>> + * Returns 0 on success, -EERROR otherwise.
>> + */
>> +int clk_bulk_enable(int num_clks, struct clk_bulk_data *clks)
>> +{
>> +   int ret;
>> +   int i;
>> +
>> +   for (i = 0; i < num_clks; i++) {
>> +   ret = clk_enable(clks[i].clk);
>> +   if (ret) {
>> +   pr_err("Failed to enable clk '%s': %d\n",
>> +   clks[i].id, ret);
>> +   goto err;
>> +   }
>> +   }
>> +
>> +   return 0;
>> +
>> +err:
>> +   while (--i >= 0)
>> +   clk_put(clks[i].clk);
>
> Shouldn't this be clk_disable?
>
> And you can probably use clk_bulk_disable(i, clks) instead

To avoid nasty surprises, clk_bulk_disable() should loop over all clocks
in reverse order.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [RFC PATCH 1/3] clk: add clk_bulk_get accessories

[Geert Uytterhoeven]

On Wed, Apr 12, 2017 at 6:03 AM, Dong Aisheng  wrote:
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -520,6 +520,23 @@ void clk_unprepare(struct clk *clk)
>  }
>  EXPORT_SYMBOL_GPL(clk_unprepare);
>
> +/**
> + * clk_bulk_unprepare - undo preparation of a bulk of clock sources
> + * @num_clks: the number of clk_bulk_data
> + * @clks: the clk_bulk_data table being ungated
> + *
> + * clk_bulk_unprepare may sleep, which differentiates it from 
> clk_bulk_disable.
> + * Returns 0 on success, -EERROR otherwise.
> + */
> +void clk_bulk_unprepare(int num_clks, struct clk_bulk_data *clks)

unsigned int num_clks (everywhere)

> +{
> +   int i;

unsigned int i (everywhere)

> +
> +   for (i = 0; i < num_clks; i++)
> +   clk_unprepare(clks[i].clk);
> +}
> +EXPORT_SYMBOL_GPL(clk_bulk_unprepare);

This does mean you have to change your "while (--i >= 0)" loops.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [RFC PATCH 1/3] clk: add clk_bulk_get accessories

[Geert Uytterhoeven]

On Wed, Apr 12, 2017 at 6:03 AM, Dong Aisheng  wrote:
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -520,6 +520,23 @@ void clk_unprepare(struct clk *clk)
>  }
>  EXPORT_SYMBOL_GPL(clk_unprepare);
>
> +/**
> + * clk_bulk_unprepare - undo preparation of a bulk of clock sources
> + * @num_clks: the number of clk_bulk_data
> + * @clks: the clk_bulk_data table being ungated
> + *
> + * clk_bulk_unprepare may sleep, which differentiates it from 
> clk_bulk_disable.
> + * Returns 0 on success, -EERROR otherwise.
> + */
> +void clk_bulk_unprepare(int num_clks, struct clk_bulk_data *clks)

unsigned int num_clks (everywhere)

> +{
> +   int i;

unsigned int i (everywhere)

> +
> +   for (i = 0; i < num_clks; i++)
> +   clk_unprepare(clks[i].clk);
> +}
> +EXPORT_SYMBOL_GPL(clk_bulk_unprepare);

This does mean you have to change your "while (--i >= 0)" loops.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [RFC PATCH 1/3] clk: add clk_bulk_get accessories

[Dong Aisheng]

On Wed, Apr 12, 2017 at 12:03:27PM +0800, Dong Aisheng wrote:
...
> @@ -445,6 +543,12 @@ static inline struct clk *clk_get(struct device *dev, 
> const char *id)
>   return NULL;
>  }
>  
> +static inline int clk_bulk_get(struct device *dev, int num_clks,
> +struct clk_bulk_data *clks)
> +{
> + return NULL;

Here needs to be changed to 'return 0'.

It's catched by 0day robot.

include/linux/clk.h: In function 'clk_bulk_get':
include/linux/stddef.h:7:14: warning: return makes integer from pointer without 
a cast [-Wint-conversion]
#define NULL ((void *)0)

Good job! Robot!

Regards
Dong Aisheng

Re: [RFC PATCH 1/3] clk: add clk_bulk_get accessories

[Dong Aisheng]

On Wed, Apr 12, 2017 at 12:03:27PM +0800, Dong Aisheng wrote:
...
> @@ -445,6 +543,12 @@ static inline struct clk *clk_get(struct device *dev, 
> const char *id)
>   return NULL;
>  }
>  
> +static inline int clk_bulk_get(struct device *dev, int num_clks,
> +struct clk_bulk_data *clks)
> +{
> + return NULL;

Here needs to be changed to 'return 0'.

It's catched by 0day robot.

include/linux/clk.h: In function 'clk_bulk_get':
include/linux/stddef.h:7:14: warning: return makes integer from pointer without 
a cast [-Wint-conversion]
#define NULL ((void *)0)

Good job! Robot!

Regards
Dong Aisheng

Re: [RFC PATCH 1/3] clk: add clk_bulk_get accessories

[Dong Aisheng]

On Tue, Apr 11, 2017 at 08:19:19PM +0300, Leonard Crestez wrote:
> On Wed, 2017-04-12 at 12:03 +0800, Dong Aisheng wrote:
> > +/**
> > + * clk_bulk_enable - ungate a bulk of clocks
> > + * @num_clks: the number of clk_bulk_data
> > + * @clks: the clk_bulk_data table being ungated
> > + *
> > + * clk_bulk_enable must not sleep
> > + * Returns 0 on success, -EERROR otherwise.
> > + */
> > +int clk_bulk_enable(int num_clks, struct clk_bulk_data *clks)
> > +{
> > +   int ret;
> > +   int i;
> > +
> > +   for (i = 0; i < num_clks; i++) {
> > +   ret = clk_enable(clks[i].clk);
> > +   if (ret) {
> > +   pr_err("Failed to enable clk '%s': %d\n",
> > +   clks[i].id, ret);
> > +   goto err;
> > +   }
> > +   }
> > +
> > +   return 0;
> > +
> > +err:
> > +   while (--i >= 0)
> > +   clk_put(clks[i].clk);
> 
> Shouldn't this be clk_disable?
> 

Good catch!
Will change in the formal version if Maintainer accepts this idea.

> And you can probably use clk_bulk_disable(i, clks) instead

Probably i'd prefer keep clk_disable to match with clk_enable
to make things more clear.

Thanks

Regards
Dong Aisheng

Re: [RFC PATCH 1/3] clk: add clk_bulk_get accessories

[Dong Aisheng]

On Tue, Apr 11, 2017 at 08:19:19PM +0300, Leonard Crestez wrote:
> On Wed, 2017-04-12 at 12:03 +0800, Dong Aisheng wrote:
> > +/**
> > + * clk_bulk_enable - ungate a bulk of clocks
> > + * @num_clks: the number of clk_bulk_data
> > + * @clks: the clk_bulk_data table being ungated
> > + *
> > + * clk_bulk_enable must not sleep
> > + * Returns 0 on success, -EERROR otherwise.
> > + */
> > +int clk_bulk_enable(int num_clks, struct clk_bulk_data *clks)
> > +{
> > +   int ret;
> > +   int i;
> > +
> > +   for (i = 0; i < num_clks; i++) {
> > +   ret = clk_enable(clks[i].clk);
> > +   if (ret) {
> > +   pr_err("Failed to enable clk '%s': %d\n",
> > +   clks[i].id, ret);
> > +   goto err;
> > +   }
> > +   }
> > +
> > +   return 0;
> > +
> > +err:
> > +   while (--i >= 0)
> > +   clk_put(clks[i].clk);
> 
> Shouldn't this be clk_disable?
> 

Good catch!
Will change in the formal version if Maintainer accepts this idea.

> And you can probably use clk_bulk_disable(i, clks) instead

Probably i'd prefer keep clk_disable to match with clk_enable
to make things more clear.

Thanks

Regards
Dong Aisheng

Re: [RFC PATCH 1/3] clk: add clk_bulk_get accessories

[Leonard Crestez]

On Wed, 2017-04-12 at 12:03 +0800, Dong Aisheng wrote:
> +/**
> + * clk_bulk_enable - ungate a bulk of clocks
> + * @num_clks: the number of clk_bulk_data
> + * @clks: the clk_bulk_data table being ungated
> + *
> + * clk_bulk_enable must not sleep
> + * Returns 0 on success, -EERROR otherwise.
> + */
> +int clk_bulk_enable(int num_clks, struct clk_bulk_data *clks)
> +{
> +   int ret;
> +   int i;
> +
> +   for (i = 0; i < num_clks; i++) {
> +   ret = clk_enable(clks[i].clk);
> +   if (ret) {
> +   pr_err("Failed to enable clk '%s': %d\n",
> +   clks[i].id, ret);
> +   goto err;
> +   }
> +   }
> +
> +   return 0;
> +
> +err:
> +   while (--i >= 0)
> +   clk_put(clks[i].clk);

Shouldn't this be clk_disable?

And you can probably use clk_bulk_disable(i, clks) instead

Re: [RFC PATCH 1/3] clk: add clk_bulk_get accessories

[Leonard Crestez]

On Wed, 2017-04-12 at 12:03 +0800, Dong Aisheng wrote:
> +/**
> + * clk_bulk_enable - ungate a bulk of clocks
> + * @num_clks: the number of clk_bulk_data
> + * @clks: the clk_bulk_data table being ungated
> + *
> + * clk_bulk_enable must not sleep
> + * Returns 0 on success, -EERROR otherwise.
> + */
> +int clk_bulk_enable(int num_clks, struct clk_bulk_data *clks)
> +{
> +   int ret;
> +   int i;
> +
> +   for (i = 0; i < num_clks; i++) {
> +   ret = clk_enable(clks[i].clk);
> +   if (ret) {
> +   pr_err("Failed to enable clk '%s': %d\n",
> +   clks[i].id, ret);
> +   goto err;
> +   }
> +   }
> +
> +   return 0;
> +
> +err:
> +   while (--i >= 0)
> +   clk_put(clks[i].clk);

Shouldn't this be clk_disable?

And you can probably use clk_bulk_disable(i, clks) instead

[RFC PATCH 1/3] clk: add clk_bulk_get accessories

[Dong Aisheng]

These helper function allows drivers to get several clk consumers in
one operation.  If any of the clk cannot be acquired then any clks
that were got will be put before returning to the caller.

This can relieve the driver owners' life who needs to handle many clocks,
as well as each clock error reporting.

Cc: Michael Turquette 
Cc: Stephen Boyd 
Cc: "Rafael J. Wysocki" 
Cc: Viresh Kumar 
Cc: Mark Brown 
Cc: Shawn Guo 
Cc: Fabio Estevam 
Cc: Sascha Hauer 
Cc: Anson Huang 
Cc: Robin Gong 
Cc: Bai Ping 
Cc: Leonard Crestez 
Cc: Octavian Purdila 
Signed-off-by: Dong Aisheng 
---
 drivers/clk/clk.c|  99 
 drivers/clk/clkdev.c |  42 +++
 include/linux/clk.h  | 115 +++
 3 files changed, 256 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index cbe..622f8069 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -520,6 +520,23 @@ void clk_unprepare(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_unprepare);
 
+/**
+ * clk_bulk_unprepare - undo preparation of a bulk of clock sources
+ * @num_clks: the number of clk_bulk_data
+ * @clks: the clk_bulk_data table being ungated
+ *
+ * clk_bulk_unprepare may sleep, which differentiates it from clk_bulk_disable.
+ * Returns 0 on success, -EERROR otherwise.
+ */
+void clk_bulk_unprepare(int num_clks, struct clk_bulk_data *clks)
+{
+   int i;
+
+   for (i = 0; i < num_clks; i++)
+   clk_unprepare(clks[i].clk);
+}
+EXPORT_SYMBOL_GPL(clk_bulk_unprepare);
+
 static int clk_core_prepare(struct clk_core *core)
 {
int ret = 0;
@@ -584,6 +601,38 @@ int clk_prepare(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_prepare);
 
+/**
+ * clk_bulk_prepare - prepare a bulk of clocks
+ * @num_clks: the number of clk_bulk_data
+ * @clks: the clk_bulk_data table being ungated
+ *
+ * clk_bulk_prepare may sleep, which differentiates it from clk_bulk_enable.
+ * Returns 0 on success, -EERROR otherwise.
+ */
+
+int __must_check clk_bulk_prepare(int num_clks, struct clk_bulk_data *clks)
+{
+   int ret;
+   int i;
+
+   for (i = 0; i < num_clks; i++) {
+   ret = clk_prepare(clks[i].clk);
+   if (ret) {
+   pr_err("Failed to prepare clk '%s': %d\n",
+   clks[i].id, ret);
+   goto err;
+   }
+   }
+
+   return 0;
+
+err:
+   while (--i >= 0)
+   clk_unprepare(clks[i].clk);
+
+   return  ret;
+}
+
 static void clk_core_disable(struct clk_core *core)
 {
lockdep_assert_held(_lock);
@@ -640,6 +689,24 @@ void clk_disable(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_disable);
 
+/**
+ * clk_bulk_disable - gate a bulk of clocks
+ * @num_clks: the number of clk_bulk_data
+ * @clks: the clk_bulk_data table being ungated
+ *
+ * clk_bulk_disable must not sleep, which differentiates it from
+ * clk_bulk_unprepare. clk_bulk_disable must be called before
+ * clk_bulk_unprepare.
+ */
+void clk_bulk_disable(int num_clks, struct clk_bulk_data *clks)
+{
+   int i;
+
+   for (i = 0; i < num_clks; i++)
+   clk_disable(clks[i].clk);
+}
+EXPORT_SYMBOL_GPL(clk_bulk_disable);
+
 static int clk_core_enable(struct clk_core *core)
 {
int ret = 0;
@@ -709,6 +776,38 @@ int clk_enable(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_enable);
 
+/**
+ * clk_bulk_enable - ungate a bulk of clocks
+ * @num_clks: the number of clk_bulk_data
+ * @clks: the clk_bulk_data table being ungated
+ *
+ * clk_bulk_enable must not sleep
+ * Returns 0 on success, -EERROR otherwise.
+ */
+int clk_bulk_enable(int num_clks, struct clk_bulk_data *clks)
+{
+   int ret;
+   int i;
+
+   for (i = 0; i < num_clks; i++) {
+   ret = clk_enable(clks[i].clk);
+   if (ret) {
+   pr_err("Failed to enable clk '%s': %d\n",
+   clks[i].id, ret);
+   goto err;
+   }
+   }
+
+   return 0;
+
+err:
+   while (--i >= 0)
+   clk_put(clks[i].clk);
+
+   return  ret;
+}
+EXPORT_SYMBOL_GPL(clk_bulk_enable);
+
 static int clk_core_prepare_enable(struct clk_core *core)
 {
int ret;
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index bb8a77a..141f2e6 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -209,12 +209,54 @@ struct clk *clk_get(struct device *dev, const char 
*con_id)
 }
 EXPORT_SYMBOL(clk_get);
 
+int clk_bulk_get(struct device *dev, int num_clks,
+struct clk_bulk_data *clks)
+{
+   int ret;
+   int i;
+
+   for (i = 0; i < 

[RFC PATCH 1/3] clk: add clk_bulk_get accessories

[Dong Aisheng]

These helper function allows drivers to get several clk consumers in
one operation.  If any of the clk cannot be acquired then any clks
that were got will be put before returning to the caller.

This can relieve the driver owners' life who needs to handle many clocks,
as well as each clock error reporting.

Cc: Michael Turquette 
Cc: Stephen Boyd 
Cc: "Rafael J. Wysocki" 
Cc: Viresh Kumar 
Cc: Mark Brown 
Cc: Shawn Guo 
Cc: Fabio Estevam 
Cc: Sascha Hauer 
Cc: Anson Huang 
Cc: Robin Gong 
Cc: Bai Ping 
Cc: Leonard Crestez 
Cc: Octavian Purdila 
Signed-off-by: Dong Aisheng 
---
 drivers/clk/clk.c|  99 
 drivers/clk/clkdev.c |  42 +++
 include/linux/clk.h  | 115 +++
 3 files changed, 256 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index cbe..622f8069 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -520,6 +520,23 @@ void clk_unprepare(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_unprepare);
 
+/**
+ * clk_bulk_unprepare - undo preparation of a bulk of clock sources
+ * @num_clks: the number of clk_bulk_data
+ * @clks: the clk_bulk_data table being ungated
+ *
+ * clk_bulk_unprepare may sleep, which differentiates it from clk_bulk_disable.
+ * Returns 0 on success, -EERROR otherwise.
+ */
+void clk_bulk_unprepare(int num_clks, struct clk_bulk_data *clks)
+{
+   int i;
+
+   for (i = 0; i < num_clks; i++)
+   clk_unprepare(clks[i].clk);
+}
+EXPORT_SYMBOL_GPL(clk_bulk_unprepare);
+
 static int clk_core_prepare(struct clk_core *core)
 {
int ret = 0;
@@ -584,6 +601,38 @@ int clk_prepare(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_prepare);
 
+/**
+ * clk_bulk_prepare - prepare a bulk of clocks
+ * @num_clks: the number of clk_bulk_data
+ * @clks: the clk_bulk_data table being ungated
+ *
+ * clk_bulk_prepare may sleep, which differentiates it from clk_bulk_enable.
+ * Returns 0 on success, -EERROR otherwise.
+ */
+
+int __must_check clk_bulk_prepare(int num_clks, struct clk_bulk_data *clks)
+{
+   int ret;
+   int i;
+
+   for (i = 0; i < num_clks; i++) {
+   ret = clk_prepare(clks[i].clk);
+   if (ret) {
+   pr_err("Failed to prepare clk '%s': %d\n",
+   clks[i].id, ret);
+   goto err;
+   }
+   }
+
+   return 0;
+
+err:
+   while (--i >= 0)
+   clk_unprepare(clks[i].clk);
+
+   return  ret;
+}
+
 static void clk_core_disable(struct clk_core *core)
 {
lockdep_assert_held(_lock);
@@ -640,6 +689,24 @@ void clk_disable(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_disable);
 
+/**
+ * clk_bulk_disable - gate a bulk of clocks
+ * @num_clks: the number of clk_bulk_data
+ * @clks: the clk_bulk_data table being ungated
+ *
+ * clk_bulk_disable must not sleep, which differentiates it from
+ * clk_bulk_unprepare. clk_bulk_disable must be called before
+ * clk_bulk_unprepare.
+ */
+void clk_bulk_disable(int num_clks, struct clk_bulk_data *clks)
+{
+   int i;
+
+   for (i = 0; i < num_clks; i++)
+   clk_disable(clks[i].clk);
+}
+EXPORT_SYMBOL_GPL(clk_bulk_disable);
+
 static int clk_core_enable(struct clk_core *core)
 {
int ret = 0;
@@ -709,6 +776,38 @@ int clk_enable(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(clk_enable);
 
+/**
+ * clk_bulk_enable - ungate a bulk of clocks
+ * @num_clks: the number of clk_bulk_data
+ * @clks: the clk_bulk_data table being ungated
+ *
+ * clk_bulk_enable must not sleep
+ * Returns 0 on success, -EERROR otherwise.
+ */
+int clk_bulk_enable(int num_clks, struct clk_bulk_data *clks)
+{
+   int ret;
+   int i;
+
+   for (i = 0; i < num_clks; i++) {
+   ret = clk_enable(clks[i].clk);
+   if (ret) {
+   pr_err("Failed to enable clk '%s': %d\n",
+   clks[i].id, ret);
+   goto err;
+   }
+   }
+
+   return 0;
+
+err:
+   while (--i >= 0)
+   clk_put(clks[i].clk);
+
+   return  ret;
+}
+EXPORT_SYMBOL_GPL(clk_bulk_enable);
+
 static int clk_core_prepare_enable(struct clk_core *core)
 {
int ret;
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index bb8a77a..141f2e6 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -209,12 +209,54 @@ struct clk *clk_get(struct device *dev, const char 
*con_id)
 }
 EXPORT_SYMBOL(clk_get);
 
+int clk_bulk_get(struct device *dev, int num_clks,
+struct clk_bulk_data *clks)
+{
+   int ret;
+   int i;
+
+   for (i = 0; i < num_clks; i++)
+   clks[i].clk = NULL;
+
+   for (i = 0; i < num_clks; i++) {
+   clks[i].clk = clk_get(dev, clks[i].id);
+   if (IS_ERR(clks[i].clk)) {
+   ret = PTR_ERR(clks[i].clk);
+   dev_err(dev, "Failed to get clk '%s': %d\n",
+