[PATCH 0/4] misc: Add power-efuse driver

[PATCH 0/4] misc: Add power-efuse driver

[Zev Weiss]

Hello,

This patch series is another incarnation of some previous efforts [0]
at enabling userspace access to the OPERATION state (and now status
flags) of PMBus devices, specifically with respect to efuses
protecting general-purpose power outputs.  This functionality is an
important component enabling a port of OpenBMC to the Delta AHE-50DC
Open19 power shelf [1].

The first patch extends the pmbus core's regulator support with an
implementation of the .get_error_flags() operation, mapping PMBus
status flags to REGULATOR_ERROR_* flags where possible, and the second
patch adds regulator support for the lm25066 driver.  These two
patches are essentially independent of the power-efuse driver (and
each other) and could potentially be applicable individually, but are
necessary for the power-efuse driver to be useful on the AHE-50DC.

The third and fourth patches add dt-bindings and the implementation of
the power-efuse driver, respectively.  The driver is fairly simple; it
merely provides a sysfs interface to enable, disable, and retrieve
error flags from an underlying regulator.

There is one aspect of its usage of the regulator API I'm a bit
uncertain about, however: this driver seems like a case where an
exclusive 'get' of the regulator (i.e. devm_regulator_get_exclusive()
instead of devm_regulator_get() in efuse_probe()) would be
appropriate, since in the intended usage no other device should be
using an efuse's regulator.  With an exclusive get though, the
regulator's use_count and the consumer's enable_count don't balance
out properly to allow the enable/disable operations to work properly
(the former ending up one more than the latter, leading to
enable_count underflows on attempts to disable the regulator).  So at
least for now it's using a non-exclusive get -- I'd be happy to hear
any pointers on a way to allow an exclusive get to work here, though.


Thanks,
Zev

[0] https://lore.kernel.org/openbmc/YGLepYLvtlO6Ikzs@hatter.bewilderbeest.net/
[1] https://www.open19.org/marketplace/delta-16kw-power-shelf/

Zev Weiss (4):
  hwmon: (pmbus) Add get_error_flags support to regulator ops
  hwmon: (pmbus) lm25066: Add regulator support
  dt-bindings: Add power-efuse binding
  misc: Add power-efuse driver

 .../devicetree/bindings/misc/power-efuse.yaml |  37 +++
 MAINTAINERS                                   |   5 +
 drivers/hwmon/pmbus/Kconfig                   |   7 +
 drivers/hwmon/pmbus/lm25066.c                 |  14 ++
 drivers/hwmon/pmbus/pmbus_core.c              |  97 ++++++++
 drivers/misc/Kconfig                          |  15 ++
 drivers/misc/Makefile                         |   1 +
 drivers/misc/power-efuse.c                    | 221 ++++++++++++++++++
 8 files changed, 397 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/power-efuse.yaml
 create mode 100644 drivers/misc/power-efuse.c

-- 
2.35.1

[PATCH 1/4] hwmon: (pmbus) Add get_error_flags support to regulator ops

[Zev Weiss]

The various PMBus status bits don't all map perfectly to the more
limited set of REGULATOR_ERROR_* flags, but there's a reasonable
number where they correspond well enough.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 drivers/hwmon/pmbus/pmbus_core.c | 97 ++++++++++++++++++++++++++++++++
 1 file changed, 97 insertions(+)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 776ee2237be2..a274e8e524a5 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -2417,10 +2417,107 @@ static int pmbus_regulator_disable(struct regulator_dev *rdev)
 	return _pmbus_regulator_on_off(rdev, 0);
 }
 
+/* A PMBus status flag and the corresponding REGULATOR_ERROR_* flag */
+struct pmbus_regulator_status_assoc {
+	int pflag, rflag;
+};
+
+/* PMBus->regulator bit mappings for a PMBus status register */
+struct pmbus_regulator_status_category {
+	int func;
+	int reg;
+	const struct pmbus_regulator_status_assoc *bits; /* zero-terminated */
+};
+
+static const struct pmbus_regulator_status_category pmbus_regulator_flag_map[] = {
+	{
+		.func = PMBUS_HAVE_STATUS_VOUT,
+		.reg = PMBUS_STATUS_VOUT,
+		.bits = (const struct pmbus_regulator_status_assoc[]) {
+			{ PB_VOLTAGE_UV_WARNING, REGULATOR_ERROR_UNDER_VOLTAGE_WARN },
+			{ PB_VOLTAGE_UV_FAULT,   REGULATOR_ERROR_UNDER_VOLTAGE },
+			{ PB_VOLTAGE_OV_WARNING, REGULATOR_ERROR_OVER_VOLTAGE_WARN },
+			{ PB_VOLTAGE_OV_FAULT,   REGULATOR_ERROR_REGULATION_OUT },
+			{ },
+		},
+	}, {
+		.func = PMBUS_HAVE_STATUS_IOUT,
+		.reg = PMBUS_STATUS_IOUT,
+		.bits = (const struct pmbus_regulator_status_assoc[]) {
+			{ PB_IOUT_OC_WARNING,    REGULATOR_ERROR_OVER_CURRENT_WARN },
+			{ PB_IOUT_OC_FAULT,      REGULATOR_ERROR_OVER_CURRENT },
+			{ PB_IOUT_OC_LV_FAULT,   REGULATOR_ERROR_OVER_CURRENT },
+			{ },
+		},
+	}, {
+		.func = PMBUS_HAVE_STATUS_TEMP,
+		.reg = PMBUS_STATUS_TEMPERATURE,
+		.bits = (const struct pmbus_regulator_status_assoc[]) {
+			{ PB_TEMP_OT_WARNING,    REGULATOR_ERROR_OVER_TEMP_WARN },
+			{ PB_TEMP_OT_FAULT,      REGULATOR_ERROR_OVER_TEMP },
+			{ },
+		},
+	},
+};
+
+static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned int *flags)
+{
+	int i, status, statusreg;
+	const struct pmbus_regulator_status_category *cat;
+	const struct pmbus_regulator_status_assoc *bit;
+	struct device *dev = rdev_get_dev(rdev);
+	struct i2c_client *client = to_i2c_client(dev->parent);
+	struct pmbus_data *data = i2c_get_clientdata(client);
+	u8 page = rdev_get_id(rdev);
+	int func = data->info->func[page];
+
+	*flags = 0;
+
+	for (i = 0; i < ARRAY_SIZE(pmbus_regulator_flag_map); i++) {
+		cat = &pmbus_regulator_flag_map[i];
+		if (!(func & cat->func))
+			continue;
+
+		status = pmbus_read_byte_data(client, page, cat->reg);
+		if (status < 0)
+			return status;
+
+		for (bit = cat->bits; bit->pflag; bit++) {
+			if (status & bit->pflag)
+				*flags |= bit->rflag;
+		}
+	}
+
+	/*
+	 * Map what bits of STATUS_{WORD,BYTE} we can to REGULATOR_ERROR_*
+	 * bits.  Some of the other bits are tempting (especially for cases
+	 * where we don't have the relevant PMBUS_HAVE_STATUS_*
+	 * functionality), but there's an unfortunate ambiguity in that
+	 * they're defined as indicating a fault *or* a warning, so we can't
+	 * easily determine whether to report REGULATOR_ERROR_<foo> or
+	 * REGULATOR_ERROR_<foo>_WARN.
+	 */
+	statusreg = data->has_status_word ? PMBUS_STATUS_WORD : PMBUS_STATUS_BYTE;
+	status = pmbus_get_status(client, page, statusreg);
+
+	if (status < 0)
+		return status;
+
+	if (pmbus_regulator_is_enabled(rdev) && (status & PB_STATUS_OFF))
+		*flags |= REGULATOR_ERROR_FAIL;
+	if (status & PB_STATUS_IOUT_OC)
+		*flags |= REGULATOR_ERROR_OVER_CURRENT;
+	if (status & PB_STATUS_VOUT_OV)
+		*flags |= REGULATOR_ERROR_REGULATION_OUT;
+
+	return 0;
+}
+
 const struct regulator_ops pmbus_regulator_ops = {
 	.enable = pmbus_regulator_enable,
 	.disable = pmbus_regulator_disable,
 	.is_enabled = pmbus_regulator_is_enabled,
+	.get_error_flags = pmbus_regulator_get_error_flags,
 };
 EXPORT_SYMBOL_NS_GPL(pmbus_regulator_ops, PMBUS);
 
-- 
2.35.1

[PATCH 2/4] hwmon: (pmbus) lm25066: Add regulator support

[Zev Weiss]

While these chips aren't strictly advertised as voltage regulators per
se, they (aside from the lm25056) support the PMBus OPERATION command
to enable and disable their outputs and have status bits for reporting
various warnings and faults, and can hence usefully support all the
pmbus_regulator_ops operations.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 drivers/hwmon/pmbus/Kconfig   |  7 +++++++
 drivers/hwmon/pmbus/lm25066.c | 14 ++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index 41f6cbf96d3b..4acf63fd69b2 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -174,6 +174,13 @@ config SENSORS_LM25066
 	  This driver can also be built as a module. If so, the module will
 	  be called lm25066.
 
+config SENSORS_LM25066_REGULATOR
+	bool "Regulator support for LM25066 and compatibles"
+	depends on SENSORS_LM25066 && REGULATOR
+	help
+	  If you say yes here you get regulator support for National
+	  Semiconductor LM25066, LM5064, and LM5066.
+
 config SENSORS_LTC2978
 	tristate "Linear Technologies LTC2978 and compatibles"
 	help
diff --git a/drivers/hwmon/pmbus/lm25066.c b/drivers/hwmon/pmbus/lm25066.c
index 8402b41520eb..09792cd03d9f 100644
--- a/drivers/hwmon/pmbus/lm25066.c
+++ b/drivers/hwmon/pmbus/lm25066.c
@@ -435,6 +435,12 @@ static int lm25066_write_word_data(struct i2c_client *client, int page, int reg,
 	return ret;
 }
 
+#if IS_ENABLED(CONFIG_SENSORS_LM25066_REGULATOR)
+static const struct regulator_desc lm25066_reg_desc[] = {
+	PMBUS_REGULATOR("vout", 0),
+};
+#endif
+
 static const struct i2c_device_id lm25066_id[] = {
 	{"lm25056", lm25056},
 	{"lm25066", lm25066},
@@ -545,6 +551,14 @@ static int lm25066_probe(struct i2c_client *client)
 	info->m[PSC_CURRENT_IN] = info->m[PSC_CURRENT_IN] * shunt / 1000;
 	info->m[PSC_POWER] = info->m[PSC_POWER] * shunt / 1000;
 
+#if IS_ENABLED(CONFIG_SENSORS_LM25066_REGULATOR)
+	/* LM25056 doesn't support OPERATION */
+	if (data->id != lm25056) {
+		info->num_regulators = ARRAY_SIZE(lm25066_reg_desc);
+		info->reg_desc = lm25066_reg_desc;
+	}
+#endif
+
 	return pmbus_do_probe(client, info);
 }
 
