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

[PATCH 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.

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     | 5 +++--
 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/i2c-designware.h                 | 7 +++++++
 7 files changed, 23 insertions(+), 10 deletions(-)
 create mode 100644 include/linux/i2c-designware.h

-- 
2.34.1

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

[Florian Fainelli]

We have a number of drivers that reference the string "i2c_designware"
yet this is copied all over the places with opportunities for this
string being mis-used. Create a shared header that defines this as a
constant that other drivers can reference.

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

diff --git a/MAINTAINERS b/MAINTAINERS
index 2d5acd6d98c4..d5e10c747f65 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/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..948669265375 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -14,6 +14,7 @@
 #include <linux/err.h>
 #include <linux/errno.h>
 #include <linux/i2c.h>
+#include <linux/i2c-designware.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
@@ -27,7 +28,7 @@
 #include "i2c-designware-core.h"
 #include "i2c-ccgx-ucsi.h"
 
-#define DRIVER_NAME "i2c-designware-pci"
+#define DRIVER_NAME I2C_DESIGNWARE_NAME "-pci"
 
 enum dw_pci_ctl_id_t {
 	medfield,
@@ -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(DRIVER_NAME);
 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..dc335a22546b 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -16,6 +16,7 @@
 #include <linux/err.h>
 #include <linux/errno.h>
 #include <linux/i2c.h>
+#include <linux/i2c-designware.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/kernel.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/i2c-designware.h b/include/linux/i2c-designware.h
new file mode 100644
index 000000000000..f20240ac89c4
--- /dev/null
+++ b/include/linux/i2c-designware.h
@@ -0,0 +1,7 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+#ifndef __I2C_DESIGNWARE_H__
+#define __I2C_DESIGNWARE_H__
+
+#define I2C_DESIGNWARE_NAME	"i2c_designware"
+
+#endif /* __I2C_DESIGNWARE_H__ */
-- 
2.34.1

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

[Andy Shevchenko]

Tue, Apr 23, 2024 at 04:36:19PM -0700, Florian Fainelli kirjoitti:
> We have a number of drivers that reference the string "i2c_designware"
> yet this is copied all over the places with opportunities for this
> string being mis-used. Create a shared header that defines this as a
> constant that other drivers can reference.

...

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

Can it be hidden in the subfolder?

...

> -#define DRIVER_NAME "i2c-designware-pci"
> +#define DRIVER_NAME I2C_DESIGNWARE_NAME "-pci"

Oh, this makes all the things hard to read.

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

I believe we shouldn't use MODULE_ALIAS() without real justification.

...

> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c

All as per above.

-- 
With Best Regards,
Andy Shevchenko

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

[Florian Fainelli]

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



On 4/23/2024 4:59 PM, Andy Shevchenko wrote:
> Tue, Apr 23, 2024 at 04:36:19PM -0700, Florian Fainelli kirjoitti:
>> We have a number of drivers that reference the string "i2c_designware"
>> yet this is copied all over the places with opportunities for this
>> string being mis-used. Create a shared header that defines this as a
>> constant that other drivers can reference.
> 
> ...
> 
>>   #include <linux/i2c.h>
>> +#include <linux/i2c-designware.h>
> 
> Can it be hidden in the subfolder?

That would require the MFD and ethernet drivers to include relative to 
where they are in the source tree, do we really want that?

> 
> ...
> 
>> -#define DRIVER_NAME "i2c-designware-pci"
>> +#define DRIVER_NAME I2C_DESIGNWARE_NAME "-pci"
> 
> Oh, this makes all the things hard to read.

OK, besides there is a change for '_' when it was a '-' before, so maybe 
I should drop that hunk.

> 
>>   /* Work with hotplug and coldplug */
>> -MODULE_ALIAS("i2c_designware-pci");
>> +MODULE_ALIAS(DRIVER_NAME);
> 
> I believe we shouldn't use MODULE_ALIAS() without real justification.

Pre-existing change.
-- 
Florian

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

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

[Andrew Lunn]

> > >   #include <linux/i2c.h>
> > > +#include <linux/i2c-designware.h>
> > 
> > Can it be hidden in the subfolder?
> 
> That would require the MFD and ethernet drivers to include relative to where
> they are in the source tree, do we really want that?

Maybe linux/platform_data/i2c-designware.h ? There are a few NAME
macros in there.

       Andrew

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

[Andy Shevchenko]

On Wed, Apr 24, 2024 at 3:18 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > > >   #include <linux/i2c.h>
> > > > +#include <linux/i2c-designware.h>
> > >
> > > Can it be hidden in the subfolder?
> >
> > That would require the MFD and ethernet drivers to include relative to where
> > they are in the source tree, do we really want that?
>
> Maybe linux/platform_data/i2c-designware.h ? There are a few NAME
> macros in there.

Yes, under subfolder I meant something like
include/linux/$SOMETHING/_this_header_.h or even deeper.

-- 
With Best Regards,
Andy Shevchenko

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

[Jarkko Nikula]

On 4/24/24 2:59 AM, Andy Shevchenko wrote:
> Tue, Apr 23, 2024 at 04:36:19PM -0700, Florian Fainelli kirjoitti:
>> We have a number of drivers that reference the string "i2c_designware"
>> yet this is copied all over the places with opportunities for this
>> string being mis-used. Create a shared header that defines this as a
>> constant that other drivers can reference.
> 
> ...
> 
>>   #include <linux/i2c.h>
>> +#include <linux/i2c-designware.h>
> 
> Can it be hidden in the subfolder?
> 
> ...
> 
>> -#define DRIVER_NAME "i2c-designware-pci"
>> +#define DRIVER_NAME I2C_DESIGNWARE_NAME "-pci"
> 
> Oh, this makes all the things hard to read.
> 
>>   /* Work with hotplug and coldplug */
>> -MODULE_ALIAS("i2c_designware-pci");
>> +MODULE_ALIAS(DRIVER_NAME);
> 
> I believe we shouldn't use MODULE_ALIAS() without real justification.
> 
I think MODULE_ALIAS() is even needless here since this device is not 
added from another driver but loaded only for known PCI IDs in device table.

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

[Andy Shevchenko]

On Thu, Apr 25, 2024 at 11:33:02AM +0300, Jarkko Nikula wrote:
> On 4/24/24 2:59 AM, Andy Shevchenko wrote:
> > Tue, Apr 23, 2024 at 04:36:19PM -0700, Florian Fainelli kirjoitti:
> > > We have a number of drivers that reference the string "i2c_designware"
> > > yet this is copied all over the places with opportunities for this
> > > string being mis-used. Create a shared header that defines this as a
> > > constant that other drivers can reference.

...

> > >   #include <linux/i2c.h>
> > > +#include <linux/i2c-designware.h>
> > 
> > Can it be hidden in the subfolder?

...

> > > -#define DRIVER_NAME "i2c-designware-pci"
> > > +#define DRIVER_NAME I2C_DESIGNWARE_NAME "-pci"
> > 
> > Oh, this makes all the things hard to read.
> > 
> > >   /* Work with hotplug and coldplug */
> > > -MODULE_ALIAS("i2c_designware-pci");
> > > +MODULE_ALIAS(DRIVER_NAME);
> > 
> > I believe we shouldn't use MODULE_ALIAS() without real justification.
> > 
> I think MODULE_ALIAS() is even needless here since this device is not added
> from another driver but loaded only for known PCI IDs in device table.

The patch that removes it got reverted :-( and then removed completely from PR.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 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..4d2c5250046f 100644
--- a/drivers/mfd/intel-lpss.c
+++ b/drivers/mfd/intel-lpss.c
@@ -19,6 +19,7 @@
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/gfp_types.h>
+#include <linux/i2c-designware.h>
 #include <linux/idr.h>
 #include <linux/io.h>
 #include <linux/ioport.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 2/4] mfd: intel-lpss: Utilize i2c-designware.h

[Andy Shevchenko]

Tue, Apr 23, 2024 at 04:36:20PM -0700, Florian Fainelli kirjoitti:
> Rather than open code the i2c_designware string, utilize the newly
> defined constant in i2c-designware.h.

...

>  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,
>  };

We have tons of drivers that are using explicit naming, why is this case
special? 

-- 
With Best Regards,
Andy Shevchenko

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

[Florian Fainelli]

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



On 4/23/2024 5:00 PM, Andy Shevchenko wrote:
> Tue, Apr 23, 2024 at 04:36:20PM -0700, Florian Fainelli kirjoitti:
>> Rather than open code the i2c_designware string, utilize the newly
>> defined constant in i2c-designware.h.
> 
> ...
> 
>>   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,
>>   };
> 
> We have tons of drivers that are using explicit naming, why is this case
> special?
> 

It is not special, just one of the 3 cases outside of drivers/i2c/busses 
that reference a driver living under drivers/i2c/busses, as I replied in 
the cover letter, this is a contract between the various device drivers 
and their users, so we should have a central place where it is defined, 
not repeated.
-- 
Florian

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

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

[Lee Jones]

On Tue, 23 Apr 2024, Florian Fainelli wrote:

> 
> 
> On 4/23/2024 5:00 PM, Andy Shevchenko wrote:
> > Tue, Apr 23, 2024 at 04:36:20PM -0700, Florian Fainelli kirjoitti:
> > > Rather than open code the i2c_designware string, utilize the newly
> > > defined constant in i2c-designware.h.
> > 
> > ...
> > 
> > >   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,
> > >   };
> > 
> > We have tons of drivers that are using explicit naming, why is this case
> > special?
> > 
> 
> It is not special, just one of the 3 cases outside of drivers/i2c/busses
> that reference a driver living under drivers/i2c/busses, as I replied in the
> cover letter, this is a contract between the various device drivers and
> their users, so we should have a central place where it is defined, not
> repeated.

I have always held the opinion that replacing user-facing strings with
defines harms debugability, since grepping becomes a multi-stage
process, often with ambiguous results (in the case of multiple
definitions with the same name.  Please keep the string in-place.

-- 
Lee Jones [李琼斯]

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

[Florian Fainelli]

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

On 5/2/24 00:17, Lee Jones wrote:
> On Tue, 23 Apr 2024, Florian Fainelli wrote:
> 
>>
>>
>> On 4/23/2024 5:00 PM, Andy Shevchenko wrote:
>>> Tue, Apr 23, 2024 at 04:36:20PM -0700, Florian Fainelli kirjoitti:
>>>> Rather than open code the i2c_designware string, utilize the newly
>>>> defined constant in i2c-designware.h.
>>>
>>> ...
>>>
>>>>    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,
>>>>    };
>>>
>>> We have tons of drivers that are using explicit naming, why is this case
>>> special?
>>>
>>
>> It is not special, just one of the 3 cases outside of drivers/i2c/busses
>> that reference a driver living under drivers/i2c/busses, as I replied in the
>> cover letter, this is a contract between the various device drivers and
>> their users, so we should have a central place where it is defined, not
>> repeated.
> 
> I have always held the opinion that replacing user-facing strings with
> defines harms debugability, since grepping becomes a multi-stage
> process, often with ambiguous results (in the case of multiple
> definitions with the same name.  Please keep the string in-place.

I am not buying into that argument and the fact that Duangiang was able 
to trip over the lack of an explicit contract between drivers seems like 
a bigger obstacle than doing a multi-stage grep. Anyway, I have no skin 
in this game, I just don't like seeing repetition and not stating 
contracts between drivers more explicitly.
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

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

[Lee Jones]

On Thu, 02 May 2024, Florian Fainelli wrote:

> On 5/2/24 00:17, Lee Jones wrote:
> > On Tue, 23 Apr 2024, Florian Fainelli wrote:
> > 
> > > 
> > > 
> > > On 4/23/2024 5:00 PM, Andy Shevchenko wrote:
> > > > Tue, Apr 23, 2024 at 04:36:20PM -0700, Florian Fainelli kirjoitti:
> > > > > Rather than open code the i2c_designware string, utilize the newly
> > > > > defined constant in i2c-designware.h.
> > > > 
> > > > ...
> > > > 
> > > > >    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,
> > > > >    };
> > > > 
> > > > We have tons of drivers that are using explicit naming, why is this case
> > > > special?
> > > > 
> > > 
> > > It is not special, just one of the 3 cases outside of drivers/i2c/busses
> > > that reference a driver living under drivers/i2c/busses, as I replied in the
> > > cover letter, this is a contract between the various device drivers and
> > > their users, so we should have a central place where it is defined, not
> > > repeated.
> > 
> > I have always held the opinion that replacing user-facing strings with
> > defines harms debugability, since grepping becomes a multi-stage
> > process, often with ambiguous results (in the case of multiple
> > definitions with the same name.  Please keep the string in-place.
> 
> I am not buying into that argument and the fact that Duangiang was able to
> trip over the lack of an explicit contract between drivers seems like a
> bigger obstacle than doing a multi-stage grep. Anyway, I have no skin in
> this game, I just don't like seeing repetition and not stating contracts
> between drivers more explicitly.

Good thing no one is asking you to buy into it then. :)

I'm not sure how or if the code that failed to match was tested or what
went wrong exactly and I'm pleased that the bug was caught and fixed.

However, swapping out matching strings with a define is a regression
from a development perspective.  One which I've felt the pain of
personally in the past.  I've pushed back on it before and I'm pushing
back again.  We're not swapping out matching strings for defines.

-- 
Lee Jones [李琼斯]

[PATCH 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..5ddc9a57ca73 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/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 3/4] mfd: intel_quark_i2c_gpio: Utilize i2c-designware.h

[Andy Shevchenko]

Tue, Apr 23, 2024 at 04:36:21PM -0700, Florian Fainelli kirjoitti:
> Rather than open code the i2c_designware string, utilize the newly
> defined constant in i2c-designware.h.

...

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

So, if you build a module separately for older version of the kernel (assuming
it allows you to modprobe), this won't work anymore.

-- 
With Best Regards,
Andy Shevchenko

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

[Florian Fainelli]

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



On 4/23/2024 5:01 PM, Andy Shevchenko wrote:
> Tue, Apr 23, 2024 at 04:36:21PM -0700, Florian Fainelli kirjoitti:
>> Rather than open code the i2c_designware string, utilize the newly
>> defined constant in i2c-designware.h.
> 
> ...
> 
>> -#define INTEL_QUARK_I2C_CONTROLLER_CLK "i2c_designware.0"
>> +#define INTEL_QUARK_I2C_CONTROLLER_CLK I2C_DESIGNWARE_NAME ".0"
> 
> So, if you build a module separately for older version of the kernel (assuming
> it allows you to modprobe), this won't work anymore.
> 

Sorry not following, was that comment supposed to be for patch #1 where 
I changed the i2c-designware-pci to i2c_designware-pci? modprobe 
recognizes both - and _ as interchangeable BTW.
-- 
Florian

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

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

[Andy Shevchenko]

On Wed, Apr 24, 2024 at 4:28 AM Florian Fainelli
<florian.fainelli@broadcom.com> wrote:
> On 4/23/2024 5:01 PM, Andy Shevchenko wrote:
> > Tue, Apr 23, 2024 at 04:36:21PM -0700, Florian Fainelli kirjoitti:
> >> Rather than open code the i2c_designware string, utilize the newly
> >> defined constant in i2c-designware.h.

...

> >> -#define INTEL_QUARK_I2C_CONTROLLER_CLK "i2c_designware.0"
> >> +#define INTEL_QUARK_I2C_CONTROLLER_CLK I2C_DESIGNWARE_NAME ".0"
> >
> > So, if you build a module separately for older version of the kernel (assuming
> > it allows you to modprobe), this won't work anymore.
>
> Sorry not following, was that comment supposed to be for patch #1 where
> I changed the i2c-designware-pci to i2c_designware-pci? modprobe
> recognizes both - and _ as interchangeable BTW.

I'm talking about something different. Let's assume you have a running
kernel (w.o. signature or version requirement for the modules), then
you have a new patch on top of it and then for an unknown reason you
changed. e.g., designware to DW in that definition. The newly built
module may not be loaded on the running kernel. Also note, here is the
instance name and not an ID in use. The replacement is wrong
semantically.

-- 
With Best Regards,
Andy Shevchenko

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

[Florian Fainelli]

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

On 4/24/24 05:37, Andy Shevchenko wrote:
> On Wed, Apr 24, 2024 at 4:28 AM Florian Fainelli
> <florian.fainelli@broadcom.com> wrote:
>> On 4/23/2024 5:01 PM, Andy Shevchenko wrote:
>>> Tue, Apr 23, 2024 at 04:36:21PM -0700, Florian Fainelli kirjoitti:
>>>> Rather than open code the i2c_designware string, utilize the newly
>>>> defined constant in i2c-designware.h.
> 
> ...
> 
>>>> -#define INTEL_QUARK_I2C_CONTROLLER_CLK "i2c_designware.0"
>>>> +#define INTEL_QUARK_I2C_CONTROLLER_CLK I2C_DESIGNWARE_NAME ".0"
>>>
>>> So, if you build a module separately for older version of the kernel (assuming
>>> it allows you to modprobe), this won't work anymore.
>>
>> Sorry not following, was that comment supposed to be for patch #1 where
>> I changed the i2c-designware-pci to i2c_designware-pci? modprobe
>> recognizes both - and _ as interchangeable BTW.
> 
> I'm talking about something different. Let's assume you have a running
> kernel (w.o. signature or version requirement for the modules), then
> you have a new patch on top of it and then for an unknown reason you
> changed. e.g., designware to DW in that definition. The newly built
> module may not be loaded on the running kernel. Also note, here is the
> instance name and not an ID in use. The replacement is wrong
> semantically.
> 

See my response in the cover letter, the instance base name is not 
independent from the i2c-designware-platdrv::driver::name because 
otherwise the clock lookup done by devm_clk_get_optional() will fail. So 
that change in this patch is entirely intentional and actually ensures 
correctness if someone were to change the i2c_designware platform driver 
name in the future for whatever reasons.

As far as catering to the specific example you gave, is not this just 
fraught with peril regardless of what is being changed in the kernel? 
Any constant that is serves as a contract between independent parts 
getting out of sync will result in some misbehavior. The only solution 
that I can think of which is edging towards over engineering is to 
export a string symbol which contains I2C_DESIGNWARE_NAME and then make 
other modules dependent upon that symbol to enforce some sort of runtime 
resolution, though I think your example could still be made to fail.
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

[PATCH 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..c601eea164f1 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
@@ -7,6 +7,7 @@
 #include <linux/clk-provider.h>
 #include <linux/clkdev.h>
 #include <linux/i2c.h>
+#include <linux/i2c-designware.h>
 #include <linux/pci.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.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 4/4] net: txgbe: Utilize i2c-designware.h

[Simon Horman]

On Tue, Apr 23, 2024 at 04:36:22PM -0700, 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>

Hi Florian,

FYI, this conflicts with:

c644920ce922 ("net: txgbe: fix i2c dev name cannot match clkdev")

But a patch-set has been submitted which reverts that commit:

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

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

[Florian Fainelli]

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

On 4/24/24 09:14, Simon Horman wrote:
> On Tue, Apr 23, 2024 at 04:36:22PM -0700, 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>
> 
> Hi Florian,
> 
> FYI, this conflicts with:
> 
> c644920ce922 ("net: txgbe: fix i2c dev name cannot match clkdev")
> 
> But a patch-set has been submitted which reverts that commit:
> 
> https://lore.kernel.org/all/20240422084109.3201-1-duanqiangwen@net-swift.com/

That's right, I mentioned in the cover letter this was based on top of 
Duanqiang's couple patches. Thanks Simon!
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

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

[Simon Horman]

On Wed, Apr 24, 2024 at 09:22:49AM -0700, Florian Fainelli wrote:
> On 4/24/24 09:14, Simon Horman wrote:
> > On Tue, Apr 23, 2024 at 04:36:22PM -0700, 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>
> > 
> > Hi Florian,
> > 
> > FYI, this conflicts with:
> > 
> > c644920ce922 ("net: txgbe: fix i2c dev name cannot match clkdev")
> > 
> > But a patch-set has been submitted which reverts that commit:
> > 
> > https://lore.kernel.org/all/20240422084109.3201-1-duanqiangwen@net-swift.com/
> 
> That's right, I mentioned in the cover letter this was based on top of
> Duanqiang's couple patches. Thanks Simon!

Thanks Florian,

Sorry for missing that.

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

[Andy Shevchenko]

Tue, Apr 23, 2024 at 04:36:18PM -0700, Florian Fainelli kirjoitti:
> 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.

Isn't that a part of ABI between drivers, i.e. whenever ones want to
request_module() or so they need to know what they are doing, no?

-- 
With Best Regards,
Andy Shevchenko

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

[Florian Fainelli]

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



On 4/23/2024 4:56 PM, Andy Shevchenko wrote:
> Tue, Apr 23, 2024 at 04:36:18PM -0700, Florian Fainelli kirjoitti:
>> 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.
> 
> Isn't that a part of ABI between drivers, i.e. whenever ones want to
> request_module() or so they need to know what they are doing, no?

Yes, the drivers should know, but as evidenced by the two patches above, 
there was still room for error. If we have to abide by a certain 
contract, which is platform_driver::driver::name, then we might as well 
have a header defining it no?
-- 
Florian

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

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

[Andy Shevchenko]

On Tue, Apr 23, 2024 at 06:21:21PM -0700, Florian Fainelli wrote:
> On 4/23/2024 4:56 PM, Andy Shevchenko wrote:
> > Tue, Apr 23, 2024 at 04:36:18PM -0700, Florian Fainelli kirjoitti:
> > > 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.
> > 
> > Isn't that a part of ABI between drivers, i.e. whenever ones want to
> > request_module() or so they need to know what they are doing, no?
> 
> Yes, the drivers should know, but as evidenced by the two patches above,
> there was still room for error. If we have to abide by a certain contract,
> which is platform_driver::driver::name, then we might as well have a header
> defining it no?

Maybe, I simply don't like the manipulations with parts of the device instance
names / driver IDs / driver name, which all become mixed after this series.

-- 
With Best Regards,
Andy Shevchenko

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

[Florian Fainelli]

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

On 4/24/24 07:26, Andy Shevchenko wrote:
> On Tue, Apr 23, 2024 at 06:21:21PM -0700, Florian Fainelli wrote:
>> On 4/23/2024 4:56 PM, Andy Shevchenko wrote:
>>> Tue, Apr 23, 2024 at 04:36:18PM -0700, Florian Fainelli kirjoitti:
>>>> 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.
>>>
>>> Isn't that a part of ABI between drivers, i.e. whenever ones want to
>>> request_module() or so they need to know what they are doing, no?
>>
>> Yes, the drivers should know, but as evidenced by the two patches above,
>> there was still room for error. If we have to abide by a certain contract,
>> which is platform_driver::driver::name, then we might as well have a header
>> defining it no?
> 
> Maybe, I simply don't like the manipulations with parts of the device instance
> names / driver IDs / driver name, which all become mixed after this series.
> 

That is fair enough, although there is definitively an expectation that 
the clock name will match the dev_name() of the i2c bus, which is why it 
changing or shortening the names as attempted by Duanqiang ended up not 
working.

This call in i2c-designware-platdrv.c is:

dev->clk = devm_clk_get_optional(&pdev->dev, NULL);

and because the name is NULL, we end-up searching for a clock named 
dev_name(), and if we have a mismatch, then we won't find it.

So the i2c_designware name is really something we must be sticking with, 
or change *globally* for a) device(s) binding to the driver and b) a 
successful clock search.
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]