i2c: tegra: Set ACPI node as primary fwnode

[PATCH] i2c: tegra: Set ACPI node as primary fwnode

Posted by Akhil R 3 years, 4 months ago

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;
-- 
2.17.1

Re: [PATCH] i2c: tegra: Set ACPI node as primary fwnode

Posted by Wolfram Sang 3 years, 4 months ago

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!

Re: [PATCH] i2c: tegra: Set ACPI node as primary fwnode

Posted by Jon Hunter 3 years, 4 months ago

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

-- 
nvpublic

Re: [PATCH] i2c: tegra: Set ACPI node as primary fwnode

Posted by Thierry Reding 3 years, 4 months ago

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

Re: [PATCH] i2c: tegra: Set ACPI node as primary fwnode

Posted by Jon Hunter 3 years, 4 months ago

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

-- 
nvpublic

RE: [PATCH] i2c: tegra: Set ACPI node as primary fwnode

Posted by Akhil R 3 years, 4 months ago

> 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

Re: [PATCH] i2c: tegra: Set ACPI node as primary fwnode

Posted by Jon Hunter 3 years, 4 months ago

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

-- 
nvpublic

Re: [PATCH] i2c: tegra: Set ACPI node as primary fwnode

Posted by Thierry Reding 3 years, 4 months ago

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>