From mboxrd@z Thu Jan 1 00:00:00 1970 From: Linus Walleij Subject: Re: [PATCH 1/4] gpio: mvebu: Add limited PWM support Date: Thu, 16 Mar 2017 17:03:05 +0100 Message-ID: References: <20170316064218.9169-1-ralph.sennhauser@gmail.com> <20170316064218.9169-2-ralph.sennhauser@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Return-path: In-Reply-To: <20170316064218.9169-2-ralph.sennhauser-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Ralph Sennhauser Cc: "linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , Andrew Lunn , Imre Kaloz , Thierry Reding , Alexandre Courbot , Rob Herring , Mark Rutland , Greg Kroah-Hartman , "David S. Miller" , Geert Uytterhoeven , Mauro Carvalho Chehab , Andrew Morton , Guenter Roeck , "open list:PWM SUBSYSTEM" , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , open list List-Id: linux-gpio@vger.kernel.org On Thu, Mar 16, 2017 at 7:42 AM, Ralph Sennhauser wrote: > From: Andrew Lunn > > Armada 370/XP devices can 'blink' gpio lines with a configurable on > and off period. This can be modelled as a PWM. > > 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. > > Due to the interleaving of registers it is not simple to separate the > PWM driver from the gpio driver. Thus the gpio driver has been > extended with a PWM driver. > > Signed-off-by: Andrew Lunn > URL: https://patchwork.ozlabs.org/patch/427287/ > URL: https://patchwork.ozlabs.org/patch/427295/ > [Ralph Sennhauser: > * port forward > * merge pwm portion into gpio-mvebu.c > * merge doc patch > * update MAINAINERS] > Signed-off-by: Ralph Sennhauser In essence I am very positive of this patch set and happy to merge it as a PWM driver inside of GPIO if Thierry is OK with it. DT bindings look fine to me. > +static void mvebu_pwm_free(struct pwm_chip *chip, struct pwm_device *pwmd) > +{ > + struct mvebu_pwm *pwm = to_mvebu_pwm(chip); > + struct gpio_desc *desc = gpio_to_desc(pwmd->pwm); > + unsigned long flags; > + > + spin_lock_irqsave(&pwm->lock, flags); > + gpiod_free(desc); > + pwm->used = false; > + spin_unlock_irqrestore(&pwm->lock, flags); > +} No need to set the output value to zero or something here? And turn off blinking? Or is that done some other way? > + u = readl_relaxed(mvebu_gpioreg_blink_select(mvchip)); > + u &= ~(1 << pwm->pin); In GPIO code I usually do this: #include u &= ~BIT(pwm->pin); > + u |= (pwm->id << pwm->pin); I don't understand this line. Above you mask BIT(pwm->pin) so we are only manipulating one bit, and then you ... shift the ID? Is the ID always 0 or 1? If that is the case then this is easier to understand: if (pwm->id) u |= BIT(pwm->pin); + a comment > +static void mvebu_pwm_suspend(struct mvebu_gpio_chip *mvchip) > +{ > + struct mvebu_pwm *pwm = mvchip->pwm; > + > + pwm->blink_select = readl_relaxed(mvebu_gpioreg_blink_select(mvchip)); > + pwm->blink_on_duration = > + readl_relaxed(mvebu_pwmreg_blink_on_duration(pwm)); > + pwm->blink_off_duration = > + readl_relaxed(mvebu_pwmreg_blink_off_duration(pwm)); > +} > + > +static void mvebu_pwm_resume(struct mvebu_gpio_chip *mvchip) > +{ > + struct mvebu_pwm *pwm = mvchip->pwm; > + > + writel_relaxed(pwm->blink_select, mvebu_gpioreg_blink_select(mvchip)); > + writel_relaxed(pwm->blink_on_duration, > + mvebu_pwmreg_blink_on_duration(pwm)); > + writel_relaxed(pwm->blink_off_duration, > + mvebu_pwmreg_blink_off_duration(pwm)); > +} I think both of these need to be tagged __maybe_unused to not give noise in randconfig builds. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753596AbdCPQDP (ORCPT ); Thu, 16 Mar 2017 12:03:15 -0400 Received: from mail-pf0-f174.google.com ([209.85.192.174]:36480 "EHLO mail-pf0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753318AbdCPQDN (ORCPT ); Thu, 16 Mar 2017 12:03:13 -0400 MIME-Version: 1.0 In-Reply-To: <20170316064218.9169-2-ralph.sennhauser@gmail.com> References: <20170316064218.9169-1-ralph.sennhauser@gmail.com> <20170316064218.9169-2-ralph.sennhauser@gmail.com> From: Linus Walleij Date: Thu, 16 Mar 2017 17:03:05 +0100 Message-ID: Subject: Re: [PATCH 1/4] gpio: mvebu: Add limited PWM support To: Ralph Sennhauser Cc: "linux-gpio@vger.kernel.org" , Andrew Lunn , Imre Kaloz , Thierry Reding , Alexandre Courbot , Rob Herring , Mark Rutland , Greg Kroah-Hartman , "David S. Miller" , Geert Uytterhoeven , Mauro Carvalho Chehab , Andrew Morton , Guenter Roeck , "open list:PWM SUBSYSTEM" , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , open list Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Mar 16, 2017 at 7:42 AM, Ralph Sennhauser wrote: > From: Andrew Lunn > > Armada 370/XP devices can 'blink' gpio lines with a configurable on > and off period. This can be modelled as a PWM. > > 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. > > Due to the interleaving of registers it is not simple to separate the > PWM driver from the gpio driver. Thus the gpio driver has been > extended with a PWM driver. > > Signed-off-by: Andrew Lunn > URL: https://patchwork.ozlabs.org/patch/427287/ > URL: https://patchwork.ozlabs.org/patch/427295/ > [Ralph Sennhauser: > * port forward > * merge pwm portion into gpio-mvebu.c > * merge doc patch > * update MAINAINERS] > Signed-off-by: Ralph Sennhauser In essence I am very positive of this patch set and happy to merge it as a PWM driver inside of GPIO if Thierry is OK with it. DT bindings look fine to me. > +static void mvebu_pwm_free(struct pwm_chip *chip, struct pwm_device *pwmd) > +{ > + struct mvebu_pwm *pwm = to_mvebu_pwm(chip); > + struct gpio_desc *desc = gpio_to_desc(pwmd->pwm); > + unsigned long flags; > + > + spin_lock_irqsave(&pwm->lock, flags); > + gpiod_free(desc); > + pwm->used = false; > + spin_unlock_irqrestore(&pwm->lock, flags); > +} No need to set the output value to zero or something here? And turn off blinking? Or is that done some other way? > + u = readl_relaxed(mvebu_gpioreg_blink_select(mvchip)); > + u &= ~(1 << pwm->pin); In GPIO code I usually do this: #include u &= ~BIT(pwm->pin); > + u |= (pwm->id << pwm->pin); I don't understand this line. Above you mask BIT(pwm->pin) so we are only manipulating one bit, and then you ... shift the ID? Is the ID always 0 or 1? If that is the case then this is easier to understand: if (pwm->id) u |= BIT(pwm->pin); + a comment > +static void mvebu_pwm_suspend(struct mvebu_gpio_chip *mvchip) > +{ > + struct mvebu_pwm *pwm = mvchip->pwm; > + > + pwm->blink_select = readl_relaxed(mvebu_gpioreg_blink_select(mvchip)); > + pwm->blink_on_duration = > + readl_relaxed(mvebu_pwmreg_blink_on_duration(pwm)); > + pwm->blink_off_duration = > + readl_relaxed(mvebu_pwmreg_blink_off_duration(pwm)); > +} > + > +static void mvebu_pwm_resume(struct mvebu_gpio_chip *mvchip) > +{ > + struct mvebu_pwm *pwm = mvchip->pwm; > + > + writel_relaxed(pwm->blink_select, mvebu_gpioreg_blink_select(mvchip)); > + writel_relaxed(pwm->blink_on_duration, > + mvebu_pwmreg_blink_on_duration(pwm)); > + writel_relaxed(pwm->blink_off_duration, > + mvebu_pwmreg_blink_off_duration(pwm)); > +} I think both of these need to be tagged __maybe_unused to not give noise in randconfig builds. Yours, Linus Walleij