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

[Biju Das <biju.das.jz@bp.renesas.com>]

Hi Laurent,

> Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
> 
> On Wed, Jun 14, 2023 at 11:30:48AM +0000, Biju Das wrote:
> > > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device
> > > API On Wed, Jun 14, 2023 at 08:21:38AM +0000, Biju Das wrote:
> >> > > Subject: Re: [PATCH v5 01/11] i2c: Enhance
> >> > > i2c_new_ancillary_device API
> > > > > On Tue, Jun 13, 2023 at 07:31:46PM +0000, Biju Das wrote:
> > > > > > > Subject: RE: [PATCH v5 01/11] i2c: Enhance
> > > > > > > i2c_new_ancillary_device API
> > > > > > > > Subject: RE: [PATCH v5 01/11] i2c: Enhance
> > > > > > > > i2c_new_ancillary_device API
> > > > > > > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance
> > > > > > > > > i2c_new_ancillary_device API
> > > > > > > > >
> > > > > > > > > Hi everyone,
> > > > > > > > >
> > > > > > > > > > Perhaps we should first think through what an
> > > > > > > > > > ancillary device really is.  My understanding is that
> > > > > > > > > > it is used to talk to secondary addresses of a multi-
> address I2C slave device.
> > > > > > > > >
> > > > > > > > > As I mentioned somewhere before, this is not the case.
> > > > > > > > > Ancillary devices are when one *driver* handles more
> than one address.
> > > > > > > > > Everything else has been handled differently in the past
> > > > > > > > > (for all the uses I am aware of).
> > > > > > > > >
> > > > > > > > > Yet, I have another idea which is so simple that I
> > > > > > > > > wonder if it maybe has already been discussed so far?
> > > > > > > > >
> > > > > > > > > * have two regs in the bindings
> > > > > > > >
> > > > > > > > OK, it is inline with DT maintainers expectation as it is
> > > > > > > > matching with real hw as single device node having two
> regs.
> > > > > > > >
> > > > > > > > > * use the second reg with i2c_new_client_device to
> instantiate the
> > > > > > > > >   RTC sibling. 'struct i2c_board_info', which is one
> parameter, should
> > > > > > > > >   have enough options to pass data, e.g it has a
> software_node.
> > > > > > > >
> > > > > > > > OK, I can see the below can be passed from PMIC to new
> client device.
> > > > > > > >
> > > > > > > > 	client->addr = info->addr;
> > > > > > > >
> > > > > > > > 	client->init_irq = info->irq;
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Should work or did I miss something here?
> > > > > > > >
> > > > > > > > I guess it will work. We instantiate appropriate device
> > > > > > > > based On PMIC revision and slave address and IRQ resource
> > > > > > > > passed through 'struct i2c_board_info'
> > > > > > > >
> > > > > > > > Will check this and update you.
> > > > > > >
> > > > > > > info.irq = irq; -->Irq fine
> > > > > > > info.addr = addr; -->slave address fine size =
> > > > > > > strscpy(info.type, name, sizeof(info.type));
> > > > > > > -->instantiation based on PMIC version fine.
> > > > > > >
> > > > > > > 1) How do we share clk details on instantiated device to
> > > > > > > find is it connected to external crystal or external clock
> > > > > > > source? as we cannot pass of_node between PMIC and
> > > > > > > "i2c_board_info" as it results in pinctrl failure.
> > > > > > > info->platformdata and
> > > > > > > Client->dev.platformdata to retrieve this info??
> > > > > >
> > > > > > Or
> > > > > >
> > > > > > I2C instantiation based on actual oscillator bit value, ie,
> > > > > > two i2c_device_id's with one for setting oscillator bit and
> > > > > > another for clearing oscillator bit
> > > > > >
> > > > > > PMIC driver parses the clock details. Based on firmware
> > > > > > version and clock, It instantiates either i2c_device_id with
> > > > > > setting oscillator bit or clearing oscillator bit.
> > > > >
> > > > > I don't like that hack. I still think that two DT nodes is the
> > > > > best option, I think you're trying hard to hack around a problem
> > > > > that is actually not a problem.
> > > >
> > > > Why do you think it is a hack? I believe rather it is actual
> > > > solution
> > > >
> > > > PMIC is a single device, with 2 regs, clocks, pinctrl and IRQ
> properties.
> > > > So it will be represented as single node with single compatible.
> > >
> > > The chip is a single package that contains two independent devices.
> > > This is not different than bundling many IP cores in an SoC, we have
> > > one DT node per IP core, not a single DT node for the SoC. The fact
> > > that we're dealing with an external physical component here isn't
> relevant.
> >
> > DT maintainer's new requirement is a single device node for a device.
> 
> That's the default rule, I haven't seen a clear statement that it has to
> apply to 100% of the cases.
> 
> Regardless, in this case there are two devices inside a package, so
> having two DT nodes doesn't break the rule.
> 
> > If a device supports more functionalities just instantiate and bind
> it.
> >
> > I already gone through mainlining MTU3a device, with 3 separate dt
> > nodes and finally ends up in single device node instantiating
> PWM/Counter/Timer nodes.
> >
> > Here also I started with 2 device nodes, and ends up in single device
> > node as it is a single device.
> >
> > I think from dt point it is correct to have single device node for a
> > device. even though device contains PMIC and RTC as separate
> > functionality With shared resources like IRQ, PINS and Clocks as at
> > the PMIC device is the one exposes to this to outside world.
> >
> > > > By instating a client device, we are sharing the relevant
> > > > resources to RTC device driver.
> > >
> > > By instantiating a client device, you create a second struct device,
> > > which is the kernel abstraction of a hardware device. This shows in
> > > my opinion that we're dealing with two devices here, hence my
> > > recommendation of using two DT nodes.
> >
> > Two DT nodes is the problem. DT maintainer's don't like it, for them
> > it is just one device which provides PMIC/RTC functionality.
> 
> Have they followed this discussion ?
> 
> > > As you've noticed, with two devices and a single DT node, pinctrl
> > > complains. You can hack around that by moving the pinctrl
> > > configuration from the PMIC DT node to another DT node, and that's
> one first hack.
> >
> > PMIC device expose pins and it binds the pins during probe. Since it
> > is a single device, we don't need to share this to RTC device. We just
> > need to add pinctrl definitions in PMIC device node. This is not a
> hack.
> >
> > > Then, you'll need to have two different device IDs depending on the
> > > PMIC version to let the RTC driver set the oscillator bit correctly,
> > > and that's a second hack.
> >
> > PMIC has all the information, so it can instantiate and bind with the
> > configuration needed for second device. So it is not a hack.
> >
> > > A solution with two DT nodes models the hardware better and is
> cleaner.
> >
> > But looks like all other people are happy with single DT node.
> 
> At the end of the day, it's not my driver, and not my subsystems, so
> I'll let you make mistakes if you're happy with them. I still strongly
> think it's a mistake, but I can't get everybody to do things right, can
> I ? :-)

As Wolfram suggesting to use "i2c_new_client_device" and DT maintainer's
are not responding to having 2 device node solution, I am going to stick with single device node solution as it is ok for DT maintainers.

Please let me know if anyone think otherwise.

Cheers,
Biju