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

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

[Olliver Schinagl]

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

[PATCH 1/6] leds: pca963x: Alphabetize headers

[Olliver Schinagl]

Re-order headers so they are in alphabetical order.

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
 drivers/leds/leds-pca963x.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/leds/leds-pca963x.c b/drivers/leds/leds-pca963x.c
index 407eba1..8dabf7a 100644
--- a/drivers/leds/leds-pca963x.c
+++ b/drivers/leds/leds-pca963x.c
@@ -25,16 +25,16 @@
  * or by adding the 'nxp,hw-blink' property to the DTS.
  */
 
-#include <linux/module.h>
-#include <linux/delay.h>
-#include <linux/string.h>
 #include <linux/ctype.h>
-#include <linux/leds.h>
+#include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/i2c.h>
-#include <linux/slab.h>
+#include <linux/leds.h>
+#include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_data/leds-pca963x.h>
+#include <linux/slab.h>
+#include <linux/string.h>
 
 /* LED select registers determine the source that drives LED outputs */
 #define PCA963X_LED_OFF		0x0	/* LED driver off */
-- 
2.8.0.rc3

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

[Olliver Schinagl]

A pca963x device can have multiple leds with a single i2c channel to
access them. Some of the registers are shared between each other. To
ensure all i2c operations are atomic within an instance, we move some
mutex locks slightly around to guard these access.

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
 drivers/leds/leds-pca963x.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/leds/leds-pca963x.c b/drivers/leds/leds-pca963x.c
index 8dabf7a..1c30e92 100644
--- a/drivers/leds/leds-pca963x.c
+++ b/drivers/leds/leds-pca963x.c
@@ -99,7 +99,7 @@ struct pca963x_led;
 
 struct pca963x {
 	struct pca963x_chipdef *chipdef;
-	struct mutex mutex;
+	struct mutex mutex; /* protect i2c access to/from the pca963x chip */
 	struct i2c_client *client;
 	struct pca963x_led *leds;
 };
@@ -156,22 +156,22 @@ static void pca963x_blink(struct pca963x_led *pca963x)
 	u8 ledout_addr = pca963x->chip->chipdef->ledout_base +
 		(pca963x->led_num / 4);
 	u8 ledout;
-	u8 mode2 = i2c_smbus_read_byte_data(pca963x->chip->client,
-							PCA963X_MODE2);
+	u8 mode2;
 	int shift = 2 * (pca963x->led_num % 4);
 	u8 mask = 0x3 << shift;
 
+	mutex_lock(&pca963x->chip->mutex);
 	i2c_smbus_write_byte_data(pca963x->chip->client,
 			pca963x->chip->chipdef->grppwm,	pca963x->gdc);
 
 	i2c_smbus_write_byte_data(pca963x->chip->client,
 			pca963x->chip->chipdef->grpfreq, pca963x->gfrq);
 
+	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);
 
