Re: [PATCH 1/4] gpio: mvebu: Add limited PWM support

[Linus Walleij]

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