Re: [PATCH v4 1/8] usb: misc: onboard_hub: rename to onboard_dev

Matthias Kaehlcke
2024-02-21T19:14:14Z

On Tue, Feb 20, 2024 at 03:05:45PM +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>
> ---
> ...-usb-hub => sysfs-bus-platform-onboard-usb-dev} | 4 +-
> MAINTAINERS | 4 +-
> drivers/usb/core/Makefile | 4 +-
> drivers/usb/core/hub.c | 8 +-
> drivers/usb/core/hub.h | 2 +-
> drivers/usb/misc/Kconfig | 16 +-
> drivers/usb/misc/Makefile | 2 +-
> drivers/usb/misc/onboard_usb_dev.c | 518 +++++++++++++++++++++
> .../misc/{onboard_usb_hub.h => onboard_usb_dev.h} | 28 +-
> ...ard_usb_hub_pdevs.c => onboard_usb_dev_pdevs.c} | 47 +-
> drivers/usb/misc/onboard_usb_hub.c | 501 --------------------
> include/linux/usb/onboard_dev.h | 18 +
> include/linux/usb/onboard_hub.h | 18 -
> 13 files changed, 594 insertions(+), 576 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-platform-onboard-usb-hub b/Documentation/ABI/testing/sysfs-bus-platform-onboard-usb-dev
> similarity index 67%
> rename from Documentation/ABI/testing/sysfs-bus-platform-onboard-usb-hub
> rename to Documentation/ABI/testing/sysfs-bus-platform-onboard-usb-dev
> index 42deb0552065..cd31f76362e7 100644
> --- a/Documentation/ABI/testing/sysfs-bus-platform-onboard-usb-hub
> +++ b/Documentation/ABI/testing/sysfs-bus-platform-onboard-usb-dev
> @@ -4,5 +4,5 @@ KernelVersion: 5.20
> Contact: Matthias Kaehlcke <matthias@kaehlcke.net>
> linux-usb@vger.kernel.org
> Description:
> - (RW) Controls whether the USB hub remains always powered
> - during system suspend or not.
> \ No newline at end of file
> + (RW) Controls whether the USB device remains always powered
> + during system suspend or not.

With patch "[2/8] usb: misc: onboard_dev: add support for non-hub devices"
this attribute isn't honed for non-hub devices. With that I'd say leave
the existing comment unchanged, but add a note that this attribute only
exists for hubs. That will also require a change in the patch mentioned
above to omit the creation of the attribute for non-hub devices.

> diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c
> new file mode 100644
> index 000000000000..2103af2cb2a6
> --- /dev/null
> +++ b/drivers/usb/misc/onboard_usb_dev.c
>
> ...
>
> +/*
> + * Returns the onboard_dev platform device that is associated with the USB
> + * device passed as parameter.
> + */
> +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);
> + }
> +
> + onboard_dev = dev_get_drvdata(&pdev->dev);
> + put_device(&pdev->dev);
> +
> + /*
> + * The presence of drvdata ('hub') indicates that the platform driver

drop "('hub')"

> diff --git a/drivers/usb/misc/onboard_usb_hub_pdevs.c b/drivers/usb/misc/onboard_usb_dev_pdevs.c
> similarity index 69%
> rename from drivers/usb/misc/onboard_usb_hub_pdevs.c
> rename to drivers/usb/misc/onboard_usb_dev_pdevs.c
> index ed22a18f4ab7..ca56f67393f1 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
> + * or not.

The last part isn't/won't be true for non-hub devices. Please change it to
something like ". For onboard hubs the platform device can optionally control ..."