Messages in this thread Patch in this message | | | Date | Tue, 30 Mar 2021 12:19:29 -0500 | From | Zev Weiss <> | Subject | Re: Enabling pmbus power control |
| |
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;
| |