Re: [RFC] [PATCH V2 1/2] input: CMA3000 Accelerometer driver

Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]
From: Jonathan Cameron
Date: Friday, May 21, 2010 - 4:57 am

On 05/21/10 07:52, Hemanth V wrote:

Hi,

The driver is nice and clean.

Still leaving aside the long argued question of whether this should be
in input.  If you care for my views on that there are plenty or other
threads! There are a few things I'd still like to suggest:

Attribute naming:  The names you have gone with are way too short. The
	  documentation helps, but if possible it is always nice to have
	  names that are effectively human readable. 'mdfftmr' is rather
	  cryptic!   Also I know you aren't trying to write a general
	  interface here, but it would be nice if the units of these
	  magic attributes were not dependant on the current mode.
	  The snag here is that this is a user visible interface, so it
	  is rather frowned upon to change it later.  I guess you could
	  get away with adding some more attributes, say:   
	  mdfftmr_scale (which could just be a string and hence floating point)

Documentation: I'd have prefered to see the sysfs attributes documented
	       in Documentation/abi/  (but that is just a matter of personal
	       taste. I don't think there are any hard and fast rules on this
	       yet)

I'm still not keen on naming the files with a wild card. Also, if the d0x
is relevant, why is this not reflected in Kconfig?

The one thing that really bothers me is the setting of a precendent
wrt to the attribute naming.

What you have here won't generalise well, particularly wrt to controls
relating to the on chip motion and free fall detectors.  We've been round
the houses with this in IIO and I fully admit we still haven't gotten it
right there.  The advantage on this stuff that we have is that, as we
are in staging, we can mess around the userspace abi till we get it right.
In mainline things are by convention much more rigid!

Still it's a good driver and obviously some of the above issues are pretty
general.

To that end, having stated my reservations, you can add

Reviewed-by: Jonathan Cameron <jic23@cam.ac.uk>

(not ack as I'm indicating that whilst I think it is a worthwhile addition
I'm not entirely happy with some aspects of the driver).



--
Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]

Messages in current thread:
Re: [RFC] [PATCH V2 1/2] input: CMA3000 Accelerometer driver, Jonathan Cameron, (Fri May 21, 4:57 am)
Re: [RFC] [PATCH V2 1/2] input: CMA3000 Accelerometer driver, Dmitry Torokhov, (Sun Aug 29, 11:24 am)
Re: [RFC] [PATCH V2 1/2] input: CMA3000 Accelerometer driver, Dmitry Torokhov, (Sun Aug 29, 11:49 am)
Re: Sensors and the input layer (was Re: [RFC] [PATCH V2 1 ..., Jonathan Cameron, (Mon Aug 30, 10:41 am)
Re: Sensors and the input layer (was Re: [RFC] [PATCH V2 1 ..., Mohamed Ikbel Boulabiar, (Tue Aug 31, 10:24 am)
Re: Sensors and the input layer (was Re: [RFC] [PATCH V2 1 ..., Jonathan Cameron, (Tue Aug 31, 11:03 am)
Re: Sensors and the input layer (was Re: [RFC] [PATCH V2 1 ..., Jonathan Cameron, (Tue Aug 31, 11:14 am)
Re: Sensors and the input layer (was Re: [RFC] [PATCH V2 1 ..., Jonathan Cameron, (Tue Aug 31, 11:20 am)