Am 15.06.2016 um 22:59 schrieb Heiner Kallweit: > Am 15.06.2016 um 09:31 schrieb Benjamin Tissoires: >> On Jun 15 2016 or thereabouts, Heiner Kallweit wrote: >>> Am 14.06.2016 um 23:49 schrieb Benjamin Tissoires: >>>> On Jun 12 2016 or thereabouts, Heiner Kallweit wrote: >>>>> The Riso Kagaku Webmail Notifier (and its clones) is supported as part of >>>>> usb/misc/usbled driver currently. This patch migrates the driver for this >>>>> device to the HID subsystem. >>>>> >>>>> Benefits: >>>>> - Avoid using USB low-level calls and use the HID subsystem instead >>>>> (as this device provides a USB HID interface) >>>>> - Use standard LED subsystem instead of proprietary sysfs entries, >>>>> this allows e.g. to use the device with features like triggers >>>>> >>>>> I know at least one cheap clone coming with green and blue LED switched. >>>>> There's a module paramater to deal with this. >>>>> >>>>> There might be users of the current module, therefore so far allow >>>>> compilation of the new driver only if the old one is disabled. >>>>> >>>>> Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx> >>>>> --- >>>>> v2: >>>>> - change config symbol from HID_RIKA to HID_RISO_KAGAKU >>>>> - use full name Riso Kagaku instead of rika in several places >>>>> - don't remove device from ignore list if CONFIG_USB_LED is defined >>>>> - fix module parameter permissions >>>> >>>> Thanks for the respin. This version (and the other for hid-rika) looks >>>> fine. Though I wonder if you should not call hid_hw_open() at the end of >>>> probe (and hid_hw_close() during remove) to keep the device opened. >>>> Otherwise I am unsure whether your usb commands will get treated (I >>>> might be wrong). >>>> >>> When looking at other hid device drivers hid_hw_open seems to be needed >>> only if the device needs special treatment. >> >> Not exactly. hid_hw_open is called whenever someone opens the hidraw or >> input node (or any other resource attached to the HID device). This >> starts the input reading, but also calls usb_autopm_get_interface() >> which will prevent the device to be autosuspended. The drivers that call >> hid_hw_open() in their probe are the ones where we know no one will open >> the hidraw or input node but we need to poll for events. >> >>> My Riso Kagaku and Dream Cheeky devices work fine with the new drivers >>> w/o calling hid_hw_open. >> >> Let's keep it that way then. >> >>> >>>> There is also one high level comment I would like to do: >>>> even if your driver is better than usbled.c, people won't use it because >>>> distributions won't enable it as there is 2 issues: >>>> - one is you are breaking kernel ABI (we already discussed it) >>>> - the other one is that if people want to use your new hid drivers, they >>>> have to disable CONFIG_USB_LED, which removes support of the >>>> DELCOM_VISUAL_SIGNAL_INDICATOR >>>> >>> Yes, I think you're right. >>> With regard to the Delcom Visual Signal Indicator: >>> The old USB LED driver supports only generation 1 of these devices which >>> have a not fully HID compliant interface. >>> In 2008 they were replace with generation 2 which has fully a HID >>> compliant interface. Generation 2 is not yet supported by the kernel. >>> (Currently I'm trying to get hold of such a device.) >>> >>> Therefore it might be better to leave support for the Gen 1 Delcom device >>> at USB misc. However this would mean that we have to change the config >>> symbol for this driver so that it doesn't conflict with the other part >>> we're moving to hid. >>> Would changing the config symbol be an issue? For now we could select >>> the new config symbol automatically if CONFIG_USB_LED is set. >> >> Why would you change the old config symbol for the existing devices. If >> (and I don't think that's the best option) you were to add a new symbol, >> you could use this to select between HID and USB for the new drivers: >> CONFIG_USB_LED_LEGACY_SUPPORT would depend on CONFIG_USB_LED, and you >> choose to enable the HID driver based on CONFIG_USB_LED_LEGACY_SUPPORT. >> >>> >>>> I really think it would not cost too much to do the whole change in 2 >>>> passes: >>>> - move the entire usbled to hid, keeping the old sysfs API in place, and >>>> eventually add a symlink from the old place (under the usb device) to >>>> the HID device to keep path stable >>> >>> I guess you don't mean symlink literally but adding a comment in Kconfig >>> that the driver was moved? >> >> No. I'd rather not having the files moved around when they are KABI. So >> either you add the old sysfs files directly at the same level, or you >> symlink to them. Some HID driver already take a pointer to the >> underlying USB device, so you can do the same. >> > OK, thanks for the further explanations. So, what I would do: > - merge new LED HID drivers to a "new USB LED" driver > - remove Riso Kagaku and Dream Cheeky support from old USB LED driver > (the old driver then just supports the non-HID Gen 1 Delcom device) > - let new USB LED driver create sysmlinks in sysfs to not break the old ABI > > The latter can be achieved easier than expected as the old sysfs > attributes share the same semantics as the brightness attribute of > the new led_classdev. Means I don't have to implement the attribute > show / store ops for the old attributes. > Therefore I think we can do the change in one pass. > > For creating the symlinks I just need something like: > > ret = devm_led_classdev_register(&dcdev->hdev->dev, &led->cdev); > if (ret) > return ret; > > node = sysfs_get_dirent(led->cdev.dev->kobj.sd, "brightness"); > kernfs_create_link(dcdev->hdev->dev.parent->kobj.sd, color_name, node); > sysfs_put(node); > > Looks a little bit scary but works perfectly fine here. > It creates the following link (same location as with old USB LED driver) > /sys/devices/pci0000:00/0000:00:14.0/usb1/1-6/1-6:1.0/green > like this: > green -> 0003:1D34:0004.0002/leds/dream_cheeky0:green/brightness > > The only issue so far is that kernfs_create_link is public but misses > an EXPORT_SYMBOL_GPL declaration. Therefore I can't use it in a module. > For my tests I exported it and the module worked fine. > > Seems like symlinking to kernfs_node is not supported yet for modules, > only symlinking to kobject. > Having said that I either have to convince the sysfs/kernfs maintainers > that this export is a good thing or the new USB LED driver is not available > as a module for now. > Greg, as original author of the USB LED driver you've been on cc anyway and just by chance you're also maintainer of sysfs / kernfs. Would exporting kernfs_create_link be fine with you and if yes: Should we add a sysfs_.. wrapper (e.g. sysfs_create_node_link) in linux/sysfs.h like for other kernfs_.. calls ? Regards, Heiner > Regards, Heiner > >>> >>>> - then add support for the classdev for the whole 3 types of devices >>>> >>>> Plus that would means only one driver to maintain instead of 3 that are >>>> close enough. >>>> >>>> Cheers, >>>> Benjamin >>>> >>> Regards, Heiner >>> >> >> Cheers, >> Benjamin >> > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
