[lm-sensors] [PATCH 2/4] hwmon: (applesmc) Do not return random value in error case

[lm-sensors] [PATCH 2/4] hwmon: (applesmc) Do not return random value in error case

[Guenter Roeck]

If a bad parameter is passed to applesmc_read_motion_sensor(), an uninitialized
and thus arbitrary value is returned. Fix by returning immediately if a bad
parameter is detected.

Cc: Henrik Rydberg <rydberg@euromail.se>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/hwmon/applesmc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index f082e48..cc8dd57 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -450,7 +450,7 @@ static int applesmc_read_motion_sensor(int index, s16 *value)
 		ret = applesmc_read_key(MOTION_SENSOR_Z_KEY, buffer, 2);
 		break;
 	default:
-		ret = -EINVAL;
+		return -EINVAL;
 	}
 
 	*value = ((s16)buffer[0] << 8) | buffer[1];
-- 
1.7.9.7


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 2/4] hwmon: (applesmc) Do not return random value in error case

[Henrik Rydberg]

On Tue, Jun 19, 2012 at 08:46:41AM -0700, Guenter Roeck wrote:
> If a bad parameter is passed to applesmc_read_motion_sensor(), an uninitialized
> and thus arbitrary value is returned. Fix by returning immediately if a bad
> parameter is detected.
> 
> Cc: Henrik Rydberg <rydberg@euromail.se>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/hwmon/applesmc.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index f082e48..cc8dd57 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -450,7 +450,7 @@ static int applesmc_read_motion_sensor(int index, s16 *value)
>  		ret = applesmc_read_key(MOTION_SENSOR_Z_KEY, buffer, 2);
>  		break;
>  	default:
> -		ret = -EINVAL;
> +		return -EINVAL;
>  	}
>  
>  	*value = ((s16)buffer[0] << 8) | buffer[1];
> -- 
> 1.7.9.7
> 

Acked-by: Henrik Rydberg <rydberg@euromail.se>

Thanks,
Henrik


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 2/4] hwmon: (applesmc) Do not return random value in error case

[Jean Delvare]

On Tue, 19 Jun 2012 08:46:41 -0700, Guenter Roeck wrote:
> If a bad parameter is passed to applesmc_read_motion_sensor(), an uninitialized
> and thus arbitrary value is returned. Fix by returning immediately if a bad
> parameter is detected.
> 
> Cc: Henrik Rydberg <rydberg@euromail.se>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/hwmon/applesmc.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index f082e48..cc8dd57 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -450,7 +450,7 @@ static int applesmc_read_motion_sensor(int index, s16 *value)
>  		ret = applesmc_read_key(MOTION_SENSOR_Z_KEY, buffer, 2);
>  		break;
>  	default:
> -		ret = -EINVAL;
> +		return -EINVAL;
>  	}
>  
>  	*value = ((s16)buffer[0] << 8) | buffer[1];

It doesn't really matter as this code is never executed anyway. That
would be a driver bug if it was. A better fix would probably be to get
rid of this branch altogether.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 2/4] hwmon: (applesmc) Do not return random value in error case

[Guenter Roeck]

On Wed, Jun 20, 2012 at 04:44:05PM +0200, Jean Delvare wrote:
> On Tue, 19 Jun 2012 08:46:41 -0700, Guenter Roeck wrote:
> > If a bad parameter is passed to applesmc_read_motion_sensor(), an uninitialized
> > and thus arbitrary value is returned. Fix by returning immediately if a bad
> > parameter is detected.
> > 
> > Cc: Henrik Rydberg <rydberg@euromail.se>
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> >  drivers/hwmon/applesmc.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> > index f082e48..cc8dd57 100644
> > --- a/drivers/hwmon/applesmc.c
> > +++ b/drivers/hwmon/applesmc.c
> > @@ -450,7 +450,7 @@ static int applesmc_read_motion_sensor(int index, s16 *value)
> >  		ret = applesmc_read_key(MOTION_SENSOR_Z_KEY, buffer, 2);
> >  		break;
> >  	default:
> > -		ret = -EINVAL;
> > +		return -EINVAL;
> >  	}
> >  
> >  	*value = ((s16)buffer[0] << 8) | buffer[1];
> 
> It doesn't really matter as this code is never executed anyway. That
> would be a driver bug if it was. A better fix would probably be to get
> rid of this branch altogether.
> 
If we do that, the compiler will likely complain that ret may be used but not set.
I could add BUG() or WARN(), but I am not sure if that is worth it.

Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 2/4] hwmon: (applesmc) Do not return random value in error case