-- 
2.35.1

[PATCH 3/4] dt-bindings: Add power-efuse binding

[Zev Weiss]

This can be used to describe a power output supplied by a regulator
device that the system controls.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 .../devicetree/bindings/misc/power-efuse.yaml | 37 +++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/misc/power-efuse.yaml

diff --git a/Documentation/devicetree/bindings/misc/power-efuse.yaml b/Documentation/devicetree/bindings/misc/power-efuse.yaml
new file mode 100644
index 000000000000..cadce15d2ce7
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/power-efuse.yaml
@@ -0,0 +1,37 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/misc/power-efuse.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Generic power efuse device
+
+maintainers:
+ - Zev Weiss <zev@bewilderbeest.net>
+
+properties:
+  compatible:
+    const: power-efuse
+
+  vout-supply:
+    description:
+      phandle to the regulator providing power for the efuse
+
+  error-flags-cache-ttl-ms:
+    description:
+      The number of milliseconds the vout-supply regulator's error
+      flags should be cached before re-fetching them.
+
+required:
+  - compatible
+  - vout-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    efuse {
+        compatible = "power-efuse";
+        vout-supply = <&efuse_reg>;
+        error-flags-cache-ttl-ms = <500>;
+    };
-- 
2.35.1

[PATCH 4/4] misc: Add power-efuse driver

[Zev Weiss]

This driver provides a sysfs interface to access the on/off state and
error flags of a regulator supplying a power output controlled by the
system.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 MAINTAINERS                |   5 +
 drivers/misc/Kconfig       |  15 +++
 drivers/misc/Makefile      |   1 +
 drivers/misc/power-efuse.c | 221 +++++++++++++++++++++++++++++++++++++
 4 files changed, 242 insertions(+)
 create mode 100644 drivers/misc/power-efuse.c

diff --git a/MAINTAINERS b/MAINTAINERS
index fca970a46e77..d1153a0389d2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7064,6 +7064,11 @@ S:	Orphan
 W:	http://aeschi.ch.eu.org/efs/
 F:	fs/efs/
 
+POWER EFUSE DRIVER
+M:	Zev Weiss <zev@bewilderbeest.net>
+S:	Maintained
+F:	drivers/misc/power-efuse.c
+
 EHEA (IBM pSeries eHEA 10Gb ethernet adapter) DRIVER
 M:	Douglas Miller <dougmill@linux.ibm.com>
 L:	netdev@vger.kernel.org
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 0f5a49fc7c9e..45fc3e8ad35d 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -470,6 +470,21 @@ config HISI_HIKEY_USB
 	  switching between the dual-role USB-C port and the USB-A host ports
 	  using only one USB controller.
 
+config POWER_EFUSE
+	tristate "Power efuse driver support"
+	depends on OF && REGULATOR
+	help
+	  This driver supports a regulator device functioning as a
+	  power efuse, with status bits and an on/off switch available
+	  via sysfs.
+
+	  A typical use for this would be for an efuse controlling a
+	  generic power output for supplying power to devices external
+	  to the system running this driver (such as in the management
+	  controller of a "smart" PDU or similar), allowing the
+	  operator to manually turn the output on and off, check if
+	  the efuse has tripped due to overload, etc.
+
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index a086197af544..7bd784b89ef8 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -59,3 +59,4 @@ obj-$(CONFIG_UACCE)		+= uacce/
 obj-$(CONFIG_XILINX_SDFEC)	+= xilinx_sdfec.o
 obj-$(CONFIG_HISI_HIKEY_USB)	+= hisi_hikey_usb.o
 obj-$(CONFIG_HI6421V600_IRQ)	+= hi6421v600-irq.o
+obj-$(CONFIG_POWER_EFUSE)	+= power-efuse.o
diff --git a/drivers/misc/power-efuse.c b/drivers/misc/power-efuse.c
new file mode 100644
index 000000000000..e974dde57615
--- /dev/null
+++ b/drivers/misc/power-efuse.c
@@ -0,0 +1,221 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * This module provides a thin wrapper around a regulator device that exposes
+ * status bits and on/off state via sysfs.
+ *
+ * Copyright (C) 2022 Zev Weiss <zev@bewilderbeest.net>
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+
+struct efuse {
+	struct regulator *reg;
+	struct {
+		unsigned int cache;
+		unsigned long ttl;
+		unsigned long fetch_time;
+		struct mutex lock;
+	} error_flags;
+};
+
+/* Ensure that the next error_flags access fetches them from the device */
+static void efuse_invalidate_error_flags(struct efuse *efuse)
+{
+	mutex_lock(&efuse->error_flags.lock);
+	efuse->error_flags.fetch_time = 0;
+	mutex_unlock(&efuse->error_flags.lock);
+}
+
+static ssize_t efuse_show_operstate(struct device *dev, struct device_attribute *attr,
+				    char *buf)
+{
+	struct efuse *efuse = dev_get_drvdata(dev);
+	int status = regulator_is_enabled(efuse->reg);
+
+	if (status < 0)
+		return status;
+
+	return sysfs_emit(buf, "%s\n", status ? "on" : "off");
+}
+
+static ssize_t efuse_set_operstate(struct device *dev, struct device_attribute *attr,
+				   const char *buf, size_t count)
+{
+	int status, wantstate;
+	struct efuse *efuse = dev_get_drvdata(dev);
+	struct regulator *reg = efuse->reg;
+
+	if (sysfs_streq(buf, "on"))
+		wantstate = 1;
+	else if (sysfs_streq(buf, "off"))
+		wantstate = 0;
+	else
+		return -EINVAL;
+
+	status = regulator_is_enabled(reg);
+
+	/*
+	 * We need to ensure our enable/disable calls don't get imbalanced, so
+	 * bail if we can't determine the current state.
+	 */
+	if (status < 0)
+		return status;
+
+	/* Return early if we're already in the desired state */
+	if (!!status == wantstate)
+		return count;
+
+	if (wantstate)
+		status = regulator_enable(reg);
+	else
+		status = regulator_disable(reg);
+
+	/*
+	 * Toggling operstate can reset latched status flags, so invalidate
+	 * the cached value.
+	 */
+	efuse_invalidate_error_flags(efuse);
+
+	if (!status && regulator_is_enabled(reg) != wantstate) {
+		/*
+		 * We could do
+		 *
+		 *   if (!wantstate)
+		 *     regulator_force_disable(reg);
+		 *
+		 * here, but it's likely to leave it such that it can't then
+		 * be re-enabled, so we'll just report the error and leave it
+		 * as it is (and hopefully as long as our enable/disable calls
+		 * remain balanced and nobody registers another consumer for
+		 * the same supply we won't end up in this situation anyway).
+		 */
+		dev_err(dev, "regulator_%sable() didn't take effect\n", wantstate ? "en" : "dis");
+		status = -EIO;
+	}
+
+	return status ? : count;
+}
+
+static int efuse_update_error_flags(struct efuse *efuse)
+{
+	int status = 0;
+	unsigned long cache_expiry;
+
+	mutex_lock(&efuse->error_flags.lock);
+
+	cache_expiry = efuse->error_flags.fetch_time + efuse->error_flags.ttl;
+
+	if (!efuse->error_flags.ttl || !efuse->error_flags.fetch_time ||
+	    time_after(jiffies, cache_expiry)) {
+		status = regulator_get_error_flags(efuse->reg, &efuse->error_flags.cache);
+		if (!status)
+			efuse->error_flags.fetch_time = jiffies;
+	}
+
+	mutex_unlock(&efuse->error_flags.lock);
+
+	return status;
+}
+
+static DEVICE_ATTR(operstate, 0644, efuse_show_operstate, efuse_set_operstate);
+
+#define EFUSE_ERROR_ATTR(name, bit)							    \
+	static ssize_t efuse_show_##name(struct device *dev, struct device_attribute *attr, \
+					 char *buf)                                         \
+	{                                                                                   \
+		struct efuse *efuse = dev_get_drvdata(dev);                                 \
+		int status = efuse_update_error_flags(efuse);                               \
+		if (status)                                                                 \
+			return status;                                                      \
+		return sysfs_emit(buf, "%d\n", !!(efuse->error_flags.cache & bit));         \
+	}                                                                                   \
+	static DEVICE_ATTR(name, 0444, efuse_show_##name, NULL)
+
+EFUSE_ERROR_ATTR(under_voltage, REGULATOR_ERROR_UNDER_VOLTAGE);
+EFUSE_ERROR_ATTR(over_current, REGULATOR_ERROR_OVER_CURRENT);
+EFUSE_ERROR_ATTR(regulation_out, REGULATOR_ERROR_REGULATION_OUT);
+EFUSE_ERROR_ATTR(fail, REGULATOR_ERROR_FAIL);
+EFUSE_ERROR_ATTR(over_temp, REGULATOR_ERROR_OVER_TEMP);
+EFUSE_ERROR_ATTR(under_voltage_warn, REGULATOR_ERROR_UNDER_VOLTAGE_WARN);
+EFUSE_ERROR_ATTR(over_current_warn, REGULATOR_ERROR_OVER_CURRENT_WARN);
+EFUSE_ERROR_ATTR(over_voltage_warn, REGULATOR_ERROR_OVER_VOLTAGE_WARN);
+EFUSE_ERROR_ATTR(over_temp_warn, REGULATOR_ERROR_OVER_TEMP_WARN);
+
+static struct attribute *attributes[] = {
+	&dev_attr_operstate.attr,
+	&dev_attr_under_voltage.attr,
+	&dev_attr_over_current.attr,
+	&dev_attr_regulation_out.attr,
+	&dev_attr_fail.attr,
+	&dev_attr_over_temp.attr,
+	&dev_attr_under_voltage_warn.attr,
+	&dev_attr_over_current_warn.attr,
+	&dev_attr_over_voltage_warn.attr,
+	&dev_attr_over_temp_warn.attr,
+	NULL,
+};
+
+static const struct attribute_group attr_group = {
+	.attrs = attributes,
+};
+
+static int efuse_probe(struct platform_device *pdev)
+{
+	int status;
+	struct regulator *reg;
+	struct efuse *efuse;
+	u32 cache_ttl_ms;
+
+	reg = devm_regulator_get(&pdev->dev, "vout");
+	if (IS_ERR(reg))
+		return PTR_ERR(reg);
+
+	status = regulator_enable(reg);
+	if (status) {
+		dev_err(&pdev->dev, "failed to enable regulator\n");
+		return status;
+	}
+
+	efuse = devm_kzalloc(&pdev->dev, sizeof(*efuse), GFP_KERNEL);
+	if (!efuse)
+		return -ENOMEM;
+
+	efuse->reg = reg;
+	mutex_init(&efuse->error_flags.lock);
+
+	if (!of_property_read_u32(pdev->dev.of_node, "error-flags-cache-ttl-ms", &cache_ttl_ms))
+		efuse->error_flags.ttl = msecs_to_jiffies(cache_ttl_ms);
+
+	platform_set_drvdata(pdev, efuse);
+
+	return sysfs_create_group(&pdev->dev.kobj, &attr_group);
+}
+
+static int efuse_remove(struct platform_device *pdev)
+{
+	sysfs_remove_group(&pdev->dev.kobj, &attr_group);
+	return 0;
+}
+
+static const struct of_device_id efuse_of_match_table[] = {
+	{ .compatible = "power-efuse" },
+	{ },
+};
+
+static struct platform_driver efuse_driver = {
+	.driver = {
+		.name = "power-efuse",
+		.of_match_table = efuse_of_match_table,
+	},
+	.probe = efuse_probe,
+	.remove = efuse_remove,
+};
+module_platform_driver(efuse_driver);
+
+MODULE_AUTHOR("Zev Weiss <zev@bewilderbeest.net>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Power efuse driver");
-- 
2.35.1

Re: [PATCH 4/4] misc: Add power-efuse driver

[Greg Kroah-Hartman]

On Thu, Feb 17, 2022 at 02:44:44AM -0800, Zev Weiss wrote:
> This driver provides a sysfs interface to access the on/off state and
> error flags of a regulator supplying a power output controlled by the
> system.
> 
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> ---
>  MAINTAINERS                |   5 +
>  drivers/misc/Kconfig       |  15 +++
>  drivers/misc/Makefile      |   1 +
>  drivers/misc/power-efuse.c | 221 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 242 insertions(+)
>  create mode 100644 drivers/misc/power-efuse.c

You add sysfs files, yet have no Documentation/ABI/ entry updates
documenting what those sysfs files do?  Please fix.

> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index fca970a46e77..d1153a0389d2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7064,6 +7064,11 @@ S:	Orphan
>  W:	http://aeschi.ch.eu.org/efs/
>  F:	fs/efs/
>  
> +POWER EFUSE DRIVER
> +M:	Zev Weiss <zev@bewilderbeest.net>
> +S:	Maintained
> +F:	drivers/misc/power-efuse.c
> +
>  EHEA (IBM pSeries eHEA 10Gb ethernet adapter) DRIVER
>  M:	Douglas Miller <dougmill@linux.ibm.com>
>  L:	netdev@vger.kernel.org
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 0f5a49fc7c9e..45fc3e8ad35d 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -470,6 +470,21 @@ config HISI_HIKEY_USB
>  	  switching between the dual-role USB-C port and the USB-A host ports
>  	  using only one USB controller.
>  
> +config POWER_EFUSE
> +	tristate "Power efuse driver support"
> +	depends on OF && REGULATOR
> +	help
> +	  This driver supports a regulator device functioning as a
> +	  power efuse, with status bits and an on/off switch available
> +	  via sysfs.
> +
> +	  A typical use for this would be for an efuse controlling a
> +	  generic power output for supplying power to devices external
> +	  to the system running this driver (such as in the management
> +	  controller of a "smart" PDU or similar), allowing the
> +	  operator to manually turn the output on and off, check if
> +	  the efuse has tripped due to overload, etc.
> +
>  source "drivers/misc/c2port/Kconfig"
>  source "drivers/misc/eeprom/Kconfig"
>  source "drivers/misc/cb710/Kconfig"
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index a086197af544..7bd784b89ef8 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -59,3 +59,4 @@ obj-$(CONFIG_UACCE)		+= uacce/
>  obj-$(CONFIG_XILINX_SDFEC)	+= xilinx_sdfec.o
>  obj-$(CONFIG_HISI_HIKEY_USB)	+= hisi_hikey_usb.o
>  obj-$(CONFIG_HI6421V600_IRQ)	+= hi6421v600-irq.o
> +obj-$(CONFIG_POWER_EFUSE)	+= power-efuse.o
> diff --git a/drivers/misc/power-efuse.c b/drivers/misc/power-efuse.c
> new file mode 100644
> index 000000000000..e974dde57615
> --- /dev/null
> +++ b/drivers/misc/power-efuse.c
> @@ -0,0 +1,221 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * This module provides a thin wrapper around a regulator device that exposes
> + * status bits and on/off state via sysfs.
> + *
> + * Copyright (C) 2022 Zev Weiss <zev@bewilderbeest.net>
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +
> +struct efuse {
> +	struct regulator *reg;
> +	struct {
> +		unsigned int cache;
> +		unsigned long ttl;
> +		unsigned long fetch_time;
> +		struct mutex lock;
> +	} error_flags;
> +};
> +
> +/* Ensure that the next error_flags access fetches them from the device */
> +static void efuse_invalidate_error_flags(struct efuse *efuse)
> +{
> +	mutex_lock(&efuse->error_flags.lock);
> +	efuse->error_flags.fetch_time = 0;
> +	mutex_unlock(&efuse->error_flags.lock);
> +}
> +
> +static ssize_t efuse_show_operstate(struct device *dev, struct device_attribute *attr,
> +				    char *buf)
> +{
> +	struct efuse *efuse = dev_get_drvdata(dev);
> +	int status = regulator_is_enabled(efuse->reg);
> +
> +	if (status < 0)
> +		return status;
> +
> +	return sysfs_emit(buf, "%s\n", status ? "on" : "off");
> +}
> +
> +static ssize_t efuse_set_operstate(struct device *dev, struct device_attribute *attr,
> +				   const char *buf, size_t count)
> +{
> +	int status, wantstate;
> +	struct efuse *efuse = dev_get_drvdata(dev);
> +	struct regulator *reg = efuse->reg;
> +
> +	if (sysfs_streq(buf, "on"))
> +		wantstate = 1;
> +	else if (sysfs_streq(buf, "off"))
> +		wantstate = 0;
> +	else
> +		return -EINVAL;
> +
> +	status = regulator_is_enabled(reg);
> +
> +	/*
> +	 * We need to ensure our enable/disable calls don't get imbalanced, so
> +	 * bail if we can't determine the current state.
> +	 */
> +	if (status < 0)
> +		return status;
> +
> +	/* Return early if we're already in the desired state */
> +	if (!!status == wantstate)
> +		return count;
> +
> +	if (wantstate)
> +		status = regulator_enable(reg);
> +	else
> +		status = regulator_disable(reg);
> +
> +	/*
> +	 * Toggling operstate can reset latched status flags, so invalidate
> +	 * the cached value.
> +	 */
> +	efuse_invalidate_error_flags(efuse);
> +
> +	if (!status && regulator_is_enabled(reg) != wantstate) {
> +		/*
> +		 * We could do
> +		 *
> +		 *   if (!wantstate)
> +		 *     regulator_force_disable(reg);
> +		 *
> +		 * here, but it's likely to leave it such that it can't then
> +		 * be re-enabled, so we'll just report the error and leave it
> +		 * as it is (and hopefully as long as our enable/disable calls
> +		 * remain balanced and nobody registers another consumer for
> +		 * the same supply we won't end up in this situation anyway).
> +		 */
> +		dev_err(dev, "regulator_%sable() didn't take effect\n", wantstate ? "en" : "dis");
> +		status = -EIO;
> +	}
> +
> +	return status ? : count;
> +}
> +
> +static int efuse_update_error_flags(struct efuse *efuse)
> +{
> +	int status = 0;
> +	unsigned long cache_expiry;
> +
> +	mutex_lock(&efuse->error_flags.lock);
> +
> +	cache_expiry = efuse->error_flags.fetch_time + efuse->error_flags.ttl;
> +
> +	if (!efuse->error_flags.ttl || !efuse->error_flags.fetch_time ||
> +	    time_after(jiffies, cache_expiry)) {
> +		status = regulator_get_error_flags(efuse->reg, &efuse->error_flags.cache);
> +		if (!status)
> +			efuse->error_flags.fetch_time = jiffies;
> +	}
> +
> +	mutex_unlock(&efuse->error_flags.lock);
> +
> +	return status;
> +}
> +
> +static DEVICE_ATTR(operstate, 0644, efuse_show_operstate, efuse_set_operstate);
> +
> +#define EFUSE_ERROR_ATTR(name, bit)							    \
> +	static ssize_t efuse_show_##name(struct device *dev, struct device_attribute *attr, \
> +					 char *buf)                                         \
> +	{                                                                                   \
> +		struct efuse *efuse = dev_get_drvdata(dev);                                 \
> +		int status = efuse_update_error_flags(efuse);                               \
> +		if (status)                                                                 \
> +			return status;                                                      \
> +		return sysfs_emit(buf, "%d\n", !!(efuse->error_flags.cache & bit));         \
> +	}                                                                                   \
> +	static DEVICE_ATTR(name, 0444, efuse_show_##name, NULL)
> +
> +EFUSE_ERROR_ATTR(under_voltage, REGULATOR_ERROR_UNDER_VOLTAGE);
> +EFUSE_ERROR_ATTR(over_current, REGULATOR_ERROR_OVER_CURRENT);
> +EFUSE_ERROR_ATTR(regulation_out, REGULATOR_ERROR_REGULATION_OUT);
> +EFUSE_ERROR_ATTR(fail, REGULATOR_ERROR_FAIL);
> +EFUSE_ERROR_ATTR(over_temp, REGULATOR_ERROR_OVER_TEMP);
> +EFUSE_ERROR_ATTR(under_voltage_warn, REGULATOR_ERROR_UNDER_VOLTAGE_WARN);
> +EFUSE_ERROR_ATTR(over_current_warn, REGULATOR_ERROR_OVER_CURRENT_WARN);
> +EFUSE_ERROR_ATTR(over_voltage_warn, REGULATOR_ERROR_OVER_VOLTAGE_WARN);
> +EFUSE_ERROR_ATTR(over_temp_warn, REGULATOR_ERROR_OVER_TEMP_WARN);
> +
> +static struct attribute *attributes[] = {
> +	&dev_attr_operstate.attr,
> +	&dev_attr_under_voltage.attr,
> +	&dev_attr_over_current.attr,
> +	&dev_attr_regulation_out.attr,
> +	&dev_attr_fail.attr,
> +	&dev_attr_over_temp.attr,
> +	&dev_attr_under_voltage_warn.attr,
> +	&dev_attr_over_current_warn.attr,
> +	&dev_attr_over_voltage_warn.attr,
> +	&dev_attr_over_temp_warn.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group attr_group = {
> +	.attrs = attributes,
> +};

ATTRIBUTE_GROUPS()?

> +
> +static int efuse_probe(struct platform_device *pdev)
> +{
> +	int status;
> +	struct regulator *reg;
> +	struct efuse *efuse;
> +	u32 cache_ttl_ms;
> +
> +	reg = devm_regulator_get(&pdev->dev, "vout");
> +	if (IS_ERR(reg))
> +		return PTR_ERR(reg);
> +
> +	status = regulator_enable(reg);
> +	if (status) {
> +		dev_err(&pdev->dev, "failed to enable regulator\n");
> +		return status;
> +	}
> +
> +	efuse = devm_kzalloc(&pdev->dev, sizeof(*efuse), GFP_KERNEL);
> +	if (!efuse)
> +		return -ENOMEM;
> +
> +	efuse->reg = reg;
> +	mutex_init(&efuse->error_flags.lock);
> +
> +	if (!of_property_read_u32(pdev->dev.of_node, "error-flags-cache-ttl-ms", &cache_ttl_ms))
> +		efuse->error_flags.ttl = msecs_to_jiffies(cache_ttl_ms);
> +
> +	platform_set_drvdata(pdev, efuse);
> +
> +	return sysfs_create_group(&pdev->dev.kobj, &attr_group);

You just raced with userspace and lost :(

Set the default groups for your platform driver and then the driver core
will automatically create/remove them for you, no need for you to do
anything directly with them at all.

thanks,

greg k-h

Re: [PATCH 3/4] dt-bindings: Add power-efuse binding

[Rob Herring]

On Thu, 17 Feb 2022 02:44:43 -0800, Zev Weiss wrote:
> This can be used to describe a power output supplied by a regulator
> device that the system controls.
> 
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> ---
>  .../devicetree/bindings/misc/power-efuse.yaml | 37 +++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/misc/power-efuse.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/misc/power-efuse.yaml:10:2: [warning] wrong indentation: expected 2 but found 1 (indentation)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1594124

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH 1/4] hwmon: (pmbus) Add get_error_flags support to regulator ops

[Guenter Roeck]

On 2/17/22 02:44, Zev Weiss wrote:
> The various PMBus status bits don't all map perfectly to the more
> limited set of REGULATOR_ERROR_* flags, but there's a reasonable
> number where they correspond well enough.
> 
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> ---
>   drivers/hwmon/pmbus/pmbus_core.c | 97 ++++++++++++++++++++++++++++++++
>   1 file changed, 97 insertions(+)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 776ee2237be2..a274e8e524a5 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -2417,10 +2417,107 @@ static int pmbus_regulator_disable(struct regulator_dev *rdev)
>   	return _pmbus_regulator_on_off(rdev, 0);
>   }
>   
> +/* A PMBus status flag and the corresponding REGULATOR_ERROR_* flag */
> +struct pmbus_regulator_status_assoc {
> +	int pflag, rflag;
> +};
> +
> +/* PMBus->regulator bit mappings for a PMBus status register */
> +struct pmbus_regulator_status_category {
> +	int func;
> +	int reg;
> +	const struct pmbus_regulator_status_assoc *bits; /* zero-terminated */
> +};
> +
> +static const struct pmbus_regulator_status_category pmbus_regulator_flag_map[] = {
> +	{
> +		.func = PMBUS_HAVE_STATUS_VOUT,
> +		.reg = PMBUS_STATUS_VOUT,
> +		.bits = (const struct pmbus_regulator_status_assoc[]) {
> +			{ PB_VOLTAGE_UV_WARNING, REGULATOR_ERROR_UNDER_VOLTAGE_WARN },
> +			{ PB_VOLTAGE_UV_FAULT,   REGULATOR_ERROR_UNDER_VOLTAGE },
> +			{ PB_VOLTAGE_OV_WARNING, REGULATOR_ERROR_OVER_VOLTAGE_WARN },
> +			{ PB_VOLTAGE_OV_FAULT,   REGULATOR_ERROR_REGULATION_OUT },
> +			{ },
> +		},
> +	}, {
> +		.func = PMBUS_HAVE_STATUS_IOUT,
> +		.reg = PMBUS_STATUS_IOUT,
> +		.bits = (const struct pmbus_regulator_status_assoc[]) {
> +			{ PB_IOUT_OC_WARNING,    REGULATOR_ERROR_OVER_CURRENT_WARN },
> +			{ PB_IOUT_OC_FAULT,      REGULATOR_ERROR_OVER_CURRENT },
> +			{ PB_IOUT_OC_LV_FAULT,   REGULATOR_ERROR_OVER_CURRENT },
> +			{ },
> +		},
> +	}, {
> +		.func = PMBUS_HAVE_STATUS_TEMP,
> +		.reg = PMBUS_STATUS_TEMPERATURE,
> +		.bits = (const struct pmbus_regulator_status_assoc[]) {
> +			{ PB_TEMP_OT_WARNING,    REGULATOR_ERROR_OVER_TEMP_WARN },
> +			{ PB_TEMP_OT_FAULT,      REGULATOR_ERROR_OVER_TEMP },
> +			{ },
> +		},
> +	},
> +};
> +
> +static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned int *flags)
> +{
> +	int i, status, statusreg;
> +	const struct pmbus_regulator_status_category *cat;
> +	const struct pmbus_regulator_status_assoc *bit;
> +	struct device *dev = rdev_get_dev(rdev);
> +	struct i2c_client *client = to_i2c_client(dev->parent);
> +	struct pmbus_data *data = i2c_get_clientdata(client);
> +	u8 page = rdev_get_id(rdev);
> +	int func = data->info->func[page];
> +
> +	*flags = 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(pmbus_regulator_flag_map); i++) {
> +		cat = &pmbus_regulator_flag_map[i];
> +		if (!(func & cat->func))
> +			continue;
> +
> +		status = pmbus_read_byte_data(client, page, cat->reg);
> +		if (status < 0)
> +			return status;
> +
> +		for (bit = cat->bits; bit->pflag; bit++) {
> +			if (status & bit->pflag)
> +				*flags |= bit->rflag;
> +		}
> +	}
> +
> +	/*
> +	 * Map what bits of STATUS_{WORD,BYTE} we can to REGULATOR_ERROR_*
> +	 * bits.  Some of the other bits are tempting (especially for cases
> +	 * where we don't have the relevant PMBUS_HAVE_STATUS_*
> +	 * functionality), but there's an unfortunate ambiguity in that
> +	 * they're defined as indicating a fault *or* a warning, so we can't
> +	 * easily determine whether to report REGULATOR_ERROR_<foo> or
> +	 * REGULATOR_ERROR_<foo>_WARN.
> +	 */
> +	statusreg = data->has_status_word ? PMBUS_STATUS_WORD : PMBUS_STATUS_BYTE;
> +	status = pmbus_get_status(client, page, statusreg);
> +

pmbus_get_status() calls data->read_status if PMBUS_STATUS_WORD is provided
as parameter, and data->read_status is set to pmbus_read_status_byte()
if reading the word status is not supported. Given that, why not just call
pmbus_get_status(client, page, PMBUS_STATUS_WORD) ?

> +	if (status < 0)
> +		return status;
> +
> +	if (pmbus_regulator_is_enabled(rdev) && (status & PB_STATUS_OFF))
> +		*flags |= REGULATOR_ERROR_FAIL;
> +	if (status & PB_STATUS_IOUT_OC)
> +		*flags |= REGULATOR_ERROR_OVER_CURRENT;

If the current status register is supported, this effectively means that
an overcurrent warning is always reported as both REGULATOR_ERROR_OVER_CURRENT
and REGULATOR_ERROR_OVER_CURRENT_WARN. Is that intentional ?


> +	if (status & PB_STATUS_VOUT_OV)
> +		*flags |= REGULATOR_ERROR_REGULATION_OUT;

Same for voltage. On the other side, temperature limit violations are not
reported at all unless the temperature status register exists.
That seems to be a bit inconsistent to me.

> +
> +	return 0;
> +}
> +
>   const struct regulator_ops pmbus_regulator_ops = {
>   	.enable = pmbus_regulator_enable,
>   	.disable = pmbus_regulator_disable,
>   	.is_enabled = pmbus_regulator_is_enabled,
> +	.get_error_flags = pmbus_regulator_get_error_flags,
>   };
>   EXPORT_SYMBOL_NS_GPL(pmbus_regulator_ops, PMBUS);
>   

