| Message ID | 1456932729-9667-10-git-send-email-lee.jones@linaro.org |
|---|---|
| State | New |
| Headers | show |
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
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
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); + + /* 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); + + /* + * 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); + + 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)); + + /* Calculate the duty cycle */ + dc = CPT_DC_MAX * high / (high + low); + + ret = sprintf(buf, "%u:%u\n", f, dc); + default: + dev_err(dev, "Internal error\n"); + } + +out: + mutex_unlock(&d->lock); + return ret; +} + static const struct pwm_ops sti_pwm_ops = { + .capture = sti_pwm_capture, .config = sti_pwm_config, .enable = sti_pwm_enable, .disable = sti_pwm_disable,
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(+) -- 1.9.1