Re: [PATCH v2 03/17] regulator: dt-bindings: add s2mpg11-pmic regulators

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

Hi Krzysztof,

Thanks for your review!

On Wed, 2025-06-11 at 10:57 +0200, Krzysztof Kozlowski wrote:
> On Fri, Jun 06, 2025 at 04:02:59PM GMT, André Draszik wrote:
> > The S2MPG11 PMIC is a Power Management IC for mobile applications with
> > buck converters, various LDOs, power meters, and additional GPIO
> > interfaces. It typically complements an S2MPG10 PMIC in a main/sub
> > configuration as the sub-PMIC.
> > 
> > S2MPG11 has 12 buck, 1 buck-boost, and 15 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.
> > 
> > Since S2MPG11 is typically used as the sub-PMIC together with an
> > S2MPG10 as the main-PMIC, the datasheet and the binding both suffix the
> > rails with an 's'.
> > 
> > Signed-off-by: André Draszik <andre.draszik@linaro.org>
> > 
> > ---
> > Note: checkpatch suggests to update MAINTAINERS, but the new file is
> > covered already due to using a wildcard.
> > 
> > v2:
> > - fix commit message typos: s2mp1 -> s2mpg1
> > - mention GPIOs in commit message
> > ---
> >  .../regulator/samsung,s2mpg11-regulator.yaml       | 150 +++++++++++++++++++++
> >  .../regulator/samsung,s2mpg10-regulator.h          |  18 +++
> >  2 files changed, 168 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/regulator/samsung,s2mpg11-regulator.yaml
> > b/Documentation/devicetree/bindings/regulator/samsung,s2mpg11-regulator.yaml
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..f2d596642501c197e2911ee3b9caac189cf541a4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/regulator/samsung,s2mpg11-regulator.yaml
> > @@ -0,0 +1,150 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/regulator/samsung,s2mpg11-regulator.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Samsung S2MPG11 Power Management IC regulators
> > +
> > +maintainers:
> > +  - André Draszik <andre.draszik@linaro.org>
> > +
> > +description: |
> > +  This is part of the device tree bindings for the S2MG11 Power Management IC
> > +  (PMIC).
> > +
> > +  The S2MPG11 PMIC provides 12 buck, 1 buck-boost, and 15 LDO regulators.
> > +
> > +  See also Documentation/devicetree/bindings/mfd/samsung,s2mps11.yaml for
> > +  additional information and example.
> > +
> > +definitions:
> > +  s2mpg11-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. The following values generally corresponding to the
> > +          respective on-chip pin are valid:
> > +            - 0 # S2MPG11_PCTRLSEL_ON - always on
> 
> Use regulator-always-on

Yes, the end-result would be the same. I still added this one for
completeness, because they all describe the hardware. I can leave
this one out and thereby force use of the regulator-always-on
property instead.

> 
> > +            - 1 # S2MPG11_PCTRLSEL_PWREN - PWREN pin
> 
> That's duplicating regulator in standby properties.

OK, I'll double-check this.

> 
> > +            - 2 # S2MPG11_PCTRLSEL_PWREN_TRG - PWREN_TRG bit in MIMICKING_CTRL
> > +            - 3 # S2MPG11_PCTRLSEL_PWREN_MIF - PWREN_MIF pin
> > +            - 4 # S2MPG11_PCTRLSEL_PWREN_MIF_TRG - PWREN_MIF_TRG bit in MIMICKING_CTRL
> > +            - 5 # S2MPG11_PCTRLSEL_AP_ACTIVE_N - ~AP_ACTIVE_N pin
> > +            - 6 # S2MPG11_PCTRLSEL_AP_ACTIVE_N_TRG - ~AP_ACTIVE_N_TRG bit in MIMICKING_CTRL
> > +            - 7 # S2MPG11_PCTRLSEL_G3D_EN - G3D_EN pin
> > +            - 8 # S2MPG11_PCTRLSEL_G3D_EN2 - G3D_EN & ~AP_ACTIVE_N pins
> > +            - 9 # S2MPG11_PCTRLSEL_AOC_VDD - AOC_VDD pin
> > +            - 10 # S2MPG11_PCTRLSEL_AOC_RET - AOC_RET pin
> > +            - 11 # S2MPG11_PCTRLSEL_UFS_EN - UFS_EN pin
> 
> Now I have doubts these are real signals. Are you saying that S2MPG11
> has a pin named UFS_EN (such pin on ballmap)?

Yes. I used the generic term 'external signal' in the descriptions above
rather than the more specific pin, because some of these are bits in
some registers, and most are indeed input pins, or a combination. Not all
of the pins are connected to actual GPIOs on the AP, though.

> 
> > +            - 12 # S2MPG11_PCTRLSEL_LDO13S_EN - VLDO13S_EN pin
> > +
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +        minimum: 0
> > +        maximum: 12
> > +
> > +      samsung,ext-control-gpios:
> 
> Same comments as previous patch.

I used this one to align with s2mps14, but will switch to enable-gpios.

Cheers,
Andre'