On Sun, 21 March 2010 Jaya Kumar [off-list ref] wrote:

quoted

Add framebuffer support to PicoLCD device with use of deferred-io.

Only changed areas of framebuffer get sent to device in order to
save USB bandwidth and especially resources on PicoLCD device or
allow higher refresh rate for a small area.

Interesting work. One minor comment, defio doesn't currently guarantee
that it is "changed areas". Just that it is "written" pages which
typically equates to "changed" but does not guarantee this.

When copying from framebuffer to shadow buffer I check if any given tile
has changed and only send that tile to the device if it has effectively
changed. So this check is done independently of defio.
(though I have to force-fully transfer the whole framebuffer at probe
or after reset - which is missing)

quoted

Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
---
 drivers/hid/Kconfig       |    7 +-
 drivers/hid/hid-picolcd.c |  454 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 460 insertions(+), 1 deletions(-)

It is customary for framebuffer drivers to live in drivers/video. This
is the first one I've reviewed that is outside of it. Is there a good
reason for this one to be outside of it? If so, could you put it in
the comments.

I've kept all the pieces of PicoLCD driver together under hid,
as it's a HID based device. Framebuffer is just one of its features and
keeping pieces together makes it easier to handle.

quoted

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 7097f0a..a474bcd 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig

@@ -230,6 +230,11 @@ config HID_PETALYNX

 config HID_PICOLCD
       tristate "PicoLCD (graphic version)"
       depends on USB_HID
+       select FB_DEFERRED_IO if FB
+       select FB_SYS_FILLRECT if FB
+       select FB_SYS_COPYAREA if FB
+       select FB_SYS_IMAGEBLIT if FB
+       select FB_SYS_FOPS if FB

I think all of that "if FB" stuff looks odd, it would disappear if it
were in the right Kconfig.

In the initial patch I did select FB as well though for v2 I decided
to make all the dependencies of non-input features optional. For the FB
case the various helper entries are needed, for the rest it's sufficient
to have #ifdef's for their device class.

Another option would be to distribute the Kconfig entries over the
various device class's Kconfig files though I don't think that would
make things easier to understand...

quoted

+/* Framebuffer visual structures */
+static const struct fb_fix_screeninfo picolcdfb_fix = {
+       .id          = PICOLCDFB_NAME,
+       .type        = FB_TYPE_PACKED_PIXELS,
+       .visual      = FB_VISUAL_MONO01,

Interesting choice. Out of curiosity, which fb client application are
you testing/using this with?

For now I've just been testing with manual read/write to /dev/fb. I've
not yet played with fb applications.


I've had a look at fb applications, seems that none wants to play with
individual bits, they all bail out requesting 8 or more bpp (some
fail politely, others just exit/segfault and don't undo changes they did)

So I guess I will have to add support for translating 8bpp to device's
1bpp to be able to use any existing fb application and switching between
1bpp and 8bpp framebuffer...

quoted

+       /*
+        * Translate the XBM format screen_base into the format needed by the
+        * PicoLCD. See display layout above.
+        * Do this one tile after the other and push those tiles that changed.
+        */

I think screen_base is in standard fb format, which you've specified
as MONO01 above. When you say, XBM format, in the above comment is it
exactly the same?

It should be the same (from what I read MONO01 versus MONO10 is just
whether 0 or 1 is "black" or "white") and in combination with
PACKED_PIXELS it makes 8 pixels per bytes.

Thanks for the review,
Bruno