| Message ID | 1521475859-16765-1-git-send-email-linux@roeck-us.net |
|---|---|
| State | RFC |
| Headers | show |
| Series | [RFC,1/2] i2c: Add i2c_verify_device_id() to verify device id | expand |
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 { > > >
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
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
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
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
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
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
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 >
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 {
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(+)