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 <[email protected]>
Signed-off-by: Akhil R <[email protected]>
---
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;
--
2.17.1
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 <[email protected]>
> Signed-off-by: Akhil R <[email protected]>
> ---
> drivers/i2c/busses/i2c-tegra.c | 1 +
> 1 file changed, 1 insertion(+)
Reviewed-by: Thierry Reding <[email protected]>
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 <[email protected]>
> Signed-off-by: Akhil R <[email protected]>
> ---
> 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
--
nvpublic
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 <[email protected]>
> > Signed-off-by: Akhil R <[email protected]>
> > ---
> > 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 <[email protected]>
>>> Signed-off-by: Akhil R <[email protected]>
>>> ---
>>> 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
--
nvpublic
> 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 <[email protected]>
> >>> Signed-off-by: Akhil R <[email protected]>
> >>> ---
> >>> 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 <[email protected]>
>>>>> Signed-off-by: Akhil R <[email protected]>
>>>>> ---
>>>>> 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
--
nvpublic
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 <[email protected]>
> Signed-off-by: Akhil R <[email protected]>
Applied to for-next, thanks!