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

Message ID 20210223172231.2224-1-andriy.shevchenko@linux.intel.com
State New
Headers show
Series
  • [v1,1/1] i2c: cht-wc: Use fwnode for the controller and IRQ domain
Related show

Commit Message

Andy Shevchenko Feb. 23, 2021, 5:22 p.m.
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(-)

Comments

Hans de Goede Feb. 23, 2021, 7:25 p.m. | #1
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
Andy Shevchenko Feb. 24, 2021, 12:51 p.m. | #2
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
Hans de Goede Feb. 24, 2021, 7:12 p.m. | #3
Hi,

On 2/24/21 1:51 PM, Andy Shevchenko wrote:
> 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.


Sorry, this is probably my bad. The first ASCII-art which I posted is
how things actually work in HW. The second one is how I assumed that
things would look like in some nested representation of the IRQ-domains
given that all the IRQs mentioned in the ASCII-art now use the same fwnode
as parent for their domain. But poking around in sysfs I don't see any
hierarchical representation of the domains at all. Actually I cannot
find any representation of the IRQ domains inside sysfs (I've never
looked at / into this before) ?

If what you say is right and the fwnode is only used to set a name (where can
I see those names ?) then your patch is probably correct.

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

> IRQ API?


There already is a regmap IRQ chip associated with the MFD device and the
IRQ handling required here is somewhat tricky (see the comments in the driver)
so I would prefer to keep this as is.

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


More testing is always welcome :)   With that said, testing these changes really
is not a lot of work for me.

Regards,

Hans
Andy Shevchenko Feb. 25, 2021, 3:44 p.m. | #4
On Thu, Feb 25, 2021 at 5:11 AM Hans de Goede <hdegoede@redhat.com> wrote:
> On 2/24/21 1:51 PM, Andy Shevchenko wrote:

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


...

> >>> +   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).


Shall I leave this or should be removed in v2?

...

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

>

> Sorry, this is probably my bad. The first ASCII-art which I posted is

> how things actually work in HW. The second one is how I assumed that

> things would look like in some nested representation of the IRQ-domains

> given that all the IRQs mentioned in the ASCII-art now use the same fwnode

> as parent for their domain. But poking around in sysfs I don't see any

> hierarchical representation of the domains at all. Actually I cannot

> find any representation of the IRQ domains inside sysfs (I've never

> looked at / into this before) ?


I have enabled  GENERIC_IRQ_DEBUGFS to see some information.

> If what you say is right and the fwnode is only used to set a name (where can

> I see those names ?) then your patch is probably correct.


I have checked again and I don't see anything except it uses it as a
domain name and takes reference count.

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

> > IRQ API?

>

> There already is a regmap IRQ chip associated with the MFD device and the

> IRQ handling required here is somewhat tricky (see the comments in the driver)

> so I would prefer to keep this as is.


Ah, that makes things complicated a bit.

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

>

> More testing is always welcome :)   With that said, testing these changes really

> is not a lot of work for me.


I would expect that we will have a clash with IRQ domain names and
thus we would need our own fwnode here.

I will think about it, but it sounds like we need to create a
hierarchy of the IRQ domains and take the device's fwnode as a parent
here.

Overall, I stumbled over of_node use in pure ACPI case (simplest "fix"
is to provide a NULL pointer there). If you think we can get rid of
of_node as intermediate step, I will send v2 with that.



-- 
With Best Regards,
Andy Shevchenko
Hans de Goede April 23, 2021, 5:33 p.m. | #5
Hi,

On 2/25/21 4:44 PM, Andy Shevchenko wrote:
> On Thu, Feb 25, 2021 at 5:11 AM Hans de Goede <hdegoede@redhat.com> wrote:

>> On 2/24/21 1:51 PM, Andy Shevchenko wrote:

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

> 

> ...

> 

>>>>> +   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).

> 

> Shall I leave this or should be removed in v2?

> 

> ...

> 

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

>>

>> Sorry, this is probably my bad. The first ASCII-art which I posted is

>> how things actually work in HW. The second one is how I assumed that

>> things would look like in some nested representation of the IRQ-domains

>> given that all the IRQs mentioned in the ASCII-art now use the same fwnode

>> as parent for their domain. But poking around in sysfs I don't see any

>> hierarchical representation of the domains at all. Actually I cannot

>> find any representation of the IRQ domains inside sysfs (I've never

>> looked at / into this before) ?

> 

> I have enabled  GENERIC_IRQ_DEBUGFS to see some information.

> 

>> If what you say is right and the fwnode is only used to set a name (where can

>> I see those names ?) then your patch is probably correct.

> 

> I have checked again and I don't see anything except it uses it as a

> domain name and takes reference count.

> 

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

>>> IRQ API?

>>

>> There already is a regmap IRQ chip associated with the MFD device and the

>> IRQ handling required here is somewhat tricky (see the comments in the driver)

>> so I would prefer to keep this as is.

> 

> Ah, that makes things complicated a bit.

> 

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

>>

>> More testing is always welcome :)   With that said, testing these changes really

>> is not a lot of work for me.

> 

> I would expect that we will have a clash with IRQ domain names and

> thus we would need our own fwnode here.

> 

> I will think about it, but it sounds like we need to create a

> hierarchy of the IRQ domains and take the device's fwnode as a parent

> here.

> 

> Overall, I stumbled over of_node use in pure ACPI case (simplest "fix"

> is to provide a NULL pointer there). If you think we can get rid of

> of_node as intermediate step, I will send v2 with that.


Sorry for being slow to respond.

I agree that the of_node use is weird, so a patch which simply replaces the
pdev->dev.of_node with NULL would be good. Otherwise I would just leave the
code as is.

Regards,

Hans

Patch

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);
 	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);
 
 	/* 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);
 	if (!adap->irq_domain)
 		return -ENOMEM;