Re: [PATCH 0/4] misc: Add power-efuse driver

[Guenter Roeck]

On 2/17/22 02:44, Zev Weiss wrote:
> Hello,
> 
> This patch series is another incarnation of some previous efforts [0]
> at enabling userspace access to the OPERATION state (and now status
> flags) of PMBus devices, specifically with respect to efuses
> protecting general-purpose power outputs.  This functionality is an
> important component enabling a port of OpenBMC to the Delta AHE-50DC
> Open19 power shelf [1].
> 
> The first patch extends the pmbus core's regulator support with an
> implementation of the .get_error_flags() operation, mapping PMBus
> status flags to REGULATOR_ERROR_* flags where possible, and the second
> patch adds regulator support for the lm25066 driver.  These two
> patches are essentially independent of the power-efuse driver (and
> each other) and could potentially be applicable individually, but are
> necessary for the power-efuse driver to be useful on the AHE-50DC.
> 

Nevertheless, the first two patches are orthogonal to the remaining
two patches and should be separate.

Guenter

> The third and fourth patches add dt-bindings and the implementation of
> the power-efuse driver, respectively.  The driver is fairly simple; it
> merely provides a sysfs interface to enable, disable, and retrieve
> error flags from an underlying regulator.
> 
> There is one aspect of its usage of the regulator API I'm a bit
> uncertain about, however: this driver seems like a case where an
> exclusive 'get' of the regulator (i.e. devm_regulator_get_exclusive()
> instead of devm_regulator_get() in efuse_probe()) would be
> appropriate, since in the intended usage no other device should be
> using an efuse's regulator.  With an exclusive get though, the
> regulator's use_count and the consumer's enable_count don't balance
> out properly to allow the enable/disable operations to work properly
> (the former ending up one more than the latter, leading to
> enable_count underflows on attempts to disable the regulator).  So at
> least for now it's using a non-exclusive get -- I'd be happy to hear
> any pointers on a way to allow an exclusive get to work here, though.
> 
> 
> Thanks,
> Zev
> 
> [0] https://lore.kernel.org/openbmc/YGLepYLvtlO6Ikzs@hatter.bewilderbeest.net/
> [1] https://www.open19.org/marketplace/delta-16kw-power-shelf/
> 
> Zev Weiss (4):
>    hwmon: (pmbus) Add get_error_flags support to regulator ops
>    hwmon: (pmbus) lm25066: Add regulator support
>    dt-bindings: Add power-efuse binding
>    misc: Add power-efuse driver
> 
>   .../devicetree/bindings/misc/power-efuse.yaml |  37 +++
>   MAINTAINERS                                   |   5 +
>   drivers/hwmon/pmbus/Kconfig                   |   7 +
>   drivers/hwmon/pmbus/lm25066.c                 |  14 ++
>   drivers/hwmon/pmbus/pmbus_core.c              |  97 ++++++++
>   drivers/misc/Kconfig                          |  15 ++
>   drivers/misc/Makefile                         |   1 +
>   drivers/misc/power-efuse.c                    | 221 ++++++++++++++++++
>   8 files changed, 397 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/misc/power-efuse.yaml
>   create mode 100644 drivers/misc/power-efuse.c
> 

Re: [PATCH 3/4] dt-bindings: Add power-efuse binding

[Rob Herring]

On Thu, Feb 17, 2022 at 02:44:43AM -0800, Zev Weiss wrote:
> This can be used to describe a power output supplied by a regulator
> device that the system controls.
> 
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> ---
>  .../devicetree/bindings/misc/power-efuse.yaml | 37 +++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/misc/power-efuse.yaml
> 
> diff --git a/Documentation/devicetree/bindings/misc/power-efuse.yaml b/Documentation/devicetree/bindings/misc/power-efuse.yaml
> new file mode 100644
> index 000000000000..cadce15d2ce7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/power-efuse.yaml
> @@ -0,0 +1,37 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/misc/power-efuse.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Generic power efuse device

No idea what this is, but I doubt any such generic device exists. This 
needs sufficient description to be convincing that it is indeed generic.

> +
> +maintainers:
> + - Zev Weiss <zev@bewilderbeest.net>
> +
> +properties:
> +  compatible:
> +    const: power-efuse
> +
> +  vout-supply:
> +    description:
> +      phandle to the regulator providing power for the efuse
> +
> +  error-flags-cache-ttl-ms:
> +    description:
> +      The number of milliseconds the vout-supply regulator's error
> +      flags should be cached before re-fetching them.

What are 'error flags'? Not something I've heard with respect to 
regulators.

> +
> +required:
> +  - compatible
> +  - vout-supply
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    efuse {
> +        compatible = "power-efuse";
> +        vout-supply = <&efuse_reg>;
> +        error-flags-cache-ttl-ms = <500>;
> +    };
> -- 
> 2.35.1
> 
> 

Re: [PATCH 4/4] misc: Add power-efuse driver

[Zev Weiss]

