| Message ID | 20221117100415.20457-1-akhilrajeev@nvidia.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | i2c: tegra: Set ACPI node as primary fwnode | expand |
On Thu, Nov 17, 2022 at 03:34:15PM +0530, Akhil R wrote: > Set ACPI node as the primary fwnode of I2C adapter to allow > enumeration of child devices from the ACPI table > > Signed-off-by: Zubair Waheed <zwaheed@nvidia.com> > Signed-off-by: Akhil R <akhilrajeev@nvidia.com> > --- > drivers/i2c/busses/i2c-tegra.c | 1 + > 1 file changed, 1 insertion(+) Reviewed-by: Thierry Reding <treding@nvidia.com>
On 17/11/2022 10:04, Akhil R wrote: > Set ACPI node as the primary fwnode of I2C adapter to allow > enumeration of child devices from the ACPI table > > Signed-off-by: Zubair Waheed <zwaheed@nvidia.com> > Signed-off-by: Akhil R <akhilrajeev@nvidia.com> > --- > drivers/i2c/busses/i2c-tegra.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > index 954022c04cc4..69c9ae161bbe 100644 > --- a/drivers/i2c/busses/i2c-tegra.c > +++ b/drivers/i2c/busses/i2c-tegra.c > @@ -1826,6 +1826,7 @@ static int tegra_i2c_probe(struct platform_device *pdev) > i2c_dev->adapter.class = I2C_CLASS_DEPRECATED; > i2c_dev->adapter.algo = &tegra_i2c_algo; > i2c_dev->adapter.nr = pdev->id; > + ACPI_COMPANION_SET(&i2c_dev->adapter.dev, ACPI_COMPANION(&pdev->dev)); > > if (i2c_dev->hw->supports_bus_clear) > i2c_dev->adapter.bus_recovery_info = &tegra_i2c_recovery_info; Do we always want to set as the primary fwnode even when booting with device-tree? I some other drivers do, but I also see some others ... if (has_acpi_companion(dev)) ACPI_COMPANION_SET(&i2c_dev->adapter.dev, ACPI_COMPANION(&pdev->dev)); It would be nice to know why it is OK to always do this even for device-tree because it is not clear to me. Jon
On Fri, Nov 18, 2022 at 09:38:52AM +0000, Jon Hunter wrote: > > On 17/11/2022 10:04, Akhil R wrote: > > Set ACPI node as the primary fwnode of I2C adapter to allow > > enumeration of child devices from the ACPI table > > > > Signed-off-by: Zubair Waheed <zwaheed@nvidia.com> > > Signed-off-by: Akhil R <akhilrajeev@nvidia.com> > > --- > > drivers/i2c/busses/i2c-tegra.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > > index 954022c04cc4..69c9ae161bbe 100644 > > --- a/drivers/i2c/busses/i2c-tegra.c > > +++ b/drivers/i2c/busses/i2c-tegra.c > > @@ -1826,6 +1826,7 @@ static int tegra_i2c_probe(struct platform_device *pdev) > > i2c_dev->adapter.class = I2C_CLASS_DEPRECATED; > > i2c_dev->adapter.algo = &tegra_i2c_algo; > > i2c_dev->adapter.nr = pdev->id; > > + ACPI_COMPANION_SET(&i2c_dev->adapter.dev, ACPI_COMPANION(&pdev->dev)); > > if (i2c_dev->hw->supports_bus_clear) > > i2c_dev->adapter.bus_recovery_info = &tegra_i2c_recovery_info; > > > Do we always want to set as the primary fwnode even when booting with > device-tree? I some other drivers do, but I also see some others ... > > if (has_acpi_companion(dev)) > ACPI_COMPANION_SET(&i2c_dev->adapter.dev, > ACPI_COMPANION(&pdev->dev)); > > It would be nice to know why it is OK to always do this even for device-tree > because it is not clear to me. ACPI_COMPANION() returns NULL if there is no ACPI companion, which will cause ACPI_COMPANION_SET() to set the primary fwnode to NULL. If I read the code for set_primary_fwnode() correctly, that's essentially a no-op for DT devices. I guess that the extra check might save a few cycles by not having to run through all the various conditionals, but it seems a rather minor saving. Either way is fine with me, though. Thierry
On 18/11/2022 10:18, Thierry Reding wrote: > On Fri, Nov 18, 2022 at 09:38:52AM +0000, Jon Hunter wrote: >> >> On 17/11/2022 10:04, Akhil R wrote: >>> Set ACPI node as the primary fwnode of I2C adapter to allow >>> enumeration of child devices from the ACPI table >>> >>> Signed-off-by: Zubair Waheed <zwaheed@nvidia.com> >>> Signed-off-by: Akhil R <akhilrajeev@nvidia.com> >>> --- >>> drivers/i2c/busses/i2c-tegra.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c >>> index 954022c04cc4..69c9ae161bbe 100644 >>> --- a/drivers/i2c/busses/i2c-tegra.c >>> +++ b/drivers/i2c/busses/i2c-tegra.c >>> @@ -1826,6 +1826,7 @@ static int tegra_i2c_probe(struct platform_device *pdev) >>> i2c_dev->adapter.class = I2C_CLASS_DEPRECATED; >>> i2c_dev->adapter.algo = &tegra_i2c_algo; >>> i2c_dev->adapter.nr = pdev->id; >>> + ACPI_COMPANION_SET(&i2c_dev->adapter.dev, ACPI_COMPANION(&pdev->dev)); >>> if (i2c_dev->hw->supports_bus_clear) >>> i2c_dev->adapter.bus_recovery_info = &tegra_i2c_recovery_info; >> >> >> Do we always want to set as the primary fwnode even when booting with >> device-tree? I some other drivers do, but I also see some others ... >> >> if (has_acpi_companion(dev)) >> ACPI_COMPANION_SET(&i2c_dev->adapter.dev, >> ACPI_COMPANION(&pdev->dev)); >> >> It would be nice to know why it is OK to always do this even for device-tree >> because it is not clear to me. > > ACPI_COMPANION() returns NULL if there is no ACPI companion, which will > cause ACPI_COMPANION_SET() to set the primary fwnode to NULL. If I read > the code for set_primary_fwnode() correctly, that's essentially a no-op > for DT devices. Yes it does, but doesn't it is not clear to me if it is a good idea to pass NULL to set_primary_fwnode(). It does seem to handle this but my biggest gripe is the lack of explanation in the commit message why this is OK. Jon
> On 18/11/2022 10:18, Thierry Reding wrote: > > On Fri, Nov 18, 2022 at 09:38:52AM +0000, Jon Hunter wrote: > >> > >> On 17/11/2022 10:04, Akhil R wrote: > >>> Set ACPI node as the primary fwnode of I2C adapter to allow > >>> enumeration of child devices from the ACPI table > >>> > >>> Signed-off-by: Zubair Waheed <zwaheed@nvidia.com> > >>> Signed-off-by: Akhil R <akhilrajeev@nvidia.com> > >>> --- > >>> drivers/i2c/busses/i2c-tegra.c | 1 + > >>> 1 file changed, 1 insertion(+) > >>> > >>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > >>> index 954022c04cc4..69c9ae161bbe 100644 > >>> --- a/drivers/i2c/busses/i2c-tegra.c > >>> +++ b/drivers/i2c/busses/i2c-tegra.c > >>> @@ -1826,6 +1826,7 @@ static int tegra_i2c_probe(struct platform_device > *pdev) > >>> i2c_dev->adapter.class = I2C_CLASS_DEPRECATED; > >>> i2c_dev->adapter.algo = &tegra_i2c_algo; > >>> i2c_dev->adapter.nr = pdev->id; > >>> + ACPI_COMPANION_SET(&i2c_dev->adapter.dev, > ACPI_COMPANION(&pdev->dev)); > >>> if (i2c_dev->hw->supports_bus_clear) > >>> i2c_dev->adapter.bus_recovery_info = > &tegra_i2c_recovery_info; > >> > >> > >> Do we always want to set as the primary fwnode even when booting with > >> device-tree? I some other drivers do, but I also see some others ... > >> > >> if (has_acpi_companion(dev)) > >> ACPI_COMPANION_SET(&i2c_dev->adapter.dev, > >> ACPI_COMPANION(&pdev->dev)); > >> > >> It would be nice to know why it is OK to always do this even for device-tree > >> because it is not clear to me. > > > > ACPI_COMPANION() returns NULL if there is no ACPI companion, which will > > cause ACPI_COMPANION_SET() to set the primary fwnode to NULL. If I read > > the code for set_primary_fwnode() correctly, that's essentially a no-op > > for DT devices. > > Yes it does, but doesn't it is not clear to me if it is a good idea to > pass NULL to set_primary_fwnode(). It does seem to handle this but my > biggest gripe is the lack of explanation in the commit message why this > is OK. I saw ACPI_COMPANION_SET() as an empty function if CONFIG_ACPI is not set. Yes, I agree that I should have mentioned this in the commit message. Shall I send a v2 with the details added in the commit description? Regards, Akhil
On 18/11/2022 14:27, Akhil R wrote: >> On 18/11/2022 10:18, Thierry Reding wrote: >>> On Fri, Nov 18, 2022 at 09:38:52AM +0000, Jon Hunter wrote: >>>> >>>> On 17/11/2022 10:04, Akhil R wrote: >>>>> Set ACPI node as the primary fwnode of I2C adapter to allow >>>>> enumeration of child devices from the ACPI table >>>>> >>>>> Signed-off-by: Zubair Waheed <zwaheed@nvidia.com> >>>>> Signed-off-by: Akhil R <akhilrajeev@nvidia.com> >>>>> --- >>>>> drivers/i2c/busses/i2c-tegra.c | 1 + >>>>> 1 file changed, 1 insertion(+) >>>>> >>>>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c >>>>> index 954022c04cc4..69c9ae161bbe 100644 >>>>> --- a/drivers/i2c/busses/i2c-tegra.c >>>>> +++ b/drivers/i2c/busses/i2c-tegra.c >>>>> @@ -1826,6 +1826,7 @@ static int tegra_i2c_probe(struct platform_device >> *pdev) >>>>> i2c_dev->adapter.class = I2C_CLASS_DEPRECATED; >>>>> i2c_dev->adapter.algo = &tegra_i2c_algo; >>>>> i2c_dev->adapter.nr = pdev->id; >>>>> + ACPI_COMPANION_SET(&i2c_dev->adapter.dev, >> ACPI_COMPANION(&pdev->dev)); >>>>> if (i2c_dev->hw->supports_bus_clear) >>>>> i2c_dev->adapter.bus_recovery_info = >> &tegra_i2c_recovery_info; >>>> >>>> >>>> Do we always want to set as the primary fwnode even when booting with >>>> device-tree? I some other drivers do, but I also see some others ... >>>> >>>> if (has_acpi_companion(dev)) >>>> ACPI_COMPANION_SET(&i2c_dev->adapter.dev, >>>> ACPI_COMPANION(&pdev->dev)); >>>> >>>> It would be nice to know why it is OK to always do this even for device-tree >>>> because it is not clear to me. >>> >>> ACPI_COMPANION() returns NULL if there is no ACPI companion, which will >>> cause ACPI_COMPANION_SET() to set the primary fwnode to NULL. If I read >>> the code for set_primary_fwnode() correctly, that's essentially a no-op >>> for DT devices. >> >> Yes it does, but doesn't it is not clear to me if it is a good idea to >> pass NULL to set_primary_fwnode(). It does seem to handle this but my >> biggest gripe is the lack of explanation in the commit message why this >> is OK. > I saw ACPI_COMPANION_SET() as an empty function if CONFIG_ACPI is not set. That's not the issue. By default CONFIG_ACPI is enabled for arm64 but for Tegra we typically boot with device-tree. So I was more concerned about the case where ACPI_COMPANION_SET() is not an empty function. > Yes, I agree that I should have mentioned this in the commit message. > Shall I send a v2 with the details added in the commit description? No need, especially as Thierry has already applied. I am not familiar with this function and primary/secondary fwnodes so wanted to understand there is no issue for device tree. Jon
On Thu, Nov 17, 2022 at 03:34:15PM +0530, Akhil R wrote: > Set ACPI node as the primary fwnode of I2C adapter to allow > enumeration of child devices from the ACPI table > > Signed-off-by: Zubair Waheed <zwaheed@nvidia.com> > Signed-off-by: Akhil R <akhilrajeev@nvidia.com> Applied to for-next, thanks!
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c index 954022c04cc4..69c9ae161bbe 100644 --- a/drivers/i2c/busses/i2c-tegra.c +++ b/drivers/i2c/busses/i2c-tegra.c @@ -1826,6 +1826,7 @@ static int tegra_i2c_probe(struct platform_device *pdev) i2c_dev->adapter.class = I2C_CLASS_DEPRECATED; i2c_dev->adapter.algo = &tegra_i2c_algo; i2c_dev->adapter.nr = pdev->id; + ACPI_COMPANION_SET(&i2c_dev->adapter.dev, ACPI_COMPANION(&pdev->dev)); if (i2c_dev->hw->supports_bus_clear) i2c_dev->adapter.bus_recovery_info = &tegra_i2c_recovery_info;