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

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3578 bytes --]

On Tue, Apr 19, 2022 at 08:45:47AM -0700, Stephen Boyd wrote:
> Quoting Mark Brown (2022-04-19 07:55:43)

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

That seems unfortunate if it's a limitation of DT :/

> 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

This is a platform device, not an I2C device.  Converting it to an I2C
device and describing the second I2C address as a separate device would
be much less of a confusion here (and I suspect may well reflect the
underlying physical implementation).  I'm mostly concerned about these
platform devices.

The other option would be to describe each regulator the device supports
as a separate IP which does have some hope of being reusable since it
reflects the actual IPs here.

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

Right, that's an issue and does tie the two I2C addresses of the device
into a single thing.

> 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?

Yeah, I think we want reg properties on the LDOs.  

> 	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 {

This looks sensible to me.

> 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

GPIOs tend to be one IP block with one set of registers that provides a
bank of GPIOs.  Regulators tend to be a separate IP block for each
individual regulator, typically repeated multiple times within a single
chip.  A binding that describes the regulators within the device should
generally be describing these individual regulator IPs rather than just
saying "there are some regulators" which doesn't add any information or
promote reuse with other PMICs sharing the same IP.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]