Hi, All.
Thanks for the remarks on my previous post.
Here is the driver for HP Mobile Data Protection System with the following
changes:
1) Fixed tabs from 8 spaces to tabs
2) Removed C++ style comments
3) Made some functions return void if their return value is not checked
anyway
4) Moved the interface from proc to sysfs
5) Made the sysfs interface similar to hdaps's so that hdapsd could be used
perhaps with small modifications
I would like to hear your remarks on this version.
Yan Burman
[email protected]
_________________________________________________________________
Express yourself instantly with MSN Messenger! Download today it's FREE!
http://messenger.msn.click-url.com/go/onm00200471ave/direct/01/
On Fri 13-10-06 15:10:29, Burman Yan wrote:
> Hi, All.
>
> Thanks for the remarks on my previous post.
> Here is the driver for HP Mobile Data Protection System
> with the following changes:
> 1) Fixed tabs from 8 spaces to tabs
> 2) Removed C++ style comments
> 3) Made some functions return void if their return value
> is not checked anyway
> 4) Moved the interface from proc to sysfs
> 5) Made the sysfs interface similar to hdaps's so that
> hdapsd could be used perhaps with small modifications
>
> I would like to hear your remarks on this version.
Please inline patches.
Is sysfs interface compatible to hdaps one?
--
Thanks for all the (sleeping) penguins.
>From: Pavel Machek <[email protected]>
>To: Burman Yan <[email protected]>
>CC: [email protected]
>Is sysfs interface compatible to hdaps one?
Well, pretty close:
1) calibrate and position attributes behave the same (as far as I can tell
by looking at hdaps source)
2) mdps has options to enable/disable mouse and power, but lacks variance,
temp and
{keyboard,mouse}_activity
But hdapsd only uses position and mouse/keyboard activity stuff. mouse,
keyboard activity
I can't provide for a simple reason that the chip does not give it to me
(got the spec).
But since position is in the same format, it should be relatively easy to
make hdaps work
with mdps as well.
Yan
_________________________________________________________________
Express yourself instantly with MSN Messenger! Download today it's FREE!
http://messenger.msn.click-url.com/go/onm00200471ave/direct/01/
>From: Luca Tettamanti <[email protected]>
>To: Yan Burman <[email protected]>
>CC: [email protected]
>Subject: Remn Yan <[email protected]> ha scritto:
>Date: Fri, 13 Oct 2006 18:58:40 +0200
>
>Burman Yan <[email protected]> ha scritto:
> > I would like to hear your remarks on this version.
>
>A couple of small issues:
>
> > +/* Sysfs stuff */
> > +static ssize_t mdps_position_show(struct device *dev,
> > + struct device_attribute *attr, char
>*buf)
> > +{
> > + int x, y;
> > + mdps_get_xy(mdps.device->handle, &x, &y);
> > + return sprintf(buf, "(%d, %d)\n", x, y);
> > +}
>
>hdaps doesn't have the space between the numbers.
>
Point taken and fixed. I missed that. I looked at calibrate and saw that
there are no spaces :)
> > +static ssize_t mdps_position3d_show(struct device *dev,
> > + struct device_attribute *attr, char
>*buf)
> > +{
>[...]
> > + return sprintf(buf, "(%d, %d, %d)\n", x, y, z);
> > +}
>
>I'd also remove the space here for consistency.
>Also sysfs should carry "unformatted" data, I would use:
>
>sprintf(buf, "%d %d %d\n", x, y, z);
>
Removed the spaces, but the brackets are there for consistency with the 2d
version and hdaps.
>Maybe we should take a moment to think about an interface for both hdaps
>and this one.
>
> > +static ssize_t mdps_calibrate_store(struct device *dev,
> > + struct device_attribute *attr, const char *buf,
>size_t count)
> > +{
> > + mdps_calibrate_mouse();
> > + return count;
> > +}
>
>No locking here?
>
I don't want to lock it here, since the mouse polling kthread is heavy as it
is.
I'd rather report a wrong value of mouse position while recalibrating.
The way I see it, if you're recalibrating your mouse, chances are you're not
playing at the same precise millisecond. In my opinion it's worth more
battery life.
> > +static ssize_t mdps_rate_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
>[...]
> > + return sprintf(buf, "sampling rate:\t%dHz\n", rate);
> > +}
>
>Raw value, i.e. sprintf(buf, "%d\n", rate).
>
>
Point taken and fixed.
I also found that I missed a comment in Kconfig after switching from proc to
sysfs, but
I'll post the corrected version after I have more fixes to it.
Thanks for noticing.
Yan
_________________________________________________________________
Express yourself instantly with MSN Messenger! Download today it's FREE!
http://messenger.msn.click-url.com/go/onm00200471ave/direct/01/
On Fri 2006-10-13 20:32:48, Burman Yan wrote:
> >From: Luca Tettamanti <[email protected]>
> >Burman Yan <[email protected]> ha scritto:
> >> I would like to hear your remarks on this version.
> >
> >> +static ssize_t mdps_calibrate_store(struct device *dev,
> >> + struct device_attribute *attr, const char *buf,
> >size_t count)
> >> +{
> >> + mdps_calibrate_mouse();
> >> + return count;
> >> +}
> >
> >No locking here?
>
> I don't want to lock it here, since the mouse polling kthread is heavy as
> it is.
> I'd rather report a wrong value of mouse position while recalibrating.
> The way I see it, if you're recalibrating your mouse, chances are you're
> not playing at the same precise millisecond. In my opinion it's worth more
> battery life.
Hmm, and are you sure it can't oops or something?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
>From: Pavel Machek <[email protected]>
>To: Burman Yan <[email protected]>
>CC: [email protected], [email protected]
>Subject: HP disk protection Re: Remn Yan <[email protected]> ha scritto:
>Date: Sat, 14 Oct 2006 10:26:08 +0200
>
>On Fri 2006-10-13 20:32:48, Burman Yan wrote:
> > >From: Luca Tettamanti <[email protected]>
>
> > >Burman Yan <[email protected]> ha scritto:
> > >> I would like to hear your remarks on this version.
> > >
> > >> +static ssize_t mdps_calibrate_store(struct device *dev,
> > >> + struct device_attribute *attr, const char *buf,
> > >size_t count)
> > >> +{
> > >> + mdps_calibrate_mouse();
> > >> + return count;
> > >> +}
> > >
> > >No locking here?
> >
> > I don't want to lock it here, since the mouse polling kthread is heavy
>as
> > it is.
>
> > I'd rather report a wrong value of mouse position while recalibrating.
> > The way I see it, if you're recalibrating your mouse, chances are you're
> > not playing at the same precise millisecond. In my opinion it's worth
>more
> > battery life.
>
>Hmm, and are you sure it can't oops or something?
Well, other than in calibrate, I use it only in one other place - the mouse
polling kthread
(mdps_mouse_kthread). In there it's used to calculate the absolute location
of the "mouse".
At most I will get a value that doesn not correspond to the current
position.
This means that the mouse pointer will perhaps jump, since the x calibration
value has
updated and y calibration has not yet.
Yan
_________________________________________________________________
FREE pop-up blocking with the new MSN Toolbar - get it now!
http://toolbar.msn.click-url.com/go/onm00200415ave/direct/01/
Burman Yan <[email protected]> ha scritto:
> I would like to hear your remarks on this version.
A couple of small issues:
> +/* Sysfs stuff */
> +static ssize_t mdps_position_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + int x, y;
> + mdps_get_xy(mdps.device->handle, &x, &y);
> + return sprintf(buf, "(%d, %d)\n", x, y);
> +}
hdaps doesn't have the space between the numbers.
> +static ssize_t mdps_position3d_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
[...]
> + return sprintf(buf, "(%d, %d, %d)\n", x, y, z);
> +}
I'd also remove the space here for consistency.
Also sysfs should carry "unformatted" data, I would use:
sprintf(buf, "%d %d %d\n", x, y, z);
Maybe we should take a moment to think about an interface for both hdaps
and this one.
> +static ssize_t mdps_calibrate_store(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t count)
> +{
> + mdps_calibrate_mouse();
> + return count;
> +}
No locking here?
> +static ssize_t mdps_rate_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
[...]
> + return sprintf(buf, "sampling rate:\t%dHz\n", rate);
> +}
Raw value, i.e. sprintf(buf, "%d\n", rate).
Luca
--
Colui che sorride quando le cose vanno male ha pensato a qualcuno a cui
dare la colpa.