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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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@gmail.com> wrote:
> > Hi Chris,
> >
> > On Tue, Jun 28, 2011 at 03:12:28PM -0400, chris.hudson.comp.eng@gmail.com wrote:
> >> From: Chris Hudson <chudson@kionix.com>
> >>
> >> - 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html