Re: [PATCH v4 1/2] usb: misc: onboard_usb_hub: Add reset-gpio support

[Alexander Stein <alexander.stein@ew.tq-group.com>]

Hello Matthias,

Am Mittwoch, 27. Juli 2022, 19:14:57 CEST schrieb Matthias Kaehlcke:
> Hi Alexander,
> 
> (copying my comments from v3 to keep the discussion on the latest version)
> 
> On Wed, Jul 27, 2022 at 04:11:16PM +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>
> > ---
> > * Patch 1 dropped as it already applied
> > 
> > Changes in v4:
> > * Rebased to [1] commit e0c6b1f3d757 ("USB: usbsevseg: convert sysfs
> > snprintf to sysfs_emit") * Added platform data for usb424
> > 
> >   Reset pulse length taken from [2], Table 3-2 Symbol RESET_N
> >   Completely untested
> > 
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git Branch
> > usb-testing [2]
> > http://ww1.microchip.com/downloads/en/devicedoc/00001692c.pdf
> > 
> >  drivers/usb/misc/onboard_usb_hub.c | 28 ++++++++++++++++++++++++++++
> >  drivers/usb/misc/onboard_usb_hub.h | 22 +++++++++++++++++-----
> >  2 files changed, 45 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/usb/misc/onboard_usb_hub.c
> > b/drivers/usb/misc/onboard_usb_hub.c index de3627af3c84..0c81417dd9a7
> > 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_pdata *pdata;
> > +	struct gpio_desc *reset_gpio;
> > 
> >  	bool always_powered_in_suspend;
> >  	bool is_powered_on;
> >  	bool going_away;
> > 
> > @@ -56,6 +59,9 @@ static int onboard_hub_power_on(struct onboard_hub *hub)
> > 
> >  		return err;
> >  	
> >  	}
> > 
> > +	fsleep(hub->pdata->reset_us);
> > +	gpiod_set_value_cansleep(hub->reset_gpio, 0);
> > +
> > 
> >  	hub->is_powered_on = true;
> >  	
> >  	return 0;
> > 
> > @@ -65,6 +71,11 @@ static int onboard_hub_power_off(struct onboard_hub
> > *hub)> 
> >  {
> >  
> >  	int err;
> > 
> > +	if (hub->reset_gpio) {
> > +		gpiod_set_value_cansleep(hub->reset_gpio, 1);
> > +		fsleep(hub->pdata->reset_us);
> 
> Is this delay here actually needed? There is a delay in
> onboard_hub_power_on(), before de-asserting the reset, isn't that enough?

If you see both delays together you are right, but I tend to think in that way 
it is to ensure whenever we apply a reset it is long enough.
As said before the powering on delay is to ensure the pulse length delay even 
if there is no reset GPIO but it is controlled by hardware.

> > +	}
> > +
> > 
> >  	err = regulator_disable(hub->vdd);
> >  	if (err) {
> >  	
> >  		dev_err(hub->dev, "failed to disable regulator: %d\n", 
err);
> > 
> > @@ -219,6 +230,7 @@ static void onboard_hub_attach_usb_driver(struct
> > work_struct *work)> 
> >  static int onboard_hub_probe(struct platform_device *pdev)
> >  {
> > 
> > +	const struct of_device_id *of_id;
> > 
> >  	struct device *dev = &pdev->dev;
> >  	struct onboard_hub *hub;
> >  	int err;
> > 
> > @@ -227,10 +239,26 @@ static int onboard_hub_probe(struct platform_device
> > *pdev)> 
> >  	if (!hub)
> >  	
> >  		return -ENOMEM;
> > 
> > +	of_id = of_match_device(onboard_hub_match, &pdev->dev);
> > +	if (!of_id)
> > +		return -ENODEV;
> > +
> > +	hub->pdata = of_id->data;
> > +	if (!hub->pdata)
> > +		return -EINVAL;
> > +
> > 
> >  	hub->vdd = devm_regulator_get(dev, "vdd");
> >  	if (IS_ERR(hub->vdd))
> >  	
> >  		return PTR_ERR(hub->vdd);
> > 
> > +	hub->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> > +						  
GPIOD_OUT_HIGH);
> > +	if (IS_ERR(hub->reset_gpio))
> > +		return dev_err_probe(dev, PTR_ERR(hub->reset_gpio), 
"failed to get
> > reset GPIO\n"); +
> > +	if (hub->reset_gpio)
> > +		fsleep(hub->pdata->reset_us);
> 
> Same question here: onboard_hub_power_on() is called a few lines below and
> has a delay before de-asserting the reset. Is the delay here really needed?

This actually looks like the delay is duplicated here. I agree with removing 
this.
How shall we proceed now that the whole series (incl. the bindings patch 1/3 
from v3) has landed in usb-testing? I can create a patch on top of this if 
this is the way to go.

Best regards,
Alexander