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

[Eddie James]

On 4/8/22 06:19, Andy Shevchenko wrote:
> 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?


milliseconds, I'll change the name to reflect that.


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


Ack.


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


Agree, but perhaps for a future enhancement?


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


Ack, and all the rest. Will get a v2 up.


Thanks Andy.

Eddie