[U-Boot] [PATCH 0/2] arm nomadik: gpio and i2c

[U-Boot] [PATCH 0/2] arm nomadik: gpio and i2c

[Alessandro Rubini]

This adds gpio and i2c support for the Nomadik evaluation kit. They
are needed to turn on the LCD backlight in order to later add LCD
support.

I have one doubt and some questions on gpio:

To use soft_i2c I need to define some macros in the config file.
Instead of writing hard numbers there I called the gpio functions, but
the config file is inluded from asm sources as well.  I don't think my
approach is beautiful at all (both #ifndef __ASSEMBLY__ and #include
"../board/"), but I didn't find a better solution.

I would like to add a gpio command, and I've found no generic gpio
stuff.  Only one board (cm-bf527) has a gpio commands, but quite a few
have similar commands to set leds or other bits.  Is time ripe for a
generic gpio driver with board-specific limits and operations? Would
that be interesting for u-boot-next?  Should I process with a board-specific
gpio command by now?

/alessandro


Alessandro Rubini (2):
  arm nomadik: add gpio support
  arm nomadik: add i2c

 board/st/nhk8815/Makefile  |    2 +-
 board/st/nhk8815/gpio.c    |   99 ++++++++++++++++++++++++++++++++++++++++++++
 board/st/nhk8815/gpio.h    |   42 ++++++++++++++++++
 board/st/nhk8815/nhk8815.c |   16 ++++++-
 include/configs/nhk8815.h  |   18 ++++++++-
 5 files changed, 173 insertions(+), 4 deletions(-)
 create mode 100644 board/st/nhk8815/gpio.c
 create mode 100644 board/st/nhk8815/gpio.h

[U-Boot] [PATCH 0/2] arm nomadik: gpio and i2c

[Wolfgang Denk]

Dear Alessandro Rubini,

In message <cover.1247999841.git.rubini@unipv.it> you wrote:
> 
> To use soft_i2c I need to define some macros in the config file.
> Instead of writing hard numbers there I called the gpio functions, but
> the config file is inluded from asm sources as well.  I don't think my
> approach is beautiful at all (both #ifndef __ASSEMBLY__ and #include
> "../board/"), but I didn't find a better solution.

Agreed. It's a bit intricate to get this done, but I don't see a
better way either.

> I would like to add a gpio command, and I've found no generic gpio
> stuff.  Only one board (cm-bf527) has a gpio commands, but quite a few
> have similar commands to set leds or other bits.  Is time ripe for a
> generic gpio driver with board-specific limits and operations? Would
> that be interesting for u-boot-next?  Should I process with a board-specific
> gpio command by now?

Well, my opinion on that is a clear "yes, but..." :-)

Yes, some generic gpio framework would be nice - for example,  if  it
would allow us to get rid of the 14 largely similar "led.c" files, to
name just one.

On the other hand, the design of such a framework should be lean  and
not  necessarily  try  to  cover 100% of all possible use cases - I'd
rather have a small and beautiful solution that  covers  90%  of  the
cases  and use board-specific exceptions where really needed, instead
of a fat thing that solves each and every problem but costs 50 kB.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The years of peak mental activity are undoubtedly between the ages of
four and eighteen. At four we know all the questions, at eighteen all
the answers.

[U-Boot] [PATCH 0/2] arm nomadik: gpio and i2c

[Heiko Schocher]

Hello Alessandro,

Alessandro Rubini wrote:
> This adds gpio and i2c support for the Nomadik evaluation kit. They
> are needed to turn on the LCD backlight in order to later add LCD
> support.
> 
> I have one doubt and some questions on gpio:
> 
> To use soft_i2c I need to define some macros in the config file.
> Instead of writing hard numbers there I called the gpio functions, but
> the config file is inluded from asm sources as well.  I don't think my
> approach is beautiful at all (both #ifndef __ASSEMBLY__ and #include
> "../board/"), but I didn't find a better solution.

Yes, thats a problem ... if we had a GPIO Framework it would be solved
by including gpio.h ...

Or, maybe, we can make a soft_i2c.h which which gets only included
if saying CONFIG_I2C_SOFT_INCLUDE is defined. soft_i2c.h defines for
example:

#define I2C_SDA(x)	i2c_soft_sda(bit)

void i2c_soft_sda(int pin);

and you can define this function i2c_soft_sda(int pin) in your board
specific code ... maybe a cleaner option?

to speak in c, I tried the following patch on the suen3 plattform,
where I have actually a similiar problem, and this worked fine :-)

[PATCH] i2c, soft: added soft_i2c.h

In case you must define functions for the I2C_XXX
defines, it is necessary to have a soft_i2c.h, which
defines functions for this macros. This functions can
then be programmed in board specific code. To activate
this it must be CONFIG_I2C_SOFT_INCLUDE defined in the
board config file.

