Re: [PATCH v1 1/1] i2c: cht-wc: Use fwnode for the controller and IRQ domain

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Tue, Feb 23, 2021 at 08:25:35PM +0100, Hans de Goede wrote:
> On 2/23/21 6:22 PM, Andy Shevchenko wrote:
> > It's better to describe the I²C controller and associated IRQ domain with
> > fwnode, so they will find their place in the hierarchy in sysfs and also
> > make easier to debug.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> > 
> > Hans, unfortunately I have no device at hand with INT34D3. This is only compile
> > tested in that sense. Also I would like to hear if you like the idea in general.
> > 
> >  drivers/i2c/busses/i2c-cht-wc.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-cht-wc.c b/drivers/i2c/busses/i2c-cht-wc.c
> > index f80d79e973cd..dbf55842b0dc 100644
> > --- a/drivers/i2c/busses/i2c-cht-wc.c
> > +++ b/drivers/i2c/busses/i2c-cht-wc.c
> > @@ -303,6 +303,7 @@ static struct bq24190_platform_data bq24190_pdata = {
> >  static int cht_wc_i2c_adap_i2c_probe(struct platform_device *pdev)
> >  {
> >  	struct intel_soc_pmic *pmic = dev_get_drvdata(pdev->dev.parent);
> > +	struct fwnode_handle *fwnode = dev_fwnode(&pdev->dev);
> 
> So this will point to the ACPi-companion fwnode of the CHT Whiskey Cove PMIC
> controller.

Right.

> >  	struct cht_wc_i2c_adap *adap;
> >  	struct i2c_board_info board_info = {
> >  		.type = "bq24190",
> > @@ -333,6 +334,7 @@ static int cht_wc_i2c_adap_i2c_probe(struct platform_device *pdev)
> >  	strlcpy(adap->adapter.name, "PMIC I2C Adapter",
> >  		sizeof(adap->adapter.name));
> >  	adap->adapter.dev.parent = &pdev->dev;
> > +	set_primary_fwnode(&adap->adapter.dev, fwnode);
> 
> So now we have the main PMIC device i2c-client, the platform-device instantiated
> for the MFD-cell for the PMIC's builtin I2C-controller; and the device instantiated
> for the adapter-device all 3 share the same ACPI-companion fwnode.

Okay, this step in this patch maybe not needed (or should be a separate change,
but I don't see clearly what would be the benefit out of it).

> >  	/* Clear and activate i2c-adapter interrupts, disable client IRQ */
> >  	adap->old_irq_mask = adap->irq_mask = ~CHT_WC_EXTCHGRIRQ_ADAP_IRQMASK;
> > @@ -350,8 +352,8 @@ static int cht_wc_i2c_adap_i2c_probe(struct platform_device *pdev)
> >  		return ret;
> >  
> >  	/* Alloc and register client IRQ */
> > -	adap->irq_domain = irq_domain_add_linear(pdev->dev.of_node, 1,
> > -						 &irq_domain_simple_ops, NULL);
> > +	adap->irq_domain = irq_domain_create_linear(fwnode, 1,
> > +						    &irq_domain_simple_ops, NULL);
> 
> Hmm, not sure this is right, admittedly the old code looks weird too, but now we
> are creating a second irq_domain at the same level as the irq_domain created for
> the IRQ-chip part of the PMIC. But this is really more of a child-domain of just
> the I2C-controller MFD-cell. The IRQ-CHIP part of the PMIC has a single IRQ for the
> I2C controller which gets raised both on i2c-transfer completions and when the
> pin on the PMIC which is reserved as input for the IRQ coming out of the charger-chip
> gets triggered.
> 
> IOW we have this:
> 
> 
>                PMIC
>                  |
>     ------------------------------
>     |       |        |           |
>    IRQ1   IRQ2      IRQ3       I2C-IRQ
>                                  |
>                    ----------------------------------
>                    |        |         |             |
>                  READIRQ   WRIRQ    NACKIRQ     CLIENT-IRQ
> 
> Where READIRQ, WRIRQ and NACKIRQ are directly consumed
> and the CLIENT-IRQ is being represented as a single IRQ on
> a new irqchip so that we can pass it along to the i2c-driver
> for the charger-chip which is connected to the Whiskey Cove's
> builtin I2C controller.
> 
> But doing as you suggest would model the IRQs as:
> 
>                PMIC
>                  |
>     --------------------------------------------------
>     |       |        |           |                    |
>    IRQ1   IRQ2      IRQ3       I2C-IRQ           CLIENT-IRQ
> 
> Which is not the same really. I guess it is better then what we
> have though ?

Hmm... There should not be difference in the hierarchy. add_linear ==
create_linear. The propagation of *device* (not an IRQ) fwnode is just
convenient way to have IRQ domain be named (instead of 'unknown-N' or so).
Maybe I have read __irq_domain_add() code wrongly.

Nevertheless, thinking more about it, why we don't add an IRQ chip via regmap
IRQ API?

> Note I can test any changes made here, but I'm not 100% convinced that
> the current version of this patch is correct.

If we settle on the idea first. I'm (slowly) looking forward to check another
CherryTrail device we have at the lab, but we lack of some (power) equipment
right now to setup it properly. I hope it may have the Whiskey Cove PMIC there.

-- 
With Best Regards,
Andy Shevchenko