linux-kernel - Re: [PATCH v2 2/2] leds: pca955x: Add HW blink 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: <CAHp75VfcmhGitd6_6mDVG6_eE02sX2B3_iqZxeiX3iFDkmPShg@mail.gmail.com>
Date:   Fri, 8 Apr 2022 14:19:15 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Eddie James <eajames@...ux.ibm.com>
Cc:     Linux LED Subsystem <linux-leds@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Pavel Machek <pavel@....cz>,
        OpenBMC Maillist <openbmc@...ts.ozlabs.org>,
        Joel Stanley <joel@....id.au>, patrick@...cx.xyz
Subject: Re: [PATCH v2 2/2] leds: pca955x: Add HW blink support

On Thu, Apr 7, 2022 at 10:43 PM Eddie James <eajames@...ux.ibm.com> wrote:
>
> Support blinking using the PCA955x chip. Use PWM0 for blinking
> instead of LED_HALF brightness. Since there is only one frequency
> and brightness register for any blinking LED, track the blink state
> of each LED and only support one HW blinking frequency. If another
> frequency is requested, fallback to software blinking.

...

> +#define PCA955X_BLINK_DEFAULT  1000

What's the unit of this number?

...

>   * Write to frequency prescaler register, used to program the
> - * period of the PWM output.  period = (PSCx + 1) / 38
> + * period of the PWM output.  period = (PSCx + 1) / <38 or 44, chip dependent>

Using <> in  formulas a bit confusing, what about

 * period of the PWM output.  period = (PSCx + 1) / coeff
 * where for ... chips coeff = 38, for ... chips coeff = 44.

?

...

> +               dev_err(&pca955x->client->dev, "%s: reg 0x%x, err %d\n",
> +                       __func__, n, ret);

Can be indented better. But I would rather see regmap, where this kind
of debugging is for free and already present in the regmap core/.

...

> +static u8 pca955x_period_to_psc(struct pca955x *pca955x, unsigned long p)
> +{
> +       p *= (unsigned long)pca955x->chipdef->blink_div;

Why casting?

> +       p /= 1000;

Does this 1000 have a meaning? (see units.h and other headers with
time / frequency multiplier definitions).

> +       p -= 1;

> +       return (u8)p;

Redundant casting.

> +}

> +static unsigned long pca955x_psc_to_period(struct pca955x *pca955x, u8 psc)
> +{
> +       unsigned long p = (unsigned long)psc;
> +
> +       p += 1;
> +       p *= 1000;
> +       p /= (unsigned long)pca955x->chipdef->blink_div;
> +
> +       return p;

Similar questions here.

> +}

...

> +       if (!p) {

Why not use a positive conditional?

> +               p = pca955x->active_blink ? pca955x->blink_period :
> +                       PCA955X_BLINK_DEFAULT;
> +       } else {
> +               if (*delay_on != *delay_off) {
> +                       ret = -EINVAL;
> +                       goto out;
> +               }
> +
> +               if (p < pca955x_psc_to_period(pca955x, 0) ||
> +                   p > pca955x_psc_to_period(pca955x, 0xff)) {
> +                       ret = -EINVAL;
> +                       goto out;
> +               }
> +       }

...

> +       if (!keep_psc0) {

Ditto.

> +               psc0 = pca955x_period_to_psc(pca955x, pca955x->blink_period);
> +               err = pca955x_write_psc(pca955x, 0, psc0);
> +       } else {
> +               err = pca955x_read_psc(pca955x, 0, &psc0);
>         }

-- 
With Best Regards,
Andy Shevchenko