diff mbox

[U-Boot,v2] gpio: Add PCA9698 40-bit I2C I/O port

Message ID 1319006040-25380-1-git-send-email-eibach@gdsys.de
State Superseded
Headers show

Commit Message

Dirk Eibach Oct. 19, 2011, 6:34 a.m. UTC 
Signed-off-by: Dirk Eibach <eibach@gdsys.de>
---
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

Comments

Stefan Roese Oct. 19, 2011, 7:48 a.m. UTC | #1 
Hi Dirk,

On Wednesday 19 October 2011 08:34:00 Dirk Eibach wrote:
> Signed-off-by: Dirk Eibach <eibach@gdsys.de>
> ---
> 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@denx.de
Dirk Eibach Oct. 19, 2011, 8:37 a.m. UTC | #2 
Hi Stefan.

> 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 Oct. 19, 2011, 2:36 p.m. UTC | #3 
On Wednesday 19 October 2011 04:37:26 Eibach, Dirk wrote:
> > 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?

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
Mike Frysinger Oct. 19, 2011, 2:41 p.m. UTC | #4 
On Wednesday 19 October 2011 02:34:00 Dirk Eibach wrote:
> --- /dev/null
> +++ b/include/pca9698.h
>
> +#ifndef __PCA9698_H_
> +#define __PCA9698_H_

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

> +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
Stefan Roese Oct. 20, 2011, 6:45 a.m. UTC | #5 
Hi Dirk,

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

--
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@denx.de
diff mbox

Patch

diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 62ec97d..38a62c3 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -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
diff --git a/drivers/gpio/pca9698.c b/drivers/gpio/pca9698.c
new file mode 100644
index 0000000..a98cd0e
--- /dev/null
+++ b/drivers/gpio/pca9698.c
@@ -0,0 +1,123 @@ 
+/*
+ * (C) Copyright 2011
+ * Dirk Eibach,  Guntermann & Drunck GmbH, eibach@gdsys.de
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+/*
+ * Driver for NXP's pca9698 40 bit I2C gpio expander
+ */
+
+#include <common.h>
+#include <i2c.h>
+#include <pca9698.h>
+
+/*
+ * The pca9698 registers
+ */
+
+#define PCA9698_REG_INPUT		0x00
+#define PCA9698_REG_OUTPUT		0x08
+#define PCA9698_REG_POLARITY		0x10
+#define PCA9698_REG_CONFIG		0x18
+
+#define PCA9698_BUFFER_SIZE		5
+
+static int pca9698_read40(u8 chip, u8 offset, u8 *buffer)
+{
+	u8 command = offset | 0x80;  /* autoincrement */
+
+	return i2c_read(chip, command, 1, buffer, PCA9698_BUFFER_SIZE);
+}
+
+static int pca9698_write40(u8 chip, u8 offset, u8 *buffer)
+{
+	u8 command = offset | 0x80;  /* autoincrement */
+
+	return i2c_write(chip, command, 1, buffer, PCA9698_BUFFER_SIZE);
+}
+
+static void pca9698_set_bit(unsigned gpio, u8 *buffer, unsigned value)
+{
+	unsigned byte = gpio / 8;
+	unsigned bit = gpio % 8;
+
+	if (value)
+		buffer[byte] |= (1 << bit);
+	else
+		buffer[byte] &= ~(1 << bit);
+}
+
+int pca9698_direction_input(u8 chip, unsigned offset)
+{
+	u8 data[PCA9698_BUFFER_SIZE];
+	int res;
+
+	res = pca9698_read40(chip, PCA9698_REG_CONFIG, data);
+	if (res)
+		return res;
+
+	pca9698_set_bit(offset, data, 1);
+	return pca9698_write40(chip, PCA9698_REG_CONFIG, data);
+}
+
+int pca9698_direction_output(u8 chip, unsigned offset)
+{
+	u8 data[PCA9698_BUFFER_SIZE];
+	int res;
+
+	res = pca9698_read40(chip, PCA9698_REG_CONFIG, data);
+	if (res)
+		return res;
+
+	pca9698_set_bit(offset, data, 0);
+	return pca9698_write40(chip, PCA9698_REG_CONFIG, data);
+}
+
+int pca9698_get_input(u8 chip, unsigned offset)
+{
+	unsigned config_byte = offset / 8;
+	unsigned config_bit = offset % 8;
+	unsigned value;
+	u8 data[PCA9698_BUFFER_SIZE];
+	int res;
+
+	res = pca9698_read40(chip, PCA9698_REG_INPUT, data);
+	if (res)
+		return -1;
+
+	value = data[config_byte] & (1 << config_bit);
+
+	return !!value;
+}
+
+int pca9698_set_output(u8 chip, unsigned offset, int value)
+{
+	u8 data[PCA9698_BUFFER_SIZE];
+	int res;
+
+	res = pca9698_read40(chip, PCA9698_REG_OUTPUT, data);
+	if (res)
+		return res;
+
+	memset(data, 0, sizeof(data));
+	pca9698_set_bit(offset, data, value);
+	return pca9698_write40(chip, PCA9698_REG_OUTPUT, data);
+}
diff --git a/include/pca9698.h b/include/pca9698.h
new file mode 100644
index 0000000..2506088
--- /dev/null
+++ b/include/pca9698.h
@@ -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_ */