[PATCH] hwmon/hdaps: Fix bug 7154 inversion of separate axis

[PATCH] hwmon/hdaps: Fix bug 7154 inversion of separate axis

[Frank Seidel]

From: Frank Seidel <frank@f-seidel.de>

Fix for kernel.org bug #7154:hdaps inversion of
each axis. This version is based on the work
from Michael Ruoss <miruoss@student.ethz.ch>.

Signed-off-by: Frank Seidel <frank@f-seidel.de>
---
 drivers/hwmon/hdaps.c |   49 ++++++++++++++++++++++++++++---------------------
 1 file changed, 28 insertions(+), 21 deletions(-)

--- a/drivers/hwmon/hdaps.c
+++ b/drivers/hwmon/hdaps.c
@@ -65,6 +65,10 @@
 #define HDAPS_INPUT_FUZZ	4	/* input event threshold */
 #define HDAPS_INPUT_FLAT	4
 
+#define HDAPS_X_AXIS		1UL
+#define HDAPS_Y_AXIS		2UL
+#define HDAPS_BOTH_AXES		3UL
+
 static struct platform_device *pdev;
 static struct input_polled_dev *hdaps_idev;
 static unsigned int hdaps_invert;
@@ -182,11 +186,11 @@ static int __hdaps_read_pair(unsigned in
 	km_activity = inb(HDAPS_PORT_KMACT);
 	__device_complete();
 
-	/* if hdaps_invert is set, negate the two values */
-	if (hdaps_invert) {
+	/* hdaps_invert is a bitvector to negate the axes */
+	if (hdaps_invert & 1)
 		*x = -*x;
+	if (hdaps_invert & 2)
 		*y = -*y;
-	}
 
 	return 0;
 }
