Re: [PATCH v2] hid: migrate Riso Kagaku LED driver from USB misc to HID

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Am 16.06.2016 um 09:08 schrieb Greg Kroah-Hartman:
> 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.
> 
OK, then I'll submit a patch for removing the old USB LED driver.

Regards, Heiner

> 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



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux