Re: [PATCH V10 7/9] regulator: Add a regulator driver for the PM8008 PMIC

From: Stephen Boyd <swboyd@chromium.org>
To: Mark Brown <broonie@kernel.org>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>,
	Rob Herring <robh+dt@kernel.org>,
	Satya Priya <quic_c_skakit@quicinc.com>,
	Lee Jones <lee.jones@linaro.org>,
	Liam Girdwood <lgirdwood@gmail.com>,
	linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, quic_collinsd@quicinc.com,
	quic_subbaram@quicinc.com, quic_jprakash@quicinc.com
Subject: Re: [PATCH V10 7/9] regulator: Add a regulator driver for the PM8008 PMIC
Date: Tue, 19 Apr 2022 08:45:47 -0700	[thread overview]
Message-ID: <CAE-0n53zWdrT7S6MKM_aktnj=AwjUKH0XKySwSkfkX8vTv2w9w@mail.gmail.com> (raw)
In-Reply-To: <Yl7Nb0mNjt7kV3uV@sirena.org.uk>

Quoting Mark Brown (2022-04-19 07:55:43)
> On Thu, Apr 14, 2022 at 05:25:49PM -0700, Stephen Boyd wrote:
> > Quoting Satya Priya (2022-04-14 05:30:16)
>
> > > +static struct platform_driver pm8008_regulator_driver = {
> > > +       .driver = {
> > > +               .name           = "qcom-pm8008-regulator",
>
> > I'd prefer to use an of_device_id table here. That would let us populate
> > a "qcom,pm8008-regulators" node that had the ldo nodes as children and
> > avoid mfd cells.
>
> That's encoding the current Linux way of splitting up drivers into the
> DT rather than describing the hardware.

The DT binding has a subnode of the pm8008@8 node for 'regulators'.
There's also a subnode for gpios (see qcom,pm8008-gpio). The gpio node
has a reg property, so I'm confused how we can even have the regulators
container node at the same level as the gpio node with a reg property.
Every node that's a child of pm8008@8 either needs to have a reg
property or not.

What benefit does it have to not describe secondary i2c addresses as
nodes in DT? I think it's necessary because the reset gpio needs to be
deasserted before i2c read/write to either address, 8 or 9, will work.
Otherwise I don't understand. Having the reset puts us into a small bind
though because child nodes sometimes have a reg property and other times
don't.

This is the current example

	i2c {
	  pm8008@8 {
	    compatible = "qcom,pm8008";
	    #address-cells = <1>;
	    #size-cells = <0>;
	    reset-gpios = <&tlmm 23 GPIO_ACTIVE_HIGH>;
	    gpios {
	      compatible = "qcom,pm8008-gpio", "qcom,spmi-gpio";
	      reg = <0xc000>;
	      ...

	    };

	    regulators {
	      vdd_l1_l2-supply = <&vreg_s8b_1p2>;

	      ldo1 {
	        regulator-name = "pm8008_l1";
	      };
	      ldo2 {
	        regulator-name = "pm8008_l2";
	      };
	    };
	  };
	};

What should the final result be? Dropping the regulators node would end
up with the same problem where ldo1 has no reg property. Adding a reg
property to ldo1 might work, but it would be a register offset inside
i2c address 9 while the binding makes it look like a register offset
within 9. The binding for the container node could get two address
cells, so that the first cell describes the i2c address offset?

	i2c {
	  pm8008@8 {
	    compatible = "qcom,pm8008";
	    #address-cells = <2>;
	    #size-cells = <0>;
	    reset-gpios = <&tlmm 23 GPIO_ACTIVE_HIGH>;

	    vdd_l1_l2-supply = <&vreg_s8b_1p2>;

	    gpios@0,c000 {
	      compatible = "qcom,pm8008-gpio", "qcom,spmi-gpio";
	      reg = <0x0 0xc000>;
	      ...

	    };

	    ldo1@1,30 {
	      compatible = "qcom,pm8008-regulator";
	      reg = <0x1 0x30>;
	      regulator-name = "pm8008_l1";
	    };
	    ldo2@1,40 {
	      compatible = "qcom,pm8008-regulator";
	      reg = <0x1 0x40>;
	      regulator-name = "pm8008_l2";
	    };
	  };
	};

We don't make a node for each gpio so I don't know why we would make a
node for each regulator. The above could be changed to have the
regulators node and a reg property like this

	i2c {
	  pm8008@8 {
	    compatible = "qcom,pm8008";
	    #address-cells = <2>;
	    #size-cells = <0>;
	    reset-gpios = <&tlmm 23 GPIO_ACTIVE_HIGH>;

	    gpios@0,c000 {
	      compatible = "qcom,pm8008-gpio", "qcom,spmi-gpio";
	      reg = <0x0 0xc000>;
	      ...

	    };

	    regulators@1,0 {
	      compatible = "qcom,pm8008-regulators";
	      vdd_l1_l2-supply = <&vreg_s8b_1p2>;

	      reg = <0x1 0x0>;
	      ldo1 {
	      regulator-name = "pm8008_l1";
	      };
	      ldo2 {
	        regulator-name = "pm8008_l2";
	      };
	    };
	  };
	};

I wonder if there's a mapping table property like i2c-ranges or
something like that which could be used to map the i2c dummy to the
first reg property. That would be super awesome so that when the
platform bus is populated we could match up the regmap for the i2c
device to the platform device automatically.

By the way, Is there any document on "best practices" for i2c devicetree
bindings?  We should add details to the document to describe this
situation so this can be conveyed faster next time.