@@ -436,7 +440,7 @@ static ssize_t hdaps_invert_store(struct
 {
 	int invert;
 
-	if (sscanf(buf, "%d", &invert) != 1 || (invert != 1 && invert != 0))
+	if (sscanf(buf, "%d", &invert) != 1 || invert < 0 || invert > 3)
 		return -EINVAL;
 
 	hdaps_invert = invert;
@@ -483,7 +487,7 @@ static int __init hdaps_dmi_match(const
 /* hdaps_dmi_match_invert - found an inverted match. */
 static int __init hdaps_dmi_match_invert(const struct dmi_system_id *id)
 {
-	hdaps_invert = 1;
+	hdaps_invert = (int)id->driver_data;
 	printk(KERN_INFO "hdaps: inverting axis readings.\n");
 	return hdaps_dmi_match(id);
 }
@@ -491,15 +495,17 @@ static int __init hdaps_dmi_match_invert
 #define HDAPS_DMI_MATCH_NORMAL(vendor, model) {		\
 	.ident = vendor " " model,			\
 	.callback = hdaps_dmi_match,			\
+	.driver_data = (void *)0UL,			\
 	.matches = {					\
 		DMI_MATCH(DMI_BOARD_VENDOR, vendor),	\
 		DMI_MATCH(DMI_PRODUCT_VERSION, model)	\
 	}						\
 }
 
-#define HDAPS_DMI_MATCH_INVERT(vendor, model) {		\
+#define HDAPS_DMI_MATCH_INVERT(vendor, model, axes) {	\
 	.ident = vendor " " model,			\
 	.callback = hdaps_dmi_match_invert,		\
+	.driver_data = (void *)axes,			\
 	.matches = {					\
 		DMI_MATCH(DMI_BOARD_VENDOR, vendor),	\
 		DMI_MATCH(DMI_PRODUCT_VERSION, model)	\
@@ -511,28 +517,28 @@ static int __init hdaps_dmi_match_invert
    If your ThinkPad is not recognized, please update to latest
    BIOS. This is especially the case for some R52 ThinkPads. */
 static struct dmi_system_id __initdata hdaps_whitelist[] = {
-	HDAPS_DMI_MATCH_INVERT("IBM", "ThinkPad R50p"),
+	HDAPS_DMI_MATCH_INVERT("IBM", "ThinkPad R50p", HDAPS_BOTH_AXES),
 	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad R50"),
 	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad R51"),
 	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad R52"),
-	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad R61i"),
-	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad R61"),
-	HDAPS_DMI_MATCH_INVERT("IBM", "ThinkPad T41p"),
+	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad R61i", HDAPS_BOTH_AXES),
+	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad R61", HDAPS_BOTH_AXES),
+	HDAPS_DMI_MATCH_INVERT("IBM", "ThinkPad T41p", HDAPS_BOTH_AXES),
 	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad T41"),
-	HDAPS_DMI_MATCH_INVERT("IBM", "ThinkPad T42p"),
+	HDAPS_DMI_MATCH_INVERT("IBM", "ThinkPad T42p", HDAPS_BOTH_AXES),
 	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad T42"),
 	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad T43"),
-	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad T60"),
-	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad T61p"),
-	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad T61"),
+	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad T60", HDAPS_BOTH_AXES),
+	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad T61p", HDAPS_BOTH_AXES),
+	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad T61", HDAPS_BOTH_AXES),
 	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad X40"),
 	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad X41"),
-	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad X60"),
-	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad X61s"),
-	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad X61"),
+	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad X60", HDAPS_BOTH_AXES),
+	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad X61s", HDAPS_BOTH_AXES),
+	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad X61", HDAPS_BOTH_AXES),
 	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad Z60m"),
-	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad Z61m"),
-	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad Z61p"),
+	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad Z61m", HDAPS_BOTH_AXES),
+	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad Z61p", HDAPS_BOTH_AXES),
 	{ .ident = NULL }
 };
 
@@ -627,8 +633,9 @@ static void __exit hdaps_exit(void)
 module_init(hdaps_init);
 module_exit(hdaps_exit);
 
-module_param_named(invert, hdaps_invert, bool, 0);
-MODULE_PARM_DESC(invert, "invert data along each axis");
+module_param_named(invert, hdaps_invert, int, 0);
+MODULE_PARM_DESC(invert, "invert data along each axis. 1 invert x-axis, "
+		 "2 invert y-axis, 3 invert both axes.");
 
 MODULE_AUTHOR("Robert Love");
 MODULE_DESCRIPTION("IBM Hard Drive Active Protection System (HDAPS) driver");

Re: [PATCH] hwmon/hdaps: Fix bug 7154 inversion of separate axis

[Dmitry Torokhov]

Hi Frank,

On Fri, Feb 13, 2009 at 01:38:01PM +0100, Frank Seidel wrote:
> From: Frank Seidel <frank@f-seidel.de>
> 
> Fix for kernel.org bug #7154:hdaps inversion of
> each axis. This version is based on the work
> from Michael Ruoss <miruoss@student.ethz.ch>.
> 
>  
> -	/* if hdaps_invert is set, negate the two values */
> -	if (hdaps_invert) {
> +	/* hdaps_invert is a bitvector to negate the axes */
> +	if (hdaps_invert & 1)
>  		*x = -*x;
> +	if (hdaps_invert & 2)
>  		*y = -*y;
> -	}

You have defined HDAPS_{X|Y}_AXIS, why not use them?

>  
> -module_param_named(invert, hdaps_invert, bool, 0);
> -MODULE_PARM_DESC(invert, "invert data along each axis");
> +module_param_named(invert, hdaps_invert, int, 0);
> +MODULE_PARM_DESC(invert, "invert data along each axis. 1 invert x-axis, "
> +		 "2 invert y-axis, 3 invert both axes.");
>  

Why don't you make these 0644? I don't see why they can't be changed
"on fly".

-- 
Dmitry

Re: [PATCH] hwmon/hdaps: Fix bug 7154 inversion of separate axis

[Shem Multinymous]

Hi Frank,

On Fri, Feb 13, 2009 at 7:38 AM, Frank Seidel <fseidel@suse.de> wrote:
> From: Frank Seidel <frank@f-seidel.de>
>
> Fix for kernel.org bug #7154:hdaps inversion of
> each axis. This version is based on the work
> from Michael Ruoss <miruoss@student.ethz.ch>.
>
> Signed-off-by: Frank Seidel <frank@f-seidel.de>
> ---
>  drivers/hwmon/hdaps.c |   49 ++++++++++++++++++++++++++++---------------------
>  1 file changed, 28 insertions(+), 21 deletions(-)
>
> --- a/drivers/hwmon/hdaps.c
> +++ b/drivers/hwmon/hdaps.c
> @@ -65,6 +65,10 @@
>  #define HDAPS_INPUT_FUZZ       4       /* input event threshold */
>  #define HDAPS_INPUT_FLAT       4
>
> +#define HDAPS_X_AXIS           1UL
> +#define HDAPS_Y_AXIS           2UL
> +#define HDAPS_BOTH_AXES                3UL

There are more possibilities than these: axes could also switched, for
a total of 8 possibilities.
See the table at the bottom of the tp_smapi page
(http://www.thinkwiki.org/wiki/Tp_smapi), or hdaps.c inside the
tp_smapi package, for more model-specific information.
It would be nice if you made the interface (constants and their
meaning) the same as in the tp_smapi version of hdaps, which is
already widely deployed and packaged by several distros.

  Shem

Re: [PATCH] hwmon/hdaps: Fix bug 7154 inversion of separate axis

[Jean Delvare]

On Sat, 14 Feb 2009 19:32:02 -0500, Shem Multinymous wrote:
> Hi Frank,
> 
> On Fri, Feb 13, 2009 at 7:38 AM, Frank Seidel <fseidel@suse.de> wrote:
> > From: Frank Seidel <frank@f-seidel.de>
> >
> > Fix for kernel.org bug #7154:hdaps inversion of
> > each axis. This version is based on the work
> > from Michael Ruoss <miruoss@student.ethz.ch>.
> >
> > Signed-off-by: Frank Seidel <frank@f-seidel.de>
> > ---
> >  drivers/hwmon/hdaps.c |   49 ++++++++++++++++++++++++++++---------------------
> >  1 file changed, 28 insertions(+), 21 deletions(-)
> >
> > --- a/drivers/hwmon/hdaps.c
> > +++ b/drivers/hwmon/hdaps.c
> > @@ -65,6 +65,10 @@
> >  #define HDAPS_INPUT_FUZZ       4       /* input event threshold */
> >  #define HDAPS_INPUT_FLAT       4
> >
> > +#define HDAPS_X_AXIS           1UL
> > +#define HDAPS_Y_AXIS           2UL
> > +#define HDAPS_BOTH_AXES                3UL
> 
> There are more possibilities than these: axes could also switched, for
> a total of 8 possibilities.

Which leads to the simple conclusion that this chip was never meant to
be used as an input device. Think about it: this chip is there to
protect the hard disk drive from shocks. Instead of this we are
proposing to the user to abuse the chip as an input device, that is:
voluntarily shock the laptop. Of course these are small movements,
nothing like a free fall, but I still believe this is conceptually
wrong.

Honestly, who uses this feature in practice? I bet this makes users
laugh for a minute when they discover the feature, and then they forget
about it. I think it would make sense to plain get rid of the input
feature of hdaps.

> See the table at the bottom of the tp_smapi page
> (http://www.thinkwiki.org/wiki/Tp_smapi), or hdaps.c inside the
> tp_smapi package, for more model-specific information.
> It would be nice if you made the interface (constants and their
> meaning) the same as in the tp_smapi version of hdaps, which is
> already widely deployed and packaged by several distros.

Why is this code not upstream? Ah yeah, I remember now: because it was
written by an anonymous developer, which makes the contribution legally
dubious. Copying such code into the upstream version of hdaps would be
no different, so we cannot do that, sorry.

-- 
Jean Delvare

Re: [PATCH] hwmon/hdaps: Fix bug 7154 inversion of separate axis

[Jean Delvare]

Hi Frank,

On Fri, 13 Feb 2009 13:38:01 +0100, Frank Seidel wrote:
> From: Frank Seidel <frank@f-seidel.de>
> 
> Fix for kernel.org bug #7154:hdaps inversion of
> each axis. This version is based on the work
> from Michael Ruoss <miruoss@student.ethz.ch>.
> 
> Signed-off-by: Frank Seidel <frank@f-seidel.de>
> ---
>  drivers/hwmon/hdaps.c |   49 ++++++++++++++++++++++++++++---------------------
>  1 file changed, 28 insertions(+), 21 deletions(-)
> 
> --- a/drivers/hwmon/hdaps.c
> +++ b/drivers/hwmon/hdaps.c
> @@ -65,6 +65,10 @@
>  #define HDAPS_INPUT_FUZZ	4	/* input event threshold */
>  #define HDAPS_INPUT_FLAT	4
>  
> +#define HDAPS_X_AXIS		1UL
> +#define HDAPS_Y_AXIS		2UL
> +#define HDAPS_BOTH_AXES		3UL

This would me much better defined as:

#define HDAPS_X_AXIS		(1 << 0)
#define HDAPS_Y_AXIS		(1 << 1)
#define HDAPS_BOTH_AXES		(HDAPS_X_AXIS | HDAPS_Y_AXIS)

> +
>  static struct platform_device *pdev;
>  static struct input_polled_dev *hdaps_idev;
>  static unsigned int hdaps_invert;
> @@ -182,11 +186,11 @@ static int __hdaps_read_pair(unsigned in
>  	km_activity = inb(HDAPS_PORT_KMACT);
>  	__device_complete();
>  
> -	/* if hdaps_invert is set, negate the two values */
> -	if (hdaps_invert) {
> +	/* hdaps_invert is a bitvector to negate the axes */
> +	if (hdaps_invert & 1)
>  		*x = -*x;
> +	if (hdaps_invert & 2)
>  		*y = -*y;

And then use these defines here (as Dmitry already suggested)...

> -	}
>  
>  	return 0;
>  }
> @@ -436,7 +440,7 @@ static ssize_t hdaps_invert_store(struct
>  {
>  	int invert;
>  
> -	if (sscanf(buf, "%d", &invert) != 1 || (invert != 1 && invert != 0))
> +	if (sscanf(buf, "%d", &invert) != 1 || invert < 0 || invert > 3)

... and here as well (3 is really HDAPS_BOTH_AXES).

>  		return -EINVAL;
>  
>  	hdaps_invert = invert;
> @@ -483,7 +487,7 @@ static int __init hdaps_dmi_match(const
>  /* hdaps_dmi_match_invert - found an inverted match. */
>  static int __init hdaps_dmi_match_invert(const struct dmi_system_id *id)
>  {
> -	hdaps_invert = 1;
> +	hdaps_invert = (int)id->driver_data;
>  	printk(KERN_INFO "hdaps: inverting axis readings.\n");
>  	return hdaps_dmi_match(id);
>  }
> @@ -491,15 +495,17 @@ static int __init hdaps_dmi_match_invert
>  #define HDAPS_DMI_MATCH_NORMAL(vendor, model) {		\
>  	.ident = vendor " " model,			\
>  	.callback = hdaps_dmi_match,			\
> +	.driver_data = (void *)0UL,			\
>  	.matches = {					\
>  		DMI_MATCH(DMI_BOARD_VENDOR, vendor),	\
>  		DMI_MATCH(DMI_PRODUCT_VERSION, model)	\
>  	}						\
>  }
>  
> -#define HDAPS_DMI_MATCH_INVERT(vendor, model) {		\
> +#define HDAPS_DMI_MATCH_INVERT(vendor, model, axes) {	\
>  	.ident = vendor " " model,			\
>  	.callback = hdaps_dmi_match_invert,		\
> +	.driver_data = (void *)axes,			\
>  	.matches = {					\
>  		DMI_MATCH(DMI_BOARD_VENDOR, vendor),	\
>  		DMI_MATCH(DMI_PRODUCT_VERSION, model)	\

If I'm not mistaken, HDAPS_DMI_MATCH_NORMAL can now be defined as:
#define HDAPS_DMI_MATCH_NORMAL(vendor, model) \
	HDAPS_DMI_MATCH_INVERT(vendor, model, 0)

Either that, or plain get rid of HDAPS_DMI_MATCH_NORMAL, rename
HDAPS_DMI_MATCH_INVERT to HDAPS_DMI_MATCH and use it everywhere.

> @@ -511,28 +517,28 @@ static int __init hdaps_dmi_match_invert
>     If your ThinkPad is not recognized, please update to latest
>     BIOS. This is especially the case for some R52 ThinkPads. */
>  static struct dmi_system_id __initdata hdaps_whitelist[] = {
> -	HDAPS_DMI_MATCH_INVERT("IBM", "ThinkPad R50p"),
> +	HDAPS_DMI_MATCH_INVERT("IBM", "ThinkPad R50p", HDAPS_BOTH_AXES),
>  	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad R50"),
>  	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad R51"),
>  	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad R52"),
> -	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad R61i"),
> -	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad R61"),
> -	HDAPS_DMI_MATCH_INVERT("IBM", "ThinkPad T41p"),
> +	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad R61i", HDAPS_BOTH_AXES),
> +	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad R61", HDAPS_BOTH_AXES),
> +	HDAPS_DMI_MATCH_INVERT("IBM", "ThinkPad T41p", HDAPS_BOTH_AXES),
>  	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad T41"),
> -	HDAPS_DMI_MATCH_INVERT("IBM", "ThinkPad T42p"),
> +	HDAPS_DMI_MATCH_INVERT("IBM", "ThinkPad T42p", HDAPS_BOTH_AXES),
>  	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad T42"),
>  	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad T43"),
> -	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad T60"),
> -	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad T61p"),
> -	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad T61"),
> +	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad T60", HDAPS_BOTH_AXES),
> +	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad T61p", HDAPS_BOTH_AXES),
> +	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad T61", HDAPS_BOTH_AXES),
>  	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad X40"),
>  	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad X41"),
> -	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad X60"),
> -	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad X61s"),
> -	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad X61"),
> +	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad X60", HDAPS_BOTH_AXES),
> +	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad X61s", HDAPS_BOTH_AXES),
> +	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad X61", HDAPS_BOTH_AXES),
>  	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad Z60m"),
> -	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad Z61m"),
> -	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad Z61p"),
> +	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad Z61m", HDAPS_BOTH_AXES),
> +	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad Z61p", HDAPS_BOTH_AXES),
>  	{ .ident = NULL }
>  };
>  
> @@ -627,8 +633,9 @@ static void __exit hdaps_exit(void)
>  module_init(hdaps_init);
>  module_exit(hdaps_exit);
>  
> -module_param_named(invert, hdaps_invert, bool, 0);
> -MODULE_PARM_DESC(invert, "invert data along each axis");
> +module_param_named(invert, hdaps_invert, int, 0);
> +MODULE_PARM_DESC(invert, "invert data along each axis. 1 invert x-axis, "
> +		 "2 invert y-axis, 3 invert both axes.");
>  
>  MODULE_AUTHOR("Robert Love");
>  MODULE_DESCRIPTION("IBM Hard Drive Active Protection System (HDAPS) driver");

But see also my rant elsewhere in this thread: I suspect we don't
really care about this feature and we could get rid of it for a much
simpler driver.

-- 
Jean Delvare

[PATCHv2] hwmon/hdaps: Fix bug 7154 inversion of separate axis

[Frank Seidel]

From: Frank Seidel <frank@f-seidel.de>

Fix for kernel.org bug #7154:hdaps inversion of
each axis. This version is based on the work
from Michael Ruoss <miruoss@student.ethz.ch>.

Signed-off-by: Frank Seidel <frank@f-seidel.de>
---
 drivers/hwmon/hdaps.c |   64 +++++++++++++++++++++++++-------------------------
 1 file changed, 33 insertions(+), 31 deletions(-)

--- a/drivers/hwmon/hdaps.c
+++ b/drivers/hwmon/hdaps.c
@@ -65,6 +65,10 @@
 #define HDAPS_INPUT_FUZZ	4	/* input event threshold */
 #define HDAPS_INPUT_FLAT	4
 
+#define HDAPS_X_AXIS		(1 << 0)
+#define HDAPS_Y_AXIS		(1 << 1)
+#define HDAPS_BOTH_AXES		(HDAPS_X_AXIS | HDAPS_Y_AXIS)
+
 static struct platform_device *pdev;
 static struct input_polled_dev *hdaps_idev;
 static unsigned int hdaps_invert;
@@ -182,11 +186,11 @@ static int __hdaps_read_pair(unsigned in
 	km_activity = inb(HDAPS_PORT_KMACT);
 	__device_complete();
 
-	/* if hdaps_invert is set, negate the two values */
-	if (hdaps_invert) {
+	/* hdaps_invert is a bitvector to negate the axes */
+	if (hdaps_invert & HDAPS_X_AXIS)
 		*x = -*x;
+	if (hdaps_invert & HDAPS_Y_AXIS)
 		*y = -*y;
-	}
 
 	return 0;
 }
@@ -436,7 +440,8 @@ static ssize_t hdaps_invert_store(struct
 {
 	int invert;
 
-	if (sscanf(buf, "%d", &invert) != 1 || (invert != 1 && invert != 0))
+	if (sscanf(buf, "%d", &invert) != 1 ||
+	    invert < 0 || invert > HDAPS_BOTH_AXES)
 		return -EINVAL;
 
 	hdaps_invert = invert;
@@ -483,56 +488,52 @@ static int __init hdaps_dmi_match(const
 /* hdaps_dmi_match_invert - found an inverted match. */
 static int __init hdaps_dmi_match_invert(const struct dmi_system_id *id)
 {
-	hdaps_invert = 1;
-	printk(KERN_INFO "hdaps: inverting axis readings.\n");
+	hdaps_invert = (int)id->driver_data;
+	printk(KERN_INFO "hdaps: inverting axis (%u) readings.\n",
+	       hdaps_invert);
 	return hdaps_dmi_match(id);
 }
 
-#define HDAPS_DMI_MATCH_NORMAL(vendor, model) {		\
-	.ident = vendor " " model,			\
-	.callback = hdaps_dmi_match,			\
-	.matches = {					\
-		DMI_MATCH(DMI_BOARD_VENDOR, vendor),	\
-		DMI_MATCH(DMI_PRODUCT_VERSION, model)	\
-	}						\
-}
-
-#define HDAPS_DMI_MATCH_INVERT(vendor, model) {		\
+#define HDAPS_DMI_MATCH_INVERT(vendor, model, axes) {	\
 	.ident = vendor " " model,			\
 	.callback = hdaps_dmi_match_invert,		\
+	.driver_data = (void *)axes,			\
 	.matches = {					\
 		DMI_MATCH(DMI_BOARD_VENDOR, vendor),	\
 		DMI_MATCH(DMI_PRODUCT_VERSION, model)	\
 	}						\
 }
 
+#define HDAPS_DMI_MATCH_NORMAL(vendor, model)		\
+	HDAPS_DMI_MATCH_INVERT(vendor, model, 0)
+
 /* Note that HDAPS_DMI_MATCH_NORMAL("ThinkPad T42") would match
    "ThinkPad T42p", so the order of the entries matters.
    If your ThinkPad is not recognized, please update to latest
    BIOS. This is especially the case for some R52 ThinkPads. */
 static struct dmi_system_id __initdata hdaps_whitelist[] = {
-	HDAPS_DMI_MATCH_INVERT("IBM", "ThinkPad R50p"),
+	HDAPS_DMI_MATCH_INVERT("IBM", "ThinkPad R50p", HDAPS_BOTH_AXES),
 	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad R50"),
 	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad R51"),
 	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad R52"),
-	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad R61i"),
-	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad R61"),
-	HDAPS_DMI_MATCH_INVERT("IBM", "ThinkPad T41p"),
+	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad R61i", HDAPS_BOTH_AXES),
+	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad R61", HDAPS_BOTH_AXES),
+	HDAPS_DMI_MATCH_INVERT("IBM", "ThinkPad T41p", HDAPS_BOTH_AXES),
 	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad T41"),
-	HDAPS_DMI_MATCH_INVERT("IBM", "ThinkPad T42p"),
+	HDAPS_DMI_MATCH_INVERT("IBM", "ThinkPad T42p", HDAPS_BOTH_AXES),
 	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad T42"),
 	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad T43"),
-	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad T60"),
-	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad T61p"),
-	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad T61"),
+	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad T60", HDAPS_BOTH_AXES),
+	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad T61p", HDAPS_BOTH_AXES),
+	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad T61", HDAPS_BOTH_AXES),
 	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad X40"),
 	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad X41"),
-	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad X60"),
-	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad X61s"),
-	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad X61"),
+	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad X60", HDAPS_BOTH_AXES),
+	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad X61s", HDAPS_BOTH_AXES),
+	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad X61", HDAPS_BOTH_AXES),
 	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad Z60m"),
-	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad Z61m"),
-	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad Z61p"),
+	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad Z61m", HDAPS_BOTH_AXES),
+	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad Z61p", HDAPS_BOTH_AXES),
 	{ .ident = NULL }
 };
 
@@ -627,8 +628,9 @@ static void __exit hdaps_exit(void)
 module_init(hdaps_init);
 module_exit(hdaps_exit);
 
-module_param_named(invert, hdaps_invert, bool, 0);
-MODULE_PARM_DESC(invert, "invert data along each axis");
+module_param_named(invert, hdaps_invert, int, 0644);
+MODULE_PARM_DESC(invert, "invert data along each axis. 1 invert x-axis, "
+		 "2 invert y-axis, 3 invert both axes.");
 
 MODULE_AUTHOR("Robert Love");
 MODULE_DESCRIPTION("IBM Hard Drive Active Protection System (HDAPS) driver");

[PATCHv3] hwmon/hdaps: Fix bug 7154 inversion of separate axis

[Frank Seidel]

From: Frank Seidel <frank@f-seidel.de>

Fix for kernel.org bug #7154:hdaps inversion of
each axis. This version is based on the work
from Michael Ruoss <miruoss@student.ethz.ch>.

Signed-off-by: Frank Seidel <frank@f-seidel.de>
---
 drivers/hwmon/hdaps.c |   64 +++++++++++++++++++++++++-------------------------
 1 file changed, 33 insertions(+), 31 deletions(-)

--- a/drivers/hwmon/hdaps.c
+++ b/drivers/hwmon/hdaps.c
@@ -65,6 +65,10 @@
 #define HDAPS_INPUT_FUZZ	4	/* input event threshold */
 #define HDAPS_INPUT_FLAT	4
 
+#define HDAPS_X_AXIS		(1 << 0)
+#define HDAPS_Y_AXIS		(1 << 1)
+#define HDAPS_BOTH_AXES		(HDAPS_X_AXIS | HDAPS_Y_AXIS)
+
 static struct platform_device *pdev;
 static struct input_polled_dev *hdaps_idev;
 static unsigned int hdaps_invert;
@@ -182,11 +186,11 @@ static int __hdaps_read_pair(unsigned in
 	km_activity = inb(HDAPS_PORT_KMACT);
 	__device_complete();
 
-	/* if hdaps_invert is set, negate the two values */
-	if (hdaps_invert) {
+	/* hdaps_invert is a bitvector to negate the axes */
+	if (hdaps_invert & HDAPS_X_AXIS)
 		*x = -*x;
+	if (hdaps_invert & HDAPS_Y_AXIS)
 		*y = -*y;
-	}
 
 	return 0;
 }
@@ -436,7 +440,8 @@ static ssize_t hdaps_invert_store(struct
 {
 	int invert;
 
-	if (sscanf(buf, "%d", &invert) != 1 || (invert != 1 && invert != 0))
+	if (sscanf(buf, "%d", &invert) != 1 ||
+	    invert < 0 || invert > HDAPS_BOTH_AXES)
 		return -EINVAL;
 
 	hdaps_invert = invert;
@@ -483,56 +488,52 @@ static int __init hdaps_dmi_match(const
 /* hdaps_dmi_match_invert - found an inverted match. */
 static int __init hdaps_dmi_match_invert(const struct dmi_system_id *id)
 {
-	hdaps_invert = 1;
-	printk(KERN_INFO "hdaps: inverting axis readings.\n");
+	hdaps_invert = (unsigned long)id->driver_data;
+	printk(KERN_INFO "hdaps: inverting axis (%u) readings.\n",
+	       hdaps_invert);
 	return hdaps_dmi_match(id);
 }
 
-#define HDAPS_DMI_MATCH_NORMAL(vendor, model) {		\
-	.ident = vendor " " model,			\
-	.callback = hdaps_dmi_match,			\
-	.matches = {					\
-		DMI_MATCH(DMI_BOARD_VENDOR, vendor),	\
-		DMI_MATCH(DMI_PRODUCT_VERSION, model)	\
-	}						\
-}
-
-#define HDAPS_DMI_MATCH_INVERT(vendor, model) {		\
+#define HDAPS_DMI_MATCH_INVERT(vendor, model, axes) {	\
 	.ident = vendor " " model,			\
 	.callback = hdaps_dmi_match_invert,		\
+	.driver_data = (void *)axes,			\
 	.matches = {					\
 		DMI_MATCH(DMI_BOARD_VENDOR, vendor),	\
 		DMI_MATCH(DMI_PRODUCT_VERSION, model)	\
 	}						\
 }
 
+#define HDAPS_DMI_MATCH_NORMAL(vendor, model)		\
+	HDAPS_DMI_MATCH_INVERT(vendor, model, 0)
+
 /* Note that HDAPS_DMI_MATCH_NORMAL("ThinkPad T42") would match
    "ThinkPad T42p", so the order of the entries matters.
    If your ThinkPad is not recognized, please update to latest
    BIOS. This is especially the case for some R52 ThinkPads. */
 static struct dmi_system_id __initdata hdaps_whitelist[] = {
-	HDAPS_DMI_MATCH_INVERT("IBM", "ThinkPad R50p"),
+	HDAPS_DMI_MATCH_INVERT("IBM", "ThinkPad R50p", HDAPS_BOTH_AXES),
 	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad R50"),
 	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad R51"),
 	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad R52"),
-	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad R61i"),
-	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad R61"),
-	HDAPS_DMI_MATCH_INVERT("IBM", "ThinkPad T41p"),
+	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad R61i", HDAPS_BOTH_AXES),
+	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad R61", HDAPS_BOTH_AXES),
+	HDAPS_DMI_MATCH_INVERT("IBM", "ThinkPad T41p", HDAPS_BOTH_AXES),
 	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad T41"),
-	HDAPS_DMI_MATCH_INVERT("IBM", "ThinkPad T42p"),
+	HDAPS_DMI_MATCH_INVERT("IBM", "ThinkPad T42p", HDAPS_BOTH_AXES),
 	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad T42"),
 	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad T43"),
-	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad T60"),
-	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad T61p"),
-	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad T61"),
+	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad T60", HDAPS_BOTH_AXES),
+	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad T61p", HDAPS_BOTH_AXES),
+	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad T61", HDAPS_BOTH_AXES),
 	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad X40"),
 	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad X41"),
-	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad X60"),
-	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad X61s"),
-	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad X61"),
+	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad X60", HDAPS_BOTH_AXES),
+	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad X61s", HDAPS_BOTH_AXES),
+	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad X61", HDAPS_BOTH_AXES),
 	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad Z60m"),
-	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad Z61m"),
-	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad Z61p"),
+	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad Z61m", HDAPS_BOTH_AXES),
+	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad Z61p", HDAPS_BOTH_AXES),
 	{ .ident = NULL }
 };
 
@@ -627,8 +628,9 @@ static void __exit hdaps_exit(void)
 module_init(hdaps_init);
 module_exit(hdaps_exit);
 
-module_param_named(invert, hdaps_invert, bool, 0);
-MODULE_PARM_DESC(invert, "invert data along each axis");
+module_param_named(invert, hdaps_invert, int, 0644);
+MODULE_PARM_DESC(invert, "invert data along each axis. 1 invert x-axis, "
+		 "2 invert y-axis, 3 invert both axes.");
 
 MODULE_AUTHOR("Robert Love");
 MODULE_DESCRIPTION("IBM Hard Drive Active Protection System (HDAPS) driver");

Re: [PATCHv2] hwmon/hdaps: Fix bug 7154 inversion of separate axis

[Frank Seidel]

Sorry for my late response an that "bad" version (spitting a
compile warning).

Thanks,
Frank

Re: [PATCH] hwmon/hdaps: Fix bug 7154 inversion of separate axis

[Jean Delvare]

On Sat, 14 Feb 2009 14:25:56 -0800, Dmitry Torokhov wrote:
> On Fri, Feb 13, 2009 at 01:38:01PM +0100, Frank Seidel wrote:
> > -module_param_named(invert, hdaps_invert, bool, 0);
> > -MODULE_PARM_DESC(invert, "invert data along each axis");
> > +module_param_named(invert, hdaps_invert, int, 0);
> > +MODULE_PARM_DESC(invert, "invert data along each axis. 1 invert x-axis, "
> > +		 "2 invert y-axis, 3 invert both axes.");
> >  
> 
> Why don't you make these 0644? I don't see why they can't be changed
> "on fly".

hdaps_invert can already be read and written via a sysfs attribute. We
really don't need 2 ways to do it, so either the sysfs file or the
module parameter should be dropped.

-- 
Jean Delvare

Re: [PATCH] hwmon/hdaps: Fix bug 7154 inversion of separate axis

[Matthew Garrett]

On Sun, Feb 15, 2009 at 10:27:07AM +0100, Jean Delvare wrote:
> On Sat, 14 Feb 2009 19:32:02 -0500, Shem Multinymous wrote:
> > See the table at the bottom of the tp_smapi page
> > (http://www.thinkwiki.org/wiki/Tp_smapi), or hdaps.c inside the
> > tp_smapi package, for more model-specific information.
> > It would be nice if you made the interface (constants and their
> > meaning) the same as in the tp_smapi version of hdaps, which is
> > already widely deployed and packaged by several distros.
> 
> Why is this code not upstream? Ah yeah, I remember now: because it was
> written by an anonymous developer, which makes the contribution legally
> dubious. Copying such code into the upstream version of hdaps would be
> no different, so we cannot do that, sorry.

You're arguing that sysfs interfaces are copyrightable?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCHv3] hwmon/hdaps: Fix bug 7154 inversion of separate axis

[Jean Delvare]

Hi Frank,

On Sun, 15 Feb 2009 14:01:00 +0100, Frank Seidel wrote:
> From: Frank Seidel <frank@f-seidel.de>
> 
> Fix for kernel.org bug #7154:hdaps inversion of
> each axis. This version is based on the work
> from Michael Ruoss <miruoss@student.ethz.ch>.
> 
> Signed-off-by: Frank Seidel <frank@f-seidel.de>
> ---
>  drivers/hwmon/hdaps.c |   64 +++++++++++++++++++++++++-------------------------
>  1 file changed, 33 insertions(+), 31 deletions(-)

Patch applied, thanks. Suggestions for incremental patches:

* Actually fix bug #7154 (apparently Thinkpad X41 needs to invert Y
  axis)
* Get rid of sysfs attribute "invert" (redundant with eponymous module
  parameter)

-- 
Jean Delvare

Re: [PATCH] hwmon/hdaps: Fix bug 7154 inversion of separate axis

[Jean Delvare]

On Sun, 15 Feb 2009 13:41:15 +0000, Matthew Garrett wrote:
> On Sun, Feb 15, 2009 at 10:27:07AM +0100, Jean Delvare wrote:
> > On Sat, 14 Feb 2009 19:32:02 -0500, Shem Multinymous wrote:
> > > See the table at the bottom of the tp_smapi page
> > > (http://www.thinkwiki.org/wiki/Tp_smapi), or hdaps.c inside the
> > > tp_smapi package, for more model-specific information.
> > > It would be nice if you made the interface (constants and their
> > > meaning) the same as in the tp_smapi version of hdaps, which is
> > > already widely deployed and packaged by several distros.
> > 
> > Why is this code not upstream? Ah yeah, I remember now: because it was
> > written by an anonymous developer, which makes the contribution legally
> > dubious. Copying such code into the upstream version of hdaps would be
> > no different, so we cannot do that, sorry.
> 
> You're arguing that sysfs interfaces are copyrightable?

I'm not arguing anything. I'll let you, Shame Multithing and anyone
else with too much spare time argue whatever you want. Me, I have many
more useful things to do.

Thanks but no thanks,
-- 
Jean Delvare

Re: [PATCH] hwmon/hdaps: Fix bug 7154 inversion of separate axis

[Matthew Garrett]

On Sun, Feb 15, 2009 at 04:23:59PM +0100, Jean Delvare wrote:
> On Sun, 15 Feb 2009 13:41:15 +0000, Matthew Garrett wrote:
> > You're arguing that sysfs interfaces are copyrightable?
> 
> I'm not arguing anything. I'll let you, Shame Multithing and anyone
> else with too much spare time argue whatever you want. Me, I have many
> more useful things to do.

Adopting an interface that's already in use in the wild is a perfectly 
reasonable thing to do. The argument is over whether the tp_smapi driver 
itself has IP issues, not over whether its sysfs interface does.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

[PATCH] hwmon/hdaps: Fix bug 7154 adaption for Thinkpad X41

[Frank Seidel]

From: Frank Seidel <frank@f-seidel.de>

Fix for kernel.org bug #7154:hdaps inversion of
actual Thinkpad X41's Y-axis.

Signed-off-by: Frank Seidel <frank@f-seidel.de>
---
 drivers/hwmon/hdaps.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/hwmon/hdaps.c
+++ b/drivers/hwmon/hdaps.c
@@ -527,7 +527,7 @@ static struct dmi_system_id __initdata h
 	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad T61p", HDAPS_BOTH_AXES),
 	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad T61", HDAPS_BOTH_AXES),
 	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad X40"),
-	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad X41"),
+	HDAPS_DMI_MATCH_INVERT("IBM", "ThinkPad X41", HDAPS_Y_AXIS),
 	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad X60", HDAPS_BOTH_AXES),
 	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad X61s", HDAPS_BOTH_AXES),
 	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad X61", HDAPS_BOTH_AXES),

[PATCH] hwmon/hdaps: remove redundant sysfs invert

[Frank Seidel]

From: Frank Seidel <frank@f-seidel.de>

Get rid of sysfs attribute "invert" as its
redundant to module parameter.

Signed-off-by: Frank Seidel <frank@f-seidel.de>
---
 drivers/hwmon/hdaps.c |   24 ------------------------
 1 file changed, 24 deletions(-)

--- a/drivers/hwmon/hdaps.c
+++ b/drivers/hwmon/hdaps.c
@@ -428,28 +428,6 @@ static ssize_t hdaps_calibrate_store(str
 	return count;
 }
 
-static ssize_t hdaps_invert_show(struct device *dev,
-				 struct device_attribute *attr, char *buf)
-{
-	return sprintf(buf, "%u\n", hdaps_invert);
-}
-
-static ssize_t hdaps_invert_store(struct device *dev,
-				  struct device_attribute *attr,
-				  const char *buf, size_t count)
-{
-	int invert;
-
-	if (sscanf(buf, "%d", &invert) != 1 ||
-	    invert < 0 || invert > HDAPS_BOTH_AXES)
-		return -EINVAL;
-
-	hdaps_invert = invert;
-	hdaps_calibrate();
-
-	return count;
-}
-
 static DEVICE_ATTR(position, 0444, hdaps_position_show, NULL);
 static DEVICE_ATTR(variance, 0444, hdaps_variance_show, NULL);
 static DEVICE_ATTR(temp1, 0444, hdaps_temp1_show, NULL);
@@ -457,7 +435,6 @@ static DEVICE_ATTR(temp2, 0444, hdaps_te
 static DEVICE_ATTR(keyboard_activity, 0444, hdaps_keyboard_activity_show, NULL);
 static DEVICE_ATTR(mouse_activity, 0444, hdaps_mouse_activity_show, NULL);
 static DEVICE_ATTR(calibrate, 0644, hdaps_calibrate_show,hdaps_calibrate_store);
-static DEVICE_ATTR(invert, 0644, hdaps_invert_show, hdaps_invert_store);
 
 static struct attribute *hdaps_attributes[] = {
 	&dev_attr_position.attr,
@@ -467,7 +444,6 @@ static struct attribute *hdaps_attribute
 	&dev_attr_keyboard_activity.attr,
 	&dev_attr_mouse_activity.attr,
 	&dev_attr_calibrate.attr,
-	&dev_attr_invert.attr,
 	NULL,
 };
 

Re: [PATCH] hwmon/hdaps: Fix bug 7154 adaption for Thinkpad X41

[Jean Delvare]

Hallo Frank,

On Thu, 19 Feb 2009 13:43:05 +0100, Frank Seidel wrote:
> From: Frank Seidel <frank@f-seidel.de>
> 
> Fix for kernel.org bug #7154:hdaps inversion of
> actual Thinkpad X41's Y-axis.
> 
> Signed-off-by: Frank Seidel <frank@f-seidel.de>
> ---
>  drivers/hwmon/hdaps.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- a/drivers/hwmon/hdaps.c
> +++ b/drivers/hwmon/hdaps.c
> @@ -527,7 +527,7 @@ static struct dmi_system_id __initdata h
>  	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad T61p", HDAPS_BOTH_AXES),
>  	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad T61", HDAPS_BOTH_AXES),
>  	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad X40"),
> -	HDAPS_DMI_MATCH_NORMAL("IBM", "ThinkPad X41"),
> +	HDAPS_DMI_MATCH_INVERT("IBM", "ThinkPad X41", HDAPS_Y_AXIS),
>  	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad X60", HDAPS_BOTH_AXES),
>  	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad X61s", HDAPS_BOTH_AXES),
>  	HDAPS_DMI_MATCH_INVERT("LENOVO", "ThinkPad X61", HDAPS_BOTH_AXES),
> 

Applied, thanks.

-- 
Jean Delvare

Re: [PATCH] hwmon/hdaps: remove redundant sysfs invert

[Jean Delvare]

Hi Frank,

On Thu, 19 Feb 2009 13:44:43 +0100, Frank Seidel wrote:
> From: Frank Seidel <frank@f-seidel.de>
> 
> Get rid of sysfs attribute "invert" as its
> redundant to module parameter.

It seems it isn't that easy:

> 
> Signed-off-by: Frank Seidel <frank@f-seidel.de>
> ---
>  drivers/hwmon/hdaps.c |   24 ------------------------
>  1 file changed, 24 deletions(-)
> 
> --- a/drivers/hwmon/hdaps.c
> +++ b/drivers/hwmon/hdaps.c
> @@ -428,28 +428,6 @@ static ssize_t hdaps_calibrate_store(str
>  	return count;
>  }
>  
> -static ssize_t hdaps_invert_show(struct device *dev,
> -				 struct device_attribute *attr, char *buf)
> -{
> -	return sprintf(buf, "%u\n", hdaps_invert);
> -}
> -
> -static ssize_t hdaps_invert_store(struct device *dev,
> -				  struct device_attribute *attr,
> -				  const char *buf, size_t count)
> -{
> -	int invert;
> -
> -	if (sscanf(buf, "%d", &invert) != 1 ||
> -	    invert < 0 || invert > HDAPS_BOTH_AXES)
> -		return -EINVAL;
> -
> -	hdaps_invert = invert;
> -	hdaps_calibrate();

Apparently recalibration is necessary when you change the inversion
settings. The module parameter, which you made writable in a previous
patch, does _not_ recalibrate when changed.

So it was probably not such a good idea to make the module parameter
writable, and that should be reverted. Which in turn means that the
sysfs attribute "invert" has to stay.

> -
> -	return count;
> -}
> -
>  static DEVICE_ATTR(position, 0444, hdaps_position_show, NULL);
>  static DEVICE_ATTR(variance, 0444, hdaps_variance_show, NULL);
>  static DEVICE_ATTR(temp1, 0444, hdaps_temp1_show, NULL);
> @@ -457,7 +435,6 @@ static DEVICE_ATTR(temp2, 0444, hdaps_te
>  static DEVICE_ATTR(keyboard_activity, 0444, hdaps_keyboard_activity_show, NULL);
>  static DEVICE_ATTR(mouse_activity, 0444, hdaps_mouse_activity_show, NULL);
>  static DEVICE_ATTR(calibrate, 0644, hdaps_calibrate_show,hdaps_calibrate_store);
> -static DEVICE_ATTR(invert, 0644, hdaps_invert_show, hdaps_invert_store);
>  
>  static struct attribute *hdaps_attributes[] = {
>  	&dev_attr_position.attr,
> @@ -467,7 +444,6 @@ static struct attribute *hdaps_attribute
>  	&dev_attr_keyboard_activity.attr,
>  	&dev_attr_mouse_activity.attr,
>  	&dev_attr_calibrate.attr,
> -	&dev_attr_invert.attr,
>  	NULL,
>  };
>  


-- 
Jean Delvare

Re: [PATCH] hwmon/hdaps: remove redundant sysfs invert

[Frank Seidel]

Hi,

Jean Delvare wrote:
> On Thu, 19 Feb 2009 13:44:43 +0100, Frank Seidel wrote:
>> -	hdaps_invert = invert;
>> -	hdaps_calibrate();
> 
> Apparently recalibration is necessary when you change the inversion
> settings. The module parameter, which you made writable in a previous
> patch, does _not_ recalibrate when changed.
> 
> So it was probably not such a good idea to make the module parameter
> writable, and that should be reverted. Which in turn means that the
> sysfs attribute "invert" has to stay.

Fully agree. I'll post a module-writeable revert patch tomorrow.

Thanks,
Frank

Re: [PATCH] hwmon/hdaps: remove redundant sysfs invert

[Jean Delvare]

On Thu, 19 Feb 2009 21:00:27 +0100, Frank Seidel wrote:
> Hi,
> 
> Jean Delvare wrote:
> > On Thu, 19 Feb 2009 13:44:43 +0100, Frank Seidel wrote:
> >> -	hdaps_invert = invert;
> >> -	hdaps_calibrate();
> > 
> > Apparently recalibration is necessary when you change the inversion
> > settings. The module parameter, which you made writable in a previous
> > patch, does _not_ recalibrate when changed.
> > 
> > So it was probably not such a good idea to make the module parameter
> > writable, and that should be reverted. Which in turn means that the
> > sysfs attribute "invert" has to stay.
> 
> Fully agree. I'll post a module-writeable revert patch tomorrow.

Don't bother, I've reverted this specific part myself already.

-- 
Jean Delvare

Re: [PATCH] hwmon/hdaps: remove redundant sysfs invert

[Frank Seidel]

Jean Delvare wrote:
> Don't bother, I've reverted this specific part myself already.

Oh, great :-) 

Thanks a lot,
Frank