Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932773AbcJMOVP (ORCPT ); Thu, 13 Oct 2016 10:21:15 -0400 Received: from mail-lf0-f65.google.com ([209.85.215.65]:34820 "EHLO mail-lf0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932426AbcJMOUo (ORCPT ); Thu, 13 Oct 2016 10:20:44 -0400 MIME-Version: 1.0 In-Reply-To: References: <1476364572-26849-1-git-send-email-matt@ranostay.consulting> From: Matt Ranostay Date: Thu, 13 Oct 2016 16:20:01 +0200 Message-ID: Subject: Re: [PATCH] leds: leds-pca963x: workaround group blink scaling issue To: Jacek Anaszewski Cc: "linux-kernel@vger.kernel.org" , "devicetree@vger.kernel.org" , Matt Ranostay , Tony Lindgren Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4602 Lines: 126 On Thu, Oct 13, 2016 at 4:05 PM, Jacek Anaszewski 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 >> Cc: Jacek Anaszewski >> Signed-off-by: Matt Ranostay >> --- >> 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