[RFC,1/2] i2c: add i2c_get_device_id() to get the standard i2c device id

Message ID 20180122113657.32094-2-peda@axentia.se
State Changes Requested
Headers show
Series
  • check I2C device id for pca984x chips
Related show

Commit Message

Peter Rosin Jan. 22, 2018, 11:36 a.m.
Can be used during probe to double check that the probed device is
what is expected.

Loosely based on code from Adrian Fiergolski <adrian.fiergolski@cern.ch>.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/i2c/i2c-core-base.c | 33 +++++++++++++++++++++++++++++++++
 include/linux/i2c.h         | 30 ++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+)

Comments

Peter Rosin March 4, 2018, 9:47 p.m. | #1
Hi Wolfram,

Did you have an opinion on this patch?

In case you like it, do you which to take it (don't forget to add a
Tested-by-tag from Adrian in that case), or should I take it and you
then get it in a pull request?

Cheers,
Peter

On 2018-01-22 12:36, Peter Rosin wrote:
> Can be used during probe to double check that the probed device is
> what is expected.
> 
> Loosely based on code from Adrian Fiergolski <adrian.fiergolski@cern.ch>.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
>  drivers/i2c/i2c-core-base.c | 33 +++++++++++++++++++++++++++++++++
>  include/linux/i2c.h         | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 63 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 5a00bf443d06..f496b917529d 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -58,6 +58,8 @@
>  #define I2C_ADDR_7BITS_MAX	0x77
>  #define I2C_ADDR_7BITS_COUNT	(I2C_ADDR_7BITS_MAX + 1)
>  
> +#define I2C_ADDR_DEVICE_ID	0x7c
> +
>  /*
>   * core_lock protects i2c_adapter_idr, and guarantees that device detection,
>   * deletion of detected devices, and attach_adapter calls are serialized
> @@ -1968,6 +1970,37 @@ int i2c_transfer_buffer_flags(const struct i2c_client *client, char *buf,
>  }
>  EXPORT_SYMBOL(i2c_transfer_buffer_flags);
>  
> +/**
> + * i2c_get_device_id - get manufacturer, part id and die revision of a device
> + * @client: The device to query
> + * @id: The queried information
> + *
> + * Returns negative errno on error, zero on success.
> + */
> +int i2c_get_device_id(const struct i2c_client *client,
> +		      struct i2c_device_identity *id)
> +{
> +	struct i2c_adapter *adap = client->adapter;
> +	union i2c_smbus_data raw_id;
> +	int ret;
> +
> +	if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_READ_I2C_BLOCK))
> +		return -EOPNOTSUPP;
> +
> +	raw_id.block[0] = 3;
> +	ret = i2c_smbus_xfer(adap, I2C_ADDR_DEVICE_ID, client->flags,
> +			     I2C_SMBUS_READ, client->addr << 1,
> +			     I2C_SMBUS_I2C_BLOCK_DATA, &raw_id);
> +	if (ret)
> +		return ret;
> +
> +	id->manufacturer_id = (raw_id.block[1] << 4) | (raw_id.block[2] >> 4);
> +	id->part_id = ((raw_id.block[2] & 0xf) << 5) | (raw_id.block[3] >> 3);
> +	id->die_revision = raw_id.block[3] & 0x7;
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(i2c_get_device_id);
> +
>  /* ----------------------------------------------------
>   * the i2c address scanning function
>   * Will not work for 10-bit addresses!
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 419a38e7c315..44ad14e016b5 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -47,6 +47,7 @@ struct i2c_algorithm;
>  struct i2c_adapter;
>  struct i2c_client;
>  struct i2c_driver;
> +struct i2c_device_identity;
>  union i2c_smbus_data;
>  struct i2c_board_info;
>  enum i2c_slave_event;
> @@ -186,8 +187,37 @@ extern s32 i2c_smbus_write_i2c_block_data(const struct i2c_client *client,
>  extern s32
>  i2c_smbus_read_i2c_block_data_or_emulated(const struct i2c_client *client,
>  					  u8 command, u8 length, u8 *values);
> +int i2c_get_device_id(const struct i2c_client *client,
> +		      struct i2c_device_identity *id);
>  #endif /* I2C */
>  
> +/**
> + * struct i2c_device_identity - i2c client device identification
> + * @manufacturer_id: 0 - 4095, database maintained by NXP
> + * @part_id: 0 - 511, according to manufacturer
> + * @die_revision: 0 - 7, according to manufacturer
> + */
> +struct i2c_device_identity {
> +	u16 manufacturer_id;
> +#define I2C_DEVICE_ID_NXP_SEMICONDUCTORS                0
> +#define I2C_DEVICE_ID_NXP_SEMICONDUCTORS_1              1
> +#define I2C_DEVICE_ID_NXP_SEMICONDUCTORS_2              2
> +#define I2C_DEVICE_ID_NXP_SEMICONDUCTORS_3              3
> +#define I2C_DEVICE_ID_RAMTRON_INTERNATIONAL             4
> +#define I2C_DEVICE_ID_ANALOG_DEVICES                    5
> +#define I2C_DEVICE_ID_STMICROELECTRONICS                6
> +#define I2C_DEVICE_ID_ON_SEMICONDUCTOR                  7
> +#define I2C_DEVICE_ID_SPRINTEK_CORPORATION              8
> +#define I2C_DEVICE_ID_ESPROS_PHOTONICS_AG               9
> +#define I2C_DEVICE_ID_FUJITSU_SEMICONDUCTOR            10
> +#define I2C_DEVICE_ID_FLIR                             11
> +#define I2C_DEVICE_ID_O2MICRO                          12
> +#define I2C_DEVICE_ID_ATMEL                            13
> +#define I2C_DEVICE_ID_NONE                         0xffff
> +	u16 part_id;
> +	u8 die_revision;
> +};
> +
>  enum i2c_alert_protocol {
>  	I2C_PROTOCOL_SMBUS_ALERT,
>  	I2C_PROTOCOL_SMBUS_HOST_NOTIFY,
>
Wolfram Sang March 5, 2018, 3:51 p.m. | #2
On Mon, Jan 22, 2018 at 12:36:56PM +0100, Peter Rosin wrote:
> Can be used during probe to double check that the probed device is
> what is expected.
> 
> Loosely based on code from Adrian Fiergolski <adrian.fiergolski@cern.ch>.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>

In general, nice! I wanted to have such a function in the core but never
had a device to test it with. So, much appreciated.

Looks mostly good, except...

> +	ret = i2c_smbus_xfer(adap, I2C_ADDR_DEVICE_ID, client->flags,

We shouldn't pass the flag of the clients (like PEC) here. I'd think it
could be plain 0 but please double-check.

> +/**
> + * struct i2c_device_identity - i2c client device identification
> + * @manufacturer_id: 0 - 4095, database maintained by NXP
> + * @part_id: 0 - 511, according to manufacturer
> + * @die_revision: 0 - 7, according to manufacturer
> + */

All is nicely documented, very good!

About the upstreaming procedure: Could you just make a seperate
pull-request out of this feature? I'll pull that in to have it in my
tree and you can still collect patches in your usual for-next branch.

When the above is fixed you can add my:

Reviewed-by: Wolfram Sang <wsa@the-dreams.de>
Peter Rosin March 5, 2018, 4:06 p.m. | #3
On 2018-03-05 16:51, Wolfram Sang wrote:
> On Mon, Jan 22, 2018 at 12:36:56PM +0100, Peter Rosin wrote:
>> Can be used during probe to double check that the probed device is
>> what is expected.
>>
>> Loosely based on code from Adrian Fiergolski <adrian.fiergolski@cern.ch>.
>>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
> 
> In general, nice! I wanted to have such a function in the core but never
> had a device to test it with. So, much appreciated.
> 
> Looks mostly good, except...
> 
>> +	ret = i2c_smbus_xfer(adap, I2C_ADDR_DEVICE_ID, client->flags,
> 
> We shouldn't pass the flag of the clients (like PEC) here. I'd think it
> could be plain 0 but please double-check.

Right,
..._TEN should definitely be cleared.
..._SLAVE will probably be cleared anyway.
..._HOST_NOTIFY, ..._WAKE and ..._SCCB I don't know about?

Are there others? The bits aren't that densely populated which makes
me worry that I'm missing something...

Cheers,
Peter

>> +/**
>> + * struct i2c_device_identity - i2c client device identification
>> + * @manufacturer_id: 0 - 4095, database maintained by NXP
>> + * @part_id: 0 - 511, according to manufacturer
>> + * @die_revision: 0 - 7, according to manufacturer
>> + */
> 
> All is nicely documented, very good!
> 
> About the upstreaming procedure: Could you just make a seperate
> pull-request out of this feature? I'll pull that in to have it in my
> tree and you can still collect patches in your usual for-next branch.
> 
> When the above is fixed you can add my:
> 
> Reviewed-by: Wolfram Sang <wsa@the-dreams.de>
>
Wolfram Sang March 5, 2018, 4:27 p.m. | #4
> ..._TEN should definitely be cleared.
> ..._SLAVE will probably be cleared anyway.
> ..._HOST_NOTIFY, ..._WAKE and ..._SCCB I don't know about?

Not relevant for device_id reading.

> Are there others? The bits aren't that densely populated which makes
> me worry that I'm missing something...

Historic reasons, there are no more.

Patch

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 5a00bf443d06..f496b917529d 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -58,6 +58,8 @@ 
 #define I2C_ADDR_7BITS_MAX	0x77
 #define I2C_ADDR_7BITS_COUNT	(I2C_ADDR_7BITS_MAX + 1)
 
+#define I2C_ADDR_DEVICE_ID	0x7c
+
 /*
  * core_lock protects i2c_adapter_idr, and guarantees that device detection,
  * deletion of detected devices, and attach_adapter calls are serialized
@@ -1968,6 +1970,37 @@  int i2c_transfer_buffer_flags(const struct i2c_client *client, char *buf,
 }
 EXPORT_SYMBOL(i2c_transfer_buffer_flags);
 
+/**
+ * i2c_get_device_id - get manufacturer, part id and die revision of a device
+ * @client: The device to query
+ * @id: The queried information
+ *
+ * Returns negative errno on error, zero on success.
+ */
+int i2c_get_device_id(const struct i2c_client *client,
+		      struct i2c_device_identity *id)
+{
+	struct i2c_adapter *adap = client->adapter;
+	union i2c_smbus_data raw_id;
+	int ret;
+
+	if (!i2c_check_functionality(adap, I2C_FUNC_SMBUS_READ_I2C_BLOCK))
+		return -EOPNOTSUPP;
+
+	raw_id.block[0] = 3;
+	ret = i2c_smbus_xfer(adap, I2C_ADDR_DEVICE_ID, client->flags,
+			     I2C_SMBUS_READ, client->addr << 1,
+			     I2C_SMBUS_I2C_BLOCK_DATA, &raw_id);
+	if (ret)
+		return ret;
+
+	id->manufacturer_id = (raw_id.block[1] << 4) | (raw_id.block[2] >> 4);
+	id->part_id = ((raw_id.block[2] & 0xf) << 5) | (raw_id.block[3] >> 3);
+	id->die_revision = raw_id.block[3] & 0x7;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(i2c_get_device_id);
+
 /* ----------------------------------------------------
  * the i2c address scanning function
  * Will not work for 10-bit addresses!
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 419a38e7c315..44ad14e016b5 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -47,6 +47,7 @@  struct i2c_algorithm;
 struct i2c_adapter;
 struct i2c_client;
 struct i2c_driver;
+struct i2c_device_identity;
 union i2c_smbus_data;
 struct i2c_board_info;
 enum i2c_slave_event;
@@ -186,8 +187,37 @@  extern s32 i2c_smbus_write_i2c_block_data(const struct i2c_client *client,
 extern s32
 i2c_smbus_read_i2c_block_data_or_emulated(const struct i2c_client *client,
 					  u8 command, u8 length, u8 *values);
+int i2c_get_device_id(const struct i2c_client *client,
+		      struct i2c_device_identity *id);
 #endif /* I2C */
 
+/**
+ * struct i2c_device_identity - i2c client device identification
+ * @manufacturer_id: 0 - 4095, database maintained by NXP
+ * @part_id: 0 - 511, according to manufacturer
+ * @die_revision: 0 - 7, according to manufacturer
+ */
+struct i2c_device_identity {
+	u16 manufacturer_id;
+#define I2C_DEVICE_ID_NXP_SEMICONDUCTORS                0
+#define I2C_DEVICE_ID_NXP_SEMICONDUCTORS_1              1
+#define I2C_DEVICE_ID_NXP_SEMICONDUCTORS_2              2
+#define I2C_DEVICE_ID_NXP_SEMICONDUCTORS_3              3
+#define I2C_DEVICE_ID_RAMTRON_INTERNATIONAL             4
+#define I2C_DEVICE_ID_ANALOG_DEVICES                    5
+#define I2C_DEVICE_ID_STMICROELECTRONICS                6
+#define I2C_DEVICE_ID_ON_SEMICONDUCTOR                  7
+#define I2C_DEVICE_ID_SPRINTEK_CORPORATION              8
+#define I2C_DEVICE_ID_ESPROS_PHOTONICS_AG               9
+#define I2C_DEVICE_ID_FUJITSU_SEMICONDUCTOR            10
+#define I2C_DEVICE_ID_FLIR                             11
+#define I2C_DEVICE_ID_O2MICRO                          12
+#define I2C_DEVICE_ID_ATMEL                            13
+#define I2C_DEVICE_ID_NONE                         0xffff
+	u16 part_id;
+	u8 die_revision;
+};
+
 enum i2c_alert_protocol {
 	I2C_PROTOCOL_SMBUS_ALERT,
 	I2C_PROTOCOL_SMBUS_HOST_NOTIFY,