| Message ID | 1409067268-956-12-git-send-email-thierry.reding@gmail.com |
|---|---|
| State | Rejected |
| Delegated to: | Heiko Schocher |
| Headers | show |
Hello Thierry, Am 26.08.2014 17:33, schrieb Thierry Reding: > From: Thierry Reding<treding@nvidia.com> > > i2c_bus_init() takes a bus number but relies on the currently selected > bus to determine which adapter to initialize. Make the function use the > bus passed in as parameter rather than the currently selected bus. While > at it, keep a pointer to the specified bus to avoid having to look it up > repeatedly. > > Signed-off-by: Thierry Reding<treding@nvidia.com> > --- > drivers/i2c/i2c_core.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) Why you could not use the current CONFIG_SYS_I2C API and init a bus with i2c_set_bus_num()? i2c_init_bus() is deprecated and should be removed if all i2c drivers are ported to the CONFIG_SYS_I2C framework ... bye, Heiko > diff --git a/drivers/i2c/i2c_core.c b/drivers/i2c/i2c_core.c > index 18d6736601c1..cca455bc9c63 100644 > --- a/drivers/i2c/i2c_core.c > +++ b/drivers/i2c/i2c_core.c > @@ -214,17 +214,20 @@ static int i2c_mux_disconnet_all(void) > * Initializes one bus. Will initialize the parent adapter. No current bus > * changes, no mux (if any) setup. > */ > -static void i2c_init_bus(unsigned int bus_no, int speed, int slaveaddr) > +static void i2c_init_bus(unsigned int bus, int speed, int slaveaddr) > { > - if (bus_no>= CONFIG_SYS_NUM_I2C_BUSES) > + struct i2c_adapter *adapter; > + > + if (bus>= CONFIG_SYS_NUM_I2C_BUSES) > return; > > - I2C_ADAP->init(I2C_ADAP, speed, slaveaddr); > + adapter = i2c_get_adapter(I2C_ADAPTER(bus)); > + adapter->init(adapter, speed, slaveaddr); > > if (gd->flags& GD_FLG_RELOC) { > - I2C_ADAP->init_done = 1; > - I2C_ADAP->speed = speed; > - I2C_ADAP->slaveaddr = slaveaddr; > + adapter->init_done = 1; > + adapter->speed = speed; > + adapter->slaveaddr = slaveaddr; > } > } >
On Wed, Aug 27, 2014 at 06:52:16AM +0200, Heiko Schocher wrote: > Hello Thierry, > > Am 26.08.2014 17:33, schrieb Thierry Reding: > >From: Thierry Reding<treding@nvidia.com> > > > >i2c_bus_init() takes a bus number but relies on the currently selected > >bus to determine which adapter to initialize. Make the function use the > >bus passed in as parameter rather than the currently selected bus. While > >at it, keep a pointer to the specified bus to avoid having to look it up > >repeatedly. > > > >Signed-off-by: Thierry Reding<treding@nvidia.com> > >--- > > drivers/i2c/i2c_core.c | 15 +++++++++------ > > 1 file changed, 9 insertions(+), 6 deletions(-) > > Why you could not use the current CONFIG_SYS_I2C API and init a bus with > i2c_set_bus_num()? That's orthogonal to this patch. i2c_set_bus_num() will end up calling the i2c_init_bus() function, too. What this patch does is fix an issue where i2c_init_bus is completely ignoring the bus_no parameter (except for sanity checking) but instead relies on the gd->cur_i2c_bus (via I2C_ADAP) to initialize a bus. That's completely unexpected and making this consistent allows the function to be reused in a more generic way as done in subsequent patches. Thierry > >diff --git a/drivers/i2c/i2c_core.c b/drivers/i2c/i2c_core.c > >index 18d6736601c1..cca455bc9c63 100644 > >--- a/drivers/i2c/i2c_core.c > >+++ b/drivers/i2c/i2c_core.c > >@@ -214,17 +214,20 @@ static int i2c_mux_disconnet_all(void) > > * Initializes one bus. Will initialize the parent adapter. No current bus > > * changes, no mux (if any) setup. > > */ > >-static void i2c_init_bus(unsigned int bus_no, int speed, int slaveaddr) > >+static void i2c_init_bus(unsigned int bus, int speed, int slaveaddr) > > { > >- if (bus_no>= CONFIG_SYS_NUM_I2C_BUSES) > >+ struct i2c_adapter *adapter; > >+ > >+ if (bus>= CONFIG_SYS_NUM_I2C_BUSES) > > return; > > > >- I2C_ADAP->init(I2C_ADAP, speed, slaveaddr); > >+ adapter = i2c_get_adapter(I2C_ADAPTER(bus)); > >+ adapter->init(adapter, speed, slaveaddr); > > > > if (gd->flags& GD_FLG_RELOC) { > >- I2C_ADAP->init_done = 1; > >- I2C_ADAP->speed = speed; > >- I2C_ADAP->slaveaddr = slaveaddr; > >+ adapter->init_done = 1; > >+ adapter->speed = speed; > >+ adapter->slaveaddr = slaveaddr; > > } > > } > > > > -- > DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Hello Thierry, Am 27.08.2014 07:12, schrieb Thierry Reding: > On Wed, Aug 27, 2014 at 06:52:16AM +0200, Heiko Schocher wrote: >> Hello Thierry, >> >> Am 26.08.2014 17:33, schrieb Thierry Reding: >>> From: Thierry Reding<treding@nvidia.com> >>> >>> i2c_bus_init() takes a bus number but relies on the currently selected >>> bus to determine which adapter to initialize. Make the function use the >>> bus passed in as parameter rather than the currently selected bus. While >>> at it, keep a pointer to the specified bus to avoid having to look it up >>> repeatedly. >>> >>> Signed-off-by: Thierry Reding<treding@nvidia.com> >>> --- >>> drivers/i2c/i2c_core.c | 15 +++++++++------ >>> 1 file changed, 9 insertions(+), 6 deletions(-) >> >> Why you could not use the current CONFIG_SYS_I2C API and init a bus with >> i2c_set_bus_num()? > > That's orthogonal to this patch. i2c_set_bus_num() will end up calling > the i2c_init_bus() function, too. What this patch does is fix an issue > where i2c_init_bus is completely ignoring the bus_no parameter (except > for sanity checking) but instead relies on the gd->cur_i2c_bus (via > I2C_ADAP) to initialize a bus. That's completely unexpected and making > this consistent allows the function to be reused in a more generic way > as done in subsequent patches. Yes, I think we could drop the bus_no parameter. bye, Heiko > > Thierry > >>> diff --git a/drivers/i2c/i2c_core.c b/drivers/i2c/i2c_core.c >>> index 18d6736601c1..cca455bc9c63 100644 >>> --- a/drivers/i2c/i2c_core.c >>> +++ b/drivers/i2c/i2c_core.c >>> @@ -214,17 +214,20 @@ static int i2c_mux_disconnet_all(void) >>> * Initializes one bus. Will initialize the parent adapter. No current bus >>> * changes, no mux (if any) setup. >>> */ >>> -static void i2c_init_bus(unsigned int bus_no, int speed, int slaveaddr) >>> +static void i2c_init_bus(unsigned int bus, int speed, int slaveaddr) >>> { >>> - if (bus_no>= CONFIG_SYS_NUM_I2C_BUSES) >>> + struct i2c_adapter *adapter; >>> + >>> + if (bus>= CONFIG_SYS_NUM_I2C_BUSES) >>> return; >>> >>> - I2C_ADAP->init(I2C_ADAP, speed, slaveaddr); >>> + adapter = i2c_get_adapter(I2C_ADAPTER(bus)); >>> + adapter->init(adapter, speed, slaveaddr); >>> >>> if (gd->flags& GD_FLG_RELOC) { >>> - I2C_ADAP->init_done = 1; >>> - I2C_ADAP->speed = speed; >>> - I2C_ADAP->slaveaddr = slaveaddr; >>> + adapter->init_done = 1; >>> + adapter->speed = speed; >>> + adapter->slaveaddr = slaveaddr; >>> } >>> } >>> >> >> -- >> DENX Software Engineering GmbH, MD: Wolfgang Denk& Detlev Zundel >> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
diff --git a/drivers/i2c/i2c_core.c b/drivers/i2c/i2c_core.c index 18d6736601c1..cca455bc9c63 100644 --- a/drivers/i2c/i2c_core.c +++ b/drivers/i2c/i2c_core.c @@ -214,17 +214,20 @@ static int i2c_mux_disconnet_all(void) * Initializes one bus. Will initialize the parent adapter. No current bus * changes, no mux (if any) setup. */ -static void i2c_init_bus(unsigned int bus_no, int speed, int slaveaddr) +static void i2c_init_bus(unsigned int bus, int speed, int slaveaddr) { - if (bus_no >= CONFIG_SYS_NUM_I2C_BUSES) + struct i2c_adapter *adapter; + + if (bus >= CONFIG_SYS_NUM_I2C_BUSES) return; - I2C_ADAP->init(I2C_ADAP, speed, slaveaddr); + adapter = i2c_get_adapter(I2C_ADAPTER(bus)); + adapter->init(adapter, speed, slaveaddr); if (gd->flags & GD_FLG_RELOC) { - I2C_ADAP->init_done = 1; - I2C_ADAP->speed = speed; - I2C_ADAP->slaveaddr = slaveaddr; + adapter->init_done = 1; + adapter->speed = speed; + adapter->slaveaddr = slaveaddr; } }