Dne St 3. března 2010 06:13:12 Pavel Machek napsal(a):
> Hi!
>
> > >> +/*
> > >> + * 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.
> >
> > I could make it a config option.
>
> You get about 10 complains already, but you are not willing to solve
> it. No, config option does not help.
>
> If you want to, make it separate patch so we can reject it separately.
>
> > >> +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?
> >
> > I looked at the backlight class and it didn't seem to be a very good fit.
> >
> > /* This structure defines all the properties of a backlight */
> > struct backlight_properties {
> >
> > /* Current User requested brightness (0 - max_brightness) */
> > int brightness;
> > /* Maximal value for brightness (read-only) */
> > int max_brightness;
> > /* Current FB Power mode (0: full on, 1..3: power saving
> >
> > modes; 4: full off), see FB_BLANK_XXX */
> >
> > int power;
> > /* FB Blanking active? (values as for power) */
> > /* Due to be removed, please use (state & BL_CORE_FBBLANK) */
> > int fb_blank;
> > /* Flags used to signal drivers of state changes */
> > /* Upper 4 bits are reserved for driver internal use */
> > unsigned int state;
> >
> > };
> >
> > The g13's backlight doesn't support any of these properties. All you can
> > set is an rgb value on the g13; no power modes, no brightness, etc. I
> > could add a brightness attribute by scaling the rgb values, but I would
> > prefer to do that from userspace rather than the driver since it's not
> > supported in the device itself.
> >
> > And, we'd still have to have the rgb attribute since the backlight class
> > doesn't have color.
>
> Or you could pretend you have 3 different backlights? ....which is
> actually what you have...?
> Pavel
Any updates on this?
btw. it'd be good if we could use the g13fb on G15 ... would that be possible?
Cheers!
On Sat, 19 Jun 2010, Marek Vasut wrote:
> > > >> +/*
> > > >> + * 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.
> > >
> > > I could make it a config option.
> >
> > You get about 10 complains already, but you are not willing to solve
> > it. No, config option does not help.
> >
> > If you want to, make it separate patch so we can reject it separately.
> >
> > > >> +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?
> > >
> > > I looked at the backlight class and it didn't seem to be a very good fit.
> > >
> > > /* This structure defines all the properties of a backlight */
> > > struct backlight_properties {
> > >
> > > /* Current User requested brightness (0 - max_brightness) */
> > > int brightness;
> > > /* Maximal value for brightness (read-only) */
> > > int max_brightness;
> > > /* Current FB Power mode (0: full on, 1..3: power saving
> > >
> > > modes; 4: full off), see FB_BLANK_XXX */
> > >
> > > int power;
> > > /* FB Blanking active? (values as for power) */
> > > /* Due to be removed, please use (state & BL_CORE_FBBLANK) */
> > > int fb_blank;
> > > /* Flags used to signal drivers of state changes */
> > > /* Upper 4 bits are reserved for driver internal use */
> > > unsigned int state;
> > >
> > > };
> > >
> > > The g13's backlight doesn't support any of these properties. All you can
> > > set is an rgb value on the g13; no power modes, no brightness, etc. I
> > > could add a brightness attribute by scaling the rgb values, but I would
> > > prefer to do that from userspace rather than the driver since it's not
> > > supported in the device itself.
> > >
> > > And, we'd still have to have the rgb attribute since the backlight class
> > > doesn't have color.
> >
> > Or you could pretend you have 3 different backlights? ....which is
> > actually what you have...?
> > Pavel
>
> Any updates on this?
>
> btw. it'd be good if we could use the g13fb on G15 ... would that be possible?
> Cheers!
Rick, please correct me if I am wrong, but as far as I remember, you have
gathered quite some feedback and were about to incorporate them and
re-submit the driver, which hasn't happened yet. Is that correct? Or have
I lost track completely?
Thanks,
--
Jiri Kosina
SUSE Labs, Novell Inc.
Jiri Kosina wrote:
> On Sat, 19 Jun 2010, Marek Vasut wrote:
>
>> > > >> +/*
>> > > >> + * 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.
>> > >
>> > > I could make it a config option.
>> >
>> > You get about 10 complains already, but you are not willing to solve
>> > it. No, config option does not help.
>> >
>> > If you want to, make it separate patch so we can reject it separately.
>> >
>> > > >> +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?
>> > >
>> > > I looked at the backlight class and it didn't seem to be a very good
>> fit.
>> > >
>> > > /* This structure defines all the properties of a backlight */
>> > > struct backlight_properties {
>> > >
>> > > /* Current User requested brightness (0 - max_brightness) */
>> > > int brightness;
>> > > /* Maximal value for brightness (read-only) */
>> > > int max_brightness;
>> > > /* Current FB Power mode (0: full on, 1..3: power saving
>> > >
>> > > modes; 4: full off), see FB_BLANK_XXX */
>> > >
>> > > int power;
>> > > /* FB Blanking active? (values as for power) */
>> > > /* Due to be removed, please use (state & BL_CORE_FBBLANK) */
>> > > int fb_blank;
>> > > /* Flags used to signal drivers of state changes */
>> > > /* Upper 4 bits are reserved for driver internal use */
>> > > unsigned int state;
>> > >
>> > > };
>> > >
>> > > The g13's backlight doesn't support any of these properties. All you
>> can
>> > > set is an rgb value on the g13; no power modes, no brightness, etc.
>> I
>> > > could add a brightness attribute by scaling the rgb values, but I
>> would
>> > > prefer to do that from userspace rather than the driver since it's
>> not
>> > > supported in the device itself.
>> > >
>> > > And, we'd still have to have the rgb attribute since the backlight
>> class
>> > > doesn't have color.
>> >
>> > Or you could pretend you have 3 different backlights? ....which is
>> > actually what you have...?
>> > Pavel
>>
>> Any updates on this?
>>
>> btw. it'd be good if we could use the g13fb on G15 ... would that be
>> possible?
>> Cheers!
>
> Rick, please correct me if I am wrong, but as far as I remember, you have
> gathered quite some feedback and were about to incorporate them and
> re-submit the driver, which hasn't happened yet. Is that correct? Or have
> I lost track completely?
>
I've been waiting to resubmit until I get some of the userspace libraries
and utilities a little more mature. There were some things I was exposing
to sysfs that I didn't absolutely need, and some others that I needed to
add.
The cairo and papyrus libraries are (preliminarily) fully working. Once I
make sure I have everything the daemon will need I'll resubmit the driver,
and once that is done I was planning on submitting the libs/utils to
Fedora.
For now, I've been dropping Fedora 12/13 patches and kernel SRPMS here:
http://miskatonic.cs.nmsu.edu/g13/
And the userspace libraries/utilities are at:
http://g13.sourceforge.net
As for the g13fb on the G15... I would imagine it could be done, but I
don't have a G15 so I can't say for sure.
If I had to guess I think you'll have to take the core functionality of
the keyboard driver and extend it with the fb code in the _same_ driver if
the keyboard and display use a similar design to the G13.
The G13 uses the same endpoint for both keyboard input and display
messages, and since a USB endpoint can't be shared across drivers both
will have to reside in the same driver.
--
Rick
Hi Rick,
Please check how your driver behaves if the framebuffer is still in use
(open and/or mmapped) by some userspace process when you unplug the
keyboard.
Probably you will get hit be the same complaints from kernel I was seeing
with PicoLCD driver (see my patch http://lkml.org/lkml/2010/5/30/15 that
is expected to fix it on PicoLCD side, the refcounting part of the patch)
Bruno
On Tue, 22 June 2010 "Rick L. Vinyard, Jr." <[email protected]> wrote:
> I've been waiting to resubmit until I get some of the userspace libraries
> and utilities a little more mature. There were some things I was exposing
> to sysfs that I didn't absolutely need, and some others that I needed to
> add.
>
> The cairo and papyrus libraries are (preliminarily) fully working. Once I
> make sure I have everything the daemon will need I'll resubmit the driver,
> and once that is done I was planning on submitting the libs/utils to
> Fedora.
>
> For now, I've been dropping Fedora 12/13 patches and kernel SRPMS here:
> http://miskatonic.cs.nmsu.edu/g13/
>
> And the userspace libraries/utilities are at:
> http://g13.sourceforge.net
>
> As for the g13fb on the G15... I would imagine it could be done, but I
> don't have a G15 so I can't say for sure.
>
> If I had to guess I think you'll have to take the core functionality of
> the keyboard driver and extend it with the fb code in the _same_ driver if
> the keyboard and display use a similar design to the G13.
>
> The G13 uses the same endpoint for both keyboard input and display
> messages, and since a USB endpoint can't be shared across drivers both
> will have to reside in the same driver.
>
> --
>
> Rick
>
On Wed, 23 Jun 2010, Bruno Pr?mont wrote:
> Please check how your driver behaves if the framebuffer is still in use
> (open and/or mmapped) by some userspace process when you unplug the
> keyboard.
>
> Probably you will get hit be the same complaints from kernel I was seeing
> with PicoLCD driver (see my patch http://lkml.org/lkml/2010/5/30/15 that
> is expected to fix it on PicoLCD side, the refcounting part of the patch)
BTW I'd like to have this fixed, but I haven't seen any Acks from
framebuffer people, which I'd really like to have before taking it through
my tree.
Thanks,
--
Jiri Kosina
SUSE Labs, Novell Inc.
> On Tue, 22 June 2010 "Rick L. Vinyard, Jr." <[email protected]> wrote:
>> I've been waiting to resubmit until I get some of the userspace
>> libraries
>> and utilities a little more mature. There were some things I was
>> exposing
>> to sysfs that I didn't absolutely need, and some others that I needed to
>> add.
>>
>> The cairo and papyrus libraries are (preliminarily) fully working. Once
>> I
>> make sure I have everything the daemon will need I'll resubmit the
>> driver,
>> and once that is done I was planning on submitting the libs/utils to
>> Fedora.
>>
>> For now, I've been dropping Fedora 12/13 patches and kernel SRPMS here:
>> http://miskatonic.cs.nmsu.edu/g13/
>>
>> And the userspace libraries/utilities are at:
>> http://g13.sourceforge.net
>>
>> As for the g13fb on the G15... I would imagine it could be done, but I
>> don't have a G15 so I can't say for sure.
>>
>> If I had to guess I think you'll have to take the core functionality of
>> the keyboard driver and extend it with the fb code in the _same_ driver
>> if
>> the keyboard and display use a similar design to the G13.
>>
>> The G13 uses the same endpoint for both keyboard input and display
>> messages, and since a USB endpoint can't be shared across drivers both
>> will have to reside in the same driver.
>>
> Bruno Prémont wrote:
> Hi Rick,
>
> Please check how your driver behaves if the framebuffer is still in use
> (open and/or mmapped) by some userspace process when you unplug the
> keyboard.
>
> Probably you will get hit be the same complaints from kernel I was seeing
> with PicoLCD driver (see my patch http://lkml.org/lkml/2010/5/30/15 that
> is expected to fix it on PicoLCD side, the refcounting part of the patch)
>
Will do. Thanks for the heads up.