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

["Cédric Le Goater" <clg@kaod.org>]

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 */
>>
>