On Tue, Jul 5, 2011 at 4:35 PM, Dmitry Torokhov
<dmitry.torokhov@xxxxxxxxx> 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@xxxxxxxxx> wrote:
>> > On Tue, Jul 05, 2011 at 03:55:54PM -0400, chris.hudson.comp.eng@xxxxxxxxx wrote:
>> >> From: Chris Hudson <chudson@xxxxxxxxxx>
>> >>
>> >> As requested, miscellaneous changes originally suggested by Jonathan Cameron,
>> >> patched against the original kxtj9 v5 patch.
>> >>
>> >> Signed-off-by: Chris Hudson <chudson@xxxxxxxxxx>
>> >> ---
>> >> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