[Jean Delvare]

On Wed, 20 Jun 2012 08:00:42 -0700, Guenter Roeck wrote:
> On Wed, Jun 20, 2012 at 04:44:05PM +0200, Jean Delvare wrote:
> > On Tue, 19 Jun 2012 08:46:41 -0700, Guenter Roeck wrote:
> > > If a bad parameter is passed to applesmc_read_motion_sensor(), an uninitialized
> > > and thus arbitrary value is returned. Fix by returning immediately if a bad
> > > parameter is detected.
> > > 
> > > Cc: Henrik Rydberg <rydberg@euromail.se>
> > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > ---
> > >  drivers/hwmon/applesmc.c |    2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> > > index f082e48..cc8dd57 100644
> > > --- a/drivers/hwmon/applesmc.c
> > > +++ b/drivers/hwmon/applesmc.c
> > > @@ -450,7 +450,7 @@ static int applesmc_read_motion_sensor(int index, s16 *value)
> > >  		ret = applesmc_read_key(MOTION_SENSOR_Z_KEY, buffer, 2);
> > >  		break;
> > >  	default:
> > > -		ret = -EINVAL;
> > > +		return -EINVAL;
> > >  	}
> > >  
> > >  	*value = ((s16)buffer[0] << 8) | buffer[1];
> > 
> > It doesn't really matter as this code is never executed anyway. That
> > would be a driver bug if it was. A better fix would probably be to get
> > rid of this branch altogether.
> > 
> If we do that, the compiler will likely complain that ret may be used but not set.
> I could add BUG() or WARN(), but I am not sure if that is worth it.

You can just replace one of the labels with default (or add default and
leave the label for clarity) to make the compiler happy. Alternatively
using an enum instead of independent constants may make the compiler
happy without a default at all.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 2/4] hwmon: (applesmc) Do not return random value in error case

[Henrik Rydberg]

> > It doesn't really matter as this code is never executed anyway. That
> > would be a driver bug if it was. A better fix would probably be to get
> > rid of this branch altogether.
> > 
> If we do that, the compiler will likely complain that ret may be used but not set.
> I could add BUG() or WARN(), but I am not sure if that is worth it.

Or we could reduce the code further, as in the patch below. I had this
outlined some time ago, but refrained from sending it since it seemed
to make no difference at the time...

Cheers,
Henrik

From 479a3d94f2e50d7917b9f7959512af0c9b01ff9c Mon Sep 17 00:00:00 2001
From: Henrik Rydberg <rydberg@euromail.se>
Date: Wed, 20 Jun 2012 18:00:06 +0200
Subject: [PATCH] hwmon: (applesmc) Skip sensor mapping

The special motion sensor mapping is unnecessary; remove it.

Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
---
 drivers/hwmon/applesmc.c |   41 +++++++++++++----------------------------
 1 file changed, 13 insertions(+), 28 deletions(-)

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 70d62f5..2bc6490 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -96,10 +96,6 @@ static const char *const fan_speed_fmt[] = {
 #define APPLESMC_INPUT_FUZZ	4	/* input event threshold */
 #define APPLESMC_INPUT_FLAT	4
 
-#define SENSOR_X 0
-#define SENSOR_Y 1
-#define SENSOR_Z 2
-
 #define to_index(attr) (to_sensor_dev_attr(attr)->index & 0xffff)
 #define to_option(attr) (to_sensor_dev_attr(attr)->index >> 16)
 
@@ -432,30 +428,19 @@ static int applesmc_has_key(const char *key, bool *value)
 }
 
 /*
- * applesmc_read_motion_sensor - Read motion sensor (X, Y or Z).
+ * applesmc_read_s16 - Read 16-bit signed big endian register
  */
