Re: [PATCH] hwmon: (lm92) Do not try to detect MAX6635

[Guenter Roeck <linux@roeck-us.net>]

On Thu, Mar 22, 2018 at 10:17:02AM +0100, Jean Delvare wrote:
> On Wed, 21 Mar 2018 16:04:52 +0100, Alvaro Gamez Machado wrote:
> > Maxim MAX663x family are mostly compatible with LM92, but they lack any
> > identification register. Weakening the detect function would make it prone
> > to false positives, and current one doesn't detect all chips.  Therefore,
> > the detect function for max6635 devices is removed in favor of explicit
> > device instatiation.
> > 
> > Signed-off-by: Alvaro Gamez Machado <alvaro.gamez@hazent.com>
> > ---
> >  Documentation/hwmon/lm92 |  4 +---
> >  drivers/hwmon/lm92.c     | 58 ------------------------------------------------
> >  2 files changed, 1 insertion(+), 61 deletions(-)
> > 
> > diff --git a/Documentation/hwmon/lm92 b/Documentation/hwmon/lm92
> > index 22f68ad032cf..f2a5adcf4ead 100644
> > --- a/Documentation/hwmon/lm92
> > +++ b/Documentation/hwmon/lm92
> > @@ -12,9 +12,7 @@ Supported chips:
> >      Datasheet: http://www.national.com/pf/LM/LM76.html
> >    * Maxim MAX6633/MAX6634/MAX6635
> >      Prefix: 'lm92'
> > -    Addresses scanned: I2C 0x48 - 0x4b
> > -    MAX6633 with address in 0x40 - 0x47, 0x4c - 0x4f needs force parameter
> > -    and MAX6634 with address in 0x4c - 0x4f needs force parameter
> > +    Addresses scanned: none, force parameter needed
> >      Datasheet: http://www.maxim-ic.com/quick_view2.cfm/qv_pk/3074
> >  
> >  Authors:
> > diff --git a/drivers/hwmon/lm92.c b/drivers/hwmon/lm92.c
> > index 2a91974a10bb..18509b5af11e 100644
> > --- a/drivers/hwmon/lm92.c
> > +++ b/drivers/hwmon/lm92.c
> > @@ -259,62 +259,6 @@ static void lm92_init_client(struct i2c_client *client)
> >  					  config & 0xFE);
> >  }
> >  
> > -/*
> > - * The MAX6635 has no identification register, so we have to use tricks
> > - * to identify it reliably. This is somewhat slow.
> > - * Note that we do NOT rely on the 2 MSB of the configuration register
> > - * always reading 0, as suggested by the datasheet, because it was once
> > - * reported not to be true.
> > - */
> > -static int max6635_check(struct i2c_client *client)
> > -{
> > -	u16 temp_low, temp_high, temp_hyst, temp_crit;
> > -	u8 conf;
> > -	int i;
> > -
> > -	/*
> > -	 * No manufacturer ID register, so a read from this address will
> > -	 * always return the last read value.
> > -	 */
> > -	temp_low = i2c_smbus_read_word_data(client, LM92_REG_TEMP_LOW);
> > -	if (i2c_smbus_read_word_data(client, LM92_REG_MAN_ID) != temp_low)
> > -		return 0;
> > -	temp_high = i2c_smbus_read_word_data(client, LM92_REG_TEMP_HIGH);
> > -	if (i2c_smbus_read_word_data(client, LM92_REG_MAN_ID) != temp_high)
> > -		return 0;
> > -
> > -	/* Limits are stored as integer values (signed, 9-bit). */
> > -	if ((temp_low & 0x7f00) || (temp_high & 0x7f00))
> > -		return 0;
> > -	temp_hyst = i2c_smbus_read_word_data(client, LM92_REG_TEMP_HYST);
> > -	temp_crit = i2c_smbus_read_word_data(client, LM92_REG_TEMP_CRIT);
> > -	if ((temp_hyst & 0x7f00) || (temp_crit & 0x7f00))
> > -		return 0;
> > -
> > -	/*
> > -	 * Registers addresses were found to cycle over 16-byte boundaries.
> > -	 * We don't test all registers with all offsets so as to save some
> > -	 * reads and time, but this should still be sufficient to dismiss
> > -	 * non-MAX6635 chips.
> > -	 */
> > -	conf = i2c_smbus_read_byte_data(client, LM92_REG_CONFIG);
> > -	for (i = 16; i < 96; i *= 2) {
> > -		if (temp_hyst != i2c_smbus_read_word_data(client,
> > -				 LM92_REG_TEMP_HYST + i - 16)
> > -		 || temp_crit != i2c_smbus_read_word_data(client,
> > -				 LM92_REG_TEMP_CRIT + i)
> > -		 || temp_low != i2c_smbus_read_word_data(client,
> > -				LM92_REG_TEMP_LOW + i + 16)
> > -		 || temp_high != i2c_smbus_read_word_data(client,
> > -				 LM92_REG_TEMP_HIGH + i + 32)
> > -		 || conf != i2c_smbus_read_byte_data(client,
> > -			    LM92_REG_CONFIG + i))
> > -			return 0;
> > -	}
> > -
> > -	return 1;
> > -}
> > -
> >  static struct attribute *lm92_attrs[] = {
> >  	&sensor_dev_attr_temp1_input.dev_attr.attr,
> >  	&sensor_dev_attr_temp1_crit.dev_attr.attr,
> > @@ -348,8 +292,6 @@ static int lm92_detect(struct i2c_client *new_client,
> >  
> >  	if ((config & 0xe0) == 0x00 && man_id == 0x0180)
> >  		pr_info("lm92: Found National Semiconductor LM92 chip\n");
> > -	else if (max6635_check(new_client))
> > -		pr_info("lm92: Found Maxim MAX6635 chip\n");
> >  	else
> >  		return -ENODEV;
> >  
> 
> Reviewed-by: Jean Delvare <jdelvare@suse.de>
> 
> It would probably make sense to add prefix "max6635" to lm92_id[] so
> that the device can be instantiated by its actual name.

Excellent idea. I took the freedom to add that to the patch.

Thanks,
Guenter

> 
> -- 
> Jean Delvare
> SUSE L3 Support