Re: [PATCH v10 0/5] USB: misc: Add onboard_usb_hub driver

From: Matthias Kaehlcke <mka@chromium.org>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Alan Stern <stern@rowland.harvard.edu>,
	Rob Herring <robh+dt@kernel.org>,
	Frank Rowand <frowand.list@gmail.com>,
	Michal Simek <michal.simek@xilinx.com>,
	devicetree@vger.kernel.org,
	Douglas Anderson <dianders@chromium.org>,
	linux-usb@vger.kernel.org, Peter Chen <peter.chen@kernel.org>,
	linux-kernel@vger.kernel.org, Stephen Boyd <swboyd@chromium.org>,
	Ravi Chandra Sadineni <ravisadineni@chromium.org>,
	Krzysztof Kozlowski <krzk@kernel.org>,
	Bastien Nocera <hadess@hadess.net>,
	Al Cooper <alcooperx@gmail.com>,
	"Alexander A. Klimov" <grandmaster@al2klimov.de>,
	Andy Gross <agross@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Christian Lamparter <chunkeey@googlemail.com>,
	Colin Ian King <colin.king@canonical.com>,
	Dmitry Osipenko <digetx@gmail.com>,
	Fabio Estevam <festevam@gmail.com>,
	Masahiro Yamada <masahiroy@kernel.org>,
	Mathias Nyman <mathias.nyman@intel.com>,
	Vinod Koul <vkoul@kernel.org>,
	linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH v10 0/5] USB: misc: Add onboard_usb_hub driver
Date: Tue, 25 May 2021 10:42:25 -0700	[thread overview]
Message-ID: <YK03AVRfbRJUGd0X@google.com> (raw)
In-Reply-To: <YKen70owPqdjy5+a@kroah.com>

On Fri, May 21, 2021 at 02:30:39PM +0200, Greg Kroah-Hartman wrote:
> On Tue, May 11, 2021 at 03:52:18PM -0700, Matthias Kaehlcke wrote:
> > This series adds:
> > - the onboard_usb_hub_driver
> > - glue in the xhci-plat driver to create the onboard_usb_hub
> >   platform device if needed
> > - a device tree binding for the Realtek RTS5411 USB hub controller
> > - device tree changes that add RTS5411 entries for the QCA SC7180
> >   based boards trogdor and lazor
> > - a couple of stubs for platform device functions to avoid
> >   unresolved symbols with certain kernel configs
> > 
> > The main issue the 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.
> 
> I get a build error when I apply this series to my tree:
> 
> drivers/usb/misc/onboard_usb_hub.c:273:6: error: redefinition of ‘of_is_onboard_usb_hub’
>   273 | bool of_is_onboard_usb_hub(const struct device_node *np)
>       |      ^~~~~~~~~~~~~~~~~~~~~
> In file included from drivers/usb/misc/onboard_usb_hub.c:21:
> ./include/linux/usb/onboard_hub.h:9:20: note: previous definition of ‘of_is_onboard_usb_hub’ with type ‘bool(const struct device_node *)’ {aka ‘_Bool(const struct device_node *)’}
>     9 | static inline bool of_is_onboard_usb_hub(const struct device_node *np)
>       |                    ^~~~~~~~~~~~~~~~~~~~~
> 
> Any thoughts?

That function keeps haunting me in different ways ...

I suspect this was a build with CONFIG_COMPILE_TEST=y. The driver is
compiled for such a config and has the actual implementation, however
the header defines the static inline unless CONFIG_USB_ONBOARD_HUB=y/m.

I realized earlier that the driver doesn't really need to include the
header, and planned to remove it in the next version, which might be
the most practical solution.