Messages in this thread Patch in this message | | | Date | Tue, 12 Apr 2016 12:35:21 +0200 | From | Thierry Reding <> | Subject | Re: [RESEND 08/11] pwm: sti: Add support for PWM Capture IRQs |
| |
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?
> + > + 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.
> + > + 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.
> + > + ret = devm_request_irq(&pdev->dev, irq, sti_pwm_interrupt, > + 0, pdev->name, (void *) pc);
No need for the explicit cast to void *.
Thierry [unhandled content-type:application/pgp-signature] | |