Skip to contentSkip to conversation

Add W83773G hwmon sensor driver and doc

PATCH Superseded Lei YU 3 participants 7 messages Nov 2, 2017 – Nov 6, 2017 Hardware monitoring

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Lei YU

author
#0 lore raw

cover Add W83773G hwmon sensor driver and doc

Nuvoton W83773G is a hardware monitoring chip, which integrates two remote and one local temperature sensors.

Lei YU (2):
drivers: hwmon: Add W83773G driver
hwmon: (w83773g) Add documentation

 Documentation/hwmon/w83773g |  28 +++++
 drivers/hwmon/Kconfig       |  11 ++
 drivers/hwmon/Makefile      |   1 +
 drivers/hwmon/w83773g.c     | 276 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 316 insertions(+)
 create mode 100644 Documentation/hwmon/w83773g
 create mode 100644 drivers/hwmon/w83773g.c
Signature · 1 line
1.9.1
author
#1 lore raw

PATCH 1/2 drivers: hwmon: Add W83773G driver

Nuvoton W83773G is a hardware monitor IC providing one local temperature and two remote temperature sensors.

Signed-off-by Lei YU <[email protected]>
---
 drivers/hwmon/Kconfig   |  11 ++
 drivers/hwmon/Makefile  |   1 +
 drivers/hwmon/w83773g.c | 276 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 288 insertions(+)
 create mode 100644 drivers/hwmon/w83773g.c
drivers/hwmon/Kconfig +11 -0
index d654314..d148b70 100644
@@ -1710,6 +1710,17 @@ config SENSORS_VT8231
 	  This driver can also be built as a module.  If so, the module
 	  will be called vt8231.
 
+config SENSORS_W83773G
+	tristate "Nuvoton W83773G"
+	depends on I2C
+	select HWMON_VID
+	help
+	  If you say yes here you get support for the Nuvoton W83773G hardware
+	  monitoring chip.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called w83773g.
+
 config SENSORS_W83781D
 	tristate "Winbond W83781D, W83782D, W83783S, Asus AS99127F"
 	depends on I2C
drivers/hwmon/Makefile +1 -0
index c84d978..0649ad8 100644
@@ -13,6 +13,7 @@ obj-$(CONFIG_SENSORS_ATK0110)	+= asus_atk0110.o
 # asb100, then w83781d go first, as they can override other drivers' addresses.
 obj-$(CONFIG_SENSORS_ASB100)	+= asb100.o
 obj-$(CONFIG_SENSORS_W83627HF)	+= w83627hf.o
+obj-$(CONFIG_SENSORS_W83773G)	+= w83773g.o
 obj-$(CONFIG_SENSORS_W83792D)	+= w83792d.o
 obj-$(CONFIG_SENSORS_W83793)	+= w83793.o
 obj-$(CONFIG_SENSORS_W83795)	+= w83795.o
drivers/hwmon/w83773g.c new file +276 -0

276 lines not on this page · raw message

Signature · 1 line
1.9.1
maintainer
#2 lore raw

Re: [PATCH 1/2] drivers: hwmon: Add W83773G driver

Quoted 3 lines · Lei YU

Nuvoton W83773G is a hardware monitor IC providing one local temperature and two remote temperature sensors.

This chip is pretty close to LM90. Have you explored adding support for it to the LM90 driver ?

Quoted 325 lines
Signed-off-by Lei YU <[email protected]>
---
  drivers/hwmon/Kconfig   |  11 ++
  drivers/hwmon/Makefile  |   1 +
  drivers/hwmon/w83773g.c | 276 ++++++++++++++++++++++++++++++++++++++++++++++++
  3 files changed, 288 insertions(+)

create mode 100644 drivers/hwmon/w83773g.c

drivers/hwmon/Kconfig +11 -0
index d654314..d148b70 100644
@@ -1710,6 +1710,17 @@ config SENSORS_VT8231
  	  This driver can also be built as a module.  If so, the module
  	  will be called vt8231.
 
+config SENSORS_W83773G
+	tristate "Nuvoton W83773G"
+	depends on I2C
+	select HWMON_VID
+	help
+	  If you say yes here you get support for the Nuvoton W83773G hardware
+	  monitoring chip.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called w83773g.
+
  config SENSORS_W83781D
  	tristate "Winbond W83781D, W83782D, W83783S, Asus AS99127F"
  	depends on I2C
drivers/hwmon/Makefile +1 -0
index c84d978..0649ad8 100644
@@ -13,6 +13,7 @@ obj-$(CONFIG_SENSORS_ATK0110)	+= asus_atk0110.o
  # asb100, then w83781d go first, as they can override other drivers' addresses.
  obj-$(CONFIG_SENSORS_ASB100)	+= asb100.o
  obj-$(CONFIG_SENSORS_W83627HF)	+= w83627hf.o
+obj-$(CONFIG_SENSORS_W83773G)	+= w83773g.o
  obj-$(CONFIG_SENSORS_W83792D)	+= w83792d.o
  obj-$(CONFIG_SENSORS_W83793)	+= w83793.o
  obj-$(CONFIG_SENSORS_W83795)	+= w83795.o
drivers/hwmon/w83773g.c new file +276 -0
new file mode 100644
index 0000000..16d5fa0
@@ -0,0 +1,276 @@
+/*
+ * Copyright (C) 2017 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ */
+
+/*
+ * Driver for the Nuvoton W83773G SMBus temperature sensor IC.
+ * Supported models: W83773G
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/jiffies.h>
+#include <linux/i2c.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/of_device.h>
+#include <linux/sysfs.h>
+
+/* Addresses to scan */
+static const unsigned short normal_i2c[] = { 0x4c, 0x4d, I2C_CLIENT_END };
+
+enum chips { w83773g };
+
+/* The W83773 registers */
+#define W83773_CONVERSION_RATE_REG_READ 0x04
+#define W83773_CONVERSION_RATE_REG_WRITE 0x0A
+#define W83773_MANUFACTURER_ID_REG 0xFE
+#define W83773_LOCAL_TEMP 0x00
+
+static const u8 W83773_STATUS[2] = { 0x02, 0x17 };
+
+static const u8 W83773_TEMP_LSB[2] = { 0x10, 0x25 };
+static const u8 W83773_TEMP_MSB[2] = { 0x01, 0x24 };
+
+static const u8 W83773_OFFSET_LSB[2] = { 0x12, 0x16 };
+static const u8 W83773_OFFSET_MSB[2] = { 0x11, 0x15 };
+
+/* Manufacturer / Device ID's */
+#define W83773_MANUFACTURER_ID			0x5c
+
+
+/* this is the number of sensors in the device */
+static const struct i2c_device_id w83773_id[] = {
+	{ "w83773g", 3 },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(i2c, w83773_id);
+
+static const struct of_device_id w83773_of_match[] = {
+	{
+		.compatible = "nuvoton,w83773g",
+		.data = (void *)3
+	},
+
+	{ },
+};
+MODULE_DEVICE_TABLE(of, w83773_of_match);
+
+/*
+ * W83773G has 3 temp sensors:
+ *   Channel 0 is the local sensor
+ *   Channel 1-2 are two remote sensors
+ */
+struct w83773_data {
+	struct i2c_client *client;
+	struct mutex update_lock;
+	u32 temp_config[4];
+	struct hwmon_channel_info temp_info;
+	const struct hwmon_channel_info *info[2];
+	struct hwmon_chip_info chip;
+	bool valid;
+	unsigned long last_updated;
+	int channels;
+	s8 temp_local;
+	s8 status[2];
+	s8 temp_hb[2];
+	s8 temp_lb[2];
+	s8 offset_hb[2];
+	s8 offset_lb[2];
+};
+
+static long temp_of_local(s8 reg)
+{
+	return reg * 1000;
+}
+
+static long temp_of_remote(s8 hb, s8 lb, s8 offset_hb, s8 offset_lb)
+{
+	return (hb + offset_hb) * 1000 + ((u8)(lb + offset_lb) >> 5) * 125;
+}
+
+
+static struct w83773_data *w83773_update_device(struct device *dev)
+{
+	struct w83773_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
+	int i;
+
+	mutex_lock(&data->update_lock);
+
+	if (time_after(jiffies, data->last_updated + 2 * HZ) || !data->valid) {
+		data->temp_local = i2c_smbus_read_byte_data(client, W83773_LOCAL_TEMP);
+		for (i = 0; i < data->channels - 1; i++) {
+			data->status[i] = i2c_smbus_read_byte_data(client, W83773_STATUS[i]);
+			data->temp_hb[i] = i2c_smbus_read_byte_data(client, W83773_TEMP_MSB[i]);
+			data->temp_lb[i] = i2c_smbus_read_byte_data(client, W83773_TEMP_LSB[i]);
+			data->offset_hb[i] = i2c_smbus_read_byte_data(client, W83773_OFFSET_MSB[i]);
+			data->offset_lb[i] = i2c_smbus_read_byte_data(client, W83773_OFFSET_LSB[i]);
+		}
+		data->last_updated = jiffies;
+		data->valid = true;
+	}
+
+	mutex_unlock(&data->update_lock);
+
+	return data;
+}
+
+static int w83773_read(struct device *dev, enum hwmon_sensor_types type,
+			u32 attr, int channel, long *val)
+{
+	struct w83773_data *w83773 = w83773_update_device(dev);
+
+	switch (attr) {
+	case hwmon_temp_input:
+		if (channel == 0) {
+			/* channel 0 is the local temp */
+			*val = temp_of_local(w83773->temp_local);
+		}
+		else {
+			/* channel 1-2 are the remote temps */
+			channel--;
+			*val = temp_of_remote(
+				w83773->temp_hb[channel],
+				w83773->temp_lb[channel],
+				w83773->offset_hb[channel],
+				w83773->offset_lb[channel]);
+		}
+		return 0;
+	case hwmon_temp_fault:
+		if (channel == 0)
+			*val = 0;
+		else
+			/* Check the status register bit 2 for faults */
+			*val = (w83773->status[channel - 1] & 0x04) >> 2;
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+}
+
+static umode_t w83773_is_visible(const void *data, enum hwmon_sensor_types type,
+				 u32 attr, int channel)
+{
+	switch (attr) {
+	case hwmon_temp_fault:
+		if (channel == 0)
+			return 0;
+		return S_IRUGO;
+	case hwmon_temp_input:
+		return S_IRUGO;
+	default:
+		return 0;
+	}
+}
+
+static int w83773_init_client(struct i2c_client *client)
+{
+	/* Set the conversion rate to 2 Hz */
+	i2c_smbus_write_byte_data(client, W83773_CONVERSION_RATE_REG_WRITE, 0x05);
+
+	return 0;
+}
+
+static int w83773_detect(struct i2c_client *client,
+			 struct i2c_board_info *info)
+{
+	enum chips kind;
+	struct i2c_adapter *adapter = client->adapter;
+	const char * const names[] = { "W83773G" };
+	u8 reg;
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
+		return -ENODEV;
+
+	reg = i2c_smbus_read_byte_data(client, W83773_MANUFACTURER_ID_REG);
+	if (reg != W83773_MANUFACTURER_ID)
+		return -ENODEV;
+
+	reg = i2c_smbus_read_byte_data(client, W83773_CONVERSION_RATE_REG_READ);
+	if (reg & 0xf8)
+		return -ENODEV;
+
+	kind = w83773g;
+
+	strlcpy(info->type, w83773_id[kind].name, I2C_NAME_SIZE);
+	dev_info(&adapter->dev, "Detected Nuvoton %s chip at 0x%02x\n",
+		 names[kind], client->addr);
+
+	return 0;
+}
+
+static const struct hwmon_ops w83773_ops = {
+	.is_visible = w83773_is_visible,
+	.read = w83773_read,
+};
+
+static int w83773_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;
+	struct device *hwmon_dev;
+	struct w83773_data *data;
+	int i, err;
+
+	data = devm_kzalloc(dev, sizeof(struct w83773_data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	mutex_init(&data->update_lock);
+	if (client->dev.of_node)
+		data->channels = (int)of_device_get_match_data(&client->dev);
+	else
+		data->channels = id->driver_data;
+	data->client = client;
+
+	err = w83773_init_client(client);
+	if (err)
+		return err;
+
+	for (i = 0; i < data->channels; i++)
+		data->temp_config[i] = HWMON_T_INPUT | HWMON_T_FAULT;
+
+	data->chip.ops = &w83773_ops;
+	data->chip.info = data->info;
+
+	data->info[0] = &data->temp_info;
+
+	data->temp_info.type = hwmon_temp;
+	data->temp_info.config = data->temp_config;
+
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
+							 data,
+							 &data->chip,
+							 NULL);
+	return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static struct i2c_driver w83773_driver = {
+	.class = I2C_CLASS_HWMON,
+	.driver = {
+		.name	= "w83773g",
+		.of_match_table = of_match_ptr(w83773_of_match),
+	},
+	.probe = w83773_probe,
+	.id_table = w83773_id,
+	.detect = w83773_detect,
+	.address_list = normal_i2c,
+};
+
+module_i2c_driver(w83773_driver);
+
+MODULE_AUTHOR("Nickolaus Gruendler <[email protected]>");
+MODULE_DESCRIPTION("W83773G temperature sensor driver");
+MODULE_LICENSE("GPL");
author
#3 lore raw

Re: [PATCH 1/2] drivers: hwmon: Add W83773G driver

Quoted 7 lines · Guenter Roeck
Quoted 4 lines · Lei YU

Nuvoton W83773G is a hardware monitor IC providing one local temperature and two remote temperature sensors.

This chip is pretty close to LM90. Have you explored adding support for it to the LM90 driver ?

Thanks for the info.
After checking the driver of lm90, I find several differences:
1. Most of devices in lm90.c have one local and one remote sensor; Only MAX6695/MAX6696 support two remote sensors;
2. Most of the devices in lm90.c support ALERT, Hight/Low limit.

Especially for 1. MAX6695/6696 needs to write config register to switch between the two remote sensors by lm90_select_remote_channel();
While W83773G has two set of registers for the two remote sensors; The code to support two remote sensors would be different.

So I would prefer to have a separated driver code for this. What do you think?

Thanks!

Quoted 345 lines
Quoted 344 lines
Signed-off-by Lei YU <[email protected]>
---
  drivers/hwmon/Kconfig   |  11 ++
  drivers/hwmon/Makefile  |   1 +
  drivers/hwmon/w83773g.c | 276
++++++++++++++++++++++++++++++++++++++++++++++++
  3 files changed, 288 insertions(+)

create mode 100644 drivers/hwmon/w83773g.c

drivers/hwmon/Kconfig +5 -0
index d654314..d148b70 100644
@@ -1710,6 +1710,17 @@ config SENSORS_VT8231
          This driver can also be built as a module.  If so, the module
          will be called vt8231.
  +config SENSORS_W83773G
+       tristate "Nuvoton W83773G"
+       depends on I2C
+       select HWMON_VID
+       help
+         If you say yes here you get support for the Nuvoton W83773G

hardware

drivers/hwmon/Kconfig +5 -0
+         monitoring chip.
+
+         This driver can also be built as a module.  If so, the module
+         will be called w83773g.
+
  config SENSORS_W83781D
        tristate "Winbond W83781D, W83782D, W83783S, Asus AS99127F"
        depends on I2C
drivers/hwmon/Makefile +0 -0
index c84d978..0649ad8 100644
@@ -13,6 +13,7 @@ obj-$(CONFIG_SENSORS_ATK0110) += asus_atk0110.o
  # asb100, then w83781d go first, as they can override other drivers'

addresses.
obj-$(CONFIG_SENSORS_ASB100) += asb100.o
obj-$(CONFIG_SENSORS_W83627HF) += w83627hf.o

drivers/hwmon/Makefile +1 -0
+obj-$(CONFIG_SENSORS_W83773G)  += w83773g.o
  obj-$(CONFIG_SENSORS_W83792D) += w83792d.o
  obj-$(CONFIG_SENSORS_W83793)  += w83793.o
  obj-$(CONFIG_SENSORS_W83795)  += w83795.o
drivers/hwmon/w83773g.c new file +29 -0
new file mode 100644
index 0000000..16d5fa0
@@ -0,0 +1,276 @@
+/*
+ * Copyright (C) 2017 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ */
+
+/*
+ * Driver for the Nuvoton W83773G SMBus temperature sensor IC.
+ * Supported models: W83773G
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/jiffies.h>
+#include <linux/i2c.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/of_device.h>
+#include <linux/sysfs.h>
+
+/* Addresses to scan */
+static const unsigned short normal_i2c[] = { 0x4c, 0x4d, I2C_CLIENT_END

};

drivers/hwmon/w83773g.c new file +70 -0
+
+enum chips { w83773g };
+
+/* The W83773 registers */
+#define W83773_CONVERSION_RATE_REG_READ 0x04
+#define W83773_CONVERSION_RATE_REG_WRITE 0x0A
+#define W83773_MANUFACTURER_ID_REG 0xFE
+#define W83773_LOCAL_TEMP 0x00
+
+static const u8 W83773_STATUS[2] = { 0x02, 0x17 };
+
+static const u8 W83773_TEMP_LSB[2] = { 0x10, 0x25 };
+static const u8 W83773_TEMP_MSB[2] = { 0x01, 0x24 };
+
+static const u8 W83773_OFFSET_LSB[2] = { 0x12, 0x16 };
+static const u8 W83773_OFFSET_MSB[2] = { 0x11, 0x15 };
+
+/* Manufacturer / Device ID's */
+#define W83773_MANUFACTURER_ID                 0x5c
+
+
+/* this is the number of sensors in the device */
+static const struct i2c_device_id w83773_id[] = {
+       { "w83773g", 3 },
+       { }
+};
+
+MODULE_DEVICE_TABLE(i2c, w83773_id);
+
+static const struct of_device_id w83773_of_match[] = {
+       {
+               .compatible = "nuvoton,w83773g",
+               .data = (void *)3
+       },
+
+       { },
+};
+MODULE_DEVICE_TABLE(of, w83773_of_match);
+
+/*
+ * W83773G has 3 temp sensors:
+ *   Channel 0 is the local sensor
+ *   Channel 1-2 are two remote sensors
+ */
+struct w83773_data {
+       struct i2c_client *client;
+       struct mutex update_lock;
+       u32 temp_config[4];
+       struct hwmon_channel_info temp_info;
+       const struct hwmon_channel_info *info[2];
+       struct hwmon_chip_info chip;
+       bool valid;
+       unsigned long last_updated;
+       int channels;
+       s8 temp_local;
+       s8 status[2];
+       s8 temp_hb[2];
+       s8 temp_lb[2];
+       s8 offset_hb[2];
+       s8 offset_lb[2];
+};
+
+static long temp_of_local(s8 reg)
+{
+       return reg * 1000;
+}
+
+static long temp_of_remote(s8 hb, s8 lb, s8 offset_hb, s8 offset_lb)
+{
+       return (hb + offset_hb) * 1000 + ((u8)(lb + offset_lb) >> 5) *

125;

drivers/hwmon/w83773g.c new file +12 -0
+}
+
+
+static struct w83773_data *w83773_update_device(struct device *dev)
+{
+       struct w83773_data *data = dev_get_drvdata(dev);
+       struct i2c_client *client = data->client;
+       int i;
+
+       mutex_lock(&data->update_lock);
+
+       if (time_after(jiffies, data->last_updated + 2 * HZ) ||

!data->valid) {
+ data->temp_local = i2c_smbus_read_byte_data(client,
W83773_LOCAL_TEMP);

drivers/hwmon/w83773g.c new file +2 -0
+               for (i = 0; i < data->channels - 1; i++) {
+                       data->status[i] = i2c_smbus_read_byte_data(client,

W83773_STATUS[i]);
+ data->temp_hb[i] =
i2c_smbus_read_byte_data(client, W83773_TEMP_MSB[i]);
+ data->temp_lb[i] =
i2c_smbus_read_byte_data(client, W83773_TEMP_LSB[i]);
+ data->offset_hb[i] =
i2c_smbus_read_byte_data(client, W83773_OFFSET_MSB[i]);
+ data->offset_lb[i] =
i2c_smbus_read_byte_data(client, W83773_OFFSET_LSB[i]);

drivers/hwmon/w83773g.c new file +45 -0
+               }
+               data->last_updated = jiffies;
+               data->valid = true;
+       }
+
+       mutex_unlock(&data->update_lock);
+
+       return data;
+}
+
+static int w83773_read(struct device *dev, enum hwmon_sensor_types type,
+                       u32 attr, int channel, long *val)
+{
+       struct w83773_data *w83773 = w83773_update_device(dev);
+
+       switch (attr) {
+       case hwmon_temp_input:
+               if (channel == 0) {
+                       /* channel 0 is the local temp */
+                       *val = temp_of_local(w83773->temp_local);
+               }
+               else {
+                       /* channel 1-2 are the remote temps */
+                       channel--;
+                       *val = temp_of_remote(
+                               w83773->temp_hb[channel],
+                               w83773->temp_lb[channel],
+                               w83773->offset_hb[channel],
+                               w83773->offset_lb[channel]);
+               }
+               return 0;
+       case hwmon_temp_fault:
+               if (channel == 0)
+                       *val = 0;
+               else
+                       /* Check the status register bit 2 for faults */
+                       *val = (w83773->status[channel - 1] & 0x04) >> 2;
+               return 0;
+       default:
+               return -EOPNOTSUPP;
+       }
+
+}
+
+static umode_t w83773_is_visible(const void *data, enum

hwmon_sensor_types type,

drivers/hwmon/w83773g.c new file +18 -0
+                                u32 attr, int channel)
+{
+       switch (attr) {
+       case hwmon_temp_fault:
+               if (channel == 0)
+                       return 0;
+               return S_IRUGO;
+       case hwmon_temp_input:
+               return S_IRUGO;
+       default:
+               return 0;
+       }
+}
+
+static int w83773_init_client(struct i2c_client *client)
+{
+       /* Set the conversion rate to 2 Hz */
+       i2c_smbus_write_byte_data(client,

W83773_CONVERSION_RATE_REG_WRITE, 0x05);

drivers/hwmon/w83773g.c new file +16 -0
+
+       return 0;
+}
+
+static int w83773_detect(struct i2c_client *client,
+                        struct i2c_board_info *info)
+{
+       enum chips kind;
+       struct i2c_adapter *adapter = client->adapter;
+       const char * const names[] = { "W83773G" };
+       u8 reg;
+
+       if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
+               return -ENODEV;
+
+       reg = i2c_smbus_read_byte_data(client,

W83773_MANUFACTURER_ID_REG);

drivers/hwmon/w83773g.c new file +4 -0
+       if (reg != W83773_MANUFACTURER_ID)
+               return -ENODEV;
+
+       reg = i2c_smbus_read_byte_data(client,

W83773_CONVERSION_RATE_REG_READ);

drivers/hwmon/w83773g.c new file +32 -0
+       if (reg & 0xf8)
+               return -ENODEV;
+
+       kind = w83773g;
+
+       strlcpy(info->type, w83773_id[kind].name, I2C_NAME_SIZE);
+       dev_info(&adapter->dev, "Detected Nuvoton %s chip at 0x%02x\n",
+                names[kind], client->addr);
+
+       return 0;
+}
+
+static const struct hwmon_ops w83773_ops = {
+       .is_visible = w83773_is_visible,
+       .read = w83773_read,
+};
+
+static int w83773_probe(struct i2c_client *client,
+                       const struct i2c_device_id *id)
+{
+       struct device *dev = &client->dev;
+       struct device *hwmon_dev;
+       struct w83773_data *data;
+       int i, err;
+
+       data = devm_kzalloc(dev, sizeof(struct w83773_data), GFP_KERNEL);
+       if (!data)
+               return -ENOMEM;
+
+       mutex_init(&data->update_lock);
+       if (client->dev.of_node)
+               data->channels =

(int)of_device_get_match_data(&client->dev);

drivers/hwmon/w83773g.c new file +20 -0
+       else
+               data->channels = id->driver_data;
+       data->client = client;
+
+       err = w83773_init_client(client);
+       if (err)
+               return err;
+
+       for (i = 0; i < data->channels; i++)
+               data->temp_config[i] = HWMON_T_INPUT | HWMON_T_FAULT;
+
+       data->chip.ops = &w83773_ops;
+       data->chip.info = data->info;
+
+       data->info[0] = &data->temp_info;
+
+       data->temp_info.type = hwmon_temp;
+       data->temp_info.config = data->temp_config;
+
+       hwmon_dev = devm_hwmon_device_register_with_info(dev,

client->name,

drivers/hwmon/w83773g.c new file +23 -0
+                                                        data,
+                                                        &data->chip,
+                                                        NULL);
+       return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static struct i2c_driver w83773_driver = {
+       .class = I2C_CLASS_HWMON,
+       .driver = {
+               .name   = "w83773g",
+               .of_match_table = of_match_ptr(w83773_of_match),
+       },
+       .probe = w83773_probe,
+       .id_table = w83773_id,
+       .detect = w83773_detect,
+       .address_list = normal_i2c,
+};
+
+module_i2c_driver(w83773_driver);
+
+MODULE_AUTHOR("Nickolaus Gruendler <[email protected]>");
+MODULE_DESCRIPTION("W83773G temperature sensor driver");
+MODULE_LICENSE("GPL");
maintainer
#4 lore raw

Re: [PATCH 1/2] drivers: hwmon: Add W83773G driver

In reply to Lei YU
Quoted 3 lines · Lei YU

Nuvoton W83773G is a hardware monitor IC providing one local temperature and two remote temperature sensors.

I agree that a separate driver for this chip make sense. Some work to do, though. This is only an initial review; later versions will probably trigger additional feedback.

Please provide a register dump for the chip so I can write a unit test.

Quoted 13 lines
Signed-off-by Lei YU <[email protected]>
---
  drivers/hwmon/Kconfig   |  11 ++
  drivers/hwmon/Makefile  |   1 +
  drivers/hwmon/w83773g.c | 276 ++++++++++++++++++++++++++++++++++++++++++++++++
  3 files changed, 288 insertions(+)

create mode 100644 drivers/hwmon/w83773g.c

drivers/hwmon/Kconfig +0 -0
index d654314..d148b70 100644

drivers/hwmon/Kconfig config SENSORS_VT8231 · line 1716

Quoted 7 lines
drivers/hwmon/Kconfig +4 -0
@@ -1710,6 +1710,17 @@ config SENSORS_VT8231
  	  This driver can also be built as a module.  If so, the module
  	  will be called vt8231.
 
+config SENSORS_W83773G
+	tristate "Nuvoton W83773G"
+	depends on I2C
+	select HWMON_VID

Why ?

Quoted 28 lines
drivers/hwmon/Kconfig +7 -0
+	help
+	  If you say yes here you get support for the Nuvoton W83773G hardware
+	  monitoring chip.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called w83773g.
+
  config SENSORS_W83781D
  	tristate "Winbond W83781D, W83782D, W83783S, Asus AS99127F"
  	depends on I2C
drivers/hwmon/Makefile +1 -0
index c84d978..0649ad8 100644
@@ -13,6 +13,7 @@ obj-$(CONFIG_SENSORS_ATK0110)	+= asus_atk0110.o
  # asb100, then w83781d go first, as they can override other drivers' addresses.
  obj-$(CONFIG_SENSORS_ASB100)	+= asb100.o
  obj-$(CONFIG_SENSORS_W83627HF)	+= w83627hf.o
+obj-$(CONFIG_SENSORS_W83773G)	+= w83773g.o
  obj-$(CONFIG_SENSORS_W83792D)	+= w83792d.o
  obj-$(CONFIG_SENSORS_W83793)	+= w83793.o
  obj-$(CONFIG_SENSORS_W83795)	+= w83795.o
drivers/hwmon/w83773g.c new file +0 -0
new file mode 100644
index 0000000..16d5fa0

drivers/hwmon/w83773g.c line 3

Quoted 3 lines
drivers/hwmon/w83773g.c new file +3 -0
@@ -0,0 +1,276 @@
+/*
+ * Copyright (C) 2017 IBM Corp.
+ *

This and the module author note below suggests that the driver was written by someone else, yet there is no credit. Do you have permission from IBM and from the author to publish this driver ?

drivers/hwmon/w83773g.c line 8

Quoted 5 lines
drivers/hwmon/w83773g.c new file +5 -0
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *

extra comment line

drivers/hwmon/w83773g.c line 13

Quoted 5 lines
drivers/hwmon/w83773g.c new file +5 -0
+ */
+
+/*
+ * Driver for the Nuvoton W83773G SMBus temperature sensor IC.
+ * Supported models: W83773G

Double comment.

drivers/hwmon/w83773g.c line 26

Quoted 13 lines
drivers/hwmon/w83773g.c new file +13 -0
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/jiffies.h>
+#include <linux/i2c.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/of_device.h>
+#include <linux/sysfs.h>

sysfs includes are unnecessary.

drivers/hwmon/w83773g.c line 32

Quoted 6 lines
drivers/hwmon/w83773g.c new file +6 -0
+
+/* Addresses to scan */
+static const unsigned short normal_i2c[] = { 0x4c, 0x4d, I2C_CLIENT_END };
+
+enum chips { w83773g };
+

The driver only supports one chip. This is unnecessary.

drivers/hwmon/w83773g.c line 37

Quoted 5 lines
drivers/hwmon/w83773g.c new file +5 -0
+/* The W83773 registers */
+#define W83773_CONVERSION_RATE_REG_READ 0x04
+#define W83773_CONVERSION_RATE_REG_WRITE 0x0A
+#define W83773_MANUFACTURER_ID_REG 0xFE
+#define W83773_LOCAL_TEMP 0x00

Please use tabs.

drivers/hwmon/w83773g.c line 50

Quoted 13 lines
drivers/hwmon/w83773g.c new file +13 -0
+
+static const u8 W83773_STATUS[2] = { 0x02, 0x17 };
+
+static const u8 W83773_TEMP_LSB[2] = { 0x10, 0x25 };
+static const u8 W83773_TEMP_MSB[2] = { 0x01, 0x24 };
+
+static const u8 W83773_OFFSET_LSB[2] = { 0x12, 0x16 };
+static const u8 W83773_OFFSET_MSB[2] = { 0x11, 0x15 };
+
+/* Manufacturer / Device ID's */
+#define W83773_MANUFACTURER_ID			0x5c
+
+

extra empty line

drivers/hwmon/w83773g.c line 53

Quoted 3 lines
drivers/hwmon/w83773g.c new file +3 -0
+/* this is the number of sensors in the device */
+static const struct i2c_device_id w83773_id[] = {
+	{ "w83773g", 3 },

The 3 is pointless. There are always three channels.

drivers/hwmon/w83773g.c line 62

Quoted 9 lines
drivers/hwmon/w83773g.c new file +9 -0
+	{ }
+};
+
+MODULE_DEVICE_TABLE(i2c, w83773_id);
+
+static const struct of_device_id w83773_of_match[] = {
+	{
+		.compatible = "nuvoton,w83773g",
+		.data = (void *)3

Same here.

Quoted 8 lines
drivers/hwmon/w83773g.c new file +8 -0
+	},
+
+	{ },
+};
+MODULE_DEVICE_TABLE(of, w83773_of_match);
+
+/*
+ * W83773G has 3 temp sensors:

drivers/hwmon/w83773g.c line 80

Quoted 10 lines
drivers/hwmon/w83773g.c new file +10 -0
+ *   Channel 0 is the local sensor
+ *   Channel 1-2 are two remote sensors
+ */
+struct w83773_data {
+	struct i2c_client *client;
+	struct mutex update_lock;
+	u32 temp_config[4];
+	struct hwmon_channel_info temp_info;
+	const struct hwmon_channel_info *info[2];
+	struct hwmon_chip_info chip;

AFAICS the channel and chip information is all static. I don't see a reason for keeping it as variables.

drivers/hwmon/w83773g.c line 89

Quoted 9 lines
drivers/hwmon/w83773g.c new file +9 -0
+	bool valid;
+	unsigned long last_updated;
+	int channels;
+	s8 temp_local;
+	s8 status[2];
+	s8 temp_hb[2];
+	s8 temp_lb[2];
+	s8 offset_hb[2];
+	s8 offset_lb[2];

Please drop register caching in the driver. Use regmap to cache non-volatile registers.

drivers/hwmon/w83773g.c line 99

Quoted 10 lines
drivers/hwmon/w83773g.c new file +10 -0
+};
+
+static long temp_of_local(s8 reg)
+{
+	return reg * 1000;
+}
+
+static long temp_of_remote(s8 hb, s8 lb, s8 offset_hb, s8 offset_lb)
+{
+	return (hb + offset_hb) * 1000 + ((u8)(lb + offset_lb) >> 5) * 125;

This is wrong. The chip applies offsets internally, and reports adjusted temperatures in its temperature registers.

drivers/hwmon/w83773g.c line 108

Quoted 9 lines
drivers/hwmon/w83773g.c new file +9 -0
+}
+
+
+static struct w83773_data *w83773_update_device(struct device *dev)
+{
+	struct w83773_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client = data->client;
+	int i;
+

As mentioned above, please drop the local cache and use regmap to cache non-volatile registers.

Quoted 22 lines
drivers/hwmon/w83773g.c new file +22 -0
+	mutex_lock(&data->update_lock);
+
+	if (time_after(jiffies, data->last_updated + 2 * HZ) || !data->valid) {
+		data->temp_local = i2c_smbus_read_byte_data(client, W83773_LOCAL_TEMP);
+		for (i = 0; i < data->channels - 1; i++) {
+			data->status[i] = i2c_smbus_read_byte_data(client, W83773_STATUS[i]);
+			data->temp_hb[i] = i2c_smbus_read_byte_data(client, W83773_TEMP_MSB[i]);
+			data->temp_lb[i] = i2c_smbus_read_byte_data(client, W83773_TEMP_LSB[i]);
+			data->offset_hb[i] = i2c_smbus_read_byte_data(client, W83773_OFFSET_MSB[i]);
+			data->offset_lb[i] = i2c_smbus_read_byte_data(client, W83773_OFFSET_LSB[i]);
+		}
+		data->last_updated = jiffies;
+		data->valid = true;
+	}
+
+	mutex_unlock(&data->update_lock);
+
+	return data;
+}
+
+static int w83773_read(struct device *dev, enum hwmon_sensor_types type,
+			u32 attr, int channel, long *val)

drivers/hwmon/w83773g.c line 140

Quoted 10 lines
drivers/hwmon/w83773g.c new file +10 -0
+{
+	struct w83773_data *w83773 = w83773_update_device(dev);
+
+	switch (attr) {
+	case hwmon_temp_input:
+		if (channel == 0) {
+			/* channel 0 is the local temp */
+			*val = temp_of_local(w83773->temp_local);
+		}
+		else {

Please run checkpatch --strict on your patch and follow kernel coding style.

drivers/hwmon/w83773g.c line 149

Quoted 9 lines
drivers/hwmon/w83773g.c new file +9 -0
+			/* channel 1-2 are the remote temps */
+			channel--;
+			*val = temp_of_remote(
+				w83773->temp_hb[channel],
+				w83773->temp_lb[channel],
+				w83773->offset_hb[channel],
+				w83773->offset_lb[channel]);
+		}
+		return 0;

Missing:
hwmon_temp_offset hwmon_chip_update_interval (for conversion rate)

Both attributes should be writable.

drivers/hwmon/w83773g.c line 152

Quoted 3 lines
drivers/hwmon/w83773g.c new file +3 -0
+	case hwmon_temp_fault:
+		if (channel == 0)
+			*val = 0;

Should never get here. Channel 0 does not report faults, and there should be no fault attribute for that channel.

Quoted 10 lines
drivers/hwmon/w83773g.c new file +10 -0
+		else
+			/* Check the status register bit 2 for faults */
+			*val = (w83773->status[channel - 1] & 0x04) >> 2;
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+}
+

drivers/hwmon/w83773g.c line 172

Quoted 10 lines
drivers/hwmon/w83773g.c new file +10 -0
+static umode_t w83773_is_visible(const void *data, enum hwmon_sensor_types type,
+				 u32 attr, int channel)
+{
+	switch (attr) {
+	case hwmon_temp_fault:
+		if (channel == 0)
+			return 0;
+		return S_IRUGO;
+	case hwmon_temp_input:
+		return S_IRUGO;

Symbolic permissions ran out of favor. checkpatch will tell you.

Quoted 27 lines
drivers/hwmon/w83773g.c new file +27 -0
+	default:
+		return 0;
+	}
+}
+
+static int w83773_init_client(struct i2c_client *client)
+{
+	/* Set the conversion rate to 2 Hz */
+	i2c_smbus_write_byte_data(client, W83773_CONVERSION_RATE_REG_WRITE, 0x05);
+
+	return 0;
+}
+
+static int w83773_detect(struct i2c_client *client,
+			 struct i2c_board_info *info)
+{
+	enum chips kind;
+	struct i2c_adapter *adapter = client->adapter;
+	const char * const names[] = { "W83773G" };
+	u8 reg;
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
+		return -ENODEV;
+
+	reg = i2c_smbus_read_byte_data(client, W83773_MANUFACTURER_ID_REG);
+	if (reg != W83773_MANUFACTURER_ID)
+		return -ENODEV;
Quoted 1 line

This is insufficient for automatic detection. Also check RDR and at least

the configuration register.

There does not seem to be a reliable means to distinguish the chip from W83775G. Given that, I wonder if a detect function even makes sense; I seriously doubt it. Does your application require it ? If not I would suggest to drop the detect function.

drivers/hwmon/w83773g.c line 204

Quoted 4 lines
drivers/hwmon/w83773g.c new file +4 -0
+	reg = i2c_smbus_read_byte_data(client, W83773_CONVERSION_RATE_REG_READ);
+	if (reg & 0xf8)
+		return -ENODEV;
+

Bit 3 (8) is valid.

drivers/hwmon/w83773g.c line 209

Quoted 5 lines
drivers/hwmon/w83773g.c new file +5 -0
+	kind = w83773g;
+
+	strlcpy(info->type, w83773_id[kind].name, I2C_NAME_SIZE);
+	dev_info(&adapter->dev, "Detected Nuvoton %s chip at 0x%02x\n",
+		 names[kind], client->addr);

All this kind stuff is unnecessary.

Quoted 16 lines
drivers/hwmon/w83773g.c new file +16 -0
+
+	return 0;
+}
+
+static const struct hwmon_ops w83773_ops = {
+	.is_visible = w83773_is_visible,
+	.read = w83773_read,
+};
+
+static int w83773_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;
+	struct device *hwmon_dev;
+	struct w83773_data *data;
+	int i, err;

drivers/hwmon/w83773g.c line 235

Quoted 10 lines
drivers/hwmon/w83773g.c new file +10 -0
+
+	data = devm_kzalloc(dev, sizeof(struct w83773_data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	mutex_init(&data->update_lock);
+	if (client->dev.of_node)
+		data->channels = (int)of_device_get_match_data(&client->dev);
+	else
+		data->channels = id->driver_data;

channels is fixed. No need to keep it as variable; a define is just as good.

Quoted 29 lines
drivers/hwmon/w83773g.c new file +29 -0
+	data->client = client;
+
+	err = w83773_init_client(client);
+	if (err)
+		return err;
+
+	for (i = 0; i < data->channels; i++)
+		data->temp_config[i] = HWMON_T_INPUT | HWMON_T_FAULT;
+
+	data->chip.ops = &w83773_ops;
+	data->chip.info = data->info;
+
+	data->info[0] = &data->temp_info;
+
+	data->temp_info.type = hwmon_temp;
+	data->temp_info.config = data->temp_config;
+
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
+							 data,
+							 &data->chip,
+							 NULL);
+	return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static struct i2c_driver w83773_driver = {
+	.class = I2C_CLASS_HWMON,
+	.driver = {
+		.name	= "w83773g",
+		.of_match_table = of_match_ptr(w83773_of_match),

drivers/hwmon/w83773g.c line 274

Quoted 10 lines
drivers/hwmon/w83773g.c new file +10 -0
+	},
+	.probe = w83773_probe,
+	.id_table = w83773_id,
+	.detect = w83773_detect,
+	.address_list = normal_i2c,
+};
+
+module_i2c_driver(w83773_driver);
+
+MODULE_AUTHOR("Nickolaus Gruendler <[email protected]>");

Does Nickolaus know about the use of his name and e-mail address and give permission ?

Quoted 3 lines
drivers/hwmon/w83773g.c new file +2 -0
+MODULE_DESCRIPTION("W83773G temperature sensor driver");
+MODULE_LICENSE("GPL");
author
#5 lore raw

Re: [PATCH 1/2] drivers: hwmon: Add W83773G driver

Hi Guenter,

Thanks a lot for the detailed review.

I will try to fix all the comments and send v2 patch.

Quoted 11 lines · Guenter Roeck
Quoted 4 lines · Lei YU

Nuvoton W83773G is a hardware monitor IC providing one local temperature and two remote temperature sensors.

I agree that a separate driver for this chip make sense. Some work to do, though. This is only an initial review; later versions will probably trigger additional feedback.

Please provide a register dump for the chip so I can write a unit test.

The dump of registers:

```
# i2cdump -y 12 0x4c
No size specified (using byte-data access)
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f    0123456789abcdef
00: 1b 17 00 01 05 46 d8 46 d8 ff ff ff ff ff ff ff    ??.??F?F?.......
10: 40 00 00 00 00 00 00 00 00 6e 6e 46 d8 00 00 ff    @........nnF?...
20: 55 0a ff ff 18 00 ff ff ff ff ff ff ff ff ff ff    U?..?...........
30: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
40: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
50: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
60: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
70: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
90: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
a0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
b0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff 00    ................
c0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
d0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff    ................
e0: 00 00 00 84 84 84 00 00 55 00 00 ff ff ff ff ff    ...???..U.......
f0: 08 00 00 00 84 00 00 17 80 fc 17 83 18 ff 5c 11    ?...?..??????.\?
```
Quoted 56 lines
Quoted 15 lines
Signed-off-by Lei YU <[email protected]>
---
  drivers/hwmon/Kconfig   |  11 ++
  drivers/hwmon/Makefile  |   1 +
  drivers/hwmon/w83773g.c | 276
++++++++++++++++++++++++++++++++++++++++++++++++
  3 files changed, 288 insertions(+)

create mode 100644 drivers/hwmon/w83773g.c

drivers/hwmon/Kconfig +0 -0
index d654314..d148b70 100644

drivers/hwmon/Kconfig config SENSORS_VT8231 · line 1715

Quoted 6 lines
drivers/hwmon/Kconfig +3 -0
@@ -1710,6 +1710,17 @@ config SENSORS_VT8231
          This driver can also be built as a module.  If so, the module
          will be called vt8231.
  +config SENSORS_W83773G
+       tristate "Nuvoton W83773G"
+       depends on I2C
+       select HWMON_VID

Why ?

Quoted 30 lines
drivers/hwmon/Kconfig +2 -0
+       help
+         If you say yes here you get support for the Nuvoton W83773G

hardware

drivers/hwmon/Kconfig +5 -0
+         monitoring chip.
+
+         This driver can also be built as a module.  If so, the module
+         will be called w83773g.
+
  config SENSORS_W83781D
        tristate "Winbond W83781D, W83782D, W83783S, Asus AS99127F"
        depends on I2C
drivers/hwmon/Makefile +0 -0
index c84d978..0649ad8 100644
@@ -13,6 +13,7 @@ obj-$(CONFIG_SENSORS_ATK0110) += asus_atk0110.o
  # asb100, then w83781d go first, as they can override other drivers'

addresses.
obj-$(CONFIG_SENSORS_ASB100) += asb100.o
obj-$(CONFIG_SENSORS_W83627HF) += w83627hf.o

drivers/hwmon/Makefile +1 -0
+obj-$(CONFIG_SENSORS_W83773G)  += w83773g.o
  obj-$(CONFIG_SENSORS_W83792D) += w83792d.o
  obj-$(CONFIG_SENSORS_W83793)  += w83793.o
  obj-$(CONFIG_SENSORS_W83795)  += w83795.o
drivers/hwmon/w83773g.c new file +0 -0
new file mode 100644
index 0000000..16d5fa0
@@ -0,0 +1,276 @@
Quoted 8 lines

drivers/hwmon/w83773g.c line 3

Quoted 3 lines
drivers/hwmon/w83773g.c new file +3 -0
+/*
+ * Copyright (C) 2017 IBM Corp.
+ *

This and the module author note below suggests that the driver was written by someone else, yet there is no credit. Do you have permission from IBM and from the author to publish this driver ?

Yes. I work for IBM. And this driver is originally written by Nickolaus Gruendler
based on tmp421.c. He primary works on HW part and has not too much knowledge about SW.
And I helped tweak the code to make it work. So I think the author should be him.

Please be noted that I am also new comer for kernel development and really appreciate your careful review and comments.

Quoted 391 lines

drivers/hwmon/w83773g.c line 8

Quoted 6 lines
drivers/hwmon/w83773g.c new file +5 -0
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *

extra comment line

drivers/hwmon/w83773g.c line 13

Quoted 5 lines
drivers/hwmon/w83773g.c new file +5 -0
+ */
+
+/*
+ * Driver for the Nuvoton W83773G SMBus temperature sensor IC.
+ * Supported models: W83773G

Double comment.

drivers/hwmon/w83773g.c line 26

Quoted 13 lines
drivers/hwmon/w83773g.c new file +13 -0
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/jiffies.h>
+#include <linux/i2c.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/of_device.h>
+#include <linux/sysfs.h>

sysfs includes are unnecessary.

drivers/hwmon/w83773g.c line 32

Quoted 7 lines
drivers/hwmon/w83773g.c new file +3 -0
+
+/* Addresses to scan */
+static const unsigned short normal_i2c[] = { 0x4c, 0x4d, I2C_CLIENT_END

};

drivers/hwmon/w83773g.c new file +3 -0
+
+enum chips { w83773g };
+

The driver only supports one chip. This is unnecessary.

drivers/hwmon/w83773g.c line 37

Quoted 5 lines
drivers/hwmon/w83773g.c new file +5 -0
+/* The W83773 registers */
+#define W83773_CONVERSION_RATE_REG_READ 0x04
+#define W83773_CONVERSION_RATE_REG_WRITE 0x0A
+#define W83773_MANUFACTURER_ID_REG 0xFE
+#define W83773_LOCAL_TEMP 0x00

Please use tabs.

drivers/hwmon/w83773g.c line 50

Quoted 13 lines
drivers/hwmon/w83773g.c new file +13 -0
+
+static const u8 W83773_STATUS[2] = { 0x02, 0x17 };
+
+static const u8 W83773_TEMP_LSB[2] = { 0x10, 0x25 };
+static const u8 W83773_TEMP_MSB[2] = { 0x01, 0x24 };
+
+static const u8 W83773_OFFSET_LSB[2] = { 0x12, 0x16 };
+static const u8 W83773_OFFSET_MSB[2] = { 0x11, 0x15 };
+
+/* Manufacturer / Device ID's */
+#define W83773_MANUFACTURER_ID                 0x5c
+
+

extra empty line

drivers/hwmon/w83773g.c line 53

Quoted 3 lines
drivers/hwmon/w83773g.c new file +3 -0
+/* this is the number of sensors in the device */
+static const struct i2c_device_id w83773_id[] = {
+       { "w83773g", 3 },

The 3 is pointless. There are always three channels.

drivers/hwmon/w83773g.c line 62

Quoted 9 lines
drivers/hwmon/w83773g.c new file +9 -0
+       { }
+};
+
+MODULE_DEVICE_TABLE(i2c, w83773_id);
+
+static const struct of_device_id w83773_of_match[] = {
+       {
+               .compatible = "nuvoton,w83773g",
+               .data = (void *)3

Same here.

Quoted 8 lines
drivers/hwmon/w83773g.c new file +8 -0
+       },
+
+       { },
+};
+MODULE_DEVICE_TABLE(of, w83773_of_match);
+
+/*
+ * W83773G has 3 temp sensors:

drivers/hwmon/w83773g.c line 80

Quoted 10 lines
drivers/hwmon/w83773g.c new file +10 -0
+ *   Channel 0 is the local sensor
+ *   Channel 1-2 are two remote sensors
+ */
+struct w83773_data {
+       struct i2c_client *client;
+       struct mutex update_lock;
+       u32 temp_config[4];
+       struct hwmon_channel_info temp_info;
+       const struct hwmon_channel_info *info[2];
+       struct hwmon_chip_info chip;

AFAICS the channel and chip information is all static. I don't see a reason for keeping it as variables.

drivers/hwmon/w83773g.c line 89

Quoted 9 lines
drivers/hwmon/w83773g.c new file +9 -0
+       bool valid;
+       unsigned long last_updated;
+       int channels;
+       s8 temp_local;
+       s8 status[2];
+       s8 temp_hb[2];
+       s8 temp_lb[2];
+       s8 offset_hb[2];
+       s8 offset_lb[2];

Please drop register caching in the driver. Use regmap to cache non-volatile registers.

Quoted 11 lines
drivers/hwmon/w83773g.c new file +10 -0
+};
+
+static long temp_of_local(s8 reg)
+{
+       return reg * 1000;
+}
+
+static long temp_of_remote(s8 hb, s8 lb, s8 offset_hb, s8 offset_lb)
+{
+       return (hb + offset_hb) * 1000 + ((u8)(lb + offset_lb) >> 5) *

125;

This is wrong. The chip applies offsets internally, and reports adjusted temperatures
in its temperature registers.

drivers/hwmon/w83773g.c line 108

Quoted 9 lines
drivers/hwmon/w83773g.c new file +9 -0
+}
+
+
+static struct w83773_data *w83773_update_device(struct device *dev)
+{
+       struct w83773_data *data = dev_get_drvdata(dev);
+       struct i2c_client *client = data->client;
+       int i;
+

As mentioned above, please drop the local cache and use regmap to cache non-volatile registers.

Quoted 29 lines
drivers/hwmon/w83773g.c new file +3 -0
+       mutex_lock(&data->update_lock);
+
+       if (time_after(jiffies, data->last_updated + 2 * HZ) ||

!data->valid) {
+ data->temp_local = i2c_smbus_read_byte_data(client,
W83773_LOCAL_TEMP);

drivers/hwmon/w83773g.c new file +2 -0
+               for (i = 0; i < data->channels - 1; i++) {
+                       data->status[i] = i2c_smbus_read_byte_data(client,

W83773_STATUS[i]);
+ data->temp_hb[i] =
i2c_smbus_read_byte_data(client, W83773_TEMP_MSB[i]);
+ data->temp_lb[i] =
i2c_smbus_read_byte_data(client, W83773_TEMP_LSB[i]);
+ data->offset_hb[i] =
i2c_smbus_read_byte_data(client, W83773_OFFSET_MSB[i]);
+ data->offset_lb[i] =
i2c_smbus_read_byte_data(client, W83773_OFFSET_LSB[i]);

drivers/hwmon/w83773g.c new file +12 -0
+               }
+               data->last_updated = jiffies;
+               data->valid = true;
+       }
+
+       mutex_unlock(&data->update_lock);
+
+       return data;
+}
+
+static int w83773_read(struct device *dev, enum hwmon_sensor_types type,
+                       u32 attr, int channel, long *val)

drivers/hwmon/w83773g.c line 140

Quoted 10 lines
drivers/hwmon/w83773g.c new file +10 -0
+{
+       struct w83773_data *w83773 = w83773_update_device(dev);
+
+       switch (attr) {
+       case hwmon_temp_input:
+               if (channel == 0) {
+                       /* channel 0 is the local temp */
+                       *val = temp_of_local(w83773->temp_local);
+               }
+               else {

Please run checkpatch --strict on your patch and follow kernel coding style.

drivers/hwmon/w83773g.c line 149

Quoted 9 lines
drivers/hwmon/w83773g.c new file +9 -0
+                       /* channel 1-2 are the remote temps */
+                       channel--;
+                       *val = temp_of_remote(
+                               w83773->temp_hb[channel],
+                               w83773->temp_lb[channel],
+                               w83773->offset_hb[channel],
+                               w83773->offset_lb[channel]);
+               }
+               return 0;

Missing:
hwmon_temp_offset hwmon_chip_update_interval (for conversion rate)

Both attributes should be writable.

drivers/hwmon/w83773g.c line 152

Quoted 3 lines
drivers/hwmon/w83773g.c new file +3 -0
+       case hwmon_temp_fault:
+               if (channel == 0)
+                       *val = 0;

Should never get here. Channel 0 does not report faults, and there should be no fault attribute for that channel.

Quoted 12 lines
drivers/hwmon/w83773g.c new file +11 -0
+               else
+                       /* Check the status register bit 2 for faults */
+                       *val = (w83773->status[channel - 1] & 0x04) >> 2;
+               return 0;
+       default:
+               return -EOPNOTSUPP;
+       }
+
+}
+
+static umode_t w83773_is_visible(const void *data, enum

hwmon_sensor_types type,

drivers/hwmon/w83773g.c line 172

Quoted 9 lines
drivers/hwmon/w83773g.c new file +9 -0
+                                u32 attr, int channel)
+{
+       switch (attr) {
+       case hwmon_temp_fault:
+               if (channel == 0)
+                       return 0;
+               return S_IRUGO;
+       case hwmon_temp_input:
+               return S_IRUGO;

Symbolic permissions ran out of favor. checkpatch will tell you.

Quoted 29 lines
drivers/hwmon/w83773g.c new file +9 -0
+       default:
+               return 0;
+       }
+}
+
+static int w83773_init_client(struct i2c_client *client)
+{
+       /* Set the conversion rate to 2 Hz */
+       i2c_smbus_write_byte_data(client,

W83773_CONVERSION_RATE_REG_WRITE, 0x05);

drivers/hwmon/w83773g.c new file +16 -0
+
+       return 0;
+}
+
+static int w83773_detect(struct i2c_client *client,
+                        struct i2c_board_info *info)
+{
+       enum chips kind;
+       struct i2c_adapter *adapter = client->adapter;
+       const char * const names[] = { "W83773G" };
+       u8 reg;
+
+       if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA))
+               return -ENODEV;
+
+       reg = i2c_smbus_read_byte_data(client,

W83773_MANUFACTURER_ID_REG);

drivers/hwmon/w83773g.c new file +2 -0
+       if (reg != W83773_MANUFACTURER_ID)
+               return -ENODEV;
Quoted 1 line

This is insufficient for automatic detection. Also check RDR and at least

the configuration register.

There does not seem to be a reliable means to distinguish the chip from W83775G. Given that, I wonder if a detect function even makes sense; I seriously doubt it. Does your application require it ? If not I would suggest to drop the detect function.

drivers/hwmon/w83773g.c line 204

Quoted 5 lines

+ reg = i2c_smbus_read_byte_data(client,
W83773_CONVERSION_RATE_REG_READ);

drivers/hwmon/w83773g.c new file +3 -0
+       if (reg & 0xf8)
+               return -ENODEV;
+

Bit 3 (8) is valid.

drivers/hwmon/w83773g.c line 209

Quoted 5 lines
drivers/hwmon/w83773g.c new file +5 -0
+       kind = w83773g;
+
+       strlcpy(info->type, w83773_id[kind].name, I2C_NAME_SIZE);
+       dev_info(&adapter->dev, "Detected Nuvoton %s chip at 0x%02x\n",
+                names[kind], client->addr);

All this kind stuff is unnecessary.

Quoted 25 lines
drivers/hwmon/w83773g.c new file +24 -0
+
+       return 0;
+}
+
+static const struct hwmon_ops w83773_ops = {
+       .is_visible = w83773_is_visible,
+       .read = w83773_read,
+};
+
+static int w83773_probe(struct i2c_client *client,
+                       const struct i2c_device_id *id)
+{
+       struct device *dev = &client->dev;
+       struct device *hwmon_dev;
+       struct w83773_data *data;
+       int i, err;
+
+       data = devm_kzalloc(dev, sizeof(struct w83773_data), GFP_KERNEL);
+       if (!data)
+               return -ENOMEM;
+
+       mutex_init(&data->update_lock);
+       if (client->dev.of_node)
+               data->channels =

(int)of_device_get_match_data(&client->dev);

drivers/hwmon/w83773g.c line 235

Quoted 2 lines
drivers/hwmon/w83773g.c new file +2 -0
+       else
+               data->channels = id->driver_data;

channels is fixed. No need to keep it as variable; a define is just as good.

Quoted 30 lines
drivers/hwmon/w83773g.c new file +18 -0
+       data->client = client;
+
+       err = w83773_init_client(client);
+       if (err)
+               return err;
+
+       for (i = 0; i < data->channels; i++)
+               data->temp_config[i] = HWMON_T_INPUT | HWMON_T_FAULT;
+
+       data->chip.ops = &w83773_ops;
+       data->chip.info = data->info;
+
+       data->info[0] = &data->temp_info;
+
+       data->temp_info.type = hwmon_temp;
+       data->temp_info.config = data->temp_config;
+
+       hwmon_dev = devm_hwmon_device_register_with_info(dev,

client->name,

drivers/hwmon/w83773g.c new file +11 -0
+                                                        data,
+                                                        &data->chip,
+                                                        NULL);
+       return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static struct i2c_driver w83773_driver = {
+       .class = I2C_CLASS_HWMON,
+       .driver = {
+               .name   = "w83773g",
+               .of_match_table = of_match_ptr(w83773_of_match),

drivers/hwmon/w83773g.c line 274

Quoted 10 lines
drivers/hwmon/w83773g.c new file +10 -0
+       },
+       .probe = w83773_probe,
+       .id_table = w83773_id,
+       .detect = w83773_detect,
+       .address_list = normal_i2c,
+};
+
+module_i2c_driver(w83773_driver);
+
+MODULE_AUTHOR("Nickolaus Gruendler <[email protected]>");

Does Nickolaus know about the use of his name and e-mail address and give permission ?

Quoted 3 lines
drivers/hwmon/w83773g.c new file +2 -0
+MODULE_DESCRIPTION("W83773G temperature sensor driver");
+MODULE_LICENSE("GPL");
build bot
#6 lore raw

Re: [PATCH 1/2] drivers: hwmon: Add W83773G driver

In reply to Lei YU
Thank you for the patch! Perhaps something to improve:

Hi Lei,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on hwmon/hwmon-next]
[also build test WARNING on v4.14-rc7 next-20171103]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Lei-YU/Add-W83773G-hwmon-sensor-driver-and-doc/20171105-015016
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 6.2.0

reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=ia64

All warnings (new ones prefixed by >>):

drivers//hwmon/w83773g.c: In function 'w83773_probe':

>> drivers//hwmon/w83773g.c:233:20: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
      data->channels = (int)of_device_get_match_data(&client->dev);
                       ^

vim +233 drivers//hwmon/w83773g.c

218
219 static int w83773_probe(struct i2c_client *client,
220 const struct i2c_device_id *id)
221 {
222 struct device *dev = &client->dev;
223 struct device *hwmon_dev;
224 struct w83773_data *data;
225 int i, err;
226
227 data = devm_kzalloc(dev, sizeof(struct w83773_data), GFP_KERNEL);
228 if (!data)
229 return -ENOMEM;
230
231 mutex_init(&data->update_lock);
232 if (client->dev.of_node)

 > 233			data->channels = (int)of_device_get_match_data(&client->dev);

234 else
235 data->channels = id->driver_data;
236 data->client = client;
237
238 err = w83773_init_client(client);
239 if (err)
240 return err;
241
242 for (i = 0; i < data->channels; i++)
243 data->temp_config[i] = HWMON_T_INPUT | HWMON_T_FAULT;
244
245 data->chip.ops = &w83773_ops;
246 data->chip.info = data->info;
247
248 data->info[0] = &data->temp_info;
249
250 data->temp_info.type = hwmon_temp;
251 data->temp_info.config = data->temp_config;
252
253 hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name,
254 data,
255 &data->chip,
256 NULL);
257 return PTR_ERR_OR_ZERO(hwmon_dev);
258 }
259

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation