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
<dmitry.torokhov@xxxxxxxxx> wrote:
> On Tue, Jul 05, 2011 at 02:08:01PM -0400, Christopher Hudson wrote:
>> On Mon, Jul 4, 2011 at 12:26 PM, Dmitry Torokhov
>> <dmitry.torokhov@xxxxxxxxx> wrote:
>> > Hi Chris,
>> >
>> > On Tue, Jun 28, 2011 at 03:12:28PM -0400, chris.hudson.comp.eng@xxxxxxxxx wrote:
>> >> From: Chris Hudson <chudson@xxxxxxxxxx>
>> >>
>> >> - 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.
>> >
>> >> +
>> >> +/* 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
