Re: [PATCH] leds: leds-pca963x: workaround group blink scaling issue

[Matt Ranostay]

On Thu, Oct 13, 2016 at 4:05 PM, Jacek Anaszewski
<j.anaszewski@xxxxxxxxxxx> wrote:
> Hi Matt,
>
> On 10/13/2016 03:16 PM, Matt Ranostay wrote:
>>
>> PCA9632TK part seems to incorrectly blink at ~1.3x of the programmed
>> rate. This patchset add a nxp,period-scale devicetree property to
>> adjust for this misconfiguration.
>>
>> Cc: Tony Lindgren <tony@xxxxxxxxxxx>
>> Cc: Jacek Anaszewski <j.anaszewski@xxxxxxxxxxx>
>> Signed-off-by: Matt Ranostay <matt@xxxxxxxxxxxxxxxxxxx>
>> ---
>>  Documentation/devicetree/bindings/leds/pca963x.txt |  3 +++
>>  drivers/leds/leds-pca963x.c                        | 18
>> +++++++++++++++---
>>  2 files changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/leds/pca963x.txt
>> b/Documentation/devicetree/bindings/leds/pca963x.txt
>> index dafbe9931c2b..dfbdb123a9bf 100644
>> --- a/Documentation/devicetree/bindings/leds/pca963x.txt
>> +++ b/Documentation/devicetree/bindings/leds/pca963x.txt
>> @@ -7,6 +7,9 @@ Optional properties:
>>  - nxp,totem-pole : use totem pole (push-pull) instead of open-drain
>> (pca9632 defaults
>>    to open-drain, newer chips to totem pole)
>>  - nxp,hw-blink : use hardware blinking instead of software blinking
>> +- nxp,period-scale : In some configurations, the chip blinks faster than
>> expected.
>> +                    This parameter provides a scaling ratio (fixed point,
>> decimal divided
>> +                    by 1000) to compensate, e.g. 1300=1.3x and 750=0.75x.
>
>
> Why DT property? Is it somehow dependent on the board configuration?
> How this period-scale value is calculated? Is it inferred empirically?
>

We empirically discovered and verified this with an logic analyzer on
multiple batches of this part.
Reason for the DT entry is we aren't 100% sure that it is always going
to be the same with different board revs.

Could be that parts clock acts differently with supply voltage.   This
has been calculated by setting it an expected value, and measuring the
actual result with the logic analyzer.

>
>>  Each led is represented as a sub-node of the nxp,pca963x device.
>>
>> diff --git a/drivers/leds/leds-pca963x.c b/drivers/leds/leds-pca963x.c
>> index 407eba11e187..b6ce1f2ec33e 100644
>> --- a/drivers/leds/leds-pca963x.c
>> +++ b/drivers/leds/leds-pca963x.c
>> @@ -59,6 +59,7 @@ struct pca963x_chipdef {
>>         u8                      grpfreq;
>>         u8                      ledout_base;
>>         int                     n_leds;
>> +       unsigned int            scaling;
>>  };
>>
>>  static struct pca963x_chipdef pca963x_chipdefs[] = {
>> @@ -189,6 +190,14 @@ static int pca963x_led_set(struct led_classdev
>> *led_cdev,
>>         return pca963x_brightness(pca963x, value);
>>  }
>>
>> +static unsigned int pca963x_period_scale(struct pca963x_led *pca963x,
>> +       unsigned int val)
>> +{
>> +       unsigned int scaling = pca963x->chip->chipdef->scaling;
>> +
>> +       return scaling ? DIV_ROUND_CLOSEST(val * scaling, 1000) : val;
>> +}
>> +
>>  static int pca963x_blink_set(struct led_classdev *led_cdev,
>>                 unsigned long *delay_on, unsigned long *delay_off)
>>  {
>> @@ -207,14 +216,14 @@ static int pca963x_blink_set(struct led_classdev
>> *led_cdev,
>>                 time_off = 500;
>>         }
>>
>> -       period = time_on + time_off;
>> +       period = pca963x_period_scale(pca963x, time_on + time_off);
>>
>>         /* If period not supported by hardware, default to someting sane.
>> */
>>         if ((period < PCA963X_BLINK_PERIOD_MIN) ||
>>             (period > PCA963X_BLINK_PERIOD_MAX)) {
>>                 time_on = 500;
>>                 time_off = 500;
>> -               period = time_on + time_off;
>> +               period = pca963x_period_scale(pca963x, 1000);
>>         }
>>
>>         /*
>> @@ -222,7 +231,7 @@ static int pca963x_blink_set(struct led_classdev
>> *led_cdev,
>>          *      (time_on / period) = (GDC / 256) ->
>>          *              GDC = ((time_on * 256) / period)
>>          */
>> -       gdc = (time_on * 256) / period;
>> +       gdc = (pca963x_period_scale(pca963x, time_on) * 256) / period;
>>
>>         /*
>>          * From manual: period = ((GFRQ + 1) / 24) in seconds.
>> @@ -294,6 +303,9 @@ pca963x_dt_init(struct i2c_client *client, struct
>> pca963x_chipdef *chip)
>>         else
>>                 pdata->blink_type = PCA963X_SW_BLINK;
>>
>> +       if (of_property_read_u32(np, "nxp,period-scale", &chip->scaling))
>> +               chip->scaling = 1000;
>> +
>>         return pdata;
>>  }
>>
>>
>
>
> --
> Best regards,
> Jacek Anaszewski