A driver for the hardware monitoring chip in the Zyxel NSA320
Subsystems (2)
PATCH v2 1/1 A driver for the hardware monitoring chip in the Zyxel NSA320
This patch series adds a driver to support the hardware monitoring MCU that is present in the Zyxel NSA320. It is believed that this is identical to the one used in the NSA325 and some variants of the NSA310.
Documentation/devicetree/bindings/hwmon/nsa320-mcu.txt | 21 ++++++++ arch/arm/boot/dts/kirkwood-nsa320.dts | 13 ++++- drivers/hwmon/Kconfig | 15 ++++++ drivers/hwmon/Makefile | 1 + drivers/hwmon/nsa320-hwmon.c | 215 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 263 insertions(+), 2 deletions(-)
Changes since v1
Update to reflect review comments from Guenter Roeck, mainly changing it to be nsa320, not nsa3xx but also making it return an errno on failure and various other tidy ups.
I'm not sure what protocol is with regard to adding tags to a patch when updating it, in v1
patch 1 received
patch 2 was reviewed by Guenter Roeck but does the reviewed by tag only get added if he agrees with the changes?
Sebastian Hesselbarth reviewed an early draft before it was posted publicly.
PATCH v2 1/3 hwmon: Define binding for the nsa320-hwmon driver
Define a binding for the hardware monitoring chip present in the Zyxel NSA-320 and some of the other Zyxel NAS devices.
--- .../devicetree/bindings/hwmon/nsa320-mcu.txt | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 Documentation/devicetree/bindings/hwmon/nsa320-mcu.txt
Documentation/devicetree/bindings/hwmon/nsa320-mcu.txt new file +21 -0
@@ -0,0 +1,21 @@ +Bindings for the fan / temperature monitor microcontroller used on +the Zyxel NSA 320 and several subsequent models. + +Required properties: +- compatible : "zyxel,nsa320-mcu" +- data-gpios : The GPIO pin connected to the data line on the MCU +- clk-gpios : The GPIO pin connected to the clock line on the MCU +- act-gpios : The GPIO pin connected to the active line on the MCU + +Example: + + hwmon { + compatible = "zyxel,nsa320-mcu"; + pinctrl-0 = <&pmx_mcu_data &pmx_mcu_clk &pmx_mcu_act>; + pinctrl-names = "default"; + + data-gpios = <&gpio0 14 GPIO_ACTIVE_HIGH>; + clk-gpios = <&gpio0 16 GPIO_ACTIVE_HIGH>; + act-gpios = <&gpio0 17 GPIO_ACTIVE_LOW>; + }; +
Signature · 1 line
2.1.4
Guenter Roeck
maintainerPATCH v2 1/3 hwmon: Define binding for the nsa320-hwmon driver
Quoted 4 lines · Adam Baker
Define a binding for the hardware monitoring chip present in the Zyxel NSA-320 and some of the other Zyxel NAS devices.
Signed-off-by Adam Baker <[email protected]>
Rob sent you an Ack for this patch. Since you did not change the patch since v1, I would expect to see it here (instead of having to look it up).
Thanks,
Guenter
Quoted 33 lines
--- .../devicetree/bindings/hwmon/nsa320-mcu.txt | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)create mode 100644
Documentation/devicetree/bindings/hwmon/nsa320-mcu.txtDocumentation/devicetree/bindings/hwmon/nsa320-mcu.txt new file +21
-0@@ -0,0 +1,21 @@ +Bindings for the fan / temperature monitor microcontroller used on +the Zyxel NSA 320 and several subsequent models. + +Required properties: +- compatible : "zyxel,nsa320-mcu" +- data-gpios : The GPIO pin connected to the data line on the MCU +- clk-gpios : The GPIO pin connected to the clock line on the MCU +- act-gpios : The GPIO pin connected to the active line on the MCU + +Example: + + hwmon { + compatible = "zyxel,nsa320-mcu"; + pinctrl-0 = <&pmx_mcu_data &pmx_mcu_clk &pmx_mcu_act>; + pinctrl-names = "default"; + + data-gpios = <&gpio0 14 GPIO_ACTIVE_HIGH>; + clk-gpios = <&gpio0 16 GPIO_ACTIVE_HIGH>; + act-gpios = <&gpio0 17 GPIO_ACTIVE_LOW>; + }; +
PATCH v2 2/3 hwmon: Create an NSA320 hardware monitoring driver
In reply to Adam BakerCreate a driver to support the hardware monitoring chip present in the Zyxel NSA320 and some of the other Zyxel NAS devices.
The driver reads fan speed and temperature from a suitably pre-programmed MCU on the device.
--- drivers/hwmon/Kconfig | 15 +++ drivers/hwmon/Makefile | 1 + drivers/hwmon/nsa320-hwmon.c | 215 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 231 insertions(+) create mode 100644 drivers/hwmon/nsa320-hwmon.c
drivers/hwmon/Kconfig +15 -0
@@ -1186,6 +1186,21 @@ config SENSORS_NCT7904 This driver can also be built as a module. If so, the module will be called nct7904. +config SENSORS_NSA320 + tristate "ZyXEL NSA320 and compatible fan speed and temperature sensors" + depends on GPIOLIB && OF + depends on MACH_KIRKWOOD || COMPILE_TEST + help + If you say yes here you get support for hardware monitoring + for the ZyXEL NSA320 Media Server and other compatible devices + (probably the NSA325 and some NSA310 variants). + + The sensor data is taken from a Holtek HT46R065 microcontroller + connected to GPIO lines. + + This driver can also be built as a module. If so, the module + will be called nsa320-hwmon. + config SENSORS_PCF8591 tristate "Philips PCF8591 ADC/DAC" depends on I2C
drivers/hwmon/Makefile +1 -0
@@ -125,6 +125,7 @@ obj-$(CONFIG_SENSORS_NCT6775) += nct6775.o obj-$(CONFIG_SENSORS_NCT7802) += nct7802.o obj-$(CONFIG_SENSORS_NCT7904) += nct7904.o obj-$(CONFIG_SENSORS_NTC_THERMISTOR) += ntc_thermistor.o +obj-$(CONFIG_SENSORS_NSA320) += nsa320-hwmon.o obj-$(CONFIG_SENSORS_PC87360) += pc87360.o obj-$(CONFIG_SENSORS_PC87427) += pc87427.o obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
drivers/hwmon/nsa320-hwmon.c new file +215 -0
215 lines not on this page · raw message
Signature · 1 line
2.1.4
Guenter Roeck
maintainerPATCH v2 2/3 hwmon: Create an NSA320 hardware monitoring driver
Quoted 7 lines · Adam Baker
Create a driver to support the hardware monitoring chip present in the Zyxel NSA320 and some of the other Zyxel NAS devices.
The driver reads fan speed and temperature from a suitably pre-programmed MCU on the device.
Signed-off-by Adam Baker <[email protected]>
Couple of additional comments below.
Quoted 1 line
---
Please provide a change log here for the next version (or a comment indicating that there is no change from the previous version). "Added Ack by ..." is a useful change log.
Quoted 3 lines
drivers/hwmon/Kconfig | 15 +++ drivers/hwmon/Makefile | 1 + drivers/hwmon/nsa320-hwmon.c | 215 +++++++++++++++++++++++++++++++++++++++++++
Please also provide Documentation/hwmon/nsa320-hwmon.
Quoted 34 lines
3 files changed, 231 insertions(+)create mode 100644
drivers/hwmon/nsa320-hwmon.cdrivers/hwmon/Kconfig +15
-0@@ -1186,6 +1186,21 @@ config SENSORS_NCT7904 This driver can also be built as a module. If so, the module will be called nct7904. +config SENSORS_NSA320 + tristate "ZyXEL NSA320 and compatible fan speed and temperature sensors" + depends on GPIOLIB && OF + depends on MACH_KIRKWOOD || COMPILE_TEST + help + If you say yes here you get support for hardware monitoring + for the ZyXEL NSA320 Media Server and other compatible devices + (probably the NSA325 and some NSA310 variants). + + The sensor data is taken from a Holtek HT46R065 microcontroller + connected to GPIO lines. + + This driver can also be built as a module. If so, the module + will be called nsa320-hwmon. + config SENSORS_PCF8591 tristate "Philips PCF8591 ADC/DAC" depends on I2Cdrivers/hwmon/Makefile +0
-0
drivers/hwmon/Makefile obj-$(CONFIG_SENSORS_NCT6775) += nct6775.o · line 128
Quoted 4 lines
drivers/hwmon/Makefile +1
-0@@ -125,6 +125,7 @@ obj-$(CONFIG_SENSORS_NCT6775) += nct6775.o obj-$(CONFIG_SENSORS_NCT7802) += nct7802.o obj-$(CONFIG_SENSORS_NCT7904) += nct7904.o obj-$(CONFIG_SENSORS_NTC_THERMISTOR) += ntc_thermistor.o +obj-$(CONFIG_SENSORS_NSA320) += nsa320-hwmon.o
NSA ... NTC
Quoted 62 lines
drivers/hwmon/Makefile +0
-0obj-$(CONFIG_SENSORS_PC87360) += pc87360.o obj-$(CONFIG_SENSORS_PC87427) += pc87427.o obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.odrivers/hwmon/nsa320-hwmon.c new file +53
-0@@ -0,0 +1,215 @@ +/* +* drivers/hwmon/nsa320-hwmon.c +* +* ZyXEL NSA320 Media Servers +* hardware monitoring +* +* Copyright (C) 2016 Adam Baker <[email protected]> +* based on a board file driver +* Copyright (C) 2012 Peter Schildmann <[email protected]> +* +* This program is free software; you can redistribute it and/or modify it +* under the terms of the GNU General Public License v2 as published by the +* Free Software Foundation. +* +* This program is distributed in the hope that it will be useful, but WITHOUT +* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +* more details. +*/ + +#include <linux/bitops.h> +#include <linux/delay.h> +#include <linux/err.h> +#include <linux/gpio/consumer.h> +#include <linux/hwmon.h> +#include <linux/hwmon-sysfs.h> +#include <linux/jiffies.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/of_platform.h> +#include <linux/platform_device.h> + +#define MAGIC_NUMBER 0x55 + +/* +* The Zyxel hwmon MCU is a Holtek HT46R065 that is factory programmed +* to perform temperature and fan speed monitoring. It is read by taking +* the active pin low. The 32 bit output word is then clocked onto the +* data line. The MSB of the data word is a magic nuber to indicate it +* has been read correctly, the next byte is the fan speed (in hundreds +* of RPM) and the last two bytes are the temperature (in tenths of a +* degree) +*/ + +struct nsa320_hwmon { +struct mutex update_lock;/* lock GPIO operations */ +unsigned long last_updated;/* jiffies */ +u32 mcu_data; +struct gpio_desc *act; +struct gpio_desc *clk; +struct gpio_desc *data;
drivers/hwmon/nsa320-hwmon.c line 63
Quoted 10 lines
drivers/hwmon/nsa320-hwmon.c new file +10
-0+}; + +enum nsa320_inputs { +NSA3XX_TEMP =0 , +NSA3XX_FAN =1 , +}; + +static const char *const nsa320_input_names[] = { + [NSA3XX_FAN ] ="Chassis Fan" , + [NSA3XX_TEMP ] ="System Temperature" ,
Swap lines, please.
drivers/hwmon/nsa320-hwmon.c line 66
Quoted 3 lines
drivers/hwmon/nsa320-hwmon.c new file +3
-0+}; + +/* Although this protocol looks similar to SPI the long delay
Please use standard multi-line comments.
Quoted 18 lines
drivers/hwmon/nsa320-hwmon.c new file +18
-0+* between the active (aka chip select) signal and the shorter +* delay between clock pulses are needed for reliable operation. +* The delays provided are taken from the manufacturer kernel, +* testing suggest they probably incorporate a reasonable safety +* margin. (The single device tested became unreliable if the +* delay was reduced to 1/10th of this value.) +*/ +static int nsa320_hwmon_update (struct device *dev) +{ +u32 mcu_data; +u32 mask; +struct nsa320_hwmon *hwmon =dev_get_drvdata (dev); + +mutex_lock (&hwmon->update_lock); + + mcu_data = hwmon->mcu_data; + +if (time_after (jiffies, hwmon->last_updated +HZ ) || mcu_data ==0 ) {
drivers/hwmon/nsa320-hwmon.c line 94
Quoted 10 lines
drivers/hwmon/nsa320-hwmon.c new file +10
-0+gpiod_set_value (hwmon->act,1 ); +msleep (100 ); + +for (mask =BIT (31 ); mask; mask >>=1 ) { +gpiod_set_value (hwmon->clk,0 ); +usleep_range (100 ,200 ); +gpiod_set_value (hwmon->clk,1 ); +usleep_range (100 ,200 ); +if (gpiod_get_value (hwmon->data)) + mcu_data |= mask;
This is problematic. The code only sets additional bits in mcu_data and never removes any (since mcu_data starts off with hwmon->mcu_data).
Quoted 23 lines
drivers/hwmon/nsa320-hwmon.c new file +23
-0+ } + +gpiod_set_value (hwmon->act,0 ); +dev_dbg (dev,"Read raw MCU data %08x\n" , mcu_data); + +if ((mcu_data >>24 ) !=MAGIC_NUMBER ) { +dev_dbg (dev,"Read invalid MCU data %08x\n" , mcu_data); +return -EIO ; + } + + hwmon->mcu_data = mcu_data; + hwmon->last_updated = jiffies; + } + +mutex_unlock (&hwmon->update_lock); + +return 0 ; +} + +static ssize_t show_label (struct device *dev, +struct device_attribute *attr,char *buf) +{ +int channel =to_sensor_dev_attr (attr)->index;
drivers/hwmon/nsa320-hwmon.c line 127
Quoted 10 lines
drivers/hwmon/nsa320-hwmon.c new file +10
-0+ +return sprintf (buf,"%s\n" , nsa320_input_names[channel]); +} + +static ssize_t show_value (struct device *dev, +struct device_attribute *attr,char *buf) +{ +int channel =to_sensor_dev_attr (attr)->index; +struct nsa320_hwmon *hwmon =dev_get_drvdata (dev); +unsigned long value =0 ;
int should be sufficient, and it does not need to be initialized.
Quoted 4 lines
drivers/hwmon/nsa320-hwmon.c new file +4
-0+int ret =nsa320_hwmon_update (dev); + +if (ret) +return ret;
drivers/hwmon/nsa320-hwmon.c line 141
Quoted 10 lines
drivers/hwmon/nsa320-hwmon.c new file +10
-0+ +switch (channel) { +case NSA3XX_TEMP : + value = (hwmon->mcu_data &0xffff ) *100 ; +break ; +case NSA3XX_FAN : + value = ((hwmon->mcu_data &0xff0000 ) >>16 ) *100 ; +break ; +default : +return -EINVAL ;
Hmmm ... this would be an implementation error, and leaves the user guessing where the error comes from. Better make the value 0 here, and display a warning with a traceback.
This shows, though, why using a single function in cases like this is not necessarily the best idea. Ultimately you end up having code which has to deal with impossible cases. I'll leave it up to you, but I think a better implementation would be to have two separate show functions.
static ssize_t show_temp(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct nsa320_hwmon *hwmon = dev_get_drvdata(dev);
int ret = nsa320_hwmon_update(dev);
if (ret)
return ret;
return sprintf(buf, "%u\n", (hwmon->mcu_data & 0xffff) * 100);
}static ssize_t show_fan(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct nsa320_hwmon *hwmon = dev_get_drvdata(dev);
int ret = nsa320_hwmon_update(dev);
if (ret)
return ret;
return sprintf(buf, "%u\n", ((hwmon->mcu_data & 0xff0000) >> 16) * 100);
}[ and you could use DEVICE_ATTR() for the show functions ]
You could simplify the functions further, by having nsa320_hwmon_update(dev) either return an error or the value directly (a valid value will never be < 0).
static ssize_t show_temp(struct device *dev,
struct device_attribute *attr, char *buf)
{
int mcu_data = nsa320_hwmon_update(dev);
if (mcu_data < 0)
return mcu_data;
return sprintf(buf, "%d\n", (mcu_data & 0xffff) * 100);
}Quoted 75 lines
drivers/hwmon/nsa320-hwmon.c new file +74
-0+ } +return sprintf (buf,"%lu\n" , value); +} + +static SENSOR_DEVICE_ATTR(temp1_label, S_IRUGO, show_label, NULL, NSA3XX_TEMP); +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_value, NULL, NSA3XX_TEMP); +static SENSOR_DEVICE_ATTR(fan1_label, S_IRUGO, show_label, NULL, NSA3XX_FAN); +static SENSOR_DEVICE_ATTR(fan1_input, S_IRUGO, show_value, NULL, NSA3XX_FAN); + +static struct attribute *nsa320_attrs[] = { + &sensor_dev_attr_temp1_label.dev_attr.attr, + &sensor_dev_attr_temp1_input.dev_attr.attr, + &sensor_dev_attr_fan1_label.dev_attr.attr, + &sensor_dev_attr_fan1_input.dev_attr.attr, +NULL +}; + +ATTRIBUTE_GROUPS (nsa320); + +static const struct of_device_id of_nsa320_hwmon_match[] = { + { .compatible ="zyxel,nsa320-mcu" , }, + { }, +}; + +static int nsa320_hwmon_probe (struct platform_device *pdev) +{ +struct nsa320_hwmon *hwmon; +struct device *classdev; + + hwmon =devm_kzalloc (&pdev->dev,sizeof (*hwmon),GFP_KERNEL ); +if (!hwmon) +return -ENOMEM ; + +/* Look up the GPIO pins to use */ + hwmon->act =devm_gpiod_get (&pdev->dev,"act" ,GPIOD_OUT_LOW ); +if (IS_ERR (hwmon->act)) +return PTR_ERR (hwmon->act); + + hwmon->clk =devm_gpiod_get (&pdev->dev,"clk" ,GPIOD_OUT_HIGH ); +if (IS_ERR (hwmon->clk)) +return PTR_ERR (hwmon->clk); + + hwmon->data =devm_gpiod_get (&pdev->dev,"data" ,GPIOD_IN ); +if (IS_ERR (hwmon->data)) +return PTR_ERR (hwmon->data); + +mutex_init (&hwmon->update_lock); + + classdev =devm_hwmon_device_register_with_groups (&pdev->dev, +"nsa320" , hwmon, nsa320_groups); + +return PTR_ERR_OR_ZERO (classdev); + +} + +/* All allocations use devres so remove() is not needed. */ + +static struct platform_driver nsa320_hwmon_driver = { + .probe = nsa320_hwmon_probe, + .driver = { + .name ="nsa320-hwmon" , + .owner =THIS_MODULE , + .of_match_table =of_match_ptr (of_nsa320_hwmon_match), + }, +}; + +module_platform_driver (nsa320_hwmon_driver); + +MODULE_DEVICE_TABLE (of, of_nsa320_hwmon_match); +MODULE_AUTHOR ("Peter Schildmann <[email protected]>" ); +MODULE_AUTHOR ("Adam Baker <[email protected]>" ); +MODULE_DESCRIPTION ("NSA3XX Hardware Monitoring" ); +MODULE_LICENSE ("GPL v2" ); +MODULE_ALIAS ("platform:nsa320-hwmon" );
PATCH v2 2/3 hwmon: Create an NSA320 hardware monitoring driver
Quoted 19 lines · Guenter Roeck
Quoted 7 lines · Adam Baker
Create a driver to support the hardware monitoring chip present in the Zyxel NSA320 and some of the other Zyxel NAS devices.
The driver reads fan speed and temperature from a suitably pre-programmed MCU on the device.
Signed-off-by Adam Baker <[email protected]>Couple of additional comments below.
Quoted 1 line
---
Please provide a change log here for the next version (or a comment indicating that
there is no change from the previous version). "Added Ack by ..." is a useful
change log.
Quoted 6 lines
Quoted 4 lines
drivers/hwmon/Kconfig | 15 +++ drivers/hwmon/Makefile | 1 + drivers/hwmon/nsa320-hwmon.c | 215+++++++++++++++++++++++++++++++++++++++++++
Please also provide
Documentation/hwmon/nsa320-hwmon.
Added in v3
Quoted 34 lines
Quoted 34 lines
3 files changed, 231 insertions(+)create mode 100644
drivers/hwmon/nsa320-hwmon.cdrivers/hwmon/Kconfig +2
-0@@ -1186,6 +1186,21 @@ config SENSORS_NCT7904 This driver can also be built as a module. If so, the module will be called nct7904. +config SENSORS_NSA320 + tristate "ZyXEL NSA320 and compatible fan speed and temperaturesensors"
drivers/hwmon/Kconfig +13
-0+ depends on GPIOLIB && OF + depends on MACH_KIRKWOOD || COMPILE_TEST + help + If you say yes here you get support for hardware monitoring + for the ZyXEL NSA320 Media Server and other compatible devices + (probably the NSA325 and some NSA310 variants). + + The sensor data is taken from a Holtek HT46R065 microcontroller + connected to GPIO lines. + + This driver can also be built as a module. If so, the module + will be called nsa320-hwmon. + config SENSORS_PCF8591 tristate "Philips PCF8591 ADC/DAC" depends on I2Cdrivers/hwmon/Makefile +0
-1--- a/drivers/hwmon/Makefile
Quoted 8 lines
drivers/hwmon/Makefile obj-$(CONFIG_SENSORS_NCT6775) += nct6775.o · line 128
Quoted 6 lines
+++ b/drivers/hwmon/Makefile
drivers/hwmon/Makefile +1
-0@@ -125,6 +125,7 @@ obj-$(CONFIG_SENSORS_NCT6775) += nct6775.o obj-$(CONFIG_SENSORS_NCT7802) += nct7802.o obj-$(CONFIG_SENSORS_NCT7904) += nct7904.o obj-$(CONFIG_SENSORS_NTC_THERMISTOR) += ntc_thermistor.o +obj-$(CONFIG_SENSORS_NSA320) += nsa320-hwmon.oNSA ... NTC
done
Quoted 73 lines
Quoted 73 lines
drivers/hwmon/Makefile +0
-0obj-$(CONFIG_SENSORS_PC87360) += pc87360.o obj-$(CONFIG_SENSORS_PC87427) += pc87427.o obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.odrivers/hwmon/nsa320-hwmon.c new file +11
-0@@ -0,0 +1,215 @@ +/* +* drivers/hwmon/nsa320-hwmon.c +* +* ZyXEL NSA320 Media Servers +* hardware monitoring +* +* Copyright (C) 2016 Adam Baker <[email protected]> +* based on a board file driver +* Copyright (C) 2012 Peter Schildmann <[email protected]> +* +* This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License v2 as published by the
+ * Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ */drivers/hwmon/nsa320-hwmon.c new file +20
-0+ +#include <linux/bitops.h> +#include <linux/delay.h> +#include <linux/err.h> +#include <linux/gpio/consumer.h> +#include <linux/hwmon.h> +#include <linux/hwmon-sysfs.h> +#include <linux/jiffies.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/of_platform.h> +#include <linux/platform_device.h> + +#define MAGIC_NUMBER 0x55 + +/* +* The Zyxel hwmon MCU is a Holtek HT46R065 that is factory programmed +* to perform temperature and fan speed monitoring. It is read by taking
+ * the active pin low. The 32 bit output word is then clocked onto the
+ * data line. The MSB of the data word is a magic nuber to indicate it
+ * has been read correctly, the next byte is the fan speed (in hundreds
+ * of RPM) and the last two bytes are the temperature (in tenths of a
+ * degree)
+ */drivers/hwmon/nsa320-hwmon.c new file +12
-0+ +struct nsa320_hwmon { +struct mutex update_lock;/* lock GPIO operations */ +unsigned long last_updated;/* jiffies */ +u32 mcu_data; +struct gpio_desc *act; +struct gpio_desc *clk; +struct gpio_desc *data; +}; + +enum nsa320_inputs { +NSA3XX_TEMP =0 ,
Quoted 8 lines
drivers/hwmon/nsa320-hwmon.c line 63
Quoted 6 lines
drivers/hwmon/nsa320-hwmon.c new file +6
-0+ NSA3XX_FAN = 1, +}; + +static const char *const nsa320_input_names[] = { + [NSA3XX_FAN ] ="Chassis Fan" , + [NSA3XX_TEMP ] ="System Temperature" ,Swap lines, please.
done
Quoted 6 lines
drivers/hwmon/nsa320-hwmon.c line 66
Quoted 4 lines
drivers/hwmon/nsa320-hwmon.c new file +3
-0+}; + +/* Although this protocol looks similar to SPI the long delay Please use standard multi-line comments.
done
Quoted 25 lines
Quoted 25 lines
+ * between the active (aka chip select) signal and the shorter
+ * delay between clock pulses are needed for reliable operation.
+ * The delays provided are taken from the manufacturer kernel,
+ * testing suggest they probably incorporate a reasonable safety
+ * margin. (The single device tested became unreliable if the
+ * delay was reduced to 1/10th of this value.)
+ */drivers/hwmon/nsa320-hwmon.c new file +11
-0+static int nsa320_hwmon_update(struct device *dev) +{ + u32 mcu_data; + u32 mask; + struct nsa320_hwmon *hwmon = dev_get_drvdata(dev); + + mutex_lock(&hwmon->update_lock); + + mcu_data = hwmon->mcu_data; + + if (time_after(jiffies, hwmon->last_updated + HZ) || mcu_data ==0) {
drivers/hwmon/nsa320-hwmon.c new file +5
-0+gpiod_set_value (hwmon->act,1 ); +msleep (100 ); + +for (mask =BIT (31 ); mask; mask >>=1 ) { +gpiod_set_value (hwmon->clk,0 );
Quoted 8 lines
drivers/hwmon/nsa320-hwmon.c line 94
Quoted 5 lines
drivers/hwmon/nsa320-hwmon.c new file +5
-0+usleep_range (100 ,200 ); +gpiod_set_value (hwmon->clk,1 ); +usleep_range (100 ,200 ); +if (gpiod_get_value (hwmon->data)) + mcu_data |= mask;This is problematic. The code only sets additional bits in
mcu_dataand never removes any (sincemcu_datastarts off withhwmon->mcu_data).
mcu_data now initialised to 0
Quoted 28 lines
Quoted 28 lines
drivers/hwmon/nsa320-hwmon.c new file +27
-0+ } + +gpiod_set_value (hwmon->act,0 ); +dev_dbg (dev,"Read raw MCU data %08x\n" , mcu_data); + +if ((mcu_data >>24 ) !=MAGIC_NUMBER ) { +dev_dbg (dev,"Read invalid MCU data %08x\n" , mcu_data); +return -EIO ; + } + + hwmon->mcu_data = mcu_data; + hwmon->last_updated = jiffies; + } + +mutex_unlock (&hwmon->update_lock); + +return 0 ; +} + +static ssize_t show_label (struct device *dev, +struct device_attribute *attr,char *buf) +{ +int channel =to_sensor_dev_attr (attr)->index; + +return sprintf (buf,"%s\n" , nsa320_input_names[channel]); +} +
Quoted 8 lines
drivers/hwmon/nsa320-hwmon.c line 127
Quoted 6 lines
drivers/hwmon/nsa320-hwmon.c new file +6
-0+static ssize_t show_value (struct device *dev, +struct device_attribute *attr,char *buf) +{ +int channel =to_sensor_dev_attr (attr)->index; +struct nsa320_hwmon *hwmon =dev_get_drvdata (dev); +unsigned long value =0 ;int should be sufficient, and it does not need to be initialized.
The temporary has gone away in the latest version, it uses the output of nsa320_hwmon_update() directly. That value is unsigned long as it needs to be unsigned to avoid any risk of undefined behaviour with the bitshift operator
and long to guarantee a minimum length of 32 bits.
Quoted 60 lines
Quoted 5 lines
drivers/hwmon/nsa320-hwmon.c new file +4
-0+int ret =nsa320_hwmon_update (dev); + +if (ret) +return ret;drivers/hwmon/nsa320-hwmon.c line 141
Quoted 10 lines
drivers/hwmon/nsa320-hwmon.c new file +10
-0+ +switch (channel) { +case NSA3XX_TEMP : + value = (hwmon->mcu_data &0xffff ) *100 ; +break ; +case NSA3XX_FAN : + value = ((hwmon->mcu_data &0xff0000 ) >>16 ) *100 ; +break ; +default : +return -EINVAL ;Hmmm ... this would be an implementation error, and leaves the user guessing where the error comes from. Better make the value 0 here, and display a warning with a traceback.
This shows, though, why using a single function in cases like this is not necessarily the best idea. Ultimately you end up having code which has to deal with impossible cases. I'll leave it up to you, but I think a better implementation would be to have two separate show functions.
static
ssize_tshow_temp(struct device *dev,
structdevice_attribute*attr, char *buf){ struct nsa320_hwmon *hwmon = dev_get_drvdata(dev); int ret = nsa320_hwmon_update(dev); if (ret) return ret; return sprintf(buf, "%u\n", (hwmon->mcu_data & 0xffff) * 100); }static
ssize_tshow_fan(struct device *dev,
structdevice_attribute*attr, char *buf){ struct nsa320_hwmon *hwmon = dev_get_drvdata(dev); int ret = nsa320_hwmon_update(dev); if (ret) return ret; return sprintf(buf, "%u\n", ((hwmon->mcu_data & 0xff0000) >> 16) *100);
}[ and you could use
DEVICE_ATTR()for the show functions ]You could simplify the functions further, by having
nsa320_hwmon_update(dev)
either return an error or the value directly (a valid value will never be < 0).static
ssize_tshow_temp(struct device *dev,
structdevice_attribute*attr, char *buf)
Quoted 8 lines
{
int mcu_data = nsa320_hwmon_update(dev);
if (mcu_data < 0)
return mcu_data;
return sprintf(buf, "%d\n", (mcu_data & 0xffff) * 100);
}I've opted for the last of your suggestions
Quoted 86 lines
Quoted 70 lines
drivers/hwmon/nsa320-hwmon.c new file +5
-0+ } +return sprintf (buf,"%lu\n" , value); +} + +static SENSOR_DEVICE_ATTR(temp1_label, S_IRUGO, show_label, NULL,
NSA3XX_TEMP);
+staticSENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_value, NULL, NSA3XX_TEMP);
+staticSENSOR_DEVICE_ATTR(fan1_label, S_IRUGO, show_label, NULL, NSA3XX_FAN);
+staticSENSOR_DEVICE_ATTR(fan1_input, S_IRUGO, show_value, NULL, NSA3XX_FAN);drivers/hwmon/nsa320-hwmon.c new file +56
-0+ +static struct attribute *nsa320_attrs[] = { + &sensor_dev_attr_temp1_label.dev_attr.attr, + &sensor_dev_attr_temp1_input.dev_attr.attr, + &sensor_dev_attr_fan1_label.dev_attr.attr, + &sensor_dev_attr_fan1_input.dev_attr.attr, +NULL +}; + +ATTRIBUTE_GROUPS (nsa320); + +static const struct of_device_id of_nsa320_hwmon_match[] = { + { .compatible ="zyxel,nsa320-mcu" , }, + { }, +}; + +static int nsa320_hwmon_probe (struct platform_device *pdev) +{ +struct nsa320_hwmon *hwmon; +struct device *classdev; + + hwmon =devm_kzalloc (&pdev->dev,sizeof (*hwmon),GFP_KERNEL ); +if (!hwmon) +return -ENOMEM ; + +/* Look up the GPIO pins to use */ + hwmon->act =devm_gpiod_get (&pdev->dev,"act" ,GPIOD_OUT_LOW ); +if (IS_ERR (hwmon->act)) +return PTR_ERR (hwmon->act); + + hwmon->clk =devm_gpiod_get (&pdev->dev,"clk" ,GPIOD_OUT_HIGH ); +if (IS_ERR (hwmon->clk)) +return PTR_ERR (hwmon->clk); + + hwmon->data =devm_gpiod_get (&pdev->dev,"data" ,GPIOD_IN ); +if (IS_ERR (hwmon->data)) +return PTR_ERR (hwmon->data); + +mutex_init (&hwmon->update_lock); + + classdev =devm_hwmon_device_register_with_groups (&pdev->dev, +"nsa320" , hwmon, nsa320_groups); + +return PTR_ERR_OR_ZERO (classdev); + +} + +/* All allocations use devres so remove() is not needed. */ + +static struct platform_driver nsa320_hwmon_driver = { + .probe = nsa320_hwmon_probe, + .driver = { + .name ="nsa320-hwmon" , + .owner =THIS_MODULE , + .of_match_table =of_match_ptr (of_nsa320_hwmon_match), + },drivers/hwmon/nsa320-hwmon.c line 215
Quoted 10 lines
drivers/hwmon/nsa320-hwmon.c new file +10
-0+}; + +module_platform_driver (nsa320_hwmon_driver); + +MODULE_DEVICE_TABLE (of, of_nsa320_hwmon_match); +MODULE_AUTHOR ("Peter Schildmann <[email protected]>" ); +MODULE_AUTHOR ("Adam Baker <[email protected]>" ); +MODULE_DESCRIPTION ("NSA3XX Hardware Monitoring" ); +MODULE_LICENSE ("GPL v2" ); +MODULE_ALIAS ("platform:nsa320-hwmon" );_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Guenter Roeck
maintainerPATCH v2 2/3 hwmon: Create an NSA320 hardware monitoring driver
Quoted 220 lines · Adam Baker
Quoted 17 lines · Guenter Roeck
Quoted 7 lines · Adam Baker
Create a driver to support the hardware monitoring chip present in the Zyxel NSA320 and some of the other Zyxel NAS devices.
The driver reads fan speed and temperature from a suitably pre-programmed MCU on the device.
Signed-off-by Adam Baker <[email protected]>Couple of additional comments below.
Quoted 1 line
---
Please provide a change log here for the next version (or a comment indicating that there is no change from the previous version). "Added Ack by ..." is a useful change log.
Quoted 5 lines
Quoted 3 lines
drivers/hwmon/Kconfig | 15 +++ drivers/hwmon/Makefile | 1 + drivers/hwmon/nsa320-hwmon.c | 215 +++++++++++++++++++++++++++++++++++++++++++Please also provide
Documentation/hwmon/nsa320-hwmon.Added in v3
Quoted 33 lines
Quoted 33 lines
3 files changed, 231 insertions(+)create mode 100644
drivers/hwmon/nsa320-hwmon.cdrivers/hwmon/Kconfig +15
-0@@ -1186,6 +1186,21 @@ config SENSORS_NCT7904 This driver can also be built as a module. If so, the module will be called nct7904. +config SENSORS_NSA320 + tristate "ZyXEL NSA320 and compatible fan speed and temperature sensors" + depends on GPIOLIB && OF + depends on MACH_KIRKWOOD || COMPILE_TEST + help + If you say yes here you get support for hardware monitoring + for the ZyXEL NSA320 Media Server and other compatible devices + (probably the NSA325 and some NSA310 variants). + + The sensor data is taken from a Holtek HT46R065 microcontroller + connected to GPIO lines. + + This driver can also be built as a module. If so, the module + will be called nsa320-hwmon. + config SENSORS_PCF8591 tristate "Philips PCF8591 ADC/DAC" depends on I2Cdrivers/hwmon/Makefile +0
-1--- a/drivers/hwmon/MakefileQuoted 8 lines
drivers/hwmon/Makefile obj-$(CONFIG_SENSORS_NCT6775) += nct6775.o · line 128
Quoted 6 lines
+++ b/drivers/hwmon/Makefile
drivers/hwmon/Makefile +1
-0@@ -125,6 +125,7 @@ obj-$(CONFIG_SENSORS_NCT6775) += nct6775.o obj-$(CONFIG_SENSORS_NCT7802) += nct7802.o obj-$(CONFIG_SENSORS_NCT7904) += nct7904.o obj-$(CONFIG_SENSORS_NTC_THERMISTOR) += ntc_thermistor.o +obj-$(CONFIG_SENSORS_NSA320) += nsa320-hwmon.oNSA ... NTC
done
Quoted 67 lines
Quoted 67 lines
drivers/hwmon/Makefile +0
-0obj-$(CONFIG_SENSORS_PC87360) += pc87360.o obj-$(CONFIG_SENSORS_PC87427) += pc87427.o obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.odrivers/hwmon/nsa320-hwmon.c new file +57
-0@@ -0,0 +1,215 @@ +/* +* drivers/hwmon/nsa320-hwmon.c +* +* ZyXEL NSA320 Media Servers +* hardware monitoring +* +* Copyright (C) 2016 Adam Baker <[email protected]> +* based on a board file driver +* Copyright (C) 2012 Peter Schildmann <[email protected]> +* +* This program is free software; you can redistribute it and/or modify it +* under the terms of the GNU General Public License v2 as published by the +* Free Software Foundation. +* +* This program is distributed in the hope that it will be useful, but WITHOUT +* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +* more details. +*/ + +#include <linux/bitops.h> +#include <linux/delay.h> +#include <linux/err.h> +#include <linux/gpio/consumer.h> +#include <linux/hwmon.h> +#include <linux/hwmon-sysfs.h> +#include <linux/jiffies.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/of_platform.h> +#include <linux/platform_device.h> + +#define MAGIC_NUMBER 0x55 + +/* +* The Zyxel hwmon MCU is a Holtek HT46R065 that is factory programmed +* to perform temperature and fan speed monitoring. It is read by taking +* the active pin low. The 32 bit output word is then clocked onto the +* data line. The MSB of the data word is a magic nuber to indicate it +* has been read correctly, the next byte is the fan speed (in hundreds +* of RPM) and the last two bytes are the temperature (in tenths of a +* degree) +*/ + +struct nsa320_hwmon { +struct mutex update_lock;/* lock GPIO operations */ +unsigned long last_updated;/* jiffies */ +u32 mcu_data; +struct gpio_desc *act; +struct gpio_desc *clk; +struct gpio_desc *data; +}; + +enum nsa320_inputs { +NSA3XX_TEMP =0 ,Quoted 8 lines
drivers/hwmon/nsa320-hwmon.c line 63
Quoted 6 lines
drivers/hwmon/nsa320-hwmon.c new file +6
-0+ NSA3XX_FAN = 1, +}; + +static const char *const nsa320_input_names[] = { + [NSA3XX_FAN ] ="Chassis Fan" , + [NSA3XX_TEMP ] ="System Temperature" ,Swap lines, please.
done
Quoted 6 lines
drivers/hwmon/nsa320-hwmon.c line 66
Quoted 4 lines
drivers/hwmon/nsa320-hwmon.c new file +3
-0+}; + +/* Although this protocol looks similar to SPI the long delay Please use standard multi-line comments.
done
Quoted 24 lines
Quoted 24 lines
drivers/hwmon/nsa320-hwmon.c new file +23
-0+* between the active (aka chip select) signal and the shorter +* delay between clock pulses are needed for reliable operation. +* The delays provided are taken from the manufacturer kernel, +* testing suggest they probably incorporate a reasonable safety +* margin. (The single device tested became unreliable if the +* delay was reduced to 1/10th of this value.) +*/ +static int nsa320_hwmon_update (struct device *dev) +{ +u32 mcu_data; +u32 mask; +struct nsa320_hwmon *hwmon =dev_get_drvdata (dev); + +mutex_lock (&hwmon->update_lock); + + mcu_data = hwmon->mcu_data; + +if (time_after (jiffies, hwmon->last_updated +HZ ) || mcu_data ==0 ) { +gpiod_set_value (hwmon->act,1 ); +msleep (100 ); + +for (mask =BIT (31 ); mask; mask >>=1 ) { +gpiod_set_value (hwmon->clk,0 );Quoted 8 lines
drivers/hwmon/nsa320-hwmon.c line 94
Quoted 5 lines
drivers/hwmon/nsa320-hwmon.c new file +5
-0+usleep_range (100 ,200 ); +gpiod_set_value (hwmon->clk,1 ); +usleep_range (100 ,200 ); +if (gpiod_get_value (hwmon->data)) + mcu_data |= mask;This is problematic. The code only sets additional bits in
mcu_dataand never removes any (sincemcu_datastarts off withhwmon->mcu_data).
mcu_datanow initialised to 0Quoted 28 lines
Quoted 28 lines
drivers/hwmon/nsa320-hwmon.c new file +27
-0+ } + +gpiod_set_value (hwmon->act,0 ); +dev_dbg (dev,"Read raw MCU data %08x\n" , mcu_data); + +if ((mcu_data >>24 ) !=MAGIC_NUMBER ) { +dev_dbg (dev,"Read invalid MCU data %08x\n" , mcu_data); +return -EIO ; + } + + hwmon->mcu_data = mcu_data; + hwmon->last_updated = jiffies; + } + +mutex_unlock (&hwmon->update_lock); + +return 0 ; +} + +static ssize_t show_label (struct device *dev, +struct device_attribute *attr,char *buf) +{ +int channel =to_sensor_dev_attr (attr)->index; + +return sprintf (buf,"%s\n" , nsa320_input_names[channel]); +} +Quoted 8 lines
drivers/hwmon/nsa320-hwmon.c line 127
Quoted 6 lines
drivers/hwmon/nsa320-hwmon.c new file +6
-0+static ssize_t show_value (struct device *dev, +struct device_attribute *attr,char *buf) +{ +int channel =to_sensor_dev_attr (attr)->index; +struct nsa320_hwmon *hwmon =dev_get_drvdata (dev); +unsigned long value =0 ;int should be sufficient, and it does not need to be initialized.
Quoted 4 lines
The temporary has gone away in the latest version, it uses the output of
nsa320_hwmon_update()directly. That value is unsigned long as it needs to be unsigned to avoid any risk of undefined behaviour with the bitshift operator and long to guarantee a minimum length of 32 bits.
The minimum length argument doesn't fly (there is no hardware supporting Linux which has an integer length of less than 16 bit), and the unsigned argument doesn't fly either - bitshift operations on a local variable do not affect the function return value of nsa320_hwmon_update().
Besides, if you are concerned about the number of bits you can declare the variable types and return values to be s32 and u32.
Guenter
PATCH v2 3/3 ARM: mvebu: Add the hardware monitor to the NSA320 device tree
In reply to Adam BakerAdd an entry for the hardware monitoring MCU
--- arch/arm/boot/dts/kirkwood-nsa320.dts | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
arch/arm/boot/dts/kirkwood-nsa320.dts +11 -2
@@ -193,10 +193,19 @@ }; }; + hwmon { + compatible = "zyxel,nsa320-mcu"; + pinctrl-0 = <&pmx_mcu_data &pmx_mcu_clk &pmx_mcu_act>; + pinctrl-names = "default"; + + data-gpios = <&gpio0 14 GPIO_ACTIVE_HIGH>; + clk-gpios = <&gpio0 16 GPIO_ACTIVE_HIGH>; + act-gpios = <&gpio0 17 GPIO_ACTIVE_LOW>; + }; + /* The following pins are currently not assigned to a driver, some of them should be configured as inputs. - pinctrl-0 = <&pmx_mcu_data &pmx_mcu_clk &pmx_mcu_act - &pmx_htp &pmx_vid_b1 + pinctrl-0 = <&pmx_htp &pmx_vid_b1 &pmx_power_resume_data &pmx_power_resume_clk>; */ };
Signature · 1 line
2.1.4