Re: [PATCH v2 1/2] usb: misc: onboard_usb_hub: Don't create platform devices for DT nodes without 'vdd-supply'

[Matthias Kaehlcke]

On Mon, Jan 02, 2023 at 03:38:19PM +0100, Alexander Stein wrote:
> Hi Stefan,
> 
> Am Montag, 2. Januar 2023, 12:44:33 CET schrieb Stefan Wahren:
> > Hello Alexander,
> > 
> > Am 02.01.23 um 10:20 schrieb Alexander Stein:
> > > Hi everybody,
> > > 
> > > Am Freitag, 23. Dezember 2022, 08:46:45 CET schrieb Icenowy Zheng:
> > >> 在 2022-12-22星期四的 11:26 -0800，Doug Anderson写道：
> > >> 
> > >>> Hi,
> > >>> 
> > >>> On Wed, Dec 21, 2022 at 6:26 PM Matthias Kaehlcke <mka@xxxxxxxxxxxx>
> > >>> 
> > >>> wrote:
> > >>>> The primary task of the onboard_usb_hub driver is to control the
> > >>>> power of an onboard USB hub. The driver gets the regulator from the
> > >>>> device tree property "vdd-supply" of the hub's DT node. Some boards
> > >>>> have device tree nodes for USB hubs supported by this driver, but
> > >>>> don't specify a "vdd-supply". This is not an error per se, it just
> > >>>> means that the onboard hub driver can't be used for these hubs, so
> > >>>> don't create platform devices for such nodes.
> > >>>> 
> > >>>> This change doesn't completely fix the reported regression. It
> > >>>> should fix it for the RPi 3 B Plus and boards with similar hub
> > >>>> configurations (compatible DT nodes without "vdd-supply"), boards
> > >>>> that actually use the onboard hub driver could still be impacted
> > >>>> by the race conditions discussed in that thread. Not creating the
> > >>>> platform devices for nodes without "vdd-supply" is the right
> > >>>> thing to do, independently from the race condition, which will
> > >>>> be fixed in future patch.
> > >>>> 
> > >>>> Fixes: 8bc063641ceb ("usb: misc: Add onboard_usb_hub driver")
> > >>>> Link:
> > >>>> https://lore.kernel.org/r/d04bcc45-3471-4417-b30b-5cf9880d785d@xxxxxxxx
> > >>>> /
> > >>>> Reported-by: Stefan Wahren <stefan.wahren@xxxxxxxx>
> > >>>> Signed-off-by: Matthias Kaehlcke <mka@xxxxxxxxxxxx>
> > >>>> ---
> > >>>> 
> > >>>> Changes in v2:
> > >>>> - don't create platform devices when "vdd-supply" is missing,
> > >>>> 
> > >>>>    rather than returning an error from _find_onboard_hub()
> > >>>> 
> > >>>> - check for "vdd-supply" not "vdd" (Johan)
> > >>>> - updated subject and commit message
> > >>>> - added 'Link' tag (regzbot)
> > >>>> 
> > >>>>   drivers/usb/misc/onboard_usb_hub_pdevs.c | 13 +++++++++++++
> > >>>>   1 file changed, 13 insertions(+)
> > >>> 
> > >>> I'm a tad bit skeptical.
> > >>> 
> > >>> It somehow feels a bit too much like "inside knowledge" to add this
> > >>> here. I guess the "onboard_usb_hub_pdevs.c" is already pretty
> > >>> entangled with "onboard_usb_hub.c", but I'd rather the "pdevs" file
> > >>> keep the absolute minimum amount of stuff in it and all of the
> > >>> details
> > >>> be in the other file.
> > >>> 
> > >>> If this was the only issue though, I'd be tempted to let it slide. As
> > >>> it is, I'm kinda worried that your patch will break Alexander Stein,
> > >>> who should have been CCed (I've CCed him now) or Icenowy Zheng (also
> > >>> CCed now). I believe those folks are using the USB hub driver
> > >>> primarily to drive a reset GPIO. Looking at the example in the
> > >>> bindings for one of them (genesys,gl850g.yaml), I even see that the
> > >>> reset-gpio is specified but not a vdd-supply. I think you'll break
> > >>> that?
> > >> 
> > >> Well technically in my final DT a regulator is included (to have the
> > >> Vbus enabled when enabling the hub), however I am still against this
> > >> patch, because the driver should work w/o vdd-supply (or w/o reset-
> > >> gpios), and changing this behavior is a DT binding stability breakage.
> > > 
> > > I second that. The bindings don't require neither vdd-supply nor
> > > reset-gpios.
> > > 
> > > But I have to admit I lack to understand the purpose of this series in the
> > > first place. What is the benefit or fix?
> > 
> > did you followed the provided link from the patch?
> 
> Ah, I didn't notice that. Thanks. Admittedly I've yet to fully understand that 
> race condition, but Matthias already suspects this series might not be enough, 
> even for boards which do use vdd-supply.
> 
> Just for the record, this series breaks my board if, as suspected by Doug 
> Anderson and Icenowy Zheng, there is no vdd-supply. The reset line will never 
> be touched.

Yes, I missed that a reset GPIO could be used in combination with an always-on
regulator (instead of specifiying 'vdd-supply') to 'switch the hub on and off'.
If we proceed with the general approach of this patch then creation of the pdev
should only be skipped when neither 'vdd-supply' nor 'reset-gpios' is specified.