[U-Boot] [PATCH v3] i2c: merge all i2c_reg_read() and i2c_reg_write() into inline functions

[U-Boot] [PATCH v3] i2c: merge all i2c_reg_read() and i2c_reg_write() into inline functions

[Timur Tabi]

All implementations of the functions i2c_reg_read() and i2c_reg_write() are
identical.  We can save space and simplify the code by converting these
functions into inlines and putting them in i2c.h.

Signed-off-by: Timur Tabi <timur@freescale.com>
---

v3: add 8xx-specific code, and include PRINTDs from blackfin and pxa

I'm posting this patch because I'm enhancing the I2C routines to support
multiple I2C busses more easily, but I need to clean up the existing code
first.

I'm going to be on vacation when the next merge window opens, so I'm posting
this now in the hopes that it will be picked up when the window does open.

 cpu/arm920t/at91rm9200/i2c.c  |   14 ---------
 cpu/arm926ejs/davinci/i2c.c   |   17 -----------
 cpu/blackfin/i2c.c            |   16 ----------
 cpu/mpc512x/i2c.c             |   17 -----------
 cpu/mpc5xxx/i2c.c             |   16 ----------
 cpu/mpc8220/i2c.c             |   16 ----------
 cpu/mpc824x/drivers/i2c/i2c.c |   14 ---------
 cpu/mpc8260/i2c.c             |   16 ----------
 cpu/mpc8xx/i2c.c              |   33 ---------------------
 cpu/ppc4xx/i2c.c              |   20 -------------
 cpu/pxa/i2c.c                 |   15 ----------
 drivers/i2c/fsl_i2c.c         |   16 ----------
 drivers/i2c/soft_i2c.c        |   19 ------------
 include/i2c.h                 |   62 +++++++++++++++++++++++++++++++++++++++-
 14 files changed, 60 insertions(+), 231 deletions(-)

diff --git a/cpu/arm920t/at91rm9200/i2c.c b/cpu/arm920t/at91rm9200/i2c.c
index b68c5dd..9fd72d3 100644
--- a/cpu/arm920t/at91rm9200/i2c.c
+++ b/cpu/arm920t/at91rm9200/i2c.c
@@ -189,20 +189,6 @@ i2c_init(int speed, int slaveaddr)
 	return;
 }
 
-uchar i2c_reg_read(uchar i2c_addr, uchar reg)
-{
-	unsigned char buf;
-
-	i2c_read(i2c_addr, reg, 1, &buf, 1);
-
-	return(buf);
-}
-
-void i2c_reg_write(uchar i2c_addr, uchar reg, uchar val)
-{
-	i2c_write(i2c_addr, reg, 1, &val, 1);
-}
-
 int i2c_set_bus_speed(unsigned int speed)
 {
 	return -1;
diff --git a/cpu/arm926ejs/davinci/i2c.c b/cpu/arm926ejs/davinci/i2c.c
index d220a4c..3ba20ef 100644
--- a/cpu/arm926ejs/davinci/i2c.c
+++ b/cpu/arm926ejs/davinci/i2c.c
@@ -331,21 +331,4 @@ int i2c_write(u_int8_t chip, u_int32_t addr, int alen, u_int8_t *buf, int len)
 	return(0);
 }
 
-
-u_int8_t i2c_reg_read(u_int8_t chip, u_int8_t reg)
-{
-	u_int8_t	tmp;
-
-	i2c_read(chip, reg, 1, &tmp, 1);
-	return(tmp);
-}
-
-
-void i2c_reg_write(u_int8_t chip, u_int8_t reg, u_int8_t val)
-{
-	u_int8_t	tmp;
-
-	i2c_write(chip, reg, 1, &tmp, 1);
-}
-
 #endif /* CONFIG_DRIVER_DAVINCI_I2C */
diff --git a/cpu/blackfin/i2c.c b/cpu/blackfin/i2c.c
index 60f03d4..2a3e223 100644
--- a/cpu/blackfin/i2c.c
+++ b/cpu/blackfin/i2c.c
@@ -425,20 +425,4 @@ int i2c_write(uchar chip, uint addr, int alen, uchar * buffer, int len)
 
 }
 
-uchar i2c_reg_read(uchar chip, uchar reg)
-{
-	uchar buf;
-
-	PRINTD("i2c_reg_read: chip=0x%02x, reg=0x%02x\n", chip, reg);
-	i2c_read(chip, reg, 0, &buf, 1);
-	return (buf);
-}
-
-void i2c_reg_write(uchar chip, uchar reg, uchar val)
-{
-	PRINTD("i2c_reg_write: chip=0x%02x, reg=0x%02x, val=0x%02x\n", chip,
-			reg, val);
-	i2c_write(chip, reg, 0, &val, 1);
-}
-
 #endif /* CONFIG_HARD_I2C */
diff --git a/cpu/mpc512x/i2c.c b/cpu/mpc512x/i2c.c
index 77a6f0d..4f6bc86 100644
--- a/cpu/mpc512x/i2c.c
+++ b/cpu/mpc512x/i2c.c
@@ -382,23 +382,6 @@ Done:
 	return ret;
 }
 
-uchar i2c_reg_read (uchar chip, uchar reg)
-{
-	uchar buf;
-
-	i2c_read (chip, reg, 1, &buf, 1);
-
-	return buf;
-}
-
-void i2c_reg_write (uchar chip, uchar reg, uchar val)
-{
-	i2c_write (chip, reg, 1, &val, 1);
-
-	return;
-}
-
-
 int i2c_set_bus_num (unsigned int bus)
 {
 	if (bus >= I2C_BUS_CNT) {
diff --git a/cpu/mpc5xxx/i2c.c b/cpu/mpc5xxx/i2c.c
index 4d16bbe..7d76274 100644
--- a/cpu/mpc5xxx/i2c.c
+++ b/cpu/mpc5xxx/i2c.c
@@ -380,20 +380,4 @@ Done:
 	return ret;
 }
 
-uchar i2c_reg_read(uchar chip, uchar reg)
-{
-	uchar buf;
-
-	i2c_read(chip, reg, 1, &buf, 1);
-
-	return buf;
-}
-
-void i2c_reg_write(uchar chip, uchar reg, uchar val)
-{
-	i2c_write(chip, reg, 1, &val, 1);
-
-	return;
-}
-
 #endif	/* CONFIG_HARD_I2C */
diff --git a/cpu/mpc8220/i2c.c b/cpu/mpc8220/i2c.c
index d67936d..76ecdf1 100644
--- a/cpu/mpc8220/i2c.c
+++ b/cpu/mpc8220/i2c.c
@@ -387,20 +387,4 @@ int i2c_write (uchar chip, uint addr, int alen, uchar * buf, int len)
 	return ret;
 }
 
