[U-Boot] [PATCH] i2c: merge all i2c_reg_read() and i2c_reg_write() into inline functions
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 ---
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.
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 | 20 -------------------- 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 | 15 +++++++++++++-- 14 files changed, 13 insertions(+), 218 deletions(-)
diff --git a/cpu/arm920t/at91rm9200/i2c.c b/cpu/arm920t/at91rm9200/i2c.c index 90f95df..fb4725d 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 af9dc03..1029b03 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 56ba443..95cc942 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 0f02e78..015256f 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 3add687..48d754e 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 c3af7b6..71dac3b 100644 --- a/cpu/mpc8260/i2c.c +++ b/cpu/mpc8260/i2c.c @@ -749,20 +749,4 @@ 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); -} - #endif /* CONFIG_HARD_I2C */ diff --git a/cpu/mpc8xx/i2c.c b/cpu/mpc8xx/i2c.c index f05b666..0a2fe95 100644 --- a/cpu/mpc8xx/i2c.c +++ b/cpu/mpc8xx/i2c.c @@ -717,24 +717,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(CFG_I2C_SPEED, CFG_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(CFG_I2C_SPEED, CFG_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 d8be2ce..39c3fef 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 df537c4..539ca25 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 264553d..a8ab0f0 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 CFG_I2C2_OFFSET diff --git a/drivers/i2c/soft_i2c.c b/drivers/i2c/soft_i2c.c index 23db2ee..4d7abdd 100644 --- a/drivers/i2c/soft_i2c.c +++ b/drivers/i2c/soft_i2c.c @@ -402,22 +402,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 a51c164..169e487 100644 --- a/include/i2c.h +++ b/include/i2c.h @@ -100,8 +100,19 @@ 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; + + i2c_read(addr, reg, 1, &buf, 1); + + return buf; +} + +static inline void i2c_reg_write(u8 addr, u8 reg, u8 val) +{ + i2c_write(addr, reg, 1, &val, 1); +}
/* * Functions for setting the current I2C bus and its speed
Wolfgang,
Who is the maintainer for I2C? I don't know who needs to ACK this patch for it to go in, and I don't know who's supposed to pick it up, either.
On Thu, Oct 2, 2008 at 10:06 AM, Timur Tabi timur@freescale.com 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
Dear Timur,
In message ed82fe3e0810031525n784bd609gf3d28b8e65992d1@mail.gmail.com you wrote:
Who is the maintainer for I2C? I don't know who needs to ACK this patch for it to go in, and I don't know who's supposed to pick it up, either.
In case there is no specific custodian, I will be the default.
Best regards,
Wolfgang Denk
Dear Timur Tabi,
In message 1222959963-5697-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.
Actually they are not identical:
+++ 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)
...
-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);
-}
This does not exactly look identical to me.
And when you claim "We can save space" - which sort of space are you talking about? Dos this reduce the memory footprint of the code?
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
-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);
-}
This does not exactly look identical to me.
The PRINTD is irrelevant. If these platforms really want to debug single I2C operations, they can add the code back. It's just debug code, so I would think that it's not worth sacrificing the improved code simplicity just for that.
And when you claim "We can save space" - which sort of space are you talking about? Dos this reduce the memory footprint of the code?
I ran a couple tests, and u-boot.bin didn't change in size, but technically, it should be smaller because the compiler will optimize the code into a normal call to i2c_read or i2c_write.
Either way, at least the source code is smaller.
Dear Timur,
In message 48E6A9D5.4050307@freescale.com you wrote:
The PRINTD is irrelevant. If these platforms really want to debug single I2C operations, they can add the code back. It's just debug code, so I would think that it's not worth sacrificing the improved code simplicity just for that.
This is your opinion. I think you cannot really speak for others. I think you should ask the respective maintainers of the code first before changing their code.
And when you claim "We can save space" - which sort of space are you talking about? Dos this reduce the memory footprint of the code?
I ran a couple tests, and u-boot.bin didn't change in size, but technically, it should be smaller because the compiler will optimize the code into a normal call to i2c_read or i2c_write.
Hm... why didn't the size change, then?
Best regards,
Wolfgang Denk
On Fri, Oct 3, 2008 at 6:39 PM, Wolfgang Denk wd@denx.de wrote:
This is your opinion. I think you cannot really speak for others. I think you should ask the respective maintainers of the code first before changing their code.
Ok.
Hm... why didn't the size change, then?
My guess: alignment.
On Fri, Oct 3, 2008 at 6:39 PM, Wolfgang Denk wd@denx.de wrote:
This is your opinion. I think you cannot really speak for others. I think you should ask the respective maintainers of the code first before changing their code.
I think the MAINTAINERS file might be out of date with respect to the blackfin and pxa boards. It lists Mike Frysinger and the blackfin maintainer and Cliff Brake as the pxa maintainer, but the mailing lists archives and the git repository show that these two individuals have not been involved in any patches for these platforms for the past six months.
So who are the maintainers for blackfin and pxa?
On Sunday 05 October 2008, Timur Tabi wrote:
On Fri, Oct 3, 2008 at 6:39 PM, Wolfgang Denk wd@denx.de wrote:
This is your opinion. I think you cannot really speak for others. I think you should ask the respective maintainers of the code first before changing their code.
I think the MAINTAINERS file might be out of date with respect to the blackfin and pxa boards. It lists Mike Frysinger and the blackfin maintainer and Cliff Brake as the pxa maintainer, but the mailing lists archives and the git repository show that these two individuals have not been involved in any patches for these platforms for the past six months.
So who are the maintainers for blackfin and pxa?
I'm pretty sure that Mike is still responsible for Blackfin. Not sure about Cliff.
BTW: I second your patch. So from me:
Acked-by: Stefan Roese sr@denx.de
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 =====================================================================
Mike and Cliff,
This I2C patch converts some I2C functions t inline, but in the case of blackfin and pxa, it removes the PRINTD() call as well. If you're okay with that, please ACK this patch.
You can find the original patch in the mailing list archive here: http://lists.denx.de/pipermail/u-boot/2008-October/041173.html
On Thu, Oct 2, 2008 at 10:06 AM, Timur Tabi timur@freescale.com 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
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.
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 | 20 -------------------- 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 | 15 +++++++++++++-- 14 files changed, 13 insertions(+), 218 deletions(-)
Dear Timur,
In message 1222959963-5697-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
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.
What about resubmitting this patch, with adding a debug() to the new, common i2c_reg_read() and i2c_reg_write() funtions?
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
What about resubmitting this patch, with adding a debug() to the new, common i2c_reg_read() and i2c_reg_write() funtions?
I can do that. First, however, I need Mike Frysinger to help me resolve the Blackfin version of i2c_reg_read(), which passes 0 instead of 1 as the length. I've emailed him twice about this, so I'm not sure why he's ignoring me.
On Tuesday 14 October 2008, Timur Tabi wrote:
Wolfgang Denk wrote:
What about resubmitting this patch, with adding a debug() to the new, common i2c_reg_read() and i2c_reg_write() funtions?
I can do that. First, however, I need Mike Frysinger to help me resolve the Blackfin version of i2c_reg_read(), which passes 0 instead of 1 as the length.
it's broken and i fixed it in the Blackfin i2c read write. feel free to make the change and ignore the weirdness.
I've emailed him twice about this, so I'm not sure why he's ignoring me.
i havent been processing any @gentoo.org e-mail ... not really ignoring you if i havent read your e-mail ;) -mike
On Sun, Oct 19, 2008 at 4:18 PM, Mike Frysinger vapier@gentoo.org wrote:
it's broken and i fixed it in the Blackfin i2c read write. feel free to make the change and ignore the weirdness.
Ah, thank you! I'll do it after WD pulls your repo.
i havent been processing any @gentoo.org e-mail ... not really ignoring you if i havent read your e-mail ;)
I don't understand - the commits to your U-Boot repo all have the @gentoo.org address, and your entry in the MAINTAINERS file also uses that address. In fact, everywhere I look I see that address. What address should I be using to contact you?
On Sunday 19 October 2008, Timur Tabi wrote:
On Sun, Oct 19, 2008 at 4:18 PM, Mike Frysinger vapier@gentoo.org wrote:
it's broken and i fixed it in the Blackfin i2c read write. feel free to make the change and ignore the weirdness.
Ah, thank you! I'll do it after WD pulls your repo.
dont wait ... it'll be a bit before i post it, so it'll be easier if i just rebase against that
i havent been processing any @gentoo.org e-mail ... not really ignoring you if i havent read your e-mail ;)
I don't understand - the commits to your U-Boot repo all have the @gentoo.org address, and your entry in the MAINTAINERS file also uses that address. In fact, everywhere I look I see that address. What address should I be using to contact you?
git activity doesnt correspond to e-mail activity ... that is the right e-mail to contact, ive just been too busy to check it lately -mike
Mike Frysinger wrote:
dont wait ... it'll be a bit before i post it, so it'll be easier if i just rebase against that
If I post the patch that assumes that that i2c_reg_read() passes 1 instead of 0, it'll break blackfin.
On Monday 20 October 2008, Timur Tabi wrote:
Mike Frysinger wrote:
dont wait ... it'll be a bit before i post it, so it'll be easier if i just rebase against that
If I post the patch that assumes that that i2c_reg_read() passes 1 instead of 0, it'll break blackfin.
no it wont. as i said, the code that's in there today is broken. you cant break what's already broken. -mike
participants (4)
-
Mike Frysinger -
Stefan Roese -
Timur Tabi -
Wolfgang Denk