| Message ID | 1351618133-14909-1-git-send-email-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Heiko Schocher |
| Headers | show |
Hello Simon, On 30.10.2012 18:28, Simon Glass wrote: > Rather than using a variable in various places, add a single function, > tegra_i2c_get_bus(), which returns a pointer to information about a > bus. > > This will make it easier to move to the new i2c framework. > > Signed-off-by: Simon Glass<sjg@chromium.org> > --- > drivers/i2c/tegra_i2c.c | 78 ++++++++++++++++++++++++++++++++++++---------- > 1 files changed, 61 insertions(+), 17 deletions(-) First question, did you tried the new i2c framework on some HW? Works it for you? > diff --git a/drivers/i2c/tegra_i2c.c b/drivers/i2c/tegra_i2c.c > index efc77fa..3e157f4 100644 > --- a/drivers/i2c/tegra_i2c.c > +++ b/drivers/i2c/tegra_i2c.c [...] > @@ -302,18 +304,48 @@ static int tegra_i2c_read_data(u32 addr, u8 *data, u32 len) > #error "Please enable device tree support to use this driver" > #endif > > +/** > + * Check that a bus number is valid and return a pointer to it > + * > + * @param bus_num Bus number to check / return > + * @return pointer to bus, if valid, else NULL > + */ > +static struct i2c_bus *tegra_i2c_get_bus(unsigned int bus_num) > +{ > + struct i2c_bus *bus; > + > + if (bus_num>= TEGRA_I2C_NUM_CONTROLLERS) { > + debug("%s: Invalid bus number %u\n", __func__, bus_num); > + return NULL; > + } > + bus =&i2c_controllers[bus_num]; > + if (!bus->inited) { > + debug("%s: Bus %u not available\n", __func__, bus_num); > + return NULL; > + } > + > + return bus; > +} > + I added, as Stephen and you suggested, in the "struct i2c_adapter" to the driver callbacks a new parameter "struct i2c_adapter *adap": struct i2c_adapter { void (*init)(struct i2c_adapter *adap, int speed, int slaveaddr); int (*probe)(struct i2c_adapter *adap, uint8_t chip); int (*read)(struct i2c_adapter *adap, uint8_t chip, uint addr, int alen, uint8_t *buffer, int len); int (*write)(struct i2c_adapter *adap, uint8_t chip, uint addr, int alen, uint8_t *buffer, int len); uint (*set_bus_speed)(struct i2c_adapter *adap, uint speed); [...] so the probe/read/write and set_bus_speed() functions gets now a "struct i2c_adapter" pointer ... i2c driver internally, we need only the "struct i2c_adapter", which the i2c core passes to the i2c driver. So this function would look like now: > +static struct i2c_bus *tegra_i2c_get_bus(struct i2c_adapter *adap) > +{ > + struct i2c_bus *bus; ^^^^^^^^^^^^^^ maybe a rename to "i2c_tegra_driver" would be good? > + bus =&i2c_controllers[adap->hwadapnr]; > + if (!bus->inited) { > + debug("%s: Bus %u not available\n", __func__, bus_num); > + return NULL; > + } > + > + return bus; > +} With this, we can get rid of i2c_bus_num in this driver. Also the probe/read/write and set_bus_speed() functions needs to be adapted. I can do this for you, and add this patchset to my v2 post, if it is ok for you? > unsigned int i2c_get_bus_speed(void) This function is not needed for the new framework! [...] > int i2c_set_bus_speed(unsigned int speed) > { > - struct i2c_bus *i2c_bus; > + struct i2c_bus *bus; > > - i2c_bus =&i2c_controllers[i2c_bus_num]; > - i2c_bus->speed = speed; > - i2c_init_controller(i2c_bus); > + bus = tegra_i2c_get_bus(i2c_bus_num); > + if (!bus) > + return 0; > + bus->speed = speed; > + i2c_init_controller(bus); > > return 0; > } > @@ -426,7 +458,7 @@ void i2c_init(int speed, int slaveaddr) [...] Rest looks good to me. Thanks! bye, Heiko
Hi Heiko, On Tue, Oct 30, 2012 at 10:26 PM, Heiko Schocher <hs@denx.de> wrote: > Hello Simon, > > > On 30.10.2012 18:28, Simon Glass wrote: >> >> Rather than using a variable in various places, add a single function, >> tegra_i2c_get_bus(), which returns a pointer to information about a >> bus. >> >> This will make it easier to move to the new i2c framework. >> >> Signed-off-by: Simon Glass<sjg@chromium.org> >> --- >> drivers/i2c/tegra_i2c.c | 78 >> ++++++++++++++++++++++++++++++++++++---------- >> 1 files changed, 61 insertions(+), 17 deletions(-) > > > First question, did you tried the new i2c framework on some HW? Works it > for you? Yes, it works fine. Tried on a seaboard. > > >> diff --git a/drivers/i2c/tegra_i2c.c b/drivers/i2c/tegra_i2c.c >> index efc77fa..3e157f4 100644 >> --- a/drivers/i2c/tegra_i2c.c >> +++ b/drivers/i2c/tegra_i2c.c > > [...] >> >> @@ -302,18 +304,48 @@ static int tegra_i2c_read_data(u32 addr, u8 *data, >> u32 len) >> #error "Please enable device tree support to use this driver" >> #endif >> >> +/** >> + * Check that a bus number is valid and return a pointer to it >> + * >> + * @param bus_num Bus number to check / return >> + * @return pointer to bus, if valid, else NULL >> + */ >> +static struct i2c_bus *tegra_i2c_get_bus(unsigned int bus_num) >> +{ >> + struct i2c_bus *bus; >> + >> + if (bus_num>= TEGRA_I2C_NUM_CONTROLLERS) { >> + debug("%s: Invalid bus number %u\n", __func__, bus_num); >> + return NULL; >> + } >> + bus =&i2c_controllers[bus_num]; >> >> + if (!bus->inited) { >> + debug("%s: Bus %u not available\n", __func__, bus_num); >> + return NULL; >> + } >> + >> + return bus; >> +} >> + > > > I added, as Stephen and you suggested, in the "struct i2c_adapter" > to the driver callbacks a new parameter "struct i2c_adapter *adap": > > struct i2c_adapter { > void (*init)(struct i2c_adapter *adap, int speed, > int slaveaddr); > int (*probe)(struct i2c_adapter *adap, uint8_t chip); > int (*read)(struct i2c_adapter *adap, uint8_t chip, > uint addr, int alen, uint8_t *buffer, > int len); > int (*write)(struct i2c_adapter *adap, uint8_t chip, > uint addr, int alen, uint8_t *buffer, > int len); > uint (*set_bus_speed)(struct i2c_adapter *adap, > uint speed); > [...] > > so the probe/read/write and set_bus_speed() functions gets now a > "struct i2c_adapter" pointer ... i2c driver internally, we need only > the "struct i2c_adapter", which the i2c core passes to the i2c driver. > So this function would look like now: > >> +static struct i2c_bus *tegra_i2c_get_bus(struct i2c_adapter *adap) Sounds good. Are you going to send a new patch? > >> +{ >> + struct i2c_bus *bus; > ^^^^^^^^^^^^^^ > maybe a rename to "i2c_tegra_driver" would be good? >> + bus =&i2c_controllers[adap->hwadapnr]; > >> + if (!bus->inited) { >> + debug("%s: Bus %u not available\n", __func__, bus_num); >> + return NULL; >> + } >> + >> + return bus; >> +} > > With this, we can get rid of i2c_bus_num in this driver. > > Also the probe/read/write and set_bus_speed() functions needs to > be adapted. I can do this for you, and add this patchset to my > v2 post, if it is ok for you? Of course. > >> unsigned int i2c_get_bus_speed(void) > > > This function is not needed for the new framework! > [...] Yes, I thought I removed it in 2/2, perhaps not. > >> int i2c_set_bus_speed(unsigned int speed) >> { >> - struct i2c_bus *i2c_bus; >> + struct i2c_bus *bus; >> >> - i2c_bus =&i2c_controllers[i2c_bus_num]; >> >> - i2c_bus->speed = speed; >> - i2c_init_controller(i2c_bus); >> + bus = tegra_i2c_get_bus(i2c_bus_num); >> + if (!bus) >> + return 0; >> + bus->speed = speed; >> + i2c_init_controller(bus); >> >> return 0; >> } >> @@ -426,7 +458,7 @@ void i2c_init(int speed, int slaveaddr) > > [...] > > Rest looks good to me. > > Thanks! Will await your further patch, and I am ready to test. Thanks. Regards, Simon > > bye, > Heiko > -- > DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Hello Simon, On 05.11.2012 21:43, Simon Glass wrote: > Hi Heiko, > > On Tue, Oct 30, 2012 at 10:26 PM, Heiko Schocher<hs@denx.de> wrote: >> Hello Simon, >> >> >> On 30.10.2012 18:28, Simon Glass wrote: >>> >>> Rather than using a variable in various places, add a single function, >>> tegra_i2c_get_bus(), which returns a pointer to information about a >>> bus. >>> >>> This will make it easier to move to the new i2c framework. >>> >>> Signed-off-by: Simon Glass<sjg@chromium.org> >>> --- >>> drivers/i2c/tegra_i2c.c | 78 >>> ++++++++++++++++++++++++++++++++++++---------- >>> 1 files changed, 61 insertions(+), 17 deletions(-) >> >> >> First question, did you tried the new i2c framework on some HW? Works it >> for you? > > Yes, it works fine. Tried on a seaboard. Great, thanks for testing! >>> diff --git a/drivers/i2c/tegra_i2c.c b/drivers/i2c/tegra_i2c.c >>> index efc77fa..3e157f4 100644 >>> --- a/drivers/i2c/tegra_i2c.c >>> +++ b/drivers/i2c/tegra_i2c.c [...] >> so the probe/read/write and set_bus_speed() functions gets now a >> "struct i2c_adapter" pointer ... i2c driver internally, we need only >> the "struct i2c_adapter", which the i2c core passes to the i2c driver. >> So this function would look like now: >> >>> +static struct i2c_bus *tegra_i2c_get_bus(struct i2c_adapter *adap) > > Sounds good. Are you going to send a new patch? Yes, want to do some more test ... and waiting for the result of the vote, which direction we should go, see previous EMail ... >>> +{ >>> + struct i2c_bus *bus; >> ^^^^^^^^^^^^^^ >> maybe a rename to "i2c_tegra_driver" would be good? >>> + bus =&i2c_controllers[adap->hwadapnr]; >> >>> + if (!bus->inited) { >>> + debug("%s: Bus %u not available\n", __func__, bus_num); >>> + return NULL; >>> + } >>> + >>> + return bus; >>> +} >> >> With this, we can get rid of i2c_bus_num in this driver. >> >> Also the probe/read/write and set_bus_speed() functions needs to >> be adapted. I can do this for you, and add this patchset to my >> v2 post, if it is ok for you? > > Of course. Ok, do this ... >>> unsigned int i2c_get_bus_speed(void) >> >> >> This function is not needed for the new framework! >> [...] > > Yes, I thought I removed it in 2/2, perhaps not. > >> >>> int i2c_set_bus_speed(unsigned int speed) >>> { >>> - struct i2c_bus *i2c_bus; >>> + struct i2c_bus *bus; >>> >>> - i2c_bus =&i2c_controllers[i2c_bus_num]; >>> >>> - i2c_bus->speed = speed; >>> - i2c_init_controller(i2c_bus); >>> + bus = tegra_i2c_get_bus(i2c_bus_num); >>> + if (!bus) >>> + return 0; >>> + bus->speed = speed; >>> + i2c_init_controller(bus); >>> >>> return 0; >>> } >>> @@ -426,7 +458,7 @@ void i2c_init(int speed, int slaveaddr) >> >> [...] >> >> Rest looks good to me. >> >> Thanks! > > Will await your further patch, and I am ready to test. Thanks. Thanks! bye, Heiko
diff --git a/drivers/i2c/tegra_i2c.c b/drivers/i2c/tegra_i2c.c index efc77fa..3e157f4 100644 --- a/drivers/i2c/tegra_i2c.c +++ b/drivers/i2c/tegra_i2c.c @@ -262,7 +262,8 @@ exit: return error; } -static int tegra_i2c_write_data(u32 addr, u8 *data, u32 len) +static int tegra_i2c_write_data(struct i2c_bus *bus, u32 addr, u8 *data, + u32 len) { int error; struct i2c_trans_info trans_info; @@ -273,14 +274,15 @@ static int tegra_i2c_write_data(u32 addr, u8 *data, u32 len) trans_info.num_bytes = len; trans_info.is_10bit_address = 0; - error = send_recv_packets(&i2c_controllers[i2c_bus_num], &trans_info); + error = send_recv_packets(bus, &trans_info); if (error) debug("tegra_i2c_write_data: Error (%d) !!!\n", error); return error; } -static int tegra_i2c_read_data(u32 addr, u8 *data, u32 len) +static int tegra_i2c_read_data(struct i2c_bus *bus, u32 addr, u8 *data, + u32 len) { int error; struct i2c_trans_info trans_info; @@ -291,7 +293,7 @@ static int tegra_i2c_read_data(u32 addr, u8 *data, u32 len) trans_info.num_bytes = len; trans_info.is_10bit_address = 0; - error = send_recv_packets(&i2c_controllers[i2c_bus_num], &trans_info); + error = send_recv_packets(bus, &trans_info); if (error) debug("tegra_i2c_read_data: Error (%d) !!!\n", error); @@ -302,18 +304,48 @@ static int tegra_i2c_read_data(u32 addr, u8 *data, u32 len) #error "Please enable device tree support to use this driver" #endif +/** + * Check that a bus number is valid and return a pointer to it + * + * @param bus_num Bus number to check / return + * @return pointer to bus, if valid, else NULL + */ +static struct i2c_bus *tegra_i2c_get_bus(unsigned int bus_num) +{ + struct i2c_bus *bus; + + if (bus_num >= TEGRA_I2C_NUM_CONTROLLERS) { + debug("%s: Invalid bus number %u\n", __func__, bus_num); + return NULL; + } + bus = &i2c_controllers[bus_num]; + if (!bus->inited) { + debug("%s: Bus %u not available\n", __func__, bus_num); + return NULL; + } + + return bus; +} + unsigned int i2c_get_bus_speed(void) { - return i2c_controllers[i2c_bus_num].speed; + struct i2c_bus *bus; + + bus = tegra_i2c_get_bus(i2c_bus_num); + if (!bus) + return 0; + return bus->speed; } int i2c_set_bus_speed(unsigned int speed) { - struct i2c_bus *i2c_bus; + struct i2c_bus *bus; - i2c_bus = &i2c_controllers[i2c_bus_num]; - i2c_bus->speed = speed; - i2c_init_controller(i2c_bus); + bus = tegra_i2c_get_bus(i2c_bus_num); + if (!bus) + return 0; + bus->speed = speed; + i2c_init_controller(bus); return 0; } @@ -426,7 +458,7 @@ void i2c_init(int speed, int slaveaddr) } /* i2c write version without the register address */ -int i2c_write_data(uchar chip, uchar *buffer, int len) +int i2c_write_data(struct i2c_bus *bus, uchar chip, uchar *buffer, int len) { int rc; @@ -438,7 +470,7 @@ int i2c_write_data(uchar chip, uchar *buffer, int len) debug("\n"); /* Shift 7-bit address over for lower-level i2c functions */ - rc = tegra_i2c_write_data(chip << 1, buffer, len); + rc = tegra_i2c_write_data(bus, chip << 1, buffer, len); if (rc) debug("i2c_write_data(): rc=%d\n", rc); @@ -446,13 +478,13 @@ int i2c_write_data(uchar chip, uchar *buffer, int len) } /* i2c read version without the register address */ -int i2c_read_data(uchar chip, uchar *buffer, int len) +int i2c_read_data(struct i2c_bus *bus, uchar chip, uchar *buffer, int len) { int rc; debug("inside i2c_read_data():\n"); /* Shift 7-bit address over for lower-level i2c functions */ - rc = tegra_i2c_read_data(chip << 1, buffer, len); + rc = tegra_i2c_read_data(bus, chip << 1, buffer, len); if (rc) { debug("i2c_read_data(): rc=%d\n", rc); return rc; @@ -470,12 +502,16 @@ int i2c_read_data(uchar chip, uchar *buffer, int len) /* Probe to see if a chip is present. */ int i2c_probe(uchar chip) { + struct i2c_bus *bus; int rc; uchar reg; debug("i2c_probe: addr=0x%x\n", chip); + bus = tegra_i2c_get_bus(i2c_get_bus_num()); + if (!bus) + return 1; reg = 0; - rc = i2c_write_data(chip, ®, 1); + rc = i2c_write_data(bus, chip, ®, 1); if (rc) { debug("Error probing 0x%x.\n", chip); return 1; @@ -492,11 +528,15 @@ static int i2c_addr_ok(const uint addr, const int alen) /* Read bytes */ int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len) { + struct i2c_bus *bus; uint offset; int i; debug("i2c_read: chip=0x%x, addr=0x%x, len=0x%x\n", chip, addr, len); + bus = tegra_i2c_get_bus(i2c_bus_num); + if (!bus) + return 1; if (!i2c_addr_ok(addr, alen)) { debug("i2c_read: Bad address %x.%d.\n", addr, alen); return 1; @@ -508,13 +548,13 @@ int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len) data[alen - i - 1] = (addr + offset) >> (8 * i); } - if (i2c_write_data(chip, data, alen)) { + if (i2c_write_data(bus, chip, data, alen)) { debug("i2c_read: error sending (0x%x)\n", addr); return 1; } } - if (i2c_read_data(chip, buffer + offset, 1)) { + if (i2c_read_data(bus, chip, buffer + offset, 1)) { debug("i2c_read: error reading (0x%x)\n", addr); return 1; } @@ -526,11 +566,15 @@ int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len) /* Write bytes */ int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len) { + struct i2c_bus *bus; uint offset; int i; debug("i2c_write: chip=0x%x, addr=0x%x, len=0x%x\n", chip, addr, len); + bus = tegra_i2c_get_bus(i2c_bus_num); + if (!bus) + return 1; if (!i2c_addr_ok(addr, alen)) { debug("i2c_write: Bad address %x.%d.\n", addr, alen); return 1; @@ -540,7 +584,7 @@ int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len) for (i = 0; i < alen; i++) data[alen - i - 1] = (addr + offset) >> (8 * i); data[alen] = buffer[offset]; - if (i2c_write_data(chip, data, alen + 1)) { + if (i2c_write_data(bus, chip, data, alen + 1)) { debug("i2c_write: error sending (0x%x)\n", addr); return 1; }
Rather than using a variable in various places, add a single function, tegra_i2c_get_bus(), which returns a pointer to information about a bus. This will make it easier to move to the new i2c framework. Signed-off-by: Simon Glass <sjg@chromium.org> --- drivers/i2c/tegra_i2c.c | 78 ++++++++++++++++++++++++++++++++++++---------- 1 files changed, 61 insertions(+), 17 deletions(-)