Re: [PATCH] hwmod: sensors: mma845x gravity sensor chips

[Jonathan Cameron <jic23@cam.ac.uk>]

On 04/14/11 04:10, Jiejing.Zhang wrote:
> Hi Jonathan,
> 
> 2011/4/13 Jonathan Cameron <jic23@cam.ac.uk>:
>> On 04/13/11 13:33, Jean Delvare wrote:
>>> On Wed, 13 Apr 2011 19:56:53 +0800, Zhang Jiejing wrote:
>>>> This patch add basic support for mma8450 mma8451 gravity
>>>> sensor chips, and will support mma8452, mma8453 in same
>>>> driver file.
>>>>
>>>> They are i2c controller and support 3-axis gravity accelerator
>>>> sensor.
>>>>
>>>> mma8450 have some different from mma845[1,2,3] in register map, so
>>>> there are some switch case between mma8450 and others.
>>>>
>>>> Product Information can be found here:
>>>> http://www.freescale.com/webapp/sps/site/prod_summary.jsp?code=MMA8450Q
>>>> http://www.freescale.com/webapp/sps/site/prod_summary.jsp?code=MMA8451Q
>>>>
>>>> Signed-off-by: Zhang Jiejing <jiejing.zhang@freescale.com>
>>>> ---
>>>>  drivers/hwmon/Kconfig   |   10 +
>>>>  drivers/hwmon/Makefile  |    1 +
>>>>  drivers/hwmon/mma845x.c |  568 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 579 insertions(+), 0 deletions(-)
>>>>  create mode 100644 drivers/hwmon/mma845x.c
>>>
>>> Nack. Accelerometers don't belong to hwmon. Several such drivers were
>>> kicked out of drivers/hwmon recently, and we won't accept any such
>>> driver in the future. Push this to drivers/misc, drivers/input or
>>> drivers/staging/iio, whichever your prefer, but not drivers/hwmon.
>> Taking a quick look, you already have a polled input device in here
>> and it makes no use of hwmon interfaces whatsoever so input is the
>> obvious target.
>>
>> Couple of quick things I noticed whilst scan reading...
>>
>> id table contains only 8451 and 8450, get_mmax845x_name contains
>> 8452 and 8453.
>>
>> Use the names in the id table to provide you with the names rather than
>> an additional function.
>>
>> Take a look at how other drivers handle subtly different parts. There
>> are much neater ways of doing it than things like get_ctrl_register.
> Could you give a filename or example driver name ? I can use them as an example.
> 
> 
> since mma845[1-3] have same operation flow, and data byte order, but
> mma8450 is different on both register address and data byte order.
> 
> Do you think split mma8450 and mma845x (8451, 8452, 8453) into two
> file is better?
Difficult to know without analysing full code.  Right now you are only
supporting a small corner of what the chips supply, so probably depends
more on what features get added to the drivers in future..
> 
> 
>>
>> Shift it over to input and I'll do a full review.
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>