On Thu, Mar 16, 2017 at 7:42 AM, Ralph Sennhauser <ralph.sennhauser@xxxxxxxxx> wrote: > From: Andrew Lunn <andrew@xxxxxxx> > > 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 <andrew@xxxxxxx> > 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 <ralph.sennhauser@xxxxxxxxx> 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 <linus/bitops.h> 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
