[PATCH 0/2] Add and use new helper acpi_use_parent_companion

[PATCH 0/2] Add and use new helper acpi_use_parent_companion

[Heiner Kallweit]

In several drivers devices use the ACPI companion of the parent.
Add a helper for this use case to avoid code duplication.

Heiner Kallweit (2):
  ACPI: Add helper acpi_use_parent_companion
  i2c: i801: Use new helper acpi_use_parent_companion

 drivers/i2c/busses/i2c-i801.c | 2 +-
 include/linux/acpi.h          | 5 +++++
 2 files changed, 6 insertions(+), 1 deletion(-)

-- 
2.42.0

[PATCH 1/2] ACPI: Add helper acpi_use_parent_companion

[Heiner Kallweit]

In several drivers devices use the ACPI companion of the parent.
Add a helper for this use case to avoid code duplication.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 include/linux/acpi.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index ba3f601b6..89efb1658 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1541,4 +1541,9 @@ static inline void acpi_device_notify(struct device *dev) { }
 static inline void acpi_device_notify_remove(struct device *dev) { }
 #endif
 
+static inline void acpi_use_parent_companion(struct device *dev)
+{
+	ACPI_COMPANION_SET(dev, ACPI_COMPANION(dev->parent));
+}
+
 #endif	/*_LINUX_ACPI_H*/
-- 
2.42.0

[PATCH 2/2] i2c: i801: Use new helper acpi_use_parent_companion

[Heiner Kallweit]

Use new helper acpi_use_parent_companion to simplify the code.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-i801.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index a41f5349a..ac223146c 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1620,7 +1620,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	priv->adapter.class = I2C_CLASS_HWMON;
 	priv->adapter.algo = &smbus_algorithm;
 	priv->adapter.dev.parent = &dev->dev;
-	ACPI_COMPANION_SET(&priv->adapter.dev, ACPI_COMPANION(&dev->dev));
+	acpi_use_parent_companion(&priv->adapter.dev);
 	priv->adapter.retries = 3;
 
 	priv->pci_dev = dev;
-- 
2.42.0

Re: [PATCH 2/2] i2c: i801: Use new helper acpi_use_parent_companion

[Wilczynski, Michal]

Hi,

On 10/15/2023 11:36 PM, Heiner Kallweit wrote:
> Use new helper acpi_use_parent_companion to simplify the code.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index a41f5349a..ac223146c 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1620,7 +1620,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	priv->adapter.class = I2C_CLASS_HWMON;
>  	priv->adapter.algo = &smbus_algorithm;
>  	priv->adapter.dev.parent = &dev->dev;
> -	ACPI_COMPANION_SET(&priv->adapter.dev, ACPI_COMPANION(&dev->dev));
> +	acpi_use_parent_companion(&priv->adapter.dev);

I think this case is a bit too trivial for a helper, it's one line before, and
one line after, so it doesn't really save much.


>  	priv->adapter.retries = 3;
>  
>  	priv->pci_dev = dev;

Re: [PATCH 2/2] i2c: i801: Use new helper acpi_use_parent_companion

[Rafael J. Wysocki]

On Mon, Oct 16, 2023 at 6:10 PM Wilczynski, Michal
<michal.wilczynski@intel.com> wrote:
>
> Hi,
>
> On 10/15/2023 11:36 PM, Heiner Kallweit wrote:
> > Use new helper acpi_use_parent_companion to simplify the code.
> >
> > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> > ---
> >  drivers/i2c/busses/i2c-i801.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> > index a41f5349a..ac223146c 100644
> > --- a/drivers/i2c/busses/i2c-i801.c
> > +++ b/drivers/i2c/busses/i2c-i801.c
> > @@ -1620,7 +1620,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
> >       priv->adapter.class = I2C_CLASS_HWMON;
> >       priv->adapter.algo = &smbus_algorithm;
> >       priv->adapter.dev.parent = &dev->dev;
> > -     ACPI_COMPANION_SET(&priv->adapter.dev, ACPI_COMPANION(&dev->dev));
> > +     acpi_use_parent_companion(&priv->adapter.dev);
>
> I think this case is a bit too trivial for a helper, it's one line before, and
> one line after, so it doesn't really save much.

If this pattern is repeated in multiple places, the helper makes sense IMO.

>
> >       priv->adapter.retries = 3;
> >
> >       priv->pci_dev = dev;
>

Re: [PATCH 2/2] i2c: i801: Use new helper acpi_use_parent_companion

[Heiner Kallweit]

On 16.10.2023 19:32, Rafael J. Wysocki wrote:
> On Mon, Oct 16, 2023 at 6:10 PM Wilczynski, Michal
> <michal.wilczynski@intel.com> wrote:
>>
>> Hi,
>>
>> On 10/15/2023 11:36 PM, Heiner Kallweit wrote:
>>> Use new helper acpi_use_parent_companion to simplify the code.
>>>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>> ---
>>>  drivers/i2c/busses/i2c-i801.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>>> index a41f5349a..ac223146c 100644
>>> --- a/drivers/i2c/busses/i2c-i801.c
>>> +++ b/drivers/i2c/busses/i2c-i801.c
>>> @@ -1620,7 +1620,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>>>       priv->adapter.class = I2C_CLASS_HWMON;
>>>       priv->adapter.algo = &smbus_algorithm;
>>>       priv->adapter.dev.parent = &dev->dev;
>>> -     ACPI_COMPANION_SET(&priv->adapter.dev, ACPI_COMPANION(&dev->dev));
>>> +     acpi_use_parent_companion(&priv->adapter.dev);
>>
>> I think this case is a bit too trivial for a helper, it's one line before, and
>> one line after, so it doesn't really save much.
> 
> If this pattern is repeated in multiple places, the helper makes sense IMO.
> 

I didn't check each usage in detail, but this should be the places where the new
helper can be used.
Another advantage IMO is that the helper, being a function instead of a macro,
is type-safe.

drivers/usb/common/ulpi.c:      ACPI_COMPANION_SET(&ulpi->dev, ACPI_COMPANION(dev));
drivers/usb/dwc3/dwc3-pci.c:    ACPI_COMPANION_SET(&dwc->dwc3->dev, ACPI_COMPANION(dev));
drivers/usb/core/message.c:             ACPI_COMPANION_SET(&intf->dev, ACPI_COMPANION(&dev->dev));
drivers/tty/serial/8250/8250_exar.c:    ACPI_COMPANION_SET(&pdev->dev, ACPI_COMPANION(&pcidev->dev));
drivers/i2c/busses/i2c-imx.c:   ACPI_COMPANION_SET(&i2c_imx->adapter.dev, ACPI_COMPANION(&pdev->dev));
drivers/i2c/busses/i2c-kempld.c:        ACPI_COMPANION_SET(&i2c->adap.dev, ACPI_COMPANION(&pdev->dev));
drivers/i2c/busses/i2c-virtio.c:        ACPI_COMPANION_SET(&vi->adap.dev, ACPI_COMPANION(vdev->dev.parent));
drivers/i2c/busses/i2c-dln2.c:  ACPI_COMPANION_SET(&dln2->adapter.dev, ACPI_COMPANION(&pdev->dev));
drivers/i2c/busses/i2c-i801.c:     ACPI_COMPANION_SET(&priv->adapter.dev, ACPI_COMPANION(&dev->dev));
drivers/i2c/busses/i2c-ismt.c:  ACPI_COMPANION_SET(&priv->adapter.dev, ACPI_COMPANION(&pdev->dev));
drivers/i2c/busses/i2c-cros-ec-tunnel.c:        ACPI_COMPANION_SET(&bus->adap.dev, ACPI_COMPANION(&pdev->dev));
drivers/i2c/busses/i2c-synquacer.c:     ACPI_COMPANION_SET(&i2c->adapter.dev, ACPI_COMPANION(&pdev->dev));
drivers/i2c/busses/i2c-designware-pcidrv.c:     ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
drivers/i2c/busses/i2c-qup.c:           ACPI_COMPANION_SET(&qup->adap.dev, ACPI_COMPANION(qup->dev));
drivers/i2c/busses/i2c-designware-platdrv.c:    ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
drivers/i2c/busses/i2c-qcom-geni.c:             ACPI_COMPANION_SET(&gi2c->adap.dev, ACPI_COMPANION(dev));
drivers/i2c/busses/i2c-amd-mp2-plat.c:  ACPI_COMPANION_SET(&i2c_dev->adap.dev, ACPI_COMPANION(&pdev->dev));
drivers/i2c/busses/i2c-xgene-slimpro.c: ACPI_COMPANION_SET(&adapter->dev, ACPI_COMPANION(&pdev->dev));
drivers/i2c/busses/i2c-tegra.c: ACPI_COMPANION_SET(&i2c_dev->adapter.dev, ACPI_COMPANION(&pdev->dev));
drivers/i2c/busses/i2c-xlp9xx.c:        ACPI_COMPANION_SET(&priv->adapter.dev, ACPI_COMPANION(&pdev->dev));


