Re: [leds-pca9532] Add gpio capability

Re: [leds-pca9532] Add gpio capability

[H Hartley Sweeten]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1696 bytes --]

>>>> + if (gpios) {
>>>> + data->gpio.label = "gpio-pca9532";
>>>> + data->gpio.set = pca9532_gpio_set_value;
>>>> + data->gpio.get = pca9532_gpio_get_value;
>>>> + data->gpio.request = pca9532_gpio_request_pin;
>>>> + data->gpio.can_sleep = 1;
>>>> + data->gpio.base = pdata->gpio_base;
>>>> + data->gpio.ngpio = 16;
>>>> + data->gpio.dev = &client->dev;
>>>> + data->gpio.owner = THIS_MODULE;
>>>
>>> I'd assume that you also need to define a direction_output-function which
>>> always
>>> returns success?
>>
>> Yes, you are right. It is not possible to set direction in hw, but I
>> should provide them.
>
> It should be okay to skip direction_input, because gpiolib will return an error
> if it is not available. This is exactly why IMO you need a direction_output
> returning 0. Otherwise drivers checking if that operation was successful, might
> be fooled.

>From the datasheet:


6.4 Pins used as GPIOs

LEDn pins not used to control LEDs can be used as General Purpose I/Os (GPIOs).

To use as input, set LEDn to high-impendance (00) and then read the pin state
via the INPUT0 or INPUT1 register.

For use as output, connect external pull-up resistor to the pin and size it
according to the DC recommended operating characteristics.  LEDn output is HIGH
when the output is programmed as high-impedance, and LOW when programmed LOW
through the 'LED selector' register.  The output can be pulse-width controlled
when PWM0 or PWM1 are used.


So, you should provide both a direction_input and direction_output.

Regards,
Hartley
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [leds-pca9532] Add gpio capability

[Wolfram Sang]

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

> From the datasheet:
[...]
> So, you should provide both a direction_input and direction_output.

He did in V2 of this patch (what made me also download the datasheet ;)).

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

RE: [leds-pca9532] Add gpio capability

[H Hartley Sweeten]

On Tuesday, February 01, 2011 1:02 AM, Wolfram Sang wrote:
>> From the datasheet:
>>[...]
>> So, you should provide both a direction_input and direction_output.
>
> He did in V2 of this patch (what made me also download the datasheet ;)).

Ok.  Just looked at the v2 patch.

> +static int pca9532_gpio_direction_input(struct gpio_chip *gc, unsigned offset)
> +{
> + struct pca9532_data *data = container_of(gc, struct pca9532_data, gpio);
> + struct pca9532_led *led = &data->leds[offset];
> +
> + /* To use as input ensure pin is not driven */
> + led->state = PCA9532_OFF;
> + pca9532_setled(led);
> +
> + return 0;
> +}
> +

This looks good.  When configured as an input the pin needs to be set high-impedance,
00 which is PCA9532_OFF).

> +static int pca9532_gpio_direction_output(struct gpio_chip *gc,
> unsigned offset, int val)
> +{
> + return 0;
> +}
> +

Not good.  When the pin is configured as an output, the direction_output method also
requests that the pin be set to a desired level.  For this driver that means that the
pca9532_gpio_direction_output function just needs to call pca9532_gpio_set_value.

Regards,
Hartley

Re: [leds-pca9532] Add gpio capability

[Joachim Eastwood]

