[PATCH v2 0/4] Define i2c_designware in a header file

[PATCH v2 0/4] Define i2c_designware in a header file

[Florian Fainelli]

This patch series depends upon the following two patches being applied:

https://lore.kernel.org/all/20240422084109.3201-1-duanqiangwen@net-swift.com/
https://lore.kernel.org/all/20240422084109.3201-2-duanqiangwen@net-swift.com/

There is no reason why each driver should have to repeat the
"i2c_designware" string all over the place, because when that happens we
see the reverts like the above being necessary.

Changes in v2:

- avoid changing i2c-designware-pcidrv.c more than necessary
- move constant to include/linux/platform_data/i2c-designware.h
- add comments as to how this constant is used and why

Florian Fainelli (4):
  i2c: designware: Create shared header hosting driver name
  mfd: intel-lpss: Utilize i2c-designware.h
  mfd: intel_quark_i2c_gpio: Utilize i2c-designware.h
  net: txgbe: Utilize i2c-designware.h

 MAINTAINERS                                    |  1 +
 drivers/i2c/busses/i2c-designware-pcidrv.c     |  3 ++-
 drivers/i2c/busses/i2c-designware-platdrv.c    |  5 +++--
 drivers/mfd/intel-lpss.c                       |  3 ++-
 drivers/mfd/intel_quark_i2c_gpio.c             |  5 +++--
 drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c |  7 ++++---
 include/linux/platform_data/i2c-designware.h   | 11 +++++++++++
 7 files changed, 26 insertions(+), 9 deletions(-)
 create mode 100644 include/linux/platform_data/i2c-designware.h

-- 
2.34.1

[PATCH v2 1/4] i2c: designware: Create shared header hosting driver name

[Florian Fainelli]

We have a number of drivers that reference the string "i2c_designware"
towards two goals:

- ensure their device gets bound to the i2c_designware platform_driver
- create a clock lookup reference that matches the i2c_designware
  instance number for the i2c-designware-platdrv.c driver to be able to
  lookup the input reference clock

Since this string is copied in a bunch of different places and since it
is possible to get this named wrong (see [1] and [2]) with unintended
consequences, create a header file that hosts that define for other
drivers to use and all agree on the correct name to use.

[1]:
https://lore.kernel.org/all/20240422084109.3201-1-duanqiangwen@net-swift.com/
[2]:
https://lore.kernel.org/all/20240422084109.3201-2-duanqiangwen@net-swift.com/

Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 MAINTAINERS                                  |  1 +
 drivers/i2c/busses/i2c-designware-pcidrv.c   |  3 ++-
 drivers/i2c/busses/i2c-designware-platdrv.c  |  5 +++--
 include/linux/platform_data/i2c-designware.h | 11 +++++++++++
 4 files changed, 17 insertions(+), 3 deletions(-)
 create mode 100644 include/linux/platform_data/i2c-designware.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 2d5acd6d98c4..da1b39df2b94 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21438,6 +21438,7 @@ R:	Jan Dabros <jsd@semihalf.com>
 L:	linux-i2c@vger.kernel.org
 S:	Supported
 F:	drivers/i2c/busses/i2c-designware-*
+F:	include/linux/platform_data/i2c-designware.h
 
 SYNOPSYS DESIGNWARE MMC/SD/SDIO DRIVER
 M:	Jaehoon Chung <jh80.chung@samsung.com>
diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
index 9be9a2658e1f..32d6f338a1ea 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -19,6 +19,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <linux/platform_data/i2c-designware.h>
 #include <linux/pm_runtime.h>
 #include <linux/power_supply.h>
 #include <linux/sched.h>
@@ -425,7 +426,7 @@ static struct pci_driver dw_i2c_driver = {
 module_pci_driver(dw_i2c_driver);
 
 /* Work with hotplug and coldplug */
-MODULE_ALIAS("i2c_designware-pci");
+MODULE_ALIAS(I2C_DESIGNWARE_NAME "-pci");
 MODULE_AUTHOR("Baruch Siach <baruch@tkos.co.il>");
 MODULE_DESCRIPTION("Synopsys DesignWare PCI I2C bus adapter");
 MODULE_LICENSE("GPL");
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 4ab41ba39d55..8aa2bf274d78 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -22,6 +22,7 @@
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/platform_data/i2c-designware.h>
 #include <linux/platform_device.h>
 #include <linux/pm.h>
 #include <linux/pm_runtime.h>
@@ -480,13 +481,13 @@ static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
 };
 
 /* Work with hotplug and coldplug */
-MODULE_ALIAS("platform:i2c_designware");
+MODULE_ALIAS("platform:" I2C_DESIGNWARE_NAME);
 
 static struct platform_driver dw_i2c_driver = {
 	.probe = dw_i2c_plat_probe,
 	.remove_new = dw_i2c_plat_remove,
 	.driver		= {
-		.name	= "i2c_designware",
+		.name	= I2C_DESIGNWARE_NAME,
 		.of_match_table = of_match_ptr(dw_i2c_of_match),
 		.acpi_match_table = ACPI_PTR(dw_i2c_acpi_match),
 		.pm	= pm_ptr(&dw_i2c_dev_pm_ops),
diff --git a/include/linux/platform_data/i2c-designware.h b/include/linux/platform_data/i2c-designware.h
new file mode 100644
index 000000000000..e385b6d70a04
--- /dev/null
+++ b/include/linux/platform_data/i2c-designware.h
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef __I2C_DESIGNWARE_PDATA_H__
+#define __I2C_DESIGNWARE_PDATA_H__
+
+/* This is the i2c-designware-platdrv.c platform driver name. This name is used
+ * to bind the device to the driver, as well as by the driver itself to request
+ * the input reference clock
+ */
+#define I2C_DESIGNWARE_NAME	"i2c_designware"
+
+#endif /* __I2C_DESIGNWARE_PDATA_H__ */
-- 
2.34.1

Re: [PATCH v2 1/4] i2c: designware: Create shared header hosting driver name

[Andy Shevchenko]

On Wed, Apr 24, 2024 at 05:26:39PM -0700, Florian Fainelli wrote:
> We have a number of drivers that reference the string "i2c_designware"
> towards two goals:
> 
> - ensure their device gets bound to the i2c_designware platform_driver
> - create a clock lookup reference that matches the i2c_designware
>   instance number for the i2c-designware-platdrv.c driver to be able to
>   lookup the input reference clock
> 
> Since this string is copied in a bunch of different places and since it
> is possible to get this named wrong (see [1] and [2]) with unintended
> consequences, create a header file that hosts that define for other
> drivers to use and all agree on the correct name to use.

> [1]:
> https://lore.kernel.org/all/20240422084109.3201-1-duanqiangwen@net-swift.com/
> [2]:
> https://lore.kernel.org/all/20240422084109.3201-2-duanqiangwen@net-swift.com/

Make them tags.

Link: URL#1 [1]
Link: URL#2 [2]

> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>

> +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
> @@ -19,6 +19,7 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>

> +#include <linux/platform_data/i2c-designware.h>

Please, make it a separate group after linux/* and before "xxx" below.

>  #include <linux/pm_runtime.h>
>  #include <linux/power_supply.h>
>  #include <linux/sched.h>

...

>  module_pci_driver(dw_i2c_driver);
>  
>  /* Work with hotplug and coldplug */
> -MODULE_ALIAS("i2c_designware-pci");
> +MODULE_ALIAS(I2C_DESIGNWARE_NAME "-pci");

As Jarkko said, please get rid of this.
You may take my patch from here:
https://lore.kernel.org/linux-i2c/20231207141653.2785124-9-andriy.shevchenko@linux.intel.com/
and incorporate in this series.

...

>  #include <linux/mfd/syscon.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/platform_data/i2c-designware.h>

As per above.

>  #include <linux/platform_device.h>
>  #include <linux/pm.h>
>  #include <linux/pm_runtime.h>

...

>  /* Work with hotplug and coldplug */
> -MODULE_ALIAS("platform:i2c_designware");
> +MODULE_ALIAS("platform:" I2C_DESIGNWARE_NAME);

See above.

...

> +/* This is the i2c-designware-platdrv.c platform driver name. This name is used
> + * to bind the device to the driver, as well as by the driver itself to request
> + * the input reference clock
> + */

/*
 * Use correct multi-line comment style. This
 * is not the net subsystem, we use traditional style.
 */

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 2/4] mfd: intel-lpss: Utilize i2c-designware.h

[Florian Fainelli]

Rather than open code the i2c_designware string, utilize the newly
defined constant in i2c-designware.h.

Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/mfd/intel-lpss.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/intel-lpss.c b/drivers/mfd/intel-lpss.c
index 2a9018112dfc..551560beff7b 100644
--- a/drivers/mfd/intel-lpss.c
+++ b/drivers/mfd/intel-lpss.c
@@ -24,6 +24,7 @@
 #include <linux/ioport.h>
 #include <linux/mfd/core.h>
 #include <linux/module.h>
+#include <linux/platform_data/i2c-designware.h>
 #include <linux/pm.h>
 #include <linux/pm_qos.h>
 #include <linux/pm_runtime.h>
@@ -116,7 +117,7 @@ static const struct mfd_cell intel_lpss_idma64_cell = {
 };
 
 static const struct mfd_cell intel_lpss_i2c_cell = {
-	.name = "i2c_designware",
+	.name = I2C_DESIGNWARE_NAME,
 	.num_resources = ARRAY_SIZE(intel_lpss_dev_resources),
 	.resources = intel_lpss_dev_resources,
 };
-- 
2.34.1

Re: [PATCH v2 2/4] mfd: intel-lpss: Utilize i2c-designware.h

[Andy Shevchenko]

On Wed, Apr 24, 2024 at 05:26:40PM -0700, Florian Fainelli wrote:
> Rather than open code the i2c_designware string, utilize the newly
> defined constant in i2c-designware.h.

...

> +#include <linux/platform_data/i2c-designware.h>

Please, group this with linux/dma/idma64.h below.

...

Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
in case this go anywhere.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/4] mfd: intel-lpss: Utilize i2c-designware.h

