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 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-input" 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 Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux