| Message ID | 20240206-onboard_xvf3500-v3-6-f85b04116688@wolfvision.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: misc: onboard_hub: add support for XMOS XVF3500 | expand |
On Tue, Feb 06, 2024 at 02:59: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 <broonie@kernel.org> though... > + vdd-supply: > + description: > + Regulator for the 1V0 supply. > + > + vdd2-supply: > + description: > + Regulator for the 3V3 supply. ...it's a bit weird that the supplies are named like this, usually there'd be some sort of meaningful name (even if it's just VDD_1V0 and VDD_3V3 or something). Are you sure these are the actual names?
On 06.02.24 15:32, Mark Brown wrote: > On Tue, Feb 06, 2024 at 02:59: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 <broonie@kernel.org> > > though... > >> + vdd-supply: >> + description: >> + Regulator for the 1V0 supply. >> + >> + vdd2-supply: >> + description: >> + Regulator for the 3V3 supply. > > ...it's a bit weird that the supplies are named like this, usually > there'd be some sort of meaningful name (even if it's just VDD_1V0 and > VDD_3V3 or something). Are you sure these are the actual names? The names in the datasheet are vdd for the 1V0 supply and vddio for the 3V3 supply. I named the latter vdd2 instead because this device does not have its own driver and instead it uses the onboard_usb_hub generic driver, where the supplies are named vdd and vdd2. Those are the names used for devm_regulator_bulk_get(). Is that not the right way to match them? Thank you and best regards, Javier Carrasco
On Tue, Feb 06, 2024 at 04:05:15PM +0100, Javier Carrasco wrote: > On 06.02.24 15:32, Mark Brown wrote: > > On Tue, Feb 06, 2024 at 02:59: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 <broonie@kernel.org> > > > > though... > > > >> + vdd-supply: > >> + description: > >> + Regulator for the 1V0 supply. > >> + > >> + vdd2-supply: > >> + description: > >> + Regulator for the 3V3 supply. > > > > ...it's a bit weird that the supplies are named like this, usually > > there'd be some sort of meaningful name (even if it's just VDD_1V0 and > > VDD_3V3 or something). Are you sure these are the actual names? > > The names in the datasheet are vdd for the 1V0 supply and vddio for the > 3V3 supply. I named the latter vdd2 instead because this device does not > have its own driver and instead it uses the onboard_usb_hub generic > driver, where the supplies are named vdd and vdd2. > > Those are the names used for devm_regulator_bulk_get(). Is that not the > right way to match them? If desired the driver could be extended to support device specific regulator names through struct onboard_hub/dev_pdata.
On Tue, Feb 06, 2024 at 04:05:15PM +0100, Javier Carrasco wrote: > The names in the datasheet are vdd for the 1V0 supply and vddio for the > 3V3 supply. I named the latter vdd2 instead because this device does not > have its own driver and instead it uses the onboard_usb_hub generic > driver, where the supplies are named vdd and vdd2. > Those are the names used for devm_regulator_bulk_get(). Is that not the > right way to match them? The binding should really use vddio instead of vdd2 but if that's an existing binding then it gets more annoying, probably that existing binding is wrong too since vddio does sound like an entirely plausible standard name for a 3.3V supply. :/ At the very least the binding should document the weird mapping, though ideally the driver would be tought to request names matching the datasheet if the compatible is the one for this device. Doing the better naming might be too much hassle though.
On 06.02.24 16:22, Matthias Kaehlcke wrote: > On Tue, Feb 06, 2024 at 04:05:15PM +0100, Javier Carrasco wrote: >> On 06.02.24 15:32, Mark Brown wrote: >>> On Tue, Feb 06, 2024 at 02:59: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 <broonie@kernel.org> >>> >>> though... >>> >>>> + vdd-supply: >>>> + description: >>>> + Regulator for the 1V0 supply. >>>> + >>>> + vdd2-supply: >>>> + description: >>>> + Regulator for the 3V3 supply. >>> >>> ...it's a bit weird that the supplies are named like this, usually >>> there'd be some sort of meaningful name (even if it's just VDD_1V0 and >>> VDD_3V3 or something). Are you sure these are the actual names? >> >> The names in the datasheet are vdd for the 1V0 supply and vddio for the >> 3V3 supply. I named the latter vdd2 instead because this device does not >> have its own driver and instead it uses the onboard_usb_hub generic >> driver, where the supplies are named vdd and vdd2. >> >> Those are the names used for devm_regulator_bulk_get(). Is that not the >> right way to match them? > > If desired the driver could be extended to support device specific regulator > names through struct onboard_hub/dev_pdata. the onboard_usb_hub driver as well and their supplies are named vdd and vdd2: Documentation/devicetree/bindings/usb/cypress,hx3.yaml I took a look at its datasheet and there is no vdd2 as such, so I think we are in a similar situation here. Actually that device requires five supplies and they have been grouped into vdd and vdd2 according to their voltage level. Best regards, Javier Carrasco
On 06.02.24 16:35, Mark Brown wrote: > On Tue, Feb 06, 2024 at 04:05:15PM +0100, Javier Carrasco wrote: > >> The names in the datasheet are vdd for the 1V0 supply and vddio for the >> 3V3 supply. I named the latter vdd2 instead because this device does not >> have its own driver and instead it uses the onboard_usb_hub generic >> driver, where the supplies are named vdd and vdd2. > >> Those are the names used for devm_regulator_bulk_get(). Is that not the >> right way to match them? > > The binding should really use vddio instead of vdd2 but if that's an > existing binding then it gets more annoying, probably that existing > binding is wrong too since vddio does sound like an entirely plausible > standard name for a 3.3V supply. :/ At the very least the binding > should document the weird mapping, though ideally the driver would be > tought to request names matching the datasheet if the compatible is the > one for this device. Doing the better naming might be too much hassle > though. That is in line with my last reply, where the bindings I used as an example mention the real names of the supplies as they are defined in the datasheet. I can add that for the next version. Best regards, Javier Carrasco
On Tue, Feb 06, 2024 at 03:35:44PM +0000, Mark Brown wrote: > On Tue, Feb 06, 2024 at 04:05:15PM +0100, Javier Carrasco wrote: > > > The names in the datasheet are vdd for the 1V0 supply and vddio for the > > 3V3 supply. I named the latter vdd2 instead because this device does not > > have its own driver and instead it uses the onboard_usb_hub generic > > driver, where the supplies are named vdd and vdd2. > > > Those are the names used for devm_regulator_bulk_get(). Is that not the > > right way to match them? > > The binding should really use vddio instead of vdd2 but if that's an > existing binding then it gets more annoying, probably that existing > binding is wrong too since vddio does sound like an entirely plausible > standard name for a 3.3V supply. :/ At the very least the binding > should document the weird mapping, though ideally the driver would be > tought to request names matching the datasheet if the compatible is the > one for this device. Doing the better naming might be too much hassle > though. Initially the driver targeted a device with a single supply, the name 'vdd' was kept generic since it was expected that other devices would be supported (except for a couple of minor bits the driver is not device specific). Later support for a device with two supplies was added, with the generic name 'vdd2' to support other devices with multiple regulators. Using the correct naming would be doable, with the caveat that the old naming still needs to be supported for backwards compatibility.
On Tue, Feb 06, 2024 at 04:35:54PM +0100, Javier Carrasco wrote: > I took a look at its datasheet and there is no vdd2 as such, so I think > we are in a similar situation here. Actually that device requires five > supplies and they have been grouped into vdd and vdd2 according to their > voltage level. That binding is just broken then and should be updated to reflect the reality of the hardware. The current thing just won't work if any of those supplies come from different regulators. It's really hard to understand how bindings like this get written :/
On Tue, Feb 06, 2024 at 03:49:35PM +0000, Matthias Kaehlcke wrote: > Initially the driver targeted a device with a single supply, the name > 'vdd' was kept generic since it was expected that other devices would be > supported (except for a couple of minor bits the driver is not device > specific). Later support for a device with two supplies was added, with > the generic name 'vdd2' to support other devices with multiple regulators. It's generally always going to be a problem to add generic names that don't reflect the actual hardware names, you still end up needing to define the mapping from the real names to the generic names that have been define when you end up with the regulators being controllable. > Using the correct naming would be doable, with the caveat that the old > naming still needs to be supported for backwards compatibility. Yes, the existing bindings need to be supported as a legacy/fallback thing.
diff --git a/Documentation/devicetree/bindings/sound/xmos,xvf3500.yaml b/Documentation/devicetree/bindings/sound/xmos,xvf3500.yaml new file mode 100644 index 000000000000..3e77bb9d5eb0 --- /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 <javier.carrasco@wolfvision.net> + +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. + + vdd2-supply: + description: + Regulator for the 3V3 supply. + +required: + - compatible + - reg + - reset-gpios + - vdd-supply + - vdd2-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>; + vdd2-supply = <&vcc3v3>; + }; + }; + +...
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/ Signed-off-by: Javier Carrasco <javier.carrasco@wolfvision.net> --- .../devicetree/bindings/sound/xmos,xvf3500.yaml | 63 ++++++++++++++++++++++ 1 file changed, 63 insertions(+)