>>
>>>       priv->adapter.retries = 3;
>>>
>>>       priv->pci_dev = dev;
>>

Re: [PATCH 1/2] ACPI: Add helper acpi_use_parent_companion

[Rafael J. Wysocki]

On Sun, Oct 15, 2023 at 11:34 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>
> In several drivers devices use the ACPI companion of the parent.
> Add a helper for this use case to avoid code duplication.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

or do you want me to apply it?

> ---
>  include/linux/acpi.h | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index ba3f601b6..89efb1658 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -1541,4 +1541,9 @@ static inline void acpi_device_notify(struct device *dev) { }
>  static inline void acpi_device_notify_remove(struct device *dev) { }
>  #endif
>
> +static inline void acpi_use_parent_companion(struct device *dev)
> +{
> +       ACPI_COMPANION_SET(dev, ACPI_COMPANION(dev->parent));
> +}
> +
>  #endif /*_LINUX_ACPI_H*/
> --
> 2.42.0
>
>

Re: [PATCH 1/2] ACPI: Add helper acpi_use_parent_companion

[Heiner Kallweit]

On 18.10.2023 12:51, Rafael J. Wysocki wrote:
> On Sun, Oct 15, 2023 at 11:34 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>
>> In several drivers devices use the ACPI companion of the parent.
>> Add a helper for this use case to avoid code duplication.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> 
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> or do you want me to apply it?
> 
Patch 2 of the series will apply cleanly only after the following
patch that has been reviewed/acked, but not applied yet.
https://patchwork.ozlabs.org/project/linux-i2c/patch/2192294e-99ab-4c7d-86b1-edff058d82f3@gmail.com/
So my preference is to apply the series through the i2c tree.
+Wolfram

>> ---
>>  include/linux/acpi.h | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
>> index ba3f601b6..89efb1658 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -1541,4 +1541,9 @@ static inline void acpi_device_notify(struct device *dev) { }
>>  static inline void acpi_device_notify_remove(struct device *dev) { }
>>  #endif
>>
>> +static inline void acpi_use_parent_companion(struct device *dev)
>> +{
>> +       ACPI_COMPANION_SET(dev, ACPI_COMPANION(dev->parent));
>> +}
>> +
>>  #endif /*_LINUX_ACPI_H*/
>> --
>> 2.42.0
>>
>>

Re: [PATCH 2/2] i2c: i801: Use new helper acpi_use_parent_companion

[Jean Delvare]

Hi Heiner and all,

On Mon, 16 Oct 2023 22:05:51 +0200, Heiner Kallweit wrote:
> On 16.10.2023 19:32, Rafael J. Wysocki wrote:
> > On Mon, Oct 16, 2023 at 6:10 PM Wilczynski, Michal
> > <michal.wilczynski@intel.com> wrote:  
> >> On 10/15/2023 11:36 PM, Heiner Kallweit wrote:  
> >>> Use new helper acpi_use_parent_companion to simplify the code.
> >>>
> >>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> >>> ---
> >>>  drivers/i2c/busses/i2c-i801.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> >>> index a41f5349a..ac223146c 100644
> >>> --- a/drivers/i2c/busses/i2c-i801.c
> >>> +++ b/drivers/i2c/busses/i2c-i801.c
> >>> @@ -1620,7 +1620,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
> >>>       priv->adapter.class = I2C_CLASS_HWMON;
> >>>       priv->adapter.algo = &smbus_algorithm;
> >>>       priv->adapter.dev.parent = &dev->dev;
> >>> -     ACPI_COMPANION_SET(&priv->adapter.dev, ACPI_COMPANION(&dev->dev));
> >>> +     acpi_use_parent_companion(&priv->adapter.dev);  
> >>
> >> I think this case is a bit too trivial for a helper, it's one line before, and
> >> one line after, so it doesn't really save much.  

