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

[Satya Priya Kakitapalli (Temp)]

Hi Lee,


Could you please share your thoughts on this?


On 8/9/2022 12:39 AM, Stephen Boyd wrote:
> Quoting Lee Jones (2022-08-05 03:51:39)
>
> > > > 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.