-	mutex_lock(&pca963x->chip->mutex);
 	ledout = i2c_smbus_read_byte_data(pca963x->chip->client, ledout_addr);
 	if ((ledout & mask) != (PCA963X_LED_GRP_PWM << shift))
 		i2c_smbus_write_byte_data(pca963x->chip->client, ledout_addr,
-- 
2.8.0.rc3

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

[Olliver Schinagl]

This patch adds some more defines so that the driver can receive
a little more future work. These new defines are then used throughout the
existing code the remove some magic values.

This patch does not produce any binary changes.

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
 drivers/leds/leds-pca963x.c | 144 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 114 insertions(+), 30 deletions(-)

diff --git a/drivers/leds/leds-pca963x.c b/drivers/leds/leds-pca963x.c
index 1c30e92..e4e668d 100644
--- a/drivers/leds/leds-pca963x.c
+++ b/drivers/leds/leds-pca963x.c
@@ -25,6 +25,7 @@
  * or by adding the 'nxp,hw-blink' property to the DTS.
  */
 
+#include <linux/bitops.h>
 #include <linux/ctype.h>
 #include <linux/delay.h>
 #include <linux/err.h>
@@ -36,17 +37,96 @@
 #include <linux/slab.h>
 #include <linux/string.h>
 
-/* LED select registers determine the source that drives LED outputs */
-#define PCA963X_LED_OFF		0x0	/* LED driver off */
-#define PCA963X_LED_ON		0x1	/* LED driver on */
-#define PCA963X_LED_PWM		0x2	/* Controlled through PWM */
-#define PCA963X_LED_GRP_PWM	0x3	/* Controlled through PWM/GRPPWM */
-
-#define PCA963X_MODE2_DMBLNK	0x20	/* Enable blinking */
-
-#define PCA963X_MODE1		0x00
-#define PCA963X_MODE2		0x01
-#define PCA963X_PWM_BASE	0x02
+/* The number of led drivers per chip */
+#define PCA9633_NUM_LEDS	4
+#define PCA9634_NUM_LEDS	8
+#define PCA9635_NUM_LEDS	16
+
+#define PCA963X_MODE1		0x00 /* Mode 1 register addr */
+#define PCA963X_MODE2		0x01 /* Mode 2 register addr */
+#define PCA963X_PWM0_ADDR	0x02 /* PWM0 duty cycle */
+#define PCA963X_PWM1_ADDR	0x03 /* PWM1 duty cycle */
+#define PCA963X_PWM2_ADDR	0x04 /* PWM2 duty cycle */
+#define PCA963X_PWM3_ADDR	0x05 /* PWM3 duty cycle */
+#define PCA963X_PWM4_ADDR	0x06 /* PWM4 duty cycle */
+#define PCA963X_PWM5_ADDR	0x07 /* PWM5 duty cycle */
+#define PCA963X_PWM6_ADDR	0x08 /* PWM6 duty cycle */
+#define PCA963X_PWM7_ADDR	0x09 /* PWM7 duty cycle */
+#define PCA963X_PWM8_ADDR	0x0a /* PWM8 duty cycle */
+#define PCA963X_PWM9_ADDR	0x0b /* PWM9 duty cycle */
+#define PCA963X_PWM10_ADDR	0x0c /* PWM10 duty cycle */
+#define PCA963X_PWM11_ADDR	0x0d /* PWM11 duty cycle */
+#define PCA963X_PWM12_ADDR	0x0e /* PWM12 duty cycle */
+#define PCA963X_PWM13_ADDR	0x0f /* PWM13 duty cycle */
+#define PCA963X_PWM14_ADDR	0x10 /* PWM14 duty cycle */
+#define PCA963X_PWM15_ADDR	0x11 /* PWM15 duty cycle */
+#define PCA9633_GRPPWM		0x06 /* Group PWM duty cycle ctrl for PCA9633 */
+#define PCA9634_GRPPWM		0x0a /* Group PWM duty cycle ctrl for PCA9634 */
+#define PCA9635_GRPPWM		0x12 /* Group PWM duty cycle ctrl for PCA9635 */
+#define PCA9633_GRPFREQ		0x07 /* Group frequency control for PCA9633 */
+#define PCA9634_GRPFREQ		0x0b /* Group frequency control for PCA9634 */
+#define PCA9635_GRPFREQ		0x13 /* Group frequency control for PCA9635 */
+#define PCA9633_LEDOUT0		0x08 /* Led output state 0 reg for PCA9633 */
+#define PCA9634_LEDOUT0		0x0c /* Led output state 0 reg for PCA9635 */
+#define PCA9634_LEDOUT1		0x0d /* Led output state 1 reg for PCA9634 */
+#define PCA9635_LEDOUT0		0x14 /* Led output state 0 reg for PCA9634 */
+#define PCA9635_LEDOUT1		0x15 /* Led output state 1 reg for PCA9635 */
+#define PCA9635_LEDOUT2		0x16 /* Led output state 2 reg for PCA9635 */
+#define PCA9635_LEDOUT3		0x17 /* Led output state 3 reg for PCA9635 */
+#define PCA9633_SUBADDR1	0x09 /* I2C subaddr 1 for PCA9633 */
+#define PCA9633_SUBADDR2	0x0a /* I2C subaddr 2 for PCA9633 */
+#define PCA9633_SUBADDR3	0x0b /* I2C subaddr 3 for PCA9633 */
+#define PCA9634_SUBADDR1	0x0e /* I2C subaddr 1 for PCA9634 */
+#define PCA9634_SUBADDR2	0x0f /* I2C subaddr 2 for PCA9634 */
+#define PCA9634_SUBADDR3	0x10 /* I2C subaddr 3 for PCA9634 */
+#define PCA9635_SUBADDR1	0x18 /* I2C subaddr 1 for PCA9635 */
+#define PCA9635_SUBADDR2	0x19 /* I2C subaddr 2 for PCA9635 */
+#define PCA9635_SUBADDR3	0x1a /* I2C subaddr 3 for PCA9635 */
+#define PCA9633_ALLCALLADDR	0x0c /* I2C Led all call address for PCA9633 */
+#define PCA9634_ALLCALLADDR	0x11 /* I2C Led all call address for PCA9634 */
+#define PCA9635_ALLCALLADDR	0x1b /* I2C Led all call address for PCA9635 */
+
+/* Helper mappings */
+#define PCA963X_PWM_ADDR(led)	(PCA963X_PWM0_ADDR + (led))
+#define PCA9633_LEDOUT_BASE	PCA9633_LEDOUT0
+#define PCA9634_LEDOUT_BASE	PCA9634_LEDOUT0
+#define PCA9635_LEDOUT_BASE	PCA9635_LEDOUT0
+
+/* MODE1 register */
+#define PCA963X_MODE1_ALLCALL_ON	BIT(0) /* Respond to LED All Call */
+#define PCA963X_MODE1_RESPOND_SUB3	BIT(1) /* Respond to Sub address 3 */
+#define PCA963X_MODE1_RESPOND_SUB2	BIT(2) /* Respond to Sub address 2 */
+#define PCA963X_MODE1_RESPOND_SUB1	BIT(3) /* Respond to Sub address 1 */
+#define PCA963X_MODE1_SLEEP		BIT(4) /* Put in low power mode */
+#define PCA963X_MODE1_AI_EN		BIT(5) /* Enable Auto-increment */
+#define PCA963X_MODE1_AI_ROLL_PWM	BIT(6) /* Auto-increment only PWM's */
+#define PCA963X_MODE1_AI_ROLL_GRP	BIT(7) /* AI only group-controls */
+
+/* MODE2 register */
+#define PCA963X_MODE2_OUTNE_OUTDRV	BIT(0) /* Outdrv determines led state */
+#define PCA963X_MODE2_OUTNE_HIZ		BIT(1) /* Led-state in Hi-Z */
+#define PCA963X_MODE2_OUTDRV_TOTEM_POLE	BIT(2) /* Outputs are totem-pole'd */
+#define PCA963X_MODE2_OCH_ACK		BIT(3) /* Out change on ACK else STOP */
+#define PCA963X_MODE2_INVRT		BIT(4) /* Output logic state inverted */
+#define PCA963X_MODE2_DMBLNK		BIT(5) /* Grp-ctrl blink else dimming */
+
+/* LED driver output state */
+#define PCA963X_LEDOUT_LED_OFF		0x0 /* LED off */
+#define PCA963X_LEDOUT_LED_ON		0x1 /* LED fully-on */
+#define PCA963X_LEDOUT_LED_PWM		0x2 /* LED PWM mode */
+#define PCA963X_LEDOUT_LED_GRP_PWM	0x3 /* LED PWM + group PWM mode */
+
+#define PCA963X_LEDOUT_MASK		PCA963X_LEDOUT_LED_GRP_PWM
+#define PCA963X_LEDOUT_LDR(x, led_num)	\
+	(((x) & PCA963X_LEDOUT_MASK) << ((led_num % 4) << 1))
+
+/* Addressing register helpers */
+#define PCA963X_SUBADDR_SET(x)		(((x) << 1) & 0xfe)
+#define PCA963X_ALLCALLADDR_SET(x)	(((x) << 1) & 0xfe)
+
+/* Software reset password */
+#define PCA963X_PASSKEY1		0xa5
+#define PCA963X_PASSKEY2		0x5a
 
 enum pca963x_type {
 	pca9633,
@@ -63,22 +143,22 @@ struct pca963x_chipdef {
 
 static struct pca963x_chipdef pca963x_chipdefs[] = {
 	[pca9633] = {
-		.grppwm		= 0x6,
-		.grpfreq	= 0x7,
-		.ledout_base	= 0x8,
-		.n_leds		= 4,
+		.grppwm		= PCA9633_GRPPWM,
+		.grpfreq	= PCA9633_GRPFREQ,
+		.ledout_base	= PCA9633_LEDOUT_BASE,
+		.n_leds		= PCA9633_NUM_LEDS,
 	},
 	[pca9634] = {
-		.grppwm		= 0xa,
-		.grpfreq	= 0xb,
-		.ledout_base	= 0xc,
-		.n_leds		= 8,
+		.grppwm		= PCA9634_GRPPWM,
+		.grpfreq	= PCA9634_GRPFREQ,
+		.ledout_base	= PCA9634_LEDOUT_BASE,
+		.n_leds		= PCA9634_NUM_LEDS,
 	},
 	[pca9635] = {
-		.grppwm		= 0x12,
-		.grpfreq	= 0x13,
-		.ledout_base	= 0x14,
-		.n_leds		= 16,
+		.grppwm		= PCA9635_GRPPWM,
+		.grpfreq	= PCA9635_GRPFREQ,
+		.ledout_base	= PCA9635_LEDOUT_BASE,
+		.n_leds		= PCA9635_NUM_LEDS,
 	},
 };
 
@@ -129,7 +209,7 @@ static int pca963x_brightness(struct pca963x_led *pca963x,
 	case LED_FULL:
 		ret = i2c_smbus_write_byte_data(pca963x->chip->client,
 			ledout_addr,
-			(ledout & ~mask) | (PCA963X_LED_ON << shift));
+			(ledout & ~mask) | (PCA963X_LEDOUT_LED_ON << shift));
 		break;
 	case LED_OFF:
 		ret = i2c_smbus_write_byte_data(pca963x->chip->client,
@@ -143,7 +223,7 @@ static int pca963x_brightness(struct pca963x_led *pca963x,
 			goto unlock;
 		ret = i2c_smbus_write_byte_data(pca963x->chip->client,
 			ledout_addr,
-			(ledout & ~mask) | (PCA963X_LED_PWM << shift));
+			(ledout & ~mask) | (PCA963X_LEDOUT_LED_PWM << shift));
 		break;
 	}
 unlock:
@@ -173,9 +253,9 @@ static void pca963x_blink(struct pca963x_led *pca963x)
 			mode2 | PCA963X_MODE2_DMBLNK);
 
 	ledout = i2c_smbus_read_byte_data(pca963x->chip->client, ledout_addr);
-	if ((ledout & mask) != (PCA963X_LED_GRP_PWM << shift))
+	if ((ledout & mask) != (PCA963X_LEDOUT_LED_GRP_PWM << shift))
 		i2c_smbus_write_byte_data(pca963x->chip->client, ledout_addr,
-			(ledout & ~mask) | (PCA963X_LED_GRP_PWM << shift));
+			(ledout & ~mask) | (PCA963X_LEDOUT_LED_GRP_PWM << shift));
 	mutex_unlock(&pca963x->chip->mutex);
 }
 
