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
