Re: Enabling pmbus power control

From: Guenter Roeck <linux@roeck-us.net>
To: Zev Weiss <zev@bewilderbeest.net>,
	Jean Delvare <jdelvare@suse.com>,
	linux-hwmon@vger.kernel.org
Cc: Andrew Jeffery <andrew@aj.id.au>,
	openbmc@lists.ozlabs.org, Mark Brown <broonie@kernel.org>,
	Liam Girdwood <lgirdwood@gmail.com>,
	linux-kernel@vger.kernel.org
Subject: Re: Enabling pmbus power control
Date: Tue, 30 Mar 2021 03:34:16 -0700	[thread overview]
Message-ID: <5105ada1-643a-8e58-a52d-d3c8dbef86b9@roeck-us.net> (raw)
In-Reply-To: <YGLepYLvtlO6Ikzs@hatter.bewilderbeest.net>

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
> 