Messages in this thread | | | Subject | Re: [PATCH v1 1/1] i2c: cht-wc: Use fwnode for the controller and IRQ domain | From | Hans de Goede <> | Date | Tue, 23 Feb 2021 20:25:35 +0100 |
| |
Hi,
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.
> 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. > > /* 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 ?
Note I can test any changes made here, but I'm not 100% convinced that the current version of this patch is correct.
Regards,
Hans
| |