[PATCH v3 0/3] driver for the hardware monitoring chip in the Zyxel NSA320

[PATCH v3 0/3] driver for the hardware monitoring chip in the Zyxel NSA320

[Adam Baker]

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 +++
 Documentation/hwmon/nsa320                             |  53 +++++++
 arch/arm/boot/dts/kirkwood-nsa320.dts                  |  13 +-
 drivers/hwmon/Kconfig                                  |  15 ++
 drivers/hwmon/Makefile                                 |   1 +
 drivers/hwmon/nsa320-hwmon.c                           | 215 ++++++++++++++++++++++++++++
 6 files changed, 316 insertions(+), 2 deletions(-)

---
Changes since v1
Update to reflect linux-0h96xk9xTtoC3MgbdEUNLxnkhkLA8VUH@public.gmane.org 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.

Changes since v2
Update patch 2/3 based on further review from Guenter Roeck, patches
1/3 and 3/3 are unchanged except for adding Rob's Ack to 1/3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 1/3] hwmon: Define binding for the nsa320-hwmon driver

[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 <linux-u8Lxuz9p/S1csCcyGdaD/Q@public.gmane.org>
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
Changes for v3, Add Robs Ack
---
 .../devicetree/bindings/hwmon/nsa320-mcu.txt        | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/nsa320-mcu.txt

diff --git a/Documentation/devicetree/bindings/hwmon/nsa320-mcu.txt b/Documentation/devicetree/bindings/hwmon/nsa320-mcu.txt
new file mode 100644
index 0000000..7b346d4
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/nsa320-mcu.txt
@@ -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>;
+	};
+
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/3] hwmon: Create an NSA320 hardware monitoring driver

[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 <linux-u8Lxuz9p/S1csCcyGdaD/Q@public.gmane.org>
---
Changes since v2
Updated based on Guenter Roeck's review,
- return a single value that is result or error from the update function
- use separate show_value routimes for fan and temp
- Added Documentation/hwmon/nsa320-hwmon
---
 Documentation/hwmon/nsa320   |  53 +++++++++++
 drivers/hwmon/Kconfig        |  15 +++
 drivers/hwmon/Makefile       |   1 +
 drivers/hwmon/nsa320-hwmon.c | 215 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 284 insertions(+)
 create mode 100644 Documentation/hwmon/nsa320
 create mode 100644 drivers/hwmon/nsa320-hwmon.c

diff --git a/Documentation/hwmon/nsa320 b/Documentation/hwmon/nsa320
new file mode 100644
index 0000000..fdbd694
--- /dev/null
+++ b/Documentation/hwmon/nsa320
@@ -0,0 +1,53 @@
+Kernel driver nsa320_hwmon
+==========================
+
+Supported chips:
+  * Holtek HT46R065 microcontroller with onboard firmware that configures
+	it to act as a hardware monitor.
+    Prefix: 'nsa320'
+    Addresses scanned: none
+    Datasheet: Not available, driver was reverse engineered based upon the
+	Zyxel kernel source
+
+Author:
+  Adam Baker <linux-u8Lxuz9p/S1csCcyGdaD/Q@public.gmane.org>
+
+Description
+-----------
+
+This chip is known to be used in the Zyxel NSA320 and NSA325 NAS Units and
+also in some variants of the NSA310 but the driver has only been tested
+on the NSA320. In all of these devices it is connected to the same 3 GPIO
+lines which are used to provide chip select, clock and data lines. The
+interface behaves similarly to SPI but at much lower speeds than are normally
+used for SPI.
+
+Following each chip select pulse the chip will generate a single 32 bit word
+that contains 0x55 as a marker to indicate that data is being read correctly,
+followed by an 8 bit fan speed in 100s of RPM and a 16 bit temperature in
+tenths of a degree.
+
+
+sysfs-Interface
+---------------
+
+temp1_input - temperature input
+fan1_input - fan speed
+
+Notes
+-----
+
+The access timings used in the driver are the same as used in the Zyxel
+provided kernel. Testing has shown that if the delay between chip select and
+the first clock pulse is reduced from 100 ms to just under 10ms then the chip
+will not produce any output. If the duration of either phase of the clock
+is reduced from 100 us to less than 15 us then data pulses are likely to be
+read twice corrupting the output. The above analysis is based upon a sample
+of one unit but suggests that the Zyxel provided delay values include a
+reasonable tolerance.
+
+The driver incorporates a limit that it will not check for updated values
+faster than once a second. This is because the hardware takes a relatively long
+time to read the data from the device and when it does it reads both temp and
+fan speed. As the most likely case for two accesses in quick succession is
+to read both of these values avoiding a second read delay is desirable.
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 80a73bf..08fd7f5 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -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
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 12a3239..9e22cf7 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -124,6 +124,7 @@ obj-$(CONFIG_SENSORS_NCT6683)	+= nct6683.o
 obj-$(CONFIG_SENSORS_NCT6775)	+= nct6775.o
 obj-$(CONFIG_SENSORS_NCT7802)	+= nct7802.o
 obj-$(CONFIG_SENSORS_NCT7904)	+= nct7904.o
+obj-$(CONFIG_SENSORS_NSA320)	+= nsa320-hwmon.o
 obj-$(CONFIG_SENSORS_NTC_THERMISTOR)	+= ntc_thermistor.o
 obj-$(CONFIG_SENSORS_PC87360)	+= pc87360.o
 obj-$(CONFIG_SENSORS_PC87427)	+= pc87427.o
diff --git a/drivers/hwmon/nsa320-hwmon.c b/drivers/hwmon/nsa320-hwmon.c
new file mode 100644
index 0000000..e833550
--- /dev/null
+++ b/drivers/hwmon/nsa320-hwmon.c
@@ -0,0 +1,215 @@
+/*
+ * drivers/hwmon/nsa320-hwmon.c
+ *
+ * ZyXEL NSA320 Media Servers
+ * hardware monitoring
+ *
+ * Copyright (C) 2016 Adam Baker <linux-u8Lxuz9p/S1csCcyGdaD/Q@public.gmane.org>
+ * based on a board file driver
+ * Copyright (C) 2012 Peter Schildmann <linux-7ojh7neMhKcWe5+p9BQgIQ@public.gmane.org>
+ *
+ * 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 */
+	unsigned long		mcu_data;
+	struct gpio_desc	*act;
+	struct gpio_desc	*clk;
+	struct gpio_desc	*data;
+};
+
+enum nsa320_inputs {
+	NSA3XX_TEMP = 0,
+	NSA3XX_FAN = 1,
+};
+
+static const char * const nsa320_input_names[] = {
+	[NSA3XX_TEMP] = "System Temperature",
+	[NSA3XX_FAN] = "Chassis Fan",
+};
+
+/*
+ * 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 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);
+
+		mcu_data = 0;
+		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;
+		}
+
+		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);
+			mcu_data = -EIO;
+		} else {
+			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;
+
+	return sprintf(buf, "%s\n", nsa320_input_names[channel]);
+}
+
+static ssize_t show_temp(struct device *dev, struct device_attribute *attr,
+			 char *buf)
+{
+	unsigned long mcu_data = nsa320_hwmon_update(dev);
+
+	if (IS_ERR_VALUE(mcu_data))
+		return mcu_data;
+
+	return sprintf(buf, "%lu\n", (mcu_data & 0xffff) * 100);
+}
+
+static ssize_t show_fan(struct device *dev, struct device_attribute *attr,
+			char *buf)
+{
+	unsigned long mcu_data = nsa320_hwmon_update(dev);
+
+	if (IS_ERR_VALUE(mcu_data))
+		return mcu_data;
+
+	return sprintf(buf, "%lu\n", ((mcu_data & 0xff0000) >> 16) * 100);
+}
+
+static SENSOR_DEVICE_ATTR(temp1_label, S_IRUGO, show_label, NULL, NSA3XX_TEMP);
+static DEVICE_ATTR(temp1_input, S_IRUGO, show_temp, NULL);
+static SENSOR_DEVICE_ATTR(fan1_label, S_IRUGO, show_label, NULL, NSA3XX_FAN);
+static DEVICE_ATTR(fan1_input, S_IRUGO, show_fan, NULL);
+
+static struct attribute *nsa320_attrs[] = {
+	&sensor_dev_attr_temp1_label.dev_attr.attr,
+	&dev_attr_temp1_input.attr,
+	&sensor_dev_attr_fan1_label.dev_attr.attr,
+	&dev_attr_fan1_input.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 <linux-7ojh7neMhKcWe5+p9BQgIQ@public.gmane.org>");
+MODULE_AUTHOR("Adam Baker <linux-u8Lxuz9p/S1csCcyGdaD/Q@public.gmane.org>");
+MODULE_DESCRIPTION("NSA3XX Hardware Monitoring");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:nsa320-hwmon");
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 3/3] ARM: mvebu: Add the hardware monitor to the NSA320 device tree

[Adam Baker]

Add an entry for the hardware monitoring MCU

Signed-off-by: Adam Baker <linux-u8Lxuz9p/S1csCcyGdaD/Q@public.gmane.org>
---
This patch is unchanged from the version Gregory CLEMENT said
he would apply once the hwmon maintainers accept 2/2
---
 arch/arm/boot/dts/kirkwood-nsa320.dts | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/kirkwood-nsa320.dts b/arch/arm/boot/dts/kirkwood-nsa320.dts
index 24f686d..2a935e9 100644
--- a/arch/arm/boot/dts/kirkwood-nsa320.dts
+++ b/arch/arm/boot/dts/kirkwood-nsa320.dts
@@ -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>; */
 };
 
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/3] hwmon: Create an NSA320 hardware monitoring driver

