[RESEND,1/2] gpio: mvebu: Add support for multiple PWM lines per GPIO chip

Message ID 1533522556-55055-2-git-send-email-aditya@kobol.io
State New
Headers show
Series
  • gpio: mvebu: Add support for multiple PWM lines
Related show

Commit Message

Aditya Prayoga Aug. 6, 2018, 2:29 a.m.
Allow more than 1 PWM request (eg. PWM fan) on the same GPIO chip.

based on initial work on LK4.4 by Alban Browaeys.
URL: https://github.com/helios-4/linux-marvell/commit/743ae97
[Aditya Prayoga: forward port, cleanup]
Signed-off-by: Aditya Prayoga <aditya@kobol.io>
---
 drivers/gpio/gpio-mvebu.c | 63 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 41 insertions(+), 22 deletions(-)

Comments

Andrew Lunn Aug. 6, 2018, 3:38 a.m. | #1
On Mon, Aug 06, 2018 at 10:29:15AM +0800, Aditya Prayoga wrote:

Hi Aditya

> +	item = kzalloc(sizeof(*item), GFP_KERNEL);
> +	if (!item)
> +		return -ENODEV;

ENOMEM would be better, since it is a memory allocation which is
failing.

>  
> -		ret = gpiod_direction_output(desc, 0);
> -		if (ret) {
> -			gpiochip_free_own_desc(desc);
> -			goto out;
> -		}
> +	spin_lock_irqsave(&mvpwm->lock, flags);
> +	desc = gpiochip_request_own_desc(&mvchip->chip,
> +					 pwm->hwpwm, "mvebu-pwm");
> +	if (IS_ERR(desc)) {
> +		ret = PTR_ERR(desc);
> +		goto out;
> +	}
>  
> -		mvpwm->gpiod = desc;
> +	ret = gpiod_direction_output(desc, 0);
> +	if (ret) {
> +		gpiochip_free_own_desc(desc);
> +		goto out;
>  	}
> +	item->gpiod = desc;
> +	item->device = pwm;
> +	INIT_LIST_HEAD(&item->node);
> +	list_add_tail(&item->node, &mvpwm->pwms);
>  out:
>  	spin_unlock_irqrestore(&mvpwm->lock, flags);
>  	return ret;

You don't cleanup item on the error path.

> @@ -630,12 +639,20 @@ static int mvebu_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
>  static void mvebu_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
>  	struct mvebu_pwm *mvpwm = to_mvebu_pwm(chip);
> +	struct mvebu_pwm_item *item, *tmp;
>  	unsigned long flags;
>  
> -	spin_lock_irqsave(&mvpwm->lock, flags);
> -	gpiochip_free_own_desc(mvpwm->gpiod);
> -	mvpwm->gpiod = NULL;
> -	spin_unlock_irqrestore(&mvpwm->lock, flags);
> +	list_for_each_entry_safe(item, tmp, &mvpwm->pwms, node) {
> +		if (item->device == pwm) {
> +			spin_lock_irqsave(&mvpwm->lock, flags);
> +			gpiochip_free_own_desc(item->gpiod);
> +			item->gpiod = NULL;
> +			item->device = NULL;

Since you are about to free item, these two lines are pointless.

> +			list_del(&item->node);
> +			spin_unlock_irqrestore(&mvpwm->lock, flags);
> +			kfree(item);
> +		}
> +	}
>  }

   Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Aditya Prayoga Aug. 8, 2018, 10:27 a.m. | #2
On Mon, Aug 6, 2018 at 10:38 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Mon, Aug 06, 2018 at 10:29:15AM +0800, Aditya Prayoga wrote:
>
> Hi Aditya
>
> > +     item = kzalloc(sizeof(*item), GFP_KERNEL);
> > +     if (!item)
> > +             return -ENODEV;
>
> ENOMEM would be better, since it is a memory allocation which is
> failing.
>
> >
> > -             ret = gpiod_direction_output(desc, 0);
> > -             if (ret) {
> > -                     gpiochip_free_own_desc(desc);
> > -                     goto out;
> > -             }
> > +     spin_lock_irqsave(&mvpwm->lock, flags);
> > +     desc = gpiochip_request_own_desc(&mvchip->chip,
> > +                                      pwm->hwpwm, "mvebu-pwm");
> > +     if (IS_ERR(desc)) {
> > +             ret = PTR_ERR(desc);
> > +             goto out;
> > +     }
> >
> > -             mvpwm->gpiod = desc;
> > +     ret = gpiod_direction_output(desc, 0);
> > +     if (ret) {
> > +             gpiochip_free_own_desc(desc);
> > +             goto out;
> >       }
> > +     item->gpiod = desc;
> > +     item->device = pwm;
> > +     INIT_LIST_HEAD(&item->node);
> > +     list_add_tail(&item->node, &mvpwm->pwms);
> >  out:
> >       spin_unlock_irqrestore(&mvpwm->lock, flags);
> >       return ret;
>
> You don't cleanup item on the error path.
>
> > @@ -630,12 +639,20 @@ static int mvebu_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> >  static void mvebu_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> >  {
> >       struct mvebu_pwm *mvpwm = to_mvebu_pwm(chip);
> > +     struct mvebu_pwm_item *item, *tmp;
> >       unsigned long flags;
> >
> > -     spin_lock_irqsave(&mvpwm->lock, flags);
> > -     gpiochip_free_own_desc(mvpwm->gpiod);
> > -     mvpwm->gpiod = NULL;
> > -     spin_unlock_irqrestore(&mvpwm->lock, flags);
> > +     list_for_each_entry_safe(item, tmp, &mvpwm->pwms, node) {
> > +             if (item->device == pwm) {
> > +                     spin_lock_irqsave(&mvpwm->lock, flags);
> > +                     gpiochip_free_own_desc(item->gpiod);
> > +                     item->gpiod = NULL;
> > +                     item->device = NULL;
>
> Since you are about to free item, these two lines are pointless.
>
> > +                     list_del(&item->node);
> > +                     spin_unlock_irqrestore(&mvpwm->lock, flags);
> > +                     kfree(item);
> > +             }
> > +     }
> >  }
>
>    Andrew

Hi Andrew,

Thanks, I will fix it on next version.

Regards,

Aditya
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Aug. 29, 2018, 7:54 a.m. | #3
On Mon, Aug 6, 2018 at 4:31 AM Aditya Prayoga <aditya@kobol.io> wrote:

> Allow more than 1 PWM request (eg. PWM fan) on the same GPIO chip.
>
> based on initial work on LK4.4 by Alban Browaeys.
> URL: https://github.com/helios-4/linux-marvell/commit/743ae97
> [Aditya Prayoga: forward port, cleanup]
> Signed-off-by: Aditya Prayoga <aditya@kobol.io>

It would be awesome to get some feedback from the MVEBU maintainers
on this patch set.

Who are most active on Marvell stuff these days? Thomas?

Likewise I'd be very grateful for a nod from the PWM maintainer that
this is OK with him.

Yours,
Linus Walleij
Thomas Petazzoni Aug. 29, 2018, 8:02 a.m. | #4
Hello Linus,

On Wed, 29 Aug 2018 09:54:04 +0200, Linus Walleij wrote:
> On Mon, Aug 6, 2018 at 4:31 AM Aditya Prayoga <aditya@kobol.io> wrote:
> 
> > Allow more than 1 PWM request (eg. PWM fan) on the same GPIO chip.
> >
> > based on initial work on LK4.4 by Alban Browaeys.
> > URL: https://github.com/helios-4/linux-marvell/commit/743ae97
> > [Aditya Prayoga: forward port, cleanup]
> > Signed-off-by: Aditya Prayoga <aditya@kobol.io>  
> 
> It would be awesome to get some feedback from the MVEBU maintainers
> on this patch set.
> 
> Who are most active on Marvell stuff these days? Thomas?

Andrew Lunn did the initial support for PWM in this driver, and he
outlined in the commit log the limitation of his first implementation:

    However, there are only two sets of PWM configuration registers for
    all the GPIO lines. This driver simply allows a single GPIO line per
    GPIO chip of 32 lines to be used as a PWM. Attempts to use more
    return EBUSY.

Andrew, perhaps you could review the patch posted by Aditya, since you
already looked at PWM support on mvebu platforms ?

Thanks!

Thomas
Gregory CLEMENT Aug. 29, 2018, 8:13 a.m. | #5
Hi Linus,
 
 On mer., août 29 2018, Linus Walleij <linus.walleij@linaro.org> wrote:

> On Mon, Aug 6, 2018 at 4:31 AM Aditya Prayoga <aditya@kobol.io> wrote:
>
>> Allow more than 1 PWM request (eg. PWM fan) on the same GPIO chip.
>>
>> based on initial work on LK4.4 by Alban Browaeys.
>> URL: https://github.com/helios-4/linux-marvell/commit/743ae97
>> [Aditya Prayoga: forward port, cleanup]
>> Signed-off-by: Aditya Prayoga <aditya@kobol.io>
>
> It would be awesome to get some feedback from the MVEBU maintainers
> on this patch set.

There already has been reviewed from Andrew and also from Richard who
worked on the PWM part too. There were many questions raised, but no
feedback yet, so for now this patch set is clearly not ready to be
merged. We are waiting for answers and a new version.

Gregory

>
> Who are most active on Marvell stuff these days? Thomas?
>
> Likewise I'd be very grateful for a nod from the PWM maintainer that
> this is OK with him.
>
> Yours,
> Linus Walleij
Linus Walleij Aug. 29, 2018, 12:09 p.m. | #6
On Wed, Aug 29, 2018 at 10:02 AM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:

