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: <20170317101747.67a09ccd@gmail.com>
Date:   Fri, 17 Mar 2017 10:17:47 +0100
From:   Ralph Sennhauser <ralph.sennhauser@...il.com>
To:     Linus Walleij <linus.walleij@...aro.org>
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, 16 Mar 2017 17:03:05 +0100
Linus Walleij <linus.walleij@...aro.org> wrote:

> 
> 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.

Hi Linus,

thanks for merging the cleanup patches. 


> 
> > +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?
>

Heh, good point, will need to look into this.


> 
> > +       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);
>

linus/bitops.h ...
    ^
Another one of those nifty macros, sure, can do so for v2, though there
are many instances of this technique already, I'll send another cleanup
patch to convert them all for consistency sake.


> 
> > +       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
>

mvebu_pwm_probe returns -EINVAL if id isn't 0 or 1.

        if (id < 0 || id > 1)
                return -EINVAL;

Guess this needs commenting as well then.

> 
> > +static void mvebu_pwm_suspend(struct mvebu_gpio_chip *mvchip)
> > +static void mvebu_pwm_resume(struct mvebu_gpio_chip *mvchip)
> 
> I think both of these need to be tagged __maybe_unused to not give
> noise in randconfig builds.

I haven't seen any warnings with CONFIG_PWM disabled. Which
configuration you expect to trigger a warning? mvebu_pwm_probe should
be the same, right?

> 
> Yours,
> Linus Walleij

Thanks for the review.
Ralph