[Guenter Roeck]

On 02/28/2016 02:30 PM, Adam Baker wrote:
> 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 <linux-u8Lxuz9p/S1csCcyGdaD/Q@public.gmane.org>
> ---
> Changes since v2
> Updated based on Guenter Roeck's review,
> - return a single value that is result or error from the update function
> - use separate show_value routimes for fan and temp
> - Added Documentation/hwmon/nsa320-hwmon
> ---
>   Documentation/hwmon/nsa320   |  53 +++++++++++
>   drivers/hwmon/Kconfig        |  15 +++
>   drivers/hwmon/Makefile       |   1 +
>   drivers/hwmon/nsa320-hwmon.c | 215 +++++++++++++++++++++++++++++++++++++++++++
>   4 files changed, 284 insertions(+)
>   create mode 100644 Documentation/hwmon/nsa320
>   create mode 100644 drivers/hwmon/nsa320-hwmon.c
>
> diff --git a/Documentation/hwmon/nsa320 b/Documentation/hwmon/nsa320
> new file mode 100644
> index 0000000..fdbd694
> --- /dev/null
> +++ b/Documentation/hwmon/nsa320
> @@ -0,0 +1,53 @@
> +Kernel driver nsa320_hwmon
> +==========================
> +
> +Supported chips:
> +  * Holtek HT46R065 microcontroller with onboard firmware that configures
> +	it to act as a hardware monitor.
> +    Prefix: 'nsa320'
> +    Addresses scanned: none
> +    Datasheet: Not available, driver was reverse engineered based upon the
> +	Zyxel kernel source
> +
> +Author:
> +  Adam Baker <linux-u8Lxuz9p/S1csCcyGdaD/Q@public.gmane.org>
> +
> +Description
> +-----------
> +
> +This chip is known to be used in the Zyxel NSA320 and NSA325 NAS Units and
> +also in some variants of the NSA310 but the driver has only been tested
> +on the NSA320. In all of these devices it is connected to the same 3 GPIO
> +lines which are used to provide chip select, clock and data lines. The
> +interface behaves similarly to SPI but at much lower speeds than are normally
> +used for SPI.
> +
> +Following each chip select pulse the chip will generate a single 32 bit word
> +that contains 0x55 as a marker to indicate that data is being read correctly,
> +followed by an 8 bit fan speed in 100s of RPM and a 16 bit temperature in
> +tenths of a degree.
> +
> +
> +sysfs-Interface
> +---------------
> +
> +temp1_input - temperature input
> +fan1_input - fan speed
> +
> +Notes
> +-----
> +
> +The access timings used in the driver are the same as used in the Zyxel
> +provided kernel. Testing has shown that if the delay between chip select and
> +the first clock pulse is reduced from 100 ms to just under 10ms then the chip
> +will not produce any output. If the duration of either phase of the clock
> +is reduced from 100 us to less than 15 us then data pulses are likely to be
> +read twice corrupting the output. The above analysis is based upon a sample
> +of one unit but suggests that the Zyxel provided delay values include a
> +reasonable tolerance.
> +
> +The driver incorporates a limit that it will not check for updated values
> +faster than once a second. This is because the hardware takes a relatively long
> +time to read the data from the device and when it does it reads both temp and
> +fan speed. As the most likely case for two accesses in quick succession is
> +to read both of these values avoiding a second read delay is desirable.
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 80a73bf..08fd7f5 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -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
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 12a3239..9e22cf7 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -124,6 +124,7 @@ obj-$(CONFIG_SENSORS_NCT6683)	+= nct6683.o
>   obj-$(CONFIG_SENSORS_NCT6775)	+= nct6775.o
>   obj-$(CONFIG_SENSORS_NCT7802)	+= nct7802.o
>   obj-$(CONFIG_SENSORS_NCT7904)	+= nct7904.o
> +obj-$(CONFIG_SENSORS_NSA320)	+= nsa320-hwmon.o
>   obj-$(CONFIG_SENSORS_NTC_THERMISTOR)	+= ntc_thermistor.o
>   obj-$(CONFIG_SENSORS_PC87360)	+= pc87360.o
>   obj-$(CONFIG_SENSORS_PC87427)	+= pc87427.o
> diff --git a/drivers/hwmon/nsa320-hwmon.c b/drivers/hwmon/nsa320-hwmon.c
> new file mode 100644
> index 0000000..e833550
> --- /dev/null
> +++ b/drivers/hwmon/nsa320-hwmon.c
> @@ -0,0 +1,215 @@
> +/*
> + * drivers/hwmon/nsa320-hwmon.c
> + *
> + * ZyXEL NSA320 Media Servers
> + * hardware monitoring
> + *
> + * Copyright (C) 2016 Adam Baker <linux-u8Lxuz9p/S1csCcyGdaD/Q@public.gmane.org>
> + * based on a board file driver
> + * Copyright (C) 2012 Peter Schildmann <linux-7ojh7neMhKcWe5+p9BQgIQ@public.gmane.org>
> + *
> + * 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 */
> +	unsigned long		mcu_data;
> +	struct gpio_desc	*act;
> +	struct gpio_desc	*clk;
> +	struct gpio_desc	*data;
> +};
> +
> +enum nsa320_inputs {
> +	NSA3XX_TEMP = 0,
> +	NSA3XX_FAN = 1,
> +};
> +
> +static const char * const nsa320_input_names[] = {
> +	[NSA3XX_TEMP] = "System Temperature",
> +	[NSA3XX_FAN] = "Chassis Fan",
> +};
> +
> +/*
> + * 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 nsa320_hwmon_update(struct device *dev)

Please make this either int or s32.

> +{
> +	u32 mcu_data;

You can (and should) still use u32 here.

Thanks,
Guenter

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/3] hwmon: Create an NSA320 hardware monitoring driver

[Adam Baker]

On 29/02/16 00:45, Guenter Roeck wrote:

>> +
>> +/*
>> + * 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 nsa320_hwmon_update(struct device *dev)
>
> Please make this either int or s32.
>
>> +{
>> +    u32 mcu_data;
>
> You can (and should) still use u32 here.
>


I'm a bit puzzled by your reasoning for preferring a signed value for 
the return value. The only reason I can think of is that the error 
return value is negative, however the macro to test if it is an error 
value is not looking at whether the value is negative

#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)

Functionally using any of s32, int or unsigned long will make no 
difference, the mask operations ensure that the top bit can't possibly 
be set so using a signed type doesn't, in this circumstance, risk an 
undefined result from the shift operation. The choice of type is 
therefore a matter of readability rather than function so if you still 
prefer s32 I'll respin it to use that.

The length does need to be at least 32 bits (to represent the returned 
data range) and no longer than unsigned long (to avoid breaking 
IS_ERR_VALUE). Whilst int is 32 bits on the only processor that is 
expected to use this device that doesn't feel like a good thing to 
depend upon so s32 seems preferable to int.

Regards

Adam
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/3] hwmon: Create an NSA320 hardware monitoring driver

[Guenter Roeck]

On 03/02/2016 03:43 PM, Adam Baker wrote:
> On 29/02/16 00:45, Guenter Roeck wrote:
>
>>> +
>>> +/*
>>> + * 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 nsa320_hwmon_update(struct device *dev)
>>
>> Please make this either int or s32.
>>
>>> +{
>>> +    u32 mcu_data;
>>
>> You can (and should) still use u32 here.
>>
>
>
> I'm a bit puzzled by your reasoning for preferring a signed value for the return value. The only reason I can think of is that the error return value is negative, however the macro to test if it is an error value is not looking at whether the value is negative
>
> #define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
>
> Functionally using any of s32, int or unsigned long will make no difference, the mask operations ensure that the top bit can't possibly be set so using a signed type doesn't, in this circumstance, risk an undefined result from the shift operation. The choice of type is therefore a matter of readability rather than function so if you still prefer s32 I'll respin it to use that.
>

IS_ERR_VALUE _only_ works with unsigned long if sizeof(long) = sizeof(int).
Someone may at some point in the future not realize this and change the function
return value to unsigned int or u32, thinking that an unsigned long is unnecessary,
and introduce a bug. Many users of IS_ERR_VALUE in the kernel actually
suffer from this problem. I prefer code which is less prone to such
unintentionally introduced problems.

I also dislike using unsigned variables to return negative error codes unless
there is a really good reason for doing it. Such a reason does not exist here.

Guenter

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html