Re: PATCH V4 1/2] input: CMA3000 Accelerometer Driver

Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]
From: Jonathan Cameron
Date: Monday, November 29, 2010 - 4:28 am

On 11/29/10 10:57, Hemanth V wrote:

Couple of nitpicks inline.  There's one unneeded NULL assignment
that should probably be cleaned up. I'd also like to see actual
part numbers listed somewhere in the Kconfig help text.

The removal of various temporary variables is something I know
at least one other reviewer has commented on.  Basically they may
make the code look cleaner, but they add (marginally) to the
review burden and that's annoys reviewers ;)

Anyhow, all the comments are trivial. Nice driver Hemanth.

Dmitry: This is clean and for now has none of the controversial
sysfs interfaces so baring the input bits (which I'm not as
familiar with and you might want to glance over) this looks
ready to merge to me.
Acked-by: Jonathan Cameron <jic23@cam.ac.uk>

(no idea if the convention is to remove the Reviewed-by when
someone has acked or not...)
Usual convention is to ensure all known part codes are listed in the
help text if not in the short form above.  Makes it easy to grep for
a particular part.
Again (not I review from the end of patches ;)
I really don't see the point in these. They just make things
more confusing for a reviewer. Just use data->pdata.whatever
directly.
Personally I'd drop out of the registration for this. If that element
of the platform data isn't there to my mind it means that it isn't being
correctly handled by the platform data.
This one is definitely a personal view though so feel free to ignore me!
Why eat the error by replacing it with another one and why are we
only allowed an error from the last one?  
Personally I'm still against these local variables existing.
They add a layer of abstraction that doesn't do anyone any
good so I'd just used the pdata directly in the calls.
Not an important point though. Just makes anyone reviewing
the driver check to see where these all go.

Why the NULL assignment?

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

Messages in current thread:
PATCH V4 1/2] input: CMA3000 Accelerometer Driver, Hemanth V, (Mon Nov 29, 3:57 am)
Re: PATCH V4 1/2] input: CMA3000 Accelerometer Driver, Jonathan Cameron, (Mon Nov 29, 4:28 am)
Re: PATCH V4 1/2] input: CMA3000 Accelerometer Driver, Dmitry Torokhov, (Tue Nov 30, 12:59 am)
Re: PATCH V4 1/2] input: CMA3000 Accelerometer Driver, Jonathan Cameron, (Tue Nov 30, 10:05 am)