[PATCH RESEND 0/2] gpio: mvebu: Add support for multiple PWM lines

[PATCH RESEND 0/2] gpio: mvebu: Add support for multiple PWM lines

[Aditya Prayoga]

Hi everyone,

Helios4, an Armada 388 based NAS SBC, provides 2 (4-pins) fan connectors.
The PWM pins on both connector are connected to GPIO on bank 1. Current gpio-
mvebu does not allow more than one PWM on the same bank.

Resend the patch to add more reviewer.

Aditya

---

Aditya Prayoga (2):
  gpio: mvebu: Add support for multiple PWM lines per GPIO chip
  gpio: mvebu: Allow to use non-default PWM counter

 drivers/gpio/gpio-mvebu.c | 111 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 92 insertions(+), 19 deletions(-)

-- 
2.7.4

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

[Aditya Prayoga]

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(-)

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))
-- 
2.7.4

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

[Andrew Lunn]

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

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

[Aditya Prayoga]

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

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

[Linus Walleij]

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

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

[Thomas Petazzoni]

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
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

[Linus Walleij]

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

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

[Andrew Lunn]

> 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

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

[Gregory CLEMENT]

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

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

[PATCH RESEND 2/2] gpio: mvebu: Allow to use non-default PWM counter

[Aditya Prayoga]

On multiple PWM lines, if the other PWM counter is unused, allocate it
to next PWM request. The priority would be:
1. Default counter assigned to the bank
2. Unused counter that is assigned to other bank
3. Fallback to default counter

For example on second bank there are three PWM request, first one would
use default counter (counter B), second one would try to use counter A,
and the third one would use counter B.

Signed-off-by: Aditya Prayoga <aditya@kobol.io>
---
 drivers/gpio/gpio-mvebu.c | 58 +++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 56 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
index 0617e66..5a39478 100644
--- a/drivers/gpio/gpio-mvebu.c
+++ b/drivers/gpio/gpio-mvebu.c
@@ -92,6 +92,11 @@
 
 #define MVEBU_MAX_GPIO_PER_BANK		32
 
