[v2,1/2] hwmon: lochnagar: Add device tree binding document

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

On 3/25/19 4:00 AM, 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>
> ---
> 
> Changes since v1:
>   - Put includes into alphabetical order
>   - Remove lochnagar pointer and use regmap directly
>   - Add documentation and overflow protection on float conversion
>   - Tighten up delays on waiting for the reading to be available
>   - Drop current/voltage sensors from nano to micro volts/amps. The
>     hardware does not have sufficent accuracy for nano scale
>     measurements.
>   - Only take one sample for temperature readings as there is no
>     need to average the reading, temps should change slowly.
>   - Remove duplicated copyright notice since we already have the
>     SPDX header
> 
> Thanks,
> Charles
> 
>   Documentation/hwmon/lochnagar   |  56 +++++++
>   MAINTAINERS                     |   3 +
>   drivers/hwmon/Kconfig           |  10 ++
>   drivers/hwmon/Makefile          |   1 +
>   drivers/hwmon/lochnagar-hwmon.c | 315 ++++++++++++++++++++++++++++++++++++++++
>   5 files changed, 385 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..7b40b0fffc3e0
> --- /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 (milliCelsius).
> +in0_input     Measured voltage for DBVDD1 (microVolts)
> +in0_label     "DBVDD1"
> +curr1_input   Measured current for DBVDD1 (microAmps)
> +curr1_label   "DBVDD1"
> +in1_input     Measured voltage for 1V8 DSP (microVolts)
> +in1_label     "1V8 DSP"
> +curr2_input   Measured current for 1V8 DSP (microAmps)
> +curr2_label   "1V8 DSP"
> +in2_input     Measured voltage for 1V8 CDC (microVolts)
> +in2_label     "1V8 CDC"
> +curr3_input   Measured current for 1V8 CDC (microAmps)
> +curr3_label   "1V8 CDC"
> +in3_input     Measured voltage for VDDCORE DSP (microVolts)
> +in3_label     "VDDCORE DSP"
> +curr4_input   Measured current for VDDCORE DSP (microAmps)
> +curr4_label   "VDDCORE DSP"
> +in4_input     Measured voltage for AVDD 1V8 (microVolts)
> +in4_label     "AVDD 1V8"
> +curr5_input   Measured current for AVDD 1V8 (microAmps)
> +curr5_label   "AVDD 1V8"
> +curr6_input   Measured current for SYSVDD (microAmps)
> +curr6_label   "SYSVDD"
> +in6_input     Measured voltage for VDDCORE CDC (microVolts)
> +in6_label     "VDDCORE CDC"
> +curr7_input   Measured current for VDDCORE CDC (microAmps)
> +curr7_label   "VDDCORE CDC"
> +in7_input     Measured voltage for MICVDD (microVolts)
> +in7_label     "MICVDD"
> +curr8_input   Measured current for MICVDD (microAmps)
> +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..cbd15adac7f37
> --- /dev/null
> +++ b/drivers/hwmon/lochnagar-hwmon.c
> @@ -0,0 +1,315 @@
> +// 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>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/i2c.h>
> +#include <linux/math64.h>
> +#include <linux/mfd/lochnagar.h>
> +#include <linux/mfd/lochnagar2_regs.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +struct lochnagar_hwmon {
> +	struct regmap *regmap;
> +
> +	/* Lock to ensure only a single sensor is read at a time */
> +	struct mutex sensor_lock;
> +};
> +
> +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",
> +};
> +
> +/**
> + * float_to_long - Convert ieee754 reading from hardware to an integer
> + *
> + * @data: Value read from the hardware
> + * @precision: Units to multiply up to eg. 1000 = milli, 1000000 = micro
> + *
> + * Return: Converted integer reading
> + *
> + * Depending on the measurement type the hardware returns an ieee754
> + * floating point value in either volts, amps or celsius. This function
> + * will convert that into an integer in a smaller unit such as micro-amps

The hwmon ABI says that voltages and current shall be reported in milli-units.
There is no option or means to report micro-units to userspace. A value of
1000 as reported to userspace means 1 Volt or 1 Ampere. Nothing else.
The above and the summary suggest that the intent here is to report units
to userspace in micro-units. This is against the ABI and not permitted.

Besides, if it takes 96 samples to read the current, an accuracy in uA has zero
value. Similar really applies to uV - it is hard to believe that any HW would be
able to return values with such an accuracy.

> + * or milli-celsius. The hardware does not return NaN, so consideration of
> + * that is not required.
> + */
> +static long float_to_long(u32 data, u32 precision)
> +{
> +	u64 man = data & 0x007FFFFF;
> +	int exp = ((data & 0x7F800000) >> 23) - 127 - 23;
> +	bool negative = data & 0x80000000;
> +	long result;
> +
> +	man = (man + (1 << 23)) * precision;
> +
> +	if (fls64(man) + exp > (int)sizeof(long) * 8 - 1)
> +		result = LONG_MAX;
> +	else if (exp < 0)
> +		result = man >> -exp;
> +	else
> +		result = man << exp;
> +
> +	return negative ? -result : result;
> +}
> +
> +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;
> +
> +	/*
> +	 * Actual measurement time is ~1.67mS per sample, approximate
> +	 * this with a 1.5mS per sample msleep and then poll for
> +	 * success upto ~0.17mS * 1023 (max nsamples). Normally for

up to

> +	 * smaller values of nsamples the poll will complete on the
> +	 * first loop due to other latency in the system.
> +	 */
> +	msleep((nsamples * 2) - (nsamples / 2));

