On 3/18/25 03:03, Encarnacion, Cedric justine wrote:
-----Original Message-----
From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck
Sent: Friday, February 28, 2025 12:33 AM
To: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
Cc: Rob Herring <robh@xxxxxxxxxx>; Encarnacion, Cedric justine
<Cedricjustine.Encarnacion@xxxxxxxxxx>; Krzysztof Kozlowski
<krzk+dt@xxxxxxxxxx>; Conor Dooley <conor+dt@xxxxxxxxxx>; Jean Delvare
<jdelvare@xxxxxxxx>; Jonathan Corbet <corbet@xxxxxxx>; Delphine CC Chiu
<Delphine_CC_Chiu@xxxxxxxxxx>; devicetree@xxxxxxxxxxxxxxx; linux-
kernel@xxxxxxxxxxxxxxx; linux-hwmon@xxxxxxxxxxxxxxx; linux-
doc@xxxxxxxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx
Subject: Re: [PATCH v2 1/2] dt-bindings: hwmon: pmbus: add lt3074
[External]
On Thu, Feb 27, 2025 at 09:50:23AM +0100, Krzysztof Kozlowski wrote:
hwmon code might need some changes, but that's not really
relevant for proper hardware description.
Normally, I would agree, but it seems generic pmbus code expects
this structure. This just came up with changing another binding
maintained by 'Not Me' to follow this structure. We're stuck with
the existing way, so I don't know that it is worth supporting 2
ways forever. OTOH, is it guaranteed that these devices will only
ever be pmbus devices or that other regulator devices which are
not handled as pmbus devices currently will be in the future. If
so, more flexibility in the bindings will be needed.
I would appreciate if someone would explain to me what the problems
with the current PMBus code actually are. I have seen several
comments claiming
Not exactly a problem but missing feature. pmbus code (at least one of
macros I looked at) expects regulator node and some sort of child of
it (vout), while such simple devices should be:
regulator {
compatible = "adi,lt3074";
regulator-name = "vout";
regulator-min-microvolt = "100000";
regulator-max-microvolt = "100000";
};
so without any of regulators and regulators/vout subnodes.
that the code should be changed, but I have no idea what the
expected changes actually are or, in other words, what the PMBus
code should be doing differently.
I did not investigate much into pmbus code, but this might be as
simple as accepting arguments for .of_match and .regulators_node and
then accepting NULLs as them as well. Or a new macro which assigns
NULLs there.
Unless I am missing something, the following should do the trick.
diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index ddb19c9726d6..289767e5d599 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -512,7 +512,6 @@ int pmbus_regulator_init_cb(struct regulator_dev *rdev,
{ \
.name = (_name), \
.of_match = of_match_ptr(_name), \
- .regulators_node = of_match_ptr("regulators"), \
.ops = &pmbus_regulator_ops, \
.type = REGULATOR_VOLTAGE, \
.owner = THIS_MODULE, \
Maybe someone can check if that works.
Thanks,
Guenter
I'd like to follow up on this one. As of this writing, my understanding
is that the dt-binding should not expect regulators subnodes for
simple devices like this. There is already a similar binding as
mentioned in this thread particularly
"dt-bindings/regulator/infineon,ir38060". I think a binding without
the subnodes should still work with or without the change above.
Interesting. I am not sure if it really works, though. I looked into
the regulator code, and I don't immediately see the code path it would
take.
With this, I'd like to know what the specific next steps are to continue
this patch series.
Can you try on hardware using a devicetree file which doesn't have the
regulators node ? If the current code works, just submit an updated
(simplified) .yaml file and we should be good. If not, I have an
untested patch series introducing another macro which doesn't set
the regulators node.
Thanks,
Guenter
