Re: [PATCH v5] input: Initial support for Kionix KXTJ9 accelerometer

Hi Dmitry,

I found a small bug in this driver when using the code as the basis
for testing another piece of hardware...I'm not sure how this got
through before:

+static int __devinit kxtj9_setup_input_device(struct kxtj9_data *tj9)
+{
+       struct input_dev *input_dev;
+       int err;
+
+       input_dev = input_allocate_device();
+       if (!tj9->input_dev) {
Should be:

if (!input_dev) {

if (!tj9->input_dev) always returns true in this context, thereby
always following the error path; you can see that nothing is assigned
to tj9->input_dev until after this check.

+               dev_err(&tj9->client->dev, "input device allocate failed\n");
+               return -ENOMEM;
+       }
+
+       tj9->input_dev = input_dev;

Should I submit a small patch against your queued version to get this fixed?

On Tue, Jul 5, 2011 at 2:39 PM, Dmitry Torokhov
[off-list ref] wrote:

On Tue, Jul 05, 2011 at 02:08:01PM -0400, Christopher Hudson wrote:

quoted

On Mon, Jul 4, 2011 at 12:26 PM, Dmitry Torokhov
[off-list ref] wrote:

quoted

Hi Chris,

On Tue, Jun 28, 2011 at 03:12:28PM -0400, chris.hudson.comp.eng@gmail.com wrote:

quoted

From: Chris Hudson <redacted>

- Added Dmitry's changes
- Now using input_polled_dev interface when in polling mode
- IRQ mode provides a separate sysfs node to change the hardware data rate

I am not happy with the fact that this implementation forces users to
select polled or interrupt-driven mode at compile time. The patch I
proposed had polled mode support optional and would automatically select
IRQ mode for clients that have interrupt assigned and try to activate
polled mode if interrupt is not available.

quoted

+
+/* kxtj9_set_poll: allows the user to select a new poll interval (in ms) */
+static ssize_t kxtj9_set_poll(struct device *dev, struct device_attribute *attr,
+                                             const char *buf, size_t count)
+{
+     struct kxtj9_data *tj9 = i2c_get_clientdata(to_i2c_client(dev));
+
+     unsigned long interval;
+     int ret = kstrtoul(buf, 10, &interval);
+     if (ret < 0)
+             return ret;
+
+     mutex_lock(&tj9->lock);
+     /* set current interval to the greater of the minimum interval or
+     the requested interval */
+     tj9->last_poll_interval = max((int)interval, tj9->pdata.min_interval);
+     mutex_unlock(&tj9->lock);

This lock does not make sense. You are protecting update of last_poll_interval
field and this update can not be partial (i.e. only LSB or MSB is
written) on all our arches, but you do not protect kxtj9_update_odr()
which alters chip state and does need protection.

I tried bringing forward my changes from the older patch into yours,
please let me know if the patch below works on top of yours.

Hi Dmitry,
Thanks for all your hard work, and yes it works fine.  There were a
couple of changes proposed by Jonathan that I had already merged into
my version though; should I submit these in a separate patch or resend
the merged version?

Cris,

Could you please send the changes proposed by Jonathan as a separate
patch relative to the v5 you posted earlier? Then I can fold v5, mine
and your new patch together and queue the driver for 3.1.

Thanks.

--
Dmitry

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