On Tue, 16 Jan 2024 08:29:04 +0100, 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. FWIW, the patterns in sound/ are somewhat outdated and need to be refreshed. Nowadays one should use DEFINE_SIMPLE_DEV_PM_OPS() instead (that should work without ifdef). thanks, Takashi