Re: [PATCH] hid Logitech G13 Driver 0.0.5

[Jaya Kumar <jayakumar.lkml-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On Wed, Mar 3, 2010 at 2:48 AM, Rick L. Vinyard Jr.
<rvinyard-qcTL/1vZYtiVc3sceRu5cw@public.gmane.org> wrote:
> [PATCH] hid Logitech G13 Driver 0.0.5

Hi Rick,

Thanks for posting this. It has improved a lot.

>
> This driver relies on two patches:
>  1. Bruno Prémont's [PATCH 2/3] hid: add suspend/resume hooks for hid drivers
>  2. [PATCH] Add sysfs support for fbdefio delay

Please help me understand in what way does this patch have to have the
global userspace exposure of defio delay in sysfs support. If that
sysfs patch was not accepted, I don't see this patch affected
negatively.

> However, the driver does use the HID feature reports extensively to
> control the backlight, LEDs, et. al.

Ok.

> - Removed the update rate sysfs attribute as it is in the second patch above.

Why? There is no consensus that exposing defio delay for _all_ fb
drivers is a good idea. I would rather you left that in here for now.

> +/*
> + * This is a default logo to be loaded upon driver initialization
> + * replacing the default Logitech G13 image loaded on device
> + * initialization. This is to provide the user a cue that the
> + * Linux driver is loaded and ready.
> + *
> + * This logo contains the text G13 in the center with two penguins
> + * on each side of the text. The penguins are a 33x40 rendition of
> + * the default framebuffer 80x80 monochrome image scaled down and
> + * cleaned up to retain something that looks a little better than
> + * a simple scaling.
> + *
> + * This logo is a simple xbm image created in GIMP and exported.
> + * To view the image copy the following two #defines plus the
> + * g13_lcd_bits to an ASCII text file and save with extension
> + * .xbm, then open with GIMP or any other graphical editor
> + * such as eog that recognizes the .xbm format.
> + */
...
> +static unsigned char g13_lcd_bits[] = {
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,

I think you'll agree above is not an elegant solution.

> +
> +static DEVICE_ATTR(rgb, 0666, g13_rgb_show, g13_rgb_store);

Reading above code, it looks like this new userspace sysfs attribute
is for backlight control. Could that be better exposed using the
existing backlight class?

Thanks,
jaya
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html