-uchar i2c_reg_read (uchar chip, uchar reg)
-{
-	uchar buf;
-
-	i2c_read (chip, reg, 1, &buf, 1);
-
-	return buf;
-}
-
-void i2c_reg_write (uchar chip, uchar reg, uchar val)
-{
-	i2c_write (chip, reg, 1, &val, 1);
-
-	return;
-}
-
 #endif /* CONFIG_HARD_I2C */
diff --git a/cpu/mpc824x/drivers/i2c/i2c.c b/cpu/mpc824x/drivers/i2c/i2c.c
index 854345e..637ae4c 100644
--- a/cpu/mpc824x/drivers/i2c/i2c.c
+++ b/cpu/mpc824x/drivers/i2c/i2c.c
@@ -267,18 +267,4 @@ int i2c_probe (uchar chip)
 	return i2c_read (chip, 0, 1, (uchar *) &tmp, 1);
 }
 
-uchar i2c_reg_read (uchar i2c_addr, uchar reg)
-{
-	uchar buf[1];
-
-	i2c_read (i2c_addr, reg, 1, buf, 1);
-
-	return (buf[0]);
-}
-
-void i2c_reg_write (uchar i2c_addr, uchar reg, uchar val)
-{
-	i2c_write (i2c_addr, reg, 1, &val, 1);
-}
-
 #endif /* CONFIG_HARD_I2C */
diff --git a/cpu/mpc8260/i2c.c b/cpu/mpc8260/i2c.c
index a934193..68c37e6 100644
--- a/cpu/mpc8260/i2c.c
+++ b/cpu/mpc8260/i2c.c
@@ -753,22 +753,6 @@ i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len)
 	return 0;
 }
 
-uchar
-i2c_reg_read(uchar chip, uchar reg)
-{
-	uchar buf;
-
-	i2c_read(chip, reg, 1, &buf, 1);
-
-	return (buf);
-}
-
-void
-i2c_reg_write(uchar chip, uchar reg, uchar val)
-{
-	i2c_write(chip, reg, 1, &val, 1);
-}
-
 #if defined(CONFIG_I2C_MULTI_BUS)
 /*
  * Functions for multiple I2C bus handling
diff --git a/cpu/mpc8xx/i2c.c b/cpu/mpc8xx/i2c.c
index 29c7c71..338caba 100644
--- a/cpu/mpc8xx/i2c.c
+++ b/cpu/mpc8xx/i2c.c
@@ -42,19 +42,6 @@ DECLARE_GLOBAL_DATA_PTR;
 /* define to enable debug messages */
 #undef	DEBUG_I2C
 
-/*-----------------------------------------------------------------------
- * Set default values
- */
-#ifndef	CONFIG_SYS_I2C_SPEED
-#define	CONFIG_SYS_I2C_SPEED	50000
-#endif
-
-#ifndef	CONFIG_SYS_I2C_SLAVE
-#define	CONFIG_SYS_I2C_SLAVE	0xFE
-#endif
-/*-----------------------------------------------------------------------
- */
-
 /* tx/rx timeout (we need the i2c early, so we don't use get_timer()) */
 #define TOUT_LOOP 1000000
 
@@ -717,24 +704,4 @@ int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len)
 	return 0;
 }
 
-uchar
-i2c_reg_read(uchar i2c_addr, uchar reg)
-{
-	uchar buf;
-
-	i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE);
-
-	i2c_read(i2c_addr, reg, 1, &buf, 1);
-
-	return (buf);
-}
-
-void
-i2c_reg_write(uchar i2c_addr, uchar reg, uchar val)
-{
-	i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE);
-
-	i2c_write(i2c_addr, reg, 1, &val, 1);
-}
-
 #endif	/* CONFIG_HARD_I2C */
diff --git a/cpu/ppc4xx/i2c.c b/cpu/ppc4xx/i2c.c
index 0deb149..4456f3f 100644
--- a/cpu/ppc4xx/i2c.c
+++ b/cpu/ppc4xx/i2c.c
@@ -420,26 +420,6 @@ int i2c_write(uchar chip, uint addr, int alen, uchar * buffer, int len)
 	return (i2c_transfer(0, chip<<1, &xaddr[4-alen], alen, buffer, len ) != 0);
 }
 
-/*-----------------------------------------------------------------------
- * Read a register
- */
-uchar i2c_reg_read(uchar i2c_addr, uchar reg)
-{
-	uchar buf;
-
-	i2c_read(i2c_addr, reg, 1, &buf, 1);
-
-	return (buf);
-}
-
-/*-----------------------------------------------------------------------
- * Write a register
- */
-void i2c_reg_write(uchar i2c_addr, uchar reg, uchar val)
-{
-	i2c_write(i2c_addr, reg, 1, &val, 1);
-}
-
 #if defined(CONFIG_I2C_MULTI_BUS)
 /*
  * Functions for multiple I2C bus handling
diff --git a/cpu/pxa/i2c.c b/cpu/pxa/i2c.c
index 08042be..6b72ba1 100644
--- a/cpu/pxa/i2c.c
+++ b/cpu/pxa/i2c.c
@@ -455,19 +455,4 @@ int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len)
 
 }
 
-uchar i2c_reg_read (uchar chip, uchar reg)
-{
-	uchar buf;
-
-	PRINTD(("i2c_reg_read(chip=0x%02x, reg=0x%02x)\n",chip,reg));
-	i2c_read(chip, reg, 1, &buf, 1);
-	return (buf);
-}
-
-void  i2c_reg_write(uchar chip, uchar reg, uchar val)
-{
-	PRINTD(("i2c_reg_write(chip=0x%02x, reg=0x%02x, val=0x%02x)\n",chip,reg,val));
-	i2c_write(chip, reg, 1, &val, 1);
-}
-
 #endif	/* CONFIG_HARD_I2C */
diff --git a/drivers/i2c/fsl_i2c.c b/drivers/i2c/fsl_i2c.c
index 281a88b..79fdc6d 100644
--- a/drivers/i2c/fsl_i2c.c
+++ b/drivers/i2c/fsl_i2c.c
@@ -368,22 +368,6 @@ i2c_probe(uchar chip)
 	return i2c_read(chip, 0, 0, NULL, 0);
 }
 
-uchar
-i2c_reg_read(uchar i2c_addr, uchar reg)
-{
-	uchar buf[1];
-
-	i2c_read(i2c_addr, reg, 1, buf, 1);
-
-	return buf[0];
-}
-
-void
-i2c_reg_write(uchar i2c_addr, uchar reg, uchar val)
-{
-	i2c_write(i2c_addr, reg, 1, &val, 1);
-}
-
 int i2c_set_bus_num(unsigned int bus)
 {
 #ifdef CONFIG_SYS_I2C2_OFFSET
diff --git a/drivers/i2c/soft_i2c.c b/drivers/i2c/soft_i2c.c
index ebe60e2..a863348 100644
--- a/drivers/i2c/soft_i2c.c
+++ b/drivers/i2c/soft_i2c.c
@@ -435,22 +435,3 @@ int  i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len)
 	return(failures);
 }
 
-/*-----------------------------------------------------------------------
- * Read a register
- */
-uchar i2c_reg_read(uchar i2c_addr, uchar reg)
-{
-	uchar buf;
-
-	i2c_read(i2c_addr, reg, 1, &buf, 1);
-
-	return(buf);
-}
-
-/*-----------------------------------------------------------------------
- * Write a register
- */
-void i2c_reg_write(uchar i2c_addr, uchar reg, uchar val)
-{
-	i2c_write(i2c_addr, reg, 1, &val, 1);
-}
diff --git a/include/i2c.h b/include/i2c.h
index 8d6f867..fad2d57 100644
--- a/include/i2c.h
+++ b/include/i2c.h
@@ -76,6 +76,20 @@
 #  define I2C_SOFT_DECLARATIONS
 # endif
 #endif
+
+#ifdef CONFIG_8xx
+/* Set default values for the I2C bus speed and slave address on 8xx. In the
+ * future, we'll define these in all 8xx board config files.
+ */
+#ifndef	CONFIG_SYS_I2C_SPEED
+#define	CONFIG_SYS_I2C_SPEED	50000
+#endif
+
+#ifndef	CONFIG_SYS_I2C_SLAVE
+#define	CONFIG_SYS_I2C_SLAVE	0xFE
+#endif
+#endif
+
 /*
  * Initialization, must be called once on start up, may be called
  * repeatedly to change the speed and slave addresses.
@@ -132,8 +146,52 @@ int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len);
 /*
  * Utility routines to read/write registers.
  */
