Re: [PATCH V15 6/9] mfd: pm8008: Use i2c_new_dummy_device() API

From: Lee Jones <lee@kernel.org>
To: Stephen Boyd <swboyd@chromium.org>
Cc: Lee Jones <lee.jones@linaro.org>,
	Satya Priya Kakitapalli <quic_c_skakit@quicinc.com>,
	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
Subject: Re: [PATCH V15 6/9] mfd: pm8008: Use i2c_new_dummy_device() API
Date: Wed, 28 Sep 2022 11:20:30 +0100	[thread overview]
Message-ID: <YzQf7hf15vvLeGse@google.com> (raw)
In-Reply-To: <CAE-0n507SLeYB7XVzGFk=RO6YjOPoGpux+_N2AyrmL354mQJ-g@mail.gmail.com>

On Mon, 08 Aug 2022, Stephen Boyd wrote:

> 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)
> > > > >               regulator-name = "pm8008_l6";
> > > > >           };
> > > > >
> > > > >           pm8008_l7: ldo7@4600 {
> > > > >               reg = <0x4600>;
> > > > >               regulator-name = "pm8008_l7";
> > > > >           };
> > > > >       };
> > > > > };
> > > > >
> > > > >
> > > > > 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 node
> > > > 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 = "qcom,pm8008";
>                 reg = <0x8>;
>                 #address-cells = <2>;
>                 #size-cells = <0>;
>                 #interrupt-cells = <2>;
> 
>                 pm8008_l1: ldo1@1,4000 {
>                         compatible = "qcom,pm8008-regulator";
>                         reg = <0x1 0x4000>;
>                         regulator-name = "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 = "qcom,pm8008";
>                 reg = <0x8>, <0x9>;
> 		reg-names = "core", "regulators";
>                 #address-cells = <2>;
>                 #size-cells = <0>;
>                 #interrupt-cells = <2>;
> 
>                 pm8008_l1: ldo1@1,4000 {
>                         compatible = "qcom,pm8008-regulator";
>                         reg = <0x1 0x4000>;
>                         regulator-name = "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.

Wouldn't it make more sense to simply separate the instantiation of
the 2 I2C devices?  Similar to what you suggested [0] in v9.  That way
they can handle their own resources and we can avoid all of the I2C
dummy / shared Regmap passing faff.

[0] https://lore.kernel.org/all/CAE-0n53G-atsuwqcgNvi3nvWyiO3P=pSj5zDUMYj0ELVYJE54Q@mail.gmail.com/

-- 
Lee Jones [李琼斯]