Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1452544 > unrolled thread

Re: [v3,2/6] power: add power sequence library

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [v3,2/6] power: add power sequence library Matthias Kaehlcke <mka@chromium.org> - 2016-07-29 22:10 +0200
    Re: [v3,2/6] power: add power sequence library Peter Chen <hzpeterchen@gmail.com> - 2016-08-01 04:30 +0200
    Re: [v3,2/6] power: add power sequence library Peter Chen <hzpeterchen@gmail.com> - 2016-08-02 05:50 +0200

#1452544 — Re: [v3,2/6] power: add power sequence library

Hi Peter,

Thanks for your work on this, a few comments inline


On 07/20/2016 02:40 AM, Peter Chen wrote:

> ...
>
> +static int pwrseq_generic_on(struct device_node *np, struct pwrseq *pwrseq)
> +{
>
> ...
>
> +	if (gpiod_reset) {
> +		u32 duration_us = 50;
> +
> +		of_property_read_u32(np, "reset-duration-us",
> +				&duration_us);
> +		usleep_range(duration_us, duration_us + 10);
The end of the range could allow for more margin. Also consider busy 
looping for very short delays as in 
http://lxr.free-electrons.com/source/drivers/regulator/core.c#L2062
> ...
>
> +static int pwrseq_generic_get(struct device_node *np, struct pwrseq *pwrseq)
> +{
> +	struct pwrseq_generic *pwrseq_gen = to_generic_pwrseq(pwrseq);
> +	enum of_gpio_flags flags;
> +	int reset_gpio, ret = 0;
> +
> +	pwrseq_gen->clk = of_clk_get_by_name(np, NULL);
This only gets the first of potentially multiple clocks, is that intended?

[toc] | [next] | [standalone]

#1452938

On Fri, Jul 29, 2016 at 01:06:48PM -0700, Matthias Kaehlcke wrote:
> Hi Peter,
> 
> Thanks for your work on this, a few comments inline
> 
> 
> On 07/20/2016 02:40 AM, Peter Chen wrote:
> 
> >...
> >
> >+static int pwrseq_generic_on(struct device_node *np, struct pwrseq *pwrseq)
> >+{
> >
> >...
> >
> >+	if (gpiod_reset) {
> >+		u32 duration_us = 50;
> >+
> >+		of_property_read_u32(np, "reset-duration-us",
> >+				&duration_us);
> >+		usleep_range(duration_us, duration_us + 10);
> The end of the range could allow for more margin. Also consider busy
> looping for very short delays as in
> http://lxr.free-electrons.com/source/drivers/regulator/core.c#L2062

Thanks, I will change it.

> >...
> >
> >+static int pwrseq_generic_get(struct device_node *np, struct pwrseq *pwrseq)
> >+{
> >+	struct pwrseq_generic *pwrseq_gen = to_generic_pwrseq(pwrseq);
> >+	enum of_gpio_flags flags;
> >+	int reset_gpio, ret = 0;
> >+
> >+	pwrseq_gen->clk = of_clk_get_by_name(np, NULL);
> This only gets the first of potentially multiple clocks, is that intended?

Since it is ran before the driver's probe, we thought one clock for
power sequence is enough. If your case really needs several clocks
to be enabled before your device can be found by bus, let me know.
I will add support for it. But what are the name for clocks, since
it is generic library? "gen1, gen2 and gen3"?

-- 

Best Regards,
Peter Chento 

[toc] | [prev] | [next] | [standalone]

#1453572

On Fri, Jul 29, 2016 at 01:06:48PM -0700, Matthias Kaehlcke wrote:
> >...
> >
> >+static int pwrseq_generic_get(struct device_node *np, struct pwrseq *pwrseq)
> >+{
> >+	struct pwrseq_generic *pwrseq_gen = to_generic_pwrseq(pwrseq);
> >+	enum of_gpio_flags flags;
> >+	int reset_gpio, ret = 0;
> >+
> >+	pwrseq_gen->clk = of_clk_get_by_name(np, NULL);
> This only gets the first of potentially multiple clocks, is that intended?

Matthias, I have added multiple input clocks support at v4 patch set,
and you are at cc list.

-- 

Best Regards,
Peter Chen

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web