Re: Enabling pmbus power control

From: Zev Weiss
Date: Tue Mar 30 2021 - 13:20:16 EST

Next message: Mark Brown: "Re: [PATCH] spi: ensure timely release of driver-allocated resources"
Previous message: Casey Schaufler: "Re: [PATCH v5 1/1] fs: Allow no_new_privs tasks to call chroot(2)"
In reply to: Mark Brown: "Re: Enabling pmbus power control"
Next in thread: Mark Brown: "Re: Enabling pmbus power control"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, Mar 30, 2021 at 06:22:54AM CDT, Mark Brown wrote:

On Tue, Mar 30, 2021 at 03:34:16AM -0700, Guenter Roeck wrote:

(and I don't know if the userspace consumer code is appropriate - you
might want to check with the regulator maintainer on that).

It's not, you should never see this in a production system.

Sorry, can you clarify what exactly "this" refers to here?

> first attempt at this ran into problems with all the
> reg-userspace-consumer instances getting attached to the first
> regulator device, I think due to all of the regulators ending up under
> the same name in the global namespace of regulator_map_list. I worked
> around that by adding an ID counter to produce a unique name for each,
> though that changes device names in userspace-visible ways that I'm
> not sure would be considered OK for backwards compatibility. (I'm not
> familiar enough with the regulator code to know if there's a better
> way of fixing that problem.) The #if-ing to keep it behind a Kconfig

Maybe ask that question on the regulator mailing list.

I can't really tell what the issue is here without more context, the
global name list should not be relevant for much in a system that's well
configured so it sounds like it's user error.

My initial attempt I guess followed the existing ltc2978 code a little too closely and I ended up with all my lm25066 regulators registered under the same (static) name, so when I went to attach the reg-userspace-consumer instances to them by way of that name I got this:

# ls -l /sys/kernel/debug/regulator/7-004?-vout0
/sys/kernel/debug/regulator/7-0040-vout0:
-r--r--r-- 1 root root 0 Jan 1 1970 bypass_count
-r--r--r-- 1 root root 0 Jan 1 1970 open_count
drwxr-xr-x 2 root root 0 Jan 1 1970 reg-userspace-consumer.0.auto-vout0
drwxr-xr-x 2 root root 0 Jan 1 1970 reg-userspace-consumer.1.auto-vout0
drwxr-xr-x 2 root root 0 Jan 1 1970 reg-userspace-consumer.2.auto-vout0
-r--r--r-- 1 root root 0 Jan 1 1970 use_count
/sys/kernel/debug/regulator/7-0041-vout0:
-r--r--r-- 1 root root 0 Jan 1 1970 bypass_count
-r--r--r-- 1 root root 0 Jan 1 1970 open_count
-r--r--r-- 1 root root 0 Jan 1 1970 use_count
/sys/kernel/debug/regulator/7-0043-vout0:
-r--r--r-- 1 root root 0 Jan 1 1970 bypass_count
-r--r--r-- 1 root root 0 Jan 1 1970 open_count
-r--r--r-- 1 root root 0 Jan 1 1970 use_count

(When of course the intent is to have one reg-userspace-consumer for each regulator.)

I now have a revised version that takes Guenter's comments into account and puts this logic in the lm25066 driver instead of the pmbus core though, and in the process arrived at what I expect is a better solution to the name-collision problem at least (below).

Thanks,
Zev

diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index 32d2fc850621..d9905e95d510 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -120,6 +120,21 @@ 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 LM25056, LM25066, LM5064, and LM5066.
+
+config SENSORS_LM25066_USERSPACE_REGULATOR_CONSUMER
+ bool "Userspace regulator consumer support for PMBus devices"
+ depends on SENSORS_LM25066_REGULATOR && REGULATOR_USERSPACE_CONSUMER
+ help
+ If you say yes here, regulator-enabled PMBus devices such as
+ the LM25066 and LTC2978 will have their on/off state
+ controllable from userspace via sysfs.
+
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 e9a66fd9e144..4e7e66d1ca8c 100644
--- a/drivers/hwmon/pmbus/lm25066.c
+++ b/drivers/hwmon/pmbus/lm25066.c
@@ -14,6 +14,9 @@
#include <linux/slab.h>
#include <linux/i2c.h>
#include <linux/log2.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/userspace-consumer.h>
+#include <linux/platform_device.h>
#include "pmbus.h"
enum chips { lm25056, lm25066, lm5064, lm5066, lm5066i };
@@ -207,6 +210,13 @@ struct lm25066_data {
int id;
u16 rlimit; /* Maximum register value */
struct pmbus_driver_info info;
+
+#if IS_ENABLED(CONFIG_SENSORS_LM25066_USERSPACE_REGULATOR_CONSUMER)
+ struct regulator_desc reg_desc;
+ struct regulator_bulk_data reg_supply;
+ struct regulator_userspace_consumer_data consumerdata;
+ struct platform_device platdev;
+#endif
};
#define to_lm25066_data(x) container_of(x, struct lm25066_data, info)
@@ -413,9 +423,15 @@ 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 int lm25066_probe(struct i2c_client *client)
{
- int config;
+ int config, status;
struct lm25066_data *data;
struct pmbus_driver_info *info;
struct __coeff *coeff;
@@ -483,7 +499,46 @@ static int lm25066_probe(struct i2c_client *client)
info->b[PSC_POWER] = coeff[PSC_POWER].b;
}
- return pmbus_do_probe(client, info);
+#if IS_ENABLED(CONFIG_SENSORS_LM25066_REGULATOR)
+ info->num_regulators = 1;
+
+#if IS_ENABLED(CONFIG_SENSORS_LM25066_USERSPACE_REGULATOR_CONSUMER)
+ data->reg_desc = lm25066_reg_desc[0];
+ data->reg_desc.name = devm_kasprintf(&client->dev, GFP_KERNEL, "%s.%s",
+ lm25066_reg_desc[0].name,
+ dev_name(&client->dev));
+ if (!data->reg_desc.name)
+ return -ENOMEM;
+
+ info->reg_desc = &data->reg_desc;
+ info->reg_valid_ops = REGULATOR_CHANGE_STATUS;
+#else
+ info->reg_desc = &lm25066_reg_desc[0];
+#endif
+#endif
+
+ status = pmbus_do_probe(client, info);
+ if (status)
+ return status;
+
+#if IS_ENABLED(CONFIG_SENSORS_LM25066_USERSPACE_REGULATOR_CONSUMER)
+ data->reg_supply.supply = data->reg_desc.name;
+ data->consumerdata = (struct regulator_userspace_consumer_data) {
+ .name = data->reg_desc.name,
+ .num_supplies = 1,
+ .supplies = &data->reg_supply,
+ .init_on = true,
+ };
+ data->platdev = (struct platform_device) {
+ .name = "reg-userspace-consumer",
+ .id = PLATFORM_DEVID_AUTO,
+ .dev = { .platform_data = &data->consumerdata, },
+ };
+
+ status = platform_device_register(&data->platdev);
+#endif
+
+ return status;
}
static const struct i2c_device_id lm25066_id[] = {
diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index 4c30ec89f5bf..153220e2c363 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -451,6 +451,7 @@ struct pmbus_driver_info {
/* Regulator functionality, if supported by this chip driver. */
int num_regulators;
const struct regulator_desc *reg_desc;
+ unsigned int reg_valid_ops;
/* custom attributes */
const struct attribute_group **groups;
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index aadea85fe630..805e3a8d8bd9 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -2314,6 +2314,8 @@ static int pmbus_regulator_register(struct pmbus_data *data)
info->reg_desc[i].name);
return PTR_ERR(rdev);
}
+
+ rdev->constraints->valid_ops_mask |= info->reg_valid_ops;
}
return 0;

Next message: Mark Brown: "Re: [PATCH] spi: ensure timely release of driver-allocated resources"
Previous message: Casey Schaufler: "Re: [PATCH v5 1/1] fs: Allow no_new_privs tasks to call chroot(2)"
In reply to: Mark Brown: "Re: Enabling pmbus power control"
Next in thread: Mark Brown: "Re: Enabling pmbus power control"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]