| Message ID | 20230427213038.1375404-1-Frank.Li@nxp.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/1] iio: light: vcnl4035: fixed chip ID check | expand |
> -----Original Message----- > From: Jonathan Cameron <jic23@kernel.org> > Sent: Sunday, April 30, 2023 12:23 PM > To: Frank Li <frank.li@nxp.com> > Cc: Lars-Peter Clausen <lars@metafoo.de>; Uwe Kleine-König <u.kleine- > koenig@pengutronix.de>; Petr Machata <petrm@nvidia.com>; Heikki > Krogerus <heikki.krogerus@linux.intel.com>; Jean Delvare > <jdelvare@suse.de>; Andy Shevchenko > <andriy.shevchenko@linux.intel.com>; Parthiban Nallathambi > <pn@denx.de>; open list:IIO SUBSYSTEM AND DRIVERS <linux- > iio@vger.kernel.org>; open list <linux-kernel@vger.kernel.org>; > imx@lists.linux.dev > Subject: [EXT] Re: [PATCH 1/1] iio: light: vcnl4035: fixed chip ID check > > Caution: This is an external email. Please take care when clicking links or > opening attachments. When in doubt, report the message using the 'Report > this email' button > > > On Thu, 27 Apr 2023 17:30:37 -0400 > Frank Li <Frank.Li@nxp.com> wrote: > > > VCNL4035 register(0xE) ID_L and ID_M define as: > > > > ID_L: 0x80 > > ID_H: 7:6 (0:0) > > 5:4 (0:0) slave address = 0x60 (7-bit) > > (0:1) slave address = 0x51 (7-bit) > > (1:0) slave address = 0x40 (7-bit) > > (1:0) slave address = 0x41 (7-bit) > > 3:0 Version code default (0:0:0:0) > > > > So just check ID_L. > > Hi Frank, > > Thanks for the fix. A few minor things inline. > > > > > Fixes: 55707294c4eb ("iio: light: Add support for vishay vcnl4035") > > > > No blank line here as the Fixes tag is part of the main tag block. > > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > Just to check, the result of this bug is that the driver probe fails > if the slave address isn't 0x60? Yes, I get chip which slave address = 0x51. I will update patch soon. > > > --- > > drivers/iio/light/vcnl4035.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/iio/light/vcnl4035.c b/drivers/iio/light/vcnl4035.c > > index 3ed37f6057fb..8b7769930f3b 100644 > > --- a/drivers/iio/light/vcnl4035.c > > +++ b/drivers/iio/light/vcnl4035.c > > @@ -413,7 +413,7 @@ static int vcnl4035_init(struct vcnl4035_data *data) > > return ret; > > } > > > > - if (id != VCNL4035_DEV_ID_VAL) { > > + if ((id & 0xff) != VCNL4035_DEV_ID_VAL) { > > Please add a define for that 0xff mask and perhaps also use > FIELD_GET() to extract the field for comparison with VCNL4035_DEV_ID_VAL. > Whilst that isn't being done elsewhere in this driver, the heavy use > of set bits means it isn't appropriate anywhere else that I can quickly > identify. You'll also need to include linux/bitfield.h if making that change. > > Thanks, > > Jonathan > > > dev_err(&data->client->dev, "Wrong id, got %x, expected %x\n", > > id, VCNL4035_DEV_ID_VAL); > > return -ENODEV;
On Thu, 27 Apr 2023 17:30:37 -0400 Frank Li <Frank.Li@nxp.com> wrote: > VCNL4035 register(0xE) ID_L and ID_M define as: > > ID_L: 0x80 > ID_H: 7:6 (0:0) > 5:4 (0:0) slave address = 0x60 (7-bit) > (0:1) slave address = 0x51 (7-bit) > (1:0) slave address = 0x40 (7-bit) > (1:0) slave address = 0x41 (7-bit) > 3:0 Version code default (0:0:0:0) > > So just check ID_L. Hi Frank, Thanks for the fix. A few minor things inline. > > Fixes: 55707294c4eb ("iio: light: Add support for vishay vcnl4035") > No blank line here as the Fixes tag is part of the main tag block. > Signed-off-by: Frank Li <Frank.Li@nxp.com> Just to check, the result of this bug is that the driver probe fails if the slave address isn't 0x60? > --- > drivers/iio/light/vcnl4035.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/iio/light/vcnl4035.c b/drivers/iio/light/vcnl4035.c > index 3ed37f6057fb..8b7769930f3b 100644 > --- a/drivers/iio/light/vcnl4035.c > +++ b/drivers/iio/light/vcnl4035.c > @@ -413,7 +413,7 @@ static int vcnl4035_init(struct vcnl4035_data *data) > return ret; > } > > - if (id != VCNL4035_DEV_ID_VAL) { > + if ((id & 0xff) != VCNL4035_DEV_ID_VAL) { Please add a define for that 0xff mask and perhaps also use FIELD_GET() to extract the field for comparison with VCNL4035_DEV_ID_VAL. Whilst that isn't being done elsewhere in this driver, the heavy use of set bits means it isn't appropriate anywhere else that I can quickly identify. You'll also need to include linux/bitfield.h if making that change. Thanks, Jonathan > dev_err(&data->client->dev, "Wrong id, got %x, expected %x\n", > id, VCNL4035_DEV_ID_VAL); > return -ENODEV;
Le 27/04/2023 à 23:30, Frank Li a écrit : > VCNL4035 register(0xE) ID_L and ID_M define as: > > ID_L: 0x80 > ID_H: 7:6 (0:0) > 5:4 (0:0) slave address = 0x60 (7-bit) > (0:1) slave address = 0x51 (7-bit) > (1:0) slave address = 0x40 (7-bit) > (1:0) slave address = 0x41 (7-bit) > 3:0 Version code default (0:0:0:0) > > So just check ID_L. > > Fixes: 55707294c4eb ("iio: light: Add support for vishay vcnl4035") > > Signed-off-by: Frank Li <Frank.Li-3arQi8VN3Tc@public.gmane.org> > --- > drivers/iio/light/vcnl4035.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/iio/light/vcnl4035.c b/drivers/iio/light/vcnl4035.c > index 3ed37f6057fb..8b7769930f3b 100644 > --- a/drivers/iio/light/vcnl4035.c > +++ b/drivers/iio/light/vcnl4035.c > @@ -413,7 +413,7 @@ static int vcnl4035_init(struct vcnl4035_data *data) > return ret; > } > > - if (id != VCNL4035_DEV_ID_VAL) { > + if ((id & 0xff) != VCNL4035_DEV_ID_VAL) { > dev_err(&data->client->dev, "Wrong id, got %x, expected %x\n", > id, VCNL4035_DEV_ID_VAL); Hi, should the error message be updated as well? Cj > return -ENODEV;
diff --git a/drivers/iio/light/vcnl4035.c b/drivers/iio/light/vcnl4035.c index 3ed37f6057fb..8b7769930f3b 100644 --- a/drivers/iio/light/vcnl4035.c +++ b/drivers/iio/light/vcnl4035.c @@ -413,7 +413,7 @@ static int vcnl4035_init(struct vcnl4035_data *data) return ret; } - if (id != VCNL4035_DEV_ID_VAL) { + if ((id & 0xff) != VCNL4035_DEV_ID_VAL) { dev_err(&data->client->dev, "Wrong id, got %x, expected %x\n", id, VCNL4035_DEV_ID_VAL); return -ENODEV;
VCNL4035 register(0xE) ID_L and ID_M define as: ID_L: 0x80 ID_H: 7:6 (0:0) 5:4 (0:0) slave address = 0x60 (7-bit) (0:1) slave address = 0x51 (7-bit) (1:0) slave address = 0x40 (7-bit) (1:0) slave address = 0x41 (7-bit) 3:0 Version code default (0:0:0:0) So just check ID_L. Fixes: 55707294c4eb ("iio: light: Add support for vishay vcnl4035") Signed-off-by: Frank Li <Frank.Li@nxp.com> --- drivers/iio/light/vcnl4035.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)