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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3704 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]