On Tue, 12 Apr 2016, Thierry Reding wrote:
> On Wed, Mar 02, 2016 at 03:32:06PM +0000, Lee Jones wrote:
> [...]
> > diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
> [...]
> > +static irqreturn_t sti_pwm_interrupt(int irq, void *data)
> > +{
> > + struct sti_pwm_chip *pc = data;
> > + struct device *dev = pc->dev;
> > + struct sti_cpt_data *d;
> > + int channel;
> > + int cpt_int_stat;
> > + int reg;
> > + int ret = IRQ_NONE;
> > +
> > + ret = regmap_field_read(pc->pwm_cpt_int_stat, &cpt_int_stat);
> > + if (ret)
> > + return ret;
> > +
> > + while (cpt_int_stat) {
> > + channel = ffs(cpt_int_stat) - 1;
> > +
> > + d = pc->cpt_data[channel];
> > +
> > + /*
> > + * Capture input:
> > + * _______ _______
> > + * | | | |
> > + * __| |_________________| |________
> > + * ^0 ^1 ^2
> > + *
> > + * Capture start by the first available rising edge
> > + * When a capture event occurs, capture value (CPT_VALx)
> > + * is stored, index incremented, capture edge changed.
> > + *
> > + * After the capture, if the index > 1, we have collected
> > + * the necessary data so we signal the thread waiting for it
> > + * and disable the capture by setting capture edge to none
> > + *
> > + */
>
> How do you deal with the situation where someone will stop the PWM
> signal half-way in? That is, suppose you've got events for the first and
> second snapshots (0 and 1) and then someone stops the PWM and the event
> for snapshot 2 never happens, how does the code recover?
The 'wait' will timeout and the cycle will be reset.
> > +
> > + regmap_read(pc->regmap,
> > + PWM_CPT_VAL(channel), &d->snapshot[d->index]);
> > +
> > + switch (d->index) {
> > + case 0:
> > + case 1:
> > + regmap_read(pc->regmap, PWM_CPT_EDGE(channel), ®);
> > + reg ^= PWM_CPT_EDGE_MASK;
> > + regmap_write(pc->regmap, PWM_CPT_EDGE(channel), reg);
> > +
> > + d->index++;
> > + break;
> > + case 2:
> > + regmap_write(pc->regmap,
> > + PWM_CPT_EDGE(channel), CPT_EDGE_DISABLED);
> > + wake_up(&d->wait);
> > + break;
> > + default:
> > + dev_err(dev, "Internal error\n");
> > + }
> > +
> > + clear_bit(channel, (unsigned long int *)&cpt_int_stat);
>
> clear_bit() is a little unusual to use on regular data types, as
> evidenced by the need for the goofy cast here.
It's just a bit neater (and provides locking) than manually bit
twiddling using bitwise operators. What do you suggest?
> > +
> > + ret = IRQ_HANDLED;
> > + }
> > +
> > + /* Just ACK everything */
> > + regmap_write(pc->regmap, PWM_INT_ACK, PWM_INT_ACK_MASK);
> > +
> > + return ret;
> > +}
> > +
> > static int sti_pwm_probe_dt(struct sti_pwm_chip *pc)
> > {
> > struct device *dev = pc->dev;
> > @@ -354,6 +425,11 @@ static int sti_pwm_probe_dt(struct sti_pwm_chip *pc)
> > if (IS_ERR(pc->pwm_cpt_int_en))
> > return PTR_ERR(pc->pwm_cpt_int_en);
> >
> > + pc->pwm_cpt_int_stat = devm_regmap_field_alloc(dev, pc->regmap,
> > + reg_fields[PWM_CPT_INT_STAT]);
> > + if (IS_ERR(pc->pwm_cpt_int_stat))
> > + return PTR_ERR(pc->pwm_cpt_int_stat);
> > +
> > return 0;
> > }
> >
> > @@ -371,7 +447,7 @@ static int sti_pwm_probe(struct platform_device *pdev)
> > struct sti_pwm_chip *pc;
> > struct resource *res;
> > unsigned int chan;
> > - int ret;
> > + int ret, irq;
> >
> > pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL);
> > if (!pc)
> > @@ -392,6 +468,19 @@ static int sti_pwm_probe(struct platform_device *pdev)
> > if (IS_ERR(pc->regmap))
> > return PTR_ERR(pc->regmap);
> >
> > + irq = platform_get_irq(pdev, 0);
> > + if (irq < 0) {
> > + dev_err(&pdev->dev, "Failed to obtain IRQ\n");
> > + return -ENODEV;
> > + }
>
> I think you need to propagate the return value of platform_get_irq()
> here.
Yes, could do. Although, I think we could go either way:
$ git grep -A5 platform_get_irq | grep "return ret\|return irq" | wc -l
176
$ git grep -A5 platform_get_irq | grep "return -EINVAL\|-ENODEV\|-ENXIO" | wc -l
256
Happy to change it though.
> > +
> > + ret = devm_request_irq(&pdev->dev, irq, sti_pwm_interrupt,
> > + 0, pdev->name, (void *) pc);
>
> No need for the explicit cast to void *.
You're right. Will drop it.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
