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

[Eddie James <eajames@linux.ibm.com>]

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