Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190705095800.43534-1-mika.westerberg@linux.intel.com>
 <20190705095800.43534-8-mika.westerberg@linux.intel.com> <CA+CmpXsak9Rvkq_RNzoxRecMercUPKqdK+KzbHv_fJC59inaHA@mail.gmail.com>
 <20190705145106.GA2640@lahna.fi.intel.com>
In-Reply-To: <20190705145106.GA2640@lahna.fi.intel.com>
From:   Yehezkel Bernat <yehezkelshb@gmail.com>
Date:   Fri, 5 Jul 2019 18:02:23 +0300
Message-ID: <CA+CmpXu27CfWeEN2F6YrtVzHTQtas6KyTk6dz4jaeo8LmckHWQ@mail.gmail.com>
Subject: Re: [PATCH 7/8] thunderbolt: Add support for Intel Ice Lake
To:     Mika Westerberg <mika.westerberg@linux.intel.com>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Andreas Noever <andreas.noever@gmail.com>,
        Michael Jamet <michael.jamet@intel.com>,
        "Rafael J . Wysocki" <rjw@rjwysocki.net>,
        Len Brown <lenb@kernel.org>, Lukas Wunner <lukas@wunner.de>,
        Mario Limonciello <Mario.Limonciello@dell.com>,
        Anthony Wong <anthony.wong@canonical.com>,
        linux-acpi@vger.kernel.org,
        Raanan Avargil <raanan.avargil@intel.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Jul 5, 2019 at 5:51 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> > > +static int nhi_suspend_power_down(struct tb *tb)
> > > +{
> > > +       int ret;
> > > +
> > > +       /*
> > > +        * If there is no device connected we need to perform an additional
> > > +        * handshake through LC mailbox and force power down before
> > > +        * entering D3.
> > > +        */
> > > +       ret = device_for_each_child(&tb->root_switch->dev, NULL,
> > > +                                   nhi_device_connected);
> > > +       if (!ret) {
> > > +               lc_mailbox_cmd(tb->nhi, LC_PREPARE_FOR_RESET);
> > > +               ret = lc_mailbox_cmd_complete(tb->nhi,
> > > +                                             LC_MAILBOX_TIMEOUT);
> > > +               if (ret)
> > > +                       return ret;
> > > +
> > > +               return nhi_power_down(tb->nhi);
> >
> > Just to be sure: unforce power is done only if no device is connected?
> > My understanding of the comment above was that unforce power should be done
> > anyway (so it should be outside of this if block), and the difference between
> > the cases is only about the additional LC mailbox message. I guess I misread it.
>
> nhi_power_down() should be only called if no device was connected so it
> should be in correct place. I can try to clarify the comment a bit,
> though.

Maybe adding the word "both" ("to perform both an additional") will make it
clearer. Maybe removing the "additional" (which to my ears sounds like "an
additional operation besides the normal one, to unforce power") is enough.
Again, your call. I'm not sure it's strictly needed, maybe it's just me.

Thanks!