[PATCH v3 0/4] leds: pca955x: add GPIO support

[PATCH v3 0/4] leds: pca955x: add GPIO support

[Cédric Le Goater]

Hello !

The PCA955x family of chips are I2C LED blinkers whose pins not used
to control LEDs can be used as general purpose I/Os (GPIOs).

The following adds support for device tree and Open Firmware to be
able do define different operation modes for each pin. See bindings
documentation for more details. The pca955x driver is then extended
with a gpio_chip when pins are operating in GPIO mode.

The driver follows the scheme of the leds-pca9532 driver which behaves
quite similarly.
 
Thanks,

C.

Changes since v2:

 - removed 'if (pdata)' tests

Changes since v1:

 - split the patchset in two : DT support and GPIO support
 - introduced the use of devm_led_classdev_register()
 - replaced the 'compatible' property with 'type' 
 - removed the 'gpio-base' property

Cédric Le Goater (4):
  leds: pca955x: add device tree support
  leds: pca955x: use devm_led_classdev_register
  leds: pca955x: add GPIO support
  dt-bindings leds: add pca955x

 .../devicetree/bindings/leds/leds-pca955x.txt      |  88 +++++++
 drivers/leds/Kconfig                               |  11 +
 drivers/leds/leds-pca955x.c                        | 254 +++++++++++++++++----
 include/dt-bindings/leds/leds-pca955x.h            |  16 ++
 4 files changed, 323 insertions(+), 46 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-pca955x.txt
 create mode 100644 include/dt-bindings/leds/leds-pca955x.h

-- 
2.7.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 1/4] leds: pca955x: add device tree support

[Cédric Le Goater]

It will be used in a following patch to define different operation
modes for each pin.

Signed-off-by: Cédric Le Goater <clg-Bxea+6Xhats@public.gmane.org>
---
Changes since v2:

 - removed 'if (pdata)' test

 drivers/leds/leds-pca955x.c | 101 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 89 insertions(+), 12 deletions(-)

diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
index 9a873118ea5f..2d34009d00e6 100644
--- a/drivers/leds/leds-pca955x.c
+++ b/drivers/leds/leds-pca955x.c
@@ -41,14 +41,17 @@
  */
 
 #include <linux/acpi.h>
-#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/gpio.h>
 #include <linux/i2c.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/of.h>
 #include <linux/slab.h>
+#include <linux/string.h>
 
 /* LED select registers determine the source that drives LED outputs */
 #define PCA955X_LS_LED_ON	0x0	/* Output LOW */
@@ -122,6 +125,12 @@ struct pca955x_led {
 	struct led_classdev	led_cdev;
 	int			led_num;	/* 0 .. 15 potentially */
 	char			name[32];
+	const char		*default_trigger;
+};
+
+struct pca955x_platform_data {
+	struct pca955x_led	*leds;
+	int			num_leds;
 };
 
 /* 8 bits per input register */
@@ -250,6 +259,70 @@ static int pca955x_led_set(struct led_classdev *led_cdev,
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_OF)
+static struct pca955x_platform_data *
+pca955x_pdata_of_init(struct i2c_client *client, struct pca955x_chipdef *chip)
+{
+	struct device_node *np = client->dev.of_node;
+	struct device_node *child;
+	struct pca955x_platform_data *pdata;
+	int count;
+
+	count = of_get_child_count(np);
+	if (!count || count > chip->bits)
+		return ERR_PTR(-ENODEV);
+
+	pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return ERR_PTR(-ENOMEM);
+
+	pdata->leds = devm_kzalloc(&client->dev,
+				   sizeof(struct pca955x_led) * chip->bits,
+				   GFP_KERNEL);
+	if (!pdata->leds)
+		return ERR_PTR(-ENOMEM);
+
+	for_each_child_of_node(np, child) {
+		const char *name;
+		u32 reg;
+		int res;
+
+		res = of_property_read_u32(child, "reg", &reg);
+		if ((res != 0) || (reg >= chip->bits))
+			continue;
+
+		if (of_property_read_string(child, "label", &name))
+			name = child->name;
+
+		snprintf(pdata->leds[reg].name, sizeof(pdata->leds[reg].name),
+			 "%s", name);
+
+		of_property_read_string(child, "linux,default-trigger",
+					&pdata->leds[reg].default_trigger);
+	}
+
+	pdata->num_leds = chip->bits;
+
+	return pdata;
+}
+
+static const struct of_device_id of_pca955x_match[] = {
+	{ .compatible = "nxp,pca9550", .data = (void *)pca9550 },
+	{ .compatible = "nxp,pca9551", .data = (void *)pca9551 },
+	{ .compatible = "nxp,pca9552", .data = (void *)pca9552 },
+	{ .compatible = "nxp,pca9553", .data = (void *)pca9553 },
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, of_pca955x_match);
+#else
+static struct pca955x_platform_data *
+pca955x_pdata_of_init(struct i2c_client *client, struct pca955x_chipdef *chip)
+{
+	return ERR_PTR(-ENODEV);
+}
+#endif
+
 static int pca955x_probe(struct i2c_client *client,
 					const struct i2c_device_id *id)
 {
@@ -257,8 +330,8 @@ static int pca955x_probe(struct i2c_client *client,
 	struct pca955x_led *pca955x_led;
 	struct pca955x_chipdef *chip;
 	struct i2c_adapter *adapter;
-	struct led_platform_data *pdata;
 	int i, err;
+	struct pca955x_platform_data *pdata;
 
 	if (id) {
 		chip = &pca955x_chipdefs[id->driver_data];
@@ -272,6 +345,11 @@ static int pca955x_probe(struct i2c_client *client,
 	}
 	adapter = to_i2c_adapter(client->dev.parent);
 	pdata = dev_get_platdata(&client->dev);
+	if (!pdata) {
+		pdata =	pca955x_pdata_of_init(client, chip);
+		if (IS_ERR(pdata))
+			return PTR_ERR(pdata);
+	}
 
 	/* Make sure the slave address / chip type combo given is possible */
 	if ((client->addr & ~((1 << chip->slv_addr_shift) - 1)) !=
@@ -288,13 +366,11 @@ static int pca955x_probe(struct i2c_client *client,
 	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
 		return -EIO;
 
-	if (pdata) {
-		if (pdata->num_leds != chip->bits) {
-			dev_err(&client->dev, "board info claims %d LEDs"
-					" on a %d-bit chip\n",
-					pdata->num_leds, chip->bits);
-			return -ENODEV;
-		}
+	if (pdata->num_leds != chip->bits) {
+		dev_err(&client->dev,
+			"board info claims %d LEDs on a %d-bit chip\n",
+			pdata->num_leds, chip->bits);
+		return -ENODEV;
 	}
 
 	pca955x = devm_kzalloc(&client->dev, sizeof(*pca955x), GFP_KERNEL);
@@ -378,6 +454,7 @@ static struct i2c_driver pca955x_driver = {
 	.driver = {
 		.name	= "leds-pca955x",
 		.acpi_match_table = ACPI_PTR(pca955x_acpi_ids),
+		.of_match_table = of_match_ptr(of_pca955x_match),
 	},
 	.probe	= pca955x_probe,
 	.remove	= pca955x_remove,
-- 
2.7.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/4] leds: pca955x: use devm_led_classdev_register

[Cédric Le Goater]

This lets us remove the loop doing the cleanup in case of failure and
also the remove handler of the i2c_driver.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 drivers/leds/leds-pca955x.c | 26 ++++----------------------
 1 file changed, 4 insertions(+), 22 deletions(-)

diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
index 2d34009d00e6..5a5d3765cfd3 100644
--- a/drivers/leds/leds-pca955x.c
+++ b/drivers/leds/leds-pca955x.c
@@ -410,10 +410,10 @@ static int pca955x_probe(struct i2c_client *client,
 		pca955x_led->led_cdev.name = pca955x_led->name;
 		pca955x_led->led_cdev.brightness_set_blocking = pca955x_led_set;
 
-		err = led_classdev_register(&client->dev,
-					&pca955x_led->led_cdev);
-		if (err < 0)
-			goto exit;
+		err = devm_led_classdev_register(&client->dev,
+						 &pca955x_led->led_cdev);
+		if (err)
+			return err;
 	}
 
 	/* Turn off LEDs */
@@ -431,23 +431,6 @@ static int pca955x_probe(struct i2c_client *client,
 	pca955x_write_psc(client, 1, 0);
 
 	return 0;
-
-exit:
-	while (i--)
-		led_classdev_unregister(&pca955x->leds[i].led_cdev);
-
-	return err;
-}
-
-static int pca955x_remove(struct i2c_client *client)
-{
-	struct pca955x *pca955x = i2c_get_clientdata(client);
-	int i;
-
-	for (i = 0; i < pca955x->chipdef->bits; i++)
-		led_classdev_unregister(&pca955x->leds[i].led_cdev);
-
-	return 0;
 }
 
 static struct i2c_driver pca955x_driver = {
@@ -457,7 +440,6 @@ static struct i2c_driver pca955x_driver = {
 		.of_match_table = of_match_ptr(of_pca955x_match),
 	},
 	.probe	= pca955x_probe,
-	.remove	= pca955x_remove,
 	.id_table = pca955x_id,
 };
 
-- 
2.7.5

[PATCH v3 3/4] leds: pca955x: add GPIO support

[Cédric Le Goater]

The PCA955x family of chips are I2C LED blinkers whose pins not used
to control LEDs can be used as general purpose I/Os (GPIOs).

The following adds such a support by defining different operation
modes for the pins (See bindings documentation for more details). The
pca955x driver is then extended with a gpio_chip when some of pins are
operating as GPIOs. The default operating mode is to behave as a LED.

The GPIO support is conditioned by CONFIG_LEDS_PCA955X_GPIO.

Signed-off-by: Cédric Le Goater <clg-Bxea+6Xhats@public.gmane.org>
---
Changes since v2:

 - removed 'if (pdata)' test

 drivers/leds/Kconfig                    |  11 +++
 drivers/leds/leds-pca955x.c             | 137 ++++++++++++++++++++++++++++----
 include/dt-bindings/leds/leds-pca955x.h |  16 ++++
 3 files changed, 147 insertions(+), 17 deletions(-)
 create mode 100644 include/dt-bindings/leds/leds-pca955x.h

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 594b24d410c3..3013cd35c65e 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -377,6 +377,17 @@ config LEDS_PCA955X
 	  LED driver chips accessed via the I2C bus.  Supported
 	  devices include PCA9550, PCA9551, PCA9552, and PCA9553.
 
+config LEDS_PCA955X_GPIO
+	bool "Enable GPIO support for PCA955X"
+	depends on LEDS_PCA955X
+	depends on GPIOLIB
+	help
+	  Allow unused pins on PCA955X to be used as gpio.
+
+	  To use a pin as gpio the pin type should be set to
+	  PCA955X_TYPE_GPIO in the device tree.
+
+
 config LEDS_PCA963X
 	tristate "LED support for PCA963x I2C chip"
 	depends on LEDS_CLASS
diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
index 5a5d3765cfd3..f062d1e7640f 100644
--- a/drivers/leds/leds-pca955x.c
+++ b/drivers/leds/leds-pca955x.c
@@ -53,6 +53,8 @@
 #include <linux/slab.h>
 #include <linux/string.h>
 
+#include <dt-bindings/leds/leds-pca955x.h>
+
 /* LED select registers determine the source that drives LED outputs */
 #define PCA955X_LS_LED_ON	0x0	/* Output LOW */
 #define PCA955X_LS_LED_OFF	0x1	/* Output HI-Z */
@@ -118,6 +120,9 @@ struct pca955x {
 	struct pca955x_led *leds;
 	struct pca955x_chipdef	*chipdef;
 	struct i2c_client	*client;
+#ifdef CONFIG_LEDS_PCA955X_GPIO
+	struct gpio_chip gpio;
+#endif
 };
 
 struct pca955x_led {
@@ -125,6 +130,7 @@ struct pca955x_led {
 	struct led_classdev	led_cdev;
 	int			led_num;	/* 0 .. 15 potentially */
 	char			name[32];
+	u32			type;
 	const char		*default_trigger;
 };
 
@@ -259,6 +265,65 @@ static int pca955x_led_set(struct led_classdev *led_cdev,
 	return 0;
 }
 
+#ifdef CONFIG_LEDS_PCA955X_GPIO
+/*
+ * Read the INPUT register, which contains the state of LEDs.
+ */
+static u8 pca955x_read_input(struct i2c_client *client, int n)
+{
+	return (u8)i2c_smbus_read_byte_data(client, n);
+}
+
+static int pca955x_gpio_request_pin(struct gpio_chip *gc, unsigned int offset)
+{
+	struct pca955x *pca955x = gpiochip_get_data(gc);
+	struct pca955x_led *led = &pca955x->leds[offset];
+
+	if (led->type == PCA955X_TYPE_GPIO)
+		return 0;
+
+	return -EBUSY;
+}
+
+static void pca955x_gpio_set_value(struct gpio_chip *gc, unsigned int offset,
+				   int val)
+{
+	struct pca955x *pca955x = gpiochip_get_data(gc);
+	struct pca955x_led *led = &pca955x->leds[offset];
+
+	if (val)
+		pca955x_led_set(&led->led_cdev, LED_FULL);
+	else
+		pca955x_led_set(&led->led_cdev, LED_OFF);
+}
+
+static int pca955x_gpio_get_value(struct gpio_chip *gc, unsigned int offset)
+{
+	struct pca955x *pca955x = gpiochip_get_data(gc);
+	struct pca955x_led *led = &pca955x->leds[offset];
+	u8 reg = pca955x_read_input(pca955x->client, led->led_num / 8);
+
+	return !!(reg & (1 << (led->led_num % 8)));
+}
+
+static int pca955x_gpio_direction_input(struct gpio_chip *gc,
+					unsigned int offset)
+{
+	/* To use as input ensure pin is not driven */
+	pca955x_gpio_set_value(gc, offset, 0);
+
+	return 0;
+}
+
+static int pca955x_gpio_direction_output(struct gpio_chip *gc,
+					 unsigned int offset, int val)
+{
+	pca955x_gpio_set_value(gc, offset, val);
+
+	return 0;
+}
+#endif /* CONFIG_LEDS_PCA955X_GPIO */
+
 #if IS_ENABLED(CONFIG_OF)
 static struct pca955x_platform_data *
 pca955x_pdata_of_init(struct i2c_client *client, struct pca955x_chipdef *chip)
@@ -297,6 +362,8 @@ pca955x_pdata_of_init(struct i2c_client *client, struct pca955x_chipdef *chip)
 		snprintf(pdata->leds[reg].name, sizeof(pdata->leds[reg].name),
 			 "%s", name);
 
+		pdata->leds[reg].type = PCA955X_TYPE_LED;
+		of_property_read_u32(child, "type", &pdata->leds[reg].type);
 		of_property_read_string(child, "linux,default-trigger",
 					&pdata->leds[reg].default_trigger);
 	}
@@ -332,6 +399,7 @@ static int pca955x_probe(struct i2c_client *client,
 	struct i2c_adapter *adapter;
 	int i, err;
 	struct pca955x_platform_data *pdata;
+	int ngpios = 0;
 
 	if (id) {
 		chip = &pca955x_chipdefs[id->driver_data];
@@ -392,9 +460,19 @@ static int pca955x_probe(struct i2c_client *client,
 		pca955x_led = &pca955x->leds[i];
 		pca955x_led->led_num = i;
 		pca955x_led->pca955x = pca955x;
-
-		/* Platform data can specify LED names and default triggers */
-		if (pdata) {
+		pca955x_led->type = pdata->leds[i].type;
+
+		switch (pca955x_led->type) {
+		case PCA955X_TYPE_NONE:
+			break;
+		case PCA955X_TYPE_GPIO:
+			ngpios++;
+			break;
+		case PCA955X_TYPE_LED:
+			/*
+			 * Platform data can specify LED names and
+			 * default triggers
+			 */
 			if (pdata->leds[i].name)
 				snprintf(pca955x_led->name,
 					sizeof(pca955x_led->name), "pca955x:%s",
@@ -402,23 +480,20 @@ static int pca955x_probe(struct i2c_client *client,
 			if (pdata->leds[i].default_trigger)
 				pca955x_led->led_cdev.default_trigger =
 					pdata->leds[i].default_trigger;
-		} else {
-			snprintf(pca955x_led->name, sizeof(pca955x_led->name),
-				 "pca955x:%d", i);
-		}
 
-		pca955x_led->led_cdev.name = pca955x_led->name;
-		pca955x_led->led_cdev.brightness_set_blocking = pca955x_led_set;
+			pca955x_led->led_cdev.name = pca955x_led->name;
+			pca955x_led->led_cdev.brightness_set_blocking =
+				pca955x_led_set;
 
-		err = devm_led_classdev_register(&client->dev,
-						 &pca955x_led->led_cdev);
-		if (err)
-			return err;
-	}
+			err = devm_led_classdev_register(&client->dev,
+							&pca955x_led->led_cdev);
+			if (err)
+				return err;
 
-	/* Turn off LEDs */
-	for (i = 0; i < pca95xx_num_led_regs(chip->bits); i++)
-		pca955x_write_ls(client, i, 0x55);
+			/* Turn off LED */
+			pca955x_led_set(&pca955x_led->led_cdev, LED_OFF);
+		}
+	}
 
 	/* PWM0 is used for half brightness or 50% duty cycle */
 	pca955x_write_pwm(client, 0, 255-LED_HALF);
@@ -430,6 +505,34 @@ static int pca955x_probe(struct i2c_client *client,
 	pca955x_write_psc(client, 0, 0);
 	pca955x_write_psc(client, 1, 0);
 
+#ifdef CONFIG_LEDS_PCA955X_GPIO
+	if (ngpios) {
+		pca955x->gpio.label = "gpio-pca955x";
+		pca955x->gpio.direction_input = pca955x_gpio_direction_input;
+		pca955x->gpio.direction_output = pca955x_gpio_direction_output;
+		pca955x->gpio.set = pca955x_gpio_set_value;
+		pca955x->gpio.get = pca955x_gpio_get_value;
+		pca955x->gpio.request = pca955x_gpio_request_pin;
+		pca955x->gpio.can_sleep = 1;
+		pca955x->gpio.base = -1;
+		pca955x->gpio.ngpio = ngpios;
+		pca955x->gpio.parent = &client->dev;
+		pca955x->gpio.owner = THIS_MODULE;
+
+		err = devm_gpiochip_add_data(&client->dev, &pca955x->gpio,
+					     pca955x);
+		if (err) {
+			/* Use data->gpio.dev as a flag for freeing gpiochip */
+			pca955x->gpio.parent = NULL;
+			dev_warn(&client->dev, "could not add gpiochip\n");
+			return err;
+		}
+		dev_info(&client->dev, "gpios %i...%i\n",
+			 pca955x->gpio.base, pca955x->gpio.base +
+			 pca955x->gpio.ngpio - 1);
+	}
+#endif
+
 	return 0;
 }
 
diff --git a/include/dt-bindings/leds/leds-pca955x.h b/include/dt-bindings/leds/leds-pca955x.h
new file mode 100644
index 000000000000..78cb7e979de7
--- /dev/null
+++ b/include/dt-bindings/leds/leds-pca955x.h
@@ -0,0 +1,16 @@
+/*
+ * This header provides constants for pca955x LED bindings.
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#ifndef _DT_BINDINGS_LEDS_PCA955X_H
+#define _DT_BINDINGS_LEDS_PCA955X_H
+
+#define PCA955X_TYPE_NONE         0
+#define PCA955X_TYPE_LED          1
+#define PCA955X_TYPE_GPIO         2
+
+#endif /* _DT_BINDINGS_LEDS_PCA955X_H */
-- 
2.7.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 4/4] dt-bindings leds: add pca955x

[Cédric Le Goater]

This adds the devicetree bindings for the PCA955x I2C LED blinkers.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 .../devicetree/bindings/leds/leds-pca955x.txt      | 88 ++++++++++++++++++++++
 1 file changed, 88 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-pca955x.txt

diff --git a/Documentation/devicetree/bindings/leds/leds-pca955x.txt b/Documentation/devicetree/bindings/leds/leds-pca955x.txt
new file mode 100644
index 000000000000..7984efb767b4
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-pca955x.txt
@@ -0,0 +1,88 @@
+* NXP - pca955x LED driver
+
+The PCA955x family of chips are I2C LED blinkers whose pins not used
+to control LEDs can be used as general purpose I/Os. The GPIO pins can
+be input or output, and output pins can also be pulse-width controlled.
+
+Required properties:
+- compatible : should be one of :
+	"nxp,pca9550"
+	"nxp,pca9551"
+	"nxp,pca9552"
+	"nxp,pca9553"
+- #address-cells: must be 1
+- #size-cells: must be 0
+- reg: I2C slave address. depends on the model.
+
+Optional properties:
+- gpio-controller: allows pins to be used as GPIOs.
+- #gpio-cells: must be 2.
+- gpio-line-names: define the names of the GPIO lines
+
+LED sub-node properties:
+- reg : number of LED line.
+		from 0 to  1 for the pca9550
+		from 0 to  7 for the pca9551
+		from 0 to 15 for the pca9552
+		from 0 to  3 for the pca9553
+- type: (optional) either
+	PCA9532_TYPE_NONE
+	PCA9532_TYPE_LED
+	PCA9532_TYPE_GPIO
+	see dt-bindings/leds/leds-pca955x.h (default to LED)
+- label : (optional)
+	see Documentation/devicetree/bindings/leds/common.txt
+- linux,default-trigger : (optional)
+	see Documentation/devicetree/bindings/leds/common.txt
+
+Examples:
+
+pca9552: pca9552@60 {
+	compatible = "nxp,pca9552";
+	#address-cells = <1>;
+        #size-cells = <0>;
+	reg = <0x60>;
+
+	gpio-controller;
+	#gpio-cells = <2>;
+	gpio-line-names = "GPIO12", "GPIO13", "GPIO14", "GPIO15";
+
+	gpio@12 {
+		reg = <12>;
+		type = <PCA955X_TYPE_GPIO>;
+	};
+	gpio@13 {
+		reg = <13>;
+		type = <PCA955X_TYPE_GPIO>;
+	};
+	gpio@14 {
+		reg = <14>;
+		type = <PCA955X_TYPE_GPIO>;
+	};
+	gpio@15 {
+		reg = <15>;
+		type = <PCA955X_TYPE_GPIO>;
+	};
+
+	led@0 {
+		label = "red:power";
+		linux,default-trigger = "default-on";
+		reg = <0>;
+		type = <PCA955X_TYPE_LED>;
+	};
+	led@1 {
+		label = "green:power";
+		reg = <1>;
+		type = <PCA955X_TYPE_LED>;
+	};
+	led@2 {
+		label = "pca9552:yellow";
+		reg = <2>;
+		type = <PCA955X_TYPE_LED>;
+	};
+	led@3 {
+		label = "pca9552:white";
+		reg = <3>;
+		type = <PCA955X_TYPE_LED>;
+	};
+};
-- 
2.7.5

Re: [PATCH v3 3/4] leds: pca955x: add GPIO support

[Pavel Machek]

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

Hi!

> The PCA955x family of chips are I2C LED blinkers whose pins not used
> to control LEDs can be used as general purpose I/Os (GPIOs).
> 
> The following adds such a support by defining different operation
> modes for the pins (See bindings documentation for more details). The
> pca955x driver is then extended with a gpio_chip when some of pins are
> operating as GPIOs. The default operating mode is to behave as a LED.
> 
> The GPIO support is conditioned by CONFIG_LEDS_PCA955X_GPIO.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

For the series,

Acked-by: Pavel Machek <pavel@ucw.cz>

with minor comments below.

> @@ -125,6 +130,7 @@ struct pca955x_led {
>  	struct led_classdev	led_cdev;
>  	int			led_num;	/* 0 .. 15 potentially */
>  	char			name[32];
> +	u32			type;

u32 is highly non-standard here. enum pca955x_led_type?

> +/*
> + * Read the INPUT register, which contains the state of LEDs.
> + */
> +static u8 pca955x_read_input(struct i2c_client *client, int n)
> +{
> +	return (u8)i2c_smbus_read_byte_data(client, n);
> +}

Is the cast needed? Should you attempt to handle errors?

> +#ifndef _DT_BINDINGS_LEDS_PCA955X_H
> +#define _DT_BINDINGS_LEDS_PCA955X_H
> +
> +#define PCA955X_TYPE_NONE         0
> +#define PCA955X_TYPE_LED          1
> +#define PCA955X_TYPE_GPIO         2

And make these into the new enum pca955x_led_type?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v3 3/4] leds: pca955x: add GPIO support

[Cédric Le Goater]

Hello,

On 08/11/2017 03:47 PM, Pavel Machek wrote:
> Hi!
> 
>> The PCA955x family of chips are I2C LED blinkers whose pins not used
>> to control LEDs can be used as general purpose I/Os (GPIOs).
>>
>> The following adds such a support by defining different operation
>> modes for the pins (See bindings documentation for more details). The
>> pca955x driver is then extended with a gpio_chip when some of pins are
>> operating as GPIOs. The default operating mode is to behave as a LED.
>>
>> The GPIO support is conditioned by CONFIG_LEDS_PCA955X_GPIO.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> 
> For the series,
> 
> Acked-by: Pavel Machek <pavel@ucw.cz>
> 
> with minor comments below.
> 
>> @@ -125,6 +130,7 @@ struct pca955x_led {
>>  	struct led_classdev	led_cdev;
>>  	int			led_num;	/* 0 .. 15 potentially */
>>  	char			name[32];
>> +	u32			type;
> 
> u32 is highly non-standard here. enum pca955x_led_type?
>
> 
>> +/*
>> + * Read the INPUT register, which contains the state of LEDs.
>> + */
>> +static u8 pca955x_read_input(struct i2c_client *client, int n)
>> +{
>> +	return (u8)i2c_smbus_read_byte_data(client, n);
>> +}
> 
> Is the cast needed? Should you attempt to handle errors?

ah. this is a left over. I can fix in a resend.

>> +#ifndef _DT_BINDINGS_LEDS_PCA955X_H
>> +#define _DT_BINDINGS_LEDS_PCA955X_H
>> +
>> +#define PCA955X_TYPE_NONE         0
>> +#define PCA955X_TYPE_LED          1
>> +#define PCA955X_TYPE_GPIO         2
> 
> And make these into the new enum pca955x_led_type?

These are used in the device tree bindings, so we should keep the u32
I think.

Thanks,

C.

Re: [PATCH v3 3/4] leds: pca955x: add GPIO support

[Pavel Machek]

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

Hi!

> > with minor comments below.
> > 
> >> @@ -125,6 +130,7 @@ struct pca955x_led {
> >>  	struct led_classdev	led_cdev;
> >>  	int			led_num;	/* 0 .. 15 potentially */
> >>  	char			name[32];
> >> +	u32			type;
> > 
> > u32 is highly non-standard here. enum pca955x_led_type?
> >
> > 
> >> +/*
> >> + * Read the INPUT register, which contains the state of LEDs.
> >> + */
> >> +static u8 pca955x_read_input(struct i2c_client *client, int n)
> >> +{
> >> +	return (u8)i2c_smbus_read_byte_data(client, n);
> >> +}
> > 
> > Is the cast needed? Should you attempt to handle errors?
> 
> ah. this is a left over. I can fix in a resend.

No need to resend just for this.

Should you WARN_ON() if the read_byte_data returns < 0 or something?

https://linuxtv.org/downloads/v4l-dvb-internals/device-drivers/API-i2c-smbus-read-byte-data.html

> >> +#ifndef _DT_BINDINGS_LEDS_PCA955X_H
> >> +#define _DT_BINDINGS_LEDS_PCA955X_H
> >> +
> >> +#define PCA955X_TYPE_NONE         0
> >> +#define PCA955X_TYPE_LED          1
> >> +#define PCA955X_TYPE_GPIO         2
> > 
> > And make these into the new enum pca955x_led_type?
> 
> These are used in the device tree bindings, so we should keep the u32
> I think.

Aha, ok, makes sense.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v3 3/4] leds: pca955x: add GPIO support

[Cédric Le Goater]

On 08/11/2017 05:37 PM, Pavel Machek wrote:
> Hi!
> 
>>> with minor comments below.
>>>
>>>> @@ -125,6 +130,7 @@ struct pca955x_led {
>>>>  	struct led_classdev	led_cdev;
>>>>  	int			led_num;	/* 0 .. 15 potentially */
>>>>  	char			name[32];
>>>> +	u32			type;
>>>
>>> u32 is highly non-standard here. enum pca955x_led_type?
>>>
>>>
>>>> +/*
>>>> + * Read the INPUT register, which contains the state of LEDs.
>>>> + */
>>>> +static u8 pca955x_read_input(struct i2c_client *client, int n)
>>>> +{
>>>> +	return (u8)i2c_smbus_read_byte_data(client, n);
>>>> +}
>>>
>>> Is the cast needed? Should you attempt to handle errors?
>>
>> ah. this is a left over. I can fix in a resend.
> 
> No need to resend just for this.
> 
> Should you WARN_ON() if the read_byte_data returns < 0 or something?
> 
> https://linuxtv.org/downloads/v4l-dvb-internals/device-drivers/API-i2c-smbus-read-byte-data.html

I don't think so as this is just to read the gpio value. 

I suppose that the i2c layer will output some errors before 
if it catches some invalid state.

C.

>>>> +#ifndef _DT_BINDINGS_LEDS_PCA955X_H
>>>> +#define _DT_BINDINGS_LEDS_PCA955X_H
>>>> +
>>>> +#define PCA955X_TYPE_NONE         0
>>>> +#define PCA955X_TYPE_LED          1
>>>> +#define PCA955X_TYPE_GPIO         2
>>>
>>> And make these into the new enum pca955x_led_type?
>>
>> These are used in the device tree bindings, so we should keep the u32
>> I think.
> 
> Aha, ok, makes sense.
> 									Pavel
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/4] leds: pca955x: add GPIO support

[Pavel Machek]

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

Hi!

> >>>> +static u8 pca955x_read_input(struct i2c_client *client, int n)
> >>>> +{
> >>>> +	return (u8)i2c_smbus_read_byte_data(client, n);
> >>>> +}
> >>>
> >>> Is the cast needed? Should you attempt to handle errors?
> >>
> >> ah. this is a left over. I can fix in a resend.
> > 
> > No need to resend just for this.
> > 
> > Should you WARN_ON() if the read_byte_data returns < 0 or something?
> > 
> > https://linuxtv.org/downloads/v4l-dvb-internals/device-drivers/API-i2c-smbus-read-byte-data.html
> 
> I don't think so as this is just to read the gpio value. 
> 
> I suppose that the i2c layer will output some errors before 
> if it catches some invalid state.

Normally its driver's job to output errors. (Same for write).

									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v3 0/4] leds: pca955x: add GPIO support

[Jacek Anaszewski]

Hi Cedric,

Thanks for the updated set.

Applied.

Best regards,
Jacek Anaszewski

On 08/08/2017 03:42 PM, Cédric Le Goater wrote:
> Hello !
> 
> The PCA955x family of chips are I2C LED blinkers whose pins not used
> to control LEDs can be used as general purpose I/Os (GPIOs).
> 
> The following adds support for device tree and Open Firmware to be
> able do define different operation modes for each pin. See bindings
> documentation for more details. The pca955x driver is then extended
> with a gpio_chip when pins are operating in GPIO mode.
> 
> The driver follows the scheme of the leds-pca9532 driver which behaves
> quite similarly.
>  
> Thanks,
> 
> C.
> 
> Changes since v2:
> 
>  - removed 'if (pdata)' tests
> 
> Changes since v1:
> 
>  - split the patchset in two : DT support and GPIO support
>  - introduced the use of devm_led_classdev_register()
>  - replaced the 'compatible' property with 'type' 
>  - removed the 'gpio-base' property
> 
> Cédric Le Goater (4):
>   leds: pca955x: add device tree support
>   leds: pca955x: use devm_led_classdev_register
>   leds: pca955x: add GPIO support
>   dt-bindings leds: add pca955x
> 
>  .../devicetree/bindings/leds/leds-pca955x.txt      |  88 +++++++
>  drivers/leds/Kconfig                               |  11 +
>  drivers/leds/leds-pca955x.c                        | 254 +++++++++++++++++----
>  include/dt-bindings/leds/leds-pca955x.h            |  16 ++
>  4 files changed, 323 insertions(+), 46 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-pca955x.txt
>  create mode 100644 include/dt-bindings/leds/leds-pca955x.h
> 

Re: [PATCH v3 3/4] leds: pca955x: add GPIO support

[Cédric Le Goater]

On 08/12/2017 10:42 AM, Pavel Machek wrote:
> Hi!
> 
>>>>>> +static u8 pca955x_read_input(struct i2c_client *client, int n)
>>>>>> +{
>>>>>> +	return (u8)i2c_smbus_read_byte_data(client, n);
>>>>>> +}
>>>>>
>>>>> Is the cast needed? Should you attempt to handle errors?
>>>>
>>>> ah. this is a left over. I can fix in a resend.
>>>
>>> No need to resend just for this.
>>>
>>> Should you WARN_ON() if the read_byte_data returns < 0 or something?
>>>
>>> https://linuxtv.org/downloads/v4l-dvb-internals/device-drivers/API-i2c-smbus-read-byte-data.html
>>
>> I don't think so as this is just to read the gpio value. 
>>
>> I suppose that the i2c layer will output some errors before 
>> if it catches some invalid state.
> 
> Normally its driver's job to output errors. (Same for write).

ok. I can respin the patchset with an extra patch adding checks
for I2C errors (see below). In which order would you want that, 
before the gpio support or after ? 

Thanks,

C.  

>From c52749964ee435eec8dc5b8634f7f76cf637e7b1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= <clg@kaod.org>
Date: Thu, 24 Aug 2017 13:27:45 +0200
Subject: [PATCH] leds: pca955x: check for I2C errors
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This should also allow probing to fail when a pca955x chip is not
found on a I2C bus.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 drivers/leds/leds-pca955x.c | 89 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 70 insertions(+), 19 deletions(-)

diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
index f062d1e7640f..17ac7cf2728d 100644
--- a/drivers/leds/leds-pca955x.c
+++ b/drivers/leds/leds-pca955x.c
@@ -165,13 +165,18 @@ static inline u8 pca955x_ledsel(u8 oldval, int led_num, int state)
  * Write to frequency prescaler register, used to program the
  * period of the PWM output.  period = (PSCx + 1) / 38
  */
-static void pca955x_write_psc(struct i2c_client *client, int n, u8 val)
+static int pca955x_write_psc(struct i2c_client *client, int n, u8 val)
 {
 	struct pca955x *pca955x = i2c_get_clientdata(client);
+	int ret;
 
-	i2c_smbus_write_byte_data(client,
+	ret = i2c_smbus_write_byte_data(client,
 		pca95xx_num_input_regs(pca955x->chipdef->bits) + 2*n,
 		val);
+	if (ret < 0)
+		dev_err(&client->dev, "%s: reg 0x%x, val 0x%x, err %d\n",
+			__func__, n, val, ret);
+	return ret;
 }
 
 /*
@@ -181,38 +186,57 @@ static void pca955x_write_psc(struct i2c_client *client, int n, u8 val)
  *
  * Duty cycle is (256 - PWMx) / 256
  */
-static void pca955x_write_pwm(struct i2c_client *client, int n, u8 val)
+static int pca955x_write_pwm(struct i2c_client *client, int n, u8 val)
 {
 	struct pca955x *pca955x = i2c_get_clientdata(client);
+	int ret;
 
-	i2c_smbus_write_byte_data(client,
+	ret = i2c_smbus_write_byte_data(client,
 		pca95xx_num_input_regs(pca955x->chipdef->bits) + 1 + 2*n,
 		val);
+	if (ret < 0)
+		dev_err(&client->dev, "%s: reg 0x%x, val 0x%x, err %d\n",
+			__func__, n, val, ret);
+	return ret;
 }
 
+
 /*
  * Write to LED selector register, which determines the source that
  * drives the LED output.
  */
-static void pca955x_write_ls(struct i2c_client *client, int n, u8 val)
+static int pca955x_write_ls(struct i2c_client *client, int n, u8 val)
 {
 	struct pca955x *pca955x = i2c_get_clientdata(client);
+	int ret;
 
-	i2c_smbus_write_byte_data(client,
+	ret = i2c_smbus_write_byte_data(client,
 		pca95xx_num_input_regs(pca955x->chipdef->bits) + 4 + n,
 		val);
+	if (ret < 0)
+		dev_err(&client->dev, "%s: reg 0x%x, val 0x%x, err %d\n",
+			__func__, n, val, ret);
+	return ret;
 }
 
 /*
  * Read the LED selector register, which determines the source that
  * drives the LED output.
  */
-static u8 pca955x_read_ls(struct i2c_client *client, int n)
+static int pca955x_read_ls(struct i2c_client *client, int n, u8 *val)
 {
 	struct pca955x *pca955x = i2c_get_clientdata(client);
+	int ret;
 
-	return (u8) i2c_smbus_read_byte_data(client,
+	ret = i2c_smbus_read_byte_data(client,
 		pca95xx_num_input_regs(pca955x->chipdef->bits) + 4 + n);
+	if (ret < 0) {
+		dev_err(&client->dev, "%s: reg 0x%x, err %d\n",
+			__func__, n, ret);
+		return ret;
+	}
+	*val = (u8) ret;
+	return 0;
 }
 
 static int pca955x_led_set(struct led_classdev *led_cdev,
@@ -223,6 +247,7 @@ static int pca955x_led_set(struct led_classdev *led_cdev,
 	u8 ls;
 	int chip_ls;	/* which LSx to use (0-3 potentially) */
 	int ls_led;	/* which set of bits within LSx to use (0-3) */
+	int ret;
 
 	pca955x_led = container_of(led_cdev, struct pca955x_led, led_cdev);
 	pca955x = pca955x_led->pca955x;
@@ -232,7 +257,9 @@ static int pca955x_led_set(struct led_classdev *led_cdev,
 
 	mutex_lock(&pca955x->lock);
 
-	ls = pca955x_read_ls(pca955x->client, chip_ls);
+	ret = pca955x_read_ls(pca955x->client, chip_ls, &ls);
+	if (ret)
+		return ret;
 
 	switch (value) {
 	case LED_FULL:
@@ -252,13 +279,17 @@ static int pca955x_led_set(struct led_classdev *led_cdev,
 		 * OFF, HALF, or FULL.  But, this is probably better than
 		 * just turning off for all other values.
 		 */
-		pca955x_write_pwm(pca955x->client, 1,
-				255 - value);
+		ret = pca955x_write_pwm(pca955x->client, 1,
+					255 - value);
+		if (ret)
+			return ret;
 		ls = pca955x_ledsel(ls, ls_led, PCA955X_LS_BLINK1);
 		break;
 	}
 
-	pca955x_write_ls(pca955x->client, chip_ls, ls);
+	ret = pca955x_write_ls(pca955x->client, chip_ls, ls);
+	if (ret)
+		return ret;
 
 	mutex_unlock(&pca955x->lock);
 
@@ -269,9 +300,18 @@ static int pca955x_led_set(struct led_classdev *led_cdev,
 /*
  * Read the INPUT register, which contains the state of LEDs.
  */
-static u8 pca955x_read_input(struct i2c_client *client, int n)
+static int pca955x_read_input(struct i2c_client *client, int n, u8 *val)
 {
-	return (u8)i2c_smbus_read_byte_data(client, n);
+	int ret = i2c_smbus_read_byte_data(client, n);
+
+	if (ret < 0) {
+		dev_err(&client->dev, "%s: reg 0x%x, err %d\n",
+			__func__, n, ret);
+		return ret;
+	}
+	*val = (u8) ret;
+	return 0;
+
 }
 
 static int pca955x_gpio_request_pin(struct gpio_chip *gc, unsigned int offset)
@@ -301,7 +341,10 @@ static int pca955x_gpio_get_value(struct gpio_chip *gc, unsigned int offset)
 {
 	struct pca955x *pca955x = gpiochip_get_data(gc);
 	struct pca955x_led *led = &pca955x->leds[offset];
-	u8 reg = pca955x_read_input(pca955x->client, led->led_num / 8);
+	u8 reg = 0;
+
+	/* There is nothing we can do about errors */
+	pca955x_read_input(pca955x->client, led->led_num / 8, &reg);
 
 	return !!(reg & (1 << (led->led_num % 8)));
 }
@@ -496,14 +539,22 @@ static int pca955x_probe(struct i2c_client *client,
 	}
 
 	/* PWM0 is used for half brightness or 50% duty cycle */
-	pca955x_write_pwm(client, 0, 255-LED_HALF);
+	err = pca955x_write_pwm(client, 0, 255-LED_HALF);
+	if (err)
+		return err;
 
 	/* PWM1 is used for variable brightness, default to OFF */
-	pca955x_write_pwm(client, 1, 0);
+	err = pca955x_write_pwm(client, 1, 0);
+	if (err)
+		return err;
 
 	/* Set to fast frequency so we do not see flashing */
-	pca955x_write_psc(client, 0, 0);
-	pca955x_write_psc(client, 1, 0);
+	err = pca955x_write_psc(client, 0, 0);
+	if (err)
+		return err;
+	err = pca955x_write_psc(client, 1, 0);
+	if (err)
+		return err;
 
 #ifdef CONFIG_LEDS_PCA955X_GPIO
 	if (ngpios) {
-- 
2.13.5

Re: [PATCH v3 3/4] leds: pca955x: add GPIO support

[Jacek Anaszewski]

Hi Cedric,

On 08/24/2017 01:30 PM, Cédric Le Goater wrote:
> On 08/12/2017 10:42 AM, Pavel Machek wrote:
>> Hi!
>>
>>>>>>> +static u8 pca955x_read_input(struct i2c_client *client, int n)
>>>>>>> +{
>>>>>>> +	return (u8)i2c_smbus_read_byte_data(client, n);
>>>>>>> +}
>>>>>>
>>>>>> Is the cast needed? Should you attempt to handle errors?
>>>>>
>>>>> ah. this is a left over. I can fix in a resend.
>>>>
>>>> No need to resend just for this.
>>>>
>>>> Should you WARN_ON() if the read_byte_data returns < 0 or something?
>>>>
>>>> https://linuxtv.org/downloads/v4l-dvb-internals/device-drivers/API-i2c-smbus-read-byte-data.html
>>>
>>> I don't think so as this is just to read the gpio value. 
>>>
>>> I suppose that the i2c layer will output some errors before 
>>> if it catches some invalid state.
>>
>> Normally its driver's job to output errors. (Same for write).
> 
> ok. I can respin the patchset with an extra patch adding checks
> for I2C errors (see below). In which order would you want that, 
> before the gpio support or after ? 

I'd prefer an incremental patch. The patch set has received 10
days of testing and the merge window is imminent. Not the best
moment to respin the for-next branch.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v3 3/4] leds: pca955x: add GPIO support

[Cédric Le Goater]

On 08/24/2017 10:03 PM, Jacek Anaszewski wrote:
> Hi Cedric,
> 
> On 08/24/2017 01:30 PM, Cédric Le Goater wrote:
>> On 08/12/2017 10:42 AM, Pavel Machek wrote:
>>> Hi!
>>>
>>>>>>>> +static u8 pca955x_read_input(struct i2c_client *client, int n)
>>>>>>>> +{
>>>>>>>> +	return (u8)i2c_smbus_read_byte_data(client, n);
>>>>>>>> +}
>>>>>>>
>>>>>>> Is the cast needed? Should you attempt to handle errors?
>>>>>>
>>>>>> ah. this is a left over. I can fix in a resend.
>>>>>
>>>>> No need to resend just for this.
>>>>>
>>>>> Should you WARN_ON() if the read_byte_data returns < 0 or something?
>>>>>
>>>>> https://linuxtv.org/downloads/v4l-dvb-internals/device-drivers/API-i2c-smbus-read-byte-data.html
>>>>
>>>> I don't think so as this is just to read the gpio value. 
>>>>
>>>> I suppose that the i2c layer will output some errors before 
>>>> if it catches some invalid state.
>>>
>>> Normally its driver's job to output errors. (Same for write).
>>
>> ok. I can respin the patchset with an extra patch adding checks
>> for I2C errors (see below). In which order would you want that, 
>> before the gpio support or after ? 
> 
> I'd prefer an incremental patch. The patch set has received 10
> days of testing and the merge window is imminent. Not the best
> moment to respin the for-next branch.

OK. The patch applies on top of the patchset. I will give it some 
more testing before sending.

Thanks,

C.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html