linux-kernel - Re: [PATCH v2 1/1] gpio: mvebu: Add support for multiple PWM lines per GPIO chip

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] [day] [month] [year] [list]

Date:   Thu, 13 Sep 2018 18:14:58 +0700
From:   Aditya Prayoga <aditya@...ol.io>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     linux-gpio@...r.kernel.org,
        Thierry Reding <thierry.reding@...il.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        linux-pwm@...r.kernel.org, linux-kernel@...r.kernel.org,
        Richard Genoud <richard.genoud@...il.com>,
        ralph.sennhauser@...il.com, gregory.clement@...tlin.com,
        Gauthier Provost <gauthier@...ol.io>,
        Dennis Gilmore <dennis@...il.us>
Subject: Re: [PATCH v2 1/1] gpio: mvebu: Add support for multiple PWM lines
 per GPIO chip

On Wed, Sep 12, 2018 at 8:01 PM Andrew Lunn <andrew@...n.ch> wrote:
>
> >  static int mvebu_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> >  {
> >       struct mvebu_pwm *mvpwm = to_mvebu_pwm(chip);
> >       struct mvebu_gpio_chip *mvchip = mvpwm->mvchip;
> >       struct gpio_desc *desc;
> > +     struct mvebu_pwm *counter;
> >       unsigned long flags;
> >       int ret = 0;
> >
> >       spin_lock_irqsave(&mvpwm->lock, flags);
> >
> > -     if (mvpwm->gpiod) {
> > -             ret = -EBUSY;
> > -     } else {
> > -             desc = gpiochip_request_own_desc(&mvchip->chip,
> > -                                              pwm->hwpwm, "mvebu-pwm");
> > -             if (IS_ERR(desc)) {
> > -                     ret = PTR_ERR(desc);
> > +     counter = mvpwm;
> > +     if (counter->gpiod) {
> > +             counter = mvebu_pwm_get_avail_counter();
> > +             if (!counter) {
> > +                     ret = -EBUSY;
>
> I don't understand this bit of code. Please could you explain what is
> going on.
Check whether bank's default counter is already used. If it's used then
try to find and check other counter. If it also being used that mean both,
counter A and counter B, already assigned to some PWM device so
return EBUSY.

>
> >                       goto out;
> >               }
> >
> > -             ret = gpiod_direction_output(desc, 0);
> > -             if (ret) {
> > -                     gpiochip_free_own_desc(desc);
> > -                     goto out;
> > -             }
> > +             pwm->chip_data = counter;
> > +     }
> >
> > -             mvpwm->gpiod = desc;
> > +     desc = gpiochip_request_own_desc(&mvchip->chip,
> > +                                      pwm->hwpwm, "mvebu-pwm");
> > +     if (IS_ERR(desc)) {
> > +             ret = PTR_ERR(desc);
> > +             goto out;
> >       }
> > +
> > +     ret = gpiod_direction_output(desc, 0);
> > +     if (ret) {
> > +             gpiochip_free_own_desc(desc);
> > +             goto out;
> > +     }
> > +
> > +     regmap_update_bits(mvchip->regs, GPIO_BLINK_CNT_SELECT_OFF +
> > +                        mvchip->offset, BIT(pwm->hwpwm),
> > +                        counter->id ? BIT(pwm->hwpwm) : 0);
> > +     regmap_read(mvchip->regs, GPIO_BLINK_CNT_SELECT_OFF +
> > +                 mvchip->offset, &counter->blink_select);
> > +
> > +     counter->gpiod = desc;
> >  out:
> >       spin_unlock_irqrestore(&mvpwm->lock, flags);
> >       return ret;
> > @@ -632,6 +666,11 @@ static void mvebu_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> >       struct mvebu_pwm *mvpwm = to_mvebu_pwm(chip);
> >       unsigned long flags;
> >
> > +     if (pwm->chip_data) {
> > +             mvpwm = (struct mvebu_pwm *) pwm->chip_data;
> > +             pwm->chip_data = NULL;
> > +     }
> > +
> >       spin_lock_irqsave(&mvpwm->lock, flags);
> >       gpiochip_free_own_desc(mvpwm->gpiod);
> >       mvpwm->gpiod = NULL;
> > @@ -648,6 +687,9 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip,
> >       unsigned long flags;
> >       u32 u;
> >
> > +     if (pwm->chip_data)
> > +             mvpwm = (struct mvebu_pwm *) pwm->chip_data;
> > +
>
> You should not need a cast here, if chip_data is a void *.
>
> What is pwm->chip_data is a NULL? Don't you then use an uninitialized
> mvpwm?

mvpwm is declared and initialized as:
struct mvebu_pwm *mvpwm = to_mvebu_pwm(chip);
so it's not an uninitialized variable.
pwm->chip_data is set when the pwm use counter other then default.
pwm->chip_data take precedence over to_mvebu_pwm(chip).

After looked at other PWM driver, i think i should use pwm_set_chip_data()
and pwm_get_chip_data() instead of directly access pwm->chip_data.

Now i think it would be better if i use
struct mvebu_pwm *mvpwm = pwm_get_chip_data(pwm);
and pwm_set_chip_data() would be called during mvebu_pwm_probe() to
set the data to bank's default counter.

Aditya

>         Andrew