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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACRpkdZ=bor380ic_X36M7vVbopDCZe9Yb9TxkmOHyO75gdAnw@mail.gmail.com>
Date:   Thu, 16 Mar 2017 17:03:05 +0100
From:   Linus Walleij <linus.walleij@...aro.org>
To:     Ralph Sennhauser <ralph.sennhauser@...il.com>
Cc:     "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        Andrew Lunn <andrew@...n.ch>, Imre Kaloz <kaloz@...nwrt.org>,
        Thierry Reding <thierry.reding@...il.com>,
        Alexandre Courbot <gnurou@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "David S. Miller" <davem@...emloft.net>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Guenter Roeck <linux@...ck-us.net>,
        "open list:PWM SUBSYSTEM" <linux-pwm@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/4] gpio: mvebu: Add limited PWM support

On Thu, Mar 16, 2017 at 7:42 AM, Ralph Sennhauser
<ralph.sennhauser@...il.com> wrote:

> From: Andrew Lunn <andrew@...n.ch>
>
> 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@...n.ch>
> 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@...il.com>

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