This series adds support for the XMOS XVF3500 VocalFusion Voice
Processor[1], a low-latency, 32-bit multicore controller for voice
processing.
The XVF3500 requires a specific power sequence, which consists of
enabling the regulators that control the 3V3 and 1V0 device supplies,
and a reset de-assertion after a delay of at least 100ns. Once in normal
operation, the XVF3500 registers itself as a regular USB device and no
device-specific management is required.
The power management provided by onboard_usb_hub is not specific for hubs
and any other USB device with the same power sequence could profit from
that driver, provided that the device does not have any specific
requirements beyond the power management. To account for non-hub devices,
the driver has been renamed and an extra flag has been added to identify
hubs and provide their specific functionality.
Support for device-specific power suply names has also been added, keeping
generic names for already supported devices to keep backwards
compatibility.
The references to onboard_usb_hub in the core and config files have been
updated as well.
The diff is way much bulkier than the actual code addition because of the
file renaming, so in order to ease reviews and catch hub-specific code
that might still affect non-hub devices, the complete renaming was moved
to a single commit.
This series has been tested with a Rockchip-based SoC and an XMOS
XVF3500-FB167-C.
[1] https://www.xmos.com/xvf3500/
To: Liam Girdwood <[email protected]>
To: Mark Brown <[email protected]>
To: Rob Herring <[email protected]>
To: Krzysztof Kozlowski <[email protected]>
To: Conor Dooley <[email protected]>
To: Matthias Kaehlcke <[email protected]>
To: Greg Kroah-Hartman <[email protected]>
To: Helen Koike <[email protected]>
To: Maarten Lankhorst <[email protected]>
To: Maxime Ripard <[email protected]>
To: Thomas Zimmermann <[email protected]>
To: David Airlie <[email protected]>
To: Daniel Vetter <[email protected]>
To: Catalin Marinas <[email protected]>
To: Will Deacon <[email protected]>
To: Russell King <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Javier Carrasco <[email protected]>
Changes in v5:
- onboard_usb_dev: move device suppy names handling to [1/8].
- onboard_usb_dev.c: make always_powered_in_suspend not visible for
non-hub devices.
- onboard_usb_dev.c: move is_hub check in suspend() to functio entry.
- onboard_usb_dev_pdevs.c: comment rephrasing to account for
hub-specific attribute.
- Link to v4: https://lore.kernel.org/r/[email protected]
Changes in v4:
- General: use device supply names and generics as fallback.
- onbord_usb_dev.c: fix suspend callback for non-hub devices.
- onboard_usb_dev.c: fix typos.
- Link to v3: https://lore.kernel.org/r/[email protected]
Changes in v3:
- onboard_usb_hub: rename to onboard_usb_dev to include non-hub devices.
- onboard_hub_dev: add flag to identify hubs and provide their extra
functionality.
- dt-bindings: add reference to usb-device.yaml and usb node in the
example.
- dt-bindings: generic node name.
- Link to v2: https://lore.kernel.org/r/[email protected]
Changes in v2:
- general: add support in onboard_usb_hub instead of using a dedicated
driver.
- dt-bindings: use generic usb-device compatible ("usbVID,PID").
- Link to v1: https://lore.kernel.org/all/[email protected]/
---
Javier Carrasco (8):
usb: misc: onboard_hub: use device supply names
usb: misc: onboard_hub: rename to onboard_dev
drm: ci: arm64.config: update ONBOARD_USB_HUB to ONBOARD_USB_DEV
arm64: defconfig: update ONBOARD_USB_HUB to ONBOARD_USB_DEV
ARM: multi_v7_defconfig: update ONBOARD_USB_HUB to ONBOAD_USB_DEV
usb: misc: onboard_dev: add support for non-hub devices
ASoC: dt-bindings: xmos,xvf3500: add XMOS XVF3500 voice processor
usb: misc: onboard_hub: add support for XMOS XVF3500
...-usb-hub => sysfs-bus-platform-onboard-usb-dev} | 3 +-
.../devicetree/bindings/sound/xmos,xvf3500.yaml | 63 +++
MAINTAINERS | 4 +-
arch/arm/configs/multi_v7_defconfig | 2 +-
arch/arm64/configs/defconfig | 2 +-
drivers/gpu/drm/ci/arm64.config | 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 | 542 +++++++++++++++++++++
.../misc/{onboard_usb_hub.h => onboard_usb_dev.h} | 58 ++-
...ard_usb_hub_pdevs.c => onboard_usb_dev_pdevs.c} | 47 +-
drivers/usb/misc/onboard_usb_hub.c | 49 +-
include/linux/usb/onboard_dev.h | 18 +
include/linux/usb/onboard_hub.h | 18 -
17 files changed, 741 insertions(+), 101 deletions(-)
---
base-commit: 41bccc98fb7931d63d03f326a746ac4d429c1dd3
change-id: 20240130-onboard_xvf3500-6c0e78d11a1b
Best regards,
--
Javier Carrasco <[email protected]>
The current implementation uses generic names for the power supplies,
which conflicts with proper name definitions in the device bindings.
Add a per-device property to include real supply names and keep generic
names for existing devices to keep backward compatibility.
Signed-off-by: Javier Carrasco <[email protected]>
---
drivers/usb/misc/onboard_usb_hub.c | 49 ++++++++++++++++++++------------------
drivers/usb/misc/onboard_usb_hub.h | 12 ++++++++++
2 files changed, 38 insertions(+), 23 deletions(-)
diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c
index 0dd2b032c90b..3755f6cc1eda 100644
--- a/drivers/usb/misc/onboard_usb_hub.c
+++ b/drivers/usb/misc/onboard_usb_hub.c
@@ -29,17 +29,6 @@
#include "onboard_usb_hub.h"
-/*
- * Use generic names, as the actual names might differ between hubs. If a new
- * hub requires more than the currently supported supplies, add a new one here.
- */
-static const char * const supply_names[] = {
- "vdd",
- "vdd2",
-};
-
-#define MAX_SUPPLIES ARRAY_SIZE(supply_names)
-
static void onboard_hub_attach_usb_driver(struct work_struct *work);
static struct usb_device_driver onboard_hub_usbdev_driver;
@@ -65,6 +54,30 @@ struct onboard_hub {
struct clk *clk;
};
+static int onboard_hub_get_regulator_bulk(struct device *dev,
+ struct onboard_hub *onboard_hub)
+{
+ unsigned int i;
+ int err;
+
+ const char * const *supply_names = onboard_hub->pdata->supply_names;
+
+ if (onboard_hub->pdata->num_supplies > MAX_SUPPLIES)
+ return dev_err_probe(dev, -EINVAL, "max %d supplies supported!\n",
+ MAX_SUPPLIES);
+
+ for (i = 0; i < onboard_hub->pdata->num_supplies; i++)
+ onboard_hub->supplies[i].supply = supply_names[i];
+
+ err = devm_regulator_bulk_get(dev, onboard_hub->pdata->num_supplies,
+ onboard_hub->supplies);
+ if (err)
+ dev_err(dev, "Failed to get regulator supplies: %pe\n",
+ ERR_PTR(err));
+
+ return err;
+}
+
static int onboard_hub_power_on(struct onboard_hub *hub)
{
int err;
@@ -253,7 +266,6 @@ static int onboard_hub_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct onboard_hub *hub;
- unsigned int i;
int err;
hub = devm_kzalloc(dev, sizeof(*hub), GFP_KERNEL);
@@ -264,18 +276,9 @@ static int onboard_hub_probe(struct platform_device *pdev)
if (!hub->pdata)
return -EINVAL;
- if (hub->pdata->num_supplies > MAX_SUPPLIES)
- return dev_err_probe(dev, -EINVAL, "max %zu supplies supported!\n",
- MAX_SUPPLIES);
-
- for (i = 0; i < hub->pdata->num_supplies; i++)
- hub->supplies[i].supply = supply_names[i];
-
- err = devm_regulator_bulk_get(dev, hub->pdata->num_supplies, hub->supplies);
- if (err) {
- dev_err(dev, "Failed to get regulator supplies: %pe\n", ERR_PTR(err));
+ err = onboard_hub_get_regulator_bulk(dev, onboard_hub);
+ if (err)
return err;
- }
hub->clk = devm_clk_get_optional(dev, NULL);
if (IS_ERR(hub->clk))
diff --git a/drivers/usb/misc/onboard_usb_hub.h b/drivers/usb/misc/onboard_usb_hub.h
index f360d5cf8d8a..ea24bd6790f0 100644
--- a/drivers/usb/misc/onboard_usb_hub.h
+++ b/drivers/usb/misc/onboard_usb_hub.h
@@ -6,54 +6,66 @@
#ifndef _USB_MISC_ONBOARD_USB_HUB_H
#define _USB_MISC_ONBOARD_USB_HUB_H
+#define MAX_SUPPLIES 2
+
struct onboard_hub_pdata {
unsigned long reset_us; /* reset pulse width in us */
unsigned int num_supplies; /* number of supplies */
+ const char * const supply_names[MAX_SUPPLIES]; /* use the real names */
};
static const struct onboard_hub_pdata microchip_usb424_data = {
.reset_us = 1,
.num_supplies = 1,
+ .supply_names = { "vdd" },
};
static const struct onboard_hub_pdata microchip_usb5744_data = {
.reset_us = 0,
.num_supplies = 2,
+ .supply_names = { "vdd", "vdd2" },
};
static const struct onboard_hub_pdata realtek_rts5411_data = {
.reset_us = 0,
.num_supplies = 1,
+ .supply_names = { "vdd" },
};
static const struct onboard_hub_pdata ti_tusb8041_data = {
.reset_us = 3000,
.num_supplies = 1,
+ .supply_names = { "vdd" },
};
static const struct onboard_hub_pdata cypress_hx3_data = {
.reset_us = 10000,
.num_supplies = 2,
+ .supply_names = { "vdd", "vdd2" },
};
static const struct onboard_hub_pdata cypress_hx2vl_data = {
.reset_us = 1,
.num_supplies = 1,
+ .supply_names = { "vdd" },
};
static const struct onboard_hub_pdata genesys_gl850g_data = {
.reset_us = 3,
.num_supplies = 1,
+ .supply_names = { "vdd" },
};
static const struct onboard_hub_pdata genesys_gl852g_data = {
.reset_us = 50,
.num_supplies = 1,
+ .supply_names = { "vdd" },
};
static const struct onboard_hub_pdata vialab_vl817_data = {
.reset_us = 10,
.num_supplies = 1,
+ .supply_names = { "vdd" },
};
static const struct of_device_id onboard_hub_match[] = {
--
2.40.1
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 <[email protected]>
---
...-usb-hub => sysfs-bus-platform-onboard-usb-dev} | 3 +-
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 | 519 +++++++++++++++++++++
.../misc/{onboard_usb_hub.h => onboard_usb_dev.h} | 28 +-
...ard_usb_hub_pdevs.c => onboard_usb_dev_pdevs.c} | 47 +-
include/linux/usb/onboard_dev.h | 18 +
include/linux/usb/onboard_hub.h | 18 -
12 files changed, 595 insertions(+), 74 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 74%
rename from Documentation/ABI/testing/sysfs-bus-platform-onboard-usb-hub
rename to Documentation/ABI/testing/sysfs-bus-platform-onboard-usb-dev
index 42deb0552065..b06a48c3c85a 100644
--- a/Documentation/ABI/testing/sysfs-bus-platform-onboard-usb-hub
+++ b/Documentation/ABI/testing/sysfs-bus-platform-onboard-usb-dev
@@ -5,4 +5,5 @@ Contact: Matthias Kaehlcke <[email protected]>
[email protected]
Description:
(RW) Controls whether the USB hub remains always powered
- during system suspend or not.
\ No newline at end of file
+ during system suspend or not. This attribute is not
+ available for non-hub devices.
diff --git a/MAINTAINERS b/MAINTAINERS
index 8999497011a2..7ad556ecca40 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16297,8 +16297,8 @@ ONBOARD USB HUB DRIVER
M: Matthias Kaehlcke <[email protected]>
L: [email protected]
S: Maintained
-F: Documentation/ABI/testing/sysfs-bus-platform-onboard-usb-hub
-F: drivers/usb/misc/onboard_usb_hub.c
+F: Documentation/ABI/testing/sysfs-bus-platform-onboard-usb-dev
+F: drivers/usb/misc/onboard_usb_dev.c
ONENAND FLASH DRIVER
M: Kyungmin Park <[email protected]>
diff --git a/drivers/usb/core/Makefile b/drivers/usb/core/Makefile
index 7d338e9c0657..ac006abd13b3 100644
--- a/drivers/usb/core/Makefile
+++ b/drivers/usb/core/Makefile
@@ -12,8 +12,8 @@ usbcore-$(CONFIG_OF) += of.o
usbcore-$(CONFIG_USB_PCI) += hcd-pci.o
usbcore-$(CONFIG_ACPI) += usb-acpi.o
-ifdef CONFIG_USB_ONBOARD_HUB
-usbcore-y += ../misc/onboard_usb_hub_pdevs.o
+ifdef CONFIG_USB_ONBOARD_DEV
+usbcore-y += ../misc/onboard_usb_dev_pdevs.o
endif
obj-$(CONFIG_USB) += usbcore.o
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index ffd7c99e24a3..f85b3e928a35 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -23,7 +23,7 @@
#include <linux/usb.h>
#include <linux/usbdevice_fs.h>
#include <linux/usb/hcd.h>
-#include <linux/usb/onboard_hub.h>
+#include <linux/usb/onboard_dev.h>
#include <linux/usb/otg.h>
#include <linux/usb/quirks.h>
#include <linux/workqueue.h>
@@ -1776,7 +1776,7 @@ static void hub_disconnect(struct usb_interface *intf)
if (hub->quirk_disable_autosuspend)
usb_autopm_put_interface(intf);
- onboard_hub_destroy_pdevs(&hub->onboard_hub_devs);
+ onboard_dev_destroy_pdevs(&hub->onboard_devs);
kref_put(&hub->kref, hub_release);
}
@@ -1895,7 +1895,7 @@ static int hub_probe(struct usb_interface *intf, const struct usb_device_id *id)
INIT_DELAYED_WORK(&hub->leds, led_work);
INIT_DELAYED_WORK(&hub->init_work, NULL);
INIT_WORK(&hub->events, hub_event);
- INIT_LIST_HEAD(&hub->onboard_hub_devs);
+ INIT_LIST_HEAD(&hub->onboard_devs);
spin_lock_init(&hub->irq_urb_lock);
timer_setup(&hub->irq_urb_retry, hub_retry_irq_urb, 0);
usb_get_intf(intf);
@@ -1925,7 +1925,7 @@ static int hub_probe(struct usb_interface *intf, const struct usb_device_id *id)
}
if (hub_configure(hub, &desc->endpoint[0].desc) >= 0) {
- onboard_hub_create_pdevs(hdev, &hub->onboard_hub_devs);
+ onboard_dev_create_pdevs(hdev, &hub->onboard_devs);
return 0;
}
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index 43ce21c96a51..3820703b11d8 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -74,7 +74,7 @@ struct usb_hub {
spinlock_t irq_urb_lock;
struct timer_list irq_urb_retry;
struct usb_port **ports;
- struct list_head onboard_hub_devs;
+ struct list_head onboard_devs;
};
/**
diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
index c510af7baa0d..50b86d531701 100644
--- a/drivers/usb/misc/Kconfig
+++ b/drivers/usb/misc/Kconfig
@@ -316,18 +316,18 @@ config BRCM_USB_PINMAP
signals, which are typically on dedicated pins on the chip,
to any gpio.
-config USB_ONBOARD_HUB
- tristate "Onboard USB hub support"
+config USB_ONBOARD_DEV
+ tristate "Onboard USB device support"
depends on OF
help
- Say Y here if you want to support discrete onboard USB hubs that
- don't require an additional control bus for initialization, but
- need some non-trivial form of initialization, such as enabling a
- power regulator. An example for such a hub is the Realtek
- RTS5411.
+ Say Y here if you want to support discrete onboard USB devices
+ that don't require an additional control bus for initialization,
+ but need some non-trivial form of initialization, such as
+ enabling a power regulator. An example for such device is the
+ Realtek RTS5411 hub.
This driver can be used as a module but its state (module vs
builtin) must match the state of the USB subsystem. Enabling
this config will enable the driver and it will automatically
match the state of the USB subsystem. If this driver is a
- module it will be called onboard_usb_hub.
+ module it will be called onboard_usb_dev.
diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
index 0bc732bcb162..0cd5bc8f52fe 100644
--- a/drivers/usb/misc/Makefile
+++ b/drivers/usb/misc/Makefile
@@ -33,4 +33,4 @@ obj-$(CONFIG_USB_CHAOSKEY) += chaoskey.o
obj-$(CONFIG_USB_SISUSBVGA) += sisusbvga/
obj-$(CONFIG_USB_LINK_LAYER_TEST) += lvstest.o
obj-$(CONFIG_BRCM_USB_PINMAP) += brcmstb-usb-pinmap.o
-obj-$(CONFIG_USB_ONBOARD_HUB) += onboard_usb_hub.o
+obj-$(CONFIG_USB_ONBOARD_DEV) += onboard_usb_dev.o
diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c
new file mode 100644
index 000000000000..e1779bd2d126
--- /dev/null
+++ b/drivers/usb/misc/onboard_usb_dev.c
@@ -0,0 +1,519 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Driver for onboard USB devices
+ *
+ * Copyright (c) 2022, Google LLC
+ */
+
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/export.h>
+#include <linux/err.h>
+#include <linux/gpio/consumer.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+#include <linux/suspend.h>
+#include <linux/sysfs.h>
+#include <linux/usb.h>
+#include <linux/usb/hcd.h>
+#include <linux/usb/onboard_dev.h>
+#include <linux/workqueue.h>
+
+#include "onboard_usb_dev.h"
+
+static void onboard_dev_attach_usb_driver(struct work_struct *work);
+
+static struct usb_device_driver onboard_dev_usbdev_driver;
+static DECLARE_WORK(attach_usb_driver_work, onboard_dev_attach_usb_driver);
+
+/************************** Platform driver **************************/
+
+struct usbdev_node {
+ struct usb_device *udev;
+ struct list_head list;
+};
+
+struct onboard_dev {
+ struct regulator_bulk_data supplies[MAX_SUPPLIES];
+ struct device *dev;
+ const struct onboard_dev_pdata *pdata;
+ struct gpio_desc *reset_gpio;
+ bool always_powered_in_suspend;
+ bool is_powered_on;
+ bool going_away;
+ struct list_head udev_list;
+ struct mutex lock;
+ struct clk *clk;
+};
+
+static int onboard_dev_get_regulator_bulk(struct device *dev,
+ struct onboard_dev *onboard_dev)
+{
+ unsigned int i;
+ int err;
+
+ const char * const *supply_names = onboard_dev->pdata->supply_names;
+
+ if (onboard_dev->pdata->num_supplies > MAX_SUPPLIES)
+ return dev_err_probe(dev, -EINVAL, "max %d supplies supported!\n",
+ MAX_SUPPLIES);
+
+ for (i = 0; i < onboard_dev->pdata->num_supplies; i++)
+ onboard_dev->supplies[i].supply = supply_names[i];
+
+ err = devm_regulator_bulk_get(dev, onboard_dev->pdata->num_supplies,
+ onboard_dev->supplies);
+ if (err)
+ dev_err(dev, "Failed to get regulator supplies: %pe\n",
+ ERR_PTR(err));
+
+ return err;
+}
+
+static int onboard_dev_power_on(struct onboard_dev *onboard_dev)
+{
+ int err;
+
+ err = clk_prepare_enable(onboard_dev->clk);
+ if (err) {
+ dev_err(onboard_dev->dev, "failed to enable clock: %pe\n",
+ ERR_PTR(err));
+ return err;
+ }
+
+ err = regulator_bulk_enable(onboard_dev->pdata->num_supplies,
+ onboard_dev->supplies);
+ if (err) {
+ dev_err(onboard_dev->dev, "failed to enable supplies: %pe\n",
+ ERR_PTR(err));
+ return err;
+ }
+
+ fsleep(onboard_dev->pdata->reset_us);
+ gpiod_set_value_cansleep(onboard_dev->reset_gpio, 0);
+
+ onboard_dev->is_powered_on = true;
+
+ return 0;
+}
+
+static int onboard_dev_power_off(struct onboard_dev *onboard_dev)
+{
+ int err;
+
+ gpiod_set_value_cansleep(onboard_dev->reset_gpio, 1);
+
+ err = regulator_bulk_disable(onboard_dev->pdata->num_supplies,
+ onboard_dev->supplies);
+ if (err) {
+ dev_err(onboard_dev->dev, "failed to disable supplies: %pe\n",
+ ERR_PTR(err));
+ return err;
+ }
+
+ clk_disable_unprepare(onboard_dev->clk);
+
+ onboard_dev->is_powered_on = false;
+
+ return 0;
+}
+
+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;
+ }
+ }
+
+ mutex_unlock(&onboard_dev->lock);
+
+ if (!power_off)
+ return 0;
+
+ return onboard_dev_power_off(onboard_dev);
+}
+
+static int __maybe_unused onboard_dev_resume(struct device *dev)
+{
+ struct onboard_dev *onboard_dev = dev_get_drvdata(dev);
+
+ if (onboard_dev->is_powered_on)
+ return 0;
+
+ return onboard_dev_power_on(onboard_dev);
+}
+
+static inline void get_udev_link_name(const struct usb_device *udev, char *buf,
+ size_t size)
+{
+ snprintf(buf, size, "usb_dev.%s", dev_name(&udev->dev));
+}
+
+static int onboard_dev_add_usbdev(struct onboard_dev *onboard_dev,
+ struct usb_device *udev)
+{
+ struct usbdev_node *node;
+ char link_name[64];
+ int err;
+
+ mutex_lock(&onboard_dev->lock);
+
+ if (onboard_dev->going_away) {
+ err = -EINVAL;
+ goto error;
+ }
+
+ node = kzalloc(sizeof(*node), GFP_KERNEL);
+ if (!node) {
+ err = -ENOMEM;
+ goto error;
+ }
+
+ node->udev = udev;
+
+ list_add(&node->list, &onboard_dev->udev_list);
+
+ mutex_unlock(&onboard_dev->lock);
+
+ get_udev_link_name(udev, link_name, sizeof(link_name));
+ WARN_ON(sysfs_create_link(&onboard_dev->dev->kobj, &udev->dev.kobj,
+ link_name));
+
+ return 0;
+
+error:
+ mutex_unlock(&onboard_dev->lock);
+
+ return err;
+}
+
+static void onboard_dev_remove_usbdev(struct onboard_dev *onboard_dev,
+ const struct usb_device *udev)
+{
+ struct usbdev_node *node;
+ char link_name[64];
+
+ get_udev_link_name(udev, link_name, sizeof(link_name));
+ sysfs_remove_link(&onboard_dev->dev->kobj, link_name);
+
+ mutex_lock(&onboard_dev->lock);
+
+ list_for_each_entry(node, &onboard_dev->udev_list, list) {
+ if (node->udev == udev) {
+ list_del(&node->list);
+ kfree(node);
+ break;
+ }
+ }
+
+ mutex_unlock(&onboard_dev->lock);
+}
+
+static ssize_t always_powered_in_suspend_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ const struct onboard_dev *onboard_dev = dev_get_drvdata(dev);
+
+ return sysfs_emit(buf, "%d\n", onboard_dev->always_powered_in_suspend);
+}
+
+static ssize_t always_powered_in_suspend_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct onboard_dev *onboard_dev = dev_get_drvdata(dev);
+ bool val;
+ int ret;
+
+ ret = kstrtobool(buf, &val);
+ if (ret < 0)
+ return ret;
+
+ onboard_dev->always_powered_in_suspend = val;
+
+ return count;
+}
+static DEVICE_ATTR_RW(always_powered_in_suspend);
+
+static struct attribute *onboard_dev_attrs[] = {
+ &dev_attr_always_powered_in_suspend.attr,
+ NULL,
+};
+ATTRIBUTE_GROUPS(onboard_dev);
+
+static void onboard_dev_attach_usb_driver(struct work_struct *work)
+{
+ int err;
+
+ err = driver_attach(&onboard_dev_usbdev_driver.driver);
+ if (err)
+ pr_err("Failed to attach USB driver: %pe\n", ERR_PTR(err));
+}
+
+static int onboard_dev_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct onboard_dev *onboard_dev;
+ int err;
+
+ onboard_dev = devm_kzalloc(dev, sizeof(*onboard_dev), GFP_KERNEL);
+ if (!onboard_dev)
+ return -ENOMEM;
+
+ onboard_dev->pdata = device_get_match_data(&pdev->dev);
+ if (!onboard_dev->pdata)
+ return -EINVAL;
+
+ err = onboard_dev_get_regulator_bulk(dev, onboard_dev);
+ if (err)
+ return err;
+
+ onboard_dev->clk = devm_clk_get_optional(dev, NULL);
+ if (IS_ERR(onboard_dev->clk))
+ return dev_err_probe(dev, PTR_ERR(onboard_dev->clk),
+ "failed to get clock\n");
+
+ onboard_dev->reset_gpio = devm_gpiod_get_optional(dev, "reset",
+ GPIOD_OUT_HIGH);
+ if (IS_ERR(onboard_dev->reset_gpio))
+ return dev_err_probe(dev, PTR_ERR(onboard_dev->reset_gpio),
+ "failed to get reset GPIO\n");
+
+ onboard_dev->dev = dev;
+ mutex_init(&onboard_dev->lock);
+ INIT_LIST_HEAD(&onboard_dev->udev_list);
+
+ dev_set_drvdata(dev, onboard_dev);
+
+ err = onboard_dev_power_on(onboard_dev);
+ if (err)
+ return err;
+
+ /*
+ * The USB driver might have been detached from the USB devices by
+ * onboard_dev_remove() (e.g. through an 'unbind' by userspace),
+ * make sure to re-attach it if needed.
+ *
+ * This needs to be done deferred to avoid self-deadlocks on systems
+ * with nested onboard hubs.
+ */
+ schedule_work(&attach_usb_driver_work);
+
+ return 0;
+}
+
+static void onboard_dev_remove(struct platform_device *pdev)
+{
+ struct onboard_dev *onboard_dev = dev_get_drvdata(&pdev->dev);
+ struct usbdev_node *node;
+ struct usb_device *udev;
+
+ onboard_dev->going_away = true;
+
+ mutex_lock(&onboard_dev->lock);
+
+ /* unbind the USB devices to avoid dangling references to this device */
+ while (!list_empty(&onboard_dev->udev_list)) {
+ node = list_first_entry(&onboard_dev->udev_list,
+ struct usbdev_node, list);
+ udev = node->udev;
+
+ /*
+ * Unbinding the driver will call onboard_dev_remove_usbdev(),
+ * which acquires onboard_dev->lock. We must release the lock
+ * first.
+ */
+ get_device(&udev->dev);
+ mutex_unlock(&onboard_dev->lock);
+ device_release_driver(&udev->dev);
+ put_device(&udev->dev);
+ mutex_lock(&onboard_dev->lock);
+ }
+
+ mutex_unlock(&onboard_dev->lock);
+
+ onboard_dev_power_off(onboard_dev);
+}
+
+MODULE_DEVICE_TABLE(of, onboard_dev_match);
+
+static const struct dev_pm_ops __maybe_unused onboard_dev_pm_ops = {
+ SET_LATE_SYSTEM_SLEEP_PM_OPS(onboard_dev_suspend, onboard_dev_resume)
+};
+
+static struct platform_driver onboard_dev_driver = {
+ .probe = onboard_dev_probe,
+ .remove_new = onboard_dev_remove,
+
+ .driver = {
+ .name = "onboard-usb-dev",
+ .of_match_table = onboard_dev_match,
+ .pm = pm_ptr(&onboard_dev_pm_ops),
+ .dev_groups = onboard_dev_groups,
+ },
+};
+
+/************************** USB driver **************************/
+
+#define VENDOR_ID_CYPRESS 0x04b4
+#define VENDOR_ID_GENESYS 0x05e3
+#define VENDOR_ID_MICROCHIP 0x0424
+#define VENDOR_ID_REALTEK 0x0bda
+#define VENDOR_ID_TI 0x0451
+#define VENDOR_ID_VIA 0x2109
+
+/*
+ * 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 indicates that the platform driver finished
+ * probing. This handles the case where (conceivably) we could be
+ * running at the exact same time as the platform driver's probe. If
+ * we detect the race we request probe deferral and we'll come back and
+ * try again.
+ */
+ if (!onboard_dev)
+ return ERR_PTR(-EPROBE_DEFER);
+
+ return onboard_dev;
+}
+
+static int onboard_dev_usbdev_probe(struct usb_device *udev)
+{
+ struct device *dev = &udev->dev;
+ struct onboard_dev *onboard_dev;
+ int err;
+
+ /* ignore supported devices without device tree node */
+ if (!dev->of_node)
+ return -ENODEV;
+
+ onboard_dev = _find_onboard_dev(dev);
+ if (IS_ERR(onboard_dev))
+ return PTR_ERR(onboard_dev);
+
+ dev_set_drvdata(dev, onboard_dev);
+
+ err = onboard_dev_add_usbdev(onboard_dev, udev);
+ if (err)
+ return err;
+
+ return 0;
+}
+
+static void onboard_dev_usbdev_disconnect(struct usb_device *udev)
+{
+ struct onboard_dev *onboard_dev = dev_get_drvdata(&udev->dev);
+
+ onboard_dev_remove_usbdev(onboard_dev, udev);
+}
+
+static const struct usb_device_id onboard_dev_id_table[] = {
+ { USB_DEVICE(VENDOR_ID_CYPRESS, 0x6504) }, /* CYUSB33{0,1,2}x/CYUSB230x 3.0 */
+ { USB_DEVICE(VENDOR_ID_CYPRESS, 0x6506) }, /* CYUSB33{0,1,2}x/CYUSB230x 2.0 */
+ { USB_DEVICE(VENDOR_ID_CYPRESS, 0x6570) }, /* CY7C6563x 2.0 */
+ { USB_DEVICE(VENDOR_ID_GENESYS, 0x0608) }, /* Genesys Logic GL850G USB 2.0 */
+ { USB_DEVICE(VENDOR_ID_GENESYS, 0x0610) }, /* Genesys Logic GL852G USB 2.0 */
+ { USB_DEVICE(VENDOR_ID_GENESYS, 0x0620) }, /* Genesys Logic GL3523 USB 3.1 */
+ { USB_DEVICE(VENDOR_ID_MICROCHIP, 0x2412) }, /* USB2412 USB 2.0 */
+ { USB_DEVICE(VENDOR_ID_MICROCHIP, 0x2514) }, /* USB2514B USB 2.0 */
+ { USB_DEVICE(VENDOR_ID_MICROCHIP, 0x2517) }, /* USB2517 USB 2.0 */
+ { USB_DEVICE(VENDOR_ID_MICROCHIP, 0x2744) }, /* USB5744 USB 2.0 */
+ { USB_DEVICE(VENDOR_ID_MICROCHIP, 0x5744) }, /* USB5744 USB 3.0 */
+ { USB_DEVICE(VENDOR_ID_REALTEK, 0x0411) }, /* RTS5411 USB 3.1 */
+ { USB_DEVICE(VENDOR_ID_REALTEK, 0x5411) }, /* RTS5411 USB 2.1 */
+ { USB_DEVICE(VENDOR_ID_REALTEK, 0x0414) }, /* RTS5414 USB 3.2 */
+ { USB_DEVICE(VENDOR_ID_REALTEK, 0x5414) }, /* RTS5414 USB 2.1 */
+ { USB_DEVICE(VENDOR_ID_TI, 0x8140) }, /* TI USB8041 3.0 */
+ { USB_DEVICE(VENDOR_ID_TI, 0x8142) }, /* TI USB8041 2.0 */
+ { USB_DEVICE(VENDOR_ID_VIA, 0x0817) }, /* VIA VL817 3.1 */
+ { USB_DEVICE(VENDOR_ID_VIA, 0x2817) }, /* VIA VL817 2.0 */
+ {}
+};
+MODULE_DEVICE_TABLE(usb, onboard_dev_id_table);
+
+static struct usb_device_driver onboard_dev_usbdev_driver = {
+ .name = "onboard-usb-dev",
+ .probe = onboard_dev_usbdev_probe,
+ .disconnect = onboard_dev_usbdev_disconnect,
+ .generic_subclass = 1,
+ .supports_autosuspend = 1,
+ .id_table = onboard_dev_id_table,
+};
+
+static int __init onboard_dev_init(void)
+{
+ int ret;
+
+ ret = usb_register_device_driver(&onboard_dev_usbdev_driver, THIS_MODULE);
+ if (ret)
+ return ret;
+
+ ret = platform_driver_register(&onboard_dev_driver);
+ if (ret)
+ usb_deregister_device_driver(&onboard_dev_usbdev_driver);
+
+ return ret;
+}
+module_init(onboard_dev_init);
+
+static void __exit onboard_dev_exit(void)
+{
+ usb_deregister_device_driver(&onboard_dev_usbdev_driver);
+ platform_driver_unregister(&onboard_dev_driver);
+
+ cancel_work_sync(&attach_usb_driver_work);
+}
+module_exit(onboard_dev_exit);
+
+MODULE_AUTHOR("Matthias Kaehlcke <[email protected]>");
+MODULE_DESCRIPTION("Driver for discrete onboard USB devices");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/usb/misc/onboard_usb_hub.h b/drivers/usb/misc/onboard_usb_dev.h
similarity index 74%
rename from drivers/usb/misc/onboard_usb_hub.h
rename to drivers/usb/misc/onboard_usb_dev.h
index ea24bd6790f0..470736483cdf 100644
--- a/drivers/usb/misc/onboard_usb_hub.h
+++ b/drivers/usb/misc/onboard_usb_dev.h
@@ -3,72 +3,72 @@
* Copyright (c) 2022, Google LLC
*/
-#ifndef _USB_MISC_ONBOARD_USB_HUB_H
-#define _USB_MISC_ONBOARD_USB_HUB_H
+#ifndef _USB_MISC_ONBOARD_USB_DEV_H
+#define _USB_MISC_ONBOARD_USB_DEV_H
#define MAX_SUPPLIES 2
-struct onboard_hub_pdata {
+struct onboard_dev_pdata {
unsigned long reset_us; /* reset pulse width in us */
unsigned int num_supplies; /* number of supplies */
const char * const supply_names[MAX_SUPPLIES]; /* use the real names */
};
-static const struct onboard_hub_pdata microchip_usb424_data = {
+static const struct onboard_dev_pdata microchip_usb424_data = {
.reset_us = 1,
.num_supplies = 1,
.supply_names = { "vdd" },
};
-static const struct onboard_hub_pdata microchip_usb5744_data = {
+static const struct onboard_dev_pdata microchip_usb5744_data = {
.reset_us = 0,
.num_supplies = 2,
.supply_names = { "vdd", "vdd2" },
};
-static const struct onboard_hub_pdata realtek_rts5411_data = {
+static const struct onboard_dev_pdata realtek_rts5411_data = {
.reset_us = 0,
.num_supplies = 1,
.supply_names = { "vdd" },
};
-static const struct onboard_hub_pdata ti_tusb8041_data = {
+static const struct onboard_dev_pdata ti_tusb8041_data = {
.reset_us = 3000,
.num_supplies = 1,
.supply_names = { "vdd" },
};
-static const struct onboard_hub_pdata cypress_hx3_data = {
+static const struct onboard_dev_pdata cypress_hx3_data = {
.reset_us = 10000,
.num_supplies = 2,
.supply_names = { "vdd", "vdd2" },
};
-static const struct onboard_hub_pdata cypress_hx2vl_data = {
+static const struct onboard_dev_pdata cypress_hx2vl_data = {
.reset_us = 1,
.num_supplies = 1,
.supply_names = { "vdd" },
};
-static const struct onboard_hub_pdata genesys_gl850g_data = {
+static const struct onboard_dev_pdata genesys_gl850g_data = {
.reset_us = 3,
.num_supplies = 1,
.supply_names = { "vdd" },
};
-static const struct onboard_hub_pdata genesys_gl852g_data = {
+static const struct onboard_dev_pdata genesys_gl852g_data = {
.reset_us = 50,
.num_supplies = 1,
.supply_names = { "vdd" },
};
-static const struct onboard_hub_pdata vialab_vl817_data = {
+static const struct onboard_dev_pdata vialab_vl817_data = {
.reset_us = 10,
.num_supplies = 1,
.supply_names = { "vdd" },
};
-static const struct of_device_id onboard_hub_match[] = {
+static const struct of_device_id onboard_dev_match[] = {
{ .compatible = "usb424,2412", .data = µchip_usb424_data, },
{ .compatible = "usb424,2514", .data = µchip_usb424_data, },
{ .compatible = "usb424,2517", .data = µchip_usb424_data, },
@@ -92,4 +92,4 @@ static const struct of_device_id onboard_hub_match[] = {
{}
};
-#endif /* _USB_MISC_ONBOARD_USB_HUB_H */
+#endif /* _USB_MISC_ONBOARD_USB_DEV_H */
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..722504752ce3 100644
--- a/drivers/usb/misc/onboard_usb_hub_pdevs.c
+++ b/drivers/usb/misc/onboard_usb_dev_pdevs.c
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: GPL-2.0-only
/*
- * API for creating and destroying USB onboard hub platform devices
+ * API for creating and destroying USB onboard platform devices
*
* Copyright (c) 2022, Google LLC
*/
@@ -15,29 +15,30 @@
#include <linux/usb.h>
#include <linux/usb/hcd.h>
#include <linux/usb/of.h>
-#include <linux/usb/onboard_hub.h>
+#include <linux/usb/onboard_dev.h>
-#include "onboard_usb_hub.h"
+#include "onboard_usb_dev.h"
struct pdev_list_entry {
struct platform_device *pdev;
struct list_head node;
};
-static bool of_is_onboard_usb_hub(const struct device_node *np)
+static bool of_is_onboard_usb_dev(struct device_node *np)
{
- return !!of_match_node(onboard_hub_match, np);
+ return !!of_match_node(onboard_dev_match, np);
}
/**
- * 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, ...). For onboard
+ * hubs, it can optionally control whether the device remains powered during
+ * system suspend or not.
*
* To keep track of the platform devices they are added to a list that is owned
* by the parent hub.
@@ -50,9 +51,9 @@ static bool of_is_onboard_usb_hub(const struct device_node *np)
* node. That means the root hubs of the primary and secondary HCD share the
* same device tree node (the HCD node). As a result this function can be called
* twice with the same DT node for root hubs. We only want to create a single
- * platform device for each physical onboard hub, hence for root hubs the loop
- * is only executed for the root hub of the primary HCD. Since the function
- * scans through all child nodes it still creates pdevs for onboard hubs
+ * platform device for each physical onboard device, hence for root hubs the
+ * loop is only executed for the root hub of the primary HCD. Since the function
+ * scans through all child nodes it still creates pdevs for onboard devices
* connected to the root hub of the secondary HCD if needed.
*
* Further there must be only one platform device for onboard hubs with a peer
@@ -63,7 +64,7 @@ static bool of_is_onboard_usb_hub(const struct device_node *np)
* the function processes the nodes of both peers. A platform device is only
* created if the peer hub doesn't have one already.
*/
-void onboard_hub_create_pdevs(struct usb_device *parent_hub, struct list_head *pdev_list)
+void onboard_dev_create_pdevs(struct usb_device *parent_hub, struct list_head *pdev_list)
{
int i;
struct usb_hcd *hcd = bus_to_hcd(parent_hub->bus);
@@ -82,7 +83,7 @@ void onboard_hub_create_pdevs(struct usb_device *parent_hub, struct list_head *p
if (!np)
continue;
- if (!of_is_onboard_usb_hub(np))
+ if (!of_is_onboard_usb_dev(np))
goto node_put;
npc = of_parse_phandle(np, "peer-hub", 0);
@@ -104,7 +105,7 @@ void onboard_hub_create_pdevs(struct usb_device *parent_hub, struct list_head *p
pdev = of_platform_device_create(np, NULL, &parent_hub->dev);
if (!pdev) {
dev_err(&parent_hub->dev,
- "failed to create platform device for onboard hub '%pOF'\n", np);
+ "failed to create platform device for onboard dev '%pOF'\n", np);
goto node_put;
}
@@ -121,16 +122,16 @@ void onboard_hub_create_pdevs(struct usb_device *parent_hub, struct list_head *p
of_node_put(np);
}
}
-EXPORT_SYMBOL_GPL(onboard_hub_create_pdevs);
+EXPORT_SYMBOL_GPL(onboard_dev_create_pdevs);
/**
- * onboard_hub_destroy_pdevs -- free resources of onboard hub platform devices
- * @pdev_list : list of onboard hub platform devices
+ * onboard_dev_destroy_pdevs -- free resources of onboard platform devices
+ * @pdev_list : list of onboard platform devices
*
* Destroys the platform devices in the given list and frees the memory associated
* with the list entry.
*/
-void onboard_hub_destroy_pdevs(struct list_head *pdev_list)
+void onboard_dev_destroy_pdevs(struct list_head *pdev_list)
{
struct pdev_list_entry *pdle, *tmp;
@@ -140,4 +141,4 @@ void onboard_hub_destroy_pdevs(struct list_head *pdev_list)
kfree(pdle);
}
}
-EXPORT_SYMBOL_GPL(onboard_hub_destroy_pdevs);
+EXPORT_SYMBOL_GPL(onboard_dev_destroy_pdevs);
diff --git a/include/linux/usb/onboard_dev.h b/include/linux/usb/onboard_dev.h
new file mode 100644
index 000000000000..b79db6d193c8
--- /dev/null
+++ b/include/linux/usb/onboard_dev.h
@@ -0,0 +1,18 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __LINUX_USB_ONBOARD_DEV_H
+#define __LINUX_USB_ONBOARD_DEV_H
+
+struct usb_device;
+struct list_head;
+
+#if IS_ENABLED(CONFIG_USB_ONBOARD_DEV)
+void onboard_dev_create_pdevs(struct usb_device *parent_dev, struct list_head *pdev_list);
+void onboard_dev_destroy_pdevs(struct list_head *pdev_list);
+#else
+static inline void onboard_dev_create_pdevs(struct usb_device *parent_dev,
+ struct list_head *pdev_list) {}
+static inline void onboard_dev_destroy_pdevs(struct list_head *pdev_list) {}
+#endif
+
+#endif /* __LINUX_USB_ONBOARD_DEV_H */
diff --git a/include/linux/usb/onboard_hub.h b/include/linux/usb/onboard_hub.h
deleted file mode 100644
index d9373230556e..000000000000
--- a/include/linux/usb/onboard_hub.h
+++ /dev/null
@@ -1,18 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-
-#ifndef __LINUX_USB_ONBOARD_HUB_H
-#define __LINUX_USB_ONBOARD_HUB_H
-
-struct usb_device;
-struct list_head;
-
-#if IS_ENABLED(CONFIG_USB_ONBOARD_HUB)
-void onboard_hub_create_pdevs(struct usb_device *parent_hub, struct list_head *pdev_list);
-void onboard_hub_destroy_pdevs(struct list_head *pdev_list);
-#else
-static inline void onboard_hub_create_pdevs(struct usb_device *parent_hub,
- struct list_head *pdev_list) {}
-static inline void onboard_hub_destroy_pdevs(struct list_head *pdev_list) {}
-#endif
-
-#endif /* __LINUX_USB_ONBOARD_HUB_H */
--
2.40.1
The onboard_usb_hub driver has been updated to support non-hub devices,
which has led to some renaming.
Update to the new name (ONBOARD_USB_DEV) accordingly.
Acked-by: Helen Koike <[email protected]>
Signed-off-by: Javier Carrasco <[email protected]>
---
drivers/gpu/drm/ci/arm64.config | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/ci/arm64.config b/drivers/gpu/drm/ci/arm64.config
index 8dbce9919a57..4140303d6260 100644
--- a/drivers/gpu/drm/ci/arm64.config
+++ b/drivers/gpu/drm/ci/arm64.config
@@ -87,7 +87,7 @@ CONFIG_DRM_PARADE_PS8640=y
CONFIG_DRM_LONTIUM_LT9611UXC=y
CONFIG_PHY_QCOM_USB_HS=y
CONFIG_QCOM_GPI_DMA=y
-CONFIG_USB_ONBOARD_HUB=y
+CONFIG_USB_ONBOARD_DEV=y
CONFIG_NVMEM_QCOM_QFPROM=y
CONFIG_PHY_QCOM_USB_SNPS_FEMTO_V2=y
@@ -97,7 +97,7 @@ CONFIG_USB_RTL8152=y
# db820c ethernet
CONFIG_ATL1C=y
# Chromebooks ethernet
-CONFIG_USB_ONBOARD_HUB=y
+CONFIG_USB_ONBOARD_DEV=y
# 888 HDK ethernet
CONFIG_USB_LAN78XX=y
--
2.40.1
The onboard_usb_hub driver has been updated to support non-hub devices,
which has led to some renaming.
Update to the new name (ONBOARD_USB_DEV) accordingly.
Signed-off-by: Javier Carrasco <[email protected]>
---
arch/arm64/configs/defconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index e6cf3e5d63c3..3c6196b6c984 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -1047,7 +1047,7 @@ CONFIG_USB_SERIAL_FTDI_SIO=m
CONFIG_USB_SERIAL_OPTION=m
CONFIG_USB_QCOM_EUD=m
CONFIG_USB_HSIC_USB3503=y
-CONFIG_USB_ONBOARD_HUB=m
+CONFIG_USB_ONBOARD_DEV=m
CONFIG_NOP_USB_XCEIV=y
CONFIG_USB_GADGET=y
CONFIG_USB_RENESAS_USBHS_UDC=m
--
2.40.1
Most of the functionality this driver provides can be used by non-hub
devices as well.
To account for the hub-specific code, add a flag to the device data
structure and check its value for hub-specific code.
The 'always_powered_in_supend' attribute is only available for hub
devices, keeping the driver's default behavior for non-hub devices (keep
on in suspend).
Signed-off-by: Javier Carrasco <[email protected]>
---
drivers/usb/misc/onboard_usb_dev.c | 25 +++++++++++++++++++++++--
drivers/usb/misc/onboard_usb_dev.h | 10 ++++++++++
2 files changed, 33 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c
index e1779bd2d126..df0ed172c7ec 100644
--- a/drivers/usb/misc/onboard_usb_dev.c
+++ b/drivers/usb/misc/onboard_usb_dev.c
@@ -132,7 +132,8 @@ static int __maybe_unused onboard_dev_suspend(struct device *dev)
struct usbdev_node *node;
bool power_off = true;
- if (onboard_dev->always_powered_in_suspend)
+ if (onboard_dev->always_powered_in_suspend &&
+ !onboard_dev->pdata->is_hub)
return 0;
mutex_lock(&onboard_dev->lock);
@@ -262,7 +263,27 @@ static struct attribute *onboard_dev_attrs[] = {
&dev_attr_always_powered_in_suspend.attr,
NULL,
};
-ATTRIBUTE_GROUPS(onboard_dev);
+
+static umode_t onboard_dev_attrs_are_visible(struct kobject *kobj,
+ struct attribute *attr,
+ int n)
+{
+ struct device *dev = kobj_to_dev(kobj);
+ struct onboard_dev *onboard_dev = dev_get_drvdata(dev);
+
+ if (attr == &dev_attr_always_powered_in_suspend.attr &&
+ !onboard_dev->pdata->is_hub)
+ return 0;
+
+ return attr->mode;
+}
+
+static const struct attribute_group onboard_dev_group = {
+ .is_visible = onboard_dev_attrs_are_visible,
+ .attrs = onboard_dev_attrs,
+};
+__ATTRIBUTE_GROUPS(onboard_dev);
+
static void onboard_dev_attach_usb_driver(struct work_struct *work)
{
diff --git a/drivers/usb/misc/onboard_usb_dev.h b/drivers/usb/misc/onboard_usb_dev.h
index 470736483cdf..106480ce72b5 100644
--- a/drivers/usb/misc/onboard_usb_dev.h
+++ b/drivers/usb/misc/onboard_usb_dev.h
@@ -12,60 +12,70 @@ struct onboard_dev_pdata {
unsigned long reset_us; /* reset pulse width in us */
unsigned int num_supplies; /* number of supplies */
const char * const supply_names[MAX_SUPPLIES]; /* use the real names */
+ bool is_hub; /* true if the device is a HUB */
};
static const struct onboard_dev_pdata microchip_usb424_data = {
.reset_us = 1,
.num_supplies = 1,
.supply_names = { "vdd" },
+ .is_hub = true,
};
static const struct onboard_dev_pdata microchip_usb5744_data = {
.reset_us = 0,
.num_supplies = 2,
.supply_names = { "vdd", "vdd2" },
+ .is_hub = true,
};
static const struct onboard_dev_pdata realtek_rts5411_data = {
.reset_us = 0,
.num_supplies = 1,
.supply_names = { "vdd" },
+ .is_hub = true,
};
static const struct onboard_dev_pdata ti_tusb8041_data = {
.reset_us = 3000,
.num_supplies = 1,
.supply_names = { "vdd" },
+ .is_hub = true,
};
static const struct onboard_dev_pdata cypress_hx3_data = {
.reset_us = 10000,
.num_supplies = 2,
.supply_names = { "vdd", "vdd2" },
+ .is_hub = true,
};
static const struct onboard_dev_pdata cypress_hx2vl_data = {
.reset_us = 1,
.num_supplies = 1,
.supply_names = { "vdd" },
+ .is_hub = true,
};
static const struct onboard_dev_pdata genesys_gl850g_data = {
.reset_us = 3,
.num_supplies = 1,
.supply_names = { "vdd" },
+ .is_hub = true,
};
static const struct onboard_dev_pdata genesys_gl852g_data = {
.reset_us = 50,
.num_supplies = 1,
.supply_names = { "vdd" },
+ .is_hub = true,
};
static const struct onboard_dev_pdata vialab_vl817_data = {
.reset_us = 10,
.num_supplies = 1,
.supply_names = { "vdd" },
+ .is_hub = true,
};
static const struct of_device_id onboard_dev_match[] = {
--
2.40.1
The onboard_usb_hub driver has been updated to support non-hub devices,
which has led to some renaming. Update to the new name accordingly.
Update to the new name (ONBOARD_USB_DEV) accordingly.
Signed-off-by: Javier Carrasco <[email protected]>
---
arch/arm/configs/multi_v7_defconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
index ecb3e286107a..6a6ebec173dc 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -876,7 +876,7 @@ CONFIG_USB_CHIPIDEA_UDC=y
CONFIG_USB_CHIPIDEA_HOST=y
CONFIG_USB_ISP1760=y
CONFIG_USB_HSIC_USB3503=y
-CONFIG_USB_ONBOARD_HUB=m
+CONFIG_USB_ONBOARD_DEV=m
CONFIG_AB8500_USB=y
CONFIG_KEYSTONE_USB_PHY=m
CONFIG_NOP_USB_XCEIV=y
--
2.40.1
The XMOS XVF3500 VocalFusion Voice Processor[1] is a low-latency, 32-bit
multicore controller for voice processing.
Add new bindings to define the device properties.
[1] https://www.xmos.com/xvf3500/
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Signed-off-by: Javier Carrasco <[email protected]>
---
.../devicetree/bindings/sound/xmos,xvf3500.yaml | 63 ++++++++++++++++++++++
1 file changed, 63 insertions(+)
diff --git a/Documentation/devicetree/bindings/sound/xmos,xvf3500.yaml b/Documentation/devicetree/bindings/sound/xmos,xvf3500.yaml
new file mode 100644
index 000000000000..fb77a61f1350
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/xmos,xvf3500.yaml
@@ -0,0 +1,63 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/xmos,xvf3500.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: XMOS XVF3500 VocalFusion Voice Processor
+
+maintainers:
+ - Javier Carrasco <[email protected]>
+
+description:
+ The XMOS XVF3500 VocalFusion Voice Processor is a low-latency, 32-bit
+ multicore controller for voice processing.
+ https://www.xmos.com/xvf3500/
+
+allOf:
+ - $ref: /schemas/usb/usb-device.yaml#
+
+properties:
+ compatible:
+ const: usb20b1,0013
+
+ reg: true
+
+ reset-gpios:
+ maxItems: 1
+
+ vdd-supply:
+ description:
+ Regulator for the 1V0 supply.
+
+ vddio-supply:
+ description:
+ Regulator for the 3V3 supply.
+
+required:
+ - compatible
+ - reg
+ - reset-gpios
+ - vdd-supply
+ - vddio-supply
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+
+ usb {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ voice_processor: voice-processor@1 {
+ compatible = "usb20b1,0013";
+ reg = <1>;
+ reset-gpios = <&gpio 5 GPIO_ACTIVE_LOW>;
+ vdd-supply = <&vcc1v0>;
+ vddio-supply = <&vcc3v3>;
+ };
+ };
+
+...
--
2.40.1
The XMOS XVF3500 VocalFusion Voice Processor[1] is a low-latency, 32-bit
multicore controller for voice processing.
This device requires a specific power sequence, which consists of
enabling the regulators that control the 3V3 and 1V0 device supplies,
and a reset de-assertion after a delay of at least 100ns. Such power
sequence is already supported by the onboard_hub driver, and it can be
reused for non-hub USB devices as well.
Once in normal operation, the XVF3500 registers itself as a USB device,
and it does not require any device-specific operations in the driver.
[1] https://www.xmos.com/xvf3500/
Signed-off-by: Javier Carrasco <[email protected]>
---
drivers/usb/misc/onboard_usb_dev.c | 2 ++
drivers/usb/misc/onboard_usb_dev.h | 8 ++++++++
2 files changed, 10 insertions(+)
diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c
index df0ed172c7ec..50f84c5278a2 100644
--- a/drivers/usb/misc/onboard_usb_dev.c
+++ b/drivers/usb/misc/onboard_usb_dev.c
@@ -405,6 +405,7 @@ static struct platform_driver onboard_dev_driver = {
#define VENDOR_ID_REALTEK 0x0bda
#define VENDOR_ID_TI 0x0451
#define VENDOR_ID_VIA 0x2109
+#define VENDOR_ID_XMOS 0x20B1
/*
* Returns the onboard_dev platform device that is associated with the USB
@@ -497,6 +498,7 @@ static const struct usb_device_id onboard_dev_id_table[] = {
{ USB_DEVICE(VENDOR_ID_TI, 0x8142) }, /* TI USB8041 2.0 */
{ USB_DEVICE(VENDOR_ID_VIA, 0x0817) }, /* VIA VL817 3.1 */
{ USB_DEVICE(VENDOR_ID_VIA, 0x2817) }, /* VIA VL817 2.0 */
+ { USB_DEVICE(VENDOR_ID_XMOS, 0x0013) }, /* XVF3500 */
{}
};
MODULE_DEVICE_TABLE(usb, onboard_dev_id_table);
diff --git a/drivers/usb/misc/onboard_usb_dev.h b/drivers/usb/misc/onboard_usb_dev.h
index 106480ce72b5..858f5814165a 100644
--- a/drivers/usb/misc/onboard_usb_dev.h
+++ b/drivers/usb/misc/onboard_usb_dev.h
@@ -78,6 +78,13 @@ static const struct onboard_dev_pdata vialab_vl817_data = {
.is_hub = true,
};
+static const struct onboard_dev_pdata xmos_xvf3500_data = {
+ .reset_us = 1,
+ .num_supplies = 2,
+ .supply_names = { "vdd", "vddio" },
+ .is_hub = false,
+};
+
static const struct of_device_id onboard_dev_match[] = {
{ .compatible = "usb424,2412", .data = µchip_usb424_data, },
{ .compatible = "usb424,2514", .data = µchip_usb424_data, },
@@ -99,6 +106,7 @@ static const struct of_device_id onboard_dev_match[] = {
{ .compatible = "usbbda,5414", .data = &realtek_rts5411_data, },
{ .compatible = "usb2109,817", .data = &vialab_vl817_data, },
{ .compatible = "usb2109,2817", .data = &vialab_vl817_data, },
+ { .compatible = "usb20b1,0013", .data = &xmos_xvf3500_data, },
{}
};
--
2.40.1
On Wed, Feb 28, 2024 at 02:51:34PM +0100, Javier Carrasco wrote:
> The XMOS XVF3500 VocalFusion Voice Processor[1] is a low-latency, 32-bit
> multicore controller for voice processing.
Acked-by: Mark Brown <[email protected]>
Hi Javier,
Thanks for moving this patch to the front of the series!
A few more comments inline.
On Wed, Feb 28, 2024 at 02:51:28PM +0100, Javier Carrasco wrote:
> The current implementation uses generic names for the power supplies,
> which conflicts with proper name definitions in the device bindings.
>
> Add a per-device property to include real supply names and keep generic
> names for existing devices to keep backward compatibility.
>
> Signed-off-by: Javier Carrasco <[email protected]>
> ---
> drivers/usb/misc/onboard_usb_hub.c | 49 ++++++++++++++++++++------------------
> drivers/usb/misc/onboard_usb_hub.h | 12 ++++++++++
> 2 files changed, 38 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c
> index 0dd2b032c90b..3755f6cc1eda 100644
> --- a/drivers/usb/misc/onboard_usb_hub.c
> +++ b/drivers/usb/misc/onboard_usb_hub.c
> @@ -29,17 +29,6 @@
>
> #include "onboard_usb_hub.h"
>
> -/*
> - * Use generic names, as the actual names might differ between hubs. If a new
> - * hub requires more than the currently supported supplies, add a new one here.
> - */
> -static const char * const supply_names[] = {
> - "vdd",
> - "vdd2",
> -};
> -
> -#define MAX_SUPPLIES ARRAY_SIZE(supply_names)
> -
> static void onboard_hub_attach_usb_driver(struct work_struct *work);
>
> static struct usb_device_driver onboard_hub_usbdev_driver;
> @@ -65,6 +54,30 @@ struct onboard_hub {
> struct clk *clk;
> };
>
> +static int onboard_hub_get_regulator_bulk(struct device *dev,
> + struct onboard_hub *onboard_hub)
Let's call this onboard_hub_get_regulators(), it's an implementation detail
that regulator_bulk_get() is used for getting them.
no need to pass 'dev', there is onboard_hub->dev
> static int onboard_hub_power_on(struct onboard_hub *hub)
> {
> int err;
> @@ -253,7 +266,6 @@ static int onboard_hub_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> struct onboard_hub *hub;
> - unsigned int i;
> int err;
>
> hub = devm_kzalloc(dev, sizeof(*hub), GFP_KERNEL);
> @@ -264,18 +276,9 @@ static int onboard_hub_probe(struct platform_device *pdev)
> if (!hub->pdata)
> return -EINVAL;
>
> - if (hub->pdata->num_supplies > MAX_SUPPLIES)
> - return dev_err_probe(dev, -EINVAL, "max %zu supplies supported!\n",
> - MAX_SUPPLIES);
> -
> - for (i = 0; i < hub->pdata->num_supplies; i++)
> - hub->supplies[i].supply = supply_names[i];
> -
> - err = devm_regulator_bulk_get(dev, hub->pdata->num_supplies, hub->supplies);
> - if (err) {
> - dev_err(dev, "Failed to get regulator supplies: %pe\n", ERR_PTR(err));
> + err = onboard_hub_get_regulator_bulk(dev, onboard_hub);
The local variable is called 'hub', not 'onboard_hub'.
> diff --git a/drivers/usb/misc/onboard_usb_hub.h b/drivers/usb/misc/onboard_usb_hub.h
> index f360d5cf8d8a..ea24bd6790f0 100644
> --- a/drivers/usb/misc/onboard_usb_hub.h
> +++ b/drivers/usb/misc/onboard_usb_hub.h
> @@ -6,54 +6,66 @@
> #ifndef _USB_MISC_ONBOARD_USB_HUB_H
> #define _USB_MISC_ONBOARD_USB_HUB_H
>
> +#define MAX_SUPPLIES 2
> +
> struct onboard_hub_pdata {
> unsigned long reset_us; /* reset pulse width in us */
> unsigned int num_supplies; /* number of supplies */
> + const char * const supply_names[MAX_SUPPLIES]; /* use the real names */
The comment isn't particularly useful or accurate. Not in all cases
real names are used and outside of the context of this change the
comment is hard to understand.
I'd say just omit it, the name of the field is self-documenting enough,
there is no need to repeat the same in a comment (as for 'num_supplies'
..)
On 28.02.24 16:37, Matthias Kaehlcke wrote:
> Hi Javier,
>
> Thanks for moving this patch to the front of the series!
>
> A few more comments inline.
>
> On Wed, Feb 28, 2024 at 02:51:28PM +0100, Javier Carrasco wrote:
>> The current implementation uses generic names for the power supplies,
>> which conflicts with proper name definitions in the device bindings.
>>
>> Add a per-device property to include real supply names and keep generic
>> names for existing devices to keep backward compatibility.
>>
>> Signed-off-by: Javier Carrasco <[email protected]>
>> ---
>> drivers/usb/misc/onboard_usb_hub.c | 49 ++++++++++++++++++++------------------
>> drivers/usb/misc/onboard_usb_hub.h | 12 ++++++++++
>> 2 files changed, 38 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c
>> index 0dd2b032c90b..3755f6cc1eda 100644
>> --- a/drivers/usb/misc/onboard_usb_hub.c
>> +++ b/drivers/usb/misc/onboard_usb_hub.c
>> @@ -29,17 +29,6 @@
>>
>> #include "onboard_usb_hub.h"
>>
>> -/*
>> - * Use generic names, as the actual names might differ between hubs. If a new
>> - * hub requires more than the currently supported supplies, add a new one here.
>> - */
>> -static const char * const supply_names[] = {
>> - "vdd",
>> - "vdd2",
>> -};
>> -
>> -#define MAX_SUPPLIES ARRAY_SIZE(supply_names)
>> -
>> static void onboard_hub_attach_usb_driver(struct work_struct *work);
>>
>> static struct usb_device_driver onboard_hub_usbdev_driver;
>> @@ -65,6 +54,30 @@ struct onboard_hub {
>> struct clk *clk;
>> };
>>
>> +static int onboard_hub_get_regulator_bulk(struct device *dev,
>> + struct onboard_hub *onboard_hub)
>
> Let's call this onboard_hub_get_regulators(), it's an implementation detail
> that regulator_bulk_get() is used for getting them.
>
> no need to pass 'dev', there is onboard_hub->dev
>
Not at this point, though. The hub->dev = dev assignment happens a few
lines below, but there is no reason not to move the line up. I will
modify this for v6.
>> static int onboard_hub_power_on(struct onboard_hub *hub)
>> {
>> int err;
>> @@ -253,7 +266,6 @@ static int onboard_hub_probe(struct platform_device *pdev)
>> {
>> struct device *dev = &pdev->dev;
>> struct onboard_hub *hub;
>> - unsigned int i;
>> int err;
>>
>> hub = devm_kzalloc(dev, sizeof(*hub), GFP_KERNEL);
>> @@ -264,18 +276,9 @@ static int onboard_hub_probe(struct platform_device *pdev)
>> if (!hub->pdata)
>> return -EINVAL;
>>
>> - if (hub->pdata->num_supplies > MAX_SUPPLIES)
>> - return dev_err_probe(dev, -EINVAL, "max %zu supplies supported!\n",
>> - MAX_SUPPLIES);
>> -
>> - for (i = 0; i < hub->pdata->num_supplies; i++)
>> - hub->supplies[i].supply = supply_names[i];
>> -
>> - err = devm_regulator_bulk_get(dev, hub->pdata->num_supplies, hub->supplies);
>> - if (err) {
>> - dev_err(dev, "Failed to get regulator supplies: %pe\n", ERR_PTR(err));
>> + err = onboard_hub_get_regulator_bulk(dev, onboard_hub);
>
> The local variable is called 'hub', not 'onboard_hub'.
>
Good catch! Actually this patch alone would have not compiled, but once
the renaming is done, everything is ok again. I will fix this for v6.
>> diff --git a/drivers/usb/misc/onboard_usb_hub.h b/drivers/usb/misc/onboard_usb_hub.h
>> index f360d5cf8d8a..ea24bd6790f0 100644
>> --- a/drivers/usb/misc/onboard_usb_hub.h
>> +++ b/drivers/usb/misc/onboard_usb_hub.h
>> @@ -6,54 +6,66 @@
>> #ifndef _USB_MISC_ONBOARD_USB_HUB_H
>> #define _USB_MISC_ONBOARD_USB_HUB_H
>>
>> +#define MAX_SUPPLIES 2
>> +
>> struct onboard_hub_pdata {
>> unsigned long reset_us; /* reset pulse width in us */
>> unsigned int num_supplies; /* number of supplies */
>> + const char * const supply_names[MAX_SUPPLIES]; /* use the real names */
>
> The comment isn't particularly useful or accurate. Not in all cases
> real names are used and outside of the context of this change the
> comment is hard to understand.
>
> I'd say just omit it, the name of the field is self-documenting enough,
> there is no need to repeat the same in a comment (as for 'num_supplies'
> ...)
I added tthe comment because I can foresee what is going to happen:
people will copy the names from existing devices, we will have to ask if
the supplies are actually called vdd and vdd2 in the datasheet, and then
the real names will be sent in v2. Especially at the beginning, when the
supported devices are using vdd and vdd2.
But if you think the field name is self-documenting, I am fine with it
too. I will remove the comment for v6.
Thanks again and best regards,
Javier Carrasco
On Wed, Feb 28, 2024 at 05:02:10PM +0100, Javier Carrasco wrote:
> On 28.02.24 16:37, Matthias Kaehlcke wrote:
> > Hi Javier,
> >
> > Thanks for moving this patch to the front of the series!
> >
> > A few more comments inline.
> >
> > On Wed, Feb 28, 2024 at 02:51:28PM +0100, Javier Carrasco wrote:
> >> The current implementation uses generic names for the power supplies,
> >> which conflicts with proper name definitions in the device bindings.
> >>
> >> Add a per-device property to include real supply names and keep generic
> >> names for existing devices to keep backward compatibility.
> >>
> >> Signed-off-by: Javier Carrasco <[email protected]>
> >> ---
> >> drivers/usb/misc/onboard_usb_hub.c | 49 ++++++++++++++++++++------------------
> >> drivers/usb/misc/onboard_usb_hub.h | 12 ++++++++++
> >> 2 files changed, 38 insertions(+), 23 deletions(-)
> >>
> >> diff --git a/drivers/usb/misc/onboard_usb_hub.c b/drivers/usb/misc/onboard_usb_hub.c
> >> index 0dd2b032c90b..3755f6cc1eda 100644
> >> --- a/drivers/usb/misc/onboard_usb_hub.c
> >> +++ b/drivers/usb/misc/onboard_usb_hub.c
> >> @@ -29,17 +29,6 @@
> >>
> >> #include "onboard_usb_hub.h"
> >>
> >> -/*
> >> - * Use generic names, as the actual names might differ between hubs. If a new
> >> - * hub requires more than the currently supported supplies, add a new one here.
> >> - */
> >> -static const char * const supply_names[] = {
> >> - "vdd",
> >> - "vdd2",
> >> -};
> >> -
> >> -#define MAX_SUPPLIES ARRAY_SIZE(supply_names)
> >> -
> >> static void onboard_hub_attach_usb_driver(struct work_struct *work);
> >>
> >> static struct usb_device_driver onboard_hub_usbdev_driver;
> >> @@ -65,6 +54,30 @@ struct onboard_hub {
> >> struct clk *clk;
> >> };
> >>
> >> +static int onboard_hub_get_regulator_bulk(struct device *dev,
> >> + struct onboard_hub *onboard_hub)
> >
> > Let's call this onboard_hub_get_regulators(), it's an implementation detail
> > that regulator_bulk_get() is used for getting them.
> >
> > no need to pass 'dev', there is onboard_hub->dev
> >
>
> Not at this point, though. The hub->dev = dev assignment happens a few
> lines below, but there is no reason not to move the line up. I will
> modify this for v6.
>
> >> static int onboard_hub_power_on(struct onboard_hub *hub)
> >> {
> >> int err;
> >> @@ -253,7 +266,6 @@ static int onboard_hub_probe(struct platform_device *pdev)
> >> {
> >> struct device *dev = &pdev->dev;
> >> struct onboard_hub *hub;
> >> - unsigned int i;
> >> int err;
> >>
> >> hub = devm_kzalloc(dev, sizeof(*hub), GFP_KERNEL);
> >> @@ -264,18 +276,9 @@ static int onboard_hub_probe(struct platform_device *pdev)
> >> if (!hub->pdata)
> >> return -EINVAL;
> >>
> >> - if (hub->pdata->num_supplies > MAX_SUPPLIES)
> >> - return dev_err_probe(dev, -EINVAL, "max %zu supplies supported!\n",
> >> - MAX_SUPPLIES);
> >> -
> >> - for (i = 0; i < hub->pdata->num_supplies; i++)
> >> - hub->supplies[i].supply = supply_names[i];
> >> -
> >> - err = devm_regulator_bulk_get(dev, hub->pdata->num_supplies, hub->supplies);
> >> - if (err) {
> >> - dev_err(dev, "Failed to get regulator supplies: %pe\n", ERR_PTR(err));
> >> + err = onboard_hub_get_regulator_bulk(dev, onboard_hub);
> >
> > The local variable is called 'hub', not 'onboard_hub'.
> >
>
> Good catch! Actually this patch alone would have not compiled, but once
> the renaming is done, everything is ok again. I will fix this for v6.
>
> >> diff --git a/drivers/usb/misc/onboard_usb_hub.h b/drivers/usb/misc/onboard_usb_hub.h
> >> index f360d5cf8d8a..ea24bd6790f0 100644
> >> --- a/drivers/usb/misc/onboard_usb_hub.h
> >> +++ b/drivers/usb/misc/onboard_usb_hub.h
> >> @@ -6,54 +6,66 @@
> >> #ifndef _USB_MISC_ONBOARD_USB_HUB_H
> >> #define _USB_MISC_ONBOARD_USB_HUB_H
> >>
> >> +#define MAX_SUPPLIES 2
> >> +
> >> struct onboard_hub_pdata {
> >> unsigned long reset_us; /* reset pulse width in us */
> >> unsigned int num_supplies; /* number of supplies */
> >> + const char * const supply_names[MAX_SUPPLIES]; /* use the real names */
> >
> > The comment isn't particularly useful or accurate. Not in all cases
> > real names are used and outside of the context of this change the
> > comment is hard to understand.
> >
> > I'd say just omit it, the name of the field is self-documenting enough,
> > there is no need to repeat the same in a comment (as for 'num_supplies'
> > ...)
>
> I added tthe comment because I can foresee what is going to happen:
> people will copy the names from existing devices, we will have to ask if
> the supplies are actually called vdd and vdd2 in the datasheet, and then
> the real names will be sent in v2. Especially at the beginning, when the
> supported devices are using vdd and vdd2.
It will probably happen, but I don't expect the comment to prevent that
in most cases. First people need to read the comment and then interpret
it correctly, which isn't a given.
> But if you think the field name is self-documenting, I am fine with it
> too. I will remove the comment for v6.
>
> Thanks again and best regards,
> Javier Carrasco
On Wed, Feb 28, 2024 at 02:51:33PM +0100, Javier Carrasco wrote:
> Most of the functionality this driver provides can be used by non-hub
> devices as well.
>
> To account for the hub-specific code, add a flag to the device data
> structure and check its value for hub-specific code.
>
> The 'always_powered_in_supend' attribute is only available for hub
> devices, keeping the driver's default behavior for non-hub devices (keep
> on in suspend).
>
> Signed-off-by: Javier Carrasco <[email protected]>
> ---
> drivers/usb/misc/onboard_usb_dev.c | 25 +++++++++++++++++++++++--
> drivers/usb/misc/onboard_usb_dev.h | 10 ++++++++++
> 2 files changed, 33 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c
> index e1779bd2d126..df0ed172c7ec 100644
> --- a/drivers/usb/misc/onboard_usb_dev.c
> +++ b/drivers/usb/misc/onboard_usb_dev.c
> @@ -132,7 +132,8 @@ static int __maybe_unused onboard_dev_suspend(struct device *dev)
> struct usbdev_node *node;
> bool power_off = true;
>
> - if (onboard_dev->always_powered_in_suspend)
> + if (onboard_dev->always_powered_in_suspend &&
> + !onboard_dev->pdata->is_hub)
> return 0;
With this non-hub devices would always be powered down, since
'always_powerd_in_suspend' is not set for them. This should be:
if (!onboard_dev->pdata->is_hub ||
onboard_dev->always_powered_in_suspend)
Checking for the (non-)hub status first is clearer IMO, also it avoids
an unneccessary check of 'always_powered' for non-hub devices.
Without code context: for hubs there can be multiple device tree nodes
for the same physical hub chip (e.g. one for the USB2 and another for
the USB3 part). I suppose this could also be the case for non-hub
devices. For hubs there is the 'peer-hub' device tree property to
establish a link between the two USB devices, as a result the onboard
driver only creates a single platform device (which is desired,
otherwise two platform devices would be in charge for power sequencing
the same phyiscal device. For non-hub devices there is currently no such
link. In many cases I expect there will be just one DT entry even though
the device has multiple USB interfaces, but it could happen and would
actually be a more accurate representation.
General support is already there (the code dealing with 'peer-hub'), but
we'd have to come up with a suitable name. 'peer-device' is the first
thing that comes to my mind, but there might be better options. If such
a generic property is added then we should deprecate 'peer-hub', but
maintain backwards compatibility.
On Wed, Feb 28, 2024 at 02:51: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 <[email protected]>
> ---
> ...-usb-hub => sysfs-bus-platform-onboard-usb-dev} | 3 +-
> 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 | 519 +++++++++++++++++++++
> .../misc/{onboard_usb_hub.h => onboard_usb_dev.h} | 28 +-
> ...ard_usb_hub_pdevs.c => onboard_usb_dev_pdevs.c} | 47 +-
> include/linux/usb/onboard_dev.h | 18 +
> include/linux/usb/onboard_hub.h | 18 -
> 12 files changed, 595 insertions(+), 74 deletions(-)
This does not rename/delete onboard_usb_hub.c. With a rename there would
probably be an actual diff for onboard_usb_dev.c instead of a new file,
which would help with reviewing.
On Wed, Feb 28, 2024 at 02:51:35PM +0100, Javier Carrasco wrote:
> The XMOS XVF3500 VocalFusion Voice Processor[1] is a low-latency, 32-bit
> multicore controller for voice processing.
>
> This device requires a specific power sequence, which consists of
> enabling the regulators that control the 3V3 and 1V0 device supplies,
> and a reset de-assertion after a delay of at least 100ns. Such power
> sequence is already supported by the onboard_hub driver, and it can be
> reused for non-hub USB devices as well.
Please update the commit message, the onboard_hub driver no longer
exists as such with the other patches of this series.
> Once in normal operation, the XVF3500 registers itself as a USB device,
> and it does not require any device-specific operations in the driver.
>
> [1] https://www.xmos.com/xvf3500/
>
> Signed-off-by: Javier Carrasco <[email protected]>
Acked-by: Matthias Kaehlcke <[email protected]>
> ---
> drivers/usb/misc/onboard_usb_dev.c | 2 ++
> drivers/usb/misc/onboard_usb_dev.h | 8 ++++++++
> 2 files changed, 10 insertions(+)
>
> diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c
> index df0ed172c7ec..50f84c5278a2 100644
> --- a/drivers/usb/misc/onboard_usb_dev.c
> +++ b/drivers/usb/misc/onboard_usb_dev.c
> @@ -405,6 +405,7 @@ static struct platform_driver onboard_dev_driver = {
> #define VENDOR_ID_REALTEK 0x0bda
> #define VENDOR_ID_TI 0x0451
> #define VENDOR_ID_VIA 0x2109
> +#define VENDOR_ID_XMOS 0x20B1
>
> /*
> * Returns the onboard_dev platform device that is associated with the USB
> @@ -497,6 +498,7 @@ static const struct usb_device_id onboard_dev_id_table[] = {
> { USB_DEVICE(VENDOR_ID_TI, 0x8142) }, /* TI USB8041 2.0 */
> { USB_DEVICE(VENDOR_ID_VIA, 0x0817) }, /* VIA VL817 3.1 */
> { USB_DEVICE(VENDOR_ID_VIA, 0x2817) }, /* VIA VL817 2.0 */
> + { USB_DEVICE(VENDOR_ID_XMOS, 0x0013) }, /* XVF3500 */
nit: be a bit more specific? e.g. XVF3500 Voice Processor, or XMOS XVF3500
The other entries were implicitly hubs since this was the 'onboard_hub'
driver. It wouldn't be a bad idea to add 'hub' to these entries in the
patch that 'renames' the driver.
On 28.02.24 19:10, Matthias Kaehlcke wrote:
> On Wed, Feb 28, 2024 at 02:51:33PM +0100, Javier Carrasco wrote:
>> Most of the functionality this driver provides can be used by non-hub
>> devices as well.
>>
>> To account for the hub-specific code, add a flag to the device data
>> structure and check its value for hub-specific code.
>>
>> The 'always_powered_in_supend' attribute is only available for hub
>> devices, keeping the driver's default behavior for non-hub devices (keep
>> on in suspend).
>>
>> Signed-off-by: Javier Carrasco <[email protected]>
>> ---
>> drivers/usb/misc/onboard_usb_dev.c | 25 +++++++++++++++++++++++--
>> drivers/usb/misc/onboard_usb_dev.h | 10 ++++++++++
>> 2 files changed, 33 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c
>> index e1779bd2d126..df0ed172c7ec 100644
>> --- a/drivers/usb/misc/onboard_usb_dev.c
>> +++ b/drivers/usb/misc/onboard_usb_dev.c
>> @@ -132,7 +132,8 @@ static int __maybe_unused onboard_dev_suspend(struct device *dev)
>> struct usbdev_node *node;
>> bool power_off = true;
>>
>> - if (onboard_dev->always_powered_in_suspend)
>> + if (onboard_dev->always_powered_in_suspend &&
>> + !onboard_dev->pdata->is_hub)
>> return 0;
>
> With this non-hub devices would always be powered down, since
> 'always_powerd_in_suspend' is not set for them. This should be:
>
May I ask you what you meant in v4 with this comment?
> Even without the sysfs attribute the field 'always_powered_in_suspend'
> could
> be set to true by probe() for non-hub devices.
> if (!onboard_dev->pdata->is_hub ||
> onboard_dev->always_powered_in_suspend)
>
> Checking for the (non-)hub status first is clearer IMO, also it avoids
> an unneccessary check of 'always_powered' for non-hub devices.
>
That makes sense and will be fixed.
> Without code context: for hubs there can be multiple device tree nodes
> for the same physical hub chip (e.g. one for the USB2 and another for
> the USB3 part). I suppose this could also be the case for non-hub
> devices. For hubs there is the 'peer-hub' device tree property to
> establish a link between the two USB devices, as a result the onboard
> driver only creates a single platform device (which is desired,
> otherwise two platform devices would be in charge for power sequencing
> the same phyiscal device. For non-hub devices there is currently no such
> link. In many cases I expect there will be just one DT entry even though
> the device has multiple USB interfaces, but it could happen and would
> actually be a more accurate representation.
>
> General support is already there (the code dealing with 'peer-hub'), but
> we'd have to come up with a suitable name. 'peer-device' is the first
> thing that comes to my mind, but there might be better options. If such
> a generic property is added then we should deprecate 'peer-hub', but
> maintain backwards compatibility.
I have nothing against that, but the first non-hub device that will be
added does not have multiple DT nodes, so I have nothing to test that
extension with real hardware.
That could be added in the future, though, if the need ever arises.
Best regards,
Javier Carrasco
On 28.02.24 19:18, Matthias Kaehlcke wrote:
> On Wed, Feb 28, 2024 at 02:51: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 <[email protected]>
>> ---
>> ...-usb-hub => sysfs-bus-platform-onboard-usb-dev} | 3 +-
>> 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 | 519 +++++++++++++++++++++
>> .../misc/{onboard_usb_hub.h => onboard_usb_dev.h} | 28 +-
>> ...ard_usb_hub_pdevs.c => onboard_usb_dev_pdevs.c} | 47 +-
>> include/linux/usb/onboard_dev.h | 18 +
>> include/linux/usb/onboard_hub.h | 18 -
>> 12 files changed, 595 insertions(+), 74 deletions(-)
>
> This does not rename/delete onboard_usb_hub.c. With a rename there would
> probably be an actual diff for onboard_usb_dev.c instead of a new file,
> which would help with reviewing.
Thanks, I noticed that when I started working on v6 and it has been
fixed. I must have lost that one when fixing conflicts during the patch
re-ordering.
Best regards,
Javier Carrasco
On Wed, Feb 28, 2024 at 09:21:00PM +0100, Javier Carrasco wrote:
> On 28.02.24 19:10, Matthias Kaehlcke wrote:
> > On Wed, Feb 28, 2024 at 02:51:33PM +0100, Javier Carrasco wrote:
> >> Most of the functionality this driver provides can be used by non-hub
> >> devices as well.
> >>
> >> To account for the hub-specific code, add a flag to the device data
> >> structure and check its value for hub-specific code.
> >>
> >> The 'always_powered_in_supend' attribute is only available for hub
> >> devices, keeping the driver's default behavior for non-hub devices (keep
> >> on in suspend).
> >>
> >> Signed-off-by: Javier Carrasco <[email protected]>
> >> ---
> >> drivers/usb/misc/onboard_usb_dev.c | 25 +++++++++++++++++++++++--
> >> drivers/usb/misc/onboard_usb_dev.h | 10 ++++++++++
> >> 2 files changed, 33 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c
> >> index e1779bd2d126..df0ed172c7ec 100644
> >> --- a/drivers/usb/misc/onboard_usb_dev.c
> >> +++ b/drivers/usb/misc/onboard_usb_dev.c
> >> @@ -132,7 +132,8 @@ static int __maybe_unused onboard_dev_suspend(struct device *dev)
> >> struct usbdev_node *node;
> >> bool power_off = true;
> >>
> >> - if (onboard_dev->always_powered_in_suspend)
> >> + if (onboard_dev->always_powered_in_suspend &&
> >> + !onboard_dev->pdata->is_hub)
> >> return 0;
> >
> > With this non-hub devices would always be powered down, since
> > 'always_powerd_in_suspend' is not set for them. This should be:
> >
>
> May I ask you what you meant in v4 with this comment?
>
> > Even without the sysfs attribute the field 'always_powered_in_suspend'
> > could
> > be set to true by probe() for non-hub devices.
struct onboard_dev always has the field 'always_powered_in_suspend',
even for non-hubs, that don't have the corresponding sysfs attribute.
Currently it is left uninitialized (i.e. false) for non-hubs. Instead
it could be initialized to true by probe() for non-hubs, which would
be semantically correct. With that it wouldn't be necessary to check
here whether a device is hub, because the field would provide the
necessary information.
> > if (!onboard_dev->pdata->is_hub ||
> > onboard_dev->always_powered_in_suspend)
> >
> > Checking for the (non-)hub status first is clearer IMO, also it avoids
> > an unneccessary check of 'always_powered' for non-hub devices.
> >
>
> That makes sense and will be fixed.
>
> > Without code context: for hubs there can be multiple device tree nodes
> > for the same physical hub chip (e.g. one for the USB2 and another for
> > the USB3 part). I suppose this could also be the case for non-hub
> > devices. For hubs there is the 'peer-hub' device tree property to
> > establish a link between the two USB devices, as a result the onboard
> > driver only creates a single platform device (which is desired,
> > otherwise two platform devices would be in charge for power sequencing
> > the same phyiscal device. For non-hub devices there is currently no such
> > link. In many cases I expect there will be just one DT entry even though
> > the device has multiple USB interfaces, but it could happen and would
> > actually be a more accurate representation.
> >
> > General support is already there (the code dealing with 'peer-hub'), but
> > we'd have to come up with a suitable name. 'peer-device' is the first
> > thing that comes to my mind, but there might be better options. If such
> > a generic property is added then we should deprecate 'peer-hub', but
> > maintain backwards compatibility.
>
> I have nothing against that, but the first non-hub device that will be
> added does not have multiple DT nodes, so I have nothing to test that
> extension with real hardware.
I see, the XVF3500 is USB 2.0 only, so it isn't suitable for testing.
> That could be added in the future, though, if the need ever arises.
I expect it will, when a DT maintainer asks the hardware to be
represented correctly for a device that is connected to more than one USB
bus. IIRC that's how 'peer-hub' was born :)
Ok, we can leave it out for now. I might send a dedicated patch after your
series landed. If a switch to 'peer-device' or similar is anticipated then
it's probably best to deprecate 'peer-hub' ASAP, to avoid it from getting
added to more bindings.
On 28.02.24 21:41, Matthias Kaehlcke wrote:
> On Wed, Feb 28, 2024 at 09:21:00PM +0100, Javier Carrasco wrote:
>> On 28.02.24 19:10, Matthias Kaehlcke wrote:
>>> On Wed, Feb 28, 2024 at 02:51:33PM +0100, Javier Carrasco wrote:
>>>> Most of the functionality this driver provides can be used by non-hub
>>>> devices as well.
>>>>
>>>> To account for the hub-specific code, add a flag to the device data
>>>> structure and check its value for hub-specific code.
>>>>
>>>> The 'always_powered_in_supend' attribute is only available for hub
>>>> devices, keeping the driver's default behavior for non-hub devices (keep
>>>> on in suspend).
>>>>
>>>> Signed-off-by: Javier Carrasco <[email protected]>
>>>> ---
>>>> drivers/usb/misc/onboard_usb_dev.c | 25 +++++++++++++++++++++++--
>>>> drivers/usb/misc/onboard_usb_dev.h | 10 ++++++++++
>>>> 2 files changed, 33 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c
>>>> index e1779bd2d126..df0ed172c7ec 100644
>>>> --- a/drivers/usb/misc/onboard_usb_dev.c
>>>> +++ b/drivers/usb/misc/onboard_usb_dev.c
>>>> @@ -132,7 +132,8 @@ static int __maybe_unused onboard_dev_suspend(struct device *dev)
>>>> struct usbdev_node *node;
>>>> bool power_off = true;
>>>>
>>>> - if (onboard_dev->always_powered_in_suspend)
>>>> + if (onboard_dev->always_powered_in_suspend &&
>>>> + !onboard_dev->pdata->is_hub)
>>>> return 0;
>>>
>>> With this non-hub devices would always be powered down, since
>>> 'always_powerd_in_suspend' is not set for them. This should be:
>>>
>>
>> May I ask you what you meant in v4 with this comment?
>>
>>> Even without the sysfs attribute the field 'always_powered_in_suspend'
>>> could
>>> be set to true by probe() for non-hub devices.
>
> struct onboard_dev always has the field 'always_powered_in_suspend',
> even for non-hubs, that don't have the corresponding sysfs attribute.
> Currently it is left uninitialized (i.e. false) for non-hubs. Instead
> it could be initialized to true by probe() for non-hubs, which would
> be semantically correct. With that it wouldn't be necessary to check
> here whether a device is hub, because the field would provide the
> necessary information.
>
That is maybe what is confusing me a bit. Should it not be false for
non-hub devices? That property is only meant for hubs, so why should
non-hub devices be always powered in suspend? I thought it should always
be false for non-hub devices, and configurable for hubs.
>>> if (!onboard_dev->pdata->is_hub ||
>>> onboard_dev->always_powered_in_suspend)
>>>
>>> Checking for the (non-)hub status first is clearer IMO, also it avoids
>>> an unneccessary check of 'always_powered' for non-hub devices.
>>>
>>
>> That makes sense and will be fixed.
>>
>>> Without code context: for hubs there can be multiple device tree nodes
>>> for the same physical hub chip (e.g. one for the USB2 and another for
>>> the USB3 part). I suppose this could also be the case for non-hub
>>> devices. For hubs there is the 'peer-hub' device tree property to
>>> establish a link between the two USB devices, as a result the onboard
>>> driver only creates a single platform device (which is desired,
>>> otherwise two platform devices would be in charge for power sequencing
>>> the same phyiscal device. For non-hub devices there is currently no such
>>> link. In many cases I expect there will be just one DT entry even though
>>> the device has multiple USB interfaces, but it could happen and would
>>> actually be a more accurate representation.
>>>
>>> General support is already there (the code dealing with 'peer-hub'), but
>>> we'd have to come up with a suitable name. 'peer-device' is the first
>>> thing that comes to my mind, but there might be better options. If such
>>> a generic property is added then we should deprecate 'peer-hub', but
>>> maintain backwards compatibility.
>>
>> I have nothing against that, but the first non-hub device that will be
>> added does not have multiple DT nodes, so I have nothing to test that
>> extension with real hardware.
>
> I see, the XVF3500 is USB 2.0 only, so it isn't suitable for testing.
>
>> That could be added in the future, though, if the need ever arises.
>
> I expect it will, when a DT maintainer asks the hardware to be
> represented correctly for a device that is connected to more than one USB
> bus. IIRC that's how 'peer-hub' was born :)
>
> Ok, we can leave it out for now. I might send a dedicated patch after your
> series landed. If a switch to 'peer-device' or similar is anticipated then
> it's probably best to deprecate 'peer-hub' ASAP, to avoid it from getting
> added to more bindings.
Best regards,
Javier Carrasco
On Wed, Feb 28, 2024 at 09:50:22PM +0100, Javier Carrasco wrote:
> On 28.02.24 21:41, Matthias Kaehlcke wrote:
> > On Wed, Feb 28, 2024 at 09:21:00PM +0100, Javier Carrasco wrote:
> >> On 28.02.24 19:10, Matthias Kaehlcke wrote:
> >>> On Wed, Feb 28, 2024 at 02:51:33PM +0100, Javier Carrasco wrote:
> >>>> Most of the functionality this driver provides can be used by non-hub
> >>>> devices as well.
> >>>>
> >>>> To account for the hub-specific code, add a flag to the device data
> >>>> structure and check its value for hub-specific code.
> >>>>
> >>>> The 'always_powered_in_supend' attribute is only available for hub
> >>>> devices, keeping the driver's default behavior for non-hub devices (keep
> >>>> on in suspend).
> >>>>
> >>>> Signed-off-by: Javier Carrasco <[email protected]>
> >>>> ---
> >>>> drivers/usb/misc/onboard_usb_dev.c | 25 +++++++++++++++++++++++--
> >>>> drivers/usb/misc/onboard_usb_dev.h | 10 ++++++++++
> >>>> 2 files changed, 33 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c
> >>>> index e1779bd2d126..df0ed172c7ec 100644
> >>>> --- a/drivers/usb/misc/onboard_usb_dev.c
> >>>> +++ b/drivers/usb/misc/onboard_usb_dev.c
> >>>> @@ -132,7 +132,8 @@ static int __maybe_unused onboard_dev_suspend(struct device *dev)
> >>>> struct usbdev_node *node;
> >>>> bool power_off = true;
> >>>>
> >>>> - if (onboard_dev->always_powered_in_suspend)
> >>>> + if (onboard_dev->always_powered_in_suspend &&
> >>>> + !onboard_dev->pdata->is_hub)
> >>>> return 0;
> >>>
> >>> With this non-hub devices would always be powered down, since
> >>> 'always_powerd_in_suspend' is not set for them. This should be:
> >>>
> >>
> >> May I ask you what you meant in v4 with this comment?
> >>
> >>> Even without the sysfs attribute the field 'always_powered_in_suspend'
> >>> could
> >>> be set to true by probe() for non-hub devices.
> >
> > struct onboard_dev always has the field 'always_powered_in_suspend',
> > even for non-hubs, that don't have the corresponding sysfs attribute.
> > Currently it is left uninitialized (i.e. false) for non-hubs. Instead
> > it could be initialized to true by probe() for non-hubs, which would
> > be semantically correct. With that it wouldn't be necessary to check
> > here whether a device is hub, because the field would provide the
> > necessary information.
> >
>
> That is maybe what is confusing me a bit. Should it not be false for
> non-hub devices? That property is only meant for hubs, so why should
> non-hub devices be always powered in suspend? I thought it should always
> be false for non-hub devices, and configurable for hubs.
I suspect the confusion stems from the sysfs attribute 'always_powered_...'
vs. the struct field with the same name.
The sysfs attribute defaults to 'false', which results in USB devices
being powered down in suspend. That was the desired behavior for a device
I was working on when I implemented this driver, but in hindsight I think
the default should have been 'true'.
We agreed that non-hub devices shouldn't be powered down in suspend. It
would be unexpected for users and could have side effects like delays
or losing status. Since (I think) we can't change the default of the
attribute we said we'd limit it to hubs, and not create it for other
types of USB devices. Other USB devices would remain powered during
system suspend.
Are we in agreement up to this point, in particular that non-hub
devices should remain powered?
struct onboard_dev has the field 'always_powered_...', which in the
existing code is *always* associated with the sysfs attribute of
the same name. But there is no reason to not to use the field when
the sysfs attribute does not exist. For any device at any given time
the field could indicate whether the device should be remain powered
during suspend. For hubs the value can be changed at runtime
through the sysfs attribute, for non-hubs it would be static and
always indicate that the device should remain powered.
Does that clarify your doubts?
On 28.02.24 22:34, Matthias Kaehlcke wrote:
> On Wed, Feb 28, 2024 at 09:50:22PM +0100, Javier Carrasco wrote:
>> On 28.02.24 21:41, Matthias Kaehlcke wrote:
>>> On Wed, Feb 28, 2024 at 09:21:00PM +0100, Javier Carrasco wrote:
>>>> On 28.02.24 19:10, Matthias Kaehlcke wrote:
>>>>> On Wed, Feb 28, 2024 at 02:51:33PM +0100, Javier Carrasco wrote:
>>>>>> Most of the functionality this driver provides can be used by non-hub
>>>>>> devices as well.
>>>>>>
>>>>>> To account for the hub-specific code, add a flag to the device data
>>>>>> structure and check its value for hub-specific code.
>>>>>>
>>>>>> The 'always_powered_in_supend' attribute is only available for hub
>>>>>> devices, keeping the driver's default behavior for non-hub devices (keep
>>>>>> on in suspend).
>>>>>>
>>>>>> Signed-off-by: Javier Carrasco <[email protected]>
>>>>>> ---
>>>>>> drivers/usb/misc/onboard_usb_dev.c | 25 +++++++++++++++++++++++--
>>>>>> drivers/usb/misc/onboard_usb_dev.h | 10 ++++++++++
>>>>>> 2 files changed, 33 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c
>>>>>> index e1779bd2d126..df0ed172c7ec 100644
>>>>>> --- a/drivers/usb/misc/onboard_usb_dev.c
>>>>>> +++ b/drivers/usb/misc/onboard_usb_dev.c
>>>>>> @@ -132,7 +132,8 @@ static int __maybe_unused onboard_dev_suspend(struct device *dev)
>>>>>> struct usbdev_node *node;
>>>>>> bool power_off = true;
>>>>>>
>>>>>> - if (onboard_dev->always_powered_in_suspend)
>>>>>> + if (onboard_dev->always_powered_in_suspend &&
>>>>>> + !onboard_dev->pdata->is_hub)
>>>>>> return 0;
>>>>>
>>>>> With this non-hub devices would always be powered down, since
>>>>> 'always_powerd_in_suspend' is not set for them. This should be:
>>>>>
>>>>
>>>> May I ask you what you meant in v4 with this comment?
>>>>
>>>>> Even without the sysfs attribute the field 'always_powered_in_suspend'
>>>>> could
>>>>> be set to true by probe() for non-hub devices.
>>>
>>> struct onboard_dev always has the field 'always_powered_in_suspend',
>>> even for non-hubs, that don't have the corresponding sysfs attribute.
>>> Currently it is left uninitialized (i.e. false) for non-hubs. Instead
>>> it could be initialized to true by probe() for non-hubs, which would
>>> be semantically correct. With that it wouldn't be necessary to check
>>> here whether a device is hub, because the field would provide the
>>> necessary information.
>>>
>>
>> That is maybe what is confusing me a bit. Should it not be false for
>> non-hub devices? That property is only meant for hubs, so why should
>> non-hub devices be always powered in suspend? I thought it should always
>> be false for non-hub devices, and configurable for hubs.
>
> I suspect the confusion stems from the sysfs attribute 'always_powered_...'
> vs. the struct field with the same name.
>
> The sysfs attribute defaults to 'false', which results in USB devices
> being powered down in suspend. That was the desired behavior for a device
> I was working on when I implemented this driver, but in hindsight I think
> the default should have been 'true'.
>
> We agreed that non-hub devices shouldn't be powered down in suspend. It
> would be unexpected for users and could have side effects like delays
> or losing status. Since (I think) we can't change the default of the
> attribute we said we'd limit it to hubs, and not create it for other
> types of USB devices. Other USB devices would remain powered during
> system suspend.
>
> Are we in agreement up to this point, in particular that non-hub
> devices should remain powered?
>
> struct onboard_dev has the field 'always_powered_...', which in the
> existing code is *always* associated with the sysfs attribute of
> the same name. But there is no reason to not to use the field when
> the sysfs attribute does not exist. For any device at any given time
> the field could indicate whether the device should be remain powered
> during suspend. For hubs the value can be changed at runtime
> through the sysfs attribute, for non-hubs it would be static and
> always indicate that the device should remain powered.
>
> Does that clarify your doubts?
It is crystal clear now, thank you.
Best regards,
Javier Carrasco