| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sun, 30 Jan 2011 20:12:10 +0100
From: Joachim Eastwood <manabian@...il.com>
To: Wolfram Sang <w.sang@...gutronix.de>
Cc: rpurdie@...ys.net, riku.voipio@....fi,
linux-kernel@...r.kernel.org, joachim.eastwood@...ron.com
Subject: Re: [leds-pca9532] Add gpio capability
Hello,
On 1/30/11, Wolfram Sang <w.sang@...gutronix.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@...ron.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@...r.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/ |
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists