RE: [PATCH] lis3lv02d: Add STMicroelectronics lis33ldlh digital

[AnilKumar, Chimata]

Hi Arnd,

On Thu, Jan 19, 2012 at 22:40:45, Arnd Bergmann wrote:
> On Thursday 19 January 2012, AnilKumar, Chimata wrote:
> > Android userspace running on TI AM335x EVM is using the interface
> > provided by lis3lv02d. They were asking some more interfaces from
> > lis3lvo2d driver.
> > 
> > There are multiple ways we can interface accelerometer to Android layers,
> > which is implemented on hardware abstraction layer (HAL) in Andriod.
> > 
> > 1) Interrupt mode
> > 2) Polling mode
> >    2a) Kernel polling
> >    2b) Timer polling
> > 
> > Based on the interfaces provided by the lis3lv02d as well as
> > lis331dlh (H/W not supporting the interrupts) they were implementing
> > the kernel polling mechanism.
> > 
> > So implementation on HAL is like this if accelerometer interface is
> > opened then kernel will start polling this driver periodically and
> > pass events to input subsystem. (It's a little bit over head)
> > 
> > Generally the device should be open but kernel should only poll
> > when an app that uses accelerometer is started.
> > 
> > The biggest requirement for them (Andriod people) is to allow user to
> > enable / disable accelerometer from user space and to configure
> > the accelerometer polling frequency.
> > 
> > Today there is no option in lis3lvo2d driver to provide this kind
> > of functionalities
> 
> Hi AnilKumar,
> 
> This all sounds like the interface is not completely thought through.
> 
> I did not realize that the driver actually uses the input subsystem
> in addition to its own interfaces. This is definitely good, and it
> means that we can move the files from drivers/misc to drivers/input/misc
> or drivers/input/accelerometer, making it Dmitry's problem instead of
> mine ;-)
> 
> Having custom user interfaces inside an input driver however is very
> bad. I'm sure that other accelerometers will have the same requirements
> regarding polling frequency and enable/disable in android as well as
> anytwhere else and it should absolutely not be handled by a user space
> HAL but instead inside of the kernel, using a common method for all
> available drivers.
> 
> Based on that, I also doubt we should apply your patch to add the
> "range" attribute (adding support for lis33ldlh is fine though).
>
> Instead I would ask you to first fix what's there by moving the
> user space interfaces into the input core from the driver
> and documenting them.
>
> I don't know what the preferred way for doing things is there
> (joystick ioctl, sysfs attribute, ...) but Dmitry should
> be able to provide advice there. Then add interfaces for the
> additional stuff you need (range, disable, ...) in the same
> place and implement them as callbacks in the driver itself.

I will remove the range attribute and I will submit v2. The rest of
the patch deals with adding support for lis33ldlh and should be fairly
non-controversial. We can revisit the range attribute addition at a
later time.

Regards
AnilKumar
¢éì®&Þ~º&¶¬–+-±éÝ¥Šw®žË±Êâmébžìdz¹Þ)í…æèw*jg¬±¨¶‰šŽŠÝj/êäz¹ÞŠà2ŠÞ¨è­Ú&¢)ß«a¶Úþø®G«éh®æj:+v‰¨Šwè†Ù>Wš±êÞiÛaxPjØm¶ŸÿÃ-»+ƒùdš_