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
