Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
In-Reply-To: <Yuz2O+lZ5W7RviuA@google.com>
References: <Yr6oLlmfWRkiAZG7@google.com> <Yr66ZZqEnBApHYMA@google.com>
 <0481d3cc-4bb9-4969-0232-76ba57ff260d@quicinc.com> <YsLhxx+L3+GJDRyO@google.com>
 <bcc5f059-b991-296a-bba6-9cb1236097f2@quicinc.com> <Ys1tYAO39LKzEAOE@google.com>
 <dc737abb-041b-491a-14f1-a584f9e64a3d@quicinc.com> <CAE-0n528QaTtZFp=WAaHShegFRpKVN_67jQfUJTtsRPr6s--zA@mail.gmail.com>
 <52039cd1-4390-7abb-d296-0eb7ac0c3b15@quicinc.com> <Yuz2O+lZ5W7RviuA@google.com>
From:   Stephen Boyd <swboyd@chromium.org>
User-Agent: alot/0.10
Date:   Mon, 8 Aug 2022 14:09:15 -0500
Message-ID: <CAE-0n507SLeYB7XVzGFk=RO6YjOPoGpux+_N2AyrmL354mQJ-g@mail.gmail.com>
Subject: Re: [PATCH V15 6/9] mfd: pm8008: Use i2c_new_dummy_device() API
To:     Lee Jones <lee.jones@linaro.org>,
        Satya Priya Kakitapalli <quic_c_skakit@quicinc.com>
Cc:     Mark Brown <broonie@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Rob Herring <robh+dt@kernel.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,
        Lee Jones <lee@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Quoting Lee Jones (2022-08-05 03:51:39)
> On Tue, 02 Aug 2022, Satya Priya Kakitapalli (Temp) wrote:
>
> >
> > On 7/27/2022 6:49 AM, Stephen Boyd wrote:
> > > Quoting Satya Priya Kakitapalli (Temp) (2022-07-21 23:31:16)
> > > >   =C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 regulato=
r-name =3D "pm8008_l6";
> > > >   =C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0 };
> > > >
> > > >   =C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0 pm8008_l7: ldo7@4600 {
> > > >   =C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 reg =3D =
<0x4600>;
> > > >   =C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 regulato=
r-name =3D "pm8008_l7";
> > > >   =C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0=C2=A0 };
> > > >   =C2=A0=C2=A0 =C2=A0};
> > > > };
> > > >
> > > >
> > > > Stephen/Mark, Please do let me know if you are OK with this design.
> > > >
> > > I was happy with the previous version of the DT node. That had one no=
de
> > > for the "pm8008 chip", which is important because it really is one
> > > package. Why isn't that possible to implement and also register i2c
> > > devices on the i2c bus for the second address?
>
> If devices have different addresses, they should have their own nodes, no=
?

There are nodes for the devices at different addresses in the design we
had settled on earlier.

        pm8008: pmic@8 {
                compatible =3D "qcom,pm8008";
                reg =3D <0x8>;
                #address-cells =3D <2>;
                #size-cells =3D <0>;
                #interrupt-cells =3D <2>;

                pm8008_l1: ldo1@1,4000 {
                        compatible =3D "qcom,pm8008-regulator";
                        reg =3D <0x1 0x4000>;
                        regulator-name =3D "pm8008_ldo1";
                };

		...

	};

pmic@8 is the i2c device at i2c address 8. ldo1@1,4000 is the i2c device
at address 9 (8 + 1) with control for ldo1 at register offset 0x4000.

I think your concern is more about the fact that the regulator sub-nodes
are platform device drivers instead of i2c device drivers. I'm not an
i2c expert but from what I can tell we only describe one i2c address in
DT and then do something like this to describe the other i2c addresses
when one physical chip responds to multiple addresses on the i2c bus.
See i2c_new_dummy_device() and i2c_new_ancillary_device() kerneldoc for
slightly more background.

It may need some modifications to the i2c core to make the regulator
nodes into i2c devices. I suspect the qcom,pm8008 i2c driver needs to
look at the 'reg' property and translate that to the parent node's reg
property (8) plus the first cell (1) to determine the i2c device's final
i2c address. Then the i2c core needs to register i2c devices that are
bound to the lifetime of the "primary" i2c device (pmic@8). The driver
for the regulator can parse the second cell of the reg property to
determine the register offset within that i2c address to use to control
the regulator. That would make it possible to create an i2c device for
each regulator node, but I don't think that is correct because the
second reg property isn't an i2c address, it's a register offset inside
the i2c address.

It sort of looks like we need to use i2c_new_ancillary_device() here. IF
we did that the DT would look like this:

        pm8008: pmic@8 {
                compatible =3D "qcom,pm8008";
                reg =3D <0x8>, <0x9>;
		reg-names =3D "core", "regulators";
                #address-cells =3D <2>;
                #size-cells =3D <0>;
                #interrupt-cells =3D <2>;

                pm8008_l1: ldo1@1,4000 {
                        compatible =3D "qcom,pm8008-regulator";
                        reg =3D <0x1 0x4000>;
                        regulator-name =3D "pm8008_ldo1";
                };

		...

	};

And a dummy i2c device would be created for i2c address 0x9 bound to the
dummy i2c driver when we called i2c_new_ancillary_device() with
"regulators" for the name. The binding of the dummy driver is preventing
us from binding another i2c driver to the i2c address. Why can't we call
i2c_new_client_device() but avoid binding a dummy driver to it like
i2c_new_ancillary_device() does? If that can be done, then the single
i2c device at 0x9 can be a pm8008-regulators (plural) device that probes
a single i2c driver that parses the subnodes looking for regulator
nodes.

Note: There is really one i2c device at address 0x9, that corresponds to
the regulators, but in DT we need to have one node per regulator so we
can configure constraints.