linux-kernel - Re: [PATCH v2 1/2] dt-bindings: hwmon: pmbus: add lt3074

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <15ce883f-444c-4b27-a48d-b17e3df5895d@roeck-us.net>
Date: Tue, 18 Mar 2025 08:17:27 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: "Encarnacion, Cedric justine" <Cedricjustine.Encarnacion@...log.com>,
 Krzysztof Kozlowski <krzk@...nel.org>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Jean Delvare <jdelvare@...e.com>,
 Jonathan Corbet <corbet@....net>,
 Delphine CC Chiu <Delphine_CC_Chiu@...ynn.com>,
 "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>,
 "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
 "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>
Subject: Re: [PATCH v2 1/2] dt-bindings: hwmon: pmbus: add lt3074

On 3/18/25 03:03, Encarnacion, Cedric justine wrote:
>> -----Original Message-----
>> From: Guenter Roeck <groeck7@...il.com> On Behalf Of Guenter Roeck
>> Sent: Friday, February 28, 2025 12:33 AM
>> To: Krzysztof Kozlowski <krzk@...nel.org>
>> Cc: Rob Herring <robh@...nel.org>; Encarnacion, Cedric justine
>> <Cedricjustine.Encarnacion@...log.com>; Krzysztof Kozlowski
>> <krzk+dt@...nel.org>; Conor Dooley <conor+dt@...nel.org>; Jean Delvare
>> <jdelvare@...e.com>; Jonathan Corbet <corbet@....net>; Delphine CC Chiu
>> <Delphine_CC_Chiu@...ynn.com>; devicetree@...r.kernel.org; linux-
>> kernel@...r.kernel.org; linux-hwmon@...r.kernel.org; linux-
>> doc@...r.kernel.org; linux-i2c@...r.kernel.org
>> 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