nsamples * 3 / 2 might be a bit easier to read.

> +
> +	ret =  regmap_read_poll_timeout(regmap, LOCHNAGAR2_IMON_CTRL3, val,
> +					val & LOCHNAGAR2_IMON_DONE_MASK,
> +					5000, 200000);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_write(regmap, LOCHNAGAR2_IMON_CTRL3, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;

Why not just return the result of regmap_write() ?

> +}
> +
> +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;

	return regmap_write() ?

> +}
> +
> +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->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_long(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, 1000000, val);
> +	case hwmon_curr:
> +		return read_sensor(dev, chan, LN2_CURRENT, 96, 1000000, val);
> +	case hwmon_temp:
> +		return read_sensor(dev, chan, LN2_TEMPERATURE, 1, 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 device *hwmon_dev;
> +	struct lochnagar_hwmon *priv;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	mutex_init(&priv->sensor_lock);
> +	priv->regmap = dev_get_regmap(dev->parent, NULL);
> +	if (!priv->regmap) {
> +		dev_err(dev, "No register map found\n");
> +		return -EINVAL;
> +	}
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, "Lochnagar", priv,
> +							 &lochnagar_chip_info,
> +							 NULL);
> +
> +	return PTR_ERR_OR_ZERO(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");
>

On Mon, Mar 25, 2019 at 04:17:31AM -0700, Guenter Roeck wrote:
> On 3/25/19 4:00 AM, 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>
> >---
> >+/**
> >+ * float_to_long - Convert ieee754 reading from hardware to an integer
> >+ *
> >+ * @data: Value read from the hardware
> >+ * @precision: Units to multiply up to eg. 1000 = milli, 1000000 = micro
> >+ *
> >+ * Return: Converted integer reading
> >+ *
> >+ * Depending on the measurement type the hardware returns an ieee754
> >+ * floating point value in either volts, amps or celsius. This function
> >+ * will convert that into an integer in a smaller unit such as micro-amps
> 
> The hwmon ABI says that voltages and current shall be reported in milli-units.
> There is no option or means to report micro-units to userspace. A value of
> 1000 as reported to userspace means 1 Volt or 1 Ampere. Nothing else.
> The above and the summary suggest that the intent here is to report units
> to userspace in micro-units. This is against the ABI and not permitted.
> 

This is the intent, mostly coming from advice from your email on
the 18th Feb 2017 (see below). At the time Lucas had been talking
about nano-amps but really the hardware is only accurate down to
about 10uA or so.

> You could on purpose violate the ABI and report the current in
> nano-Amps with currX_input
> (properly documented). This way the sensors command would still work after proper
> adjustments in /etc/sensors3.conf. If the current sense resistor is configurable,
> you could also not tell me at all and assume a current sense resistor which is
> 1,000,000 times larger than the one you actually use (after all, currency is
> usually measured as voltage loss over a current sense resistor).

> Another possibility would be to add a curr1_input_na attribute, but I would prefer
> the above mechanism.

Apologies couldn't find an archive for the hwmon list going
back far enough, so just had to copy that in, hopefully your
own records go back far enough. I am certainly open to other
options such as your other suggest of a newer higher precision
file being added.

> Besides, if it takes 96 samples to read the current, an accuracy in uA has zero
> value. Similar really applies to uV - it is hard to believe that any HW would be
> able to return values with such an accuracy.

This bit however I have to disagree with.

The purpose of this monitor hardware is generally accessing the
power consumption of embedded audio hardware. Our hardware will
frequently be operating in situations where it is drawing only
a couple or even sometimes less than a milli-amp. So reporting
on the scale of milli-amps is certainly not acceptable for
our use-cases.

As regards the averaging, the draw of the audio hardware can be
quite spikey and since we want to check the power consumption
averaging is greatly preferable. As I mentioned in my previous
email the average actually takes place in analog so it helps
us get power numbers that include the spikes. Without that one
could greatly over/under estimate depending on your luck with
the timing of the current measurements.

We could potentially switch to millivolts rather than microvolts,
as our use-cases don't generally need the precision on voltage,
if you prefer? But for current we will need to come up with some
solution to pass more accurate measurements.

> >+	/*
> >+	 * Actual measurement time is ~1.67mS per sample, approximate
> >+	 * this with a 1.5mS per sample msleep and then poll for
> >+	 * success upto ~0.17mS * 1023 (max nsamples). Normally for
> 
> up to
> 
> >+	 * smaller values of nsamples the poll will complete on the
> >+	 * first loop due to other latency in the system.
> >+	 */
> >+	msleep((nsamples * 2) - (nsamples / 2));
> 
> nsamples * 3 / 2 might be a bit easier to read.
> 
> >+	ret = regmap_write(regmap, LOCHNAGAR2_IMON_CTRL3, 0);
> >+	if (ret < 0)
> >+		return ret;
> >+
> >+	return 0;
> 
> Why not just return the result of regmap_write() ?
> 
> >+	ret = regmap_write(regmap, LOCHNAGAR2_IMON_CTRL4, 0);
> >+	if (ret < 0)
> >+		return ret;
> >+
> >+	return 0;
> 
> 	return regmap_write() ?
> 

No problem with any of these will get them fixed up for v3.

Thanks,
Charles

On Mon, Mar 25, 2019 at 01:16:51PM +0000, Charles Keepax wrote:
> On Mon, Mar 25, 2019 at 04:17:31AM -0700, Guenter Roeck wrote:
> > On 3/25/19 4:00 AM, 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>
> > >---
> > >+/**
> > >+ * float_to_long - Convert ieee754 reading from hardware to an integer
> > >+ *
> > >+ * @data: Value read from the hardware
> > >+ * @precision: Units to multiply up to eg. 1000 = milli, 1000000 = micro
> > >+ *
> > >+ * Return: Converted integer reading
> > >+ *
> > >+ * Depending on the measurement type the hardware returns an ieee754
> > >+ * floating point value in either volts, amps or celsius. This function
> > >+ * will convert that into an integer in a smaller unit such as micro-amps
> > 
> > The hwmon ABI says that voltages and current shall be reported in milli-units.
> > There is no option or means to report micro-units to userspace. A value of
> > 1000 as reported to userspace means 1 Volt or 1 Ampere. Nothing else.
> > The above and the summary suggest that the intent here is to report units
> > to userspace in micro-units. This is against the ABI and not permitted.
> > 
> 
> This is the intent, mostly coming from advice from your email on
> the 18th Feb 2017 (see below). At the time Lucas had been talking
> about nano-amps but really the hardware is only accurate down to
> about 10uA or so.
> 
> > You could on purpose violate the ABI and report the current in
> > nano-Amps with currX_input
> > (properly documented). This way the sensors command would still work after proper
> > adjustments in /etc/sensors3.conf. If the current sense resistor is configurable,
> > you could also not tell me at all and assume a current sense resistor which is
> > 1,000,000 times larger than the one you actually use (after all, currency is
> > usually measured as voltage loss over a current sense resistor).
> 
> > Another possibility would be to add a curr1_input_na attribute, but I would prefer
> > the above mechanism.
> 
> Apologies couldn't find an archive for the hwmon list going
> back far enough, so just had to copy that in, hopefully your
> own records go back far enough. I am certainly open to other
> options such as your other suggest of a newer higher precision
> file being added.
> 
Try lore.kernel.org/linux-hwmon.

> > Besides, if it takes 96 samples to read the current, an accuracy in uA has zero
> > value. Similar really applies to uV - it is hard to believe that any HW would be
> > able to return values with such an accuracy.
> 
> This bit however I have to disagree with.
> 
> The purpose of this monitor hardware is generally accessing the
> power consumption of embedded audio hardware. Our hardware will
> frequently be operating in situations where it is drawing only
> a couple or even sometimes less than a milli-amp. So reporting
> on the scale of milli-amps is certainly not acceptable for
> our use-cases.
> 
> As regards the averaging, the draw of the audio hardware can be
> quite spikey and since we want to check the power consumption
> averaging is greatly preferable. As I mentioned in my previous
> email the average actually takes place in analog so it helps
> us get power numbers that include the spikes. Without that one
> could greatly over/under estimate depending on your luck with
> the timing of the current measurements.
> 
That makes some sense. Maybe add a note to the code describing the
reason for taking several measurements ?

> We could potentially switch to millivolts rather than microvolts,
> as our use-cases don't generally need the precision on voltage,
> if you prefer? But for current we will need to come up with some
> solution to pass more accurate measurements.
> 
I do have some second thoughts about my 2017 comments. From an ABI
perspective, it is highly undesirable to provide false data on purpose.

Do you need micro-units for all measurements ? If not, or even if so,
a simple solution might be to provide currX_input_ua and possibly
inX_input_uv for those attributes where needed, in addition to the
default attributes, which would be reported in mV / mA. That would meet
both the ABI and your requirements.

There is another possibility which might meet your requirements: If the
important attribute is power consumption, you might consider providing
power attributes. Those _are provided in micro-units. That isn't exactly
as expected, as drivers should only provide power attributes if actually
reported by the HW, but there is an argument to make that it makes sense
here. You could then even provide powerX_average and make the number
of samples indirectly configurable with powerX_average_interval.

Thanks,
Guenter

> > >+	/*
> > >+	 * Actual measurement time is ~1.67mS per sample, approximate
> > >+	 * this with a 1.5mS per sample msleep and then poll for
> > >+	 * success upto ~0.17mS * 1023 (max nsamples). Normally for
> > 
> > up to
> > 
> > >+	 * smaller values of nsamples the poll will complete on the
> > >+	 * first loop due to other latency in the system.
> > >+	 */
> > >+	msleep((nsamples * 2) - (nsamples / 2));
> > 
> > nsamples * 3 / 2 might be a bit easier to read.
> > 
> > >+	ret = regmap_write(regmap, LOCHNAGAR2_IMON_CTRL3, 0);
> > >+	if (ret < 0)
> > >+		return ret;
> > >+
> > >+	return 0;
> > 
> > Why not just return the result of regmap_write() ?
> > 
> > >+	ret = regmap_write(regmap, LOCHNAGAR2_IMON_CTRL4, 0);
> > >+	if (ret < 0)
> > >+		return ret;
> > >+
> > >+	return 0;
> > 
> > 	return regmap_write() ?
> > 
> 
> No problem with any of these will get them fixed up for v3.
> 
> Thanks,
> Charles

On Mon, Mar 25, 2019 at 09:46:22AM -0700, Guenter Roeck wrote:
> On Mon, Mar 25, 2019 at 01:16:51PM +0000, Charles Keepax wrote:
> > On Mon, Mar 25, 2019 at 04:17:31AM -0700, Guenter Roeck wrote:
> > > On 3/25/19 4:00 AM, Charles Keepax wrote:
> > > >From: Lucas Tanure <tanureal@opensource.cirrus.com>
> > Apologies couldn't find an archive for the hwmon list going
> > back far enough, so just had to copy that in, hopefully your
> > own records go back far enough. I am certainly open to other
> > options such as your other suggest of a newer higher precision
> > file being added.
> > 
> Try lore.kernel.org/linux-hwmon.
> 

Indeed here we go:

https://lore.kernel.org/linux-hwmon/32c30882-2466-a992-158b-870cc8af5b40@opensource.cirrus.com/

Apologies for not finding that, over reliance on Google.

> > As regards the averaging, the draw of the audio hardware can be
> > quite spikey and since we want to check the power consumption
> > averaging is greatly preferable. As I mentioned in my previous
> > email the average actually takes place in analog so it helps
> > us get power numbers that include the spikes. Without that one
> > could greatly over/under estimate depending on your luck with
> > the timing of the current measurements.
> > 
> That makes some sense. Maybe add a note to the code describing the
> reason for taking several measurements ?
> 

Can do.

> > We could potentially switch to millivolts rather than microvolts,
> > as our use-cases don't generally need the precision on voltage,
> > if you prefer? But for current we will need to come up with some
> > solution to pass more accurate measurements.
> > 
> I do have some second thoughts about my 2017 comments. From an ABI
> perspective, it is highly undesirable to provide false data on purpose.
> 
> Do you need micro-units for all measurements ? If not, or even if so,
> a simple solution might be to provide currX_input_ua and possibly
> inX_input_uv for those attributes where needed, in addition to the
> default attributes, which would be reported in mV / mA. That would meet
> both the ABI and your requirements.
> 
> There is another possibility which might meet your requirements: If the
> important attribute is power consumption, you might consider providing
> power attributes. Those _are provided in micro-units. That isn't exactly
> as expected, as drivers should only provide power attributes if actually
> reported by the HW, but there is an argument to make that it makes sense
> here. You could then even provide powerX_average and make the number
> of samples indirectly configurable with powerX_average_interval.
> 

Thank you for the suggestions I will investigate them and see
where I get to. The power option does sound tempting as the
ability to configure the number of samples feels like something
that could be handy in the future. But on the flip side adding
high accuracy APIs might be useful for others in the future.

Thanks,
Charles

On Mon, Mar 25, 2019 at 05:10:54PM +0000, Charles Keepax wrote:
> On Mon, Mar 25, 2019 at 09:46:22AM -0700, Guenter Roeck wrote:
> > On Mon, Mar 25, 2019 at 01:16:51PM +0000, Charles Keepax wrote:
> > > On Mon, Mar 25, 2019 at 04:17:31AM -0700, Guenter Roeck wrote:
> > > > On 3/25/19 4:00 AM, Charles Keepax wrote:
> > > > >From: Lucas Tanure <tanureal@opensource.cirrus.com>
> > > Apologies couldn't find an archive for the hwmon list going
> > > back far enough, so just had to copy that in, hopefully your
> > > own records go back far enough. I am certainly open to other
> > > options such as your other suggest of a newer higher precision
> > > file being added.
> > > 
> > Try lore.kernel.org/linux-hwmon.
> > 
> 
> Indeed here we go:
> 
> https://lore.kernel.org/linux-hwmon/32c30882-2466-a992-158b-870cc8af5b40@opensource.cirrus.com/
> 
> Apologies for not finding that, over reliance on Google.
> 
> > > As regards the averaging, the draw of the audio hardware can be
> > > quite spikey and since we want to check the power consumption
> > > averaging is greatly preferable. As I mentioned in my previous
> > > email the average actually takes place in analog so it helps
> > > us get power numbers that include the spikes. Without that one
> > > could greatly over/under estimate depending on your luck with
> > > the timing of the current measurements.
> > > 
> > That makes some sense. Maybe add a note to the code describing the
> > reason for taking several measurements ?
> > 
> 
> Can do.
> 
> > > We could potentially switch to millivolts rather than microvolts,
> > > as our use-cases don't generally need the precision on voltage,
> > > if you prefer? But for current we will need to come up with some
> > > solution to pass more accurate measurements.
> > > 
> > I do have some second thoughts about my 2017 comments. From an ABI
> > perspective, it is highly undesirable to provide false data on purpose.
> > 
> > Do you need micro-units for all measurements ? If not, or even if so,
> > a simple solution might be to provide currX_input_ua and possibly
> > inX_input_uv for those attributes where needed, in addition to the
> > default attributes, which would be reported in mV / mA. That would meet
> > both the ABI and your requirements.
> > 
> > There is another possibility which might meet your requirements: If the
> > important attribute is power consumption, you might consider providing
> > power attributes. Those _are provided in micro-units. That isn't exactly
> > as expected, as drivers should only provide power attributes if actually
> > reported by the HW, but there is an argument to make that it makes sense
> > here. You could then even provide powerX_average and make the number
> > of samples indirectly configurable with powerX_average_interval.
> > 
> 
> Thank you for the suggestions I will investigate them and see
> where I get to. The power option does sound tempting as the
> ability to configure the number of samples feels like something
> that could be handy in the future. But on the flip side adding
> high accuracy APIs might be useful for others in the future.
> 
Agreed, but we would have to think about it more before jumping into
it. There would be several possible solutions. Adding new sysfs files
might be one. Another might be "scale" attributes or similar. Or get rid
of the sysfs ABI entirely and use something similar to iio. Or go further
and create a hwmon->iio bridge and use iio to report high resolution
information.

Guenter

On Mon, Mar 25, 2019 at 11:47:11AM -0700, Guenter Roeck wrote:
> On Mon, Mar 25, 2019 at 05:10:54PM +0000, Charles Keepax wrote:
> > On Mon, Mar 25, 2019 at 09:46:22AM -0700, Guenter Roeck wrote:
> > > On Mon, Mar 25, 2019 at 01:16:51PM +0000, Charles Keepax wrote:
> > > > On Mon, Mar 25, 2019 at 04:17:31AM -0700, Guenter Roeck wrote:
> > > > > On 3/25/19 4:00 AM, Charles Keepax wrote:
> > > > > >From: Lucas Tanure <tanureal@opensource.cirrus.com>
> > > There is another possibility which might meet your requirements: If the
> > > important attribute is power consumption, you might consider providing
> > > power attributes. Those _are provided in micro-units. That isn't exactly
> > > as expected, as drivers should only provide power attributes if actually
> > > reported by the HW, but there is an argument to make that it makes sense
> > > here. You could then even provide powerX_average and make the number
> > > of samples indirectly configurable with powerX_average_interval.
> > > 
> > 
> > Thank you for the suggestions I will investigate them and see
> > where I get to. The power option does sound tempting as the
> > ability to configure the number of samples feels like something
> > that could be handy in the future. But on the flip side adding
> > high accuracy APIs might be useful for others in the future.
> > 
> Agreed, but we would have to think about it more before jumping into
> it. There would be several possible solutions. Adding new sysfs files
> might be one. Another might be "scale" attributes or similar. Or get rid
> of the sysfs ABI entirely and use something similar to iio. Or go further
> and create a hwmon->iio bridge and use iio to report high resolution
> information.
> 

Ok I think I will have a look at the using the power entries
first and we can see what that looks like. I am leaning in that
direction as it would be nice to get something merged in the not
too distant future and having configuration for the averaging
does seem like it protects against the hardware guys going "for
this project we need to average over this many samples".

On another note I have not really done a lot with hwmon/iio
before, my assumption was this should really be an hwmon device
since it is monitoring the state of the hardware and only
supports simple single readings, no buffering etc. Are you also
comfortable that this is the sub-system this device belongs in?

Thanks,
Charles

On 3/26/19 3:05 AM, Charles Keepax wrote:
> On Mon, Mar 25, 2019 at 11:47:11AM -0700, Guenter Roeck wrote:
>> On Mon, Mar 25, 2019 at 05:10:54PM +0000, Charles Keepax wrote:
>>> On Mon, Mar 25, 2019 at 09:46:22AM -0700, Guenter Roeck wrote:
>>>> On Mon, Mar 25, 2019 at 01:16:51PM +0000, Charles Keepax wrote:
>>>>> On Mon, Mar 25, 2019 at 04:17:31AM -0700, Guenter Roeck wrote:
>>>>>> On 3/25/19 4:00 AM, Charles Keepax wrote:
>>>>>>> From: Lucas Tanure <tanureal@opensource.cirrus.com>
>>>> There is another possibility which might meet your requirements: If the
>>>> important attribute is power consumption, you might consider providing
>>>> power attributes. Those _are provided in micro-units. That isn't exactly
>>>> as expected, as drivers should only provide power attributes if actually
>>>> reported by the HW, but there is an argument to make that it makes sense
>>>> here. You could then even provide powerX_average and make the number
>>>> of samples indirectly configurable with powerX_average_interval.
>>>>
>>>
>>> Thank you for the suggestions I will investigate them and see
>>> where I get to. The power option does sound tempting as the
>>> ability to configure the number of samples feels like something
>>> that could be handy in the future. But on the flip side adding
>>> high accuracy APIs might be useful for others in the future.
>>>
>> Agreed, but we would have to think about it more before jumping into
>> it. There would be several possible solutions. Adding new sysfs files
>> might be one. Another might be "scale" attributes or similar. Or get rid
>> of the sysfs ABI entirely and use something similar to iio. Or go further
>> and create a hwmon->iio bridge and use iio to report high resolution
>> information.
>>
> 
> Ok I think I will have a look at the using the power entries
> first and we can see what that looks like. I am leaning in that
> direction as it would be nice to get something merged in the not
> too distant future and having configuration for the averaging
> does seem like it protects against the hardware guys going "for
> this project we need to average over this many samples".
> 
> On another note I have not really done a lot with hwmon/iio
> before, my assumption was this should really be an hwmon device
> since it is monitoring the state of the hardware and only
> supports simple single readings, no buffering etc. Are you also
> comfortable that this is the sub-system this device belongs in?
> 

Yes. I was talking about the ABI, not the subsystem.

Guenter