| Message ID | 1409067268-956-14-git-send-email-thierry.reding@gmail.com |
|---|---|
| State | Rejected |
| Delegated to: | Heiko Schocher |
| Headers | show |
Hello Thierry, Am 26.08.2014 17:34, schrieb Thierry Reding: > From: Thierry Reding<treding@nvidia.com> > > This API operates on I2C adapters or I2C clients (a new type of object which is a bad idea ... > that refers to a particular slave connected to an adapter). This is > useful to avoid having to call i2c_set_bus_num() whenever a device is > being accessed. But thats the supose of i2c_set_bus_num()! ... if you use an i2c bus, you must check before every access, if you are on it, if not, you must switch back to it... This is collected in i2c_set_bus_num() ... before, every "user" did this on his own ... if you are on the bus you want to access, the overhead is not so big, see: http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/i2c/i2c_core.c;h=18d6736601c161f45cb7d81b5eae53bdeaaf6b0b;hb=7bee1c91a94db19bd26f92cc67be35d3592c6429#l278 278 int i2c_set_bus_num(unsigned int bus) 279 { 280 int max; 281 282 if ((bus == I2C_BUS) && (I2C_ADAP->init_done > 0)) 283 return 0; And you must be aware of i2c muxes! You directly use the read/write functions from the i2c adapter, but what is if you have i2c muxes? Maybe there is on one i2c adapter a i2c mux with 4 ports. On one is an eeprom, on the other is a PMIC ... your code in patch "power: Add AMS AS3722 PMIC support" does access with your functions the PMIC ... what is, if between this accesses someone accesses the eeprom? If he switches the mux, you never switch back! Your code did not check this! Why is i2c_set_bus_num() such a problem? > Drivers for I2C devices are supposed to embed a struct i2c_client within > a driver-specific data structure and call i2c_client_init() on it, > passing in a pointer to the parent I2C adapter and the slave address of > the device. > > Signed-off-by: Thierry Reding<treding@nvidia.com> > --- > drivers/i2c/i2c_core.c | 53 ++++++++++++++++++++++++++++ > include/i2c.h | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 149 insertions(+) > > diff --git a/drivers/i2c/i2c_core.c b/drivers/i2c/i2c_core.c > index f6179a16244b..88c7af546b0b 100644 > --- a/drivers/i2c/i2c_core.c > +++ b/drivers/i2c/i2c_core.c > @@ -410,3 +410,56 @@ void __i2c_init(int speed, int slaveaddr) > } > void i2c_init(int speed, int slaveaddr) > __attribute__((weak, alias("__i2c_init"))); > + > +struct i2c_adapter *i2c_adapter_get(unsigned int index) > +{ > + struct i2c_adapter *adapter = ll_entry_start(struct i2c_adapter, i2c); > + unsigned int num = ll_entry_count(struct i2c_adapter, i2c); > + unsigned int i; > + > + if (index>= num) > + return NULL; > + > + for (i = 0; i< index; i++) > + adapter++; > + > + i2c_adapter_init(adapter, adapter->speed, adapter->slaveaddr); > + return adapter; > +} > + > +int i2c_adapter_read(struct i2c_adapter *adapter, uint8_t chip, > + unsigned int address, size_t alen, void *buffer, > + size_t size) > +{ > + return adapter->read(adapter, chip, address, alen, buffer, size); > +} > + > +int i2c_adapter_write(struct i2c_adapter *adapter, uint8_t chip, > + unsigned int address, size_t alen, void *buffer, > + size_t size) > +{ > + return adapter->write(adapter, chip, address, alen, buffer, size); > +} > + > +int i2c_client_init(struct i2c_client *client, struct i2c_adapter *adapter, > + uint8_t address) > +{ > + client->adapter = adapter; > + client->address = address; > + > + return 0; > +} > + > +int i2c_client_read(struct i2c_client *client, unsigned int address, > + size_t alen, void *buffer, size_t size) > +{ > + return i2c_adapter_read(client->adapter, client->address, address, > + alen, buffer, size); > +} > + > +int i2c_client_write(struct i2c_client *client, unsigned int address, > + size_t alen, void *buffer, size_t size) > +{ > + return i2c_adapter_write(client->adapter, client->address, address, > + alen, buffer, size); > +} > diff --git a/include/i2c.h b/include/i2c.h > index 1b4078ed62fe..8f73ba93c614 100644 > --- a/include/i2c.h > +++ b/include/i2c.h > @@ -55,6 +55,20 @@ > #define CONFIG_SYS_SPD_BUS_NUM 0 > #endif > > +/** > + * struct i2c_adapter - I2C adapter > + * @init: initialize I2C adapter > + * @probe: probe for a device on the I2C bus > + * @read: read data from a device on the I2C bus > + * @write: write data to a device on the I2C bus > + * @set_bus_speed: configure the I2C bus speed > + * @speed: current I2C bus speed > + * @waitdelay: > + * @slaveaddr: own address (when used as a slave) > + * @init_done: flag to indicate whether or not the adapter has been initialized > + * @hwadapnr: the hardware number of this adapter > + * @name: name of this adapter > + */ > struct i2c_adapter { > void (*init)(struct i2c_adapter *adap, int speed, > int slaveaddr); > @@ -75,6 +89,16 @@ struct i2c_adapter { > char *name; > }; > > +/** > + * struct i2c_client - I2C slave > + * @adapter: I2C adapter providing the slave's parent bus > + * address: address of the I2C slave on the parent bus > + */ > +struct i2c_client { > + struct i2c_adapter *adapter; > + unsigned int address; > +}; > + > #define U_BOOT_I2C_MKENT_COMPLETE(_init, _probe, _read, _write, \ > _set_speed, _speed, _slaveaddr, _hwadapnr, _name) \ > { \ > @@ -271,6 +295,78 @@ unsigned int i2c_get_bus_speed(void); > * Adjusts I2C pointers after U-Boot is relocated to DRAM > */ > void i2c_reloc_fixup(void); > + > +/* > + * i2c_adapter_get() - get the I2C adapter associated with a given index > + * @index: index of the I2C adapter > + */ > +struct i2c_adapter *i2c_adapter_get(unsigned int index); > + > +/* > + * i2c_adapter_read() - read data from an I2C slave at a given address > + * @adapter: I2C adapter > + * @chip: address of the I2C slave to read from > + * @address: address within the I2C slave to read from > + * @alen: length of address > + * @buffer: buffer to receive data from I2C slave > + * @size: number of bytes to read > + */ > +int i2c_adapter_read(struct i2c_adapter *adapter, uint8_t chip, > + unsigned int address, size_t alen, void *buffer, > + size_t size); > + > +/* > + * i2c_adapter_write() - write data to an I2C slave at a given address > + * @adapter: I2C adapter > + * @chip: address of the I2C slave to write to > + * @address: address within the I2C slave to write to > + * @alen: length of address > + * @buffer: buffer containing the data to write > + * @size: number of bytes to write > + * > + * Ideally the function would take a const void * buffer, but the underlying > + * infrastructure doesn't properly propagate const and adding it here would > + * cause a lot of build warnings. > + */ > +int i2c_adapter_write(struct i2c_adapter *adapter, uint8_t chip, > + unsigned int address, size_t alen, void *buffer, > + size_t size); > + > +/* > + * i2c_client_init() - initialize an I2C slave > + * @client: I2C slave > + * @adapter: parent I2C adapter > + * @address: address of I2C slave > + */ > +int i2c_client_init(struct i2c_client *client, struct i2c_adapter *adapter, > + uint8_t address); > + > +/* > + * i2c_client_read() - read data from an I2C slave > + * @client: I2C slave > + * @address: address within the I2C slave to read from > + * @alen: length of address > + * @buffer: buffer to receive data from I2C slave > + * @size: number of bytes to read > + */ > +int i2c_client_read(struct i2c_client *client, unsigned int address, > + size_t alen, void *buffer, size_t size); > + > +/* > + * i2c_client_write() - write data to an I2C slave > + * @client: I2C slave > + * @address: address within the I2C slave to write to > + * @alen: length of address > + * @buffer: buffer containing the data to write > + * @size: number of bytes to write > + * > + * Ideally the function would take a const void * buffer, but the underlying > + * infrastructure doesn't properly propagate const and adding it here would > + * cause a lot of build warnings. > + */ > +int i2c_client_write(struct i2c_client *client, unsigned int address, > + size_t alen, void *buffer, size_t size); > + > #if defined(CONFIG_SYS_I2C_SOFT) > void i2c_soft_init(void); > void i2c_soft_active(void);
On Wed, Aug 27, 2014 at 07:21:51AM +0200, Heiko Schocher wrote: > Hello Thierry, > > Am 26.08.2014 17:34, schrieb Thierry Reding: > >From: Thierry Reding<treding@nvidia.com> > > > >This API operates on I2C adapters or I2C clients (a new type of object > > which is a bad idea ... > > >that refers to a particular slave connected to an adapter). This is > >useful to avoid having to call i2c_set_bus_num() whenever a device is > >being accessed. > > But thats the supose of i2c_set_bus_num()! ... if you use an i2c bus, > you must check before every access, if you are on it, if not, you must > switch back to it... That's not what code does today. Everybody calls i2c_set_bus_num() once and then does a bunch of transactions on that bus. Given that U-Boot doesn't run multithreaded this works. If you're really concerned about this being a problem, then it should be solved at a different level. It could be part of i2c_client for example, so that i2c_client_read() and i2c_client_write() would always perform this step. Burdening users with this isn't going to work (in a multithreaded environment the switch to a different mux could happen between the call to i2c_set_bus_num() and the bus access). In fact I think this would even have to be solved at the controller level if you want to make sure that client transactions are atomic. > This is collected in i2c_set_bus_num() ... before, every "user" did > this on his own ... if you are on the bus you want to access, the > overhead is not so big, see: > > http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/i2c/i2c_core.c;h=18d6736601c161f45cb7d81b5eae53bdeaaf6b0b;hb=7bee1c91a94db19bd26f92cc67be35d3592c6429#l278 > > 278 int i2c_set_bus_num(unsigned int bus) > 279 { > 280 int max; > 281 > 282 if ((bus == I2C_BUS) && (I2C_ADAP->init_done > 0)) > 283 return 0; > > And you must be aware of i2c muxes! You directly use the read/write > functions from the i2c adapter, but what is if you have i2c muxes? That's complexity that users shouldn't have to worry about. They should simply access an adapter and the adapter (or rather the core) should take care of setting up any muxes correctly. > Maybe there is on one i2c adapter a i2c mux with 4 ports. On one is > an eeprom, on the other is a PMIC ... your code in patch > "power: Add AMS AS3722 PMIC support" does access with your functions > the PMIC ... what is, if between this accesses someone accesses the eeprom? > If he switches the mux, you never switch back! > > Your code did not check this! Like I said, a lot of code in U-Boot doesn't check this. And quite frankly as long as this isn't handled in the core I don't think people will get it right. > Why is i2c_set_bus_num() such a problem? Because it's completely confusing. And it's exposing an implementation detail to users instead of handling it transparently in the core. Thierry
Hello Thierry, Am 27.08.2014 08:21, schrieb Thierry Reding: > On Wed, Aug 27, 2014 at 07:21:51AM +0200, Heiko Schocher wrote: >> Hello Thierry, >> >> Am 26.08.2014 17:34, schrieb Thierry Reding: >>> From: Thierry Reding<treding@nvidia.com> >>> >>> This API operates on I2C adapters or I2C clients (a new type of object >> >> which is a bad idea ... >> >>> that refers to a particular slave connected to an adapter). This is >>> useful to avoid having to call i2c_set_bus_num() whenever a device is >>> being accessed. >> >> But thats the supose of i2c_set_bus_num()! ... if you use an i2c bus, >> you must check before every access, if you are on it, if not, you must >> switch back to it... > > That's not what code does today. Everybody calls i2c_set_bus_num() once > and then does a bunch of transactions on that bus. Given that U-Boot Yes, sadly. This has historical reasons ... > doesn't run multithreaded this works. If you're really concerned about Yes, U-Boot is singlethread only. > this being a problem, then it should be solved at a different level. It > could be part of i2c_client for example, so that i2c_client_read() and > i2c_client_write() would always perform this step. Burdening users with Exactly, thats right, and this is a goal from the CONFIG_SYS_I2C API! But why do you introduce i2c_client_read/write and do not add this step to i2c_read/write? - convert all i2c drivers, which are not yet converted to CONFIG_SYS_I2C (get also rid od CONFIG_HARD_I2C) - add busnumber to i2c_read/write API and make i2c_set_bus_num() static ... and fix all i2c_read/write() calls in U-Boot code ... I know, this is a big change and a lot of work ... thats the reason why we are not at this point ... nobody volunteered to go forward, and I did not found time to do it ... > this isn't going to work (in a multithreaded environment the switch to a > different mux could happen between the call to i2c_set_bus_num() and the > bus access). > > In fact I think this would even have to be solved at the controller > level if you want to make sure that client transactions are atomic. As U-Boot is single threaded all i2c_read/writes are atomic. >> This is collected in i2c_set_bus_num() ... before, every "user" did >> this on his own ... if you are on the bus you want to access, the >> overhead is not so big, see: >> >> http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/i2c/i2c_core.c;h=18d6736601c161f45cb7d81b5eae53bdeaaf6b0b;hb=7bee1c91a94db19bd26f92cc67be35d3592c6429#l278 >> >> 278 int i2c_set_bus_num(unsigned int bus) >> 279 { >> 280 int max; >> 281 >> 282 if ((bus == I2C_BUS)&& (I2C_ADAP->init_done> 0)) >> 283 return 0; >> >> And you must be aware of i2c muxes! You directly use the read/write >> functions from the i2c adapter, but what is if you have i2c muxes? > > That's complexity that users shouldn't have to worry about. They should Exactly! > simply access an adapter and the adapter (or rather the core) should > take care of setting up any muxes correctly. Yes! I think you mix here i2c adapter with bus. An "U-Boot i2c adapter" is a hw adapter (or special case soft i2c adapter). An "i2c bus" is a hw adapter maybe with m muxes, and each bus has exactly one way through the i2c muxes, see for an example the README: http://git.denx.de/?p=u-boot.git;a=blob;f=README;h=14d6b227d689825025f9dfc98fb305021882446d;hb=7bee1c91a94db19bd26f92cc67be35d3592c6429#l2349 So the only thing a User must know when he wants to use an i2c bus is his number. The switching to this i2c adapter, initializing it and maybe set i2c muxes does the i2c subsystem ... >> Maybe there is on one i2c adapter a i2c mux with 4 ports. On one is >> an eeprom, on the other is a PMIC ... your code in patch >> "power: Add AMS AS3722 PMIC support" does access with your functions >> the PMIC ... what is, if between this accesses someone accesses the eeprom? >> If he switches the mux, you never switch back! >> >> Your code did not check this! > > Like I said, a lot of code in U-Boot doesn't check this. And quite With using i2c_set_bus_num() you have not to check this! You only have to call i2c_set_bus_num() before calling i2c_read/write ... and yes, that would be nice, if we just pass the bus number to i2c_read/write() and drop the i2c_set_bus_num() call all over the code ... Patches welcome! > frankly as long as this isn't handled in the core I don't think people > will get it right. Yes, full ack, which is the goal from CONFIG_SYS_I2C. If all i2c driver are converted to it, we can make i2c_set_bus_num() static, and add to the i2c API the bus number as a function parameter! >> Why is i2c_set_bus_num() such a problem? > > Because it's completely confusing. And it's exposing an implementation > detail to users instead of handling it transparently in the core. Yes! Full Ack ... but I do not accept a new API for that! Please fix the i2c_read/write() functions! bye, Heiko
On Wed, Aug 27, 2014 at 09:07:58AM +0200, Heiko Schocher wrote: > Hello Thierry, > > Am 27.08.2014 08:21, schrieb Thierry Reding: > >On Wed, Aug 27, 2014 at 07:21:51AM +0200, Heiko Schocher wrote: > >>Hello Thierry, > >> > >>Am 26.08.2014 17:34, schrieb Thierry Reding: > >>>From: Thierry Reding<treding@nvidia.com> > >>> > >>>This API operates on I2C adapters or I2C clients (a new type of object > >> > >>which is a bad idea ... > >> > >>>that refers to a particular slave connected to an adapter). This is > >>>useful to avoid having to call i2c_set_bus_num() whenever a device is > >>>being accessed. > >> > >>But thats the supose of i2c_set_bus_num()! ... if you use an i2c bus, > >>you must check before every access, if you are on it, if not, you must > >>switch back to it... > > > >That's not what code does today. Everybody calls i2c_set_bus_num() once > >and then does a bunch of transactions on that bus. Given that U-Boot > > Yes, sadly. This has historical reasons ... > > >doesn't run multithreaded this works. If you're really concerned about > > Yes, U-Boot is singlethread only. > > >this being a problem, then it should be solved at a different level. It > >could be part of i2c_client for example, so that i2c_client_read() and > >i2c_client_write() would always perform this step. Burdening users with > > Exactly, thats right, and this is a goal from the CONFIG_SYS_I2C API! > > But why do you introduce i2c_client_read/write and do not add this step > to i2c_read/write? > > - convert all i2c drivers, which are not yet converted to CONFIG_SYS_I2C > (get also rid od CONFIG_HARD_I2C) > - add busnumber to i2c_read/write API and make i2c_set_bus_num() static ... > and fix all i2c_read/write() calls in U-Boot code ... I don't think adding a bus number as parameter is useful. Why not just use the I2C adapter directly? That way we don't have to keep looking it up in an array every time. > I know, this is a big change and a lot of work ... thats the reason > why we are not at this point ... nobody volunteered to go forward, and > I did not found time to do it ... I suppose that would be one possibility to do it. But I consider i2c_client more of a convenience around the lower-level i2c_read() and i2c_write(). The idea is that users set up an I2C client once and then refer to the client, which will automatically use the correct adapter and slave address rather than having that duplicated in every driver. > >this isn't going to work (in a multithreaded environment the switch to a > >different mux could happen between the call to i2c_set_bus_num() and the > >bus access). > > > >In fact I think this would even have to be solved at the controller > >level if you want to make sure that client transactions are atomic. > > As U-Boot is single threaded all i2c_read/writes are atomic. In which case you don't have to call i2c_set_bus_num() for every access, only whenever you don't know exactly where you're coming from. Functions that perform a sequence of accesses only need to set it once. Also, if we directly talk to an adapter instead, then the bulk of what i2c_set_bus_num() does isn't even required. It would require that adapters are made aware of a hierarchy if there are muxes, but I think that's worthwhile to do in any case. Also if ever I2C muxing needs to gain device tree support having the muxes set up dynamically will be pretty much a prerequisite. > >>This is collected in i2c_set_bus_num() ... before, every "user" did > >>this on his own ... if you are on the bus you want to access, the > >>overhead is not so big, see: > >> > >>http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/i2c/i2c_core.c;h=18d6736601c161f45cb7d81b5eae53bdeaaf6b0b;hb=7bee1c91a94db19bd26f92cc67be35d3592c6429#l278 > >> > >> 278 int i2c_set_bus_num(unsigned int bus) > >> 279 { > >> 280 int max; > >> 281 > >> 282 if ((bus == I2C_BUS)&& (I2C_ADAP->init_done> 0)) > >> 283 return 0; > >> > >>And you must be aware of i2c muxes! You directly use the read/write > >>functions from the i2c adapter, but what is if you have i2c muxes? > > > >That's complexity that users shouldn't have to worry about. They should > > Exactly! > > >simply access an adapter and the adapter (or rather the core) should > >take care of setting up any muxes correctly. > > Yes! > > I think you mix here i2c adapter with bus. An "U-Boot i2c adapter" is a > hw adapter (or special case soft i2c adapter). An "i2c bus" is a hw adapter > maybe with m muxes, and each bus has exactly one way through the > i2c muxes, see for an example the README: > > http://git.denx.de/?p=u-boot.git;a=blob;f=README;h=14d6b227d689825025f9dfc98fb305021882446d;hb=7bee1c91a94db19bd26f92cc67be35d3592c6429#l2349 > > So the only thing a User must know when he wants to use an i2c bus is > his number. The switching to this i2c adapter, initializing it and maybe > set i2c muxes does the i2c subsystem ... The above doesn't preclude an I2C adapter representing one of the ports of a mux. That way you can still talk to an adapter rather than having to refer to the bus by number. Adapter would become a little more abstract than it is now, since it would be simply an output that I2C slaves are connected to (either a HW controller directly or a mux connected to a HW controller). > >>Maybe there is on one i2c adapter a i2c mux with 4 ports. On one is > >>an eeprom, on the other is a PMIC ... your code in patch > >>"power: Add AMS AS3722 PMIC support" does access with your functions > >>the PMIC ... what is, if between this accesses someone accesses the eeprom? > >>If he switches the mux, you never switch back! > >> > >>Your code did not check this! > > > >Like I said, a lot of code in U-Boot doesn't check this. And quite > > With using i2c_set_bus_num() you have not to check this! You only have > to call i2c_set_bus_num() before calling i2c_read/write ... and yes, > that would be nice, if we just pass the bus number to i2c_read/write() > and drop the i2c_set_bus_num() call all over the code ... > > Patches welcome! How about a slightly different proposal: introduce a new level of abstraction (like i2c_client) and start using it in new I2C slave drivers. At the same time existing drivers could be converted one at a time without having the big flag date when i2c_read() and i2c_write() are switched over all at once. When that new level of abstraction is used, we can hide all the details behind that and the implementation no longer influences any of the drivers. Then we could transparently rework adapters and muxes to our heart's content without needing to update users of the high-level API. > >frankly as long as this isn't handled in the core I don't think people > >will get it right. > > Yes, full ack, which is the goal from CONFIG_SYS_I2C. If all i2c > driver are converted to it, we can make i2c_set_bus_num() static, and > add to the i2c API the bus number as a function parameter! > > >>Why is i2c_set_bus_num() such a problem? > > > >Because it's completely confusing. And it's exposing an implementation > >detail to users instead of handling it transparently in the core. > > Yes! Full Ack ... but I do not accept a new API for that! Please > fix the i2c_read/write() functions! Doing this kind of conversion is a nightmare. We'd be changing an API that has around 600 occurrences in U-Boot, all of which need to be changed *at once* to avoid build breakage. At the same time we need to make sure any patches in development use the same API, which means that they can't even be build-tested until the the API has been changed. Transitioning step by step is a lot less complicated. Thierry
Hello Thierry, Am 27.08.2014 10:51, schrieb Thierry Reding: > On Wed, Aug 27, 2014 at 09:07:58AM +0200, Heiko Schocher wrote: >> Hello Thierry, >> >> Am 27.08.2014 08:21, schrieb Thierry Reding: >>> On Wed, Aug 27, 2014 at 07:21:51AM +0200, Heiko Schocher wrote: >>>> Hello Thierry, >>>> >>>> Am 26.08.2014 17:34, schrieb Thierry Reding: >>>>> From: Thierry Reding<treding@nvidia.com> >>>>> >>>>> This API operates on I2C adapters or I2C clients (a new type of object >>>> >>>> which is a bad idea ... >>>> >>>>> that refers to a particular slave connected to an adapter). This is >>>>> useful to avoid having to call i2c_set_bus_num() whenever a device is >>>>> being accessed. >>>> >>>> But thats the supose of i2c_set_bus_num()! ... if you use an i2c bus, >>>> you must check before every access, if you are on it, if not, you must >>>> switch back to it... >>> >>> That's not what code does today. Everybody calls i2c_set_bus_num() once >>> and then does a bunch of transactions on that bus. Given that U-Boot >> >> Yes, sadly. This has historical reasons ... >> >>> doesn't run multithreaded this works. If you're really concerned about >> >> Yes, U-Boot is singlethread only. >> >>> this being a problem, then it should be solved at a different level. It >>> could be part of i2c_client for example, so that i2c_client_read() and >>> i2c_client_write() would always perform this step. Burdening users with >> >> Exactly, thats right, and this is a goal from the CONFIG_SYS_I2C API! >> >> But why do you introduce i2c_client_read/write and do not add this step >> to i2c_read/write? >> >> - convert all i2c drivers, which are not yet converted to CONFIG_SYS_I2C >> (get also rid od CONFIG_HARD_I2C) >> - add busnumber to i2c_read/write API and make i2c_set_bus_num() static ... >> and fix all i2c_read/write() calls in U-Boot code ... > > I don't think adding a bus number as parameter is useful. Why not just > use the I2C adapter directly? That way we don't have to keep looking it > up in an array every time. You again just talk from i2c_adapter ... why you ignore i2c muxes? A bus is not only an i2c adapter ... Currently we have two "versions" of i2c_adapter: In a system without muxes, you can say: i2c bus == i2c adapter but in a system with muxes we have: i2c bus == i2c_bus_hose i2c commands use also a "bus number" starting from 0 ... the bus number has historical reasons... we could change this ... But if we introduce a new API, please with mux functionallity ... Hmm.. thinking about it ... if you want to introduce a new API, please start with using the DM for I2C! >> I know, this is a big change and a lot of work ... thats the reason >> why we are not at this point ... nobody volunteered to go forward, and >> I did not found time to do it ... > > I suppose that would be one possibility to do it. But I consider > i2c_client more of a convenience around the lower-level i2c_read() and > i2c_write(). The idea is that users set up an I2C client once and then > refer to the client, which will automatically use the correct adapter > and slave address rather than having that duplicated in every driver. Hmm... Ok, if we want to have a i2c_client struct instead an int ... What do others think? But, if we(you ;-) touch this, please start with using the DM for I2C! This is also a step which should be done, and I do not want to have another API, if somedays we find time to switch to DM! >>> this isn't going to work (in a multithreaded environment the switch to a >>> different mux could happen between the call to i2c_set_bus_num() and the >>> bus access). >>> >>> In fact I think this would even have to be solved at the controller >>> level if you want to make sure that client transactions are atomic. >> >> As U-Boot is single threaded all i2c_read/writes are atomic. > > In which case you don't have to call i2c_set_bus_num() for every access, > only whenever you don't know exactly where you're coming from. Functions > that perform a sequence of accesses only need to set it once. Yes ... which really is a pro for having i2c_set_bus_num() not static ... > Also, if we directly talk to an adapter instead, then the bulk of what You ignore again muxes ... > i2c_set_bus_num() does isn't even required. It would require that What does i2c_set_bus_num() ? - first check, if current bus = new bus, and if it is initialized if so -> all is good, return! Thats the main case ... is this so expensive? This checks should be always done (except we do a bulk of i2c accesses, yes). I think, this has also to be done somewhere with your approach ... or? What is done in the case, we switch to another bus: - If we have no muxes, save new bus for further use - look if new bus is initialized, if not initialize it All this is hidden from the user ... and actions, which must be done, whatever API you define ... If I understand you correct, your big change is not using "int bus" instead introducing i2c_client ... and define some functions, which use this struct as parameter ... Why not defining: [1]: int i2c_bus_read(int bus, uint8_t chip, unsigned int address, size_t alen, void *buffer, size_t size) { i2c_set_bus_num(bus); return i2c_read(chip, address, alen, buffer, size); } int i2c_bus_write(int bus, uint8_t chip, unsigned int address, size_t alen, void *buffer, size_t size) { i2c_set_bus_num(bus); return i2c_write(chip, address, alen, buffer, size); } and you never have to call something like i2c_init(), as i2c_set_bus_num() does this all for you! You only have to use in your driver i2c_bus_read/write ... without taking any attention ... looking deeper in your approach: +int i2c_client_init(struct i2c_client *client, struct i2c_adapter *adapter, + uint8_t address) +{ + client->adapter = adapter; + client->address = address; + + return 0; +} Where is here called adapter->init() ? Nor you check, if the i2c adapter is intialized ... if more than one driver uses this function, each intializes the hw? Or currently none :-( > adapters are made aware of a hierarchy if there are muxes, but I think > that's worthwhile to do in any case. Also if ever I2C muxing needs to > gain device tree support having the muxes set up dynamically will be > pretty much a prerequisite. > >>>> This is collected in i2c_set_bus_num() ... before, every "user" did >>>> this on his own ... if you are on the bus you want to access, the >>>> overhead is not so big, see: >>>> >>>> http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/i2c/i2c_core.c;h=18d6736601c161f45cb7d81b5eae53bdeaaf6b0b;hb=7bee1c91a94db19bd26f92cc67be35d3592c6429#l278 >>>> >>>> 278 int i2c_set_bus_num(unsigned int bus) >>>> 279 { >>>> 280 int max; >>>> 281 >>>> 282 if ((bus == I2C_BUS)&& (I2C_ADAP->init_done> 0)) >>>> 283 return 0; >>>> >>>> And you must be aware of i2c muxes! You directly use the read/write >>>> functions from the i2c adapter, but what is if you have i2c muxes? >>> >>> That's complexity that users shouldn't have to worry about. They should >> >> Exactly! >> >>> simply access an adapter and the adapter (or rather the core) should >>> take care of setting up any muxes correctly. >> >> Yes! >> >> I think you mix here i2c adapter with bus. An "U-Boot i2c adapter" is a >> hw adapter (or special case soft i2c adapter). An "i2c bus" is a hw adapter >> maybe with m muxes, and each bus has exactly one way through the >> i2c muxes, see for an example the README: >> >> http://git.denx.de/?p=u-boot.git;a=blob;f=README;h=14d6b227d689825025f9dfc98fb305021882446d;hb=7bee1c91a94db19bd26f92cc67be35d3592c6429#l2349 >> >> So the only thing a User must know when he wants to use an i2c bus is >> his number. The switching to this i2c adapter, initializing it and maybe >> set i2c muxes does the i2c subsystem ... > > The above doesn't preclude an I2C adapter representing one of the ports > of a mux. That way you can still talk to an adapter rather than having > to refer to the bus by number. Adapter would become a little more > abstract than it is now, since it would be simply an output that I2C Oh! > slaves are connected to (either a HW controller directly or a mux > connected to a HW controller). Thats sounds for me, that you should use DM ... I do not know, what your plans are! >>>> Maybe there is on one i2c adapter a i2c mux with 4 ports. On one is >>>> an eeprom, on the other is a PMIC ... your code in patch >>>> "power: Add AMS AS3722 PMIC support" does access with your functions >>>> the PMIC ... what is, if between this accesses someone accesses the eeprom? >>>> If he switches the mux, you never switch back! >>>> >>>> Your code did not check this! >>> >>> Like I said, a lot of code in U-Boot doesn't check this. And quite >> >> With using i2c_set_bus_num() you have not to check this! You only have >> to call i2c_set_bus_num() before calling i2c_read/write ... and yes, >> that would be nice, if we just pass the bus number to i2c_read/write() >> and drop the i2c_set_bus_num() call all over the code ... >> >> Patches welcome! > > How about a slightly different proposal: introduce a new level of > abstraction (like i2c_client) and start using it in new I2C slave > drivers. At the same time existing drivers could be converted one at a > time without having the big flag date when i2c_read() and i2c_write() > are switched over all at once. Sound like use/introduce DM for I2C! > When that new level of abstraction is used, we can hide all the > details behind that and the implementation no longer influences any of > the drivers. Then we could transparently rework adapters and muxes to > our heart's content without needing to update users of the high-level > API. Ok, with some functions like in [1], maybe you introduce i2c_client, and use this instead "int bus" ... >>> frankly as long as this isn't handled in the core I don't think people >>> will get it right. >> >> Yes, full ack, which is the goal from CONFIG_SYS_I2C. If all i2c >> driver are converted to it, we can make i2c_set_bus_num() static, and >> add to the i2c API the bus number as a function parameter! >> >>>> Why is i2c_set_bus_num() such a problem? >>> >>> Because it's completely confusing. And it's exposing an implementation >>> detail to users instead of handling it transparently in the core. >> >> Yes! Full Ack ... but I do not accept a new API for that! Please >> fix the i2c_read/write() functions! > > Doing this kind of conversion is a nightmare. We'd be changing an API Full Ack. > that has around 600 occurrences in U-Boot, all of which need to be > changed *at once* to avoid build breakage. At the same time we need to > make sure any patches in development use the same API, which means that > they can't even be build-tested until the the API has been changed. > > Transitioning step by step is a lot less complicated. Ok, it was worth a try ;-) So what do you think about defining functions like in [1] ? bye, Heiko
On Wed, Aug 27, 2014 at 11:56:41AM +0200, Heiko Schocher wrote: > Hello Thierry, > > Am 27.08.2014 10:51, schrieb Thierry Reding: > >On Wed, Aug 27, 2014 at 09:07:58AM +0200, Heiko Schocher wrote: > >>Hello Thierry, > >> > >>Am 27.08.2014 08:21, schrieb Thierry Reding: > >>>On Wed, Aug 27, 2014 at 07:21:51AM +0200, Heiko Schocher wrote: > >>>>Hello Thierry, > >>>> > >>>>Am 26.08.2014 17:34, schrieb Thierry Reding: > >>>>>From: Thierry Reding<treding@nvidia.com> > >>>>> > >>>>>This API operates on I2C adapters or I2C clients (a new type of object > >>>> > >>>>which is a bad idea ... > >>>> > >>>>>that refers to a particular slave connected to an adapter). This is > >>>>>useful to avoid having to call i2c_set_bus_num() whenever a device is > >>>>>being accessed. > >>>> > >>>>But thats the supose of i2c_set_bus_num()! ... if you use an i2c bus, > >>>>you must check before every access, if you are on it, if not, you must > >>>>switch back to it... > >>> > >>>That's not what code does today. Everybody calls i2c_set_bus_num() once > >>>and then does a bunch of transactions on that bus. Given that U-Boot > >> > >>Yes, sadly. This has historical reasons ... > >> > >>>doesn't run multithreaded this works. If you're really concerned about > >> > >>Yes, U-Boot is singlethread only. > >> > >>>this being a problem, then it should be solved at a different level. It > >>>could be part of i2c_client for example, so that i2c_client_read() and > >>>i2c_client_write() would always perform this step. Burdening users with > >> > >>Exactly, thats right, and this is a goal from the CONFIG_SYS_I2C API! > >> > >>But why do you introduce i2c_client_read/write and do not add this step > >>to i2c_read/write? > >> > >>- convert all i2c drivers, which are not yet converted to CONFIG_SYS_I2C > >> (get also rid od CONFIG_HARD_I2C) > >>- add busnumber to i2c_read/write API and make i2c_set_bus_num() static ... > >> and fix all i2c_read/write() calls in U-Boot code ... > > > >I don't think adding a bus number as parameter is useful. Why not just > >use the I2C adapter directly? That way we don't have to keep looking it > >up in an array every time. > > You again just talk from i2c_adapter ... why you ignore i2c muxes? > A bus is not only an i2c adapter ... I know. I keep saying i2c_adapter because in the rough sketch that I have in mind for how this could work eventually, an mux is just another special kind of i2c_adapter. > Currently we have two "versions" of i2c_adapter: > > In a system without muxes, you can say: i2c bus == i2c adapter > but in a system with muxes we have: i2c bus == i2c_bus_hose > > i2c commands use also a "bus number" starting from 0 ... the bus number > has historical reasons... we could change this ... > > But if we introduce a new API, please with mux functionallity ... > Hmm.. thinking about it ... if you want to introduce a new API, please > start with using the DM for I2C! I can look into it, but it sounds like a task way beyond what I have time for right now. > >>>this isn't going to work (in a multithreaded environment the switch to a > >>>different mux could happen between the call to i2c_set_bus_num() and the > >>>bus access). > >>> > >>>In fact I think this would even have to be solved at the controller > >>>level if you want to make sure that client transactions are atomic. > >> > >>As U-Boot is single threaded all i2c_read/writes are atomic. > > > >In which case you don't have to call i2c_set_bus_num() for every access, > >only whenever you don't know exactly where you're coming from. Functions > >that perform a sequence of accesses only need to set it once. > > Yes ... which really is a pro for having i2c_set_bus_num() not static ... Or if said functionality is encapsulated within adapters then they can be done as necessary with the same effect. > >Also, if we directly talk to an adapter instead, then the bulk of what > > You ignore again muxes ... > > >i2c_set_bus_num() does isn't even required. It would require that > > What does i2c_set_bus_num() ? > > - first check, if current bus = new bus, and if it is initialized > if so -> all is good, return! > > Thats the main case ... is this so expensive? This checks should > be always done (except we do a bulk of i2c accesses, yes). I think, > this has also to be done somewhere with your approach ... or? It's currently done as part of i2c_adapter_get() which as you already point out only work when muxes aren't used. But the main issue about the current approach is that we have these global indices and pass them around in global state and need to be careful to always set the current bus where appropriate. If we simply pass adapters around (provided they handle muxes correctly) we can simply address each slave as an (adapter, slave address) pair, rather than repeating the same arguments over and over and calling very low-level bus management functions. But we already agree that this isn't optimal. The disagreement seems to be mostly about the implementation details. > What is done in the case, we switch to another bus: > > - If we have no muxes, save new bus for further use > - look if new bus is initialized, if not initialize it > > All this is hidden from the user ... and actions, which must be done, > whatever API you define ... > > If I understand you correct, your big change is not using "int bus" > instead introducing i2c_client ... and define some functions, which > use this struct as parameter ... Correct. > Why not defining: > > [1]: > int i2c_bus_read(int bus, uint8_t chip, > unsigned int address, size_t alen, void *buffer, > size_t size) > { > i2c_set_bus_num(bus); > return i2c_read(chip, address, alen, buffer, size); > } > > int i2c_bus_write(int bus, uint8_t chip, > unsigned int address, size_t alen, void *buffer, > size_t size) > { > i2c_set_bus_num(bus); > return i2c_write(chip, address, alen, buffer, size); > } > > and you never have to call something like i2c_init(), as > i2c_set_bus_num() does this all for you! You only have to use in your > driver i2c_bus_read/write ... without taking any attention ... Yes, that sounds like a useful change in itself. But it still requires users to explicitly manage the bus and chip numbers. The goal of the i2c_client API is to standardize on how to talk to clients. Essentially the goal would be for a driver to get a hold of a struct i2c_client via some standard interface (perhaps with DM this could be done completely outside of the driver itself) and then talks to the device via this standard object. Currently drivers either need to hardcode the bus number and slave addresses, or they need to store them in some custom structure. i2c_client gives uniformity to that. There are really two parts of a puzzle here. One is the client-side API that drivers for slave devices use and the other is the I2C controller drivers that provide the struct i2c_adapter. > looking deeper in your approach: > > +int i2c_client_init(struct i2c_client *client, struct i2c_adapter *adapter, > + uint8_t address) > +{ > + client->adapter = adapter; > + client->address = address; > + > + return 0; > +} > > Where is here called adapter->init() ? Nor you check, if the i2c > adapter is intialized ... if more than one driver uses this function, > each intializes the hw? Or currently none :-( It's done as part of the i2c_adapter_get(). It takes an index (which is roughly the bus number) initializes the corresponding adapter (if found) and returns a pointer to it. To handle muxes I think this could work by adding hooks that could be called before doing transactions to make sure that the correct muxing is set up. > >adapters are made aware of a hierarchy if there are muxes, but I think > >that's worthwhile to do in any case. Also if ever I2C muxing needs to > >gain device tree support having the muxes set up dynamically will be > >pretty much a prerequisite. > > > >>>>This is collected in i2c_set_bus_num() ... before, every "user" did > >>>>this on his own ... if you are on the bus you want to access, the > >>>>overhead is not so big, see: > >>>> > >>>>http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/i2c/i2c_core.c;h=18d6736601c161f45cb7d81b5eae53bdeaaf6b0b;hb=7bee1c91a94db19bd26f92cc67be35d3592c6429#l278 > >>>> > >>>> 278 int i2c_set_bus_num(unsigned int bus) > >>>> 279 { > >>>> 280 int max; > >>>> 281 > >>>> 282 if ((bus == I2C_BUS)&& (I2C_ADAP->init_done> 0)) > >>>> 283 return 0; > >>>> > >>>>And you must be aware of i2c muxes! You directly use the read/write > >>>>functions from the i2c adapter, but what is if you have i2c muxes? > >>> > >>>That's complexity that users shouldn't have to worry about. They should > >> > >>Exactly! > >> > >>>simply access an adapter and the adapter (or rather the core) should > >>>take care of setting up any muxes correctly. > >> > >>Yes! > >> > >>I think you mix here i2c adapter with bus. An "U-Boot i2c adapter" is a > >>hw adapter (or special case soft i2c adapter). An "i2c bus" is a hw adapter > >>maybe with m muxes, and each bus has exactly one way through the > >>i2c muxes, see for an example the README: > >> > >>http://git.denx.de/?p=u-boot.git;a=blob;f=README;h=14d6b227d689825025f9dfc98fb305021882446d;hb=7bee1c91a94db19bd26f92cc67be35d3592c6429#l2349 > >> > >>So the only thing a User must know when he wants to use an i2c bus is > >>his number. The switching to this i2c adapter, initializing it and maybe > >>set i2c muxes does the i2c subsystem ... > > > >The above doesn't preclude an I2C adapter representing one of the ports > >of a mux. That way you can still talk to an adapter rather than having > >to refer to the bus by number. Adapter would become a little more > >abstract than it is now, since it would be simply an output that I2C > > Oh! > > >slaves are connected to (either a HW controller directly or a mux > >connected to a HW controller). > > Thats sounds for me, that you should use DM ... I do not know, what > your plans are! Yes, perhaps DM is part of the solution. But I don't have any concrete plans. I'd rather tackle this step by step rather than as one monolithic patch series. > >When that new level of abstraction is used, we can hide all the > >details behind that and the implementation no longer influences any of > >the drivers. Then we could transparently rework adapters and muxes to > >our heart's content without needing to update users of the high-level > >API. > > Ok, with some functions like in [1], maybe you introduce i2c_client, > and use this instead "int bus" ... This would probably work fine to approach the problem from two sides. An API like the i2c_client could be used to abstract away all the implementation details on the controller/mux side and then more work could be done to convert the adapter side of things to whatever works best. Thierry
Hi Thierry, On 27 August 2014 05:41, Thierry Reding <thierry.reding@gmail.com> wrote: > On Wed, Aug 27, 2014 at 11:56:41AM +0200, Heiko Schocher wrote: >> Hello Thierry, >> >> Am 27.08.2014 10:51, schrieb Thierry Reding: >> >On Wed, Aug 27, 2014 at 09:07:58AM +0200, Heiko Schocher wrote: >> >>Hello Thierry, >> >> >> >>Am 27.08.2014 08:21, schrieb Thierry Reding: >> >>>On Wed, Aug 27, 2014 at 07:21:51AM +0200, Heiko Schocher wrote: >> >>>>Hello Thierry, >> >>>> >> >>>>Am 26.08.2014 17:34, schrieb Thierry Reding: >> >>>>>From: Thierry Reding<treding@nvidia.com> >> >>>>> >> >>>>>This API operates on I2C adapters or I2C clients (a new type of object >> >>>> >> >>>>which is a bad idea ... >> >>>> >> >>>>>that refers to a particular slave connected to an adapter). This is >> >>>>>useful to avoid having to call i2c_set_bus_num() whenever a device is >> >>>>>being accessed. >> >>>> >> >>>>But thats the supose of i2c_set_bus_num()! ... if you use an i2c bus, >> >>>>you must check before every access, if you are on it, if not, you must >> >>>>switch back to it... >> >>> >> >>>That's not what code does today. Everybody calls i2c_set_bus_num() once >> >>>and then does a bunch of transactions on that bus. Given that U-Boot >> >> >> >>Yes, sadly. This has historical reasons ... >> >> >> >>>doesn't run multithreaded this works. If you're really concerned about >> >> >> >>Yes, U-Boot is singlethread only. >> >> >> >>>this being a problem, then it should be solved at a different level. It >> >>>could be part of i2c_client for example, so that i2c_client_read() and >> >>>i2c_client_write() would always perform this step. Burdening users with >> >> >> >>Exactly, thats right, and this is a goal from the CONFIG_SYS_I2C API! >> >> >> >>But why do you introduce i2c_client_read/write and do not add this step >> >>to i2c_read/write? >> >> >> >>- convert all i2c drivers, which are not yet converted to CONFIG_SYS_I2C >> >> (get also rid od CONFIG_HARD_I2C) >> >>- add busnumber to i2c_read/write API and make i2c_set_bus_num() static ... >> >> and fix all i2c_read/write() calls in U-Boot code ... >> > >> >I don't think adding a bus number as parameter is useful. Why not just >> >use the I2C adapter directly? That way we don't have to keep looking it >> >up in an array every time. >> >> You again just talk from i2c_adapter ... why you ignore i2c muxes? >> A bus is not only an i2c adapter ... > > I know. I keep saying i2c_adapter because in the rough sketch that I > have in mind for how this could work eventually, an mux is just another > special kind of i2c_adapter. > >> Currently we have two "versions" of i2c_adapter: >> >> In a system without muxes, you can say: i2c bus == i2c adapter >> but in a system with muxes we have: i2c bus == i2c_bus_hose >> >> i2c commands use also a "bus number" starting from 0 ... the bus number >> has historical reasons... we could change this ... >> >> But if we introduce a new API, please with mux functionallity ... >> Hmm.. thinking about it ... if you want to introduce a new API, please >> start with using the DM for I2C! > > I can look into it, but it sounds like a task way beyond what I have > time for right now. > I can help if you are interested. I have patches for SPI at u-boot-dm.git (branch working) and would like to have I2C. They are sort-of similar so it might not be to hard to convert Tegra I2C over. The concept of a 'current' bus is broken IMO. Maybe the command line should have this concept, and maintain a 'current hose' or whatever. But drives and other code should specific what they need with each transaction. With DM that will likely be automatic. Regards, Simon
Hello Simon, Am 27.08.2014 21:10, schrieb Simon Glass: > Hi Thierry, > > On 27 August 2014 05:41, Thierry Reding<thierry.reding@gmail.com> wrote: >> On Wed, Aug 27, 2014 at 11:56:41AM +0200, Heiko Schocher wrote: >>> Hello Thierry, >>> >>> Am 27.08.2014 10:51, schrieb Thierry Reding: >>>> On Wed, Aug 27, 2014 at 09:07:58AM +0200, Heiko Schocher wrote: >>>>> Hello Thierry, >>>>> >>>>> Am 27.08.2014 08:21, schrieb Thierry Reding: >>>>>> On Wed, Aug 27, 2014 at 07:21:51AM +0200, Heiko Schocher wrote: >>>>>>> Hello Thierry, >>>>>>> >>>>>>> Am 26.08.2014 17:34, schrieb Thierry Reding: >>>>>>>> From: Thierry Reding<treding@nvidia.com> >>>>>>>> >>>>>>>> This API operates on I2C adapters or I2C clients (a new type of object >>>>>>> >>>>>>> which is a bad idea ... >>>>>>> >>>>>>>> that refers to a particular slave connected to an adapter). This is >>>>>>>> useful to avoid having to call i2c_set_bus_num() whenever a device is >>>>>>>> being accessed. >>>>>>> >>>>>>> But thats the supose of i2c_set_bus_num()! ... if you use an i2c bus, >>>>>>> you must check before every access, if you are on it, if not, you must >>>>>>> switch back to it... >>>>>> >>>>>> That's not what code does today. Everybody calls i2c_set_bus_num() once >>>>>> and then does a bunch of transactions on that bus. Given that U-Boot >>>>> >>>>> Yes, sadly. This has historical reasons ... >>>>> >>>>>> doesn't run multithreaded this works. If you're really concerned about >>>>> >>>>> Yes, U-Boot is singlethread only. >>>>> >>>>>> this being a problem, then it should be solved at a different level. It >>>>>> could be part of i2c_client for example, so that i2c_client_read() and >>>>>> i2c_client_write() would always perform this step. Burdening users with >>>>> >>>>> Exactly, thats right, and this is a goal from the CONFIG_SYS_I2C API! >>>>> >>>>> But why do you introduce i2c_client_read/write and do not add this step >>>>> to i2c_read/write? >>>>> >>>>> - convert all i2c drivers, which are not yet converted to CONFIG_SYS_I2C >>>>> (get also rid od CONFIG_HARD_I2C) >>>>> - add busnumber to i2c_read/write API and make i2c_set_bus_num() static ... >>>>> and fix all i2c_read/write() calls in U-Boot code ... >>>> >>>> I don't think adding a bus number as parameter is useful. Why not just >>>> use the I2C adapter directly? That way we don't have to keep looking it >>>> up in an array every time. >>> >>> You again just talk from i2c_adapter ... why you ignore i2c muxes? >>> A bus is not only an i2c adapter ... >> >> I know. I keep saying i2c_adapter because in the rough sketch that I >> have in mind for how this could work eventually, an mux is just another >> special kind of i2c_adapter. >> >>> Currently we have two "versions" of i2c_adapter: >>> >>> In a system without muxes, you can say: i2c bus == i2c adapter >>> but in a system with muxes we have: i2c bus == i2c_bus_hose >>> >>> i2c commands use also a "bus number" starting from 0 ... the bus number >>> has historical reasons... we could change this ... >>> >>> But if we introduce a new API, please with mux functionallity ... >>> Hmm.. thinking about it ... if you want to introduce a new API, please >>> start with using the DM for I2C! >> >> I can look into it, but it sounds like a task way beyond what I have >> time for right now. >> > > I can help if you are interested. I have patches for SPI at > u-boot-dm.git (branch working) and would like to have I2C. They are > sort-of similar so it might not be to hard to convert Tegra I2C over. > > The concept of a 'current' bus is broken IMO. Maybe the command line > should have this concept, and maintain a 'current hose' or whatever. > But drives and other code should specific what they need with each > transaction. With DM that will likely be automatic. That sounds great! I vote for using DM. bye, Heiko
diff --git a/drivers/i2c/i2c_core.c b/drivers/i2c/i2c_core.c index f6179a16244b..88c7af546b0b 100644 --- a/drivers/i2c/i2c_core.c +++ b/drivers/i2c/i2c_core.c @@ -410,3 +410,56 @@ void __i2c_init(int speed, int slaveaddr) } void i2c_init(int speed, int slaveaddr) __attribute__((weak, alias("__i2c_init"))); + +struct i2c_adapter *i2c_adapter_get(unsigned int index) +{ + struct i2c_adapter *adapter = ll_entry_start(struct i2c_adapter, i2c); + unsigned int num = ll_entry_count(struct i2c_adapter, i2c); + unsigned int i; + + if (index >= num) + return NULL; + + for (i = 0; i < index; i++) + adapter++; + + i2c_adapter_init(adapter, adapter->speed, adapter->slaveaddr); + return adapter; +} + +int i2c_adapter_read(struct i2c_adapter *adapter, uint8_t chip, + unsigned int address, size_t alen, void *buffer, + size_t size) +{ + return adapter->read(adapter, chip, address, alen, buffer, size); +} + +int i2c_adapter_write(struct i2c_adapter *adapter, uint8_t chip, + unsigned int address, size_t alen, void *buffer, + size_t size) +{ + return adapter->write(adapter, chip, address, alen, buffer, size); +} + +int i2c_client_init(struct i2c_client *client, struct i2c_adapter *adapter, + uint8_t address) +{ + client->adapter = adapter; + client->address = address; + + return 0; +} + +int i2c_client_read(struct i2c_client *client, unsigned int address, + size_t alen, void *buffer, size_t size) +{ + return i2c_adapter_read(client->adapter, client->address, address, + alen, buffer, size); +} + +int i2c_client_write(struct i2c_client *client, unsigned int address, + size_t alen, void *buffer, size_t size) +{ + return i2c_adapter_write(client->adapter, client->address, address, + alen, buffer, size); +} diff --git a/include/i2c.h b/include/i2c.h index 1b4078ed62fe..8f73ba93c614 100644 --- a/include/i2c.h +++ b/include/i2c.h @@ -55,6 +55,20 @@ #define CONFIG_SYS_SPD_BUS_NUM 0 #endif +/** + * struct i2c_adapter - I2C adapter + * @init: initialize I2C adapter + * @probe: probe for a device on the I2C bus + * @read: read data from a device on the I2C bus + * @write: write data to a device on the I2C bus + * @set_bus_speed: configure the I2C bus speed + * @speed: current I2C bus speed + * @waitdelay: + * @slaveaddr: own address (when used as a slave) + * @init_done: flag to indicate whether or not the adapter has been initialized + * @hwadapnr: the hardware number of this adapter + * @name: name of this adapter + */ struct i2c_adapter { void (*init)(struct i2c_adapter *adap, int speed, int slaveaddr); @@ -75,6 +89,16 @@ struct i2c_adapter { char *name; }; +/** + * struct i2c_client - I2C slave + * @adapter: I2C adapter providing the slave's parent bus + * address: address of the I2C slave on the parent bus + */ +struct i2c_client { + struct i2c_adapter *adapter; + unsigned int address; +}; + #define U_BOOT_I2C_MKENT_COMPLETE(_init, _probe, _read, _write, \ _set_speed, _speed, _slaveaddr, _hwadapnr, _name) \ { \ @@ -271,6 +295,78 @@ unsigned int i2c_get_bus_speed(void); * Adjusts I2C pointers after U-Boot is relocated to DRAM */ void i2c_reloc_fixup(void); + +/* + * i2c_adapter_get() - get the I2C adapter associated with a given index + * @index: index of the I2C adapter + */ +struct i2c_adapter *i2c_adapter_get(unsigned int index); + +/* + * i2c_adapter_read() - read data from an I2C slave at a given address + * @adapter: I2C adapter + * @chip: address of the I2C slave to read from + * @address: address within the I2C slave to read from + * @alen: length of address + * @buffer: buffer to receive data from I2C slave + * @size: number of bytes to read + */ +int i2c_adapter_read(struct i2c_adapter *adapter, uint8_t chip, + unsigned int address, size_t alen, void *buffer, + size_t size); + +/* + * i2c_adapter_write() - write data to an I2C slave at a given address + * @adapter: I2C adapter + * @chip: address of the I2C slave to write to + * @address: address within the I2C slave to write to + * @alen: length of address + * @buffer: buffer containing the data to write + * @size: number of bytes to write + * + * Ideally the function would take a const void * buffer, but the underlying + * infrastructure doesn't properly propagate const and adding it here would + * cause a lot of build warnings. + */ +int i2c_adapter_write(struct i2c_adapter *adapter, uint8_t chip, + unsigned int address, size_t alen, void *buffer, + size_t size); + +/* + * i2c_client_init() - initialize an I2C slave + * @client: I2C slave + * @adapter: parent I2C adapter + * @address: address of I2C slave + */ +int i2c_client_init(struct i2c_client *client, struct i2c_adapter *adapter, + uint8_t address); + +/* + * i2c_client_read() - read data from an I2C slave + * @client: I2C slave + * @address: address within the I2C slave to read from + * @alen: length of address + * @buffer: buffer to receive data from I2C slave + * @size: number of bytes to read + */ +int i2c_client_read(struct i2c_client *client, unsigned int address, + size_t alen, void *buffer, size_t size); + +/* + * i2c_client_write() - write data to an I2C slave + * @client: I2C slave + * @address: address within the I2C slave to write to + * @alen: length of address + * @buffer: buffer containing the data to write + * @size: number of bytes to write + * + * Ideally the function would take a const void * buffer, but the underlying + * infrastructure doesn't properly propagate const and adding it here would + * cause a lot of build warnings. + */ +int i2c_client_write(struct i2c_client *client, unsigned int address, + size_t alen, void *buffer, size_t size); + #if defined(CONFIG_SYS_I2C_SOFT) void i2c_soft_init(void); void i2c_soft_active(void);