linux-kernel - Re: [RFC PATCH v2 1/6] dt-bindings: ROHM BD96801 PMIC regulators

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <5994ff29-c916-4b5d-a634-8521e79e2417@gmail.com>
Date: Mon, 15 Apr 2024 09:51:28 +0300
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
 Matti Vaittinen <matti.vaittinen@...rohmeurope.com>
Cc: Lee Jones <lee@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>, Liam Girdwood <lgirdwood@...il.com>,
 Mark Brown <broonie@...nel.org>, Wim Van Sebroeck <wim@...ux-watchdog.org>,
 Guenter Roeck <linux@...ck-us.net>, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-watchdog@...r.kernel.org
Subject: Re: [RFC PATCH v2 1/6] dt-bindings: ROHM BD96801 PMIC regulators

On 4/14/24 00:27, Krzysztof Kozlowski wrote:
> On 12/04/2024 13:21, Matti Vaittinen wrote:
>> ROHM BD96801 is a highly configurable automotive grade PMIC. Introduce
>> DT bindings for the BD96801 regulators.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@...il.com>
>> ---
>> Revision history:
>> - No changes since RFCv1
> 
> Subject: missing "regulator" prefix, as first.
> 
>>
>>   .../regulator/rohm,bd96801-regulator.yaml     | 69 +++++++++++++++++++
>>   1 file changed, 69 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/regulator/rohm,bd96801-regulator.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/regulator/rohm,bd96801-regulator.yaml b/Documentation/devicetree/bindings/regulator/rohm,bd96801-regulator.yaml
>> new file mode 100644
>> index 000000000000..4015802a3d84
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/regulator/rohm,bd96801-regulator.yaml
>> @@ -0,0 +1,69 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/regulator/rohm,bd96801-regulator.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: ROHM BD96801 Power Management Integrated Circuit regulators
>> +
>> +maintainers:
>> +  - Matti Vaittinen <matti.vaittinen@...rohmeurope.com>
>> +
>> +description: |
>> +  This module is part of the ROHM BD96801 MFD device. For more details
>> +  see Documentation/devicetree/bindings/mfd/rohm,bd96801-pmic.yaml.
>> +
>> +  The regulator controller is represented as a sub-node of the PMIC node
>> +  on the device tree.
>> +
>> +  Regulator nodes should be named to BUCK_<number> and LDO_<number>.
>> +  The valid names for BD96801 regulator nodes are
>> +  BUCK1, BUCK2, BUCK3, BUCK4, LDO5, LDO6, LDO7
>> +
>> +patternProperties:
>> +  "^LDO[5-7]$":
> 
> lowercase
> 
>> +    type: object
>> +    description:
>> +      Properties for single LDO regulator.
>> +    $ref: regulator.yaml#
> 
> Missing unevaluatedProperties: false
> 
>> +
>> +    properties:
>> +      regulator-name:
>> +        pattern: "^ldo[5-7]$"
>> +        description:
>> +          Name of the regulator. Should be "ldo5", ..., "ldo7"
> 
> Why do you enforce the name? The name should match board schematics, not
> regulator datasheet.

If my memory serves me right, the slightly peculiar thing with the 
regulator core is it does matching of the regulators based on the names 
of the nodes. There was the regulator-compatible property, but I think 
it has been deprecated long ago.

https://elixir.bootlin.com/linux/latest/source/drivers/regulator/of_regulator.c#L380

Hence the regulators tend to have fixed names for the nodes. Unless 
there has been some recent changes I am not aware of...

>> +      rohm,initial-voltage-microvolt:
>> +        description:
>> +          Initial voltage for regulator. Voltage can be tuned +/-150 mV from
>> +          this value. NOTE, This can be modified via I2C only when PMIC is in
>> +          STBY state.
>> +        minimum: 300000
>> +        maximum: 3300000
> 
> Hm, regulator min/max microvolts properties don't work for you? The
> initial will be just middle?

I had not even thought of this!

I think this is a good idea. The problem I see is if the system where 
the PMIC is used will need to have 'initial power level' at start-up, 
which is near the one end of the allowed voltage area. (This because the 
"tuning"-range is quite narrow after the initial voltage is set). Wide 
allowed voltage range may be needed if the PMIC is reconfigured using 
the PMIC STBY state during the runtime.

Eg, sequence would look like:

Bootup:
PMIC STBY:
  - initial value 'A' from DT
=> PMIC ACTIVE
  - desired (early) voltages 'A' + 'tune'

..

Voltage state differing more than the 'tune' needed due to some runtime 
use-case:
=> PMIC STBY
  - initial value 'B'
=> PMIC ACTIVE
  - desired voltages 'B' + 'tune'

Now, if the 'A' can be 'far' from the mid point of the 'allowed 
voltages' -range.

I have no idea how valid this use-case is though. Once again, I work for 
a component vendor and don't get to see the forest from the trees... But 
sure I would like to enable as many possible use-cases as, well, possible :)

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~