Signed-off-by: Heiko Schocher <hs@denx.de>
---
 drivers/i2c/soft_i2c.c |    3 +++
 include/soft_i2c.h     |   16 ++++++++++++++++
 2 files changed, 19 insertions(+), 0 deletions(-)
 create mode 100644 include/soft_i2c.h

diff --git a/drivers/i2c/soft_i2c.c b/drivers/i2c/soft_i2c.c
index 59883a5..30e24f3 100644
--- a/drivers/i2c/soft_i2c.c
+++ b/drivers/i2c/soft_i2c.c
@@ -43,6 +43,9 @@
 #if defined(CONFIG_MPC852T) || defined(CONFIG_MPC866)
 #include <asm/io.h>
 #endif
+#if defined(CONFIG_SOFT_I2C_INCLUDE)
+#include <soft_i2c.h>
+#endif
 #include <i2c.h>

 /* #define	DEBUG_I2C	*/
diff --git a/include/soft_i2c.h b/include/soft_i2c.h
new file mode 100644
index 0000000..39f9a35
--- /dev/null
+++ b/include/soft_i2c.h
@@ -0,0 +1,16 @@
+#ifndef _CONFIG_SOFT_I2C
+#define _CONFIG_SOFT_I2C
+#define I2C_ACTIVE	i2c_soft_active();
+#define I2C_TRISTATE	i2c_soft_tristate();
+#define I2C_READ	i2c_soft_read();
+#define I2C_SDA(bit)	i2c_soft_sda(bit);
+#define I2C_SCL(bit)	i2c_soft_scl(bit);
+#define	I2C_DELAY	i2c_soft_delay();
+
+void	i2c_soft_active(void);
+void	i2c_soft_tristate(void);
+int	i2c_soft_read(void);
+void	i2c_soft_sda(int value);
+void	i2c_soft_scl(int value);
+void	i2c_soft_delay(void);
+#endif
-- 
1.6.0.GIT

Maybe you can try it too?

> I would like to add a gpio command, and I've found no generic gpio
> stuff.  Only one board (cm-bf527) has a gpio commands, but quite a few
> have similar commands to set leds or other bits.  Is time ripe for a
> generic gpio driver with board-specific limits and operations? Would
> that be interesting for u-boot-next?  Should I process with a board-specific
> gpio command by now?

I vote for making a gpio framework, but that will take a while I think ...
Hmm.. maybe we use my proposal for such a soft_i2c.h, so I think, it is
okay for such a board specific gpio (unless we have a gpio framework).

bye
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH 0/2] arm nomadik: gpio and i2c

[Heiko Schocher]

Hello Wolfgang,

Wolfgang Denk wrote:
> Dear Alessandro Rubini,
> 
> In message <cover.1247999841.git.rubini@unipv.it> you wrote:
>> To use soft_i2c I need to define some macros in the config file.
>> Instead of writing hard numbers there I called the gpio functions, but
>> the config file is inluded from asm sources as well.  I don't think my
>> approach is beautiful at all (both #ifndef __ASSEMBLY__ and #include
>> "../board/"), but I didn't find a better solution.
> 
> Agreed. It's a bit intricate to get this done, but I don't see a
> better way either.

Hmm.. maybe my previous patch is a better solution?

>> I would like to add a gpio command, and I've found no generic gpio
>> stuff.  Only one board (cm-bf527) has a gpio commands, but quite a few
>> have similar commands to set leds or other bits.  Is time ripe for a
>> generic gpio driver with board-specific limits and operations? Would
>> that be interesting for u-boot-next?  Should I process with a board-specific
>> gpio command by now?
> 
> Well, my opinion on that is a clear "yes, but..." :-)
> 
> Yes, some generic gpio framework would be nice - for example,  if  it
> would allow us to get rid of the 14 largely similar "led.c" files, to
> name just one.
> 
> On the other hand, the design of such a framework should be lean  and
> not  necessarily  try  to  cover 100% of all possible use cases - I'd
> rather have a small and beautiful solution that  covers  90%  of  the
> cases  and use board-specific exceptions where really needed, instead
> of a fat thing that solves each and every problem but costs 50 kB.

Agreed. So we need an gpio_core.c / .h which defines the following
functions (just a proposal):

typedef struct gpio_adapter {
	int	(*init_pin)(int pin);
	int	(*set)(int pin, value);
	int	(*get)(int pin);
	int	(*dir)(int pin, int direction);	
	int	(*level)(int pin, int level);
}

int gpio_init(gpio_adapter *adap);
int gpio_init_pin(pin);
	? maybe with setting a marker, that this pin is initialized,
	  so this can be checked in the above functions ... ?
int gpio_set(pin, value);
int gpio_get(pin);
int gpio_dir(pin, dir);
int gpio_level(pin, dir);

bye
Heiko

[U-Boot] [PATCH 0/2] arm nomadik: gpio and i2c

[Alessandro Rubini]

> Agreed. So we need an gpio_core.c / .h which defines the following
> functions (just a proposal):

Yes.

> typedef struct gpio_adapter {
> 	int	(*init_pin)(int pin);
> 	int	(*set)(int pin, value);
> 	int	(*get)(int pin);
> 	int	(*dir)(int pin, int direction);	
> 	int	(*level)(int pin, int level);
> }

I don't understand the init_pin function, nor what "level" is.
Actually, even "dir" can be dropped: a get configures as input, a set
configures as output, the extra instruction is very little overhead.

But an alternate function configuration is definitely needed: everybody
has alternate functions associated to the pins. Just say "0" is gpio
and 1...n is SoC-specific.

So, are you going to write it? Or should someone else do that?

thanks
/alessandro

[U-Boot] [PATCH 0/2] arm nomadik: gpio and i2c

[Heiko Schocher]

Hello Alessandro,

Alessandro Rubini wrote:
>> Agreed. So we need an gpio_core.c / .h which defines the following
>> functions (just a proposal):
> 
> Yes.
> 
>> typedef struct gpio_adapter {
>> 	int	(*init_pin)(int pin);
>> 	int	(*set)(int pin, value);
>> 	int	(*get)(int pin);
>> 	int	(*dir)(int pin, int direction);	
>> 	int	(*level)(int pin, int level);
>> }
> 
> I don't understand the init_pin function, nor what "level" is.

init_pin:
  you call this function, if you want to use this pin as an gpio.
  (BTW: init pin should look like this,
    changing init_pin(pin, dir, value);
      with value only used if setting it as an output)
  functionality:
    - Setup the GPIO registers for using this pin as GPIO
    - Maybe setting a marker in the gpio_core, that this pin is usable
      now for gpio. This marker can be checked in the other functions,
      which use this pin ...

level:
  we can set the output level with this function directly ... ok,
  not really needed ... but maybe nice to have ... if thinking for
  the i2c bitbang driver, we save on all set() calls the output
  settings ... maybe more then one register changes, if switching
  between input/output ...

> Actually, even "dir" can be dropped: a get configures as input, a set
> configures as output, the extra instruction is very little overhead.

See my comment above. I just like the idea, that a user can control this
separately.

> But an alternate function configuration is definitely needed: everybody
> has alternate functions associated to the pins. Just say "0" is gpio
> and 1...n is SoC-specific.

what with deinit_pin(pin, function)? That would be in shape with init_pin()?

(There, we could also unset the marker, that this pin is no longer used
for gpio ...)

> So, are you going to write it? Or should someone else do that?

Hmm... if nobody else volunteers, maybe I find time for it ... but I
think, there should be more ratings, if this is a way to go ... also,
if we make a gpio_core, would we have some commands for it (show
status of gpios, directly access the gpios per commandshell, ...)?

Also, a board can have more then one gpio adapter, how we address
such a case?

Just a fast thought about that:
- adding to typedef struct gpio_adapter also a "name" field
- allow to register more than one gpio adapter
- addressing the different adapters through an int ... called gpio_device?

So the core functions look something like that:

int gpio_init(gpio_adap *adap); (returns a int gpio_device)

int gpio_init_pin(gpio_device, pin, dir, value);
int gpio_set(gpio_device, pin, value)
int gpio_get(gpio_device, pin)
int gpio_dir(gpio_device, pin, dir)
int gpio_level(gpio_device, pin, level)

bye
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH 0/2] arm nomadik: gpio and i2c

[Alessandro Rubini]

> what with deinit_pin(pin, function)? That would be in shape with init_pin()?

No, it's not clear what it is. I'd rename "init" to "setup", adding an AF
argument. So I can setup it as AF2, or as GPIO-OUT, or whatever.

> (There, we could also unset the marker, that this pin is no longer used
> for gpio ...)

No, I wouldn't like the marker. It's a boot loader, it shouldn't
overdo sanity checks.  Most of the times it runs the same "bootcmd"
over and over. In the rare but important case it's a debugging tool, it
shouldn't force policy, in my opinion (I already have problems with the
kernel gpiolib, that doesn't let me fix mishaps at will).

> Also, a board can have more then one gpio adapter, how we address
> such a case?

As Wolfgang suggested, we don't.  If atmel calls it PORTC-12 I have no
problem calling it gpio-76 by concatentating the ports as 0..31,
32..64, ...  It's still better to have a gpio command than doing "mw
<addr> <val>" over and over.

In my old-fashioned way, it should be as simple as possible, but no simpler.

/alessandro

[U-Boot] [PATCH 0/2] arm nomadik: gpio and i2c

[Heiko Schocher]

Hello Alessandro,

Alessandro Rubini wrote:
>> what with deinit_pin(pin, function)? That would be in shape with init_pin()?
> 
> No, it's not clear what it is. I'd rename "init" to "setup", adding an AF

ok.

> argument. So I can setup it as AF2, or as GPIO-OUT, or whatever.

No I don;t understand you ;-) What means AF? Ah, while typing this, my
brain parsed AF to Alternate Function, right?

So, I am fine with that.

>> (There, we could also unset the marker, that this pin is no longer used
>> for gpio ...)
> 
> No, I wouldn't like the marker. It's a boot loader, it shouldn't
> overdo sanity checks.  Most of the times it runs the same "bootcmd"
> over and over. In the rare but important case it's a debugging tool, it
> shouldn't force policy, in my opinion (I already have problems with the
> kernel gpiolib, that doesn't let me fix mishaps at will).

Ok, it was just a thought. If others agree I am fine with it.

>> Also, a board can have more then one gpio adapter, how we address
>> such a case?
> 
> As Wolfgang suggested, we don't.  If atmel calls it PORTC-12 I have no
> problem calling it gpio-76 by concatentating the ports as 0..31,
> 32..64, ...  It's still better to have a gpio command than doing "mw
> <addr> <val>" over and over.

Huh, missed I an Email? Didn;t see a response from Wolfgang ...

Ah, yes, this is also an option, so we need to configure when
adding a gpio_adapter, with which GPIO number the GPIOs in this adapter
starts and how many GPIOs are accessible through it... OK, I am also
fine with such an option.

> In my old-fashioned way, it should be as simple as possible, but no simpler.

;-)

bye
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH 0/2] arm nomadik: gpio and i2c

[Wolfgang Denk]

Dear Heiko Schocher,

In message <4A6422FC.6030508@invitel.hu> you wrote:
> 
> typedef struct gpio_adapter {
> 	int	(*init_pin)(int pin);
> 	int	(*set)(int pin, value);
> 	int	(*get)(int pin);
> 	int	(*dir)(int pin, int direction);	
> 	int	(*level)(int pin, int level);
> }
> 
> int gpio_init(gpio_adapter *adap);
> int gpio_init_pin(pin);
> 	? maybe with setting a marker, that this pin is initialized,
> 	  so this can be checked in the above functions ... ?
> int gpio_set(pin, value);
> int gpio_get(pin);
> int gpio_dir(pin, dir);
> int gpio_level(pin, dir);

What does "level" mean in this context?

gpio_init_pin() and gpio_dir() seem to be redundant  -  or  does  pin
initialization not include the setting of the direction (and, in case
of an output pin, it's initial state) ?

gpio_get() returns the current state of the pin?

gpio_set() returns the previous state of the pin? Or the  new  state?
If  it  returns  the  new  state,  we could use value=1 to set a pin,
value=0 to unset a pin, and value=-1 to just read it's value  without
changing it; then we could "#define gpio_get(pin) gpio_set(pin,-1)".

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The speed of time is one second per second.

[U-Boot] [PATCH 0/2] arm nomadik: gpio and i2c

[Wolfgang Denk]

Dear Heiko Schocher,

In message <4A6437A7.40909@denx.de> you wrote:
> 
> Also, a board can have more then one gpio adapter, how we address
> such a case?

How far do you want to take that? What about an I/O expander at the
I2C bus?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
When it is incorrect, it is, at least *authoritatively* incorrect.
                                    - Hitchiker's Guide To The Galaxy

[U-Boot] [PATCH 0/2] arm nomadik: gpio and i2c

[Heiko Schocher]

Hello Wolfgang,

Wolfgang Denk wrote:
> Dear Heiko Schocher,
> 
> In message <4A6422FC.6030508@invitel.hu> you wrote:
>> typedef struct gpio_adapter {
>> 	int	(*init_pin)(int pin);
>> 	int	(*set)(int pin, value);
>> 	int	(*get)(int pin);
>> 	int	(*dir)(int pin, int direction);	
>> 	int	(*level)(int pin, int level);
>> }
>>
>> int gpio_init(gpio_adapter *adap);
>> int gpio_init_pin(pin);
>> 	? maybe with setting a marker, that this pin is initialized,
>> 	  so this can be checked in the above functions ... ?
>> int gpio_set(pin, value);
>> int gpio_get(pin);
>> int gpio_dir(pin, dir);
>> int gpio_level(pin, dir);
> 
> What does "level" mean in this context?

Yesterday, when I emailed with Alessandro, it looked like, if we
call gpio_set(), we also set the direction to output, so I thought,
it would be nice to have a function which really just sets the value.

Now, I think, we should set the direction only with gpio_dir()
(if output also set the value).

And with gpio_set() we only set the output value, without switching
the direction ...

Or should we do allways a set direction, when calling gpio_set() ...?

> gpio_init_pin() and gpio_dir() seem to be redundant  -  or  does  pin
> initialization not include the setting of the direction (and, in case
> of an output pin, it's initial state) ?

Yes, you are right, also redundant.

> gpio_get() returns the current state of the pin?

Yep.

> gpio_set() returns the previous state of the pin? Or the  new  state?
> If  it  returns  the  new  state,  we could use value=1 to set a pin,
> value=0 to unset a pin, and value=-1 to just read it's value  without
> changing it; then we could "#define gpio_get(pin) gpio_set(pin,-1)".

Yes, thats a good point.

bye
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH 0/2] arm nomadik: gpio and i2c

[Heiko Schocher]

Hello Wolfgang,

Wolfgang Denk wrote:
> Dear Heiko Schocher,
> 
> In message <4A6437A7.40909@denx.de> you wrote:
>> Also, a board can have more then one gpio adapter, how we address
>> such a case?
> 
> How far do you want to take that? What about an I/O expander at the
> I2C bus?

Hmmm.. good question ;-)

