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

[Hans de Goede]

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