Re: [PATCH] Logitech G13 driver (fixed cc list --- ignore others)

Re: [PATCH] Logitech G13 driver (fixed cc list --- ignore others)

[Rick L. Vinyard, Jr.]

Jaya Kumar wrote:
> On Tue, Dec 15, 2009 at 5:22 AM, Rick L. Vinyard Jr.
> <rvinyard@cs.nmsu.edu> wrote:
>> Additionally, this device contains a 160x43 monochrome LCD display.
>> A registered framebuffer device manages this display. The design
>> of this portion of the driver was based on the design of the
>> hecubafb driver with deferred framebuffer I/O since there is
>> no real memory to map.
>
> Hi Rick,
>
> Interesting work. I recommend CCing linux-fbdev@vger.kernel.org too
> since it contains a fbdev interface.
>

Thanks. Added.

>> +config LOGITECH_G13
>> +       tristate "Logitech G13 gameboard support"
>> +       depends on HID_LOGITECH
>> +       depends on FB
>> +       select FB_SYS_FILLRECT
>> +       select FB_SYS_COPYAREA
>> +       select FB_SYS_IMAGEBLIT
>> +       select FB_SYS_FOPS
>
> Does this need to select FB_DEFERRED_IO?
>

Added.

>> --- /dev/null
>> +++ b/drivers/hid/hid-g13-logo.xbm
>> @@ -0,0 +1,75 @@
>> +#define g13_lcd_width 160
>> +#define g13_lcd_height 43
>> +static unsigned char g13_lcd_bits[] = {
>> +   0x00, 0x00, 0x07, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>> 0x00,
>
> Was there a reason for having a new logo file? If so, you might want
> to put it in the comments and also put it together with the standard
> kernel logo.
>

There really isn't a need for a separate file since it's pretty small. I
only separated it out to make it more visible.

I moved it inline, and resized, cleaned up, and inverted the default
penguin monochrome logo down to 40x40 from 80x80.

If anyone would like to add the 40x40 version of tux I made I'd be happy
to provide it.

>> +/* 160*43 rounded to nearest whole byte which is 160*48 since bytes are
>> +   vertical the y component must be a multiple of 8 */
>> +#define G13FB_SIZE (160*48/8)
>
> Minor nit, I think there is a macro for this, DIV_ROUND_UP, or maybe
> this could be better done in the function that uses this value.
>

It's used in several places so I'd prefer to keep the #define as I think
it makes the code more readable. As it turned out, this was a minor bug
left from an earlier iteration of the driver (before I added the
framebuffer code). The actual size need for the framebuffer is simply the
line length (160/8) * height (43).

The vbitmap of vertical bits actually transmitted still needs the rounding
plus a 32 byte offset at the beginning, resulting in a buffer of size 992.
I hard coded this into the #define and put the calculation in a comment.

>> +static ssize_t g13_set_mled(struct hid_device *hdev, unsigned mled);
>
> Does this need to be here or can the code be reordered?
>

Reordered.

>> +static struct fb_var_screeninfo g13fb_var = {
>> +       .xres = G13FB_WIDTH,
>> +       .yres = G13FB_HEIGHT,
>> +       .xres_virtual = G13FB_WIDTH,
>> +       .yres_virtual = G13FB_HEIGHT,
>> +       .bits_per_pixel = 1,
>> +       .nonstd = 1,
>> +};
>
> I think the nonstd is a bug. Yes, hecubafb and metronomefb seem to
> have the same bug as well. nonstd is used if you want something like
> FB_NONSTD_HAM or FB_NONSTD_REV_PIX_IN_B, which I don't think is what
> you want.
>

Removed.

> Hope this helps.
>
> Best regards,
> jaya
>

It definitely did. Thanks.

---

Rick

Re: [PATCH] Logitech G13 driver (fixed cc list --- ignore others)

[Giacomo A. Catenazzi]

Sorry to enter in this discussion so late.

What about g15 keyboards and related keyboards?

It would nice if your driver could handle also the other keyboards.

The package g15daemon handles such keyboards (or LCD screens):

# Logitech g11 -- extra keys, no LCD
# Logitech G15 (blue) -- extra keys and LCD
# Logitech G15 v2 (orange) -- extra keys and LCD
# Logitech Z10 -- extra keys and LCD, shared with audio, not a keyboard
# Logitech G15 Gamepanel -- extra keys and LCD

but using an daemon has it own problems, so I would like to
move the support to the kernel.

Is it ok for you?

Could you use a more generic name for configuration?
(e.g. CONFIG_LOGITECH_G_SERIES)

ciao
	cate


On 07.01.2010 16:59, Rick L. Vinyard, Jr. wrote:
> Jaya Kumar wrote:
>> On Tue, Dec 15, 2009 at 5:22 AM, Rick L. Vinyard Jr.
>> <rvinyard@cs.nmsu.edu>  wrote:
>>> Additionally, this device contains a 160x43 monochrome LCD display.
>>> A registered framebuffer device manages this display. The design
>>> of this portion of the driver was based on the design of the
>>> hecubafb driver with deferred framebuffer I/O since there is
>>> no real memory to map.
>>
>> Hi Rick,
>>
>> Interesting work. I recommend CCing linux-fbdev@vger.kernel.org too
>> since it contains a fbdev interface.
>>
>
> Thanks. Added.
>
>>> +config LOGITECH_G13
>>> +       tristate "Logitech G13 gameboard support"
>>> +       depends on HID_LOGITECH
>>> +       depends on FB
>>> +       select FB_SYS_FILLRECT
>>> +       select FB_SYS_COPYAREA
>>> +       select FB_SYS_IMAGEBLIT
>>> +       select FB_SYS_FOPS

Re: [PATCH] Logitech G13 driver (fixed cc list --- ignore others)

[Rick L. Vinyard, Jr.]

Hello,

Giacomo A. Catenazzi wrote:
> On 07.01.2010 16:59, Rick L. Vinyard, Jr. wrote:
>> Jaya Kumar wrote:
>>> On Tue, Dec 15, 2009 at 5:22 AM, Rick L. Vinyard Jr.
>>> <rvinyard@cs.nmsu.edu>  wrote:
>>>> Additionally, this device contains a 160x43 monochrome LCD display.
>>>> A registered framebuffer device manages this display. The design
>>>> of this portion of the driver was based on the design of the
>>>> hecubafb driver with deferred framebuffer I/O since there is
>>>> no real memory to map.
>>>
>>> Hi Rick,
>>>
>>> Interesting work. I recommend CCing linux-fbdev@vger.kernel.org too
>>> since it contains a fbdev interface.
>>>
>>
>> Thanks. Added.
>>
>>>> +config LOGITECH_G13
>>>> +       tristate "Logitech G13 gameboard support"
>>>> +       depends on HID_LOGITECH
>>>> +       depends on FB
>>>> +       select FB_SYS_FILLRECT
>>>> +       select FB_SYS_COPYAREA
>>>> +       select FB_SYS_IMAGEBLIT
>>>> +       select FB_SYS_FOPS
>
> Sorry to enter in this discussion so late.
>
> What about g15 keyboards and related keyboards?
>
> It would nice if your driver could handle also the other keyboards.
>

I don't have one to test. Technically _I_ don't even have a g13. The two I
currently have are borrowed.

> The package g15daemon handles such keyboards (or LCD screens):
>
> # Logitech g11 -- extra keys, no LCD
> # Logitech G15 (blue) -- extra keys and LCD
> # Logitech G15 v2 (orange) -- extra keys and LCD
> # Logitech Z10 -- extra keys and LCD, shared with audio, not a keyboard
> # Logitech G15 Gamepanel -- extra keys and LCD
>
> but using an daemon has it own problems, so I would like to
> move the support to the kernel.
>

I think the ideal approach is to use a split between a userspace daemon
and the kernel driver. I've exposed a lot of the driver to userspace
through sysfs to allow a great deal of control through a userspace daemon
for the G13.

In particular I think a similar approach with the framebuffer for those
devices would be particularly beneficial. It allows things such as the
cairo library to be used to draw on the LCD which opens up the possibility
for all kinds of userspace applets.

> Is it ok for you?
>

I don't have a problem with it, but I think there might be issues;
especially if the feature reports are different.

There is similar framebuffer code that could be shared even if the usbhid
reports differ since the LCD image format is the same.

But, I don't think the framebuffer code could be completely separated
since the G13 uses the same interrupt pipe for images and key reports.
That's why the framebuffer code (as minimal as it is) is inside the hid
driver.

> Could you use a more generic name for configuration?
> (e.g. CONFIG_LOGITECH_G_SERIES)
>

I don't have a problem with it, but my gut feeling is that they will be
separate drivers. So perhaps a menu option for the G series with the G
series drivers under it???

---

Rick