diff mbox series

[v2,4/5] hwmon: (pmbus/lm25066) Use PMBUS_REGULATOR_ONE to declare regulator

Message ID 20240223-player-buckskin-01405c5889c4@spud
State New
Headers
Series PMBUS Regulator cleanups |

Commit Message

Conor Dooley Feb. 23, 2024, 4:21 p.m. UTC 
  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.

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>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/hwmon/pmbus/lm25066.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Zev Weiss Feb. 23, 2024, 11:16 p.m. UTC | #1 
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
Zev Weiss Feb. 23, 2024, 11:26 p.m. UTC | #2 
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
Guenter Roeck Feb. 24, 2024, 12:12 a.m. UTC | #3 
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
Conor Dooley Feb. 24, 2024, 7:35 p.m. UTC | #4 
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 mbox series

Patch

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