Re: [RESEND 08/11] pwm: sti: Add support for PWM Capture IRQs

[Lee Jones]

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);
> > +			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