On Fri, 16 May 2008, Jiri Slaby wrote:
> static const struct hid_device_id hid_blacklist[] = {
> + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_MX3000_RECEIVER) },
> + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_S510_RECEIVER) },
> + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_S510_RECEIVER_2) },
> { }
> };
This shouldn't be needed as soon as the userspace supports the proper
module autoloading, right?
> +#define LOGITECH_RDESC 0x1
> +
> +/*
> + * Certain Logitech keyboards send in report #3 keys which are far
> + * above the logical maximum described in descriptor. This extends
> + * the original value of 0x28c of logical maximum to 0x104d
> + */
> +static void lg_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> + unsigned int rsize)
> +{
> + if (rsize >= 90 && rdesc[83] == 0x26
> + && rdesc[84] == 0x8c
> + && rdesc[85] == 0x02) {
> + dev_info(&hdev->dev, "fixing up Logitech keyboard report "
> + "descriptor\n");
> + rdesc[84] = rdesc[89] = 0x4d;
> + rdesc[85] = rdesc[90] = 0x10;
> + }
> +}
> +
> +static const struct hid_device_id lg_devices[] = {
> + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_MX3000_RECEIVER),
> + .driver_data = LOGITECH_RDESC },
> + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_S510_RECEIVER),
> + .driver_data = LOGITECH_RDESC },
> + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_S510_RECEIVER_2),
> + .driver_data = LOGITECH_RDESC },
> + { }
You set the LOGITECH_RDESC flag here, but it is then never used anywhere.
I guess that your original intent was to check for this flag in
lg_report_fixup(), right?
Thanks,
--
Jiri Kosina
SUSE Labs
On 06/04/2008 03:21 PM, Jiri Kosina wrote:
> On Fri, 16 May 2008, Jiri Slaby wrote:
>
>> static const struct hid_device_id hid_blacklist[] = {
>> + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_MX3000_RECEIVER) },
>> + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_S510_RECEIVER) },
>> + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_S510_RECEIVER_2) },
>> { }
>> };
>
> This shouldn't be needed as soon as the userspace supports the proper
> module autoloading, right?
This is needed to tell generic drivers not to bind these, its' generic
blacklist. I have no idea how this could be done better with current drivers/base/.
>> +#define LOGITECH_RDESC 0x1
>> +
>> +/*
>> + * Certain Logitech keyboards send in report #3 keys which are far
>> + * above the logical maximum described in descriptor. This extends
>> + * the original value of 0x28c of logical maximum to 0x104d
>> + */
>> +static void lg_report_fixup(struct hid_device *hdev, __u8 *rdesc,
>> + unsigned int rsize)
>> +{
>> + if (rsize >= 90 && rdesc[83] == 0x26
>> + && rdesc[84] == 0x8c
>> + && rdesc[85] == 0x02) {
>> + dev_info(&hdev->dev, "fixing up Logitech keyboard report "
>> + "descriptor\n");
>> + rdesc[84] = rdesc[89] = 0x4d;
>> + rdesc[85] = rdesc[90] = 0x10;
>> + }
>> +}
>> +
>> +static const struct hid_device_id lg_devices[] = {
>> + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_MX3000_RECEIVER),
>> + .driver_data = LOGITECH_RDESC },
>> + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_S510_RECEIVER),
>> + .driver_data = LOGITECH_RDESC },
>> + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_S510_RECEIVER_2),
>> + .driver_data = LOGITECH_RDESC },
>> + { }
>
> You set the LOGITECH_RDESC flag here, but it is then never used anywhere.
> I guess that your original intent was to check for this flag in
> lg_report_fixup(), right?
I assumed use of these in the second round (see my previous email) :). Anyway
they are useless now.
On Mon, 9 Jun 2008, Jiri Slaby wrote:
> > > static const struct hid_device_id hid_blacklist[] = {
> > > + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
> > > USB_DEVICE_ID_MX3000_RECEIVER) },
> > > + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_S510_RECEIVER)
> > > },
> > > + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
> > > USB_DEVICE_ID_S510_RECEIVER_2) },
> > > { }
> > > };
> > This shouldn't be needed as soon as the userspace supports the proper module
> > autoloading, right?
> This is needed to tell generic drivers not to bind these, its' generic
> blacklist. I have no idea how this could be done better with current
> drivers/base/.
Hmm ... but if we make sure that the order in `modules.order' puts all the
specialized drivers before the generic one, the binding should be done
correctly even without blacklist, right?
--
Jiri Kosina
SUSE Labs
Am Mittwoch 11 Juni 2008 16:13:43 schrieb Jiri Kosina:
> On Mon, 9 Jun 2008, Jiri Slaby wrote:
>
> > > > static const struct hid_device_id hid_blacklist[] = {
> > > > + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
> > > > USB_DEVICE_ID_MX3000_RECEIVER) },
> > > > + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_S510_RECEIVER)
> > > > },
> > > > + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
> > > > USB_DEVICE_ID_S510_RECEIVER_2) },
> > > > { }
> > > > };
> > > This shouldn't be needed as soon as the userspace supports the proper module
> > > autoloading, right?
> > This is needed to tell generic drivers not to bind these, its' generic
> > blacklist. I have no idea how this could be done better with current
> > drivers/base/.
>
> Hmm ... but if we make sure that the order in `modules.order' puts all the
> specialized drivers before the generic one, the binding should be done
> correctly even without blacklist, right?
No. You might have two devices connected. The first correctly triggers
the loading of the generic driver. The second would first load the specialised
driver but the already loaded driver will be faster.
Regards
Oliver
Jiri Kosina wrote:
> On Mon, 9 Jun 2008, Jiri Slaby wrote:
>
>>>> static const struct hid_device_id hid_blacklist[] = {
>>>> + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
>>>> USB_DEVICE_ID_MX3000_RECEIVER) },
>>>> + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_S510_RECEIVER)
>>>> },
>>>> + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
>>>> USB_DEVICE_ID_S510_RECEIVER_2) },
>>>> { }
>>>> };
>>> This shouldn't be needed as soon as the userspace supports the proper module
>>> autoloading, right?
>> This is needed to tell generic drivers not to bind these, its' generic
>> blacklist. I have no idea how this could be done better with current
>> drivers/base/.
>
> Hmm ... but if we make sure that the order in `modules.order' puts all the
> specialized drivers before the generic one, the binding should be done
> correctly even without blacklist, right?
A little off-topic, but is there a module-init-tools repository with
modules.order handling somewhere, or does it (still) only exist as
not-yet-integrated patches?
--
Anssi Hannula
On 06/11/2008 05:39 PM, Oliver Neukum wrote:
> Am Mittwoch 11 Juni 2008 16:13:43 schrieb Jiri Kosina:
>> On Mon, 9 Jun 2008, Jiri Slaby wrote:
>>
>>>>> static const struct hid_device_id hid_blacklist[] = {
>>>>> + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
>>>>> USB_DEVICE_ID_MX3000_RECEIVER) },
>>>>> + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_S510_RECEIVER)
>>>>> },
>>>>> + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
>>>>> USB_DEVICE_ID_S510_RECEIVER_2) },
>>>>> { }
>>>>> };
>>>> This shouldn't be needed as soon as the userspace supports the proper module
>>>> autoloading, right?
>>> This is needed to tell generic drivers not to bind these, its' generic
>>> blacklist. I have no idea how this could be done better with current
>>> drivers/base/.
>> Hmm ... but if we make sure that the order in `modules.order' puts all the
>> specialized drivers before the generic one, the binding should be done
>> correctly even without blacklist, right?
>
> No. You might have two devices connected. The first correctly triggers
> the loading of the generic driver. The second would first load the specialised
> driver but the already loaded driver will be faster.
Hm, the problem here is, that report (supported inputs et al.) parsing needs to
be done some time. Since some devices have reports broken too, some of their
reports need to be fixed before parsing. So the parsing is postponed after
*first* driver binds, then it's checked if the coming driver has report_fixup
hook and if yes, it's executed and the device is finally parsed and set up. If
it has not (e.g. generic), it's just parsed and set up.
If you bind the generic driver as the first driver and the particular device
needs report fixing, it never performs the fixup. Actually what I don't know is
how to solve this effectively.