| Message ID | 20240214194342.3678254-3-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | hwmon: (pmbus) Use PMBUS_REGULATOR_ONE to declare regulators with single output | expand |
On Wed, Feb 14, 2024 at 11:43:41AM PST, Guenter Roeck wrote: >If a chip only provides a single regulator, it should be named 'vout' >and not 'vout0'. Declare regulator using PMBUS_REGULATOR_ONE() to make >that happen. > Hi Guenter, This will necessitate a DTS update on at least one platform to maintain compatibility (Delta ahe50dc BMC, [1]). I'm not sure offhand if there are process/policy rules about mixing code changes and device-tree changes in the same commit, but changing either one without the other would break things. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/aspeed/aspeed-bmc-delta-ahe50dc.dts?id=8d3dea210042f54b952b481838c1e7dfc4ec751d#n21 Thanks, Zev
On 2/14/24 17:04, Zev Weiss wrote: > On Wed, Feb 14, 2024 at 11:43:41AM PST, Guenter Roeck wrote: >> If a chip only provides a single regulator, it should be named 'vout' >> and not 'vout0'. Declare regulator using PMBUS_REGULATOR_ONE() to make >> that happen. >> > > Hi Guenter, > > This will necessitate a DTS update on at least one platform to maintain compatibility (Delta ahe50dc BMC, [1]). I'm not sure offhand if there are process/policy rules about mixing code changes and device-tree changes in the same commit, but changing either one without the other would break things. > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/aspeed/aspeed-bmc-delta-ahe50dc.dts?id=8d3dea210042f54b952b481838c1e7dfc4ec751d#n21 > > Sigh. Agreed, especially since changing the dts file in the kernel won't change the dtb files on actual hardware. I really have no good solution for this. We (Well, I) didn't realize that there are regulator naming conventions/restrictions when we introduced regulator support into PMBus drivers. My bad. Let's see what others say. Guenter
On Wed, Feb 14, 2024 at 05:22:35PM PST, Guenter Roeck wrote: >On 2/14/24 17:04, Zev Weiss wrote: >>On Wed, Feb 14, 2024 at 11:43:41AM PST, Guenter Roeck wrote: >>>If a chip only provides a single regulator, it should be named 'vout' >>>and not 'vout0'. Declare regulator using PMBUS_REGULATOR_ONE() to make >>>that happen. >>> >> >>Hi Guenter, >> >>This will necessitate a DTS update on at least one platform to maintain compatibility (Delta ahe50dc BMC, [1]). I'm not sure offhand if there are process/policy rules about mixing code changes and device-tree changes in the same commit, but changing either one without the other would break things. >> >>[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/aspeed/aspeed-bmc-delta-ahe50dc.dts?id=8d3dea210042f54b952b481838c1e7dfc4ec751d#n21 >> >> > >Sigh. Agreed, especially since changing the dts file in the kernel >won't change the dtb files on actual hardware. > >I really have no good solution for this. We (Well, I) didn't realize that >there are regulator naming conventions/restrictions when we introduced >regulator support into PMBus drivers. My bad. Let's see what others say. > >Guenter > Well, perhaps mitigating that slightly: I don't see any obvious cases of any other platforms' device-trees having any dependencies on the regulator naming that would be affected by this (judging by 'git grep vout0 arch/*/boot/dts' anyway), and at least with OpenBMC on the ahe50dc (the primary and AFAIK only user of that device-tree) the dtb would also be updated along with any kernel update. So I wouldn't expect it to cause anyone any actual problems if we went ahead and changed it anyway; as long as the dts & driver do stay in sync with each other, maybe we could let it slide if it's otherwise a desirable change to make? Zev
On Thu, Feb 15, 2024 at 02:14:37AM -0800, Zev Weiss wrote: > On Wed, Feb 14, 2024 at 05:22:35PM PST, Guenter Roeck wrote: > > On 2/14/24 17:04, Zev Weiss wrote: > > > On Wed, Feb 14, 2024 at 11:43:41AM PST, Guenter Roeck wrote: > > > > If a chip only provides a single regulator, it should be named 'vout' > > > > and not 'vout0'. Declare regulator using PMBUS_REGULATOR_ONE() to make > > > > that happen. > > > > > > > > > > Hi Guenter, > > > > > > This will necessitate a DTS update on at least one platform to maintain compatibility (Delta ahe50dc BMC, [1]). I'm not sure offhand if there are process/policy rules about mixing code changes and device-tree changes in the same commit, but changing either one without the other would break things. > > > > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/aspeed/aspeed-bmc-delta-ahe50dc.dts?id=8d3dea210042f54b952b481838c1e7dfc4ec751d#n21 > > > > > > > > > > Sigh. Agreed, especially since changing the dts file in the kernel > > won't change the dtb files on actual hardware. > > > > I really have no good solution for this. We (Well, I) didn't realize that > > there are regulator naming conventions/restrictions when we introduced > > regulator support into PMBus drivers. My bad. Let's see what others say. > > > > Guenter > > > > Well, perhaps mitigating that slightly: I don't see any obvious cases of any > other platforms' device-trees having any dependencies on the regulator > naming that would be affected by this (judging by 'git grep vout0 > arch/*/boot/dts' anyway), Which is a good thing, as none of these devices' bindings actually allow regulator. Aspeed devicetrees are a mess of non-compliance with the bindings, so I suppose that this is not surprising, but somewhere in the wall of complaints there is a: aspeed-bmc-delta-ahe50dc.dtb: /ahb/apb/bus@1e78a000/i2c-bus@40/pca9541@79/i2c-arb/efuse@12: failed to match any schema with compatible: ['lm25066'] I mentioned this on the other thread, but I guess the kernel is actually using the i2c_device_ids to probe the driver and not the compatible, since the documented compatible and the compatibles in the driver have a vendor prefix? > and at least with OpenBMC on the ahe50dc (the > primary and AFAIK only user of that device-tree) the dtb would also be > updated along with any kernel update. > > So I wouldn't expect it to cause anyone any actual problems if we went ahead > and changed it anyway; as long as the dts & driver do stay in sync with each > other, maybe we could let it slide if it's otherwise a desirable change to > make? I think having "vout0" when there's nothing else looks a bit odd, but I'm not gonna lose sleep over "vout0" being used if its used in device documentation or is convention. Whatever is done, please document the regulator child node in the binding for this device and fix the dts to use a real compatible. Cheers, Conor.
diff --git a/drivers/hwmon/pmbus/lm25066.c b/drivers/hwmon/pmbus/lm25066.c index 3a20df5a43ec..cfffa4cdc0df 100644 --- a/drivers/hwmon/pmbus/lm25066.c +++ b/drivers/hwmon/pmbus/lm25066.c @@ -437,7 +437,7 @@ static int lm25066_write_word_data(struct i2c_client *client, int page, int reg, #if IS_ENABLED(CONFIG_SENSORS_LM25066_REGULATOR) static const struct regulator_desc lm25066_reg_desc[] = { - PMBUS_REGULATOR("vout", 0), + PMBUS_REGULATOR_ONE("vout"), }; #endif
If a chip only provides a single regulator, it should be named 'vout' and not 'vout0'. Declare regulator using PMBUS_REGULATOR_ONE() to make that happen. Cc: Conor Dooley <conor@kernel.org> Cc: Naresh Solanki <naresh.solanki@9elements.com> Cc: Zev Weiss <zev@bewilderbeest.net> Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- drivers/hwmon/pmbus/lm25066.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)