Hi Guenter,
Sorry for the late review.
On Tue, 5 Jul 2011 07:32:05 -0700, Guenter Roeck wrote:
> On Mon, Jun 27, 2011 at 04:38:37PM -0400, Guenter Roeck wrote:
> > The LM95241 driver accepts every chip ID equal to or larger than 0xA4 as its
> > own, and other chips such as LM95245 use chip IDs in the accepted ID range.
> > This results in false chip detection.
> >
> > Fix problem by accepting only the known LM95241 chip ID. Also remove the debug
> > message displayed if chip detection failed since it just adds noise and no real
> > value.
> >
> > Signed-off-by: Guenter Roeck <guenter.roeck@xxxxxxxxxxxx>
>
> Anyone with objections to this patch please speak up now or be silent forever ...
> I'll submit it to Linus sometime this week.
OK, I'll speak now then.
> > ---
> > Kept name variable to prepare for possible later addition of LM95231 support
> > (after I get samples).
> >
> > drivers/hwmon/lm95241.c | 17 ++++++-----------
> > 1 files changed, 6 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/hwmon/lm95241.c b/drivers/hwmon/lm95241.c
> > index 1a6dfb6..f2cfecf 100644
> > --- a/drivers/hwmon/lm95241.c
> > +++ b/drivers/hwmon/lm95241.c
> > @@ -330,24 +330,19 @@ static int lm95241_detect(struct i2c_client *new_client,
> > struct i2c_board_info *info)
> > {
> > struct i2c_adapter *adapter = new_client->adapter;
> > - int address = new_client->addr;
> > const char *name;
> >
> > if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> > return -ENODEV;
> >
> > - if ((i2c_smbus_read_byte_data(new_client, LM95241_REG_R_MAN_ID)
> > - == MANUFACTURER_ID)
> > - && (i2c_smbus_read_byte_data(new_client, LM95241_REG_R_CHIP_ID)
> > - >= DEFAULT_REVISION)) {
> > - name = DEVNAME;
> > - } else {
> > - dev_dbg(&adapter->dev, "LM95241 detection failed at 0x%02x\n",
> > - address);
> > + if (i2c_smbus_read_byte_data(new_client, LM95241_REG_R_MAN_ID)
> > + != MANUFACTURER_ID
> > + || i2c_smbus_read_byte_data(new_client, LM95241_REG_R_CHIP_ID)
> > + != DEFAULT_REVISION)
> > return -ENODEV;
> > - }
> >
> > - /* Fill the i2c board info */
> > + name = DEVNAME;
> > +
> > strlcpy(info->type, name, I2C_NAME_SIZE);
> > return 0;
> > }
You are hiding the key change with cleanups and logic inversion. That's
not really desirable in general, and even less so for a fix that IMHO
should go to stable trees. Given that you are reworking the logic in a
later patch anyway ("Add support for LM95231") I'd much prefer that
this first patch simply turns ">=" into "==". This will make the fix
much more obvious.
Thanks,
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
