Re: [PATCH 05/24] thunderbolt: Rework capability handling

[Mika Westerberg]

On Thu, May 18, 2017 at 07:38:29PM +0300, Andy Shevchenko wrote:
> On Thu, May 18, 2017 at 5:38 PM, Mika Westerberg
> <mika.westerberg@xxxxxxxxxxxxxxx> wrote:
> > Organization of the capabilities in switches and ports is not so random
> > after all. Rework the capability handling functionality so that it
> > follows how capabilities are organized and provide two new functions
> > (tb_switch_find_vsec_cap() and tb_port_find_cap()) which can be used to
> > extract capabilities for ports and switches. Then convert the current
> > users over these.
> 
> One nit here.
> 
> > +int tb_port_find_cap(struct tb_port *port, enum tb_port_cap cap)
> >  {
> 
> > +       u32 offset;
> >
> 
> > +       /*
> > +        * DP out adapters claim to implement TMU capability but in
> > +        * reality they do not so we hard code the adapter specific
> > +        * capability offset here.
> > +        */
> > +       if (port->config.type == TB_TYPE_DP_HDMI_OUT)
> > +               offset = 0x39;
> >         else
> 
> > +               offset = 0x1;
> > +
> > +       do {
> > +               struct tb_cap_any header;
> > +               int ret;
> > +
> > +               ret = tb_port_read(port, &header, TB_CFG_PORT, offset, 1);
> > +               if (ret)
> > +                       return ret;
> > +
> > +               if (header.basic.cap == cap)
> > +                       return offset;
> > +
> > +               offset = header.basic.next;
> > +       } while (offset);
> > +
> > +       return -ENOENT;
> >  }
> 
> > +static int tb_switch_find_cap(struct tb_switch *sw, enum tb_switch_cap cap)
> >  {
> 
> > +       int offset = sw->config.first_cap_offset;
> > +
> > +       while (offset > 0 && offset < CAP_OFFSET_MAX) {
> > +               struct tb_cap_any header;
> > +               int ret;
> > +
> > +               ret = tb_sw_read(sw, &header, TB_CFG_SWITCH, offset, 1);
> > +               if (ret)
> > +                       return ret;
> > +
> > +               if (header.basic.cap == cap)
> > +                       return offset;
> > +
> > +               offset = header.basic.next;
> >         }
> 
> > +
> > +       return -ENOENT;
> >  }
> 
> Both has quite similar bodies.
> Wouldn't be nice to split out a helper which takes initial offset and
> type as parameters?

The whole point of this rework was to separate port vs. switch
capability to follow how the hardware is organized instead of having one
more complex function handling everything :)

Sure, I can merge them back together but IMHO it will be not that
readable anymore.