| Message ID | 20230725143023.86325-5-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Andi Shyti |
| Headers | show |
| Series | i2c: designware: code consolidation & cleanups | expand |
On 7/25/23 17:30, Andy Shevchenko wrote: > Propagate firmware node by using a specific API call, i.e. device_set_node(). > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/i2c/busses/i2c-designware-core.h | 6 ++++-- > drivers/i2c/busses/i2c-designware-pcidrv.c | 2 -- > drivers/i2c/busses/i2c-designware-platdrv.c | 2 -- > 3 files changed, 4 insertions(+), 6 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h > index 03f4d44ae94c..f0c683ad860f 100644 > --- a/drivers/i2c/busses/i2c-designware-core.h > +++ b/drivers/i2c/busses/i2c-designware-core.h > @@ -10,11 +10,11 @@ > */ > > #include <linux/bits.h> > -#include <linux/compiler_types.h> > #include <linux/completion.h> > -#include <linux/dev_printk.h> > +#include <linux/device.h> > #include <linux/errno.h> > #include <linux/i2c.h> > +#include <linux/property.h> > #include <linux/regmap.h> > #include <linux/types.h> > > @@ -363,6 +363,8 @@ static inline int i2c_dw_probe_slave(struct dw_i2c_dev *dev) { return -EINVAL; } > > static inline int i2c_dw_probe(struct dw_i2c_dev *dev) > { > + device_set_node(&dev->adapter.dev, dev_fwnode(dev->dev)); > + Would this be better to put in the same place where ACPI_COMPANION_SET() is removed like below? I'd keep this static inline function in the header file as simple as possible. All extra code might invite adding even more. > switch (dev->mode) { > case DW_IC_SLAVE: > return i2c_dw_probe_slave(dev); > diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c > index 7f5a04538c71..a42a47e0032d 100644 > --- a/drivers/i2c/busses/i2c-designware-pcidrv.c > +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c > @@ -9,7 +9,6 @@ > * Copyright (C) 2009 Provigent Ltd. > * Copyright (C) 2011, 2015, 2016 Intel Corporation. > */ > -#include <linux/acpi.h> > #include <linux/delay.h> > #include <linux/err.h> > #include <linux/errno.h> > @@ -325,7 +324,6 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev, > adap = &dev->adapter; > adap->owner = THIS_MODULE; > adap->class = 0; > - ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev)); > adap->nr = controller->bus_num;
On Fri, Jul 28, 2023 at 03:25:58PM +0300, Jarkko Nikula wrote: > On 7/25/23 17:30, Andy Shevchenko wrote: > > Propagate firmware node by using a specific API call, i.e. device_set_node(). ... > > + device_set_node(&dev->adapter.dev, dev_fwnode(dev->dev)); > > Would this be better to put in the same place where ACPI_COMPANION_SET() is > removed like below? I'd keep this static inline function in the header file > as simple as possible. All extra code might invite adding even more. We come again to the duplication and prone to deviation code, I wouldn't like to go this way. The idea of this call is to unify and avoid mistakes, like updating only in ACPI or DT (or any new one if happens in the future) case and leaving the second one unconsidered. That said, I would rather drop this patch until i2c core will take this once for all (may be never in the reasonable future :-).
Hi Andy, On Mon, Jul 31, 2023 at 11:09:24PM +0300, Andy Shevchenko wrote: > On Fri, Jul 28, 2023 at 03:25:58PM +0300, Jarkko Nikula wrote: > > On 7/25/23 17:30, Andy Shevchenko wrote: > > > Propagate firmware node by using a specific API call, i.e. device_set_node(). > > ... > > > > + device_set_node(&dev->adapter.dev, dev_fwnode(dev->dev)); > > > > Would this be better to put in the same place where ACPI_COMPANION_SET() is > > removed like below? I'd keep this static inline function in the header file > > as simple as possible. All extra code might invite adding even more. > > We come again to the duplication and prone to deviation code, I wouldn't like > to go this way. The idea of this call is to unify and avoid mistakes, like > updating only in ACPI or DT (or any new one if happens in the future) case > and leaving the second one unconsidered. it's anyway an inline function becoming a bit too fat. Can't we make it not inline? > That said, I would rather drop this patch until i2c core will take this > once for all (may be never in the reasonable future :-). Which patch are you referring to that should be taken into i2c core? Andi
On Fri, Aug 04, 2023 at 10:59:56PM +0200, Andi Shyti wrote: > On Mon, Jul 31, 2023 at 11:09:24PM +0300, Andy Shevchenko wrote: > > On Fri, Jul 28, 2023 at 03:25:58PM +0300, Jarkko Nikula wrote: > > > On 7/25/23 17:30, Andy Shevchenko wrote: > > > > Propagate firmware node by using a specific API call, i.e. device_set_node(). ... > > > > + device_set_node(&dev->adapter.dev, dev_fwnode(dev->dev)); > > > > > > Would this be better to put in the same place where ACPI_COMPANION_SET() is > > > removed like below? I'd keep this static inline function in the header file > > > as simple as possible. All extra code might invite adding even more. > > > > We come again to the duplication and prone to deviation code, I wouldn't like > > to go this way. The idea of this call is to unify and avoid mistakes, like > > updating only in ACPI or DT (or any new one if happens in the future) case > > and leaving the second one unconsidered. > > it's anyway an inline function becoming a bit too fat. Can't we > make it not inline? > > > That said, I would rather drop this patch until i2c core will take this > > once for all (may be never in the reasonable future :-). > > Which patch are you referring to that should be taken into i2c > core? Something I tried to do in the past but failed. https://lore.kernel.org/linux-i2c/20211207162457.18450-1-andriy.shevchenko@linux.intel.com/
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h index 03f4d44ae94c..f0c683ad860f 100644 --- a/drivers/i2c/busses/i2c-designware-core.h +++ b/drivers/i2c/busses/i2c-designware-core.h @@ -10,11 +10,11 @@ */ #include <linux/bits.h> -#include <linux/compiler_types.h> #include <linux/completion.h> -#include <linux/dev_printk.h> +#include <linux/device.h> #include <linux/errno.h> #include <linux/i2c.h> +#include <linux/property.h> #include <linux/regmap.h> #include <linux/types.h> @@ -363,6 +363,8 @@ static inline int i2c_dw_probe_slave(struct dw_i2c_dev *dev) { return -EINVAL; } static inline int i2c_dw_probe(struct dw_i2c_dev *dev) { + device_set_node(&dev->adapter.dev, dev_fwnode(dev->dev)); + switch (dev->mode) { case DW_IC_SLAVE: return i2c_dw_probe_slave(dev); diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c index 7f5a04538c71..a42a47e0032d 100644 --- a/drivers/i2c/busses/i2c-designware-pcidrv.c +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c @@ -9,7 +9,6 @@ * Copyright (C) 2009 Provigent Ltd. * Copyright (C) 2011, 2015, 2016 Intel Corporation. */ -#include <linux/acpi.h> #include <linux/delay.h> #include <linux/err.h> #include <linux/errno.h> @@ -325,7 +324,6 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev, adap = &dev->adapter; adap->owner = THIS_MODULE; adap->class = 0; - ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev)); adap->nr = controller->bus_num; r = i2c_dw_probe(dev); diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index d35a6bbcb6fb..512fb1d8ddfc 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -357,8 +357,6 @@ static int dw_i2c_plat_probe(struct platform_device *pdev) adap->owner = THIS_MODULE; adap->class = dmi_check_system(dw_i2c_hwmon_class_dmi) ? I2C_CLASS_HWMON : I2C_CLASS_DEPRECATED; - ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev)); - adap->dev.of_node = pdev->dev.of_node; adap->nr = -1; if (dev->flags & ACCESS_NO_IRQ_SUSPEND) {
Propagate firmware node by using a specific API call, i.e. device_set_node(). Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/i2c/busses/i2c-designware-core.h | 6 ++++-- drivers/i2c/busses/i2c-designware-pcidrv.c | 2 -- drivers/i2c/busses/i2c-designware-platdrv.c | 2 -- 3 files changed, 4 insertions(+), 6 deletions(-)