On 2/1/11, H Hartley Sweeten <hartleys@visionengravers.com> wrote:
> On Tuesday, February 01, 2011 1:02 AM, Wolfram Sang wrote:
>>> From the datasheet:
>>>[...]
>>> So, you should provide both a direction_input and direction_output.
>>
>> He did in V2 of this patch (what made me also download the datasheet ;)).
>
> Ok.  Just looked at the v2 patch.
>
>> +static int pca9532_gpio_direction_input(struct gpio_chip *gc, unsigned
>> offset)
>> +{
>> + struct pca9532_data *data = container_of(gc, struct pca9532_data, gpio);
>> + struct pca9532_led *led = &data->leds[offset];
>> +
>> + /* To use as input ensure pin is not driven */
>> + led->state = PCA9532_OFF;
>> + pca9532_setled(led);
>> +
>> + return 0;
>> +}
>> +
>
> This looks good.  When configured as an input the pin needs to be set
> high-impedance,
> 00 which is PCA9532_OFF).
>
>> +static int pca9532_gpio_direction_output(struct gpio_chip *gc,
>> unsigned offset, int val)
>> +{
>> + return 0;
>> +}
>> +
>
> Not good.  When the pin is configured as an output, the direction_output
> method also
> requests that the pin be set to a desired level.  For this driver that means
> that the
> pca9532_gpio_direction_output function just needs to call
> pca9532_gpio_set_value.

ops, I overlooked the 'val' parameter in the direction_output
function.  I will create a v3.

Seems like I have been a bit careless with the direction setting... I
didn't really think through it since PCA9532 has no explicit direction
register.

Thanks for looking!

regards
Joachim Eastwood

> Regards,
> Hartley
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [leds-pca9532] Add gpio capability

[Wolfram Sang]

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


> > +static int pca9532_gpio_direction_output(struct gpio_chip *gc,
> > unsigned offset, int val)
> > +{
> > + return 0;
> > +}
> > +
> 
> Not good.  When the pin is configured as an output, the direction_output method also
> requests that the pin be set to a desired level.  For this driver that means that the
> pca9532_gpio_direction_output function just needs to call pca9532_gpio_set_value.

Ack, missed that. Thanks a lot!

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[leds-pca9532] Add gpio capability

[Joachim Eastwood]

Hello,

The patch below allows unused leds on pca9532 to be used as gpio. The
board I am working on now has no less than 6 pca9532 chips. One chips
is used for only leds, one has 14 leds and 2 gpio and the rest of the
chips are pure gpio.

There is also one board in mainline which could use this capabilty;
arch/arm/mach-iop32x/n2100.c
 232         {       .type = PCA9532_TYPE_NONE }, /* power OFF gpio */
 233         {       .type = PCA9532_TYPE_NONE }, /* reset gpio */

This patch defines a new pin type, PCA9532_TYPE_GPIO, and registers a
gpiochip if any pin has this type set. The gpio will registers all
chip pins but will filter on gpio_request.
Since it is a bug to call get/set value on unrequested gpio's the
check for PCA9532_TYPE_GPIO could be removed from
pca9532_gpio_get_value/pca9532_gpio_set_value functions. But I have
kept it for this posting.

Tested on custom ARM board.

-------------------------------------------------------------------
    [leds-pca9532] Add gpio capability

    Pins not used for leds can now be used as gpio by setting pin type as
    PCA9532_TYPE_GPIO and defining a gpio_base in platform data.

    Signed-off-by: Joachim Eastwood <joachim.eastwood@jotron.com>

diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c
index afac338..b036fa5 100644
--- a/drivers/leds/leds-pca9532.c
+++ b/drivers/leds/leds-pca9532.c
@@ -19,7 +19,9 @@
 #include <linux/mutex.h>
 #include <linux/workqueue.h>
 #include <linux/leds-pca9532.h>
+#include <linux/gpio.h>

+#define PCA9532_REG_INPUT(i) ((i)/8)
 #define PCA9532_REG_PSC(i) (0x2+(i)*2)
 #define PCA9532_REG_PWM(i) (0x3+(i)*2)
 #define PCA9532_REG_LS0  0x6
@@ -34,6 +36,7 @@ struct pca9532_data {
 	struct mutex update_lock;
 	struct input_dev *idev;
 	struct work_struct work;
+	struct gpio_chip gpio;
 	u8 pwm[2];
 	u8 psc[2];
 };
@@ -200,16 +203,58 @@ static void pca9532_led_work(struct work_struct *work)
 	pca9532_setled(led);
 }

-static void pca9532_destroy_devices(struct pca9532_data *data, int n_devs)
+static int pca9532_gpio_request_pin(struct gpio_chip *gc, unsigned off)
 {
-	int i = n_devs;
+	struct pca9532_data *data = container_of(gc, struct pca9532_data, gpio);
+	struct pca9532_led *led = &data->leds[off];
+
+	if (led->type == PCA9532_TYPE_GPIO)
+		return 0;
+
+	return -EINVAL;
+}
+
+static void pca9532_gpio_set_value(struct gpio_chip *gc, unsigned off, int val)
+{
+	struct pca9532_data *data = container_of(gc, struct pca9532_data, gpio);
+	struct pca9532_led *led = &data->leds[off];
+
+	if (led->type == PCA9532_TYPE_GPIO) {
+		if (val)
+			led->state = PCA9532_ON;
+		else
+			led->state = PCA9532_OFF;
+
+		pca9532_setled(led);
+	}
+}
+
+static int pca9532_gpio_get_value(struct gpio_chip *gc, unsigned off)
+{
+	struct pca9532_data *data = container_of(gc, struct pca9532_data, gpio);
+	struct pca9532_led *led = &data->leds[off];
+	unsigned char reg;
+
+	if (led->type == PCA9532_TYPE_GPIO) {
+		reg = i2c_smbus_read_byte_data(data->client,
+						PCA9532_REG_INPUT(off));
+		return !!(reg & (1<<(off % 8)));
+	}
+
+	return -1;
+}
+
+static int pca9532_destroy_devices(struct pca9532_data *data, int n_devs)
+{
+	int err, i = n_devs;

 	if (!data)
-		return;
+		return -EINVAL;

 	while (--i >= 0) {
 		switch (data->leds[i].type) {
 		case PCA9532_TYPE_NONE:
+		case PCA9532_TYPE_GPIO:
 			break;
 		case PCA9532_TYPE_LED:
 			led_classdev_unregister(&data->leds[i].ldev);
@@ -224,12 +269,24 @@ static void pca9532_destroy_devices(struct
pca9532_data *data, int n_devs)
 			break;
 		}
 	}
+
+	if (data->gpio.dev) {
+		err = gpiochip_remove(&data->gpio);
+		if (err) {
+			dev_err(&data->client->dev, "%s failed, %d\n",
+						"gpiochip_remove()", err);
+			return err;
+		}
+	}
+
+	return 0;
 }

 static int pca9532_configure(struct i2c_client *client,
 	struct pca9532_data *data, struct pca9532_platform_data *pdata)
 {
 	int i, err = 0;
+	int gpios = 0;

 	for (i = 0; i < 2; i++)	{
 		data->pwm[i] = pdata->pwm[i];
@@ -249,6 +306,9 @@ static int pca9532_configure(struct i2c_client *client,
 		switch (led->type) {
 		case PCA9532_TYPE_NONE:
 			break;
+		case PCA9532_TYPE_GPIO:
+			gpios++;
+			break;
 		case PCA9532_TYPE_LED:
 			led->state = pled->state;
 			led->name = pled->name;
@@ -297,6 +357,30 @@ static int pca9532_configure(struct i2c_client *client,
 			break;
 		}
 	}
+
+	if (gpios) {
+		data->gpio.label = "gpio-pca9532";
+		data->gpio.set = pca9532_gpio_set_value;
+		data->gpio.get = pca9532_gpio_get_value;
+		data->gpio.request = pca9532_gpio_request_pin;
+		data->gpio.can_sleep = 1;
+		data->gpio.base = pdata->gpio_base;
+		data->gpio.ngpio = 16;
+		data->gpio.dev = &client->dev;
+		data->gpio.owner = THIS_MODULE;
+
+		err = gpiochip_add(&data->gpio);
+		if (err) {
+			/* Use data->gpio.dev as a flag for freeing gpiochip */
+			data->gpio.dev = NULL;
+			dev_info(&client->dev, "could not add gpiochip\n");
+		} else {
+			dev_info(&client->dev, "gpios %i...%i\n",
+				data->gpio.base, data->gpio.base +
+				data->gpio.ngpio - 1);
+		}
+	}
+
 	return 0;

 exit:
@@ -337,7 +421,12 @@ static int pca9532_probe(struct i2c_client *client,
 static int pca9532_remove(struct i2c_client *client)
 {
 	struct pca9532_data *data = i2c_get_clientdata(client);
-	pca9532_destroy_devices(data, 16);
+	int err;
+
+	err = pca9532_destroy_devices(data, 16);
+	if (err)
+		return err;
+
 	kfree(data);
 	return 0;
 }
diff --git a/include/linux/leds-pca9532.h b/include/linux/leds-pca9532.h
index f158eb1..b8d6fff 100644
--- a/include/linux/leds-pca9532.h
+++ b/include/linux/leds-pca9532.h
@@ -25,7 +25,7 @@ enum pca9532_state {
 };

 enum pca9532_type { PCA9532_TYPE_NONE, PCA9532_TYPE_LED,
-	PCA9532_TYPE_N2100_BEEP };
+	PCA9532_TYPE_N2100_BEEP, PCA9532_TYPE_GPIO };

 struct pca9532_led {
 	u8 id;
@@ -41,6 +41,7 @@ struct pca9532_platform_data {
 	struct pca9532_led leds[16];
 	u8 pwm[2];
 	u8 psc[2];
+	int gpio_base;
 };

 #endif /* __LINUX_PCA9532_H */

Re: [leds-pca9532] Add gpio capability

[Wolfram Sang]

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

Hi,

> Since it is a bug to call get/set value on unrequested gpio's the
> check for PCA9532_TYPE_GPIO could be removed from
> pca9532_gpio_get_value/pca9532_gpio_set_value functions. But I have
> kept it for this posting.

If you keep it, you could print a warning maybe. Or just remove the check.

> -------------------------------------------------------------------
>     [leds-pca9532] Add gpio capability
> 
>     Pins not used for leds can now be used as gpio by setting pin type as
>     PCA9532_TYPE_GPIO and defining a gpio_base in platform data.
> 
>     Signed-off-by: Joachim Eastwood <joachim.eastwood@jotron.com>

Some Kconfig-magic is missing. Iff that feature is used, then it depends on
GENERIC_GPIO.

> 
> diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c
> index afac338..b036fa5 100644
> --- a/drivers/leds/leds-pca9532.c
> +++ b/drivers/leds/leds-pca9532.c
> @@ -19,7 +19,9 @@
>  #include <linux/mutex.h>
>  #include <linux/workqueue.h>
>  #include <linux/leds-pca9532.h>
> +#include <linux/gpio.h>
> 
> +#define PCA9532_REG_INPUT(i) ((i)/8)
>  #define PCA9532_REG_PSC(i) (0x2+(i)*2)
>  #define PCA9532_REG_PWM(i) (0x3+(i)*2)
>  #define PCA9532_REG_LS0  0x6
> @@ -34,6 +36,7 @@ struct pca9532_data {
>  	struct mutex update_lock;
>  	struct input_dev *idev;
>  	struct work_struct work;
> +	struct gpio_chip gpio;
>  	u8 pwm[2];
>  	u8 psc[2];
>  };
> @@ -200,16 +203,58 @@ static void pca9532_led_work(struct work_struct *work)
>  	pca9532_setled(led);
>  }
> 
> -static void pca9532_destroy_devices(struct pca9532_data *data, int n_devs)
> +static int pca9532_gpio_request_pin(struct gpio_chip *gc, unsigned off)

Nitpick: maybe 'offset' as variable name? I first thought 'off' was a
description of the state.

>  {
> -	int i = n_devs;
> +	struct pca9532_data *data = container_of(gc, struct pca9532_data, gpio);
> +	struct pca9532_led *led = &data->leds[off];
> +
> +	if (led->type == PCA9532_TYPE_GPIO)
> +		return 0;
> +
> +	return -EINVAL;

-EBUSY? The value is valid, just the pin is used already.

> +}
> +
> +static void pca9532_gpio_set_value(struct gpio_chip *gc, unsigned off, int val)
> +{
> +	struct pca9532_data *data = container_of(gc, struct pca9532_data, gpio);
> +	struct pca9532_led *led = &data->leds[off];
> +
> +	if (led->type == PCA9532_TYPE_GPIO) {
> +		if (val)
> +			led->state = PCA9532_ON;
> +		else
> +			led->state = PCA9532_OFF;
> +
> +		pca9532_setled(led);
> +	}
> +}
> +
> +static int pca9532_gpio_get_value(struct gpio_chip *gc, unsigned off)
> +{
> +	struct pca9532_data *data = container_of(gc, struct pca9532_data, gpio);
> +	struct pca9532_led *led = &data->leds[off];
> +	unsigned char reg;
> +
> +	if (led->type == PCA9532_TYPE_GPIO) {
> +		reg = i2c_smbus_read_byte_data(data->client,
> +						PCA9532_REG_INPUT(off));
> +		return !!(reg & (1<<(off % 8)));

Spaces around operators.

> +	}
> +
> +	return -1;

According to gpio.txt, you should return 0 here (there are no errorcodes).

> +}
> +
> +static int pca9532_destroy_devices(struct pca9532_data *data, int n_devs)
> +{
> +	int err, i = n_devs;
> 
>  	if (!data)
> -		return;
> +		return -EINVAL;
> 
>  	while (--i >= 0) {
>  		switch (data->leds[i].type) {
>  		case PCA9532_TYPE_NONE:
> +		case PCA9532_TYPE_GPIO:
>  			break;
>  		case PCA9532_TYPE_LED:
>  			led_classdev_unregister(&data->leds[i].ldev);
> @@ -224,12 +269,24 @@ static void pca9532_destroy_devices(struct
> pca9532_data *data, int n_devs)
>  			break;
>  		}
>  	}
> +
> +	if (data->gpio.dev) {
> +		err = gpiochip_remove(&data->gpio);
> +		if (err) {
> +			dev_err(&data->client->dev, "%s failed, %d\n",
> +						"gpiochip_remove()", err);
> +			return err;
> +		}
> +	}
> +
> +	return 0;
>  }
> 
>  static int pca9532_configure(struct i2c_client *client,
>  	struct pca9532_data *data, struct pca9532_platform_data *pdata)
>  {
>  	int i, err = 0;
> +	int gpios = 0;
> 
>  	for (i = 0; i < 2; i++)	{
>  		data->pwm[i] = pdata->pwm[i];
> @@ -249,6 +306,9 @@ static int pca9532_configure(struct i2c_client *client,
>  		switch (led->type) {
>  		case PCA9532_TYPE_NONE:
>  			break;
> +		case PCA9532_TYPE_GPIO:
> +			gpios++;
> +			break;
>  		case PCA9532_TYPE_LED:
>  			led->state = pled->state;
>  			led->name = pled->name;
> @@ -297,6 +357,30 @@ static int pca9532_configure(struct i2c_client *client,
>  			break;
>  		}
>  	}
> +
> +	if (gpios) {
> +		data->gpio.label = "gpio-pca9532";
> +		data->gpio.set = pca9532_gpio_set_value;
> +		data->gpio.get = pca9532_gpio_get_value;
> +		data->gpio.request = pca9532_gpio_request_pin;
> +		data->gpio.can_sleep = 1;
> +		data->gpio.base = pdata->gpio_base;
> +		data->gpio.ngpio = 16;
> +		data->gpio.dev = &client->dev;
> +		data->gpio.owner = THIS_MODULE;

I'd assume that you also need to define a direction_output-function which always
returns success?

> +
> +		err = gpiochip_add(&data->gpio);
> +		if (err) {
> +			/* Use data->gpio.dev as a flag for freeing gpiochip */
> +			data->gpio.dev = NULL;
> +			dev_info(&client->dev, "could not add gpiochip\n");

dev_warning?

> +		} else {
> +			dev_info(&client->dev, "gpios %i...%i\n",
> +				data->gpio.base, data->gpio.base +
> +				data->gpio.ngpio - 1);
> +		}
> +	}
> +
>  	return 0;
> 
>  exit:
> @@ -337,7 +421,12 @@ static int pca9532_probe(struct i2c_client *client,
>  static int pca9532_remove(struct i2c_client *client)
>  {
>  	struct pca9532_data *data = i2c_get_clientdata(client);
> -	pca9532_destroy_devices(data, 16);
> +	int err;
> +
> +	err = pca9532_destroy_devices(data, 16);
> +	if (err)
> +		return err;
> +
>  	kfree(data);
>  	return 0;
>  }
> diff --git a/include/linux/leds-pca9532.h b/include/linux/leds-pca9532.h
> index f158eb1..b8d6fff 100644
> --- a/include/linux/leds-pca9532.h
> +++ b/include/linux/leds-pca9532.h
> @@ -25,7 +25,7 @@ enum pca9532_state {
>  };
> 
>  enum pca9532_type { PCA9532_TYPE_NONE, PCA9532_TYPE_LED,
> -	PCA9532_TYPE_N2100_BEEP };
> +	PCA9532_TYPE_N2100_BEEP, PCA9532_TYPE_GPIO };
> 
>  struct pca9532_led {
>  	u8 id;
> @@ -41,6 +41,7 @@ struct pca9532_platform_data {
>  	struct pca9532_led leds[16];
>  	u8 pwm[2];
>  	u8 psc[2];
> +	int gpio_base;
>  };
> 
>  #endif /* __LINUX_PCA9532_H */
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

Re: [leds-pca9532] Add gpio capability

[Joachim Eastwood]

Hello,

On 1/30/11, Wolfram Sang <w.sang@pengutronix.de> wrote:
> Hi,
>
>> Since it is a bug to call get/set value on unrequested gpio's the
>> check for PCA9532_TYPE_GPIO could be removed from
>> pca9532_gpio_get_value/pca9532_gpio_set_value functions. But I have
>> kept it for this posting.
>
> If you keep it, you could print a warning maybe. Or just remove the check.

I'll think I will remove the check.

>
>> -------------------------------------------------------------------
>>     [leds-pca9532] Add gpio capability
>>
>>     Pins not used for leds can now be used as gpio by setting pin type as
>>     PCA9532_TYPE_GPIO and defining a gpio_base in platform data.
>>
>>     Signed-off-by: Joachim Eastwood <joachim.eastwood@jotron.com>
>
> Some Kconfig-magic is missing. Iff that feature is used, then it depends on
> GENERIC_GPIO.

I though about it just when I had hit send. I'll create an updated
version where gpio capability is optional.

GENERIC_GPIO? Shouldn't it be GPIOLIB in this case?

>
>>
>> diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c
>> index afac338..b036fa5 100644
>> --- a/drivers/leds/leds-pca9532.c
>> +++ b/drivers/leds/leds-pca9532.c
>> @@ -19,7 +19,9 @@
>>  #include <linux/mutex.h>
>>  #include <linux/workqueue.h>
>>  #include <linux/leds-pca9532.h>
>> +#include <linux/gpio.h>
>>
>> +#define PCA9532_REG_INPUT(i) ((i)/8)
>>  #define PCA9532_REG_PSC(i) (0x2+(i)*2)
>>  #define PCA9532_REG_PWM(i) (0x3+(i)*2)
>>  #define PCA9532_REG_LS0  0x6
>> @@ -34,6 +36,7 @@ struct pca9532_data {
>>  	struct mutex update_lock;
>>  	struct input_dev *idev;
>>  	struct work_struct work;
>> +	struct gpio_chip gpio;
>>  	u8 pwm[2];
>>  	u8 psc[2];
>>  };
>> @@ -200,16 +203,58 @@ static void pca9532_led_work(struct work_struct
>> *work)
>>  	pca9532_setled(led);
>>  }
>>
>> -static void pca9532_destroy_devices(struct pca9532_data *data, int
>> n_devs)
>> +static int pca9532_gpio_request_pin(struct gpio_chip *gc, unsigned off)
>
> Nitpick: maybe 'offset' as variable name? I first thought 'off' was a
> description of the state.

Yes, I guess 'offset' would be better. This seems to used in other
gpio drivers as well.

>>  {
>> -	int i = n_devs;
>> +	struct pca9532_data *data = container_of(gc, struct pca9532_data, gpio);
>> +	struct pca9532_led *led = &data->leds[off];
>> +
>> +	if (led->type == PCA9532_TYPE_GPIO)
>> +		return 0;
>> +
>> +	return -EINVAL;
>
> -EBUSY? The value is valid, just the pin is used already.

Agreed.

>> +}
>> +
>> +static void pca9532_gpio_set_value(struct gpio_chip *gc, unsigned off,
>> int val)
>> +{
>> +	struct pca9532_data *data = container_of(gc, struct pca9532_data, gpio);
>> +	struct pca9532_led *led = &data->leds[off];
>> +
>> +	if (led->type == PCA9532_TYPE_GPIO) {
>> +		if (val)
>> +			led->state = PCA9532_ON;
>> +		else
>> +			led->state = PCA9532_OFF;
>> +
>> +		pca9532_setled(led);
>> +	}
>> +}
>> +
>> +static int pca9532_gpio_get_value(struct gpio_chip *gc, unsigned off)
>> +{
>> +	struct pca9532_data *data = container_of(gc, struct pca9532_data, gpio);
>> +	struct pca9532_led *led = &data->leds[off];
>> +	unsigned char reg;
>> +
>> +	if (led->type == PCA9532_TYPE_GPIO) {
>> +		reg = i2c_smbus_read_byte_data(data->client,
>> +						PCA9532_REG_INPUT(off));
>> +		return !!(reg & (1<<(off % 8)));
>
> Spaces around operators.

Will fix.

>> +	}
>> +
>> +	return -1;
>
> According to gpio.txt, you should return 0 here (there are no errorcodes).

I will remove the check for PCA9532_TYPE_GPIO so this will go away.

>> +}
>> +
>> +static int pca9532_destroy_devices(struct pca9532_data *data, int n_devs)
>> +{
>> +	int err, i = n_devs;
>>
>>  	if (!data)
>> -		return;
>> +		return -EINVAL;
>>
>>  	while (--i >= 0) {
>>  		switch (data->leds[i].type) {
>>  		case PCA9532_TYPE_NONE:
>> +		case PCA9532_TYPE_GPIO:
>>  			break;
>>  		case PCA9532_TYPE_LED:
>>  			led_classdev_unregister(&data->leds[i].ldev);
>> @@ -224,12 +269,24 @@ static void pca9532_destroy_devices(struct
>> pca9532_data *data, int n_devs)
>>  			break;
>>  		}
>>  	}
>> +
>> +	if (data->gpio.dev) {
>> +		err = gpiochip_remove(&data->gpio);
>> +		if (err) {
>> +			dev_err(&data->client->dev, "%s failed, %d\n",
>> +						"gpiochip_remove()", err);
>> +			return err;
>> +		}
>> +	}
>> +
>> +	return 0;
>>  }
>>
>>  static int pca9532_configure(struct i2c_client *client,
>>  	struct pca9532_data *data, struct pca9532_platform_data *pdata)
>>  {
>>  	int i, err = 0;
>> +	int gpios = 0;
>>
>>  	for (i = 0; i < 2; i++)	{
>>  		data->pwm[i] = pdata->pwm[i];
>> @@ -249,6 +306,9 @@ static int pca9532_configure(struct i2c_client
>> *client,
>>  		switch (led->type) {
>>  		case PCA9532_TYPE_NONE:
>>  			break;
>> +		case PCA9532_TYPE_GPIO:
>> +			gpios++;
>> +			break;
>>  		case PCA9532_TYPE_LED:
>>  			led->state = pled->state;
>>  			led->name = pled->name;
>> @@ -297,6 +357,30 @@ static int pca9532_configure(struct i2c_client
>> *client,
>>  			break;
>>  		}
>>  	}
>> +
>> +	if (gpios) {
>> +		data->gpio.label = "gpio-pca9532";
>> +		data->gpio.set = pca9532_gpio_set_value;
>> +		data->gpio.get = pca9532_gpio_get_value;
>> +		data->gpio.request = pca9532_gpio_request_pin;
>> +		data->gpio.can_sleep = 1;
>> +		data->gpio.base = pdata->gpio_base;
>> +		data->gpio.ngpio = 16;
>> +		data->gpio.dev = &client->dev;
>> +		data->gpio.owner = THIS_MODULE;
>
> I'd assume that you also need to define a direction_output-function which
> always
> returns success?

Yes, you are right. It is not possible to set direction in hw, but I
should provide them.

>> +
>> +		err = gpiochip_add(&data->gpio);
>> +		if (err) {
>> +			/* Use data->gpio.dev as a flag for freeing gpiochip */
>> +			data->gpio.dev = NULL;
>> +			dev_info(&client->dev, "could not add gpiochip\n");
>
> dev_warning?

Yes, that is better.

>> +		} else {
>> +			dev_info(&client->dev, "gpios %i...%i\n",
>> +				data->gpio.base, data->gpio.base +
>> +				data->gpio.ngpio - 1);
>> +		}
>> +	}
>> +
>>  	return 0;
>>
>>  exit:
>> @@ -337,7 +421,12 @@ static int pca9532_probe(struct i2c_client *client,
>>  static int pca9532_remove(struct i2c_client *client)
>>  {
>>  	struct pca9532_data *data = i2c_get_clientdata(client);
>> -	pca9532_destroy_devices(data, 16);
>> +	int err;
>> +
>> +	err = pca9532_destroy_devices(data, 16);
>> +	if (err)
>> +		return err;
>> +
>>  	kfree(data);
>>  	return 0;
>>  }
>> diff --git a/include/linux/leds-pca9532.h b/include/linux/leds-pca9532.h
>> index f158eb1..b8d6fff 100644
>> --- a/include/linux/leds-pca9532.h
>> +++ b/include/linux/leds-pca9532.h
>> @@ -25,7 +25,7 @@ enum pca9532_state {
>>  };
>>
>>  enum pca9532_type { PCA9532_TYPE_NONE, PCA9532_TYPE_LED,
>> -	PCA9532_TYPE_N2100_BEEP };
>> +	PCA9532_TYPE_N2100_BEEP, PCA9532_TYPE_GPIO };
>>
>>  struct pca9532_led {
>>  	u8 id;
>> @@ -41,6 +41,7 @@ struct pca9532_platform_data {
>>  	struct pca9532_led leds[16];
>>  	u8 pwm[2];
>>  	u8 psc[2];
>> +	int gpio_base;
>>  };
>>
>>  #endif /* __LINUX_PCA9532_H */
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/

Thanks for the review. I'll send out an updated patch later tonight.

regards
Joachim Eastwood

> Regards,
>
>    Wolfram
>
> --
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
>

Re: [leds-pca9532] Add gpio capability

[Wolfram Sang]

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


> > Some Kconfig-magic is missing. Iff that feature is used, then it depends on
> > GENERIC_GPIO.
> 
> I though about it just when I had hit send. I'll create an updated
> version where gpio capability is optional.
> 
> GENERIC_GPIO? Shouldn't it be GPIOLIB in this case?

Yes, you are right. You are adding a gpiochip, so you need GPIOLIB.

> >> +	if (gpios) {
> >> +		data->gpio.label = "gpio-pca9532";
> >> +		data->gpio.set = pca9532_gpio_set_value;
> >> +		data->gpio.get = pca9532_gpio_get_value;
> >> +		data->gpio.request = pca9532_gpio_request_pin;
> >> +		data->gpio.can_sleep = 1;
> >> +		data->gpio.base = pdata->gpio_base;
> >> +		data->gpio.ngpio = 16;
> >> +		data->gpio.dev = &client->dev;
> >> +		data->gpio.owner = THIS_MODULE;
> >
> > I'd assume that you also need to define a direction_output-function which
> > always
> > returns success?
> 
> Yes, you are right. It is not possible to set direction in hw, but I
> should provide them.

It should be okay to skip direction_input, because gpiolib will return an error
if it is not available. This is exactly why IMO you need a direction_output
returning 0. Otherwise drivers checking if that operation was successful, might
be fooled.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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