Re: [PATCH] hid: Logitech G13 driver 0.0.3

[Rick L. Vinyard, Jr.]

Jiri Kosina wrote:
> On Thu, 7 Jan 2010, Rick L. Vinyard, Jr. wrote:
>
>> >> +static ssize_t g13_mled_store(struct device *dev,
>> >> +                             struct device_attribute *attr,
>> >> +        const char *buf, size_t count)
>> >> +{
>> >> +       struct hid_device *hdev;
>> >> +       int i;
>> >> +       unsigned m[4];
>> >> +       unsigned mled;
>> >> +       ssize_t set_result;
>> >> +
>> >> +       /* Get the hid associated with the device */
>> >> +       hdev = container_of(dev, struct hid_device, dev);
>> >> +
>> >> +       /* If we have an invalid pointer we'll return ENODATA */
>> >> +       if (hdev == NULL || &(hdev->dev) != dev)
>> >> +               return -ENODATA;
>> >> +
>> >> +       i = sscanf(buf, "%u %u %u %u", m, m+1, m+2, m+3);
>> >> +       if (!(i == 4 || i == 1)) {
>> >> +               printk(KERN_ERR "unrecognized input: %s", buf);
>> >> +               return -1;
>> >> +       }
>> >> +
>> >> +       if (i == 1)
>> >> +               mled = m[0];
>> >> +       else
>> >> +               mled = (m[0] ? 1 : 0) | (m[1] ? 2 : 0) |
>> >> +                               (m[2] ? 4 : 0) | (m[3] ? 8 : 0);
>> >> +
>> >> +       set_result = g13_set_mled(hdev, mled);
>> >> +
>> >> +       if (set_result < 0)
>> >> +               return set_result;
>> >> +
>> >> +       return count;
>> >> +}
>> >> +
>> >> +static DEVICE_ATTR(mled, 0666, g13_mled_show, g13_mled_store);
>> >
>> > Have you considered the use of the LED class driver as an alternative
>> > to introducing these sysfs led controls for the device?
>> >
>>
>> I did, but this seemed a simpler approach to let user space (such as a
>> daemon) manage the leds. In particular this could be used by a user
>> space
>> program to map the keys. The MR led could be used to indicate an active
>> record mode, etc.
>
> I finally had some time to go through the driver as well (thanks Dmitry
> for beating me with proper review).
>
> Could you be more specific about reasons why you are hesitating to use LED
> subsystem instead of the whole mled stuff in the driver?
>
> I don't see anything that couldn't be achieved using LED class.
>

It seems to overly complicate the driver, especially as these are LEDS
inside the buttons. I don't think they will leverage the bulk of the LED
structure such as triggers, brightness, et. al.

It also seems to complicate the userspace access. A single
sysfs_get_device_attr() is replaced with looking up a unique device
identifier, creating a unique led device path, an additional
sysfs_open_device_path() and then the sysfs_get_device_attr().

But, I've added the LED class support.

However, the question I have now is that each device as it's registered
will have to be assigned a unique identifier so that each led can be
uniquely named for each device, otherwise there will be conflicts in
/sys/class/leds/.

Since the led naming scheme is "device:color:function" I was thinking of
using the minor number of the hid device and name the leds
"g13_minor#:red:m1" et. al.

But, I'd really prefer to use something that userspace can access from
sysfs to build the led device path. That way, given a udev device and a
numeric led value you could do something like:

  struct sysfs_device* leddev;
  struct sysfs_attribute* bright_attr;
  char* ledsyspath;
  char ledvalstr[4] = {0};
  ledsyspath = malloc(strlen(udev_device_get_syspath(udev_device)+22);

  *** Here you need to get the minor hid # ***

  sprintf(ledsyspath, "%s/leds/g13_%d:red:m1",
          udev_device_get_syspath(udev_device), minor);
  leddev = sysfs_open_device_path(ledsyspath);
  bright_attr = sysfs_get_device_attr(leddev, "brightness");
  sprintf(ledvalstr,"%d",ledval);
  sysfs_write_attribute(sysfsattr, ledvalstr, 4);

Any suggestions other than the minor number?

Thanks,

---

Rick


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/