| Message ID | 20240220-onboard_xvf3500-v4-0-dc1617cc5dd4@wolfvision.net |
|---|---|
| Headers | show |
| Series | usb: misc: onboard_hub: add support for XMOS XVF3500 | expand |
On 20/02/2024 11:05, Javier Carrasco wrote: > 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 <javier.carrasco@wolfvision.net> Acked-by: Helen Koike <helen.koike@collabora.com> Regards, Helen > --- > 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 > >
Hi, Am Dienstag, 20. Februar 2024, 15:05:44 CET schrieb Javier Carrasco: > 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. While reading this, [1] come into my mind. Best regards, Alexander [1] https://lore.kernel.org/all/20211006035407.1147909-1-dmitry.baryshkov@linaro.org/ > 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 been added, keeping generic names > for backwards compatibility and as a fallback mechanism. > > 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. > > The device binding has been added to sound/ because it is the subsystem > that covers its functionality (voice processing) during normal > operation. If it should reside under usb/ instead, it will be moved as > required. > > 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 <lgirdwood@gmail.com> > To: Mark Brown <broonie@kernel.org> > To: Rob Herring <robh+dt@kernel.org> > To: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org> > To: Conor Dooley <conor+dt@kernel.org> > To: Matthias Kaehlcke <mka@chromium.org> > To: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > To: Helen Koike <helen.koike@collabora.com> > To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > To: Maxime Ripard <mripard@kernel.org> > To: Thomas Zimmermann <tzimmermann@suse.de> > To: David Airlie <airlied@gmail.com> > To: Daniel Vetter <daniel@ffwll.ch> > To: Catalin Marinas <catalin.marinas@arm.com> > To: Will Deacon <will@kernel.org> > To: Russell King <linux@armlinux.org.uk> > Cc: linux-sound@vger.kernel.org > Cc: devicetree@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: linux-usb@vger.kernel.org > Cc: dri-devel@lists.freedesktop.org > Cc: linux-arm-kernel@lists.infradead.org > Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net> > > 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/20240206-onboard_xvf3500-v3-0-f85b04116688@wolfvision.net > > 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/20240130-onboard_xvf3500-v1-0-51b5398406cb@wolfvision.net > > 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/20240115-feature-xvf3500_driver-v1-0-ed9cfb48bb85@wolfvision.net/ > > --- > Javier Carrasco (8): > usb: misc: onboard_hub: rename to onboard_dev > usb: misc: onboard_dev: add support for non-hub devices > 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 name > usb: misc: onboard_dev: use device supply names > 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} | 4 +- > .../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 | 525 +++++++++++++++++++++ > .../misc/{onboard_usb_hub.h => onboard_usb_dev.h} | 69 ++- > ...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 - > 17 files changed, 709 insertions(+), 580 deletions(-) > --- > base-commit: 41bccc98fb7931d63d03f326a746ac4d429c1dd3 > change-id: 20240130-onboard_xvf3500-6c0e78d11a1b > > Best regards, >
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 ..."
On Tue, Feb 20, 2024 at 03:05:46PM +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. Please mention that the driver doesn't power off non-hub devices during system suspend. > Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net> > --- > drivers/usb/misc/onboard_usb_dev.c | 3 ++- > drivers/usb/misc/onboard_usb_dev.h | 10 ++++++++++ > 2 files changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c > index 2103af2cb2a6..f43130a6786f 100644 > --- a/drivers/usb/misc/onboard_usb_dev.c > +++ b/drivers/usb/misc/onboard_usb_dev.c > @@ -129,7 +129,8 @@ static int __maybe_unused onboard_dev_suspend(struct device *dev) > if (!device_may_wakeup(node->udev->bus->controller)) > continue; > > - if (usb_wakeup_enabled_descendants(node->udev)) { > + if (usb_wakeup_enabled_descendants(node->udev) || > + !onboard_dev->pdata->is_hub) { This check isn't dependent on characteristics of the USB devices processed in this loop, therefore it can be performed at function entry. Please combine it with the check of 'always_powered_in_suspend'. It's also an option to omit the check completely, 'always_powered_in_suspend' will never be set for non-hub devices (assuming the sysfs attribute isn't added). > power_off = false; > break; > } Without code context: please omit the creation of the 'always_powered_in_suspend' attribute for non-hub devices. As per above we don't plan to hone it, so it shouldn't exist.
On Tue, Feb 20, 2024 at 03:05:50PM +0100, Javier Carrasco wrote: > The current mechanism 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 as a fallback mechanism for backward compatibility. > > Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net> > --- > drivers/usb/misc/onboard_usb_dev.c | 54 ++++++++++++++++++++------------------ > drivers/usb/misc/onboard_usb_dev.h | 23 ++++++++++++++++ > 2 files changed, 52 insertions(+), 25 deletions(-) > > diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c > index f43130a6786f..e66fcac93006 100644 > --- a/drivers/usb/misc/onboard_usb_dev.c > +++ b/drivers/usb/misc/onboard_usb_dev.c > @@ -29,18 +29,6 @@ > > #include "onboard_usb_dev.h" > > -/* > - * Use generic names, as the actual names might differ between devices. If a new > - * device 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_dev_attach_usb_driver(struct work_struct *work); > > static struct usb_device_driver onboard_dev_usbdev_driver; > @@ -66,6 +54,33 @@ struct onboard_dev { > 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 %zu supplies supported!\n", > + MAX_SUPPLIES); > + > + if (!supply_names[0]) > + supply_names = generic_supply_names; Please change to 'if (!supply_names)' and omit the initialization of .supply_names for devices that use the generic supply names. > diff --git a/drivers/usb/misc/onboard_usb_dev.h b/drivers/usb/misc/onboard_usb_dev.h > index ebe83e19d818..59dced6bd339 100644 > --- a/drivers/usb/misc/onboard_usb_dev.h > +++ b/drivers/usb/misc/onboard_usb_dev.h > @@ -6,63 +6,86 @@ > #ifndef _USB_MISC_ONBOARD_USB_DEV_H > #define _USB_MISC_ONBOARD_USB_DEV_H > > +/* > + * Fallback supply names for backwards compatibility. If the device requires > + * more than the currently supported supplies, add a new one here, and if > + * possible, the real name supplies to the device-specific data. > + */ > +static const char * const generic_supply_names[] = { > + "vdd", > + "vdd2", > +}; > + > +#define MAX_SUPPLIES ARRAY_SIZE(generic_supply_names) This will have to change when support for a device with more than 2 non-generic supply names gets added. Please use a literal value for MAX_SUPPLIES instead of ARRAY_SIZE. If the literal is 2 it would still need to change for future devices with more supplies, but that change would be more straighforward. > + > struct onboard_dev_pdata { > unsigned long reset_us; /* reset pulse width in us */ > unsigned int num_supplies; /* number of supplies */ > bool is_hub; > + const char * const supply_names[MAX_SUPPLIES]; > + > }; > > static const struct onboard_dev_pdata microchip_usb424_data = { > .reset_us = 1, > .num_supplies = 1, > + .supply_names = { NULL }, > .is_hub = true, > }; > > ...
On 21.02.24 21:25, Matthias Kaehlcke wrote: > On Tue, Feb 20, 2024 at 03:05:50PM +0100, Javier Carrasco wrote: >> The current mechanism 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 as a fallback mechanism for backward compatibility. >> >> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net> >> --- >> drivers/usb/misc/onboard_usb_dev.c | 54 ++++++++++++++++++++------------------ >> drivers/usb/misc/onboard_usb_dev.h | 23 ++++++++++++++++ >> 2 files changed, 52 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c >> index f43130a6786f..e66fcac93006 100644 >> --- a/drivers/usb/misc/onboard_usb_dev.c >> +++ b/drivers/usb/misc/onboard_usb_dev.c >> @@ -29,18 +29,6 @@ >> >> #include "onboard_usb_dev.h" >> >> -/* >> - * Use generic names, as the actual names might differ between devices. If a new >> - * device 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_dev_attach_usb_driver(struct work_struct *work); >> >> static struct usb_device_driver onboard_dev_usbdev_driver; >> @@ -66,6 +54,33 @@ struct onboard_dev { >> 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 %zu supplies supported!\n", >> + MAX_SUPPLIES); >> + >> + if (!supply_names[0]) >> + supply_names = generic_supply_names; > > Please change to 'if (!supply_names)' and omit the initialization of > .supply_names for devices that use the generic supply names. > That looks much cleaner, I will apply it to the next version. >> diff --git a/drivers/usb/misc/onboard_usb_dev.h b/drivers/usb/misc/onboard_usb_dev.h >> index ebe83e19d818..59dced6bd339 100644 >> --- a/drivers/usb/misc/onboard_usb_dev.h >> +++ b/drivers/usb/misc/onboard_usb_dev.h >> @@ -6,63 +6,86 @@ >> #ifndef _USB_MISC_ONBOARD_USB_DEV_H >> #define _USB_MISC_ONBOARD_USB_DEV_H >> >> +/* >> + * Fallback supply names for backwards compatibility. If the device requires >> + * more than the currently supported supplies, add a new one here, and if >> + * possible, the real name supplies to the device-specific data. >> + */ >> +static const char * const generic_supply_names[] = { >> + "vdd", >> + "vdd2", >> +}; >> + >> +#define MAX_SUPPLIES ARRAY_SIZE(generic_supply_names) > > This will have to change when support for a device with more than 2 non-generic > supply names gets added. Please use a literal value for MAX_SUPPLIES instead of > ARRAY_SIZE. If the literal is 2 it would still need to change for future devices > with more supplies, but that change would be more straighforward. > I am not completely sure about this. Someone could increase MAX_SUPPLIES without adding a generic name. Actually two modifications will be necessary for every addition (name and MAX_SUPPLIES). If ARRAY_SIZE is used, only new names are required, and MAX_SUPPLIES is automatically increased. I understand that the whole point of this is getting rid of the generic names, but we still have to provide generic names for every extra supply, at least for code consistency and to avoid size mismatches between real an generic supply names. >> + >> struct onboard_dev_pdata { >> unsigned long reset_us; /* reset pulse width in us */ >> unsigned int num_supplies; /* number of supplies */ >> bool is_hub; >> + const char * const supply_names[MAX_SUPPLIES]; >> + >> }; >> >> static const struct onboard_dev_pdata microchip_usb424_data = { >> .reset_us = 1, >> .num_supplies = 1, >> + .supply_names = { NULL }, >> .is_hub = true, >> }; >> >> ... Thanks again for your feedback. Best regards, Javier Carrasco
Hi Javier, On Wed, Feb 21, 2024 at 09:40:38PM +0100, Javier Carrasco wrote: > On 21.02.24 21:25, Matthias Kaehlcke wrote: > > On Tue, Feb 20, 2024 at 03:05:50PM +0100, Javier Carrasco wrote: > >> The current mechanism 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 as a fallback mechanism for backward compatibility. > >> > >> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net> > >> --- > >> drivers/usb/misc/onboard_usb_dev.c | 54 ++++++++++++++++++++------------------ > >> drivers/usb/misc/onboard_usb_dev.h | 23 ++++++++++++++++ > >> 2 files changed, 52 insertions(+), 25 deletions(-) > >> > >> diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c > >> index f43130a6786f..e66fcac93006 100644 > >> --- a/drivers/usb/misc/onboard_usb_dev.c > >> +++ b/drivers/usb/misc/onboard_usb_dev.c > >> @@ -29,18 +29,6 @@ > >> > >> #include "onboard_usb_dev.h" > >> > >> -/* > >> - * Use generic names, as the actual names might differ between devices. If a new > >> - * device 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_dev_attach_usb_driver(struct work_struct *work); > >> > >> static struct usb_device_driver onboard_dev_usbdev_driver; > >> @@ -66,6 +54,33 @@ struct onboard_dev { > >> 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 %zu supplies supported!\n", > >> + MAX_SUPPLIES); > >> + > >> + if (!supply_names[0]) > >> + supply_names = generic_supply_names; > > > > Please change to 'if (!supply_names)' and omit the initialization of > > .supply_names for devices that use the generic supply names. > > > > That looks much cleaner, I will apply it to the next version. > > >> diff --git a/drivers/usb/misc/onboard_usb_dev.h b/drivers/usb/misc/onboard_usb_dev.h > >> index ebe83e19d818..59dced6bd339 100644 > >> --- a/drivers/usb/misc/onboard_usb_dev.h > >> +++ b/drivers/usb/misc/onboard_usb_dev.h > >> @@ -6,63 +6,86 @@ > >> #ifndef _USB_MISC_ONBOARD_USB_DEV_H > >> #define _USB_MISC_ONBOARD_USB_DEV_H > >> > >> +/* > >> + * Fallback supply names for backwards compatibility. If the device requires > >> + * more than the currently supported supplies, add a new one here, and if > >> + * possible, the real name supplies to the device-specific data. > >> + */ > >> +static const char * const generic_supply_names[] = { > >> + "vdd", > >> + "vdd2", > >> +}; > >> + > >> +#define MAX_SUPPLIES ARRAY_SIZE(generic_supply_names) > > > > This will have to change when support for a device with more than 2 non-generic > > supply names gets added. Please use a literal value for MAX_SUPPLIES instead of > > ARRAY_SIZE. If the literal is 2 it would still need to change for future devices > > with more supplies, but that change would be more straighforward. > > > > I am not completely sure about this. Someone could increase MAX_SUPPLIES > without adding a generic name. That's perfectly fine and intended. MAX_SUPPLIES is a max, any list shorther than that is valid. Any longer list will result in probe() being aborted with a clear error message. > Actually two modifications will be necessary for every addition (name > and MAX_SUPPLIES). If ARRAY_SIZE is used, only new names are required, > and MAX_SUPPLIES is automatically increased. As per above it's not necessary to add a new name when MAX_SUPPLIES is increased to support more non-generic names. It would only be necessary if more generic names were added, my understanding is that this should not happen because any newly supported onboard devices are supposed to use device specific supply names. I don't like to idea of adding unused pseudo supply names to the list, just for the sake of using ARRAY_SIZE. > I understand that the whole point of this is getting rid of the generic > names, but we still have to provide generic names for every extra > supply, at least for code consistency and to avoid size mismatches > between real an generic supply names. Please let me know if you still think the extra names are needed.
On 21.02.24 22:18, Matthias Kaehlcke wrote: >>>> +/* >>>> + * Fallback supply names for backwards compatibility. If the device requires >>>> + * more than the currently supported supplies, add a new one here, and if >>>> + * possible, the real name supplies to the device-specific data. >>>> + */ >>>> +static const char * const generic_supply_names[] = { >>>> + "vdd", >>>> + "vdd2", >>>> +}; >>>> + >>>> +#define MAX_SUPPLIES ARRAY_SIZE(generic_supply_names) >>> >>> This will have to change when support for a device with more than 2 non-generic >>> supply names gets added. Please use a literal value for MAX_SUPPLIES instead of >>> ARRAY_SIZE. If the literal is 2 it would still need to change for future devices >>> with more supplies, but that change would be more straighforward. >>> >> >> I am not completely sure about this. Someone could increase MAX_SUPPLIES >> without adding a generic name. > > That's perfectly fine and intended. MAX_SUPPLIES is a max, any list > shorther than that is valid. Any longer list will result in probe() > being aborted with a clear error message. > >> Actually two modifications will be necessary for every addition (name >> and MAX_SUPPLIES). If ARRAY_SIZE is used, only new names are required, >> and MAX_SUPPLIES is automatically increased. > > As per above it's not necessary to add a new name when MAX_SUPPLIES is > increased to support more non-generic names. It would only be necessary > if more generic names were added, my understanding is that this > should not happen because any newly supported onboard devices are > supposed to use device specific supply names. I don't like to idea of > adding unused pseudo supply names to the list, just for the sake of > using ARRAY_SIZE. > >> I understand that the whole point of this is getting rid of the generic >> names, but we still have to provide generic names for every extra >> supply, at least for code consistency and to avoid size mismatches >> between real an generic supply names. > > Please let me know if you still think the extra names are needed. Not really, the only case I could come up is if an existing device that uses generic names might end up requiring a third supply, which would also be generic. But even such an unlikely event would be cover without ARRAY_SIZE. Actually one could argue that every existing device could have "vdd" and "vdd2" as their supply names and remove checks and the generic array. Best regards, Javier Carrasco
On Wed, Feb 21, 2024 at 10:33:53PM +0100, Javier Carrasco wrote: > On 21.02.24 22:18, Matthias Kaehlcke wrote: > >>>> +/* > >>>> + * Fallback supply names for backwards compatibility. If the device requires > >>>> + * more than the currently supported supplies, add a new one here, and if > >>>> + * possible, the real name supplies to the device-specific data. > >>>> + */ > >>>> +static const char * const generic_supply_names[] = { > >>>> + "vdd", > >>>> + "vdd2", > >>>> +}; > >>>> + > >>>> +#define MAX_SUPPLIES ARRAY_SIZE(generic_supply_names) > >>> > >>> This will have to change when support for a device with more than 2 non-generic > >>> supply names gets added. Please use a literal value for MAX_SUPPLIES instead of > >>> ARRAY_SIZE. If the literal is 2 it would still need to change for future devices > >>> with more supplies, but that change would be more straighforward. > >>> > >> > >> I am not completely sure about this. Someone could increase MAX_SUPPLIES > >> without adding a generic name. > > > > That's perfectly fine and intended. MAX_SUPPLIES is a max, any list > > shorther than that is valid. Any longer list will result in probe() > > being aborted with a clear error message. > > > >> Actually two modifications will be necessary for every addition (name > >> and MAX_SUPPLIES). If ARRAY_SIZE is used, only new names are required, > >> and MAX_SUPPLIES is automatically increased. > > > > As per above it's not necessary to add a new name when MAX_SUPPLIES is > > increased to support more non-generic names. It would only be necessary > > if more generic names were added, my understanding is that this > > should not happen because any newly supported onboard devices are > > supposed to use device specific supply names. I don't like to idea of > > adding unused pseudo supply names to the list, just for the sake of > > using ARRAY_SIZE. > > > >> I understand that the whole point of this is getting rid of the generic > >> names, but we still have to provide generic names for every extra > >> supply, at least for code consistency and to avoid size mismatches > >> between real an generic supply names. > > > > Please let me know if you still think the extra names are needed. > > Not really, the only case I could come up is if an existing device that > uses generic names might end up requiring a third supply, which would > also be generic. But even such an unlikely event would be cover without > ARRAY_SIZE. > > Actually one could argue that every existing device could have "vdd" and > "vdd2" as their supply names and remove checks and the generic array. Sounds good to me. Another similar option would be to assign 'generic_supply_names' to '.supply_names'. I don't have a strong preference.
On 21.02.24 20:24, Matthias Kaehlcke wrote: > On Tue, Feb 20, 2024 at 03:05:46PM +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. > > Please mention that the driver doesn't power off non-hub devices > during system suspend. > >> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net> >> --- >> drivers/usb/misc/onboard_usb_dev.c | 3 ++- >> drivers/usb/misc/onboard_usb_dev.h | 10 ++++++++++ >> 2 files changed, 12 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c >> index 2103af2cb2a6..f43130a6786f 100644 >> --- a/drivers/usb/misc/onboard_usb_dev.c >> +++ b/drivers/usb/misc/onboard_usb_dev.c >> @@ -129,7 +129,8 @@ static int __maybe_unused onboard_dev_suspend(struct device *dev) >> if (!device_may_wakeup(node->udev->bus->controller)) >> continue; >> >> - if (usb_wakeup_enabled_descendants(node->udev)) { >> + if (usb_wakeup_enabled_descendants(node->udev) || >> + !onboard_dev->pdata->is_hub) { > > > This check isn't dependent on characteristics of the USB devices processed > in this loop, therefore it can be performed at function entry. Please combine > it with the check of 'always_powered_in_suspend'. It's also an option to > omit the check completely, 'always_powered_in_suspend' will never be set for > non-hub devices (assuming the sysfs attribute isn't added). > The attribute will not be available for non-hub devices in v5. However, if the check is completely removed, will power_off not stay true at the end of the function, always leading to a device power off? As you said, 'always_powered_in_suspend' will not be set for non-hub devices. >> power_off = false; >> break; >> } > > Without code context: please omit the creation of the 'always_powered_in_suspend' > attribute for non-hub devices. As per above we don't plan to hone it, so it > shouldn't exist. Best regards, Javier Carrasco
On Thu, Feb 22, 2024 at 03:42:26PM +0100, Javier Carrasco wrote: > On 21.02.24 20:24, Matthias Kaehlcke wrote: > > On Tue, Feb 20, 2024 at 03:05:46PM +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. > > > > Please mention that the driver doesn't power off non-hub devices > > during system suspend. > > > >> Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net> > >> --- > >> drivers/usb/misc/onboard_usb_dev.c | 3 ++- > >> drivers/usb/misc/onboard_usb_dev.h | 10 ++++++++++ > >> 2 files changed, 12 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c > >> index 2103af2cb2a6..f43130a6786f 100644 > >> --- a/drivers/usb/misc/onboard_usb_dev.c > >> +++ b/drivers/usb/misc/onboard_usb_dev.c > >> @@ -129,7 +129,8 @@ static int __maybe_unused onboard_dev_suspend(struct device *dev) > >> if (!device_may_wakeup(node->udev->bus->controller)) > >> continue; > >> > >> - if (usb_wakeup_enabled_descendants(node->udev)) { > >> + if (usb_wakeup_enabled_descendants(node->udev) || > >> + !onboard_dev->pdata->is_hub) { > > > > > > This check isn't dependent on characteristics of the USB devices processed > > in this loop, therefore it can be performed at function entry. Please combine > > it with the check of 'always_powered_in_suspend'. It's also an option to > > omit the check completely, 'always_powered_in_suspend' will never be set for > > non-hub devices (assuming the sysfs attribute isn't added). > > > > The attribute will not be available for non-hub devices in v5. However, > if the check is completely removed, will power_off not stay true at the > end of the function, always leading to a device power off? As you said, > 'always_powered_in_suspend' will not be set for non-hub devices. Even without the sysfs attribute the field 'always_powered_in_suspend' could be set to true by probe() for non-hub devices.
On Tue, Feb 20, 2024 at 03:05:50PM +0100, Javier Carrasco wrote: > The current mechanism 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 as a fallback mechanism for backward compatibility. > > Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net> For v5 you could consider making this [1/8] (i.e. before the renaming of the driver). That way support for new hubs doesn't necessarily have to wait for the entire series to land. Since this series is underway I think new bindings shouldn't use 'vdd-supply' but the device specific name of the supply.
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 been added, keeping generic names for backwards compatibility and as a fallback mechanism. 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. The device binding has been added to sound/ because it is the subsystem that covers its functionality (voice processing) during normal operation. If it should reside under usb/ instead, it will be moved as required. 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 <lgirdwood@gmail.com> To: Mark Brown <broonie@kernel.org> To: Rob Herring <robh+dt@kernel.org> To: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org> To: Conor Dooley <conor+dt@kernel.org> To: Matthias Kaehlcke <mka@chromium.org> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org> To: Helen Koike <helen.koike@collabora.com> To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> To: Maxime Ripard <mripard@kernel.org> To: Thomas Zimmermann <tzimmermann@suse.de> To: David Airlie <airlied@gmail.com> To: Daniel Vetter <daniel@ffwll.ch> To: Catalin Marinas <catalin.marinas@arm.com> To: Will Deacon <will@kernel.org> To: Russell King <linux@armlinux.org.uk> Cc: linux-sound@vger.kernel.org Cc: devicetree@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: linux-usb@vger.kernel.org Cc: dri-devel@lists.freedesktop.org Cc: linux-arm-kernel@lists.infradead.org Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net> 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/20240206-onboard_xvf3500-v3-0-f85b04116688@wolfvision.net 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/20240130-onboard_xvf3500-v1-0-51b5398406cb@wolfvision.net 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/20240115-feature-xvf3500_driver-v1-0-ed9cfb48bb85@wolfvision.net/ --- Javier Carrasco (8): usb: misc: onboard_hub: rename to onboard_dev usb: misc: onboard_dev: add support for non-hub devices 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 name usb: misc: onboard_dev: use device supply names 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} | 4 +- .../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 | 525 +++++++++++++++++++++ .../misc/{onboard_usb_hub.h => onboard_usb_dev.h} | 69 ++- ...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 - 17 files changed, 709 insertions(+), 580 deletions(-) --- base-commit: 41bccc98fb7931d63d03f326a746ac4d429c1dd3 change-id: 20240130-onboard_xvf3500-6c0e78d11a1b Best regards,