From: Lee Jones <lee@kernel.org>
To: Florian Fainelli <florian.fainelli@broadcom.com>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>,
linux-kernel@vger.kernel.org,
Jarkko Nikula <jarkko.nikula@linux.intel.com>,
Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
Mika Westerberg <mika.westerberg@linux.intel.com>,
Jan Dabros <jsd@semihalf.com>, Andi Shyti <andi.shyti@kernel.org>,
Jiawen Wu <jiawenwu@trustnetic.com>,
Mengyuan Lou <mengyuanlou@net-swift.com>,
"David S. Miller" <davem@davemloft.net>,
Eric Dumazet <edumazet@google.com>,
Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
Maciej Fijalkowski <maciej.fijalkowski@intel.com>,
Andrew Lunn <andrew@lunn.ch>,
Duanqiang Wen <duanqiangwen@net-swift.com>,
"open list:SYNOPSYS DESIGNWARE I2C DRIVER"
<linux-i2c@vger.kernel.org>,
"open list:WANGXUN ETHERNET DRIVER" <netdev@vger.kernel.org>
Subject: Re: [PATCH 2/4] mfd: intel-lpss: Utilize i2c-designware.h
Date: Thu, 2 May 2024 17:42:44 +0100 [thread overview]
Message-ID: <20240502164244.GA1200070@google.com> (raw)
In-Reply-To: <6646b690-7b05-4a0e-a524-375b389ad591@broadcom.com>
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 [李琼斯]
next prev parent reply other threads:[~2024-05-02 16:42 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-23 23:36 [PATCH 0/4] Define i2c_designware in a header file Florian Fainelli
2024-04-23 23:36 ` [PATCH 1/4] i2c: designware: Create shared header hosting driver name Florian Fainelli
2024-04-23 23:59 ` Andy Shevchenko
2024-04-24 1:22 ` Florian Fainelli
2024-04-24 12:18 ` Andrew Lunn
2024-04-24 12:45 ` Andy Shevchenko
2024-04-25 8:33 ` Jarkko Nikula
2024-04-25 9:20 ` Andy Shevchenko
2024-04-23 23:36 ` [PATCH 2/4] mfd: intel-lpss: Utilize i2c-designware.h Florian Fainelli
2024-04-24 0:00 ` Andy Shevchenko
2024-04-24 3:20 ` Florian Fainelli
2024-05-02 7:17 ` Lee Jones
2024-05-02 16:19 ` Florian Fainelli
2024-05-02 16:42 ` Lee Jones [this message]
2024-04-23 23:36 ` [PATCH 3/4] mfd: intel_quark_i2c_gpio: " Florian Fainelli
2024-04-24 0:01 ` Andy Shevchenko
2024-04-24 1:28 ` Florian Fainelli
2024-04-24 12:37 ` Andy Shevchenko
2024-04-24 16:35 ` Florian Fainelli
2024-04-23 23:36 ` [PATCH 4/4] net: txgbe: " Florian Fainelli
2024-04-24 16:14 ` Simon Horman
2024-04-24 16:22 ` Florian Fainelli
2024-04-24 17:34 ` Simon Horman
2024-04-23 23:56 ` [PATCH 0/4] Define i2c_designware in a header file Andy Shevchenko
2024-04-24 1:21 ` Florian Fainelli
2024-04-24 14:26 ` Andy Shevchenko
2024-04-24 16:26 ` Florian Fainelli
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240502164244.GA1200070@google.com \
--to=lee@kernel.org \
--cc=andi.shyti@kernel.org \
--cc=andrew@lunn.ch \
--cc=andriy.shevchenko@linux.intel.com \
--cc=andy.shevchenko@gmail.com \
--cc=davem@davemloft.net \
--cc=duanqiangwen@net-swift.com \
--cc=edumazet@google.com \
--cc=florian.fainelli@broadcom.com \
--cc=jarkko.nikula@linux.intel.com \
--cc=jiawenwu@trustnetic.com \
--cc=jsd@semihalf.com \
--cc=kuba@kernel.org \
--cc=linux-i2c@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=maciej.fijalkowski@intel.com \
--cc=mengyuanlou@net-swift.com \
--cc=mika.westerberg@linux.intel.com \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).