| Message ID | 20240206-onboard_xvf3500-v3-2-f85b04116688@wolfvision.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: misc: onboard_hub: add support for XMOS XVF3500 | expand |
On Tue, Feb 06, 2024 at 02:59:30PM +0100, Javier Carrasco wrote: > Most of the functionality this driver provides can be used by non-hub > devices as well. > > To account for the hub-specific code, add a flag to the device data > structure and check its value for hub-specific code. > > Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net> > --- > drivers/usb/misc/onboard_usb_dev.c | 3 +++ > drivers/usb/misc/onboard_usb_dev.h | 10 ++++++++++ > 2 files changed, 13 insertions(+) > > diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c > index e2e1e1e30c1e..3ac21ec38ac0 100644 > --- a/drivers/usb/misc/onboard_usb_dev.c > +++ b/drivers/usb/misc/onboard_usb_dev.c > @@ -123,6 +123,9 @@ static int __maybe_unused onboard_dev_suspend(struct device *dev) > if (onboard_dev->always_powered_in_suspend) > return 0; > > + if (!onboard_dev->pdata->is_hub) > + return onboard_dev_power_off(onboard_dev); Why turn the device always off when it isn't a hub? It could be a device with wakeup support. I really regret making 'off in suspend' the default :( > + > mutex_lock(&onboard_dev->lock); > > list_for_each_entry(node, &onboard_dev->udev_list, list) { > diff --git a/drivers/usb/misc/onboard_usb_dev.h b/drivers/usb/misc/onboard_usb_dev.h > index f13d11a84371..ebe83e19d818 100644 > --- a/drivers/usb/misc/onboard_usb_dev.h > +++ b/drivers/usb/misc/onboard_usb_dev.h > @@ -9,51 +9,61 @@ > struct onboard_dev_pdata { > unsigned long reset_us; /* reset pulse width in us */ > unsigned int num_supplies; /* number of supplies */ > + bool is_hub; > }; > > static const struct onboard_dev_pdata microchip_usb424_data = { > .reset_us = 1, > .num_supplies = 1, > + .is_hub = true, Depending on when 'is_hub' is checked it could be an option to initialize it at runtime (depending on USB_CLASS_HUB), e.g. in onboard_dev_add_usbdev(). Might not be worth the hassle tough if more than the current check(s) get added. > }; > > static const struct onboard_dev_pdata microchip_usb5744_data = { > .reset_us = 0, > .num_supplies = 2, > + .is_hub = true, > }; > > static const struct onboard_dev_pdata realtek_rts5411_data = { > .reset_us = 0, > .num_supplies = 1, > + .is_hub = true, > }; > > static const struct onboard_dev_pdata ti_tusb8041_data = { > .reset_us = 3000, > .num_supplies = 1, > + .is_hub = true, > }; > > static const struct onboard_dev_pdata cypress_hx3_data = { > .reset_us = 10000, > .num_supplies = 2, > + .is_hub = true, > }; > > static const struct onboard_dev_pdata cypress_hx2vl_data = { > .reset_us = 1, > .num_supplies = 1, > + .is_hub = true, > }; > > static const struct onboard_dev_pdata genesys_gl850g_data = { > .reset_us = 3, > .num_supplies = 1, > + .is_hub = true, > }; > > static const struct onboard_dev_pdata genesys_gl852g_data = { > .reset_us = 50, > .num_supplies = 1, > + .is_hub = true, > }; > > static const struct onboard_dev_pdata vialab_vl817_data = { > .reset_us = 10, > .num_supplies = 1, > + .is_hub = true, > }; > > static const struct of_device_id onboard_dev_match[] = { > > -- > 2.40.1 >
On 06.02.24 19:40, Matthias Kaehlcke wrote: > On Tue, Feb 06, 2024 at 02:59:30PM +0100, Javier Carrasco wrote: >> Most of the functionality this driver provides can be used by non-hub >> devices as well. >> >> To account for the hub-specific code, add a flag to the device data >> structure and check its value for hub-specific code. >> >> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net> >> --- >> drivers/usb/misc/onboard_usb_dev.c | 3 +++ >> drivers/usb/misc/onboard_usb_dev.h | 10 ++++++++++ >> 2 files changed, 13 insertions(+) >> >> diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c >> index e2e1e1e30c1e..3ac21ec38ac0 100644 >> --- a/drivers/usb/misc/onboard_usb_dev.c >> +++ b/drivers/usb/misc/onboard_usb_dev.c >> @@ -123,6 +123,9 @@ static int __maybe_unused onboard_dev_suspend(struct device *dev) >> if (onboard_dev->always_powered_in_suspend) >> return 0; >> >> + if (!onboard_dev->pdata->is_hub) >> + return onboard_dev_power_off(onboard_dev); > > Why turn the device always off when it isn't a hub? It could be a device > with wakeup support. > > I really regret making 'off in suspend' the default :( > The power management seems to be a critical point to consider. Maybe we keep the current implementation and add support to non-hub devices by simply adding a check to set power_off to false: static int __maybe_unused onboard_dev_suspend(struct device *dev) { struct onboard_dev *onboard_dev = dev_get_drvdata(dev); struct usbdev_node *node; bool power_off = true; if (onboard_dev->always_powered_in_suspend) return 0; mutex_lock(&onboard_dev->lock); list_for_each_entry(node, &onboard_dev->udev_list, list) { if (!device_may_wakeup(node->udev->bus->controller)) continue; ~ if (usb_wakeup_enabled_descendants(node->udev) || + !onboard_dev->pdata->is_hub) { power_off = false; break; } } mutex_unlock(&onboard_dev->lock); if (!power_off) return 0; return onboard_dev_power_off(onboard_dev); } Best regards, Javier Carrasco
On Tue, Feb 13, 2024 at 11:00:43AM +0100, Javier Carrasco wrote: > On 06.02.24 19:40, Matthias Kaehlcke wrote: > > On Tue, Feb 06, 2024 at 02:59:30PM +0100, Javier Carrasco wrote: > >> Most of the functionality this driver provides can be used by non-hub > >> devices as well. > >> > >> To account for the hub-specific code, add a flag to the device data > >> structure and check its value for hub-specific code. > >> > >> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net> > >> --- > >> drivers/usb/misc/onboard_usb_dev.c | 3 +++ > >> drivers/usb/misc/onboard_usb_dev.h | 10 ++++++++++ > >> 2 files changed, 13 insertions(+) > >> > >> diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c > >> index e2e1e1e30c1e..3ac21ec38ac0 100644 > >> --- a/drivers/usb/misc/onboard_usb_dev.c > >> +++ b/drivers/usb/misc/onboard_usb_dev.c > >> @@ -123,6 +123,9 @@ static int __maybe_unused onboard_dev_suspend(struct device *dev) > >> if (onboard_dev->always_powered_in_suspend) > >> return 0; > >> > >> + if (!onboard_dev->pdata->is_hub) > >> + return onboard_dev_power_off(onboard_dev); > > > > Why turn the device always off when it isn't a hub? It could be a device > > with wakeup support. > > > > I really regret making 'off in suspend' the default :( > > > > > The power management seems to be a critical point to consider. > > Maybe we keep the current implementation and add support to non-hub > devices by simply adding a check to set power_off to false: > > static int __maybe_unused onboard_dev_suspend(struct device *dev) > > { > > struct onboard_dev *onboard_dev = dev_get_drvdata(dev); > > struct usbdev_node *node; > > bool power_off = true; > > > > if (onboard_dev->always_powered_in_suspend) > > return 0; > > > > mutex_lock(&onboard_dev->lock); > > > > list_for_each_entry(node, &onboard_dev->udev_list, list) { > > if (!device_may_wakeup(node->udev->bus->controller)) > > continue; > > > > ~ if (usb_wakeup_enabled_descendants(node->udev) || > > + !onboard_dev->pdata->is_hub) { > > power_off = false; > > break; > > } It isn't really necessary to perform this check in the loop, it isn't dependent on any properties of the USB devices, right? It could be combined with the check of 'always_powered_in_suspend' above.
diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c index e2e1e1e30c1e..3ac21ec38ac0 100644 --- a/drivers/usb/misc/onboard_usb_dev.c +++ b/drivers/usb/misc/onboard_usb_dev.c @@ -123,6 +123,9 @@ static int __maybe_unused onboard_dev_suspend(struct device *dev) if (onboard_dev->always_powered_in_suspend) return 0; + if (!onboard_dev->pdata->is_hub) + return onboard_dev_power_off(onboard_dev); + mutex_lock(&onboard_dev->lock); list_for_each_entry(node, &onboard_dev->udev_list, list) { diff --git a/drivers/usb/misc/onboard_usb_dev.h b/drivers/usb/misc/onboard_usb_dev.h index f13d11a84371..ebe83e19d818 100644 --- a/drivers/usb/misc/onboard_usb_dev.h +++ b/drivers/usb/misc/onboard_usb_dev.h @@ -9,51 +9,61 @@ struct onboard_dev_pdata { unsigned long reset_us; /* reset pulse width in us */ unsigned int num_supplies; /* number of supplies */ + bool is_hub; }; static const struct onboard_dev_pdata microchip_usb424_data = { .reset_us = 1, .num_supplies = 1, + .is_hub = true, }; static const struct onboard_dev_pdata microchip_usb5744_data = { .reset_us = 0, .num_supplies = 2, + .is_hub = true, }; static const struct onboard_dev_pdata realtek_rts5411_data = { .reset_us = 0, .num_supplies = 1, + .is_hub = true, }; static const struct onboard_dev_pdata ti_tusb8041_data = { .reset_us = 3000, .num_supplies = 1, + .is_hub = true, }; static const struct onboard_dev_pdata cypress_hx3_data = { .reset_us = 10000, .num_supplies = 2, + .is_hub = true, }; static const struct onboard_dev_pdata cypress_hx2vl_data = { .reset_us = 1, .num_supplies = 1, + .is_hub = true, }; static const struct onboard_dev_pdata genesys_gl850g_data = { .reset_us = 3, .num_supplies = 1, + .is_hub = true, }; static const struct onboard_dev_pdata genesys_gl852g_data = { .reset_us = 50, .num_supplies = 1, + .is_hub = true, }; static const struct onboard_dev_pdata vialab_vl817_data = { .reset_us = 10, .num_supplies = 1, + .is_hub = true, }; static const struct of_device_id onboard_dev_match[] = {
Most of the functionality this driver provides can be used by non-hub devices as well. To account for the hub-specific code, add a flag to the device data structure and check its value for hub-specific code. Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net> --- drivers/usb/misc/onboard_usb_dev.c | 3 +++ drivers/usb/misc/onboard_usb_dev.h | 10 ++++++++++ 2 files changed, 13 insertions(+)