i2c: Add i2c_verify_device_id() to verify device id

RFC PATCH In review Guenter Roeck 3 participants 11 messages Mar 19, 2018 – Apr 9, 2018

Subsystems (3)

I2C subsystem host drivers I2C I2C muxes

lore

Share & tools

lore

Guenter Roeck

author

2018-03-19 16:10 UTC #0 lore raw

RFC PATCH 1/2 i2c: Add i2c_verify_device_id() to verify device id

Commit dde67eb1beeb ("i2c: add i2c_get_device_id() to get the standard I2C device id") added a function to return the standard I2C device ID. Use that function to verify the device ID of a given device.

Cc Peter Rosin <[email protected]>

Signed-off-by Guenter Roeck <[email protected]>

---
RFC:
- Compile tested only
- Should there also be I2C_DEVICE_PART_ID_ANY to enable maching against all parts from a given manufacturer ?

 drivers/i2c/i2c-core-base.c | 34 ++++++++++++++++++++++++++++++++++
 include/linux/i2c.h         |  3 +++
 2 files changed, 37 insertions(+)

drivers/i2c/i2c-core-base.c +34 -0

index 16a3b73375a6..4e4372b064f6 100644
@@ -2009,6 +2009,40 @@ int i2c_get_device_id(const struct i2c_client *client,
 }
 EXPORT_SYMBOL_GPL(i2c_get_device_id);
 
