[PATCH 1/3] hwmon: lochnagar: Add device tree binding document

[PATCH 1/3] hwmon: lochnagar: Add device tree binding document

[Charles Keepax]

Lochnagar is an evaluation and development board for Cirrus
Logic Smart CODEC and Amp devices. It allows the connection of
most Cirrus Logic devices on mini-cards, as well as allowing
connection of various application processor systems to provide a
full evaluation platform. This driver supports the board
controller chip on the Lochnagar board.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 .../devicetree/bindings/hwmon/cirrus,lochnagar.txt | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/cirrus,lochnagar.txt

diff --git a/Documentation/devicetree/bindings/hwmon/cirrus,lochnagar.txt b/Documentation/devicetree/bindings/hwmon/cirrus,lochnagar.txt
new file mode 100644
index 0000000000000..ffb79ccf51ee3
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/cirrus,lochnagar.txt
@@ -0,0 +1,26 @@
+Cirrus Logic Lochnagar Audio Development Board
+
+Lochnagar is an evaluation and development board for Cirrus Logic
+Smart CODEC and Amp devices. It allows the connection of most Cirrus
+Logic devices on mini-cards, as well as allowing connection of
+various application processor systems to provide a full evaluation
+platform.  Audio system topology, clocking and power can all be
+controlled through the Lochnagar, allowing the device under test
+to be used in a variety of possible use cases.
+
+This binding document describes the binding for the hardware monitor
+portion of the driver.
+
+This binding must be part of the Lochnagar MFD binding:
+  [4] ../mfd/cirrus,lochnagar.txt
+
+Required properties:
+
+  - compatible : One of the following strings:
+                 "cirrus,lochnagar2-hwmon"
+
+Example:
+
+lochnagar-hwmon {
+	compatible = "cirrus,lochnagar2-hwmon";
+};
-- 
2.11.0

[PATCH 2/3] hwmon: Add convience macro to define simple static sensors

[Charles Keepax]

It takes a fair amount of boiler plate code to add new sensors, add a
macro that can be used to specify simple static sensors.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 include/linux/hwmon.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
index 99e0c1b0b5fb3..7a8cc06a0d61d 100644
--- a/include/linux/hwmon.h
+++ b/include/linux/hwmon.h
@@ -365,6 +365,14 @@ struct hwmon_channel_info {
 	const u32 *config;
 };
 
+#define HWMON_CHANNEL_INFO(stype, ...)	\
+	(&(struct hwmon_channel_info) {	\
+		.type = hwmon_##stype,	\
+		.config = (u32 []) {	\
+			__VA_ARGS__, 0	\
+		}			\
+	})
+
 /**
  * Chip configuration
  * @ops:	Pointer to hwmon operations.
-- 
2.11.0

Re: [PATCH 2/3] hwmon: Add convience macro to define simple static sensors

[Guenter Roeck]

On Wed, Mar 20, 2019 at 02:58:17PM +0000, Charles Keepax wrote:
> It takes a fair amount of boiler plate code to add new sensors, add a
> macro that can be used to specify simple static sensors.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>

Great idea!

Applied to hwmon-next; no need to resend.

Thanks,
Guenter

> ---
>  include/linux/hwmon.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
> index 99e0c1b0b5fb3..7a8cc06a0d61d 100644
> --- a/include/linux/hwmon.h
> +++ b/include/linux/hwmon.h
> @@ -365,6 +365,14 @@ struct hwmon_channel_info {
>  	const u32 *config;
>  };
>  
> +#define HWMON_CHANNEL_INFO(stype, ...)	\
> +	(&(struct hwmon_channel_info) {	\
> +		.type = hwmon_##stype,	\
> +		.config = (u32 []) {	\
> +			__VA_ARGS__, 0	\
> +		}			\
> +	})
> +
>  /**
>   * Chip configuration
>   * @ops:	Pointer to hwmon operations.
> -- 
> 2.11.0
> 

[PATCH 3/3] hwmon: lochnagar: Add Lochnagar 2 hardware monitoring driver

[Charles Keepax]

From: Lucas Tanure <tanureal@opensource.cirrus.com>

Lochnagar is an evaluation and development board for Cirrus
Logic Smart CODEC and Amp devices. It allows the connection of
most Cirrus Logic devices on mini-cards, as well as allowing
connection of various application processor systems to provide a
full evaluation platform.

This driver adds support for the hardware monitoring features of
the Lochnagar 2 to the hwmon API. Monitoring is provided for
the board voltages, currents and temperature supported by the
board controller chip.

Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 Documentation/hwmon/lochnagar   |  56 ++++++++
 MAINTAINERS                     |   3 +
 drivers/hwmon/Kconfig           |  10 ++
 drivers/hwmon/Makefile          |   1 +
 drivers/hwmon/lochnagar-hwmon.c | 293 ++++++++++++++++++++++++++++++++++++++++
 5 files changed, 363 insertions(+)
 create mode 100644 Documentation/hwmon/lochnagar
 create mode 100644 drivers/hwmon/lochnagar-hwmon.c

diff --git a/Documentation/hwmon/lochnagar b/Documentation/hwmon/lochnagar
new file mode 100644
index 0000000000000..60e869a8c564e
--- /dev/null
+++ b/Documentation/hwmon/lochnagar
@@ -0,0 +1,56 @@
+Kernel Driver Lochnagar
+========================
+
+Supported systems:
+  * Cirrus Logic : Lochnagar 2
+
+Author: Lucas A. Tanure Alves
+
+Description
+-----------
+
+Lochnagar 2 features built-in Current Monitor circuitry that allows for the
+measurement of both voltage and current on up to eight of the supply voltage
+rails provided to the minicards. The Current Monitor does not require any
+hardware modifications or external circuitry to operate.
+
+The current and voltage measurements are obtained through the standard register
+map interface to the Lochnagar board controller, and can therefore be monitored
+by software.
+
+Sysfs attributes
+----------------
+
+temp1_input   The Lochnagar board temperature.
+in0_input     Measured voltage for DBVDD1 (nanoVolts)
+in0_label     "DBVDD1"
+curr1_input   Measured current for DBVDD1 (nanoAmps)
+curr1_label   "DBVDD1"
+in1_input     Measured voltage for 1V8 DSP (nanoVolts)
+in1_label     "1V8 DSP"
+curr2_input   Measured current for 1V8 DSP (nanoAmps)
+curr2_label   "1V8 DSP"
+in2_input     Measured voltage for 1V8 CDC (nanoVolts)
+in2_label     "1V8 CDC"
+curr3_input   Measured current for 1V8 CDC (nanoAmps)
+curr3_label   "1V8 CDC"
+in3_input     Measured voltage for VDDCORE DSP (nanoVolts)
+in3_label     "VDDCORE DSP"
+curr4_input   Measured current for VDDCORE DSP (nanoAmps)
+curr4_label   "VDDCORE DSP"
+in4_input     Measured voltage for AVDD 1V8 (nanoVolts)
+in4_label     "AVDD 1V8"
+curr5_input   Measured current for AVDD 1V8 (nanoAmps)
+curr5_label   "AVDD 1V8"
+curr6_input   Measured current for SYSVDD (nanoAmps)
+curr6_label   "SYSVDD"
+in6_input     Measured voltage for VDDCORE CDC (nanoVolts)
+in6_label     "VDDCORE CDC"
+curr7_input   Measured current for VDDCORE CDC (nanoAmps)
+curr7_label   "VDDCORE CDC"
+in7_input     Measured voltage for MICVDD (nanoVolts)
+in7_label     "MICVDD"
+curr8_input   Measured current for MICVDD (nanoAmps)
+curr8_label   "MICVDD"
+
+Note: It is not possible to measure voltage on the SYSVDD rail.
diff --git a/MAINTAINERS b/MAINTAINERS
index e17ebf70b5480..4cd3c281d8b56 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3796,6 +3796,7 @@ M:	Richard Fitzgerald <rf@opensource.cirrus.com>
 L:	patches@opensource.cirrus.com
 S:	Supported
 F:	drivers/clk/clk-lochnagar.c
+F:	drivers/hwmon/lochnagar-hwmon.c
 F:	drivers/mfd/lochnagar-i2c.c
 F:	drivers/pinctrl/cirrus/pinctrl-lochnagar.c
 F:	drivers/regulator/lochnagar-regulator.c
@@ -3804,8 +3805,10 @@ F:	include/dt-bindings/pinctrl/lochnagar.h
 F:	include/linux/mfd/lochnagar*
 F:	Documentation/devicetree/bindings/mfd/cirrus,lochnagar.txt
 F:	Documentation/devicetree/bindings/clock/cirrus,lochnagar.txt
+F:	Documentation/devicetree/bindings/hwmon/cirrus,lochnagar.txt
 F:	Documentation/devicetree/bindings/pinctrl/cirrus,lochnagar.txt
 F:	Documentation/devicetree/bindings/regulator/cirrus,lochnagar.txt
+F:	Documentation/hwmon/lochnagar
 
 CISCO FCOE HBA DRIVER
 M:	Satish Kharat <satishkh@cisco.com>
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index d0f1dfe2bcbbd..dedd5febd3aa6 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -705,6 +705,16 @@ config SENSORS_LINEAGE
 	  This driver can also be built as a module. If so, the module
 	  will be called lineage-pem.
 
+config SENSORS_LOCHNAGAR
+	tristate "Lochnagar Hardware Monitor"
+	depends on MFD_LOCHNAGAR
+	help
+	  If you say yes here you get support for Lochnagar 2 temperature,
+	  voltage and current sensors abilities.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called lochnagar-hwmon.
+
 config SENSORS_LTC2945
 	tristate "Linear Technology LTC2945"
 	depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index f5c7b442e69e5..8db472ea04f00 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -89,6 +89,7 @@ obj-$(CONFIG_SENSORS_JZ4740)	+= jz4740-hwmon.o
 obj-$(CONFIG_SENSORS_K8TEMP)	+= k8temp.o
 obj-$(CONFIG_SENSORS_K10TEMP)	+= k10temp.o
 obj-$(CONFIG_SENSORS_LINEAGE)	+= lineage-pem.o
+obj-$(CONFIG_SENSORS_LOCHNAGAR)	+= lochnagar-hwmon.o
 obj-$(CONFIG_SENSORS_LM63)	+= lm63.o
 obj-$(CONFIG_SENSORS_LM70)	+= lm70.o
 obj-$(CONFIG_SENSORS_LM73)	+= lm73.o
diff --git a/drivers/hwmon/lochnagar-hwmon.c b/drivers/hwmon/lochnagar-hwmon.c
new file mode 100644
index 0000000000000..1f6b82bb9205d
--- /dev/null
+++ b/drivers/hwmon/lochnagar-hwmon.c
@@ -0,0 +1,293 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Lochnagar hardware monitoring features
+ *
+ * Copyright (c) 2016-2019 Cirrus Logic, Inc. and
+ *                         Cirrus Logic International Semiconductor Ltd.
+ *
+ * Author: Lucas Tanure <tanureal@opensource.cirrus.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/platform_device.h>
+#include <linux/delay.h>
+#include <linux/mfd/lochnagar.h>
+#include <linux/mfd/lochnagar2_regs.h>
+#include <linux/regmap.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/i2c.h>
+#include <asm/div64.h>
+
+struct lochnagar_hwmon {
+	struct lochnagar *lochnagar;
+
+	/* Lock to ensure only a single sensor is read at a time */
+	struct mutex sensor_lock;
+	struct device *hwmon_dev;
+};
+
+enum lochnagar_measure_mode {
+	LN2_CURRENT = 0,
+	LN2_VOLTAGE,
+	LN2_TEMPERATURE,
+};
+
+static const char * const lochnagar_chan_names[] = {
+	"DBVDD1",
+	"1V8 DSP",
+	"1V8 CDC",
+	"VDDCORE DSP",
+	"AVDD 1V8",
+	"SYSVDD",
+	"VDDCORE CDC",
+	"MICVDD",
+};
+
+static u64 float_to_int(u32 data, u32 precision)
+{
+	s64 man = data & 0x007FFFFF;
+	s32 exp = ((data & 0x7F800000) >> 23) - 127 - 23;
+	bool negative = data & 0x80000000;
+
+	man = (man + (1 << 23)) * precision;
+
+	if (exp > 0)
+		man <<= exp;
+	else if (exp < 0)
+		man >>= -exp;
+
+	return negative ? -man : man;
+}
+
+static int do_measurement(struct regmap *regmap, int chan,
+			  enum lochnagar_measure_mode mode, int nsamples)
+{
+	unsigned int val;
+	int ret;
+
+	chan = 1 << (chan + LOCHNAGAR2_IMON_MEASURED_CHANNELS_SHIFT);
+
+	ret = regmap_write(regmap, LOCHNAGAR2_IMON_CTRL1,
+			   LOCHNAGAR2_IMON_ENA_MASK | chan | mode);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_write(regmap, LOCHNAGAR2_IMON_CTRL2, nsamples);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_write(regmap, LOCHNAGAR2_IMON_CTRL3,
+			   LOCHNAGAR2_IMON_CONFIGURE_MASK);
+	if (ret < 0)
+		return ret;
+
+	ret =  regmap_read_poll_timeout(regmap, LOCHNAGAR2_IMON_CTRL3, val,
+					val & LOCHNAGAR2_IMON_DONE_MASK,
+					1000, 10000);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_write(regmap, LOCHNAGAR2_IMON_CTRL3,
+			   LOCHNAGAR2_IMON_MEASURE_MASK);
+	if (ret < 0)
+		return ret;
+
+	msleep(nsamples);
+
+	ret =  regmap_read_poll_timeout(regmap, LOCHNAGAR2_IMON_CTRL3, val,
+					val & LOCHNAGAR2_IMON_DONE_MASK,
+					5000, 2000000);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_write(regmap, LOCHNAGAR2_IMON_CTRL3, 0);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int request_data(struct regmap *regmap, int chan, u32 *data)
+{
+	unsigned int val;
+	int ret;
+
+	ret = regmap_write(regmap, LOCHNAGAR2_IMON_CTRL4,
+			   LOCHNAGAR2_IMON_DATA_REQ_MASK |
+			   chan << LOCHNAGAR2_IMON_CH_SEL_SHIFT);
+	if (ret < 0)
+		return ret;
+
+	ret =  regmap_read_poll_timeout(regmap, LOCHNAGAR2_IMON_CTRL4, val,
+					val & LOCHNAGAR2_IMON_DATA_RDY_MASK,
+					1000, 10000);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_read(regmap, LOCHNAGAR2_IMON_DATA1, &val);
+	if (ret < 0)
+		return ret;
+
+	*data = val << 16;
+
+	ret = regmap_read(regmap, LOCHNAGAR2_IMON_DATA2, &val);
+	if (ret < 0)
+		return ret;
+
+	*data |= val;
+
+	ret = regmap_write(regmap, LOCHNAGAR2_IMON_CTRL4, 0);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int read_sensor(struct device *dev, int chan,
+		       enum lochnagar_measure_mode mode, int nsamples,
+		       unsigned int precision, long *val)
+{
+	struct lochnagar_hwmon *priv = dev_get_drvdata(dev);
+	struct regmap *regmap = priv->lochnagar->regmap;
+	u32 data;
+	int ret;
+
+	mutex_lock(&priv->sensor_lock);
+
+	ret = do_measurement(regmap, chan, mode, nsamples);
+	if (ret < 0) {
+		dev_err(dev, "Failed to perform measurement: %d\n", ret);
+		goto error;
+	}
+
+	ret = request_data(regmap, chan, &data);
+	if (ret < 0) {
+		dev_err(dev, "Failed to read measurement: %d\n", ret);
+		goto error;
+	}
+
+	*val = float_to_int(data, precision);
+
+error:
+	mutex_unlock(&priv->sensor_lock);
+
+	return ret;
+}
+
+static umode_t lochnagar_is_visible(const void *drvdata,
+				    enum hwmon_sensor_types type,
+				    u32 attr, int chan)
+{
+	if (type == hwmon_in && !strcmp("SYSVDD", lochnagar_chan_names[chan]))
+		return 0;
+
+	return 0444;
+}
+
+static int lochnagar_read(struct device *dev, enum hwmon_sensor_types type,
+			  u32 attr, int chan, long *val)
+{
+	switch (type) {
+	case hwmon_in:
+		return read_sensor(dev, chan, LN2_VOLTAGE, 1, 1000000000, val);
+	case hwmon_curr:
+		return read_sensor(dev, chan, LN2_CURRENT, 96, 1000000000, val);
+	case hwmon_temp:
+		return read_sensor(dev, chan, LN2_TEMPERATURE, 1023, 1000, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int lochnagar_read_string(struct device *dev,
+				 enum hwmon_sensor_types type, u32 attr,
+				 int chan, const char **str)
+{
+	switch (type) {
+	case hwmon_in:
+	case hwmon_curr:
+		*str = lochnagar_chan_names[chan];
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static const struct hwmon_ops lochnagar_ops = {
+	.is_visible = lochnagar_is_visible,
+	.read = lochnagar_read,
+	.read_string = lochnagar_read_string,
+};
+
+static const struct hwmon_channel_info *lochnagar_info[] = {
+	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
+	HWMON_CHANNEL_INFO(in,   HWMON_I_INPUT | HWMON_I_LABEL,
+				 HWMON_I_INPUT | HWMON_I_LABEL,
+				 HWMON_I_INPUT | HWMON_I_LABEL,
+				 HWMON_I_INPUT | HWMON_I_LABEL,
+				 HWMON_I_INPUT | HWMON_I_LABEL,
+				 HWMON_I_INPUT | HWMON_I_LABEL,
+				 HWMON_I_INPUT | HWMON_I_LABEL,
+				 HWMON_I_INPUT | HWMON_I_LABEL),
+	HWMON_CHANNEL_INFO(curr, HWMON_C_INPUT | HWMON_C_LABEL,
+				 HWMON_C_INPUT | HWMON_C_LABEL,
+				 HWMON_C_INPUT | HWMON_C_LABEL,
+				 HWMON_C_INPUT | HWMON_C_LABEL,
+				 HWMON_C_INPUT | HWMON_C_LABEL,
+				 HWMON_C_INPUT | HWMON_C_LABEL,
+				 HWMON_C_INPUT | HWMON_C_LABEL,
+				 HWMON_C_INPUT | HWMON_C_LABEL),
+	NULL
+};
+
+static const struct hwmon_chip_info lochnagar_chip_info = {
+	.ops = &lochnagar_ops,
+	.info = lochnagar_info,
+};
+
+static const struct of_device_id lochnagar_of_match[] = {
+	{ .compatible = "cirrus,lochnagar2-hwmon" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, lochnagar_of_match);
+
+static int lochnagar_hwmon_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct lochnagar *lochnagar = dev_get_drvdata(dev->parent);
+	struct lochnagar_hwmon *priv;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	mutex_init(&priv->sensor_lock);
+	priv->lochnagar = lochnagar;
+
+	priv->hwmon_dev =
+		devm_hwmon_device_register_with_info(dev, "Lochnagar", priv,
+						     &lochnagar_chip_info,
+						     NULL);
+
+	return PTR_ERR_OR_ZERO(priv->hwmon_dev);
+}
+
+static struct platform_driver lochnagar_hwmon_driver = {
+	.driver = {
+		.name = "lochnagar-hwmon",
+		.of_match_table = lochnagar_of_match,
+	},
+	.probe = lochnagar_hwmon_probe,
+};
+module_platform_driver(lochnagar_hwmon_driver);
+
+MODULE_AUTHOR("Lucas Tanure <tanureal@opensource.cirrus.com>");
+MODULE_DESCRIPTION("Lochnagar hardware monitoring features");
+MODULE_LICENSE("GPL");
-- 
2.11.0

Re: [PATCH 3/3] hwmon: lochnagar: Add Lochnagar 2 hardware monitoring driver

[Guenter Roeck]

On Wed, Mar 20, 2019 at 02:58:18PM +0000, Charles Keepax wrote:
> From: Lucas Tanure <tanureal@opensource.cirrus.com>
> 
> Lochnagar is an evaluation and development board for Cirrus
> Logic Smart CODEC and Amp devices. It allows the connection of
> most Cirrus Logic devices on mini-cards, as well as allowing
> connection of various application processor systems to provide a
> full evaluation platform.
> 
> This driver adds support for the hardware monitoring features of
> the Lochnagar 2 to the hwmon API. Monitoring is provided for
> the board voltages, currents and temperature supported by the
> board controller chip.
> 
> Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---
>  Documentation/hwmon/lochnagar   |  56 ++++++++
>  MAINTAINERS                     |   3 +
>  drivers/hwmon/Kconfig           |  10 ++
>  drivers/hwmon/Makefile          |   1 +
>  drivers/hwmon/lochnagar-hwmon.c | 293 ++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 363 insertions(+)
>  create mode 100644 Documentation/hwmon/lochnagar
>  create mode 100644 drivers/hwmon/lochnagar-hwmon.c
> 
> diff --git a/Documentation/hwmon/lochnagar b/Documentation/hwmon/lochnagar
> new file mode 100644
> index 0000000000000..60e869a8c564e
> --- /dev/null
> +++ b/Documentation/hwmon/lochnagar
> @@ -0,0 +1,56 @@
> +Kernel Driver Lochnagar
> +========================
> +
> +Supported systems:
> +  * Cirrus Logic : Lochnagar 2
> +
> +Author: Lucas A. Tanure Alves
> +
> +Description
> +-----------
> +
> +Lochnagar 2 features built-in Current Monitor circuitry that allows for the
> +measurement of both voltage and current on up to eight of the supply voltage
> +rails provided to the minicards. The Current Monitor does not require any
> +hardware modifications or external circuitry to operate.
> +
> +The current and voltage measurements are obtained through the standard register
> +map interface to the Lochnagar board controller, and can therefore be monitored
> +by software.
> +
> +Sysfs attributes
> +----------------
> +
> +temp1_input   The Lochnagar board temperature.
> +in0_input     Measured voltage for DBVDD1 (nanoVolts)
> +in0_label     "DBVDD1"
> +curr1_input   Measured current for DBVDD1 (nanoAmps)
> +curr1_label   "DBVDD1"
> +in1_input     Measured voltage for 1V8 DSP (nanoVolts)
> +in1_label     "1V8 DSP"
> +curr2_input   Measured current for 1V8 DSP (nanoAmps)
> +curr2_label   "1V8 DSP"
> +in2_input     Measured voltage for 1V8 CDC (nanoVolts)
> +in2_label     "1V8 CDC"
> +curr3_input   Measured current for 1V8 CDC (nanoAmps)
> +curr3_label   "1V8 CDC"
> +in3_input     Measured voltage for VDDCORE DSP (nanoVolts)
> +in3_label     "VDDCORE DSP"
> +curr4_input   Measured current for VDDCORE DSP (nanoAmps)
> +curr4_label   "VDDCORE DSP"
> +in4_input     Measured voltage for AVDD 1V8 (nanoVolts)
> +in4_label     "AVDD 1V8"
> +curr5_input   Measured current for AVDD 1V8 (nanoAmps)
> +curr5_label   "AVDD 1V8"
> +curr6_input   Measured current for SYSVDD (nanoAmps)
> +curr6_label   "SYSVDD"
> +in6_input     Measured voltage for VDDCORE CDC (nanoVolts)
> +in6_label     "VDDCORE CDC"
> +curr7_input   Measured current for VDDCORE CDC (nanoAmps)
> +curr7_label   "VDDCORE CDC"
> +in7_input     Measured voltage for MICVDD (nanoVolts)
> +in7_label     "MICVDD"
> +curr8_input   Measured current for MICVDD (nanoAmps)
> +curr8_label   "MICVDD"
> +
> +Note: It is not possible to measure voltage on the SYSVDD rail.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e17ebf70b5480..4cd3c281d8b56 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3796,6 +3796,7 @@ M:	Richard Fitzgerald <rf@opensource.cirrus.com>
>  L:	patches@opensource.cirrus.com
>  S:	Supported
>  F:	drivers/clk/clk-lochnagar.c
> +F:	drivers/hwmon/lochnagar-hwmon.c
>  F:	drivers/mfd/lochnagar-i2c.c
>  F:	drivers/pinctrl/cirrus/pinctrl-lochnagar.c
>  F:	drivers/regulator/lochnagar-regulator.c
> @@ -3804,8 +3805,10 @@ F:	include/dt-bindings/pinctrl/lochnagar.h
>  F:	include/linux/mfd/lochnagar*
>  F:	Documentation/devicetree/bindings/mfd/cirrus,lochnagar.txt
>  F:	Documentation/devicetree/bindings/clock/cirrus,lochnagar.txt
> +F:	Documentation/devicetree/bindings/hwmon/cirrus,lochnagar.txt
>  F:	Documentation/devicetree/bindings/pinctrl/cirrus,lochnagar.txt
>  F:	Documentation/devicetree/bindings/regulator/cirrus,lochnagar.txt
> +F:	Documentation/hwmon/lochnagar
>  
>  CISCO FCOE HBA DRIVER
>  M:	Satish Kharat <satishkh@cisco.com>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index d0f1dfe2bcbbd..dedd5febd3aa6 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -705,6 +705,16 @@ config SENSORS_LINEAGE
>  	  This driver can also be built as a module. If so, the module
>  	  will be called lineage-pem.
>  
> +config SENSORS_LOCHNAGAR
> +	tristate "Lochnagar Hardware Monitor"
> +	depends on MFD_LOCHNAGAR
> +	help
> +	  If you say yes here you get support for Lochnagar 2 temperature,
> +	  voltage and current sensors abilities.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called lochnagar-hwmon.
> +
>  config SENSORS_LTC2945
>  	tristate "Linear Technology LTC2945"
>  	depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index f5c7b442e69e5..8db472ea04f00 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -89,6 +89,7 @@ obj-$(CONFIG_SENSORS_JZ4740)	+= jz4740-hwmon.o
>  obj-$(CONFIG_SENSORS_K8TEMP)	+= k8temp.o
>  obj-$(CONFIG_SENSORS_K10TEMP)	+= k10temp.o
>  obj-$(CONFIG_SENSORS_LINEAGE)	+= lineage-pem.o
> +obj-$(CONFIG_SENSORS_LOCHNAGAR)	+= lochnagar-hwmon.o
>  obj-$(CONFIG_SENSORS_LM63)	+= lm63.o
>  obj-$(CONFIG_SENSORS_LM70)	+= lm70.o
>  obj-$(CONFIG_SENSORS_LM73)	+= lm73.o
> diff --git a/drivers/hwmon/lochnagar-hwmon.c b/drivers/hwmon/lochnagar-hwmon.c
> new file mode 100644
> index 0000000000000..1f6b82bb9205d
> --- /dev/null
> +++ b/drivers/hwmon/lochnagar-hwmon.c
> @@ -0,0 +1,293 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Lochnagar hardware monitoring features
> + *
> + * Copyright (c) 2016-2019 Cirrus Logic, Inc. and
> + *                         Cirrus Logic International Semiconductor Ltd.
> + *
> + * Author: Lucas Tanure <tanureal@opensource.cirrus.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/platform_device.h>
> +#include <linux/delay.h>
> +#include <linux/mfd/lochnagar.h>
> +#include <linux/mfd/lochnagar2_regs.h>
> +#include <linux/regmap.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/i2c.h>
> +#include <asm/div64.h>
> +

Alphabetic order, please. Also, please include linux/math64.h
instead of asm/div64.h.

> +struct lochnagar_hwmon {
> +	struct lochnagar *lochnagar;

The only use of this structure is to dereference regmap from it.
Please replace it with a pointer to struct regmap.

> +
> +	/* Lock to ensure only a single sensor is read at a time */
> +	struct mutex sensor_lock;
> +	struct device *hwmon_dev;

Not used anywhere.

> +};
> +
> +enum lochnagar_measure_mode {
> +	LN2_CURRENT = 0,
> +	LN2_VOLTAGE,
> +	LN2_TEMPERATURE,
> +};
> +
> +static const char * const lochnagar_chan_names[] = {
> +	"DBVDD1",
> +	"1V8 DSP",
> +	"1V8 CDC",
> +	"VDDCORE DSP",
> +	"AVDD 1V8",
> +	"SYSVDD",
> +	"VDDCORE CDC",
> +	"MICVDD",
> +};
> +
> +static u64 float_to_int(u32 data, u32 precision)

A return type of 'long' would make more sense. Also see below.

> +{
> +	s64 man = data & 0x007FFFFF;
> +	s32 exp = ((data & 0x7F800000) >> 23) - 127 - 23;
> +	bool negative = data & 0x80000000;
> +
> +	man = (man + (1 << 23)) * precision;
> +
> +	if (exp > 0)
> +		man <<= exp;
> +	else if (exp < 0)
> +		man >>= -exp;
> +

Is this an ieee754 floating point format ? Please add a comment stating it.
Also, if the format supports it, please check for NaN. If the HW guarantees
to never return NaN, please add a respective comment.

How likely is it that the values overflow ? A precision multipler of
1,000,000,000 means that numbers will overflow quite easily. And does
the the hardware really report voltages and currents in pico-units,
and are temperatures really reported in micro-degrees C ?

> +	return negative ? -man : man;

The return type is u64. There are no negative numbers. Maybe it should be s64 ?
Even better would be to return and to add a check for overflows (and return
the limit on overflow).

Overall it might make sense to reconfigure the hardware into continuous
measurement mode and get rid of the delays (if that can be configured -
the user guide isn't detailed enough to be able to determine for sure).
After all, it is quite unlikely that the board will be used in an
environment where the power savings would be worth the inconvenience
of having to wait more than two seconds for a set of measurement values
(adding all the current and temperature delays up).

> +}
> +
> +static int do_measurement(struct regmap *regmap, int chan,
> +			  enum lochnagar_measure_mode mode, int nsamples)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	chan = 1 << (chan + LOCHNAGAR2_IMON_MEASURED_CHANNELS_SHIFT);
> +
> +	ret = regmap_write(regmap, LOCHNAGAR2_IMON_CTRL1,
> +			   LOCHNAGAR2_IMON_ENA_MASK | chan | mode);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_write(regmap, LOCHNAGAR2_IMON_CTRL2, nsamples);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_write(regmap, LOCHNAGAR2_IMON_CTRL3,
> +			   LOCHNAGAR2_IMON_CONFIGURE_MASK);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret =  regmap_read_poll_timeout(regmap, LOCHNAGAR2_IMON_CTRL3, val,
> +					val & LOCHNAGAR2_IMON_DONE_MASK,
> +					1000, 10000);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_write(regmap, LOCHNAGAR2_IMON_CTRL3,
> +			   LOCHNAGAR2_IMON_MEASURE_MASK);
> +	if (ret < 0)
> +		return ret;
> +
> +	msleep(nsamples);
> +

This needs some explanation how nsamples translates into millisecond waits,
especially since that wait can add up significantly (reading the temperature
will take forever, as will reading all currents).

> +	ret =  regmap_read_poll_timeout(regmap, LOCHNAGAR2_IMON_CTRL3, val,
> +					val & LOCHNAGAR2_IMON_DONE_MASK,
> +					5000, 2000000);

Can that indeed take another two seconds on top of the sleep above ?

> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_write(regmap, LOCHNAGAR2_IMON_CTRL3, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int request_data(struct regmap *regmap, int chan, u32 *data)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_write(regmap, LOCHNAGAR2_IMON_CTRL4,
> +			   LOCHNAGAR2_IMON_DATA_REQ_MASK |
> +			   chan << LOCHNAGAR2_IMON_CH_SEL_SHIFT);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret =  regmap_read_poll_timeout(regmap, LOCHNAGAR2_IMON_CTRL4, val,
> +					val & LOCHNAGAR2_IMON_DATA_RDY_MASK,
> +					1000, 10000);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_read(regmap, LOCHNAGAR2_IMON_DATA1, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	*data = val << 16;
> +
> +	ret = regmap_read(regmap, LOCHNAGAR2_IMON_DATA2, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	*data |= val;
> +
> +	ret = regmap_write(regmap, LOCHNAGAR2_IMON_CTRL4, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int read_sensor(struct device *dev, int chan,
> +		       enum lochnagar_measure_mode mode, int nsamples,
> +		       unsigned int precision, long *val)
> +{
> +	struct lochnagar_hwmon *priv = dev_get_drvdata(dev);
> +	struct regmap *regmap = priv->lochnagar->regmap;
> +	u32 data;
> +	int ret;
> +
> +	mutex_lock(&priv->sensor_lock);
> +
> +	ret = do_measurement(regmap, chan, mode, nsamples);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to perform measurement: %d\n", ret);
> +		goto error;
> +	}
> +
> +	ret = request_data(regmap, chan, &data);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to read measurement: %d\n", ret);
> +		goto error;
> +	}
> +
> +	*val = float_to_int(data, precision);

float_to_int returns u64. This needs overflow protection, either here or
in float_to_int().

> +
> +error:
> +	mutex_unlock(&priv->sensor_lock);
> +
> +	return ret;
> +}
> +
> +static umode_t lochnagar_is_visible(const void *drvdata,
> +				    enum hwmon_sensor_types type,
> +				    u32 attr, int chan)
> +{
> +	if (type == hwmon_in && !strcmp("SYSVDD", lochnagar_chan_names[chan]))
> +		return 0;
> +
> +	return 0444;
> +}
> +
> +static int lochnagar_read(struct device *dev, enum hwmon_sensor_types type,
> +			  u32 attr, int chan, long *val)
> +{
> +	switch (type) {
> +	case hwmon_in:
> +		return read_sensor(dev, chan, LN2_VOLTAGE, 1, 1000000000, val);
> +	case hwmon_curr:
> +		return read_sensor(dev, chan, LN2_CURRENT, 96, 1000000000, val);

96 ms to read a current value ?

> +	case hwmon_temp:
> +		return read_sensor(dev, chan, LN2_TEMPERATURE, 1023, 1000, val);

With the msleep() in do_measurement(), this means that reading the temperature
will require more than one second, not even counting regmap poll timeouts,
which can take another two seconds. Is this really the case ? It seems
excessively long.

> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int lochnagar_read_string(struct device *dev,
> +				 enum hwmon_sensor_types type, u32 attr,
> +				 int chan, const char **str)
> +{
> +	switch (type) {
> +	case hwmon_in:
> +	case hwmon_curr:
> +		*str = lochnagar_chan_names[chan];
> +		return 0;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static const struct hwmon_ops lochnagar_ops = {
> +	.is_visible = lochnagar_is_visible,
> +	.read = lochnagar_read,
> +	.read_string = lochnagar_read_string,
> +};
> +
> +static const struct hwmon_channel_info *lochnagar_info[] = {
> +	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
> +	HWMON_CHANNEL_INFO(in,   HWMON_I_INPUT | HWMON_I_LABEL,
> +				 HWMON_I_INPUT | HWMON_I_LABEL,
> +				 HWMON_I_INPUT | HWMON_I_LABEL,
> +				 HWMON_I_INPUT | HWMON_I_LABEL,
> +				 HWMON_I_INPUT | HWMON_I_LABEL,
> +				 HWMON_I_INPUT | HWMON_I_LABEL,
> +				 HWMON_I_INPUT | HWMON_I_LABEL,
> +				 HWMON_I_INPUT | HWMON_I_LABEL),
> +	HWMON_CHANNEL_INFO(curr, HWMON_C_INPUT | HWMON_C_LABEL,
> +				 HWMON_C_INPUT | HWMON_C_LABEL,
> +				 HWMON_C_INPUT | HWMON_C_LABEL,
> +				 HWMON_C_INPUT | HWMON_C_LABEL,
> +				 HWMON_C_INPUT | HWMON_C_LABEL,
> +				 HWMON_C_INPUT | HWMON_C_LABEL,
> +				 HWMON_C_INPUT | HWMON_C_LABEL,
> +				 HWMON_C_INPUT | HWMON_C_LABEL),
> +	NULL
> +};
> +
> +static const struct hwmon_chip_info lochnagar_chip_info = {
> +	.ops = &lochnagar_ops,
> +	.info = lochnagar_info,
> +};
> +
> +static const struct of_device_id lochnagar_of_match[] = {
> +	{ .compatible = "cirrus,lochnagar2-hwmon" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, lochnagar_of_match);
> +
> +static int lochnagar_hwmon_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct lochnagar *lochnagar = dev_get_drvdata(dev->parent);
> +	struct lochnagar_hwmon *priv;
> +

It might make sense to add a check to ensure that lochnagar is not NULL.

> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	mutex_init(&priv->sensor_lock);
> +	priv->lochnagar = lochnagar;
> +
> +	priv->hwmon_dev =
> +		devm_hwmon_device_register_with_info(dev, "Lochnagar", priv,
> +						     &lochnagar_chip_info,
> +						     NULL);
> +
> +	return PTR_ERR_OR_ZERO(priv->hwmon_dev);
> +}
> +
> +static struct platform_driver lochnagar_hwmon_driver = {
> +	.driver = {
> +		.name = "lochnagar-hwmon",
> +		.of_match_table = lochnagar_of_match,
> +	},
> +	.probe = lochnagar_hwmon_probe,
> +};
> +module_platform_driver(lochnagar_hwmon_driver);
> +
> +MODULE_AUTHOR("Lucas Tanure <tanureal@opensource.cirrus.com>");
> +MODULE_DESCRIPTION("Lochnagar hardware monitoring features");
> +MODULE_LICENSE("GPL");
> -- 
> 2.11.0
> 

Re: [PATCH 3/3] hwmon: lochnagar: Add Lochnagar 2 hardware monitoring driver

[Charles Keepax]

On Wed, Mar 20, 2019 at 09:40:10AM -0700, Guenter Roeck wrote:
> On Wed, Mar 20, 2019 at 02:58:18PM +0000, Charles Keepax wrote:
> > From: Lucas Tanure <tanureal@opensource.cirrus.com>
> > 
> > Lochnagar is an evaluation and development board for Cirrus
> > Logic Smart CODEC and Amp devices. It allows the connection of
> > most Cirrus Logic devices on mini-cards, as well as allowing
> > connection of various application processor systems to provide a
> > full evaluation platform.
> > 
> > This driver adds support for the hardware monitoring features of
> > the Lochnagar 2 to the hwmon API. Monitoring is provided for
> > the board voltages, currents and temperature supported by the
> > board controller chip.
> > 
> > Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
> > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> > ---
> > +#include <linux/of_device.h>
> > +#include <linux/i2c.h>
> > +#include <asm/div64.h>
> > +
> 
> Alphabetic order, please. Also, please include linux/math64.h
> instead of asm/div64.h.
> 
> > +struct lochnagar_hwmon {
> > +	struct lochnagar *lochnagar;
> 
> The only use of this structure is to dereference regmap from it.
> Please replace it with a pointer to struct regmap.
> 
> > +
> > +	/* Lock to ensure only a single sensor is read at a time */
> > +	struct mutex sensor_lock;
> > +	struct device *hwmon_dev;
> 
> Not used anywhere.
> 
> > +static u64 float_to_int(u32 data, u32 precision)
> 
> A return type of 'long' would make more sense. Also see below.

Yup will get those all fixed up.

> 
> > +{
> > +	s64 man = data & 0x007FFFFF;
> > +	s32 exp = ((data & 0x7F800000) >> 23) - 127 - 23;
> > +	bool negative = data & 0x80000000;
> > +
> > +	man = (man + (1 << 23)) * precision;
> > +
> > +	if (exp > 0)
> > +		man <<= exp;
> > +	else if (exp < 0)
> > +		man >>= -exp;
> > +
> 
> Is this an ieee754 floating point format ? Please add a comment stating it.
> Also, if the format supports it, please check for NaN. If the HW guarantees
> to never return NaN, please add a respective comment.
> 
> How likely is it that the values overflow ? A precision multipler of
> 1,000,000,000 means that numbers will overflow quite easily. And does
> the the hardware really report voltages and currents in pico-units,
> and are temperatures really reported in micro-degrees C ?
> 

I believe the hardware can't return NaN. But will check that,
what ranges are possible and add some overflow checking.

> Overall it might make sense to reconfigure the hardware into continuous
> measurement mode and get rid of the delays (if that can be configured -
> the user guide isn't detailed enough to be able to determine for sure).
> After all, it is quite unlikely that the board will be used in an
> environment where the power savings would be worth the inconvenience
> of having to wait more than two seconds for a set of measurement values
> (adding all the current and temperature delays up).
> 

Yeah the hardware is quite slow and regrettably doesn't have a
continuous measurement mode. We could potentially add a thread to
poll them but mostly the usage for this data is just taking power
measurements of various audio use-cases so the large delay isn't
a huge problem and not sure it warrents the additional
complexity.

> > +	msleep(nsamples);
> > +
> 
> This needs some explanation how nsamples translates into millisecond waits,
> especially since that wait can add up significantly (reading the temperature
> will take forever, as will reading all currents).
> 
> > +	ret =  regmap_read_poll_timeout(regmap, LOCHNAGAR2_IMON_CTRL3, val,
> > +					val & LOCHNAGAR2_IMON_DONE_MASK,
> > +					5000, 2000000);
> 
> Can that indeed take another two seconds on top of the sleep above ?
> 

It does about about 1.5-2 seconds I think last time I checked.

Essentially the hardware will average a number of readings and
return that. The number of readings given in the driver is what
the hardware guys recommended for each, that said we could
potentially make it configurable if that helps at all? Also
I could get the msleep a bit closer to the actual runtime,
just need to check how linear the time is with the number of
samples take (I assume very).

> > +static int lochnagar_hwmon_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct lochnagar *lochnagar = dev_get_drvdata(dev->parent);
> > +	struct lochnagar_hwmon *priv;
> > +
> 
> It might make sense to add a check to ensure that lochnagar is not NULL.
> 

Can do.

Thanks,
Charles

Re: [PATCH 3/3] hwmon: lochnagar: Add Lochnagar 2 hardware monitoring driver

[Guenter Roeck]

On 3/21/19 4:59 AM, Charles Keepax wrote:
> On Wed, Mar 20, 2019 at 09:40:10AM -0700, Guenter Roeck wrote:
>> On Wed, Mar 20, 2019 at 02:58:18PM +0000, Charles Keepax wrote:
>>> From: Lucas Tanure <tanureal@opensource.cirrus.com>
>>>
>>> Lochnagar is an evaluation and development board for Cirrus
>>> Logic Smart CODEC and Amp devices. It allows the connection of
>>> most Cirrus Logic devices on mini-cards, as well as allowing
>>> connection of various application processor systems to provide a
>>> full evaluation platform.
>>>
>>> This driver adds support for the hardware monitoring features of
>>> the Lochnagar 2 to the hwmon API. Monitoring is provided for
>>> the board voltages, currents and temperature supported by the
>>> board controller chip.
>>>
>>> Signed-off-by: Lucas Tanure <tanureal@opensource.cirrus.com>
>>> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
>>> ---
>>> +#include <linux/of_device.h>
>>> +#include <linux/i2c.h>
>>> +#include <asm/div64.h>
>>> +
>>
>> Alphabetic order, please. Also, please include linux/math64.h
>> instead of asm/div64.h.
>>
>>> +struct lochnagar_hwmon {
>>> +	struct lochnagar *lochnagar;
>>
>> The only use of this structure is to dereference regmap from it.
>> Please replace it with a pointer to struct regmap.
>>
>>> +
>>> +	/* Lock to ensure only a single sensor is read at a time */
>>> +	struct mutex sensor_lock;
>>> +	struct device *hwmon_dev;
>>
>> Not used anywhere.
>>
>>> +static u64 float_to_int(u32 data, u32 precision)
>>
>> A return type of 'long' would make more sense. Also see below.
> 
> Yup will get those all fixed up.
> 
>>
>>> +{
>>> +	s64 man = data & 0x007FFFFF;
>>> +	s32 exp = ((data & 0x7F800000) >> 23) - 127 - 23;
>>> +	bool negative = data & 0x80000000;
>>> +
>>> +	man = (man + (1 << 23)) * precision;
>>> +
>>> +	if (exp > 0)
>>> +		man <<= exp;
>>> +	else if (exp < 0)
>>> +		man >>= -exp;
>>> +
>>
>> Is this an ieee754 floating point format ? Please add a comment stating it.
>> Also, if the format supports it, please check for NaN. If the HW guarantees
>> to never return NaN, please add a respective comment.
>>
>> How likely is it that the values overflow ? A precision multipler of
>> 1,000,000,000 means that numbers will overflow quite easily. And does
>> the the hardware really report voltages and currents in pico-units,
>> and are temperatures really reported in micro-degrees C ?
>>
> 
> I believe the hardware can't return NaN. But will check that,
> what ranges are possible and add some overflow checking.
> 

How about the units ? Maybe add a note explaining what the HW actually returns.

>> Overall it might make sense to reconfigure the hardware into continuous
>> measurement mode and get rid of the delays (if that can be configured -
>> the user guide isn't detailed enough to be able to determine for sure).
>> After all, it is quite unlikely that the board will be used in an
>> environment where the power savings would be worth the inconvenience
>> of having to wait more than two seconds for a set of measurement values
>> (adding all the current and temperature delays up).
>>
> 
> Yeah the hardware is quite slow and regrettably doesn't have a
> continuous measurement mode. We could potentially add a thread to
> poll them but mostly the usage for this data is just taking power
> measurements of various audio use-cases so the large delay isn't
> a huge problem and not sure it warrents the additional
> complexity.
> 
>>> +	msleep(nsamples);
>>> +
>>
>> This needs some explanation how nsamples translates into millisecond waits,
>> especially since that wait can add up significantly (reading the temperature
>> will take forever, as will reading all currents).
>>
>>> +	ret =  regmap_read_poll_timeout(regmap, LOCHNAGAR2_IMON_CTRL3, val,
>>> +					val & LOCHNAGAR2_IMON_DONE_MASK,
>>> +					5000, 2000000);
>>
>> Can that indeed take another two seconds on top of the sleep above ?
>>
> 
> It does about about 1.5-2 seconds I think last time I checked.
> 
> Essentially the hardware will average a number of readings and
> return that. The number of readings given in the driver is what
> the hardware guys recommended for each, that said we could
> potentially make it configurable if that helps at all? Also
> I could get the msleep a bit closer to the actual runtime,
> just need to check how linear the time is with the number of
> samples take (I assume very).
> 

Brr. Just add a note explaining that the long times are indeed intentional,
and that the HW takes that long.

Thanks,
Guenter

>>> +static int lochnagar_hwmon_probe(struct platform_device *pdev)
>>> +{
>>> +	struct device *dev = &pdev->dev;
>>> +	struct lochnagar *lochnagar = dev_get_drvdata(dev->parent);
>>> +	struct lochnagar_hwmon *priv;
>>> +
>>
>> It might make sense to add a check to ensure that lochnagar is not NULL.
>>
> 
> Can do.
> 
> Thanks,
> Charles
> 

Re: [PATCH 3/3] hwmon: lochnagar: Add Lochnagar 2 hardware monitoring driver

[Charles Keepax]

On Thu, Mar 21, 2019 at 06:14:23AM -0700, Guenter Roeck wrote:
> On 3/21/19 4:59 AM, Charles Keepax wrote:
> >On Wed, Mar 20, 2019 at 09:40:10AM -0700, Guenter Roeck wrote:
> >>On Wed, Mar 20, 2019 at 02:58:18PM +0000, Charles Keepax wrote:
> >>>From: Lucas Tanure <tanureal@opensource.cirrus.com>
> >>Is this an ieee754 floating point format ? Please add a comment stating it.
> >>Also, if the format supports it, please check for NaN. If the HW guarantees
> >>to never return NaN, please add a respective comment.
> >>
> >>How likely is it that the values overflow ? A precision multipler of
> >>1,000,000,000 means that numbers will overflow quite easily. And does
> >>the the hardware really report voltages and currents in pico-units,
> >>and are temperatures really reported in micro-degrees C ?
> >>
> >
> >I believe the hardware can't return NaN. But will check that,
> >what ranges are possible and add some overflow checking.
> >
> 
> How about the units ? Maybe add a note explaining what the HW actually returns.
> 

Yup can do.

> >>Overall it might make sense to reconfigure the hardware into continuous
> >>measurement mode and get rid of the delays (if that can be configured -
> >>the user guide isn't detailed enough to be able to determine for sure).
> >>After all, it is quite unlikely that the board will be used in an
> >>environment where the power savings would be worth the inconvenience
> >>of having to wait more than two seconds for a set of measurement values
> >>(adding all the current and temperature delays up).
> >>
> >
> >Yeah the hardware is quite slow and regrettably doesn't have a
> >continuous measurement mode. We could potentially add a thread to
> >poll them but mostly the usage for this data is just taking power
> >measurements of various audio use-cases so the large delay isn't
> >a huge problem and not sure it warrents the additional
> >complexity.
> >
> >>>+	msleep(nsamples);
> >>>+
> >>
> >>This needs some explanation how nsamples translates into millisecond waits,
> >>especially since that wait can add up significantly (reading the temperature
> >>will take forever, as will reading all currents).
> >>
> >>>+	ret =  regmap_read_poll_timeout(regmap, LOCHNAGAR2_IMON_CTRL3, val,
> >>>+					val & LOCHNAGAR2_IMON_DONE_MASK,
> >>>+					5000, 2000000);
> >>
> >>Can that indeed take another two seconds on top of the sleep above ?
> >>
> >
> >It does about about 1.5-2 seconds I think last time I checked.
> >
> >Essentially the hardware will average a number of readings and
> >return that. The number of readings given in the driver is what
> >the hardware guys recommended for each, that said we could
> >potentially make it configurable if that helps at all? Also
> >I could get the msleep a bit closer to the actual runtime,
> >just need to check how linear the time is with the number of
> >samples take (I assume very).
> >
> 
> Brr. Just add a note explaining that the long times are indeed intentional,
> and that the HW takes that long.
> 

From some more detailed discussions with the hardware guys
(shame on me for following the documentation) it seems
there isn't really much need for the temp to take so many
measurements. The current one makes sense as the hardware
actually does some analogue averaging so it should take account
of spikes in the current draw from the part under test. But
the temp should really only change slowly. I have also managed
to refine the estimates for the time taken by the measurements
so will factor that lot into the v2 as well.

Thanks very much for the review, will probably take me a few more
days to beat the last few clarifications out of the hardware guys
then I will fire up a v2.

Thanks,
Charles

Re: [PATCH 3/3] hwmon: lochnagar: Add Lochnagar 2 hardware monitoring driver

[Guenter Roeck]

On Thu, Mar 21, 2019 at 03:47:53PM +0000, Charles Keepax wrote:
> On Thu, Mar 21, 2019 at 06:14:23AM -0700, Guenter Roeck wrote:
> > On 3/21/19 4:59 AM, Charles Keepax wrote:
> > >On Wed, Mar 20, 2019 at 09:40:10AM -0700, Guenter Roeck wrote:
> > >>On Wed, Mar 20, 2019 at 02:58:18PM +0000, Charles Keepax wrote:
> > >>>From: Lucas Tanure <tanureal@opensource.cirrus.com>
> > >>Is this an ieee754 floating point format ? Please add a comment stating it.
> > >>Also, if the format supports it, please check for NaN. If the HW guarantees
> > >>to never return NaN, please add a respective comment.
> > >>
> > >>How likely is it that the values overflow ? A precision multipler of
> > >>1,000,000,000 means that numbers will overflow quite easily. And does
> > >>the the hardware really report voltages and currents in pico-units,
> > >>and are temperatures really reported in micro-degrees C ?
> > >>
> > >
> > >I believe the hardware can't return NaN. But will check that,
> > >what ranges are possible and add some overflow checking.
> > >
> > 
> > How about the units ? Maybe add a note explaining what the HW actually returns.
> > 
> 
> Yup can do.
> 
> > >>Overall it might make sense to reconfigure the hardware into continuous
> > >>measurement mode and get rid of the delays (if that can be configured -
> > >>the user guide isn't detailed enough to be able to determine for sure).
> > >>After all, it is quite unlikely that the board will be used in an
> > >>environment where the power savings would be worth the inconvenience
> > >>of having to wait more than two seconds for a set of measurement values
> > >>(adding all the current and temperature delays up).
> > >>
> > >
> > >Yeah the hardware is quite slow and regrettably doesn't have a
> > >continuous measurement mode. We could potentially add a thread to
> > >poll them but mostly the usage for this data is just taking power
> > >measurements of various audio use-cases so the large delay isn't
> > >a huge problem and not sure it warrents the additional
> > >complexity.
> > >
> > >>>+	msleep(nsamples);
> > >>>+
> > >>
> > >>This needs some explanation how nsamples translates into millisecond waits,
> > >>especially since that wait can add up significantly (reading the temperature
> > >>will take forever, as will reading all currents).
> > >>
> > >>>+	ret =  regmap_read_poll_timeout(regmap, LOCHNAGAR2_IMON_CTRL3, val,
> > >>>+					val & LOCHNAGAR2_IMON_DONE_MASK,
> > >>>+					5000, 2000000);
> > >>
> > >>Can that indeed take another two seconds on top of the sleep above ?
> > >>
> > >
> > >It does about about 1.5-2 seconds I think last time I checked.
> > >
> > >Essentially the hardware will average a number of readings and
> > >return that. The number of readings given in the driver is what
> > >the hardware guys recommended for each, that said we could
> > >potentially make it configurable if that helps at all? Also
> > >I could get the msleep a bit closer to the actual runtime,
> > >just need to check how linear the time is with the number of
> > >samples take (I assume very).
> > >
> > 
> > Brr. Just add a note explaining that the long times are indeed intentional,
> > and that the HW takes that long.
> > 
> 
> From some more detailed discussions with the hardware guys
> (shame on me for following the documentation) it seems
> there isn't really much need for the temp to take so many
> measurements. The current one makes sense as the hardware
> actually does some analogue averaging so it should take account
> of spikes in the current draw from the part under test. But
> the temp should really only change slowly. I have also managed
> to refine the estimates for the time taken by the measurements
> so will factor that lot into the v2 as well.
> 

Ok, that makes more sense.

> Thanks very much for the review, will probably take me a few more

Mu pleasure. And many thanks for the new macro - that is really very useful.

> days to beat the last few clarifications out of the hardware guys
> then I will fire up a v2.
> 

Take your time. It is a few weeks until the next commit window,
after all.

Thanks,
Guenter