-uchar i2c_reg_read (uchar chip, uchar reg);
-void  i2c_reg_write(uchar chip, uchar reg, uchar val);
+static inline u8 i2c_reg_read(u8 addr, u8 reg)
+{
+	u8 buf;
+
+#ifdef CONFIG_8xx
+	/* MPC8xx needs this.  Maybe one day we can get rid of it. */
+	i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE);
+#endif
+
+#ifdef DEBUG
+	printf("%s: addr=0x%02x, reg=0x%02x\n", __func__, addr, reg);
+#endif
+
+#ifdef CONFIG_BLACKFIN
+	/* This ifdef will become unneccessary in a future version of the
+	 * blackfin I2C driver.
+	 */
+	i2c_read(addr, reg, 0, &buf, 1);
+#else
+	i2c_read(addr, reg, 1, &buf, 1);
+#endif
+
+	return buf;
+}
+
+static inline void i2c_reg_write(u8 addr, u8 reg, u8 val)
+{
+#ifdef CONFIG_8xx
+	/* MPC8xx needs this.  Maybe one day we can get rid of it. */
+	i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE);
+#endif
+
+#ifdef DEBUG
+	printf("%s: addr=0x%02x, reg=0x%02x, val=0x%02x\n",
+	       __func__, addr, reg, val);
+#endif
+
+#ifdef CONFIG_BLACKFIN
+	/* This ifdef will become unneccessary in a future version of the
+	 * blackfin I2C driver.
+	 */
+	i2c_write(addr, reg, 0, &val, 1);
+#else
+	i2c_write(addr, reg, 1, &val, 1);
+#endif
+}
 
 /*
  * Functions for setting the current I2C bus and its speed
-- 
1.5.5

[U-Boot] [PATCH v3] i2c: merge all i2c_reg_read() and i2c_reg_write() into inline functions

[Jean-Christophe PLAGNIOL-VILLARD]

On 11:28 Wed 03 Dec     , Timur Tabi wrote:
> All implementations of the functions i2c_reg_read() and i2c_reg_write() are
> identical.  We can save space and simplify the code by converting these
> functions into inlines and putting them in i2c.h.
> 
> Signed-off-by: Timur Tabi <timur@freescale.com>
Ack-By: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>

Best Regards,
J.

[U-Boot] [PATCH v3] i2c: merge all i2c_reg_read() and i2c_reg_write() into inline functions

[Wolfgang Denk]

Dear Timur Tabi,

In message <1228325310-19275-1-git-send-email-timur@freescale.com> you wrote:
> All implementations of the functions i2c_reg_read() and i2c_reg_write() are
> identical.  We can save space and simplify the code by converting these
> functions into inlines and putting them in i2c.h.
> 
> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---
> 
> v3: add 8xx-specific code, and include PRINTDs from blackfin and pxa
> 
> I'm posting this patch because I'm enhancing the I2C routines to support
> multiple I2C busses more easily, but I need to clean up the existing code
> first.
> 
> I'm going to be on vacation when the next merge window opens, so I'm posting
> this now in the hopes that it will be picked up when the window does open.

We've been discussing this long enough when the previous window was
open, so ...

>  cpu/arm920t/at91rm9200/i2c.c  |   14 ---------
>  cpu/arm926ejs/davinci/i2c.c   |   17 -----------
>  cpu/blackfin/i2c.c            |   16 ----------
>  cpu/mpc512x/i2c.c             |   17 -----------
>  cpu/mpc5xxx/i2c.c             |   16 ----------
>  cpu/mpc8220/i2c.c             |   16 ----------
>  cpu/mpc824x/drivers/i2c/i2c.c |   14 ---------
>  cpu/mpc8260/i2c.c             |   16 ----------
>  cpu/mpc8xx/i2c.c              |   33 ---------------------
>  cpu/ppc4xx/i2c.c              |   20 -------------
>  cpu/pxa/i2c.c                 |   15 ----------
>  drivers/i2c/fsl_i2c.c         |   16 ----------
>  drivers/i2c/soft_i2c.c        |   19 ------------
>  include/i2c.h                 |   62 +++++++++++++++++++++++++++++++++++++++-
>  14 files changed, 60 insertions(+), 231 deletions(-)

Applied, thanks.

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
All these theories, diverse as they are, have two things  in  common:
they explain the observed facts, and they are completeley and utterly
wrong.                       - Terry Pratchett, _The Light Fantastic_

[U-Boot] [PATCH v3] i2c: merge all i2c_reg_read() and i2c_reg_write() into inline functions

[ksi at koi8.net]

On Mon, 15 Dec 2008, Wolfgang Denk wrote:

Are you going to implement support for multiple I2C busses on some boards?
I'm working on something like this now so it would be nice to coordinate our
efforts somehow...

There is a provision for using 2 I2C busses in fsl-i2c.c now but I'm trying
to make it more uniform to allow for different controllers on the same
board. I have the first prototype of our MPC8548E based motherboard in PCB
fab right now and I do have 3 I2C busses on it all of which are required for
U-Boot. That is 2 busses off of 2 MPC8548E I2C controllers and the third one
off of Silicon Motion SM502 MFD.

I have SM502 I2C driver written but not tested (no real hardware to test it
on yet, will be ready in something like a month) but the entire multibus
multicontroller I2C framework is not in U-Boot yet.

Are you working on something like this or just on moving i2c_reg_read() and
i2c_reg_write() functions to inlines?

> Dear Timur Tabi,
>
> In message <1228325310-19275-1-git-send-email-timur@freescale.com> you
> wrote:
>> All implementations of the functions i2c_reg_read() and
> i2c_reg_write() are
>> identical.  We can save space and simplify the code by converting
> these
>> functions into inlines and putting them in i2c.h.
>>
>> Signed-off-by: Timur Tabi <timur@freescale.com>
>> ---
>>
>> v3: add 8xx-specific code, and include PRINTDs from blackfin and pxa
>>
>> I'm posting this patch because I'm enhancing the I2C routines to
> support
>> multiple I2C busses more easily, but I need to clean up the existing
> code
>> first.
>>
>> I'm going to be on vacation when the next merge window opens, so I'm
> posting
>> this now in the hopes that it will be picked up when the window does
> open.
>
> We've been discussing this long enough when the previous window was
> open, so ...
>
>>  cpu/arm920t/at91rm9200/i2c.c  |   14 ---------
>>  cpu/arm926ejs/davinci/i2c.c   |   17 -----------
>>  cpu/blackfin/i2c.c            |   16 ----------
>>  cpu/mpc512x/i2c.c             |   17 -----------
>>  cpu/mpc5xxx/i2c.c             |   16 ----------
>>  cpu/mpc8220/i2c.c             |   16 ----------
>>  cpu/mpc824x/drivers/i2c/i2c.c |   14 ---------
>>  cpu/mpc8260/i2c.c             |   16 ----------
>>  cpu/mpc8xx/i2c.c              |   33 ---------------------
>>  cpu/ppc4xx/i2c.c              |   20 -------------
>>  cpu/pxa/i2c.c                 |   15 ----------
>>  drivers/i2c/fsl_i2c.c         |   16 ----------
>>  drivers/i2c/soft_i2c.c        |   19 ------------
>>  include/i2c.h                 |   62
> +++++++++++++++++++++++++++++++++++++++-
>>  14 files changed, 60 insertions(+), 231 deletions(-)
>
> Applied, thanks.
>
> 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
> All these theories, diverse as they are, have two things  in  common:
> they explain the observed facts, and they are completeley and utterly
> wrong.                       - Terry Pratchett, _The Light Fantastic_
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>

---
******************************************************************
*  KSI at home    KOI8 Net  < >  The impossible we do immediately.  *
*  Las Vegas   NV, USA   < >  Miracles require 24-hour notice.   *
******************************************************************

[U-Boot] [PATCH v3] i2c: merge all i2c_reg_read() and i2c_reg_write() into inline functions

[Timur Tabi]

ksi at koi8.net wrote:
> On Mon, 15 Dec 2008, Wolfgang Denk wrote:
> 
> Are you going to implement support for multiple I2C busses on some boards?
> I'm working on something like this now so it would be nice to coordinate our
> efforts somehow...

Yes, my goal is to add low-level i2c functions that take an i2c bus as a
parameter.  So instead of

	i2c_set_bus_num(1);
	i2c_read(...);

you can do

	i2c_read(1, ...)

or something like that.  My goal is to eliminate i2c_set_bus_num() and
everything related to it.

It sounds like we're working on something very similar.  This is very
low-priority for me, though.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH v3] i2c: merge all i2c_reg_read() and i2c_reg_write() into inline functions

[ksi at koi8.net]

On Mon, 15 Dec 2008, Timur Tabi wrote:

> ksi at koi8.net wrote:
>> On Mon, 15 Dec 2008, Wolfgang Denk wrote:
>>
>> Are you going to implement support for multiple I2C busses on some
> boards?
>> I'm working on something like this now so it would be nice to
> coordinate our
>> efforts somehow...
>
> Yes, my goal is to add low-level i2c functions that take an i2c bus as a
> parameter.  So instead of
>
> 	i2c_set_bus_num(1);
> 	i2c_read(...);
>
> you can do
>
> 	i2c_read(1, ...)
>
> or something like that.  My goal is to eliminate i2c_set_bus_num() and
> everything related to it.
>
> It sounds like we're working on something very similar.  This is very
> low-priority for me, though.

That looks similar. But why do you want to remove i2c_set_bus_num()? I think
it would be less work to keep it. This way you can leave 90% or so of
existing I2C code unchanged by setting bus number to 0 at init. In your case
you will have to find each and every call to i2c_read... and change it to
the new format that includes bus number in parameters list.

My idea is to have global bus number variable in a single place and a single
i2c_set_bus_num() that can be excluded for most boards with a single bus
with #ifdef...

Then, we could use some kind of array of I2C structures each containing
pointers to appropriate i2c-{read,write,probe,init}() functions with generic
i2c functions just calling those pointers using bus number as index into
that array.

That would allow for unlimited number of different adapters for any board.
Initial code for initializing such an array would have to go into each and
every $(BOARD).c board specific file.

We could even make some default #ifdef'd structure that would've included
automagically if something like CONFIG_I2C_MULTIBUS was not defined thus
making most of the boards (actually almost all of them) work without
modifications.

Please let me know what you think...

---
******************************************************************
*  KSI at home    KOI8 Net  < >  The impossible we do immediately.  *
*  Las Vegas   NV, USA   < >  Miracles require 24-hour notice.   *
******************************************************************

[U-Boot] [PATCH v3] i2c: merge all i2c_reg_read() and i2c_reg_write() into inline functions

[Timur Tabi]

ksi at koi8.net wrote:

> That looks similar. But why do you want to remove i2c_set_bus_num()? I think
> it would be less work to keep it. 

Perhaps, but it would be even better to get rid of it.  IMHO, it's a kludge.  It
was a hack added to allow existing I2C routines to function while adding minimal
support for multiple buses on those platforms that needed it.

> This way you can leave 90% or so of
> existing I2C code unchanged by setting bus number to 0 at init. 

I only intend on exporting the multiple-bus versions of the I2C function if
CONFIG_I2C_MULTI_BUS is defined.

> My idea is to have global bus number variable in a single place and a single
> i2c_set_bus_num() that can be excluded for most boards with a single bus
> with #ifdef...

We already have something like that.  A global variable is inconvenient because
every time you want to access the bus, you need to do something like this:

	bus = i2c_get_bus_num();
	i2c_set_bus_num(x);
	i2c_write(...)
	i2c_set_bus_num(bus);

We need to save/restore the current bus number, because the I2C command-line has
the concept of a

> Then, we could use some kind of array of I2C structures each containing
> pointers to appropriate i2c-{read,write,probe,init}() functions with generic
> i2c functions just calling those pointers using bus number as index into
> that array.

Sounds complicated.

> That would allow for unlimited number of different adapters for any board.

Ah, now this is something else entirely.  I don't think U-boot supports this at
all.  I think you're being too ambitious.  It's a noble idea, and I think U-boot
should support it, but I think we need to simplify the support for multiple
buses first.

> Initial code for initializing such an array would have to go into each and
> every $(BOARD).c board specific file.

Ugh.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH v3] i2c: merge all i2c_reg_read() and i2c_reg_write() into inline functions

[Scott Wood]

On Mon, Dec 15, 2008 at 04:24:26PM -0800, ksi at koi8.net wrote:
> Then, we could use some kind of array of I2C structures each containing
> pointers to appropriate i2c-{read,write,probe,init}() functions with generic
> i2c functions just calling those pointers using bus number as index into
> that array.

Why not just pass a pointer to the i2c device, rather than an index?

-Scott

[U-Boot] [PATCH v3] i2c: merge all i2c_reg_read() and i2c_reg_write() into inline functions

[ksi at koi8.net]

On Tue, 16 Dec 2008, Timur Tabi wrote:

> ksi at koi8.net wrote:
>
>> That looks similar. But why do you want to remove i2c_set_bus_num()? I
> think
>> it would be less work to keep it.
>
> Perhaps, but it would be even better to get rid of it.  IMHO, it's a
> kludge.  It
> was a hack added to allow existing I2C routines to function while adding
> minimal
> support for multiple buses on those platforms that needed it.

I have nothing against changing parameters list for i2c_read/write(), it's
just more work and less elegant.

>> This way you can leave 90% or so of
>> existing I2C code unchanged by setting bus number to 0 at init.
>
> I only intend on exporting the multiple-bus versions of the I2C function
> if
> CONFIG_I2C_MULTI_BUS is defined.

That looks messy... Why would we use two different versions if we can make
everything uniform?

>> My idea is to have global bus number variable in a single place and a
> single
>> i2c_set_bus_num() that can be excluded for most boards with a single
> bus
>> with #ifdef...
>
> We already have something like that.  A global variable is inconvenient
> because
> every time you want to access the bus, you need to do something like
> this:
>
> 	bus = i2c_get_bus_num();
> 	i2c_set_bus_num(x);
> 	i2c_write(...)
> 	i2c_set_bus_num(bus);
>
> We need to save/restore the current bus number, because the I2C
> command-line has
> the concept of a

Eh, you can just set bus number every time you're gonna do i2c read/write...
That i2c_get_bus_num() doesn't make any sence at all. Just set bus number
every time you access i2c device. U-boot is single-task so there is no other
process that can change it from under you and you do not save anything with
checking that bus number. I can't see any practical use for
i2c_get_bus_num() except for showing it with an interactive command. That
is, in turn, is also redundant -- one would not save any keystrokes by
typing something like "i2c bus" and not typing "i2c bus X" if that X is what
he wants. Just type the second command every time when in doubt and you
won't have to type the first one :)

Also, for all boards with a single I2C bus we can assume bus=0 so there is
no need for bus-related functions/commands at all...

>> Then, we could use some kind of array of I2C structures each
> containing
>> pointers to appropriate i2c-{read,write,probe,init}() functions with
> generic
>> i2c functions just calling those pointers using bus number as index
> into
>> that array.
>
> Sounds complicated.

What is complicated? It is something like 3-4 lines of code per I2C bus.
Look how e.g. NAND subsystem is initialized...

And, BTW, this initialization can even go into include/configs/$(BOARD).h.

>> That would allow for unlimited number of different adapters for any
> board.
>
> Ah, now this is something else entirely.  I don't think U-boot supports
> this at
> all.  I think you're being too ambitious.  It's a noble idea, and I
> think U-boot
> should support it, but I think we need to simplify the support for
> multiple
> buses first.

It ain't rocket science :) The reason why I'm doing this is exactly what you
said -- U-boot does NOT support it at all. And I do really need this and I
think I'm not the only one.

>> Initial code for initializing such an array would have to go into each
> and
>> every $(BOARD).c board specific file.
>
> Ugh.

Ah, that's not biggie. And I volunteer to do this and come up with patches.

---
******************************************************************
*  KSI at home    KOI8 Net  < >  The impossible we do immediately.  *
*  Las Vegas   NV, USA   < >  Miracles require 24-hour notice.   *
******************************************************************

[U-Boot] [PATCH v3] i2c: merge all i2c_reg_read() and i2c_reg_write() into inline functions

[ksi at koi8.net]

On Tue, 16 Dec 2008, Scott Wood wrote:

> On Mon, Dec 15, 2008 at 04:24:26PM -0800, ksi at koi8.net wrote:
>> Then, we could use some kind of array of I2C structures each
> containing
>> pointers to appropriate i2c-{read,write,probe,init}() functions with
> generic
>> i2c functions just calling those pointers using bus number as index
> into
>> that array.
>
> Why not just pass a pointer to the i2c device, rather than an index?

Because it is easier to change bus with i2c_set_bus() function and then use
regular i2c_{read,write,probe}() functions. We are NOT passing anything. One
global variable, bus_num or so, that is changed with i2c_set_bus() is used
as an index for any i2c_{read,write,probe}() function. Those functions are
just one line inlines calling appropriate pointers in i2c_func[bus_num]
area. This way one can still use regular e.g. i2c_write() that translates to
i2c_func[bus_num].i2c_write.

---
******************************************************************
*  KSI at home    KOI8 Net  < >  The impossible we do immediately.  *
*  Las Vegas   NV, USA   < >  Miracles require 24-hour notice.   *
******************************************************************

[U-Boot] [PATCH v3] i2c: merge all i2c_reg_read() and i2c_reg_write() into inline functions

[Timur Tabi]

ksi at koi8.net wrote:

> That looks messy... Why would we use two different versions if we can make
> everything uniform?

Because we already have something that makes it uniform, and it's broken.  The
idea of having a "current i2c bus" that needs to be set before read/write
operations can be performed is the broken part!

> Eh, you can just set bus number every time you're gonna do i2c read/write...

Not with the current i2c command line.  We would need another global variable in
the i2c command line code to store what IT thinks is the current bus.

> That i2c_get_bus_num() doesn't make any sence at all. Just set bus number
> every time you access i2c device. 

That's risky.  Sooner or later, you will want to know what the current bus
number is, at least for debugging or status purposes.

> U-boot is single-task so there is no other
> process that can change it from under you and you do not save anything with
> checking that bus number. 

Sounds to me like you haven't really looked at the U-Boot code.  There are
plenty of places where one function does I2C operations, then calls another
function that does its own.

I think all this boils down to one core disagreement we have: I think the idea
of a "current" i2c bus is a bad one.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH v3] i2c: merge all i2c_reg_read() and i2c_reg_write() into inline functions

[ksi at koi8.net]

On Tue, 16 Dec 2008, Timur Tabi wrote:

> ksi at koi8.net wrote:
>
>> That looks messy... Why would we use two different versions if we can
> make
>> everything uniform?
>
> Because we already have something that makes it uniform, and it's
> broken.  The
> idea of having a "current i2c bus" that needs to be set before
> read/write
> operations can be performed is the broken part!

Eh, we don't have any uniformity. That "uniformity" we do have is only for a
trivial case of a single I2C bus. Everything else is a bunch of SoC specific
hacks made differently fo different platforms. The fsl-i2c.c e.g. uses local
bus number variable.

>> Eh, you can just set bus number every time you're gonna do i2c
> read/write...
>
> Not with the current i2c command line.  We would need another global
> variable in
> the i2c command line code to store what IT thinks is the current bus.

That can be taken care of.

>> That i2c_get_bus_num() doesn't make any sence at all. Just set bus
> number
>> every time you access i2c device.
>
> That's risky.  Sooner or later, you will want to know what the current
> bus
> number is, at least for debugging or status purposes.

So what? Just use i2c_get_bus() for this... We can even add a string to
i2c_func structure for textual representation.

>> U-boot is single-task so there is no other
>> process that can change it from under you and you do not save anything
> with
>> checking that bus number.
>
> Sounds to me like you haven't really looked at the U-Boot code.  There
> are
> plenty of places where one function does I2C operations, then calls
> another
> function that does its own.

So what? Does it make it multitasking? Can that another function preempt the
first one and change bus number from under it?

> I think all this boils down to one core disagreement we have: I think
> the idea
> of a "current" i2c bus is a bad one.

I can see nothing wrong with that but I also have nothing against changing
all i2c_{read,write,probe}() functions to take bus number as an argument.

I offered 4 possible scenarios and additional parameter to i2c functions was
one of them. Wolfgang said that current bus approach looks better than
others and I agree with him. But it is not rocket science to use an
additional parameter either. The only thing is it MUST be consistent, i.e.
we should NOT have 2 different sets of functions based on CONFIG_MULTIBUS or
whatever. If we are to make this change ALL boards must be multibus with
majority of them having bus count of 1.

Does anybody else have something to say on this? I'm going to write code so
let's make some decision. I don't want this to end up as a company hack and
making it properly affects a lot of U-boot...

---
******************************************************************
*  KSI at home    KOI8 Net  < >  The impossible we do immediately.  *
*  Las Vegas   NV, USA   < >  Miracles require 24-hour notice.   *
******************************************************************

[U-Boot] [PATCH v3] i2c: merge all i2c_reg_read() and i2c_reg_write() into inline functions

[Jerry Van Baren]

ksi at koi8.net wrote:
> On Tue, 16 Dec 2008, Timur Tabi wrote:
> 
>> ksi at koi8.net wrote:
>>
>>> That looks messy... Why would we use two different versions if we can
>> make
>>> everything uniform?
>> Because we already have something that makes it uniform, and it's
>> broken.  The
>> idea of having a "current i2c bus" that needs to be set before
>> read/write
>> operations can be performed is the broken part!
> 
> Eh, we don't have any uniformity. That "uniformity" we do have is only for a
> trivial case of a single I2C bus. Everything else is a bunch of SoC specific
> hacks made differently fo different platforms. The fsl-i2c.c e.g. uses local
> bus number variable.

[snip]

>> I think all this boils down to one core disagreement we have: I think
>> the idea
>> of a "current" i2c bus is a bad one.
> 
> I can see nothing wrong with that but I also have nothing against changing
> all i2c_{read,write,probe}() functions to take bus number as an argument.
> 
> I offered 4 possible scenarios and additional parameter to i2c functions was
> one of them. Wolfgang said that current bus approach looks better than
> others and I agree with him. But it is not rocket science to use an
> additional parameter either. The only thing is it MUST be consistent, i.e.
> we should NOT have 2 different sets of functions based on CONFIG_MULTIBUS or
> whatever. If we are to make this change ALL boards must be multibus with
> majority of them having bus count of 1.
> 
> Does anybody else have something to say on this? I'm going to write code so
> let's make some decision. I don't want this to end up as a company hack and
> making it properly affects a lot of U-boot...

IMHO Sergey's approach is reasonable.  It is pragmatic in that it builds 
on and improves the existing method.  It only touches boards with 
multiple i2c buses.

The best is the enemy of the good. - Voltaire
   <http://www.bartleby.com/66/2/63002.html>

My two cents,
gvb

[U-Boot] [PATCH v3] i2c: merge all i2c_reg_read() and i2c_reg_write() into inline functions

[Wolfgang Denk]

Dear Timur,

In message <4947F8B4.8070804@freescale.com> you wrote:
> ksi at koi8.net wrote:
> 
> > That looks messy... Why would we use two different versions if we can make
> > everything uniform?
> 
> Because we already have something that makes it uniform, and it's broken.  The
> idea of having a "current i2c bus" that needs to be set before read/write
> operations can be performed is the broken part!

Hm... what exactly is broken with the concept of  having  a  "current
device"  or  a  "current  bus"?  We  use it elasewhere, too (like for
selection IDE or S-ATA devices and such), and so far I am  not  aware
of fundamental issues because of that.

You sound as if we were designing a system where tens of  independend
users  could  log in simultaneously and concurrently access different
devices on different busses.

But this  is  NOT  the  case.  We're  strictly  single  user,  single
threaded,  and  when one command is runnign we are certain that there
cannot be any interference from other I2C accesses.

> > Eh, you can just set bus number every time you're gonna do i2c read/write...
> 
> Not with the current i2c command line.  We would need another global variable in
> the i2c command line code to store what IT thinks is the current bus.

Not really. It is really sufficient to set the bus for each  command.
There  is  no  need  to  save and restore any previous state, because
accesses cannot be nested because of the way how U-Boot is designed.

> > That i2c_get_bus_num() doesn't make any sence at all. Just set bus number
> > every time you access i2c device. 
> 
> That's risky.  Sooner or later, you will want to know what the current bus
> number is, at least for debugging or status purposes.

The design has been discussed before, and I see little reason why we
should not throw all this away and come up with something new, more
complicated.

> Sounds to me like you haven't really looked at the U-Boot code.  There are
> plenty of places where one function does I2C operations, then calls another
> function that does its own.

Really? Where exactly does such a thing happen?

I tend to call this a bug that needs to be fixed, if there is ssuch
code.

> I think all this boils down to one core disagreement we have: I think the idea
> of a "current" i2c bus is a bad one.

This statement is probably correct: I don't share your point of  view
here, either.

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
Be wiser than other people if you can, but do not tell them so.
                                       -- Philip Earl of Chesterfield

[U-Boot] [PATCH v3] i2c: merge all i2c_reg_read() and i2c_reg_write() into inline functions

[Wolfgang Denk]

Dear ksi at koi8.net,

In message <Pine.LNX.4.64ksi.0812161102580.28667@home-gw.koi8.net> you wrote:
> 
> I offered 4 possible scenarios and additional parameter to i2c functions was
> one of them. Wolfgang said that current bus approach looks better than
> others and I agree with him. But it is not rocket science to use an

You and me agree on this. 

> additional parameter either. The only thing is it MUST be consistent, i.e.
> we should NOT have 2 different sets of functions based on CONFIG_MULTIBUS or

We agree on this, too.

> whatever. If we are to make this change ALL boards must be multibus with
> majority of them having bus count of 1.

I don't think this makes sense - it would  just  add  complexity  and
memory footprint without added benefit.

> Does anybody else have something to say on this? I'm going to write code so
> let's make some decision. I don't want this to end up as a company hack and
> making it properly affects a lot of U-boot...

I support your position, not Timur's.

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
We fight only when there is no other choice. We prefer  the  ways  of
peaceful contact.
	-- Kirk, "Spectre of the Gun", stardate 4385.3

[U-Boot] [PATCH v3] i2c: merge all i2c_reg_read() and i2c_reg_write() into inline functions

[Timur Tabi]

Wolfgang Denk wrote:

> Hm... what exactly is broken with the concept of  having  a  "current
> device"  or  a  "current  bus"?  We  use it elasewhere, too (like for
> selection IDE or S-ATA devices and such), and so far I am  not  aware
> of fundamental issues because of that.

I think it's a kludge because you have to set the current device before you 
can access it.  It seems ridiculous that you have to do this:

	i2c_set_bus_num(x)
	i2c_write(...)

when you could do this:

	i2c_write(x, ...)

>> Sounds to me like you haven't really looked at the U-Boot code.  There are
>> plenty of places where one function does I2C operations, then calls another
>> function that does its own.
>
> Really? Where exactly does such a thing happen?

Well, I can't find a real example, but here's something close.  Take a look at 
function pib_init() in mpc8568mds.c.  This function needs to talk to the 2nd 
I2C bus.  It has no idea who called it, so it has no idea what the current I2C 
bus is.  Therefore, it has to save and restore it.

Now consider the code that calls pib_init().  Technically, this code cannot 
know that pib_init() does I2C operations.  So it doesn't know that pib_init() 
could change the current I2C bus.  So it wouldn't know to save and restore 
what it needs.

This situation could happen anywhere.

> I tend to call this a bug that needs to be fixed, if there is ssuch
> code.

It's not a bug.  Function X does I2C operations, and function Y also does I2C 
operations, but on a different device.  If function X calls function Y, then Y 
needs to save and restore the current bus number.  You will never get rid of 
this requirement as long as you have the concept of a current I2C bus number.

So you're still going to need i2c_get_bus_num() and i2c_set_bus_num(), and 
you're still going to have code like this:

	bus = i2c_get_bus_num();
	i2c_set_bus_num(x);
	i2c_write(...)
	i2c_set_bus_num(bus);

> This statement is probably correct: I don't share your point of  view
> here, either.

In that case, I have no interest in working on this new I2C design.  You guys 
obviously have everything under control, so good luck.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

[U-Boot] [PATCH v3] i2c: merge all i2c_reg_read() and i2c_reg_write() into inline functions

[ksi at koi8.net]

On Tue, 16 Dec 2008, Timur Tabi wrote:

> Wolfgang Denk wrote:
>
>> Hm... what exactly is broken with the concept of  having  a  "current
>> device"  or  a  "current  bus"?  We  use it elasewhere, too (like for
>> selection IDE or S-ATA devices and such), and so far I am  not  aware
>> of fundamental issues because of that.
>
> I think it's a kludge because you have to set the current device before
> you 
> can access it.  It seems ridiculous that you have to do this:
>
> 	i2c_set_bus_num(x)
> 	i2c_write(...)
>
> when you could do this:
>
> 	i2c_write(x, ...)

Only if you have more than one I2C bus that 95% of supported boards don't
have (95% is a wild guess; in reality, methinks, it is even closer to 99%.)
That means you should only do this for less than 5% of existing boards.

Adding a parameter would require you to make changes for 100% of boards.

common/cmd_i2c.c is a single file, common for all platforms so it is a
special case.

>>> Sounds to me like you haven't really looked at the U-Boot code.
> There are
>>> plenty of places where one function does I2C operations, then calls 
>>> another
>>> function that does its own.
>> 
>> Really? Where exactly does such a thing happen?
>
> Well, I can't find a real example, but here's something close.  Take a
> look 
> at function pib_init() in mpc8568mds.c.  This function needs to talk to
> the 
> 2nd I2C bus.  It has no idea who called it, so it has no idea what the 
> current I2C bus is.  Therefore, it has to save and restore it.
>
> Now consider the code that calls pib_init().  Technically, this code
> cannot 
> know that pib_init() does I2C operations.  So it doesn't know that
> pib_init() 
> could change the current I2C bus.  So it wouldn't know to save and
> restore 
> what it needs.
>
> This situation could happen anywhere.

No, that is not an issue. That pib_init() does not happen out of a blue, it
is YOU who call it in $(BOARD).c file so you MUST know what you are doing.

>> I tend to call this a bug that needs to be fixed, if there is ssuch
>> code.
>
> It's not a bug.  Function X does I2C operations, and function Y also
> does I2C 
> operations, but on a different device.  If function X calls function Y,
> then 
> Y needs to save and restore the current bus number.  You will never get
> rid 
> of this requirement as long as you have the concept of a current I2C bus
> number.
>
> So you're still going to need i2c_get_bus_num() and i2c_set_bus_num(),
> and 
> you're still going to have code like this:
>
> 	bus = i2c_get_bus_num();
> 	i2c_set_bus_num(x);
> 	i2c_write(...)
> 	i2c_set_bus_num(bus);
>
>> This statement is probably correct: I don't share your point of  view
>> here, either.
>
> In that case, I have no interest in working on this new I2C design.  You
> guys 
> obviously have everything under control, so good luck.
>
> -- 
> Timur Tabi
> Linux Kernel Developer @ Freescale
>

---
******************************************************************
*  KSI at home    KOI8 Net  < >  The impossible we do immediately.  *
*  Las Vegas   NV, USA   < >  Miracles require 24-hour notice.   *
******************************************************************

[U-Boot] [PATCH v3] i2c: merge all i2c_reg_read() and i2c_reg_write() into inline functions

[Wolfgang Denk]

Dear Timur,

In message <49483DD7.5080908@freescale.com> you wrote:
> 
> I think it's a kludge because you have to set the current device before you 
> can access it.  It seems ridiculous that you have to do this:
> 
> 	i2c_set_bus_num(x)
> 	i2c_write(...)
> 
> when you could do this:
> 
> 	i2c_write(x, ...)

That's your argument. The other side's argument is that it's overhead
(note that I do not use such a strong term  as  "ridiculous"  as  you
did) to add a bus number argument to each and every board, where only
a tiny fraction of them will ever have mnore than one I2C bus used.

So there is arguments bor both implementations, and it was decided to
implement the first approach.

We've been trhough this before, it makes little sense that yous tir
this up again.


> > Really? Where exactly does such a thing happen?
> 
> Well, I can't find a real example, but here's something close.  Take a look at 
> function pib_init() in mpc8568mds.c.  This function needs to talk to the 2nd 
> I2C bus.  It has no idea who called it, so it has no idea what the current I2C 
> bus is.  Therefore, it has to save and restore it.

IUt has to set it, yes. But there is no need to restore it, because
there are no nested I2C calls.

> This situation could happen anywhere.

It could, if you intentionally create such a case. In real life, it
never happens. Or can trivially be avoided.

> It's not a bug.  Function X does I2C operations, and function Y also does I2C 
> operations, but on a different device.  If function X calls function Y, then Y 
> needs to save and restore the current bus number.  You will never get rid of 
> this requirement as long as you have the concept of a current I2C bus number.

There are no examples for such code. And I cannot see where it would
be necessary.

> So you're still going to need i2c_get_bus_num() and i2c_set_bus_num(), and 
> you're still going to have code like this:
> 
> 	bus = i2c_get_bus_num();
> 	i2c_set_bus_num(x);
> 	i2c_write(...)
> 	i2c_set_bus_num(bus);

Nope. This is not necessary. 

I repeat: there are no nested I2C calls.

That's full in line with the design philosophy that we  are  strictly
single  tasking. There are no multiple tasks using the same driver or
bus.

> In that case, I have no interest in working on this new I2C design.  You guys 
> obviously have everything under control, so good luck.

C'me on. Don't act like a prima donna.

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
"Life sucks, but it's better than the alternative."
- Peter da Silva

[U-Boot] [PATCH v3] i2c: merge all i2c_reg_read() and i2c_reg_write() into inline functions

[ksi at koi8.net]

On Tue, 16 Dec 2008, Wolfgang Denk wrote:

> Dear ksi at koi8.net,
>
> In message <Pine.LNX.4.64ksi.0812161102580.28667@home-gw.koi8.net> you
> wrote:
>>
>> I offered 4 possible scenarios and additional parameter to i2c
> functions was
>> one of them. Wolfgang said that current bus approach looks better than
>> others and I agree with him. But it is not rocket science to use an
>
> You and me agree on this.
>
>> additional parameter either. The only thing is it MUST be consistent,
> i.e.
>> we should NOT have 2 different sets of functions based on
> CONFIG_MULTIBUS or
>
> We agree on this, too.
>
>> whatever. If we are to make this change ALL boards must be multibus
> with
>> majority of them having bus count of 1.
>
> I don't think this makes sense - it would  just  add  complexity  and
> memory footprint without added benefit.
>
>> Does anybody else have something to say on this? I'm going to write
> code so
>> let's make some decision. I don't want this to end up as a company
> hack and
>> making it properly affects a lot of U-boot...
>
> I support your position, not Timur's.

OK, so I'm on it. It might take a couple of weeks for patches to show up.

---
******************************************************************
*  KSI at home    KOI8 Net  < >  The impossible we do immediately.  *
*  Las Vegas   NV, USA   < >  Miracles require 24-hour notice.   *
******************************************************************