RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Laurent, > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API > > On Wed, Jun 14, 2023 at 11:30:48AM +, 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 +, 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 +, 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, > >
RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi All, > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API > > > > > Without of_node, devm_clk_get() and friends falls back to registered > > > clkdevs. So you could call clk_register_clkdev() from within the > > > PMIC driver, and can keep on using devm_clk_get_optional() in the > > > ISL1208 driver. > > > > Seriously, how many hacks are we piling ? :-) > > For this particular case, why do you consider this a hack? I previously > suggested the solution that the PMIC driver exposes a clock to be consumed > for the RTC driver even for the "two DT node solution". Because it then > avoids a custom property with a phandle to the other node with regular DT > clock bindings. I'd think we need sth like that in any case. OK, Will use clk_register_clkdev() in PMIC driver, so that there is no code change needed in RTC driver for clk handling. Cheers, Biju
Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
> > Without of_node, devm_clk_get() and friends falls back to registered > > clkdevs. So you could call clk_register_clkdev() from within the > > PMIC driver, and can keep on using devm_clk_get_optional() in the > > ISL1208 driver. > > Seriously, how many hacks are we piling ? :-) For this particular case, why do you consider this a hack? I previously suggested the solution that the PMIC driver exposes a clock to be consumed for the RTC driver even for the "two DT node solution". Because it then avoids a custom property with a phandle to the other node with regular DT clock bindings. I'd think we need sth like that in any case. signature.asc Description: PGP signature
Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
On Wed, Jun 14, 2023 at 11:30:48AM +, 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 +, 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 +, 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
Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
On Thu, Jun 15, 2023 at 10:07:39AM +0200, Geert Uytterhoeven wrote: > On Wed, Jun 14, 2023 at 9:53 AM Geert Uytterhoeven > wrote: > > On Tue, Jun 13, 2023 at 6:11 PM Biju Das wrote: > > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API > > > > On Tue, Jun 13, 2023 at 12:45 PM Biju Das > > > > wrote: > > > > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device > > > > > > API On Mon, Jun 12, 2023 at 10:43 PM Wolfram Sang > > > > > > wrote: > > > > > > > > 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 > > > > > > > * 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. > > > > > > > > > > > > > > Should work or did I miss something here? > > > > > > > > > > > > That should work, mostly (i2c_new_dummy_device() also calls > > > > > > i2c_new_client_device()). And as i2c_board_info has an of_node > > > > > > member (something I had missed before!), the new I2C device can > > > > > > access the clocks in the DT node using the standard way. > > > > > > > > > > Looks like, I cannot assign of_node member like below as it results in > > > > > pinctrl failure[1] during device bind. > > > > > > > > > > info.of_node = client->dev.of_node; > > > > > > > > > > [1] > > > > > pinctrl-rzg2l 1103.pinctrl: pin P43_0 already requested by 3-0012; > > > > > cannot claim for 3-006f pinctrl-rzg2l 1103.pinctrl: pin-344 > > > > > (3-006f) status -22 pinctrl-rzg2l 1103.pinctrl: could not request > > > > > pin 344 (P43_0) from group pmic on device pinctrl-rzg2l > > > > > raa215300 3-006f: Error applying setting, reverse things back > > > > > > > > Where do you have a reference to pin P43_0 in your DT? > > > > > > The reference to pin P43_0 is added in the PMIC node. > > > > > > I have done modification on my board to test PMIC INT# on RZ/G2L SMARC EVK > > > by wiring R83 on SoM module and PMOD0 PIN7. > > > > > > > The last versions you posted did not have any pinctrl properties? > > > > > > By default, PMIC_INT# is not populated RZ/G2L SMARC EVK, so I haven't > > > added > > > Support for PMIC_INT# for the patches posted till date. > > > > > > Yesterday I checked with HW people, is there a way to enable PMIC_INT# > > > and they told me to do the above HW modification. > > > > > > Today I found this issue, with this modified HW and PMIC INT# enabled on > > > the DT, > > > while assigning of_node of PMIC with info.of_node. It is just a > > > coincidence. > > > > IC. > > > > So you now have two Linux devices pointing to the same DT node, > > causing pinctrl issues... > > So don't set info.of_node? ;-) > > Without of_node, devm_clk_get() and friends falls back to registered > clkdevs. So you could call clk_register_clkdev() from within the > PMIC driver, and can keep on using devm_clk_get_optional() in the > ISL1208 driver. Seriously, how many hacks are we piling ? :-) > If that fails, there's also software_node.properties, or even the good > old platform_data... -- Regards, Laurent Pinchart
Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Biju, On Wed, Jun 14, 2023 at 1:04 PM Biju Das wrote: > > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API > > On Wed, Jun 14, 2023 at 10:21 AM 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 +, 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 clearin
Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Biju,
On Wed, Jun 14, 2023 at 9:53 AM Geert Uytterhoeven wrote:
> On Tue, Jun 13, 2023 at 6:11 PM Biju Das wrote:
> > > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
> > > On Tue, Jun 13, 2023 at 12:45 PM Biju Das
> > > wrote:
> > > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device
> > > > > API On Mon, Jun 12, 2023 at 10:43 PM Wolfram Sang
> > > wrote:
> > > > > > > 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
> > > > > > * 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.
> > > > > >
> > > > > > Should work or did I miss something here?
> > > > >
> > > > > That should work, mostly (i2c_new_dummy_device() also calls
> > > > > i2c_new_client_device()). And as i2c_board_info has an of_node
> > > > > member (something I had missed before!), the new I2C device can
> > > > > access the clocks in the DT node using the standard way.
> > > >
> > > > Looks like, I cannot assign of_node member like below as it results in
> > > > pinctrl failure[1] during device bind.
> > > >
> > > > info.of_node = client->dev.of_node;
> > > >
> > > > [1]
> > > > pinctrl-rzg2l 1103.pinctrl: pin P43_0 already requested by 3-0012;
> > > > cannot claim for 3-006f pinctrl-rzg2l 1103.pinctrl: pin-344
> > > > (3-006f) status -22 pinctrl-rzg2l 1103.pinctrl: could not request
> > > > pin 344 (P43_0) from group pmic on device pinctrl-rzg2l
> > > > raa215300 3-006f: Error applying setting, reverse things back
> > >
> > > Where do you have a reference to pin P43_0 in your DT?
> >
> > The reference to pin P43_0 is added in the PMIC node.
> >
> > I have done modification on my board to test PMIC INT# on RZ/G2L SMARC EVK
> > by wiring R83 on SoM module and PMOD0 PIN7.
> >
> > > The last versions you posted did not have any pinctrl properties?
> >
> > By default, PMIC_INT# is not populated RZ/G2L SMARC EVK, so I haven't added
> > Support for PMIC_INT# for the patches posted till date.
> >
> > Yesterday I checked with HW people, is there a way to enable PMIC_INT#
> > and they told me to do the above HW modification.
> >
> > Today I found this issue, with this modified HW and PMIC INT# enabled on
> > the DT,
> > while assigning of_node of PMIC with info.of_node. It is just a coincidence.
>
> IC.
>
> So you now have two Linux devices pointing to the same DT node,
> causing pinctrl issues...
So don't set info.of_node? ;-)
Without of_node, devm_clk_get() and friends falls back to registered
clkdevs. So you could call clk_register_clkdev() from within the
PMIC driver, and can keep on using devm_clk_get_optional() in the
ISL1208 driver.
If that fails, there's also software_node.properties, or even the good
old platform_data...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Laurent, Thanks for the feedback. > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API > > On Wed, Jun 14, 2023 at 08:21:38AM +, Biju Das wrote: > > Hi Laurent, > > > > Thanks for the feedback. > > > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device > > > API > > > > > > On Tue, Jun 13, 2023 at 07:31:46PM +, 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 i
RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Geert, Thanks for the feedback. > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API > > Hi Biju, > > On Wed, Jun 14, 2023 at 10:21 AM 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 +, 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 reg
Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
On Wed, Jun 14, 2023 at 08:21:38AM +, Biju Das wrote: > Hi Laurent, > > Thanks for the feedback. > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API > > > > On Tue, Jun 13, 2023 at 07:31:46PM +, 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. > 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. 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. 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. A solution with two DT nodes models the hardware better and is cleaner. -- Regards, Laurent Pinchart
Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Biju,
On Wed, Jun 14, 2023 at 10:21 AM 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 +, 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.
>
> By instating a client device, we are sharing the relevant resources to RTC
> device driver.
Exactly. RAA215300 is a PMIC with an integrated ISL1208-derivative.
My biggest concern with using 2 separate nodes in DT is that one day
we might discover another integration issue, which needs communication
between the two parts.
Things from the top of my head:
1. The device has a single interrupt pin. Is there any interaction
or coordination between PMIC and RTC interrupts?
2. On the real ISL1208, the interrupt pin can also be used as a clock
output. Perhaps this is fed to some PMIC part in the
RAA215300, too?
2. Does the battery charger circuit in the PMIC impact the VBAT
input of the RTC?
3. Are there other I2C addresses the chip listens to?
I only have access to the Short-Form Datasheet for the RAA215300,
so I cannot check myself...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Laurent, Thanks for the feedback. > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API > > On Tue, Jun 13, 2023 at 07:31:46PM +, 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. By instating a client device, we are sharing the relevant resources to RTC device driver. Cheers, Biju
Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
On Tue, Jun 13, 2023 at 07:31:46PM +, 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. -- Regards, Laurent Pinchart
RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Geert,
Thanks for the feedback.
> Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
>
> Hi Biju,
>
> On Tue, Jun 13, 2023 at 6:11 PM Biju Das
> wrote:
> > > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device
> > > API On Tue, Jun 13, 2023 at 12:45 PM Biju Das
> > >
> > > wrote:
> > > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance
> > > > > i2c_new_ancillary_device API On Mon, Jun 12, 2023 at 10:43 PM
> > > > > Wolfram Sang
> > > wrote:
> > > > > > > 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
> > > > > > * 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.
> > > > > >
> > > > > > Should work or did I miss something here?
> > > > >
> > > > > That should work, mostly (i2c_new_dummy_device() also calls
> > > > > i2c_new_client_device()). And as i2c_board_info has an of_node
> > > > > member (something I had missed before!), the new I2C device can
> > > > > access the clocks in the DT node using the standard way.
> > > >
> > > > Looks like, I cannot assign of_node member like below as it
> > > > results in pinctrl failure[1] during device bind.
> > > >
> > > > info.of_node = client->dev.of_node;
> > > >
> > > > [1]
> > > > pinctrl-rzg2l 1103.pinctrl: pin P43_0 already requested by
> > > > 3-0012; cannot claim for 3-006f pinctrl-rzg2l 1103.pinctrl:
> > > > pin-344
> > > > (3-006f) status -22 pinctrl-rzg2l 1103.pinctrl: could not
> > > > request pin 344 (P43_0) from group pmic on device pinctrl-rzg2l
> > > > raa215300 3-006f: Error applying setting, reverse things back
> > >
> > > Where do you have a reference to pin P43_0 in your DT?
> >
> > The reference to pin P43_0 is added in the PMIC node.
> >
> > I have done modification on my board to test PMIC INT# on RZ/G2L SMARC
> > EVK by wiring R83 on SoM module and PMOD0 PIN7.
> >
> > > The last versions you posted did not have any pinctrl properties?
> >
> > By default, PMIC_INT# is not populated RZ/G2L SMARC EVK, so I haven't
> > added Support for PMIC_INT# for the patches posted till date.
> >
> > Yesterday I checked with HW people, is there a way to enable PMIC_INT#
> > and they told me to do the above HW modification.
> >
> > Today I found this issue, with this modified HW and PMIC INT# enabled
> > on the DT, while assigning of_node of PMIC with info.of_node. It is just
> a coincidence.
>
> IC.
>
> So you now have two Linux devices pointing to the same DT node, causing
> pinctrl issues...
>
> I know this won't solve the core issue, but what is the exact pintrl
> configuration you are using? Is this using a GPIO with interrupt
> capabilities, or a dedicated interrupt pin? In case of the former, you
> don't need a pinctrl property in DT, as the GPIO controller itself should
> take care of that by asking the pin controller to configure the pin
> properly through pinctrl_gpio_request().
I was testing with both. This issue is triggered while configuring IRQ4 as
PMIC_INT#.
pmic_pins: pmic {
pinmux = ; /* IRQ4 */
};
{
raa215300: pmic@12 {
pinctrl-0 = <_pins>;
pinctrl-names = "default";
compatible = "renesas,raa215300";
reg = <0x12>, <0x6f>;
reg-names = "main", "rtc";
clocks = <>;
clock-names = "xin";
//interrupt-parent = <>;
//interrupts = ;
interrupt-parent = <>;
interrupts = ;
};
};
Cheers,
Biju
Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Biju,
On Tue, Jun 13, 2023 at 6:11 PM Biju Das wrote:
> > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
> > On Tue, Jun 13, 2023 at 12:45 PM Biju Das
> > wrote:
> > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device
> > > > API On Mon, Jun 12, 2023 at 10:43 PM Wolfram Sang
> > wrote:
> > > > > > 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
> > > > > * 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.
> > > > >
> > > > > Should work or did I miss something here?
> > > >
> > > > That should work, mostly (i2c_new_dummy_device() also calls
> > > > i2c_new_client_device()). And as i2c_board_info has an of_node
> > > > member (something I had missed before!), the new I2C device can
> > > > access the clocks in the DT node using the standard way.
> > >
> > > Looks like, I cannot assign of_node member like below as it results in
> > > pinctrl failure[1] during device bind.
> > >
> > > info.of_node = client->dev.of_node;
> > >
> > > [1]
> > > pinctrl-rzg2l 1103.pinctrl: pin P43_0 already requested by 3-0012;
> > > cannot claim for 3-006f pinctrl-rzg2l 1103.pinctrl: pin-344
> > > (3-006f) status -22 pinctrl-rzg2l 1103.pinctrl: could not request
> > > pin 344 (P43_0) from group pmic on device pinctrl-rzg2l
> > > raa215300 3-006f: Error applying setting, reverse things back
> >
> > Where do you have a reference to pin P43_0 in your DT?
>
> The reference to pin P43_0 is added in the PMIC node.
>
> I have done modification on my board to test PMIC INT# on RZ/G2L SMARC EVK
> by wiring R83 on SoM module and PMOD0 PIN7.
>
> > The last versions you posted did not have any pinctrl properties?
>
> By default, PMIC_INT# is not populated RZ/G2L SMARC EVK, so I haven't added
> Support for PMIC_INT# for the patches posted till date.
>
> Yesterday I checked with HW people, is there a way to enable PMIC_INT#
> and they told me to do the above HW modification.
>
> Today I found this issue, with this modified HW and PMIC INT# enabled on the
> DT,
> while assigning of_node of PMIC with info.of_node. It is just a coincidence.
IC.
So you now have two Linux devices pointing to the same DT node,
causing pinctrl issues...
I know this won't solve the core issue, but what is the exact pintrl
configuration you are using? Is this using a GPIO with interrupt
capabilities, or a dedicated interrupt pin? In case of the former,
you don't need a pinctrl property in DT, as the GPIO controller itself
should take care of that by asking the pin controller to configure
the pin properly through pinctrl_gpio_request().
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Wolfram, > Subject: RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API > > Hi Wolfram, > > Thanks for the feedback. > > > Subject: RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device > > API > > > > Hi Wolfram, > > > > Thanks for the feedback. > > > > > 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. Cheers, Biju
RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Wolfram, Thanks for the feedback. > Subject: RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API > > Hi Wolfram, > > Thanks for the feedback. > > > 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?? Cheers, Biju
RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Geert, Thanks for the feedback. > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API > > Hi Biju, > > On Tue, Jun 13, 2023 at 12:45 PM Biju Das > wrote: > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device > > > API On Mon, Jun 12, 2023 at 10:43 PM Wolfram Sang > wrote: > > > > > 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 > > > > * 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. > > > > > > > > Should work or did I miss something here? > > > > > > That should work, mostly (i2c_new_dummy_device() also calls > > > i2c_new_client_device()). And as i2c_board_info has an of_node > > > member (something I had missed before!), the new I2C device can > > > access the clocks in the DT node using the standard way. > > > > Looks like, I cannot assign of_node member like below as it results in > > pinctrl failure[1] during device bind. > > > > info.of_node = client->dev.of_node; > > > > [1] > > pinctrl-rzg2l 1103.pinctrl: pin P43_0 already requested by 3-0012; > > cannot claim for 3-006f pinctrl-rzg2l 1103.pinctrl: pin-344 > > (3-006f) status -22 pinctrl-rzg2l 1103.pinctrl: could not request > > pin 344 (P43_0) from group pmic on device pinctrl-rzg2l > > raa215300 3-006f: Error applying setting, reverse things back > > Where do you have a reference to pin P43_0 in your DT? The reference to pin P43_0 is added in the PMIC node. I have done modification on my board to test PMIC INT# on RZ/G2L SMARC EVK by wiring R83 on SoM module and PMOD0 PIN7. > The last versions you posted did not have any pinctrl properties? By default, PMIC_INT# is not populated RZ/G2L SMARC EVK, so I haven't added Support for PMIC_INT# for the patches posted till date. Yesterday I checked with HW people, is there a way to enable PMIC_INT# and they told me to do the above HW modification. Today I found this issue, with this modified HW and PMIC INT# enabled on the DT, while assigning of_node of PMIC with info.of_node. It is just a coincidence. Cheers, Biju
Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Biju,
On Tue, Jun 13, 2023 at 12:45 PM Biju Das wrote:
> > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
> > On Mon, Jun 12, 2023 at 10:43 PM Wolfram Sang wrote:
> > > > 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
> > > * 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.
> > >
> > > Should work or did I miss something here?
> >
> > That should work, mostly (i2c_new_dummy_device() also calls
> > i2c_new_client_device()). And as i2c_board_info has an of_node member
> > (something I had missed before!), the new I2C device can access the clocks
> > in the DT node using the standard way.
>
> Looks like, I cannot assign of_node member like below as it results in
> pinctrl failure[1]
> during device bind.
>
> info.of_node = client->dev.of_node;
>
> [1]
> pinctrl-rzg2l 1103.pinctrl: pin P43_0 already requested by 3-0012; cannot
> claim for 3-006f
> pinctrl-rzg2l 1103.pinctrl: pin-344 (3-006f) status -22
> pinctrl-rzg2l 1103.pinctrl: could not request pin 344 (P43_0) from group
> pmic on device pinctrl-rzg2l
> raa215300 3-006f: Error applying setting, reverse things back
Where do you have a reference to pin P43_0 in your DT?
The last versions you posted did not have any pinctrl properties?
v6:
https://lore.kernel.org/linux-renesas-soc/20230602142426.438375-5-biju.das...@bp.renesas.com
v5:
https://lore.kernel.org/linux-renesas-soc/20230522101849.297499-12-biju.das...@bp.renesas.com
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Geert, Thanks for the feedback. > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API > > Hi Wolfram, > > On Mon, Jun 12, 2023 at 10:43 PM Wolfram Sang wrote: > > > 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 > > * 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. > > > > Should work or did I miss something here? > > That should work, mostly (i2c_new_dummy_device() also calls > i2c_new_client_device()). And as i2c_board_info has an of_node member > (something I had missed before!), the new I2C device can access the clocks > in the DT node using the standard way. Looks like, I cannot assign of_node member like below as it results in pinctrl failure[1] during device bind. info.of_node = client->dev.of_node; [1] pinctrl-rzg2l 1103.pinctrl: pin P43_0 already requested by 3-0012; cannot claim for 3-006f pinctrl-rzg2l 1103.pinctrl: pin-344 (3-006f) status -22 pinctrl-rzg2l 1103.pinctrl: could not request pin 344 (P43_0) from group pmic on device pinctrl-rzg2l raa215300 3-006f: Error applying setting, reverse things back Cheers, Biju
Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Wolfram,
On Mon, Jun 12, 2023 at 10:43 PM Wolfram Sang wrote:
> > 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
> * 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.
>
> Should work or did I miss something here?
That should work, mostly (i2c_new_dummy_device() also calls
i2c_new_client_device()). And as i2c_board_info has an of_node
member (something I had missed before!), the new I2C device
can access the clocks in the DT node using the standard way.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Wolfram, Thanks for the feedback. > 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. Cheers, Biju
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 * 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. Should work or did I miss something here? Happy hacking, Wolfram signature.asc Description: PGP signature
Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
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 +, 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 +, 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 = <>;
> > > > > 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 = <_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 = <>;
> > > > > clock-names = "xin";
> > > > > renesas,raa215300-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
Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Laurent,
On Mon, Jun 12, 2023 at 2:54 PM Laurent Pinchart
wrote:
> On Mon, Jun 12, 2023 at 12:42:33PM +, 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 +, 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 = <>;
> > > > 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 = <_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 = <>;
> > > > clock-names = "xin";
> > > > renesas,raa215300-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...
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...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Wolfram,
On Mon, Jun 12, 2023 at 2:48 PM Wolfram Sang wrote:
> > > Would this binding allow to not use the RTC if the second reg is
> > > missing? What are the advantages of not enabling RTC? Saving power?
> >
> > It doesn't work if there is no clock?
>
> Maybe I am confusing something now, but if the RTC _needs_ to be
> enabled, then why we don't do it unconditionally?
1. DT describes the hardware, which listens to two addresses, so the
device node should have two entries in the reg property.
2. The RTC is enabled by instantiating an i2c ancillary device, and lets the
isl1208 driver bind against it.
> > > Thinking more about this: DT is hardware description, so the RTC should
> > > always be described in DT. If the RTC is actually activated is more a
> > > configuration thing, or? Brainstorming: maybe the PMIC driver could try
> > > to find the node with reg == 0x6f and see if firmware has enabled it or
> > > not?
> >
> > I guess the RTC part would acknowledge anyway?
> > It is always present, it is just part of the RAA215300.
>
> I mean the driver should scan for the DT node. Not on the bus. But a
> phandle is probably safer.
>
> > Sure, you can put that in DT. But it's a pity you have to do that,
> > as the device (the PMIC part) does know the revision...
> > That's why I suggested to let the PMIC part instantiate an i2c ancillary
> > device...
>
> I see. I'll let it sink in some more.
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.
What's different here compared to e.g. adv748x?
- RAA215300 has a PMIC and an RTC, and there exists a separate
RTC driver for a similar part (which is thus Linux-specific,
not DT-specific!),
- I don't know much about adv748x, but I understand there is a
single driver talking to all subcomponents.
What if in the future we e.g. would want to spin off part of it
in a subdriver, as a subcomponent appeared in an unrelated device?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
On Mon, Jun 12, 2023 at 02:44:33PM +0200, Geert Uytterhoeven wrote:
> On Mon, Jun 12, 2023 at 2:23 PM Laurent Pinchart wrote:
> > On Mon, Jun 12, 2023 at 09:53:02AM +, Biju Das wrote:
> > > Hi All,
> > >
> > > 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 = <>;
> > > 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 = <_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";
>
> "renesas,raa215300-rtc", "isil,isl1208".
>
> However, that would suggest the RAA215300 RTC can be treated as
> an ISL1208, which is not true for all revisions...
It depends. If we add a renesas,invert-xtoscb DT property, then it
becomes true for all revisions.
> > Btw, it would be nice to convert
> > Documentation/devicetree/bindings/rtc/isil,isl1208.txt to YAML.
>
> Hey, look at patch 2 in this series ;-)
--
Regards,
Laurent Pinchart
RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Wolfram,
Thanks for the feedback.
> Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
>
> Hi Biju,
>
> > DT-Maintainers suggestion:
> > [1]
> > raa215300: pmic@12 {
> > compatible = "renesas,raa215300";
> > reg = <0x12>, <0x6f>;
> > reg-names = "main", "rtc";
> >
> > clocks = <>;
> > clock-names = "xin";
> > /* Add Optional shared IRQ resource and share it to child and handle
> > it both in parent and child */ };
>
> Would this binding allow to not use the RTC if the second reg is missing?
> What are the advantages of not enabling RTC? Saving power?
Some use case, just use PMIC for regulators, reset and STR(suspend to RAM) as
it may have
RTC support in SoC. for eg: RZ/N1.
>
> >
> > 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 = <_raa215300>; /* Parse the handle
> and
> > Enable RTC , if present.*/
>
> Thinking more about this: DT is hardware description, so the RTC should
> always be described in DT. If the RTC is actually activated is more a
> configuration thing, or? Brainstorming: maybe the PMIC driver could try to
> find the node with reg == 0x6f and see if firmware has enabled it or not?
In-built RTC is always present on the PMIC chips.
But RTC is enabled or not disabled is based on system design.
If X1N/XOUT both grounded means you cannot use RTC.
If XIN connected to external clock source means internal oscillator is disabled.
If XIN/XOUT connected to external crystal means internal oscillator is enabled.
>
> > };
> >
> > rtc_raa215300: rtc@6f {
> > compatible = "renesas,raa215300-isl1208";
> > reg = <0x6f>;
> >
> > /* Add Optional shared IRQ */
> > clocks = <>;
> > clock-names = "xin";
> > renesas,raa215300-pmic = <>; /* Parse the handle to get
> PMIC version to check Oscillator bit is inverted or not */
> > };
>
> I have been scratching my head around this and wondered about one thing.
> The RTC driver needs to know if the oscillator bit is inverted. AFAIU this
> depends on the version of the PMIC (which includes the RTC). So, can't we
> simply encode the version in the compatible string?
PMIC is a component on the SoM module. So SoM's may have different PMIC
versions.
I need to support all PMIC version with single image.
If we encode the version in the compatible string means, we need to detect PMIC
version in bootloader and update the rtc compatible and merge with kernel
device tree by bootloader.
Again, we need to define 1 extra compatible "renesas,raa215300-isl1208-a0"
in DT documentation for describing A0 chip that has inverted oscillator bit.
Cheers,
Biju
>
> > compatible = "renesas,raa215300-isl1208-01";
> > compatible = "renesas,raa215300-isl1208-a0";
>
> I dunno the exact versions, but you probably get the idea.
>
> Happy hacking,
>
>Wolfram
Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Biju,
On Mon, Jun 12, 2023 at 12:42:33PM +, Biju Das wrote:
> Hi Laurent,
>
> Thanks for the feedback.
>
> > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
> >
> > On Mon, Jun 12, 2023 at 09:53:02AM +, Biju Das wrote:
> > > Hi All,
> > >
> > > 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 = <>;
> > > 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 = <_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";
> >
> > Btw, it would be nice to convert
> > Documentation/devicetree/bindings/rtc/isil,isl1208.txt to YAML.
>
> It is already posted see [1] and [2]
> [1]
> https://patchwork.kernel.org/project/linux-renesas-soc/patch/20230602142426.438375-6-biju.das...@bp.renesas.com/
>
> [2]
> https://patchwork.kernel.org/project/linux-renesas-soc/patch/20230602142426.438375-7-biju.das...@bp.renesas.com/
Great :-) Thanks.
> > > reg = <0x6f>;
> > >
> > > /* Add Optional shared IRQ */
> > > clocks = <>;
> > > clock-names = "xin";
> > > renesas,raa215300-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.
> > > };
> > > > -Original Message-
> > > > From: Biju Das
> > > > Sent: Thursday, June 8, 2023 1:57 PM
> > > > To: Laurent Pinchart
> > > > Cc: Wolfram Sang ; Geert Uytterhoeven > > > m68k.org>; Krzysztof Kozlowski ;
> > > > Rob Herring ; Andrzej Hajda
> > > > ; Neil Armstrong
> > > > ; Robert Foss ; David
> > > > Airlie ; Daniel Vetter ; Kieran
> > > > Bingham ;
> > > > Mauro Carvalho Chehab ; Hans Verkuil > > > ci...@xs4all.nl>; Alessandro Zummo ; Alexandre
> > > > Belloni ; Jonas Karlman
> > > > ; Jernej Skrabec ; Uwe
> > > > Kleine-König ; Corey Minyard
> > > > ; Marek Behún ; Jiasheng
> > > > Jiang ; Antonio Borneo
> > > > ; Abhinav Kumar
> > > > ; Ahmad Fatoum ;
> > > > dri-devel@lists.freedesktop.org; linux-...@vger.kernel.org;
> > > > linux-me...@vger.kernel.org; Geert Uytterhoeven
> > > >
Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Geert, > > Would this binding allow to not use the RTC if the second reg is > > missing? What are the advantages of not enabling RTC? Saving power? > > It doesn't work if there is no clock? Maybe I am confusing something now, but if the RTC _needs_ to be enabled, then why we don't do it unconditionally? > > Thinking more about this: DT is hardware description, so the RTC should > > always be described in DT. If the RTC is actually activated is more a > > configuration thing, or? Brainstorming: maybe the PMIC driver could try > > to find the node with reg == 0x6f and see if firmware has enabled it or > > not? > > I guess the RTC part would acknowledge anyway? > It is always present, it is just part of the RAA215300. I mean the driver should scan for the DT node. Not on the bus. But a phandle is probably safer. > Sure, you can put that in DT. But it's a pity you have to do that, > as the device (the PMIC part) does know the revision... > That's why I suggested to let the PMIC part instantiate an i2c ancillary > device... I see. I'll let it sink in some more. Happy hacking, Wolfram signature.asc Description: PGP signature
Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Laurent,
On Mon, Jun 12, 2023 at 2:23 PM Laurent Pinchart
wrote:
> On Mon, Jun 12, 2023 at 09:53:02AM +, Biju Das wrote:
> > Hi All,
> >
> > 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 = <>;
> > 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 = <_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";
"renesas,raa215300-rtc", "isil,isl1208".
However, that would suggest the RAA215300 RTC can be treated as
an ISL1208, which is not true for all revisions...
> Btw, it would be nice to convert
> Documentation/devicetree/bindings/rtc/isil,isl1208.txt to YAML.
Hey, look at patch 2 in this series ;-)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Laurent,
Thanks for the feedback.
> Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
>
> On Mon, Jun 12, 2023 at 09:53:02AM +, Biju Das wrote:
> > Hi All,
> >
> > 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 = <>;
> > 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 = <_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";
>
> Btw, it would be nice to convert
> Documentation/devicetree/bindings/rtc/isil,isl1208.txt to YAML.
It is already posted see [1] and [2]
[1]
https://patchwork.kernel.org/project/linux-renesas-soc/patch/20230602142426.438375-6-biju.das...@bp.renesas.com/
[2]
https://patchwork.kernel.org/project/linux-renesas-soc/patch/20230602142426.438375-7-biju.das...@bp.renesas.com/
>
> > reg = <0x6f>;
> >
> > /* Add Optional shared IRQ */
> > clocks = <>;
> > clock-names = "xin";
> > renesas,raa215300-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.
> 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??
Cheers,
Biju
>
> > };
>
>
>
> > > -Original Message-
> > > From: Biju Das
> > > Sent: Thursday, June 8, 2023 1:57 PM
> > > To: Laurent Pinchart
> > > Cc: Wolfram Sang ; Geert Uytterhoeven > > m68k.org>; Krzysztof Kozlowski ;
> > > Rob Herring ; Andrzej Hajda
> > > ; Neil Armstrong
> > > ; Robert Foss ; David
> > > Airlie ; Daniel Vetter ; Kieran
> > > Bingham ;
> > > Mauro Carvalho Chehab ; Hans Verkuil > > ci...@xs4all.nl>; Alessandro Zummo ; Alexandre
> > > Belloni ; Jonas Karlman
> > > ; Jernej Skrabec ; Uwe
> > > Kleine-König ; Corey Minyard
> > > ; Marek Behún ; Jiasheng
> > > Jiang ; Antonio Borneo
> > > ; Abhinav Kumar
> > > ; Ahmad Fatoum ;
> > > dri-devel@lists.freedesktop.org; linux-...@vger.kernel.org;
> > > linux-me...@vger.kernel.org; Geert Uytterhoeven
> > > ; Fabrizio Castro
> > > ; linux-renesas-...@vger.kernel.org;
> > > Mark Brown
> > > Subject: RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device
> > > API
> > >
> > > Hi Laurent,
> > >
> > > Thanks for the feedback.
> > >
> > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance
> > > > i2c_new_ancillary_device API
> > > >
> > > > Hi Biju,
> > > >
> > > > On Thu, Jun 08, 2023 at 11:00:19AM +, Biju Das wrote:
> > > > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance
> > > > > > i2c_new_ancillary_device API On Thu, Jun 08, 2023 at
> 06:41:35AM+, 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
> > &g
Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Wolfram, Laurent,
On Mon, Jun 12, 2023 at 2:36 PM Wolfram Sang wrote:
> > DT-Maintainers suggestion:
> > [1]
> > raa215300: pmic@12 {
> > compatible = "renesas,raa215300";
If you go for separate nodes: "renesas,raa215300-pmic".
> > reg = <0x12>, <0x6f>;
> > reg-names = "main", "rtc";
> >
> > clocks = <>;
> > clock-names = "xin";
> > /* Add Optional shared IRQ resource and share it to child and handle
> > it both in parent and child */
> > };
>
> Would this binding allow to not use the RTC if the second reg is
> missing? What are the advantages of not enabling RTC? Saving power?
It doesn't work if there is no clock?
> > 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 = <_raa215300>; /* Parse the handle
> > and Enable RTC , if present.*/
>
> Thinking more about this: DT is hardware description, so the RTC should
> always be described in DT. If the RTC is actually activated is more a
> configuration thing, or? Brainstorming: maybe the PMIC driver could try
> to find the node with reg == 0x6f and see if firmware has enabled it or
> not?
I guess the RTC part would acknowledge anyway?
It is always present, it is just part of the RAA215300.
> > };
> >
> > rtc_raa215300: rtc@6f {
> > compatible = "renesas,raa215300-isl1208";
If you go for separate nodes: "renesas,raa215300-rtc".
> > reg = <0x6f>;
> >
> > /* Add Optional shared IRQ */
> > clocks = <>;
> > clock-names = "xin";
> > renesas,raa215300-pmic = <>; /* Parse the handle to get
> > PMIC version to check Oscillator bit is inverted or not */
> > };
>
> I have been scratching my head around this and wondered about one thing.
> The RTC driver needs to know if the oscillator bit is inverted. AFAIU
> this depends on the version of the PMIC (which includes the RTC). So,
> can't we simply encode the version in the compatible string?
>
> > compatible = "renesas,raa215300-isl1208-01";
> > compatible = "renesas,raa215300-isl1208-a0";
>
> I dunno the exact versions, but you probably get the idea.
Sure, you can put that in DT. But it's a pity you have to do that,
as the device (the PMIC part) does know the revision...
That's why I suggested to let the PMIC part instantiate an i2c ancillary
device...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Biju,
> DT-Maintainers suggestion:
> [1]
> raa215300: pmic@12 {
> compatible = "renesas,raa215300";
> reg = <0x12>, <0x6f>;
> reg-names = "main", "rtc";
>
> clocks = <>;
> clock-names = "xin";
> /* Add Optional shared IRQ resource and share it to child and handle it
> both in parent and child */
> };
Would this binding allow to not use the RTC if the second reg is
missing? What are the advantages of not enabling RTC? Saving power?
>
> 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 = <_raa215300>; /* Parse the handle
> and Enable RTC , if present.*/
Thinking more about this: DT is hardware description, so the RTC should
always be described in DT. If the RTC is actually activated is more a
configuration thing, or? Brainstorming: maybe the PMIC driver could try
to find the node with reg == 0x6f and see if firmware has enabled it or
not?
> };
>
> rtc_raa215300: rtc@6f {
> compatible = "renesas,raa215300-isl1208";
> reg = <0x6f>;
>
> /* Add Optional shared IRQ */
> clocks = <>;
> clock-names = "xin";
> renesas,raa215300-pmic = <>; /* Parse the handle to get
> PMIC version to check Oscillator bit is inverted or not */
> };
I have been scratching my head around this and wondered about one thing.
The RTC driver needs to know if the oscillator bit is inverted. AFAIU
this depends on the version of the PMIC (which includes the RTC). So,
can't we simply encode the version in the compatible string?
> compatible = "renesas,raa215300-isl1208-01";
> compatible = "renesas,raa215300-isl1208-a0";
I dunno the exact versions, but you probably get the idea.
Happy hacking,
Wolfram
signature.asc
Description: PGP signature
Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
On Mon, Jun 12, 2023 at 09:53:02AM +, Biju Das wrote:
> Hi All,
>
> 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 = <>;
> 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 = <_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";
Btw, it would be nice to convert
Documentation/devicetree/bindings/rtc/isil,isl1208.txt to YAML.
> reg = <0x6f>;
>
> /* Add Optional shared IRQ */
> clocks = <>;
> clock-names = "xin";
> renesas,raa215300-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, one option is to perform the auto-detection in the boot
loader and update the DT dynamically there.
> };
> > -Original Message-
> > From: Biju Das
> > Sent: Thursday, June 8, 2023 1:57 PM
> > To: Laurent Pinchart
> > Cc: Wolfram Sang ; Geert Uytterhoeven > m68k.org>; Krzysztof Kozlowski ; Rob
> > Herring ; Andrzej Hajda ;
> > Neil Armstrong ; Robert Foss
> > ; David Airlie ; Daniel Vetter
> > ; Kieran Bingham ;
> > Mauro Carvalho Chehab ; Hans Verkuil > ci...@xs4all.nl>; Alessandro Zummo ; Alexandre
> > Belloni ; Jonas Karlman ;
> > Jernej Skrabec ; Uwe Kleine-König > koe...@pengutronix.de>; Corey Minyard ; Marek Behún
> > ; Jiasheng Jiang ; Antonio Borneo
> > ; Abhinav Kumar ;
> > Ahmad Fatoum ; dri-devel@lists.freedesktop.org;
> > linux-...@vger.kernel.org; linux-me...@vger.kernel.org; Geert
> > Uytterhoeven ; Fabrizio Castro
> > ; linux-renesas-...@vger.kernel.org; Mark
> > Brown
> > Subject: RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
> >
> > Hi Laurent,
> >
> > Thanks for the feedback.
> >
> > > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device
> > > API
> > >
> > > Hi Biju,
> > >
> > > On Thu, Jun 08, 2023 at 11:00:19AM +, Biju Das wrote:
> > > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance
> > > > > i2c_new_ancillary_device API On Thu, Jun 08, 2023 at 06:41:35AM+,
> > > > > 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
> > > > > > > >
> > > > > > > > Hi all,
> > > > > > > >
> > > > > > > > sorry for not being able to chime in earlier.
> > > > > > > >
> > > > > > > > > In Biju's particular use case, the i2c device responds to
> > > > > > > > > two addresses, which is the standard i2c ancillary use case.
> > > > > > > > > However, what's special
> > > > > > > >
> > > > > > > > Not quite. ancillary is used when a *driver* needs to take
> > > > > > > > care of two addresses. We already have devices bundling two
> > > > > > > > features into the same chip. I recall at least RTC + EEPROM
> > > > > > > > somewhere. And so far, we have been handling this by
> > > > > > > > creating two nodes in DT and have proper binding docs.
> > > > > > > > I think this is cleaner. First, you can see in DT already
> > > > > > > > what the compound device really
RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi All,
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 = <>;
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 = <_raa215300>; /* Parse the handle
and Enable RTC , if present.*/
};
rtc_raa215300: rtc@6f {
compatible = "renesas,raa215300-isl1208";
reg = <0x6f>;
/* Add Optional shared IRQ */
clocks = <>;
clock-names = "xin";
renesas,raa215300-pmic = <>; /* Parse the handle to get
PMIC version to check Oscillator bit is inverted or not */
};
Cheers,
Biju
> -Original Message-
> From: Biju Das
> Sent: Thursday, June 8, 2023 1:57 PM
> To: Laurent Pinchart
> Cc: Wolfram Sang ; Geert Uytterhoeven m68k.org>; Krzysztof Kozlowski ; Rob
> Herring ; Andrzej Hajda ;
> Neil Armstrong ; Robert Foss
> ; David Airlie ; Daniel Vetter
> ; Kieran Bingham ;
> Mauro Carvalho Chehab ; Hans Verkuil ci...@xs4all.nl>; Alessandro Zummo ; Alexandre
> Belloni ; Jonas Karlman ;
> Jernej Skrabec ; Uwe Kleine-König koe...@pengutronix.de>; Corey Minyard ; Marek Behún
> ; Jiasheng Jiang ; Antonio Borneo
> ; Abhinav Kumar ;
> Ahmad Fatoum ; dri-devel@lists.freedesktop.org;
> linux-...@vger.kernel.org; linux-me...@vger.kernel.org; Geert
> Uytterhoeven ; Fabrizio Castro
> ; linux-renesas-...@vger.kernel.org; Mark
> Brown
> Subject: RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
>
> Hi Laurent,
>
> Thanks for the feedback.
>
> > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device
> > API
> >
> > Hi Biju,
> >
> > On Thu, Jun 08, 2023 at 11:00:19AM +, Biju Das wrote:
> > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance
> > > > i2c_new_ancillary_device API On Thu, Jun 08, 2023 at 06:41:35AM
> +, 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
> > > > > > >
> > > > > > > Hi all,
> > > > > > >
> > > > > > > sorry for not being able to chime in earlier.
> > > > > > >
> > > > > > > > In Biju's particular use case, the i2c device responds to
> > > > > > > > two addresses, which is the standard i2c ancillary use
> case.
> > > > > > > > However, what's special
> > > > > > >
> > > > > > > Not quite. ancillary is used when a *driver* needs to take
> > > > > > > care of two addresses. We already have devices bundling two
> > > > > > > features into the same chip. I recall at least RTC + EEPROM
> > > > > > > somewhere. And so far, we have been handling this by
> > > > > > > creating
> > two nodes in DT and have proper binding docs.
> > > > > > > I think this is cleaner. First, you can see in DT already
> > > > > > > what the compound device really consists of. In this case,
> > > > > > > which RTC and RTC driver is exactly needed. Second, the code
> > > > > > > added here adds complexity to the I2C core with another
> > > > > > > layer of
> > inderection for dummy devices.
> > > > > >
> > > > > > FYI, please see [1] and [2]
> > > > > >
> > > > > > As per DT maintainers, most of PMICs are described with one
> > > > > > node, even though RTC is on separate address. According to
> > > > > > them the DT schema allows multiple addresses for children.
> > > > > > But currently we lacks implementation for that. The
> > > > > > enhancement to this API allows that.
> > > > > >
> > > > > > > > As some resources are shared (knowledge about the
RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Laurent, Thanks for the feedback. > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API > > Hi Biju, > > On Thu, Jun 08, 2023 at 11:00:19AM +, Biju Das wrote: > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device > > > API On Thu, Jun 08, 2023 at 06:41:35AM +, 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 > > > > > > > > > > > > Hi all, > > > > > > > > > > > > sorry for not being able to chime in earlier. > > > > > > > > > > > > > In Biju's particular use case, the i2c device responds to > > > > > > > two addresses, which is the standard i2c ancillary use case. > > > > > > > However, what's special > > > > > > > > > > > > Not quite. ancillary is used when a *driver* needs to take > > > > > > care of two addresses. We already have devices bundling two > > > > > > features into the same chip. I recall at least RTC + EEPROM > > > > > > somewhere. And so far, we have been handling this by creating > two nodes in DT and have proper binding docs. > > > > > > I think this is cleaner. First, you can see in DT already what > > > > > > the compound device really consists of. In this case, which > > > > > > RTC and RTC driver is exactly needed. Second, the code added > > > > > > here adds complexity to the I2C core with another layer of > inderection for dummy devices. > > > > > > > > > > FYI, please see [1] and [2] > > > > > > > > > > As per DT maintainers, most of PMICs are described with one > > > > > node, even though RTC is on separate address. According to them > > > > > the DT schema allows multiple addresses for children. > > > > > But currently we lacks implementation for that. The enhancement > > > > > to this API allows that. > > > > > > > > > > > > As some resources are shared (knowledge about the clocks), > > > > > > > splitting this in two distinct devices in DT (which is what > > > > > > > Biju's initial patch series did) would need phandles to link > both nodes together. > > > > > > > > > > > > > > Do you have a better idea how to represent this? > > > > > > > > > > > > Not sure if I understood this chip correctly, but maybe: The > > > > > > PMIC driver exposes a clock gate which can be consumed by the > RTC driver? > > > > > > > > Let me give me some details of this PMIC chip. > > > > > > > > PMIC device has 2 addresses "0x12:- PMIC" , "0x6f"- rtc. > > > > > > > > It has XIN, XOUT, INT# pins and a register for firmware revisions. > > > > > > Is the firmware revision register accessed through address 0x12 > > > (PMIC) or 0x6f (RTC) ? > > > > 0x12(PMIC). > > > > > > Based on the system design, > > > > > > > > If XIN and XOUT is connected to external crystal, Internal > > > > oscillator is enabled for RTC. In this case we need to set the > > > > oscillator bit to "0". > > > > > > > > If XIN is connected to external clock source, Internal oscillator > > > > is disabled for RTC. In this case we need to set the oscillator > > > > bit to "1". > > > > > > Same here, which address is the oscillator bit accessed through ? > > > > RTC (0x6F)--> to set oscillator bit. > > And does the PMIC part depend on the oscillator bit being set correctly, > or is that used for the RTC only ? PMIC part does not. It is used only in RTC. Based on PMIC revision, we need to set the oscillator bit in RTC block for PMIC rev a0 and rest of the PMIC chips. On PMIC rev0, oscillator bit is inverted. Cheers, Biju > > > > > If XIN and XOUT not connected RTC operation not possible. > > > > > > > > IRQ# (optional) functionality is shared between PMIC and RTC. > > > > (PMIC fault for various bucks/LDOs/WDT/OTP/NVM and alarm > condition). > > > > > > IRQs can be shared between multiple devices so this shouldn't be a > > > problem. > > > > OK. How do we represent this IRQ in DT? > > You can simply reference the same IRQ from the interrupts property of > different DT nodes. > > > > > The board, I have doesn't populate IRQ# pin. If needed some > > > > customers can populate IRQ# pin and use it for PMIC fault and RTC > alarm. > > > > > > > > Also, currently my board has PMIC rev a0 where oscillator bit is > > > > inverted and internal oscillator is enabled (ie: XIN and XOUT is > > > > connected to external crystal) > > -- > Regards, > > Laurent Pinchart
Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Biju, On Thu, Jun 08, 2023 at 11:00:19AM +, Biju Das wrote: > > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API > > On Thu, Jun 08, 2023 at 06:41:35AM +, 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 > > > > > > > > > > Hi all, > > > > > > > > > > sorry for not being able to chime in earlier. > > > > > > > > > > > In Biju's particular use case, the i2c device responds to two > > > > > > addresses, which is the standard i2c ancillary use case. > > > > > > However, what's special > > > > > > > > > > Not quite. ancillary is used when a *driver* needs to take care of > > > > > two addresses. We already have devices bundling two features into > > > > > the same chip. I recall at least RTC + EEPROM somewhere. And so > > > > > far, we have been handling this by creating two nodes in DT and have > > > > > proper binding docs. > > > > > I think this is cleaner. First, you can see in DT already what the > > > > > compound device really consists of. In this case, which RTC and > > > > > RTC driver is exactly needed. Second, the code added here adds > > > > > complexity to the I2C core with another layer of inderection for > > > > > dummy devices. > > > > > > > > FYI, please see [1] and [2] > > > > > > > > As per DT maintainers, most of PMICs are described with one node, > > > > even though RTC is on separate address. According to them the DT > > > > schema allows multiple addresses for children. > > > > But currently we lacks implementation for that. The enhancement to > > > > this API allows that. > > > > > > > > > > As some resources are shared (knowledge about the clocks), > > > > > > splitting this in two distinct devices in DT (which is what > > > > > > Biju's initial patch series did) would need phandles to link both > > > > > > nodes together. > > > > > > > > > > > > Do you have a better idea how to represent this? > > > > > > > > > > Not sure if I understood this chip correctly, but maybe: The PMIC > > > > > driver exposes a clock gate which can be consumed by the RTC driver? > > > > > > Let me give me some details of this PMIC chip. > > > > > > PMIC device has 2 addresses "0x12:- PMIC" , "0x6f"- rtc. > > > > > > It has XIN, XOUT, INT# pins and a register for firmware revisions. > > > > Is the firmware revision register accessed through address 0x12 (PMIC) or > > 0x6f (RTC) ? > > 0x12(PMIC). > > > > Based on the system design, > > > > > > If XIN and XOUT is connected to external crystal, Internal oscillator > > > is enabled for RTC. In this case we need to set the oscillator bit to > > > "0". > > > > > > If XIN is connected to external clock source, Internal oscillator is > > > disabled for RTC. In this case we need to set the oscillator bit to > > > "1". > > > > Same here, which address is the oscillator bit accessed through ? > > RTC (0x6F)--> to set oscillator bit. And does the PMIC part depend on the oscillator bit being set correctly, or is that used for the RTC only ? > > > If XIN and XOUT not connected RTC operation not possible. > > > > > > IRQ# (optional) functionality is shared between PMIC and RTC. (PMIC > > > fault for various bucks/LDOs/WDT/OTP/NVM and alarm condition). > > > > IRQs can be shared between multiple devices so this shouldn't be a > > problem. > > OK. How do we represent this IRQ in DT? You can simply reference the same IRQ from the interrupts property of different DT nodes. > > > The board, I have doesn't populate IRQ# pin. If needed some customers > > > can populate IRQ# pin and use it for PMIC fault and RTC alarm. > > > > > > Also, currently my board has PMIC rev a0 where oscillator bit is > > > inverted and internal oscillator is enabled (ie: XIN and XOUT is > > > connected to external crystal) -- Regards, Laurent Pinchart
RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Laurent, Thanks for the feedback. > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API > > Hi Biju, > > On Thu, Jun 08, 2023 at 06:41:35AM +, 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 > > > > > > > > Hi all, > > > > > > > > sorry for not being able to chime in earlier. > > > > > > > > > In Biju's particular use case, the i2c device responds to two > > > > > addresses, which is the standard i2c ancillary use case. > > > > > However, what's special > > > > > > > > Not quite. ancillary is used when a *driver* needs to take care of > > > > two addresses. We already have devices bundling two features into > > > > the same chip. I recall at least RTC + EEPROM somewhere. And so > > > > far, we have been handling this by creating two nodes in DT and > have proper binding docs. > > > > I think this is cleaner. First, you can see in DT already what the > > > > compound device really consists of. In this case, which RTC and > > > > RTC driver is exactly needed. Second, the code added here adds > > > > complexity to the I2C core with another layer of inderection for > dummy devices. > > > > > > FYI, please see [1] and [2] > > > > > > As per DT maintainers, most of PMICs are described with one node, > > > even though RTC is on separate address. According to them the DT > > > schema allows multiple addresses for children. > > > But currently we lacks implementation for that. The enhancement to > > > this API allows that. > > > > > > > > As some resources are shared (knowledge about the clocks), > > > > > splitting this in two distinct devices in DT (which is what > > > > > Biju's initial patch series did) would need phandles to link both > nodes together. > > > > > > > > > > Do you have a better idea how to represent this? > > > > > > > > Not sure if I understood this chip correctly, but maybe: The PMIC > > > > driver exposes a clock gate which can be consumed by the RTC > driver? > > > > Let me give me some details of this PMIC chip. > > > > PMIC device has 2 addresses "0x12:- PMIC" , "0x6f"- rtc. > > > > It has XIN, XOUT, INT# pins and a register for firmware revisions. > > Is the firmware revision register accessed through address 0x12 (PMIC) or > 0x6f (RTC) ? 0x12(PMIC). > > > Based on the system design, > > > > If XIN and XOUT is connected to external crystal, Internal oscillator > > is enabled for RTC. In this case we need to set the oscillator bit to > > "0". > > > > If XIN is connected to external clock source, Internal oscillator is > > disabled for RTC. In this case we need to set the oscillator bit to > > "1". > > Same here, which address is the oscillator bit accessed through ? RTC (0x6F)--> to set oscillator bit. > > > If XIN and XOUT not connected RTC operation not possible. > > > > IRQ# (optional) functionality is shared between PMIC and RTC. (PMIC > > fault for various bucks/LDOs/WDT/OTP/NVM and alarm condition). > > IRQs can be shared between multiple devices so this shouldn't be a > problem. OK. How do we represent this IRQ in DT? Cheers, Biju > > > The board, I have doesn't populate IRQ# pin. If needed some customers > > can populate IRQ# pin and use it for PMIC fault and RTC alarm. > > > > Also, currently my board has PMIC rev a0 where oscillator bit is > > inverted and internal oscillator is enabled (ie: XIN and XOUT is > > connected to external crystal) > > -- > Regards, > > Laurent Pinchart
Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Biju, On Thu, Jun 08, 2023 at 06:41:35AM +, 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 > > > > > > Hi all, > > > > > > sorry for not being able to chime in earlier. > > > > > > > In Biju's particular use case, the i2c device responds to two > > > > addresses, which is the standard i2c ancillary use case. However, > > > > what's special > > > > > > Not quite. ancillary is used when a *driver* needs to take care of two > > > addresses. We already have devices bundling two features into the same > > > chip. I recall at least RTC + EEPROM somewhere. And so far, we have > > > been handling this by creating two nodes in DT and have proper binding > > > docs. > > > I think this is cleaner. First, you can see in DT already what the > > > compound device really consists of. In this case, which RTC and RTC > > > driver is exactly needed. Second, the code added here adds complexity > > > to the I2C core with another layer of inderection for dummy devices. > > > > FYI, please see [1] and [2] > > > > As per DT maintainers, most of PMICs are described with one node, even > > though RTC is on separate address. According to them the DT schema allows > > multiple addresses for children. > > But currently we lacks implementation for that. The enhancement to this > > API allows that. > > > > > > As some resources are shared (knowledge about the clocks), splitting > > > > this in two distinct devices in DT (which is what Biju's initial > > > > patch series did) would need phandles to link both nodes together. > > > > > > > > Do you have a better idea how to represent this? > > > > > > Not sure if I understood this chip correctly, but maybe: The PMIC > > > driver exposes a clock gate which can be consumed by the RTC driver? > > Let me give me some details of this PMIC chip. > > PMIC device has 2 addresses "0x12:- PMIC" , "0x6f"- rtc. > > It has XIN, XOUT, INT# pins and a register for firmware revisions. Is the firmware revision register accessed through address 0x12 (PMIC) or 0x6f (RTC) ? > Based on the system design, > > If XIN and XOUT is connected to external crystal, Internal oscillator > is enabled for RTC. In this case we need to set the oscillator bit to > "0". > > If XIN is connected to external clock source, Internal oscillator is > disabled for RTC. In this case we need to set the oscillator bit to > "1". Same here, which address is the oscillator bit accessed through ? > If XIN and XOUT not connected RTC operation not possible. > > IRQ# (optional) functionality is shared between PMIC and RTC. (PMIC > fault for various bucks/LDOs/WDT/OTP/NVM or alarm condition). IRQs can be shared between multiple devices so this shouldn't be a problem. > The board, I have doesn't populate IRQ# pin. If needed some customers > can populate IRQ# pin and use it for PMIC fault or RTC alarm. > > Also, currently my board has PMIC rev a0 where oscillator bit is > inverted and internal oscillator is enabled (ie: XIN and XOUT is > connected to external crystal) -- Regards, Laurent Pinchart
RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Wolfram, > Subject: RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API > > > Hi Wolfram, > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device > > API > > > > Hi all, > > > > sorry for not being able to chime in earlier. > > > > > In Biju's particular use case, the i2c device responds to two > > > addresses, which is the standard i2c ancillary use case. However, > > > what's special > > > > Not quite. ancillary is used when a *driver* needs to take care of two > > addresses. We already have devices bundling two features into the same > > chip. I recall at least RTC + EEPROM somewhere. And so far, we have > > been handling this by creating two nodes in DT and have proper binding > docs. > > I think this is cleaner. First, you can see in DT already what the > > compound device really consists of. In this case, which RTC and RTC > > driver is exactly needed. Second, the code added here adds complexity > > to the I2C core with another layer of inderection for dummy devices. > > FYI, please see [1] and [2] > > As per DT maintainers, most of PMICs are described with one node, even > though RTC is on separate address. According to them the DT schema allows > multiple addresses for children. > But currently we lacks implementation for that. The enhancement to this > API allows that. > > > > > > > As some resources are shared (knowledge about the clocks), splitting > > > this in two distinct devices in DT (which is what Biju's initial > > > patch series did) would need phandles to link both nodes together. > > > > > > Do you have a better idea how to represent this? > > > > Not sure if I understood this chip correctly, but maybe: The PMIC > > driver exposes a clock gate which can be consumed by the RTC driver? Let me give me some details of this PMIC chip. PMIC device has 2 addresses "0x12:- PMIC" , "0x6f"- rtc. It has XIN, XOUT, INT# pins and a register for firmware revisions. Based on the system design, If XIN and XOUT is connected to external crystal, Internal oscillator is enabled for RTC. In this case we need to set the oscillator bit to "0". If XIN is connected to external clock source, Internal oscillator is disabled for RTC. In this case we need to set the oscillator bit to "1". If XIN and XOUT not connected RTC operation not possible. IRQ# (optional) functionality is shared between PMIC and RTC. (PMIC fault for various bucks/LDOs/WDT/OTP/NVM or alarm condition). The board, I have doesn't populate IRQ# pin. If needed some customers can populate IRQ# pin and use it for PMIC fault or RTC alarm. Also, currently my board has PMIC rev a0 where oscillator bit is inverted and internal oscillator is enabled (ie: XIN and XOUT is connected to external crystal) Cheers, Biju
RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Wolfram, > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API > > Hi all, > > sorry for not being able to chime in earlier. > > > In Biju's particular use case, the i2c device responds to two > > addresses, which is the standard i2c ancillary use case. However, > > what's special > > Not quite. ancillary is used when a *driver* needs to take care of two > addresses. We already have devices bundling two features into the same > chip. I recall at least RTC + EEPROM somewhere. And so far, we have been > handling this by creating two nodes in DT and have proper binding docs. > I think this is cleaner. First, you can see in DT already what the > compound device really consists of. In this case, which RTC and RTC driver > is exactly needed. Second, the code added here adds complexity to the I2C > core with another layer of inderection for dummy devices. FYI, please see [1] and [2] As per DT maintainers, most of PMICs are described with one node, even though RTC is on separate address. According to them the DT schema allows multiple addresses for children. But currently we lacks implementation for that. The enhancement to this API allows that. [1] https://lore.kernel.org/linux-renesas-soc/bce6cbcd-b693-4027-7d17-8e582b8fa...@linaro.org/ and [2] https://lore.kernel.org/linux-renesas-soc/CAMuHMdVrH5R4mAjm1c9zRqiGhNsfT7Y13xxaV-v05T-MCJ6=r...@mail.gmail.com/ Cheers, Biju > > > As some resources are shared (knowledge about the clocks), splitting > > this in two distinct devices in DT (which is what Biju's initial patch > > series did) would need phandles to link both nodes together. > > > > Do you have a better idea how to represent this? > > Not sure if I understood this chip correctly, but maybe: The PMIC driver > exposes a clock gate which can be consumed by the RTC driver? > > Happy hacking, > >Wolfram
Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi all, sorry for not being able to chime in earlier. > In Biju's particular use case, the i2c device responds to two addresses, > which is the standard i2c ancillary use case. However, what's special Not quite. ancillary is used when a *driver* needs to take care of two addresses. We already have devices bundling two features into the same chip. I recall at least RTC + EEPROM somewhere. And so far, we have been handling this by creating two nodes in DT and have proper binding docs. I think this is cleaner. First, you can see in DT already what the compound device really consists of. In this case, which RTC and RTC driver is exactly needed. Second, the code added here adds complexity to the I2C core with another layer of inderection for dummy devices. > As some resources are shared (knowledge about the clocks), splitting > this in two distinct devices in DT (which is what Biju's initial patch > series did) would need phandles to link both nodes together. > > Do you have a better idea how to represent this? Not sure if I understood this chip correctly, but maybe: The PMIC driver exposes a clock gate which can be consumed by the RTC driver? Happy hacking, Wolfram signature.asc Description: PGP signature
Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
> Wolfram: time to chime in ;-) I'll have a look this week. signature.asc Description: PGP signature
RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi All, > Subject: RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API > > Hi Laurent, > > Thanks for the feedback. > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device > > API > > > > On Wed, May 31, 2023 at 12:53:18PM +0000, Biju Das wrote: > > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance > > > > i2c_new_ancillary_device API On Wed, May 31, 2023 at 09:34:06AM > +, Biju Das wrote: > > > > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance > > > > > > i2c_new_ancillary_device API On Mon, May 29, 2023 at > > > > > > 09:00:43AM > > +, Biju Das wrote: > > > > > > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance > > > > > > > > i2c_new_ancillary_device API On Mon, May 22, 2023 at > > 11:18:39AM +0100, Biju Das wrote: > > > > > > > > > Renesas PMIC RAA215300 exposes two separate i2c devices, > > > > > > > > > one for the main device and another for rtc device. > > > > > > > > > > > > > > > > > > Enhance i2c_new_ancillary_device() to instantiate a real > > device. > > > > > > > > > > > > > > > > Doesn't it already instantiate a real device ? > > > > > > > > > > > > > And that function calls i2c_new_client_device(), which > > > > > > allocates a struct i2c_client that embeds a struct device, and > > > > > > registers that device with the kernel device core. How is that > > > > > > different, in terms of instantiating a "real device", from > > > > > > what this patch > > does ? > > > > > > > > > > There is a difference, right? it instantiates new "i2c dummy > > client" > > > > > driver and a "real i2c client device" driver like rtc device > > > > client(rtc-isl2108)?? > > > > > > > > I don't see how there's a difference in behaviour in the code you > > > > have implemented, sorry. > > > > > > Will reword, > > > > > > Enhance i2c_new_ancillary_device() to instantiate a "I2C client > > device" > > > apart from the already supported "I2C dummy client device". > > > > > > > > > > > > (eg: Instantiate rtc device from PMIC driver) > > > > > > > > > > > > > > > > > > Added helper function __i2c_new_dummy_device to share > > > > > > > > > the code between i2c_new_dummy_device and > > i2c_new_ancillary_device(). > > > > > > > > > > > > > > > > > > Also added helper function __i2c_new_client_device() to > > > > > > > > > pass parent dev parameter, so that the ancillary device > > > > > > > > > can assign its parent during creation. > > > > > > > > > > > > > > > > > > Suggested-by: Geert Uytterhoeven > > > > > > > > > > > > > > > > > > Signed-off-by: Biju Das > > > > > > > > > --- > > > > > > > > > v4->v5: > > > > > > > > > * Replaced parameter dev->parent in > > __i2c_new_client_device() and > > > > > > > > >__i2c_new_dummy_device(). > > > > > > > > > * Improved error message in __i2c_new_dummy_device() by > > printing device name. > > > > > > > > > * Updated comment for ancillary's device parent > > > > > > > > > * Dropped aux_device_name check in > > i2c_new_ancillary_device(). > > > > > > > > > v3->v4: > > > > > > > > > * Dropped Rb tag from Geert as there are new changes. > > > > > > > > > * Introduced __i2c_new_dummy_device() to share the code > > between > > > > > > > > >i2c_new_dummy_device and i2c_new_ancillary_device(). > > > > > > > > > * Introduced __i2c_new_client_device() to pass parent > dev > > > > > > > > >parameter, so that the ancillary device can assign > > > > > > > > > its > > parent during > > > > > > > > >creation. > > > > > > > > > v3: > > > > > > > > > * New pa
RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Geert,
Thanks for the feedback.
> Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
>
> Hi Biju,
>
> On Wed, May 31, 2023 at 2:53 PM Biju Das
> wrote:
> > > > * This function creates and returns an I2C ancillary client whose
> > > > I2C address
> > > > * is retrieved from the platform firmware based on the given slave
> > > > name. If
> > > > * aux_device_name is not NULL, the ancillary's device parent
> > > > * will be set to the primary device otherwise it will be set to
> > > > I2C
> > > adapter.
> > >
> > > The wording is better, but this is not what you have implemented in
> > > the code. The name doesn't select which parent is used.
> >
> > It is the same implemented in the code.
> >
> > For the existing users, aux_device_name is NULL --> The parent is set
> as "I2C adapter".
> >
> > For instantiating a "i2c client device", aux_device_name is not NULL -
> -> The parent is set as primary device.
> >
> > The primary device is the one instantiated the "i2c client device"
> > using i2c_new_ancillary_device().
> >
> > Please correct me if anything wrong here.
> >
> > >
> > > > * If no address is specified by the firmware default_addr is used.
> > > >
> > > > > > > > the ancillary's device parent
> > > > > > > > + * will be set to the primary device.
> > > > > > >
> > > > > > > This doesn't seem to match the implementation. With this
> > > > > > > patch the ancillary device's parent is always the primary
> > > > > > > device. Are you sure this won't cause any regression ?
> > > > > >
> > > > > > There is no regression as existing users only instantiate
> > > > > > dummy
> > > > > device.
> > > > >
> > > > > Sorry, I don't follow you here. Existing callers of
> > > > > i2c_new_ancillary_device() today get an i2c_client device whose
> > > > > parent is the I2C adapter. With this patch they will get an
> > > > > i2c_client device whose parent is the main i2c_client. That's a
> > > > > change in behaviour, which could cause all sorts of issues.
> > > >
> > > > Please see the patch snippet below, there is no regression.
> > > >
> > > > client->dev.parent = parent ? parent : >adapter->dev;
> > >
> > > When called from i2c_new_ancillary_device(),
> > > __i2c_new_dummy_device() as a non-NULL parent argument. There is no
> > > change of behaviour *for i2c_new_dummy_device()*, but thre is a
> > > change of behaviour *for i2c_new_ancillary_device()*.
> >
> >
> > I don't think I understand what you mean.
> >
> > For existing users, i2c_new_ancillary_device(...,
> aux_device_name=NULL) the behaviour is not changed.
> >
> > Could you please elaborate further?
>
> Laurent is right, there is a small issue:
>
> struct i2c_client *i2c_new_ancillary_device(struct i2c_client
> *client,
>const char *name,
>u16 default_addr,
>const char
> *aux_device_name)
> {
> ...
>return __i2c_new_dummy_device(client->adapter, addr,
> aux_device_name,
> >dev);
> }
>
> To preserve backwards compatibility, the last parameter should be
>
> aux_device_name ? >dev : NULL
OK, will fix this in next version.
Cheers,
Biju
RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Laurent, Thanks for the feedback. > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API > > On Wed, May 31, 2023 at 12:53:18PM +, Biju Das wrote: > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device > > > API On Wed, May 31, 2023 at 09:34:06AM +, Biju Das wrote: > > > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance > > > > > i2c_new_ancillary_device API On Mon, May 29, 2023 at 09:00:43AM > +0000, Biju Das wrote: > > > > > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance > > > > > > > i2c_new_ancillary_device API On Mon, May 22, 2023 at > 11:18:39AM +0100, Biju Das wrote: > > > > > > > > Renesas PMIC RAA215300 exposes two separate i2c devices, > > > > > > > > one for the main device and another for rtc device. > > > > > > > > > > > > > > > > Enhance i2c_new_ancillary_device() to instantiate a real > device. > > > > > > > > > > > > > > Doesn't it already instantiate a real device ? > > > > > > > > > > > And that function calls i2c_new_client_device(), which allocates > > > > > a struct i2c_client that embeds a struct device, and registers > > > > > that device with the kernel device core. How is that different, > > > > > in terms of instantiating a "real device", from what this patch > does ? > > > > > > > > There is a difference, right? it instantiates new "i2c dummy > client" > > > > driver and a "real i2c client device" driver like rtc device > > > client(rtc-isl2108)?? > > > > > > I don't see how there's a difference in behaviour in the code you > > > have implemented, sorry. > > > > Will reword, > > > > Enhance i2c_new_ancillary_device() to instantiate a "I2C client > device" > > apart from the already supported "I2C dummy client device". > > > > > > > > > > (eg: Instantiate rtc device from PMIC driver) > > > > > > > > > > > > > > > > Added helper function __i2c_new_dummy_device to share the > > > > > > > > code between i2c_new_dummy_device and > i2c_new_ancillary_device(). > > > > > > > > > > > > > > > > Also added helper function __i2c_new_client_device() to > > > > > > > > pass parent dev parameter, so that the ancillary device > > > > > > > > can assign its parent during creation. > > > > > > > > > > > > > > > > Suggested-by: Geert Uytterhoeven > > > > > > > > Signed-off-by: Biju Das > > > > > > > > --- > > > > > > > > v4->v5: > > > > > > > > * Replaced parameter dev->parent in > __i2c_new_client_device() and > > > > > > > >__i2c_new_dummy_device(). > > > > > > > > * Improved error message in __i2c_new_dummy_device() by > printing device name. > > > > > > > > * Updated comment for ancillary's device parent > > > > > > > > * Dropped aux_device_name check in > i2c_new_ancillary_device(). > > > > > > > > v3->v4: > > > > > > > > * Dropped Rb tag from Geert as there are new changes. > > > > > > > > * Introduced __i2c_new_dummy_device() to share the code > between > > > > > > > >i2c_new_dummy_device and i2c_new_ancillary_device(). > > > > > > > > * Introduced __i2c_new_client_device() to pass parent dev > > > > > > > >parameter, so that the ancillary device can assign its > parent during > > > > > > > >creation. > > > > > > > > v3: > > > > > > > > * New patch > > > > > > > > > > > > > > > > Ref: > > > > > > > > > > > > > > > > --- > > > > > > > > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 6 +- > > > > > > > > drivers/i2c/i2c-core-base.c | 92 > +--- > > > > > > > > drivers/media/i2c/adv748x/adv748x-core.c | 2 +- > > > > > > > > drivers/media/i2c/adv7
Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Biju,
On Wed, May 31, 2023 at 2:53 PM Biju Das wrote:
> > > * This function creates and returns an I2C ancillary client whose I2C
> > > address
> > > * is retrieved from the platform firmware based on the given slave
> > > name. If
> > > * aux_device_name is not NULL, the ancillary's device parent
> > > * will be set to the primary device otherwise it will be set to I2C
> > adapter.
> >
> > The wording is better, but this is not what you have implemented in the
> > code. The name doesn't select which parent is used.
>
> It is the same implemented in the code.
>
> For the existing users, aux_device_name is NULL --> The parent is set as "I2C
> adapter".
>
> For instantiating a "i2c client device", aux_device_name is not NULL --> The
> parent is set as primary device.
>
> The primary device is the one instantiated the "i2c client device" using
> i2c_new_ancillary_device().
>
> Please correct me if anything wrong here.
>
> >
> > > * If no address is specified by the firmware default_addr is used.
> > >
> > > > > > > the ancillary's device parent
> > > > > > > + * will be set to the primary device.
> > > > > >
> > > > > > This doesn't seem to match the implementation. With this patch
> > > > > > the ancillary device's parent is always the primary device. Are
> > > > > > you sure this won't cause any regression ?
> > > > >
> > > > > There is no regression as existing users only instantiate dummy
> > > > device.
> > > >
> > > > Sorry, I don't follow you here. Existing callers of
> > > > i2c_new_ancillary_device() today get an i2c_client device whose
> > > > parent is the I2C adapter. With this patch they will get an
> > > > i2c_client device whose parent is the main i2c_client. That's a
> > > > change in behaviour, which could cause all sorts of issues.
> > >
> > > Please see the patch snippet below, there is no regression.
> > >
> > > client->dev.parent = parent ? parent : >adapter->dev;
> >
> > When called from i2c_new_ancillary_device(), __i2c_new_dummy_device() as
> > a non-NULL parent argument. There is no change of behaviour *for
> > i2c_new_dummy_device()*, but thre is a change of behaviour *for
> > i2c_new_ancillary_device()*.
>
>
> I don't think I understand what you mean.
>
> For existing users, i2c_new_ancillary_device(..., aux_device_name=NULL) the
> behaviour is not changed.
>
> Could you please elaborate further?
Laurent is right, there is a small issue:
struct i2c_client *i2c_new_ancillary_device(struct i2c_client *client,
const char *name,
u16 default_addr,
const char *aux_device_name)
{
...
return __i2c_new_dummy_device(client->adapter, addr, aux_device_name,
>dev);
}
To preserve backwards compatibility, the last parameter should be
aux_device_name ? >dev : NULL
Sorry for missing that before.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Laurent,
On Wed, May 31, 2023 at 3:37 PM Laurent Pinchart
wrote:
> On Wed, May 31, 2023 at 02:51:48PM +0200, Geert Uytterhoeven wrote:
> > On Wed, May 31, 2023 at 10:59 AM Laurent Pinchart wrote:
> > > On Mon, May 29, 2023 at 09:00:43AM +, Biju Das wrote:
> > > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device
> > > > > API
> > > > > And why do you need this ?
> > > >
> > > > As per Krzysztof [2],
> > > >
> > > > The DT schema allows multiple addresses for children. But we lack
> > > > implementation of parent child relationship, As parent owns the
> > > > resources.
> > > > Child device needs to parse parent node to get some resource
> > > > like clocks.
> > > >
> > > > [2]
> > > > https://lore.kernel.org/linux-renesas-soc/tycpr01mb5933bffd4eb556f5fb4ea82186...@tycpr01mb5933.jpnprd01.prod.outlook.com/
> > >
> > > The I2C ancillary clients are not meant to be handled by separate
> > > drivers. You're supposed to have one device node in DT, which causes the
> > > I2C core to instantiate a main i2c_client, and bind it to one driver.
> > > That driver then uses i2c_new_ancillary_device() to create other
> > > i2c_client instances for the secondary I2C addresses. Those i2c_client
> > > instances are not bound to a separate driver, so there should be no code
> > > that needs to look at the parent for resources.
> >
> > In Biju's particular use case, the i2c device responds to two addresses,
> > which is the standard i2c ancillary use case. However, what's special
> > is that the second instance is a derivative of an existing i2c device
> > with an existing Linux driver. Hence the desire to make the existing
> > driver match against the second instance, which requires these changes
> > to i2c_new_ancillary_device().
> >
> > As some resources are shared (knowledge about the clocks), splitting
> > this in two distinct devices in DT (which is what Biju's initial patch
> > series did) would need phandles to link both nodes together.
> >
> > Do you have a better idea how to represent this?
>
> MFD ? Otherwise, I'll delegate that to Wolfram, I've spent enough time
> on this patch series I'm afraid :-)
That was v2... (do I need to repeat I don't like mfds? ;-)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Geert, On Wed, May 31, 2023 at 02:51:48PM +0200, Geert Uytterhoeven wrote: > On Wed, May 31, 2023 at 10:59 AM Laurent Pinchart wrote: > > On Mon, May 29, 2023 at 09:00:43AM +, Biju Das wrote: > > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API > > > > And why do you need this ? > > > > > > As per Krzysztof [2], > > > > > > The DT schema allows multiple addresses for children. But we lack > > > implementation of parent child relationship, As parent owns the resources. > > > Child device needs to parse parent node to get some resource > > > like clocks. > > > > > > [2] > > > https://lore.kernel.org/linux-renesas-soc/tycpr01mb5933bffd4eb556f5fb4ea82186...@tycpr01mb5933.jpnprd01.prod.outlook.com/ > > > > The I2C ancillary clients are not meant to be handled by separate > > drivers. You're supposed to have one device node in DT, which causes the > > I2C core to instantiate a main i2c_client, and bind it to one driver. > > That driver then uses i2c_new_ancillary_device() to create other > > i2c_client instances for the secondary I2C addresses. Those i2c_client > > instances are not bound to a separate driver, so there should be no code > > that needs to look at the parent for resources. > > In Biju's particular use case, the i2c device responds to two addresses, > which is the standard i2c ancillary use case. However, what's special > is that the second instance is a derivative of an existing i2c device > with an existing Linux driver. Hence the desire to make the existing > driver match against the second instance, which requires these changes > to i2c_new_ancillary_device(). > > As some resources are shared (knowledge about the clocks), splitting > this in two distinct devices in DT (which is what Biju's initial patch > series did) would need phandles to link both nodes together. > > Do you have a better idea how to represent this? MFD ? Otherwise, I'll delegate that to Wolfram, I've spent enough time on this patch series I'm afraid :-) > Wolfram: time to chime in ;-) -- Regards, Laurent Pinchart
Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
On Wed, May 31, 2023 at 12:53:18PM +, Biju Das wrote: > > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API > > On Wed, May 31, 2023 at 09:34:06AM +, Biju Das wrote: > > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device > > > > API On Mon, May 29, 2023 at 09:00:43AM +, Biju Das wrote: > > > > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance > > > > > > i2c_new_ancillary_device API On Mon, May 22, 2023 at 11:18:39AM > > > > > > +0100, Biju Das wrote: > > > > > > > Renesas PMIC RAA215300 exposes two separate i2c devices, one > > > > > > > for the main device and another for rtc device. > > > > > > > > > > > > > > Enhance i2c_new_ancillary_device() to instantiate a real device. > > > > > > > > > > > > Doesn't it already instantiate a real device ? > > > > > > > > > And that function calls i2c_new_client_device(), which allocates a > > > > struct i2c_client that embeds a struct device, and registers that > > > > device with the kernel device core. How is that different, in terms > > > > of instantiating a "real device", from what this patch does ? > > > > > > There is a difference, right? it instantiates new "i2c dummy client" > > > driver and a "real i2c client device" driver like rtc device > > client(rtc-isl2108)?? > > > > I don't see how there's a difference in behaviour in the code you have > > implemented, sorry. > > Will reword, > > Enhance i2c_new_ancillary_device() to instantiate a "I2C client device" > apart from the already supported "I2C dummy client device". > > > > > > > > (eg: Instantiate rtc device from PMIC driver) > > > > > > > > > > > > > > Added helper function __i2c_new_dummy_device to share the code > > > > > > > between i2c_new_dummy_device and i2c_new_ancillary_device(). > > > > > > > > > > > > > > Also added helper function __i2c_new_client_device() to pass > > > > > > > parent dev parameter, so that the ancillary device can assign > > > > > > > its parent during creation. > > > > > > > > > > > > > > Suggested-by: Geert Uytterhoeven > > > > > > > Signed-off-by: Biju Das > > > > > > > --- > > > > > > > v4->v5: > > > > > > > * Replaced parameter dev->parent in __i2c_new_client_device() and > > > > > > >__i2c_new_dummy_device(). > > > > > > > * Improved error message in __i2c_new_dummy_device() by printing > > > > > > > device name. > > > > > > > * Updated comment for ancillary's device parent > > > > > > > * Dropped aux_device_name check in i2c_new_ancillary_device(). > > > > > > > v3->v4: > > > > > > > * Dropped Rb tag from Geert as there are new changes. > > > > > > > * Introduced __i2c_new_dummy_device() to share the code between > > > > > > >i2c_new_dummy_device and i2c_new_ancillary_device(). > > > > > > > * Introduced __i2c_new_client_device() to pass parent dev > > > > > > >parameter, so that the ancillary device can assign its parent > > > > > > > during > > > > > > >creation. > > > > > > > v3: > > > > > > > * New patch > > > > > > > > > > > > > > Ref: > > > > > > > > > > > > > > --- > > > > > > > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 6 +- > > > > > > > drivers/i2c/i2c-core-base.c | 92 > > > > > > > +--- > > > > > > > drivers/media/i2c/adv748x/adv748x-core.c | 2 +- > > > > > > > drivers/media/i2c/adv7604.c | 3 +- > > > > > > > include/linux/i2c.h | 3 +- > > > > > > > 5 files changed, 69 insertions(+), 37 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > > > > > > b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > > > > > > index ddceafa7b637..
Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Laurent,
On Wed, May 31, 2023 at 10:59 AM Laurent Pinchart
wrote:
> On Mon, May 29, 2023 at 09:00:43AM +, Biju Das wrote:
> > > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
> > > And why do you need this ?
> >
> > As per Krzysztof [2],
> >
> > The DT schema allows multiple addresses for children. But we lack
> > implementation of parent child relationship, As parent owns the resources.
> > Child device needs to parse parent node to get some resource
> > like clocks.
> >
> > [2]
> > https://lore.kernel.org/linux-renesas-soc/tycpr01mb5933bffd4eb556f5fb4ea82186...@tycpr01mb5933.jpnprd01.prod.outlook.com/
>
> The I2C ancillary clients are not meant to be handled by separate
> drivers. You're supposed to have one device node in DT, which causes the
> I2C core to instantiate a main i2c_client, and bind it to one driver.
> That driver then uses i2c_new_ancillary_device() to create other
> i2c_client instances for the secondary I2C addresses. Those i2c_client
> instances are not bound to a separate driver, so there should be no code
> that needs to look at the parent for resources.
In Biju's particular use case, the i2c device responds to two addresses,
which is the standard i2c ancillary use case. However, what's special
is that the second instance is a derivative of an existing i2c device
with an existing Linux driver. Hence the desire to make the existing
driver match against the second instance, which requires these changes
to i2c_new_ancillary_device().
As some resources are shared (knowledge about the clocks), splitting
this in two distinct devices in DT (which is what Biju's initial patch
series did) would need phandles to link both nodes together.
Do you have a better idea how to represent this?
Wolfram: time to chime in ;-)
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Laurent, Thanks for the feedback. > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API > > Hi Biju, > > On Wed, May 31, 2023 at 09:34:06AM +, Biju Das wrote: > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device > > > API On Mon, May 29, 2023 at 09:00:43AM +, Biju Das wrote: > > > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance > > > > > i2c_new_ancillary_device API On Mon, May 22, 2023 at 11:18:39AM > +0100, Biju Das wrote: > > > > > > Renesas PMIC RAA215300 exposes two separate i2c devices, one > > > > > > for the main device and another for rtc device. > > > > > > > > > > > > Enhance i2c_new_ancillary_device() to instantiate a real > device. > > > > > > > > > > Doesn't it already instantiate a real device ? > > > > > > > And that function calls i2c_new_client_device(), which allocates a > > > struct i2c_client that embeds a struct device, and registers that > > > device with the kernel device core. How is that different, in terms > > > of instantiating a "real device", from what this patch does ? > > > > There is a difference, right? it instantiates new "i2c dummy client" > > driver and a "real i2c client device" driver like rtc device > client(rtc-isl2108)?? > > I don't see how there's a difference in behaviour in the code you have > implemented, sorry. Will reword, Enhance i2c_new_ancillary_device() to instantiate a "I2C client device" apart from the already supported "I2C dummy client device". > > > > > > > (eg: Instantiate rtc device from PMIC driver) > > > > > > > > > > > > Added helper function __i2c_new_dummy_device to share the code > > > > > > between i2c_new_dummy_device and i2c_new_ancillary_device(). > > > > > > > > > > > > Also added helper function __i2c_new_client_device() to pass > > > > > > parent dev parameter, so that the ancillary device can assign > > > > > > its parent during creation. > > > > > > > > > > > > Suggested-by: Geert Uytterhoeven > > > > > > Signed-off-by: Biju Das > > > > > > --- > > > > > > v4->v5: > > > > > > * Replaced parameter dev->parent in __i2c_new_client_device() > and > > > > > >__i2c_new_dummy_device(). > > > > > > * Improved error message in __i2c_new_dummy_device() by > printing device name. > > > > > > * Updated comment for ancillary's device parent > > > > > > * Dropped aux_device_name check in > i2c_new_ancillary_device(). > > > > > > v3->v4: > > > > > > * Dropped Rb tag from Geert as there are new changes. > > > > > > * Introduced __i2c_new_dummy_device() to share the code > between > > > > > >i2c_new_dummy_device and i2c_new_ancillary_device(). > > > > > > * Introduced __i2c_new_client_device() to pass parent dev > > > > > >parameter, so that the ancillary device can assign its > parent during > > > > > >creation. > > > > > > v3: > > > > > > * New patch > > > > > > > > > > > > Ref: > > > > > > > > > > > > --- > > > > > > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 6 +- > > > > > > drivers/i2c/i2c-core-base.c | 92 > +-- > > > > > > > > - > > > > > > drivers/media/i2c/adv748x/adv748x-core.c | 2 +- > > > > > > drivers/media/i2c/adv7604.c | 3 +- > > > > > > include/linux/i2c.h | 3 +- > > > > > > 5 files changed, 69 insertions(+), 37 deletions(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > > > > > b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > > > > > index ddceafa7b637..86306b010a0a 100644 > > > > > > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > > > > > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > > > > > @@ -1072,7 +1072,7 @@ static int > > > > > > adv7511_init_cec_regmap(struct > > > adv7511 *adv) > > > > > > int ret; > > > >
Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Biju, On Wed, May 31, 2023 at 09:34:06AM +, Biju Das wrote: > > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API > > On Mon, May 29, 2023 at 09:00:43AM +, Biju Das wrote: > > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device > > > > API On Mon, May 22, 2023 at 11:18:39AM +0100, Biju Das wrote: > > > > > Renesas PMIC RAA215300 exposes two separate i2c devices, one for > > > > > the main device and another for rtc device. > > > > > > > > > > Enhance i2c_new_ancillary_device() to instantiate a real device. > > > > > > > > Doesn't it already instantiate a real device ? > > > > > And that function calls i2c_new_client_device(), which allocates a > > struct i2c_client that embeds a struct device, and registers that device > > with the kernel device core. How is that different, in terms of > > instantiating a "real device", from what this patch does ? > > There is a difference, right? it instantiates new "i2c dummy client" driver > and > a "real i2c client device" driver like rtc device client(rtc-isl2108)?? I don't see how there's a difference in behaviour in the code you have implemented, sorry. > > > > > (eg: Instantiate rtc device from PMIC driver) > > > > > > > > > > Added helper function __i2c_new_dummy_device to share the code > > > > > between i2c_new_dummy_device and i2c_new_ancillary_device(). > > > > > > > > > > Also added helper function __i2c_new_client_device() to pass > > > > > parent dev parameter, so that the ancillary device can assign its > > > > > parent during creation. > > > > > > > > > > Suggested-by: Geert Uytterhoeven > > > > > Signed-off-by: Biju Das > > > > > --- > > > > > v4->v5: > > > > > * Replaced parameter dev->parent in __i2c_new_client_device() and > > > > >__i2c_new_dummy_device(). > > > > > * Improved error message in __i2c_new_dummy_device() by printing > > > > > device name. > > > > > * Updated comment for ancillary's device parent > > > > > * Dropped aux_device_name check in i2c_new_ancillary_device(). > > > > > v3->v4: > > > > > * Dropped Rb tag from Geert as there are new changes. > > > > > * Introduced __i2c_new_dummy_device() to share the code between > > > > >i2c_new_dummy_device and i2c_new_ancillary_device(). > > > > > * Introduced __i2c_new_client_device() to pass parent dev > > > > >parameter, so that the ancillary device can assign its parent > > > > > during > > > > >creation. > > > > > v3: > > > > > * New patch > > > > > > > > > > Ref: > > > > > > > > > > --- > > > > > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 6 +- > > > > > drivers/i2c/i2c-core-base.c | 92 +-- > > > > > > - > > > > > drivers/media/i2c/adv748x/adv748x-core.c | 2 +- > > > > > drivers/media/i2c/adv7604.c | 3 +- > > > > > include/linux/i2c.h | 3 +- > > > > > 5 files changed, 69 insertions(+), 37 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > > > > b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > > > > index ddceafa7b637..86306b010a0a 100644 > > > > > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > > > > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > > > > @@ -1072,7 +1072,7 @@ static int adv7511_init_cec_regmap(struct > > adv7511 *adv) > > > > > int ret; > > > > > > > > > > adv->i2c_cec = i2c_new_ancillary_device(adv->i2c_main, > > "cec", > > > > > - > > > > > ADV7511_CEC_I2C_ADDR_DEFAULT); > > > > > + ADV7511_CEC_I2C_ADDR_DEFAULT, NULL); > > > > > if (IS_ERR(adv->i2c_cec)) > > > > > return PTR_ERR(adv->i2c_cec); > > > > > > > > > > @@ -1261,7 +1261,7 @@ static int adv7511_probe(struct i2c_client > > *i2c) > > > > > adv7511_packet_disable(adv751
RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Laurent,
Thanks for the feedback.
> Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
>
> Hi Biju,
>
> On Mon, May 29, 2023 at 09:00:43AM +, Biju Das wrote:
> > > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device
> > > API On Mon, May 22, 2023 at 11:18:39AM +0100, Biju Das wrote:
> > > > Renesas PMIC RAA215300 exposes two separate i2c devices, one for
> > > > the main device and another for rtc device.
> > > >
> > > > Enhance i2c_new_ancillary_device() to instantiate a real device.
> > >
> > > Doesn't it already instantiate a real device ?
> >
> And that function calls i2c_new_client_device(), which allocates a
> struct i2c_client that embeds a struct device, and registers that device
> with the kernel device core. How is that different, in terms of
> instantiating a "real device", from what this patch does ?
There is a difference, right? it instantiates new "i2c dummy client" driver and
a "real i2c client device" driver like rtc device client(rtc-isl2108)??
>
> > > > (eg: Instantiate rtc device from PMIC driver)
> > > >
> > > > Added helper function __i2c_new_dummy_device to share the code
> > > > between i2c_new_dummy_device and i2c_new_ancillary_device().
> > > >
> > > > Also added helper function __i2c_new_client_device() to pass
> > > > parent dev parameter, so that the ancillary device can assign its
> > > > parent during creation.
> > > >
> > > > Suggested-by: Geert Uytterhoeven
> > > > Signed-off-by: Biju Das
> > > > ---
> > > > v4->v5:
> > > > * Replaced parameter dev->parent in __i2c_new_client_device() and
> > > >__i2c_new_dummy_device().
> > > > * Improved error message in __i2c_new_dummy_device() by printing
> device name.
> > > > * Updated comment for ancillary's device parent
> > > > * Dropped aux_device_name check in i2c_new_ancillary_device().
> > > > v3->v4:
> > > > * Dropped Rb tag from Geert as there are new changes.
> > > > * Introduced __i2c_new_dummy_device() to share the code between
> > > >i2c_new_dummy_device and i2c_new_ancillary_device().
> > > > * Introduced __i2c_new_client_device() to pass parent dev
> > > >parameter, so that the ancillary device can assign its parent
> during
> > > >creation.
> > > > v3:
> > > > * New patch
> > > >
> > > > Ref:
> > > >
> > > > ---
> > > > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 6 +-
> > > > drivers/i2c/i2c-core-base.c | 92 +--
>
> > > -
> > > > drivers/media/i2c/adv748x/adv748x-core.c | 2 +-
> > > > drivers/media/i2c/adv7604.c | 3 +-
> > > > include/linux/i2c.h | 3 +-
> > > > 5 files changed, 69 insertions(+), 37 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > > > b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > > > index ddceafa7b637..86306b010a0a 100644
> > > > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > > > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > > > @@ -1072,7 +1072,7 @@ static int adv7511_init_cec_regmap(struct
> adv7511 *adv)
> > > > int ret;
> > > >
> > > > adv->i2c_cec = i2c_new_ancillary_device(adv->i2c_main,
> "cec",
> > > > -
> > > > ADV7511_CEC_I2C_ADDR_DEFAULT);
> > > > + ADV7511_CEC_I2C_ADDR_DEFAULT, NULL);
> > > > if (IS_ERR(adv->i2c_cec))
> > > > return PTR_ERR(adv->i2c_cec);
> > > >
> > > > @@ -1261,7 +1261,7 @@ static int adv7511_probe(struct i2c_client
> *i2c)
> > > > adv7511_packet_disable(adv7511, 0x);
> > > >
> > > > adv7511->i2c_edid = i2c_new_ancillary_device(i2c, "edid",
> > > > - ADV7511_EDID_I2C_ADDR_DEFAULT);
> > > > + ADV7511_EDID_I2C_ADDR_DEFAULT,
> NULL);
> > > > if (IS_ERR(adv7511->i2c_edid)) {
> > > > ret = PTR_ERR(adv7511->i2c_edid);
> > > &g
Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Biju,
On Mon, May 29, 2023 at 09:00:43AM +, Biju Das wrote:
> > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
> > On Mon, May 22, 2023 at 11:18:39AM +0100, Biju Das wrote:
> > > Renesas PMIC RAA215300 exposes two separate i2c devices, one for the
> > > main device and another for rtc device.
> > >
> > > Enhance i2c_new_ancillary_device() to instantiate a real device.
> >
> > Doesn't it already instantiate a real device ?
>
> No, as per the code it instantiates dummy device[1]
> [1]
> https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-core-base.c#L1156
And that function calls i2c_new_client_device(), which allocates a
struct i2c_client that embeds a struct device, and registers that device
with the kernel device core. How is that different, in terms of
instantiating a "real device", from what this patch does ?
> > > (eg: Instantiate rtc device from PMIC driver)
> > >
> > > Added helper function __i2c_new_dummy_device to share the code between
> > > i2c_new_dummy_device and i2c_new_ancillary_device().
> > >
> > > Also added helper function __i2c_new_client_device() to pass parent
> > > dev parameter, so that the ancillary device can assign its parent
> > > during creation.
> > >
> > > Suggested-by: Geert Uytterhoeven
> > > Signed-off-by: Biju Das
> > > ---
> > > v4->v5:
> > > * Replaced parameter dev->parent in __i2c_new_client_device() and
> > >__i2c_new_dummy_device().
> > > * Improved error message in __i2c_new_dummy_device() by printing device
> > > name.
> > > * Updated comment for ancillary's device parent
> > > * Dropped aux_device_name check in i2c_new_ancillary_device().
> > > v3->v4:
> > > * Dropped Rb tag from Geert as there are new changes.
> > > * Introduced __i2c_new_dummy_device() to share the code between
> > >i2c_new_dummy_device and i2c_new_ancillary_device().
> > > * Introduced __i2c_new_client_device() to pass parent dev
> > >parameter, so that the ancillary device can assign its parent during
> > >creation.
> > > v3:
> > > * New patch
> > >
> > > Ref:
> > >
> > > ---
> > > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 6 +-
> > > drivers/i2c/i2c-core-base.c | 92 +--
> > -
> > > drivers/media/i2c/adv748x/adv748x-core.c | 2 +-
> > > drivers/media/i2c/adv7604.c | 3 +-
> > > include/linux/i2c.h | 3 +-
> > > 5 files changed, 69 insertions(+), 37 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > > b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > > index ddceafa7b637..86306b010a0a 100644
> > > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > > @@ -1072,7 +1072,7 @@ static int adv7511_init_cec_regmap(struct adv7511
> > > *adv)
> > > int ret;
> > >
> > > adv->i2c_cec = i2c_new_ancillary_device(adv->i2c_main, "cec",
> > > - ADV7511_CEC_I2C_ADDR_DEFAULT);
> > > + ADV7511_CEC_I2C_ADDR_DEFAULT, NULL);
> > > if (IS_ERR(adv->i2c_cec))
> > > return PTR_ERR(adv->i2c_cec);
> > >
> > > @@ -1261,7 +1261,7 @@ static int adv7511_probe(struct i2c_client *i2c)
> > > adv7511_packet_disable(adv7511, 0x);
> > >
> > > adv7511->i2c_edid = i2c_new_ancillary_device(i2c, "edid",
> > > - ADV7511_EDID_I2C_ADDR_DEFAULT);
> > > + ADV7511_EDID_I2C_ADDR_DEFAULT, NULL);
> > > if (IS_ERR(adv7511->i2c_edid)) {
> > > ret = PTR_ERR(adv7511->i2c_edid);
> > > goto uninit_regulators;
> > > @@ -1271,7 +1271,7 @@ static int adv7511_probe(struct i2c_client *i2c)
> > >adv7511->i2c_edid->addr << 1);
> > >
> > > adv7511->i2c_packet = i2c_new_ancillary_device(i2c, "packet",
> > > - ADV7511_PACKET_I2C_ADDR_DEFAULT);
> > > + ADV7511_PACKET_I2C_ADDR_DEFAULT, NULL);
> > > if (IS_ERR(adv7511->i2c_packet)) {
> > > ret = PTR_ERR(adv7511->i2c_packet);
> > > goto err_i2c_unregister_edid;
>
RE: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Laurent,
Thanks for the feedback.
> Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
>
> Hi Biju,
>
> Thank you for the patch.
>
> On Mon, May 22, 2023 at 11:18:39AM +0100, Biju Das wrote:
> > Renesas PMIC RAA215300 exposes two separate i2c devices, one for the
> > main device and another for rtc device.
> >
> > Enhance i2c_new_ancillary_device() to instantiate a real device.
>
> Doesn't it already instantiate a real device ?
No, as per the code it instantiates dummy device[1]
[1]
https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-core-base.c#L1156
>
> > (eg: Instantiate rtc device from PMIC driver)
> >
> > Added helper function __i2c_new_dummy_device to share the code between
> > i2c_new_dummy_device and i2c_new_ancillary_device().
> >
> > Also added helper function __i2c_new_client_device() to pass parent
> > dev parameter, so that the ancillary device can assign its parent
> > during creation.
> >
> > Suggested-by: Geert Uytterhoeven
> > Signed-off-by: Biju Das
> > ---
> > v4->v5:
> > * Replaced parameter dev->parent in __i2c_new_client_device() and
> >__i2c_new_dummy_device().
> > * Improved error message in __i2c_new_dummy_device() by printing
> device name.
> > * Updated comment for ancillary's device parent
> > * Dropped aux_device_name check in i2c_new_ancillary_device().
> > v3->v4:
> > * Dropped Rb tag from Geert as there are new changes.
> > * Introduced __i2c_new_dummy_device() to share the code between
> >i2c_new_dummy_device and i2c_new_ancillary_device().
> > * Introduced __i2c_new_client_device() to pass parent dev
> >parameter, so that the ancillary device can assign its parent
> during
> >creation.
> > v3:
> > * New patch
> >
> > Ref:
> >
> > ---
> > drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 6 +-
> > drivers/i2c/i2c-core-base.c | 92 +--
> -
> > drivers/media/i2c/adv748x/adv748x-core.c | 2 +-
> > drivers/media/i2c/adv7604.c | 3 +-
> > include/linux/i2c.h | 3 +-
> > 5 files changed, 69 insertions(+), 37 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > index ddceafa7b637..86306b010a0a 100644
> > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > @@ -1072,7 +1072,7 @@ static int adv7511_init_cec_regmap(struct
> adv7511 *adv)
> > int ret;
> >
> > adv->i2c_cec = i2c_new_ancillary_device(adv->i2c_main, "cec",
> > - ADV7511_CEC_I2C_ADDR_DEFAULT);
> > + ADV7511_CEC_I2C_ADDR_DEFAULT, NULL);
> > if (IS_ERR(adv->i2c_cec))
> > return PTR_ERR(adv->i2c_cec);
> >
> > @@ -1261,7 +1261,7 @@ static int adv7511_probe(struct i2c_client *i2c)
> > adv7511_packet_disable(adv7511, 0x);
> >
> > adv7511->i2c_edid = i2c_new_ancillary_device(i2c, "edid",
> > - ADV7511_EDID_I2C_ADDR_DEFAULT);
> > + ADV7511_EDID_I2C_ADDR_DEFAULT, NULL);
> > if (IS_ERR(adv7511->i2c_edid)) {
> > ret = PTR_ERR(adv7511->i2c_edid);
> > goto uninit_regulators;
> > @@ -1271,7 +1271,7 @@ static int adv7511_probe(struct i2c_client *i2c)
> > adv7511->i2c_edid->addr << 1);
> >
> > adv7511->i2c_packet = i2c_new_ancillary_device(i2c, "packet",
> > - ADV7511_PACKET_I2C_ADDR_DEFAULT);
> > + ADV7511_PACKET_I2C_ADDR_DEFAULT, NULL);
> > if (IS_ERR(adv7511->i2c_packet)) {
> > ret = PTR_ERR(adv7511->i2c_packet);
> > goto err_i2c_unregister_edid;
> > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> > index ae3af738b03f..3442aa80290f 100644
> > --- a/drivers/i2c/i2c-core-base.c
> > +++ b/drivers/i2c/i2c-core-base.c
> > @@ -893,24 +893,10 @@ int i2c_dev_irq_from_resources(const struct
> resource *resources,
> > return 0;
> > }
> >
> > -/**
> > - * i2c_new_client_device - instantiate an i2c device
> > - * @adap: the adapter managing the device
> > - * @info: describes one I2C device; bus_num is ignored
> > - * Context: can sleep
> > - *
> > - * Create an i
Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Hi Biju,
Thank you for the patch.
On Mon, May 22, 2023 at 11:18:39AM +0100, Biju Das wrote:
> Renesas PMIC RAA215300 exposes two separate i2c devices, one for the main
> device and another for rtc device.
>
> Enhance i2c_new_ancillary_device() to instantiate a real device.
Doesn't it already instantiate a real device ?
> (eg: Instantiate rtc device from PMIC driver)
>
> Added helper function __i2c_new_dummy_device to share the code
> between i2c_new_dummy_device and i2c_new_ancillary_device().
>
> Also added helper function __i2c_new_client_device() to pass parent dev
> parameter, so that the ancillary device can assign its parent during
> creation.
>
> Suggested-by: Geert Uytterhoeven
> Signed-off-by: Biju Das
> ---
> v4->v5:
> * Replaced parameter dev->parent in __i2c_new_client_device() and
>__i2c_new_dummy_device().
> * Improved error message in __i2c_new_dummy_device() by printing device name.
> * Updated comment for ancillary's device parent
> * Dropped aux_device_name check in i2c_new_ancillary_device().
> v3->v4:
> * Dropped Rb tag from Geert as there are new changes.
> * Introduced __i2c_new_dummy_device() to share the code between
>i2c_new_dummy_device and i2c_new_ancillary_device().
> * Introduced __i2c_new_client_device() to pass parent dev
>parameter, so that the ancillary device can assign its parent during
>creation.
> v3:
> * New patch
>
> Ref:
>
> https://patchwork.kernel.org/project/linux-renesas-soc/patch/20230505172530.357455-5-biju.das...@bp.renesas.com/
> ---
> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 6 +-
> drivers/i2c/i2c-core-base.c | 92 +---
> drivers/media/i2c/adv748x/adv748x-core.c | 2 +-
> drivers/media/i2c/adv7604.c | 3 +-
> include/linux/i2c.h | 3 +-
> 5 files changed, 69 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index ddceafa7b637..86306b010a0a 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -1072,7 +1072,7 @@ static int adv7511_init_cec_regmap(struct adv7511 *adv)
> int ret;
>
> adv->i2c_cec = i2c_new_ancillary_device(adv->i2c_main, "cec",
> - ADV7511_CEC_I2C_ADDR_DEFAULT);
> + ADV7511_CEC_I2C_ADDR_DEFAULT, NULL);
> if (IS_ERR(adv->i2c_cec))
> return PTR_ERR(adv->i2c_cec);
>
> @@ -1261,7 +1261,7 @@ static int adv7511_probe(struct i2c_client *i2c)
> adv7511_packet_disable(adv7511, 0x);
>
> adv7511->i2c_edid = i2c_new_ancillary_device(i2c, "edid",
> - ADV7511_EDID_I2C_ADDR_DEFAULT);
> + ADV7511_EDID_I2C_ADDR_DEFAULT, NULL);
> if (IS_ERR(adv7511->i2c_edid)) {
> ret = PTR_ERR(adv7511->i2c_edid);
> goto uninit_regulators;
> @@ -1271,7 +1271,7 @@ static int adv7511_probe(struct i2c_client *i2c)
>adv7511->i2c_edid->addr << 1);
>
> adv7511->i2c_packet = i2c_new_ancillary_device(i2c, "packet",
> - ADV7511_PACKET_I2C_ADDR_DEFAULT);
> + ADV7511_PACKET_I2C_ADDR_DEFAULT, NULL);
> if (IS_ERR(adv7511->i2c_packet)) {
> ret = PTR_ERR(adv7511->i2c_packet);
> goto err_i2c_unregister_edid;
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index ae3af738b03f..3442aa80290f 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -893,24 +893,10 @@ int i2c_dev_irq_from_resources(const struct resource
> *resources,
> return 0;
> }
>
> -/**
> - * i2c_new_client_device - instantiate an i2c device
> - * @adap: the adapter managing the device
> - * @info: describes one I2C device; bus_num is ignored
> - * Context: can sleep
> - *
> - * Create an i2c device. Binding is handled through driver model
> - * probe()/remove() methods. A driver may be bound to this device when we
> - * return from this function, or any later moment (e.g. maybe hotplugging
> will
> - * load the driver module). This call is not appropriate for use by
> mainboard
> - * initialization logic, which usually runs during an arch_initcall() long
> - * before any i2c_adapter could exist.
> - *
> - * This returns the new i2c client, which may be saved for later use with
> - * i2c_unregister_device(); or an ERR_PTR to describe the error.
> - */
> -struct i2c_client *
> -i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const
> *info)
> +static struct i2c_client *
> +__i2c_new_client_device(struct i2c_adapter *adap,
> + struct i2c_board_info const *info,
> + struct device *parent)
> {
> struct i2c_client *client;
> int
Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
On Mon, May 22, 2023 at 12:19 PM Biju Das wrote:
> Renesas PMIC RAA215300 exposes two separate i2c devices, one for the main
> device and another for rtc device.
>
> Enhance i2c_new_ancillary_device() to instantiate a real device.
> (eg: Instantiate rtc device from PMIC driver)
>
> Added helper function __i2c_new_dummy_device to share the code
> between i2c_new_dummy_device and i2c_new_ancillary_device().
>
> Also added helper function __i2c_new_client_device() to pass parent dev
> parameter, so that the ancillary device can assign its parent during
> creation.
>
> Suggested-by: Geert Uytterhoeven
> Signed-off-by: Biju Das
> ---
> v4->v5:
> * Replaced parameter dev->parent in __i2c_new_client_device() and
>__i2c_new_dummy_device().
> * Improved error message in __i2c_new_dummy_device() by printing device name.
> * Updated comment for ancillary's device parent
> * Dropped aux_device_name check in i2c_new_ancillary_device().
Reviewed-by: Geert Uytterhoeven
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
On 22/05/2023 12:18, Biju Das wrote:
> Renesas PMIC RAA215300 exposes two separate i2c devices, one for the main
> device and another for rtc device.
>
> Enhance i2c_new_ancillary_device() to instantiate a real device.
> (eg: Instantiate rtc device from PMIC driver)
>
> Added helper function __i2c_new_dummy_device to share the code
> between i2c_new_dummy_device and i2c_new_ancillary_device().
>
> Also added helper function __i2c_new_client_device() to pass parent dev
> parameter, so that the ancillary device can assign its parent during
> creation.
>
> Suggested-by: Geert Uytterhoeven
> Signed-off-by: Biju Das
> ---
> v4->v5:
> * Replaced parameter dev->parent in __i2c_new_client_device() and
>__i2c_new_dummy_device().
> * Improved error message in __i2c_new_dummy_device() by printing device name.
> * Updated comment for ancillary's device parent
> * Dropped aux_device_name check in i2c_new_ancillary_device().
> v3->v4:
> * Dropped Rb tag from Geert as there are new changes.
> * Introduced __i2c_new_dummy_device() to share the code between
>i2c_new_dummy_device and i2c_new_ancillary_device().
> * Introduced __i2c_new_client_device() to pass parent dev
>parameter, so that the ancillary device can assign its parent during
>creation.
> v3:
> * New patch
>
> Ref:
>
> https://patchwork.kernel.org/project/linux-renesas-soc/patch/20230505172530.357455-5-biju.das...@bp.renesas.com/
> ---
> drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 6 +-
> drivers/i2c/i2c-core-base.c | 92 +---
> drivers/media/i2c/adv748x/adv748x-core.c | 2 +-
> drivers/media/i2c/adv7604.c | 3 +-
> include/linux/i2c.h | 3 +-
> 5 files changed, 69 insertions(+), 37 deletions(-)
For the media and adv7511 bits:
Reviewed-by: Hans Verkuil
Regards,
Hans
>
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index ddceafa7b637..86306b010a0a 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -1072,7 +1072,7 @@ static int adv7511_init_cec_regmap(struct adv7511 *adv)
> int ret;
>
> adv->i2c_cec = i2c_new_ancillary_device(adv->i2c_main, "cec",
> - ADV7511_CEC_I2C_ADDR_DEFAULT);
> + ADV7511_CEC_I2C_ADDR_DEFAULT, NULL);
> if (IS_ERR(adv->i2c_cec))
> return PTR_ERR(adv->i2c_cec);
>
> @@ -1261,7 +1261,7 @@ static int adv7511_probe(struct i2c_client *i2c)
> adv7511_packet_disable(adv7511, 0x);
>
> adv7511->i2c_edid = i2c_new_ancillary_device(i2c, "edid",
> - ADV7511_EDID_I2C_ADDR_DEFAULT);
> + ADV7511_EDID_I2C_ADDR_DEFAULT, NULL);
> if (IS_ERR(adv7511->i2c_edid)) {
> ret = PTR_ERR(adv7511->i2c_edid);
> goto uninit_regulators;
> @@ -1271,7 +1271,7 @@ static int adv7511_probe(struct i2c_client *i2c)
>adv7511->i2c_edid->addr << 1);
>
> adv7511->i2c_packet = i2c_new_ancillary_device(i2c, "packet",
> - ADV7511_PACKET_I2C_ADDR_DEFAULT);
> + ADV7511_PACKET_I2C_ADDR_DEFAULT, NULL);
> if (IS_ERR(adv7511->i2c_packet)) {
> ret = PTR_ERR(adv7511->i2c_packet);
> goto err_i2c_unregister_edid;
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index ae3af738b03f..3442aa80290f 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -893,24 +893,10 @@ int i2c_dev_irq_from_resources(const struct resource
> *resources,
> return 0;
> }
>
> -/**
> - * i2c_new_client_device - instantiate an i2c device
> - * @adap: the adapter managing the device
> - * @info: describes one I2C device; bus_num is ignored
> - * Context: can sleep
> - *
> - * Create an i2c device. Binding is handled through driver model
> - * probe()/remove() methods. A driver may be bound to this device when we
> - * return from this function, or any later moment (e.g. maybe hotplugging
> will
> - * load the driver module). This call is not appropriate for use by
> mainboard
> - * initialization logic, which usually runs during an arch_initcall() long
> - * before any i2c_adapter could exist.
> - *
> - * This returns the new i2c client, which may be saved for later use with
> - * i2c_unregister_device(); or an ERR_PTR to describe the error.
> - */
> -struct i2c_client *
> -i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const
> *info)
> +static struct i2c_client *
> +__i2c_new_client_device(struct i2c_adapter *adap,
> + struct i2c_board_info const *info,
> + struct device *parent)
> {
> struct i2c_client *client;
> int status;
> @@
[PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device API
Renesas PMIC RAA215300 exposes two separate i2c devices, one for the main
device and another for rtc device.
Enhance i2c_new_ancillary_device() to instantiate a real device.
(eg: Instantiate rtc device from PMIC driver)
Added helper function __i2c_new_dummy_device to share the code
between i2c_new_dummy_device and i2c_new_ancillary_device().
Also added helper function __i2c_new_client_device() to pass parent dev
parameter, so that the ancillary device can assign its parent during
creation.
Suggested-by: Geert Uytterhoeven
Signed-off-by: Biju Das
---
v4->v5:
* Replaced parameter dev->parent in __i2c_new_client_device() and
__i2c_new_dummy_device().
* Improved error message in __i2c_new_dummy_device() by printing device name.
* Updated comment for ancillary's device parent
* Dropped aux_device_name check in i2c_new_ancillary_device().
v3->v4:
* Dropped Rb tag from Geert as there are new changes.
* Introduced __i2c_new_dummy_device() to share the code between
i2c_new_dummy_device and i2c_new_ancillary_device().
* Introduced __i2c_new_client_device() to pass parent dev
parameter, so that the ancillary device can assign its parent during
creation.
v3:
* New patch
Ref:
https://patchwork.kernel.org/project/linux-renesas-soc/patch/20230505172530.357455-5-biju.das...@bp.renesas.com/
---
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 6 +-
drivers/i2c/i2c-core-base.c | 92 +---
drivers/media/i2c/adv748x/adv748x-core.c | 2 +-
drivers/media/i2c/adv7604.c | 3 +-
include/linux/i2c.h | 3 +-
5 files changed, 69 insertions(+), 37 deletions(-)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index ddceafa7b637..86306b010a0a 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -1072,7 +1072,7 @@ static int adv7511_init_cec_regmap(struct adv7511 *adv)
int ret;
adv->i2c_cec = i2c_new_ancillary_device(adv->i2c_main, "cec",
- ADV7511_CEC_I2C_ADDR_DEFAULT);
+ ADV7511_CEC_I2C_ADDR_DEFAULT, NULL);
if (IS_ERR(adv->i2c_cec))
return PTR_ERR(adv->i2c_cec);
@@ -1261,7 +1261,7 @@ static int adv7511_probe(struct i2c_client *i2c)
adv7511_packet_disable(adv7511, 0x);
adv7511->i2c_edid = i2c_new_ancillary_device(i2c, "edid",
- ADV7511_EDID_I2C_ADDR_DEFAULT);
+ ADV7511_EDID_I2C_ADDR_DEFAULT, NULL);
if (IS_ERR(adv7511->i2c_edid)) {
ret = PTR_ERR(adv7511->i2c_edid);
goto uninit_regulators;
@@ -1271,7 +1271,7 @@ static int adv7511_probe(struct i2c_client *i2c)
adv7511->i2c_edid->addr << 1);
adv7511->i2c_packet = i2c_new_ancillary_device(i2c, "packet",
- ADV7511_PACKET_I2C_ADDR_DEFAULT);
+ ADV7511_PACKET_I2C_ADDR_DEFAULT, NULL);
if (IS_ERR(adv7511->i2c_packet)) {
ret = PTR_ERR(adv7511->i2c_packet);
goto err_i2c_unregister_edid;
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index ae3af738b03f..3442aa80290f 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -893,24 +893,10 @@ int i2c_dev_irq_from_resources(const struct resource
*resources,
return 0;
}
-/**
- * i2c_new_client_device - instantiate an i2c device
- * @adap: the adapter managing the device
- * @info: describes one I2C device; bus_num is ignored
- * Context: can sleep
- *
- * Create an i2c device. Binding is handled through driver model
- * probe()/remove() methods. A driver may be bound to this device when we
- * return from this function, or any later moment (e.g. maybe hotplugging will
- * load the driver module). This call is not appropriate for use by mainboard
- * initialization logic, which usually runs during an arch_initcall() long
- * before any i2c_adapter could exist.
- *
- * This returns the new i2c client, which may be saved for later use with
- * i2c_unregister_device(); or an ERR_PTR to describe the error.
- */
-struct i2c_client *
-i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const
*info)
+static struct i2c_client *
+__i2c_new_client_device(struct i2c_adapter *adap,
+ struct i2c_board_info const *info,
+ struct device *parent)
{
struct i2c_client *client;
int status;
@@ -944,7 +930,7 @@ i2c_new_client_device(struct i2c_adapter *adap, struct
i2c_board_info const *inf
if (status)
goto out_err;
- client->dev.parent = >adapter->dev;
+ client->dev.parent = parent ? parent : >adapter->dev;
client->dev.bus = _bus_type;
