----- Original Message -----
From: "Jonathan Cameron" <[email protected]>
To: "Hemanth V" <[email protected]>
Cc: <[email protected]>; <[email protected]>;
<[email protected]>
Sent: Friday, May 21, 2010 5:27 PM
Subject: Re: [RFC] [PATCH V2 1/2] input: CMA3000 Accelerometer driver
> On 05/21/10 07:52, Hemanth V wrote:
>> From: Hemanth V <[email protected]>
>> Date: Thu, 20 May 2010 20:18:17 +0530
>> Subject: [PATCH] input: CMA3000 Accelerometer Driver
>>
>> This patch adds support for CMA3000 Tri-axis accelerometer, which
>> supports Motion detect, Measurement and Free fall modes.
>> CMA3000 supports both I2C/SPI bus for communication, currently the
>> driver supports I2C based communication.
>>
>> Driver reports acceleration data through input subsystem and supports
>> sysfs for configuration changes.
>>
>> This is V2 of patch, which fixes open source review comments
>>
>
> 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:
>
Jonathan, Andrew
Is there any way forward for this patch, I am unable to induce any response
from the input maintainer
Thanks
Hemanth
Hemanth
I have a few comments on this patch.
+static ssize_t cma3000_store_attr_mdfftmr(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct cma3000_accl_data *data = platform_get_drvdata(pdev);
+ unsigned long val;
+ int error;
+
+ error = strict_strtoul(buf, 0, &val);
+ if (error)
+ return error;
+
+ mutex_lock(&data->mutex);
+ data->pdata.mdfftmr = val;
+
+ disable_irq(data->client->irq);
You should use disable_irq_nosync here. This may not work properly on SMP.
> + if (val == CMARANGE_2G) {
> + ctrl |= CMA3000_RANGE2G;
> + data->pdata.g_range = CMARANGE_2G;
> + } else if (val == CMARANGE_8G) {
> + ctrl |= CMA3000_RANGE8G;
> + data->pdata.g_range = CMARANGE_8G;
Why are you modifying the platform data? Why not just keep it in a global or a glocal structure and modify it that way?
> + } else {
> + error = -EINVAL;
> + goto err_op_failed;
> + }
> +
> + g_range = data->pdata.g_range;
> + fuzz_x = data->pdata.fuzz_x;
> + fuzz_y = data->pdata.fuzz_y;
> + fuzz_z = data->pdata.fuzz_z;
Why are you storing these locally and then using them once can't we eliminate these completely and just pass the platform data values into the set params?
> +
> + disable_irq(data->client->irq);
You should use disable_irq_nosync here. This may not work properly on SMP.
> + cma3000_set(data, CMA3000_CTRL, ctrl, "ctrl");
> +
> + input_set_abs_params(data->input_dev, ABS_X, -g_range,
> + g_range, fuzz_x, 0);
> + input_set_abs_params(data->input_dev, ABS_Y, -g_range,
> + g_range, fuzz_y, 0);
> + input_set_abs_params(data->input_dev, ABS_Z, -g_range,
> + g_range, fuzz_z, 0);
Don't necessarily agree with modifying the parameters for the input device on the fly. Some implementations may be a read once on init and do not go back and check this.
+ ret = request_threaded_irq(data->client->irq, NULL,
+ cma3000_thread_irq,
+ irqflags | IRQF_ONESHOT,
+ data->client->name, data);
This is implemented wrong. You are doing a lot of processing in the IRQ context here. Especially calls out to a peripheral. The NULL should be your handler thread where you do all the device processing.
Also this implementation only suggests that the HW has the IRQ connected what about devices that the IRQ line was not connected?
I have changed this driver to adjust all the issues above. Let me know if you want the patches or the files.
Dan
-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of V, Hemanth
Sent: Friday, August 13, 2010 7:47 AM
To: Jonathan Cameron; Andrew Morton
Cc: [email protected]; [email protected]; [email protected]
Subject: Re: [RFC] [PATCH V2 1/2] input: CMA3000 Accelerometer driver
----- Original Message -----
From: "Jonathan Cameron" <[email protected]>
To: "Hemanth V" <[email protected]>
Cc: <[email protected]>; <[email protected]>;
<[email protected]>
Sent: Friday, May 21, 2010 5:27 PM
Subject: Re: [RFC] [PATCH V2 1/2] input: CMA3000 Accelerometer driver
> On 05/21/10 07:52, Hemanth V wrote:
>> From: Hemanth V <[email protected]>
>> Date: Thu, 20 May 2010 20:18:17 +0530
>> Subject: [PATCH] input: CMA3000 Accelerometer Driver
>>
>> This patch adds support for CMA3000 Tri-axis accelerometer, which
>> supports Motion detect, Measurement and Free fall modes.
>> CMA3000 supports both I2C/SPI bus for communication, currently the
>> driver supports I2C based communication.
>>
>> Driver reports acceleration data through input subsystem and supports
>> sysfs for configuration changes.
>>
>> This is V2 of patch, which fixes open source review comments
>>
>
> 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:
>
Jonathan, Andrew
Is there any way forward for this patch, I am unable to induce any response
from the input maintainer
Thanks
Hemanth
From: "Murphy, Dan" <[email protected]>
> Hemanth
> I have a few comments on this patch.
>
> +static ssize_t cma3000_store_attr_mdfftmr(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct cma3000_accl_data *data = platform_get_drvdata(pdev);
> + unsigned long val;
> + int error;
> +
> + error = strict_strtoul(buf, 0, &val);
> + if (error)
> + return error;
> +
> + mutex_lock(&data->mutex);
> + data->pdata.mdfftmr = val;
> +
> + disable_irq(data->client->irq);
> You should use disable_irq_nosync here. This may not work properly on SMP.
Can u explain why disable_irq will not work on SMP.
>
>> + if (val == CMARANGE_2G) {
>> + ctrl |= CMA3000_RANGE2G;
>> + data->pdata.g_range = CMARANGE_2G;
>> + } else if (val == CMARANGE_8G) {
>> + ctrl |= CMA3000_RANGE8G;
>> + data->pdata.g_range = CMARANGE_8G;
>
> Why are you modifying the platform data? Why not just keep it in a global or a glocal structure and modify it that way?
If you look carefully, the variable data is indeed a local structure.
>> + } else {
>> + error = -EINVAL;
>> + goto err_op_failed;
>> + }
>> +
>> + g_range = data->pdata.g_range;
>> + fuzz_x = data->pdata.fuzz_x;
>> + fuzz_y = data->pdata.fuzz_y;
>> + fuzz_z = data->pdata.fuzz_z;
> Why are you storing these locally and then using them once can't we eliminate these completely and just pass the platform data values into the set
> params?
I belive this is already discussed and agreed upon in the previous thread of discussion.
Pl refer the same.
>> +
>> + disable_irq(data->client->irq);
> You should use disable_irq_nosync here. This may not work properly on SMP.
>
Same comment as above
>> + cma3000_set(data, CMA3000_CTRL, ctrl, "ctrl");
>> +
>> + input_set_abs_params(data->input_dev, ABS_X, -g_range,
>> + g_range, fuzz_x, 0);
>> + input_set_abs_params(data->input_dev, ABS_Y, -g_range,
>> + g_range, fuzz_y, 0);
>> + input_set_abs_params(data->input_dev, ABS_Z, -g_range,
>> + g_range, fuzz_z, 0);
> Don't necessarily agree with modifying the parameters for the input device on the fly. Some implementations may be a read once on init and do not
> go back and check this.
Its the user space code that can modify the grange if required, so I suppose it will need to check
these values after modifying the range.
>
>
> + ret = request_threaded_irq(data->client->irq, NULL,
> + cma3000_thread_irq,
> + irqflags | IRQF_ONESHOT,
> + data->client->name, data);
>
> This is implemented wrong. You are doing a lot of processing in the IRQ context here. Especially calls out to a peripheral. The NULL should be
> your handler thread where you do all the device processing.
Are u sure u are referring to threaded irq, all the processing is being done in thread
context. Pl refer documentation for more details on threaded irqs.
>
> Also this implementation only suggests that the HW has the IRQ connected what about devices that the IRQ line was not connected?
>
This is currently not implemented, since I am not aware of any boards with this configuration. A polling method could be added
in future if the need arises.
On Fri, Aug 13, 2010 at 08:34:07AM -0500, Murphy, Dan wrote:
> Hemanth
> I have a few comments on this patch.
>
> +static ssize_t cma3000_store_attr_mdfftmr(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct cma3000_accl_data *data = platform_get_drvdata(pdev);
> + unsigned long val;
> + int error;
> +
> + error = strict_strtoul(buf, 0, &val);
> + if (error)
> + return error;
> +
> + mutex_lock(&data->mutex);
> + data->pdata.mdfftmr = val;
> +
> + disable_irq(data->client->irq);
> You should use disable_irq_nosync here. This may not work properly on SMP.
This is an incorrect statement. disable_irq() is not safe to use on SMP when
caller may not sleep (since it will wait for the running handler to
complete), but in this context it is perfectly safe,
--
Dmitry