Hi Henrik
On Mon, Jul 16, 2012 at 10:59 PM, Henrik Rydberg <rydberg@xxxxxxxxxxx> wrote:
>> I am sorry, I misunderstood you. Yes, in fact, this is what wiimote
>> currently does. Well, it uses HID_CONNECT_HIDRAW but this has no
>> effect if CONFIG_HIDRAW is not set so this is equivalent to 0. My
>> first attempt was to make this work, however, this means refactoring
>> hid_connect() a lot as we need to differentiate between
>> hidraw_connect() failing and HID_CONNECT_HIDRAW being not set. Same
>> for hidinput_connect() and hiddev_connect(). That is, if
>> hid_hw_start() is called with HID_CONNECT_HIDDEV set, but
>> hiddev_connect() fails? Should the core bail out or let the device
>> through? In most cases bailing out is the best option. However, what
>> to do for the wiimote case? It requests hidraw but if hidraw_connect()
>> fails, then the wiimote driver can still work without it so in this
>> case we must not bail out.
>>
>> Taking this into account I really have no idea how to implement this
>> in a cleaner and safer way than using HID_CONNECT_OTHER. Btw., what
>> should the wiimote driver pass to hid_hw_start() in your case? It
>> wants HID_CONNECT_HIDRAW but also wants to get through if
>> CONFIG_HIDRAW is not set. It cannot pass 0 as this would always
>> disable HIDRAW. But in the case that HIDRAW is not available, it
>> cannot tell the core that it wants to stay on the bus. Hence, I think
>> using HID_CONNECT_OTHER is the only way, isn't it?
>>
>> > To catch possible mistakes, one could check for the presence of
>> > raw_event() instead, for instance.
>> >
>> > What I am getting at is that we really do not need to create any more
>> > backdoors into the hid core - on the contrary, we can most likely
>> > easily remove some of them instead.
>>
>> I fully agree, but I have to admit that I didn't find an easier way
>> that actually works.
>>
>> Thanks a lot for having a look at this. If you have any other ideas I
>> will gladly implement and test them, but I am currently out of ideas.
>
> Would something like this work for you?
Yes, that works fine. I just want some nice comment there so people
know we are using some heuristics. If you don't mind, I will wrap this
up tomorrow and send a new version of the patch(set). Otherwise, feel
free to send it yourself.
Thanks for your reviews!
David
> Henrik
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 4c87276..a43e14c 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1373,8 +1373,8 @@ int hid_connect(struct hid_device *hdev, unsigned int connect_mask)
> if ((connect_mask & HID_CONNECT_HIDRAW) && !hidraw_connect(hdev))
> hdev->claimed |= HID_CLAIMED_HIDRAW;
>
> - if (!hdev->claimed) {
> - hid_err(hdev, "claimed by neither input, hiddev nor hidraw\n");
> + if (!hdev->claimed && !hdev->driver->raw_event) {
> + hid_err(hdev, "device has no listeners, quitting\n");
> return -ENODEV;
> }
>
> diff --git a/drivers/hid/hid-picolcd.c b/drivers/hid/hid-picolcd.c
> index 45c3433..74c388d 100644
> --- a/drivers/hid/hid-picolcd.c
> +++ b/drivers/hid/hid-picolcd.c
> @@ -2613,11 +2613,7 @@ static int picolcd_probe(struct hid_device *hdev,
> goto err_cleanup_data;
> }
>
> - /* We don't use hidinput but hid_hw_start() fails if nothing is
> - * claimed. So spoof claimed input. */
> - hdev->claimed = HID_CLAIMED_INPUT;
> error = hid_hw_start(hdev, 0);
> - hdev->claimed = 0;
> if (error) {
> hid_err(hdev, "hardware start failed\n");
> goto err_cleanup_data;
--
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
