| Message ID | 1639660402-31207-3-git-send-email-akhilrajeev@nvidia.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Enable named interrupt smbus-alert for ACPI as well | expand |
On Thu, Dec 16, 2021 at 3:14 PM Akhil R <akhilrajeev@nvidia.com> wrote: > > Change of_ functions to device_ for firmware agnostic usage. of_*() device_*() > This allows to have smbus_alert interrupt without any changes > in the controller drivers using ACPI table. ... > - irq = of_property_match_string(adapter->dev.of_node, "interrupt-names", > - "smbus_alert"); > + irq = device_property_match_string(adapter->dev.parent, "interrupt-names", > + "smbus_alert"); Hmm... Adapter device node is not the same as the node for its parent. Do you have some code that propagates of_node from parent to child? I.o.w. I would expect to see irq = device_property_match_string(&adapter->dev, "interrupt-names", here. > if (irq == -EINVAL || irq == -ENODATA) > return 0; > else if (irq < 0) TBH the entire code smells. "Interesting" way of getting an optional named interrupt.
> On Thu, Dec 16, 2021 at 3:14 PM Akhil R <akhilrajeev@nvidia.com> wrote: > > > > Change of_ functions to device_ for firmware agnostic usage. > > of_*() > device_*() > > > This allows to have smbus_alert interrupt without any changes in the > > controller drivers using ACPI table. > > ... > > > - irq = of_property_match_string(adapter->dev.of_node, "interrupt- > names", > > - "smbus_alert"); > > + irq = device_property_match_string(adapter->dev.parent, "interrupt- > names", > > + "smbus_alert"); > > Hmm... Adapter device node is not the same as the node for its parent. > Do you have some code that propagates of_node from parent to child? Adapter device does not have an of_node unless the adapter driver sets it, I guess. I see all the adapter drivers add the of_node and parent for adapter. Also, there are many places in i2c-core-base and i2c-core-acpi where adapter->dev.parent is referred to as the adapter driver device. Basically, adapter->dev.parent and adapter->dev.of_node would ultimately refer to the same device (or the of_node of that device), as far as I understand. > > I.o.w. I would expect to see > > irq = device_property_match_string(&adapter->dev, "interrupt-names", > > here. It would then require adding the fw_node as well from the adapter driver. I felt it made more sense to refer adapter->dev.parent here as most of the (or rather all of the) adapter drivers already sets it. > > > if (irq == -EINVAL || irq == -ENODATA) > > return 0; > > else if (irq < 0) > > TBH the entire code smells. "Interesting" way of getting an optional named > interrupt. I felt it useful to have it this way as it would remain agnostic to device tree and the ACPI. We would not have to add redundant codes in adapter drivers that are using ACPI table. Named interrupts for the ACPI as well, I feel would be a useful addition that can prove to be of value more than this change; I believe. Thanks, Akhil
On Thu, Dec 16, 2021 at 6:08 PM Akhil R <akhilrajeev@nvidia.com> wrote: > > On Thu, Dec 16, 2021 at 3:14 PM Akhil R <akhilrajeev@nvidia.com> wrote: ... > > > - irq = of_property_match_string(adapter->dev.of_node, "interrupt- > > names", > > > - "smbus_alert"); > > > + irq = device_property_match_string(adapter->dev.parent, "interrupt- > > names", > > > + "smbus_alert"); > > > > Hmm... Adapter device node is not the same as the node for its parent. > > Do you have some code that propagates of_node from parent to child? > Adapter device does not have an of_node unless the adapter driver > sets it, I guess. I see all the adapter drivers add the of_node and > parent for adapter. Also, there are many places in i2c-core-base and > i2c-core-acpi where adapter->dev.parent is referred to as the adapter > driver device. > > Basically, adapter->dev.parent and adapter->dev.of_node would > ultimately refer to the same device (or the of_node of that device), > as far as I understand. > > > > I.o.w. I would expect to see > > > > irq = device_property_match_string(&adapter->dev, "interrupt-names", > > > > here. > It would then require adding the fw_node as well from the adapter driver. > I felt it made more sense to refer adapter->dev.parent here as most of the > (or rather all of the) adapter drivers already sets it. Is this https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-core-base.c#L1047 what you are looking for? ... > > > if (irq == -EINVAL || irq == -ENODATA) > > > return 0; > > > else if (irq < 0) > > > > TBH the entire code smells. "Interesting" way of getting an optional named > > interrupt. > I felt it useful to have it this way as it would remain agnostic to device tree and > the ACPI. We would not have to add redundant codes in adapter drivers that > are using ACPI table. > > Named interrupts for the ACPI as well, I feel would be a useful addition that can > prove to be of value more than this change; I believe. Me too. My comment was about current state of affairs, and not to your change.
> On Thu, Dec 16, 2021 at 6:08 PM Akhil R <akhilrajeev@nvidia.com> wrote: > > > On Thu, Dec 16, 2021 at 3:14 PM Akhil R <akhilrajeev@nvidia.com> wrote: > > ... > > > > > - irq = of_property_match_string(adapter->dev.of_node, "interrupt- > > > names", > > > > - "smbus_alert"); > > > > + irq = device_property_match_string(adapter->dev.parent, > > > > + "interrupt- > > > names", > > > > + "smbus_alert"); > > > > > > Hmm... Adapter device node is not the same as the node for its parent. > > > Do you have some code that propagates of_node from parent to child? > > Adapter device does not have an of_node unless the adapter driver sets > > it, I guess. I see all the adapter drivers add the of_node and parent > > for adapter. Also, there are many places in i2c-core-base and > > i2c-core-acpi where adapter->dev.parent is referred to as the adapter > > driver device. > > > > Basically, adapter->dev.parent and adapter->dev.of_node would > > ultimately refer to the same device (or the of_node of that device), > > as far as I understand. > > > > > > I.o.w. I would expect to see > > > > > > irq = device_property_match_string(&adapter->dev, > > > "interrupt-names", > > > > > > here. > > It would then require adding the fw_node as well from the adapter driver. > > I felt it made more sense to refer adapter->dev.parent here as most of > > the (or rather all of the) adapter drivers already sets it. > > Is this > https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-core-base.c#L1047 > > what you are looking for? This, I suppose, is for the i2c client driver. I meant the individual adapter drivers. https://elixir.bootlin.com/linux/latest/source/drivers/i2c/busses/i2c-tegra.c#L1786 similar is there in all drivers. If to use adapter->dev for interrupt-names, I assume, it would require to add adapter->dev.fwnode = i2c_dev->dev->fwnode; in all drivers (or at least in the drivers which does not use devicetree). I thought it would be decent to use adapter->dev.parent as all the drivers already set it. > > ... > > > > > if (irq == -EINVAL || irq == -ENODATA) > > > > return 0; > > > > else if (irq < 0) > > > > > > TBH the entire code smells. "Interesting" way of getting an optional > > > named interrupt. > > I felt it useful to have it this way as it would remain agnostic to > > device tree and the ACPI. We would not have to add redundant codes in > > adapter drivers that are using ACPI table. > > > > Named interrupts for the ACPI as well, I feel would be a useful > > addition that can prove to be of value more than this change; I believe. > > Me too. My comment was about current state of affairs, and not to your > change. Got it :) Thanks, Akhil
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 1072a47..8e6c7a1 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -1574,7 +1574,7 @@ static int i2c_register_adapter(struct i2c_adapter *adap) goto out_list; } - res = of_i2c_setup_smbus_alert(adap); + res = i2c_setup_smbus_alert(adap); if (res) goto out_reg; diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c index e5b2d14..4c24c84 100644 --- a/drivers/i2c/i2c-core-smbus.c +++ b/drivers/i2c/i2c-core-smbus.c @@ -701,13 +701,13 @@ struct i2c_client *i2c_new_smbus_alert_device(struct i2c_adapter *adapter, } EXPORT_SYMBOL_GPL(i2c_new_smbus_alert_device); -#if IS_ENABLED(CONFIG_I2C_SMBUS) && IS_ENABLED(CONFIG_OF) -int of_i2c_setup_smbus_alert(struct i2c_adapter *adapter) +#if IS_ENABLED(CONFIG_I2C_SMBUS) +int i2c_setup_smbus_alert(struct i2c_adapter *adapter) { int irq; - irq = of_property_match_string(adapter->dev.of_node, "interrupt-names", - "smbus_alert"); + irq = device_property_match_string(adapter->dev.parent, "interrupt-names", + "smbus_alert"); if (irq == -EINVAL || irq == -ENODATA) return 0; else if (irq < 0) @@ -715,5 +715,5 @@ int of_i2c_setup_smbus_alert(struct i2c_adapter *adapter) return PTR_ERR_OR_ZERO(i2c_new_smbus_alert_device(adapter, NULL)); } -EXPORT_SYMBOL_GPL(of_i2c_setup_smbus_alert); +EXPORT_SYMBOL_GPL(i2c_setup_smbus_alert); #endif diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c index d3d06e3..fdd6d97 100644 --- a/drivers/i2c/i2c-smbus.c +++ b/drivers/i2c/i2c-smbus.c @@ -128,7 +128,7 @@ static int smbalert_probe(struct i2c_client *ara, if (setup) { irq = setup->irq; } else { - irq = of_irq_get_byname(adapter->dev.of_node, "smbus_alert"); + irq = device_irq_get_byname(adapter->dev.parent, "smbus_alert"); if (irq <= 0) return irq; } diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h index 1ef4218..95cf902 100644 --- a/include/linux/i2c-smbus.h +++ b/include/linux/i2c-smbus.h @@ -30,10 +30,10 @@ struct i2c_client *i2c_new_smbus_alert_device(struct i2c_adapter *adapter, struct i2c_smbus_alert_setup *setup); int i2c_handle_smbus_alert(struct i2c_client *ara); -#if IS_ENABLED(CONFIG_I2C_SMBUS) && IS_ENABLED(CONFIG_OF) -int of_i2c_setup_smbus_alert(struct i2c_adapter *adap); +#if IS_ENABLED(CONFIG_I2C_SMBUS) +int i2c_setup_smbus_alert(struct i2c_adapter *adap); #else -static inline int of_i2c_setup_smbus_alert(struct i2c_adapter *adap) +static inline int i2c_setup_smbus_alert(struct i2c_adapter *adap) { return 0; }
Change of_ functions to device_ for firmware agnostic usage. This allows to have smbus_alert interrupt without any changes in the controller drivers using ACPI table. Signed-off-by: Akhil R <akhilrajeev@nvidia.com> --- drivers/i2c/i2c-core-base.c | 2 +- drivers/i2c/i2c-core-smbus.c | 10 +++++----- drivers/i2c/i2c-smbus.c | 2 +- include/linux/i2c-smbus.h | 6 +++--- 4 files changed, 10 insertions(+), 10 deletions(-)