Re: [PATCH v2 2/3] usb: misc: onboard_usb_hub: Add reset-gpio support

[Matthias Kaehlcke <mka@chromium.org>]

On Wed, Jul 20, 2022 at 09:15:12AM +0200, Alexander Stein wrote:
> Hi Matthias,
> 
> Am Freitag, 15. Juli 2022, 21:44:12 CEST schrieb Matthias Kaehlcke:
> > On Fri, Jul 15, 2022 at 09:32:59AM +0200, Alexander Stein wrote:
> > > Despite default reset upon probe, release reset line after powering up
> > > the hub and assert reset again before powering down.
> > > 
> > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > > ---
> > > Changes in v2:
> > > * Use device specific sleep times, if present
> > > * Use fsleep instead of usleep_range
> > > 
> > >  drivers/usb/misc/onboard_usb_hub.c | 29 +++++++++++++++++++++++++++++
> > >  drivers/usb/misc/onboard_usb_hub.h |  5 +++++
> > >  2 files changed, 34 insertions(+)
> > > 
> > > diff --git a/drivers/usb/misc/onboard_usb_hub.c
> > > b/drivers/usb/misc/onboard_usb_hub.c index 6b9b949d17d3..1dd7f4767def
> > > 100644
> > > --- a/drivers/usb/misc/onboard_usb_hub.c
> > > +++ b/drivers/usb/misc/onboard_usb_hub.c
> > > @@ -7,6 +7,7 @@
> > > 
> > >  #include <linux/device.h>
> > >  #include <linux/export.h>
> > > 
> > > +#include <linux/gpio/consumer.h>
> > > 
> > >  #include <linux/init.h>
> > >  #include <linux/kernel.h>
> > >  #include <linux/list.h>
> > > 
> > > @@ -38,6 +39,8 @@ struct usbdev_node {
> > > 
> > >  struct onboard_hub {
> > >  
> > >  	struct regulator *vdd;
> > >  	struct device *dev;
> > > 
> > > +	const struct onboard_hub_devtype_data *devtype_data;
> > 
> > This kind of device specific data is often call platform data, let's
> > call the struct 'onboard_hub_pdata' and the field 'pdata'?
> 
> I took flexcan as a reference, but I do not mind using other names if that is 
> preferred.
> 
> > > +	struct gpio_desc *reset_gpio;
> > > 
> > >  	bool always_powered_in_suspend;
> > >  	bool is_powered_on;
> > >  	bool going_away;
> > > 
> > > @@ -56,6 +59,11 @@ static int onboard_hub_power_on(struct onboard_hub
> > > *hub)
> > > 
> > >  		return err;
> > >  	
> > >  	}
> > > 
> > > +	if (hub->devtype_data)
> > 
> > Instead of these checks let's make sure all hubs have pdata. Actually your
> > onboard_hub_probe() already esnures that the field is assigned.
> 
> For Realtek hubs (no platform data yet), of_id->data therefore hub-
> >devtype_data is NULL.
> But I agree that providing platformdata for all hubs seems reasonable to get 
> rid of these checks.
> 
> > > +		fsleep(hub->devtype_data->power_stable_time);
> > > +	if (hub->reset_gpio)
> > > +		gpiod_set_value_cansleep(hub->reset_gpio, 0);
> > 
> > I would have expected:
> > 
> >   	if (hub->reset_gpio) {
> > 		fsleep(hub->devtype_data->power_stable_time);
> > 		gpiod_set_value_cansleep(hub->reset_gpio, 0);
> > 	}
> > 
> > For the TUSB8041 the 'power_stable_time' (td2 in the datasheet) is "VDD and
> > VDD33 stable before de-assertion of GRSTz", so no delay without reset.
> 
> Your suggestion only works if you control the reset line (GRSTz) by GPIO. If 
> this line is controlled by hardware using RC circuits this waiting time still 
> has to be respected when power is supplied.
> I prefer that the USB hub is properly reset once this function exits. Without 
> waiting time the hardware controlled GRSTz might not yet be at a proper level.

Ok, in that case let's omit the check for 'hub->reset_gpio' and just do:

	fsleep(hub->devtype_data->power_stable_time);
	gpiod_set_value_cansleep(hub->reset_gpio, 0);

The point of adding the check was to skip the delay when no reset GPIO is
specified, but apparently that's not what we actually want.