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

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

[Jiejing.Zhang]

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?


>
> 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

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

[Jonathan Cameron]

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
>