But I don;t think it is not so exotic to have more than one gpio adapter
on one board. On the suen3 plattform where I actually work, there are
the CPU GPIO pins and GPIO pins over an I/O Expander used ... so it
should be possible to have more than one gpio adapter I think. And to
make such an I/O Expander gpio adapter should not so difficult ... and
if we now really make such a gpio lib, it should support more than one
adapter ...

I like Alessandros suggestion for concatening such multiple
adapters, see:
http://lists.denx.de/pipermail/u-boot/2009-July/056949.html

bye
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH 0/2] arm nomadik: gpio and i2c

[Wolfgang Denk]

Dear Heiko Schocher,

In message <4A655C25.8080507@denx.de> you wrote:
> 
> And with gpio_set() we only set the output value, without switching
> the direction ...

Right.

> Or should we do allways a set direction, when calling gpio_set() ...?

This makes no sense to me.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
A little suffering is good for the soul.
	-- Kirk, "The Corbomite Maneuver", stardate 1514.0

[U-Boot] [PATCH 0/2] arm nomadik: gpio and i2c

[Daniel Gorsulowski]

Hello Heiko,

Heiko Schocher wrote:
> Hello Alessandro,
> 
> Alessandro Rubini wrote:
>> This adds gpio and i2c support for the Nomadik evaluation kit. They
>> are needed to turn on the LCD backlight in order to later add LCD
>> support.
>>
>> I have one doubt and some questions on gpio:
>>
>> To use soft_i2c I need to define some macros in the config file.
>> Instead of writing hard numbers there I called the gpio functions, but
>> the config file is inluded from asm sources as well.  I don't think my
>> approach is beautiful at all (both #ifndef __ASSEMBLY__ and #include
>> "../board/"), but I didn't find a better solution.
> 
> Yes, thats a problem ... if we had a GPIO Framework it would be solved
> by including gpio.h ...
> 
> Or, maybe, we can make a soft_i2c.h which which gets only included
> if saying CONFIG_I2C_SOFT_INCLUDE is defined. soft_i2c.h defines for
> example:
> 
> #define I2C_SDA(x)	i2c_soft_sda(bit)
> 
> void i2c_soft_sda(int pin);
> 
> and you can define this function i2c_soft_sda(int pin) in your board
> specific code ... maybe a cleaner option?
> 
> to speak in c, I tried the following patch on the suen3 plattform,
> where I have actually a similiar problem, and this worked fine :-)
> 
> [PATCH] i2c, soft: added soft_i2c.h
> 
> In case you must define functions for the I2C_XXX
> defines, it is necessary to have a soft_i2c.h, which
> defines functions for this macros. This functions can
> then be programmed in board specific code. To activate
> this it must be CONFIG_I2C_SOFT_INCLUDE defined in the
> board config file.
> 
> Signed-off-by: Heiko Schocher <hs@denx.de>
> ---
>  drivers/i2c/soft_i2c.c |    3 +++
>  include/soft_i2c.h     |   16 ++++++++++++++++
>  2 files changed, 19 insertions(+), 0 deletions(-)
>  create mode 100644 include/soft_i2c.h
> 
> diff --git a/drivers/i2c/soft_i2c.c b/drivers/i2c/soft_i2c.c
> index 59883a5..30e24f3 100644
> --- a/drivers/i2c/soft_i2c.c
> +++ b/drivers/i2c/soft_i2c.c
> @@ -43,6 +43,9 @@
>  #if defined(CONFIG_MPC852T) || defined(CONFIG_MPC866)
>  #include <asm/io.h>
>  #endif
> +#if defined(CONFIG_SOFT_I2C_INCLUDE)
> +#include <soft_i2c.h>
> +#endif
>  #include <i2c.h>
> 
>  /* #define	DEBUG_I2C	*/
> diff --git a/include/soft_i2c.h b/include/soft_i2c.h
> new file mode 100644
> index 0000000..39f9a35
> --- /dev/null
> +++ b/include/soft_i2c.h
> @@ -0,0 +1,16 @@
> +#ifndef _CONFIG_SOFT_I2C
> +#define _CONFIG_SOFT_I2C
> +#define I2C_ACTIVE	i2c_soft_active();
> +#define I2C_TRISTATE	i2c_soft_tristate();
> +#define I2C_READ	i2c_soft_read();
> +#define I2C_SDA(bit)	i2c_soft_sda(bit);
> +#define I2C_SCL(bit)	i2c_soft_scl(bit);
> +#define	I2C_DELAY	i2c_soft_delay();
> +
> +void	i2c_soft_active(void);
> +void	i2c_soft_tristate(void);
> +int	i2c_soft_read(void);
> +void	i2c_soft_sda(int value);
> +void	i2c_soft_scl(int value);
> +void	i2c_soft_delay(void);
> +#endif
> -- 1.6.0.GIT Maybe you can try it too?
> > I would like to add a gpio command, and I've found no generic gpio
> > stuff.  Only one board (cm-bf527) has a gpio commands, but quite a few
> > have similar commands to set leds or other bits.  Is time ripe for a
> > generic gpio driver with board-specific limits and operations? Would
> > that be interesting for u-boot-next?  Should I process with a board-specific
> > gpio command by now?
>
> I vote for making a gpio framework, but that will take a while I think ...
> Hmm.. maybe we use my proposal for such a soft_i2c.h, so I think, it is
> okay for such a board specific gpio (unless we have a gpio framework).
>
> bye
> Heiko

I tried your patch on AT91SAM9263EK based OTC-570 board (patch will come to ML
within next merge window). It worked fine on it.
But some boards (the OTC-570 too) need a i2c_soft_init() function. So I extended
your patch as follows:

[PATCH] i2c, soft: added soft_i2c.h

In case you must define functions for the I2C_XXX
defines, it is necessary to have a soft_i2c.h, which
defines functions for this macros. This functions can
then be programmed in board specific code. To activate
this it must be CONFIG_I2C_SOFT_INCLUDE defined in the
board config file.

Signed-off-by: Heiko Schocher <hs@denx.de>
---
 drivers/i2c/soft_i2c.c |    3 +++
 include/soft_i2c.h     |   20 ++++++++++++++++++++
 2 files changed, 23 insertions(+), 0 deletions(-)
 create mode 100644 include/soft_i2c.h

diff --git a/drivers/i2c/soft_i2c.c b/drivers/i2c/soft_i2c.c
index 59883a5..30e24f3 100644
--- a/drivers/i2c/soft_i2c.c
+++ b/drivers/i2c/soft_i2c.c
@@ -43,6 +43,9 @@
 #if defined(CONFIG_MPC852T) || defined(CONFIG_MPC866)
 #include <asm/io.h>
 #endif
+#if defined(CONFIG_SOFT_I2C_INCLUDE)
+#include <soft_i2c.h>
+#endif
 #include <i2c.h>

 /* #define	DEBUG_I2C	*/
diff --git a/include/soft_i2c.h b/include/soft_i2c.h
new file mode 100644
index 0000000..c1b770d
--- /dev/null
+++ b/include/soft_i2c.h
@@ -0,0 +1,20 @@
+#ifndef _CONFIG_SOFT_I2C
+#define _CONFIG_SOFT_I2C
+#ifdef CONFIG_SOFT_I2C_INIT
+#define I2C_INIT	i2c_soft_init()
+#endif
+#define I2C_ACTIVE	i2c_soft_active()
+#define I2C_TRISTATE	i2c_soft_tristate()
+#define I2C_READ	i2c_soft_read()
+#define I2C_SDA(bit)	i2c_soft_sda(bit)
+#define I2C_SCL(bit)	i2c_soft_scl(bit)
+#define	I2C_DELAY	i2c_soft_delay()
+
+void	i2c_soft_init(void);
+void	i2c_soft_active(void);
+void	i2c_soft_tristate(void);
+int	i2c_soft_read(void);
+void	i2c_soft_sda(int value);
+void	i2c_soft_scl(int value);
+void	i2c_soft_delay(void);
+#endif
-- 
1.5.3

Moreover I removed the semicolons at the end of the #define lines,
they are not needed.
Maybe you apply these changes to your I2C tree?
(Or is there a better solution in the meantime?)

Bye
Daniel

[U-Boot] [PATCH 0/2] arm nomadik: gpio and i2c

[Jean-Christophe PLAGNIOL-VILLARD]

> 
> I tried your patch on AT91SAM9263EK based OTC-570 board (patch will come to ML
> within next merge window). It worked fine on it.
> But some boards (the OTC-570 too) need a i2c_soft_init() function. So I extended
> your patch as follows:
> 
> [PATCH] i2c, soft: added soft_i2c.h
> 
> In case you must define functions for the I2C_XXX
> defines, it is necessary to have a soft_i2c.h, which
> defines functions for this macros. This functions can
> then be programmed in board specific code. To activate
> this it must be CONFIG_I2C_SOFT_INCLUDE defined in the
> board config file.
> 
> Signed-off-by: Heiko Schocher <hs@denx.de>
> ---
Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>

Best Regards,
J.

[U-Boot] [PATCH 0/2] arm nomadik: gpio and i2c

[Heiko Schocher]

Hello Daniel,

