On Thu, Jun 16, 2016 at 07:45:35AM +0200, Heiner Kallweit wrote: > 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 ? Ick, no. Wait, I wrote this? Oh yeah, no, just delete it, and use your new version instead. No need to preserve this bizarre one-off api for any reason, I don't think anyone uses it. The device I had is long-gone. If anyone objects after we remove it, we can think about adding it back, but for now, let's just delete it. thanks, greg k-h -- 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
