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

Groups > linux.kernel > #1376698 > unrolled thread

Re: [RESEND 09/11] pwm: sti: Add PWM Capture call-back

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: [RESEND 09/11] pwm: sti: Add PWM Capture call-back Thierry Reding <thierry.reding@gmail.com> - 2016-04-12 13:00 +0200
    Re: [RESEND 09/11] pwm: sti: Add PWM Capture call-back Lee Jones <lee.jones@linaro.org> - 2016-04-13 12:30 +0200
      Re: [RESEND 09/11] pwm: sti: Add PWM Capture call-back Thierry Reding <thierry.reding@gmail.com> - 2016-04-13 17:30 +0200
        Re: [RESEND 09/11] pwm: sti: Add PWM Capture call-back Lee Jones <lee.jones@linaro.org> - 2016-04-15 10:30 +0200
          Re: [RESEND 09/11] pwm: sti: Add PWM Capture call-back Thierry Reding <thierry.reding@gmail.com> - 2016-04-15 16:30 +0200

#1376698 — Re: [RESEND 09/11] pwm: sti: Add PWM Capture call-back

[Multipart message — attachments visible in raw view] — view raw

On Wed, Mar 02, 2016 at 03:32:07PM +0000, Lee Jones wrote:
> Once a PWM Capture has been initiated, the capture call
> enables a rising edge detection IRQ, then waits.  Once each
> of the 3 phase changes have been recorded the thread then
> wakes.  The remaining part of the call carries out the
> relevant calculations and passes back a formatted string to
> the caller.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/pwm/pwm-sti.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
> index 82a69e4..8de9b4a 100644
> --- a/drivers/pwm/pwm-sti.c
> +++ b/drivers/pwm/pwm-sti.c
> @@ -309,7 +309,79 @@ static void sti_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
>  	clear_bit(pwm->hwpwm, &pc->configured);
>  }
>  
> +static int sti_pwm_capture(struct pwm_chip *chip, struct pwm_device *pwm,
> +			   int channel, char *buf)
> +{
> +	struct sti_pwm_chip *pc = to_sti_pwmchip(chip);
> +	struct sti_pwm_compat_data *cdata = pc->cdata;
> +	struct sti_cpt_data *d = pc->cpt_data[channel];
> +	struct device *dev = pc->dev;
> +	unsigned int f, dc;
> +	unsigned int high, low;
> +	bool level;
> +	int ret;
> +
> +	if (channel > cdata->cpt_num_chan - 1) {
> +		dev_err(dev, "Channel %d is not valid\n", channel);
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&d->lock);

Should this perhaps reuse the struct pwm_device's ->lock?

> +
> +	/* Prepare capture measurement */
> +	d->index = 0;
> +	regmap_write(pc->regmap, PWM_CPT_EDGE(channel), CPT_EDGE_RISING);
> +	regmap_field_write(pc->pwm_cpt_int_en, BIT(channel));
> +	ret = wait_event_interruptible_timeout(d->wait, d->index > 1, HZ);

The timeout here should make sure callers don't hang forever. But maybe
you can still make sure that when the PWM gets disabled the wait queue
is woken and perhaps return an appropriate error code to let users know
that the operation was interrupted.

Also, how about letting callers choose the value of the timeout? In some
cases they may be interested in long-running signals. In other cases the
whole second timeout may be much too long.

> +
> +	/*
> +	 * In case we woke up for another reason than completion
> +	 * make sure to disable the capture.
> +	 */
> +	regmap_write(pc->regmap, PWM_CPT_EDGE(channel), CPT_EDGE_DISABLED);

The comment here is slightly confusing because it implies that disabling
the capture should be done conditionally, whereas it is always disabled.

> +
> +	if (ret == -ERESTARTSYS)
> +		goto out;
> +
> +	switch (d->index) {
> +	case 0:
> +	case 1:
> +		/*
> +		 * Getting here could mean :
> +		 *  - input signal is constant of less than 1Hz
> +		 *  - there is no input signal at all
> +		 *
> +		 * In such case the frequency is rounded down to 0
> +		 * level of the supposed constant signal is reported
> +		 * using duty cycle min and max values.
> +		 */
> +		level = gpio_get_value(d->gpio);
> +
> +		ret = sprintf(buf, "0:%u\n", level ? CPT_DC_MAX : 0);
> +		break;
> +	case 2:
> +		/* We have evertying we need */
> +		high = d->snapshot[1] - d->snapshot[0];
> +		low  = d->snapshot[2] - d->snapshot[1];
> +
> +		/* Calculate frequency in Hz */
> +		f = clk_get_rate(pc->cpt_clk) / (1 * (high + low));

The multiplication by 1 is unnecessary.

Thierry

[toc] | [next] | [standalone]

#1377739

On Tue, 12 Apr 2016, Thierry Reding wrote:

> On Wed, Mar 02, 2016 at 03:32:07PM +0000, Lee Jones wrote:
> > Once a PWM Capture has been initiated, the capture call
> > enables a rising edge detection IRQ, then waits.  Once each
> > of the 3 phase changes have been recorded the thread then
> > wakes.  The remaining part of the call carries out the
> > relevant calculations and passes back a formatted string to
> > the caller.
> > 
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  drivers/pwm/pwm-sti.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 72 insertions(+)
> > 
> > diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
> > index 82a69e4..8de9b4a 100644
> > --- a/drivers/pwm/pwm-sti.c
> > +++ b/drivers/pwm/pwm-sti.c
> > @@ -309,7 +309,79 @@ static void sti_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> >  	clear_bit(pwm->hwpwm, &pc->configured);
> >  }
> >  
> > +static int sti_pwm_capture(struct pwm_chip *chip, struct pwm_device *pwm,
> > +			   int channel, char *buf)
> > +{
> > +	struct sti_pwm_chip *pc = to_sti_pwmchip(chip);
> > +	struct sti_pwm_compat_data *cdata = pc->cdata;
> > +	struct sti_cpt_data *d = pc->cpt_data[channel];
> > +	struct device *dev = pc->dev;
> > +	unsigned int f, dc;
> > +	unsigned int high, low;
> > +	bool level;
> > +	int ret;
> > +
> > +	if (channel > cdata->cpt_num_chan - 1) {
> > +		dev_err(dev, "Channel %d is not valid\n", channel);
> > +		return -EINVAL;
> > +	}
> > +
> > +	mutex_lock(&d->lock);
> 
> Should this perhaps reuse the struct pwm_device's ->lock?
> 
> > +
> > +	/* Prepare capture measurement */
> > +	d->index = 0;
> > +	regmap_write(pc->regmap, PWM_CPT_EDGE(channel), CPT_EDGE_RISING);
> > +	regmap_field_write(pc->pwm_cpt_int_en, BIT(channel));
> > +	ret = wait_event_interruptible_timeout(d->wait, d->index > 1, HZ);
> 
> The timeout here should make sure callers don't hang forever. But maybe
> you can still make sure that when the PWM gets disabled the wait queue
> is woken and perhaps return an appropriate error code to let users know
> that the operation was interrupted.

Sure.  I'll look into that.

> Also, how about letting callers choose the value of the timeout? In some
> cases they may be interested in long-running signals. In other cases the
> whole second timeout may be much too long.

I'm not opposed to it.  How do you suggest we do that?

> > +	/*
> > +	 * In case we woke up for another reason than completion
> > +	 * make sure to disable the capture.
> > +	 */
> > +	regmap_write(pc->regmap, PWM_CPT_EDGE(channel), CPT_EDGE_DISABLED);
> 
> The comment here is slightly confusing because it implies that disabling
> the capture should be done conditionally, whereas it is always disabled.

Not really.  We do it unconditionally for reason explained.

It says:

  "disable the capture just in case X happens"

rather than

  "disable the capture if X happens".

Perhaps the language is too subtle.  I can reword for clarity.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

#1378046

[Multipart message — attachments visible in raw view] — view raw

On Wed, Apr 13, 2016 at 11:25:54AM +0100, Lee Jones wrote:
> On Tue, 12 Apr 2016, Thierry Reding wrote:
> 
> > On Wed, Mar 02, 2016 at 03:32:07PM +0000, Lee Jones wrote:
> > > Once a PWM Capture has been initiated, the capture call
> > > enables a rising edge detection IRQ, then waits.  Once each
> > > of the 3 phase changes have been recorded the thread then
> > > wakes.  The remaining part of the call carries out the
> > > relevant calculations and passes back a formatted string to
> > > the caller.
> > > 
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > ---
> > >  drivers/pwm/pwm-sti.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 72 insertions(+)
> > > 
> > > diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
> > > index 82a69e4..8de9b4a 100644
> > > --- a/drivers/pwm/pwm-sti.c
> > > +++ b/drivers/pwm/pwm-sti.c
> > > @@ -309,7 +309,79 @@ static void sti_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> > >  	clear_bit(pwm->hwpwm, &pc->configured);
> > >  }
> > >  
> > > +static int sti_pwm_capture(struct pwm_chip *chip, struct pwm_device *pwm,
> > > +			   int channel, char *buf)
> > > +{
> > > +	struct sti_pwm_chip *pc = to_sti_pwmchip(chip);
> > > +	struct sti_pwm_compat_data *cdata = pc->cdata;
> > > +	struct sti_cpt_data *d = pc->cpt_data[channel];
> > > +	struct device *dev = pc->dev;
> > > +	unsigned int f, dc;
> > > +	unsigned int high, low;
> > > +	bool level;
> > > +	int ret;
> > > +
> > > +	if (channel > cdata->cpt_num_chan - 1) {
> > > +		dev_err(dev, "Channel %d is not valid\n", channel);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	mutex_lock(&d->lock);
> > 
> > Should this perhaps reuse the struct pwm_device's ->lock?

That was actually a stupid suggestion by me, because that lock is being
removed in an unrelated patch series.

> > > +	/* Prepare capture measurement */
> > > +	d->index = 0;
> > > +	regmap_write(pc->regmap, PWM_CPT_EDGE(channel), CPT_EDGE_RISING);
> > > +	regmap_field_write(pc->pwm_cpt_int_en, BIT(channel));
> > > +	ret = wait_event_interruptible_timeout(d->wait, d->index > 1, HZ);
> > 
> > The timeout here should make sure callers don't hang forever. But maybe
> > you can still make sure that when the PWM gets disabled the wait queue
> > is woken and perhaps return an appropriate error code to let users know
> > that the operation was interrupted.
> 
> Sure.  I'll look into that.
> 
> > Also, how about letting callers choose the value of the timeout? In some
> > cases they may be interested in long-running signals. In other cases the
> > whole second timeout may be much too long.
> 
> I'm not opposed to it.  How do you suggest we do that?

The easiest would probably be to add an unsigned long timeout parameter
to the pwm_capture() function and ->capture() callbacks.

But thinking about this further I'm wondering if it might not be easier
and more flexible to move the timeout completely outside of this code
and into callers. I suspect that the most simple way to do that would be
to add a completion to struct pwm_capture that callers can use to wait
for completion of a capture. This would make the whole process
asynchronous and allow interesting things like making the sysfs capture
file pollable, for example.

> > > +	/*
> > > +	 * In case we woke up for another reason than completion
> > > +	 * make sure to disable the capture.
> > > +	 */
> > > +	regmap_write(pc->regmap, PWM_CPT_EDGE(channel), CPT_EDGE_DISABLED);
> > 
> > The comment here is slightly confusing because it implies that disabling
> > the capture should be done conditionally, whereas it is always disabled.
> 
> Not really.  We do it unconditionally for reason explained.
> 
> It says:
> 
>   "disable the capture just in case X happens"
> 
> rather than
> 
>   "disable the capture if X happens".
> 
> Perhaps the language is too subtle.  I can reword for clarity.

I'd be okay with just dropping the comment altogether, it seems rather
obvious to me. But clarifying is okay with me, too.

Thierry

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

#1379547

On Wed, 13 Apr 2016, Thierry Reding wrote:

> On Wed, Apr 13, 2016 at 11:25:54AM +0100, Lee Jones wrote:
> > On Tue, 12 Apr 2016, Thierry Reding wrote:
> > 
> > > On Wed, Mar 02, 2016 at 03:32:07PM +0000, Lee Jones wrote:
> > > > Once a PWM Capture has been initiated, the capture call
> > > > enables a rising edge detection IRQ, then waits.  Once each
> > > > of the 3 phase changes have been recorded the thread then
> > > > wakes.  The remaining part of the call carries out the
> > > > relevant calculations and passes back a formatted string to
> > > > the caller.
> > > > 
> > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > ---
> > > >  drivers/pwm/pwm-sti.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 72 insertions(+)
> > > > 
> > > > diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
> > > > index 82a69e4..8de9b4a 100644
> > > > --- a/drivers/pwm/pwm-sti.c
> > > > +++ b/drivers/pwm/pwm-sti.c

[...]

> > > > +	/* Prepare capture measurement */
> > > > +	d->index = 0;
> > > > +	regmap_write(pc->regmap, PWM_CPT_EDGE(channel), CPT_EDGE_RISING);
> > > > +	regmap_field_write(pc->pwm_cpt_int_en, BIT(channel));
> > > > +	ret = wait_event_interruptible_timeout(d->wait, d->index > 1, HZ);
> > > 
> > > The timeout here should make sure callers don't hang forever. But maybe
> > > you can still make sure that when the PWM gets disabled the wait queue
> > > is woken and perhaps return an appropriate error code to let users know
> > > that the operation was interrupted.
> > 
> > Sure.  I'll look into that.
> > 
> > > Also, how about letting callers choose the value of the timeout? In some
> > > cases they may be interested in long-running signals. In other cases the
> > > whole second timeout may be much too long.
> > 
> > I'm not opposed to it.  How do you suggest we do that?
> 
> The easiest would probably be to add an unsigned long timeout parameter
> to the pwm_capture() function and ->capture() callbacks.
> 
> But thinking about this further I'm wondering if it might not be easier
> and more flexible to move the timeout completely outside of this code
> and into callers. I suspect that the most simple way to do that would be
> to add a completion to struct pwm_capture that callers can use to wait
> for completion of a capture. This would make the whole process
> asynchronous and allow interesting things like making the sysfs capture
> file pollable, for example.

Okay, so how do you propose we handle this with sysfs?  Perhaps
another RW file to set it?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

#1379857

[Multipart message — attachments visible in raw view] — view raw

On Fri, Apr 15, 2016 at 09:29:00AM +0100, Lee Jones wrote:
> On Wed, 13 Apr 2016, Thierry Reding wrote:
> 
> > On Wed, Apr 13, 2016 at 11:25:54AM +0100, Lee Jones wrote:
> > > On Tue, 12 Apr 2016, Thierry Reding wrote:
> > > 
> > > > On Wed, Mar 02, 2016 at 03:32:07PM +0000, Lee Jones wrote:
> > > > > Once a PWM Capture has been initiated, the capture call
> > > > > enables a rising edge detection IRQ, then waits.  Once each
> > > > > of the 3 phase changes have been recorded the thread then
> > > > > wakes.  The remaining part of the call carries out the
> > > > > relevant calculations and passes back a formatted string to
> > > > > the caller.
> > > > > 
> > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > > ---
> > > > >  drivers/pwm/pwm-sti.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 72 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
> > > > > index 82a69e4..8de9b4a 100644
> > > > > --- a/drivers/pwm/pwm-sti.c
> > > > > +++ b/drivers/pwm/pwm-sti.c
> 
> [...]
> 
> > > > > +	/* Prepare capture measurement */
> > > > > +	d->index = 0;
> > > > > +	regmap_write(pc->regmap, PWM_CPT_EDGE(channel), CPT_EDGE_RISING);
> > > > > +	regmap_field_write(pc->pwm_cpt_int_en, BIT(channel));
> > > > > +	ret = wait_event_interruptible_timeout(d->wait, d->index > 1, HZ);
> > > > 
> > > > The timeout here should make sure callers don't hang forever. But maybe
> > > > you can still make sure that when the PWM gets disabled the wait queue
> > > > is woken and perhaps return an appropriate error code to let users know
> > > > that the operation was interrupted.
> > > 
> > > Sure.  I'll look into that.
> > > 
> > > > Also, how about letting callers choose the value of the timeout? In some
> > > > cases they may be interested in long-running signals. In other cases the
> > > > whole second timeout may be much too long.
> > > 
> > > I'm not opposed to it.  How do you suggest we do that?
> > 
> > The easiest would probably be to add an unsigned long timeout parameter
> > to the pwm_capture() function and ->capture() callbacks.
> > 
> > But thinking about this further I'm wondering if it might not be easier
> > and more flexible to move the timeout completely outside of this code
> > and into callers. I suspect that the most simple way to do that would be
> > to add a completion to struct pwm_capture that callers can use to wait
> > for completion of a capture. This would make the whole process
> > asynchronous and allow interesting things like making the sysfs capture
> > file pollable, for example.
> 
> Okay, so how do you propose we handle this with sysfs?  Perhaps
> another RW file to set it?

I'm unfamiliar with how this is done in other drivers, so I'd have to
look at them first. I suspect that it would be fine for now to simply
redesign the PWM API parts and keep some default timeout in sysfs. It
could be extended with some mechanism to override the default timeout
in the future.

Thierry

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web