On Fri, Jul 5, 2019 at 5:51 PM Mika Westerberg
<mika.westerberg@xxxxxxxxxxxxxxx> 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!
