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.

Attachments:

(No filename) (955.00 B)
signature.asc (499.00 B)
Download all attachments

2024-02-06 15:36:37

by Javier Carrasco

[permalink] [raw]

Subject: Re: [PATCH v3 6/7] ASoC: dt-bindings: xmos,xvf3500: add XMOS XVF3500 voice processor

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 <[email protected]>
>>>
>>> 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

2024-02-06 15:50:18

by Matthias Kaehlcke

[permalink] [raw]

Subject: Re: [PATCH v3 6/7] ASoC: dt-bindings: xmos,xvf3500: add XMOS XVF3500 voice processor

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.

2024-02-06 16:00:24

by Javier Carrasco

[permalink] [raw]

Subject: Re: [PATCH v3 6/7] ASoC: dt-bindings: xmos,xvf3500: add XMOS XVF3500 voice processor

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

2024-02-06 16:23:32

by Mark Brown

[permalink] [raw]

Subject: Re: [PATCH v3 6/7] ASoC: dt-bindings: xmos,xvf3500: add XMOS XVF3500 voice processor

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 :/

Attachments:

(No filename) (574.00 B)
signature.asc (499.00 B)
Download all attachments

2024-02-06 16:36:31

On Tue, Feb 13, 2024 at 11:36:25AM +0100, Javier Carrasco wrote:
> Hi Matthias,
>
> On 06.02.24 18:55, Matthias Kaehlcke wrote:
> > Hi Javier,
> >
> > a few comments inline
> >
> > On Tue, Feb 06, 2024 at 02:59:29PM +0100, Javier Carrasco wrote:
> >> +static struct onboard_dev *_find_onboard_dev(struct device *dev)
> >> +{
> >> + struct platform_device *pdev;
> >> + struct device_node *np;
> >> + struct onboard_dev *onboard_dev;
> >> +
> >> + pdev = of_find_device_by_node(dev->of_node);
> >> + if (!pdev) {
> >> + np = of_parse_phandle(dev->of_node, "peer-hub", 0);
> >> + if (!np) {
> >> + dev_err(dev, "failed to find device node for peer hub\n");
> >> + return ERR_PTR(-EINVAL);
> >> + }
> >> +
> >> + pdev = of_find_device_by_node(np);
> >> + of_node_put(np);
> >> +
> >> + if (!pdev)
> >> + return ERR_PTR(-ENODEV);
> >> + }
> >
> > The above branch should probably be guarded by 'if (!onboard_dev->pdata->is_hub)',
> > this is also a change for ""usb: misc: onboard_dev: add support for non-hub devices"
> >
> I am not sure how to guard the branch like that because onboard_dev is
> retrieved by means of pdev->dev, which is not available if
> of_find_device_by_node returns NULL. The non-hub device will not have a
> peer-hub property according to its bindings anyway, right?

Right, onboard_dev isn't available yet at that point, my bad. I thought it
would be nice to clearly separate the hub related logic from other devices,
but that isn't an option in this case.