Re: [PATCH v5 6/8] usb: misc: onboard_dev: add support for non-hub devices

From: Matthias Kaehlcke <mka@chromium.org>
To: Javier Carrasco <javier.carrasco@wolfvision.net>
Cc: Liam Girdwood <lgirdwood@gmail.com>,
	Mark Brown <broonie@kernel.org>, Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Helen Koike <helen.koike@collabora.com>,
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	Maxime Ripard <mripard@kernel.org>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Russell King <linux@armlinux.org.uk>,
	linux-sound@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org,
	dri-devel@lists.freedesktop.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v5 6/8] usb: misc: onboard_dev: add support for non-hub devices
Date: Wed, 28 Feb 2024 20:41:42 +0000	[thread overview]
Message-ID: <Zd-ahtPpI8zbAYQ9@google.com> (raw)
In-Reply-To: <ecf303c3-d7a1-49d5-a997-32596215e43d@wolfvision.net>

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@wolfvision.net>
> >> ---
> >>  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.

> >   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.