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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jul 27, 2022 at 09:32:03AM -0700, Matthias Kaehlcke wrote:
> Hi Alexander,
> 
> On Wed, Jul 27, 2022 at 11:38:00AM +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@xxxxxxxxxxxxxxx>
> > ---
> > Thanks Matthias for your review.
> > 
> > Changes in v3:
> > * Require platform data unconditionally
> > * Removed unecessary checks for that reason
> > * Merged power_stable_time into reset_duration (no difference for now)
> > * Rename 'reset_duration' to 'reset_us'
> > * Renamed platform structure to onboard_hub_pdata
> > * Renamed device struct field to pdata as well
> > 
> > Changes in v2:
> > * Use device specific sleep times, if present
> > * Use fsleep instead of usleep_range
> > 
> >  drivers/usb/misc/onboard_usb_hub.c | 28 ++++++++++++++++++++++++++++
> >  drivers/usb/misc/onboard_usb_hub.h | 16 ++++++++++++----
> >  2 files changed, 40 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c
> > index 6b9b949d17d3..2aa1db528b31 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?
> 
> > +	}
> > +
> >  	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?

I didn't notice that you had already posted v4 when I replied, let's keep the
discussion on that version for simplicity.



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux