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

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

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 ? :-)

-- 
Regards,

Laurent Pinchart