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@xxxxxxxxxxxxxxx>
> ---
>
> 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
