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