+enum mvebu_pwm_counter {
+	MVEBU_PWM_COUNTER_A = 0,
+	MVEBU_PWM_COUNTER_B,
+};
+
 struct mvebu_pwm_item {
 	struct gpio_desc	*gpiod;
 	struct pwm_device	*device;
@@ -101,7 +106,8 @@ struct mvebu_pwm_item {
 struct mvebu_pwm {
 	void __iomem		*membase;
 	unsigned long		 clk_rate;
-	int	 id;
+	enum mvebu_pwm_counter	 id;
+	struct list_head	 node;
 	struct list_head	 pwms;
 	struct pwm_chip		 chip;
 	spinlock_t		 lock;
@@ -113,6 +119,8 @@ struct mvebu_pwm {
 	u32			 blink_off_duration;
 };
 
+static LIST_HEAD(mvebu_pwm_list);
+
 struct mvebu_gpio_chip {
 	struct gpio_chip   chip;
 	struct regmap     *regs;
@@ -601,12 +609,24 @@ static struct mvebu_pwm *to_mvebu_pwm(struct pwm_chip *chip)
 	return container_of(chip, struct mvebu_pwm, chip);
 }
 
+static struct mvebu_pwm *mvebu_pwm_get_avail_counter(void)
+{
+	struct mvebu_pwm *counter;
+
+	list_for_each_entry(counter, &mvebu_pwm_list, node) {
+		if (list_empty(&counter->pwms))
+			return counter;
+	}
+	return NULL;
+}
+
 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;
+	struct mvebu_pwm *counter;
 	unsigned long flags;
 	int ret = 0;
 
@@ -615,6 +635,14 @@ static int mvebu_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 		return -ENODEV;
 
 	spin_lock_irqsave(&mvpwm->lock, flags);
+	regmap_read(mvchip->regs, GPIO_BLINK_EN_OFF + mvchip->offset,
+		    &mvchip->blink_en_reg);
+
+	if (mvchip->blink_en_reg & BIT(pwm->hwpwm)) {
+		ret = -EBUSY;
+		goto out;
+	}
+
 	desc = gpiochip_request_own_desc(&mvchip->chip,
 					 pwm->hwpwm, "mvebu-pwm");
 	if (IS_ERR(desc)) {
@@ -627,10 +655,25 @@ static int mvebu_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 		gpiochip_free_own_desc(desc);
 		goto out;
 	}
+
+	counter = mvpwm;
+	if (!list_empty(&mvpwm->pwms)) {
+		counter = mvebu_pwm_get_avail_counter();
+		if (counter)
+			pwm->chip_data = counter;
+		else
+			counter = mvpwm;
+	}
+
+	regmap_update_bits(mvchip->regs, GPIO_BLINK_CNT_SELECT_OFF +
+				mvchip->offset,	BIT(pwm->hwpwm),
+				counter->id ? BIT(pwm->hwpwm) : 0);
+	regmap_read(mvchip->regs, GPIO_BLINK_CNT_SELECT_OFF +
+			mvchip->offset, &mvpwm->blink_select);
 	item->gpiod = desc;
 	item->device = pwm;
 	INIT_LIST_HEAD(&item->node);
-	list_add_tail(&item->node, &mvpwm->pwms);
+	list_add_tail(&item->node, &counter->pwms);
 out:
 	spin_unlock_irqrestore(&mvpwm->lock, flags);
 	return ret;
@@ -642,6 +685,9 @@ static void mvebu_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 	struct mvebu_pwm_item *item, *tmp;
 	unsigned long flags;
 
+	if (pwm->chip_data)
+		mvpwm = (struct mvebu_pwm *) pwm->chip_data;
+
 	list_for_each_entry_safe(item, tmp, &mvpwm->pwms, node) {
 		if (item->device == pwm) {
 			spin_lock_irqsave(&mvpwm->lock, flags);
@@ -665,6 +711,9 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip,
 	unsigned long flags;
 	u32 u;
 
+	if (pwm->chip_data)
+		mvpwm = (struct mvebu_pwm *) pwm->chip_data;
+
 	spin_lock_irqsave(&mvpwm->lock, flags);
 
 	val = (unsigned long long)
@@ -712,6 +761,9 @@ static int mvebu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	unsigned long flags;
 	unsigned int on, off;
 
+	if (pwm->chip_data)
+		mvpwm = (struct mvebu_pwm *) pwm->chip_data;
+
 	val = (unsigned long long) mvpwm->clk_rate * state->duty_cycle;
 	do_div(val, NSEC_PER_SEC);
 	if (val > UINT_MAX)
@@ -823,6 +875,7 @@ static int mvebu_pwm_probe(struct platform_device *pdev,
 	mvpwm->mvchip = mvchip;
 	mvpwm->id     = id;
 	INIT_LIST_HEAD(&mvpwm->pwms);
+	INIT_LIST_HEAD(&mvpwm->node);
 
 	mvpwm->membase = devm_ioremap_resource(dev, res);
 	if (IS_ERR(mvpwm->membase))
@@ -846,6 +899,7 @@ static int mvebu_pwm_probe(struct platform_device *pdev,
 	mvpwm->chip.base = -1;
 
 	spin_lock_init(&mvpwm->lock);
+	list_add_tail(&mvpwm->node, &mvebu_pwm_list);
 
 	return pwmchip_add(&mvpwm->chip);
 }
-- 
2.7.4

Re: [PATCH RESEND 2/2] gpio: mvebu: Allow to use non-default PWM counter

[Andrew Lunn]

On Mon, Aug 06, 2018 at 10:29:16AM +0800, Aditya Prayoga wrote:
> On multiple PWM lines, if the other PWM counter is unused, allocate it
> to next PWM request. The priority would be:
> 1. Default counter assigned to the bank
> 2. Unused counter that is assigned to other bank
> 3. Fallback to default counter
> 
> For example on second bank there are three PWM request, first one would
> use default counter (counter B), second one would try to use counter A,
> and the third one would use counter B.

Hi Aditya

There are only two PWM counters for all the GPIO lines. So you cannot
support 3 PWM requests. You have to enforce a maximum of two PWMs.

When i implemented this PWM code, i only needed one PWM. So it took
the easy option. GPIO bank 0 uses counter A, GPIO bank1 uses counter
B. For the hardware you have, this is not sufficient, so you need to
generalise this. Any PWM can use any counter, whatever is available
when the PWM is requested.

Rather than have a linked list of PWM, i think it would be better to
have a static array of two mvebu_pwm structures. Index 0 uses counter
A, index 1 uses counter B. You can then keep with the concept of
pwm->pgiod != NULL means the counter is in use. The request() call can
then find an unused PWM, set pwm->gpiod, and point mvchip->mvpwm to
one of the two static instances.

    Andrew

Re: [PATCH RESEND 2/2] gpio: mvebu: Allow to use non-default PWM counter

[Aditya Prayoga]

On Mon, Aug 6, 2018 at 8:53 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Mon, Aug 06, 2018 at 10:29:16AM +0800, Aditya Prayoga wrote:
> > On multiple PWM lines, if the other PWM counter is unused, allocate it
> > to next PWM request. The priority would be:
> > 1. Default counter assigned to the bank
> > 2. Unused counter that is assigned to other bank
> > 3. Fallback to default counter
> >
> > For example on second bank there are three PWM request, first one would
> > use default counter (counter B), second one would try to use counter A,
> > and the third one would use counter B.
>
> Hi Aditya
>
> There are only two PWM counters for all the GPIO lines. So you cannot
> support 3 PWM requests. You have to enforce a maximum of two PWMs.
>
> When i implemented this PWM code, i only needed one PWM. So it took
> the easy option. GPIO bank 0 uses counter A, GPIO bank1 uses counter
> B. For the hardware you have, this is not sufficient, so you need to
> generalise this. Any PWM can use any counter, whatever is available
> when the PWM is requested.

Hi Andrew

Understood. I will change it in next version.

> Rather than have a linked list of PWM, i think it would be better to
> have a static array of two mvebu_pwm structures. Index 0 uses counter
> A, index 1 uses counter B. You can then keep with the concept of
> pwm->pgiod != NULL means the counter is in use. The request() call can
> then find an unused PWM, set pwm->gpiod, and point mvchip->mvpwm to
> one of the two static instances.

That was my initial idea to use static array but then I thought maybe
I could generalise it for future device by using linked list.

Regards,

Aditya
>
>     Andrew

Re: [PATCH RESEND 2/2] gpio: mvebu: Allow to use non-default PWM counter

[Richard Genoud]

Hi,

On 06/08/2018 15:52, Andrew Lunn wrote:
> On Mon, Aug 06, 2018 at 10:29:16AM +0800, Aditya Prayoga wrote:
>> On multiple PWM lines, if the other PWM counter is unused, allocate it
>> to next PWM request. The priority would be:
>> 1. Default counter assigned to the bank
>> 2. Unused counter that is assigned to other bank
>> 3. Fallback to default counter
>>
>> For example on second bank there are three PWM request, first one would
>> use default counter (counter B), second one would try to use counter A,
>> and the third one would use counter B.
> 
> Hi Aditya
> 
> There are only two PWM counters for all the GPIO lines. So you cannot
> support 3 PWM requests. You have to enforce a maximum of two PWMs.
> 
> When i implemented this PWM code, i only needed one PWM. So it took
> the easy option. GPIO bank 0 uses counter A, GPIO bank1 uses counter
> B. For the hardware you have, this is not sufficient, so you need to
> generalise this. Any PWM can use any counter, whatever is available
> when the PWM is requested.
> 
> Rather than have a linked list of PWM, i think it would be better to
> have a static array of two mvebu_pwm structures. Index 0 uses counter
> A, index 1 uses counter B. You can then keep with the concept of
> pwm->pgiod != NULL means the counter is in use. The request() call can
> then find an unused PWM, set pwm->gpiod, and point mvchip->mvpwm to
> one of the two static instances.
> 
>     Andrew
> 
I'm not sure that the logic:
1. Default counter assigned to the bank
2. Unused counter that is assigned to other bank
3. Fallback to default counter
is the best one.

I gave the code a try, and I've been a little confused.
I declared:
- fan1 as gpio1 22 
- fan2 as gpio1 11
- fan3 as gpio0 22

and I did:
echo 10 > hwmon1/pwm1  # ok
echo 100 > hwmon2/pwm1 # still ok
echo 200 > hwmon3/pwm1 # hey !! my fan2 is now at 200 (I can see it with the scope)
# but
cat hwmon2/pwm1
100
# okay, I want my fan2 back, So I turn off fan3:
echo 0 > hwmon3/pwm1 # fan2 and fan3 are stopped
echo 100 > hwmon2/pwm1 # not working.. fan2 is still at 0 on the scope
but:
cat hwmon2/pwm1
100

IMHO, I would either:
- allow only 2 pwm and no more (but that's a pity)
- allow lots of fans, but once 2 different speeds are set, return EINVAL for another different speed (even if it's on another bank)
That way, we'll be able to switch on/off 1, 2, 3 or more fans, as long as they have the same speed.
I'll give an example :

echo 10 > hwmon1/pwm1 # ok
echo 100 > hwmon2/pwm1 # still ok
echo 200 > hwmon3/pwm1 # returns EINVAL
echo 10 > hwmon3/pwm1 # ok

The headache will come when we want to change the speed...
echo 50 > hwmon3/pwm1 # should this change the hwmon1 as well or return EINVAL ?
I'd say that it changes hwmon1 as well, as if hwmon1 and hwmon3 were tied.
But, If I then do :
echo 100 > hwmon3/pwm1 # fan2 was already at 100
What should happen ? Do fan1, fan2 and fan3 be set to 100 ?
Or fan1 stay at 10 and fan3 at 100 ? (in this case, fan3 won't be tied to fan1 anymore, but to fan2)

Hum...
I don't know if anyone followed me on this...
Anyway, I think I convinced myself that only allowing 2 pwm is less confusing than anything else :)



Richard.

Re: [PATCH RESEND 2/2] gpio: mvebu: Allow to use non-default PWM counter

[Andrew Lunn]

> I'm not sure that the logic:
> 1. Default counter assigned to the bank
> 2. Unused counter that is assigned to other bank
> 3. Fallback to default counter
> is the best one.

Hi Richard

It it totally broken, as you point out. That is why i said it needs to
be limited to two PWMs.

> IMHO, I would either:
> - allow only 2 pwm and no more (but that's a pity)
> - allow lots of fans, but once 2 different speeds are set, return
>   EINVAL for another different speed (even if it's on another bank)

This second option also breaks the Linux PWM model.

What you should be thinking about is extending the Linux PWM model so
that one PWM can drive more than one pin.

     Andrew