| Message ID | 20240223-player-buckskin-01405c5889c4@spud (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | PMBUS Regulator cleanups | expand |
On Fri, Feb 23, 2024 at 08:21:08AM PST, Conor Dooley wrote: >From: Guenter Roeck <linux@roeck-us.net> > >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. > As mentioned on Guenter's v1, this change necessitates a corresponding update to arch/arm/boot/dts/aspeed/aspeed-bmc-delta-ahe50dc.dts, which has a dependency on the name of the regulator. Given (AFAICT) the lack of any combined dts & driver patches anywhere in the kernel git history I guess maybe doing both atomically in a single commit might not be considered kosher, but could it at least be included in the same patch series? Thanks, Zev
On Fri, Feb 23, 2024 at 03:16:28PM PST, Zev Weiss wrote: >On Fri, Feb 23, 2024 at 08:21:08AM PST, Conor Dooley wrote: >>From: Guenter Roeck <linux@roeck-us.net> >> >>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. >> > >Given (AFAICT) the >lack of any combined dts & driver patches anywhere >in the kernel git history I guess maybe doing both atomically in a >single commit might not be considered kosher, but could it at least be >included in the same patch series? > Ah, except I realize now I neglected to pass '--full-diff' to 'git log' when checking that, and after fixing that I see there is in fact some precedent for commits changing device-trees and driver code together, so ideally that would be my preference here too. Thanks, Zev
On 2/23/24 15:26, Zev Weiss wrote: > On Fri, Feb 23, 2024 at 03:16:28PM PST, Zev Weiss wrote: >> On Fri, Feb 23, 2024 at 08:21:08AM PST, Conor Dooley wrote: >>> From: Guenter Roeck <linux@roeck-us.net> >>> >>> 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. >>> > > >> >> Given (AFAICT) the >lack of any combined dts & driver patches anywhere in the kernel git history I guess maybe doing both atomically in a single commit might not be considered kosher, but could it at least be included in the same patch series? >> > > Ah, except I realize now I neglected to pass '--full-diff' to 'git log' when checking that, and after fixing that I see there is in fact some precedent for commits changing device-trees and driver code together, so ideally that would be my preference here too. > That is not going to happen in the hwmon subsystem unless something slips by. In a large project like the Linux kernel you'll find precedents for everything, so citing one is not a valid argument. As a general rule, I don't apply patches in .dts[i] files through the hwmon branch at all, not even as part of a patch series. Architecture maintainers tend to strongly oppose that idea, for the simple reason that it creates the risk of merge conflicts and thus of bugs during commit windows. Guenter
On Fri, Feb 23, 2024 at 03:16:26PM -0800, Zev Weiss wrote: > On Fri, Feb 23, 2024 at 08:21:08AM PST, Conor Dooley wrote: > > From: Guenter Roeck <linux@roeck-us.net> > > > > 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. > > > > As mentioned on Guenter's v1, this change necessitates a corresponding > update to arch/arm/boot/dts/aspeed/aspeed-bmc-delta-ahe50dc.dts, which has a > dependency on the name of the regulator. Given (AFAICT) the lack of any > combined dts & driver patches anywhere in the kernel git history I guess > maybe doing both atomically in a single commit might not be considered > kosher, but could it at least be included in the same patch series? I was gonna do it in isolation if this went ahead, but I see you went and did it yourself - thanks. As Guenter pointed out later in the thread, any patch that combines dts changes with drivers or bindings is something that likely snuck in or was some sort of extenuating circumstance. 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