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

[Jacek Anaszewski]

On 10/13/2016 04:20 PM, Matt Ranostay wrote:
> 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.

I'd like to have DT maintainer's ack for this.

Cc Rob and Mark.

> > >  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
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
Best regards,
Jacek Anaszewski