Enabling pmbus power control

Enabling pmbus power control

[Zev Weiss]

Hello,

I'm working on a board that has a handful of LM25066 PMICs controlling
the power supply to various devices, and I'd like to have both the
existing hwmon sensor functionality as well as userspace power on/off
control, which does not currently seem to be available (other than via
'i2cset -f', which I'd of course prefer to avoid).  I've drafted up a
couple possible versions of this, and was hoping to get some opinions
on the appropriate overall approach.

One option is to add a read-write sysfs attribute to the existing
hwmon directory (current incarnation of the patch:
https://thorn.bewilderbeest.net/~zev/patches/pmbus-statectl.patch).
This bears a vague resemblance to a patch that was rejected a couple
years ago
(https://lore.kernel.org/linux-hwmon/20190417161817.GA13109@roeck-us.net/),
but is different enough that I wonder if it might potentially be
tolerable?  (It exposes significantly less, for one thing.)

The other approach involves layering a regulator device over the pmbus
device as is done in the LTC2978 driver, and then putting a
reg-userspace-consumer on top of that (current patch:
https://thorn.bewilderbeest.net/~zev/patches/pmbus-ureg.patch).  My
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
option is also kind of ugly as it stands.

The first version seems simpler to me (and avoids the rather more
cumbersome sysfs paths the second one produces, for what that's
worth).  I think the second is (at least structurally) perhaps more
aligned with what Guenter was saying in the previous discussion linked
above, though.  Does anyone have any advice on the best way to proceed
with this?  If the reg-userspace-consumer approach is the preferred
route, suggestions on a better fix for the name collision problem
would be welcome.


Thanks,
Zev

Enabling pmbus power control

[Zev Weiss]

Hello,

I'm working on a board that has a handful of LM25066 PMICs controlling
the power supply to various devices, and I'd like to have both the
existing hwmon sensor functionality as well as userspace power on/off
control, which does not currently seem to be available (other than via
'i2cset -f', which I'd of course prefer to avoid).  I've drafted up a
couple possible versions of this, and was hoping to get some opinions
on the appropriate overall approach.

One option is to add a read-write sysfs attribute to the existing
hwmon directory (current incarnation of the patch:
https://thorn.bewilderbeest.net/~zev/patches/pmbus-statectl.patch).
This bears a vague resemblance to a patch that was rejected a couple
years ago
(https://lore.kernel.org/linux-hwmon/20190417161817.GA13109@roeck-us.net/),
but is different enough that I wonder if it might potentially be
tolerable?  (It exposes significantly less, for one thing.)

The other approach involves layering a regulator device over the pmbus
device as is done in the LTC2978 driver, and then putting a
reg-userspace-consumer on top of that (current patch:
https://thorn.bewilderbeest.net/~zev/patches/pmbus-ureg.patch).  My
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
option is also kind of ugly as it stands.

The first version seems simpler to me (and avoids the rather more
cumbersome sysfs paths the second one produces, for what that's
worth).  I think the second is (at least structurally) perhaps more
aligned with what Guenter was saying in the previous discussion linked
above, though.  Does anyone have any advice on the best way to proceed
with this?  If the reg-userspace-consumer approach is the preferred
route, suggestions on a better fix for the name collision problem
would be welcome.


Thanks,
Zev

Re: Enabling pmbus power control

[Guenter Roeck]

On 3/30/21 1:17 AM, Zev Weiss wrote:
> 
> Hello,
> 
> I'm working on a board that has a handful of LM25066 PMICs controlling
> the power supply to various devices, and I'd like to have both the
> existing hwmon sensor functionality as well as userspace power on/off
> control, which does not currently seem to be available (other than via
> 'i2cset -f', which I'd of course prefer to avoid).  I've drafted up a
> couple possible versions of this, and was hoping to get some opinions
> on the appropriate overall approach.
> 
> One option is to add a read-write sysfs attribute to the existing
> hwmon directory (current incarnation of the patch:
> https://thorn.bewilderbeest.net/~zev/patches/pmbus-statectl.patch).
> This bears a vague resemblance to a patch that was rejected a couple
> years ago
> (https://lore.kernel.org/linux-hwmon/20190417161817.GA13109@roeck-us.net/),
> but is different enough that I wonder if it might potentially be
> tolerable?  (It exposes significantly less, for one thing.)
> 

This is a no-go. We are not going to replicate regulator functionality
in the hwmon subsystem, no matter by what means.

> The other approach involves layering a regulator device over the pmbus
> device as is done in the LTC2978 driver, and then putting a
> reg-userspace-consumer on top of that (current patch:
> https://thorn.bewilderbeest.net/~zev/patches/pmbus-ureg.patch).  My

This is the way to go, but the regulator descriptor (what is currently
CONFIG_PMBUS_USERSPACE_REGULATOR_CONSUMER) should be in the lm25066
driver. I don't want to pollute the pmbus core with that at this point
(and I don't know if the userspace consumer code is appropriate - you
might want to check with the regulator maintainer on that).

> 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.

Guenter

> option is also kind of ugly as it stands.
> 
> The first version seems simpler to me (and avoids the rather more
> cumbersome sysfs paths the second one produces, for what that's
> worth).  I think the second is (at least structurally) perhaps more
> aligned with what Guenter was saying in the previous discussion linked
> above, though.  Does anyone have any advice on the best way to proceed
> with this?  If the reg-userspace-consumer approach is the preferred
> route, suggestions on a better fix for the name collision problem
> would be welcome.
> 
> 
> Thanks,
> Zev
> 

Re: Enabling pmbus power control

[Guenter Roeck]

On 3/30/21 1:17 AM, Zev Weiss wrote:
> 
> Hello,
> 
> I'm working on a board that has a handful of LM25066 PMICs controlling
> the power supply to various devices, and I'd like to have both the
> existing hwmon sensor functionality as well as userspace power on/off
> control, which does not currently seem to be available (other than via
> 'i2cset -f', which I'd of course prefer to avoid).  I've drafted up a
> couple possible versions of this, and was hoping to get some opinions
> on the appropriate overall approach.
> 
> One option is to add a read-write sysfs attribute to the existing
> hwmon directory (current incarnation of the patch:
> https://thorn.bewilderbeest.net/~zev/patches/pmbus-statectl.patch).
> This bears a vague resemblance to a patch that was rejected a couple
> years ago
> (https://lore.kernel.org/linux-hwmon/20190417161817.GA13109@roeck-us.net/),
> but is different enough that I wonder if it might potentially be
> tolerable?  (It exposes significantly less, for one thing.)
> 

This is a no-go. We are not going to replicate regulator functionality
in the hwmon subsystem, no matter by what means.

> The other approach involves layering a regulator device over the pmbus
> device as is done in the LTC2978 driver, and then putting a
> reg-userspace-consumer on top of that (current patch:
> https://thorn.bewilderbeest.net/~zev/patches/pmbus-ureg.patch).  My

This is the way to go, but the regulator descriptor (what is currently
CONFIG_PMBUS_USERSPACE_REGULATOR_CONSUMER) should be in the lm25066
driver. I don't want to pollute the pmbus core with that at this point
(and I don't know if the userspace consumer code is appropriate - you
might want to check with the regulator maintainer on that).

> 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.

Guenter

> option is also kind of ugly as it stands.
> 
> The first version seems simpler to me (and avoids the rather more
> cumbersome sysfs paths the second one produces, for what that's
> worth).  I think the second is (at least structurally) perhaps more
> aligned with what Guenter was saying in the previous discussion linked
> above, though.  Does anyone have any advice on the best way to proceed
> with this?  If the reg-userspace-consumer approach is the preferred
> route, suggestions on a better fix for the name collision problem
> would be welcome.
> 
> 
> Thanks,
> Zev
> 

Re: Enabling pmbus power control

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1171 bytes --]

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.

> > 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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Enabling pmbus power control

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1171 bytes --]

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.

> > 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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Enabling pmbus power control

[Zev Weiss]

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;

Re: Enabling pmbus power control

[Zev Weiss]

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;

Re: Enabling pmbus power control

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1361 bytes --]

On Tue, Mar 30, 2021 at 12:19:29PM -0500, Zev Weiss wrote:
> 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?

The userspace consumer.

> > 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:

I don't know what you're trying to do or why, nor how you're going about
achieving it so I can't really comment.  Like I say anything that's
instantiating a userspace consumer in upstream code is broken, it's
there for test during development of regulator drivers.  Whatever device
is supplied by the regulator should have a driver which should control
the regulator at runtime if that is needed.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Enabling pmbus power control

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1361 bytes --]

On Tue, Mar 30, 2021 at 12:19:29PM -0500, Zev Weiss wrote:
> 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?

The userspace consumer.

> > 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:

I don't know what you're trying to do or why, nor how you're going about
achieving it so I can't really comment.  Like I say anything that's
instantiating a userspace consumer in upstream code is broken, it's
there for test during development of regulator drivers.  Whatever device
is supplied by the regulator should have a driver which should control
the regulator at runtime if that is needed.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Enabling pmbus power control

[Zev Weiss]

On Tue, Mar 30, 2021 at 12:42:21PM CDT, Mark Brown wrote:
>On Tue, Mar 30, 2021 at 12:19:29PM -0500, Zev Weiss wrote:
>> 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?
>
>The userspace consumer.
>
>> > 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:
>
>I don't know what you're trying to do or why, nor how you're going about
>achieving it so I can't really comment.  Like I say anything that's
>instantiating a userspace consumer in upstream code is broken, it's
>there for test during development of regulator drivers.  Whatever device
>is supplied by the regulator should have a driver which should control
>the regulator at runtime if that is needed.

Okay, to expand a bit on the description in my initial message -- we've
got a single chassis with multiple server boards and a single manager 
board that handles, among other things, power control for the servers.
The manager board has one LM25066 for each attached server, which acts 
as the "power switch" for that server.  There thus really isn't any 
driver to speak of for the downstream device.

We need to be able to toggle that power switch from userspace on the 
manager board, which we could achieve via i2cset, but we don't want to 
give up the sensors provided by the hwmon driver.

The hardware seems perfectly capable of providing the functionality we 
need -- is there any way of implementing the requisite kernel support in 
a way that the relevant subsystem maintainers would find palatable, or 
is carrying an out-of-tree patch my only option for this?


Thanks,
Zev

Re: Enabling pmbus power control

[Zev Weiss]

On Tue, Mar 30, 2021 at 12:42:21PM CDT, Mark Brown wrote:
>On Tue, Mar 30, 2021 at 12:19:29PM -0500, Zev Weiss wrote:
>> 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?
>
>The userspace consumer.
>
>> > 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:
>
>I don't know what you're trying to do or why, nor how you're going about
>achieving it so I can't really comment.  Like I say anything that's
>instantiating a userspace consumer in upstream code is broken, it's
>there for test during development of regulator drivers.  Whatever device
>is supplied by the regulator should have a driver which should control
>the regulator at runtime if that is needed.

Okay, to expand a bit on the description in my initial message -- we've
got a single chassis with multiple server boards and a single manager 
board that handles, among other things, power control for the servers.
The manager board has one LM25066 for each attached server, which acts 
as the "power switch" for that server.  There thus really isn't any 
driver to speak of for the downstream device.

We need to be able to toggle that power switch from userspace on the 
manager board, which we could achieve via i2cset, but we don't want to 
give up the sensors provided by the hwmon driver.

The hardware seems perfectly capable of providing the functionality we 
need -- is there any way of implementing the requisite kernel support in 
a way that the relevant subsystem maintainers would find palatable, or 
is carrying an out-of-tree patch my only option for this?


Thanks,
Zev

Re: Enabling pmbus power control

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 879 bytes --]

On Tue, Mar 30, 2021 at 12:56:56PM -0500, Zev Weiss wrote:

> Okay, to expand a bit on the description in my initial message -- we've
> got a single chassis with multiple server boards and a single manager board
> that handles, among other things, power control for the servers.
> The manager board has one LM25066 for each attached server, which acts as
> the "power switch" for that server.  There thus really isn't any driver to
> speak of for the downstream device.

This sounds like you need a driver representing those server boards (or
the slots they plug into perhaps) that represents everything about those
boards to userspace, including power switching.  I don't see why you
wouldn't have a driver for that - it's a thing that physically exists
and clearly has some software control, and you'd presumably also expect
to represent some labelling about the slot as well.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Enabling pmbus power control

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 879 bytes --]

On Tue, Mar 30, 2021 at 12:56:56PM -0500, Zev Weiss wrote:

> Okay, to expand a bit on the description in my initial message -- we've
> got a single chassis with multiple server boards and a single manager board
> that handles, among other things, power control for the servers.
> The manager board has one LM25066 for each attached server, which acts as
> the "power switch" for that server.  There thus really isn't any driver to
> speak of for the downstream device.

This sounds like you need a driver representing those server boards (or
the slots they plug into perhaps) that represents everything about those
boards to userspace, including power switching.  I don't see why you
wouldn't have a driver for that - it's a thing that physically exists
and clearly has some software control, and you'd presumably also expect
to represent some labelling about the slot as well.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Enabling pmbus power control

[Guenter Roeck]

On Tue, Mar 30, 2021 at 07:02:00PM +0100, Mark Brown wrote:
> On Tue, Mar 30, 2021 at 12:56:56PM -0500, Zev Weiss wrote:
> 
> > Okay, to expand a bit on the description in my initial message -- we've
> > got a single chassis with multiple server boards and a single manager board
> > that handles, among other things, power control for the servers.
> > The manager board has one LM25066 for each attached server, which acts as
> > the "power switch" for that server.  There thus really isn't any driver to
> > speak of for the downstream device.
> 
> This sounds like you need a driver representing those server boards (or
> the slots they plug into perhaps) that represents everything about those
> boards to userspace, including power switching.  I don't see why you
> wouldn't have a driver for that - it's a thing that physically exists
> and clearly has some software control, and you'd presumably also expect
> to represent some labelling about the slot as well.

Absolutely agree.

Thanks,
Guenter

Re: Enabling pmbus power control

[Guenter Roeck]

On Tue, Mar 30, 2021 at 07:02:00PM +0100, Mark Brown wrote:
> On Tue, Mar 30, 2021 at 12:56:56PM -0500, Zev Weiss wrote:
> 
> > Okay, to expand a bit on the description in my initial message -- we've
> > got a single chassis with multiple server boards and a single manager board
> > that handles, among other things, power control for the servers.
> > The manager board has one LM25066 for each attached server, which acts as
> > the "power switch" for that server.  There thus really isn't any driver to
> > speak of for the downstream device.
> 
> This sounds like you need a driver representing those server boards (or
> the slots they plug into perhaps) that represents everything about those
> boards to userspace, including power switching.  I don't see why you
> wouldn't have a driver for that - it's a thing that physically exists
> and clearly has some software control, and you'd presumably also expect
> to represent some labelling about the slot as well.

Absolutely agree.

Thanks,
Guenter

Re: Enabling pmbus power control

[Zev Weiss]

On Tue, Mar 30, 2021 at 02:38:10PM CDT, Guenter Roeck wrote:
>On Tue, Mar 30, 2021 at 07:02:00PM +0100, Mark Brown wrote:
>> On Tue, Mar 30, 2021 at 12:56:56PM -0500, Zev Weiss wrote:
>>
>> > Okay, to expand a bit on the description in my initial message -- we've
>> > got a single chassis with multiple server boards and a single manager board
>> > that handles, among other things, power control for the servers.
>> > The manager board has one LM25066 for each attached server, which acts as
>> > the "power switch" for that server.  There thus really isn't any driver to
>> > speak of for the downstream device.
>>
>> This sounds like you need a driver representing those server boards (or
>> the slots they plug into perhaps) that represents everything about those
>> boards to userspace, including power switching.  I don't see why you
>> wouldn't have a driver for that - it's a thing that physically exists
>> and clearly has some software control, and you'd presumably also expect
>> to represent some labelling about the slot as well.
>
>Absolutely agree.
>
>Thanks,
>Guenter

Hi Guenter, Mark,

I wanted to return to this to try to explain why structuring the kernel 
support for this in a way that's specific to the device behind the PMIC 
seems like an awkward fit to me, and ultimately kind of artificial.

In the system I described, the manager board with the LM25066 devices is 
its own little self-contained BMC system running its own Linux kernel 
(though "BMC" is perhaps a slightly misleading term because there's no 
host system that it manages).  The PMICs are really the only relevant 
connection it has to the servers it controls power for -- they have 
their own dedicated local BMCs on board as well doing all the usual BMC 
tasks.  A hypothetical dedicated driver for this on the manager board 
wouldn't have any other hardware to touch aside from the pmbus interface 
of the LM25066 itself, so calling it a server board driver seems pretty 
arbitrary -- and in fact the same system also has another LM25066 that 
controls the power supply to the chassis cooling fans (a totally 
different downstream device, but one that would be equally well-served 
by the same software).

More recently, another system has entered the picture for us that might 
illustrate it more starkly -- the Delta Open19 power shelf [0] supported 
by a recent code release from LinkedIn [1].  This is a rackmount power 
supply with fifty outputs, each independently switchable via an LM25066 
attached to an on-board BMC-style management controller (though again, 
no host system involved).  We (Equinix Metal) are interested in porting 
a modern OpenBMC to it to replace the dated, crufty, 
pre-Linux-Foundation version of OpenBMC it currently runs (as found in 
the linked repo).  The exact nature of the devices powered by its 
outputs is well outside the scope of the firmware running on that 
controller (it could be any arbitrary thing that runs on 12VDC), but we 
still want to be able to both (a) retrieve per-output 
voltage/current/power readings as provided by the existing LM25066 hwmon 
support, and (b) control the on/off state of those outputs from 
userspace.

Given the array of possible use-cases, an approach of adding 
power-switch functionality to the existing LM25066 support seems like 
the most obvious way to support this, so I'm hoping to see if I can get 
some idea of what an acceptable implementation of that might look like.  


Thanks,
Zev

[0] https://www.open19.org/marketplace/delta-16kw-power-shelf/
[1] https://github.com/linkedin/o19-bmc-firmware/tree/master/meta-openbmc/meta-linkedin/meta-deltapower

Re: Enabling pmbus power control

[Zev Weiss]

On Tue, Mar 30, 2021 at 02:38:10PM CDT, Guenter Roeck wrote:
>On Tue, Mar 30, 2021 at 07:02:00PM +0100, Mark Brown wrote:
>> On Tue, Mar 30, 2021 at 12:56:56PM -0500, Zev Weiss wrote:
>>
>> > Okay, to expand a bit on the description in my initial message -- we've
>> > got a single chassis with multiple server boards and a single manager board
>> > that handles, among other things, power control for the servers.
>> > The manager board has one LM25066 for each attached server, which acts as
>> > the "power switch" for that server.  There thus really isn't any driver to
>> > speak of for the downstream device.
>>
>> This sounds like you need a driver representing those server boards (or
>> the slots they plug into perhaps) that represents everything about those
>> boards to userspace, including power switching.  I don't see why you
>> wouldn't have a driver for that - it's a thing that physically exists
>> and clearly has some software control, and you'd presumably also expect
>> to represent some labelling about the slot as well.
>
>Absolutely agree.
>
>Thanks,
>Guenter

Hi Guenter, Mark,

I wanted to return to this to try to explain why structuring the kernel 
support for this in a way that's specific to the device behind the PMIC 
seems like an awkward fit to me, and ultimately kind of artificial.

In the system I described, the manager board with the LM25066 devices is 
its own little self-contained BMC system running its own Linux kernel 
(though "BMC" is perhaps a slightly misleading term because there's no 
host system that it manages).  The PMICs are really the only relevant 
connection it has to the servers it controls power for -- they have 
their own dedicated local BMCs on board as well doing all the usual BMC 
tasks.  A hypothetical dedicated driver for this on the manager board 
wouldn't have any other hardware to touch aside from the pmbus interface 
of the LM25066 itself, so calling it a server board driver seems pretty 
arbitrary -- and in fact the same system also has another LM25066 that 
controls the power supply to the chassis cooling fans (a totally 
different downstream device, but one that would be equally well-served 
by the same software).

More recently, another system has entered the picture for us that might 
illustrate it more starkly -- the Delta Open19 power shelf [0] supported 
by a recent code release from LinkedIn [1].  This is a rackmount power 
supply with fifty outputs, each independently switchable via an LM25066 
attached to an on-board BMC-style management controller (though again, 
no host system involved).  We (Equinix Metal) are interested in porting 
a modern OpenBMC to it to replace the dated, crufty, 
pre-Linux-Foundation version of OpenBMC it currently runs (as found in 
the linked repo).  The exact nature of the devices powered by its 
outputs is well outside the scope of the firmware running on that 
controller (it could be any arbitrary thing that runs on 12VDC), but we 
still want to be able to both (a) retrieve per-output 
voltage/current/power readings as provided by the existing LM25066 hwmon 
support, and (b) control the on/off state of those outputs from 
userspace.

Given the array of possible use-cases, an approach of adding 
power-switch functionality to the existing LM25066 support seems like 
the most obvious way to support this, so I'm hoping to see if I can get 
some idea of what an acceptable implementation of that might look like.  


Thanks,
Zev

[0] https://www.open19.org/marketplace/delta-16kw-power-shelf/
[1] https://github.com/linkedin/o19-bmc-firmware/tree/master/meta-openbmc/meta-linkedin/meta-deltapower

Re: Enabling pmbus power control

[Guenter Roeck]

On Mon, Apr 19, 2021 at 08:29:53PM -0500, Zev Weiss wrote:
> On Tue, Mar 30, 2021 at 02:38:10PM CDT, Guenter Roeck wrote:
> > On Tue, Mar 30, 2021 at 07:02:00PM +0100, Mark Brown wrote:
> > > On Tue, Mar 30, 2021 at 12:56:56PM -0500, Zev Weiss wrote:
> > > 
> > > > Okay, to expand a bit on the description in my initial message -- we've
> > > > got a single chassis with multiple server boards and a single manager board
> > > > that handles, among other things, power control for the servers.
> > > > The manager board has one LM25066 for each attached server, which acts as
> > > > the "power switch" for that server.  There thus really isn't any driver to
> > > > speak of for the downstream device.
> > > 
> > > This sounds like you need a driver representing those server boards (or
> > > the slots they plug into perhaps) that represents everything about those
> > > boards to userspace, including power switching.  I don't see why you
> > > wouldn't have a driver for that - it's a thing that physically exists
> > > and clearly has some software control, and you'd presumably also expect
> > > to represent some labelling about the slot as well.
> > 
> > Absolutely agree.
> > 
> > Thanks,
> > Guenter
> 
> Hi Guenter, Mark,
> 
> I wanted to return to this to try to explain why structuring the kernel
> support for this in a way that's specific to the device behind the PMIC
> seems like an awkward fit to me, and ultimately kind of artificial.
> 
> In the system I described, the manager board with the LM25066 devices is its
> own little self-contained BMC system running its own Linux kernel (though
> "BMC" is perhaps a slightly misleading term because there's no host system
> that it manages).  The PMICs are really the only relevant connection it has
> to the servers it controls power for -- they have their own dedicated local
> BMCs on board as well doing all the usual BMC tasks.  A hypothetical
> dedicated driver for this on the manager board wouldn't have any other
> hardware to touch aside from the pmbus interface of the LM25066 itself, so
> calling it a server board driver seems pretty arbitrary -- and in fact the
> same system also has another LM25066 that controls the power supply to the
> chassis cooling fans (a totally different downstream device, but one that
> would be equally well-served by the same software).
> 
> More recently, another system has entered the picture for us that might
> illustrate it more starkly -- the Delta Open19 power shelf [0] supported by
> a recent code release from LinkedIn [1].  This is a rackmount power supply

All I can see is that this code is a mess.

> with fifty outputs, each independently switchable via an LM25066 attached to
> an on-board BMC-style management controller (though again, no host system
> involved).  We (Equinix Metal) are interested in porting a modern OpenBMC to
> it to replace the dated, crufty, pre-Linux-Foundation version of OpenBMC it
> currently runs (as found in the linked repo).  The exact nature of the
> devices powered by its outputs is well outside the scope of the firmware
> running on that controller (it could be any arbitrary thing that runs on
> 12VDC), but we still want to be able to both (a) retrieve per-output
> voltage/current/power readings as provided by the existing LM25066 hwmon
> support, and (b) control the on/off state of those outputs from userspace.
> 
> Given the array of possible use-cases, an approach of adding power-switch
> functionality to the existing LM25066 support seems like the most obvious
> way to support this, so I'm hoping to see if I can get some idea of what an
> acceptable implementation of that might look like.
> 

Sorry, that is simply a no-go for the LM25066 driver. I mentioned it before,
and it is still true: The hwmon subsystem is not in the business of power
control.

Have you looked into enclosures (drivers/misc/enclosure.c) ? Maybe that
or something similar could be used for your purposes.

Thanks,
Guenter

Re: Enabling pmbus power control

[Guenter Roeck]

On Mon, Apr 19, 2021 at 08:29:53PM -0500, Zev Weiss wrote:
> On Tue, Mar 30, 2021 at 02:38:10PM CDT, Guenter Roeck wrote:
> > On Tue, Mar 30, 2021 at 07:02:00PM +0100, Mark Brown wrote:
> > > On Tue, Mar 30, 2021 at 12:56:56PM -0500, Zev Weiss wrote:
> > > 
> > > > Okay, to expand a bit on the description in my initial message -- we've
> > > > got a single chassis with multiple server boards and a single manager board
> > > > that handles, among other things, power control for the servers.
> > > > The manager board has one LM25066 for each attached server, which acts as
> > > > the "power switch" for that server.  There thus really isn't any driver to
> > > > speak of for the downstream device.
> > > 
> > > This sounds like you need a driver representing those server boards (or
> > > the slots they plug into perhaps) that represents everything about those
> > > boards to userspace, including power switching.  I don't see why you
> > > wouldn't have a driver for that - it's a thing that physically exists
> > > and clearly has some software control, and you'd presumably also expect
> > > to represent some labelling about the slot as well.
> > 
> > Absolutely agree.
> > 
> > Thanks,
> > Guenter
> 
> Hi Guenter, Mark,
> 
> I wanted to return to this to try to explain why structuring the kernel
> support for this in a way that's specific to the device behind the PMIC
> seems like an awkward fit to me, and ultimately kind of artificial.
> 
> In the system I described, the manager board with the LM25066 devices is its
> own little self-contained BMC system running its own Linux kernel (though
> "BMC" is perhaps a slightly misleading term because there's no host system
> that it manages).  The PMICs are really the only relevant connection it has
> to the servers it controls power for -- they have their own dedicated local
> BMCs on board as well doing all the usual BMC tasks.  A hypothetical
> dedicated driver for this on the manager board wouldn't have any other
> hardware to touch aside from the pmbus interface of the LM25066 itself, so
> calling it a server board driver seems pretty arbitrary -- and in fact the
> same system also has another LM25066 that controls the power supply to the
> chassis cooling fans (a totally different downstream device, but one that
> would be equally well-served by the same software).
> 
> More recently, another system has entered the picture for us that might
> illustrate it more starkly -- the Delta Open19 power shelf [0] supported by
> a recent code release from LinkedIn [1].  This is a rackmount power supply

All I can see is that this code is a mess.

> with fifty outputs, each independently switchable via an LM25066 attached to
> an on-board BMC-style management controller (though again, no host system
> involved).  We (Equinix Metal) are interested in porting a modern OpenBMC to
> it to replace the dated, crufty, pre-Linux-Foundation version of OpenBMC it
> currently runs (as found in the linked repo).  The exact nature of the
> devices powered by its outputs is well outside the scope of the firmware
> running on that controller (it could be any arbitrary thing that runs on
> 12VDC), but we still want to be able to both (a) retrieve per-output
> voltage/current/power readings as provided by the existing LM25066 hwmon
> support, and (b) control the on/off state of those outputs from userspace.
> 
> Given the array of possible use-cases, an approach of adding power-switch
> functionality to the existing LM25066 support seems like the most obvious
> way to support this, so I'm hoping to see if I can get some idea of what an
> acceptable implementation of that might look like.
> 

Sorry, that is simply a no-go for the LM25066 driver. I mentioned it before,
and it is still true: The hwmon subsystem is not in the business of power
control.

Have you looked into enclosures (drivers/misc/enclosure.c) ? Maybe that
or something similar could be used for your purposes.

Thanks,
Guenter

Re: Enabling pmbus power control

[Zev Weiss]

On Mon, Apr 19, 2021 at 10:36:48PM CDT, Guenter Roeck wrote:
>On Mon, Apr 19, 2021 at 08:29:53PM -0500, Zev Weiss wrote:
>> On Tue, Mar 30, 2021 at 02:38:10PM CDT, Guenter Roeck wrote:
>> > On Tue, Mar 30, 2021 at 07:02:00PM +0100, Mark Brown wrote:
>> > > On Tue, Mar 30, 2021 at 12:56:56PM -0500, Zev Weiss wrote:
>> > >
>> > > > Okay, to expand a bit on the description in my initial message -- we've
>> > > > got a single chassis with multiple server boards and a single manager board
>> > > > that handles, among other things, power control for the servers.
>> > > > The manager board has one LM25066 for each attached server, which acts as
>> > > > the "power switch" for that server.  There thus really isn't any driver to
>> > > > speak of for the downstream device.
>> > >
>> > > This sounds like you need a driver representing those server boards (or
>> > > the slots they plug into perhaps) that represents everything about those
>> > > boards to userspace, including power switching.  I don't see why you
>> > > wouldn't have a driver for that - it's a thing that physically exists
>> > > and clearly has some software control, and you'd presumably also expect
>> > > to represent some labelling about the slot as well.
>> >
>> > Absolutely agree.
>> >
>> > Thanks,
>> > Guenter
>>
>> Hi Guenter, Mark,
>>
>> I wanted to return to this to try to explain why structuring the kernel
>> support for this in a way that's specific to the device behind the PMIC
>> seems like an awkward fit to me, and ultimately kind of artificial.
>>
>> In the system I described, the manager board with the LM25066 devices is its
>> own little self-contained BMC system running its own Linux kernel (though
>> "BMC" is perhaps a slightly misleading term because there's no host system
>> that it manages).  The PMICs are really the only relevant connection it has
>> to the servers it controls power for -- they have their own dedicated local
>> BMCs on board as well doing all the usual BMC tasks.  A hypothetical
>> dedicated driver for this on the manager board wouldn't have any other
>> hardware to touch aside from the pmbus interface of the LM25066 itself, so
>> calling it a server board driver seems pretty arbitrary -- and in fact the
>> same system also has another LM25066 that controls the power supply to the
>> chassis cooling fans (a totally different downstream device, but one that
>> would be equally well-served by the same software).
>>
>> More recently, another system has entered the picture for us that might
>> illustrate it more starkly -- the Delta Open19 power shelf [0] supported by
>> a recent code release from LinkedIn [1].  This is a rackmount power supply
>
>All I can see is that this code is a mess.
>
>> with fifty outputs, each independently switchable via an LM25066 attached to
>> an on-board BMC-style management controller (though again, no host system
>> involved).  We (Equinix Metal) are interested in porting a modern OpenBMC to
>> it to replace the dated, crufty, pre-Linux-Foundation version of OpenBMC it
>> currently runs (as found in the linked repo).  The exact nature of the
>> devices powered by its outputs is well outside the scope of the firmware
>> running on that controller (it could be any arbitrary thing that runs on
>> 12VDC), but we still want to be able to both (a) retrieve per-output
>> voltage/current/power readings as provided by the existing LM25066 hwmon
>> support, and (b) control the on/off state of those outputs from userspace.
>>
>> Given the array of possible use-cases, an approach of adding power-switch
>> functionality to the existing LM25066 support seems like the most obvious
>> way to support this, so I'm hoping to see if I can get some idea of what an
>> acceptable implementation of that might look like.
>>
>
>Sorry, that is simply a no-go for the LM25066 driver. I mentioned it before,
>and it is still true: The hwmon subsystem is not in the business of power
>control.
>
>Have you looked into enclosures (drivers/misc/enclosure.c) ? Maybe that
>or something similar could be used for your purposes.
>
>Thanks,
>Guenter

I had a glance at the enclosure driver; it looks pretty geared toward 
SES-like things (drivers/scsi/ses.c being its only usage I can see in 
the kernel at the moment) and while it could perhaps be pressed into 
working for this it seems like it would probably drag in a fair amount 
of boilerplate and result in a somewhat gratuitously confusing driver 
arrangement (calling the things involved in the cases we're looking at 
"enclosures" seems like a bit of a stretch).

As an alternative, would something like the patch below be more along 
the lines of what you're suggesting?  And if so, would it make sense to 
generalize it into something like 'pmbus-switch.c' and add a 
PMBUS_HAVE_POWERSWITCH functionality bit or similar in the pmbus code 
instead of hardcoding it for only LM25066 support?



Thanks,
Zev


 From f4c0a3637371d69a414b6fb882497d22e5de3ba0 Mon Sep 17 00:00:00 2001
From: Zev Weiss <zev@bewilderbeest.net>
Date: Wed, 31 Mar 2021 01:58:35 -0500
Subject: [PATCH] misc: add lm25066-switch driver

This driver allows an lm25066 to be switched on and off from userspace
via sysfs.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
  drivers/misc/Kconfig          |   7 ++
  drivers/misc/Makefile         |   1 +
  drivers/misc/lm25066-switch.c | 120 ++++++++++++++++++++++++++++++++++
  3 files changed, 128 insertions(+)
  create mode 100644 drivers/misc/lm25066-switch.c

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index f532c59bb59b..384b6022ec15 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -445,6 +445,13 @@ config HISI_HIKEY_USB
  	  switching between the dual-role USB-C port and the USB-A host ports
  	  using only one USB controller.
  
+config LM25066_SWITCH
+	tristate "LM25066 power switch support"
+	depends on OF && SENSORS_LM25066
+	help
+	  This driver augments LM25066 hwmon support with power switch
+	  functionality controllable from userspace via sysfs.
+
  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 99b6f15a3c70..c948510d0cc9 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -56,3 +56,4 @@ obj-$(CONFIG_HABANA_AI)		+= habanalabs/
  obj-$(CONFIG_UACCE)		+= uacce/
  obj-$(CONFIG_XILINX_SDFEC)	+= xilinx_sdfec.o
  obj-$(CONFIG_HISI_HIKEY_USB)	+= hisi_hikey_usb.o
+obj-$(CONFIG_LM25066_SWITCH)	+= lm25066-switch.o
diff --git a/drivers/misc/lm25066-switch.c b/drivers/misc/lm25066-switch.c
new file mode 100644
index 000000000000..85f3677dc277
--- /dev/null
+++ b/drivers/misc/lm25066-switch.c
@@ -0,0 +1,120 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * This module provides a thin wrapper around the lm25066 hwmon driver that
+ * additionally exposes a userspace-controllable on/off power switch via
+ * sysfs.
+ *
+ * Author: Zev Weiss <zweiss@equinix.com>
+ *
+ * Copyright (C) 2021 Equinix Services, Inc.
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/i2c.h>
+#include <linux/platform_device.h>
+
+#include "../hwmon/pmbus/pmbus.h"
+
+static ssize_t switch_show_state(struct device *dev, struct device_attribute *attr,
+                                 char *buf)
+{
+	struct i2c_client *pmic = dev_get_drvdata(dev);
+	ssize_t ret = pmbus_read_byte_data(pmic, 0, PMBUS_OPERATION);
+	if (ret < 0)
+		return ret;
+
+	return sysfs_emit(buf, "%s\n",
+	                  (ret & PB_OPERATION_CONTROL_ON) ? "on" : "off");
+}
+
+static ssize_t switch_set_state(struct device *dev, struct device_attribute *attr,
+                                const char *buf, size_t count)
+{
+	int state, status;
+	struct i2c_client *pmic = dev_get_drvdata(dev);
+	if (sysfs_streq(buf, "on"))
+		state = PB_OPERATION_CONTROL_ON;
+	else if (sysfs_streq(buf, "off"))
+		state = 0;
+	else
+		return -EINVAL;
+	status = pmbus_update_byte_data(pmic, 0, PMBUS_OPERATION,
+	                                PB_OPERATION_CONTROL_ON, state);
+	return status ? : count;
+}
+
+static DEVICE_ATTR(state, 0644, switch_show_state, switch_set_state);
+
+static struct attribute *attributes[] = {
+	&dev_attr_state.attr,
+	NULL,
+};
+
+static const struct attribute_group attr_group = {
+	.attrs = attributes,
+};
+
+static int lm25066_switch_probe(struct platform_device *pdev)
+{
+	int status;
+	struct device_node *np = pdev->dev.of_node;
+	struct device_node *pmic_np;
+	struct i2c_client *pmic;
+
+	pmic_np = of_parse_phandle(np, "pmic", 0);
+	if (!pmic_np) {
+		dev_err(&pdev->dev, "Cannot parse lm25066-switch pmic\n");
+		return -ENODEV;
+	}
+
+	if (!of_device_is_compatible(pmic_np, "lm25066")) {
+		dev_err(&pdev->dev, "lm25066-switch pmic not lm25066 compatible");
+		status = -ENODEV;
+		goto out;
+	}
+
+	pmic = of_find_i2c_device_by_node(pmic_np);
+	if (!pmic) {
+		status = -EPROBE_DEFER;
+		goto out;
+	}
+
+	platform_set_drvdata(pdev, pmic);
+
+	status = sysfs_create_group(&pdev->dev.kobj, &attr_group);
+
+out:
+	of_node_put(pmic_np);
+	return status;
+}
+
+static int lm25066_switch_remove(struct platform_device *pdev)
+{
+	struct i2c_client *pmic = platform_get_drvdata(pdev);
+
+	sysfs_remove_group(&pdev->dev.kobj, &attr_group);
+	put_device(&pmic->dev);
+
+	return 0;
+}
+
+static const struct of_device_id lm25066_switch_table[] = {
+	{ .compatible = "lm25066-switch" },
+	{ },
+};
+
+static struct platform_driver lm25066_switch_driver = {
+	.driver = {
+		.name = "lm25066-switch",
+		.of_match_table = lm25066_switch_table,
+	},
+	.probe = lm25066_switch_probe,
+	.remove = lm25066_switch_remove,
+};
+
+module_platform_driver(lm25066_switch_driver);
+
+MODULE_AUTHOR("Zev Weiss <zweiss@equinix.com>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("LM25066 power-switch driver");
-- 
2.31.1

Re: Enabling pmbus power control

[Zev Weiss]

On Mon, Apr 19, 2021 at 10:36:48PM CDT, Guenter Roeck wrote:
>On Mon, Apr 19, 2021 at 08:29:53PM -0500, Zev Weiss wrote:
>> On Tue, Mar 30, 2021 at 02:38:10PM CDT, Guenter Roeck wrote:
>> > On Tue, Mar 30, 2021 at 07:02:00PM +0100, Mark Brown wrote:
>> > > On Tue, Mar 30, 2021 at 12:56:56PM -0500, Zev Weiss wrote:
>> > >
>> > > > Okay, to expand a bit on the description in my initial message -- we've
>> > > > got a single chassis with multiple server boards and a single manager board
>> > > > that handles, among other things, power control for the servers.
>> > > > The manager board has one LM25066 for each attached server, which acts as
>> > > > the "power switch" for that server.  There thus really isn't any driver to
>> > > > speak of for the downstream device.
>> > >
>> > > This sounds like you need a driver representing those server boards (or
>> > > the slots they plug into perhaps) that represents everything about those
>> > > boards to userspace, including power switching.  I don't see why you
>> > > wouldn't have a driver for that - it's a thing that physically exists
>> > > and clearly has some software control, and you'd presumably also expect
>> > > to represent some labelling about the slot as well.
>> >
>> > Absolutely agree.
>> >
>> > Thanks,
>> > Guenter
>>
>> Hi Guenter, Mark,
>>
>> I wanted to return to this to try to explain why structuring the kernel
>> support for this in a way that's specific to the device behind the PMIC
>> seems like an awkward fit to me, and ultimately kind of artificial.
>>
>> In the system I described, the manager board with the LM25066 devices is its
>> own little self-contained BMC system running its own Linux kernel (though
>> "BMC" is perhaps a slightly misleading term because there's no host system
>> that it manages).  The PMICs are really the only relevant connection it has
>> to the servers it controls power for -- they have their own dedicated local
>> BMCs on board as well doing all the usual BMC tasks.  A hypothetical
>> dedicated driver for this on the manager board wouldn't have any other
>> hardware to touch aside from the pmbus interface of the LM25066 itself, so
>> calling it a server board driver seems pretty arbitrary -- and in fact the
>> same system also has another LM25066 that controls the power supply to the
>> chassis cooling fans (a totally different downstream device, but one that
>> would be equally well-served by the same software).
>>
>> More recently, another system has entered the picture for us that might
>> illustrate it more starkly -- the Delta Open19 power shelf [0] supported by
>> a recent code release from LinkedIn [1].  This is a rackmount power supply
>
>All I can see is that this code is a mess.
>
>> with fifty outputs, each independently switchable via an LM25066 attached to
>> an on-board BMC-style management controller (though again, no host system
>> involved).  We (Equinix Metal) are interested in porting a modern OpenBMC to
>> it to replace the dated, crufty, pre-Linux-Foundation version of OpenBMC it
>> currently runs (as found in the linked repo).  The exact nature of the
>> devices powered by its outputs is well outside the scope of the firmware
>> running on that controller (it could be any arbitrary thing that runs on
>> 12VDC), but we still want to be able to both (a) retrieve per-output
>> voltage/current/power readings as provided by the existing LM25066 hwmon
>> support, and (b) control the on/off state of those outputs from userspace.
>>
>> Given the array of possible use-cases, an approach of adding power-switch
>> functionality to the existing LM25066 support seems like the most obvious
>> way to support this, so I'm hoping to see if I can get some idea of what an
>> acceptable implementation of that might look like.
>>
>
>Sorry, that is simply a no-go for the LM25066 driver. I mentioned it before,
>and it is still true: The hwmon subsystem is not in the business of power
>control.
>
>Have you looked into enclosures (drivers/misc/enclosure.c) ? Maybe that
>or something similar could be used for your purposes.
>
>Thanks,
>Guenter

I had a glance at the enclosure driver; it looks pretty geared toward 
SES-like things (drivers/scsi/ses.c being its only usage I can see in 
the kernel at the moment) and while it could perhaps be pressed into 
working for this it seems like it would probably drag in a fair amount 
of boilerplate and result in a somewhat gratuitously confusing driver 
arrangement (calling the things involved in the cases we're looking at 
"enclosures" seems like a bit of a stretch).

As an alternative, would something like the patch below be more along 
the lines of what you're suggesting?  And if so, would it make sense to 
generalize it into something like 'pmbus-switch.c' and add a 
PMBUS_HAVE_POWERSWITCH functionality bit or similar in the pmbus code 
instead of hardcoding it for only LM25066 support?



Thanks,
Zev


 From f4c0a3637371d69a414b6fb882497d22e5de3ba0 Mon Sep 17 00:00:00 2001
From: Zev Weiss <zev@bewilderbeest.net>
Date: Wed, 31 Mar 2021 01:58:35 -0500
Subject: [PATCH] misc: add lm25066-switch driver

This driver allows an lm25066 to be switched on and off from userspace
via sysfs.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
  drivers/misc/Kconfig          |   7 ++
  drivers/misc/Makefile         |   1 +
  drivers/misc/lm25066-switch.c | 120 ++++++++++++++++++++++++++++++++++
  3 files changed, 128 insertions(+)
  create mode 100644 drivers/misc/lm25066-switch.c

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index f532c59bb59b..384b6022ec15 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -445,6 +445,13 @@ config HISI_HIKEY_USB
  	  switching between the dual-role USB-C port and the USB-A host ports
  	  using only one USB controller.
  
+config LM25066_SWITCH
+	tristate "LM25066 power switch support"
+	depends on OF && SENSORS_LM25066
+	help
+	  This driver augments LM25066 hwmon support with power switch
+	  functionality controllable from userspace via sysfs.
+
  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 99b6f15a3c70..c948510d0cc9 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -56,3 +56,4 @@ obj-$(CONFIG_HABANA_AI)		+= habanalabs/
  obj-$(CONFIG_UACCE)		+= uacce/
  obj-$(CONFIG_XILINX_SDFEC)	+= xilinx_sdfec.o
  obj-$(CONFIG_HISI_HIKEY_USB)	+= hisi_hikey_usb.o
+obj-$(CONFIG_LM25066_SWITCH)	+= lm25066-switch.o
diff --git a/drivers/misc/lm25066-switch.c b/drivers/misc/lm25066-switch.c
new file mode 100644
index 000000000000..85f3677dc277
--- /dev/null
+++ b/drivers/misc/lm25066-switch.c
@@ -0,0 +1,120 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * This module provides a thin wrapper around the lm25066 hwmon driver that
+ * additionally exposes a userspace-controllable on/off power switch via
+ * sysfs.
+ *
+ * Author: Zev Weiss <zweiss@equinix.com>
+ *
+ * Copyright (C) 2021 Equinix Services, Inc.
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/i2c.h>
+#include <linux/platform_device.h>
+
+#include "../hwmon/pmbus/pmbus.h"
+
+static ssize_t switch_show_state(struct device *dev, struct device_attribute *attr,
+                                 char *buf)
+{
+	struct i2c_client *pmic = dev_get_drvdata(dev);
+	ssize_t ret = pmbus_read_byte_data(pmic, 0, PMBUS_OPERATION);
+	if (ret < 0)
+		return ret;
+
+	return sysfs_emit(buf, "%s\n",
+	                  (ret & PB_OPERATION_CONTROL_ON) ? "on" : "off");
+}
+
+static ssize_t switch_set_state(struct device *dev, struct device_attribute *attr,
+                                const char *buf, size_t count)
+{
+	int state, status;
+	struct i2c_client *pmic = dev_get_drvdata(dev);
+	if (sysfs_streq(buf, "on"))
+		state = PB_OPERATION_CONTROL_ON;
+	else if (sysfs_streq(buf, "off"))
+		state = 0;
+	else
+		return -EINVAL;
+	status = pmbus_update_byte_data(pmic, 0, PMBUS_OPERATION,
+	                                PB_OPERATION_CONTROL_ON, state);
+	return status ? : count;
+}
+
+static DEVICE_ATTR(state, 0644, switch_show_state, switch_set_state);
+
+static struct attribute *attributes[] = {
+	&dev_attr_state.attr,
+	NULL,
+};
+
+static const struct attribute_group attr_group = {
+	.attrs = attributes,
+};
+
+static int lm25066_switch_probe(struct platform_device *pdev)
+{
+	int status;
+	struct device_node *np = pdev->dev.of_node;
+	struct device_node *pmic_np;
+	struct i2c_client *pmic;
+
+	pmic_np = of_parse_phandle(np, "pmic", 0);
+	if (!pmic_np) {
+		dev_err(&pdev->dev, "Cannot parse lm25066-switch pmic\n");
+		return -ENODEV;
+	}
+
+	if (!of_device_is_compatible(pmic_np, "lm25066")) {
+		dev_err(&pdev->dev, "lm25066-switch pmic not lm25066 compatible");
+		status = -ENODEV;
+		goto out;
+	}
+
+	pmic = of_find_i2c_device_by_node(pmic_np);
+	if (!pmic) {
+		status = -EPROBE_DEFER;
+		goto out;
+	}
+
+	platform_set_drvdata(pdev, pmic);
+
+	status = sysfs_create_group(&pdev->dev.kobj, &attr_group);
+
+out:
+	of_node_put(pmic_np);
+	return status;
+}
+
+static int lm25066_switch_remove(struct platform_device *pdev)
+{
+	struct i2c_client *pmic = platform_get_drvdata(pdev);
+
+	sysfs_remove_group(&pdev->dev.kobj, &attr_group);
+	put_device(&pmic->dev);
+
+	return 0;
+}
+
+static const struct of_device_id lm25066_switch_table[] = {
+	{ .compatible = "lm25066-switch" },
+	{ },
+};
+
+static struct platform_driver lm25066_switch_driver = {
+	.driver = {
+		.name = "lm25066-switch",
+		.of_match_table = lm25066_switch_table,
+	},
+	.probe = lm25066_switch_probe,
+	.remove = lm25066_switch_remove,
+};
+
+module_platform_driver(lm25066_switch_driver);
+
+MODULE_AUTHOR("Zev Weiss <zweiss@equinix.com>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("LM25066 power-switch driver");
-- 
2.31.1

Re: Enabling pmbus power control

[Guenter Roeck]

On 4/19/21 10:50 PM, Zev Weiss wrote:
[ ... ]

> I had a glance at the enclosure driver; it looks pretty geared toward SES-like things (drivers/scsi/ses.c being its only usage I can see in the kernel at the moment) and while it could perhaps be pressed into working for this it seems like it would probably drag in a fair amount of boilerplate and result in a somewhat gratuitously confusing driver arrangement (calling the things involved in the cases we're looking at "enclosures" seems like a bit of a stretch).
> 
> As an alternative, would something like the patch below be more along the lines of what you're suggesting?  And if so, would it make sense to generalize it into something like 'pmbus-switch.c' and add a PMBUS_HAVE_POWERSWITCH functionality bit or similar in the pmbus code instead of hardcoding it for only LM25066 support?
> 
> 

No. Don't access pmbus functions from outside drivers/hwmon/pmbus.

I used to be opposed to function export restrictions (aka export namespaces),
but you are making a good case that we need to introduce them for pmbus
functions.

Guenter

Re: Enabling pmbus power control

[Guenter Roeck]

On 4/19/21 10:50 PM, Zev Weiss wrote:
[ ... ]

> I had a glance at the enclosure driver; it looks pretty geared toward SES-like things (drivers/scsi/ses.c being its only usage I can see in the kernel at the moment) and while it could perhaps be pressed into working for this it seems like it would probably drag in a fair amount of boilerplate and result in a somewhat gratuitously confusing driver arrangement (calling the things involved in the cases we're looking at "enclosures" seems like a bit of a stretch).
> 
> As an alternative, would something like the patch below be more along the lines of what you're suggesting?  And if so, would it make sense to generalize it into something like 'pmbus-switch.c' and add a PMBUS_HAVE_POWERSWITCH functionality bit or similar in the pmbus code instead of hardcoding it for only LM25066 support?
> 
> 

No. Don't access pmbus functions from outside drivers/hwmon/pmbus.

I used to be opposed to function export restrictions (aka export namespaces),
but you are making a good case that we need to introduce them for pmbus
functions.

Guenter

Re: Enabling pmbus power control

[Zev Weiss]

On Tue, Apr 20, 2021 at 01:00:25AM CDT, Guenter Roeck wrote:
>On 4/19/21 10:50 PM, Zev Weiss wrote:
>[ ... ]
>
>> I had a glance at the enclosure driver; it looks pretty geared toward SES-like things (drivers/scsi/ses.c being its only usage I can see in the kernel at the moment) and while it could perhaps be pressed into working for this it seems like it would probably drag in a fair amount of boilerplate and result in a somewhat gratuitously confusing driver arrangement (calling the things involved in the cases we're looking at "enclosures" seems like a bit of a stretch).
>>
>> As an alternative, would something like the patch below be more along the lines of what you're suggesting?  And if so, would it make sense to generalize it into something like 'pmbus-switch.c' and add a PMBUS_HAVE_POWERSWITCH functionality bit or similar in the pmbus code instead of hardcoding it for only LM25066 support?
>>
>>
>
>No. Don't access pmbus functions from outside drivers/hwmon/pmbus.
>
>I used to be opposed to function export restrictions (aka export namespaces),
>but you are making a good case that we need to introduce them for pmbus
>functions.
>
>Guenter

Okay -- I figured that was likely to be frowned upon, but the 
alternative seemed to be effectively duplicating non-trivial chunks of 
the pmbus code.  However, upon realizing that the LM25066 doesn't 
actually require any of the paging work the generic pmbus code provides, 
I suppose it can actually be done with a simple smbus read & write.  
Does this version look better?


Zev


 From 1662e1c59c498ad6b208e6ab450bd467d71def34 Mon Sep 17 00:00:00 2001
From: Zev Weiss <zev@bewilderbeest.net>
Date: Wed, 31 Mar 2021 01:58:35 -0500
Subject: [PATCH] misc: add lm25066-switch driver

This driver allows an lm25066 to be switched on and off from userspace
via sysfs.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
  drivers/misc/Kconfig          |   7 ++
  drivers/misc/Makefile         |   1 +
  drivers/misc/lm25066-switch.c | 126 ++++++++++++++++++++++++++++++++++
  3 files changed, 134 insertions(+)
  create mode 100644 drivers/misc/lm25066-switch.c

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index f532c59bb59b..384b6022ec15 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -445,6 +445,13 @@ config HISI_HIKEY_USB
  	  switching between the dual-role USB-C port and the USB-A host ports
  	  using only one USB controller.
  
+config LM25066_SWITCH
+	tristate "LM25066 power switch support"
+	depends on OF && SENSORS_LM25066
+	help
+	  This driver augments LM25066 hwmon support with power switch
+	  functionality controllable from userspace via sysfs.
+
  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 99b6f15a3c70..c948510d0cc9 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -56,3 +56,4 @@ obj-$(CONFIG_HABANA_AI)		+= habanalabs/
  obj-$(CONFIG_UACCE)		+= uacce/
  obj-$(CONFIG_XILINX_SDFEC)	+= xilinx_sdfec.o
  obj-$(CONFIG_HISI_HIKEY_USB)	+= hisi_hikey_usb.o
+obj-$(CONFIG_LM25066_SWITCH)	+= lm25066-switch.o
diff --git a/drivers/misc/lm25066-switch.c b/drivers/misc/lm25066-switch.c
new file mode 100644
index 000000000000..9adc67c320f9
--- /dev/null
+++ b/drivers/misc/lm25066-switch.c
@@ -0,0 +1,126 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * This module provides a thin wrapper around the lm25066 hwmon driver that
+ * additionally exposes a userspace-controllable on/off power switch via
+ * sysfs.
+ *
+ * Author: Zev Weiss <zweiss@equinix.com>
+ *
+ * Copyright (C) 2021 Equinix Services, Inc.
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/i2c.h>
+#include <linux/platform_device.h>
+
+/*
+ * The relevant PMBus command and data values for controlling the LM25066
+ * power state.  Because it's not a paged device we skip the usual paging
+ * business other PMBus devices might require.
+ */
+#define CMD_OPERATION 0x01
+#define OPERATION_ON 0x80
+#define OPERATION_OFF 0x00
+
+static ssize_t switch_show_state(struct device *dev, struct device_attribute *attr,
+                                 char *buf)
+{
+	struct i2c_client *pmic = dev_get_drvdata(dev);
+	ssize_t ret = i2c_smbus_read_byte_data(pmic, CMD_OPERATION);
+	if (ret < 0)
+		return ret;
+
+	return sysfs_emit(buf, "%s\n", (ret & OPERATION_ON) ? "on" : "off");
+}
+
+static ssize_t switch_set_state(struct device *dev, struct device_attribute *attr,
+                                const char *buf, size_t count)
+{
+	int status;
+	u8 value;
+	struct i2c_client *pmic = dev_get_drvdata(dev);
+	if (sysfs_streq(buf, "on"))
+		value = OPERATION_ON;
+	else if (sysfs_streq(buf, "off"))
+		value = OPERATION_OFF;
+	else
+		return -EINVAL;
+	status = i2c_smbus_write_byte_data(pmic, CMD_OPERATION, value);
+	return status ? : count;
+}
+
+static DEVICE_ATTR(state, 0644, switch_show_state, switch_set_state);
+
+static struct attribute *attributes[] = {
+	&dev_attr_state.attr,
+	NULL,
+};
+
+static const struct attribute_group attr_group = {
+	.attrs = attributes,
+};
+
+static int lm25066_switch_probe(struct platform_device *pdev)
+{
+	int status;
+	struct device_node *np = pdev->dev.of_node;
+	struct device_node *pmic_np;
+	struct i2c_client *pmic;
+
+	pmic_np = of_parse_phandle(np, "pmic", 0);
+	if (!pmic_np) {
+		dev_err(&pdev->dev, "Cannot parse lm25066-switch pmic\n");
+		return -ENODEV;
+	}
+
+	if (!of_device_is_compatible(pmic_np, "lm25066")) {
+		dev_err(&pdev->dev, "lm25066-switch pmic not lm25066 compatible");
+		status = -ENODEV;
+		goto out;
+	}
+
+	pmic = of_find_i2c_device_by_node(pmic_np);
+	if (!pmic) {
+		status = -EPROBE_DEFER;
+		goto out;
+	}
+
+	platform_set_drvdata(pdev, pmic);
+
+	status = sysfs_create_group(&pdev->dev.kobj, &attr_group);
+
+out:
+	of_node_put(pmic_np);
+	return status;
+}
+
+static int lm25066_switch_remove(struct platform_device *pdev)
+{
+	struct i2c_client *pmic = platform_get_drvdata(pdev);
+
+	sysfs_remove_group(&pdev->dev.kobj, &attr_group);
+	put_device(&pmic->dev);
+
+	return 0;
+}
+
+static const struct of_device_id lm25066_switch_table[] = {
+	{ .compatible = "lm25066-switch" },
+	{ },
+};
+
+static struct platform_driver lm25066_switch_driver = {
+	.driver = {
+		.name = "lm25066-switch",
+		.of_match_table = lm25066_switch_table,
+	},
+	.probe = lm25066_switch_probe,
+	.remove = lm25066_switch_remove,
+};
+
+module_platform_driver(lm25066_switch_driver);
+
+MODULE_AUTHOR("Zev Weiss <zweiss@equinix.com>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("LM25066 power-switch driver");
-- 
2.31.1

Re: Enabling pmbus power control

[Zev Weiss]

On Tue, Apr 20, 2021 at 01:00:25AM CDT, Guenter Roeck wrote:
>On 4/19/21 10:50 PM, Zev Weiss wrote:
>[ ... ]
>
>> I had a glance at the enclosure driver; it looks pretty geared toward SES-like things (drivers/scsi/ses.c being its only usage I can see in the kernel at the moment) and while it could perhaps be pressed into working for this it seems like it would probably drag in a fair amount of boilerplate and result in a somewhat gratuitously confusing driver arrangement (calling the things involved in the cases we're looking at "enclosures" seems like a bit of a stretch).
>>
>> As an alternative, would something like the patch below be more along the lines of what you're suggesting?  And if so, would it make sense to generalize it into something like 'pmbus-switch.c' and add a PMBUS_HAVE_POWERSWITCH functionality bit or similar in the pmbus code instead of hardcoding it for only LM25066 support?
>>
>>
>
>No. Don't access pmbus functions from outside drivers/hwmon/pmbus.
>
>I used to be opposed to function export restrictions (aka export namespaces),
>but you are making a good case that we need to introduce them for pmbus
>functions.
>
>Guenter

Okay -- I figured that was likely to be frowned upon, but the 
alternative seemed to be effectively duplicating non-trivial chunks of 
the pmbus code.  However, upon realizing that the LM25066 doesn't 
actually require any of the paging work the generic pmbus code provides, 
I suppose it can actually be done with a simple smbus read & write.  
Does this version look better?


Zev


 From 1662e1c59c498ad6b208e6ab450bd467d71def34 Mon Sep 17 00:00:00 2001
From: Zev Weiss <zev@bewilderbeest.net>
Date: Wed, 31 Mar 2021 01:58:35 -0500
Subject: [PATCH] misc: add lm25066-switch driver

This driver allows an lm25066 to be switched on and off from userspace
via sysfs.

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
  drivers/misc/Kconfig          |   7 ++
  drivers/misc/Makefile         |   1 +
  drivers/misc/lm25066-switch.c | 126 ++++++++++++++++++++++++++++++++++
  3 files changed, 134 insertions(+)
  create mode 100644 drivers/misc/lm25066-switch.c

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index f532c59bb59b..384b6022ec15 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -445,6 +445,13 @@ config HISI_HIKEY_USB
  	  switching between the dual-role USB-C port and the USB-A host ports
  	  using only one USB controller.
  
+config LM25066_SWITCH
+	tristate "LM25066 power switch support"
+	depends on OF && SENSORS_LM25066
+	help
+	  This driver augments LM25066 hwmon support with power switch
+	  functionality controllable from userspace via sysfs.
+
  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 99b6f15a3c70..c948510d0cc9 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -56,3 +56,4 @@ obj-$(CONFIG_HABANA_AI)		+= habanalabs/
  obj-$(CONFIG_UACCE)		+= uacce/
  obj-$(CONFIG_XILINX_SDFEC)	+= xilinx_sdfec.o
  obj-$(CONFIG_HISI_HIKEY_USB)	+= hisi_hikey_usb.o
+obj-$(CONFIG_LM25066_SWITCH)	+= lm25066-switch.o
diff --git a/drivers/misc/lm25066-switch.c b/drivers/misc/lm25066-switch.c
new file mode 100644
index 000000000000..9adc67c320f9
--- /dev/null
+++ b/drivers/misc/lm25066-switch.c
@@ -0,0 +1,126 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * This module provides a thin wrapper around the lm25066 hwmon driver that
+ * additionally exposes a userspace-controllable on/off power switch via
+ * sysfs.
+ *
+ * Author: Zev Weiss <zweiss@equinix.com>
+ *
+ * Copyright (C) 2021 Equinix Services, Inc.
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/i2c.h>
+#include <linux/platform_device.h>
+
+/*
+ * The relevant PMBus command and data values for controlling the LM25066
+ * power state.  Because it's not a paged device we skip the usual paging
+ * business other PMBus devices might require.
+ */
+#define CMD_OPERATION 0x01
+#define OPERATION_ON 0x80
+#define OPERATION_OFF 0x00
+
+static ssize_t switch_show_state(struct device *dev, struct device_attribute *attr,
+                                 char *buf)
+{
+	struct i2c_client *pmic = dev_get_drvdata(dev);
+	ssize_t ret = i2c_smbus_read_byte_data(pmic, CMD_OPERATION);
+	if (ret < 0)
+		return ret;
+
+	return sysfs_emit(buf, "%s\n", (ret & OPERATION_ON) ? "on" : "off");
+}
+
+static ssize_t switch_set_state(struct device *dev, struct device_attribute *attr,
+                                const char *buf, size_t count)
+{
+	int status;
+	u8 value;
+	struct i2c_client *pmic = dev_get_drvdata(dev);
+	if (sysfs_streq(buf, "on"))
+		value = OPERATION_ON;
+	else if (sysfs_streq(buf, "off"))
+		value = OPERATION_OFF;
+	else
+		return -EINVAL;
+	status = i2c_smbus_write_byte_data(pmic, CMD_OPERATION, value);
+	return status ? : count;
+}
+
+static DEVICE_ATTR(state, 0644, switch_show_state, switch_set_state);
+
+static struct attribute *attributes[] = {
+	&dev_attr_state.attr,
+	NULL,
+};
+
+static const struct attribute_group attr_group = {
+	.attrs = attributes,
+};
+
+static int lm25066_switch_probe(struct platform_device *pdev)
+{
+	int status;
+	struct device_node *np = pdev->dev.of_node;
+	struct device_node *pmic_np;
+	struct i2c_client *pmic;
+
+	pmic_np = of_parse_phandle(np, "pmic", 0);
+	if (!pmic_np) {
+		dev_err(&pdev->dev, "Cannot parse lm25066-switch pmic\n");
+		return -ENODEV;
+	}
+
+	if (!of_device_is_compatible(pmic_np, "lm25066")) {
+		dev_err(&pdev->dev, "lm25066-switch pmic not lm25066 compatible");
+		status = -ENODEV;
+		goto out;
+	}
+
+	pmic = of_find_i2c_device_by_node(pmic_np);
+	if (!pmic) {
+		status = -EPROBE_DEFER;
+		goto out;
+	}
+
+	platform_set_drvdata(pdev, pmic);
+
+	status = sysfs_create_group(&pdev->dev.kobj, &attr_group);
+
+out:
+	of_node_put(pmic_np);
+	return status;
+}
+
+static int lm25066_switch_remove(struct platform_device *pdev)
+{
+	struct i2c_client *pmic = platform_get_drvdata(pdev);
+
+	sysfs_remove_group(&pdev->dev.kobj, &attr_group);
+	put_device(&pmic->dev);
+
+	return 0;
+}
+
+static const struct of_device_id lm25066_switch_table[] = {
+	{ .compatible = "lm25066-switch" },
+	{ },
+};
+
+static struct platform_driver lm25066_switch_driver = {
+	.driver = {
+		.name = "lm25066-switch",
+		.of_match_table = lm25066_switch_table,
+	},
+	.probe = lm25066_switch_probe,
+	.remove = lm25066_switch_remove,
+};
+
+module_platform_driver(lm25066_switch_driver);
+
+MODULE_AUTHOR("Zev Weiss <zweiss@equinix.com>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("LM25066 power-switch driver");
-- 
2.31.1

Re: Enabling pmbus power control

[Guenter Roeck]

On 4/20/21 12:06 AM, Zev Weiss wrote:
> On Tue, Apr 20, 2021 at 01:00:25AM CDT, Guenter Roeck wrote:
>> On 4/19/21 10:50 PM, Zev Weiss wrote:
>> [ ... ]
>>
>>> I had a glance at the enclosure driver; it looks pretty geared toward SES-like things (drivers/scsi/ses.c being its only usage I can see in the kernel at the moment) and while it could perhaps be pressed into working for this it seems like it would probably drag in a fair amount of boilerplate and result in a somewhat gratuitously confusing driver arrangement (calling the things involved in the cases we're looking at "enclosures" seems like a bit of a stretch).
>>>
>>> As an alternative, would something like the patch below be more along the lines of what you're suggesting?  And if so, would it make sense to generalize it into something like 'pmbus-switch.c' and add a PMBUS_HAVE_POWERSWITCH functionality bit or similar in the pmbus code instead of hardcoding it for only LM25066 support?
>>>
>>>
>>
>> No. Don't access pmbus functions from outside drivers/hwmon/pmbus.
>>
>> I used to be opposed to function export restrictions (aka export namespaces),
>> but you are making a good case that we need to introduce them for pmbus
>> functions.
>>
>> Guenter
> 
> Okay -- I figured that was likely to be frowned upon, but the alternative seemed to be effectively duplicating non-trivial chunks of the pmbus code.  However, upon realizing that the LM25066 doesn't actually require any of the paging work the generic pmbus code provides, I suppose it can actually be done with a simple smbus read & write.  Does this version look better?
> 

It is just getting worse and worse. You are re-implementing regulator
support for the lm25066. The correct solution would be to implement
regulator support in the lm25066 and use it from the secondary driver
(which should be chip independent).

Guenter

> 
> Zev
> 
> 
> From 1662e1c59c498ad6b208e6ab450bd467d71def34 Mon Sep 17 00:00:00 2001
> From: Zev Weiss <zev@bewilderbeest.net>
> Date: Wed, 31 Mar 2021 01:58:35 -0500
> Subject: [PATCH] misc: add lm25066-switch driver
> 
> This driver allows an lm25066 to be switched on and off from userspace
> via sysfs.
> 
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> ---
>  drivers/misc/Kconfig          |   7 ++
>  drivers/misc/Makefile         |   1 +
>  drivers/misc/lm25066-switch.c | 126 ++++++++++++++++++++++++++++++++++
>  3 files changed, 134 insertions(+)
>  create mode 100644 drivers/misc/lm25066-switch.c
> 
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index f532c59bb59b..384b6022ec15 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -445,6 +445,13 @@ config HISI_HIKEY_USB
>        switching between the dual-role USB-C port and the USB-A host ports
>        using only one USB controller.
>  
> +config LM25066_SWITCH
> +    tristate "LM25066 power switch support"
> +    depends on OF && SENSORS_LM25066
> +    help
> +      This driver augments LM25066 hwmon support with power switch
> +      functionality controllable from userspace via sysfs.
> +
>  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 99b6f15a3c70..c948510d0cc9 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -56,3 +56,4 @@ obj-$(CONFIG_HABANA_AI)        += habanalabs/
>  obj-$(CONFIG_UACCE)        += uacce/
>  obj-$(CONFIG_XILINX_SDFEC)    += xilinx_sdfec.o
>  obj-$(CONFIG_HISI_HIKEY_USB)    += hisi_hikey_usb.o
> +obj-$(CONFIG_LM25066_SWITCH)    += lm25066-switch.o
> diff --git a/drivers/misc/lm25066-switch.c b/drivers/misc/lm25066-switch.c
> new file mode 100644
> index 000000000000..9adc67c320f9
> --- /dev/null
> +++ b/drivers/misc/lm25066-switch.c
> @@ -0,0 +1,126 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * This module provides a thin wrapper around the lm25066 hwmon driver that
> + * additionally exposes a userspace-controllable on/off power switch via
> + * sysfs.
> + *
> + * Author: Zev Weiss <zweiss@equinix.com>
> + *
> + * Copyright (C) 2021 Equinix Services, Inc.
> + */
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/i2c.h>
> +#include <linux/platform_device.h>
> +
> +/*
> + * The relevant PMBus command and data values for controlling the LM25066
> + * power state.  Because it's not a paged device we skip the usual paging
> + * business other PMBus devices might require.
> + */
> +#define CMD_OPERATION 0x01
> +#define OPERATION_ON 0x80
> +#define OPERATION_OFF 0x00
> +
> +static ssize_t switch_show_state(struct device *dev, struct device_attribute *attr,
> +                                 char *buf)
> +{
> +    struct i2c_client *pmic = dev_get_drvdata(dev);
> +    ssize_t ret = i2c_smbus_read_byte_data(pmic, CMD_OPERATION);
> +    if (ret < 0)
> +        return ret;
> +
> +    return sysfs_emit(buf, "%s\n", (ret & OPERATION_ON) ? "on" : "off");
> +}
> +
> +static ssize_t switch_set_state(struct device *dev, struct device_attribute *attr,
> +                                const char *buf, size_t count)
> +{
> +    int status;
> +    u8 value;
> +    struct i2c_client *pmic = dev_get_drvdata(dev);
> +    if (sysfs_streq(buf, "on"))
> +        value = OPERATION_ON;
> +    else if (sysfs_streq(buf, "off"))
> +        value = OPERATION_OFF;
> +    else
> +        return -EINVAL;
> +    status = i2c_smbus_write_byte_data(pmic, CMD_OPERATION, value);
> +    return status ? : count;
> +}
> +
> +static DEVICE_ATTR(state, 0644, switch_show_state, switch_set_state);
> +
> +static struct attribute *attributes[] = {
> +    &dev_attr_state.attr,
> +    NULL,
> +};
> +
> +static const struct attribute_group attr_group = {
> +    .attrs = attributes,
> +};
> +
> +static int lm25066_switch_probe(struct platform_device *pdev)
> +{
> +    int status;
> +    struct device_node *np = pdev->dev.of_node;
> +    struct device_node *pmic_np;
> +    struct i2c_client *pmic;
> +
> +    pmic_np = of_parse_phandle(np, "pmic", 0);
> +    if (!pmic_np) {
> +        dev_err(&pdev->dev, "Cannot parse lm25066-switch pmic\n");
> +        return -ENODEV;
> +    }
> +
> +    if (!of_device_is_compatible(pmic_np, "lm25066")) {
> +        dev_err(&pdev->dev, "lm25066-switch pmic not lm25066 compatible");
> +        status = -ENODEV;
> +        goto out;
> +    }
> +
> +    pmic = of_find_i2c_device_by_node(pmic_np);
> +    if (!pmic) {
> +        status = -EPROBE_DEFER;
> +        goto out;
> +    }
> +
> +    platform_set_drvdata(pdev, pmic);
> +
> +    status = sysfs_create_group(&pdev->dev.kobj, &attr_group);
> +
> +out:
> +    of_node_put(pmic_np);
> +    return status;
> +}
> +
> +static int lm25066_switch_remove(struct platform_device *pdev)
> +{
> +    struct i2c_client *pmic = platform_get_drvdata(pdev);
> +
> +    sysfs_remove_group(&pdev->dev.kobj, &attr_group);
> +    put_device(&pmic->dev);
> +
> +    return 0;
> +}
> +
> +static const struct of_device_id lm25066_switch_table[] = {
> +    { .compatible = "lm25066-switch" },
> +    { },
> +};
> +
> +static struct platform_driver lm25066_switch_driver = {
> +    .driver = {
> +        .name = "lm25066-switch",
> +        .of_match_table = lm25066_switch_table,
> +    },
> +    .probe = lm25066_switch_probe,
> +    .remove = lm25066_switch_remove,
> +};
> +
> +module_platform_driver(lm25066_switch_driver);
> +
> +MODULE_AUTHOR("Zev Weiss <zweiss@equinix.com>");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("LM25066 power-switch driver");

Re: Enabling pmbus power control

[Guenter Roeck]

On 4/20/21 12:06 AM, Zev Weiss wrote:
> On Tue, Apr 20, 2021 at 01:00:25AM CDT, Guenter Roeck wrote:
>> On 4/19/21 10:50 PM, Zev Weiss wrote:
>> [ ... ]
>>
>>> I had a glance at the enclosure driver; it looks pretty geared toward SES-like things (drivers/scsi/ses.c being its only usage I can see in the kernel at the moment) and while it could perhaps be pressed into working for this it seems like it would probably drag in a fair amount of boilerplate and result in a somewhat gratuitously confusing driver arrangement (calling the things involved in the cases we're looking at "enclosures" seems like a bit of a stretch).
>>>
>>> As an alternative, would something like the patch below be more along the lines of what you're suggesting?  And if so, would it make sense to generalize it into something like 'pmbus-switch.c' and add a PMBUS_HAVE_POWERSWITCH functionality bit or similar in the pmbus code instead of hardcoding it for only LM25066 support?
>>>
>>>
>>
>> No. Don't access pmbus functions from outside drivers/hwmon/pmbus.
>>
>> I used to be opposed to function export restrictions (aka export namespaces),
>> but you are making a good case that we need to introduce them for pmbus
>> functions.
>>
>> Guenter
> 
> Okay -- I figured that was likely to be frowned upon, but the alternative seemed to be effectively duplicating non-trivial chunks of the pmbus code.  However, upon realizing that the LM25066 doesn't actually require any of the paging work the generic pmbus code provides, I suppose it can actually be done with a simple smbus read & write.  Does this version look better?
> 

It is just getting worse and worse. You are re-implementing regulator
support for the lm25066. The correct solution would be to implement
regulator support in the lm25066 and use it from the secondary driver
(which should be chip independent).

Guenter

> 
> Zev
> 
> 
> From 1662e1c59c498ad6b208e6ab450bd467d71def34 Mon Sep 17 00:00:00 2001
> From: Zev Weiss <zev@bewilderbeest.net>
> Date: Wed, 31 Mar 2021 01:58:35 -0500
> Subject: [PATCH] misc: add lm25066-switch driver
> 
> This driver allows an lm25066 to be switched on and off from userspace
> via sysfs.
> 
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> ---
>  drivers/misc/Kconfig          |   7 ++
>  drivers/misc/Makefile         |   1 +
>  drivers/misc/lm25066-switch.c | 126 ++++++++++++++++++++++++++++++++++
>  3 files changed, 134 insertions(+)
>  create mode 100644 drivers/misc/lm25066-switch.c
> 
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index f532c59bb59b..384b6022ec15 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -445,6 +445,13 @@ config HISI_HIKEY_USB
>        switching between the dual-role USB-C port and the USB-A host ports
>        using only one USB controller.
>  
> +config LM25066_SWITCH
> +    tristate "LM25066 power switch support"
> +    depends on OF && SENSORS_LM25066
> +    help
> +      This driver augments LM25066 hwmon support with power switch
> +      functionality controllable from userspace via sysfs.
> +
>  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 99b6f15a3c70..c948510d0cc9 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -56,3 +56,4 @@ obj-$(CONFIG_HABANA_AI)        += habanalabs/
>  obj-$(CONFIG_UACCE)        += uacce/
>  obj-$(CONFIG_XILINX_SDFEC)    += xilinx_sdfec.o
>  obj-$(CONFIG_HISI_HIKEY_USB)    += hisi_hikey_usb.o
> +obj-$(CONFIG_LM25066_SWITCH)    += lm25066-switch.o
> diff --git a/drivers/misc/lm25066-switch.c b/drivers/misc/lm25066-switch.c
> new file mode 100644
> index 000000000000..9adc67c320f9
> --- /dev/null
> +++ b/drivers/misc/lm25066-switch.c
> @@ -0,0 +1,126 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * This module provides a thin wrapper around the lm25066 hwmon driver that
> + * additionally exposes a userspace-controllable on/off power switch via
> + * sysfs.
> + *
> + * Author: Zev Weiss <zweiss@equinix.com>
> + *
> + * Copyright (C) 2021 Equinix Services, Inc.
> + */
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/i2c.h>
> +#include <linux/platform_device.h>
> +
> +/*
> + * The relevant PMBus command and data values for controlling the LM25066
> + * power state.  Because it's not a paged device we skip the usual paging
> + * business other PMBus devices might require.
> + */
> +#define CMD_OPERATION 0x01
> +#define OPERATION_ON 0x80
> +#define OPERATION_OFF 0x00
> +
> +static ssize_t switch_show_state(struct device *dev, struct device_attribute *attr,
> +                                 char *buf)
> +{
> +    struct i2c_client *pmic = dev_get_drvdata(dev);
> +    ssize_t ret = i2c_smbus_read_byte_data(pmic, CMD_OPERATION);
> +    if (ret < 0)
> +        return ret;
> +
> +    return sysfs_emit(buf, "%s\n", (ret & OPERATION_ON) ? "on" : "off");
> +}
> +
> +static ssize_t switch_set_state(struct device *dev, struct device_attribute *attr,
> +                                const char *buf, size_t count)
> +{
> +    int status;
> +    u8 value;
> +    struct i2c_client *pmic = dev_get_drvdata(dev);
> +    if (sysfs_streq(buf, "on"))
> +        value = OPERATION_ON;
> +    else if (sysfs_streq(buf, "off"))
> +        value = OPERATION_OFF;
> +    else
> +        return -EINVAL;
> +    status = i2c_smbus_write_byte_data(pmic, CMD_OPERATION, value);
> +    return status ? : count;
> +}
> +
> +static DEVICE_ATTR(state, 0644, switch_show_state, switch_set_state);
> +
> +static struct attribute *attributes[] = {
> +    &dev_attr_state.attr,
> +    NULL,
> +};
> +
> +static const struct attribute_group attr_group = {
> +    .attrs = attributes,
> +};
> +
> +static int lm25066_switch_probe(struct platform_device *pdev)
> +{
> +    int status;
> +    struct device_node *np = pdev->dev.of_node;
> +    struct device_node *pmic_np;
> +    struct i2c_client *pmic;
> +
> +    pmic_np = of_parse_phandle(np, "pmic", 0);
> +    if (!pmic_np) {
> +        dev_err(&pdev->dev, "Cannot parse lm25066-switch pmic\n");
> +        return -ENODEV;
> +    }
> +
> +    if (!of_device_is_compatible(pmic_np, "lm25066")) {
> +        dev_err(&pdev->dev, "lm25066-switch pmic not lm25066 compatible");
> +        status = -ENODEV;
> +        goto out;
> +    }
> +
> +    pmic = of_find_i2c_device_by_node(pmic_np);
> +    if (!pmic) {
> +        status = -EPROBE_DEFER;
> +        goto out;
> +    }
> +
> +    platform_set_drvdata(pdev, pmic);
> +
> +    status = sysfs_create_group(&pdev->dev.kobj, &attr_group);
> +
> +out:
> +    of_node_put(pmic_np);
> +    return status;
> +}
> +
> +static int lm25066_switch_remove(struct platform_device *pdev)
> +{
> +    struct i2c_client *pmic = platform_get_drvdata(pdev);
> +
> +    sysfs_remove_group(&pdev->dev.kobj, &attr_group);
> +    put_device(&pmic->dev);
> +
> +    return 0;
> +}
> +
> +static const struct of_device_id lm25066_switch_table[] = {
> +    { .compatible = "lm25066-switch" },
> +    { },
> +};
> +
> +static struct platform_driver lm25066_switch_driver = {
> +    .driver = {
> +        .name = "lm25066-switch",
> +        .of_match_table = lm25066_switch_table,
> +    },
> +    .probe = lm25066_switch_probe,
> +    .remove = lm25066_switch_remove,
> +};
> +
> +module_platform_driver(lm25066_switch_driver);
> +
> +MODULE_AUTHOR("Zev Weiss <zweiss@equinix.com>");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("LM25066 power-switch driver");

Re: Enabling pmbus power control

[Zev Weiss]

On Tue, Apr 20, 2021 at 05:52:16AM CDT, Guenter Roeck wrote:
>On 4/20/21 12:06 AM, Zev Weiss wrote:
>> On Tue, Apr 20, 2021 at 01:00:25AM CDT, Guenter Roeck wrote:
>>> On 4/19/21 10:50 PM, Zev Weiss wrote:
>>> [ ... ]
>>>
>>>> I had a glance at the enclosure driver; it looks pretty geared toward SES-like things (drivers/scsi/ses.c being its only usage I can see in the kernel at the moment) and while it could perhaps be pressed into working for this it seems like it would probably drag in a fair amount of boilerplate and result in a somewhat gratuitously confusing driver arrangement (calling the things involved in the cases we're looking at "enclosures" seems like a bit of a stretch).
>>>>
>>>> As an alternative, would something like the patch below be more along the lines of what you're suggesting?  And if so, would it make sense to generalize it into something like 'pmbus-switch.c' and add a PMBUS_HAVE_POWERSWITCH functionality bit or similar in the pmbus code instead of hardcoding it for only LM25066 support?
>>>>
>>>>
>>>
>>> No. Don't access pmbus functions from outside drivers/hwmon/pmbus.
>>>
>>> I used to be opposed to function export restrictions (aka export namespaces),
>>> but you are making a good case that we need to introduce them for pmbus
>>> functions.
>>>
>>> Guenter
>>
>> Okay -- I figured that was likely to be frowned upon, but the alternative seemed to be effectively duplicating non-trivial chunks of the pmbus code.  However, upon realizing that the LM25066 doesn't actually require any of the paging work the generic pmbus code provides, I suppose it can actually be done with a simple smbus read & write.  Does this version look better?
>>
>
>It is just getting worse and worse. You are re-implementing regulator
>support for the lm25066. The correct solution would be to implement
>regulator support in the lm25066 and use it from the secondary driver
>(which should be chip independent).
>
>Guenter
>

Implementing it by way of regulator support seems like it would make 
sense, but that in turn sounds like it would then end up just being a 
reimplementation of REGULATOR_USERSPACE_CONSUMER, which (unless there 
was some more subtle distinction that I missed) Mark already told me in 
no uncertain terms is not the way to go.  So I hope I could be forgiven 
for feeling a bit stuck here.

Mark, do you have any further input on what a viable approach might look 
like?


Thanks,
Zev

Re: Enabling pmbus power control

[Zev Weiss]

On Tue, Apr 20, 2021 at 05:52:16AM CDT, Guenter Roeck wrote:
>On 4/20/21 12:06 AM, Zev Weiss wrote:
>> On Tue, Apr 20, 2021 at 01:00:25AM CDT, Guenter Roeck wrote:
>>> On 4/19/21 10:50 PM, Zev Weiss wrote:
>>> [ ... ]
>>>
>>>> I had a glance at the enclosure driver; it looks pretty geared toward SES-like things (drivers/scsi/ses.c being its only usage I can see in the kernel at the moment) and while it could perhaps be pressed into working for this it seems like it would probably drag in a fair amount of boilerplate and result in a somewhat gratuitously confusing driver arrangement (calling the things involved in the cases we're looking at "enclosures" seems like a bit of a stretch).
>>>>
>>>> As an alternative, would something like the patch below be more along the lines of what you're suggesting?  And if so, would it make sense to generalize it into something like 'pmbus-switch.c' and add a PMBUS_HAVE_POWERSWITCH functionality bit or similar in the pmbus code instead of hardcoding it for only LM25066 support?
>>>>
>>>>
>>>
>>> No. Don't access pmbus functions from outside drivers/hwmon/pmbus.
>>>
>>> I used to be opposed to function export restrictions (aka export namespaces),
>>> but you are making a good case that we need to introduce them for pmbus
>>> functions.
>>>
>>> Guenter
>>
>> Okay -- I figured that was likely to be frowned upon, but the alternative seemed to be effectively duplicating non-trivial chunks of the pmbus code.  However, upon realizing that the LM25066 doesn't actually require any of the paging work the generic pmbus code provides, I suppose it can actually be done with a simple smbus read & write.  Does this version look better?
>>
>
>It is just getting worse and worse. You are re-implementing regulator
>support for the lm25066. The correct solution would be to implement
>regulator support in the lm25066 and use it from the secondary driver
>(which should be chip independent).
>
>Guenter
>

Implementing it by way of regulator support seems like it would make 
sense, but that in turn sounds like it would then end up just being a 
reimplementation of REGULATOR_USERSPACE_CONSUMER, which (unless there 
was some more subtle distinction that I missed) Mark already told me in 
no uncertain terms is not the way to go.  So I hope I could be forgiven 
for feeling a bit stuck here.

Mark, do you have any further input on what a viable approach might look 
like?


Thanks,
Zev

Re: Enabling pmbus power control

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 561 bytes --]

On Tue, Apr 20, 2021 at 10:19:04AM -0500, Zev Weiss wrote:

> Mark, do you have any further input on what a viable approach might look
> like?

I already suggested writing a driver or drivers that represent the
hardware you have, that advice remains.  It's hard to follow what you
were trying to say with your long mail earlier today but it seems like
you basically don't want to use any abstraction or framework, but that's
not really suitable for upstream integration - other hardware that looks
similar to the end user should look similar in the kernel too.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Enabling pmbus power control

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 561 bytes --]

On Tue, Apr 20, 2021 at 10:19:04AM -0500, Zev Weiss wrote:

> Mark, do you have any further input on what a viable approach might look
> like?

I already suggested writing a driver or drivers that represent the
hardware you have, that advice remains.  It's hard to follow what you
were trying to say with your long mail earlier today but it seems like
you basically don't want to use any abstraction or framework, but that's
not really suitable for upstream integration - other hardware that looks
similar to the end user should look similar in the kernel too.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Enabling pmbus power control

[Zev Weiss]

On Tue, Apr 20, 2021 at 11:13:18AM CDT, Mark Brown wrote:
>On Tue, Apr 20, 2021 at 10:19:04AM -0500, Zev Weiss wrote:
>
>> Mark, do you have any further input on what a viable approach might look
>> like?
>
>I already suggested writing a driver or drivers that represent the
>hardware you have, that advice remains.  It's hard to follow what you
>were trying to say with your long mail earlier today but it seems like

That email was an attempt to explain why writing a driver for the 
specific hardware devices we're powering seems like a poor fit to me.  
To summarize:

  - There's a wide variety of different devices potentially behind an 
    LM25066.

  - A hypothetical driver for any one of them would be completely 
    non-specific to that device and functionally identical to a driver 
    for any other, because the only hardware it would actually be 
    touching is the LM25066, and in ways that are again completely 
    non-specific to anything but the LM25066 itself.

>you basically don't want to use any abstraction or framework, but that's
>not really suitable for upstream integration

I'm not at all opposed to using a abstractions or frameworks (I'd very 
much like to do so in fact).  The problem is that I've thus far been 
unable to determine exactly what the appropriate one is.

>other hardware that looks similar to the end user should look similar 
>in the kernel too.

Agreed -- hence my disinclination to write drivers artificially specific 
to whatever is behind the LM25066.  What it looks like to the end user, 
and I'd hope evetually the kernel as well, is a simple power switch and 
nothing more.


Zev

Re: Enabling pmbus power control

[Zev Weiss]

On Tue, Apr 20, 2021 at 11:13:18AM CDT, Mark Brown wrote:
>On Tue, Apr 20, 2021 at 10:19:04AM -0500, Zev Weiss wrote:
>
>> Mark, do you have any further input on what a viable approach might look
>> like?
>
>I already suggested writing a driver or drivers that represent the
>hardware you have, that advice remains.  It's hard to follow what you
>were trying to say with your long mail earlier today but it seems like

That email was an attempt to explain why writing a driver for the 
specific hardware devices we're powering seems like a poor fit to me.  
To summarize:

  - There's a wide variety of different devices potentially behind an 
    LM25066.

  - A hypothetical driver for any one of them would be completely 
    non-specific to that device and functionally identical to a driver 
    for any other, because the only hardware it would actually be 
    touching is the LM25066, and in ways that are again completely 
    non-specific to anything but the LM25066 itself.

>you basically don't want to use any abstraction or framework, but that's
>not really suitable for upstream integration

I'm not at all opposed to using a abstractions or frameworks (I'd very 
much like to do so in fact).  The problem is that I've thus far been 
unable to determine exactly what the appropriate one is.

>other hardware that looks similar to the end user should look similar 
>in the kernel too.

Agreed -- hence my disinclination to write drivers artificially specific 
to whatever is behind the LM25066.  What it looks like to the end user, 
and I'd hope evetually the kernel as well, is a simple power switch and 
nothing more.


Zev

Re: Enabling pmbus power control

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2013 bytes --]

On Tue, Apr 20, 2021 at 11:40:24AM -0500, Zev Weiss wrote:
> On Tue, Apr 20, 2021 at 11:13:18AM CDT, Mark Brown wrote:

> > I already suggested writing a driver or drivers that represent the
> > hardware you have, that advice remains.  It's hard to follow what you
> > were trying to say with your long mail earlier today but it seems like

> That email was an attempt to explain why writing a driver for the specific
> hardware devices we're powering seems like a poor fit to me.  To summarize:

>  - There's a wide variety of different devices potentially behind an
> LM25066.

This is true for lots of hardware, we still integrate things into
frameworks.

>  - A hypothetical driver for any one of them would be completely
> non-specific to that device and functionally identical to a driver    for
> any other, because the only hardware it would actually be    touching is the
> LM25066, and in ways that are again completely    non-specific to anything
> but the LM25066 itself.

I don't see why that would be the case at all.  Even within the indended
application as a power controller for a hotpluggable bus there's plenty
of potential for integration into a wider representation of the socket
things get inserted into - for example I've worked with buses that had
support for operator signalling of hotplug (buttons to press to initiate
hot removal, with lights to signal when a clean shutdown of the card had
been completed), you might also want to have additional environment
monitoring and of course the labelling that I mentioned in an earlier
post.  I can imagine you probably have some other connection of some
kind to the host too (eg, network ports) to join up and perhaps sync
hotplug for.

> I'm not at all opposed to using a abstractions or frameworks (I'd very much
> like to do so in fact).  The problem is that I've thus far been unable to
> determine exactly what the appropriate one is.

Perhaps you need to write some kind of generic system for hotplugging
modules if you can't find one.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Enabling pmbus power control

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2013 bytes --]

On Tue, Apr 20, 2021 at 11:40:24AM -0500, Zev Weiss wrote:
> On Tue, Apr 20, 2021 at 11:13:18AM CDT, Mark Brown wrote:

> > I already suggested writing a driver or drivers that represent the
> > hardware you have, that advice remains.  It's hard to follow what you
> > were trying to say with your long mail earlier today but it seems like

> That email was an attempt to explain why writing a driver for the specific
> hardware devices we're powering seems like a poor fit to me.  To summarize:

>  - There's a wide variety of different devices potentially behind an
> LM25066.

This is true for lots of hardware, we still integrate things into
frameworks.

>  - A hypothetical driver for any one of them would be completely
> non-specific to that device and functionally identical to a driver    for
> any other, because the only hardware it would actually be    touching is the
> LM25066, and in ways that are again completely    non-specific to anything
> but the LM25066 itself.

I don't see why that would be the case at all.  Even within the indended
application as a power controller for a hotpluggable bus there's plenty
of potential for integration into a wider representation of the socket
things get inserted into - for example I've worked with buses that had
support for operator signalling of hotplug (buttons to press to initiate
hot removal, with lights to signal when a clean shutdown of the card had
been completed), you might also want to have additional environment
monitoring and of course the labelling that I mentioned in an earlier
post.  I can imagine you probably have some other connection of some
kind to the host too (eg, network ports) to join up and perhaps sync
hotplug for.

> I'm not at all opposed to using a abstractions or frameworks (I'd very much
> like to do so in fact).  The problem is that I've thus far been unable to
> determine exactly what the appropriate one is.

Perhaps you need to write some kind of generic system for hotplugging
modules if you can't find one.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Enabling pmbus power control

[Zev Weiss]

On Tue, Apr 20, 2021 at 12:15:40PM CDT, Mark Brown wrote:
>On Tue, Apr 20, 2021 at 11:40:24AM -0500, Zev Weiss wrote:
>> On Tue, Apr 20, 2021 at 11:13:18AM CDT, Mark Brown wrote:
>
>> > I already suggested writing a driver or drivers that represent the
>> > hardware you have, that advice remains.  It's hard to follow what you
>> > were trying to say with your long mail earlier today but it seems like
>
>> That email was an attempt to explain why writing a driver for the specific
>> hardware devices we're powering seems like a poor fit to me.  To summarize:
>
>>  - There's a wide variety of different devices potentially behind an
>> LM25066.
>
>This is true for lots of hardware, we still integrate things into
>frameworks.
>
>>  - A hypothetical driver for any one of them would be completely
>> non-specific to that device and functionally identical to a driver    for
>> any other, because the only hardware it would actually be    touching is the
>> LM25066, and in ways that are again completely    non-specific to anything
>> but the LM25066 itself.
>
>I don't see why that would be the case at all.  Even within the indended
>application as a power controller for a hotpluggable bus there's plenty
>of potential for integration into a wider representation of the socket
>things get inserted into - for example I've worked with buses that had
>support for operator signalling of hotplug (buttons to press to initiate
>hot removal, with lights to signal when a clean shutdown of the card had
>been completed), you might also want to have additional environment
>monitoring and of course the labelling that I mentioned in an earlier
>post.  I can imagine you probably have some other connection of some
>kind to the host too (eg, network ports) to join up and perhaps sync
>hotplug for.
>

Consider the power shelf I mentioned earlier -- it's a rackmount power 
supply and that's about it.  It provides DC power to arbitrary devices 
that it has no other connection to, just ground and +12V.  Those devices 
might be servers, or cooling fans, or vacuum cleaners or floodlights -- 
the power shelf doesn't know, or care.  It's a lot like a switchable 
network PDU in that it just provides a way for an operator to remotely 
cut power to a thing that's plugged into it.  There's no other bus or 
anything in the picture.  (And pragmatically, given that its most common 
usage is likely to be to provide a cold power cycle as a last-ditch 
recovery option when things are wedged in some unresponsive state, 
attempting any sort of coordination with the downstream device would 
probably be a dead end anyway.)


Zev

Re: Enabling pmbus power control

[Zev Weiss]

On Tue, Apr 20, 2021 at 12:15:40PM CDT, Mark Brown wrote:
>On Tue, Apr 20, 2021 at 11:40:24AM -0500, Zev Weiss wrote:
>> On Tue, Apr 20, 2021 at 11:13:18AM CDT, Mark Brown wrote:
>
>> > I already suggested writing a driver or drivers that represent the
>> > hardware you have, that advice remains.  It's hard to follow what you
>> > were trying to say with your long mail earlier today but it seems like
>
>> That email was an attempt to explain why writing a driver for the specific
>> hardware devices we're powering seems like a poor fit to me.  To summarize:
>
>>  - There's a wide variety of different devices potentially behind an
>> LM25066.
>
>This is true for lots of hardware, we still integrate things into
>frameworks.
>
>>  - A hypothetical driver for any one of them would be completely
>> non-specific to that device and functionally identical to a driver    for
>> any other, because the only hardware it would actually be    touching is the
>> LM25066, and in ways that are again completely    non-specific to anything
>> but the LM25066 itself.
>
>I don't see why that would be the case at all.  Even within the indended
>application as a power controller for a hotpluggable bus there's plenty
>of potential for integration into a wider representation of the socket
>things get inserted into - for example I've worked with buses that had
>support for operator signalling of hotplug (buttons to press to initiate
>hot removal, with lights to signal when a clean shutdown of the card had
>been completed), you might also want to have additional environment
>monitoring and of course the labelling that I mentioned in an earlier
>post.  I can imagine you probably have some other connection of some
>kind to the host too (eg, network ports) to join up and perhaps sync
>hotplug for.
>

Consider the power shelf I mentioned earlier -- it's a rackmount power 
supply and that's about it.  It provides DC power to arbitrary devices 
that it has no other connection to, just ground and +12V.  Those devices 
might be servers, or cooling fans, or vacuum cleaners or floodlights -- 
the power shelf doesn't know, or care.  It's a lot like a switchable 
network PDU in that it just provides a way for an operator to remotely 
cut power to a thing that's plugged into it.  There's no other bus or 
anything in the picture.  (And pragmatically, given that its most common 
usage is likely to be to provide a cold power cycle as a last-ditch 
recovery option when things are wedged in some unresponsive state, 
attempting any sort of coordination with the downstream device would 
probably be a dead end anyway.)


Zev

Re: Enabling pmbus power control

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 813 bytes --]

On Tue, Apr 20, 2021 at 01:54:10PM -0500, Zev Weiss wrote:

> Consider the power shelf I mentioned earlier -- it's a rackmount power
> supply and that's about it.  It provides DC power to arbitrary devices that
> it has no other connection to, just ground and +12V.  Those devices might be
> servers, or cooling fans, or vacuum cleaners or floodlights -- the power
> shelf doesn't know, or care.  It's a lot like a switchable network PDU in

If your chassis is particularly simple then it will be particularly
simple to fit into a generic framework so that should make your life a
lot easier here.  Generally the simpler your system is the easier it
will be to use in something generic, it's not going to be stretching
ideas about how things should look and is more likely to have good
helpers available already.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Enabling pmbus power control

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 813 bytes --]

On Tue, Apr 20, 2021 at 01:54:10PM -0500, Zev Weiss wrote:

> Consider the power shelf I mentioned earlier -- it's a rackmount power
> supply and that's about it.  It provides DC power to arbitrary devices that
> it has no other connection to, just ground and +12V.  Those devices might be
> servers, or cooling fans, or vacuum cleaners or floodlights -- the power
> shelf doesn't know, or care.  It's a lot like a switchable network PDU in

If your chassis is particularly simple then it will be particularly
simple to fit into a generic framework so that should make your life a
lot easier here.  Generally the simpler your system is the easier it
will be to use in something generic, it's not going to be stretching
ideas about how things should look and is more likely to have good
helpers available already.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Enabling pmbus power control

[Zev Weiss]

On Wed, Apr 21, 2021 at 06:05:40AM CDT, Mark Brown wrote:
>On Tue, Apr 20, 2021 at 01:54:10PM -0500, Zev Weiss wrote:
>
>> Consider the power shelf I mentioned earlier -- it's a rackmount power
>> supply and that's about it.  It provides DC power to arbitrary devices that
>> it has no other connection to, just ground and +12V.  Those devices might be
>> servers, or cooling fans, or vacuum cleaners or floodlights -- the power
>> shelf doesn't know, or care.  It's a lot like a switchable network PDU in
>
>If your chassis is particularly simple then it will be particularly
>simple to fit into a generic framework so that should make your life a
>lot easier here.  Generally the simpler your system is the easier it
>will be to use in something generic, it's not going to be stretching
>ideas about how things should look and is more likely to have good
>helpers available already.

The simplicity of the use-case should make it easy to implement via a 
generic framework, yes.  But at the same time, if we're talking about 
that being a new framework that doesn't currently exist, the minimal 
needs of this case make it difficult for me to see what sort of 
structure or additional functionality would be required of such a 
framework to support more complex cases, because the simple/minimal case 
is the only example I have at hand.  I think there's also (quite 
reasonably) a general reluctance to merge infrastructure that doesn't 
have any users.

Given that, I'd think the appropriate approach for a first-cut 
implementation of that would be to only implement what's presently 
needed, and put off incorporating any other bells and whistles until 
there's something that would use them.  It seems like a minimal, 
only-what's-needed version of that at present would end up looking 
extremely similar to reg-userspace-consumer though.  Would that not be 
problematic?


Zev

Re: Enabling pmbus power control

[Zev Weiss]

On Wed, Apr 21, 2021 at 06:05:40AM CDT, Mark Brown wrote:
>On Tue, Apr 20, 2021 at 01:54:10PM -0500, Zev Weiss wrote:
>
>> Consider the power shelf I mentioned earlier -- it's a rackmount power
>> supply and that's about it.  It provides DC power to arbitrary devices that
>> it has no other connection to, just ground and +12V.  Those devices might be
>> servers, or cooling fans, or vacuum cleaners or floodlights -- the power
>> shelf doesn't know, or care.  It's a lot like a switchable network PDU in
>
>If your chassis is particularly simple then it will be particularly
>simple to fit into a generic framework so that should make your life a
>lot easier here.  Generally the simpler your system is the easier it
>will be to use in something generic, it's not going to be stretching
>ideas about how things should look and is more likely to have good
>helpers available already.

The simplicity of the use-case should make it easy to implement via a 
generic framework, yes.  But at the same time, if we're talking about 
that being a new framework that doesn't currently exist, the minimal 
needs of this case make it difficult for me to see what sort of 
structure or additional functionality would be required of such a 
framework to support more complex cases, because the simple/minimal case 
is the only example I have at hand.  I think there's also (quite 
reasonably) a general reluctance to merge infrastructure that doesn't 
have any users.

Given that, I'd think the appropriate approach for a first-cut 
implementation of that would be to only implement what's presently 
needed, and put off incorporating any other bells and whistles until 
there's something that would use them.  It seems like a minimal, 
only-what's-needed version of that at present would end up looking 
extremely similar to reg-userspace-consumer though.  Would that not be 
problematic?


Zev