Clean up address probing and validity checking code
Subsystems (2)
cover Clean up address probing and validity checking code
Here comes an update of the patchset I posted on April 9th, 2010. Now applies cleanly on top of Linus' tree.
[PATCH 1/4] i2c: Share the I2C device presence detection code [PATCH 2/4] i2c: Check for address validity on client registration [PATCH 3/4] i2c: Document reserved I2C addresses
[PATCH 4/4] i2c: Rename i2c_check_addr to i2c_check_addr_busy
Signature · 1 line
Jean Delvare
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH 1/4 i2c: Share the I2C device presence detection code
Use the same I2C device presence detection code for legacy and new device detection functions. This is more consistent and makes the code smaller.
--- drivers/i2c/i2c-core.c | 80 ++++++++++++++++++++++++------------------------ 1 file changed, 40 insertions(+), 40 deletions(-)
drivers/i2c/i2c-core.c +40 -40
@@ -1277,6 +1277,41 @@ EXPORT_SYMBOL(i2c_master_recv);* ---------------------------------------------------- */ +/* +* Legacy default probe function, mostly relevant for SMBus. The default +* probe method is a quick write, but it is known to corrupt the 24RF08 +* EEPROMs due to a state machine bug, and could also irreversibly +* write-protect some EEPROMs, so for address ranges 0x30-0x37 and 0x50-0x5f, +* we use a short byte read instead. Also, some bus drivers don't implement +* quick write, so we fallback to a byte read in that case too. +* On x86, there is another special case for FSC hardware monitoring chips, +* which want regular byte reads (address 0x73.) Fortunately, these are the +* only known chips using this I2C address on PC hardware. +* Returns 1 if probe succeeded, 0 if not. +*/ +static int i2c_default_probe (struct i2c_adapter *adap,unsigned short addr) +{ +int err; +union i2c_smbus_data dummy; + +#ifdef CONFIG_X86 +if (addr ==0x73 && (adap->class &I2C_CLASS_HWMON ) + &&i2c_check_functionality (adap,I2C_FUNC_SMBUS_READ_BYTE_DATA )) + err =i2c_smbus_xfer (adap, addr,0 ,I2C_SMBUS_READ ,0 , +I2C_SMBUS_BYTE_DATA , &dummy); + else +#endif +if ((addr & ~0x07 ) ==0x30 || (addr & ~0x0f ) ==0x50 + || !i2c_check_functionality (adap,I2C_FUNC_SMBUS_QUICK )) + err =i2c_smbus_xfer (adap, addr,0 ,I2C_SMBUS_READ ,0 , +I2C_SMBUS_BYTE , &dummy); +else + err =i2c_smbus_xfer (adap, addr,0 ,I2C_SMBUS_WRITE ,0 , +I2C_SMBUS_QUICK ,NULL ); + +return err >=0 ; +} +static int i2c_detect_address (struct i2c_client *temp_client,struct i2c_driver *driver) {
@@ -1297,23 +1332,8 @@ static int i2c_detect_address(struct i2creturn 0 ;/* Make sure there is something at this address */ -if (addr ==0x73 && (adapter->class &I2C_CLASS_HWMON )) { -/* Special probe for FSC hwmon chips */ -union i2c_smbus_data dummy; - -if (i2c_smbus_xfer (adapter, addr,0 ,I2C_SMBUS_READ ,0 , -I2C_SMBUS_BYTE_DATA , &dummy) <0 ) -return 0 ; - }else { -if (i2c_smbus_xfer (adapter, addr,0 ,I2C_SMBUS_WRITE ,0 , -I2C_SMBUS_QUICK ,NULL ) <0 ) -return 0 ; - -/* Prevent 24RF08 corruption */ -if ((addr & ~0x0f ) ==0x50 ) -i2c_smbus_xfer (adapter, addr,0 ,I2C_SMBUS_WRITE ,0 , -I2C_SMBUS_QUICK ,NULL ); - } +if (!i2c_default_probe (adapter, addr)) +return 0 ;/* Finally call the custom detection function */ memset (&info,0 ,sizeof (struct i2c_board_info ));
@@ -1420,29 +1440,9 @@ i2c_new_probed_device(struct i2c_adaptercontinue ; } -/* Test address responsiveness -The default probe method is a quick write, but it is known -to corrupt the 24RF08 EEPROMs due to a state machine bug, -and could also irreversibly write-protect some EEPROMs, so -for address ranges 0x30-0x37 and 0x50-0x5f, we use a byte -read instead. Also, some bus drivers don't implement -quick write, so we fallback to a byte read it that case -too. */ -if ((addr_list[i] & ~0x07 ) ==0x30 - || (addr_list[i] & ~0x0f ) ==0x50 - || !i2c_check_functionality (adap,I2C_FUNC_SMBUS_QUICK )) { -union i2c_smbus_data data; - -if (i2c_smbus_xfer (adap, addr_list[i],0 , -I2C_SMBUS_READ ,0 , -I2C_SMBUS_BYTE , &data) >=0 ) -break ; - }else { -if (i2c_smbus_xfer (adap, addr_list[i],0 , -I2C_SMBUS_WRITE ,0 , -I2C_SMBUS_QUICK ,NULL ) >=0 ) -break ; - } +/* Test address responsiveness */ +if (i2c_default_probe (adap, addr_list[i])) +break ; }if (addr_list[i] ==I2C_CLIENT_END ) {
Signature · 1 line
Jean Delvare
PATCH 2/4 i2c: Check for address validity on client registration
In reply to a message not in the archiveDo basic address validity checks when a client is being registered. We already had checks in place for devices which are being detected, but not for devices which are simply instantiated.
This is a very basic check. We don't want to do strict checking here because some devices are known to infringe the I2C address constraints (e.g. IR receivers at 7-bit address 0x7a while this value is supposedly reserved for 10-bit addresses.) So we assume the caller knows what it is doing.
--- drivers/i2c/i2c-core.c | 33 ++++++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 7 deletions(-)
drivers/i2c/i2c-core.c +26 -7
@@ -371,6 +371,22 @@ struct i2c_client *i2c_verify_client(strEXPORT_SYMBOL (i2c_verify_client); +/* This is a permissive address validity check, I2C address map constraints +* are purposedly not enforced, except for the general call address. */ +static int i2c_check_client_addr_validity (const struct i2c_client *client) +{ +if (client->flags &I2C_CLIENT_TEN ) { +/* 10-bit address, all values are valid */ +if (client->addr >0x3ff ) +return -EINVAL ; + }else { +/* 7-bit address, reject the general call address */ +if (client->addr ==0x00 || client->addr >0x7f ) +return -EINVAL ; + } +return 0 ; +} +/** * i2c_new_device - instantiate an i2c device * @adap: the adapter managing the device
@@ -410,6 +426,14 @@ i2c_new_device(struct i2c_adapter *adap,strlcpy (client->name, info->type,sizeof (client->name)); +/* Check for address validity */ + status =i2c_check_client_addr_validity (client); +if (status) { +dev_err (&adap->dev,"Invalid %d-bit I2C address 0x%02hx\n" , + client->flags &I2C_CLIENT_TEN ?10 :7 , client->addr); + goto out_err_silent; + } +/* Check for address business */ status =i2c_check_addr (adap, client->addr);if (status)
@@ -436,6 +460,7 @@ i2c_new_device(struct i2c_adapter *adap, out_err:dev_err (&adap->dev,"Failed to register i2c client %s at 0x%02x " "(%d)\n" , client->name, client->addr, status); +out_err_silent:kfree (client);return NULL ; }
@@ -561,15 +586,9 @@ i2c_sysfs_new_device(struct device *dev,return -EINVAL ; } -if (info.addr <0x03 || info.addr >0x77 ) { -dev_err (dev,"%s: Invalid I2C address 0x%hx\n" ,"new_device" , - info.addr); -return -EINVAL ; - } - client =i2c_new_device (adap, &info);if (!client) -return -EEXIST ; +return -EINVAL ;/* Keep track of the added device */ i2c_lock_adapter (adap);
Signature · 1 line
Jean Delvare
PATCH 3/4 i2c: Document reserved I2C addresses
In reply to a message not in the archiveMove strict I2C address validity check to a single function, and document the reserved I2C addresses there.
--- drivers/i2c/i2c-core.c | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-)
drivers/i2c/i2c-core.c +25 -3
@@ -387,6 +387,27 @@ static int i2c_check_client_addr_validitreturn 0 ; } +/* And this is a strict address validity check, used when probing. If a +* device uses a reserved address, then it shouldn't be probed. 7-bit +* addressing is assumed, 10-bit address devices are rare and should be +* explicitly enumerated. */ +static int i2c_check_addr_validity (unsigned short addr) +{ +/* +* Reserved addresses per I2C specification: +* 0x00 General call address / START byte +* 0x01 CBUS address +* 0x02 Reserved for different bus format +* 0x03 Reserved for future purposes +* 0x04-0x07 Hs-mode master code +* 0x78-0x7b 10-bit slave addressing +* 0x7c-0x7f Reserved for future purposes +*/ +if (addr <0x08 || addr >0x77 ) +return -EINVAL ; +return 0 ; +} +/** * i2c_new_device - instantiate an i2c device * @adap: the adapter managing the device
@@ -1340,10 +1361,11 @@ static int i2c_detect_address(struct i2cint err;/* Make sure the address is valid */ -if (addr <0x03 || addr >0x77 ) { + err =i2c_check_addr_validity (addr); +if (err) {dev_warn (&adapter->dev,"Invalid probe address 0x%02x\n" , addr); -return -EINVAL ; +return err; }/* Skip if already in use */
@@ -1446,7 +1468,7 @@ i2c_new_probed_device(struct i2c_adapterfor (i =0 ; addr_list[i] !=I2C_CLIENT_END ; i++) {/* Check address validity */ -if (addr_list[i] <0x03 || addr_list[i] >0x77 ) { +if (i2c_check_addr_validity (addr_list[i]) <0 ) {dev_warn (&adap->dev,"Invalid 7-bit address " "0x%02x\n" , addr_list[i]);continue ;
Signature · 1 line
Jean Delvare
PATCH 4/4 i2c: Rename i2c_check_addr to i2c_check_addr_busy
In reply to a message not in the archiveOtherwise it's not clear what it is checking.
Also move the function to save a forward declaration.
--- drivers/i2c/i2c-core.c | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-)
drivers/i2c/i2c-core.c +19 -19
@@ -47,7 +47,6 @@ static DEFINE_MUTEX(core_lock);static DEFINE_IDR (i2c_adapter_idr);static struct device_type i2c_client_type; -static int i2c_check_addr (struct i2c_adapter *adapter,int addr);static int i2c_detect (struct i2c_adapter *adapter,struct i2c_driver *driver);/* ------------------------------------------------------------------------- */
@@ -408,6 +407,22 @@ static int i2c_check_addr_validity(unsigreturn 0 ; } +static int __i2c_check_addr_busy (struct device *dev,void *addrp) +{ +struct i2c_client *client =i2c_verify_client (dev); +int addr = *(int *)addrp; + +if (client && client->addr == addr) +return -EBUSY ; +return 0 ; +} + +static int i2c_check_addr_busy (struct i2c_adapter *adapter,int addr) +{ +return device_for_each_child (&adapter->dev, &addr, + __i2c_check_addr_busy); +} +/** * i2c_new_device - instantiate an i2c device * @adap: the adapter managing the device
@@ -456,7 +471,7 @@ i2c_new_device(struct i2c_adapter *adap, }/* Check for address business */ - status =i2c_check_addr (adap, client->addr); + status =i2c_check_addr_busy (adap, client->addr);if (status) goto out_err;
@@ -1064,21 +1079,6 @@ EXPORT_SYMBOL(i2c_del_driver);/* ------------------------------------------------------------------------- */ -static int __i2c_check_addr (struct device *dev,void *addrp) -{ -struct i2c_client *client =i2c_verify_client (dev); -int addr = *(int *)addrp; - -if (client && client->addr == addr) -return -EBUSY ; -return 0 ; -} - -static int i2c_check_addr (struct i2c_adapter *adapter,int addr) -{ -return device_for_each_child (&adapter->dev, &addr, __i2c_check_addr); -} -/** * i2c_use_client - increments the reference count of the i2c client structure * @client: the client being referenced
@@ -1369,7 +1369,7 @@ static int i2c_detect_address(struct i2c }/* Skip if already in use */ -if (i2c_check_addr (adapter, addr)) +if (i2c_check_addr_busy (adapter, addr))return 0 ;/* Make sure there is something at this address */
@@ -1475,7 +1475,7 @@ i2c_new_probed_device(struct i2c_adapter }/* Check address availability */ -if (i2c_check_addr (adap, addr_list[i])) { +if (i2c_check_addr_busy (adap, addr_list[i])) {dev_dbg (&adap->dev,"Address 0x%02x already in " "use, not probing\n" , addr_list[i]);continue ;
Signature · 1 line
Jean Delvare