Patchwork [U-Boot] gpio: add driver for PCA9539 16-bit I2C gpio expander

login
register
mail settings
Submitter Chris Packham
Date Dec. 6, 2010, 9:48 p.m.
Message ID <AANLkTikUpzYoU-WQ7BXi8OxXnR-rDD+RL2BgM16qU9oc@mail.gmail.com>
Download mbox | patch
Permalink /patch/74467/
State Superseded
Headers show

Comments

Chris Packham - Dec. 6, 2010, 9:48 p.m.
This adds support for the PCA9539 family of gpio devices which have 16
output pins. The devices are similar to chips that use the pca953x driver
except the register map is expanded (in a non-backwards compatible manner)
to allow for the extra 8 pins.

This driver has one gotcha that you can only set the top or bottom 8 bits
at a time. For example to set the 16 pins to be outputs you need to make
the function calls

  pca9539_set_dir(PCA9539_ADDR, 0xff00, 0x0000);
  pca9539_set_dir(PCA9539_ADDR, 0x00ff, 0x0000);

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
Hi,

I have a couple of questions related to my patch. Firstly is anyone
interested in
support for these gpio devices? Secondly is my gotcha acceptable or should I
put some effort into making it handle a 16-bit argument in 1 hit?

I toyed with the idea of making this part of the pca953x driver. I
didn't because
the register layout is different so I'd still need to define PCA9539 specific
functions and macros (although pca953x_reg_write could be re-used). I also
figured that some people may want to define only CONFIG_PCA9539 (that is
the case for me at the moment but the overhead of CONFIG_PCA953X wouldn't
push the size of my image over any boundary).

 drivers/gpio/Makefile  |    1 +
 drivers/gpio/pca9539.c |   92 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/pca9539.h      |   36 +++++++++++++++++++
 3 files changed, 129 insertions(+), 0 deletions(-)
 create mode 100644 drivers/gpio/pca9539.c
 create mode 100644 include/pca9539.h
Peter Tyser - Dec. 7, 2010, 12:13 a.m.
Hi Chris,

On Tue, 2010-12-07 at 10:48 +1300, Chris Packham wrote:
> This adds support for the PCA9539 family of gpio devices which have 16
> output pins. The devices are similar to chips that use the pca953x driver
> except the register map is expanded (in a non-backwards compatible manner)
> to allow for the extra 8 pins.
> 
> This driver has one gotcha that you can only set the top or bottom 8 bits
> at a time. For example to set the 16 pins to be outputs you need to make
> the function calls
> 
>   pca9539_set_dir(PCA9539_ADDR, 0xff00, 0x0000);
>   pca9539_set_dir(PCA9539_ADDR, 0x00ff, 0x0000);
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> Hi,
> 
> I have a couple of questions related to my patch. Firstly is anyone
> interested in
> support for these gpio devices? Secondly is my gotcha acceptable or should I
> put some effort into making it handle a 16-bit argument in 1 hit?
> 
> I toyed with the idea of making this part of the pca953x driver. I
> didn't because
> the register layout is different so I'd still need to define PCA9539 specific
> functions and macros (although pca953x_reg_write could be re-used). I also
> figured that some people may want to define only CONFIG_PCA9539 (that is
> the case for me at the moment but the overhead of CONFIG_PCA953X wouldn't
> push the size of my image over any boundary).

I think it'd be nice to add support for 16-bit pca953x devices.  I don't
personally use them, but others might in the future, or they may choose
to use them since they are well supported in U-Boot and Linux.  If you
already use the pca9539, getting it in mainline U-Boot will be nice for
you in the long run either way.

I think it'd be worth attempting to combine your changes in the existing
pca953x driver though.  They are similar enough that it should be doable
without too much headache, and it'll be more maintainable to have a
unified driver.

Linux combines the 2 for what its worth (see drivers/gpio/pca953x.c).
It uses the number of GPIO pins to conditionally support 8 or 16-bit
expanders.  You could do the same thing to the U-Boot pca953x driver.
Eg at the top you could add:
#ifdef CONFIG_PCA953X_16BIT
#define NGPIO = 16
#else
#define NGPIO = 8
#endif

You could also change the read/write parameters to be uint16's instead
of the current uint8's to support 16-bits of GPIO.  I think with those 2
changes, plus some additional conditionals you could combine the 2
drivers without too much effort.

Best,
Peter
Chris Packham - Dec. 7, 2010, 1:12 a.m.
On Tue, Dec 7, 2010 at 1:13 PM, Peter Tyser <ptyser@xes-inc.com> wrote:
> Hi Chris,
>
> On Tue, 2010-12-07 at 10:48 +1300, Chris Packham wrote:
>> This adds support for the PCA9539 family of gpio devices which have 16
>> output pins. The devices are similar to chips that use the pca953x driver
>> except the register map is expanded (in a non-backwards compatible manner)
>> to allow for the extra 8 pins.
>>
>> This driver has one gotcha that you can only set the top or bottom 8 bits
>> at a time. For example to set the 16 pins to be outputs you need to make
>> the function calls
>>
>>   pca9539_set_dir(PCA9539_ADDR, 0xff00, 0x0000);
>>   pca9539_set_dir(PCA9539_ADDR, 0x00ff, 0x0000);
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> ---
>> Hi,
>>
>> I have a couple of questions related to my patch. Firstly is anyone
>> interested in
>> support for these gpio devices? Secondly is my gotcha acceptable or should I
>> put some effort into making it handle a 16-bit argument in 1 hit?
>>
>> I toyed with the idea of making this part of the pca953x driver. I
>> didn't because
>> the register layout is different so I'd still need to define PCA9539 specific
>> functions and macros (although pca953x_reg_write could be re-used). I also
>> figured that some people may want to define only CONFIG_PCA9539 (that is
>> the case for me at the moment but the overhead of CONFIG_PCA953X wouldn't
>> push the size of my image over any boundary).
>
> I think it'd be nice to add support for 16-bit pca953x devices.  I don't
> personally use them, but others might in the future, or they may choose
> to use them since they are well supported in U-Boot and Linux.  If you
> already use the pca9539, getting it in mainline U-Boot will be nice for
> you in the long run either way.
>
> I think it'd be worth attempting to combine your changes in the existing
> pca953x driver though.  They are similar enough that it should be doable
> without too much headache, and it'll be more maintainable to have a
> unified driver.

I'll give it a try and see what the code looks like (see below).

> Linux combines the 2 for what its worth (see drivers/gpio/pca953x.c).
> It uses the number of GPIO pins to conditionally support 8 or 16-bit
> expanders.

Thanks I knew Linux supported it I just hadn't checked how. I hadn't
considered using word read/writes in my naive poking around with i2c
md I managed to convince myself that it wouldn't work, I'll check
again.

> You could do the same thing to the U-Boot pca953x driver.
> Eg at the top you could add:
> #ifdef CONFIG_PCA953X_16BIT
> #define NGPIO = 16
> #else
> #define NGPIO = 8
> #endif

I have a small problem with this due to the fact that we have some
designs here that use a pca9534 and a pca9539. Having a compile-time
conditional like this means you can only support one or the other.
Linux can handle this because it's actually got a structure that
contains the 8 vs 16 whereas u-boot only has the address an no other
information. Any suggestions on handling this would be welcome.

>
> You could also change the read/write parameters to be uint16's instead
> of the current uint8's to support 16-bits of GPIO.  I think with those 2
> changes, plus some additional conditionals you could combine the 2
> drivers without too much effort.
>
> Best,
> Peter
>
>
Peter Tyser - Dec. 7, 2010, 1:34 a.m.
<snip>

> > You could do the same thing to the U-Boot pca953x driver.
> > Eg at the top you could add:
> > #ifdef CONFIG_PCA953X_16BIT
> > #define NGPIO = 16
> > #else
> > #define NGPIO = 8
> > #endif
> 
> I have a small problem with this due to the fact that we have some
> designs here that use a pca9534 and a pca9539. Having a compile-time
> conditional like this means you can only support one or the other.
> Linux can handle this because it's actually got a structure that
> contains the 8 vs 16 whereas u-boot only has the address an no other
> information. Any suggestions on handling this would be welcome.

Some ideas:
- Do a series of byte reads and determine the number of GPIOs based on
mirroring of registers, or reads failing if they are past the "end" of
the device.  eg reading register 4 on a 8-bit device might fail, or wrap
around and return the same data as register 0.

- Make CONFIG_PCA953X_16BIT an array that lists which I2C address
support 16 bits, then use it to determine which devices are 8 vs 16 bits
dynamically.

- Convert the driver to be more intelligent.  eg add an interface like
pca953x_add_dev(u8 addr, u8 bits) which each board calls to add a device
to a list of devices maintained by the pca953x driver.  Eg in my board
code I'd do:
pca953x_add_dev(0x18, 8);
pca953x_add_dev(0x1c, 8);
pca953x_add_dev(0x1e, 8);
pca953x_add_dev(0x1f, 8);

Best,
Peter
Chris Packham - Dec. 7, 2010, 1:50 a.m.
On Tue, Dec 7, 2010 at 2:34 PM, Peter Tyser <ptyser@xes-inc.com> wrote:
> <snip>
>
>> > You could do the same thing to the U-Boot pca953x driver.
>> > Eg at the top you could add:
>> > #ifdef CONFIG_PCA953X_16BIT
>> > #define NGPIO = 16
>> > #else
>> > #define NGPIO = 8
>> > #endif
>>
>> I have a small problem with this due to the fact that we have some
>> designs here that use a pca9534 and a pca9539. Having a compile-time
>> conditional like this means you can only support one or the other.
>> Linux can handle this because it's actually got a structure that
>> contains the 8 vs 16 whereas u-boot only has the address an no other
>> information. Any suggestions on handling this would be welcome.
>
> Some ideas:
> - Do a series of byte reads and determine the number of GPIOs based on
> mirroring of registers, or reads failing if they are past the "end" of
> the device.  eg reading register 4 on a 8-bit device might fail, or wrap
> around and return the same data as register 0.

Hmm, I can see problems ahead for that approach.

> - Make CONFIG_PCA953X_16BIT an array that lists which I2C address
> support 16 bits, then use it to determine which devices are 8 vs 16 bits
> dynamically.

I think this is probably the best option. It shouldn't be that hard to
implement.

> - Convert the driver to be more intelligent.  eg add an interface like
> pca953x_add_dev(u8 addr, u8 bits) which each board calls to add a device
> to a list of devices maintained by the pca953x driver.  Eg in my board
> code I'd do:
> pca953x_add_dev(0x18, 8);
> pca953x_add_dev(0x1c, 8);
> pca953x_add_dev(0x1e, 8);
> pca953x_add_dev(0x1f, 8);

I like the idea but I was hoping not to affect other boards, but if
people are on-board and able to test I could look into this option.

Another approach I though about was having a global variable which
would default to 8 but could be modified via an appropriate setter
prior to making calls to pca953x_set_xyz.
Peter Tyser - Dec. 7, 2010, 4:31 a.m.
Chris Packham wrote:
> On Tue, Dec 7, 2010 at 2:34 PM, Peter Tyser <ptyser@xes-inc.com> wrote:
>   
>> <snip>
>>
>>     
>>>> You could do the same thing to the U-Boot pca953x driver.
>>>> Eg at the top you could add:
>>>> #ifdef CONFIG_PCA953X_16BIT
>>>> #define NGPIO = 16
>>>> #else
>>>> #define NGPIO = 8
>>>> #endif
>>>>         
>>> I have a small problem with this due to the fact that we have some
>>> designs here that use a pca9534 and a pca9539. Having a compile-time
>>> conditional like this means you can only support one or the other.
>>> Linux can handle this because it's actually got a structure that
>>> contains the 8 vs 16 whereas u-boot only has the address an no other
>>> information. Any suggestions on handling this would be welcome.
>>>       
>> Some ideas:
>> - Do a series of byte reads and determine the number of GPIOs based on
>> mirroring of registers, or reads failing if they are past the "end" of
>> the device.  eg reading register 4 on a 8-bit device might fail, or wrap
>> around and return the same data as register 0.
>>     
>
> Hmm, I can see problems ahead for that approach.
>
>   
>> - Make CONFIG_PCA953X_16BIT an array that lists which I2C address
>> support 16 bits, then use it to determine which devices are 8 vs 16 bits
>> dynamically.
>>     
>
> I think this is probably the best option. It shouldn't be that hard to
> implement.
>
>   
>> - Convert the driver to be more intelligent.  eg add an interface like
>> pca953x_add_dev(u8 addr, u8 bits) which each board calls to add a device
>> to a list of devices maintained by the pca953x driver.  Eg in my board
>> code I'd do:
>> pca953x_add_dev(0x18, 8);
>> pca953x_add_dev(0x1c, 8);
>> pca953x_add_dev(0x1e, 8);
>> pca953x_add_dev(0x1f, 8);
>>     
>
> I like the idea but I was hoping not to affect other boards, but if
> people are on-board and able to test I could look into this option.
>
> Another approach I though about was having a global variable which
> would default to 8 but could be modified via an appropriate setter
> prior to making calls to pca953x_set_xyz.
>   

I believe the only mainline users of the pca953x driver are my company's 
boards.  I'm more than happy to test any modifications to the driver and 
to tweak our boards as needed, so I'd recommend choosing whichever 
approach is the cleanest in the long run.

Best,
Peter

Patch

diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 398024c..3d7bde5 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -29,6 +29,7 @@  COBJS-$(CONFIG_AT91_GPIO)	+= at91_gpio.o
 COBJS-$(CONFIG_KIRKWOOD_GPIO)	+= kw_gpio.o
 COBJS-$(CONFIG_MXC_GPIO)	+= mxc_gpio.o
 COBJS-$(CONFIG_PCA953X)		+= pca953x.o
+COBJS-$(CONFIG_PCA9539)		+= pca9539.o
 COBJS-$(CONFIG_S5P)		+= s5p_gpio.o

 COBJS	:= $(COBJS-y)
diff --git a/drivers/gpio/pca9539.c b/drivers/gpio/pca9539.c
new file mode 100644
index 0000000..f27ee1b
--- /dev/null
+++ b/drivers/gpio/pca9539.c
@@ -0,0 +1,92 @@ 
+/*
+ * Support for NXP's 16-bit I2C gpio expander
+ *
+ * Copyright (c) 2010 Allied Telesis Labs
+ *
+ * Based on pca953x.c
+ * Copyright 2008 Extreme Engineering Solutions, Inc.
+ *
+ * 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.
+ */
+#include <common.h>
+#include <i2c.h>
+#include <pca9539.h>
+
+static int pca9539_reg_write(uint8_t chip, uint addr, uint mask, uint data)
+{
+	uint8_t val;
+
+	if (i2c_read(chip, addr, 1, &val, 1))
+		return -1;
+
+	val &= ~mask;
+	val |= data;
+
+	return i2c_write(chip, addr, 1, &val, 1);
+}
+
+/*
+ * Set output value of IO pins in 'mask' to corresponding value in 'data'
+ * 0 = low, 1 = high
+ */
+int pca9539_set_val(uint8_t chip, uint mask, uint data)
+{
+	uint addr = PCA9539_OUT0;
+	if (mask & 0xff00) {
+		addr = PCA9539_OUT1;
+		mask = mask >> 8;
+		data = data >> 8;
+	}
+	return pca9539_reg_write(chip, addr, mask, data);
+}
+
+/*
+ * Set read polarity of IO pins in 'mask' to corresponding value in 'data'
+ * 0 = read pin value, 1 = read inverted pin value
+ */
+int pca9539_set_pol(uint8_t chip, uint mask, uint data)
+{
+	uint addr = PCA9539_POL0;
+	if (mask & 0xff00) {
+		addr = PCA9539_POL1;
+		mask = mask >> 8;
+		data = data >> 8;
+	}
+	return pca9539_reg_write(chip, addr, mask, data);
+}
+
+/*
+ * Set direction of IO pins in 'mask' to corresponding value in 'data'
+ * 0 = output, 1 = input
+ */
+int pca9539_set_dir(uint8_t chip, uint mask, uint data)
+{
+	uint addr = PCA9539_CONF0;
+	if (mask & 0xff00) {
+		addr = PCA9539_CONF1;
+		mask = mask >> 8;
+		data = data >> 8;
+	}
+	return pca9539_reg_write(chip, addr, mask, data);
+}
+
+/*
+ * Read current logic level of all IO pins
+ */
+int pca9539_get_val(uint8_t chip)
+{
+	uint8_t val;
+	int value;
+
+	if (i2c_read(chip, PCA9539_IN0, 1, &val, 1))
+		return -1;
+	value = val;
+
+	if (i2c_read(chip, PCA9539_IN1, 1, &val, 1))
+		return -1;
+	value |= val << 8;
+
+	return value;
+}
diff --git a/include/pca9539.h b/include/pca9539.h
new file mode 100644
index 0000000..83793b1
--- /dev/null
+++ b/include/pca9539.h
@@ -0,0 +1,36 @@ 
+/*
+ * Copyright (c) 2010 Allied Telesis Labs
+ *
+ * Based on pca953x.h
+ * Copyright 2008 Extreme Engineering Solutions, Inc.
+ *
+ * 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 __PCA9539_H_
+#define __PCA9539_H_
+
+#define PCA9539_IN0	0x00
+#define PCA9539_IN1	0x01
+#define PCA9539_OUT0	0x02
+#define PCA9539_OUT1	0x03
+#define PCA9539_POL0	0x04
+#define PCA9539_POL1	0x05
+#define PCA9539_CONF0	0x06
+#define PCA9539_CONF1	0x07
+
+#define PCA9539_OUT_LOW		0
+#define PCA9539_OUT_HIGH	1
+#define PCA9539_POL_NORMAL	0
+#define PCA9539_POL_INVERT	1
+#define PCA9539_DIR_OUT		0
+#define PCA9539_DIR_IN		1
+
+int pca9539_set_val(u8 chip, uint mask, uint data);
+int pca9539_set_pol(u8 chip, uint mask, uint data);
+int pca9539_set_dir(u8 chip, uint mask, uint data);
+int pca9539_get_val(u8 chip);
+
+#endif /* __PCA9539_H_ */