* [PATCH 0/3] ARM: mvebu: Add driver for the Zyxel NSA3XX hwmon MCU
@ 2016-02-10 23:33 ` Adam Baker
0 siblings, 0 replies; 16+ messages in thread
From: Adam Baker @ 2016-02-10 23:33 UTC (permalink / raw)
To: Jason Cooper, Andrew Lunn, Gregory Clement,
Sebastian Hesselbarth, Jean Delvare, Guenter Roeck
Cc: Mark Rutland, devicetree, Russell King, Pawel Moll, Ian Campbell,
lm-sensors, Rob Herring, Kumar Gala, linux-arm-kernel
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(-)
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 0/3] ARM: mvebu: Add driver for the Zyxel NSA3XX hwmon MCU
@ 2016-02-10 23:33 ` Adam Baker
0 siblings, 0 replies; 16+ messages in thread
From: Adam Baker @ 2016-02-10 23:33 UTC (permalink / raw)
To: linux-arm-kernel
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(-)
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/3] ARM: mvebu: Define binding for the nsa3xx-hwmon driver
2016-02-10 23:33 ` Adam Baker
@ 2016-02-10 23:33 ` Adam Baker
-1 siblings, 0 replies; 16+ messages in thread
From: Adam Baker @ 2016-02-10 23:33 UTC (permalink / raw)
To: Jason Cooper, Andrew Lunn, Gregory Clement,
Sebastian Hesselbarth, Jean Delvare, Guenter Roeck
Cc: Mark Rutland, devicetree, Russell King, Pawel Moll, Ian Campbell,
Adam Baker, lm-sensors, Rob Herring, Kumar Gala,
linux-arm-kernel
Define a binding for the hardware monitoring chip present in several
variants of the Zyxel NSA3xx NAS devices.
Signed-off-by: Adam Baker <linux@baker-net.org.uk>
---
.../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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 1/3] ARM: mvebu: Define binding for the nsa3xx-hwmon driver
@ 2016-02-10 23:33 ` Adam Baker
0 siblings, 0 replies; 16+ messages in thread
From: Adam Baker @ 2016-02-10 23:33 UTC (permalink / raw)
To: linux-arm-kernel
Define a binding for the hardware monitoring chip present in several
variants of the Zyxel NSA3xx NAS devices.
Signed-off-by: Adam Baker <linux@baker-net.org.uk>
---
.../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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/3] ARM: mvebu: Create a NSA3xx hardware monitoring driver
2016-02-10 23:33 ` Adam Baker
@ 2016-02-10 23:33 ` Adam Baker
-1 siblings, 0 replies; 16+ messages in thread
From: Adam Baker @ 2016-02-10 23:33 UTC (permalink / raw)
To: Jason Cooper, Andrew Lunn, Gregory Clement,
Sebastian Hesselbarth, Jean Delvare, Guenter Roeck
Cc: Mark Rutland, devicetree, Russell King, Pawel Moll, Ian Campbell,
Adam Baker, lm-sensors, Rob Herring, Kumar Gala,
linux-arm-kernel
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 <linux@baker-net.org.uk>
---
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
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 80a73bf..8801b78 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -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"
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 12a3239..e85414a 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -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
diff --git a/drivers/hwmon/nsa3xx-hwmon.c b/drivers/hwmon/nsa3xx-hwmon.c
new file mode 100644
index 0000000..0fbf118
--- /dev/null
+++ b/drivers/hwmon/nsa3xx-hwmon.c
@@ -0,0 +1,223 @@
+/*
+ * drivers/hwmon/nsa3xx-hwmon.c
+ *
+ * ZyXEL NSA3xx Media Servers
+ * hardware monitoring
+ *
+ * Copyright (C) 2016 Adam Baker <linux@baker-net.org.uk>
+ * based on a board file driver
+ * Copyright (C) 2012 Peter Schildmann <linux@schildmann.info>
+ *
+ * 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>
+#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>
+
+#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 {
+ struct device *classdev;
+ 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 nsa3xx_inputs {
+ NSA3XX_FAN = 1,
+ NSA3XX_TEMP = 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;
+
+ 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);
+ 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_err(dev, "Read invalid MCU data %08x\n", mcu_data);
+ mcu_data = 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;
+
+ 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;
+
+ pr_debug("channel value 0x%02x!\n", channel);
+ 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;
+
+ hwmon = devm_kzalloc(&pdev->dev, sizeof(*hwmon), GFP_KERNEL);
+ if (!hwmon)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, hwmon);
+
+ /* 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);
+
+ mutex_init(&hwmon->update_lock);
+
+ hwmon->classdev = devm_hwmon_device_register_with_groups(&pdev->dev,
+ "nsa3xx", hwmon, nsa3xx_groups);
+ if (IS_ERR(hwmon->classdev))
+ return PTR_ERR(hwmon->classdev);
+
+ dev_dbg(&pdev->dev, "initialized\n");
+
+ 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;
+}
+
+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);
+
+MODULE_DEVICE_TABLE(of, of_nsa3xx_hwmon_match);
+MODULE_AUTHOR("Peter Schildmann <linux@schildmann.info>");
+MODULE_AUTHOR("Adam Baker <linux@baker-net.org.uk>");
+MODULE_DESCRIPTION("NSA3XX Hardware Monitoring");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:nsa3xx-hwmon");
--
2.1.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/3] ARM: mvebu: Create a NSA3xx hardware monitoring driver
@ 2016-02-10 23:33 ` Adam Baker
0 siblings, 0 replies; 16+ messages in thread
From: Adam Baker @ 2016-02-10 23:33 UTC (permalink / raw)
To: linux-arm-kernel
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 <linux@baker-net.org.uk>
---
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
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 80a73bf..8801b78 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -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"
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 12a3239..e85414a 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -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
diff --git a/drivers/hwmon/nsa3xx-hwmon.c b/drivers/hwmon/nsa3xx-hwmon.c
new file mode 100644
index 0000000..0fbf118
--- /dev/null
+++ b/drivers/hwmon/nsa3xx-hwmon.c
@@ -0,0 +1,223 @@
+/*
+ * drivers/hwmon/nsa3xx-hwmon.c
+ *
+ * ZyXEL NSA3xx Media Servers
+ * hardware monitoring
+ *
+ * Copyright (C) 2016 Adam Baker <linux@baker-net.org.uk>
+ * based on a board file driver
+ * Copyright (C) 2012 Peter Schildmann <linux@schildmann.info>
+ *
+ * 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>
+#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>
+
+#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 {
+ struct device *classdev;
+ 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 nsa3xx_inputs {
+ NSA3XX_FAN = 1,
+ NSA3XX_TEMP = 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;
+
+ 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);
+ 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_err(dev, "Read invalid MCU data %08x\n", mcu_data);
+ mcu_data = 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;
+
+ 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;
+
+ pr_debug("channel value 0x%02x!\n", channel);
+ 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;
+
+ hwmon = devm_kzalloc(&pdev->dev, sizeof(*hwmon), GFP_KERNEL);
+ if (!hwmon)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, hwmon);
+
+ /* 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);
+
+ mutex_init(&hwmon->update_lock);
+
+ hwmon->classdev = devm_hwmon_device_register_with_groups(&pdev->dev,
+ "nsa3xx", hwmon, nsa3xx_groups);
+ if (IS_ERR(hwmon->classdev))
+ return PTR_ERR(hwmon->classdev);
+
+ dev_dbg(&pdev->dev, "initialized\n");
+
+ 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;
+}
+
+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);
+
+MODULE_DEVICE_TABLE(of, of_nsa3xx_hwmon_match);
+MODULE_AUTHOR("Peter Schildmann <linux@schildmann.info>");
+MODULE_AUTHOR("Adam Baker <linux@baker-net.org.uk>");
+MODULE_DESCRIPTION("NSA3XX Hardware Monitoring");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:nsa3xx-hwmon");
--
2.1.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/3] ARM: mvebu: Add the hardware monitor to the NSA320 device tree
2016-02-10 23:33 ` Adam Baker
@ 2016-02-10 23:33 ` Adam Baker
-1 siblings, 0 replies; 16+ messages in thread
From: Adam Baker @ 2016-02-10 23:33 UTC (permalink / raw)
To: Jason Cooper, Andrew Lunn, Gregory Clement,
Sebastian Hesselbarth, Jean Delvare, Guenter Roeck
Cc: Mark Rutland, devicetree, Russell King, Pawel Moll, Ian Campbell,
Adam Baker, lm-sensors, Rob Herring, Kumar Gala,
linux-arm-kernel
Add an entry for the hardware monitoring MCU
Signed-off-by: Adam Baker <linux@baker-net.org.uk>
---
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/3] ARM: mvebu: Add the hardware monitor to the NSA320 device tree
@ 2016-02-10 23:33 ` Adam Baker
0 siblings, 0 replies; 16+ messages in thread
From: Adam Baker @ 2016-02-10 23:33 UTC (permalink / raw)
To: linux-arm-kernel
Add an entry for the hardware monitoring MCU
Signed-off-by: Adam Baker <linux@baker-net.org.uk>
---
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] ARM: mvebu: Define binding for the nsa3xx-hwmon driver
2016-02-10 23:33 ` Adam Baker
@ 2016-02-12 15:59 ` Rob Herring
-1 siblings, 0 replies; 16+ messages in thread
From: Rob Herring @ 2016-02-12 15:59 UTC (permalink / raw)
To: Adam Baker
Cc: Jason Cooper, Andrew Lunn, Gregory Clement,
Sebastian Hesselbarth, Jean Delvare, Guenter Roeck, Mark Rutland,
devicetree-u79uwXL29TY76Z2rM5mHXA, Russell King, Pawel Moll,
Ian Campbell, lm-sensors-GZX6beZjE8VD60Wz+7aTrA, Kumar Gala,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
On Wed, Feb 10, 2016 at 11:33:42PM +0000, Adam Baker wrote:
> Define a binding for the hardware monitoring chip present in several
> variants of the Zyxel NSA3xx NAS devices.
>
> Signed-off-by: Adam Baker <linux-u8Lxuz9p/S1csCcyGdaD/Q@public.gmane.org>
> ---
> .../devicetree/bindings/hwmon/nsa320-mcu.txt | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/hwmon/nsa320-mcu.txt
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/3] ARM: mvebu: Define binding for the nsa3xx-hwmon driver
@ 2016-02-12 15:59 ` Rob Herring
0 siblings, 0 replies; 16+ messages in thread
From: Rob Herring @ 2016-02-12 15:59 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Feb 10, 2016 at 11:33:42PM +0000, Adam Baker wrote:
> Define a binding for the hardware monitoring chip present in several
> variants of the Zyxel NSA3xx NAS devices.
>
> Signed-off-by: Adam Baker <linux@baker-net.org.uk>
> ---
> .../devicetree/bindings/hwmon/nsa320-mcu.txt | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/hwmon/nsa320-mcu.txt
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] ARM: mvebu: Add driver for the Zyxel NSA3XX hwmon MCU
2016-02-10 23:33 ` Adam Baker
@ 2016-02-12 16:35 ` Gregory CLEMENT
-1 siblings, 0 replies; 16+ messages in thread
From: Gregory CLEMENT @ 2016-02-12 16:35 UTC (permalink / raw)
To: Adam Baker
Cc: Jason Cooper, Andrew Lunn, Sebastian Hesselbarth, Jean Delvare,
Guenter Roeck, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Russell King,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
lm-sensors-GZX6beZjE8VD60Wz+7aTrA
Hi Adam,
On jeu., févr. 11 2016, Adam Baker <linux-u8Lxuz9p/S1csCcyGdaD/Q@public.gmane.org> wrote:
> 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
>
> [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(-)
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 0/3] ARM: mvebu: Add driver for the Zyxel NSA3XX hwmon MCU
@ 2016-02-12 16:35 ` Gregory CLEMENT
0 siblings, 0 replies; 16+ messages in thread
From: Gregory CLEMENT @ 2016-02-12 16:35 UTC (permalink / raw)
To: linux-arm-kernel
Hi Adam,
On jeu., f?vr. 11 2016, Adam Baker <linux@baker-net.org.uk> wrote:
> 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
>
> [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(-)
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] ARM: mvebu: Create a NSA3xx hardware monitoring driver
2016-02-10 23:33 ` Adam Baker
@ 2016-02-13 2:35 ` Guenter Roeck
-1 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2016-02-13 2:35 UTC (permalink / raw)
To: Adam Baker
Cc: Jason Cooper, Andrew Lunn, Gregory Clement,
Sebastian Hesselbarth, Jean Delvare, Rob Herring, Pawel Moll,
Mark Rutland, Ian Campbell, Kumar Gala, Russell King,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
lm-sensors-GZX6beZjE8VD60Wz+7aTrA
Hi,
On Wed, Feb 10, 2016 at 11:33:43PM +0000, Adam Baker wrote:
> 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 <linux-u8Lxuz9p/S1csCcyGdaD/Q@public.gmane.org>
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
> ---
> 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
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 80a73bf..8801b78 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -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.
> + tristate "ZyXEL NSA3xx fan speed and temperature sensors"
... NSA-320 and compatible ...
> + 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 ?
> + 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.
> +
> + The sensor data is taken from a Holtek HT46R065 microcontroller
> + connected to GPIO lines.
> +
Is that relevant ?
> + This driver can also be built as a module. If so, the module
> + will be called nsa3xx-hwmon.
> +
> if ACPI
>
> comment "ACPI drivers"
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 12a3239..e85414a 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -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.
> obj-$(CONFIG_SENSORS_MCP3021) += mcp3021.o
> obj-$(CONFIG_SENSORS_MENF21BMC_HWMON) += menf21bmc_hwmon.o
> obj-$(CONFIG_SENSORS_NCT6683) += nct6683.o
> diff --git a/drivers/hwmon/nsa3xx-hwmon.c b/drivers/hwmon/nsa3xx-hwmon.c
> new file mode 100644
> index 0000000..0fbf118
> --- /dev/null
> +++ b/drivers/hwmon/nsa3xx-hwmon.c
nsa320-hwmon.c
> @@ -0,0 +1,223 @@
> +/*
> + * drivers/hwmon/nsa3xx-hwmon.c
> + *
> + * ZyXEL NSA3xx Media Servers
Please be specific.
> + * 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/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.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/delay.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_platform.h>
> +
Please order include files alphabetically.
> +#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.
> + 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.
> + 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.
> +};
> +
> +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;
> +
> + 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.
> + 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.
> + 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_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.
> + }
> +
> + 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", 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.
> + 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().
> + 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;
> +
> + hwmon = devm_kzalloc(&pdev->dev, sizeof(*hwmon), GFP_KERNEL);
> + if (!hwmon)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, hwmon);
Unnecessary.
> +
> + /* 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().
> +
> + 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.
> + 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);
> + 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.
> +
> +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);
> +
> +MODULE_DEVICE_TABLE(of, of_nsa3xx_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");
GPL v2
> +MODULE_ALIAS("platform:nsa3xx-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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/3] ARM: mvebu: Create a NSA3xx hardware monitoring driver
@ 2016-02-13 2:35 ` Guenter Roeck
0 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2016-02-13 2:35 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On Wed, Feb 10, 2016 at 11:33:43PM +0000, Adam Baker wrote:
> 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 <linux@baker-net.org.uk>
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
> ---
> 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
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 80a73bf..8801b78 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -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.
> + tristate "ZyXEL NSA3xx fan speed and temperature sensors"
... NSA-320 and compatible ...
> + 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 ?
> + 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.
> +
> + The sensor data is taken from a Holtek HT46R065 microcontroller
> + connected to GPIO lines.
> +
Is that relevant ?
> + This driver can also be built as a module. If so, the module
> + will be called nsa3xx-hwmon.
> +
> if ACPI
>
> comment "ACPI drivers"
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 12a3239..e85414a 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -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.
> obj-$(CONFIG_SENSORS_MCP3021) += mcp3021.o
> obj-$(CONFIG_SENSORS_MENF21BMC_HWMON) += menf21bmc_hwmon.o
> obj-$(CONFIG_SENSORS_NCT6683) += nct6683.o
> diff --git a/drivers/hwmon/nsa3xx-hwmon.c b/drivers/hwmon/nsa3xx-hwmon.c
> new file mode 100644
> index 0000000..0fbf118
> --- /dev/null
> +++ b/drivers/hwmon/nsa3xx-hwmon.c
nsa320-hwmon.c
> @@ -0,0 +1,223 @@
> +/*
> + * drivers/hwmon/nsa3xx-hwmon.c
> + *
> + * ZyXEL NSA3xx Media Servers
Please be specific.
> + * hardware monitoring
> + *
> + * Copyright (C) 2016 Adam Baker <linux@baker-net.org.uk>
> + * based on a board file driver
> + * Copyright (C) 2012 Peter Schildmann <linux@schildmann.info>
> + *
> + * 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>
> +#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.
> +#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.
> + 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.
> + 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.
> +};
> +
> +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;
> +
> + 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.
> + 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.
> + 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_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.
> + }
> +
> + 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", 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.
> + 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().
> + 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;
> +
> + hwmon = devm_kzalloc(&pdev->dev, sizeof(*hwmon), GFP_KERNEL);
> + if (!hwmon)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, hwmon);
Unnecessary.
> +
> + /* 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().
> +
> + 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.
> + 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);
> + 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.
> +
> +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);
> +
> +MODULE_DEVICE_TABLE(of, of_nsa3xx_hwmon_match);
> +MODULE_AUTHOR("Peter Schildmann <linux@schildmann.info>");
> +MODULE_AUTHOR("Adam Baker <linux@baker-net.org.uk>");
> +MODULE_DESCRIPTION("NSA3XX Hardware Monitoring");
> +MODULE_LICENSE("GPL");
GPL v2
> +MODULE_ALIAS("platform:nsa3xx-hwmon");
> --
> 2.1.4
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] ARM: mvebu: Create a NSA3xx hardware monitoring driver
2016-02-13 2:35 ` Guenter Roeck
@ 2016-02-15 23:08 ` Adam Baker
-1 siblings, 0 replies; 16+ messages in thread
From: Adam Baker @ 2016-02-15 23:08 UTC (permalink / raw)
To: Guenter Roeck
Cc: Mark Rutland, Andrew Lunn, Jean Delvare, Russell King,
Jason Cooper, Pawel Moll, Ian Campbell, lm-sensors, devicetree,
Rob Herring, Kumar Gala, Gregory Clement, linux-arm-kernel,
Sebastian Hesselbarth
On 13/02/16 02:35, Guenter Roeck wrote:
> Hi,
>
> On Wed, Feb 10, 2016 at 11:33:43PM +0000, Adam Baker wrote:
>> 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 <linux@baker-net.org.uk>
>
> 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
>> ---
>> 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
>>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 80a73bf..8801b78 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -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.
done
>
>> + tristate "ZyXEL NSA3xx fan speed and temperature sensors"
>
> ... NSA-320 and compatible ...
done
>
>> + 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 ?
done - depends on MACH_KIRKWOOD || COMPILE_TEST
>
>> + 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
>> +
>> + 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.
>> + This driver can also be built as a module. If so, the module
>> + will be called nsa3xx-hwmon.
>> +
>> if ACPI
>>
>> comment "ACPI drivers"
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index 12a3239..e85414a 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -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.
done
>
>> obj-$(CONFIG_SENSORS_MCP3021) += mcp3021.o
>> obj-$(CONFIG_SENSORS_MENF21BMC_HWMON) += menf21bmc_hwmon.o
>> obj-$(CONFIG_SENSORS_NCT6683) += nct6683.o
>> diff --git a/drivers/hwmon/nsa3xx-hwmon.c b/drivers/hwmon/nsa3xx-hwmon.c
>> new file mode 100644
>> index 0000000..0fbf118
>> --- /dev/null
>> +++ b/drivers/hwmon/nsa3xx-hwmon.c
>
> nsa320-hwmon.c
done
>
>> @@ -0,0 +1,223 @@
>> +/*
>> + * drivers/hwmon/nsa3xx-hwmon.c
>> + *
>> + * ZyXEL NSA3xx Media Servers
>
> Please be specific.
done
>
>> + * hardware monitoring
>> + *
>> + * Copyright (C) 2016 Adam Baker <linux@baker-net.org.uk>
>> + * based on a board file driver
>> + * Copyright (C) 2012 Peter Schildmann <linux@schildmann.info>
>> + *
>> + * 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>
>> +#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
>
>> +#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.
done
>
>> + 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
>> + 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
>
>> +};
>> +
>> +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;
>> +
>> + 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
>
>> + 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.
>
done
>> + 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_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.
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.
>
>> + }
>> +
>> + 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", 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.
nsa320_hwmon_update() now returns an int error code or zero and the
caller gets the data fro the struct if it succeeds
>
>> + 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
>
>> + 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;
>> +
>> + hwmon = devm_kzalloc(&pdev->dev, sizeof(*hwmon), GFP_KERNEL);
>> + if (!hwmon)
>> + return -ENOMEM;
>> +
>> + platform_set_drvdata(pdev, hwmon);
>
> Unnecessary.
removed
>
>> +
>> + /* 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
>
>> +
>> + 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
>
>> + 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
>
>> + 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
>> +
>> +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);
>> +
>> +MODULE_DEVICE_TABLE(of, of_nsa3xx_hwmon_match);
>> +MODULE_AUTHOR("Peter Schildmann <linux@schildmann.info>");
>> +MODULE_AUTHOR("Adam Baker <linux@baker-net.org.uk>");
>> +MODULE_DESCRIPTION("NSA3XX Hardware Monitoring");
>> +MODULE_LICENSE("GPL");
>
> GPL v2
done
>
>> +MODULE_ALIAS("platform:nsa3xx-hwmon");
>> --
>> 2.1.4
>>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/3] ARM: mvebu: Create a NSA3xx hardware monitoring driver
@ 2016-02-15 23:08 ` Adam Baker
0 siblings, 0 replies; 16+ messages in thread
From: Adam Baker @ 2016-02-15 23:08 UTC (permalink / raw)
To: linux-arm-kernel
On 13/02/16 02:35, Guenter Roeck wrote:
> Hi,
>
> On Wed, Feb 10, 2016 at 11:33:43PM +0000, Adam Baker wrote:
>> 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 <linux@baker-net.org.uk>
>
> 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
>> ---
>> 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
>>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 80a73bf..8801b78 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -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.
done
>
>> + tristate "ZyXEL NSA3xx fan speed and temperature sensors"
>
> ... NSA-320 and compatible ...
done
>
>> + 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 ?
done - depends on MACH_KIRKWOOD || COMPILE_TEST
>
>> + 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
>> +
>> + 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.
>> + This driver can also be built as a module. If so, the module
>> + will be called nsa3xx-hwmon.
>> +
>> if ACPI
>>
>> comment "ACPI drivers"
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index 12a3239..e85414a 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -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.
done
>
>> obj-$(CONFIG_SENSORS_MCP3021) += mcp3021.o
>> obj-$(CONFIG_SENSORS_MENF21BMC_HWMON) += menf21bmc_hwmon.o
>> obj-$(CONFIG_SENSORS_NCT6683) += nct6683.o
>> diff --git a/drivers/hwmon/nsa3xx-hwmon.c b/drivers/hwmon/nsa3xx-hwmon.c
>> new file mode 100644
>> index 0000000..0fbf118
>> --- /dev/null
>> +++ b/drivers/hwmon/nsa3xx-hwmon.c
>
> nsa320-hwmon.c
done
>
>> @@ -0,0 +1,223 @@
>> +/*
>> + * drivers/hwmon/nsa3xx-hwmon.c
>> + *
>> + * ZyXEL NSA3xx Media Servers
>
> Please be specific.
done
>
>> + * hardware monitoring
>> + *
>> + * Copyright (C) 2016 Adam Baker <linux@baker-net.org.uk>
>> + * based on a board file driver
>> + * Copyright (C) 2012 Peter Schildmann <linux@schildmann.info>
>> + *
>> + * 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>
>> +#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
>
>> +#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.
done
>
>> + 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
>> + 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
>
>> +};
>> +
>> +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;
>> +
>> + 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
>
>> + 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.
>
done
>> + 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_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.
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.
>
>> + }
>> +
>> + 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", 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.
nsa320_hwmon_update() now returns an int error code or zero and the
caller gets the data fro the struct if it succeeds
>
>> + 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
>
>> + 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;
>> +
>> + hwmon = devm_kzalloc(&pdev->dev, sizeof(*hwmon), GFP_KERNEL);
>> + if (!hwmon)
>> + return -ENOMEM;
>> +
>> + platform_set_drvdata(pdev, hwmon);
>
> Unnecessary.
removed
>
>> +
>> + /* 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
>
>> +
>> + 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
>
>> + 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
>
>> + 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
>> +
>> +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);
>> +
>> +MODULE_DEVICE_TABLE(of, of_nsa3xx_hwmon_match);
>> +MODULE_AUTHOR("Peter Schildmann <linux@schildmann.info>");
>> +MODULE_AUTHOR("Adam Baker <linux@baker-net.org.uk>");
>> +MODULE_DESCRIPTION("NSA3XX Hardware Monitoring");
>> +MODULE_LICENSE("GPL");
>
> GPL v2
done
>
>> +MODULE_ALIAS("platform:nsa3xx-hwmon");
>> --
>> 2.1.4
>>
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2016-02-15 23:08 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-10 23:33 [PATCH 0/3] ARM: mvebu: Add driver for the Zyxel NSA3XX hwmon MCU Adam Baker
2016-02-10 23:33 ` Adam Baker
2016-02-10 23:33 ` [PATCH 1/3] ARM: mvebu: Define binding for the nsa3xx-hwmon driver Adam Baker
2016-02-10 23:33 ` Adam Baker
[not found] ` <1455147224-6742-2-git-send-email-linux-u8Lxuz9p/S1csCcyGdaD/Q@public.gmane.org>
2016-02-12 15:59 ` Rob Herring
2016-02-12 15:59 ` Rob Herring
2016-02-10 23:33 ` [PATCH 2/3] ARM: mvebu: Create a NSA3xx hardware monitoring driver Adam Baker
2016-02-10 23:33 ` Adam Baker
[not found] ` <1455147224-6742-3-git-send-email-linux-u8Lxuz9p/S1csCcyGdaD/Q@public.gmane.org>
2016-02-13 2:35 ` Guenter Roeck
2016-02-13 2:35 ` Guenter Roeck
2016-02-15 23:08 ` Adam Baker
2016-02-15 23:08 ` Adam Baker
2016-02-10 23:33 ` [PATCH 3/3] ARM: mvebu: Add the hardware monitor to the NSA320 device tree Adam Baker
2016-02-10 23:33 ` Adam Baker
[not found] ` <1455147224-6742-1-git-send-email-linux-u8Lxuz9p/S1csCcyGdaD/Q@public.gmane.org>
2016-02-12 16:35 ` [PATCH 0/3] ARM: mvebu: Add driver for the Zyxel NSA3XX hwmon MCU Gregory CLEMENT
2016-02-12 16:35 ` Gregory CLEMENT
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.