-static int applesmc_read_motion_sensor(int index, s16 *value)
+static int applesmc_read_s16(const char *key, s16 *value)
 {
 	u8 buffer[2];
 	int ret;
 
-	switch (index) {
-	case SENSOR_X:
-		ret = applesmc_read_key(MOTION_SENSOR_X_KEY, buffer, 2);
-		break;
-	case SENSOR_Y:
-		ret = applesmc_read_key(MOTION_SENSOR_Y_KEY, buffer, 2);
-		break;
-	case SENSOR_Z:
-		ret = applesmc_read_key(MOTION_SENSOR_Z_KEY, buffer, 2);
-		break;
-	default:
-		ret = -EINVAL;
-	}
+	ret = applesmc_read_key(key, buffer, 2);
+	if (ret)
+		return ret;
 
 	*value = ((s16)buffer[0] << 8) | buffer[1];
-
-	return ret;
+	return 0;
 }
 
 /*
@@ -624,8 +609,8 @@ static struct platform_driver applesmc_driver = {
  */
 static void applesmc_calibrate(void)
 {
-	applesmc_read_motion_sensor(SENSOR_X, &rest_x);
-	applesmc_read_motion_sensor(SENSOR_Y, &rest_y);
+	applesmc_read_s16(MOTION_SENSOR_X_KEY, &rest_x);
+	applesmc_read_s16(MOTION_SENSOR_Y_KEY, &rest_y);
 	rest_x = -rest_x;
 }
 
@@ -634,9 +619,9 @@ static void applesmc_idev_poll(struct input_polled_dev *dev)
 	struct input_dev *idev = dev->input;
 	s16 x, y;
 
-	if (applesmc_read_motion_sensor(SENSOR_X, &x))
+	if (applesmc_read_s16(MOTION_SENSOR_X_KEY, &x))
 		return;
-	if (applesmc_read_motion_sensor(SENSOR_Y, &y))
+	if (applesmc_read_s16(MOTION_SENSOR_Y_KEY, &y))
 		return;
 
 	x = -x;
@@ -659,13 +644,13 @@ static ssize_t applesmc_position_show(struct device *dev,
 	int ret;
 	s16 x, y, z;
 
-	ret = applesmc_read_motion_sensor(SENSOR_X, &x);
+	ret = applesmc_read_s16(MOTION_SENSOR_X_KEY, &x);
 	if (ret)
 		goto out;
-	ret = applesmc_read_motion_sensor(SENSOR_Y, &y);
+	ret = applesmc_read_s16(MOTION_SENSOR_Y_KEY, &y);
 	if (ret)
 		goto out;
-	ret = applesmc_read_motion_sensor(SENSOR_Z, &z);
+	ret = applesmc_read_s16(MOTION_SENSOR_Z_KEY, &z);
 	if (ret)
 		goto out;
 
-- 
1.7.10.4


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 2/4] hwmon: (applesmc) Do not return random value in error case

[Guenter Roeck]

