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

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

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 <biju.das.jz@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:
>   1. The device has a single interrupt pin.  Is there any interaction
>      or coordination between PMIC and RTC interrupts?

PMIC has Fault status registers for Bucks/LDOs, so this has to be handled in PMIC driver
once we add regulator/INT# support as clearing of latch registers are in PMIC block. RTC interrupt should be handled by RTC as clearing of ALRM bit is in RTC block.

Currently when I enabled PMIC_INT# for RTC, I got IRQ storm during boot due to latched registers. RT

    /* Clear all except RTC */
    regmap_read(pmic->regmap, 0x6c, &val);
    val &= BIT(6);
    regmap_write(pmic->regmap, 0x6c, val);

 

    /*Clear latched registers */
    regmap_read(pmic->regmap, 0x59, &val);
    regmap_write(pmic->regmap, 0x59, val);
    regmap_read(pmic->regmap, 0x5e, &val);
    regmap_write(pmic->regmap, 0x5e, val);

 

    regmap_write(pmic->regmap, 0x64, 0x3f);
    regmap_write(pmic->regmap, 0x65, 0x0f);
    regmap_write(pmic->regmap, 0x66, 0x3f);
    regmap_write(pmic->regmap, 0x67, 0x01);
    regmap_write(pmic->regmap, 0x68, 0xff);
    regmap_write(pmic->regmap, 0x69, 0x1f);


The RAA215300 has various monitors, warnings, and fault protection features.
If a fault is detected during normal operation, both a latched (sticky) and a live fault status bit are set. INT# is
asserted if the fault interrupt is supported and not masked out. Certain fault events can be configured to shut down
all rails (enter {FAULT_OUT}), or to keep all rails operating (do not enter {FAULT_OUT}). A latched fault bit
remains set until cleared by the host writing a 1 to the latched register bit after the event has subsided. The live
status bits show the real-time condition and are used to indicate if the fault has subsided or persists. For more
information see Interrupt and Fault and Status Monitoring.
If a fault event shuts down the RAA215300 power rails, all the reset outputs are asserted and the output rails are powered down following the power-off sequence.

>   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.

>   2. Does the battery charger circuit in the PMIC impact the VBAT
>      input of the RTC?

There are two power supply inputs for the RTC circuit (VCHG and VBAT). The RAA215300 contains internal circuitry to automatically switch over to the backup battery when the main VCHG supply fails and switches back from the battery to VCHG when the main supply recovers.

>   3. Are there other I2C addresses the chip listens to?

No, only 2 address 0x12 and 0x6f.

> 
> I only have access to the Short-Form Datasheet for the RAA215300, so I
> cannot check myself...

I will ask Chris to share the details.

Cheers,
Biju