Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220805111836.1.Id5a4dc0a2c046236116693aa55672295513a0f2a@changeid>
In-Reply-To: <20220805111836.1.Id5a4dc0a2c046236116693aa55672295513a0f2a@changeid>
From:   Doug Anderson <dianders@chromium.org>
Date:   Fri, 5 Aug 2022 12:26:35 -0700
Message-ID: <CAD=FV=W9VWbvWqdEEY9=OnNSsAnQ+CgQPRifbAu2ixrgPQd54A@mail.gmail.com>
Subject: Re: [PATCH] usb: misc: onboard_usb_hub: Drop reset delay in onboard_hub_power_off()
To:     Matthias Kaehlcke <mka@chromium.org>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Alexander Stein <alexander.stein@ew.tq-group.com>,
        Linux USB List <linux-usb@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi,

On Fri, Aug 5, 2022 at 11:19 AM Matthias Kaehlcke <mka@chromium.org> wrote:
>
> onboard_hub_power_off() currently has a delay after asserting the
> reset of the hub. There is already a delay in onboard_hub_power_on()
> before de-asserting the reset, which ensures that the reset is
> asserted for the required time, so the delay in _power_off() is not
> needed.
>
> Skip the reset GPIO check before calling gpiod_set_value_cansleep(),
> the function returns early when the GPIO descriptor is NULL.
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>
>  drivers/usb/misc/onboard_usb_hub.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)

I was trying to figure out what this "reset" was defined to do and I
looked for the device tree bindings. They don't seem to exist. Was
that an oversight?

In any case, I'm not convinced that your patch is correct. Timing
diagrams often show a needed delay between adjusting a reset GPIO and
turning on/off the power. The timing diagrams can sometimes show a
required delay on both sides. I guess at the moment the only user of
this reset GPIO has a symmetric delay, but I can totally expect that
someone could come along and say that they needed 10 ms on one side
and 1 ms on the other side...