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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fea7ce9a-01a9-cab8-8675-be5c44cb8a27@redhat.com>
Date:   Tue, 23 Feb 2021 20:25:35 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/1] i2c: cht-wc: Use fwnode for the controller and IRQ
 domain

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