[lm-sensors] [PATCH] hwmon: (lm95241) Fix chip detection code

[Guenter Roeck]

On Thu, Jul 07, 2011 at 05:29:48AM -0400, Jean Delvare wrote:
> 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 at ericsson.com>
> > 
> > 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.
> 
Hi Jean,

Modified as suggested, and I resubmitted the series.

Thanks,
Guenter