+/**
+ * i2c_verify_device_id - verify device ID
+ * @client: The device to query
+ * @id: Expected device ID
+ *
+ * Returns negative errno on error, zero on success.
+ */
+int i2c_verify_device_id(const struct i2c_client *client,
+			 const struct i2c_device_identity *id)
+{
+	struct i2c_device_identity real_id;
+	int ret;
+
+	if (id->manufacturer_id == I2C_DEVICE_ID_NONE)
+		return 0;
+
+	ret = i2c_get_device_id(client, &real_id);
+	if (ret < 0)
+		return ret;
+
+	if (id->manufacturer_id != real_id.manufacturer_id ||
+	    id->part_id != real_id.part_id ||
+	    (id->die_revision != I2C_DEVICE_DIE_REVISION_ANY &&
+	     id->die_revision != real_id.die_revision)) {
+		dev_err(&client->dev, "unexpected device id %03x-%03x-%x\n",
+			real_id.manufacturer_id, real_id.part_id,
+			real_id.die_revision);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(i2c_verify_device_id);
+
 /* ----------------------------------------------------
  * the i2c address scanning function
  * Will not work for 10-bit addresses!

include/linux/i2c.h +3 -0

index 44ad14e016b5..45bae9717ecb 100644
@@ -189,6 +189,8 @@ 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);
+int i2c_verify_device_id(const struct i2c_client *client,
+			 const struct i2c_device_identity *id);
 #endif /* I2C */
 
 /**

@@ -216,6 +218,7 @@ struct i2c_device_identity {
 #define I2C_DEVICE_ID_NONE                         0xffff
 	u16 part_id;
 	u8 die_revision;
+#define I2C_DEVICE_DIE_REVISION_ANY		     0xff
 };
 
 enum i2c_alert_protocol {

Signature · 1 line

2.7.4

Guenter Roeck

author

2018-03-19 16:10 UTC #1 lore raw

RFC PATCH 2/2 i2c: mux: pca954x: Use API function to verify device ID

Use i2c_verify_device_id() to verify the device ID in the probe function.

Cc Peter Rosin <[email protected]>

Signed-off-by Guenter Roeck <[email protected]>

---
RFC: Compile tested only

 drivers/i2c/muxes/i2c-mux-pca954x.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

drivers/i2c/muxes/i2c-mux-pca954x.c +7 ~~-17~~

index 09bafd3e68fa..80b7db26b02a 100644
@@ -148,6 +148,7 @@ static const struct chip_desc chips[] = {
 		.id = {
 			.manufacturer_id = I2C_DEVICE_ID_NXP_SEMICONDUCTORS,
 			.part_id = 0x10b,
+			.die_revision = I2C_DEVICE_DIE_REVISION_ANY,
 		},
 	},
 	[pca_9847] = {

@@ -157,6 +158,7 @@ static const struct chip_desc chips[] = {
 		.id = {
 			.manufacturer_id = I2C_DEVICE_ID_NXP_SEMICONDUCTORS,
 			.part_id = 0x108,
+			.die_revision = I2C_DEVICE_DIE_REVISION_ANY,
 		},
 	},
 	[pca_9848] = {

@@ -165,6 +167,7 @@ static const struct chip_desc chips[] = {
 		.id = {
 			.manufacturer_id = I2C_DEVICE_ID_NXP_SEMICONDUCTORS,
 			.part_id = 0x10a,
+			.die_revision = I2C_DEVICE_DIE_REVISION_ANY,
 		},
 	},
 	[pca_9849] = {

@@ -174,6 +177,7 @@ static const struct chip_desc chips[] = {
 		.id = {
 			.manufacturer_id = I2C_DEVICE_ID_NXP_SEMICONDUCTORS,
 			.part_id = 0x109,
+			.die_revision = I2C_DEVICE_DIE_REVISION_ANY,
 		},
 	},
 };

@@ -400,23 +404,9 @@ static int pca954x_probe(struct i2c_client *client,
 	else
 		data->chip = &chips[id->driver_data];
 
-	if (data->chip->id.manufacturer_id != I2C_DEVICE_ID_NONE) {
-		struct i2c_device_identity id;
-
-		ret = i2c_get_device_id(client, &id);
-		if (ret && ret != -EOPNOTSUPP)
-			return ret;
-
-		if (!ret &&
-		    (id.manufacturer_id != data->chip->id.manufacturer_id ||
-		     id.part_id != data->chip->id.part_id)) {
-			dev_warn(&client->dev,
-				 "unexpected device id %03x-%03x-%x\n",
-				 id.manufacturer_id, id.part_id,
-				 id.die_revision);
-			return -ENODEV;
-		}
-	}
+	ret = i2c_verify_device_id(client, &data->chip->id);
+	if (ret < 0 && ret != -EOPNOTSUPP)
+		return ret;
 
 	/* Write the mux register at addr to verify
 	 * that the mux is in fact present. This also

Signature · 1 line

2.7.4

Peter Rosin

2018-03-19 16:47 UTC #2 lore raw

In reply to Guenter Roeck

Quoted 3 lines · Guenter Roeck

Commit dde67eb1beeb ("i2c: add i2c_get_device_id() to get the standard I2C device id") added a function to return the standard I2C device ID. Use that function to verify the device ID of a given device.

Yeah, you're moving complexity from the driver to the core, reducing the indentation level and generally make things look neater. In fact, I thought about adding something like this, but didn't since I only had the one user.

The only negative is the added line count, but I suppose more drivers will call this function down the line so it should be a net win in the long run. There was the PCA9641 chip earlier today e.g., but maybe we should wait for more device id users?

I wonder when other manufacturers will get on board?

I also wonder if NXP will ever release a chip with part-id 0 and die-revision 0? If not, an all zero struct i2c_device_identity could be used instead of manufacturer_id 0xffff and that would simplify the pca954x driver code a bit more. But I guess we can never know the answer to that question. And even if we did, the answer might change later. But it would be nice...

Quoted 5 lines

Cc Peter Rosin <[email protected]>
Signed-off-by Guenter Roeck <[email protected]>
---
RFC:
- Compile tested only

Can't test either since I have no chips, but the code looks good.

Adrian have HW, but maybe he's getting tried of testing?

Hmmm, for testing purposes it would be nice if a linux slave device implemented this. But I don't have HW that supports that either so it wouldn't help *me* anyway...

Anyway, ack from me for both patches. But maybe I'm the one picking them up? Wolfram?

Cheers,
Peter

Quoted 74 lines

- Should there also be I2C_DEVICE_PART_ID_ANY to enable maching against all parts from a given manufacturer ?

 drivers/i2c/i2c-core-base.c | 34 ++++++++++++++++++++++++++++++++++
 include/linux/i2c.h         |  3 +++
 2 files changed, 37 insertions(+)

drivers/i2c/i2c-core-base.c +34 -0

index 16a3b73375a6..4e4372b064f6 100644
@@ -2009,6 +2009,40 @@ int i2c_get_device_id(const struct i2c_client *client,
 }
 EXPORT_SYMBOL_GPL(i2c_get_device_id);
 
+/**
+ * i2c_verify_device_id - verify device ID
+ * @client: The device to query
+ * @id: Expected device ID
+ *
+ * Returns negative errno on error, zero on success.
+ */
+int i2c_verify_device_id(const struct i2c_client *client,
+			 const struct i2c_device_identity *id)
+{
+	struct i2c_device_identity real_id;
+	int ret;
+
+	if (id->manufacturer_id == I2C_DEVICE_ID_NONE)
+		return 0;
+
+	ret = i2c_get_device_id(client, &real_id);
+	if (ret < 0)
+		return ret;
+
+	if (id->manufacturer_id != real_id.manufacturer_id ||
+	    id->part_id != real_id.part_id ||
+	    (id->die_revision != I2C_DEVICE_DIE_REVISION_ANY &&
+	     id->die_revision != real_id.die_revision)) {
+		dev_err(&client->dev, "unexpected device id %03x-%03x-%x\n",
+			real_id.manufacturer_id, real_id.part_id,
+			real_id.die_revision);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(i2c_verify_device_id);
+
 /* ----------------------------------------------------
  * the i2c address scanning function
  * Will not work for 10-bit addresses!

include/linux/i2c.h +3 -0

index 44ad14e016b5..45bae9717ecb 100644
@@ -189,6 +189,8 @@ 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);
+int i2c_verify_device_id(const struct i2c_client *client,
+			 const struct i2c_device_identity *id);
 #endif /* I2C */
 
 /**

@@ -216,6 +218,7 @@ struct i2c_device_identity {
 #define I2C_DEVICE_ID_NONE                         0xffff
 	u16 part_id;
 	u8 die_revision;
+#define I2C_DEVICE_DIE_REVISION_ANY		     0xff
 };
 
 enum i2c_alert_protocol {

Guenter Roeck

author

2018-03-19 18:48 UTC #3 lore raw

Quoted 10 lines · Peter Rosin

Quoted 3 lines · Guenter Roeck
Commit dde67eb1beeb ("i2c: add i2c_get_device_id() to get the standard I2C device id") added a function to return the standard I2C device ID. Use that function to verify the device ID of a given device.
Yeah, you're moving complexity from the driver to the core, reducing the indentation level and generally make things look neater. In fact, I thought about adding something like this, but didn't since I only had the one user.

Quoted 5 lines

The only negative is the added line count, but I suppose more drivers will call this function down the line so it should be a net win in the long run. There was the PCA9641 chip earlier today e.g., but maybe we should wait for more device id users?

Unfortunately it looks like only NXPs GPIO expanders and i2c muxes support it, or at least I didn't find any other chips. I figured though that it would be worth it even if only two drivers (or even one) end up using it.

Quoted 2 lines

I wonder when other manufacturers will get on board?

Hah, good question. As mentioned above, I didn't find any.

Quoted 7 lines

I also wonder if NXP will ever release a chip with part-id 0 and die-revision 0? If not, an all zero struct i2c_device_identity could be used instead of manufacturer_id 0xffff and that would simplify the pca954x driver code a bit more. But I guess we can never know the answer to that question. And even if we did, the answer might change later. But it would be nice...

That would be nice. You could ask at [email protected], but I guess it would always be somewhat risky since the standard doesn't restrict its use, and some product manager at NXP might decide in the future that a device ID of 0x00 would be "cool".

Guenter

Quoted 95 lines

Quoted 5 lines

Cc Peter Rosin <[email protected]>
Signed-off-by Guenter Roeck <[email protected]>
---
RFC:
- Compile tested only

Can't test either since I have no chips, but the code looks good.

Adrian have HW, but maybe he's getting tried of testing?

Hmmm, for testing purposes it would be nice if a linux slave device implemented this. But I don't have HW that supports that either so it wouldn't help *me* anyway...

Anyway, ack from me for both patches. But maybe I'm the one picking them up? Wolfram?

Cheers,
Peter

Quoted 74 lines

- Should there also be I2C_DEVICE_PART_ID_ANY to enable maching against all parts from a given manufacturer ?

 drivers/i2c/i2c-core-base.c | 34 ++++++++++++++++++++++++++++++++++
 include/linux/i2c.h         |  3 +++
 2 files changed, 37 insertions(+)

drivers/i2c/i2c-core-base.c +34 -0

index 16a3b73375a6..4e4372b064f6 100644
@@ -2009,6 +2009,40 @@ int i2c_get_device_id(const struct i2c_client *client,
 }
 EXPORT_SYMBOL_GPL(i2c_get_device_id);
 
+/**
+ * i2c_verify_device_id - verify device ID
+ * @client: The device to query
+ * @id: Expected device ID
+ *
+ * Returns negative errno on error, zero on success.
+ */
+int i2c_verify_device_id(const struct i2c_client *client,
+			 const struct i2c_device_identity *id)
+{
+	struct i2c_device_identity real_id;
+	int ret;
+
+	if (id->manufacturer_id == I2C_DEVICE_ID_NONE)
+		return 0;
+
+	ret = i2c_get_device_id(client, &real_id);
+	if (ret < 0)
+		return ret;
+
+	if (id->manufacturer_id != real_id.manufacturer_id ||
+	    id->part_id != real_id.part_id ||
+	    (id->die_revision != I2C_DEVICE_DIE_REVISION_ANY &&
+	     id->die_revision != real_id.die_revision)) {
+		dev_err(&client->dev, "unexpected device id %03x-%03x-%x\n",
+			real_id.manufacturer_id, real_id.part_id,
+			real_id.die_revision);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(i2c_verify_device_id);
+
 /* ----------------------------------------------------
  * the i2c address scanning function
  * Will not work for 10-bit addresses!

include/linux/i2c.h +3 -0

index 44ad14e016b5..45bae9717ecb 100644
@@ -189,6 +189,8 @@ 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);
+int i2c_verify_device_id(const struct i2c_client *client,
+			 const struct i2c_device_identity *id);
 #endif /* I2C */
 
 /**

@@ -216,6 +218,7 @@ struct i2c_device_identity {
 #define I2C_DEVICE_ID_NONE                         0xffff
 	u16 part_id;
 	u8 die_revision;
+#define I2C_DEVICE_DIE_REVISION_ANY		     0xff
 };
 
 enum i2c_alert_protocol {

Peter Rosin

2018-03-19 19:55 UTC #4 lore raw

Quoted 9 lines · Guenter Roeck

Quoted 7 lines · Peter Rosin
I also wonder if NXP will ever release a chip with part-id 0 and die-revision 0? If not, an all zero struct i2c_device_identity could be used instead of manufacturer_id 0xffff and that would simplify the pca954x driver code a bit more. But I guess we can never know the answer to that question. And even if we did, the answer might change later. But it would be nice...

Quoted 4 lines

That would be nice. You could ask at [email protected], but I guess it would always be somewhat risky since the standard doesn't restrict its use, and some product manager at NXP might decide in the future that a device ID of 0x00 would be "cool".

No need to bother NXP, PCA9848 has already claimed 0-0-0. Sigh.

But while I googled that I found old datasheets for the chips PCA9672 through PCA9675 which use a different layout for the three bytes in the device id. They have 8 manufacturer bits, 7 category bits, 6 bits of feature indication and then 3 bits of revision. The top category bits are zero so it is compatible for NXP chips. But since noone else has implemented this, it is probably safe, but still a little bit disturbing.

I also found that NXP apparently uses the same part id (0x100) and die revision (0) for PCA9570 and PCA9670. That seems odd.

Example old datasheet (2006):
https://www.digchip.com/datasheets/download_datasheet.php?id=1098812&part-number=PCA9672

Cheers,
Peter

Guenter Roeck

author

2018-03-19 20:50 UTC #5 lore raw

Quoted 17 lines · Peter Rosin

Quoted 9 lines · Guenter Roeck
Quoted 7 lines · Peter Rosin
I also wonder if NXP will ever release a chip with part-id 0 and die-revision 0? If not, an all zero struct i2c_device_identity could be used instead of manufacturer_id 0xffff and that would simplify the pca954x driver code a bit more. But I guess we can never know the answer to that question. And even if we did, the answer might change later. But it would be nice...
Quoted 4 lines
That would be nice. You could ask at [email protected], but I guess it would always be somewhat risky since the standard doesn't restrict its use, and some product manager at NXP might decide in the future that a device ID of 0x00 would be "cool".
No need to bother NXP, PCA9848 has already claimed 0-0-0. Sigh.

Quoted 8 lines

But while I googled that I found old datasheets for the chips PCA9672 through PCA9675 which use a different layout for the three bytes in the device id. They have 8 manufacturer bits, 7 category bits, 6 bits of feature indication and then 3 bits of revision. The top category bits are zero so it is compatible for NXP chips. But since noone else has implemented this, it is probably safe, but still a little bit disturbing.

The PCA9570 datasheet is especially interesting.

"9 bits with the part identification, assigned by manufacturer, the 7 MSBs with the category ID and the 6 LSBs with the feature ID (for example PCA9570 4-bit I/O expander)"

Maybe there is a magic compression scheme to squash 7 MSBs and 6 LSBs into a 9-bit field, or the category and feature IDs have quite some overlap, or the document would benefit from some proof-reading.

Quoted 3 lines

I also found that NXP apparently uses the same part id (0x100) and die revision (0) for PCA9570 and PCA9670. That seems odd.

... especially since one has 4 channels and the other has 8 channels. It would be interesting to see if reality and datasheets match; this might as well be a curt-and-paste error. Of course, it might as well be that both chips use the same die and that some pins are just not exposed on the 4 channel version.

Quoted 3 lines

Example old datasheet (2006):
https://www.digchip.com/datasheets/download_datasheet.php?id=1098812&part-number=PCA9672

On the other side this has been corrected in more recent datasheet versions, so I would not be too concerned about that.

Guenter

Peter Rosin

2018-03-19 23:09 UTC #6 lore raw

Quoted 29 lines · Guenter Roeck

Quoted 17 lines · Peter Rosin
Quoted 9 lines · Guenter Roeck
Quoted 7 lines · Peter Rosin
I also wonder if NXP will ever release a chip with part-id 0 and die-revision 0? If not, an all zero struct i2c_device_identity could be used instead of manufacturer_id 0xffff and that would simplify the pca954x driver code a bit more. But I guess we can never know the answer to that question. And even if we did, the answer might change later. But it would be nice...
Quoted 4 lines
That would be nice. You could ask at [email protected], but I guess it would always be somewhat risky since the standard doesn't restrict its use, and some product manager at NXP might decide in the future that a device ID of 0x00 would be "cool".
No need to bother NXP, PCA9848 has already claimed 0-0-0. Sigh.
Quoted 8 lines
But while I googled that I found old datasheets for the chips PCA9672 through PCA9675 which use a different layout for the three bytes in the device id. They have 8 manufacturer bits, 7 category bits, 6 bits of feature indication and then 3 bits of revision. The top category bits are zero so it is compatible for NXP chips. But since noone else has implemented this, it is probably safe, but still a little bit disturbing.
The PCA9570 datasheet is especially interesting.

Quoted 7 lines

"9 bits with the part identification, assigned by manufacturer, the 7 MSBs with the category ID and the 6 LSBs with the feature ID (for example PCA9570 4-bit I/O expander)"
Maybe there is a magic compression scheme to squash 7 MSBs and 6 LSBs into a 9-bit field, or the category and feature IDs have quite some overlap, or the document would benefit from some proof-reading.

Hmm, I think I'll put my money on the compression thing.

Quoted 9 lines

Quoted 3 lines
I also found that NXP apparently uses the same part id (0x100) and die revision (0) for PCA9570 and PCA9670. That seems odd.
... especially since one has 4 channels and the other has 8 channels. It would be interesting to see if reality and datasheets match; this might as well be a curt-and-paste error. Of course, it might as well be that both chips use the same die and that some pins are just not exposed on the 4 channel version.

Quoted 5 lines

Quoted 3 lines
Example old datasheet (2006):
https://www.digchip.com/datasheets/download_datasheet.php?id=1098812&part-number=PCA9672
On the other side this has been corrected in more recent datasheet versions, so I would not be too concerned about that.

PCA9671 is still using the old bit field layout in its most recent datasheet (2011). I couldn't find anything newer anyway, and the link seems canonical enough...

https://www.nxp.com/docs/en/data-sheet/PCA9671.pdf

Enough of this, but I'm going to send a note to NXP about the issues we did find.

Cheers,
Peter

Wolfram Sang

2018-04-08 07:34 UTC #7 lore raw

In reply to Guenter Roeck

Hi,

Quoted 3 lines · Guenter Roeck

Commit dde67eb1beeb ("i2c: add i2c_get_device_id() to get the standard I2C device id") added a function to return the standard I2C device ID. Use that function to verify the device ID of a given device.

I am very open to these patches, just...

Quoted 6 lines

Cc Peter Rosin <[email protected]>
Signed-off-by Guenter Roeck <[email protected]>
---
RFC:
- Compile tested only

... I would really like to have them tested. After that happened, Peter and I can figure out who should apply them for seamless upstreaming.

Quoted 2 lines

- Should there also be I2C_DEVICE_PART_ID_ANY to enable maching against all parts from a given manufacturer ?

Can't we just add it when we need it?

drivers/i2c/i2c-core-base.c int i2c_get_device_id(const struct i2c_client *client, · line 2039

Quoted 4 lines

drivers/i2c/i2c-core-base.c +4 -0

+		dev_err(&client->dev, "unexpected device id %03x-%03x-%x\n",
+			real_id.manufacturer_id, real_id.part_id,
+			real_id.die_revision);
+		return -ENODEV;

I wonder about the ERR loglevel. ENODEV is not an error, I'd think?

Regards,

Wolfram

Peter Rosin

2018-04-08 09:08 UTC #8 lore raw

Quoted 22 lines · Wolfram Sang

Hi,
Quoted 3 lines · Guenter Roeck
Commit dde67eb1beeb ("i2c: add i2c_get_device_id() to get the standard I2C device id") added a function to return the standard I2C device ID. Use that function to verify the device ID of a given device.
I am very open to these patches, just...
Quoted 6 lines
Cc Peter Rosin <[email protected]>
Signed-off-by Guenter Roeck <[email protected]>
---
RFC:
- Compile tested only
... I would really like to have them tested. After that happened, Peter and I can figure out who should apply them for seamless upstreaming.
Quoted 2 lines
- Should there also be I2C_DEVICE_PART_ID_ANY to enable maching against all parts from a given manufacturer ?

Quoted 8 lines

Can't we just add it when we need it?
drivers/i2c/i2c-core-base.c int i2c_get_device_id(const struct i2c_client *client, · line 2039
Quoted 4 lines
drivers/i2c/i2c-core-base.c +4 -0
+		dev_err(&client->dev, "unexpected device id %03x-%03x-%x\n",
+			real_id.manufacturer_id, real_id.part_id,
+			real_id.die_revision);
+		return -ENODEV;
I wonder about the ERR loglevel. ENODEV is not an error, I'd think?

Well, in this case someone has said that I2C addr <xyz> is a <uvw> device, but when verifying the actual device at that addr, that's not what is found. Hence, I think an error is appropriate? On the other hand, a driver that can handle different kinds of devices might not want the error. But for that case, maybe the driver should be using i2c_get_device_id() and figure out the details by itself?

Cheers,
Peter

Peter Rosin

2018-04-08 10:28 UTC #9 lore raw

Sorry for replying to self...

Quoted 17 lines · Peter Rosin

Quoted 16 lines · Wolfram Sang
Hi,
Quoted 3 lines · Guenter Roeck
Commit dde67eb1beeb ("i2c: add i2c_get_device_id() to get the standard I2C device id") added a function to return the standard I2C device ID. Use that function to verify the device ID of a given device.
I am very open to these patches, just...
Quoted 6 lines
Cc Peter Rosin <[email protected]>
Signed-off-by Guenter Roeck <[email protected]>
---
RFC:
- Compile tested only

Quoted 7 lines

Quoted 7 lines
... I would really like to have them tested. After that happened, Peter and I can figure out who should apply them for seamless upstreaming.
Quoted 2 lines
- Should there also be I2C_DEVICE_PART_ID_ANY to enable maching against all parts from a given manufacturer ?
Can't we just add it when we need it?

Is it really reasonable to verify just the manufacturer? I don't see the use case? I mean, we can never know if the verified manufacturer will release unexpected chips further down the line.

Quoted 7 lines

Quoted 6 lines
drivers/i2c/i2c-core-base.c int i2c_get_device_id(const struct i2c_client *client, · line 2039
Quoted 4 lines
drivers/i2c/i2c-core-base.c +4 -0
+		dev_err(&client->dev, "unexpected device id %03x-%03x-%x\n",
+			real_id.manufacturer_id, real_id.part_id,
+			real_id.die_revision);
+		return -ENODEV;
I wonder about the ERR loglevel. ENODEV is not an error, I'd think?

Quoted 6 lines

Well, in this case someone has said that I2C addr <xyz> is a <uvw> device, but when verifying the actual device at that addr, that's not what is found. Hence, I think an error is appropriate? On the other hand, a driver that can handle different kinds of devices might not want the error. But for that case, maybe the driver should be using i2c_get_device_id() and figure out the details by itself?

Maybe it should just be -EINVAL, and that's your real objection?

Cheers,
Peter

Guenter Roeck

author

2018-04-09 18:29 UTC #10 lore raw

In reply to Wolfram Sang

Quoted 10 lines · Wolfram Sang

Hi,
Quoted 3 lines · Guenter Roeck
Commit dde67eb1beeb ("i2c: add i2c_get_device_id() to get the standard I2C device id") added a function to return the standard I2C device ID. Use that function to verify the device ID of a given device.
I am very open to these patches, just...
Quoted 1 line

Quoted 9 lines

Quoted 5 lines
Cc Peter Rosin <[email protected]>
Signed-off-by Guenter Roeck <[email protected]>
---
RFC:
- Compile tested only
... I would really like to have them tested. After that happened, Peter and I can figure out who should apply them for seamless upstreaming.

Patch 2/2 was for real HW. I don't have access to such HW right now. Guess we'll have to wait until someone does, unless Adrian is willing to test it.

Quoted 5 lines

Quoted 2 lines
- Should there also be I2C_DEVICE_PART_ID_ANY to enable maching against all parts from a given manufacturer ?
Can't we just add it when we need it?

Perfectly fine with me.

Quoted 7 lines

drivers/i2c/i2c-core-base.c int i2c_get_device_id(const struct i2c_client *client, · line 2039
Quoted 4 lines
drivers/i2c/i2c-core-base.c +4 -0
+		dev_err(&client->dev, "unexpected device id %03x-%03x-%x\n",
+			real_id.manufacturer_id, real_id.part_id,
+			real_id.die_revision);
+		return -ENODEV;
I wonder about the ERR loglevel. ENODEV is not an error, I'd think?

2d74187d5b4e, where this is derived from, uses dev_warn() instead, so that may be a better choice.

Thanks,
Guenter

Quoted 4 lines

Regards,
Wolfram