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

From: Jacek Anaszewski
Date: Fri Oct 14 2016 - 02:37:33 EST


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