Hello Jacek,
On 05/11/2017 10:23 PM, Jacek Anaszewski wrote:
> Hi Cedric,
>
> Thanks for the patch.
>
> Generally I'd split it into two patches:
> 1/2: addition of LED class specific DT support
> 2/2: addition of GPIO support (for this one please cc also
> GPIO subsystem maintainer)
OK. I will in next version.
> Please also see my comments below.
>
> On 05/09/2017 08:36 AM, 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 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.
>>
>> Signed-off-by: Cédric Le Goater <clg@xxxxxxxx>
>> ---
>> .../devicetree/bindings/leds/leds-pca955x.txt | 103 ++++++++
>> drivers/leds/Kconfig | 11 +
>> drivers/leds/leds-pca955x.c | 290 ++++++++++++++++++---
>> 3 files changed, 374 insertions(+), 30 deletions(-)
>> 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..98d1053dd1b0
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/leds-pca955x.txt
>> @@ -0,0 +1,103 @@
>> +LEDs connected to pca9550, pca9551, pca9552, pca9553,
>> +
>> +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: if present, must not be 0.
>> +- gpio-base : base number of the pins used as GPIOs. If there are more
>> + than one, they should be contiguous. See 'type' property
>> + below.
>> +
>> +LED sub-node properties:
>> +- reg : number of LED line.
>> + from 0 to 1 in pca9550
>> + from 0 to 7 in pca9551
>> + from 0 to 15 in pca9552
>> + from 0 to 3 in pca9553
>> +- compatible: either "none", "led" (default) or "gpio".
>> +- 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-base = <12>;
>> +
>> + gpio@12 {
>> + label = "GPIO12";
>> + reg = <12>;
>> + compatible = "gpio";
>> + };
>> + gpio@13 {
>> + label = "GPIO13";
>> + reg = <13>;
>> + compatible = "gpio";
>> + };
>> + gpio@14 {
>> + label = "GPIO14";
>> + reg = <14>;
>> + compatible = "gpio";
>> + };
>> + gpio@15 {
>> + label = "GPIO15";
>> + reg = <15>;
>> + compatible = "gpio";
>> + };
>
> I think that for GPIO pins we should have GPIO specific bindings,
> but DT maintainer would have to give his opinion here.
I have got my inspiration from the tca6507 bindings.
> Also I'm not sure if using compatible property this way is correct.
yes.
>
>> + led@0 {
>> + label = "LED0";
>
> This is not in line with LED class device naming scheme:
>
> "devicename:colour:function"
>
> Please compare other LED class bindings.
OK. I didn't pay enough attention to the naming. I will change
that.
>
>> + linux,default-trigger = "default-on";
>> + reg = <0>;
>> + };
>> + led@1 {
>> + label = "LED1";
>> + reg = <1>;
>> + };
>> + led@2 {
>> + label = "LED2";
>> + reg = <2>;
>> + };
>> + led@3 {
>> + label = "LED3";
>> + reg = <3>;
>> + };
>> +
>> + none@4 {
>> + reg = <4>;
>> + compatible = "none";
>> + };
>> + none@5 {
>> + reg = <5>;
>> + compatible = "none";
>> + };
>> + none@6 {
>> + reg = <6>;
>> + compatible = "none";
>> + };
>> + none@7 {
>> + reg = <7>;
>> + compatible = "none";
>> + };
>
> Why would we need to define nodes for pins that will not be used?
yes :) I agree. And anyway, the driver will consider the pins unused
if the 'reg' property is not defined in the DT.
>> +
>> + led@8 {
>> + label = "LED8";
>> + reg = <8>;
>> + };
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index c621cbbb5768..0177a36dab3d 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -352,6 +352,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.
>> +
>
> I wonder if GPIO subsystem wouldn't be a better place for this config.
>
> Adding Linus Walleij.
This config is similar to :
config LEDS_PCA9532_GPIO
which is defined under the LED subsystem.
>> 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 78a7ce816a47..afbdccb0e8b6 100644
>> --- a/drivers/leds/leds-pca955x.c
>> +++ b/drivers/leds/leds-pca955x.c
>> @@ -45,10 +45,13 @@
>> #include <linux/delay.h>
>> #include <linux/string.h>
>> #include <linux/ctype.h>
>> +#include <linux/gpio.h>
>> #include <linux/leds.h>
>> #include <linux/err.h>
>> #include <linux/i2c.h>
>> #include <linux/slab.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>
> Please sort include directives alphabetically as we are at it.
ok
>>
>> /* LED select registers determine the source that drives LED outputs */
>> #define PCA955X_LS_LED_ON 0x0 /* Output LOW */
>> @@ -56,6 +59,10 @@
>> #define PCA955X_LS_BLINK0 0x2 /* Blink at PWM0 rate */
>> #define PCA955X_LS_BLINK1 0x3 /* Blink at PWM1 rate */
>>
>> +#define PCA955X_TYPE_NONE 0
>> +#define PCA955X_TYPE_LED 1
>> +#define PCA955X_TYPE_GPIO 2
>> +
>> enum pca955x_type {
>> pca9550,
>> pca9551,
>> @@ -115,6 +122,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 {
>> @@ -122,6 +132,14 @@ struct pca955x_led {
>> struct led_classdev led_cdev;
>> int led_num; /* 0 .. 15 potentially */
>> char name[32];
>> + u32 type;
>> + const char *default_trigger;
>> +};
>> +
>> +struct pca955x_platform_data {
>> + struct pca955x_led *leds;
>> + int num_leds;
>> + int gpio_base;
>> };
>>
>> /* 8 bits per input register */
>> @@ -200,6 +218,14 @@ static u8 pca955x_read_ls(struct i2c_client *client, int n)
>> pca95xx_num_input_regs(pca955x->chipdef->bits) + 4 + n);
>> }
>>
>> +/*
>> + * Read the INPUT register, which constains 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_led_set(struct led_classdev *led_cdev,
>> enum led_brightness value)
>> {
>> @@ -250,6 +276,156 @@ static int pca955x_led_set(struct led_classdev *led_cdev,
>> return 0;
>> }
>>
>> +#ifdef CONFIG_LEDS_PCA955X_GPIO
>> +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[gc->base + 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[gc->base + 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[gc->base + 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)
>> +{
>> + 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", ®);
>> + 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);
>> +
>> + pdata->leds[reg].type = PCA955X_TYPE_LED;
>> + if (of_property_match_string(child, "compatible", "gpio") >= 0)
>> + pdata->leds[reg].type = PCA955X_TYPE_GPIO;
>> + if (of_property_match_string(child, "compatible", "none") >= 0)
>> + pdata->leds[reg].type = PCA955X_TYPE_NONE;
>> +
>> + of_property_read_u32(child, "type", &pdata->leds[reg].type);
>> + of_property_read_string(child, "linux,default-trigger",
>> + &pdata->leds[reg].default_trigger);
>> + }
>> +
>> + pdata->num_leds = chip->bits;
>> +
>> + if (of_property_read_u32(np, "gpio-base", &pdata->gpio_base))
>> + pdata->gpio_base = -1;
>> +
>> + 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 pca963x_chipdef *chip)
>> +{
>> + return ERR_PTR(-ENODEV);
>> +}
>> +#endif
>> +
>> +static int pca955x_destroy_devices(struct pca955x *data, int n_devs)
>> +{
>> + int i = n_devs;
>> +
>> + if (!data)
>> + return -EINVAL;
>> +
>> + while (--i >= 0) {
>> + switch (data->leds[i].type) {
>> + case PCA955X_TYPE_NONE:
>> + case PCA955X_TYPE_GPIO:
>> + break;
>> + case PCA955X_TYPE_LED:
>> + led_classdev_unregister(&data->leds[i].led_cdev);
>
> If you switched to using devm_led_classdev_register() then you
> wouldn't need this loop.
ok.
Thanks for the review,
C.
>> + break;
>> + }
>> + }
>> +
>> +#ifdef CONFIG_LEDS_PCA955X_GPIO
>> + if (data->gpio.parent)
>> + gpiochip_remove(&data->gpio);
>> +#endif
>> +
>> + return 0;
>> +}
>> +
>> static int pca955x_probe(struct i2c_client *client,
>> const struct i2c_device_id *id)
>> {
>> @@ -257,8 +433,9 @@ 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;
>> + int ngpios = 0;
>>
>> if (id) {
>> chip = &pca955x_chipdefs[id->driver_data];
>> @@ -272,6 +449,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)) !=
>> @@ -313,36 +495,52 @@ static int pca955x_probe(struct i2c_client *client,
>> pca955x->chipdef = chip;
>>
>> for (i = 0; i < chip->bits; i++) {
>> + struct pca955x_led *pled = &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 = pled->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 = led_classdev_register(&client->dev,
>> - &pca955x_led->led_cdev);
>> - if (err < 0)
>> - goto exit;
>> - }
>> + err = led_classdev_register(&client->dev,
>> + &pca955x_led->led_cdev);
>> + if (err < 0)
>> + goto exit;
>>
>> - /* Turn off LEDs */
>> - for (i = 0; i < pca95xx_num_led_regs(chip->bits); i++)
>> - pca955x_write_ls(client, i, 0x55);
>> + /* Turn off LEDs */
>> + 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);
>> @@ -354,22 +552,53 @@ 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) {
>> + if (pdata->gpio_base == -1) {
>> + dev_warn(&client->dev, "gpio-base is undefined\n");
>> + goto exit;
>> + }
>> +
>> + 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 = pdata->gpio_base;
>> + pca955x->gpio.ngpio = ngpios;
>> + pca955x->gpio.parent = &client->dev;
>> + pca955x->gpio.owner = THIS_MODULE;
>> +
>> + err = gpiochip_add_data(&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");
>> + } else {
>> + dev_info(&client->dev, "gpios %i...%i\n",
>> + pca955x->gpio.base, pca955x->gpio.base +
>> + pca955x->gpio.ngpio - 1);
>> + }
>> + }
>> +#endif
>> +
>> return 0;
>>
>> exit:
>> - while (i--)
>> - led_classdev_unregister(&pca955x->leds[i].led_cdev);
>> -
>> + pca955x_destroy_devices(pca955x, i);
>> return err;
>> }
>>
>> static int pca955x_remove(struct i2c_client *client)
>> {
>> struct pca955x *pca955x = i2c_get_clientdata(client);
>> - int i;
>> + int err;
>>
>> - for (i = 0; i < pca955x->chipdef->bits; i++)
>> - led_classdev_unregister(&pca955x->leds[i].led_cdev);
>> + err = pca955x_destroy_devices(pca955x, pca955x->chipdef->bits);
>> + if (err)
>> + return err;
>>
>> return 0;
>> }
>> @@ -378,6 +607,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,
>>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
