Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Geert,

On Mon, Jun 12, 2023 at 03:08:46PM +0200, Geert Uytterhoeven wrote:
> On Mon, Jun 12, 2023 at 2:54 PM Laurent Pinchart wrote:
> > On Mon, Jun 12, 2023 at 12:42:33PM +0000, Biju Das wrote:
> > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
> > > > On Mon, Jun 12, 2023 at 09:53:02AM +0000, Biju Das wrote:
> > > > > How do we proceed here between [1] and [2]?
> > > > >
> > > > > DT-Maintainers suggestion:
> > > > > [1]
> > > > > raa215300: pmic@12 {
> > > > >   compatible = "renesas,raa215300";
> > > > >   reg = <0x12>, <0x6f>;
> > > > >   reg-names = "main", "rtc";
> > > > >
> > > > >   clocks = <&x2>;
> > > > >   clock-names = "xin";
> > > > >   /* Add Optional shared IRQ resource and share it to child and handle
> > > > > it both in parent and child */ };
> > > > >
> > > > > Laurent/Wolfram suggestion to split it into two nodes and get rid of this patch:
> > > > > [2]
> > > > >   raa215300: pmic @12 {
> > > > >           compatible = "renesas,raa215300";
> > > > >           reg = <0x12>;
> > > > >
> > > > >           /* Add Optional shared IRQ */
> > > > >           renesas,raa215300-rtc = <&rtc_raa215300>; /* Parse the handle and Enable RTC , if present.*/
> > > > >   };
> > > > >
> > > > >   rtc_raa215300: rtc@6f {
> > > > >           compatible = "renesas,raa215300-isl1208";
> > > >
> > > > Make this
> > > >
> > > >             compatible = "renesas,raa215300-isl1208", "isil,isl1208";
> > > >
> > > > >           reg = <0x6f>;
> > > > >
> > > > >           /* Add Optional shared IRQ */
> > > > >           clocks = <&x2>;
> > > > >           clock-names = "xin";
> > > > >           renesas,raa215300-pmic = <&pmic>; /* Parse the handle to get PMIC
> > > > > version to check Oscillator bit is inverted or not */
> > > >
> > > > This isn't nice. I would instead add a renesas,invert-xtoscb boolean
> > > > property. If you don't want different DT sources for different revisions
> > > > of the PMIC,
> > >
> > > I need to support all PMIC versions with same image, as PMIC is just a component on the
> > > SoM module. So SoM's have different PMIC versions.
> >
> > I understand it's not convenient, so let's try to find a good solution.
> >
> > > > one option is to perform the auto-detection in the boot
> > > > loader and update the DT dynamically there.
> > >
> > > Yes, this is an option. Bootloader updates "renesas,invert-xtoscb" property based
> > > on PMIC version.
> > >
> > > Not sure, From binding perspective, Documenting "renesas,invert-xtoscb" is OK for
> > > the relevant maintainers??
> >
> > It's fine with me at least :-) I think a property makes sense, as it
> > describes the device. Updating the device tree in the boot loader based
> > on auto-detection of features is also fairly common (to set the amount
> > of DRAM for instance).
> >
> > What I'm not entirely sure about in this case is if a property would be
> > the best option, or two different compatible strings. I'll let the
> > appropriate maintainer recommend one of those two options. In either
> > case, the boot loader would be responsible for updating the DT.
> 
> Indeed. DT binding best practices 101: do not use properties to
> distinguish, use compatible values instead.
> 
> And don't use different compatible values if you can distinguish using
> a version register.  Unfortunately the version register is part of the
> main/first device (the PMIC), so the RTC cannot find out easily...

That's not very different from having IP cores whose integration is
different between different SoC versions. We could easily add SoC match
code in drivers and map the SoC version to integration data, but it's
not a good practice. DT helps decoupling integration (and quirks) from
drivers and allows getting rid of lots of cross-driver communication
(which used to be handled through board files).

> So basically you have an i2c mfd.  The Linux mfd subsystem is tailored
> for platform devices, so it's not a good match.  The closest we have
> in i2c is the ancillary device...

I think an MFD-type solution that's way too much trouble to handle the
issue at hand. I recommend a DT property here.

-- 
Regards,

Laurent Pinchart