On Wed, 2012-06-20 at 12:03 -0400, Henrik Rydberg wrote:
> > > It doesn't really matter as this code is never executed anyway. That
> > > would be a driver bug if it was. A better fix would probably be to get
> > > rid of this branch altogether.
> > > 
> > If we do that, the compiler will likely complain that ret may be used but not set.
> > I could add BUG() or WARN(), but I am not sure if that is worth it.
> 
> Or we could reduce the code further, as in the patch below. I had this
> outlined some time ago, but refrained from sending it since it seemed
> to make no difference at the time...
> 
> Cheers,
> Henrik
> 
> From 479a3d94f2e50d7917b9f7959512af0c9b01ff9c Mon Sep 17 00:00:00 2001
> From: Henrik Rydberg <rydberg@euromail.se>
> Date: Wed, 20 Jun 2012 18:00:06 +0200
> Subject: [PATCH] hwmon: (applesmc) Skip sensor mapping
> 
> The special motion sensor mapping is unnecessary; remove it.
> 
> Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
> ---
>  drivers/hwmon/applesmc.c |   41 +++++++++++++----------------------------
>  1 file changed, 13 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index 70d62f5..2bc6490 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -96,10 +96,6 @@ static const char *const fan_speed_fmt[] = {
>  #define APPLESMC_INPUT_FUZZ	4	/* input event threshold */
>  #define APPLESMC_INPUT_FLAT	4
>  
> -#define SENSOR_X 0
> -#define SENSOR_Y 1
> -#define SENSOR_Z 2
> -
>  #define to_index(attr) (to_sensor_dev_attr(attr)->index & 0xffff)
>  #define to_option(attr) (to_sensor_dev_attr(attr)->index >> 16)
>  
> @@ -432,30 +428,19 @@ static int applesmc_has_key(const char *key, bool *value)
>  }
>  
>  /*
> - * applesmc_read_motion_sensor - Read motion sensor (X, Y or Z).
> + * applesmc_read_s16 - Read 16-bit signed big endian register
>   */
> -static int applesmc_read_motion_sensor(int index, s16 *value)
> +static int applesmc_read_s16(const char *key, s16 *value)
>  {
>  	u8 buffer[2];
>  	int ret;
>  
> -	switch (index) {
> -	case SENSOR_X:
> -		ret = applesmc_read_key(MOTION_SENSOR_X_KEY, buffer, 2);
> -		break;
> -	case SENSOR_Y:
> -		ret = applesmc_read_key(MOTION_SENSOR_Y_KEY, buffer, 2);
> -		break;
> -	case SENSOR_Z:
> -		ret = applesmc_read_key(MOTION_SENSOR_Z_KEY, buffer, 2);
> -		break;
> -	default:
> -		ret = -EINVAL;
> -	}
> +	ret = applesmc_read_key(key, buffer, 2);
> +	if (ret)
> +		return ret;
>  
>  	*value = ((s16)buffer[0] << 8) | buffer[1];
> -
> -	return ret;
> +	return 0;
>  }
>  
>  /*
> @@ -624,8 +609,8 @@ static struct platform_driver applesmc_driver = {
>   */
>  static void applesmc_calibrate(void)
>  {
> -	applesmc_read_motion_sensor(SENSOR_X, &rest_x);
> -	applesmc_read_motion_sensor(SENSOR_Y, &rest_y);
> +	applesmc_read_s16(MOTION_SENSOR_X_KEY, &rest_x);
> +	applesmc_read_s16(MOTION_SENSOR_Y_KEY, &rest_y);
>  	rest_x = -rest_x;
>  }
>  
> @@ -634,9 +619,9 @@ static void applesmc_idev_poll(struct input_polled_dev *dev)
>  	struct input_dev *idev = dev->input;
>  	s16 x, y;
>  
> -	if (applesmc_read_motion_sensor(SENSOR_X, &x))
> +	if (applesmc_read_s16(MOTION_SENSOR_X_KEY, &x))
>  		return;
> -	if (applesmc_read_motion_sensor(SENSOR_Y, &y))
> +	if (applesmc_read_s16(MOTION_SENSOR_Y_KEY, &y))
>  		return;
>  
>  	x = -x;
> @@ -659,13 +644,13 @@ static ssize_t applesmc_position_show(struct device *dev,
>  	int ret;
>  	s16 x, y, z;
>  
> -	ret = applesmc_read_motion_sensor(SENSOR_X, &x);
> +	ret = applesmc_read_s16(MOTION_SENSOR_X_KEY, &x);
>  	if (ret)
>  		goto out;
> -	ret = applesmc_read_motion_sensor(SENSOR_Y, &y);
> +	ret = applesmc_read_s16(MOTION_SENSOR_Y_KEY, &y);
>  	if (ret)
>  		goto out;
> -	ret = applesmc_read_motion_sensor(SENSOR_Z, &z);
> +	ret = applesmc_read_s16(MOTION_SENSOR_Z_KEY, &z);
>  	if (ret)
>  		goto out;
>  
I like it - much better than the other options. Jean, what do you
think ?

Thanks,
Guenter




_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 2/4] hwmon: (applesmc) Do not return random value in error case

[Jean Delvare]

On Wed, 20 Jun 2012 09:38:33 -0700, Guenter Roeck wrote:
> On Wed, 2012-06-20 at 12:03 -0400, Henrik Rydberg wrote:
> > Or we could reduce the code further, as in the patch below. I had this
> > outlined some time ago, but refrained from sending it since it seemed
> > to make no difference at the time...
> > 
> > Cheers,
> > Henrik
> > 
> > From 479a3d94f2e50d7917b9f7959512af0c9b01ff9c Mon Sep 17 00:00:00 2001
> > From: Henrik Rydberg <rydberg@euromail.se>
> > Date: Wed, 20 Jun 2012 18:00:06 +0200
> > Subject: [PATCH] hwmon: (applesmc) Skip sensor mapping
> > 
> > The special motion sensor mapping is unnecessary; remove it.
> > 
> > Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
> > ---
> >  drivers/hwmon/applesmc.c |   41 +++++++++++++----------------------------
> >  1 file changed, 13 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> > index 70d62f5..2bc6490 100644
> > --- a/drivers/hwmon/applesmc.c
> > +++ b/drivers/hwmon/applesmc.c
> > @@ -96,10 +96,6 @@ static const char *const fan_speed_fmt[] = {
> >  #define APPLESMC_INPUT_FUZZ	4	/* input event threshold */
> >  #define APPLESMC_INPUT_FLAT	4
> >  
> > -#define SENSOR_X 0
> > -#define SENSOR_Y 1
> > -#define SENSOR_Z 2
> > -
> >  #define to_index(attr) (to_sensor_dev_attr(attr)->index & 0xffff)
> >  #define to_option(attr) (to_sensor_dev_attr(attr)->index >> 16)
> >  
> > @@ -432,30 +428,19 @@ static int applesmc_has_key(const char *key, bool *value)
> >  }
> >  
> >  /*
> > - * applesmc_read_motion_sensor - Read motion sensor (X, Y or Z).
> > + * applesmc_read_s16 - Read 16-bit signed big endian register
> >   */
> > -static int applesmc_read_motion_sensor(int index, s16 *value)
> > +static int applesmc_read_s16(const char *key, s16 *value)
> >  {
> >  	u8 buffer[2];
> >  	int ret;
> >  
> > -	switch (index) {
> > -	case SENSOR_X:
> > -		ret = applesmc_read_key(MOTION_SENSOR_X_KEY, buffer, 2);
> > -		break;
> > -	case SENSOR_Y:
> > -		ret = applesmc_read_key(MOTION_SENSOR_Y_KEY, buffer, 2);
> > -		break;
> > -	case SENSOR_Z:
> > -		ret = applesmc_read_key(MOTION_SENSOR_Z_KEY, buffer, 2);
> > -		break;
> > -	default:
> > -		ret = -EINVAL;
> > -	}
> > +	ret = applesmc_read_key(key, buffer, 2);
> > +	if (ret)
> > +		return ret;
> >  
> >  	*value = ((s16)buffer[0] << 8) | buffer[1];
> > -
> > -	return ret;
> > +	return 0;
> >  }
> >  
> >  /*
> > @@ -624,8 +609,8 @@ static struct platform_driver applesmc_driver = {
> >   */
> >  static void applesmc_calibrate(void)
> >  {
> > -	applesmc_read_motion_sensor(SENSOR_X, &rest_x);
> > -	applesmc_read_motion_sensor(SENSOR_Y, &rest_y);
> > +	applesmc_read_s16(MOTION_SENSOR_X_KEY, &rest_x);
> > +	applesmc_read_s16(MOTION_SENSOR_Y_KEY, &rest_y);
> >  	rest_x = -rest_x;
> >  }
> >  
> > @@ -634,9 +619,9 @@ static void applesmc_idev_poll(struct input_polled_dev *dev)
> >  	struct input_dev *idev = dev->input;
> >  	s16 x, y;
> >  
> > -	if (applesmc_read_motion_sensor(SENSOR_X, &x))
> > +	if (applesmc_read_s16(MOTION_SENSOR_X_KEY, &x))
> >  		return;
> > -	if (applesmc_read_motion_sensor(SENSOR_Y, &y))
> > +	if (applesmc_read_s16(MOTION_SENSOR_Y_KEY, &y))
> >  		return;
> >  
> >  	x = -x;
> > @@ -659,13 +644,13 @@ static ssize_t applesmc_position_show(struct device *dev,
> >  	int ret;
> >  	s16 x, y, z;
> >  
> > -	ret = applesmc_read_motion_sensor(SENSOR_X, &x);
> > +	ret = applesmc_read_s16(MOTION_SENSOR_X_KEY, &x);
> >  	if (ret)
> >  		goto out;
> > -	ret = applesmc_read_motion_sensor(SENSOR_Y, &y);
> > +	ret = applesmc_read_s16(MOTION_SENSOR_Y_KEY, &y);
> >  	if (ret)
> >  		goto out;
> > -	ret = applesmc_read_motion_sensor(SENSOR_Z, &z);
> > +	ret = applesmc_read_s16(MOTION_SENSOR_Z_KEY, &z);
> >  	if (ret)
> >  		goto out;
> >  
> I like it - much better than the other options. Jean, what do you
> think ?

I like it a lot too, the diffstat speaks for itself.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 2/4] hwmon: (applesmc) Do not return random value in error case

