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@xxxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxx>
+ * based on a board file driver
+ * Copyright (C) 2012 Peter Schildmann <linux@xxxxxxxxxxxxxxx>
+ *
+ * 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@xxxxxxxxxxxxxxx>");
+MODULE_AUTHOR("Adam Baker <linux@xxxxxxxxxxxxxxxx>");
+MODULE_DESCRIPTION("NSA3XX Hardware Monitoring");
+MODULE_LICENSE("GPL");
GPL v2
done
+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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