@@ -358,7 +438,8 @@ static int pca963x_probe(struct i2c_client *client,
 
 	/* Turn off LEDs by default*/
 	for (i = 0; i < chip->n_leds / 4; i++)
-		i2c_smbus_write_byte_data(client, chip->ledout_base + i, 0x00);
+		i2c_smbus_write_byte_data(client, chip->ledout_base + i,
+				PCA963X_LEDOUT_LDR(PCA963X_LEDOUT_LED_OFF, i));
 
 	for (i = 0; i < chip->n_leds; i++) {
 		pca963x[i].led_num = i;
@@ -397,9 +478,12 @@ static int pca963x_probe(struct i2c_client *client,
 	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);
+			i2c_smbus_write_byte_data(client, PCA963X_MODE2,
+					PCA963X_MODE2_OUTNE_OUTDRV);
 		else
-			i2c_smbus_write_byte_data(client, PCA963X_MODE2, 0x05);
+			i2c_smbus_write_byte_data(client, PCA963X_MODE2,
+					PCA963X_MODE2_OUTNE_OUTDRV |
+					PCA963X_MODE2_OUTDRV_TOTEM_POLE);
 	}
 
 	return 0;
-- 
2.8.0.rc3

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2695 bytes --]

Hi,

[auto build test ERROR on j.anaszewski-leds/for-next]
[also build test ERROR on v4.6-rc4 next-20160419]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Olliver-Schinagl/leds-pca9653x-support-inverted-outputs-and-cleanups/20160419-154536
base:   https://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds for-next
config: x86_64-randconfig-x010-201616 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

Note: the linux-review/Olliver-Schinagl/leds-pca9653x-support-inverted-outputs-and-cleanups/20160419-154536 HEAD 08d56036eb1d895519b5f9589b55341bed74f121 builds fine.
      It only hurts bisectibility.

All errors (new ones prefixed by >>):

   drivers/leds/leds-pca963x.c: In function 'pca963x_brightness':
>> drivers/leds/leds-pca963x.c:220:4: error: 'PCA963X_PWM_BASE' undeclared (first use in this function)
       PCA963X_PWM_BASE + pca963x->led_num,
       ^
   drivers/leds/leds-pca963x.c:220:4: note: each undeclared identifier is reported only once for each function it appears in

vim +/PCA963X_PWM_BASE +220 drivers/leds/leds-pca963x.c

75cb2e1d drivers/leds/leds-pca9633.c Peter Meerwald          2012-03-23  214  	case LED_OFF:
5029a2e3 drivers/leds/leds-pca963x.c Andrew Lunn             2015-08-20  215  		ret = i2c_smbus_write_byte_data(pca963x->chip->client,
5029a2e3 drivers/leds/leds-pca963x.c Andrew Lunn             2015-08-20  216  			ledout_addr, ledout & ~mask);
75cb2e1d drivers/leds/leds-pca9633.c Peter Meerwald          2012-03-23  217  		break;
75cb2e1d drivers/leds/leds-pca9633.c Peter Meerwald          2012-03-23  218  	default:
5029a2e3 drivers/leds/leds-pca963x.c Andrew Lunn             2015-08-20  219  		ret = i2c_smbus_write_byte_data(pca963x->chip->client,
56a1740c drivers/leds/leds-pca963x.c Ricardo Ribalda Delgado 2013-08-14 @220  			PCA963X_PWM_BASE + pca963x->led_num,
5029a2e3 drivers/leds/leds-pca963x.c Andrew Lunn             2015-08-20  221  			brightness);
5029a2e3 drivers/leds/leds-pca963x.c Andrew Lunn             2015-08-20  222  		if (ret < 0)
5029a2e3 drivers/leds/leds-pca963x.c Andrew Lunn             2015-08-20  223  			goto unlock;

:::::: The code at line 220 was first introduced by commit
:::::: 56a1740c21e4396164265c3ec80e29990ddcdc36 leds-pca9633: Rename to leds-pca963x

:::::: TO: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
:::::: CC: Bryan Wu <cooloney@gmail.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 23708 bytes --]

[PATCH 4/6] leds: pca963x: Reduce magic values

