Re: Enabling pmbus power control

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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;




[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux