Re: [PATCH v2 2/6] hid: add framebuffer support to PicoLCD device

[Jaya Kumar <jayakumar.lkml@gmail.com>]

Hi Bruno,

On Sun, Mar 21, 2010 at 12:04 AM, Bruno Prémont
<bonbons@linux-vserver.org> 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.

>
> 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.

>
> 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.

> +/* 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?

> +       /*
> +        * 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?

Thanks,
jaya