* [PATCH] input:Misc changes against kxtj9 v5 patch @ 2011-07-05 19:55 chris.hudson.comp.eng 2011-07-05 20:05 ` Dmitry Torokhov 0 siblings, 1 reply; 5+ messages in thread From: chris.hudson.comp.eng @ 2011-07-05 19:55 UTC (permalink / raw) To: linux-input; +Cc: dmitry.torokhov, jic23, Chris Hudson 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; tj9->ctrl_reg1 &= 0xe7; - tj9->ctrl_reg1 |= new_g_range; + tj9->ctrl_reg1 |= kxtj9_range[new_g_range].g_range; return 0; } @@ -181,13 +174,8 @@ static int kxtj9_update_odr(struct kxtj9_data *tj9, int poll_interval) static int kxtj9_device_power_on(struct kxtj9_data *tj9) { - int err; - - if (tj9->pdata.power_on) { - err = tj9->pdata.power_on(); - if (err < 0) - return err; - } + if (tj9->pdata.power_on) + return tj9->pdata.power_on(); return 0; } diff --git a/include/linux/input/kxtj9.h b/include/linux/input/kxtj9.h index cc5928b..6064d45 100644 --- a/include/linux/input/kxtj9.h +++ b/include/linux/input/kxtj9.h @@ -22,11 +22,6 @@ #define KXTJ9_I2C_ADDR 0x0F -/* These shift values are used to provide consistent output data */ -#define SHIFT_ADJ_2G 4 -#define SHIFT_ADJ_4G 3 -#define SHIFT_ADJ_8G 2 - #ifdef __KERNEL__ struct kxtj9_platform_data { int poll_interval; /* current poll interval (in milli-seconds) */ @@ -49,10 +44,7 @@ struct kxtj9_platform_data { #define RES_8BIT 0 #define RES_12BIT (1 << 6) u8 res_12bit; - /* Output g-range: +/-2g, 4g, or 8g */ - #define KXTJ9_G_2G 0 - #define KXTJ9_G_4G (1 << 3) - #define KXTJ9_G_8G (1 << 4) + /* Output g-range: 0 = +/-2g, 1 = 4g, or 2 = 8g */ u8 g_range; /* DATA_CTRL_REG: controls the output data rate of the part */ -- 1.5.4.3 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] input:Misc changes against kxtj9 v5 patch 2011-07-05 19:55 [PATCH] input:Misc changes against kxtj9 v5 patch chris.hudson.comp.eng @ 2011-07-05 20:05 ` Dmitry Torokhov [not found] ` <CAB7Re7XDB6Rmckxidgr+90Br5LhYk_5Pi_JQXTLd9QS2V+Vm0A@mail.gmail.com> 0 siblings, 1 reply; 5+ messages in thread From: Dmitry Torokhov @ 2011-07-05 20:05 UTC (permalink / raw) To: chris.hudson.comp.eng; +Cc: linux-input, jic23, Chris Hudson 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. -- Dmitry ^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <CAB7Re7XDB6Rmckxidgr+90Br5LhYk_5Pi_JQXTLd9QS2V+Vm0A@mail.gmail.com>]
[parent not found: <20110705203502.GC15876@core.coreip.homeip.net>]
* Re: [PATCH] input:Misc changes against kxtj9 v5 patch [not found] ` <20110705203502.GC15876@core.coreip.homeip.net> @ 2011-07-06 15:42 ` Christopher Hudson 2011-07-06 15:58 ` Christopher Hudson 0 siblings, 1 reply; 5+ messages in thread From: Christopher Hudson @ 2011-07-06 15:42 UTC (permalink / raw) To: Dmitry Torokhov, Jonathan Cameron, linux-input 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. > > -- > 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] input:Misc changes against kxtj9 v5 patch 2011-07-06 15:42 ` Christopher Hudson @ 2011-07-06 15:58 ` Christopher Hudson 2011-07-07 5:19 ` Dmitry Torokhov 0 siblings, 1 reply; 5+ messages in thread From: Christopher Hudson @ 2011-07-06 15:58 UTC (permalink / raw) To: Dmitry Torokhov, Jonathan Cameron, linux-input 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> --- drivers/input/misc/kxtj9.c | 18 +++++------------- include/linux/input/kxtj9.h | 5 ----- 2 files changed, 5 insertions(+), 18 deletions(-) diff --git a/drivers/input/misc/kxtj9.c b/drivers/input/misc/kxtj9.c index 7faf155..4cfce82 100644 --- a/drivers/input/misc/kxtj9.c +++ b/drivers/input/misc/kxtj9.c @@ -131,17 +131,14 @@ 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; + tj9->shift = 4; break; - case KXTJ9_G_4G: - tj9->shift = SHIFT_ADJ_4G; + tj9->shift = 3; break; - case KXTJ9_G_8G: - tj9->shift = SHIFT_ADJ_8G; + tj9->shift = 2; break; - default: return -EINVAL; } @@ -181,13 +178,8 @@ static int kxtj9_update_odr(struct kxtj9_data *tj9, int poll_interval) static int kxtj9_device_power_on(struct kxtj9_data *tj9) { - int err; - - if (tj9->pdata.power_on) { - err = tj9->pdata.power_on(); - if (err < 0) - return err; - } + if (tj9->pdata.power_on) + return tj9->pdata.power_on(); return 0; } diff --git a/include/linux/input/kxtj9.h b/include/linux/input/kxtj9.h index cc5928b..2575205 100644 --- a/include/linux/input/kxtj9.h +++ b/include/linux/input/kxtj9.h @@ -22,11 +22,6 @@ #define KXTJ9_I2C_ADDR 0x0F -/* These shift values are used to provide consistent output data */ -#define SHIFT_ADJ_2G 4 -#define SHIFT_ADJ_4G 3 -#define SHIFT_ADJ_8G 2 - #ifdef __KERNEL__ struct kxtj9_platform_data { int poll_interval; /* current poll interval (in milli-seconds) */ -- 1.5.4.3 > >> >> -- >> 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] input:Misc changes against kxtj9 v5 patch 2011-07-06 15:58 ` Christopher Hudson @ 2011-07-07 5:19 ` Dmitry Torokhov 0 siblings, 0 replies; 5+ messages in thread From: Dmitry Torokhov @ 2011-07-07 5:19 UTC (permalink / raw) To: Christopher Hudson; +Cc: Jonathan Cameron, linux-input 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-07-07 5:19 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-07-05 19:55 [PATCH] input:Misc changes against kxtj9 v5 patch chris.hudson.comp.eng
2011-07-05 20:05 ` Dmitry Torokhov
[not found] ` <CAB7Re7XDB6Rmckxidgr+90Br5LhYk_5Pi_JQXTLd9QS2V+Vm0A@mail.gmail.com>
[not found] ` <20110705203502.GC15876@core.coreip.homeip.net>
2011-07-06 15:42 ` Christopher Hudson
2011-07-06 15:58 ` Christopher Hudson
2011-07-07 5:19 ` Dmitry Torokhov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).