diff mbox series

[v1,1/2] hwmon: (lm90): Add support for NCT7716, NCT7717 and NCT7718

Message ID 20250110082612.4107571-2-a0282524688@gmail.com (mailing list archive)
State Changes Requested
Headers show
Series hwmon: lm90: Add support for NCT7716, NCT7717 and NCT7718 | expand

Commit Message

Ming Yu Jan. 10, 2025, 8:26 a.m. UTC 
NCT7716 is similar to NCT7717 but has one more address support,
both of them only have a 8 bit resolution local thermal sensor.
NCT7718 has 11 bit resoulution remote thermal sensor.

Signed-off-by: Ming Yu <a0282524688@gmail.com>
---
 Documentation/hwmon/lm90.rst | 43 +++++++++++++++++++++++
 drivers/hwmon/Kconfig        |  2 +-
 drivers/hwmon/lm90.c         | 67 ++++++++++++++++++++++++++++++++----
 3 files changed, 105 insertions(+), 7 deletions(-)

Comments

Guenter Roeck Jan. 10, 2025, 3:56 p.m. UTC | #1 
On 1/10/25 00:26, Ming Yu wrote:
...
> @@ -2288,7 +2329,19 @@ static const char *lm90_detect_nuvoton(struct i2c_client *client, int chip_id,
>   	if (config2 < 0)
>   		return NULL;
>   
> -	if (address == 0x4c && !(config1 & 0x2a) && !(config2 & 0xf8)) {
> +	if (address == 0x48 && !(config1 & 0x30) && !(config2 & 0xfe) &&

Why config1 & 0x30 (instead of 0x3e) ?

> +	    convrate <= 0x08) {
> +		if (chip_id == 0x90)
> +			name = "nct7717";
> +		else if (chip_id == 0x91)
> +			name = "nct7716";
> +	} else if (address == 0x49 && !(config1 & 0x30) && !(config2 & 0xfe) &&
> +		   convrate <= 0x08) {
> +		name = "nct7716";

Please also check the chip ID, and the other unused configuration register bits.

> +	} else if (address == 0x4c && !(config1 & 0x18) && !(config2 & 0xf8) &&
> +		   convrate <= 0x08) {
> +		name = "nct7718";

Please also check the chip ID (0x90 according to the datasheet). Why not check bit 5
of config1 ?

If there is a reason for not checking the reserved configuration register bits,
please add a comment to the code explaining the reason.

> +	} else if (address == 0x4c && !(config1 & 0x2a) && !(config2 & 0xf8)) {
>   		if (chip_id == 0x01 && convrate <= 0x09) {
>   			/* W83L771W/G */
>   			name = "w83l771";
> @@ -2297,6 +2350,7 @@ static const char *lm90_detect_nuvoton(struct i2c_client *client, int chip_id,
>   			name = "w83l771";
>   		}
>   	}
> +
>   	return name;
>   }
>   
> @@ -2484,6 +2538,10 @@ static int lm90_detect(struct i2c_client *client, struct i2c_board_info *info)
>   		name = lm90_detect_maxim(client, common_address, chip_id,
>   					 config1, convrate);
>   		break;
> +	case 0x50:	/* Nuvoton */
> +	case 0x5c:	/* Winbond/Nuvoton */

The new detection code should be implemented as separate function to avoid
weakening the detection mechanism. I would suggest to rename the current
lm90_detect_nuvoton() to lm90_detect_winbond() and introduce a new
lm90_detect_nuvoton(). Alternatively, add something like lm90_detect_nuvoton_50().

Given that all new chips have a chip ID register (called device ID), I would suggest
to arrange the new code around the chip IDs. Since all chips have another chip ID
register at address 0xfd, it would make sense to check that register as well.
That would only require a single check since it looks like the value is the same
for all chips. Something like

	int chid = i2c_smbus_read_byte_data(client, 0xfd);
	...

	if (chid < 0 || config2 < 0)
		return NULL;

	if (chid != 0x50 || convrate > 0x08)
		return NULL;

	switch (chip_id) {
	case 0x90:
		...
	case 0x91:
		...
	default:
		...
	}

Thanks,
Guenter
Ming Yu Jan. 15, 2025, 7:42 a.m. UTC | #2 
Dear Guenter,

Thank you for your comments,

Guenter Roeck <linux@roeck-us.net> 於 2025年1月10日 週五 下午11:56寫道：
>
> On 1/10/25 00:26, Ming Yu wrote:
> ...
> > @@ -2288,7 +2329,19 @@ static const char *lm90_detect_nuvoton(struct i2c_client *client, int chip_id,
> >       if (config2 < 0)
> >               return NULL;
> >
> > -     if (address == 0x4c && !(config1 & 0x2a) && !(config2 & 0xf8)) {
> > +     if (address == 0x48 && !(config1 & 0x30) && !(config2 & 0xfe) &&
>
> Why config1 & 0x30 (instead of 0x3e) ?
>

Fix it in the next patch.

> > +         convrate <= 0x08) {
> > +             if (chip_id == 0x90)
> > +                     name = "nct7717";
> > +             else if (chip_id == 0x91)
> > +                     name = "nct7716";
> > +     } else if (address == 0x49 && !(config1 & 0x30) && !(config2 & 0xfe) &&
> > +                convrate <= 0x08) {
> > +             name = "nct7716";
>
> Please also check the chip ID, and the other unused configuration register bits.
>

Fix it in the next patch.

> > +     } else if (address == 0x4c && !(config1 & 0x18) && !(config2 & 0xf8) &&
> > +                convrate <= 0x08) {
> > +             name = "nct7718";
>
> Please also check the chip ID (0x90 according to the datasheet). Why not check bit 5
> of config1 ?
>
> If there is a reason for not checking the reserved configuration register bits,
> please add a comment to the code explaining the reason.
>

Fix it in the next patch.

> > +     } else if (address == 0x4c && !(config1 & 0x2a) && !(config2 & 0xf8)) {
> >               if (chip_id == 0x01 && convrate <= 0x09) {
> >                       /* W83L771W/G */
> >                       name = "w83l771";
> > @@ -2297,6 +2350,7 @@ static const char *lm90_detect_nuvoton(struct i2c_client *client, int chip_id,
> >                       name = "w83l771";
> >               }
> >       }
> > +
> >       return name;
> >   }
> >
> > @@ -2484,6 +2538,10 @@ static int lm90_detect(struct i2c_client *client, struct i2c_board_info *info)
> >               name = lm90_detect_maxim(client, common_address, chip_id,
> >                                        config1, convrate);
> >               break;
> > +     case 0x50:      /* Nuvoton */
> > +     case 0x5c:      /* Winbond/Nuvoton */
>
> The new detection code should be implemented as separate function to avoid
> weakening the detection mechanism. I would suggest to rename the current
> lm90_detect_nuvoton() to lm90_detect_winbond() and introduce a new
> lm90_detect_nuvoton(). Alternatively, add something like lm90_detect_nuvoton_50().
>
> Given that all new chips have a chip ID register (called device ID), I would suggest
> to arrange the new code around the chip IDs. Since all chips have another chip ID
> register at address 0xfd, it would make sense to check that register as well.
> That would only require a single check since it looks like the value is the same
> for all chips. Something like
>
>         int chid = i2c_smbus_read_byte_data(client, 0xfd);
>         ...
>
>         if (chid < 0 || config2 < 0)
>                 return NULL;
>
>         if (chid != 0x50 || convrate > 0x08)
>                 return NULL;
>
>         switch (chip_id) {
>         case 0x90:
>                 ...
>         case 0x91:
>                 ...
>         default:
>                 ...
>         }
>
Okay, I will make these modifications in the next patch.


Best regards,
Ming
diff mbox series

Patch

diff --git a/Documentation/hwmon/lm90.rst b/Documentation/hwmon/lm90.rst
index 23af17a0ab44..a53c82a8c15e 100644
--- a/Documentation/hwmon/lm90.rst
+++ b/Documentation/hwmon/lm90.rst
@@ -365,6 +365,34 @@  Supported chips:
 
     Datasheet: Not publicly available, can be requested from Nuvoton
 
+  * Nuvoton NCT7716
+
+    Prefix: 'nct7716'
+
+    Addresses scanned: I2C 0x48, 0x49
+
+    Datasheet: Not publicly available, can be requested from Nuvoton
+
+  * Nuvoton NCT7717
+
+    Prefix: 'nct7717'
+
+    Addresses scanned: I2C 0x48
+
+    Datasheet: Publicly available at Nuvoton website
+
+               https://www.nuvoton.com/resource-files/Nuvoton_NCT7717U_Datasheet_V111.pdf
+
+  * Nuvoton NCT7718
+
+    Prefix: 'nct7718'
+
+    Addresses scanned: I2C 04xc
+
+    Datasheet: Publicly available at Nuvoton website
+
+               https://www.nuvoton.com/resource-files/Nuvoton_NCT7718W_Datasheet_V11.pdf
+
   * Philips/NXP SA56004X
 
     Prefix: 'sa56004'
@@ -573,6 +601,21 @@  W83L771AWG/ASG
   * The AWG and ASG variants only differ in package format.
   * Diode ideality factor configuration (remote sensor) at 0xE3
 
+NCT7716:
+  * 8 bit sensor resolution
+  * Selectable address
+  * Configurable conversion rate
+
+NCT7717:
+  * 8 bit sensor resolution
+  * Configurable conversion rate
+
+NCT7718:
+  * Temperature offset register for remote temperature sensor
+  * 11 bit resolution for remote temperature sensor
+  * Low temperature limits
+  * Configurable conversion rate
+
 SA56004X:
   * Better local resolution
 
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index dd376602f3f1..70c4717b37b4 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1517,7 +1517,7 @@  config SENSORS_LM90
 	  MAX6657, MAX6658, MAX6659, MAX6680, MAX6681, MAX6692, MAX6695,
 	  MAX6696,
 	  ON Semiconductor NCT1008, NCT210, NCT72, NCT214, NCT218,
-	  Winbond/Nuvoton W83L771W/G/AWG/ASG,
+	  Winbond/Nuvoton W83L771W/G/AWG/ASG, NCT7716, NCT7717 and NCT7718,
 	  Philips NE1618, SA56004, GMT G781, Texas Instruments TMP451 and TMP461
 	  sensor chips.
 
diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 511d95a0efb3..266998c6fd1e 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -90,6 +90,9 @@ 
  * This driver also supports NE1618 from Philips. It is similar to NE1617
  * but supports 11 bit external temperature values.
  *
+ * This driver also supports NCT7716, NCT7717 and NCT7718 from Nuvoton.
+ * The NCT7716 is similar to NCT7717 but has one more address support.
+ *
  * Since the LM90 was the first chipset supported by this driver, most
  * comments will refer to this chipset, but are actually general and
  * concern all supported chipsets, unless mentioned otherwise.
@@ -119,13 +122,15 @@ 
  * Address is fully defined internally and cannot be changed except for
  * MAX6659, MAX6680 and MAX6681.
  * LM86, LM89, LM90, LM99, ADM1032, ADM1032-1, ADT7461, ADT7461A, MAX6649,
- * MAX6657, MAX6658, NCT1008 and W83L771 have address 0x4c.
+ * MAX6657, MAX6658, NCT1008, NCT7718 and W83L771 have address 0x4c.
  * ADM1032-2, ADT7461-2, ADT7461A-2, LM89-1, LM99-1, MAX6646, and NCT1008D
  * have address 0x4d.
  * MAX6647 has address 0x4e.
  * MAX6659 can have address 0x4c, 0x4d or 0x4e.
  * MAX6654, MAX6680, and MAX6681 can have address 0x18, 0x19, 0x1a, 0x29,
  * 0x2a, 0x2b, 0x4c, 0x4d or 0x4e.
+ * NCT7716 can have address 0x48 or 0x49.
+ * NCT7717 has address 0x48.
  * SA56004 can have address 0x48 through 0x4F.
  */
 
@@ -136,7 +141,8 @@  static const unsigned short normal_i2c[] = {
 enum chips { adm1023, adm1032, adt7461, adt7461a, adt7481,
 	g781, lm84, lm90, lm99,
 	max1617, max6642, max6646, max6648, max6654, max6657, max6659, max6680, max6696,
-	nct210, nct72, ne1618, sa56004, tmp451, tmp461, w83l771,
+	nct210, nct72, nct7716, nct7717, nct7718, ne1618, sa56004, tmp451, tmp461,
+	w83l771,
 };
 
 /*
@@ -275,6 +281,9 @@  static const struct i2c_device_id lm90_id[] = {
 	{ "nct214", nct72 },
 	{ "nct218", nct72 },
 	{ "nct72", nct72 },
+	{ "nct7716", nct7716 },
+	{ "nct7717", nct7717 },
+	{ "nct7718", nct7718 },
 	{ "ne1618", ne1618 },
 	{ "w83l771", w83l771 },
 	{ "sa56004", sa56004 },
@@ -382,6 +391,18 @@  static const struct of_device_id __maybe_unused lm90_of_match[] = {
 		.compatible = "onnn,nct72",
 		.data = (void *)nct72
 	},
+	{
+		.compatible = "nuvoton,nct7716",
+		.data = (void *)nct7716
+	},
+	{
+		.compatible = "nuvoton,nct7717",
+		.data = (void *)nct7717
+	},
+	{
+		.compatible = "nuvoton,nct7718",
+		.data = (void *)nct7718
+	},
 	{
 		.compatible = "winbond,w83l771",
 		.data = (void *)w83l771
@@ -593,6 +614,26 @@  static const struct lm90_params lm90_params[] = {
 		.max_convrate = 10,
 		.resolution = 10,
 	},
+	[nct7716] = {
+		.flags = LM90_HAVE_ALARMS | LM90_HAVE_CONVRATE,
+		.alert_alarms = 0x40,
+		.resolution = 8,
+		.max_convrate = 8,
+	},
+	[nct7717] = {
+		.flags = LM90_HAVE_ALARMS | LM90_HAVE_CONVRATE,
+		.alert_alarms = 0x40,
+		.resolution = 8,
+		.max_convrate = 8,
+	},
+	[nct7718] = {
+		.flags = LM90_HAVE_OFFSET | LM90_HAVE_REM_LIMIT_EXT | LM90_HAVE_CRIT
+		  | LM90_HAVE_ALARMS | LM90_HAVE_LOW | LM90_HAVE_CONVRATE
+		  | LM90_HAVE_REMOTE_EXT,
+		.alert_alarms = 0x7c,
+		.resolution = 11,
+		.max_convrate = 8,
+	},
 	[nct210] = {
 		.flags = LM90_HAVE_ALARMS | LM90_HAVE_BROKEN_ALERT
 		  | LM90_HAVE_REM_LIMIT_EXT | LM90_HAVE_LOW | LM90_HAVE_CONVRATE
@@ -2288,7 +2329,19 @@  static const char *lm90_detect_nuvoton(struct i2c_client *client, int chip_id,
 	if (config2 < 0)
 		return NULL;
 
-	if (address == 0x4c && !(config1 & 0x2a) && !(config2 & 0xf8)) {
+	if (address == 0x48 && !(config1 & 0x30) && !(config2 & 0xfe) &&
+	    convrate <= 0x08) {
+		if (chip_id == 0x90)
+			name = "nct7717";
+		else if (chip_id == 0x91)
+			name = "nct7716";
+	} else if (address == 0x49 && !(config1 & 0x30) && !(config2 & 0xfe) &&
+		   convrate <= 0x08) {
+		name = "nct7716";
+	} else if (address == 0x4c && !(config1 & 0x18) && !(config2 & 0xf8) &&
+		   convrate <= 0x08) {
+		name = "nct7718";
+	} else if (address == 0x4c && !(config1 & 0x2a) && !(config2 & 0xf8)) {
 		if (chip_id == 0x01 && convrate <= 0x09) {
 			/* W83L771W/G */
 			name = "w83l771";
@@ -2297,6 +2350,7 @@  static const char *lm90_detect_nuvoton(struct i2c_client *client, int chip_id,
 			name = "w83l771";
 		}
 	}
+
 	return name;
 }
 
@@ -2484,6 +2538,10 @@  static int lm90_detect(struct i2c_client *client, struct i2c_board_info *info)
 		name = lm90_detect_maxim(client, common_address, chip_id,
 					 config1, convrate);
 		break;
+	case 0x50:	/* Nuvoton */
+	case 0x5c:	/* Winbond/Nuvoton */
+		name = lm90_detect_nuvoton(client, chip_id, config1, convrate);
+		break;
 	case 0x54:	/* ON MC1066, Microchip TC1068, TCM1617 (originally TelCom) */
 		if (common_address && !(config1 & 0x3f) && !(convrate & 0xf8))
 			name = "mc1066";
@@ -2491,9 +2549,6 @@  static int lm90_detect(struct i2c_client *client, struct i2c_board_info *info)
 	case 0x55:	/* TI */
 		name = lm90_detect_ti(client, chip_id, config1, convrate);
 		break;
-	case 0x5c:	/* Winbond/Nuvoton */
-		name = lm90_detect_nuvoton(client, chip_id, config1, convrate);
-		break;
 	case 0xa1:	/*  NXP Semiconductor/Philips */
 		name = lm90_detect_nxp(client, common_address, chip_id, config1, convrate);
 		break;