Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1603128

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

Show all headers | View raw

On Thu, 16 Mar 2017 17:03:05 +0100
Linus Walleij <linus.walleij@linaro.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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH 0/4] gpio: mvebu: Add PWM fan support Ralph Sennhauser <ralph.sennhauser@gmail.com> - 2017-03-16 07:50 +0100
  [PATCH 3/4] ARM: mvebu: Enable SENSORS_PWM_FAN in defconfig Ralph Sennhauser <ralph.sennhauser@gmail.com> - 2017-03-16 07:50 +0100
  [PATCH 1/4] gpio: mvebu: Add limited PWM support Ralph Sennhauser <ralph.sennhauser@gmail.com> - 2017-03-16 07:50 +0100
    Re: [PATCH 1/4] gpio: mvebu: Add limited PWM support Linus Walleij <linus.walleij@linaro.org> - 2017-03-16 17:10 +0100
      Re: [PATCH 1/4] gpio: mvebu: Add limited PWM support Ralph Sennhauser <ralph.sennhauser@gmail.com> - 2017-03-17 10:20 +0100
        Re: [PATCH 1/4] gpio: mvebu: Add limited PWM support Thierry Reding <thierry.reding@gmail.com> - 2017-03-20 15:00 +0100
          Re: [PATCH 1/4] gpio: mvebu: Add limited PWM support Ralph Sennhauser <ralph.sennhauser@gmail.com> - 2017-03-21 07:40 +0100
          Re: [PATCH 1/4] gpio: mvebu: Add limited PWM support Linus Walleij <linus.walleij@linaro.org> - 2017-03-23 11:20 +0100
            Re: [PATCH 1/4] gpio: mvebu: Add limited PWM support Ralph Sennhauser <ralph.sennhauser@gmail.com> - 2017-03-23 11:40 +0100
      Re: [PATCH 1/4] gpio: mvebu: Add limited PWM support Andrew Lunn <andrew@lunn.ch> - 2017-03-18 16:40 +0100
        Re: [PATCH 1/4] gpio: mvebu: Add limited PWM support Thierry Reding <thierry.reding@gmail.com> - 2017-03-20 15:00 +0100
      Re: [PATCH 1/4] gpio: mvebu: Add limited PWM support Thierry Reding <thierry.reding@gmail.com> - 2017-03-20 15:50 +0100
    Re: [PATCH 1/4] gpio: mvebu: Add limited PWM support Thierry Reding <thierry.reding@gmail.com> - 2017-03-20 14:50 +0100
      Re: [PATCH 1/4] gpio: mvebu: Add limited PWM support Ralph Sennhauser <ralph.sennhauser@gmail.com> - 2017-03-21 07:40 +0100
        Re: [PATCH 1/4] gpio: mvebu: Add limited PWM support Andrew Lunn <andrew@lunn.ch> - 2017-03-21 16:00 +0100
  Re: [PATCH 0/4] gpio: mvebu: Add PWM fan support Linus Walleij <linus.walleij@linaro.org> - 2017-03-16 16:50 +0100
  Re: [PATCH 0/4] gpio: mvebu: Add PWM fan support Andrew Lunn <andrew@lunn.ch> - 2017-03-18 16:40 +0100
    Re: [PATCH 0/4] gpio: mvebu: Add PWM fan support Ralph Sennhauser <ralph.sennhauser@gmail.com> - 2017-03-18 17:20 +0100

csiph-web