Hi, On Fri, Aug 5, 2022 at 12:56 PM Matthias Kaehlcke <mka@xxxxxxxxxxxx> wrote: > > Hi Doug, > > On Fri, Aug 05, 2022 at 12:26:35PM -0700, Doug Anderson wrote: > > Hi, > > > > On Fri, Aug 5, 2022 at 11:19 AM Matthias Kaehlcke <mka@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> > > > --- > > > > > > 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? > > It's not in the binding of the RTS5411 which I guess you looked at, > because that hub doesn't have a reset line. > > The reset functionality was initially added for the TI USB8041, the > binding has the reset, but I found it hasn't landed yet: > > https://patchwork.kernel.org/project/linux-usb/patch/20220727093801.687361-1-alexander.stein@xxxxxxxxxxxxxxx/ Ah, I see. I actually clicked on the "Link" for the patches that landed and found that they landed without a bindings patch. I guess it dropped on v4, which I see you yelled about in v3. ;-) > > 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... > > As of now none of the supported hubs (there are only two of them) has > an asymmetric delay. The RTS5411 doesn't have a reset line, and the > TI USB8041 only specifies a power on delay (in my interpretation). > > [1] has some discussion between Alexander and me about this second > reset. The patch that added the delay was merged before this > discussion concluded. > > If the driver is going to support a hub that needs an additional > reset delay when the hub is powered off I'm totally in favor of > adding that delay, however that isn't currently the case in my > understanding. If you draw a different conclusion from the TI > USB8041 datasheet please let me know. > > [1] https://patchwork.kernel.org/project/linux-usb/patch/20220727141117.909361-1-alexander.stein@xxxxxxxxxxxxxxx/ Ah, OK. With this context: Reviewed-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
