Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1376670 > unrolled thread

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

Started byThierry Reding <thierry.reding@gmail.com>
First post2016-04-12 12:40 +0200
Last post2016-04-13 17:20 +0200
Articles 3 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RESEND 08/11] pwm: sti: Add support for PWM Capture IRQs Thierry Reding <thierry.reding@gmail.com> - 2016-04-12 12:40 +0200
    Re: [RESEND 08/11] pwm: sti: Add support for PWM Capture IRQs Lee Jones <lee.jones@linaro.org> - 2016-04-13 12:10 +0200
      Re: [RESEND 08/11] pwm: sti: Add support for PWM Capture IRQs Thierry Reding <thierry.reding@gmail.com> - 2016-04-13 17:20 +0200

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

FromThierry Reding <thierry.reding@gmail.com>
Date2016-04-12 12:40 +0200
SubjectRe: [RESEND 08/11] pwm: sti: Add support for PWM Capture IRQs
Message-ID<rn3TI-5Z0-29@gated-at.bofh.it>

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [next] | [standalone]

#1377721

FromLee Jones <lee.jones@linaro.org>
Date2016-04-13 12:10 +0200
Message-ID<rnpUe-86v-13@gated-at.bofh.it>
In reply to#1376670
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

[toc] | [prev] | [next] | [standalone]

#1378034

FromThierry Reding <thierry.reding@gmail.com>
Date2016-04-13 17:20 +0200
Message-ID<rnuKe-3cP-13@gated-at.bofh.it>
In reply to#1377721

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web