Re: [PATCH 2/3] ASoC: dt-bindings: xmos,xvf3500: add bindings for XMOS XVF3500

[Javier Carrasco <javier.carrasco@xxxxxxxxxxxxxx>]

On 22.01.24 13:29, Krzysztof Kozlowski wrote:
> On 16/01/2024 08:29, Javier Carrasco wrote:
>> On 15.01.24 21:43, Krzysztof Kozlowski wrote:
>>> On 15/01/2024 20:43, Javier Carrasco wrote:
>>>> On 15.01.24 19:11, Krzysztof Kozlowski wrote:
>>>>> On 15/01/2024 17:24, Javier Carrasco wrote:
>>>>>> Do you mean that the XVF3500 should not be represented as a platform
>>>>>> device and instead it should turn into an USB device represented as a
>>>>>> node of an USB controller? Something like this (Rockchip SoC):
>>>>>>
>>>>>> &usb_host1_xhci {
>>>>>> 	...
>>>>>>
>>>>>> 	xvf3500 {
>>>>>> 		...
>>>>>> 	};
>>>>>> };
>>>>>>
>>>>>> Did I get you right or is that not the correct representation? Thank you
>>>>>> again.
>>>>>
>>>>> I believe it should be just like onboard hub. I don't understand why
>>>>> onboard hub was limited to hub, because other USB devices also could be
>>>>> designed similarly by hardware folks :/
>>>>>
>>>>> And if we talk about Linux drivers, then your current solution does not
>>>>> support suspend/resume and device unbind.
>>>>>
>>>>> Best regards,
>>>>> Krzysztof
>>>>>
>>>>
>>>> Actually this series is an attempt to get rid of a misuse of the
>>>> onboard_usb_hub driver by a device that is not a HUB, but requires the
>>>> platform-part of that driver for the initialization.
>>>
>>> That's just naming issue, isn't it?
>>>
>>>>
>>>> What would be the best approach to provide support upstream? Should I
>>>> turn this driver into a generic USB driver that does what the
>>>> platform-part of the onboard HUB does? Or are we willing to accept
>>>
>>> No, because you did not solve the problems I mentioned. This is neither
>>> accurate hardware description nor proper Linux driver model handling PM
>>> and unbind.
>>>
>> You mentioned the PM handling twice, but I am not sure what you mean.
>> The driver provides callbacks for SIMPLE_DEV_PM_OPS, which I tested in
>> freeze and memory power states with positive results. On the other hand,
>> I suppose that you insisted for a good reason, so I would be grateful if
>> you could show me what I am doing wrong. The macro pattern was taken
>> from other devices under sound/, which also check CONFIG_PM_SLEEP,
>> but maybe I took a bad example or missed something.
> 
> You have two Linux devices: USB device and platform device. The platform
> device controls power of USB device. If platform device goes to some
> variant of sleep (PRM, system PM) before the USB device, how will USB
> device react? Will it work? I doubt.
> 
> You have no ordering / dependencies / device links between these devices
> thus possible problems.
> 
Thank you for clarifying this point. I only covered the platform device
and such device links are missing. I will opt for the inclusion in
onboard_usb_hub, which already covers both devices.
>>>> non-HUB devices in the onboard_usb_hub driver even though it supports
>>>> more operations?
>>>>
>>>> I am adding linux-usb to this thread in case someone has other suggestions.
>>>
>>> I don't see any difference between this device and onboard hub. The
>>> concept and the problem is the same. Therefore either treat it as as
>>> onboard hub or come with USB-version of PCI power sequencing.
>>>
>> I have nothing against adding this device to onboard_usb_hub as long as
>> it is valid upstream, so no conflicts arise with new additions to the
>> list (which was the trigger for all of this with v6.7). That is
> 
> I am sorry, but we talk here only about upstream. I don't know therefore
> what "valid upstream" means. You cannot send a patch which is "not valid
> upstream". I mean, technically you can, but this would be waste of our
> time and receive rather annoyed responses.
> 
I meant valid upstream, so sending such patch makes sense to the
community (and not only to us). So far you are the source of feedback
upstream, so I will follow your advice.
>> obviously the most trivial solution and as you said, it is justs a
>> naming issue because the power sequence is not HUB-specific.
> 
> I would ack it. Others? No clue, I also do not remember full story
> behind onboard USB hub and why it was called "hub" instead of "onboard
> USB device".
> 
> Best regards,
> Krzysztof
> 

Thank you and best regards,
Javier Carrasco