[Guenter Roeck]

On Wed, 2012-06-20 at 13:34 -0400, Jean Delvare wrote:
> On Wed, 20 Jun 2012 09:38:33 -0700, Guenter Roeck wrote:
> > On Wed, 2012-06-20 at 12:03 -0400, Henrik Rydberg wrote:
> > > Or we could reduce the code further, as in the patch below. I had this
> > > outlined some time ago, but refrained from sending it since it seemed
> > > to make no difference at the time...
> > > 
> > > Cheers,
> > > Henrik
> > > 
> > > From 479a3d94f2e50d7917b9f7959512af0c9b01ff9c Mon Sep 17 00:00:00 2001
> > > From: Henrik Rydberg <rydberg@euromail.se>
> > > Date: Wed, 20 Jun 2012 18:00:06 +0200
> > > Subject: [PATCH] hwmon: (applesmc) Skip sensor mapping
> > > 
> > > The special motion sensor mapping is unnecessary; remove it.
> > > 
> > > Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
> > > ---
> > >  drivers/hwmon/applesmc.c |   41 +++++++++++++----------------------------
> > >  1 file changed, 13 insertions(+), 28 deletions(-)
> > > 
> > > diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> > > index 70d62f5..2bc6490 100644
> > > --- a/drivers/hwmon/applesmc.c
> > > +++ b/drivers/hwmon/applesmc.c
> > > @@ -96,10 +96,6 @@ static const char *const fan_speed_fmt[] = {
> > >  #define APPLESMC_INPUT_FUZZ	4	/* input event threshold */
> > >  #define APPLESMC_INPUT_FLAT	4
> > >  
> > > -#define SENSOR_X 0
> > > -#define SENSOR_Y 1
> > > -#define SENSOR_Z 2
> > > -
> > >  #define to_index(attr) (to_sensor_dev_attr(attr)->index & 0xffff)
> > >  #define to_option(attr) (to_sensor_dev_attr(attr)->index >> 16)
> > >  
> > > @@ -432,30 +428,19 @@ static int applesmc_has_key(const char *key, bool *value)
> > >  }
> > >  
> > >  /*
> > > - * applesmc_read_motion_sensor - Read motion sensor (X, Y or Z).
> > > + * applesmc_read_s16 - Read 16-bit signed big endian register
> > >   */
> > > -static int applesmc_read_motion_sensor(int index, s16 *value)
> > > +static int applesmc_read_s16(const char *key, s16 *value)
> > >  {
> > >  	u8 buffer[2];
> > >  	int ret;
> > >  
> > > -	switch (index) {
> > > -	case SENSOR_X:
> > > -		ret = applesmc_read_key(MOTION_SENSOR_X_KEY, buffer, 2);
> > > -		break;
> > > -	case SENSOR_Y:
> > > -		ret = applesmc_read_key(MOTION_SENSOR_Y_KEY, buffer, 2);
> > > -		break;
> > > -	case SENSOR_Z:
> > > -		ret = applesmc_read_key(MOTION_SENSOR_Z_KEY, buffer, 2);
> > > -		break;
> > > -	default:
> > > -		ret = -EINVAL;
> > > -	}
> > > +	ret = applesmc_read_key(key, buffer, 2);
> > > +	if (ret)
> > > +		return ret;
> > >  
> > >  	*value = ((s16)buffer[0] << 8) | buffer[1];
> > > -
> > > -	return ret;
> > > +	return 0;
> > >  }
> > >  
> > >  /*
> > > @@ -624,8 +609,8 @@ static struct platform_driver applesmc_driver = {
> > >   */
> > >  static void applesmc_calibrate(void)
> > >  {
> > > -	applesmc_read_motion_sensor(SENSOR_X, &rest_x);
> > > -	applesmc_read_motion_sensor(SENSOR_Y, &rest_y);
> > > +	applesmc_read_s16(MOTION_SENSOR_X_KEY, &rest_x);
> > > +	applesmc_read_s16(MOTION_SENSOR_Y_KEY, &rest_y);
> > >  	rest_x = -rest_x;
> > >  }
> > >  
> > > @@ -634,9 +619,9 @@ static void applesmc_idev_poll(struct input_polled_dev *dev)
> > >  	struct input_dev *idev = dev->input;
> > >  	s16 x, y;
> > >  
> > > -	if (applesmc_read_motion_sensor(SENSOR_X, &x))
> > > +	if (applesmc_read_s16(MOTION_SENSOR_X_KEY, &x))
> > >  		return;
> > > -	if (applesmc_read_motion_sensor(SENSOR_Y, &y))
> > > +	if (applesmc_read_s16(MOTION_SENSOR_Y_KEY, &y))
> > >  		return;
> > >  
> > >  	x = -x;
> > > @@ -659,13 +644,13 @@ static ssize_t applesmc_position_show(struct device *dev,
> > >  	int ret;
> > >  	s16 x, y, z;
> > >  
> > > -	ret = applesmc_read_motion_sensor(SENSOR_X, &x);
> > > +	ret = applesmc_read_s16(MOTION_SENSOR_X_KEY, &x);
> > >  	if (ret)
> > >  		goto out;
> > > -	ret = applesmc_read_motion_sensor(SENSOR_Y, &y);
> > > +	ret = applesmc_read_s16(MOTION_SENSOR_Y_KEY, &y);
> > >  	if (ret)
> > >  		goto out;
> > > -	ret = applesmc_read_motion_sensor(SENSOR_Z, &z);
> > > +	ret = applesmc_read_s16(MOTION_SENSOR_Z_KEY, &z);
> > >  	if (ret)
> > >  		goto out;
> > >  
> > I like it - much better than the other options. Jean, what do you
> > think ?
> 
> I like it a lot too, the diffstat speaks for itself.
> 
Great. Applied to my -next branch.

Thanks,
Guenter



_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors