This patch series depends upon the following two patches being applied:
https://lore.kernel.org/all/[email protected]/
https://lore.kernel.org/all/[email protected]/
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
Rather than open code the i2c_designware string, utilize the newly
defined constant in i2c-designware.h.
Signed-off-by: Florian Fainelli <[email protected]>
---
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
Rather than open code the i2c_designware string, utilize the newly
defined constant in i2c-designware.h.
Signed-off-by: Florian Fainelli <[email protected]>
---
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
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 <[email protected]>
in case this go anywhere.
--
With Best Regards,
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
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/[email protected]/
> https://lore.kernel.org/all/[email protected]/
>
> 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);
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 <[email protected]>
> ---
> 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 [李琼斯]