gpio: Add PCA9698 40-bit I2C I/O port

PATCH Superseded Dirk Eibach 4 participants 6 messages Oct 19, 2011 – Oct 20, 2011 GPIO

lore

Share & tools

lore

Dirk Eibach

author

2011-10-19 06:34 UTC #0 lore raw

PATCH v2 1/1 gpio: Add PCA9698 40-bit I2C I/O port

Signed-off-by Dirk Eibach <[email protected]>

---
Changes for v2:
- fixed parameters for memset() in pca9698_set_output()

 drivers/gpio/Makefile  |    1 +
 drivers/gpio/pca9698.c |  123 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/pca9698.h      |    9 ++++
 3 files changed, 133 insertions(+), 0 deletions(-)
 create mode 100644 drivers/gpio/pca9698.c
 create mode 100644 include/pca9698.h

drivers/gpio/Makefile +1 -0

index 62ec97d..38a62c3 100644
@@ -30,6 +30,7 @@ COBJS-$(CONFIG_KIRKWOOD_GPIO)	+= kw_gpio.o
 COBJS-$(CONFIG_MARVELL_MFP)	+= mvmfp.o
 COBJS-$(CONFIG_MXC_GPIO)	+= mxc_gpio.o
 COBJS-$(CONFIG_PCA953X)		+= pca953x.o
+COBJS-$(CONFIG_PCA9698)		+= pca9698.o
 COBJS-$(CONFIG_S5P)		+= s5p_gpio.o
 COBJS-$(CONFIG_TEGRA2_GPIO)	+= tegra2_gpio.o
 COBJS-$(CONFIG_DA8XX_GPIO)	+= da8xx_gpio.o

drivers/gpio/pca9698.c new file +123 -0

123 lines not on this page · raw message

include/pca9698.h new file +9 -0

new file mode 100644
index 0000000..2506088
@@ -0,0 +1,9 @@
+#ifndef __PCA9698_H_
+#define __PCA9698_H_
+
+int pca9698_direction_input(u8 chip, unsigned offset);
+int pca9698_direction_output(u8 chip, unsigned offset);
+int pca9698_get_input(u8 chip, unsigned offset);
+int pca9698_set_output(u8 chip, unsigned offset, int value);
+
+#endif /* __PCA9698_H_ */

Signature · 1 line

1.5.6.5

Stefan Roese

2011-10-19 07:48 UTC #1 lore raw

PATCH v2 1/1 gpio: Add PCA9698 40-bit I2C I/O port

Hi Dirk,

Quoted 4 lines · On Wednesday 19 October 2011 08:34:00 Dirk Eibach wrote:

Signed-off-by Dirk Eibach <[email protected]>
---
Changes for v2:
- fixed parameters for memset() in pca9698_set_output()

Thanks.

While looking again, I noticed that you are not using the "standard" GPIO API borrowed from Linux. Please take a look at drivers/gpio/mxc_gpio.c or drivers/gpio/mvgpio.c as an example.

Sorry for not mentioning this earlier. Could you please update this patch and your Io64 BSP patch by using this "standard" GPIO API?

Thanks.

Best regards,
Stefan

--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office at denx.de

Eibach

2011-10-19 08:37 UTC #2 lore raw

PATCH v2 1/1 gpio: Add PCA9698 40-bit I2C I/O port

Hi Stefan.

Quoted 7 lines

While looking again, I noticed that you are not using the "standard" GPIO API borrowed from Linux. Please take a look at drivers/gpio/mxc_gpio.c or drivers/gpio/mvgpio.c as an example.
Sorry for not mentioning this earlier. Could you please update this patch and your Io64 BSP patch by using this "standard" GPIO API?

I see that the drivers you mentioned are for processor gpio, mine is for an i2c port expander.
The gpio API "implementation" in these drivers is a mess because it does not support managing multiple gpio providers. Using this API would make it impossible to use pca9698 on such platforms. If someone decided to build a processor gpio driver like this for ppc4xx we would even have a collision for Io64.

Until the whole GPIO API idea is more refined I am not too happy adapting pca9698 implementation.
Do you insist?

Cheers
Dirk

Mike Frysinger

2011-10-19 14:36 UTC #3 lore raw

PATCH v2 1/1 gpio: Add PCA9698 40-bit I2C I/O port

Quoted 16 lines · Dirk

Quoted 7 lines
While looking again, I noticed that you are not using the "standard" GPIO API borrowed from Linux. Please take a look at drivers/gpio/mxc_gpio.c or drivers/gpio/mvgpio.c as an example.
Sorry for not mentioning this earlier. Could you please update this patch and your Io64 BSP patch by using this "standard" GPIO API?
I see that the drivers you mentioned are for processor gpio, mine is for an i2c port expander.
The gpio API "implementation" in these drivers is a mess because it does not support managing multiple gpio providers. Using this API would make it impossible to use pca9698 on such platforms. If someone decided to build a processor gpio driver like this for ppc4xx we would even have a collision for Io64.

Quoted 3 lines

Until the whole GPIO API idea is more refined I am not too happy adapting pca9698 implementation.
Do you insist?

the GPIO API is not specific to processors. atm, only SoC's are implementing it. what you're looking for is the gpiolib which Linux supports. we haven't bothered adding that layer yet as no one was adding GPIO expanders.

you still have your API logically line up with the common GPIO API so that when said layer lands, it's easy to transition to.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111019/816872e3/attachment.pgp

Stefan Roese

2011-10-20 06:45 UTC #4 lore raw

PATCH v2 1/1 gpio: Add PCA9698 40-bit I2C I/O port

Hi Dirk,

Quoted 7 lines · On Wednesday 19 October 2011 16:36:08 Mike Frysinger wrote:

the GPIO API is not specific to processors. atm, only SoC's are implementing it. what you're looking for is the gpiolib which Linux supports. we haven't bothered adding that layer yet as no one was adding GPIO expanders.
you still have your API logically line up with the common GPIO API so that when said layer lands, it's easy to transition to.

Full ACK. Dirk, please address the comments from Mike in your next patch version (shouldn't be too hard) and I'll gladly ACK your patch.

Thanks.

Best regards,
Stefan

Mike Frysinger

2011-10-19 14:41 UTC #5 lore raw

PATCH v2 1/1 gpio: Add PCA9698 40-bit I2C I/O port

In reply to Dirk Eibach

include/pca9698.h line 2

Quoted 5 lines · On Wednesday 19 October 2011 02:34:00 Dirk Eibach wrote:

include/pca9698.h new file +2 -0
 
+#ifndef __PCA9698_H_
+#define __PCA9698_H_

missing copyright/license/etc... comment block at top of file

include/pca9698.h line 7

Quoted 4 lines

include/pca9698.h new file +4 -0

+int pca9698_direction_input(u8 chip, unsigned offset);
+int pca9698_direction_output(u8 chip, unsigned offset);
+int pca9698_get_input(u8 chip, unsigned offset);
+int pca9698_set_output(u8 chip, unsigned offset, int value);

what is "chip" ? an i2c addr ? and "offset" is some internal value indicating which pin to drive ? if so, better names might be "u8 addr" and "unsigned gpio". a single comment here would go a long way.

in terms of conforming to existing API:
- the direction_output() function needs to take a "value"
- get_input() should be get_value()
- set_output() should be set_value()
- you should have a request() func to validate the GPIO
- add a stub free() func
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111019/5bf55da6/attachment.pgp