From mboxrd@z Thu Jan 1 00:00:00 1970 From: "eric miao" Subject: Re: [i2c] [RFC PATCH 3/8] Add PCA9536 4 bit I2C GPIO extender support to the pca9539 GPIO driver Date: Thu, 31 Jan 2008 09:31:02 +0800 Message-ID: References: <20080127224559.4eed7cea@hyperion.delvare> <200801272218.04137.david-b@pacbell.net> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Content-Disposition: inline List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: video4linux-list-bounces@redhat.com Errors-To: video4linux-list-bounces@redhat.com To: Guennadi Liakhovetski Cc: David Brownell , Jean Delvare , video4linux-list@redhat.com, i2c@lm-sensors.org List-Id: linux-i2c@vger.kernel.org I like this patch, overall looks ok. See my comments below. On Jan 31, 2008 12:48 AM, Guennadi Liakhovetski wrote: > PCA9539 and PCA9536 have identical register layout, and differ only in the > number of GPIOs and, respectively, register size. Extend the pca9539 > driver to also support PCA9536. The driver can further be extended in the > future to also support PCA953[4578] chips. > > Signed-off-by: Guennadi Liakhovetski > > --- > > On Sun, 27 Jan 2008, David Brownell wrote: > > > On Sunday 27 January 2008, Jean Delvare wrote: > > > > > > There is a new GPIO subsystem (under driver/gpio), that's where you > > > should add this driver. > > > > That should merge soonish into kernel GIT -- Linus is merging up a > > storm before LCA starts, but Andrew's not sent it yet -- but it's > > easy to grab from current MM. > > Hope it does, as now my video patches depend on those. > > > There's a pca9539 driver there, and ISTR the 9536 is very similar > > except it has no IRQ support and only has 8-bit registers. You > > should be able to reuse its platform_data structure. It has a > > registration callback that should let you establish the linkage to > > the camera(s) you want to control. > > I didn't need anything like that. I just used the platform knowledge of > the GPIO number. This way the camera driver just obtains the GPIO, > requests and uses it. > > > I suspect you'll come across one other issue... one potential fix > > being to add a mechanism matching a NOTE in the gpiolib sources. > > You mean gpio numbers? See above - platform code knows, that it's GPIO 1 > on the extender, that the cameras need. > > > I think you should consider making one driver that can handle three > > chips: the 4-bit pca9536 (8 pins) and pca9537 (10 pins -- add irq and > > reset), plus their 8-bit pca9538 sibling (16 pins -- four more GPIOs, > > and two address bits). Their register model is *identical* ... all > > that would mean is understanding that for some chips the high nibble > > is actually valid. (And eventually, maybe, that some chips support > > input-changed IRQs that can help avoid synchronous register reads. > > If the i2c_client doesn't list an IRQ that's of course not an issue.) > > I think, there are more than 3. But I'd prefer staying with the two at the > moment - others can be added later. And I haven't found in the driver > support for IRQ and reset pins, so, no differentiation is made for those > either. > > Note, the patch below uses git-specific "renamed from/to" tags, so, it > won't work with GNU patch. But it is easier to review it this way. > > Thanks > Guennadi > > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > index 74fac0f..713c0fe 100644 > --- a/drivers/gpio/Kconfig > +++ b/drivers/gpio/Kconfig > @@ -27,15 +27,15 @@ config DEBUG_GPIO > > comment "I2C GPIO expanders:" > > -config GPIO_PCA9539 > - tristate "PCA9539 16-bit I/O port" > +config GPIO_PCA953X > + tristate "PCA953X I/O port" > depends on I2C > help > - Say yes here to support the PCA9539 16-bit I/O port. These > - parts are made by NXP and TI. > + Say yes here to support PCA9539 16-bit and PCA9536 4-bit I/O ports. > + These parts are made by NXP and TI. > > This driver can also be built as a module. If so, the module > - will be called pca9539. > + will be called pca953x. > > config GPIO_PCF857X > tristate "PCF857x, PCA857x, and PCA967x I2C GPIO expanders" > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile > index 470ecd6..fdde992 100644 > --- a/drivers/gpio/Makefile > +++ b/drivers/gpio/Makefile > @@ -5,5 +5,5 @@ ccflags-$(CONFIG_DEBUG_GPIO) += -DDEBUG > obj-$(CONFIG_HAVE_GPIO_LIB) += gpiolib.o > > obj-$(CONFIG_GPIO_MCP23S08) += mcp23s08.o > -obj-$(CONFIG_GPIO_PCA9539) += pca9539.o > +obj-$(CONFIG_GPIO_PCA953X) += pca953x.o > obj-$(CONFIG_GPIO_PCF857X) += pcf857x.o > diff --git a/drivers/gpio/pca9539.c b/drivers/gpio/pca953x.c > similarity index 67% > rename from drivers/gpio/pca9539.c > rename to drivers/gpio/pca953x.c > index 3e85c92..3e9d650 100644 > @@ -1,5 +1,5 @@ > /* > - * pca9539.c - 16-bit I/O port with interrupt and reset > + * pca953x.c - I/O port with interrupt and reset > * > * Copyright (C) 2005 Ben Gardner > * Copyright (C) 2007 Marvell International Ltd. > @@ -14,19 +14,42 @@ > #include > #include > #include > -#include > +#include > > #include > > +enum { > + PCA9536, > + PCA9539, > +}; > > -#define NR_PCA9539_GPIOS 16 > +struct pca953x_desc { > + unsigned int gpios; > + unsigned int input; > + unsigned int output; > + unsigned int invert; > + unsigned int direction; > +}; I guess the register address can be inferred either from the number of GPIOs or a single member field of "shift" may be sufficient. > > -#define PCA9539_INPUT 0 > -#define PCA9539_OUTPUT 2 > -#define PCA9539_INVERT 4 > -#define PCA9539_DIRECTION 6 > +static const struct pca953x_desc pca953x_descs[] = { > + [PCA9536] = { > + .gpios = 4, > + .input = 0, > + .output = 1, > + .invert = 2, > + .direction = 3, > + }, > + [PCA9539] = { > + .gpios = 16, > + .input = 0, > + .output = 2, > + .invert = 4, > + .direction = 6, > + }, > +}; > > -struct pca9539_chip { > +struct pca953x_chip { > + struct pca953x_desc const *desc; > unsigned gpio_start; > uint16_t reg_output; > uint16_t reg_direction; > @@ -38,19 +61,32 @@ struct pca9539_chip { > /* NOTE: we can't currently rely on fault codes to come from SMBus > * calls, so we map all errors to EIO here and return zero otherwise. > */ > -static int pca9539_write_reg(struct pca9539_chip *chip, int reg, uint16_t val) > +static int pca953x_write_reg(struct pca953x_chip *chip, int reg, uint16_t val) > { > - if (i2c_smbus_write_word_data(chip->client, reg, val) < 0) > - return -EIO; > + int ret; > + > + if (chip->desc->gpios <= 8) > + ret = i2c_smbus_write_byte_data(chip->client, reg, val); > else > - return 0; > + ret = i2c_smbus_write_word_data(chip->client, reg, val); > + > + if (ret < 0) { > + dev_err(&chip->client->dev, "failed writing register\n"); > + return -EIO; > + } > + > + return 0; > } > > -static int pca9539_read_reg(struct pca9539_chip *chip, int reg, uint16_t *val) > +static int pca953x_read_reg(struct pca953x_chip *chip, int reg, uint16_t *val) > { > int ret; > > - ret = i2c_smbus_read_word_data(chip->client, reg); > + if (chip->desc->gpios <= 8) > + ret = i2c_smbus_read_byte_data(chip->client, reg); > + else > + ret = i2c_smbus_read_word_data(chip->client, reg); > + > if (ret < 0) { > dev_err(&chip->client->dev, "failed reading register\n"); > return -EIO; > @@ -60,16 +96,16 @@ static int pca9539_read_reg(struct pca9539_chip *chip, int reg, uint16_t *val) > return 0; > } > > -static int pca9539_gpio_direction_input(struct gpio_chip *gc, unsigned off) > +static int pca953x_gpio_direction_input(struct gpio_chip *gc, unsigned off) > { > - struct pca9539_chip *chip; > + struct pca953x_chip *chip; > uint16_t reg_val; > int ret; > > - chip = container_of(gc, struct pca9539_chip, gpio_chip); > + chip = container_of(gc, struct pca953x_chip, gpio_chip); > > reg_val = chip->reg_direction | (1u << off); > - ret = pca9539_write_reg(chip, PCA9539_DIRECTION, reg_val); > + ret = pca953x_write_reg(chip, chip->desc->direction, reg_val); > if (ret) > return ret; > > @@ -77,14 +113,14 @@ static int pca9539_gpio_direction_input(struct gpio_chip *gc, unsigned off) > return 0; > } > > -static int pca9539_gpio_direction_output(struct gpio_chip *gc, > +static int pca953x_gpio_direction_output(struct gpio_chip *gc, > unsigned off, int val) > { > - struct pca9539_chip *chip; > + struct pca953x_chip *chip; > uint16_t reg_val; > int ret; > > - chip = container_of(gc, struct pca9539_chip, gpio_chip); > + chip = container_of(gc, struct pca953x_chip, gpio_chip); > > /* set output level */ > if (val) > @@ -92,7 +128,7 @@ static int pca9539_gpio_direction_output(struct gpio_chip *gc, > else > reg_val = chip->reg_output & ~(1u << off); > > - ret = pca9539_write_reg(chip, PCA9539_OUTPUT, reg_val); > + ret = pca953x_write_reg(chip, chip->desc->output, reg_val); > if (ret) > return ret; > > @@ -100,7 +136,7 @@ static int pca9539_gpio_direction_output(struct gpio_chip *gc, > > /* then direction */ > reg_val = chip->reg_direction & ~(1u << off); > - ret = pca9539_write_reg(chip, PCA9539_DIRECTION, reg_val); > + ret = pca953x_write_reg(chip, chip->desc->direction, reg_val); > if (ret) > return ret; > > @@ -108,15 +144,15 @@ static int pca9539_gpio_direction_output(struct gpio_chip *gc, > return 0; > } > > -static int pca9539_gpio_get_value(struct gpio_chip *gc, unsigned off) > +static int pca953x_gpio_get_value(struct gpio_chip *gc, unsigned off) > { > - struct pca9539_chip *chip; > + struct pca953x_chip *chip; > uint16_t reg_val; > int ret; > > - chip = container_of(gc, struct pca9539_chip, gpio_chip); > + chip = container_of(gc, struct pca953x_chip, gpio_chip); > > - ret = pca9539_read_reg(chip, PCA9539_INPUT, ®_val); > + ret = pca953x_read_reg(chip, chip->desc->input, ®_val); > if (ret < 0) { > /* NOTE: diagnostic already emitted; that's all we should > * do unless gpio_*_value_cansleep() calls become different > @@ -128,58 +164,67 @@ static int pca9539_gpio_get_value(struct gpio_chip *gc, unsigned off) > return (reg_val & (1u << off)) ? 1 : 0; > } > > -static void pca9539_gpio_set_value(struct gpio_chip *gc, unsigned off, int val) > +static void pca953x_gpio_set_value(struct gpio_chip *gc, unsigned off, int val) > { > - struct pca9539_chip *chip; > + struct pca953x_chip *chip; > uint16_t reg_val; > int ret; > > - chip = container_of(gc, struct pca9539_chip, gpio_chip); > + chip = container_of(gc, struct pca953x_chip, gpio_chip); > > if (val) > reg_val = chip->reg_output | (1u << off); > else > reg_val = chip->reg_output & ~(1u << off); > > - ret = pca9539_write_reg(chip, PCA9539_OUTPUT, reg_val); > + ret = pca953x_write_reg(chip, chip->desc->output, reg_val); > if (ret) > return; > > chip->reg_output = reg_val; > } > > -static int pca9539_init_gpio(struct pca9539_chip *chip) > +static int pca953x_init_gpio(struct pca953x_chip *chip) > { > struct gpio_chip *gc; > > gc = &chip->gpio_chip; > > - gc->direction_input = pca9539_gpio_direction_input; > - gc->direction_output = pca9539_gpio_direction_output; > - gc->get = pca9539_gpio_get_value; > - gc->set = pca9539_gpio_set_value; > + gc->direction_input = pca953x_gpio_direction_input; > + gc->direction_output = pca953x_gpio_direction_output; > + gc->get = pca953x_gpio_get_value; > + gc->set = pca953x_gpio_set_value; > > gc->base = chip->gpio_start; > - gc->ngpio = NR_PCA9539_GPIOS; > - gc->label = "pca9539"; > + gc->ngpio = chip->desc->gpios; > + gc->label = "pca953x"; > > return gpiochip_add(gc); > } > > -static int __devinit pca9539_probe(struct i2c_client *client) > +static int __devinit pca953x_probe(struct i2c_client *client) > { > - struct pca9539_platform_data *pdata; > - struct pca9539_chip *chip; > + struct pca953x_platform_data *pdata; > + struct pca953x_chip *chip; > int ret; > + int variant; > > pdata = client->dev.platform_data; > if (pdata == NULL) > return -ENODEV; > > - chip = kzalloc(sizeof(struct pca9539_chip), GFP_KERNEL); > + if (!strcmp("pca9539", pdata->name)) > + variant = PCA9539; > + else if (!strcmp("pca9536", pdata->name)) > + variant = PCA9536; > + else > + return -ENODEV; > + I prefer integer here more than string, for simplicity and efficiency. What about: switch (pdata->model) { case 9536: chip->num_gpio = 8; case 9539: chip->num_gpio = 16; default: warning - not supported. } And maybe the chip->num_gpio can serve as the "shift" to calculate the register offset, thus avoiding the introducing of "pca953x_desc" structure at all. > + chip = kzalloc(sizeof(struct pca953x_chip), GFP_KERNEL); > if (chip == NULL) > return -ENOMEM; > > + chip->desc = &pca953x_descs[variant]; > chip->client = client; > > chip->gpio_start = pdata->gpio_base; > @@ -187,20 +232,20 @@ static int __devinit pca9539_probe(struct i2c_client *client) > /* initialize cached registers from their original values. > * we can't share this chip with another i2c master. > */ > - ret = pca9539_read_reg(chip, PCA9539_OUTPUT, &chip->reg_output); > + ret = pca953x_read_reg(chip, chip->desc->output, &chip->reg_output); > if (ret) > goto out_failed; > > - ret = pca9539_read_reg(chip, PCA9539_DIRECTION, &chip->reg_direction); > + ret = pca953x_read_reg(chip, chip->desc->direction, &chip->reg_direction); > if (ret) > goto out_failed; > > /* set platform specific polarity inversion */ > - ret = pca9539_write_reg(chip, PCA9539_INVERT, pdata->invert); > + ret = pca953x_write_reg(chip, chip->desc->invert, pdata->invert); > if (ret) > goto out_failed; > > - ret = pca9539_init_gpio(chip); > + ret = pca953x_init_gpio(chip); > if (ret) > goto out_failed; > > @@ -219,10 +264,10 @@ out_failed: > return ret; > } > > -static int pca9539_remove(struct i2c_client *client) > +static int pca953x_remove(struct i2c_client *client) > { > - struct pca9539_platform_data *pdata = client->dev.platform_data; > - struct pca9539_chip *chip = i2c_get_clientdata(client); > + struct pca953x_platform_data *pdata = client->dev.platform_data; > + struct pca953x_chip *chip = i2c_get_clientdata(client); > int ret = 0; > > if (pdata->teardown) { > @@ -246,26 +291,26 @@ static int pca9539_remove(struct i2c_client *client) > return 0; > } > > -static struct i2c_driver pca9539_driver = { > +static struct i2c_driver pca953x_driver = { > .driver = { > - .name = "pca9539", > + .name = "pca953x", > }, > - .probe = pca9539_probe, > - .remove = pca9539_remove, > + .probe = pca953x_probe, > + .remove = pca953x_remove, > }; > > -static int __init pca9539_init(void) > +static int __init pca953x_init(void) > { > - return i2c_add_driver(&pca9539_driver); > + return i2c_add_driver(&pca953x_driver); > } > -module_init(pca9539_init); > +module_init(pca953x_init); > > -static void __exit pca9539_exit(void) > +static void __exit pca953x_exit(void) > { > - i2c_del_driver(&pca9539_driver); > + i2c_del_driver(&pca953x_driver); > } > -module_exit(pca9539_exit); > +module_exit(pca953x_exit); > > MODULE_AUTHOR("eric miao "); > -MODULE_DESCRIPTION("GPIO expander driver for PCA9539"); > +MODULE_DESCRIPTION("GPIO expander driver for PCA953X"); > MODULE_LICENSE("GPL"); > diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h > index f29a502..806b86c 100644 > --- a/include/asm-generic/gpio.h > +++ b/include/asm-generic/gpio.h > @@ -16,6 +16,10 @@ > #define ARCH_NR_GPIOS 256 > #endif > > +#ifndef NO_GPIO > +#define NO_GPIO ((unsigned int)-1) > +#endif > + I don't understand this. > struct seq_file; > > /** > diff --git a/include/linux/i2c/pca9539.h b/include/linux/i2c/pca953x.h > similarity index 67% > rename from include/linux/i2c/pca9539.h > rename to include/linux/i2c/pca953x.h > index 611d84a..8977e57 100644 > --- a/include/linux/i2c/pca9539.h > +++ b/include/linux/i2c/pca953x.h > @@ -1,9 +1,12 @@ > -/* platform data for the PCA9539 16-bit I/O expander driver */ > +/* platform data for the PCA953x I/O expander driver */ > > -struct pca9539_platform_data { > +struct pca953x_platform_data { > /* number of the first GPIO */ > unsigned gpio_base; > > + /* chip variant. Currently supported "pca9536" and "pca9539" */ > + const char *name; > + > /* initial polarity inversion setting */ > uint16_t invert; > > -- > video4linux-list mailing list > Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe > https://www.redhat.com/mailman/listinfo/video4linux-list > -- Cheers - eric -- video4linux-list mailing list Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe https://www.redhat.com/mailman/listinfo/video4linux-list