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

On Tue, 12 Apr 2016, Thierry Reding wrote:

On Wed, Mar 02, 2016 at 03:32:06PM +0000, Lee Jones wrote:
[...]

quoted

diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c

[...]

quoted

+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.

quoted

+
+		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?

quoted

+
+		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.

quoted

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