| Message ID | 20240220-onboard_xvf3500-v4-2-dc1617cc5dd4@wolfvision.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: misc: onboard_hub: add support for XMOS XVF3500 | expand |
On Tue, Feb 20, 2024 at 03:05:46PM +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. Please mention that the driver doesn't power off non-hub devices during system suspend. > 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, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c > index 2103af2cb2a6..f43130a6786f 100644 > --- a/drivers/usb/misc/onboard_usb_dev.c > +++ b/drivers/usb/misc/onboard_usb_dev.c > @@ -129,7 +129,8 @@ static int __maybe_unused onboard_dev_suspend(struct device *dev) > if (!device_may_wakeup(node->udev->bus->controller)) > continue; > > - if (usb_wakeup_enabled_descendants(node->udev)) { > + if (usb_wakeup_enabled_descendants(node->udev) || > + !onboard_dev->pdata->is_hub) { This check isn't dependent on characteristics of the USB devices processed in this loop, therefore it can be performed at function entry. Please combine it with the check of 'always_powered_in_suspend'. It's also an option to omit the check completely, 'always_powered_in_suspend' will never be set for non-hub devices (assuming the sysfs attribute isn't added). > power_off = false; > break; > } Without code context: please omit the creation of the 'always_powered_in_suspend' attribute for non-hub devices. As per above we don't plan to hone it, so it shouldn't exist.
On 21.02.24 20:24, Matthias Kaehlcke wrote: > On Tue, Feb 20, 2024 at 03:05:46PM +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. > > Please mention that the driver doesn't power off non-hub devices > during system suspend. > >> 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, 12 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c >> index 2103af2cb2a6..f43130a6786f 100644 >> --- a/drivers/usb/misc/onboard_usb_dev.c >> +++ b/drivers/usb/misc/onboard_usb_dev.c >> @@ -129,7 +129,8 @@ static int __maybe_unused onboard_dev_suspend(struct device *dev) >> if (!device_may_wakeup(node->udev->bus->controller)) >> continue; >> >> - if (usb_wakeup_enabled_descendants(node->udev)) { >> + if (usb_wakeup_enabled_descendants(node->udev) || >> + !onboard_dev->pdata->is_hub) { > > > This check isn't dependent on characteristics of the USB devices processed > in this loop, therefore it can be performed at function entry. Please combine > it with the check of 'always_powered_in_suspend'. It's also an option to > omit the check completely, 'always_powered_in_suspend' will never be set for > non-hub devices (assuming the sysfs attribute isn't added). > The attribute will not be available for non-hub devices in v5. However, if the check is completely removed, will power_off not stay true at the end of the function, always leading to a device power off? As you said, 'always_powered_in_suspend' will not be set for non-hub devices. >> power_off = false; >> break; >> } > > Without code context: please omit the creation of the 'always_powered_in_suspend' > attribute for non-hub devices. As per above we don't plan to hone it, so it > shouldn't exist. Best regards, Javier Carrasco
On Thu, Feb 22, 2024 at 03:42:26PM +0100, Javier Carrasco wrote: > On 21.02.24 20:24, Matthias Kaehlcke wrote: > > On Tue, Feb 20, 2024 at 03:05:46PM +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. > > > > Please mention that the driver doesn't power off non-hub devices > > during system suspend. > > > >> 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, 12 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c > >> index 2103af2cb2a6..f43130a6786f 100644 > >> --- a/drivers/usb/misc/onboard_usb_dev.c > >> +++ b/drivers/usb/misc/onboard_usb_dev.c > >> @@ -129,7 +129,8 @@ static int __maybe_unused onboard_dev_suspend(struct device *dev) > >> if (!device_may_wakeup(node->udev->bus->controller)) > >> continue; > >> > >> - if (usb_wakeup_enabled_descendants(node->udev)) { > >> + if (usb_wakeup_enabled_descendants(node->udev) || > >> + !onboard_dev->pdata->is_hub) { > > > > > > This check isn't dependent on characteristics of the USB devices processed > > in this loop, therefore it can be performed at function entry. Please combine > > it with the check of 'always_powered_in_suspend'. It's also an option to > > omit the check completely, 'always_powered_in_suspend' will never be set for > > non-hub devices (assuming the sysfs attribute isn't added). > > > > The attribute will not be available for non-hub devices in v5. However, > if the check is completely removed, will power_off not stay true at the > end of the function, always leading to a device power off? As you said, > 'always_powered_in_suspend' will not be set for non-hub devices. Even without the sysfs attribute the field 'always_powered_in_suspend' could be set to true by probe() for non-hub devices.
diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c index 2103af2cb2a6..f43130a6786f 100644 --- a/drivers/usb/misc/onboard_usb_dev.c +++ b/drivers/usb/misc/onboard_usb_dev.c @@ -129,7 +129,8 @@ static int __maybe_unused onboard_dev_suspend(struct device *dev) if (!device_may_wakeup(node->udev->bus->controller)) continue; - if (usb_wakeup_enabled_descendants(node->udev)) { + if (usb_wakeup_enabled_descendants(node->udev) || + !onboard_dev->pdata->is_hub) { power_off = false; break; } 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, 12 insertions(+), 1 deletion(-)