Messages in this thread | | | From | "Burman Yan" <> | Subject | RE: Remn Yan <> ha scritto: | Date | Fri, 13 Oct 2006 20:32:48 +0200 |
| |
>From: Luca Tettamanti <kronos.it@gmail.com> >To: Yan Burman <yan_952@hotmail.com> >CC: linux-kernel@vger.kernel.org >Subject: Remn Yan <yan_952@hotmail.com> ha scritto: >Date: Fri, 13 Oct 2006 18:58:40 +0200 > >Burman Yan <yan_952@hotmail.com> 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/
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |