Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965032AbaDIUmP (ORCPT ); Wed, 9 Apr 2014 16:42:15 -0400 Received: from mail-lb0-f169.google.com ([209.85.217.169]:39104 "EHLO mail-lb0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934010AbaDIUmJ (ORCPT ); Wed, 9 Apr 2014 16:42:09 -0400 MIME-Version: 1.0 In-Reply-To: <1396631227-11770-1-git-send-email-tolga@cevel.net> References: <1396631227-11770-1-git-send-email-tolga@cevel.net> Date: Wed, 9 Apr 2014 16:42:08 -0400 Message-ID: Subject: Re: [PATCH 3/4] HID: microsoft: added Sidewinder X4 / X6 sysfs support From: Benjamin Tissoires To: Tolga Cakir Cc: Benjamin Tissoires , Derya , Jiri Kosina , Reyad Attiyat , linux-input , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Apr 4, 2014 at 1:07 PM, Tolga Cakir wrote: > This patch enables us to set the profile, LEDs and read the key mask via sysfs. Documentation is included in this patch. > > Signed-off-by: Tolga Cakir > --- > .../ABI/testing/sysfs-driver-hid-microsoft | 30 +++ > drivers/hid/hid-microsoft.c | 231 +++++++++++++++++++++ > 2 files changed, 261 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-microsoft > > diff --git a/Documentation/ABI/testing/sysfs-driver-hid-microsoft b/Documentation/ABI/testing/sysfs-driver-hid-microsoft > new file mode 100644 > index 0000000..9fdfad7 > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-driver-hid-microsoft > @@ -0,0 +1,30 @@ > +What: /sys/bus/usb/devices/-:./::./auto_led > +Date: April 2014 > +Contact: Tolga Cakir > +Description: This file allows you to set and view the Auto LED status. > + Valid values are 0 and 1. > + > +What: /sys/bus/usb/devices/-:./::./profile > +Date: April 2014 > +Contact: Tolga Cakir > +Description: Both, the Sidewinder X4 and the X6 can handle profiles. > + They can be switched by writing to this file. Profile LEDs will be > + set accordingly. > + Valid values are 1 - 3. > + > +What: /sys/bus/usb/devices/-:./::./key_mask > +Date: April 2014 > +Contact: Tolga Cakir > +Description: This file is read-only and outputs an unsigned long integer. > + Every bit represents one of the S1 - S6 extra keys on the Sidewinder > + X4 and S1 - S30 on the Sidewinder X6. The least significant bit > + represents S1 on both gaming keyboards, the most significant bit > + represents the Bank switch button on both keyboards. Multiple > + special keys can be pressed at the same time. So that means that the user space polls the sysfs to trigger the actual macros? Or maybe I am missing something. > + > +What: /sys/bus/usb/devices/-:./::./record_led > +Date: April 2014 > +Contact: Tolga Cakir > +Description: This file allows you to set and view the Record LED status. > + Valid values are 0 - 2. 0 stands for off, 1 for solid mode and 2 > + for blinking mode. > diff --git a/drivers/hid/hid-microsoft.c b/drivers/hid/hid-microsoft.c > index 5b5d40f..5674c0c 100644 > --- a/drivers/hid/hid-microsoft.c > +++ b/drivers/hid/hid-microsoft.c > @@ -19,6 +19,8 @@ > #include > #include > #include > +#include > +#include iiiik, I really don't like seeing this in a HID driver. I spent too much try fighting to make HID usb agnostic to accept that blindy. Fortunately, it doesn't seems to be used in the final patch. > > #include "hid-ids.h" > > @@ -209,6 +211,219 @@ static int ms_sidewinder_kb_quirk(struct hid_input *hi, struct hid_usage *usage, > return 1; > } > > +static int ms_sidewinder_control(struct hid_device *hdev, __u8 setup) > +{ > + struct ms_data *sc = hid_get_drvdata(hdev); > + struct ms_sidewinder_extra *sidewinder = sc->extra; > + struct hid_report *report = > + hdev->report_enum[HID_FEATURE_REPORT].report_id_hash[7]; > + > + /* > + * LEDs 1 - 3 should not be set simultaneously, however > + * they can be set in any combination with Auto or Record LEDs. > + */ > + report->field[0]->value[0] = (setup & 0x01) ? 0x01 : 0x00; /* X6 only: Macro Pad toggle */ > + report->field[0]->value[1] = (setup & 0x02) ? 0x01 : 0x00; /* LED Auto */ > + report->field[0]->value[2] = (setup & 0x04) ? 0x01 : 0x00; /* LED 1 */ > + report->field[0]->value[3] = (setup & 0x08) ? 0x01 : 0x00; /* LED 2 */ > + report->field[0]->value[4] = (setup & 0x10) ? 0x01 : 0x00; /* LED 3 */ > + report->field[1]->value[0] = 0x00; /* Clear Record LED */ > + > + switch (setup & 0x60) { > + case 0x40: report->field[1]->value[0] = 0x02; break; /* Record LED Blink */ > + case 0x20: report->field[1]->value[0] = 0x03; break; /* Record LED Solid */ do we have any guarantees that setup & 0x60 != 0x60? > + } > + > + hid_hw_request(hdev, report, HID_REQ_SET_REPORT); > + sidewinder->status = setup; > + > + return 0; > +} > + > +/* > + * Sidewinder sysfs > + * @key_mask: show pressed special keys > + * @profile: show and set profile count and LED status > + * @auto_led: show and set LED Auto > + * @record_led: show and set Record LED > + */ > +static ssize_t ms_sidewinder_key_mask_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct hid_device *hdev = container_of(dev, struct hid_device, dev); > + struct ms_data *sc = hid_get_drvdata(hdev); > + struct ms_sidewinder_extra *sidewinder = sc->extra; > + > + return snprintf(buf, PAGE_SIZE, "%lu\n", sidewinder->key_mask); > +} > + > +static struct device_attribute dev_attr_ms_sidewinder_key_mask = { > + .attr = { .name = __stringify(key_mask), .mode = S_IRUGO }, > + .show = ms_sidewinder_key_mask_show > +}; > + > +static ssize_t ms_sidewinder_profile_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct hid_device *hdev = container_of(dev, struct hid_device, dev); > + struct ms_data *sc = hid_get_drvdata(hdev); > + struct ms_sidewinder_extra *sidewinder = sc->extra; > + > + return snprintf(buf, PAGE_SIZE, "%1u\n", sidewinder->profile); > +} > + > +static ssize_t ms_sidewinder_profile_store(struct device *dev, > + struct device_attribute *attr, char const *buf, size_t count) > +{ > + struct hid_device *hdev = container_of(dev, struct hid_device, dev); > + struct ms_data *sc = hid_get_drvdata(hdev); > + struct ms_sidewinder_extra *sidewinder = sc->extra; > + __u8 setup = sidewinder->status & ~(0x1c); /* Clear Profile LEDs */ > + > + if (sscanf(buf, "%1u", &sidewinder->profile) != 1) > + return -EINVAL; > + > + if (sidewinder->profile < 1 || sidewinder->profile > 3) { > + return -EINVAL; > + } else { > + setup |= 0x02 << sidewinder->profile; > + ms_sidewinder_control(hdev, setup); > + return strnlen(buf, PAGE_SIZE); > + } > +} > + > +static struct device_attribute dev_attr_ms_sidewinder_profile = > + __ATTR(profile, S_IWUSR | S_IRUGO, > + ms_sidewinder_profile_show, > + ms_sidewinder_profile_store); > + > +static ssize_t ms_sidewinder_macro_pad_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct hid_device *hdev = container_of(dev, struct hid_device, dev); > + struct ms_data *sc = hid_get_drvdata(hdev); > + struct ms_sidewinder_extra *sidewinder = sc->extra; > + > + return snprintf(buf, PAGE_SIZE, "%1u\n", (sidewinder->status & 0x01)); > +} > + > +static ssize_t ms_sidewinder_macro_pad_store(struct device *dev, > + struct device_attribute *attr, char const *buf, size_t count) > +{ > + struct hid_device *hdev = container_of(dev, struct hid_device, dev); > + struct ms_data *sc = hid_get_drvdata(hdev); > + struct ms_sidewinder_extra *sidewinder = sc->extra; > + unsigned int value; > + __u8 setup; > + > + if (sscanf(buf, "%1u", &value) != 1) > + return -EINVAL; > + > + if (value < 0 || value > 1) { > + return -EINVAL; > + } else { > + setup = sidewinder->status & ~(0x01); /* Clear Macro Pad */ > + if (value) > + setup |= 0x01; > + ms_sidewinder_control(hdev, setup); > + return strnlen(buf, PAGE_SIZE); > + } > +} > + > +static struct device_attribute dev_attr_ms_sidewinder_macro_pad = > + __ATTR(macro_pad, S_IWUSR | S_IRUGO, > + ms_sidewinder_macro_pad_show, > + ms_sidewinder_macro_pad_store); > + > +static ssize_t ms_sidewinder_record_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct hid_device *hdev = container_of(dev, struct hid_device, dev); > + struct ms_data *sc = hid_get_drvdata(hdev); > + struct ms_sidewinder_extra *sidewinder = sc->extra; > + > + return snprintf(buf, PAGE_SIZE, "%1d\n", (sidewinder->status & 0x60) >> 5); > +} > + > +static ssize_t ms_sidewinder_record_store(struct device *dev, > + struct device_attribute *attr, char const *buf, size_t count) > +{ > + struct hid_device *hdev = container_of(dev, struct hid_device, dev); > + struct ms_data *sc = hid_get_drvdata(hdev); > + struct ms_sidewinder_extra *sidewinder = sc->extra; > + unsigned int value; > + __u8 setup; > + > + if (sscanf(buf, "%1u", &value) != 1) > + return -EINVAL; > + > + if (value < 0 || value > 2) { > + return -EINVAL; > + } else { > + setup = sidewinder->status & ~(0xe0); /* Clear Record LED */ > + if (value) > + setup |= 0x10 << value; > + ms_sidewinder_control(hdev, setup); > + return strnlen(buf, PAGE_SIZE); > + } > +} > + > +static struct device_attribute dev_attr_ms_sidewinder_record = > + __ATTR(record_led, S_IWUSR | S_IRUGO, > + ms_sidewinder_record_show, > + ms_sidewinder_record_store); > + > +static ssize_t ms_sidewinder_auto_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct hid_device *hdev = container_of(dev, struct hid_device, dev); > + struct ms_data *sc = hid_get_drvdata(hdev); > + struct ms_sidewinder_extra *sidewinder = sc->extra; > + > + return snprintf(buf, PAGE_SIZE, "%1d\n", (sidewinder->status & 0x02) >> 1); > +} > + > +static ssize_t ms_sidewinder_auto_store(struct device *dev, > + struct device_attribute *attr, char const *buf, size_t count) > +{ > + struct hid_device *hdev = container_of(dev, struct hid_device, dev); > + struct ms_data *sc = hid_get_drvdata(hdev); > + struct ms_sidewinder_extra *sidewinder = sc->extra; > + unsigned int value; > + __u8 setup; > + > + if (sscanf(buf, "%1d", &value) != 1) > + return -EINVAL; > + > + if (value < 0 || value > 1) { > + return -EINVAL; > + } else { > + setup = sidewinder->status & ~(0x02); /* Clear Auto LED */ > + if (value) > + setup |= 0x02; > + ms_sidewinder_control(hdev, setup); > + return strnlen(buf, PAGE_SIZE); > + } > +} > + > +static struct device_attribute dev_attr_ms_sidewinder_auto = > + __ATTR(auto_led, S_IWUSR | S_IRUGO, > + ms_sidewinder_auto_show, > + ms_sidewinder_auto_store); > + > +static struct attribute *ms_attributes[] = { > + &dev_attr_ms_sidewinder_key_mask.attr, > + &dev_attr_ms_sidewinder_profile.attr, > + &dev_attr_ms_sidewinder_macro_pad.attr, > + &dev_attr_ms_sidewinder_record.attr, > + &dev_attr_ms_sidewinder_auto.attr, > + NULL > +}; > + > +static const struct attribute_group ms_attr_group = { > + .attrs = ms_attributes, > +}; > + > static int ms_input_mapping(struct hid_device *hdev, struct hid_input *hi, > struct hid_field *field, struct hid_usage *usage, > unsigned long **bit, int *max) > @@ -357,6 +572,13 @@ static int ms_probe(struct hid_device *hdev, const struct hid_device_id *id) > return -ENOMEM; > } > sc->extra = sidewinder; > + > + /* Create sysfs files for the Consumer Control Device only */ > + if (hdev->type == 2) { type is an enum, so using HID_TYPE_USBNONE makes more sense. > + if (sysfs_create_group(&hdev->dev.kobj, &ms_attr_group)) { > + hid_warn(hdev, "Could not create sysfs group\n"); > + } > + } > } > > ret = hid_parse(hdev); > @@ -377,6 +599,14 @@ err_free: > return ret; > } > > +static void ms_remove(struct hid_device *hdev) > +{ > + sysfs_remove_group(&hdev->dev.kobj, > + &ms_attr_group); I guess this should be protected by some "if". Regarding the rest of the sysfs code. I did not went too deeper, but it on overall looks good to me. However, I am a little bit concerned by the API you are providing. We have already a LED API (have a look at hid-sony.c for instance) which seems to be more commonly used. It may not gives all of the features you need, but having a specific API for only two keyboards seems too much for me. Also, If I read correctly, this patch set does not provide a way to use the macro keys: they are just stored and can be polled through sysfs (which I don't like). I have no idea how the other keyboards deal with macro keys, but this implementation seems not enough for me. Cheers, Benjamin > + > + hid_hw_stop(hdev); > +} > + > static const struct hid_device_id ms_devices[] = { > { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_SIDEWINDER_GV), > .driver_data = MS_HIDINPUT }, > @@ -419,6 +649,7 @@ static struct hid_driver ms_driver = { > .input_mapped = ms_input_mapped, > .event = ms_event, > .probe = ms_probe, > + .remove = ms_remove, > }; > module_hid_driver(ms_driver); > > -- > 1.9.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/