From: thierry.reding@gmail•com (Thierry Reding)
To: linux-arm-kernel@lists•infradead.org
Subject: [RESEND 09/11] pwm: sti: Add PWM Capture call-back
Date: Wed, 13 Apr 2016 17:22:29 +0200 [thread overview]
Message-ID: <20160413152229.GC29509@ulmo.ba.sec> (raw)
In-Reply-To: <20160413102554.GQ8094@x1>
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160413/82f89061/attachment.sig>
next prev parent reply other threads:[~2016-04-13 15:22 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-03-02 15:31 [RESEND 00/11] pwm: Add support for PWM Capture Lee Jones
2016-03-02 15:31 ` [RESEND 01/11] pwm: Add PWM Capture support Lee Jones
2016-04-12 10:08 ` Thierry Reding
2016-04-13 9:36 ` Lee Jones
2016-04-13 14:50 ` Thierry Reding
2016-03-02 15:32 ` [RESEND 02/11] pwm: sysfs: " Lee Jones
2016-04-12 10:15 ` Thierry Reding
2016-04-13 9:40 ` Lee Jones
2016-03-02 15:32 ` [RESEND 03/11] pwm: sti: Reorganise register names in preparation for new functionality Lee Jones
2016-03-02 15:32 ` [RESEND 04/11] pwm: sti: Only request clock rate when you need to Lee Jones
2016-03-02 15:32 ` [RESEND 05/11] pwm: sti: Supply PWM Capture register addresses and bit locations Lee Jones
2016-03-02 15:32 ` [RESEND 06/11] pwm: sti: Supply PWM Capture clock handling Lee Jones
2016-03-02 15:32 ` [RESEND 07/11] pwm: sti: Initialise PWM Capture channel data Lee Jones
2016-04-12 10:29 ` Thierry Reding
2016-04-15 12:39 ` Lee Jones
2016-04-15 14:22 ` Thierry Reding
2016-04-15 14:31 ` Lee Jones
2016-04-15 13:11 ` Lee Jones
2016-04-15 14:23 ` Thierry Reding
2016-03-02 15:32 ` [RESEND 08/11] pwm: sti: Add support for PWM Capture IRQs Lee Jones
2016-04-12 10:35 ` Thierry Reding
2016-04-13 10:05 ` Lee Jones
2016-04-13 15:16 ` Thierry Reding
2016-03-02 15:32 ` [RESEND 09/11] pwm: sti: Add PWM Capture call-back Lee Jones
2016-04-12 10:53 ` Thierry Reding
2016-04-13 10:25 ` Lee Jones
2016-04-13 15:22 ` Thierry Reding [this message]
2016-04-15 8:29 ` Lee Jones
2016-04-15 14:20 ` Thierry Reding
2016-03-02 15:32 ` [RESEND 10/11] pwm: sti: Enable PWM Capture Lee Jones
2016-04-12 10:56 ` Thierry Reding
2016-03-02 15:32 ` [RESEND 11/11] pwm: sti: Take the opportunity to conduct a little house keeping Lee Jones
2016-04-12 7:28 ` [RESEND 00/11] pwm: Add support for PWM Capture Lee Jones
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160413152229.GC29509@ulmo.ba.sec \
--to=thierry.reding@gmail$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox