Hi Biju,
On Wed, Jun 14, 2023 at 1:04 PM Biju Das <biju.das...@bp.renesas.com> 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 <biju.das...@bp.renesas.com>
> > wrote:
> > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance i2c_new_ancillary_device
> > > > API On Tue, Jun 13, 2023 at 07:31:46PM +0000, Biju Das wrote:
> > > > > > Subject: RE: [PATCH v5 01/11] i2c: Enhance
> > > > > > i2c_new_ancillary_device API
> > > > > > > Subject: RE: [PATCH v5 01/11] i2c: Enhance
> > > > > > > i2c_new_ancillary_device API
> > > > > > > > Subject: Re: [PATCH v5 01/11] i2c: Enhance
> > > > > > > > i2c_new_ancillary_device API
> > > > > > > >
> > > > > > > > Hi everyone,
> > > > > > > >
> > > > > > > > > Perhaps we should first think through what an ancillary
> > > > > > > > > device really is. My understanding is that it is used to
> > > > > > > > > talk to secondary addresses of a multi-address I2C slave
> > device.
> > > > > > > >
> > > > > > > > As I mentioned somewhere before, this is not the case.
> > > > > > > > Ancillary devices are when one *driver* handles more than one
> > address.
> > > > > > > > Everything else has been handled differently in the past
> > > > > > > > (for all the uses I am aware of).
> > > > > > > >
> > > > > > > > Yet, I have another idea which is so simple that I wonder if
> > > > > > > > it maybe has already been discussed so far?
> > > > > > > >
> > > > > > > > * have two regs in the bindings
> > > > > > >
> > > > > > > OK, it is inline with DT maintainers expectation as it is
> > > > > > > matching with real hw as single device node having two regs.
> > > > > > >
> > > > > > > > * use the second reg with i2c_new_client_device to instantiate
> > the
> > > > > > > > RTC sibling. 'struct i2c_board_info', which is one
> > > > > > > > parameter,
> > > > should
> > > > > > > > have enough options to pass data, e.g it has a
> > software_node.
> > > > > > >
> > > > > > > OK, I can see the below can be passed from PMIC to new client
> > > > device.
> > > > > > >
> > > > > > > client->addr = info->addr;
> > > > > > >
> > > > > > > client->init_irq = info->irq;
> > > > > > >
> > > > > > > >
> > > > > > > > Should work or did I miss something here?
> > > > > > >
> > > > > > > I guess it will work. We instantiate appropriate device based
> > > > > > > On PMIC revision and slave address and IRQ resource passed
> > > > > > > through 'struct i2c_board_info'
> > > > > > >
> > > > > > > Will check this and update you.
> > > > > >
> > > > > > info.irq = irq; -->Irq fine
> > > > > > info.addr = addr; -->slave address fine size =
> > > > > > strscpy(info.type, name, sizeof(info.type)); -->instantiation
> > > > > > based on PMIC version fine.
> > > > > >
> > > > > > 1) How do we share clk details on instantiated device to find is
> > > > > > it connected to external crystal or external clock source? as we
> > > > > > cannot pass of_node between PMIC and "i2c_board_info" as it
> > > > > > results in pinctrl failure. info->platformdata and
> > > > > > Client->dev.platformdata to retrieve this info??
> > > > >
> > > > > Or
> > > > >
> > > > > I2C instantiation based on actual oscillator bit value, ie, two
> > > > > i2c_device_id's with one for setting oscillator bit and another
> > > > > for clearing oscillator bit
> > > > >
> > > > > PMIC driver parses the clock details. Based on firmware version
> > > > > and clock, It instantiates either i2c_device_id with setting
> > > > > oscillator bit or clearing oscillator bit.
> > > >
> > > > I don't like that hack. I still think that two DT nodes is the best
> > > > option, I think you're trying hard to hack around a problem that is
> > > > actually not a problem.
> > >
> > > Why do you think it is a hack? I believe rather it is actual solution
> > >
> > > PMIC is a single device, with 2 regs, clocks, pinctrl and IRQ
> > properties.
> > > So it will be represented as single node with single compatible.
> > >
> > > 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:
> > 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?
>
> The ISL1208 driver doesn't support clock output. It is same as ISL1208, but
> difference is
> since same INT# pin used for PMIC, I guess we won't be able to use PMIC
> interrupt, if RTC configured for clock output.
Exactly.
The documentation confirms it can also be configured as clock signal
output in Frequency Output (FOUT) mode of the RTC on RAA215300.
> > 3. Are there other I2C addresses the chip listens to?
>
> No, only 2 address 0x12 and 0x6f.
Both addresses are programmable, and can even be the same!
Interesting, but more challenging for the Linux driver model...
> > I only have access to the Short-Form Datasheet for the RAA215300, so I
> > cannot check myself...
Thanks, got access through the kind people behind the Renesas website.
Other things I noticed during a quick glance:
- To activate the RTC, the host must first set the RTC EN bit =
1 and the WRTC bit = 1.
The RTC EN bit is located in the PMIC address space.
IMHO this precludes using a separate DT node.
- ISL1208 supports 400 kHz I2C, RAA215300 supports 1 MHz.
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
