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

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

[Matt Ranostay]

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@atomide.com>
Cc: Jacek Anaszewski <j.anaszewski@samsung.com>
Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
---
 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.
 
 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;
 }
 
-- 
2.7.4

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

[Jacek Anaszewski]

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@atomide.com>
> Cc: Jacek Anaszewski <j.anaszewski@samsung.com>
> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
> ---
>  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?

>  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

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@samsung.com> 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@atomide.com>
>> Cc: Jacek Anaszewski <j.anaszewski@samsung.com>
>> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
>> ---
>>  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

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@samsung.com> 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@atomide.com>
>>> Cc: Jacek Anaszewski <j.anaszewski@samsung.com>
>>> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
>>> ---
>>>  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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>


-- 
Best regards,
Jacek Anaszewski

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

[Tony Lindgren]

* Jacek Anaszewski <j.anaszewski@samsung.com> [161013 23:37]:
> On 10/13/2016 04:20 PM, Matt Ranostay wrote:
> > On Thu, Oct 13, 2016 at 4:05 PM, Jacek Anaszewski
> > <j.anaszewski@samsung.com> wrote:
> > > 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.

How about do this based on the compatible property instead? If there
are multiple manufacturers for this part and only a certain
parts have this issue we should have multiple compatible properties.

Then if it turns out all of them need this scaling there's no need
to update the binding.

Regards,

Tony

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

[Matt Ranostay]

On Fri, Oct 14, 2016 at 7:20 AM, Tony Lindgren <tony@atomide.com> wrote:
> * Jacek Anaszewski <j.anaszewski@samsung.com> [161013 23:37]:
>> On 10/13/2016 04:20 PM, Matt Ranostay wrote:
>> > On Thu, Oct 13, 2016 at 4:05 PM, Jacek Anaszewski
>> > <j.anaszewski@samsung.com> wrote:
>> > > 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.
>
> How about do this based on the compatible property instead? If there
> are multiple manufacturers for this part and only a certain
> parts have this issue we should have multiple compatible properties.
>

I could only find that NXP as the manufacturer of that part. It is
possible since the clock is internal to the chipset that the vdd of
2.5V is doing something undefined.

> Then if it turns out all of them need this scaling there's no need
> to update the binding.

Understandable.

>
> Regards,
>
> Tony

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

[Jacek Anaszewski]

On 10/15/2016 02:00 PM, Matt Ranostay wrote:
> On Fri, Oct 14, 2016 at 7:20 AM, Tony Lindgren <tony@atomide.com> wrote:
>> * Jacek Anaszewski <j.anaszewski@samsung.com> [161013 23:37]:
>>> On 10/13/2016 04:20 PM, Matt Ranostay wrote:
>>>> On Thu, Oct 13, 2016 at 4:05 PM, Jacek Anaszewski
>>>> <j.anaszewski@samsung.com> wrote:
>>>>> 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.
>>
>> How about do this based on the compatible property instead? If there
>> are multiple manufacturers for this part and only a certain
>> parts have this issue we should have multiple compatible properties.
>>
>
> I could only find that NXP as the manufacturer of that part. It is
> possible since the clock is internal to the chipset that the vdd of
> 2.5V is doing something undefined.
>
>> Then if it turns out all of them need this scaling there's no need
>> to update the binding.
>
> Understandable.

Since at present we can't guarantee that all produced devices
are affected, then we should strive to avoid breaking any existing
users of the possible non-affected devices.

In view of that the addition of a new "compatible" proposed by Tony
seems most reasonable.

Still, DT maintainer's opinion is required.

-- 
Best regards,
Jacek Anaszewski

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

[Rob Herring]

On Mon, Oct 17, 2016 at 09:58:26AM +0200, Jacek Anaszewski wrote:
> On 10/15/2016 02:00 PM, Matt Ranostay wrote:
> > On Fri, Oct 14, 2016 at 7:20 AM, Tony Lindgren <tony@atomide.com> wrote:
> > > * Jacek Anaszewski <j.anaszewski@samsung.com> [161013 23:37]:
> > > > On 10/13/2016 04:20 PM, Matt Ranostay wrote:
> > > > > On Thu, Oct 13, 2016 at 4:05 PM, Jacek Anaszewski
> > > > > <j.anaszewski@samsung.com> wrote:
> > > > > > 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.
> > > 
> > > How about do this based on the compatible property instead? If there
> > > are multiple manufacturers for this part and only a certain
> > > parts have this issue we should have multiple compatible properties.
> > > 
> > 
> > I could only find that NXP as the manufacturer of that part. It is
> > possible since the clock is internal to the chipset that the vdd of
> > 2.5V is doing something undefined.
> > 
> > > Then if it turns out all of them need this scaling there's no need
> > > to update the binding.
> > 
> > Understandable.
> 
> Since at present we can't guarantee that all produced devices
> are affected, then we should strive to avoid breaking any existing
> users of the possible non-affected devices.
> 
> In view of that the addition of a new "compatible" proposed by Tony
> seems most reasonable.
> 
> Still, DT maintainer's opinion is required.

Seems like a quirk of this board, so I think the added property is fine.

It could be existing users just didn't notice the rate being off. 30% is 
probably not all that noticeable to the human eye.

Rob

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

[Jacek Anaszewski]

On 10/18/2016 03:49 PM, Rob Herring wrote:
> On Mon, Oct 17, 2016 at 09:58:26AM +0200, Jacek Anaszewski wrote:
>> On 10/15/2016 02:00 PM, Matt Ranostay wrote:
>>> On Fri, Oct 14, 2016 at 7:20 AM, Tony Lindgren <tony@atomide.com> wrote:
>>>> * Jacek Anaszewski <j.anaszewski@samsung.com> [161013 23:37]:
>>>>> On 10/13/2016 04:20 PM, Matt Ranostay wrote:
>>>>>> On Thu, Oct 13, 2016 at 4:05 PM, Jacek Anaszewski
>>>>>> <j.anaszewski@samsung.com> wrote:
>>>>>>> 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.
>>>>
>>>> How about do this based on the compatible property instead? If there
>>>> are multiple manufacturers for this part and only a certain
>>>> parts have this issue we should have multiple compatible properties.
>>>>
>>>
>>> I could only find that NXP as the manufacturer of that part. It is
>>> possible since the clock is internal to the chipset that the vdd of
>>> 2.5V is doing something undefined.
>>>
>>>> Then if it turns out all of them need this scaling there's no need
>>>> to update the binding.
>>>
>>> Understandable.
>>
>> Since at present we can't guarantee that all produced devices
>> are affected, then we should strive to avoid breaking any existing
>> users of the possible non-affected devices.
>>
>> In view of that the addition of a new "compatible" proposed by Tony
>> seems most reasonable.
>>
>> Still, DT maintainer's opinion is required.
>
> Seems like a quirk of this board, so I think the added property is fine.
>
> It could be existing users just didn't notice the rate being off. 30% is
> probably not all that noticeable to the human eye.

Thanks for the feedback. I infer that you wouldn't mind if
I added your ack to this commit then?

-- 
Best regards,
Jacek Anaszewski

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

[Jacek Anaszewski]

Hi Matt,

Patch applied.

Thanks,
Jacek Anaszewski

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@atomide.com>
> Cc: Jacek Anaszewski <j.anaszewski@samsung.com>
> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
> ---
>  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.
>
>  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