[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20191006121536.6u7noudvfiri5h3s@wunner.de>
Date: Sun, 6 Oct 2019 14:15:36 +0200
From: Lukas Wunner <lukas@...ner.de>
To: Mika Westerberg <mika.westerberg@...ux.intel.com>
Cc: Oliver Neukum <oneukum@...e.com>, linux-usb@...r.kernel.org,
Anthony Wong <anthony.wong@...onical.com>,
Mario.Limonciello@...l.com,
Andreas Noever <andreas.noever@...il.com>,
Yehezkel Bernat <YehezkelShB@...il.com>,
Michael Jamet <michael.jamet@...el.com>,
Rajmohan Mani <rajmohan.mani@...el.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Nicholas Johnson <nicholas.johnson-opensource@...look.com.au>,
Alan Stern <stern@...land.harvard.edu>,
linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 05/22] thunderbolt: Add helper macros to iterate over
switch ports
On Wed, Oct 02, 2019 at 05:28:59PM +0300, Mika Westerberg wrote:
> On Wed, Oct 02, 2019 at 04:17:54PM +0200, Oliver Neukum wrote:
> > Am Dienstag, den 01.10.2019, 14:38 +0300 schrieb Mika Westerberg:
> > > @@ -1975,10 +1972,8 @@ void tb_switch_suspend(struct tb_switch *sw)
> > > if (err)
> > > return;
> > >
> > > - for (i = 1; i <= sw->config.max_port_number; i++) {
> > > - if (tb_port_has_remote(&sw->ports[i]))
> > > - tb_switch_suspend(sw->ports[i].remote->sw);
> > > - }
> > > + tb_switch_for_each_remote_port(sw, i)
> > > + tb_switch_suspend(sw->ports[i].remote->sw);
> >
> > This macro looks a bit prone to misunderstanding.
> > I guess the function would be better if the test could be seen.
>
> The macro does not really save too many lines so I think I can just drop
> this patch for now and keep these open-coded.
Introducing a macro is fine.
However instead of using an index "i" as iterator, I'd suggest using a
pointer to struct tb_port. This reduces the horizontal width of the
code because you can replace the "sw->ports[i]" with just "port".
In most of the loops this also saves 1 line for an assignment:
"struct tb_port *port = &sw->ports[i];"
In fact, I've already proposed such a macro more than a year ago
but it never got merged:
https://lore.kernel.org/patchwork/patch/983863/
/**
* tb_sw_for_each_port() - iterate over ports on a switch
* @switch: pointer to struct tb_switch over whose ports shall be iterated
* @port: pointer to struct tb_port which shall be used as the iterator
*
* Excludes port 0, which is the switch itself and therefore irrelevant for
* most iterations.
*/
#define tb_sw_for_each_port(switch, port) \
for (port = &(switch)->ports[1]; \
port <= &(switch)->ports[(switch)->config.max_port_number]; \
port++)
Thanks,
Lukas
Powered by blists - more mailing lists