[Lee Jones]

On Wed, 24 Apr 2024, Florian Fainelli wrote:

> Rather than open code the i2c_designware string, utilize the newly
> defined constant in i2c-designware.h.
> 
> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---
>  drivers/mfd/intel-lpss.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mfd/intel-lpss.c b/drivers/mfd/intel-lpss.c
> index 2a9018112dfc..551560beff7b 100644
> --- a/drivers/mfd/intel-lpss.c
> +++ b/drivers/mfd/intel-lpss.c
> @@ -24,6 +24,7 @@
>  #include <linux/ioport.h>
>  #include <linux/mfd/core.h>
>  #include <linux/module.h>
> +#include <linux/platform_data/i2c-designware.h>
>  #include <linux/pm.h>
>  #include <linux/pm_qos.h>
>  #include <linux/pm_runtime.h>
> @@ -116,7 +117,7 @@ static const struct mfd_cell intel_lpss_idma64_cell = {
>  };
>  
>  static const struct mfd_cell intel_lpss_i2c_cell = {
> -	.name = "i2c_designware",
> +	.name = I2C_DESIGNWARE_NAME,
>  	.num_resources = ARRAY_SIZE(intel_lpss_dev_resources),
>  	.resources = intel_lpss_dev_resources,

I explained why I don't like this in v1 this morning.

Please take a look.

-- 
Lee Jones [李琼斯]

[PATCH v2 3/4] mfd: intel_quark_i2c_gpio: Utilize i2c-designware.h

[Florian Fainelli]

Rather than open code the i2c_designware string, utilize the newly
defined constant in i2c-designware.h.

Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/mfd/intel_quark_i2c_gpio.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/intel_quark_i2c_gpio.c b/drivers/mfd/intel_quark_i2c_gpio.c
index 9b9c76bd067b..08e296fc56f3 100644
--- a/drivers/mfd/intel_quark_i2c_gpio.c
+++ b/drivers/mfd/intel_quark_i2c_gpio.c
@@ -17,6 +17,7 @@
 #include <linux/clk-provider.h>
 #include <linux/dmi.h>
 #include <linux/i2c.h>
+#include <linux/platform_data/i2c-designware.h>
 #include <linux/property.h>
 
 /* PCI BAR for register base address */
@@ -30,7 +31,7 @@
 #define INTEL_QUARK_IORES_MEM	0
 #define INTEL_QUARK_IORES_IRQ	1
 
-#define INTEL_QUARK_I2C_CONTROLLER_CLK "i2c_designware.0"
+#define INTEL_QUARK_I2C_CONTROLLER_CLK I2C_DESIGNWARE_NAME ".0"
 
 /* The Quark I2C controller source clock */
 #define INTEL_QUARK_I2C_CLK_HZ	33000000
@@ -136,7 +137,7 @@ static const struct software_node *intel_quark_gpio_node_group[] = {
 static struct mfd_cell intel_quark_mfd_cells[] = {
 	[MFD_I2C_BAR] = {
 		.id = MFD_I2C_BAR,
-		.name = "i2c_designware",
+		.name = I2C_DESIGNWARE_NAME,
 		.acpi_match = &intel_quark_acpi_match_i2c,
 		.num_resources = ARRAY_SIZE(intel_quark_i2c_res),
 		.resources = intel_quark_i2c_res,
-- 
2.34.1

Re: [PATCH v2 3/4] mfd: intel_quark_i2c_gpio: Utilize i2c-designware.h

[Andy Shevchenko]

On Wed, Apr 24, 2024 at 05:26:41PM -0700, Florian Fainelli wrote:
> Rather than open code the i2c_designware string, utilize the newly
> defined constant in i2c-designware.h.

...

> +++ b/drivers/mfd/intel_quark_i2c_gpio.c
> @@ -17,6 +17,7 @@
>  #include <linux/clk-provider.h>
>  #include <linux/dmi.h>
>  #include <linux/i2c.h>

> +#include <linux/platform_data/i2c-designware.h>

Make it a separate group after linux/* ...

>  #include <linux/property.h>
>  

...here.

...

> -#define INTEL_QUARK_I2C_CONTROLLER_CLK "i2c_designware.0"
> +#define INTEL_QUARK_I2C_CONTROLLER_CLK I2C_DESIGNWARE_NAME ".0"

I'm not fan of this, but I think creating another macro to help with
constant device instance naming might be more cumbersome and overkill.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 4/4] net: txgbe: Utilize i2c-designware.h

[Florian Fainelli]

Rather than open code the i2c_designware string, utilize the newly
defined constant in i2c-designware.h.

Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
index 93295916b1d2..7545aacc9c19 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
@@ -8,6 +8,7 @@
 #include <linux/clkdev.h>
 #include <linux/i2c.h>
 #include <linux/pci.h>
+#include <linux/platform_data/i2c-designware.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/pcs/pcs-xpcs.h>
@@ -571,8 +572,8 @@ static int txgbe_clock_register(struct txgbe *txgbe)
 	char clk_name[32];
 	struct clk *clk;
 
-	snprintf(clk_name, sizeof(clk_name), "i2c_designware.%d",
-		 pci_dev_id(pdev));
+	snprintf(clk_name, sizeof(clk_name), "%s.%d",
+		 I2C_DESIGNWARE_NAME, pci_dev_id(pdev));
 
 	clk = clk_register_fixed_rate(NULL, clk_name, NULL, 0, 156250000);
 	if (IS_ERR(clk))
@@ -634,7 +635,7 @@ static int txgbe_i2c_register(struct txgbe *txgbe)
 
 	info.parent = &pdev->dev;
 	info.fwnode = software_node_fwnode(txgbe->nodes.group[SWNODE_I2C]);
-	info.name = "i2c_designware";
+	info.name = I2C_DESIGNWARE_NAME;
 	info.id = pci_dev_id(pdev);
 
 	info.res = &DEFINE_RES_IRQ(pdev->irq);
-- 
2.34.1

Re: [PATCH v2 4/4] net: txgbe: Utilize i2c-designware.h

[Andy Shevchenko]

On Wed, Apr 24, 2024 at 05:26:42PM -0700, Florian Fainelli wrote:
> Rather than open code the i2c_designware string, utilize the newly
> defined constant in i2c-designware.h.

...

> +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
> @@ -8,6 +8,7 @@
>  #include <linux/clkdev.h>
>  #include <linux/i2c.h>
>  #include <linux/pci.h>
> +#include <linux/platform_data/i2c-designware.h>

Same comment, make this a separate group, it will be easier to see the quite
specific niche headers, that are not related to the generic libraries / or
subsystem-level ones.

>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
>  #include <linux/pcs/pcs-xpcs.h>

...

> -	snprintf(clk_name, sizeof(clk_name), "i2c_designware.%d",
> -		 pci_dev_id(pdev));
> +	snprintf(clk_name, sizeof(clk_name), "%s.%d",
> +		 I2C_DESIGNWARE_NAME, pci_dev_id(pdev));

Why do you make it %s? It was constant literal and is, no need to use %s.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/4] Define i2c_designware in a header file

[Jarkko Nikula]

On 4/25/24 3:26 AM, Florian Fainelli wrote:
> This patch series depends upon the following two patches being applied:
> 
> https://lore.kernel.org/all/20240422084109.3201-1-duanqiangwen@net-swift.com/
> https://lore.kernel.org/all/20240422084109.3201-2-duanqiangwen@net-swift.com/
> 
> There is no reason why each driver should have to repeat the
> "i2c_designware" string all over the place, because when that happens we
> see the reverts like the above being necessary.
> 
> Changes in v2:
> 
> - avoid changing i2c-designware-pcidrv.c more than necessary
> - move constant to include/linux/platform_data/i2c-designware.h
> - add comments as to how this constant is used and why
> 
> Florian Fainelli (4):
>    i2c: designware: Create shared header hosting driver name
>    mfd: intel-lpss: Utilize i2c-designware.h
>    mfd: intel_quark_i2c_gpio: Utilize i2c-designware.h
>    net: txgbe: Utilize i2c-designware.h
> 
>   MAINTAINERS                                    |  1 +
>   drivers/i2c/busses/i2c-designware-pcidrv.c     |  3 ++-
>   drivers/i2c/busses/i2c-designware-platdrv.c    |  5 +++--
>   drivers/mfd/intel-lpss.c                       |  3 ++-
>   drivers/mfd/intel_quark_i2c_gpio.c             |  5 +++--
>   drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c |  7 ++++---
>   include/linux/platform_data/i2c-designware.h   | 11 +++++++++++
>   7 files changed, 26 insertions(+), 9 deletions(-)
>   create mode 100644 include/linux/platform_data/i2c-designware.h
> 
I have mixed feeling about this set will it help maintaining and 
developing new code or do the opposite. Surely misnaming as referenced 
above can happen but I think it's not too common pattern while having 
single define header put under include/ feels added burden.

Also I personally like explicit strings put into MODULE_ALIAS() since 
they are easier to grep from sources and modules.alias file when 
debugging autoloading issues. So change like below makes that debugging 
one step more labor.

-MODULE_ALIAS("platform:i2c_designware");
+MODULE_ALIAS("platform:" I2C_DESIGNWARE_NAME);