linux-kernel - Re: [PATCH v2 02/17] regulator: dt-bindings: add s2mpg10-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: <20250611-statuesque-dolphin-of-felicity-6fbf54@kuoka>
Date: Wed, 11 Jun 2025 10:55:44 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: André Draszik <andre.draszik@...aro.org>
Cc: Tudor Ambarus <tudor.ambarus@...aro.org>, 
	Rob Herring <robh@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>, Lee Jones <lee@...nel.org>, 
	Linus Walleij <linus.walleij@...aro.org>, Bartosz Golaszewski <brgl@...ev.pl>, 
	Peter Griffin <peter.griffin@...aro.org>, Will McVicker <willmcvicker@...gle.com>, kernel-team@...roid.com, 
	linux-kernel@...r.kernel.org, linux-samsung-soc@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-gpio@...r.kernel.org
Subject: Re: [PATCH v2 02/17] regulator: dt-bindings: add s2mpg10-pmic
 regulators

On Fri, Jun 06, 2025 at 04:02:58PM GMT, André Draszik wrote:
> The S2MPG10 PMIC is a Power Management IC for mobile applications with
> buck converters, various LDOs, power meters, RTC, clock outputs, and
> additional GPIO interfaces.
> 
> It has 10 buck and 31 LDO rails. Several of these can either be
> controlled via software or via external signals, e.g. input pins
> connected to a main processor's GPIO pins.
> 
> Add documentation related to the regulator (buck & ldo) parts like
> devicetree definitions, regulator naming patterns, and additional
> properties.
> 
> S2MPG10 is typically used as the main-PMIC together with an S2MPG11
> PMIC in a main/sub configuration, hence the datasheet and the binding
> both suffix the rails with an 'm'.
> 
> Signed-off-by: André Draszik <andre.draszik@...aro.org>
> 
> ---
> v2:
> - drop | (literal style mark) from samsung,ext-control-gpios
>   description
> ---
>  .../regulator/samsung,s2mpg10-regulator.yaml       | 147 +++++++++++++++++++++
>  MAINTAINERS                                        |   1 +
>  .../regulator/samsung,s2mpg10-regulator.h          |  48 +++++++
>  3 files changed, 196 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/regulator/samsung,s2mpg10-regulator.yaml b/Documentation/devicetree/bindings/regulator/samsung,s2mpg10-regulator.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..82f2b06205e9bdb15cf90b1e896fe52c335c52c4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/samsung,s2mpg10-regulator.yaml
> @@ -0,0 +1,147 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/regulator/samsung,s2mpg10-regulator.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Samsung S2MPG10 Power Management IC regulators
> +
> +maintainers:
> +  - André Draszik <andre.draszik@...aro.org>
> +
> +description: |
> +  This is part of the device tree bindings for the S2MG10 Power Management IC
> +  (PMIC).
> +
> +  The S2MPG10 PMIC provides 10 buck and 31 LDO regulators.
> +
> +  See also Documentation/devicetree/bindings/mfd/samsung,s2mps11.yaml for
> +  additional information and example.
> +
> +definitions:
> +  s2mpg10-ext-control:
> +    properties:
> +      samsung,ext-control:
> +        description: |
> +          These rails can be controlled via one of several possible external
> +          (hardware) signals. If so, this property configures the signal the PMIC
> +          should monitor. For S2MPG10 rails where external control is possible other
> +          than ldo20m, the following values generally corresponding to the
> +          respective on-chip pin are valid:
> +            - 0 # S2MPG10_PCTRLSEL_ON - always on
> +            - 1 # S2MPG10_PCTRLSEL_PWREN - PWREN pin
> +            - 2 # S2MPG10_PCTRLSEL_PWREN_TRG - PWREN_TRG bit in MIMICKING_CTRL
> +            - 3 # S2MPG10_PCTRLSEL_PWREN_MIF - PWREN_MIF pin
> +            - 4 # S2MPG10_PCTRLSEL_PWREN_MIF_TRG - PWREN_MIF_TRG bit in MIMICKING_CTRL
> +            - 5 # S2MPG10_PCTRLSEL_AP_ACTIVE_N - ~AP_ACTIVE_N pin
> +            - 6 # S2MPG10_PCTRLSEL_AP_ACTIVE_N_TRG - ~AP_ACTIVE_N_TRG bit in MIMICKING_CTRL
> +            - 7 # S2MPG10_PCTRLSEL_CPUCL1_EN - CPUCL1_EN pin
> +            - 8 # S2MPG10_PCTRLSEL_CPUCL1_EN2 - CPUCL1_EN & PWREN pins
> +            - 9 # S2MPG10_PCTRLSEL_CPUCL2_EN - CPUCL2_EN pin
> +            - 10 # S2MPG10_PCTRLSEL_CPUCL2_EN2 - CPUCL2_E2 & PWREN pins
> +            - 11 # S2MPG10_PCTRLSEL_TPU_EN - TPU_EN pin
> +            - 12 # S2MPG10_PCTRLSEL_TPU_EN2 - TPU_EN & ~AP_ACTIVE_N pins
> +            - 13 # S2MPG10_PCTRLSEL_TCXO_ON - TCXO_ON pin
> +            - 14 # S2MPG10_PCTRLSEL_TCXO_ON2 - TCXO_ON & ~AP_ACTIVE_N pins
> +
> +          For S2MPG10 ldo20m, the following values are valid
> +            - 0 # S2MPG10_PCTRLSEL_LDO20M_ON - always on

No, use standard regulator properties - regulator-always-on


> +            - 1 # S2MPG10_PCTRLSEL_LDO20M_EN_SFR - VLDO20M_EN & LDO20M_SFR
> +            - 2 # S2MPG10_PCTRLSEL_LDO20M_EN - VLDO20M_EN pin
> +            - 3 # S2MPG10_PCTRLSEL_LDO20M_SFR - LDO20M_SFR in LDO_CTRL1 register
> +            - 4 # S2MPG10_PCTRLSEL_LDO20M_OFF - disable

I don't think we allowed such property in the past. I don't get what is
here the actual signal - you described registers in multiple places, not
signals. Few of these duplicate standard properties, so this looks like
exact copy of downstream which was doing exactly that way and that was
exactly never upstreamed.


> +
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        minimum: 0
> +        maximum: 14
> +
> +      samsung,ext-control-gpios:

enable-gpios


> +        description:
> +          For rails where external control is done via a GPIO, this optional
> +          property describes the GPIO line used.
> +
> +        maxItems: 1

And this can be dropped.

Best regards,
Krzysztof