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

[PATCH v2 0/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 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 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                        | 263 +++++++++++++++++----
 include/dt-bindings/leds/leds-pca955x.h            |  16 ++
 4 files changed, 333 insertions(+), 45 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-pca955x.txt
 create mode 100644 include/dt-bindings/leds/leds-pca955x.h

-- 
2.7.5

[PATCH v2 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@kaod.org>
---
 drivers/leds/leds-pca955x.c | 89 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 84 insertions(+), 5 deletions(-)

diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
index 9a873118ea5f..3afd481a760f 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)) !=
@@ -378,6 +456,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

[PATCH v2 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-Bxea+6Xhats@public.gmane.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 3afd481a760f..a8918ff0a7e9 100644
--- a/drivers/leds/leds-pca955x.c
+++ b/drivers/leds/leds-pca955x.c
@@ -412,10 +412,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 */
@@ -433,23 +433,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 = {
@@ -459,7 +442,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

--
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 v2 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>
---
 drivers/leds/Kconfig                    |  11 +++
 drivers/leds/leds-pca955x.c             | 158 +++++++++++++++++++++++++++-----
 include/dt-bindings/leds/leds-pca955x.h |  16 ++++
 3 files changed, 162 insertions(+), 23 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 a8918ff0a7e9..ac0f726ff1af 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];
@@ -391,36 +459,52 @@ static int pca955x_probe(struct i2c_client *client,
 	pca955x->chipdef = chip;
 
 	for (i = 0; i < chip->bits; i++) {
+		struct pca955x_led *pdata_led = &pdata->leds[i];
+
 		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) {
-			if (pdata->leds[i].name)
+		pca955x_led->type = pdata_led->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) {
+				if (pdata->leds[i].name)
+					snprintf(pca955x_led->name,
+						 sizeof(pca955x_led->name),
+						 "pca955x:%s",
+						 pdata->leds[i].name);
+				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:%s",
-					pdata->leds[i].name);
-			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);
-		}
+					 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);
@@ -432,6 +516,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 v2 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-Bxea+6Xhats@public.gmane.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

--
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 v2 0/4] leds: pca955x: add GPIO support

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 769 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 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.

Is there reason not to treat pca955x chip as a GPIO extender, and then
use leds-gpio on top?

									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 v2 0/4] leds: pca955x: add GPIO support

[Cédric Le Goater]

On 08/02/2017 01:33 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 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.
> 
> Is there reason not to treat pca955x chip as a GPIO extender, and then
> use leds-gpio on top?

I would say mostly because there is already a leds-pca955x driver and
that the primary nature of the chip is being a LED blinker. It also 
supports different blinking rates.

If all pins are defined as GPIOs, the device then behaves as a GPIO 
extender on which we can use leds-gpio. It might be a bit awkward
that way but it should work.

Thanks,

C.

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

[Pavel Machek]

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

On Wed 2017-08-02 13:57:29, Cédric Le Goater wrote:
> On 08/02/2017 01:33 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 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.
> > 
> > Is there reason not to treat pca955x chip as a GPIO extender, and then
> > use leds-gpio on top?
> 
> I would say mostly because there is already a leds-pca955x driver and
> that the primary nature of the chip is being a LED blinker. It also 
> supports different blinking rates.

Ok, I guess gpio-led driver can not to blinking, so this 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 v2 1/4] leds: pca955x: add device tree support

[Jacek Anaszewski]

Hi Cedric,

Thanks for the updated patch set.
Please refer below to my comments.

On 08/01/2017 02:09 PM, Cédric Le Goater wrote:
> 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>
> ---
>  drivers/leds/leds-pca955x.c | 89 ++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 84 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
> index 9a873118ea5f..3afd481a760f 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);
> +	}

Since from this moment pdata should be in every case available you
could get rid of "if (pdata) check" right after

"if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))".



>  	/* Make sure the slave address / chip type combo given is possible */
>  	if ((client->addr & ~((1 << chip->slv_addr_shift) - 1)) !=
> @@ -378,6 +456,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,
> 

-- 
Best regards,
Jacek Anaszewski
--
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 v2 3/4] leds: pca955x: add GPIO support

[Jacek Anaszewski]

Hi Cedric,

On 08/01/2017 02:09 PM, Cédric Le Goater wrote:
> 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>
> ---
>  drivers/leds/Kconfig                    |  11 +++
>  drivers/leds/leds-pca955x.c             | 158 +++++++++++++++++++++++++++-----
>  include/dt-bindings/leds/leds-pca955x.h |  16 ++++
>  3 files changed, 162 insertions(+), 23 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 a8918ff0a7e9..ac0f726ff1af 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];
> @@ -391,36 +459,52 @@ static int pca955x_probe(struct i2c_client *client,
>  	pca955x->chipdef = chip;
>  
>  	for (i = 0; i < chip->bits; i++) {
> +		struct pca955x_led *pdata_led = &pdata->leds[i];
> +
>  		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) {
> -			if (pdata->leds[i].name)
> +		pca955x_led->type = pdata_led->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) {

Similarly as in 1/4: this check seems to be redundant and we could
simplify the code a bit here.

> +				if (pdata->leds[i].name)
> +					snprintf(pca955x_led->name,
> +						 sizeof(pca955x_led->name),
> +						 "pca955x:%s",
> +						 pdata->leds[i].name);

DT label property should contain also vendor prefix, and there should be
no need to concatenate the segments here. The situation when label is
not provided and LED class device name is taken from DT node name
should be considered as a fallback.


> +				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:%s",
> -					pdata->leds[i].name);
> -			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);
> -		}
> +					 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);
> @@ -432,6 +516,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 */
> 

-- 
Best regards,
Jacek Anaszewski

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

[Cédric Le Goater]

On 08/06/2017 11:42 PM, Jacek Anaszewski wrote:
> Hi Cedric,
> 
> Thanks for the updated patch set.
> Please refer below to my comments.
> 
> On 08/01/2017 02:09 PM, Cédric Le Goater wrote:
>> It will be used in a following patch to define different operation
>> modes for each pin.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  drivers/leds/leds-pca955x.c | 89 ++++++++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 84 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
>> index 9a873118ea5f..3afd481a760f 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);
>> +	}
> 
> Since from this moment pdata should be in every case available you
> could get rid of "if (pdata) check" right after
> 
> "if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))".

Sure. Will do.

Thanks,

C. 

> 
> 
>>  	/* Make sure the slave address / chip type combo given is possible */
>>  	if ((client->addr & ~((1 << chip->slv_addr_shift) - 1)) !=
>> @@ -378,6 +456,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,
>>
> 

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

[Cédric Le Goater]

On 08/06/2017 11:42 PM, Jacek Anaszewski wrote:
> Hi Cedric,
> 
> On 08/01/2017 02:09 PM, Cédric Le Goater wrote:
>> 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>
>> ---
>>  drivers/leds/Kconfig                    |  11 +++
>>  drivers/leds/leds-pca955x.c             | 158 +++++++++++++++++++++++++++-----
>>  include/dt-bindings/leds/leds-pca955x.h |  16 ++++
>>  3 files changed, 162 insertions(+), 23 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 a8918ff0a7e9..ac0f726ff1af 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];
>> @@ -391,36 +459,52 @@ static int pca955x_probe(struct i2c_client *client,
>>  	pca955x->chipdef = chip;
>>  
>>  	for (i = 0; i < chip->bits; i++) {
>> +		struct pca955x_led *pdata_led = &pdata->leds[i];
>> +
>>  		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) {
>> -			if (pdata->leds[i].name)
>> +		pca955x_led->type = pdata_led->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) {
> 
> Similarly as in 1/4: this check seems to be redundant and we could
> simplify the code a bit here.

yes.

 
>> +				if (pdata->leds[i].name)
>> +					snprintf(pca955x_led->name,
>> +						 sizeof(pca955x_led->name),
>> +						 "pca955x:%s",
>> +						 pdata->leds[i].name);
> 
> DT label property should contain also vendor prefix, and there should be
> no need to concatenate the segments here. The situation when label is
> not provided and LED class device name is taken from DT node name
> should be considered as a fallback.

So, should I start the patchset with a preliminary cleanup removing all
the calls doing snprintf(pdata->leds[reg].name ...) ? This is the case in :
pca955x_pdata_of_init()

Thanks,

C.

> 
>> +				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:%s",
>> -					pdata->leds[i].name);
>> -			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);
>> -		}
>> +					 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);
>> @@ -432,6 +516,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 */
>>
> 

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

[Jacek Anaszewski]

On 08/07/2017 11:09 AM, Cédric Le Goater wrote:
> On 08/06/2017 11:42 PM, Jacek Anaszewski wrote:
>> Hi Cedric,
>>
>> On 08/01/2017 02:09 PM, Cédric Le Goater wrote:
>>> 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>
>>> ---
>>>  drivers/leds/Kconfig                    |  11 +++
>>>  drivers/leds/leds-pca955x.c             | 158 +++++++++++++++++++++++++++-----
>>>  include/dt-bindings/leds/leds-pca955x.h |  16 ++++
>>>  3 files changed, 162 insertions(+), 23 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 a8918ff0a7e9..ac0f726ff1af 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];
>>> @@ -391,36 +459,52 @@ static int pca955x_probe(struct i2c_client *client,
>>>  	pca955x->chipdef = chip;
>>>  
>>>  	for (i = 0; i < chip->bits; i++) {
>>> +		struct pca955x_led *pdata_led = &pdata->leds[i];
>>> +
>>>  		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) {
>>> -			if (pdata->leds[i].name)
>>> +		pca955x_led->type = pdata_led->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) {
>>
>> Similarly as in 1/4: this check seems to be redundant and we could
>> simplify the code a bit here.
> 
> yes.
> 
>  
>>> +				if (pdata->leds[i].name)
>>> +					snprintf(pca955x_led->name,
>>> +						 sizeof(pca955x_led->name),
>>> +						 "pca955x:%s",
>>> +						 pdata->leds[i].name);
>>
>> DT label property should contain also vendor prefix, and there should be
>> no need to concatenate the segments here. The situation when label is
>> not provided and LED class device name is taken from DT node name
>> should be considered as a fallback.
> 
> So, should I start the patchset with a preliminary cleanup removing all
> the calls doing snprintf(pdata->leds[reg].name ...) ? This is the case in :
> pca955x_pdata_of_init()

I've looked at the DT documentation and it fact it doesn't mention
the label format. This is Documentation/leds/leds-class.txt which
defines the LED class device name format. Since some other LED class
drivers also add device prefix to the name taken from platform data,
and what's more there are users of this one, let's not modify that.
Please only remove redundant "if (pdata)" checks.

Please note that I'll be able to review the next patch set no sooner
than in the beginning of the next week.

-- 
Best regards,
Jacek Anaszewski