> Andrew Lunn did the initial support for PWM in this driver, and he
> outlined in the commit log the limitation of his first implementation:
>
>     However, there are only two sets of PWM configuration registers for
>     all the GPIO lines. This driver simply allows a single GPIO line per
>     GPIO chip of 32 lines to be used as a PWM. Attempts to use more
>     return EBUSY.
>
> Andrew, perhaps you could review the patch posted by Aditya, since you
> already looked at PWM support on mvebu platforms ?

Thanks yes Andrew is involved, I will wait for Andrew's definitive ACK
before merging any of this.

Yours,
Linus Walleij
Andrew Lunn Aug. 29, 2018, 12:52 p.m. | #7
> Thanks yes Andrew is involved, I will wait for Andrew's definitive ACK
> before merging any of this.

Hi Linus

Given the review comments i made so far, it has a NACK.

I will happily review the next version.

  Andrew

Patch

diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
index 6e02148..0617e66 100644
--- a/drivers/gpio/gpio-mvebu.c
+++ b/drivers/gpio/gpio-mvebu.c
@@ -92,10 +92,17 @@ 
 
 #define MVEBU_MAX_GPIO_PER_BANK		32
 
+struct mvebu_pwm_item {
+	struct gpio_desc	*gpiod;
+	struct pwm_device	*device;
+	struct list_head	 node;
+};
+
 struct mvebu_pwm {
 	void __iomem		*membase;
 	unsigned long		 clk_rate;
-	struct gpio_desc	*gpiod;
+	int	 id;
+	struct list_head	 pwms;
 	struct pwm_chip		 chip;
 	spinlock_t		 lock;
 	struct mvebu_gpio_chip	*mvchip;
@@ -599,29 +606,31 @@  static int mvebu_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 	struct mvebu_pwm *mvpwm = to_mvebu_pwm(chip);
 	struct mvebu_gpio_chip *mvchip = mvpwm->mvchip;
 	struct gpio_desc *desc;
+	struct mvebu_pwm_item *item;
 	unsigned long flags;
 	int ret = 0;
 
-	spin_lock_irqsave(&mvpwm->lock, flags);
-
-	if (mvpwm->gpiod) {
-		ret = -EBUSY;
-	} else {
-		desc = gpiochip_request_own_desc(&mvchip->chip,
-						 pwm->hwpwm, "mvebu-pwm");
-		if (IS_ERR(desc)) {
-			ret = PTR_ERR(desc);
-			goto out;
-		}
+	item = kzalloc(sizeof(*item), GFP_KERNEL);
+	if (!item)
+		return -ENODEV;
 
-		ret = gpiod_direction_output(desc, 0);
-		if (ret) {
-			gpiochip_free_own_desc(desc);
-			goto out;
-		}
+	spin_lock_irqsave(&mvpwm->lock, flags);
+	desc = gpiochip_request_own_desc(&mvchip->chip,
+					 pwm->hwpwm, "mvebu-pwm");
+	if (IS_ERR(desc)) {
+		ret = PTR_ERR(desc);
+		goto out;
+	}
 
-		mvpwm->gpiod = desc;
+	ret = gpiod_direction_output(desc, 0);
+	if (ret) {
+		gpiochip_free_own_desc(desc);
+		goto out;
 	}
+	item->gpiod = desc;
+	item->device = pwm;
+	INIT_LIST_HEAD(&item->node);
+	list_add_tail(&item->node, &mvpwm->pwms);
 out:
 	spin_unlock_irqrestore(&mvpwm->lock, flags);
 	return ret;
@@ -630,12 +639,20 @@  static int mvebu_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 static void mvebu_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct mvebu_pwm *mvpwm = to_mvebu_pwm(chip);
+	struct mvebu_pwm_item *item, *tmp;
 	unsigned long flags;
 
-	spin_lock_irqsave(&mvpwm->lock, flags);
-	gpiochip_free_own_desc(mvpwm->gpiod);
-	mvpwm->gpiod = NULL;
-	spin_unlock_irqrestore(&mvpwm->lock, flags);
+	list_for_each_entry_safe(item, tmp, &mvpwm->pwms, node) {
+		if (item->device == pwm) {
+			spin_lock_irqsave(&mvpwm->lock, flags);
+			gpiochip_free_own_desc(item->gpiod);
+			item->gpiod = NULL;
+			item->device = NULL;
+			list_del(&item->node);
+			spin_unlock_irqrestore(&mvpwm->lock, flags);
+			kfree(item);
+		}
+	}
 }
 
 static void mvebu_pwm_get_state(struct pwm_chip *chip,
@@ -804,6 +821,8 @@  static int mvebu_pwm_probe(struct platform_device *pdev,
 		return -ENOMEM;
 	mvchip->mvpwm = mvpwm;
 	mvpwm->mvchip = mvchip;
+	mvpwm->id     = id;
+	INIT_LIST_HEAD(&mvpwm->pwms);
 
 	mvpwm->membase = devm_ioremap_resource(dev, res);
 	if (IS_ERR(mvpwm->membase))