On 28.02.24 21:41, Matthias Kaehlcke wrote: > On Wed, Feb 28, 2024 at 09:21:00PM +0100, Javier Carrasco wrote: >> On 28.02.24 19:10, Matthias Kaehlcke wrote: >>> On Wed, Feb 28, 2024 at 02:51:33PM +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. >>>> >>>> The 'always_powered_in_supend' attribute is only available for hub >>>> devices, keeping the driver's default behavior for non-hub devices (keep >>>> on in suspend). >>>> >>>> Signed-off-by: Javier Carrasco <javier.carrasco@xxxxxxxxxxxxxx> >>>> --- >>>> drivers/usb/misc/onboard_usb_dev.c | 25 +++++++++++++++++++++++-- >>>> drivers/usb/misc/onboard_usb_dev.h | 10 ++++++++++ >>>> 2 files changed, 33 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c >>>> index e1779bd2d126..df0ed172c7ec 100644 >>>> --- a/drivers/usb/misc/onboard_usb_dev.c >>>> +++ b/drivers/usb/misc/onboard_usb_dev.c >>>> @@ -132,7 +132,8 @@ static int __maybe_unused onboard_dev_suspend(struct device *dev) >>>> struct usbdev_node *node; >>>> bool power_off = true; >>>> >>>> - if (onboard_dev->always_powered_in_suspend) >>>> + if (onboard_dev->always_powered_in_suspend && >>>> + !onboard_dev->pdata->is_hub) >>>> return 0; >>> >>> With this non-hub devices would always be powered down, since >>> 'always_powerd_in_suspend' is not set for them. This should be: >>> >> >> May I ask you what you meant in v4 with this comment? >> >>> Even without the sysfs attribute the field 'always_powered_in_suspend' >>> could >>> be set to true by probe() for non-hub devices. > > struct onboard_dev always has the field 'always_powered_in_suspend', > even for non-hubs, that don't have the corresponding sysfs attribute. > Currently it is left uninitialized (i.e. false) for non-hubs. Instead > it could be initialized to true by probe() for non-hubs, which would > be semantically correct. With that it wouldn't be necessary to check > here whether a device is hub, because the field would provide the > necessary information. > That is maybe what is confusing me a bit. Should it not be false for non-hub devices? That property is only meant for hubs, so why should non-hub devices be always powered in suspend? I thought it should always be false for non-hub devices, and configurable for hubs. >>> if (!onboard_dev->pdata->is_hub || >>> onboard_dev->always_powered_in_suspend) >>> >>> Checking for the (non-)hub status first is clearer IMO, also it avoids >>> an unneccessary check of 'always_powered' for non-hub devices. >>> >> >> That makes sense and will be fixed. >> >>> Without code context: for hubs there can be multiple device tree nodes >>> for the same physical hub chip (e.g. one for the USB2 and another for >>> the USB3 part). I suppose this could also be the case for non-hub >>> devices. For hubs there is the 'peer-hub' device tree property to >>> establish a link between the two USB devices, as a result the onboard >>> driver only creates a single platform device (which is desired, >>> otherwise two platform devices would be in charge for power sequencing >>> the same phyiscal device. For non-hub devices there is currently no such >>> link. In many cases I expect there will be just one DT entry even though >>> the device has multiple USB interfaces, but it could happen and would >>> actually be a more accurate representation. >>> >>> General support is already there (the code dealing with 'peer-hub'), but >>> we'd have to come up with a suitable name. 'peer-device' is the first >>> thing that comes to my mind, but there might be better options. If such >>> a generic property is added then we should deprecate 'peer-hub', but >>> maintain backwards compatibility. >> >> I have nothing against that, but the first non-hub device that will be >> added does not have multiple DT nodes, so I have nothing to test that >> extension with real hardware. > > I see, the XVF3500 is USB 2.0 only, so it isn't suitable for testing. > >> That could be added in the future, though, if the need ever arises. > > I expect it will, when a DT maintainer asks the hardware to be > represented correctly for a device that is connected to more than one USB > bus. IIRC that's how 'peer-hub' was born :) > > Ok, we can leave it out for now. I might send a dedicated patch after your > series landed. If a switch to 'peer-device' or similar is anticipated then > it's probably best to deprecate 'peer-hub' ASAP, to avoid it from getting > added to more bindings. Best regards, Javier Carrasco