[Olliver Schinagl]

This patch uses the newly introduced defines to further reduce magic
values and magic shifts. These changes have a slightly bigger impact as
they do introduce binary changes. There should be no logical changes
however.

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
 drivers/leds/leds-pca963x.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/drivers/leds/leds-pca963x.c b/drivers/leds/leds-pca963x.c
index e4e668d..14690f2 100644
--- a/drivers/leds/leds-pca963x.c
+++ b/drivers/leds/leds-pca963x.c
@@ -199,33 +199,32 @@ static int pca963x_brightness(struct pca963x_led *pca963x,
 	u8 ledout_addr = pca963x->chip->chipdef->ledout_base
 		+ (pca963x->led_num / 4);
 	u8 ledout;
-	int shift = 2 * (pca963x->led_num % 4);
-	u8 mask = 0x3 << shift;
 	int ret;
 
 	mutex_lock(&pca963x->chip->mutex);
 	ledout = i2c_smbus_read_byte_data(pca963x->chip->client, ledout_addr);
+	ledout &= ~PCA963X_LEDOUT_LDR(PCA963X_LEDOUT_MASK, pca963x->led_num);
 	switch (brightness) {
 	case LED_FULL:
-		ret = i2c_smbus_write_byte_data(pca963x->chip->client,
-			ledout_addr,
-			(ledout & ~mask) | (PCA963X_LEDOUT_LED_ON << shift));
+		ledout |= PCA963X_LEDOUT_LDR(PCA963X_LEDOUT_LED_ON,
+					     pca963x->led_num);
 		break;
 	case LED_OFF:
-		ret = i2c_smbus_write_byte_data(pca963x->chip->client,
-			ledout_addr, ledout & ~mask);
+		ledout |= PCA963X_LEDOUT_LDR(PCA963X_LEDOUT_LED_OFF,
+					     pca963x->led_num);
 		break;
 	default:
 		ret = i2c_smbus_write_byte_data(pca963x->chip->client,
-			PCA963X_PWM_BASE + pca963x->led_num,
+			PCA963X_PWM_ADDR(pca963x->led_num),
 			brightness);
 		if (ret < 0)
 			goto unlock;
-		ret = i2c_smbus_write_byte_data(pca963x->chip->client,
-			ledout_addr,
-			(ledout & ~mask) | (PCA963X_LEDOUT_LED_PWM << shift));
+		ledout |= PCA963X_LEDOUT_LDR(PCA963X_LEDOUT_LED_PWM,
+					     pca963x->led_num);
 		break;
 	}
+	ret = i2c_smbus_write_byte_data(pca963x->chip->client, ledout_addr,
+					ledout);
 unlock:
 	mutex_unlock(&pca963x->chip->mutex);
 	return ret;
@@ -237,8 +236,6 @@ static void pca963x_blink(struct pca963x_led *pca963x)
 		(pca963x->led_num / 4);
 	u8 ledout;
 	u8 mode2;
-	int shift = 2 * (pca963x->led_num % 4);
-	u8 mask = 0x3 << shift;
 
 	mutex_lock(&pca963x->chip->mutex);
 	i2c_smbus_write_byte_data(pca963x->chip->client,
@@ -253,9 +250,14 @@ static void pca963x_blink(struct pca963x_led *pca963x)
 			mode2 | PCA963X_MODE2_DMBLNK);
 
 	ledout = i2c_smbus_read_byte_data(pca963x->chip->client, ledout_addr);
-	if ((ledout & mask) != (PCA963X_LEDOUT_LED_GRP_PWM << shift))
+	if ((ledout &
+	     PCA963X_LEDOUT_LDR(PCA963X_LEDOUT_MASK, pca963x->led_num)) !=
+	     PCA963X_LEDOUT_LDR(PCA963X_LEDOUT_LED_GRP_PWM, pca963x->led_num)) {
+		ledout |= PCA963X_LEDOUT_LDR(PCA963X_LEDOUT_LED_GRP_PWM,
+					     pca963x->led_num);
 		i2c_smbus_write_byte_data(pca963x->chip->client, ledout_addr,
-			(ledout & ~mask) | (PCA963X_LEDOUT_LED_GRP_PWM << shift));
+					  ledout);
+	}
 	mutex_unlock(&pca963x->chip->mutex);
 }
 
-- 
2.8.0.rc3

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

[Olliver Schinagl]

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

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

[Rob Herring]

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

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

[Olliver Schinagl]

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

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

[Rob Herring]

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

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

[Olliver Schinagl]

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.

[PATCH 6/6] leds: pca963x: Remove whitespace and checkpatch problems

[Olliver Schinagl]

This patch does some whitespace fixing to make the entire driver more
consistent, especially with regards to alignment. Because of this it
also reduces quite some of the checkpatch warnings.

Two slightly 80 char warnings remain however to satisfy the alignment
check.

This patch does not introduce any binary changes.

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
 drivers/leds/leds-pca963x.c | 53 ++++++++++++++++++++++++---------------------
 1 file changed, 28 insertions(+), 25 deletions(-)

diff --git a/drivers/leds/leds-pca963x.c b/drivers/leds/leds-pca963x.c
index 85dd506..5c4bf77 100644
--- a/drivers/leds/leds-pca963x.c
+++ b/drivers/leds/leds-pca963x.c
@@ -128,6 +128,10 @@
 #define PCA963X_PASSKEY1		0xa5
 #define PCA963X_PASSKEY2		0x5a
 
+/* Total blink period in milliseconds */
+#define PCA963X_BLINK_PERIOD_MIN	42
+#define PCA963X_BLINK_PERIOD_MAX	10667
+
 enum pca963x_type {
 	pca9633,
 	pca9634,
@@ -162,16 +166,12 @@ static struct pca963x_chipdef pca963x_chipdefs[] = {
 	},
 };
 
-/* Total blink period in milliseconds */
-#define PCA963X_BLINK_PERIOD_MIN	42
-#define PCA963X_BLINK_PERIOD_MAX	10667
-
 static const struct i2c_device_id pca963x_id[] = {
 	{ "pca9632", pca9633 },
 	{ "pca9633", pca9633 },
 	{ "pca9634", pca9634 },
 	{ "pca9635", pca9635 },
-	{ }
+	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(i2c, pca963x_id);
 
@@ -194,10 +194,10 @@ struct pca963x_led {
 };
 
 static int pca963x_brightness(struct pca963x_led *pca963x,
-			       enum led_brightness brightness)
+			      enum led_brightness brightness)
 {
-	u8 ledout_addr = pca963x->chip->chipdef->ledout_base
-		+ (pca963x->led_num / 4);
+	u8 ledout_addr = pca963x->chip->chipdef->ledout_base +
+			 (pca963x->led_num / 4);
 	u8 ledout;
 	int ret;
 
@@ -215,8 +215,8 @@ static int pca963x_brightness(struct pca963x_led *pca963x,
 		break;
 	default:
 		ret = i2c_smbus_write_byte_data(pca963x->chip->client,
-			PCA963X_PWM_ADDR(pca963x->led_num),
-			brightness);
+						PCA963X_PWM_ADDR(pca963x->led_num),
+						brightness);
 		if (ret < 0)
 			goto unlock;
 		ledout |= PCA963X_LEDOUT_LDR(PCA963X_LEDOUT_LED_PWM,
@@ -233,21 +233,23 @@ unlock:
 static void pca963x_blink(struct pca963x_led *pca963x)
 {
 	u8 ledout_addr = pca963x->chip->chipdef->ledout_base +
-		(pca963x->led_num / 4);
+			 (pca963x->led_num / 4);
 	u8 ledout;
 	u8 mode2;
 
 	mutex_lock(&pca963x->chip->mutex);
 	i2c_smbus_write_byte_data(pca963x->chip->client,
-			pca963x->chip->chipdef->grppwm,	pca963x->gdc);
+				  pca963x->chip->chipdef->grppwm,
+				  pca963x->gdc);
 
 	i2c_smbus_write_byte_data(pca963x->chip->client,
-			pca963x->chip->chipdef->grpfreq, pca963x->gfrq);
+				  pca963x->chip->chipdef->grpfreq,
+				  pca963x->gfrq);
 
 	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);
+					  mode2 | PCA963X_MODE2_DMBLNK);
 
 	ledout = i2c_smbus_read_byte_data(pca963x->chip->client, ledout_addr);
 	if ((ledout &
@@ -262,7 +264,7 @@ static void pca963x_blink(struct pca963x_led *pca963x)
 }
 
 static int pca963x_led_set(struct led_classdev *led_cdev,
-	enum led_brightness value)
+			   enum led_brightness value)
 {
 	struct pca963x_led *pca963x;
 
@@ -272,7 +274,7 @@ static int pca963x_led_set(struct led_classdev *led_cdev,
 }
 
 static int pca963x_blink_set(struct led_classdev *led_cdev,
-		unsigned long *delay_on, unsigned long *delay_off)
+			     unsigned long *delay_on, unsigned long *delay_off)
 {
 	struct pca963x_led *pca963x;
 	unsigned long time_on, time_off, period;
@@ -291,7 +293,7 @@ static int pca963x_blink_set(struct led_classdev *led_cdev,
 
 	period = time_on + time_off;
 
-	/* If period not supported by hardware, default to someting sane. */
+	/* If period not supported by hardware, default to something sane. */
 	if ((period < PCA963X_BLINK_PERIOD_MIN) ||
 	    (period > PCA963X_BLINK_PERIOD_MAX)) {
 		time_on = 500;
@@ -338,7 +340,8 @@ pca963x_dt_init(struct i2c_client *client, struct pca963x_chipdef *chip)
 		return ERR_PTR(-ENODEV);
 
 	pca963x_leds = devm_kzalloc(&client->dev,
-			sizeof(struct led_info) * chip->n_leds, GFP_KERNEL);
+				    sizeof(struct led_info) * chip->n_leds,
+				    GFP_KERNEL);
 	if (!pca963x_leds)
 		return ERR_PTR(-ENOMEM);
 
@@ -399,7 +402,7 @@ pca963x_dt_init(struct i2c_client *client, struct pca963x_chipdef *chip)
 #endif
 
 static int pca963x_probe(struct i2c_client *client,
-					const struct i2c_device_id *id)
+			 const struct i2c_device_id *id)
 {
 	struct pca963x *pca963x_chip;
 	struct pca963x_led *pca963x;
@@ -419,18 +422,18 @@ static int pca963x_probe(struct i2c_client *client,
 	}
 
 	if (pdata && (pdata->leds.num_leds < 1 ||
-				 pdata->leds.num_leds > chip->n_leds)) {
+		      pdata->leds.num_leds > chip->n_leds)) {
 		dev_err(&client->dev, "board info must claim 1-%d LEDs",
-								chip->n_leds);
+			chip->n_leds);
 		return -EINVAL;
 	}
 
 	pca963x_chip = devm_kzalloc(&client->dev, sizeof(*pca963x_chip),
-								GFP_KERNEL);
+				    GFP_KERNEL);
 	if (!pca963x_chip)
 		return -ENOMEM;
 	pca963x = devm_kzalloc(&client->dev, chip->n_leds * sizeof(*pca963x),
-								GFP_KERNEL);
+			       GFP_KERNEL);
 	if (!pca963x)
 		return -ENOMEM;
 
@@ -444,7 +447,7 @@ static int pca963x_probe(struct i2c_client *client,
 	/* Turn off LEDs by default*/
 	for (i = 0; i < chip->n_leds / 4; i++)
 		i2c_smbus_write_byte_data(client, chip->ledout_base + i,
-				PCA963X_LEDOUT_LDR(PCA963X_LEDOUT_LED_OFF, i));
+					  PCA963X_LEDOUT_LDR(PCA963X_LEDOUT_LED_OFF, i));
 
 	for (i = 0; i < chip->n_leds; i++) {
 		pca963x[i].led_num = i;
@@ -461,7 +464,7 @@ static int pca963x_probe(struct i2c_client *client,
 					pdata->leds.leds[i].default_trigger;
 		}
 		if (!pdata || i >= pdata->leds.num_leds ||
-						!pdata->leds.leds[i].name)
+		    !pdata->leds.leds[i].name)
 			snprintf(pca963x[i].name, sizeof(pca963x[i].name),
 				 "pca963x:%d:%.2x:%d", client->adapter->nr,
 				 client->addr, i);
-- 
2.8.0.rc3

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

[Jacek Anaszewski]

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

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

[Olliver Schinagl]

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(-)
>>
>
>

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

[Ricardo Ribalda Delgado]

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

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

[Olliver Schinagl]

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(-)
>>>>
>>>
>
>

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

[Ricardo Ribalda Delgado]

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

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

[Olliver Schinagl]

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

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

[Ricardo Ribalda Delgado]

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

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

[Olliver Schinagl]

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

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

[Ricardo Ribalda Delgado]

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

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

[Olliver Schinagl]

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
>

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

[Ricardo Ribalda Delgado]

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

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

[Olliver Schinagl]

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!
>
>
>

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

[Olliver Schinagl]

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!
>
>
>

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

[Olliver Schinagl]

Hey Ricardo,

On 11-05-16 23:38, Ricardo Ribalda Delgado wrote:
> Some update
>
> I have not received anything to test it and I will be out of the
> office from today until the 30th :S. So it seems that I have not way
> to test the changes.
That's understandable, meanwhile I've been running some experiments and 
mostly reading the datasheet more closely. You hadn't replied to my 
earlier mention of it so I'll only talk about that now.

Right now, the GDC (e.g. GRPPWM) register is set via some math using the 
period:
     /*
      * From manual: duty cycle = (GDC / 256) ->
      *    (time_on / period) = (GDC / 256) ->
      *        GDC = ((time_on * 256) / period)
      */
     gdc = (time_on * 256) / period;

This is wrong in my opinion, as the manual states:

When DMBLNK bit is programmed with 1, GRPPWM and GRPFREQ registers define a
global blinking pattern, where GRPFREQ contains the blinking period 
(from 24 Hz to
10.73 s) and GRPPWM the duty cycle (ON/OFF ratio in %).

First, when DMBLNK is programmed with 0, GRPFREQ is a don't care, so no 
'blinking' takes place. Which means, since we want blink_set to work, we 
need DMBLNK and that can be enabled by default, as the LDR register 
controls whether we can blink at all (11b vs 10b)

Then the GRPFREQ register, as done now, sets a frequency for the blink 
between 41ms and 10.73 seconds.

