* Re: [PATCH] hwmon/hdaps: Fix bug 7154 inversion of separate axis
2009-02-13 12:38 [PATCH] hwmon/hdaps: Fix bug 7154 inversion of separate axis Frank Seidel
@ 2009-02-14 22:25 ` Dmitry Torokhov
2009-02-15 13:34 ` Jean Delvare
2009-02-15 0:32 ` Shem Multinymous
` (2 subsequent siblings)
3 siblings, 1 reply; 20+ messages in thread
From: Dmitry Torokhov @ 2009-02-14 22:25 UTC (permalink / raw)
To: Frank Seidel
Cc: linux kernel, akpm, rlove, Jean Delvare, protasnb, Michael Ruoss,
Tim Gardner, Frank Seidel
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] hwmon/hdaps: Fix bug 7154 inversion of separate axis
2009-02-14 22:25 ` Dmitry Torokhov
@ 2009-02-15 13:34 ` Jean Delvare
0 siblings, 0 replies; 20+ messages in thread
From: Jean Delvare @ 2009-02-15 13:34 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Frank Seidel, linux kernel, akpm, rlove, protasnb, Michael Ruoss,
Tim Gardner, Frank Seidel
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] hwmon/hdaps: Fix bug 7154 inversion of separate axis
2009-02-13 12:38 [PATCH] hwmon/hdaps: Fix bug 7154 inversion of separate axis Frank Seidel
2009-02-14 22:25 ` Dmitry Torokhov
@ 2009-02-15 0:32 ` Shem Multinymous
2009-02-15 9:27 ` Jean Delvare
2009-02-15 9:36 ` Jean Delvare
2009-02-15 12:16 ` [PATCHv2] " Frank Seidel
3 siblings, 1 reply; 20+ messages in thread
From: Shem Multinymous @ 2009-02-15 0:32 UTC (permalink / raw)
To: Frank Seidel
Cc: linux kernel, akpm, rlove, Jean Delvare, protasnb, Michael Ruoss,
Dmitry Torokhov, Tim Gardner, Frank Seidel
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] hwmon/hdaps: Fix bug 7154 inversion of separate axis
2009-02-15 0:32 ` Shem Multinymous
@ 2009-02-15 9:27 ` Jean Delvare
2009-02-15 13:41 ` Matthew Garrett
0 siblings, 1 reply; 20+ messages in thread
From: Jean Delvare @ 2009-02-15 9:27 UTC (permalink / raw)
To: Shem Multinymous
Cc: Frank Seidel, linux kernel, akpm, rlove, protasnb, Michael Ruoss,
Dmitry Torokhov, Tim Gardner, Frank Seidel
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] hwmon/hdaps: Fix bug 7154 inversion of separate axis
2009-02-15 9:27 ` Jean Delvare
@ 2009-02-15 13:41 ` Matthew Garrett
2009-02-15 15:23 ` Jean Delvare
0 siblings, 1 reply; 20+ messages in thread
From: Matthew Garrett @ 2009-02-15 13:41 UTC (permalink / raw)
To: Jean Delvare
Cc: Shem Multinymous, Frank Seidel, linux kernel, akpm, rlove,
protasnb, Michael Ruoss, Dmitry Torokhov, Tim Gardner,
Frank Seidel
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] hwmon/hdaps: Fix bug 7154 inversion of separate axis
2009-02-15 13:41 ` Matthew Garrett
@ 2009-02-15 15:23 ` Jean Delvare
2009-02-15 15:28 ` Matthew Garrett
0 siblings, 1 reply; 20+ messages in thread
From: Jean Delvare @ 2009-02-15 15:23 UTC (permalink / raw)
To: Matthew Garrett
Cc: Shem Multinymous, Frank Seidel, linux kernel, akpm, rlove,
protasnb, Michael Ruoss, Dmitry Torokhov, Tim Gardner,
Frank Seidel
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] hwmon/hdaps: Fix bug 7154 inversion of separate axis
2009-02-15 15:23 ` Jean Delvare
@ 2009-02-15 15:28 ` Matthew Garrett
0 siblings, 0 replies; 20+ messages in thread
From: Matthew Garrett @ 2009-02-15 15:28 UTC (permalink / raw)
To: Jean Delvare
Cc: Shem Multinymous, Frank Seidel, linux kernel, akpm, rlove,
protasnb, Michael Ruoss, Dmitry Torokhov, Tim Gardner,
Frank Seidel
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] hwmon/hdaps: Fix bug 7154 inversion of separate axis
2009-02-13 12:38 [PATCH] hwmon/hdaps: Fix bug 7154 inversion of separate axis Frank Seidel
2009-02-14 22:25 ` Dmitry Torokhov
2009-02-15 0:32 ` Shem Multinymous
@ 2009-02-15 9:36 ` Jean Delvare
2009-02-15 12:16 ` [PATCHv2] " Frank Seidel
3 siblings, 0 replies; 20+ messages in thread
From: Jean Delvare @ 2009-02-15 9:36 UTC (permalink / raw)
To: Frank Seidel
Cc: linux kernel, akpm, rlove, protasnb, Michael Ruoss,
Dmitry Torokhov, Tim Gardner, Frank Seidel
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
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCHv2] hwmon/hdaps: Fix bug 7154 inversion of separate axis
2009-02-13 12:38 [PATCH] hwmon/hdaps: Fix bug 7154 inversion of separate axis Frank Seidel
` (2 preceding siblings ...)
2009-02-15 9:36 ` Jean Delvare
@ 2009-02-15 12:16 ` Frank Seidel
2009-02-15 13:01 ` [PATCHv3] " Frank Seidel
2009-02-15 13:02 ` [PATCHv2] hwmon/hdaps: Fix bug 7154 inversion of separate axis Frank Seidel
3 siblings, 2 replies; 20+ messages in thread
From: Frank Seidel @ 2009-02-15 12:16 UTC (permalink / raw)
To: linux kernel
Cc: akpm, rlove, Jean Delvare, protasnb, Michael Ruoss,
Dmitry Torokhov, Tim Gardner, Frank Seidel, Frank Seidel,
multinymous
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");
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCHv3] hwmon/hdaps: Fix bug 7154 inversion of separate axis
2009-02-15 12:16 ` [PATCHv2] " Frank Seidel
@ 2009-02-15 13:01 ` Frank Seidel
2009-02-15 14:37 ` Jean Delvare
2009-02-15 13:02 ` [PATCHv2] hwmon/hdaps: Fix bug 7154 inversion of separate axis Frank Seidel
1 sibling, 1 reply; 20+ messages in thread
From: Frank Seidel @ 2009-02-15 13:01 UTC (permalink / raw)
To: linux kernel
Cc: akpm, rlove, Jean Delvare, protasnb, Michael Ruoss,
Dmitry Torokhov, Tim Gardner, Frank Seidel, multinymous,
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");
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCHv3] hwmon/hdaps: Fix bug 7154 inversion of separate axis
2009-02-15 13:01 ` [PATCHv3] " Frank Seidel
@ 2009-02-15 14:37 ` Jean Delvare
2009-02-19 12:43 ` [PATCH] hwmon/hdaps: Fix bug 7154 adaption for Thinkpad X41 Frank Seidel
2009-02-19 12:44 ` [PATCH] hwmon/hdaps: remove redundant sysfs invert Frank Seidel
0 siblings, 2 replies; 20+ messages in thread
From: Jean Delvare @ 2009-02-15 14:37 UTC (permalink / raw)
To: Frank Seidel
Cc: linux kernel, akpm, rlove, protasnb, Michael Ruoss,
Dmitry Torokhov, Tim Gardner, Frank Seidel, multinymous
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH] hwmon/hdaps: Fix bug 7154 adaption for Thinkpad X41
2009-02-15 14:37 ` Jean Delvare
@ 2009-02-19 12:43 ` Frank Seidel
2009-02-19 17:16 ` Jean Delvare
2009-02-19 12:44 ` [PATCH] hwmon/hdaps: remove redundant sysfs invert Frank Seidel
1 sibling, 1 reply; 20+ messages in thread
From: Frank Seidel @ 2009-02-19 12:43 UTC (permalink / raw)
To: Jean Delvare
Cc: linux kernel, akpm, rlove, protasnb, Michael Ruoss,
Dmitry Torokhov, Tim Gardner, Frank Seidel, multinymous,
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),
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] hwmon/hdaps: Fix bug 7154 adaption for Thinkpad X41
2009-02-19 12:43 ` [PATCH] hwmon/hdaps: Fix bug 7154 adaption for Thinkpad X41 Frank Seidel
@ 2009-02-19 17:16 ` Jean Delvare
0 siblings, 0 replies; 20+ messages in thread
From: Jean Delvare @ 2009-02-19 17:16 UTC (permalink / raw)
To: Frank Seidel
Cc: linux kernel, akpm, rlove, protasnb, Michael Ruoss,
Dmitry Torokhov, Tim Gardner, Frank Seidel, multinymous,
Frank Seidel
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH] hwmon/hdaps: remove redundant sysfs invert
2009-02-15 14:37 ` Jean Delvare
2009-02-19 12:43 ` [PATCH] hwmon/hdaps: Fix bug 7154 adaption for Thinkpad X41 Frank Seidel
@ 2009-02-19 12:44 ` Frank Seidel
2009-02-19 17:27 ` Jean Delvare
1 sibling, 1 reply; 20+ messages in thread
From: Frank Seidel @ 2009-02-19 12:44 UTC (permalink / raw)
To: Jean Delvare
Cc: linux kernel, akpm, rlove, protasnb, Michael Ruoss,
Dmitry Torokhov, Tim Gardner, Frank Seidel, multinymous,
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,
};
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] hwmon/hdaps: remove redundant sysfs invert
2009-02-19 12:44 ` [PATCH] hwmon/hdaps: remove redundant sysfs invert Frank Seidel
@ 2009-02-19 17:27 ` Jean Delvare
2009-02-19 20:00 ` Frank Seidel
0 siblings, 1 reply; 20+ messages in thread
From: Jean Delvare @ 2009-02-19 17:27 UTC (permalink / raw)
To: Frank Seidel
Cc: linux kernel, akpm, rlove, protasnb, Michael Ruoss,
Dmitry Torokhov, Tim Gardner, Frank Seidel, multinymous,
Frank Seidel
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] hwmon/hdaps: remove redundant sysfs invert
2009-02-19 17:27 ` Jean Delvare
@ 2009-02-19 20:00 ` Frank Seidel
2009-02-19 20:34 ` Jean Delvare
0 siblings, 1 reply; 20+ messages in thread
From: Frank Seidel @ 2009-02-19 20:00 UTC (permalink / raw)
To: Jean Delvare
Cc: linux kernel, akpm, rlove, protasnb, Michael Ruoss,
Dmitry Torokhov, Tim Gardner, Frank Seidel, multinymous
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] hwmon/hdaps: remove redundant sysfs invert
2009-02-19 20:00 ` Frank Seidel
@ 2009-02-19 20:34 ` Jean Delvare
2009-02-20 8:35 ` Frank Seidel
0 siblings, 1 reply; 20+ messages in thread
From: Jean Delvare @ 2009-02-19 20:34 UTC (permalink / raw)
To: Frank Seidel
Cc: linux kernel, akpm, rlove, protasnb, Michael Ruoss,
Dmitry Torokhov, Tim Gardner, Frank Seidel, multinymous
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] hwmon/hdaps: remove redundant sysfs invert
2009-02-19 20:34 ` Jean Delvare
@ 2009-02-20 8:35 ` Frank Seidel
0 siblings, 0 replies; 20+ messages in thread
From: Frank Seidel @ 2009-02-20 8:35 UTC (permalink / raw)
To: Jean Delvare
Cc: Frank Seidel, linux kernel, akpm, rlove, protasnb, Michael Ruoss,
Dmitry Torokhov, Tim Gardner, multinymous
Jean Delvare wrote:
> Don't bother, I've reverted this specific part myself already.
Oh, great :-)
Thanks a lot,
Frank
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCHv2] hwmon/hdaps: Fix bug 7154 inversion of separate axis
2009-02-15 12:16 ` [PATCHv2] " Frank Seidel
2009-02-15 13:01 ` [PATCHv3] " Frank Seidel
@ 2009-02-15 13:02 ` Frank Seidel
1 sibling, 0 replies; 20+ messages in thread
From: Frank Seidel @ 2009-02-15 13:02 UTC (permalink / raw)
To: linux kernel
Cc: akpm, rlove, Jean Delvare, protasnb, Michael Ruoss,
Dmitry Torokhov, Tim Gardner, Frank Seidel, multinymous
Sorry for my late response an that "bad" version (spitting a
compile warning).
Thanks,
Frank
^ permalink raw reply [flat|nested] 20+ messages in thread