On Tue, Jul 5, 2011 at 4:35 PM, Dmitry Torokhov
[off-list ref] wrote:
On Tue, Jul 05, 2011 at 04:19:32PM -0400, Christopher Hudson wrote:
quoted
On Tue, Jul 5, 2011 at 4:05 PM, Dmitry Torokhov
[off-list ref] wrote:
quoted
On Tue, Jul 05, 2011 at 03:55:54PM -0400, chris.hudson.comp.eng@gmail.com wrote:
quoted
From: Chris Hudson <redacted>
As requested, miscellaneous changes originally suggested by Jonathan Cameron,
patched against the original kxtj9 v5 patch.
Signed-off-by: Chris Hudson <redacted>
---
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
On Wed, Jul 6, 2011 at 11:42 AM, Christopher Hudson
[off-list ref] wrote:
On Tue, Jul 5, 2011 at 4:35 PM, Dmitry Torokhov
[off-list ref] wrote:
quoted
On Tue, Jul 05, 2011 at 04:19:32PM -0400, Christopher Hudson wrote:
quoted
On Tue, Jul 5, 2011 at 4:05 PM, Dmitry Torokhov
[off-list ref] wrote:
quoted
On Tue, Jul 05, 2011 at 03:55:54PM -0400, chris.hudson.comp.eng@gmail.com wrote:
quoted
From: Chris Hudson <redacted>
As requested, miscellaneous changes originally suggested by Jonathan Cameron,
patched against the original kxtj9 v5 patch.
Signed-off-by: Chris Hudson <redacted>
---
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 <redacted>
---
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
On Wed, Jul 06, 2011 at 11:58:41AM -0400, Christopher Hudson wrote:
On Wed, Jul 6, 2011 at 11:42 AM, Christopher Hudson
[off-list ref] wrote:
quoted
On Tue, Jul 5, 2011 at 4:35 PM, Dmitry Torokhov
[off-list ref] wrote:
quoted
On Tue, Jul 05, 2011 at 04:19:32PM -0400, Christopher Hudson wrote:
quoted
On Tue, Jul 5, 2011 at 4:05 PM, Dmitry Torokhov
[off-list ref] wrote:
quoted
On Tue, Jul 05, 2011 at 03:55:54PM -0400, chris.hudson.comp.eng@gmail.com wrote:
quoted
From: Chris Hudson <redacted>
As requested, miscellaneous changes originally suggested by Jonathan Cameron,
patched against the original kxtj9 v5 patch.
Signed-off-by: Chris Hudson <redacted>
---
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 <redacted>
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