I must say I share Michal's skepticism.

> > If this pattern is repeated in multiple places, the helper makes sense IMO.
> 
> I didn't check each usage in detail, but this should be the places where the new
> helper can be used.
> Another advantage IMO is that the helper, being a function instead of a macro,
> is type-safe.

If type safety is a concern then I'd rather turn ACPI_COMPANION_SET to
an inline function (which would make more sense than a macro anyway
IMHO, as it has an intended side effect).

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 1/2] ACPI: Add helper acpi_use_parent_companion

[Andi Shyti]

Hi Heiner,

On Sun, Oct 15, 2023 at 11:34:25PM +0200, Heiner Kallweit wrote:
> In several drivers devices use the ACPI companion of the parent.
> Add a helper for this use case to avoid code duplication.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Andi Shyti <andi.shyti@kernel.org>

Wolfram, can we take this in i2c? Otherwise this will take two
release rounds to get in.

Andi

Re: [PATCH 2/2] i2c: i801: Use new helper acpi_use_parent_companion

[Andi Shyti]

Hi Heiner,

On Sun, Oct 15, 2023 at 11:36:17PM +0200, Heiner Kallweit wrote:
> Use new helper acpi_use_parent_companion to simplify the code.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index a41f5349a..ac223146c 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1620,7 +1620,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	priv->adapter.class = I2C_CLASS_HWMON;
>  	priv->adapter.algo = &smbus_algorithm;
>  	priv->adapter.dev.parent = &dev->dev;
> -	ACPI_COMPANION_SET(&priv->adapter.dev, ACPI_COMPANION(&dev->dev));
> +	acpi_use_parent_companion(&priv->adapter.dev);

I find this neater.

Reviewed-by: Andi Shyti <andi.shyti@kernel.org>

Andi

Re: [PATCH 1/2] ACPI: Add helper acpi_use_parent_companion

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 301 bytes --]


> Wolfram, can we take this in i2c? Otherwise this will take two
> release rounds to get in.

In general, I can, no problem. But there is still on-going discussion
regarding patch 2. So, I will wait for a conclusion there. I am not
familiar enough with ACPI to have an own opinion about that topic.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] ACPI: Add helper acpi_use_parent_companion

[Heiner Kallweit]

On 26.10.2023 19:26, Wolfram Sang wrote:
> 
>> Wolfram, can we take this in i2c? Otherwise this will take two
>> release rounds to get in.
> 
> In general, I can, no problem. But there is still on-going discussion
> regarding patch 2. So, I will wait for a conclusion there. I am not
> familiar enough with ACPI to have an own opinion about that topic.
> 
Not sure whether the discussion is ongoing, my impression is it stalled.
Rafael as ACPI maintainer thinks the helper makes sense and therefore
acked it. Andi shares this opinion, and others like Jean have doubts.
For sure I won't receive a Turing award for this helper, but it's IMO
better than the cascaded macros.

Re: [PATCH 2/2] i2c: i801: Use new helper acpi_use_parent_companion

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 788 bytes --]


> > >> I think this case is a bit too trivial for a helper, it's one line before, and
> > >> one line after, so it doesn't really save much.  
> 
> I must say I share Michal's skepticism.

Because Rafael likes it, I will pick it up.

> > > If this pattern is repeated in multiple places, the helper makes sense IMO.
> > 
> > I didn't check each usage in detail, but this should be the places where the new
> > helper can be used.
> > Another advantage IMO is that the helper, being a function instead of a macro,
> > is type-safe.
> 
> If type safety is a concern then I'd rather turn ACPI_COMPANION_SET to
> an inline function (which would make more sense than a macro anyway
> IMHO, as it has an intended side effect).

I guess this can still be done seperately?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] ACPI: Add helper acpi_use_parent_companion

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 290 bytes --]

On Sun, Oct 15, 2023 at 11:34:25PM +0200, Heiner Kallweit wrote:
> In several drivers devices use the ACPI companion of the parent.
> Add a helper for this use case to avoid code duplication.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] i2c: i801: Use new helper acpi_use_parent_companion

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 227 bytes --]

On Sun, Oct 15, 2023 at 11:36:17PM +0200, Heiner Kallweit wrote:
> Use new helper acpi_use_parent_companion to simplify the code.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]