Received-SPF: pass (google.com: domain of linux-kernel+bounces-55427-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Tue, 6 Feb 2024 17:55:42 +0000
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>,
	linux-sound@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org,
	Matthias Kaehlcke <matthias@kaehlcke.net>
Subject: Re: [PATCH v3 1/7] usb: misc: onboard_hub: rename to onboard_dev
Message-ID: <ZcJynrwp7zcs-aIT@google.com>
References: <20240206-onboard_xvf3500-v3-0-f85b04116688@wolfvision.net>
 <20240206-onboard_xvf3500-v3-1-f85b04116688@wolfvision.net>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20240206-onboard_xvf3500-v3-1-f85b04116688@wolfvision.net>

Hi Javier,

a few comments inline

On Tue, Feb 06, 2024 at 02:59:29PM +0100, Javier Carrasco wrote:
> This patch prepares onboad_hub to support non-hub devices by renaming
> the driver files and their content, the headers and their references.
> 
> The comments and descriptions have been slightly modified to keep
> coherence and account for the specific cases that only affect onboard
> hubs (e.g. peer-hub).
> 
> The "hub" variables in functions where "dev" (and similar names) variables
> already exist have been renamed to onboard_dev for clarity, which adds a
> few lines in cases where more than 80 characters are used.
> 
> No new functionality has been added.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net>

> diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c
> new file mode 100644
> index 000000000000..e2e1e1e30c1e
> --- /dev/null
> +++ b/drivers/usb/misc/onboard_usb_dev.c
>
> ...
>
> +/*
> + * Use generic names, as the actual names might differ between cevices. If a new

s/cevices/devices/

<snip>

> +static int __maybe_unused onboard_dev_suspend(struct device *dev)
> +{
> +	struct onboard_dev *onboard_dev = dev_get_drvdata(dev);
> +	struct usbdev_node *node;
> +	bool power_off = true;
> +
> +	if (onboard_dev->always_powered_in_suspend)
> +		return 0;
> +
> +	mutex_lock(&onboard_dev->lock);
> +
> +	list_for_each_entry(node, &onboard_dev->udev_list, list) {
> +		if (!device_may_wakeup(node->udev->bus->controller))
> +			continue;
> +
> +		if (usb_wakeup_enabled_descendants(node->udev)) {
> +			power_off = false;
> +			break;
> +		}

The above branch should probably be limited to hub devices (though in practice it
shouldn't make a difference). This should be done by "usb: misc: onboard_dev:
add support for non-hub devices", commenting here since this patch includes the
code.

> +static struct onboard_dev *_find_onboard_dev(struct device *dev)
> +{
> +	struct platform_device *pdev;
> +	struct device_node *np;
> +	struct onboard_dev *onboard_dev;
> +
> +	pdev = of_find_device_by_node(dev->of_node);
> +	if (!pdev) {
> +		np = of_parse_phandle(dev->of_node, "peer-hub", 0);
> +		if (!np) {
> +			dev_err(dev, "failed to find device node for peer hub\n");
> +			return ERR_PTR(-EINVAL);
> +		}
> +
> +		pdev = of_find_device_by_node(np);
> +		of_node_put(np);
> +
> +		if (!pdev)
> +			return ERR_PTR(-ENODEV);
> +	}

The above branch should probably be guarded by 'if (!onboard_dev->pdata->is_hub)',
this is also a change for ""usb: misc: onboard_dev: add support for non-hub devices"

> diff --git a/drivers/usb/misc/onboard_usb_hub_pdevs.c b/drivers/usb/misc/onboard_usb_dev_pdevs.c
> similarity index 68%
> rename from drivers/usb/misc/onboard_usb_hub_pdevs.c
> rename to drivers/usb/misc/onboard_usb_dev_pdevs.c
> index ed22a18f4ab7..fce860b65958 100644
> --- a/drivers/usb/misc/onboard_usb_hub_pdevs.c
> +++ b/drivers/usb/misc/onboard_usb_dev_pdevs.c
>
> ...
>
>  /**
> - * onboard_hub_create_pdevs -- create platform devices for onboard USB hubs
> - * @parent_hub	: parent hub to scan for connected onboard hubs
> - * @pdev_list	: list of onboard hub platform devices owned by the parent hub
> + * onboard_dev_create_pdevs -- create platform devices for onboard USB devices
> + * @parent_hub	: parent hub to scan for connected onboard devices
> + * @pdev_list	: list of onboard platform devices owned by the parent hub
>   *
> - * Creates a platform device for each supported onboard hub that is connected to
> - * the given parent hub. The platform device is in charge of initializing the
> - * hub (enable regulators, take the hub out of reset, ...) and can optionally
> - * control whether the hub remains powered during system suspend or not.
> + * Creates a platform device for each supported onboard device that is connected
> + * to * the given parent hub. The platform device is in charge of initializing
> + * the * device (enable regulators, take the device out of reset, ...) and can
> + * optionally * control whether the device remains powered during system suspend

Remove '*'s in the above 3 lines.

I'm doubting whether the option to power down the device during system suspend
should be limited to hubs. In particular I'm concerned about the default of
powering the device down, which could be unexpected. Then again, it might be
desired to power down a device if it consumes significant power during  suspend
on a battery powered system.