Re: [PATCH v2 02/17] regulator: dt-bindings: add s2mpg10-pmic regulators

["André Draszik" <andre.draszik@linaro.org>]

Hi Krzysztof,

Thanks for your review!

On Wed, 2025-06-11 at 10:55 +0200, Krzysztof Kozlowski wrote:
> 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@linaro.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@linaro.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

Same reason as in patch 3. I'll update this one.

> 
> > +            - 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.

For lack of a better word, I used the generic term 'signal' in the
descriptions to refer to either a pin (VLDO20M_EN in this case), or
a PMIC-internal register bit (LDO20M_SFR in this case), or a
combination, they're all signals to the internal PMIC logic. That's
what the property description above is trying to convey.

The 'samsung,ext-control' property itself is required to configure the
PMIC so that it knows which of the signals (e.g. which input pin)
should affect a certain rail. In effect, this allows controlling more
than one rail atomically, while being flexible in the board design wrt.
which rail is affected by which pin.

This is not a copy of downstream, and the ones that in effect duplicate
standard properties like 'always on' I simply added for completeness,
to fully describe the hardware. I'll leave them out in the next revision
to avoid possible abuse.

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

OK.

> 
> 
> > +        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.

OK.

Cheers,
Andre'