Finally, the GRPPWM/gdc register can be used to globally control the 
brightness of leds which are linked (via LDR=11b). So if we have a color 
setup, e.g. #4488FF and we globally dim this color via gdc to 10%, we 
get #04080F on the output. To add this behavior we'd be adding some more 
hacks as the led framework doesn't really support linked outputs at all, 
(nice new feature, to link leds together with a global control?, other 
subsystems do something like that, cpufreq i guess) who decides the 
value of gdc. There is no toggle to set the global brightness. What 
could be possible, is to leave whatever setting is in the brightness and 
read/report the individual led brightness as it where the gdc. Which 
could actually work thinking about it. But (and I think it is impossible 
now anyway) it would break individual color control during the blink. 
e.g. /sys/class/led/[red,green,blue]/brightness are always equal and 
writing to either during blink sets the global brightness.

But that should be a separate patch that I'm thinking a bit more about.

So for now, I recommend to set GRPPWM initially to 0xff, e.g. leds full 
on, until the above patch is added. We then blink at whatever color is 
set to the output and only control the blink via the GRPFREQ, without 
the possibility to change brightness during a blink.

Olliver
> Sorry about that.
>
> Regards!
>
> On Fri, Apr 22, 2016 at 10:48 AM, Ricardo Ribalda Delgado
> <ricardo.ribalda@gmail.com> wrote:
>> Hi
>>
>> I  am on trip until next monday. I have arranged also some hw to be sent to
>> me that day.
>>
>> Can we continue the conversation then? I know I told you that I will review
>> this yesterday, but I did not have the time , sorry
>>
>> Regards!
>>
>> On 22 Apr 2016 09:21, "Olliver Schinagl" <oliver@schinagl.nl> wrote:
>>
>> 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!
>>>
>>>
>>>
>

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

[Olliver Schinagl]

Hey Ricardo,

On 11-05-16 23:38, Ricardo Ribalda Delgado wrote:
> Some update
>
> I have not received anything to test it and I will be out of the
> office from today until the 30th :S. So it seems that I have not way
> to test the changes.
That's understandable, meanwhile I've been running some experiments and 
mostly reading the datasheet more closely. You hadn't replied to my 
earlier mention of it so I'll only talk about that now.

Right now, the GDC (e.g. GRPPWM) register is set via some math using the 
period:
     /*
      * From manual: duty cycle = (GDC / 256) ->
      *    (time_on / period) = (GDC / 256) ->
      *        GDC = ((time_on * 256) / period)
      */
     gdc = (time_on * 256) / period;

This is wrong in my opinion, as the manual states:

When DMBLNK bit is programmed with 1, GRPPWM and GRPFREQ registers define a
global blinking pattern, where GRPFREQ contains the blinking period 
(from 24 Hz to
10.73 s) and GRPPWM the duty cycle (ON/OFF ratio in %).

First, when DMBLNK is programmed with 0, GRPFREQ is a don't care, so no 
'blinking' takes place. Which means, since we want blink_set to work, we 
need DMBLNK and that can be enabled by default, as the LDR register 
controls whether we can blink at all (11b vs 10b)

Then the GRPFREQ register, as done now, sets a frequency for the blink 
between 41ms and 10.73 seconds.

Finally, the GRPPWM/gdc register can be used to globally control the 
brightness of leds which are linked (via LDR=11b). So if we have a color 
setup, e.g. #4488FF and we globally dim this color via gdc to 10%, we 
get #04080F on the output. To add this behavior we'd be adding some more 
hacks as the led framework doesn't really support linked outputs at all, 
(nice new feature, to link leds together with a global control?, other 
subsystems do something like that, cpufreq i guess) who decides the 
value of gdc. There is no toggle to set the global brightness. What 
could be possible, is to leave whatever setting is in the brightness and 
read/report the individual led brightness as it where the gdc. Which 
could actually work thinking about it. But (and I think it is impossible 
now anyway) it would break individual color control during the blink. 
e.g. /sys/class/led/[red,green,blue]/brightness are always equal and 
writing to either during blink sets the global brightness.

But that should be a separate patch that I'm thinking a bit more about.

So for now, I recommend to set GRPPWM initially to 0xff, e.g. leds full 
on, until the above patch is added. We then blink at whatever color is 
set to the output and only control the blink via the GRPFREQ, without 
the possibility to change brightness during a blink.

Olliver
> Sorry about that.
>
> Regards!
>
> On Fri, Apr 22, 2016 at 10:48 AM, Ricardo Ribalda Delgado
> <ricardo.ribalda@gmail.com>  wrote:
>> Hi
>>
>> I  am on trip until next monday. I have arranged also some hw to be sent to
>> me that day.
>>
>> Can we continue the conversation then? I know I told you that I will review
>> this yesterday, but I did not have the time , sorry
>>
>> Regards!
>>
>> On 22 Apr 2016 09:21, "Olliver Schinagl"<oliver@schinagl.nl>  wrote:
>>
>> 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!
>>>
>>>
>>>