linux-kernel - Re: [PATCH 1/3] picolcd: driver for PicoLCD HID device

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20100225123214.0523a310@neptune.home>
Date:	Thu, 25 Feb 2010 12:32:14 +0100
From:	Bruno Prémont <bonbons@...ux-vserver.org>
To:	Jiri Kosina <jkosina@...e.cz>
Cc:	linux-input@...r.kernel.org, linux-usb@...r.kernel.org,
	linux-fbdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	"Rick L. Vinyard Jr." <rvinyard@...nmsu.edu>,
	Nicu Pavel <npavel@...ner.com>
Subject: Re: [PATCH 1/3] picolcd: driver for PicoLCD HID device

On Thu, 25 February 2010 Jiri Kosina <jkosina@...e.cz> wrote:
> On Wed, 24 Feb 2010, Bruno Prémont wrote:
> 
> > +config HID_PICOLCD
> > +	tristate "Minibox PicoLCD (graphic version)"
> > +	depends on FB
> > +	select FB_DEFERRED_IO
> > +	select FB_SYS_FILLRECT
> > +	select FB_SYS_COPYAREA
> > +	select FB_SYS_IMAGEBLIT
> > +	select FB_SYS_FOPS
> > +	select BACKLIGHT_LCD_SUPPORT
> > +	select BACKLIGHT_CLASS_DEVICE
> > +	select LCD_CLASS_DEVICE
> 
> I guess you need dependency on USB_HID as well, right?

Yep, will add

> > --- a/drivers/hid/hid-picolcd.c
> > +++ b/drivers/hid/hid-picolcd.c
> > @@ -0,0 +1,1075 @@
> > +/***************************************************************************
> > + *   Copyright (C) 2010 by Bruno Prémont <bonbons@...ux-vserver.org>       *
> > + *                                                                         *
> > + *   Based on Logitech G13 driver (v0.4)                                   *
> > + *     Copyright (C) 2009 by Rick L. Vinyard, Jr. <rvinyard@...nmsu.edu>   *
> > + *                                                                         *
> > + *   This program is free software: you can redistribute it and/or modify  *
> > + *   it under the terms of the GNU General Public License as published by  *
> > + *   the Free Software Foundation, either version 2 of the License.        *
> 
> I support removing the 'or any later' clause, but I think your
> version has the grammar wrong :)

Oops, will fix

> > +/* Update fb_vbitmap from the screen_base and send changed tiles to device */
> > +static void picolcd_fb_update(struct picolcd_data *data)
> > +{
> > +	int chip, tile;
> > +
> > +	spin_lock(&data->lock);
> > +	if (!(data->status & PICOLCD_READY_FB)) {
> > +		spin_unlock(&data->lock);
> > +		picolcd_fb_reset(data->hdev, 0);
> > +	} else
> > +		spin_unlock(&data->lock);
> 
> Please put the brackets to the else branch as well.

Ok, will add the brackets while switching from spin_(un)lock to
spin_(un)lock_irq{save|restore}.

> Also, it'd be great if the framebuffer part would get Ack from
> someone more familiar with framebuffer code than me.

I would appreciate such one as well, especially regarding the
deferredio part and the more advanced fb use. For now I only tested
read/write from /dev/fbx.


For the two sysfs attributes I currently use, the 'reset' one shall
probably be moved to debugfs (I would like to place it under
/sys/kernel/debug/hid/$device/ next to rdesc and event).

By the way, I'm wondering why event does not list any of the reports
coming from my device though as I understand the code it should be
doing that before my raw_event function gets called...

The one for deferredio refresh rate should ideally go below fb device
(and I guess that might also be of use for other users of deferredio)

Thanks for the review,
Bruno
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/