| Message ID | 1456932729-9667-9-git-send-email-lee.jones@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
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
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.
On Wed, Apr 13, 2016 at 11:05:21AM +0100, Lee Jones wrote: > 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 [...] > > > + > > > + 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? I think the cast indicates that you're mixing unrelated interfaces. Also looking at patch 9/11 it seems like you'll need extra locking to protect against concurrent accesses in the interrupt handler and the ->capture() implementation anyway, so might as well use that lock for this access. > > > @@ -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. I think all the latter are really wrong. Looking at the implementation of platform_get_irq() there are a number of error codes that it can return (-EPROBE_DEFER amongst them), so collapsing them all into an -EINVAL, -ENODEV or -ENXIO is very wrong. Thierry
diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c index fca692a..82a69e4 100644 --- a/drivers/pwm/pwm-sti.c +++ b/drivers/pwm/pwm-sti.c @@ -12,6 +12,7 @@ #include <linux/clk.h> #include <linux/gpio.h> +#include <linux/interrupt.h> #include <linux/math64.h> #include <linux/mfd/syscon.h> #include <linux/module.h> @@ -94,7 +95,9 @@ struct sti_pwm_chip { struct regmap_field *prescale_low; struct regmap_field *prescale_high; struct regmap_field *pwm_out_en; + struct regmap_field *pwm_cpt_en; struct regmap_field *pwm_cpt_int_en; + struct regmap_field *pwm_cpt_int_stat; struct pwm_chip chip; struct pwm_device *cur; unsigned long configured; @@ -314,6 +317,74 @@ static const struct pwm_ops sti_pwm_ops = { .owner = THIS_MODULE, }; +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 + * + */ + + 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); + + 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; + } + + ret = devm_request_irq(&pdev->dev, irq, sti_pwm_interrupt, + 0, pdev->name, (void *) pc); + if (ret < 0) { + dev_err(&pdev->dev, "Failed to request IRQ\n"); + return ret; + } + /* * Setup PWM data with default values: some values could be replaced * with specific ones provided from Device Tree.
Here we're requesting the PWM Capture IRQ and supplying the handler which will be called in the event of an IRQ fire to handle it. Signed-off-by: Lee Jones <lee.jones@linaro.org> --- drivers/pwm/pwm-sti.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 90 insertions(+), 1 deletion(-)