Re: [PATCH v4 2/2] USB: misc: Add onboard_usb_hub driver

From: Alan Stern <stern@rowland.harvard.edu>
To: Matthias Kaehlcke <mka@chromium.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Rob Herring <robh+dt@kernel.org>,
	Frank Rowand <frowand.list@gmail.com>,
	linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org,
	Bastien Nocera <hadess@hadess.net>,
	Stephen Boyd <swboyd@chromium.org>,
	Douglas Anderson <dianders@chromium.org>,
	Ravi Chandra Sadineni <ravisadineni@chromium.org>,
	Krzysztof Kozlowski <krzk@kernel.org>,
	devicetree@vger.kernel.org, Peter Chen <peter.chen@nxp.com>,
	"Alexander A. Klimov" <grandmaster@al2klimov.de>,
	"David S. Miller" <davem@davemloft.net>,
	Johan Hovold <johan@kernel.org>,
	Masahiro Yamada <yamada.masahiro@socionext.com>,
	Mauro Carvalho Chehab <mchehab+huawei@kernel.org>,
	Rob Herring <robh@kernel.org>
Subject: Re: [PATCH v4 2/2] USB: misc: Add onboard_usb_hub driver
Date: Mon, 28 Sep 2020 14:47:59 -0400	[thread overview]
Message-ID: <20200928184759.GB142254@rowland.harvard.edu> (raw)
In-Reply-To: <20200928101326.v4.2.I7c9a1f1d6ced41dd8310e8a03da666a32364e790@changeid>

On Mon, Sep 28, 2020 at 10:13:55AM -0700, Matthias Kaehlcke wrote:
> The main issue this driver addresses is that a USB hub needs to be
> powered before it can be discovered. For discrete onboard hubs (an
> example for such a hub is the Realtek RTS5411) this is often solved
> by supplying the hub with an 'always-on' regulator, which is kind
> of a hack. Some onboard hubs may require further initialization
> steps, like changing the state of a GPIO or enabling a clock, which
> requires even more hacks. This driver creates a platform device
> representing the hub which performs the necessary initialization.
> Currently it only supports switching on a single regulator, support
> for multiple regulators or other actions can be added as needed.
> Different initialization sequences can be supported based on the
> compatible string.
> 
> Besides performing the initialization the driver can be configured
> to power the hub off during system suspend. This can help to extend
> battery life on battery powered devices which have no requirements
> to keep the hub powered during suspend. The driver can also be
> configured to leave the hub powered when a wakeup capable USB device
> is connected when suspending, and power it off otherwise.
> 
> Technically the driver consists of two drivers, the platform driver
> described above and a very thin USB driver that subclasses the
> generic driver. The purpose of this driver is to provide the platform
> driver with the USB devices corresponding to the hub(s) (a hub
> controller may provide multiple 'logical' hubs, e.g. one to support
> USB 2.0 and another for USB 3.x).
> 
> Co-developed-by: Ravi Chandra Sadineni <ravisadineni@chromium.org>
> Signed-off-by: Ravi Chandra Sadineni <ravisadineni@chromium.org>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---

Minor cut & paste error:

> +static int onboard_hub_power_off(struct onboard_hub *hub)
> +{
> +	int err;
> +
> +	err = regulator_disable(hub->vdd);
> +	if (err) {
> +		dev_err(hub->dev, "failed to enable regulator: %d\n", err);

s/enable/disable/

Have you tried manually unbinding and rebinding the two drivers a few 
times to make sure they will still work?  I'm a little concerned about 
all the devm_* stuff in here; does that get released when the driver is 
unbound from the device or when the device is unregistered?  And if the 
latter, what happens if you have multiple sysfs attribute groups going 
at the same time?

Apart from those worries and the typo, this looks pretty good to me.

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Alan Stern