Re: [PATCH v2 2/2] leds: pca955x: Add HW blink support

[Andy Shevchenko]

On Thu, Apr 7, 2022 at 10:43 PM Eddie James <eajames@xxxxxxxxxxxxx> 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