Re: [PATCH] input:Misc changes against kxtj9 v5 patch

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

On Wed, Jul 06, 2011 at 11:58:41AM -0400, Christopher Hudson wrote:
> On Wed, Jul 6, 2011 at 11:42 AM, Christopher Hudson
> <chris.hudson.comp.eng@gmail.com> wrote:
> > On Tue, Jul 5, 2011 at 4:35 PM, Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> >> On Tue, Jul 05, 2011 at 04:19:32PM -0400, Christopher Hudson wrote:
> >>> On Tue, Jul 5, 2011 at 4:05 PM, Dmitry Torokhov
> >>> <dmitry.torokhov@gmail.com> wrote:
> >>> > On Tue, Jul 05, 2011 at 03:55:54PM -0400, chris.hudson.comp.eng@gmail.com wrote:
> >>> >> From: Chris Hudson <chudson@kionix.com>
> >>> >>
> >>> >> As requested, miscellaneous changes originally suggested by Jonathan Cameron,
> >>> >> patched against the original kxtj9 v5 patch.
> >>> >>
> >>> >> Signed-off-by: Chris Hudson <chudson@kionix.com>
> >>> >> ---
> >>> >>  drivers/input/misc/kxtj9.c  |   38 +++++++++++++-------------------------
> >>> >>  include/linux/input/kxtj9.h |   10 +---------
> >>> >>  2 files changed, 14 insertions(+), 34 deletions(-)
> >>> >>
> >>> >> diff --git a/drivers/input/misc/kxtj9.c b/drivers/input/misc/kxtj9.c
> >>> >> index 7faf155..b2da841 100644
> >>> >> --- a/drivers/input/misc/kxtj9.c
> >>> >> +++ b/drivers/input/misc/kxtj9.c
> >>> >> @@ -71,6 +71,15 @@ static const struct {
> >>> >>       { 0,    ODR12_5F},
> >>> >>  };
> >>> >>
> >>> >> +static const struct {
> >>> >> +     u8 g_range;
> >>> >> +     u8 shift;
> >>> >> +} kxtj9_range[] = {
> >>> >> +     { 0,            4 },    /* +/-2g */
> >>> >> +     { (1 << 3),     3 },    /* +/-4g */
> >>> >> +     { (1 << 4),     2 },    /* +/-8g */
> >>> >> +};
> >>> >> +
> >>> >>  struct kxtj9_data {
> >>> >>       struct i2c_client *client;
> >>> >>       struct kxtj9_platform_data pdata;
> >>> >> @@ -129,25 +138,9 @@ static void kxtj9_report_acceleration_data(struct kxtj9_data *tj9)
> >>> >>
> >>> >>  static int kxtj9_update_g_range(struct kxtj9_data *tj9, u8 new_g_range)
> >>> >>  {
> >>> >> -     switch (new_g_range) {
> >>> >> -     case KXTJ9_G_2G:
> >>> >> -             tj9->shift = SHIFT_ADJ_2G;
> >>> >> -             break;
> >>> >> -
> >>> >> -     case KXTJ9_G_4G:
> >>> >> -             tj9->shift = SHIFT_ADJ_4G;
> >>> >> -             break;
> >>> >> -
> >>> >> -     case KXTJ9_G_8G:
> >>> >> -             tj9->shift = SHIFT_ADJ_8G;
> >>> >> -             break;
> >>> >> -
> >>> >> -     default:
> >>> >> -             return -EINVAL;
> >>> >> -     }
> >>> >> -
> >>> >> +     tj9->shift = kxtj9_range[new_g_range].shift;
> >>> >
> >>> > This is not safe, you still need to sanitize input to make sure you do
> >>> > not go beyond array boundaries.
> >>>
> >>> Oops...would a simple
> >>> if (new_g_range > 2)
> >>>         return -EINVAL;
> >>> do the trick?
> >>
> >> Or "if (new_g_range >= ARRAY_SIZE(kxtj9_range)) ..."
> >>
> >> However, why do we use indices instead of values? I.e. why don't we
> >> allow users to specify 2, 4 and 8 in platform data and use array to
> >> validate the supplied value and return -EINVAL if this value is not
> >> found?
> >
> > Before we were allowing the user to specify KXTJ9_G_2G, etc, which
> > were bitmasks that represented each available g-range.  I think
> > Jonathan's intent was to optimize/clean up the update_g_range
> > function, but I just couldn't figure out the most efficient way to do
> > this.  It seems that just replacing the globally defined shift
> > variables with their numerical counterparts within the update_g_range
> > function probably gives us the best combination of efficiency and
> > ease-of-use.
> 
> My apologies I meant to include the following revised version:
> 
> Signed-off-by: Chris Hudson <chudson@kionix.com>

All folded and applied. Thanks Chris.

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