* [RFC PATCH 1/2] i2c: Add i2c_verify_device_id() to verify device id
@ 2018-03-19 16:10 Guenter Roeck
2018-03-19 16:10 ` [RFC PATCH 2/2] i2c: mux: pca954x: Use API function to verify device ID Guenter Roeck
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Guenter Roeck @ 2018-03-19 16:10 UTC (permalink / raw)
To: Peter Rosin; +Cc: Wolfram Sang, linux-i2c, linux-kernel, 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.
Cc: Peter Rosin <peda@axentia.se>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
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(+)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 16a3b73375a6..4e4372b064f6 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -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!
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 44ad14e016b5..45bae9717ecb 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -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 {
--
2.7.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [RFC PATCH 2/2] i2c: mux: pca954x: Use API function to verify device ID
2018-03-19 16:10 [RFC PATCH 1/2] i2c: Add i2c_verify_device_id() to verify device id Guenter Roeck
@ 2018-03-19 16:10 ` Guenter Roeck
2018-03-19 16:47 ` [RFC PATCH 1/2] i2c: Add i2c_verify_device_id() to verify device id Peter Rosin
2018-04-08 7:34 ` Wolfram Sang
2 siblings, 0 replies; 11+ messages in thread
From: Guenter Roeck @ 2018-03-19 16:10 UTC (permalink / raw)
To: Peter Rosin; +Cc: Wolfram Sang, linux-i2c, linux-kernel, Guenter Roeck
Use i2c_verify_device_id() to verify the device ID in the probe function.
Cc: Peter Rosin <peda@axentia.se>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
RFC: Compile tested only
drivers/i2c/muxes/i2c-mux-pca954x.c | 24 +++++++-----------------
1 file changed, 7 insertions(+), 17 deletions(-)
diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 09bafd3e68fa..80b7db26b02a 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -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
--
2.7.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 1/2] i2c: Add i2c_verify_device_id() to verify device id
2018-03-19 16:10 [RFC PATCH 1/2] i2c: Add i2c_verify_device_id() to verify device id Guenter Roeck
2018-03-19 16:10 ` [RFC PATCH 2/2] i2c: mux: pca954x: Use API function to verify device ID Guenter Roeck
@ 2018-03-19 16:47 ` Peter Rosin
2018-03-19 18:48 ` Guenter Roeck
2018-04-08 7:34 ` Wolfram Sang
2 siblings, 1 reply; 11+ messages in thread
From: Peter Rosin @ 2018-03-19 16:47 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Wolfram Sang, linux-i2c, linux-kernel, Adrian Fiergolski
On 2018-03-19 17:10, Guenter Roeck wrote:
> 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...
> Cc: Peter Rosin <peda@axentia.se>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> 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
> - 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(+)
>
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 16a3b73375a6..4e4372b064f6 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -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!
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 44ad14e016b5..45bae9717ecb 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -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 {
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 1/2] i2c: Add i2c_verify_device_id() to verify device id
2018-03-19 16:47 ` [RFC PATCH 1/2] i2c: Add i2c_verify_device_id() to verify device id Peter Rosin
@ 2018-03-19 18:48 ` Guenter Roeck
2018-03-19 19:55 ` Peter Rosin
0 siblings, 1 reply; 11+ messages in thread
From: Guenter Roeck @ 2018-03-19 18:48 UTC (permalink / raw)
To: Peter Rosin; +Cc: Wolfram Sang, linux-i2c, linux-kernel, Adrian Fiergolski
On Mon, Mar 19, 2018 at 05:47:05PM +0100, Peter Rosin wrote:
> On 2018-03-19 17:10, Guenter Roeck wrote:
> > 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?
>
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.
> I wonder when other manufacturers will get on board?
>
Hah, good question. As mentioned above, I didn't find any.
> 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 i2c.support@nxp.com, 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
> > Cc: Peter Rosin <peda@axentia.se>
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> > 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
>
> > - 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(+)
> >
> > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> > index 16a3b73375a6..4e4372b064f6 100644
> > --- a/drivers/i2c/i2c-core-base.c
> > +++ b/drivers/i2c/i2c-core-base.c
> > @@ -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!
> > diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> > index 44ad14e016b5..45bae9717ecb 100644
> > --- a/include/linux/i2c.h
> > +++ b/include/linux/i2c.h
> > @@ -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 {
> >
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 1/2] i2c: Add i2c_verify_device_id() to verify device id
2018-03-19 18:48 ` Guenter Roeck
@ 2018-03-19 19:55 ` Peter Rosin
2018-03-19 20:50 ` Guenter Roeck
0 siblings, 1 reply; 11+ messages in thread
From: Peter Rosin @ 2018-03-19 19:55 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Wolfram Sang, linux-i2c, linux-kernel, Adrian Fiergolski
On 2018-03-19 19:48, Guenter Roeck wrote:
> On Mon, Mar 19, 2018 at 05:47:05PM +0100, Peter Rosin wrote:
>> 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 i2c.support@nxp.com, 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 1/2] i2c: Add i2c_verify_device_id() to verify device id
2018-03-19 19:55 ` Peter Rosin
@ 2018-03-19 20:50 ` Guenter Roeck
2018-03-19 23:09 ` Peter Rosin
0 siblings, 1 reply; 11+ messages in thread
From: Guenter Roeck @ 2018-03-19 20:50 UTC (permalink / raw)
To: Peter Rosin; +Cc: Wolfram Sang, linux-i2c, linux-kernel, Adrian Fiergolski
On Mon, Mar 19, 2018 at 08:55:55PM +0100, Peter Rosin wrote:
> On 2018-03-19 19:48, Guenter Roeck wrote:
> > On Mon, Mar 19, 2018 at 05:47:05PM +0100, Peter Rosin wrote:
> >> 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 i2c.support@nxp.com, 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.
>
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.
> 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.
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 1/2] i2c: Add i2c_verify_device_id() to verify device id
2018-03-19 20:50 ` Guenter Roeck
@ 2018-03-19 23:09 ` Peter Rosin
0 siblings, 0 replies; 11+ messages in thread
From: Peter Rosin @ 2018-03-19 23:09 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Wolfram Sang, linux-i2c, linux-kernel, Adrian Fiergolski
On 2018-03-19 21:50, Guenter Roeck wrote:
> On Mon, Mar 19, 2018 at 08:55:55PM +0100, Peter Rosin wrote:
>> On 2018-03-19 19:48, Guenter Roeck wrote:
>>> On Mon, Mar 19, 2018 at 05:47:05PM +0100, Peter Rosin wrote:
>>>> 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 i2c.support@nxp.com, 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.
>>
>
> 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.
Hmm, I think I'll put my money on the compression thing.
>> 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.
>
>> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 1/2] i2c: Add i2c_verify_device_id() to verify device id
2018-03-19 16:10 [RFC PATCH 1/2] i2c: Add i2c_verify_device_id() to verify device id Guenter Roeck
2018-03-19 16:10 ` [RFC PATCH 2/2] i2c: mux: pca954x: Use API function to verify device ID Guenter Roeck
2018-03-19 16:47 ` [RFC PATCH 1/2] i2c: Add i2c_verify_device_id() to verify device id Peter Rosin
@ 2018-04-08 7:34 ` Wolfram Sang
2018-04-08 9:08 ` Peter Rosin
2018-04-09 18:29 ` Guenter Roeck
2 siblings, 2 replies; 11+ messages in thread
From: Wolfram Sang @ 2018-04-08 7:34 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Peter Rosin, linux-i2c, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1038 bytes --]
Hi,
On Mon, Mar 19, 2018 at 09:10:58AM -0700, Guenter Roeck wrote:
> 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...
>
> Cc: Peter Rosin <peda@axentia.se>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> 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.
> - 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?
> + 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 1/2] i2c: Add i2c_verify_device_id() to verify device id
2018-04-08 7:34 ` Wolfram Sang
@ 2018-04-08 9:08 ` Peter Rosin
2018-04-08 10:28 ` Peter Rosin
2018-04-09 18:29 ` Guenter Roeck
1 sibling, 1 reply; 11+ messages in thread
From: Peter Rosin @ 2018-04-08 9:08 UTC (permalink / raw)
To: Wolfram Sang, Guenter Roeck; +Cc: linux-i2c, linux-kernel
On 2018-04-08 09:34, Wolfram Sang wrote:
> Hi,
>
> On Mon, Mar 19, 2018 at 09:10:58AM -0700, Guenter Roeck wrote:
>> 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...
>
>>
>> Cc: Peter Rosin <peda@axentia.se>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>> 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.
>
>> - 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?
>
>> + 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 1/2] i2c: Add i2c_verify_device_id() to verify device id
2018-04-08 9:08 ` Peter Rosin
@ 2018-04-08 10:28 ` Peter Rosin
0 siblings, 0 replies; 11+ messages in thread
From: Peter Rosin @ 2018-04-08 10:28 UTC (permalink / raw)
To: Wolfram Sang, Guenter Roeck; +Cc: linux-i2c, linux-kernel
Sorry for replying to self...
On 2018-04-08 11:08, Peter Rosin wrote:
> On 2018-04-08 09:34, Wolfram Sang wrote:
>> Hi,
>>
>> On Mon, Mar 19, 2018 at 09:10:58AM -0700, Guenter Roeck wrote:
>>> 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...
>>
>>>
>>> Cc: Peter Rosin <peda@axentia.se>
>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>> ---
>>> 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.
>>
>>> - 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.
>>> + 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?
Maybe it should just be -EINVAL, and that's your real objection?
Cheers,
Peter
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH 1/2] i2c: Add i2c_verify_device_id() to verify device id
2018-04-08 7:34 ` Wolfram Sang
2018-04-08 9:08 ` Peter Rosin
@ 2018-04-09 18:29 ` Guenter Roeck
1 sibling, 0 replies; 11+ messages in thread
From: Guenter Roeck @ 2018-04-09 18:29 UTC (permalink / raw)
To: Wolfram Sang; +Cc: Peter Rosin, linux-i2c, linux-kernel
On Sun, Apr 08, 2018 at 09:34:36AM +0200, Wolfram Sang wrote:
> Hi,
>
> On Mon, Mar 19, 2018 at 09:10:58AM -0700, Guenter Roeck wrote:
> > 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...
>
> >
> > Cc: Peter Rosin <peda@axentia.se>
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> > 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.
> > - 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.
> > + 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
> Regards,
>
> Wolfram
>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-04-09 18:29 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-19 16:10 [RFC PATCH 1/2] i2c: Add i2c_verify_device_id() to verify device id Guenter Roeck
2018-03-19 16:10 ` [RFC PATCH 2/2] i2c: mux: pca954x: Use API function to verify device ID Guenter Roeck
2018-03-19 16:47 ` [RFC PATCH 1/2] i2c: Add i2c_verify_device_id() to verify device id Peter Rosin
2018-03-19 18:48 ` Guenter Roeck
2018-03-19 19:55 ` Peter Rosin
2018-03-19 20:50 ` Guenter Roeck
2018-03-19 23:09 ` Peter Rosin
2018-04-08 7:34 ` Wolfram Sang
2018-04-08 9:08 ` Peter Rosin
2018-04-08 10:28 ` Peter Rosin
2018-04-09 18:29 ` Guenter Roeck
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.