Messages in this thread | | | Date | Mon, 11 Apr 2022 11:06:35 -0500 | Subject | Re: [PATCH v2 2/2] leds: pca955x: Add HW blink support | From | Eddie James <> |
| |
On 4/8/22 06:19, Andy Shevchenko wrote: > On Thu, Apr 7, 2022 at 10:43 PM Eddie James <eajames@linux.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?
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
| |