On Thu, Feb 17, 2022 at 05:34:56AM PST, Greg Kroah-Hartman wrote:
>On Thu, Feb 17, 2022 at 02:44:44AM -0800, Zev Weiss wrote:
>> This driver provides a sysfs interface to access the on/off state and
>> error flags of a regulator supplying a power output controlled by the
>> system.
>>
>> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
>> ---
>>  MAINTAINERS                |   5 +
>>  drivers/misc/Kconfig       |  15 +++
>>  drivers/misc/Makefile      |   1 +
>>  drivers/misc/power-efuse.c | 221 +++++++++++++++++++++++++++++++++++++
>>  4 files changed, 242 insertions(+)
>>  create mode 100644 drivers/misc/power-efuse.c
>
>You add sysfs files, yet have no Documentation/ABI/ entry updates
>documenting what those sysfs files do?  Please fix.
>
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index fca970a46e77..d1153a0389d2 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -7064,6 +7064,11 @@ S:	Orphan
>>  W:	http://aeschi.ch.eu.org/efs/
>>  F:	fs/efs/
>>
>> +POWER EFUSE DRIVER
>> +M:	Zev Weiss <zev@bewilderbeest.net>
>> +S:	Maintained
>> +F:	drivers/misc/power-efuse.c
>> +
>>  EHEA (IBM pSeries eHEA 10Gb ethernet adapter) DRIVER
>>  M:	Douglas Miller <dougmill@linux.ibm.com>
>>  L:	netdev@vger.kernel.org
>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>> index 0f5a49fc7c9e..45fc3e8ad35d 100644
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>> @@ -470,6 +470,21 @@ config HISI_HIKEY_USB
>>  	  switching between the dual-role USB-C port and the USB-A host ports
>>  	  using only one USB controller.
>>
>> +config POWER_EFUSE
>> +	tristate "Power efuse driver support"
>> +	depends on OF && REGULATOR
>> +	help
>> +	  This driver supports a regulator device functioning as a
>> +	  power efuse, with status bits and an on/off switch available
>> +	  via sysfs.
>> +
>> +	  A typical use for this would be for an efuse controlling a
>> +	  generic power output for supplying power to devices external
>> +	  to the system running this driver (such as in the management
>> +	  controller of a "smart" PDU or similar), allowing the
>> +	  operator to manually turn the output on and off, check if
>> +	  the efuse has tripped due to overload, etc.
>> +
>>  source "drivers/misc/c2port/Kconfig"
>>  source "drivers/misc/eeprom/Kconfig"
>>  source "drivers/misc/cb710/Kconfig"
>> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
>> index a086197af544..7bd784b89ef8 100644
>> --- a/drivers/misc/Makefile
>> +++ b/drivers/misc/Makefile
>> @@ -59,3 +59,4 @@ obj-$(CONFIG_UACCE)		+= uacce/
>>  obj-$(CONFIG_XILINX_SDFEC)	+= xilinx_sdfec.o
>>  obj-$(CONFIG_HISI_HIKEY_USB)	+= hisi_hikey_usb.o
>>  obj-$(CONFIG_HI6421V600_IRQ)	+= hi6421v600-irq.o
>> +obj-$(CONFIG_POWER_EFUSE)	+= power-efuse.o
>> diff --git a/drivers/misc/power-efuse.c b/drivers/misc/power-efuse.c
>> new file mode 100644
>> index 000000000000..e974dde57615
>> --- /dev/null
>> +++ b/drivers/misc/power-efuse.c
>> @@ -0,0 +1,221 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * This module provides a thin wrapper around a regulator device that exposes
>> + * status bits and on/off state via sysfs.
>> + *
>> + * Copyright (C) 2022 Zev Weiss <zev@bewilderbeest.net>
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regulator/consumer.h>
>> +
>> +struct efuse {
>> +	struct regulator *reg;
>> +	struct {
>> +		unsigned int cache;
>> +		unsigned long ttl;
>> +		unsigned long fetch_time;
>> +		struct mutex lock;
>> +	} error_flags;
>> +};
>> +
>> +/* Ensure that the next error_flags access fetches them from the device */
>> +static void efuse_invalidate_error_flags(struct efuse *efuse)
>> +{
>> +	mutex_lock(&efuse->error_flags.lock);
>> +	efuse->error_flags.fetch_time = 0;
>> +	mutex_unlock(&efuse->error_flags.lock);
>> +}
>> +
>> +static ssize_t efuse_show_operstate(struct device *dev, struct device_attribute *attr,
>> +				    char *buf)
>> +{
>> +	struct efuse *efuse = dev_get_drvdata(dev);
>> +	int status = regulator_is_enabled(efuse->reg);
>> +
>> +	if (status < 0)
>> +		return status;
>> +
>> +	return sysfs_emit(buf, "%s\n", status ? "on" : "off");
>> +}
>> +
>> +static ssize_t efuse_set_operstate(struct device *dev, struct device_attribute *attr,
>> +				   const char *buf, size_t count)
>> +{
>> +	int status, wantstate;
>> +	struct efuse *efuse = dev_get_drvdata(dev);
>> +	struct regulator *reg = efuse->reg;
>> +
>> +	if (sysfs_streq(buf, "on"))
>> +		wantstate = 1;
>> +	else if (sysfs_streq(buf, "off"))
>> +		wantstate = 0;
>> +	else
>> +		return -EINVAL;
>> +
>> +	status = regulator_is_enabled(reg);
>> +
>> +	/*
>> +	 * We need to ensure our enable/disable calls don't get imbalanced, so
>> +	 * bail if we can't determine the current state.
>> +	 */
>> +	if (status < 0)
>> +		return status;
>> +
>> +	/* Return early if we're already in the desired state */
>> +	if (!!status == wantstate)
>> +		return count;
>> +
>> +	if (wantstate)
>> +		status = regulator_enable(reg);
>> +	else
>> +		status = regulator_disable(reg);
>> +
>> +	/*
>> +	 * Toggling operstate can reset latched status flags, so invalidate
>> +	 * the cached value.
>> +	 */
>> +	efuse_invalidate_error_flags(efuse);
>> +
>> +	if (!status && regulator_is_enabled(reg) != wantstate) {
>> +		/*
>> +		 * We could do
>> +		 *
>> +		 *   if (!wantstate)
>> +		 *     regulator_force_disable(reg);
>> +		 *
>> +		 * here, but it's likely to leave it such that it can't then
>> +		 * be re-enabled, so we'll just report the error and leave it
>> +		 * as it is (and hopefully as long as our enable/disable calls
>> +		 * remain balanced and nobody registers another consumer for
>> +		 * the same supply we won't end up in this situation anyway).
>> +		 */
>> +		dev_err(dev, "regulator_%sable() didn't take effect\n", wantstate ? "en" : "dis");
>> +		status = -EIO;
>> +	}
>> +
>> +	return status ? : count;
>> +}
>> +
>> +static int efuse_update_error_flags(struct efuse *efuse)
>> +{
>> +	int status = 0;
>> +	unsigned long cache_expiry;
>> +
>> +	mutex_lock(&efuse->error_flags.lock);
>> +
>> +	cache_expiry = efuse->error_flags.fetch_time + efuse->error_flags.ttl;
>> +
>> +	if (!efuse->error_flags.ttl || !efuse->error_flags.fetch_time ||
>> +	    time_after(jiffies, cache_expiry)) {
>> +		status = regulator_get_error_flags(efuse->reg, &efuse->error_flags.cache);
>> +		if (!status)
>> +			efuse->error_flags.fetch_time = jiffies;
>> +	}
>> +
>> +	mutex_unlock(&efuse->error_flags.lock);
>> +
>> +	return status;
>> +}
>> +
>> +static DEVICE_ATTR(operstate, 0644, efuse_show_operstate, efuse_set_operstate);
>> +
>> +#define EFUSE_ERROR_ATTR(name, bit)							    \
>> +	static ssize_t efuse_show_##name(struct device *dev, struct device_attribute *attr, \
>> +					 char *buf)                                         \
>> +	{                                                                                   \
>> +		struct efuse *efuse = dev_get_drvdata(dev);                                 \
>> +		int status = efuse_update_error_flags(efuse);                               \
>> +		if (status)                                                                 \
>> +			return status;                                                      \
>> +		return sysfs_emit(buf, "%d\n", !!(efuse->error_flags.cache & bit));         \
>> +	}                                                                                   \
>> +	static DEVICE_ATTR(name, 0444, efuse_show_##name, NULL)
>> +
>> +EFUSE_ERROR_ATTR(under_voltage, REGULATOR_ERROR_UNDER_VOLTAGE);
>> +EFUSE_ERROR_ATTR(over_current, REGULATOR_ERROR_OVER_CURRENT);
>> +EFUSE_ERROR_ATTR(regulation_out, REGULATOR_ERROR_REGULATION_OUT);
>> +EFUSE_ERROR_ATTR(fail, REGULATOR_ERROR_FAIL);
>> +EFUSE_ERROR_ATTR(over_temp, REGULATOR_ERROR_OVER_TEMP);
>> +EFUSE_ERROR_ATTR(under_voltage_warn, REGULATOR_ERROR_UNDER_VOLTAGE_WARN);
>> +EFUSE_ERROR_ATTR(over_current_warn, REGULATOR_ERROR_OVER_CURRENT_WARN);
>> +EFUSE_ERROR_ATTR(over_voltage_warn, REGULATOR_ERROR_OVER_VOLTAGE_WARN);
>> +EFUSE_ERROR_ATTR(over_temp_warn, REGULATOR_ERROR_OVER_TEMP_WARN);
>> +
>> +static struct attribute *attributes[] = {
>> +	&dev_attr_operstate.attr,
>> +	&dev_attr_under_voltage.attr,
>> +	&dev_attr_over_current.attr,
>> +	&dev_attr_regulation_out.attr,
>> +	&dev_attr_fail.attr,
>> +	&dev_attr_over_temp.attr,
>> +	&dev_attr_under_voltage_warn.attr,
>> +	&dev_attr_over_current_warn.attr,
>> +	&dev_attr_over_voltage_warn.attr,
>> +	&dev_attr_over_temp_warn.attr,
>> +	NULL,
>> +};
>> +
>> +static const struct attribute_group attr_group = {
>> +	.attrs = attributes,
>> +};
>
>ATTRIBUTE_GROUPS()?
>
>> +
>> +static int efuse_probe(struct platform_device *pdev)
>> +{
>> +	int status;
>> +	struct regulator *reg;
>> +	struct efuse *efuse;
>> +	u32 cache_ttl_ms;
>> +
>> +	reg = devm_regulator_get(&pdev->dev, "vout");
>> +	if (IS_ERR(reg))
>> +		return PTR_ERR(reg);
>> +
>> +	status = regulator_enable(reg);
>> +	if (status) {
>> +		dev_err(&pdev->dev, "failed to enable regulator\n");
>> +		return status;
>> +	}
>> +
>> +	efuse = devm_kzalloc(&pdev->dev, sizeof(*efuse), GFP_KERNEL);
>> +	if (!efuse)
>> +		return -ENOMEM;
>> +
>> +	efuse->reg = reg;
>> +	mutex_init(&efuse->error_flags.lock);
>> +
>> +	if (!of_property_read_u32(pdev->dev.of_node, "error-flags-cache-ttl-ms", &cache_ttl_ms))
>> +		efuse->error_flags.ttl = msecs_to_jiffies(cache_ttl_ms);
>> +
>> +	platform_set_drvdata(pdev, efuse);
>> +
>> +	return sysfs_create_group(&pdev->dev.kobj, &attr_group);
>
>You just raced with userspace and lost :(
>
>Set the default groups for your platform driver and then the driver core
>will automatically create/remove them for you, no need for you to do
>anything directly with them at all.
>

Ack, thanks for the review -- I'll fix all three points in v2.


Zev

Re: [PATCH 1/4] hwmon: (pmbus) Add get_error_flags support to regulator ops

[Zev Weiss]

On Thu, Feb 17, 2022 at 10:11:32AM PST, Guenter Roeck wrote:
>On 2/17/22 02:44, Zev Weiss wrote:
>>The various PMBus status bits don't all map perfectly to the more
>>limited set of REGULATOR_ERROR_* flags, but there's a reasonable
>>number where they correspond well enough.
>>
>>Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
>>---
>>  drivers/hwmon/pmbus/pmbus_core.c | 97 ++++++++++++++++++++++++++++++++
>>  1 file changed, 97 insertions(+)
>>
>>diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
>>index 776ee2237be2..a274e8e524a5 100644
>>--- a/drivers/hwmon/pmbus/pmbus_core.c
>>+++ b/drivers/hwmon/pmbus/pmbus_core.c
>>@@ -2417,10 +2417,107 @@ static int pmbus_regulator_disable(struct regulator_dev *rdev)
>>  	return _pmbus_regulator_on_off(rdev, 0);
>>  }
>>+/* A PMBus status flag and the corresponding REGULATOR_ERROR_* flag */
>>+struct pmbus_regulator_status_assoc {
>>+	int pflag, rflag;
>>+};
>>+
>>+/* PMBus->regulator bit mappings for a PMBus status register */
>>+struct pmbus_regulator_status_category {
>>+	int func;
>>+	int reg;
>>+	const struct pmbus_regulator_status_assoc *bits; /* zero-terminated */
>>+};
>>+
>>+static const struct pmbus_regulator_status_category pmbus_regulator_flag_map[] = {
>>+	{
>>+		.func = PMBUS_HAVE_STATUS_VOUT,
>>+		.reg = PMBUS_STATUS_VOUT,
>>+		.bits = (const struct pmbus_regulator_status_assoc[]) {
>>+			{ PB_VOLTAGE_UV_WARNING, REGULATOR_ERROR_UNDER_VOLTAGE_WARN },
>>+			{ PB_VOLTAGE_UV_FAULT,   REGULATOR_ERROR_UNDER_VOLTAGE },
>>+			{ PB_VOLTAGE_OV_WARNING, REGULATOR_ERROR_OVER_VOLTAGE_WARN },
>>+			{ PB_VOLTAGE_OV_FAULT,   REGULATOR_ERROR_REGULATION_OUT },
>>+			{ },
>>+		},
>>+	}, {
>>+		.func = PMBUS_HAVE_STATUS_IOUT,
>>+		.reg = PMBUS_STATUS_IOUT,
>>+		.bits = (const struct pmbus_regulator_status_assoc[]) {
>>+			{ PB_IOUT_OC_WARNING,    REGULATOR_ERROR_OVER_CURRENT_WARN },
>>+			{ PB_IOUT_OC_FAULT,      REGULATOR_ERROR_OVER_CURRENT },
>>+			{ PB_IOUT_OC_LV_FAULT,   REGULATOR_ERROR_OVER_CURRENT },
>>+			{ },
>>+		},
>>+	}, {
>>+		.func = PMBUS_HAVE_STATUS_TEMP,
>>+		.reg = PMBUS_STATUS_TEMPERATURE,
>>+		.bits = (const struct pmbus_regulator_status_assoc[]) {
>>+			{ PB_TEMP_OT_WARNING,    REGULATOR_ERROR_OVER_TEMP_WARN },
>>+			{ PB_TEMP_OT_FAULT,      REGULATOR_ERROR_OVER_TEMP },
>>+			{ },
>>+		},
>>+	},
>>+};
>>+
>>+static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned int *flags)
>>+{
>>+	int i, status, statusreg;
>>+	const struct pmbus_regulator_status_category *cat;
>>+	const struct pmbus_regulator_status_assoc *bit;
>>+	struct device *dev = rdev_get_dev(rdev);
>>+	struct i2c_client *client = to_i2c_client(dev->parent);
>>+	struct pmbus_data *data = i2c_get_clientdata(client);
>>+	u8 page = rdev_get_id(rdev);
>>+	int func = data->info->func[page];
>>+
>>+	*flags = 0;
>>+
>>+	for (i = 0; i < ARRAY_SIZE(pmbus_regulator_flag_map); i++) {
>>+		cat = &pmbus_regulator_flag_map[i];
>>+		if (!(func & cat->func))
>>+			continue;
>>+
>>+		status = pmbus_read_byte_data(client, page, cat->reg);
>>+		if (status < 0)
>>+			return status;
>>+
>>+		for (bit = cat->bits; bit->pflag; bit++) {
>>+			if (status & bit->pflag)
>>+				*flags |= bit->rflag;
>>+		}
>>+	}
>>+
>>+	/*
>>+	 * Map what bits of STATUS_{WORD,BYTE} we can to REGULATOR_ERROR_*
>>+	 * bits.  Some of the other bits are tempting (especially for cases
>>+	 * where we don't have the relevant PMBUS_HAVE_STATUS_*
>>+	 * functionality), but there's an unfortunate ambiguity in that
>>+	 * they're defined as indicating a fault *or* a warning, so we can't
>>+	 * easily determine whether to report REGULATOR_ERROR_<foo> or
>>+	 * REGULATOR_ERROR_<foo>_WARN.
>>+	 */
>>+	statusreg = data->has_status_word ? PMBUS_STATUS_WORD : PMBUS_STATUS_BYTE;
>>+	status = pmbus_get_status(client, page, statusreg);
>>+
>
>pmbus_get_status() calls data->read_status if PMBUS_STATUS_WORD is provided
>as parameter, and data->read_status is set to pmbus_read_status_byte()
>if reading the word status is not supported. Given that, why not just call
>pmbus_get_status(client, page, PMBUS_STATUS_WORD) ?

Good point, I'll change it to do that instead.  (And send v2 separately 
from the power-efuse driver patches.)

>
>>+	if (status < 0)
>>+		return status;
>>+
>>+	if (pmbus_regulator_is_enabled(rdev) && (status & PB_STATUS_OFF))
>>+		*flags |= REGULATOR_ERROR_FAIL;
>>+	if (status & PB_STATUS_IOUT_OC)
>>+		*flags |= REGULATOR_ERROR_OVER_CURRENT;
>
>If the current status register is supported, this effectively means that
>an overcurrent warning is always reported as both REGULATOR_ERROR_OVER_CURRENT
>and REGULATOR_ERROR_OVER_CURRENT_WARN. Is that intentional ?
>

No, but I don't think (by my reading of the spec) that's what would 
happen?

I'm looking at table 16 ("STATUS_WORD Message Contents") in section 17.2 
("STATUS_WORD") of Part II of revision 1.3.1 of the PMBus spec, which 
says that bit 4 of the low byte (PB_STATUS_IOUT_OC) indicates an output 
overcurrent fault, not a warning (in contrast to most of the other bits, 
which may indicate either).

>
>>+	if (status & PB_STATUS_VOUT_OV)
>>+		*flags |= REGULATOR_ERROR_REGULATION_OUT;
>
>Same for voltage.

Likewise, PB_STATUS_VOUT_OV is specified as indicating a fault, not a 
warning.

>On the other side, temperature limit violations are not
>reported at all unless the temperature status register exists.
>That seems to be a bit inconsistent to me.
>

Right -- that's because PB_STATUS_TEMPERATURE is one of the "fault or 
warning" bits (unlike VOUT_OV and IOUT_OC), and hence it's an ambiguous 
case as described in the comment before the pmbus_get_status() call.

It's certainly not ideal, but it seemed like the best approach I could 
see given the semantics of the available flags -- I'm open to other 
possibilities though if there's something else that would work better.

Thanks for the review,
Zev

Re: [PATCH 1/4] hwmon: (pmbus) Add get_error_flags support to regulator ops

[Guenter Roeck]

On 2/17/22 15:37, Zev Weiss wrote:
> On Thu, Feb 17, 2022 at 10:11:32AM PST, Guenter Roeck wrote:
>> On 2/17/22 02:44, Zev Weiss wrote:
>>> The various PMBus status bits don't all map perfectly to the more
>>> limited set of REGULATOR_ERROR_* flags, but there's a reasonable
>>> number where they correspond well enough.
>>>
>>> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
>>> ---
>>>  drivers/hwmon/pmbus/pmbus_core.c | 97 ++++++++++++++++++++++++++++++++
>>>  1 file changed, 97 insertions(+)
>>>
>>> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
>>> index 776ee2237be2..a274e8e524a5 100644
>>> --- a/drivers/hwmon/pmbus/pmbus_core.c
>>> +++ b/drivers/hwmon/pmbus/pmbus_core.c
>>> @@ -2417,10 +2417,107 @@ static int pmbus_regulator_disable(struct regulator_dev *rdev)
>>>      return _pmbus_regulator_on_off(rdev, 0);
>>>  }
>>> +/* A PMBus status flag and the corresponding REGULATOR_ERROR_* flag */
>>> +struct pmbus_regulator_status_assoc {
>>> +    int pflag, rflag;
>>> +};
>>> +
>>> +/* PMBus->regulator bit mappings for a PMBus status register */
>>> +struct pmbus_regulator_status_category {
>>> +    int func;
>>> +    int reg;
>>> +    const struct pmbus_regulator_status_assoc *bits; /* zero-terminated */
>>> +};
>>> +
>>> +static const struct pmbus_regulator_status_category pmbus_regulator_flag_map[] = {
>>> +    {
>>> +        .func = PMBUS_HAVE_STATUS_VOUT,
>>> +        .reg = PMBUS_STATUS_VOUT,
>>> +        .bits = (const struct pmbus_regulator_status_assoc[]) {
>>> +            { PB_VOLTAGE_UV_WARNING, REGULATOR_ERROR_UNDER_VOLTAGE_WARN },
>>> +            { PB_VOLTAGE_UV_FAULT,   REGULATOR_ERROR_UNDER_VOLTAGE },
>>> +            { PB_VOLTAGE_OV_WARNING, REGULATOR_ERROR_OVER_VOLTAGE_WARN },
>>> +            { PB_VOLTAGE_OV_FAULT,   REGULATOR_ERROR_REGULATION_OUT },
>>> +            { },
>>> +        },
>>> +    }, {
>>> +        .func = PMBUS_HAVE_STATUS_IOUT,
>>> +        .reg = PMBUS_STATUS_IOUT,
>>> +        .bits = (const struct pmbus_regulator_status_assoc[]) {
>>> +            { PB_IOUT_OC_WARNING,    REGULATOR_ERROR_OVER_CURRENT_WARN },
>>> +            { PB_IOUT_OC_FAULT,      REGULATOR_ERROR_OVER_CURRENT },
>>> +            { PB_IOUT_OC_LV_FAULT,   REGULATOR_ERROR_OVER_CURRENT },
>>> +            { },
>>> +        },
>>> +    }, {
>>> +        .func = PMBUS_HAVE_STATUS_TEMP,
>>> +        .reg = PMBUS_STATUS_TEMPERATURE,
>>> +        .bits = (const struct pmbus_regulator_status_assoc[]) {
>>> +            { PB_TEMP_OT_WARNING,    REGULATOR_ERROR_OVER_TEMP_WARN },
>>> +            { PB_TEMP_OT_FAULT,      REGULATOR_ERROR_OVER_TEMP },
>>> +            { },
>>> +        },
>>> +    },
>>> +};
>>> +
>>> +static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned int *flags)
>>> +{
>>> +    int i, status, statusreg;
>>> +    const struct pmbus_regulator_status_category *cat;
>>> +    const struct pmbus_regulator_status_assoc *bit;
>>> +    struct device *dev = rdev_get_dev(rdev);
>>> +    struct i2c_client *client = to_i2c_client(dev->parent);
>>> +    struct pmbus_data *data = i2c_get_clientdata(client);
>>> +    u8 page = rdev_get_id(rdev);
>>> +    int func = data->info->func[page];
>>> +
>>> +    *flags = 0;
>>> +
>>> +    for (i = 0; i < ARRAY_SIZE(pmbus_regulator_flag_map); i++) {
>>> +        cat = &pmbus_regulator_flag_map[i];
>>> +        if (!(func & cat->func))
>>> +            continue;
>>> +
>>> +        status = pmbus_read_byte_data(client, page, cat->reg);
>>> +        if (status < 0)
>>> +            return status;
>>> +
>>> +        for (bit = cat->bits; bit->pflag; bit++) {
>>> +            if (status & bit->pflag)
>>> +                *flags |= bit->rflag;
>>> +        }
>>> +    }
>>> +
>>> +    /*
>>> +     * Map what bits of STATUS_{WORD,BYTE} we can to REGULATOR_ERROR_*
>>> +     * bits.  Some of the other bits are tempting (especially for cases
>>> +     * where we don't have the relevant PMBUS_HAVE_STATUS_*
>>> +     * functionality), but there's an unfortunate ambiguity in that
>>> +     * they're defined as indicating a fault *or* a warning, so we can't
>>> +     * easily determine whether to report REGULATOR_ERROR_<foo> or
>>> +     * REGULATOR_ERROR_<foo>_WARN.
>>> +     */
>>> +    statusreg = data->has_status_word ? PMBUS_STATUS_WORD : PMBUS_STATUS_BYTE;
>>> +    status = pmbus_get_status(client, page, statusreg);
>>> +
>>
>> pmbus_get_status() calls data->read_status if PMBUS_STATUS_WORD is provided
>> as parameter, and data->read_status is set to pmbus_read_status_byte()
>> if reading the word status is not supported. Given that, why not just call
>> pmbus_get_status(client, page, PMBUS_STATUS_WORD) ?
> 
> Good point, I'll change it to do that instead.  (And send v2 separately from the power-efuse driver patches.)
> 
>>
>>> +    if (status < 0)
>>> +        return status;
>>> +
>>> +    if (pmbus_regulator_is_enabled(rdev) && (status & PB_STATUS_OFF))
>>> +        *flags |= REGULATOR_ERROR_FAIL;
>>> +    if (status & PB_STATUS_IOUT_OC)
>>> +        *flags |= REGULATOR_ERROR_OVER_CURRENT;
>>
>> If the current status register is supported, this effectively means that
>> an overcurrent warning is always reported as both REGULATOR_ERROR_OVER_CURRENT
>> and REGULATOR_ERROR_OVER_CURRENT_WARN. Is that intentional ?
>>
> 
> No, but I don't think (by my reading of the spec) that's what would happen?
> 
> I'm looking at table 16 ("STATUS_WORD Message Contents") in section 17.2 ("STATUS_WORD") of Part II of revision 1.3.1 of the PMBus spec, which says that bit 4 of the low byte (PB_STATUS_IOUT_OC) indicates an output overcurrent fault, not a warning (in contrast to most of the other bits, which may indicate either).
> 
>>
>>> +    if (status & PB_STATUS_VOUT_OV)
>>> +        *flags |= REGULATOR_ERROR_REGULATION_OUT;
>>
>> Same for voltage.
> 
> Likewise, PB_STATUS_VOUT_OV is specified as indicating a fault, not a warning.
> 

Ok, that makes sense.

>> On the other side, temperature limit violations are not
>> reported at all unless the temperature status register exists.
>> That seems to be a bit inconsistent to me.
>>
> 
> Right -- that's because PB_STATUS_TEMPERATURE is one of the "fault or warning" bits (unlike VOUT_OV and IOUT_OC), and hence it's an ambiguous case as described in the comment before the pmbus_get_status() call.
> 
> It's certainly not ideal, but it seemed like the best approach I could see given the semantics of the available flags -- I'm open to other possibilities though if there's something else that would work better.
> 

My approach would be to report a warning if no temperature warning/fault
is set from PMBUS_STATUS_TEMPERATURE but PB_STATUS_TEMPERATURE is set
in the status register.

Something like

	if (!(*flags & (REGULATOR_ERROR_OVER_TEMP | REGULATOR_ERROR_OVER_TEMP_WARN))
	    && (status & PB_STATUS_TEMPERATURE))
		*flags |= REGULATOR_ERROR_OVER_TEMP_WARN;

While not perfect, it would be better than reporting nothing.

Guenter

Re: [PATCH 3/4] dt-bindings: Add power-efuse binding

[Zev Weiss]

On Thu, Feb 17, 2022 at 02:35:12PM PST, Rob Herring wrote:
>On Thu, Feb 17, 2022 at 02:44:43AM -0800, Zev Weiss wrote:
>> This can be used to describe a power output supplied by a regulator
>> device that the system controls.
>>
>> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
>> ---
>>  .../devicetree/bindings/misc/power-efuse.yaml | 37 +++++++++++++++++++
>>  1 file changed, 37 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/misc/power-efuse.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/misc/power-efuse.yaml b/Documentation/devicetree/bindings/misc/power-efuse.yaml
>> new file mode 100644
>> index 000000000000..cadce15d2ce7
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/misc/power-efuse.yaml
>> @@ -0,0 +1,37 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/misc/power-efuse.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Generic power efuse device
>
>No idea what this is, but I doubt any such generic device exists. This
>needs sufficient description to be convincing that it is indeed generic.
>

I was struggling a bit to come up with a reasonably concise title; this 
admittedly isn't great.

Would a description like the following clarify it adequately?

   description: |
     This binding describes a physical power output supplied by a 
     regulator providing efuse functionality (manual on/off control, and 
     auto-shutoff if current, voltage, or thermal limits are exceeded).
     
     These may be found on systems such as "smart" network PDUs, and 
     typically supply power to devices entirely separate from the system 
     described by the device-tree by way of an external connector such as 
     an Open19 power cable:

     https://www.open19.org/marketplace/coolpower-cable-assembly-8ru/


>> +
>> +maintainers:
>> + - Zev Weiss <zev@bewilderbeest.net>
>> +
>> +properties:
>> +  compatible:
>> +    const: power-efuse
>> +
>> +  vout-supply:
>> +    description:
>> +      phandle to the regulator providing power for the efuse
>> +
>> +  error-flags-cache-ttl-ms:
>> +    description:
>> +      The number of milliseconds the vout-supply regulator's error
>> +      flags should be cached before re-fetching them.
>
>What are 'error flags'? Not something I've heard with respect to
>regulators.
>

That refers to the REGULATOR_ERROR_* flags in 
include/linux/regulator/consumer.h, in whatever "physical" form those 
ultimately take -- for example, in the PMBus-based case I'm currently 
aiming to support, they'd map to the flags returned by PMBus STATUS_* 
commands.  


Thanks for the review,
Zev

Re: [PATCH 1/4] hwmon: (pmbus) Add get_error_flags support to regulator ops

[Zev Weiss]

On Thu, Feb 17, 2022 at 04:02:58PM PST, Guenter Roeck wrote:
>On 2/17/22 15:37, Zev Weiss wrote:
>>On Thu, Feb 17, 2022 at 10:11:32AM PST, Guenter Roeck wrote:
>>>On 2/17/22 02:44, Zev Weiss wrote:
>>>>The various PMBus status bits don't all map perfectly to the more
>>>>limited set of REGULATOR_ERROR_* flags, but there's a reasonable
>>>>number where they correspond well enough.
>>>>
>>>>Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
>>>>---
>>>> drivers/hwmon/pmbus/pmbus_core.c | 97 ++++++++++++++++++++++++++++++++
>>>> 1 file changed, 97 insertions(+)
>>>>
>>>>diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
>>>>index 776ee2237be2..a274e8e524a5 100644
>>>>--- a/drivers/hwmon/pmbus/pmbus_core.c
>>>>+++ b/drivers/hwmon/pmbus/pmbus_core.c
>>>>@@ -2417,10 +2417,107 @@ static int pmbus_regulator_disable(struct regulator_dev *rdev)
>>>>     return _pmbus_regulator_on_off(rdev, 0);
>>>> }
>>>>+/* A PMBus status flag and the corresponding REGULATOR_ERROR_* flag */
>>>>+struct pmbus_regulator_status_assoc {
>>>>+    int pflag, rflag;
>>>>+};
>>>>+
>>>>+/* PMBus->regulator bit mappings for a PMBus status register */
>>>>+struct pmbus_regulator_status_category {
>>>>+    int func;
>>>>+    int reg;
>>>>+    const struct pmbus_regulator_status_assoc *bits; /* zero-terminated */
>>>>+};
>>>>+
>>>>+static const struct pmbus_regulator_status_category pmbus_regulator_flag_map[] = {
>>>>+    {
>>>>+        .func = PMBUS_HAVE_STATUS_VOUT,
>>>>+        .reg = PMBUS_STATUS_VOUT,
>>>>+        .bits = (const struct pmbus_regulator_status_assoc[]) {
>>>>+            { PB_VOLTAGE_UV_WARNING, REGULATOR_ERROR_UNDER_VOLTAGE_WARN },
>>>>+            { PB_VOLTAGE_UV_FAULT,   REGULATOR_ERROR_UNDER_VOLTAGE },
>>>>+            { PB_VOLTAGE_OV_WARNING, REGULATOR_ERROR_OVER_VOLTAGE_WARN },
>>>>+            { PB_VOLTAGE_OV_FAULT,   REGULATOR_ERROR_REGULATION_OUT },
>>>>+            { },
>>>>+        },
>>>>+    }, {
>>>>+        .func = PMBUS_HAVE_STATUS_IOUT,
>>>>+        .reg = PMBUS_STATUS_IOUT,
>>>>+        .bits = (const struct pmbus_regulator_status_assoc[]) {
>>>>+            { PB_IOUT_OC_WARNING,    REGULATOR_ERROR_OVER_CURRENT_WARN },
>>>>+            { PB_IOUT_OC_FAULT,      REGULATOR_ERROR_OVER_CURRENT },
>>>>+            { PB_IOUT_OC_LV_FAULT,   REGULATOR_ERROR_OVER_CURRENT },
>>>>+            { },
>>>>+        },
>>>>+    }, {
>>>>+        .func = PMBUS_HAVE_STATUS_TEMP,
>>>>+        .reg = PMBUS_STATUS_TEMPERATURE,
>>>>+        .bits = (const struct pmbus_regulator_status_assoc[]) {
>>>>+            { PB_TEMP_OT_WARNING,    REGULATOR_ERROR_OVER_TEMP_WARN },
>>>>+            { PB_TEMP_OT_FAULT,      REGULATOR_ERROR_OVER_TEMP },
>>>>+            { },
>>>>+        },
>>>>+    },
>>>>+};
>>>>+
>>>>+static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned int *flags)
>>>>+{
>>>>+    int i, status, statusreg;
>>>>+    const struct pmbus_regulator_status_category *cat;
>>>>+    const struct pmbus_regulator_status_assoc *bit;
>>>>+    struct device *dev = rdev_get_dev(rdev);
>>>>+    struct i2c_client *client = to_i2c_client(dev->parent);
>>>>+    struct pmbus_data *data = i2c_get_clientdata(client);
>>>>+    u8 page = rdev_get_id(rdev);
>>>>+    int func = data->info->func[page];
>>>>+
>>>>+    *flags = 0;
>>>>+
>>>>+    for (i = 0; i < ARRAY_SIZE(pmbus_regulator_flag_map); i++) {
>>>>+        cat = &pmbus_regulator_flag_map[i];
>>>>+        if (!(func & cat->func))
>>>>+            continue;
>>>>+
>>>>+        status = pmbus_read_byte_data(client, page, cat->reg);
>>>>+        if (status < 0)
>>>>+            return status;
>>>>+
>>>>+        for (bit = cat->bits; bit->pflag; bit++) {
>>>>+            if (status & bit->pflag)
>>>>+                *flags |= bit->rflag;
>>>>+        }
>>>>+    }
>>>>+
>>>>+    /*
>>>>+     * Map what bits of STATUS_{WORD,BYTE} we can to REGULATOR_ERROR_*
>>>>+     * bits.  Some of the other bits are tempting (especially for cases
>>>>+     * where we don't have the relevant PMBUS_HAVE_STATUS_*
>>>>+     * functionality), but there's an unfortunate ambiguity in that
>>>>+     * they're defined as indicating a fault *or* a warning, so we can't
>>>>+     * easily determine whether to report REGULATOR_ERROR_<foo> or
>>>>+     * REGULATOR_ERROR_<foo>_WARN.
>>>>+     */
>>>>+    statusreg = data->has_status_word ? PMBUS_STATUS_WORD : PMBUS_STATUS_BYTE;
>>>>+    status = pmbus_get_status(client, page, statusreg);
>>>>+
>>>
>>>pmbus_get_status() calls data->read_status if PMBUS_STATUS_WORD is provided
>>>as parameter, and data->read_status is set to pmbus_read_status_byte()
>>>if reading the word status is not supported. Given that, why not just call
>>>pmbus_get_status(client, page, PMBUS_STATUS_WORD) ?
>>
>>Good point, I'll change it to do that instead.  (And send v2 separately from the power-efuse driver patches.)
>>
>>>
>>>>+    if (status < 0)
>>>>+        return status;
>>>>+
>>>>+    if (pmbus_regulator_is_enabled(rdev) && (status & PB_STATUS_OFF))
>>>>+        *flags |= REGULATOR_ERROR_FAIL;
>>>>+    if (status & PB_STATUS_IOUT_OC)
>>>>+        *flags |= REGULATOR_ERROR_OVER_CURRENT;
>>>
>>>If the current status register is supported, this effectively means that
>>>an overcurrent warning is always reported as both REGULATOR_ERROR_OVER_CURRENT
>>>and REGULATOR_ERROR_OVER_CURRENT_WARN. Is that intentional ?
>>>
>>
>>No, but I don't think (by my reading of the spec) that's what would happen?
>>
>>I'm looking at table 16 ("STATUS_WORD Message Contents") in section 17.2 ("STATUS_WORD") of Part II of revision 1.3.1 of the PMBus spec, which says that bit 4 of the low byte (PB_STATUS_IOUT_OC) indicates an output overcurrent fault, not a warning (in contrast to most of the other bits, which may indicate either).
>>
>>>
>>>>+    if (status & PB_STATUS_VOUT_OV)
>>>>+        *flags |= REGULATOR_ERROR_REGULATION_OUT;
>>>
>>>Same for voltage.
>>
>>Likewise, PB_STATUS_VOUT_OV is specified as indicating a fault, not a warning.
>>
>
>Ok, that makes sense.
>
>>>On the other side, temperature limit violations are not
>>>reported at all unless the temperature status register exists.
>>>That seems to be a bit inconsistent to me.
>>>
>>
>>Right -- that's because PB_STATUS_TEMPERATURE is one of the "fault or warning" bits (unlike VOUT_OV and IOUT_OC), and hence it's an ambiguous case as described in the comment before the pmbus_get_status() call.
>>
>>It's certainly not ideal, but it seemed like the best approach I could see given the semantics of the available flags -- I'm open to other possibilities though if there's something else that would work better.
>>
>
>My approach would be to report a warning if no temperature warning/fault
>is set from PMBUS_STATUS_TEMPERATURE but PB_STATUS_TEMPERATURE is set
>in the status register.
>
>Something like
>
>	if (!(*flags & (REGULATOR_ERROR_OVER_TEMP | REGULATOR_ERROR_OVER_TEMP_WARN))
>	    && (status & PB_STATUS_TEMPERATURE))
>		*flags |= REGULATOR_ERROR_OVER_TEMP_WARN;
>
>While not perfect, it would be better than reporting nothing.
>

That sounds like a good idea -- I'll add it in v2.


Thanks,
Zev