* [PATCH 1/2] i2c-powermac: Refactor i2c_powermac_smbus_xfer
@ 2009-10-10 12:19 Jean Delvare
[not found] ` <20091010141908.0be884a5-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
0 siblings, 1 reply; 3+ messages in thread
From: Jean Delvare @ 2009-10-10 12:19 UTC (permalink / raw)
To: Linux I2C; +Cc: Benjamin Herrenschmidt, Paul Mackerras
I wanted to add some error logging to the i2c-powermac driver, but
found that it was very difficult due to the way the
i2c_powermac_smbus_xfer function is organized. Refactor the code in
this function so that each low-level function is only called once.
Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
Cc: Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
Cc: Paul Mackerras <paulus-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
---
This needs testing! Thanks.
drivers/i2c/busses/i2c-powermac.c | 85 +++++++++++++++++--------------------
1 file changed, 41 insertions(+), 44 deletions(-)
--- linux-2.6.32-rc3.orig/drivers/i2c/busses/i2c-powermac.c 2009-10-10 14:08:39.000000000 +0200
+++ linux-2.6.32-rc3/drivers/i2c/busses/i2c-powermac.c 2009-10-10 14:13:04.000000000 +0200
@@ -49,48 +49,38 @@ static s32 i2c_powermac_smbus_xfer( stru
int rc = 0;
int read = (read_write == I2C_SMBUS_READ);
int addrdir = (addr << 1) | read;
+ int mode, subsize, len;
+ u32 subaddr;
+ u8 *buf;
u8 local[2];
- rc = pmac_i2c_open(bus, 0);
- if (rc)
- return rc;
+ if (size == I2C_SMBUS_QUICK || size == I2C_SMBUS_BYTE) {
+ mode = pmac_i2c_mode_std;
+ subsize = 0;
+ subaddr = 0;
+ } else {
+ mode = read ? pmac_i2c_mode_combined : pmac_i2c_mode_stdsub;
+ subsize = 1;
+ subaddr = command;
+ }
switch (size) {
case I2C_SMBUS_QUICK:
- rc = pmac_i2c_setmode(bus, pmac_i2c_mode_std);
- if (rc)
- goto bail;
- rc = pmac_i2c_xfer(bus, addrdir, 0, 0, NULL, 0);
+ buf = NULL;
+ len = 0;
break;
case I2C_SMBUS_BYTE:
- rc = pmac_i2c_setmode(bus, pmac_i2c_mode_std);
- if (rc)
- goto bail;
- rc = pmac_i2c_xfer(bus, addrdir, 0, 0, &data->byte, 1);
- break;
case I2C_SMBUS_BYTE_DATA:
- rc = pmac_i2c_setmode(bus, read ?
- pmac_i2c_mode_combined :
- pmac_i2c_mode_stdsub);
- if (rc)
- goto bail;
- rc = pmac_i2c_xfer(bus, addrdir, 1, command, &data->byte, 1);
+ buf = &data->byte;
+ len = 1;
break;
case I2C_SMBUS_WORD_DATA:
- rc = pmac_i2c_setmode(bus, read ?
- pmac_i2c_mode_combined :
- pmac_i2c_mode_stdsub);
- if (rc)
- goto bail;
if (!read) {
local[0] = data->word & 0xff;
local[1] = (data->word >> 8) & 0xff;
}
- rc = pmac_i2c_xfer(bus, addrdir, 1, command, local, 2);
- if (rc == 0 && read) {
- data->word = ((u16)local[1]) << 8;
- data->word |= local[0];
- }
+ buf = local;
+ len = 2;
break;
/* Note that these are broken vs. the expected smbus API where
@@ -105,28 +95,35 @@ static s32 i2c_powermac_smbus_xfer( stru
* a repeat start/addr phase (but not stop in between)
*/
case I2C_SMBUS_BLOCK_DATA:
- rc = pmac_i2c_setmode(bus, read ?
- pmac_i2c_mode_combined :
- pmac_i2c_mode_stdsub);
- if (rc)
- goto bail;
- rc = pmac_i2c_xfer(bus, addrdir, 1, command, data->block,
- data->block[0] + 1);
-
+ buf = data->block;
+ len = data->block[0] + 1;
break;
case I2C_SMBUS_I2C_BLOCK_DATA:
- rc = pmac_i2c_setmode(bus, read ?
- pmac_i2c_mode_combined :
- pmac_i2c_mode_stdsub);
- if (rc)
- goto bail;
- rc = pmac_i2c_xfer(bus, addrdir, 1, command,
- &data->block[1], data->block[0]);
+ buf = &data->block[1];
+ len = data->block[0];
break;
default:
- rc = -EINVAL;
+ return -EINVAL;
+ }
+
+ rc = pmac_i2c_open(bus, 0);
+ if (rc)
+ return rc;
+
+ rc = pmac_i2c_setmode(bus, mode);
+ if (rc)
+ goto bail;
+
+ rc = pmac_i2c_xfer(bus, addrdir, subsize, subaddr, buf, len);
+ if (rc)
+ goto bail;
+
+ if (size == I2C_SMBUS_WORD_DATA && read) {
+ data->word = ((u16)local[1]) << 8;
+ data->word |= local[0];
}
+
bail:
pmac_i2c_close(bus);
return rc;
--
Jean Delvare
^ permalink raw reply [flat|nested] 3+ messages in thread[parent not found: <20091010141908.0be884a5-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>]
* Re: [PATCH 1/2] i2c-powermac: Refactor i2c_powermac_smbus_xfer [not found] ` <20091010141908.0be884a5-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org> @ 2009-10-10 23:05 ` Benjamin Herrenschmidt 2009-10-22 13:25 ` Jean Delvare 0 siblings, 1 reply; 3+ messages in thread From: Benjamin Herrenschmidt @ 2009-10-10 23:05 UTC (permalink / raw) To: Jean Delvare; +Cc: Linux I2C, Paul Mackerras On Sat, 2009-10-10 at 14:19 +0200, Jean Delvare wrote: > I wanted to add some error logging to the i2c-powermac driver, but > found that it was very difficult due to the way the > i2c_powermac_smbus_xfer function is organized. Refactor the code in > this function so that each low-level function is only called once. > > Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> > Cc: Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org> > Cc: Paul Mackerras <paulus-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> > --- > This needs testing! Thanks. Ok, will give it a go next week. Cheers, Ben. > drivers/i2c/busses/i2c-powermac.c | 85 +++++++++++++++++-------------------- > 1 file changed, 41 insertions(+), 44 deletions(-) > > --- linux-2.6.32-rc3.orig/drivers/i2c/busses/i2c-powermac.c 2009-10-10 14:08:39.000000000 +0200 > +++ linux-2.6.32-rc3/drivers/i2c/busses/i2c-powermac.c 2009-10-10 14:13:04.000000000 +0200 > @@ -49,48 +49,38 @@ static s32 i2c_powermac_smbus_xfer( stru > int rc = 0; > int read = (read_write == I2C_SMBUS_READ); > int addrdir = (addr << 1) | read; > + int mode, subsize, len; > + u32 subaddr; > + u8 *buf; > u8 local[2]; > > - rc = pmac_i2c_open(bus, 0); > - if (rc) > - return rc; > + if (size == I2C_SMBUS_QUICK || size == I2C_SMBUS_BYTE) { > + mode = pmac_i2c_mode_std; > + subsize = 0; > + subaddr = 0; > + } else { > + mode = read ? pmac_i2c_mode_combined : pmac_i2c_mode_stdsub; > + subsize = 1; > + subaddr = command; > + } > > switch (size) { > case I2C_SMBUS_QUICK: > - rc = pmac_i2c_setmode(bus, pmac_i2c_mode_std); > - if (rc) > - goto bail; > - rc = pmac_i2c_xfer(bus, addrdir, 0, 0, NULL, 0); > + buf = NULL; > + len = 0; > break; > case I2C_SMBUS_BYTE: > - rc = pmac_i2c_setmode(bus, pmac_i2c_mode_std); > - if (rc) > - goto bail; > - rc = pmac_i2c_xfer(bus, addrdir, 0, 0, &data->byte, 1); > - break; > case I2C_SMBUS_BYTE_DATA: > - rc = pmac_i2c_setmode(bus, read ? > - pmac_i2c_mode_combined : > - pmac_i2c_mode_stdsub); > - if (rc) > - goto bail; > - rc = pmac_i2c_xfer(bus, addrdir, 1, command, &data->byte, 1); > + buf = &data->byte; > + len = 1; > break; > case I2C_SMBUS_WORD_DATA: > - rc = pmac_i2c_setmode(bus, read ? > - pmac_i2c_mode_combined : > - pmac_i2c_mode_stdsub); > - if (rc) > - goto bail; > if (!read) { > local[0] = data->word & 0xff; > local[1] = (data->word >> 8) & 0xff; > } > - rc = pmac_i2c_xfer(bus, addrdir, 1, command, local, 2); > - if (rc == 0 && read) { > - data->word = ((u16)local[1]) << 8; > - data->word |= local[0]; > - } > + buf = local; > + len = 2; > break; > > /* Note that these are broken vs. the expected smbus API where > @@ -105,28 +95,35 @@ static s32 i2c_powermac_smbus_xfer( stru > * a repeat start/addr phase (but not stop in between) > */ > case I2C_SMBUS_BLOCK_DATA: > - rc = pmac_i2c_setmode(bus, read ? > - pmac_i2c_mode_combined : > - pmac_i2c_mode_stdsub); > - if (rc) > - goto bail; > - rc = pmac_i2c_xfer(bus, addrdir, 1, command, data->block, > - data->block[0] + 1); > - > + buf = data->block; > + len = data->block[0] + 1; > break; > case I2C_SMBUS_I2C_BLOCK_DATA: > - rc = pmac_i2c_setmode(bus, read ? > - pmac_i2c_mode_combined : > - pmac_i2c_mode_stdsub); > - if (rc) > - goto bail; > - rc = pmac_i2c_xfer(bus, addrdir, 1, command, > - &data->block[1], data->block[0]); > + buf = &data->block[1]; > + len = data->block[0]; > break; > > default: > - rc = -EINVAL; > + return -EINVAL; > + } > + > + rc = pmac_i2c_open(bus, 0); > + if (rc) > + return rc; > + > + rc = pmac_i2c_setmode(bus, mode); > + if (rc) > + goto bail; > + > + rc = pmac_i2c_xfer(bus, addrdir, subsize, subaddr, buf, len); > + if (rc) > + goto bail; > + > + if (size == I2C_SMBUS_WORD_DATA && read) { > + data->word = ((u16)local[1]) << 8; > + data->word |= local[0]; > } > + > bail: > pmac_i2c_close(bus); > return rc; > > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 1/2] i2c-powermac: Refactor i2c_powermac_smbus_xfer 2009-10-10 23:05 ` Benjamin Herrenschmidt @ 2009-10-22 13:25 ` Jean Delvare 0 siblings, 0 replies; 3+ messages in thread From: Jean Delvare @ 2009-10-22 13:25 UTC (permalink / raw) To: Benjamin Herrenschmidt; +Cc: Linux I2C, Paul Mackerras Hi Ben, On Sun, 11 Oct 2009 10:05:31 +1100, Benjamin Herrenschmidt wrote: > On Sat, 2009-10-10 at 14:19 +0200, Jean Delvare wrote: > > I wanted to add some error logging to the i2c-powermac driver, but > > found that it was very difficult due to the way the > > i2c_powermac_smbus_xfer function is organized. Refactor the code in > > this function so that each low-level function is only called once. > > > > Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> > > Cc: Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org> > > Cc: Paul Mackerras <paulus-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> > > --- > > This needs testing! Thanks. > > Ok, will give it a go next week. Did you finally find some time to test my i2c-powermac patches? Thanks, -- Jean Delvare ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2009-10-22 13:25 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-10-10 12:19 [PATCH 1/2] i2c-powermac: Refactor i2c_powermac_smbus_xfer Jean Delvare
[not found] ` <20091010141908.0be884a5-ig7AzVSIIG7kN2dkZ6Wm7A@public.gmane.org>
2009-10-10 23:05 ` Benjamin Herrenschmidt
2009-10-22 13:25 ` Jean Delvare
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox