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.
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
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..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
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..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
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 <[email protected]>
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/[email protected]/
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 <[email protected]>
>
> 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/[email protected]/
That's right, I mentioned in the cover letter this was based on top of
Duanqiang's couple patches. Thanks Simon!
--
Florian
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 <[email protected]>
> >
> > 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/[email protected]/
>
> 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.
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 [李琼斯]