Messages in this thread | | | From | Andy Shevchenko <> | Date | Fri, 8 Apr 2022 14:19:15 +0300 | Subject | Re: [PATCH v2 2/2] leds: pca955x: Add HW blink support |
| |
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?
...
> * 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
| |