From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bruno =?UTF-8?B?UHLDqW1vbnQ=?= Date: Sun, 21 Mar 2010 16:11:28 +0000 Subject: Re: [PATCH v2 2/6] hid: add framebuffer support to PicoLCD device Message-Id: <20100321171128.324a606f@neptune.home> List-Id: References: <20100320170014.440959a8@neptune.home> <20100320170415.6ee219c8@neptune.home> <45a44e481003210024l2c66938fp9436d34f6473f811@mail.gmail.com> In-Reply-To: <45a44e481003210024l2c66938fp9436d34f6473f811@mail.gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: Jaya Kumar Cc: Jiri Kosina , linux-input@vger.kernel.org, linux-usb@vger.kernel.org, linux-fbdev@vger.kernel.org, linux-kernel@vger.kernel.org, "Rick L. Vinyard Jr." , Nicu Pavel , Oliver Neukum On Sun, 21 March 2010 Jaya Kumar wrote: > > 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) > > Signed-off-by: Bruno Prémont > > --- > >  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. > > 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... > > +/* 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... > > +       /* > > +        * 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