Daniel Gorsulowski wrote:
> Heiko Schocher wrote:
>> Hello Alessandro,
>>
>> Alessandro Rubini wrote:
>>> This adds gpio and i2c support for the Nomadik evaluation kit. They
>>> are needed to turn on the LCD backlight in order to later add LCD
>>> support.
>>>
>>> I have one doubt and some questions on gpio:
>>>
>>> To use soft_i2c I need to define some macros in the config file.
>>> Instead of writing hard numbers there I called the gpio functions, but
>>> the config file is inluded from asm sources as well.  I don't think my
>>> approach is beautiful at all (both #ifndef __ASSEMBLY__ and #include
>>> "../board/"), but I didn't find a better solution.
>> Yes, thats a problem ... if we had a GPIO Framework it would be solved
>> by including gpio.h ...
>>
>> Or, maybe, we can make a soft_i2c.h which which gets only included
>> if saying CONFIG_I2C_SOFT_INCLUDE is defined. soft_i2c.h defines for
>> example:
>>
>> #define I2C_SDA(x)	i2c_soft_sda(bit)
>>
>> void i2c_soft_sda(int pin);
>>
>> and you can define this function i2c_soft_sda(int pin) in your board
>> specific code ... maybe a cleaner option?
>>
>> to speak in c, I tried the following patch on the suen3 plattform,
>> where I have actually a similiar problem, and this worked fine :-)
>>
>> [PATCH] i2c, soft: added soft_i2c.h
>>
>> In case you must define functions for the I2C_XXX
>> defines, it is necessary to have a soft_i2c.h, which
>> defines functions for this macros. This functions can
>> then be programmed in board specific code. To activate
>> this it must be CONFIG_I2C_SOFT_INCLUDE defined in the
>> board config file.
>>
>> Signed-off-by: Heiko Schocher <hs@denx.de>
>> ---
>>  drivers/i2c/soft_i2c.c |    3 +++
>>  include/soft_i2c.h     |   16 ++++++++++++++++
>>  2 files changed, 19 insertions(+), 0 deletions(-)
>>  create mode 100644 include/soft_i2c.h
[...]
>> -- 1.6.0.GIT Maybe you can try it too?
>>> I would like to add a gpio command, and I've found no generic gpio
>>> stuff.  Only one board (cm-bf527) has a gpio commands, but quite a few
>>> have similar commands to set leds or other bits.  Is time ripe for a
>>> generic gpio driver with board-specific limits and operations? Would
>>> that be interesting for u-boot-next?  Should I process with a board-specific
>>> gpio command by now?
>> I vote for making a gpio framework, but that will take a while I think ...
>> Hmm.. maybe we use my proposal for such a soft_i2c.h, so I think, it is
>> okay for such a board specific gpio (unless we have a gpio framework).
>>
>> bye
>> Heiko
> 
> I tried your patch on AT91SAM9263EK based OTC-570 board (patch will come to ML
> within next merge window). It worked fine on it.
> But some boards (the OTC-570 too) need a i2c_soft_init() function. So I extended
> your patch as follows:
[...]

OK, thanks, but this patch was just a fast thought, and
as I wrote, I think, Wolfgang will not Ack this patch ...
Wolfgang?

bye
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH 0/2] arm nomadik: gpio and i2c

[Wolfgang Denk]

Dear Heiko,

in message <4A6ED200.3070703@denx.de> you wrote:
> 
> OK, thanks, but this patch was just a fast thought, and
> as I wrote, I think, Wolfgang will not Ack this patch ...
> Wolfgang?

Correct. If we are going to rework the Soft-I2C implementation, this
should be done for all boards (which is a pretty big action), and
probably should be done as part of the "i2c: rework multibus/multi-
adapter functionality" changes.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"It is better to have tried and failed than to have  failed  to  try,
but the result's the same."                           - Mike Dennison

[U-Boot] [PATCH 0/2] arm nomadik: gpio and i2c

[Heiko Schocher]

Hello Wolfgang,

Wolfgang Denk wrote:
> in message <4A6ED200.3070703@denx.de> you wrote:
>> OK, thanks, but this patch was just a fast thought, and
>> as I wrote, I think, Wolfgang will not Ack this patch ...
>> Wolfgang?
> 
> Correct. If we are going to rework the Soft-I2C implementation, this
> should be done for all boards (which is a pretty big action), and
> probably should be done as part of the "i2c: rework multibus/multi-
> adapter functionality" changes.

Hmm... I don;t understand why we must rework all I2C Soft implementations.
Just boards, who must/want call functions instead of direct using
the I2C_XXX macros, can use this patch ... existing boards are
work as they are ... but as we discussed this, this patch is
not a need, because we can do this functionality also at the
moment (of course with this unlovley include in the config file)
If this is in future a no go (as Jean-Christophe suggested for the
Kconfig support), we can then maybe switch to this proposal ...

Hmm... also I don;t want to do this with the "multibus/multiadapter"
rework, because this rework is for themselve a big change, which
where I wouldn;t add one more big change ... so we maybe lost control,
why things no longer work ...

bye
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] [PATCH 0/2] arm nomadik: gpio and i2c

[Wolfgang Denk]

Dear Heiko Schocher,

In message <4A6EF6E4.2060407@denx.de> you wrote:
> 
> > Correct. If we are going to rework the Soft-I2C implementation, this
> > should be done for all boards (which is a pretty big action), and
> > probably should be done as part of the "i2c: rework multibus/multi-
> > adapter functionality" changes.
> 
> Hmm... I don;t understand why we must rework all I2C Soft implementations.
> Just boards, who must/want call functions instead of direct using
> the I2C_XXX macros, can use this patch ... existing boards are

Maybe I miss the point - but what is the problem with "direct using
the I2C_XXX macros"? You can always #define the I2C_XXX macros sucht
hat they indeed call the required functions, right? I really don;t get
the point.

> work as they are ... but as we discussed this, this patch is
> not a need, because we can do this functionality also at the
> moment (of course with this unlovley include in the config file)

What's the difference between include one file or another? I see no
real difference between including <asm/gpio.h> versus <soft_i2c.h>.

> If this is in future a no go (as Jean-Christophe suggested for the
> Kconfig support), we can then maybe switch to this proposal ...

Well, we should not assume that  we  can  use  Kconfig  to  sole  all
configuration  issues.  This  is  IMO  not  possible  with reasonable
efforts and within a reasonable time frame. Let's be  happy  when  we
cover 95 or even 90%, and then focus on the rest. But don't lets make
(eventually  false)  assumptions  which  constructs  may  or  may not
conflict with a not even yet existing implementation.

You know - early optimizian has never been a good idea.

> Hmm... also I don;t want to do this with the "multibus/multiadapter"
> rework, because this rework is for themselve a big change, which
> where I wouldn;t add one more big change ... so we maybe lost control,
> why things no longer work ...

Heh. Clever, you didn't swallow the bait ;-)

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
If all you have is a hammer, everything looks like a nail.

[U-Boot] [PATCH 0/2] arm nomadik: gpio and i2c

[Heiko Schocher]

Hello Wolfgang,

Wolfgang Denk wrote:
> In message <4A6EF6E4.2060407@denx.de> you wrote:
>>> Correct. If we are going to rework the Soft-I2C implementation, this
>>> should be done for all boards (which is a pretty big action), and
>>> probably should be done as part of the "i2c: rework multibus/multi-
>>> adapter functionality" changes.
>> Hmm... I don;t understand why we must rework all I2C Soft implementations.
>> Just boards, who must/want call functions instead of direct using
>> the I2C_XXX macros, can use this patch ... existing boards are
> 
> Maybe I miss the point - but what is the problem with "direct using
> the I2C_XXX macros"? You can always #define the I2C_XXX macros sucht
> hat they indeed call the required functions, right? I really don;t get
> the point.

The problem is actual (if I see this right ;-) ), that if someone
wants to call a function, this function is somewhere defined, and
the soft_i2c driver didn;t know how this function look like ... and my
patch just defines an "API", so that not everybody comes with new
names and new includes ... And this include sits no longer in the
config file but in the soft_i2c driver ... thats it.
I just did this as a fast proposal for what Alessandro suggested:
"I don't think my approach is beautiful at all (both #ifndef __ASSEMBLY__
and #include "../board/"), but I didn't find a better solution"
see:

http://lists.denx.de/pipermail/u-boot/2009-July/056867.html

So, I thought, to prevent such includes in a lot of config files,
and a lot of new function names for the same thing, it would be
nice to have such an "API" ...

>> work as they are ... but as we discussed this, this patch is
>> not a need, because we can do this functionality also at the
>> moment (of course with this unlovley include in the config file)
> 
> What's the difference between include one file or another? I see no
> real difference between including <asm/gpio.h> versus <soft_i2c.h>.

This is true if we have gpio api! So we just include the gpio.h
in soft_i2c.c and it works, but we have not such an API yet ...

>> If this is in future a no go (as Jean-Christophe suggested for the
>> Kconfig support), we can then maybe switch to this proposal ...
> 
> Well, we should not assume that  we  can  use  Kconfig  to  sole  all
> configuration  issues.  This  is  IMO  not  possible  with reasonable
> efforts and within a reasonable time frame. Let's be  happy  when  we
> cover 95 or even 90%, and then focus on the rest. But don't lets make
> (eventually  false)  assumptions  which  constructs  may  or  may not
> conflict with a not even yet existing implementation.
> 
> You know - early optimizian has never been a good idea.
> 
>> Hmm... also I don;t want to do this with the "multibus/multiadapter"
>> rework, because this rework is for themselve a big change, which
>> where I wouldn;t add one more big change ... so we maybe lost control,
>> why things no longer work ...
> 
> Heh. Clever, you didn't swallow the bait ;-)

:-)

bye
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany