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

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

[Alvaro Gamez Machado]

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;
 
-- 
2.16.2

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

[Guenter Roeck]

On Wed, Mar 21, 2018 at 04:04:52PM +0100, Alvaro G. M 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>

Applied to hwmon-next.

Thanks,
Guenter

> ---
>  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;
>  

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

[Jean Delvare]

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.

-- 
Jean Delvare
SUSE L3 Support

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

[Guenter Roeck]

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