From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1751699AbdESIMm (ORCPT <rfc822;w@1wt.eu>);
        Fri, 19 May 2017 04:12:42 -0400
Received: from mga09.intel.com ([134.134.136.24]:44578 "EHLO mga09.intel.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1750755AbdESIMl (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
        Fri, 19 May 2017 04:12:41 -0400
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.38,363,1491289200"; 
   d="scan'208";a="1132474794"
Date: Fri, 19 May 2017 11:12:34 +0300
From: Mika Westerberg <mika.westerberg@linux.intel.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Andreas Noever <andreas.noever@gmail.com>,
        Michael Jamet <michael.jamet@intel.com>,
        Yehezkel Bernat <yehezkel.bernat@intel.com>,
        Lukas Wunner <lukas@wunner.de>, Amir Levy <amir.jer.levy@intel.com>,
        Andy Lutomirski <luto@kernel.org>,
        Mario Limonciello <Mario.Limonciello@dell.com>,
        Jared.Dominguez@dell.com,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 05/24] thunderbolt: Rework capability handling
Message-ID: <20170519081234.GC3267@lahna.fi.intel.com>
References: <20170518143914.60902-1-mika.westerberg@linux.intel.com>
 <20170518143914.60902-6-mika.westerberg@linux.intel.com>
 <CAHp75Vc-=AknaHoOuAYtamjmKuuFJESZhdYDqHwZhJ=1B50Zzg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAHp75Vc-=AknaHoOuAYtamjmKuuFJESZhdYDqHwZhJ=1B50Zzg@mail.gmail.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
User-Agent: Mutt/1.7.1 (2016-10-04)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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@linux.intel.com> 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.