ARM: mvebu: Add driver for the Zyxel NSA3XX hwmon MCU
Subsystems (2)
cover ARM: mvebu: Add driver for the Zyxel NSA3XX hwmon MCU
This patch series adds a driver to support the hardware monitoring MCU that is present in some models of the Zyxel NSA3xx series of NAS units.
A patch is included to add it to the device tree for the NSA320 model, patches to the other device trees will be included if testers come forward (I already have a possible tester for NSA325).
There are no dependencies between the patches so I guess the first two should go via the hardware monitoring maintainers and the third via the ARM mvebu maintainers.
[PATCH 1/3] Define a binding for the hardware monitoring chip present [PATCH 2/3] Create a driver to support the hardware monitoring chip [PATCH 3/3] Add an entry for the hardware monitoring MCU
Documentation/devicetree/bindings/hwmon/nsa320-mcu.txt | 21 ++++++++ arch/arm/boot/dts/kirkwood-nsa320.dts | 13 ++++- drivers/hwmon/Kconfig | 13 +++++ drivers/hwmon/Makefile | 1 + drivers/hwmon/nsa3xx-hwmon.c | 223 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 269 insertions(+), 2 deletions(-)
PATCH 1/3 ARM: mvebu: Define binding for the nsa3xx-hwmon driver
Define a binding for the hardware monitoring chip present in several variants of the Zyxel NSA3xx 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
Rob Herring
maintainerPATCH 1/3 ARM: mvebu: Define binding for the nsa3xx-hwmon driver
Quoted 8 lines · Adam Baker
Define a binding for the hardware monitoring chip present in several variants of the Zyxel NSA3xx NAS devices.
Signed-off-by Adam Baker <[email protected]>--- .../devicetree/bindings/hwmon/nsa320-mcu.txt | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 Documentation/devicetree/bindings/hwmon/nsa320-mcu.txt
PATCH 2/3 ARM: mvebu: Create a NSA3xx hardware monitoring driver
In reply to Adam BakerCreate a driver to support the hardware monitoring chip present in several models of Zyxel NSA3xx NAS devices.
The driver reads fan speed and temperature from a suitably programmed MCU on the device.
---
I've tested this on an NSA-320, I've had someone offer to test it on NSA-325 so hopefully I will get a tested by back.
--- drivers/hwmon/Kconfig | 13 +++ drivers/hwmon/Makefile | 1 + drivers/hwmon/nsa3xx-hwmon.c | 223 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 237 insertions(+) create mode 100644 drivers/hwmon/nsa3xx-hwmon.c
drivers/hwmon/Kconfig +13 -0
@@ -1757,6 +1757,19 @@ config SENSORS_ULTRA45 This driver provides support for the Ultra45 workstation environmental sensors. +config SENSORS_NSA3XX + tristate "ZyXEL NSA3xx fan speed and temperature sensors" + depends on GPIOLIB && OF + help + If you say yes here you get support for hardware monitoring + for the ZyXEL NSA3XX Media Servers. + + 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 nsa3xx-hwmon. + if ACPI comment "ACPI drivers"
drivers/hwmon/Makefile +1 -0
@@ -118,6 +118,7 @@ obj-$(CONFIG_SENSORS_MAX6650) += max6650.o obj-$(CONFIG_SENSORS_MAX6697) += max6697.o obj-$(CONFIG_SENSORS_MAX31790) += max31790.o obj-$(CONFIG_SENSORS_MC13783_ADC)+= mc13783-adc.o +obj-$(CONFIG_SENSORS_NSA3XX) += nsa3xx-hwmon.o obj-$(CONFIG_SENSORS_MCP3021) += mcp3021.o obj-$(CONFIG_SENSORS_MENF21BMC_HWMON) += menf21bmc_hwmon.o obj-$(CONFIG_SENSORS_NCT6683) += nct6683.o
drivers/hwmon/nsa3xx-hwmon.c new file +223 -0
223 lines not on this page · raw message
Signature · 1 line
2.1.4
Guenter Roeck
maintainerPATCH 2/3 ARM: mvebu: Create a NSA3xx hardware monitoring driver
Hi,
Quoted 7 lines · Adam Baker
Create a driver to support the hardware monitoring chip present in several models of Zyxel NSA3xx NAS devices.
The driver reads fan speed and temperature from a suitably programmed MCU on the device.
Signed-off-by Adam Baker <[email protected]>
Please name the driver nsa320. It may ultimately support other devices, but that doesn't mean it is going to support NSA-35x or other future similar devices.
Got the headline, please use
hwmon: Add driver for Zyxel NSA-320
Quoted 15 lines
---
I've tested this on an NSA-320, I've had someone offer to test it on NSA-325 so hopefully I will get a tested by back.--- drivers/hwmon/Kconfig | 13 +++ drivers/hwmon/Makefile | 1 + drivers/hwmon/nsa3xx-hwmon.c | 223 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 237 insertions(+) create mode 100644 drivers/hwmon/nsa3xx-hwmon.cdrivers/hwmon/Kconfig +0
-0
drivers/hwmon/Kconfig config SENSORS_ULTRA45 · line 1760
Quoted 4 lines
drivers/hwmon/Kconfig +1
-0@@ -1757,6 +1757,19 @@ config SENSORS_ULTRA45 This driver provides support for the Ultra45 workstation environmental sensors. +config SENSORS_NSA3XX
Please try to stick with alphabetic order.
drivers/hwmon/Kconfig config SENSORS_ULTRA45 · line 1761
Quoted 1 line
drivers/hwmon/Kconfig +1
-0+ tristate "ZyXEL NSA3xx fan speed and temperature sensors"
... NSA-320 and compatible ...
drivers/hwmon/Kconfig config SENSORS_ULTRA45 · line 1762
Quoted 1 line
drivers/hwmon/Kconfig +1
-0+ depends on GPIOLIB && OF
Can you add some additional dependencies ?
It is quite unlikely that the driver is needed on an X86. Please use at least
depends on ARM || COMPILE_TEST
or a more specific dependency if the SoC is known. MACH_KIRKWOOD, maybe ?
drivers/hwmon/Kconfig config SENSORS_ULTRA45 · line 1765
Quoted 3 lines
drivers/hwmon/Kconfig +3
-0+ help + If you say yes here you get support for hardware monitoring + for the ZyXEL NSA3XX Media Servers.
Please be specific which devices are supported. Feel free to add "and compatibles", but please no generic and misleading statements such as 3XX.
drivers/hwmon/Kconfig config SENSORS_ULTRA45 · line 1769
Quoted 4 lines
drivers/hwmon/Kconfig +4
-0+ + The sensor data is taken from a Holtek HT46R065 microcontroller + connected to GPIO lines. +
Is that relevant ?
Quoted 11 lines
drivers/hwmon/Kconfig +3
-0+ This driver can also be built as a module. If so, the module + will be called nsa3xx-hwmon. + if ACPI comment "ACPI drivers"drivers/hwmon/Makefile +0
-0
drivers/hwmon/Makefile obj-$(CONFIG_SENSORS_MAX6650) += max6650.o · line 121
Quoted 4 lines
drivers/hwmon/Makefile +1
-0@@ -118,6 +118,7 @@ obj-$(CONFIG_SENSORS_MAX6650) += max6650.o obj-$(CONFIG_SENSORS_MAX6697) += max6697.o obj-$(CONFIG_SENSORS_MAX31790) += max31790.o obj-$(CONFIG_SENSORS_MC13783_ADC)+= mc13783-adc.o +obj-$(CONFIG_SENSORS_NSA3XX) += nsa3xx-hwmon.o
Please try to stick with alphabetic order.
drivers/hwmon/nsa3xx-hwmon.c
Quoted 8 lines
drivers/hwmon/Makefile +0
-0obj-$(CONFIG_SENSORS_MCP3021) += mcp3021.o obj-$(CONFIG_SENSORS_MENF21BMC_HWMON) += menf21bmc_hwmon.o obj-$(CONFIG_SENSORS_NCT6683) += nct6683.odrivers/hwmon/nsa3xx-hwmon.c new file +0
-0
nsa320-hwmon.c
drivers/hwmon/nsa3xx-hwmon.c line 4
Quoted 5 lines
drivers/hwmon/nsa3xx-hwmon.c new file +4
-0@@ -0,0 +1,223 @@ +/* +* drivers/hwmon/nsa3xx-hwmon.c +* +* ZyXEL NSA3xx Media Servers
Please be specific.
Quoted 19 lines
drivers/hwmon/nsa3xx-hwmon.c new file +19
-0+* 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/module.h> +#include <linux/mutex.h> +#include <linux/platform_device.h>
drivers/hwmon/nsa3xx-hwmon.c line 33
Quoted 10 lines
drivers/hwmon/nsa3xx-hwmon.c new file +10
-0+#include <linux/err.h> +#include <linux/gpio/consumer.h> +#include <linux/hwmon.h> +#include <linux/hwmon-sysfs.h> +#include <linux/jiffies.h> +#include <linux/delay.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/of_platform.h> +
Please order include files alphabetically.
drivers/hwmon/nsa3xx-hwmon.c line 46
Quoted 13 lines
drivers/hwmon/nsa3xx-hwmon.c new file +13
-0+#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 nsa3xx_hwmon {
s/nsa3xx/nsa320/g for the entire file, please.
drivers/hwmon/nsa3xx-hwmon.c line 50
Quoted 4 lines
drivers/hwmon/nsa3xx-hwmon.c new file +4
-0+struct device *classdev; +struct mutex update_lock;/* lock GPIO operations */ +unsigned long last_updated;/* jiffies */ +u32 mcu_data;
I would suggest to use unsigned long here. See below for reasons.
drivers/hwmon/nsa3xx-hwmon.c line 58
Quoted 8 lines
drivers/hwmon/nsa3xx-hwmon.c new file +8
-0+struct gpio_desc *act; +struct gpio_desc *clk; +struct gpio_desc *data; +}; + +enum nsa3xx_inputs { +NSA3XX_FAN =1 , +NSA3XX_TEMP =0 ,
Any special reason for the unusual order ? If yes, please explain.
Quoted 18 lines
drivers/hwmon/nsa3xx-hwmon.c new file +18
-0+}; + +static const char *const nsa3xx_input_names[] = { + [NSA3XX_FAN ] ="Chassis Fan" , + [NSA3XX_TEMP ] ="System Temperature" , +}; + +/* Although this protocol looks similar to SPI the long delay +* 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 unsigned long nsa3xx_hwmon_update (struct device *dev) +{ +u32 mcu_data;
drivers/hwmon/nsa3xx-hwmon.c line 86
Quoted 10 lines
drivers/hwmon/nsa3xx-hwmon.c new file +10
-0+u32 mask; +struct nsa3xx_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 ) { +
Please no blank line here. Also, please align continuation lines with '('. However, unless I am missing something, the continuation line is not needed in the first place. Please no unnecessary continuation lines.
drivers/hwmon/nsa3xx-hwmon.c line 90
Quoted 4 lines
drivers/hwmon/nsa3xx-hwmon.c new file +4
-0+gpiod_set_value (hwmon->act,1 ); +msleep (100 ); + +for (mask =BIT (31 ); mask; mask >>=1 ) {
Since you are using BIT(), please include linux/bitops.h.
Quoted 4 lines
drivers/hwmon/nsa3xx-hwmon.c new file +4
-0+gpiod_set_value (hwmon->clk,0 ); +usleep_range (100 ,200 ); +gpiod_set_value (hwmon->clk,1 ); +usleep_range (100 ,200 );
drivers/hwmon/nsa3xx-hwmon.c line 104
Quoted 10 lines
drivers/hwmon/nsa3xx-hwmon.c new file +10
-0+if (gpiod_get_value (hwmon->data)) + mcu_data |= mask; + } + +gpiod_set_value (hwmon->act,0 ); +dev_dbg (dev,"Read raw MCU data %08x\n" , mcu_data); + +if ((mcu_data >>24 ) !=MAGIC_NUMBER ) { +dev_err (dev,"Read invalid MCU data %08x\n" , mcu_data); + mcu_data =0 ;
Instead of an error message, it would be better to return an error code (probably -EIO since we don't know better).
The calling code can then return the error to user space.
Quoted 16 lines
drivers/hwmon/nsa3xx-hwmon.c new file +16
-0+ } + + hwmon->mcu_data = mcu_data; + hwmon->last_updated = jiffies; + } + +mutex_unlock (&hwmon->update_lock); + +return mcu_data; +} + +static ssize_t show_label (struct device *dev, +struct device_attribute *attr,char *buf) +{ +int channel =to_sensor_dev_attr (attr)->index; +
drivers/hwmon/nsa3xx-hwmon.c line 130
Quoted 10 lines
drivers/hwmon/nsa3xx-hwmon.c new file +10
-0+return sprintf (buf,"%s\n" , nsa3xx_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; +unsigned long mcu_data =nsa3xx_hwmon_update (dev); +unsigned long value =0 ; +
If nsa3xx_hwmon_update() returns an error code, you can use
if (IS_ERR_VALUE(mcu_data)) return (long)mcu_data;
or even better have nsa3xx_hwmon_update() return int directly to indicate an error.
drivers/hwmon/nsa3xx-hwmon.c line 131
Quoted 1 line
drivers/hwmon/nsa3xx-hwmon.c new file +1
-0+pr_debug ("channel value 0x%02x!\n" , channel);
Is this really useful ? I would suggest to drop debug messages unless really needed. If you really think the message is useful, please use dev_dbg().
Quoted 30 lines
drivers/hwmon/nsa3xx-hwmon.c new file +30
-0+switch (channel) { +case NSA3XX_TEMP : + value = (mcu_data &0xffff ) *100 ; +break ; +case NSA3XX_FAN : + value = ((mcu_data &0xff0000 ) >>16 ) *100 ; +break ; + } +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 *nsa3xx_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 (nsa3xx); + +static const struct of_device_id of_nsa3xx_hwmon_match[] = { + { .compatible ="zyxel,nsa320-mcu" , }, + { }, +};
drivers/hwmon/nsa3xx-hwmon.c line 171
Quoted 10 lines
drivers/hwmon/nsa3xx-hwmon.c new file +10
-0+ +static int nsa3xx_hwmon_probe (struct platform_device *pdev) +{ +struct nsa3xx_hwmon *hwmon; + + hwmon =devm_kzalloc (&pdev->dev,sizeof (*hwmon),GFP_KERNEL ); +if (!hwmon) +return -ENOMEM ; + +platform_set_drvdata (pdev, hwmon);
Unnecessary.
drivers/hwmon/nsa3xx-hwmon.c line 183
Quoted 12 lines
drivers/hwmon/nsa3xx-hwmon.c new file +12
-0+ +/* Look up the GPIO pins to use */ + hwmon->act =devm_gpiod_get (&pdev->dev,"act" ,GPIOD_OUT_LOW ); + hwmon->clk =devm_gpiod_get (&pdev->dev,"clk" ,GPIOD_OUT_HIGH ); + hwmon->data =devm_gpiod_get (&pdev->dev,"data" ,GPIOD_IN ); + +if (IS_ERR (hwmon->act)) +return PTR_ERR (hwmon->act); +if (IS_ERR (hwmon->clk)) +return PTR_ERR (hwmon->clk); +if (IS_ERR (hwmon->data)) +return PTR_ERR (hwmon->data);
Please reorder to have the error checks immediately after the calls to devm_gpiod_get().
drivers/hwmon/nsa3xx-hwmon.c line 188
Quoted 5 lines
drivers/hwmon/nsa3xx-hwmon.c new file +5
-0+ +mutex_init (&hwmon->update_lock); + + hwmon->classdev =devm_hwmon_device_register_with_groups (&pdev->dev, +"nsa3xx" , hwmon, nsa3xx_groups);
classdev is only used in this function and thus does not have to be kept in struct nsa3xx_hwmon.
drivers/hwmon/nsa3xx-hwmon.c line 193
Quoted 5 lines
drivers/hwmon/nsa3xx-hwmon.c new file +5
-0+if (IS_ERR (hwmon->classdev)) +return PTR_ERR (hwmon->classdev); + +dev_dbg (&pdev->dev,"initialized\n" ); +
This message is really unnecessary. Please just use
return PTR_ERR_OR_ZERO(classdev);
drivers/hwmon/nsa3xx-hwmon.c line 204
Quoted 11 lines
drivers/hwmon/nsa3xx-hwmon.c new file +11
-0+return 0 ; +} + +static int nsa3xx_hwmon_remove (struct platform_device *pdev) +{ +/* All resources are allocated with devm_*() so +* there is nothing to do here. +*/ + +return 0 ; +}
If the remove function is not needed, it is not needed and should not exist in the first place. Please no dummy functions.
Quoted 8 lines
drivers/hwmon/nsa3xx-hwmon.c new file +8
-0+ +static struct platform_driver nsa3xx_hwmon_driver = { + .probe = nsa3xx_hwmon_probe, + .remove = nsa3xx_hwmon_remove, + .driver = { + .name ="nsa3xx-hwmon" , + .owner =THIS_MODULE , + .of_match_table =of_match_ptr (of_nsa3xx_hwmon_match),
drivers/hwmon/nsa3xx-hwmon.c line 222
Quoted 10 lines
drivers/hwmon/nsa3xx-hwmon.c new file +10
-0+ }, +}; + +module_platform_driver (nsa3xx_hwmon_driver); + +MODULE_DEVICE_TABLE (of, of_nsa3xx_hwmon_match); +MODULE_AUTHOR ("Peter Schildmann <[email protected]>" ); +MODULE_AUTHOR ("Adam Baker <[email protected]>" ); +MODULE_DESCRIPTION ("NSA3XX Hardware Monitoring" ); +MODULE_LICENSE ("GPL" );
GPL v2
Quoted 4 lines
drivers/hwmon/nsa3xx-hwmon.c new file +1
-0+MODULE_ALIAS ("platform:nsa3xx-hwmon" );Signature · 1 line
2.1.4
PATCH 2/3 ARM: mvebu: Create a NSA3xx hardware monitoring driver
Quoted 11 lines · Guenter Roeck
Hi,
Quoted 7 lines · Adam Baker
Create a driver to support the hardware monitoring chip present in several models of Zyxel NSA3xx NAS devices.
The driver reads fan speed and temperature from a suitably programmed MCU on the device.
Signed-off-by Adam Baker <[email protected]>
Quoted 8 lines
Please name the driver nsa320. It may ultimately support other devices, but that doesn't mean it is going to support NSA-35x or other future similar devices.
Got the headline, please use
hwmon: Add driver for Zyxel NSA-320
OK
Quoted 18 lines
Quoted 18 lines
---
I've tested this on an NSA-320, I've had someone offer to test it on NSA-325 so hopefully I will get a tested by back.--- drivers/hwmon/Kconfig | 13 +++ drivers/hwmon/Makefile | 1 + drivers/hwmon/nsa3xx-hwmon.c | 223 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 237 insertions(+)create mode 100644
drivers/hwmon/nsa3xx-hwmon.cdrivers/hwmon/Kconfig +0
-0@@ -1757,6 +1757,19 @@ config SENSORS_ULTRA45 This driver provides support for the Ultra45 workstation environmental sensors.
Quoted 3 lines
drivers/hwmon/Kconfig config SENSORS_ULTRA45 · line 1760
Quoted 1 line
drivers/hwmon/Kconfig +1
-0+config SENSORS_NSA3XXPlease try to stick with alphabetic order.
done
Quoted 4 lines
drivers/hwmon/Kconfig config SENSORS_ULTRA45 · line 1761
Quoted 2 lines
drivers/hwmon/Kconfig +1
-0+ tristate "ZyXEL NSA3xx fan speed and temperature sensors"... NSA-320 and compatible ...
done
Quoted 11 lines
drivers/hwmon/Kconfig config SENSORS_ULTRA45 · line 1762
Quoted 2 lines
drivers/hwmon/Kconfig +1
-0+ depends on GPIOLIB && OFCan you add some additional dependencies ?
It is quite unlikely that the driver is needed on an X86. Please use at least
depends on ARM || COMPILE_TESTor a more specific dependency if the SoC is known.
MACH_KIRKWOOD, maybe ?
done - depends on MACH_KIRKWOOD || COMPILE_TEST
Quoted 8 lines
drivers/hwmon/Kconfig config SENSORS_ULTRA45 · line 1765
Quoted 4 lines
drivers/hwmon/Kconfig +3
-0+ help + If you say yes here you get support for hardware monitoring + for the ZyXEL NSA3XX Media Servers.Please be specific which devices are supported. Feel free to add "and compatibles", but please no generic and misleading statements such as 3XX.
Changed to NSA320 and compatible and noted NSA325 and some NSA310 variants are probably compatible
Quoted 6 lines
drivers/hwmon/Kconfig config SENSORS_ULTRA45 · line 1769
Quoted 4 lines
drivers/hwmon/Kconfig +4
-0+ + The sensor data is taken from a Holtek HT46R065 microcontroller + connected to GPIO lines. +Is that relevant ?
I'm guessing that it is if someone is trying to work out if the unit they've got is likely to be compatible.
Quoted 5 lines
Quoted 5 lines
drivers/hwmon/Kconfig +3
-0+ This driver can also be built as a module. If so, the module + will be called nsa3xx-hwmon. + if ACPI
Quoted 12 lines
drivers/hwmon/Makefile obj-$(CONFIG_SENSORS_MAX6650) += max6650.o · line 121
Quoted 10 lines
drivers/hwmon/Kconfig +0
-0comment "ACPI drivers"drivers/hwmon/Makefile +1
-0@@ -118,6 +118,7 @@ obj-$(CONFIG_SENSORS_MAX6650) += max6650.o obj-$(CONFIG_SENSORS_MAX6697) += max6697.o obj-$(CONFIG_SENSORS_MAX31790) += max31790.o obj-$(CONFIG_SENSORS_MC13783_ADC)+= mc13783-adc.o +obj-$(CONFIG_SENSORS_NSA3XX) += nsa3xx-hwmon.oPlease try to stick with alphabetic order.
done
Quoted 11 lines
drivers/hwmon/nsa3xx-hwmon.c
Quoted 9 lines
drivers/hwmon/Makefile +0
-0obj-$(CONFIG_SENSORS_MCP3021) += mcp3021.o obj-$(CONFIG_SENSORS_MENF21BMC_HWMON) += menf21bmc_hwmon.o obj-$(CONFIG_SENSORS_NCT6683) += nct6683.odrivers/hwmon/nsa3xx-hwmon.c new file +0
-0
nsa320-hwmon.c
done
Quoted 8 lines
drivers/hwmon/nsa3xx-hwmon.c line 4
Quoted 6 lines
drivers/hwmon/nsa3xx-hwmon.c new file +4
-0@@ -0,0 +1,223 @@ +/* +* drivers/hwmon/nsa3xx-hwmon.c +* +* ZyXEL NSA3xx Media Servers Please be specific.
done
Quoted 23 lines
Quoted 23 lines
drivers/hwmon/nsa3xx-hwmon.c new file +22
-0+* 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/module.h> +#include <linux/mutex.h> +#include <linux/platform_device.h> +#include <linux/err.h> +#include <linux/gpio/consumer.h> +#include <linux/hwmon.h>
Quoted 8 lines
drivers/hwmon/nsa3xx-hwmon.c line 33
Quoted 7 lines
drivers/hwmon/nsa3xx-hwmon.c new file +7
-0+#include <linux/hwmon-sysfs.h> +#include <linux/jiffies.h> +#include <linux/delay.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/of_platform.h> +Please order include files alphabetically.
done
Quoted 16 lines
Quoted 4 lines
drivers/hwmon/nsa3xx-hwmon.c new file +3
-0+#define MAGIC_NUMBER 0x55 + +/* drivers/hwmon/nsa3xx-hwmon.c line 46
Quoted 10 lines
drivers/hwmon/nsa3xx-hwmon.c new file +10
-0+* 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 nsa3xx_hwmon {s/nsa3xx/nsa320/g for the entire file, please.
done
Quoted 8 lines
drivers/hwmon/nsa3xx-hwmon.c line 50
Quoted 5 lines
drivers/hwmon/nsa3xx-hwmon.c new file +4
-0+struct device *classdev; +struct mutex update_lock;/* lock GPIO operations */ +unsigned long last_updated;/* jiffies */ +u32 mcu_data;I would suggest to use unsigned long here. See below for reasons.
I've changed the return value to int
Quoted 10 lines
drivers/hwmon/nsa3xx-hwmon.c line 58
Quoted 8 lines
drivers/hwmon/nsa3xx-hwmon.c new file +8
-0+struct gpio_desc *act; +struct gpio_desc *clk; +struct gpio_desc *data; +}; + +enum nsa3xx_inputs { +NSA3XX_FAN =1 , +NSA3XX_TEMP =0 ,Any special reason for the unusual order ? If yes, please explain.
No, changed
Quoted 25 lines
Quoted 25 lines
drivers/hwmon/nsa3xx-hwmon.c new file +24
-0+}; + +static const char *const nsa3xx_input_names[] = { + [NSA3XX_FAN ] ="Chassis Fan" , + [NSA3XX_TEMP ] ="System Temperature" , +}; + +/* Although this protocol looks similar to SPI the long delay +* 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 unsigned long nsa3xx_hwmon_update (struct device *dev) +{ +u32 mcu_data; +u32 mask; +struct nsa3xx_hwmon *hwmon =dev_get_drvdata (dev); + +mutex_lock (&hwmon->update_lock); + + mcu_data = hwmon->mcu_data;
Quoted 8 lines
drivers/hwmon/nsa3xx-hwmon.c line 86
Quoted 4 lines
drivers/hwmon/nsa3xx-hwmon.c new file +4
-0+ +if (time_after (jiffies, hwmon->last_updated +HZ ) || + mcu_data ==0 ) { +Please no blank line here. Also, please align continuation lines with '('. However, unless I am missing something, the continuation line is not needed in the first place. Please no unnecessary continuation lines.
done
Quoted 8 lines
drivers/hwmon/nsa3xx-hwmon.c line 90
Quoted 5 lines
drivers/hwmon/nsa3xx-hwmon.c new file +4
-0+gpiod_set_value (hwmon->act,1 ); +msleep (100 ); + +for (mask =BIT (31 ); mask; mask >>=1 ) {Since you are using
BIT(), please includelinux/bitops.h.
done
Quoted 18 lines
Quoted 4 lines
drivers/hwmon/nsa3xx-hwmon.c new file +4
-0+gpiod_set_value (hwmon->clk,0 ); +usleep_range (100 ,200 ); +gpiod_set_value (hwmon->clk,1 ); +usleep_range (100 ,200 );drivers/hwmon/nsa3xx-hwmon.c line 104
Quoted 10 lines
drivers/hwmon/nsa3xx-hwmon.c new file +10
-0+if (gpiod_get_value (hwmon->data)) + mcu_data |= mask; + } + +gpiod_set_value (hwmon->act,0 ); +dev_dbg (dev,"Read raw MCU data %08x\n" , mcu_data); + +if ((mcu_data >>24 ) !=MAGIC_NUMBER ) { +dev_err (dev,"Read invalid MCU data %08x\n" , mcu_data); + mcu_data =0 ;Instead of an error message, it would be better to return an error code (probably
-EIOsince we don't know better).
Quoted 1 line
The calling code can then return the error to user space.
nsa320_hwmon_update() now returns an int, either 0 or -EIO and the caller can read the data from the nsa320_hwmon structure if it returns
0. dev_err changed to dev_dbg as the raw data is helpful for debugging.
Quoted 30 lines
Quoted 17 lines
drivers/hwmon/nsa3xx-hwmon.c new file +16
-0+ } + + hwmon->mcu_data = mcu_data; + hwmon->last_updated = jiffies; + } + +mutex_unlock (&hwmon->update_lock); + +return mcu_data; +} + +static ssize_t show_label (struct device *dev, +struct device_attribute *attr,char *buf) +{ +int channel =to_sensor_dev_attr (attr)->index; +drivers/hwmon/nsa3xx-hwmon.c line 130
Quoted 10 lines
drivers/hwmon/nsa3xx-hwmon.c new file +10
-0+return sprintf (buf,"%s\n" , nsa3xx_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; +unsigned long mcu_data =nsa3xx_hwmon_update (dev); +unsigned long value =0 ; +If
nsa3xx_hwmon_update()returns an error code, you can use
Quoted 5 lines
if (IS_ERR_VALUE(mcu_data)) return (long)mcu_data;or even better have
nsa3xx_hwmon_update()return int directly to indicate an error.
nsa320_hwmon_update() now returns an int error code or zero and the caller gets the data fro the struct if it succeeds
Quoted 6 lines
drivers/hwmon/nsa3xx-hwmon.c line 131
Quoted 2 lines
drivers/hwmon/nsa3xx-hwmon.c new file +1
-0+pr_debug ("channel value 0x%02x!\n" , channel);Is this really useful ? I would suggest to drop debug messages unless really needed. If you really think the message is useful, please use
dev_dbg().
deleted
Quoted 35 lines
Quoted 35 lines
drivers/hwmon/nsa3xx-hwmon.c new file +34
-0+switch (channel) { +case NSA3XX_TEMP : + value = (mcu_data &0xffff ) *100 ; +break ; +case NSA3XX_FAN : + value = ((mcu_data &0xff0000 ) >>16 ) *100 ; +break ; + } +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 *nsa3xx_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 (nsa3xx); + +static const struct of_device_id of_nsa3xx_hwmon_match[] = { + { .compatible ="zyxel,nsa320-mcu" , }, + { }, +}; + +static int nsa3xx_hwmon_probe (struct platform_device *pdev) +{ +struct nsa3xx_hwmon *hwmon;
Quoted 8 lines
drivers/hwmon/nsa3xx-hwmon.c line 171
Quoted 6 lines
drivers/hwmon/nsa3xx-hwmon.c new file +6
-0+ + hwmon =devm_kzalloc (&pdev->dev,sizeof (*hwmon),GFP_KERNEL ); +if (!hwmon) +return -ENOMEM ; + +platform_set_drvdata (pdev, hwmon);Unnecessary.
removed
Quoted 16 lines
drivers/hwmon/nsa3xx-hwmon.c line 183
Quoted 13 lines
drivers/hwmon/nsa3xx-hwmon.c new file +12
-0+ +/* Look up the GPIO pins to use */ + hwmon->act =devm_gpiod_get (&pdev->dev,"act" ,GPIOD_OUT_LOW ); + hwmon->clk =devm_gpiod_get (&pdev->dev,"clk" ,GPIOD_OUT_HIGH ); + hwmon->data =devm_gpiod_get (&pdev->dev,"data" ,GPIOD_IN ); + +if (IS_ERR (hwmon->act)) +return PTR_ERR (hwmon->act); +if (IS_ERR (hwmon->clk)) +return PTR_ERR (hwmon->clk); +if (IS_ERR (hwmon->data)) +return PTR_ERR (hwmon->data);Please reorder to have the error checks immediately after the calls to
devm_gpiod_get().
done
Quoted 9 lines
drivers/hwmon/nsa3xx-hwmon.c line 188
Quoted 6 lines
drivers/hwmon/nsa3xx-hwmon.c new file +5
-0+ +mutex_init (&hwmon->update_lock); + + hwmon->classdev =devm_hwmon_device_register_with_groups (&pdev->dev, +"nsa3xx" , hwmon, nsa3xx_groups);classdev is only used in this function and thus does not have to be kept in struct
nsa3xx_hwmon.
removed
Quoted 9 lines
drivers/hwmon/nsa3xx-hwmon.c line 193
Quoted 6 lines
drivers/hwmon/nsa3xx-hwmon.c new file +5
-0+if (IS_ERR (hwmon->classdev)) +return PTR_ERR (hwmon->classdev); + +dev_dbg (&pdev->dev,"initialized\n" ); +This message is really unnecessary. Please just use
return PTR_ERR_OR_ZERO(classdev);
done
Quoted 16 lines
drivers/hwmon/nsa3xx-hwmon.c line 204
Quoted 12 lines
drivers/hwmon/nsa3xx-hwmon.c new file +11
-0+return 0 ; +} + +static int nsa3xx_hwmon_remove (struct platform_device *pdev) +{ +/* All resources are allocated with devm_*() so +* there is nothing to do here. +*/ + +return 0 ; +}If the remove function is not needed, it is not needed and should not exist in the first place. Please no dummy functions.
done, I've left a comment to explain why there is no remove to prompt anyone who changes the code to use devres or add a remove method
Quoted 12 lines
Quoted 12 lines
drivers/hwmon/nsa3xx-hwmon.c new file +12
-0+ +static struct platform_driver nsa3xx_hwmon_driver = { + .probe = nsa3xx_hwmon_probe, + .remove = nsa3xx_hwmon_remove, + .driver = { + .name ="nsa3xx-hwmon" , + .owner =THIS_MODULE , + .of_match_table =of_match_ptr (of_nsa3xx_hwmon_match), + }, +}; + +module_platform_driver (nsa3xx_hwmon_driver);
Quoted 8 lines
drivers/hwmon/nsa3xx-hwmon.c line 222
Quoted 6 lines
drivers/hwmon/nsa3xx-hwmon.c new file +6
-0+ +MODULE_DEVICE_TABLE (of, of_nsa3xx_hwmon_match); +MODULE_AUTHOR ("Peter Schildmann <[email protected]>" ); +MODULE_AUTHOR ("Adam Baker <[email protected]>" ); +MODULE_DESCRIPTION ("NSA3XX Hardware Monitoring" ); +MODULE_LICENSE ("GPL" );GPL v2
done
Quoted 5 lines
Quoted 5 lines
drivers/hwmon/nsa3xx-hwmon.c new file +1
-1+MODULE_ALIAS ("platform:nsa3xx-hwmon" ); --2.1.4
PATCH 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
cover ARM: mvebu: Add driver for the Zyxel NSA3XX hwmon MCU
In reply to Adam BakerHi Adam,
Quoted 10 lines · Adam Baker
This patch series adds a driver to support the hardware monitoring MCU that is present in some models of the Zyxel NSA3xx series of NAS units.
A patch is included to add it to the device tree for the NSA320 model, patches to the other device trees will be included if testers come forward (I already have a possible tester for NSA325).
There are no dependencies between the patches so I guess the first two should go via the hardware monitoring maintainers and the third via the ARM mvebu maintainers.
You are right. However the first tow patch are misnamed, they better should be called:
hwmn: Define binding for the nsa3xx-hwmon driver
hwmn: Create a driver to support the hardware monitoring chip
The last patch seems OK for me, and the binding has been approved. So when you will get a review from the hwmon maintainer, I will apply the last patch in the mvebu tree.
Thanks,
Gregory
Quoted 12 lines
[PATCH 1/3] Define a binding for the hardware monitoring chip present [PATCH 2/3] Create a driver to support the hardware monitoring chip [PATCH 3/3] Add an entry for the hardware monitoring MCU
Documentation/devicetree/bindings/hwmon/nsa320-mcu.txt | 21 ++++++++ arch/arm/boot/dts/kirkwood-nsa320.dts | 13 ++++- drivers/hwmon/Kconfig | 13 +++++ drivers/hwmon/Makefile | 1 + drivers/hwmon/nsa3xx-hwmon.c | 223 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 269 insertions(+), 2 deletions(-)
Signature · 4 lines
Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com