Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1382241 > unrolled thread

[PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

Back to article view | Back to linux.kernel

Contents

  [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups Olliver Schinagl <oliver@schinagl.nl> - 2016-04-19 09:50 +0200
    [PATCH 5/6] leds: pca963x: Inform the output that it is inverted Olliver Schinagl <oliver@schinagl.nl> - 2016-04-19 09:50 +0200
      Re: [PATCH 5/6] leds: pca963x: Inform the output that it is inverted Rob Herring <robh@kernel.org> - 2016-04-21 17:10 +0200
        Re: [PATCH 5/6] leds: pca963x: Inform the output that it is inverted Olliver Schinagl <oliver@schinagl.nl> - 2016-04-22 14:40 +0200
          Re: [PATCH 5/6] leds: pca963x: Inform the output that it is inverted Rob Herring <robh@kernel.org> - 2016-04-22 15:20 +0200
            Re: [PATCH 5/6] leds: pca963x: Inform the output that it is inverted Olliver Schinagl <oliver@schinagl.nl> - 2016-04-22 17:50 +0200
    Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups Jacek Anaszewski <j.anaszewski@samsung.com> - 2016-04-19 11:30 +0200
      Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and  cleanups Olliver Schinagl <oliver@schinagl.nl> - 2016-04-19 11:50 +0200
        Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> - 2016-04-19 13:20 +0200
          Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and  cleanups Olliver Schinagl <oliver@schinagl.nl> - 2016-04-19 15:30 +0200
            Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> - 2016-04-19 15:50 +0200
              Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and  cleanups Olliver Schinagl <oliver@schinagl.nl> - 2016-04-20 09:30 +0200
                Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> - 2016-04-20 10:10 +0200
                  Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and  cleanups Olliver Schinagl <oliver@schinagl.nl> - 2016-04-20 11:00 +0200
                    Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> - 2016-04-20 11:00 +0200
                      Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and  cleanups Olliver Schinagl <oliver@schinagl.nl> - 2016-04-20 11:10 +0200
                        Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> - 2016-04-20 11:20 +0200
                          Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and  cleanups Olliver Schinagl <oliver@schinagl.nl> - 2016-04-20 12:20 +0200
                          Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and  cleanups Olliver Schinagl <oliver@schinagl.nl> - 2016-04-22 09:30 +0200

#1382241 — [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

Using the pca963x for a while, I noticed something that may look like some
i2c accessing issues where sometimes data was incorrectly written to the bus,
possibly because we where not properly locking the i2c reads. Though I'm not
familiar enough with the i2c framework to be certain reads need to be locked
at all. A patch was added to properly lock i2c access more tightly.

Furthermore there was no method to support inverted outputs. This series
adds a property to the device tree to inform the driver that the output
is inverted (active-high vs active-low).

Additionally, this patch set does some cleanups to please checkpatch, and
removes a few magic values.

Olliver Schinagl (6):
  leds: pca963x: Alphabetize headers
  leds: pca963x: Lock i2c r/w access
  leds: pca963x: Add defines and remove some magic values
  leds: pca963x: Reduce magic values
  leds: pca963x: Inform the output that it is inverted
  leds: pca963x: Remove whitespace and checkpatch problems

 Documentation/devicetree/bindings/leds/pca963x.txt |   1 +
 drivers/leds/leds-pca963x.c                        | 243 ++++++++++++++-------
 include/linux/platform_data/leds-pca963x.h         |   1 +
 3 files changed, 171 insertions(+), 74 deletions(-)

-- 
2.8.0.rc3

[toc] | [next] | [standalone]

#1382244 — [PATCH 5/6] leds: pca963x: Inform the output that it is inverted

When leds are connected in a totem-pole configuration, they can be
connected either in a active-high, or active-low manor. The driver
currently always assumes active-high. This patch adds the
'nxp,inverted-out' boolean property to tell the driver that the leds
are driven active-low, or rather, that the behavior is inverted to what
is normally expected.

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
 Documentation/devicetree/bindings/leds/pca963x.txt |  1 +
 drivers/leds/leds-pca963x.c                        | 20 +++++++++++++-------
 include/linux/platform_data/leds-pca963x.h         |  1 +
 3 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/leds/pca963x.txt b/Documentation/devicetree/bindings/leds/pca963x.txt
index dafbe99..7b23725 100644
--- a/Documentation/devicetree/bindings/leds/pca963x.txt
+++ b/Documentation/devicetree/bindings/leds/pca963x.txt
@@ -6,6 +6,7 @@ Required properties:
 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,inverted-out: the connected leds are active-low, default to active-high
 - nxp,hw-blink : use hardware blinking instead of software blinking
 
 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 14690f2..85dd506 100644
--- a/drivers/leds/leds-pca963x.c
+++ b/drivers/leds/leds-pca963x.c
@@ -370,6 +370,9 @@ pca963x_dt_init(struct i2c_client *client, struct pca963x_chipdef *chip)
 	else
 		pdata->outdrv = PCA963X_OPEN_DRAIN;
 
+	/* default to normal output unless inverted output is specified */
+	pdata->inverted_out = of_property_read_bool(np, "nxp,inverted-out");
+
 	/* default to software blinking unless hardware blinking is specified */
 	if (of_property_read_bool(np, "nxp,hw-blink"))
 		pdata->blink_type = PCA963X_HW_BLINK;
@@ -478,14 +481,17 @@ static int pca963x_probe(struct i2c_client *client,
 	i2c_smbus_write_byte_data(client, PCA963X_MODE1, 0x00);
 
 	if (pdata) {
+		/* Always enable LED output */
+		u8 mode2 = PCA963X_MODE2_OUTNE_OUTDRV;
+
 		/* Configure output: open-drain or totem pole (push-pull) */
-		if (pdata->outdrv == PCA963X_OPEN_DRAIN)
-			i2c_smbus_write_byte_data(client, PCA963X_MODE2,
-					PCA963X_MODE2_OUTNE_OUTDRV);
-		else
-			i2c_smbus_write_byte_data(client, PCA963X_MODE2,
-					PCA963X_MODE2_OUTNE_OUTDRV |
-					PCA963X_MODE2_OUTDRV_TOTEM_POLE);
+		if (pdata->outdrv == PCA963X_TOTEM_POLE)
+			mode2 |= PCA963X_MODE2_OUTDRV_TOTEM_POLE;
+		/* Configure output: inverted output */
+		if (pdata->inverted_out)
+			mode2 |= PCA963X_MODE2_INVRT;
+
+		i2c_smbus_write_byte_data(client, PCA963X_MODE2, mode2);
 	}
 
 	return 0;
diff --git a/include/linux/platform_data/leds-pca963x.h b/include/linux/platform_data/leds-pca963x.h
index e731f00..6f784d4 100644
--- a/include/linux/platform_data/leds-pca963x.h
+++ b/include/linux/platform_data/leds-pca963x.h
@@ -36,6 +36,7 @@ enum pca963x_blink_type {
 struct pca963x_platform_data {
 	struct led_platform_data leds;
 	enum pca963x_outdrv outdrv;
+	bool inverted_out;
 	enum pca963x_blink_type blink_type;
 };
 
-- 
2.8.0.rc3

[toc] | [prev] | [next] | [standalone]

#1384332 — Re: [PATCH 5/6] leds: pca963x: Inform the output that it is inverted

On Tue, Apr 19, 2016 at 09:40:49AM +0200, Olliver Schinagl wrote:
> When leds are connected in a totem-pole configuration, they can be
> connected either in a active-high, or active-low manor. The driver
> currently always assumes active-high. This patch adds the
> 'nxp,inverted-out' boolean property to tell the driver that the leds
> are driven active-low, or rather, that the behavior is inverted to what
> is normally expected.

How do I know what is normally expected?

> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> ---
>  Documentation/devicetree/bindings/leds/pca963x.txt |  1 +
>  drivers/leds/leds-pca963x.c                        | 20 +++++++++++++-------
>  include/linux/platform_data/leds-pca963x.h         |  1 +
>  3 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/leds/pca963x.txt b/Documentation/devicetree/bindings/leds/pca963x.txt
> index dafbe99..7b23725 100644
> --- a/Documentation/devicetree/bindings/leds/pca963x.txt
> +++ b/Documentation/devicetree/bindings/leds/pca963x.txt
> @@ -6,6 +6,7 @@ Required properties:
>  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,inverted-out: the connected leds are active-low, default to active-high

Just state what mode you want: nxp,active-low

Rob

[toc] | [prev] | [next] | [standalone]

#1385123 — Re: [PATCH 5/6] leds: pca963x: Inform the output that it is inverted

Hey Rob,

On 21-04-16 17:07, Rob Herring wrote:
> On Tue, Apr 19, 2016 at 09:40:49AM +0200, Olliver Schinagl wrote:
>> When leds are connected in a totem-pole configuration, they can be
>> connected either in a active-high, or active-low manor. The driver
>> currently always assumes active-high. This patch adds the
>> 'nxp,inverted-out' boolean property to tell the driver that the leds
>> are driven active-low, or rather, that the behavior is inverted to what
>> is normally expected.
> How do I know what is normally expected?
fair point, and in fact, you don't. The text is bad here.
>
>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>> ---
>>   Documentation/devicetree/bindings/leds/pca963x.txt |  1 +
>>   drivers/leds/leds-pca963x.c                        | 20 +++++++++++++-------
>>   include/linux/platform_data/leds-pca963x.h         |  1 +
>>   3 files changed, 15 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/leds/pca963x.txt b/Documentation/devicetree/bindings/leds/pca963x.txt
>> index dafbe99..7b23725 100644
>> --- a/Documentation/devicetree/bindings/leds/pca963x.txt
>> +++ b/Documentation/devicetree/bindings/leds/pca963x.txt
>> @@ -6,6 +6,7 @@ Required properties:
>>   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,inverted-out: the connected leds are active-low, default to active-high
> Just state what mode you want: nxp,active-low
But that's not what happens, which is why my text is bad :) It depends 
on how the board is wired and if it is push-pull or open-drain. Though 
this goes beyond my electronics knowledge. So I'll reduce the text to 
say exactly what we mean, inverted output (or not).

Unless you can explain that it would be unrelated and it is actually 
active-high/low. I'll be more than happy to oblige.

Olliver
>
> Rob

[toc] | [prev] | [next] | [standalone]

#1385155 — Re: [PATCH 5/6] leds: pca963x: Inform the output that it is inverted

On Fri, Apr 22, 2016 at 7:38 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
> Hey Rob,
>
> On 21-04-16 17:07, Rob Herring wrote:
>>
>> On Tue, Apr 19, 2016 at 09:40:49AM +0200, Olliver Schinagl wrote:
>>>
>>> When leds are connected in a totem-pole configuration, they can be
>>> connected either in a active-high, or active-low manor. The driver
>>> currently always assumes active-high. This patch adds the
>>> 'nxp,inverted-out' boolean property to tell the driver that the leds
>>> are driven active-low, or rather, that the behavior is inverted to what
>>> is normally expected.
>>
>> How do I know what is normally expected?
>
> fair point, and in fact, you don't. The text is bad here.

The problem is not so much the text here, but the property is also
meaningless without some explanation.

>>
>>
>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>>> ---
>>>   Documentation/devicetree/bindings/leds/pca963x.txt |  1 +
>>>   drivers/leds/leds-pca963x.c                        | 20
>>> +++++++++++++-------
>>>   include/linux/platform_data/leds-pca963x.h         |  1 +
>>>   3 files changed, 15 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/leds/pca963x.txt
>>> b/Documentation/devicetree/bindings/leds/pca963x.txt
>>> index dafbe99..7b23725 100644
>>> --- a/Documentation/devicetree/bindings/leds/pca963x.txt
>>> +++ b/Documentation/devicetree/bindings/leds/pca963x.txt
>>> @@ -6,6 +6,7 @@ Required properties:
>>>   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,inverted-out: the connected leds are active-low, default to
>>> active-high
>>
>> Just state what mode you want: nxp,active-low
>
> But that's not what happens, which is why my text is bad :) It depends on
> how the board is wired and if it is push-pull or open-drain. Though this
> goes beyond my electronics knowledge. So I'll reduce the text to say exactly
> what we mean, inverted output (or not).
>
> Unless you can explain that it would be unrelated and it is actually
> active-high/low. I'll be more than happy to oblige.

I'm not familiar with this chip, but googling for "active low LED
circuit" can give you lots of examples. To put it simply, you are
defining whether the control/switch is on the cathode or anode side of
the LED. Open-drain vs. push-pull is also related to how the circuit
is done, but is probably an independent property. I'd think the only
reason you would use open-drain here is if you wanted to control the
LED from 2 different signals. Whether you wanted the controls to
function as OR or AND logic to turn on would determine what the active
state needs to be.

Rob

[toc] | [prev] | [next] | [standalone]

#1385291 — Re: [PATCH 5/6] leds: pca963x: Inform the output that it is inverted

On April 22, 2016 3:09:35 PM CEST, Rob Herring <robh@kernel.org> wrote:
>On Fri, Apr 22, 2016 at 7:38 AM, Olliver Schinagl <oliver@schinagl.nl>
>wrote:
>> Hey Rob,
>>
>> On 21-04-16 17:07, Rob Herring wrote:
>>>
>>> On Tue, Apr 19, 2016 at 09:40:49AM +0200, Olliver Schinagl wrote:
>>>>
>>>> When leds are connected in a totem-pole configuration, they can be
>>>> connected either in a active-high, or active-low manor. The driver
>>>> currently always assumes active-high. This patch adds the
>>>> 'nxp,inverted-out' boolean property to tell the driver that the
>leds
>>>> are driven active-low, or rather, that the behavior is inverted to
>what
>>>> is normally expected.
>>>
>>> How do I know what is normally expected?
>>
>> fair point, and in fact, you don't. The text is bad here.
>
>The problem is not so much the text here, but the property is also
>meaningless without some explanation.
>
Well the datasheet states only that it will invert the output. So would it make sense to just state the same here?
>>>
>>>
>>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>>>> ---
>>>>   Documentation/devicetree/bindings/leds/pca963x.txt |  1 +
>>>>   drivers/leds/leds-pca963x.c                        | 20
>>>> +++++++++++++-------
>>>>   include/linux/platform_data/leds-pca963x.h         |  1 +
>>>>   3 files changed, 15 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/leds/pca963x.txt
>>>> b/Documentation/devicetree/bindings/leds/pca963x.txt
>>>> index dafbe99..7b23725 100644
>>>> --- a/Documentation/devicetree/bindings/leds/pca963x.txt
>>>> +++ b/Documentation/devicetree/bindings/leds/pca963x.txt
>>>> @@ -6,6 +6,7 @@ Required properties:
>>>>   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,inverted-out: the connected leds are active-low, default to
>>>> active-high
>>>
>>> Just state what mode you want: nxp,active-low
>>
>> But that's not what happens, which is why my text is bad :) It
>depends on
>> how the board is wired and if it is push-pull or open-drain. Though
>this
>> goes beyond my electronics knowledge. So I'll reduce the text to say
>exactly
>> what we mean, inverted output (or not).
>>
>> Unless you can explain that it would be unrelated and it is actually
>> active-high/low. I'll be more than happy to oblige.
>
>I'm not familiar with this chip, but googling for "active low LED
>circuit" can give you lots of examples. To put it simply, you are
>defining whether the control/switch is on the cathode or anode side of
>the LED. Open-drain vs. push-pull is also related to how the circuit
>is done, but is probably an independent property. I'd think the only
>reason you would use open-drain here is if you wanted to control the
>LED from 2 different signals. Whether you wanted the controls to
>function as OR or AND logic to turn on would determine what the active
>state needs to be.
I was thinking that the output behaves differently depending whether it is configured as push-pull or open-drain.

So to summarize, does the output always go into active-low mode, or does it depend on other parameters as well?

Olliver
>
>Rob

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[toc] | [prev] | [next] | [standalone]

#1382318

Hi Olliver,

Thanks for the patches.
Adding driver authors on cc.

On 04/19/2016 09:40 AM, Olliver Schinagl wrote:
> Using the pca963x for a while, I noticed something that may look like some
> i2c accessing issues where sometimes data was incorrectly written to the bus,
> possibly because we where not properly locking the i2c reads. Though I'm not
> familiar enough with the i2c framework to be certain reads need to be locked
> at all. A patch was added to properly lock i2c access more tightly.
>
> Furthermore there was no method to support inverted outputs. This series
> adds a property to the device tree to inform the driver that the output
> is inverted (active-high vs active-low).
>
> Additionally, this patch set does some cleanups to please checkpatch, and
> removes a few magic values.
>
> Olliver Schinagl (6):
>    leds: pca963x: Alphabetize headers
>    leds: pca963x: Lock i2c r/w access
>    leds: pca963x: Add defines and remove some magic values
>    leds: pca963x: Reduce magic values
>    leds: pca963x: Inform the output that it is inverted
>    leds: pca963x: Remove whitespace and checkpatch problems
>
>   Documentation/devicetree/bindings/leds/pca963x.txt |   1 +
>   drivers/leds/leds-pca963x.c                        | 243 ++++++++++++++-------
>   include/linux/platform_data/leds-pca963x.h         |   1 +
>   3 files changed, 171 insertions(+), 74 deletions(-)
>


-- 
Best regards,
Jacek Anaszewski

[toc] | [prev] | [next] | [standalone]

#1382334 — Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

On 19-04-16 11:23, Jacek Anaszewski wrote:
> Hi Olliver,
>
> Thanks for the patches.
> Adding driver authors on cc.
Ah sorry about that, thanks. I guess get_maintainers doesn't do that.

As for the compile bug, I'll fix that with v2, it only applies on the 
intermediate patches, not on the whole set.

Olliver
>
> On 04/19/2016 09:40 AM, Olliver Schinagl wrote:
>> Using the pca963x for a while, I noticed something that may look like 
>> some
>> i2c accessing issues where sometimes data was incorrectly written to 
>> the bus,
>> possibly because we where not properly locking the i2c reads. Though 
>> I'm not
>> familiar enough with the i2c framework to be certain reads need to be 
>> locked
>> at all. A patch was added to properly lock i2c access more tightly.
>>
>> Furthermore there was no method to support inverted outputs. This series
>> adds a property to the device tree to inform the driver that the output
>> is inverted (active-high vs active-low).
>>
>> Additionally, this patch set does some cleanups to please checkpatch, 
>> and
>> removes a few magic values.
>>
>> Olliver Schinagl (6):
>>    leds: pca963x: Alphabetize headers
>>    leds: pca963x: Lock i2c r/w access
>>    leds: pca963x: Add defines and remove some magic values
>>    leds: pca963x: Reduce magic values
>>    leds: pca963x: Inform the output that it is inverted
>>    leds: pca963x: Remove whitespace and checkpatch problems
>>
>>   Documentation/devicetree/bindings/leds/pca963x.txt |   1 +
>>   drivers/leds/leds-pca963x.c                        | 243 
>> ++++++++++++++-------
>>   include/linux/platform_data/leds-pca963x.h         |   1 +
>>   3 files changed, 171 insertions(+), 74 deletions(-)
>>
>
>

[toc] | [prev] | [next] | [standalone]

#1382401

Hi Ollivier

Sorry to not reply to the patches, but I am not subscribed to linux-leds

Regarding:
[PATCH 2/6] leds: pca963x: Lock i2c r/w access

I am not sure why this patch is needed. the only thing that should be
protected is the write to ledout.

It seems that mode2 needs to be set to PCA963X_MODE2_DMBLNK, or at
least, the driver never clears that bit. Couldnt we just set it at
probe time and remove the read/write of it? I do not have the hardware
at the moment, so it should be something that you need to test.

[PATCH 4/6] leds: pca963x: Reduce magic values
Maybe you want to create the inverse macro of PCA963X_LEDOUT_LDR, so
you can do something linke

PCA963X_LEDOUT_LDR_INV(ledout, pca963x->led_num) != PCA963X_LEDOUT_LED_GRP_PWM


[PATCH 3/6] leds: pca963x: Add defines and remove some magic values

I am not big fan of defining things that are not used. and the magic
assigment to n_leds is perfectly fine IMHO

For PCA963X_LEDOUT_LDR.  Do not forget the parenthesis around led_num.
Also replace %4 with &3 to be consisten.t

Regards!

On Tue, Apr 19, 2016 at 11:39 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
> On 19-04-16 11:23, Jacek Anaszewski wrote:
>>
>> Hi Olliver,
>>
>> Thanks for the patches.
>> Adding driver authors on cc.
>
> Ah sorry about that, thanks. I guess get_maintainers doesn't do that.
>
> As for the compile bug, I'll fix that with v2, it only applies on the
> intermediate patches, not on the whole set.
>
> Olliver
>
>>
>> On 04/19/2016 09:40 AM, Olliver Schinagl wrote:
>>>
>>> Using the pca963x for a while, I noticed something that may look like
>>> some
>>> i2c accessing issues where sometimes data was incorrectly written to the
>>> bus,
>>> possibly because we where not properly locking the i2c reads. Though I'm
>>> not
>>> familiar enough with the i2c framework to be certain reads need to be
>>> locked
>>> at all. A patch was added to properly lock i2c access more tightly.
>>>
>>> Furthermore there was no method to support inverted outputs. This series
>>> adds a property to the device tree to inform the driver that the output
>>> is inverted (active-high vs active-low).
>>>
>>> Additionally, this patch set does some cleanups to please checkpatch, and
>>> removes a few magic values.
>>>
>>> Olliver Schinagl (6):
>>>    leds: pca963x: Alphabetize headers
>>>    leds: pca963x: Lock i2c r/w access
>>>    leds: pca963x: Add defines and remove some magic values
>>>    leds: pca963x: Reduce magic values
>>>    leds: pca963x: Inform the output that it is inverted
>>>    leds: pca963x: Remove whitespace and checkpatch problems
>>>
>>>   Documentation/devicetree/bindings/leds/pca963x.txt |   1 +
>>>   drivers/leds/leds-pca963x.c                        | 243
>>> ++++++++++++++-------
>>>   include/linux/platform_data/leds-pca963x.h         |   1 +
>>>   3 files changed, 171 insertions(+), 74 deletions(-)
>>>
>>
>>
>



-- 
Ricardo Ribalda

[toc] | [prev] | [next] | [standalone]

#1382510 — Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

Hey Ricardo,

On 19-04-16 13:18, Ricardo Ribalda Delgado wrote:
> Hi Ollivier
>
> Sorry to not reply to the patches, but I am not subscribed to linux-leds
>
> Regarding:
> [PATCH 2/6] leds: pca963x: Lock i2c r/w access
>
> I am not sure why this patch is needed. the only thing that should be
> protected is the write to ledout.
>
> It seems that mode2 needs to be set to PCA963X_MODE2_DMBLNK, or at
> least, the driver never clears that bit. Couldnt we just set it at
> probe time and remove the read/write of it? I do not have the hardware
> at the moment, so it should be something that you need to test.
Without actually looking at the code right now, but the driver does a 
read/modify/write on the register, and a register is shared among 
several leds. So in that regard, it makes sense and I don't think it's 
very expensive to move the lock, since we have to lock for the write a 
few lines down anyway.
>
> [PATCH 4/6] leds: pca963x: Reduce magic values
> Maybe you want to create the inverse macro of PCA963X_LEDOUT_LDR, so
> you can do something linke
>
> PCA963X_LEDOUT_LDR_INV(ledout, pca963x->led_num) != PCA963X_LEDOUT_LED_GRP_PWM
Good point, I'll add it, I like it.
>
>
> [PATCH 3/6] leds: pca963x: Add defines and remove some magic values
>
> I am not big fan of defining things that are not used. and the magic
> assigment to n_leds is perfectly fine IMHO
Well i needed some of the defines for the invert part and then I figured 
just add everything that the datasheet defines to make everything 
exlusive/easy to use.

But I can remove unused defines if desired.
>
> For PCA963X_LEDOUT_LDR.  Do not forget the parenthesis around led_num.
> Also replace %4 with &3 to be consisten.t
Yeah, i'll check and fix that.
>
> Regards!
>
> On Tue, Apr 19, 2016 at 11:39 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
>> On 19-04-16 11:23, Jacek Anaszewski wrote:
>>> Hi Olliver,
>>>
>>> Thanks for the patches.
>>> Adding driver authors on cc.
>> Ah sorry about that, thanks. I guess get_maintainers doesn't do that.
>>
>> As for the compile bug, I'll fix that with v2, it only applies on the
>> intermediate patches, not on the whole set.
>>
>> Olliver
>>
>>> On 04/19/2016 09:40 AM, Olliver Schinagl wrote:
>>>> Using the pca963x for a while, I noticed something that may look like
>>>> some
>>>> i2c accessing issues where sometimes data was incorrectly written to the
>>>> bus,
>>>> possibly because we where not properly locking the i2c reads. Though I'm
>>>> not
>>>> familiar enough with the i2c framework to be certain reads need to be
>>>> locked
>>>> at all. A patch was added to properly lock i2c access more tightly.
>>>>
>>>> Furthermore there was no method to support inverted outputs. This series
>>>> adds a property to the device tree to inform the driver that the output
>>>> is inverted (active-high vs active-low).
>>>>
>>>> Additionally, this patch set does some cleanups to please checkpatch, and
>>>> removes a few magic values.
>>>>
>>>> Olliver Schinagl (6):
>>>>     leds: pca963x: Alphabetize headers
>>>>     leds: pca963x: Lock i2c r/w access
>>>>     leds: pca963x: Add defines and remove some magic values
>>>>     leds: pca963x: Reduce magic values
>>>>     leds: pca963x: Inform the output that it is inverted
>>>>     leds: pca963x: Remove whitespace and checkpatch problems
>>>>
>>>>    Documentation/devicetree/bindings/leds/pca963x.txt |   1 +
>>>>    drivers/leds/leds-pca963x.c                        | 243
>>>> ++++++++++++++-------
>>>>    include/linux/platform_data/leds-pca963x.h         |   1 +
>>>>    3 files changed, 171 insertions(+), 74 deletions(-)
>>>>
>>>
>
>

[toc] | [prev] | [next] | [standalone]

#1382534

Hi again

On Tue, Apr 19, 2016 at 3:27 PM, Olliver Schinagl <oliver@schinagl.nl> wrote:
> Hey Ricardo,

> Without actually looking at the code right now, but the driver does a
> read/modify/write on the register, and a register is shared among several
> leds. So in that regard, it makes sense and I don't think it's very
> expensive to move the lock, since we have to lock for the write a few lines
> down anyway.

Actually, the code is only making sure that PCA963X_MODE2_DMBLNK is
on. It is never cleared afterwards.

It will be great if you could set that bit on probe and remove those
two lines and verify that it works on real hardware.


The move of the lock can be a bit expensive. i2c writes can take a
while to be performed, this is why only ledout was protected
initially.

Best regards

[toc] | [prev] | [next] | [standalone]

#1383106 — Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

Hey Ricardo,

On 19-04-16 15:42, Ricardo Ribalda Delgado wrote:
> Hi again
>
> On Tue, Apr 19, 2016 at 3:27 PM, Olliver Schinagl <oliver@schinagl.nl> wrote:
>> Hey Ricardo,
>> Without actually looking at the code right now, but the driver does a
>> read/modify/write on the register, and a register is shared among several
>> leds. So in that regard, it makes sense and I don't think it's very
>> expensive to move the lock, since we have to lock for the write a few lines
>> down anyway.
> Actually, the code is only making sure that PCA963X_MODE2_DMBLNK is
> on. It is never cleared afterwards.
i do not think this can work at all actually.

While trying to move those lines to probe and thinking about the 
consequences, I noticed blink is now never enabled again.
E.g. the probe reads the blink bit at probe, updates its internal 
trigger to timer etc and now during probe, if there is no 
default-trigger, we now have the correct trigger set.

However, when we enable blink via the timer trigger for example, the 
blink_set() gets executed and it writes the blink bit.

mode2 = i2c_smbus_read_byte_data(pca963x->chip->client, PCA963X_MODE2);
  	if (!(mode2 & PCA963X_MODE2_DMBLNK))
  		i2c_smbus_write_byte_data(pca963x->chip->client, PCA963X_MODE2,
  			mode2 | PCA963X_MODE2_DMBLNK);


so after the read, we immediatly do a write.

Now I understand your concern, the i2c operations are slow and time 
consuming making the mutex very expensive.

The thing is, to be able to write the blink bit, we need to read the 
whole mode2 register, to do a proper read-modify-write. We don't know 
what's in the mode2 register and we only want to write the bit if it is 
actually not set to begin with, to save a i2c write operation.

We start this function already however with with two write calls of 
sequential registers, the grp and pwm enable registers. There is even a 
call to automatically update these registers, which I think we'd use 
i2c_master_send() to set the address via the auto-increment register and 
enable auto increment of these two registers. Now we reduced the 2 
seperate calls into one bigger 'faster' call. So 1 win there. But! it 
will require us however to change the other calls to disable auto 
increment via de mode1 register. Since this is an extra i2c_write 
operation, it makes the other i2c writes more expensive, which may 
happen much more often (enable blink only happens occasionally, changing 
the brightness may change a lot (fade in fade out).

So unless i'm totally misunderstanding something, I don't think we can 
safe much here at all.

The only win would be by not reading the mode2 in the mutex, but what if 
we read the register, someone else modifies it, and we write to it again?

olliver

>
> It will be great if you could set that bit on probe and remove those
> two lines and verify that it works on real hardware.
>
>
> The move of the lock can be a bit expensive. i2c writes can take a
> while to be performed, this is why only ledout was protected
> initially.
>
> Best regards

[toc] | [prev] | [next] | [standalone]

#1383134

Hi Ollivier


On Wed, Apr 20, 2016 at 9:21 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:

What I am propossing is at probe():

replace:

if (pdata) {
/* Configure output: open-drain or totem pole (push-pull) */
if (pdata->outdrv == PCA963X_OPEN_DRAIN)
i2c_smbus_write_byte_data(client, PCA963X_MODE2, 0x01);
else
i2c_smbus_write_byte_data(client, PCA963X_MODE2, 0x05);
}

with something like (forgive the spacing):


u8 mode2 = PCA963X_MODE2_DMBLNK | 0x1;
if (pdata && pdata->outdrv == PCA963X_OPEN_DRAIN)
   mode2 |= 0x4;
i2c_smbus_write_byte_data(pca963x->chip->client, PCA963X_MODE2, mode2);


and then remove from pca963x_blink() these lines:

u8 mode2 = i2c_smbus_read_byte_data(pca963x->chip->client,
PCA963X_MODE2);

if (!(mode2 & PCA963X_MODE2_DMBLNK))
i2c_smbus_write_byte_data(pca963x->chip->client, PCA963X_MODE2,
mode2 | PCA963X_MODE2_DMBLNK);

As I said before, the reason for this proposal is that the code NEVER
clears PCA963X_MODE2_DMBLNK, only sets it.
Unfortunately I do not have the HW to test this change.

>
> Now I understand your concern, the i2c operations are slow and time
> consuming making the mutex very expensive.
>
> The thing is, to be able to write the blink bit, we need to read the whole
> mode2 register, to do a proper read-modify-write. We don't know what's in
> the mode2 register and we only want to write the bit if it is actually not
> set to begin with, to save a i2c write operation.

As I said earlier, nowhere in the code clears that bit. The bit is
only set. so no reason to read/modify/write. We can set that bit at
probe time and assume that it will not be changed.

>
> We start this function already however with with two write calls of
> sequential registers, the grp and pwm enable registers. There is even a call
> to automatically update these registers, which I think we'd use
> i2c_master_send() to set the address via the auto-increment register and
> enable auto increment of these two registers. Now we reduced the 2 seperate
> calls into one bigger 'faster' call. So 1 win there. But! it will require us
> however to change the other calls to disable auto increment via de mode1
> register. Since this is an extra i2c_write operation, it makes the other i2c
> writes more expensive, which may happen much more often (enable blink only
> happens occasionally, changing the brightness may change a lot (fade in fade
> out).

Be careful with that. Sometimes this chips are connected to the smbus,
wich has a limited number of operations/lengths. We should keep the
i2c_smbus_write_byte_data() call, because that makes the driver
compatible with more hardware.

>
> So unless i'm totally misunderstanding something, I don't think we can safe
> much here at all.

To be clear: My proposal is that (after being tested), move the set of
DMBLINK to probe, remove the read/modify/write from blink() and keep
the locking as it is now, only protecting ledout.

Also you need to fix the patch that breaks bisect, but I believe that
you are already working on that.

I have reviewed a bit Documentation/device-tree and it seems that
there is already a binding for active-low. Instead of nxp,active-low
you should call it just active-low. But I am not a device-tree expert.

Finally, you mention that you are fixing some checkpatch errors, but I
cannot replicate those in my side :S

ricardo@pilix:~/curro/linux$ scripts/checkpatch.pl -f
drivers/leds/leds-pca963x.c
total: 0 errors, 0 warnings, 439 lines checked

drivers/leds/leds-pca963x.c has no obvious style problems and is ready
for submission.

So maybe the errors you are fixing are introduced by your patches.
About the other style patches, I do not know what is the policy of the
Maintainer in that matter, especially when the driver did not break
originally checkpatch.



Regards!


---
Ricardo

[toc] | [prev] | [next] | [standalone]

#1383172 — Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

Hey Ricardo,

On 20-04-16 10:01, Ricardo Ribalda Delgado wrote:
> Hi Ollivier
>
>
> On Wed, Apr 20, 2016 at 9:21 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
>
> What I am propossing is at probe():
>
> replace:
>
> if (pdata) {
> /* Configure output: open-drain or totem pole (push-pull) */
> if (pdata->outdrv == PCA963X_OPEN_DRAIN)
> i2c_smbus_write_byte_data(client, PCA963X_MODE2, 0x01);
> else
> i2c_smbus_write_byte_data(client, PCA963X_MODE2, 0x05);
> }
>
> with something like (forgive the spacing):
>
>
> u8 mode2 = PCA963X_MODE2_DMBLNK | 0x1;
> if (pdata && pdata->outdrv == PCA963X_OPEN_DRAIN)
>     mode2 |= 0x4;
> i2c_smbus_write_byte_data(pca963x->chip->client, PCA963X_MODE2, mode2);
>
>
> and then remove from pca963x_blink() these lines:
>
> u8 mode2 = i2c_smbus_read_byte_data(pca963x->chip->client,
> PCA963X_MODE2);
>
> if (!(mode2 & PCA963X_MODE2_DMBLNK))
> i2c_smbus_write_byte_data(pca963x->chip->client, PCA963X_MODE2,
> mode2 | PCA963X_MODE2_DMBLNK);
>
> As I said before, the reason for this proposal is that the code NEVER
> clears PCA963X_MODE2_DMBLNK, only sets it.
> Unfortunately I do not have the HW to test this change.
The code never clears it, but the hardware does. So we have to set it 
everytime we enable blink.

So I don't think this can work at all, as you now never enable blink 
(when you enable it from user space, at probe it works fine via the 
default trigger).

What about storing the mode2 register on the chip level, since we never 
change it after probe, and only toggle the blink bit in blink_set() and 
write the stored mode2 register

so i2c_write(pca963x->chip->mode2 | MODE2_DMBLNK)

The only advantage I see here though is that we save a read + (quite 
likley) potentially a write, with an always write. So it saves 1 i2c 
read command.

Olliver
>
>> Now I understand your concern, the i2c operations are slow and time
>> consuming making the mutex very expensive.
>>
>> The thing is, to be able to write the blink bit, we need to read the whole
>> mode2 register, to do a proper read-modify-write. We don't know what's in
>> the mode2 register and we only want to write the bit if it is actually not
>> set to begin with, to save a i2c write operation.
> As I said earlier, nowhere in the code clears that bit. The bit is
> only set. so no reason to read/modify/write. We can set that bit at
> probe time and assume that it will not be changed.
>
>> We start this function already however with with two write calls of
>> sequential registers, the grp and pwm enable registers. There is even a call
>> to automatically update these registers, which I think we'd use
>> i2c_master_send() to set the address via the auto-increment register and
>> enable auto increment of these two registers. Now we reduced the 2 seperate
>> calls into one bigger 'faster' call. So 1 win there. But! it will require us
>> however to change the other calls to disable auto increment via de mode1
>> register. Since this is an extra i2c_write operation, it makes the other i2c
>> writes more expensive, which may happen much more often (enable blink only
>> happens occasionally, changing the brightness may change a lot (fade in fade
>> out).
> Be careful with that. Sometimes this chips are connected to the smbus,
> wich has a limited number of operations/lengths. We should keep the
> i2c_smbus_write_byte_data() call, because that makes the driver
> compatible with more hardware.
>
>> So unless i'm totally misunderstanding something, I don't think we can safe
>> much here at all.
> To be clear: My proposal is that (after being tested), move the set of
> DMBLINK to probe, remove the read/modify/write from blink() and keep
> the locking as it is now, only protecting ledout.
>
> Also you need to fix the patch that breaks bisect, but I believe that
> you are already working on that.
>
> I have reviewed a bit Documentation/device-tree and it seems that
> there is already a binding for active-low. Instead of nxp,active-low
> you should call it just active-low. But I am not a device-tree expert.
>
> Finally, you mention that you are fixing some checkpatch errors, but I
> cannot replicate those in my side :S
>
> ricardo@pilix:~/curro/linux$ scripts/checkpatch.pl -f
> drivers/leds/leds-pca963x.c
> total: 0 errors, 0 warnings, 439 lines checked
>
> drivers/leds/leds-pca963x.c has no obvious style problems and is ready
> for submission.
>
> So maybe the errors you are fixing are introduced by your patches.
> About the other style patches, I do not know what is the policy of the
> Maintainer in that matter, especially when the driver did not break
> originally checkpatch.
>
>
>
> Regards!
>
>
> ---
> Ricardo

[toc] | [prev] | [next] | [standalone]

#1383177

Hi

On Wed, Apr 20, 2016 at 10:51 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:

>> As I said before, the reason for this proposal is that the code NEVER
>> clears PCA963X_MODE2_DMBLNK, only sets it.
>> Unfortunately I do not have the HW to test this change.
>
> The code never clears it, but the hardware does. So we have to set it
> everytime we enable blink.

Ok, that was the part I was missing. I was not aware that the hw was
clearing it.

Saving mode2 sounds like a good compromise then.

But I still believe that we should limit the lock to ledout. No matter
what we do, we cannot have two leds blinking at different frequencies
on the same chip.


Regards

-- 
Ricardo Ribalda

[toc] | [prev] | [next] | [standalone]

#1383184 — Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

On 20-04-16 10:56, Ricardo Ribalda Delgado wrote:
> Hi
>
> On Wed, Apr 20, 2016 at 10:51 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
>
>>> As I said before, the reason for this proposal is that the code NEVER
>>> clears PCA963X_MODE2_DMBLNK, only sets it.
>>> Unfortunately I do not have the HW to test this change.
>> The code never clears it, but the hardware does. So we have to set it
>> everytime we enable blink.
> Ok, that was the part I was missing. I was not aware that the hw was
> clearing it.
The devil is in the details :)
> Saving mode2 sounds like a good compromise then.
>
> But I still believe that we should limit the lock to ledout. No matter
> what we do, we cannot have two leds blinking at different frequencies
> on the same chip.
So to save a mutex a little bit, we take the risk that nobody else 
enables the blink or if they do, enable it in the same way?
If it saves so much, then I guess its worth the risk I suppose?
>
>
> Regards
>

[toc] | [prev] | [next] | [standalone]

#1383186

Hello again

On Wed, Apr 20, 2016 at 11:06 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:

> The devil is in the details :)
:)
>>
>> Saving mode2 sounds like a good compromise then.
>>
>> But I still believe that we should limit the lock to ledout. No matter
>> what we do, we cannot have two leds blinking at different frequencies
>> on the same chip.
>
> So to save a mutex a little bit, we take the risk that nobody else enables
> the blink or if they do, enable it in the same way?
> If it saves so much, then I guess its worth the risk I suppose?

Give me a day to go through the chip doc and see if I can find a good
compromise, that at least warranties that the leds that are enable
stay enabled ;)

Regards!



-- 
Ricardo Ribalda

[toc] | [prev] | [next] | [standalone]

#1383240 — Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

Hey,

On 20-04-16 11:17, Ricardo Ribalda Delgado wrote:
> Hello again
>
> On Wed, Apr 20, 2016 at 11:06 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
>
>> The devil is in the details :)
> :)
>>> Saving mode2 sounds like a good compromise then.
>>>
>>> But I still believe that we should limit the lock to ledout. No matter
>>> what we do, we cannot have two leds blinking at different frequencies
>>> on the same chip.
>> So to save a mutex a little bit, we take the risk that nobody else enables
>> the blink or if they do, enable it in the same way?
>> If it saves so much, then I guess its worth the risk I suppose?
> Give me a day to go through the chip doc and see if I can find a good
> compromise, that at least warranties that the leds that are enable
> stay enabled ;)
sure thing. I have read the docs quite a few times for using it directly 
via i2c, but yeah I'll wait.
>
> Regards!
>
>
>

[toc] | [prev] | [next] | [standalone]

#1384738 — Re: [PATCHv1 0/6] leds: pca9653x: support inverted outputs and cleanups

Hi Ricardo,

On 20-04-16 11:17, Ricardo Ribalda Delgado wrote:
> Hello again
>
> On Wed, Apr 20, 2016 at 11:06 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
>
>> The devil is in the details :)
> :)
>>> Saving mode2 sounds like a good compromise then.
>>>
>>> But I still believe that we should limit the lock to ledout. No matter
>>> what we do, we cannot have two leds blinking at different frequencies
>>> on the same chip.
>> So to save a mutex a little bit, we take the risk that nobody else enables
>> the blink or if they do, enable it in the same way?
>> If it saves so much, then I guess its worth the risk I suppose?
> Give me a day to go through the chip doc and see if I can find a good
> compromise, that at least warranties that the leds that are enable
> stay enabled ;)
Right, I also went over the datasheet, and I think we can simplyfy two 
things.

For one, yes, move the mode2 register completly to the probe section. 
Set the DMBLINK led to always 1. It does not get cleared, I was wrong. 
We have to set it to as with 0 we do not get any blinking at all 
(grpfreq gets ignored).

Furthermore, we should change:
  -    gdc = (time_on * 256) / period;
+   gdc = 0x00;

Because the calculation does not make sense. GDC is the global 
brightness/pwm/dimming control. It is used to uniformly change the blink 
rate on all the linked leds.

"General brightness for the 16 outputs is controlled through 256 linear 
steps to FFh"
I don't think that is the intention of the gdc is it? Looking at the 
original gdc code, it thus sets the global BRIGHTNESS based on the 
period/on_time. I don't think that is what we expect when we enable blink.

 From my understanding, the grppwm is super-imposed, thus by setting gdc 
to 0, we do not superimpose anything and the original brightness is 
retained. (If i'm wrong here, we need to set gdc to 0xff.

Because of this, I even recommend removing gdc all together, which saves 
another i2c write.

Or am I wrong here?

Olliver
>
> Regards!
>
>
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web