Pawel, On 08/08/2019 07:12, Pawel Laszczak wrote: > Hi Roger, > >> >> >> On 23/07/2019 07:32, Pawel Laszczak wrote: >> >>> Hi, >> >>> >> >>>> On Mon 2019-07-22 13:56:44, Pavel Machek wrote: >> >>>>> Hi! >> >>>>> >> >>>>>>>>> This patch introduce new Cadence USBSS DRD driver to linux kernel. >> >>>>>>>>> >> >>>>>>>>> The Cadence USBSS DRD Controller is a highly configurable IP Core which >> >>>>>>>>> can be instantiated as Dual-Role Device (DRD), Peripheral Only and >> >>>>>>>>> Host Only (XHCI)configurations. >> >>>>>>>> >> >>>>>>>> I see you are using debugfs to select between DRD, peripheral-onlyh and XHCI... >> >>>>>>>> >> >>>>>>>> Is that good idea? >> >>>>>>> >> >>>>>>> Yes driver allows selecting dr_mode by debugfs. Controller also support such functionality >> >>>>>>> so I don't understand why would it not be a good idea. >> >>>>>>> >> >>>>>>> I personally use this for testing but it can be used to limit controller functionality without >> >>>>>>> recompiling kernel. >> >>>>>> >> >>>>>> debugfs is ONLY for debugging, never rely on it being enabled, or >> >>>>>> mounted, on a system in order to have any normal operation happen. >> >>>>>> >> >>>>>> So for testing, yes, this is fine. If this is going to be the normal >> >>>>>> api/interface for how to control this driver, no, that is not acceptable >> >>>>>> at all. >> >>>>> >> >>>>> It makes a lot of sense for end-user to toggle this... for example >> >>>>> when he is lacking right cable for proper otg detection. As it is >> >>>>> third driver offering this functionality, I believe we should stop >> >>>>> treating it as debugging. >> >>>> >> >>>> At least renesas usb controller seems to have variables in sysfs: >> >>>> drivers/phy/renesas/phy-rcar-gen3-usb2.c : functions role_show and >> >>>> role_store. See also >> >>>> Documentation/ABI/testing/sysfs-platform-phy-rcar-gen3-usb2 . >> >>>> >> >>>> I believe this driver should do same. >> >>>> >> >>> >> >>> CDNS3 driver use the role framework and also has such variable defined >> >>> in role switch framework. >> >>> >> >>> https://urldefense.proofpoint.com/v2/url?u=https- >> 3A__elixir.bootlin.com_linux_latest_source_drivers_usb_roles_class.c&d=DwICaQ&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ- >> _haXqY&r=e1OgxfvkL0qo9XO6fX1gscva-w03uSYC1nIyxl89-rI&m=_jBsEOB3gtoQVvsVk8k2Pz8dp9zhzZbbL4M0tINJLR8&s=mq5ce-d4Td- >> lc3OvcvektfSHhXPAL2Go2gWP-q9QwTY&e= >> > > The meaning is little different. Role switch framework allow to changing role [ host -> device, device -> host ] > > The debugfs.c allows to limit dr_mode. > >> >> Can we get rid of the debugfs interface for user initiated role change and just >> >> rely on role switch framework via sysfs? >> >> >> >> We do need user initiated role changes in production systems. So we can't >> >> rely on debugfs for this. > > But I assume that in production systems this will be disabled. > cdns3-$(CONFIG_DEBUG_FS) += debugfs.o > But we do want user space based role switching in production systems. > I think that I understand your concerns. My idea was not to expand the supported > dr_mode. Rather I wanted to have possibility to limit this (only for testing). > > Eg. > If cdns->dr_mode = USB_DR_MODE_OTG > then we can limit mode to HOST or DEVICE or DRD In this case we register with role switch framework. > if cdns->dr_mode == USB_DR_MODE_HOST || > cdns->dr_mode == USB_DR_MODE_PERIPHERAL) > then driver can't change anything In this case we don't register to role switch framework. > > It allows me for testing some functionality using only single board > and even with lacking right cable for proper otg detection. > > So, removing this can cause that testing some functionality > will be limited on my boards. > > If you rely want to remove this, maybe we could do this > after putting this driver to kernel ?. I don't want you to remove the user based role change functionality. I'm just asking to rely on role switch framework for that and not debugfs. > > Maintaining this as my internal code before putting this driver > to kernel will be problematic. > > Regards, > Pawell > > -- cheers, -roger Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
