* [PATCH v1 1/1] i2c: cht-wc: Use fwnode for the controller and IRQ domain @ 2021-02-23 17:22 Andy Shevchenko 2021-02-23 19:25 ` Hans de Goede 0 siblings, 1 reply; 6+ messages in thread From: Andy Shevchenko @ 2021-02-23 17:22 UTC (permalink / raw) To: Andy Shevchenko, linux-i2c, linux-kernel; +Cc: Hans de Goede 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); 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; -- 2.30.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v1 1/1] i2c: cht-wc: Use fwnode for the controller and IRQ domain 2021-02-23 17:22 [PATCH v1 1/1] i2c: cht-wc: Use fwnode for the controller and IRQ domain Andy Shevchenko @ 2021-02-23 19:25 ` Hans de Goede 2021-02-24 12:51 ` Andy Shevchenko 0 siblings, 1 reply; 6+ messages in thread From: Hans de Goede @ 2021-02-23 19:25 UTC (permalink / raw) To: Andy Shevchenko, linux-i2c, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1 1/1] i2c: cht-wc: Use fwnode for the controller and IRQ domain 2021-02-23 19:25 ` Hans de Goede @ 2021-02-24 12:51 ` Andy Shevchenko 2021-02-24 19:12 ` Hans de Goede 0 siblings, 1 reply; 6+ messages in thread From: Andy Shevchenko @ 2021-02-24 12:51 UTC (permalink / raw) To: Hans de Goede; +Cc: linux-i2c, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1 1/1] i2c: cht-wc: Use fwnode for the controller and IRQ domain 2021-02-24 12:51 ` Andy Shevchenko @ 2021-02-24 19:12 ` Hans de Goede 2021-02-25 15:44 ` Andy Shevchenko 0 siblings, 1 reply; 6+ messages in thread From: Hans de Goede @ 2021-02-24 19:12 UTC (permalink / raw) To: Andy Shevchenko; +Cc: linux-i2c, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1 1/1] i2c: cht-wc: Use fwnode for the controller and IRQ domain 2021-02-24 19:12 ` Hans de Goede @ 2021-02-25 15:44 ` Andy Shevchenko 2021-04-23 17:33 ` Hans de Goede 0 siblings, 1 reply; 6+ messages in thread From: Andy Shevchenko @ 2021-02-25 15:44 UTC (permalink / raw) To: Hans de Goede; +Cc: Andy Shevchenko, linux-i2c, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1 1/1] i2c: cht-wc: Use fwnode for the controller and IRQ domain 2021-02-25 15:44 ` Andy Shevchenko @ 2021-04-23 17:33 ` Hans de Goede 0 siblings, 0 replies; 6+ messages in thread From: Hans de Goede @ 2021-04-23 17:33 UTC (permalink / raw) To: Andy Shevchenko; +Cc: Andy Shevchenko, linux-i2c, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-04-23 17:34 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-23 17:22 [PATCH v1 1/1] i2c: cht-wc: Use fwnode for the controller and IRQ domain Andy Shevchenko 2021-02-23 19:25 ` Hans de Goede 2021-02-24 12:51 ` Andy Shevchenko 2021-02-24 19:12 ` Hans de Goede 2021-02-25 15:44 ` Andy Shevchenko 2021-04-23 17:33 ` Hans de Goede
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.