| Message ID | 1533522556-55055-2-git-send-email-aditya@kobol.io |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | gpio: mvebu: Add support for multiple PWM lines | expand |
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
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
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
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
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
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
> 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
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))
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(-)