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
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 <[email protected]>
---
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-order headers so they are in alphabetical order.
Signed-off-by: Olliver Schinagl <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
:::::: CC: Bryan Wu <[email protected]>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
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
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(-)
>>
>
>
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 <[email protected]> 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
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 <[email protected]> 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(-)
>>>>
>>>
>
>
Hi again
On Tue, Apr 19, 2016 at 3:27 PM, Olliver Schinagl <[email protected]> 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
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 <[email protected]> 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
Hi Ollivier
On Wed, Apr 20, 2016 at 9:21 AM, Olliver Schinagl <[email protected]> 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
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 <[email protected]> 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
Hi
On Wed, Apr 20, 2016 at 10:51 AM, Olliver Schinagl <[email protected]> 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
On 20-04-16 10:56, Ricardo Ribalda Delgado wrote:
> Hi
>
> On Wed, Apr 20, 2016 at 10:51 AM, Olliver Schinagl <[email protected]> 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
>
Hello again
On Wed, Apr 20, 2016 at 11:06 AM, Olliver Schinagl <[email protected]> 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
Hey,
On 20-04-16 11:17, Ricardo Ribalda Delgado wrote:
> Hello again
>
> On Wed, Apr 20, 2016 at 11:06 AM, Olliver Schinagl <[email protected]> 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!
>
>
>
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 <[email protected]>
> ---
> 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
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 <[email protected]> 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!
>
>
>
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 <[email protected]>
>> ---
>> 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
On Fri, Apr 22, 2016 at 7:38 AM, Olliver Schinagl <[email protected]> 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 <[email protected]>
>>> ---
>>> 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
On April 22, 2016 3:09:35 PM CEST, Rob Herring <[email protected]> wrote:
>On Fri, Apr 22, 2016 at 7:38 AM, Olliver Schinagl <[email protected]>
>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 <[email protected]>
>>>> ---
>>>> 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.