[PATCH] lis3: Add axes module parameter for custom axis-mapping

[PATCH] lis3: Add axes module parameter for custom axis-mapping

[Takashi Iwai]

The axis-mapping of lis3dev device on many (rather most) HP machines
doesn't follow the standard.  When each new model appears, users need
to adjust again.  Testing this requires the rebuild of kernel, thus
it's not trivial for end-users.

This patch adds a module parameter "axes" to allow a custom
axis-mapping without patching and recompiling the kernel driver.
User can pass the parameter such as axes=3,2,1.  Also it can be
changed via sysfs.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/hwmon/hp_accel.c  |    5 ++++-
 drivers/hwmon/lis3lv02d.c |   45 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 1 deletions(-)

diff --git a/drivers/hwmon/hp_accel.c b/drivers/hwmon/hp_accel.c
index 7580f55..3462a5d 100644
--- a/drivers/hwmon/hp_accel.c
+++ b/drivers/hwmon/hp_accel.c
@@ -297,7 +297,10 @@ static int lis3lv02d_add(struct acpi_device *device)
 	lis3lv02d_enum_resources(device);
 
 	/* If possible use a "standard" axes order */
-	if (dmi_check_system(lis3lv02d_dmi_ids) == 0) {
+	if (lis3_dev.ac.x && lis3_dev.ac.y && lis3_dev.ac.z) {
+		printk(KERN_INFO DRIVER_NAME ": Using custom axes %d,%d,%d\n",
+		       lis3_dev.ac.x, lis3_dev.ac.y, lis3_dev.ac.z);
+	} else if (dmi_check_system(lis3lv02d_dmi_ids) == 0) {
 		printk(KERN_INFO DRIVER_NAME ": laptop model unknown, "
 				 "using default axes configuration\n");
 		lis3_dev.ac = lis3lv02d_axis_normal;
diff --git a/drivers/hwmon/lis3lv02d.c b/drivers/hwmon/lis3lv02d.c
index 6138f03..4b179d5 100644
--- a/drivers/hwmon/lis3lv02d.c
+++ b/drivers/hwmon/lis3lv02d.c
@@ -75,6 +75,51 @@ struct lis3lv02d lis3_dev = {
 
 EXPORT_SYMBOL_GPL(lis3_dev);
 
+static int axis_array[3];
+static void copy_to_axis_array(void)
+{
+	axis_array[0] = lis3_dev.ac.x;
+	axis_array[1] = lis3_dev.ac.y;
+	axis_array[2] = lis3_dev.ac.z;
+}
+
+static void copy_from_axis_array(void)
+{
+	lis3_dev.ac.x = axis_array[0];
+	lis3_dev.ac.y = axis_array[1];
+	lis3_dev.ac.z = axis_array[2];
+}
+
+static int param_set_axis(const char *val, const struct kernel_param *kp)
+{
+	int ret;
+	copy_to_axis_array();
+	ret = param_set_int(val, kp);
+	if (!ret) {
+		int val = *(int *)kp->arg;
+		if (val < 0)
+			val = -val;
+		if (!val || val > 3)
+			return -EINVAL;
+		copy_from_axis_array();
+	}
+	return ret;
+}
+
+static int param_get_axis(char *buffer, const struct kernel_param *kp)
+{
+	copy_to_axis_array();
+	return param_get_int(buffer, kp);
+}
+
+static struct kernel_param_ops param_ops_axis = {
+	.set = param_set_axis,
+	.get = param_get_axis,
+};
+
+module_param_array_named(axes, axis_array, axis, NULL, 0644);
+MODULE_PARM_DESC(axes, "Axis-mapping for x,y,z directions");
+
 static s16 lis3lv02d_read_8(struct lis3lv02d *lis3, int reg)
 {
 	s8 lo;
-- 
1.7.2.1

Re: [PATCH] lis3: Add axes module parameter for custom axis-mapping

[Takashi Iwai]

At Mon, 23 Aug 2010 14:23:25 +0200,
Takashi Iwai wrote:
> 
> The axis-mapping of lis3dev device on many (rather most) HP machines
> doesn't follow the standard.  When each new model appears, users need
> to adjust again.  Testing this requires the rebuild of kernel, thus
> it's not trivial for end-users.
> 
> This patch adds a module parameter "axes" to allow a custom
> axis-mapping without patching and recompiling the kernel driver.
> User can pass the parameter such as axes=3,2,1.  Also it can be
> changed via sysfs.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

Can anyone review / take this?
Jean, should it be through hwmon tree? 


thanks,

Takashi

> ---
>  drivers/hwmon/hp_accel.c  |    5 ++++-
>  drivers/hwmon/lis3lv02d.c |   45 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 49 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/hwmon/hp_accel.c b/drivers/hwmon/hp_accel.c
> index 7580f55..3462a5d 100644
> --- a/drivers/hwmon/hp_accel.c
> +++ b/drivers/hwmon/hp_accel.c
> @@ -297,7 +297,10 @@ static int lis3lv02d_add(struct acpi_device *device)
>  	lis3lv02d_enum_resources(device);
>  
>  	/* If possible use a "standard" axes order */
> -	if (dmi_check_system(lis3lv02d_dmi_ids) == 0) {
> +	if (lis3_dev.ac.x && lis3_dev.ac.y && lis3_dev.ac.z) {
> +		printk(KERN_INFO DRIVER_NAME ": Using custom axes %d,%d,%d\n",
> +		       lis3_dev.ac.x, lis3_dev.ac.y, lis3_dev.ac.z);
> +	} else if (dmi_check_system(lis3lv02d_dmi_ids) == 0) {
>  		printk(KERN_INFO DRIVER_NAME ": laptop model unknown, "
>  				 "using default axes configuration\n");
>  		lis3_dev.ac = lis3lv02d_axis_normal;
> diff --git a/drivers/hwmon/lis3lv02d.c b/drivers/hwmon/lis3lv02d.c
> index 6138f03..4b179d5 100644
> --- a/drivers/hwmon/lis3lv02d.c
> +++ b/drivers/hwmon/lis3lv02d.c
> @@ -75,6 +75,51 @@ struct lis3lv02d lis3_dev = {
>  
>  EXPORT_SYMBOL_GPL(lis3_dev);
>  
> +static int axis_array[3];
> +static void copy_to_axis_array(void)
> +{
> +	axis_array[0] = lis3_dev.ac.x;
> +	axis_array[1] = lis3_dev.ac.y;
> +	axis_array[2] = lis3_dev.ac.z;
> +}
> +
> +static void copy_from_axis_array(void)
> +{
> +	lis3_dev.ac.x = axis_array[0];
> +	lis3_dev.ac.y = axis_array[1];
> +	lis3_dev.ac.z = axis_array[2];
> +}
> +
> +static int param_set_axis(const char *val, const struct kernel_param *kp)
> +{
> +	int ret;
> +	copy_to_axis_array();
> +	ret = param_set_int(val, kp);
> +	if (!ret) {
> +		int val = *(int *)kp->arg;
> +		if (val < 0)
> +			val = -val;
> +		if (!val || val > 3)
> +			return -EINVAL;
> +		copy_from_axis_array();
> +	}
> +	return ret;
> +}
> +
> +static int param_get_axis(char *buffer, const struct kernel_param *kp)
> +{
> +	copy_to_axis_array();
> +	return param_get_int(buffer, kp);
> +}
> +
> +static struct kernel_param_ops param_ops_axis = {
> +	.set = param_set_axis,
> +	.get = param_get_axis,
> +};
> +
> +module_param_array_named(axes, axis_array, axis, NULL, 0644);
> +MODULE_PARM_DESC(axes, "Axis-mapping for x,y,z directions");
> +
>  static s16 lis3lv02d_read_8(struct lis3lv02d *lis3, int reg)
>  {
>  	s8 lo;
> -- 
> 1.7.2.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH] lis3: Add axes module parameter for custom axis-mapping

[Jean Delvare]

Hi Takashi,

On Tue, 07 Sep 2010 08:45:35 +0200, Takashi Iwai wrote:
> At Mon, 23 Aug 2010 14:23:25 +0200,
> Takashi Iwai wrote:
> > 
> > The axis-mapping of lis3dev device on many (rather most) HP machines
> > doesn't follow the standard.  When each new model appears, users need
> > to adjust again.  Testing this requires the rebuild of kernel, thus
> > it's not trivial for end-users.
> > 
> > This patch adds a module parameter "axes" to allow a custom
> > axis-mapping without patching and recompiling the kernel driver.
> > User can pass the parameter such as axes=3,2,1.  Also it can be
> > changed via sysfs.
> > 
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> 
> Can anyone review / take this?
> Jean, should it be through hwmon tree? 

Not really. The lis3lv02d driver shouldn't be in the hwmon directory to
start with, its features aren't ones I am familiar with, so it makes
little sense for me to pick patches. Better get it merged by Andrew.

At some point we will really have to move all these accelerometer
drivers to a different directory to avoid the confusion.

> > ---
> >  drivers/hwmon/hp_accel.c  |    5 ++++-
> >  drivers/hwmon/lis3lv02d.c |   45 +++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 49 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/hwmon/hp_accel.c b/drivers/hwmon/hp_accel.c
> > index 7580f55..3462a5d 100644
> > --- a/drivers/hwmon/hp_accel.c
> > +++ b/drivers/hwmon/hp_accel.c
> > @@ -297,7 +297,10 @@ static int lis3lv02d_add(struct acpi_device *device)
> >  	lis3lv02d_enum_resources(device);
> >  
> >  	/* If possible use a "standard" axes order */
> > -	if (dmi_check_system(lis3lv02d_dmi_ids) == 0) {
> > +	if (lis3_dev.ac.x && lis3_dev.ac.y && lis3_dev.ac.z) {
> > +		printk(KERN_INFO DRIVER_NAME ": Using custom axes %d,%d,%d\n",
> > +		       lis3_dev.ac.x, lis3_dev.ac.y, lis3_dev.ac.z);
> > +	} else if (dmi_check_system(lis3lv02d_dmi_ids) == 0) {
> >  		printk(KERN_INFO DRIVER_NAME ": laptop model unknown, "
> >  				 "using default axes configuration\n");
> >  		lis3_dev.ac = lis3lv02d_axis_normal;
> > diff --git a/drivers/hwmon/lis3lv02d.c b/drivers/hwmon/lis3lv02d.c
> > index 6138f03..4b179d5 100644
> > --- a/drivers/hwmon/lis3lv02d.c
> > +++ b/drivers/hwmon/lis3lv02d.c
> > @@ -75,6 +75,51 @@ struct lis3lv02d lis3_dev = {
> >  
> >  EXPORT_SYMBOL_GPL(lis3_dev);
> >  
> > +static int axis_array[3];
> > +static void copy_to_axis_array(void)
> > +{
> > +	axis_array[0] = lis3_dev.ac.x;
> > +	axis_array[1] = lis3_dev.ac.y;
> > +	axis_array[2] = lis3_dev.ac.z;
> > +}
> > +
> > +static void copy_from_axis_array(void)
> > +{
> > +	lis3_dev.ac.x = axis_array[0];
> > +	lis3_dev.ac.y = axis_array[1];
> > +	lis3_dev.ac.z = axis_array[2];
> > +}
> > +
> > +static int param_set_axis(const char *val, const struct kernel_param *kp)
> > +{
> > +	int ret;
> > +	copy_to_axis_array();
> > +	ret = param_set_int(val, kp);
> > +	if (!ret) {
> > +		int val = *(int *)kp->arg;
> > +		if (val < 0)
> > +			val = -val;
> > +		if (!val || val > 3)
> > +			return -EINVAL;
> > +		copy_from_axis_array();
> > +	}
> > +	return ret;
> > +}
> > +
> > +static int param_get_axis(char *buffer, const struct kernel_param *kp)
> > +{
> > +	copy_to_axis_array();
> > +	return param_get_int(buffer, kp);
> > +}
> > +
> > +static struct kernel_param_ops param_ops_axis = {
> > +	.set = param_set_axis,
> > +	.get = param_get_axis,
> > +};
> > +
> > +module_param_array_named(axes, axis_array, axis, NULL, 0644);
> > +MODULE_PARM_DESC(axes, "Axis-mapping for x,y,z directions");
> > +
> >  static s16 lis3lv02d_read_8(struct lis3lv02d *lis3, int reg)
> >  {
> >  	s8 lo;

I'm not familiar with kernel_param_ops, but the code looks sane to me.

Reviewed-by: Jean Delvare <khali@linux-fr.org>


-- 
Jean Delvare

Re: [PATCH] lis3: Add axes module parameter for custom axis-mapping

[Takashi Iwai]

At Tue, 7 Sep 2010 11:14:15 +0200,
Jean Delvare wrote:
> 
> Hi Takashi,
> 
> On Tue, 07 Sep 2010 08:45:35 +0200, Takashi Iwai wrote:
> > At Mon, 23 Aug 2010 14:23:25 +0200,
> > Takashi Iwai wrote:
> > > 
> > > The axis-mapping of lis3dev device on many (rather most) HP machines
> > > doesn't follow the standard.  When each new model appears, users need
> > > to adjust again.  Testing this requires the rebuild of kernel, thus
> > > it's not trivial for end-users.
> > > 
> > > This patch adds a module parameter "axes" to allow a custom
> > > axis-mapping without patching and recompiling the kernel driver.
> > > User can pass the parameter such as axes=3,2,1.  Also it can be
> > > changed via sysfs.
> > > 
> > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > 
> > Can anyone review / take this?
> > Jean, should it be through hwmon tree? 
> 
> Not really. The lis3lv02d driver shouldn't be in the hwmon directory to
> start with, its features aren't ones I am familiar with, so it makes
> little sense for me to pick patches. Better get it merged by Andrew.
> 
> At some point we will really have to move all these accelerometer
> drivers to a different directory to avoid the confusion.

I see.  It's why I didn't put you to Cc at the first post ;)

> 
> > > ---
> > >  drivers/hwmon/hp_accel.c  |    5 ++++-
> > >  drivers/hwmon/lis3lv02d.c |   45 +++++++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 49 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/hwmon/hp_accel.c b/drivers/hwmon/hp_accel.c
> > > index 7580f55..3462a5d 100644
> > > --- a/drivers/hwmon/hp_accel.c
> > > +++ b/drivers/hwmon/hp_accel.c
> > > @@ -297,7 +297,10 @@ static int lis3lv02d_add(struct acpi_device *device)
> > >  	lis3lv02d_enum_resources(device);
> > >  
> > >  	/* If possible use a "standard" axes order */
> > > -	if (dmi_check_system(lis3lv02d_dmi_ids) == 0) {
> > > +	if (lis3_dev.ac.x && lis3_dev.ac.y && lis3_dev.ac.z) {
> > > +		printk(KERN_INFO DRIVER_NAME ": Using custom axes %d,%d,%d\n",
> > > +		       lis3_dev.ac.x, lis3_dev.ac.y, lis3_dev.ac.z);
> > > +	} else if (dmi_check_system(lis3lv02d_dmi_ids) == 0) {
> > >  		printk(KERN_INFO DRIVER_NAME ": laptop model unknown, "
> > >  				 "using default axes configuration\n");
> > >  		lis3_dev.ac = lis3lv02d_axis_normal;
> > > diff --git a/drivers/hwmon/lis3lv02d.c b/drivers/hwmon/lis3lv02d.c
> > > index 6138f03..4b179d5 100644
> > > --- a/drivers/hwmon/lis3lv02d.c
> > > +++ b/drivers/hwmon/lis3lv02d.c
> > > @@ -75,6 +75,51 @@ struct lis3lv02d lis3_dev = {
> > >  
> > >  EXPORT_SYMBOL_GPL(lis3_dev);
> > >  
> > > +static int axis_array[3];
> > > +static void copy_to_axis_array(void)
> > > +{
> > > +	axis_array[0] = lis3_dev.ac.x;
> > > +	axis_array[1] = lis3_dev.ac.y;
> > > +	axis_array[2] = lis3_dev.ac.z;
> > > +}
> > > +
> > > +static void copy_from_axis_array(void)
> > > +{
> > > +	lis3_dev.ac.x = axis_array[0];
> > > +	lis3_dev.ac.y = axis_array[1];
> > > +	lis3_dev.ac.z = axis_array[2];
> > > +}
> > > +
> > > +static int param_set_axis(const char *val, const struct kernel_param *kp)
> > > +{
> > > +	int ret;
> > > +	copy_to_axis_array();
> > > +	ret = param_set_int(val, kp);
> > > +	if (!ret) {
> > > +		int val = *(int *)kp->arg;
> > > +		if (val < 0)
> > > +			val = -val;
> > > +		if (!val || val > 3)
> > > +			return -EINVAL;
> > > +		copy_from_axis_array();
> > > +	}
> > > +	return ret;
> > > +}
> > > +
> > > +static int param_get_axis(char *buffer, const struct kernel_param *kp)
> > > +{
> > > +	copy_to_axis_array();
> > > +	return param_get_int(buffer, kp);
> > > +}
> > > +
> > > +static struct kernel_param_ops param_ops_axis = {
> > > +	.set = param_set_axis,
> > > +	.get = param_get_axis,
> > > +};
> > > +
> > > +module_param_array_named(axes, axis_array, axis, NULL, 0644);
> > > +MODULE_PARM_DESC(axes, "Axis-mapping for x,y,z directions");
> > > +
> > >  static s16 lis3lv02d_read_8(struct lis3lv02d *lis3, int reg)
> > >  {
> > >  	s8 lo;
> 
> I'm not familiar with kernel_param_ops, but the code looks sane to me.
> 
> Reviewed-by: Jean Delvare <khali@linux-fr.org>

Thanks!


Takashi

Re: [PATCH] lis3: Add axes module parameter for custom axis-mapping

[Éric Piel]

Op 07-09-10 08:45, Takashi Iwai schreef:
> At Mon, 23 Aug 2010 14:23:25 +0200,
> Takashi Iwai wrote:
>>
>> The axis-mapping of lis3dev device on many (rather most) HP machines
>> doesn't follow the standard.  When each new model appears, users need
>> to adjust again.  Testing this requires the rebuild of kernel, thus
>> it's not trivial for end-users.
>>
>> This patch adds a module parameter "axes" to allow a custom
>> axis-mapping without patching and recompiling the kernel driver.
>> User can pass the parameter such as axes=3,2,1.  Also it can be
>> changed via sysfs.
>>
>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> 
> Can anyone review / take this?
> Jean, should it be through hwmon tree? 
Sorry for not answering before, but I'm very busy currently.

I like the idea, but the implementation seems weird. Maybe it's just
that I don't understand well enough module_param_array_named() though.

Each time the user changes the axis parameter, param_set_axis() is
called three times (once per axis), right? Why always calling
copy_to_axis_array() ? Does param_set_int(val, kp) return null only on
the 3rd time? This function really looks like a kludge. Either you need
to put more comments in order to make clear why things work, or rework it.

Wouldn't it be much cleaner if lis3_dev.ac was an array instead of a
struct? I don't mind that it involves changing more line if it makes
things more understandable.

Cheers,
Eric

> 
> 
> thanks,
> 
> Takashi
> 
>> ---
>>  drivers/hwmon/hp_accel.c  |    5 ++++-
>>  drivers/hwmon/lis3lv02d.c |   45 +++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 49 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/hwmon/hp_accel.c b/drivers/hwmon/hp_accel.c
>> index 7580f55..3462a5d 100644
>> --- a/drivers/hwmon/hp_accel.c
>> +++ b/drivers/hwmon/hp_accel.c
>> @@ -297,7 +297,10 @@ static int lis3lv02d_add(struct acpi_device *device)
>>  	lis3lv02d_enum_resources(device);
>>  
>>  	/* If possible use a "standard" axes order */
>> -	if (dmi_check_system(lis3lv02d_dmi_ids) == 0) {
>> +	if (lis3_dev.ac.x && lis3_dev.ac.y && lis3_dev.ac.z) {
>> +		printk(KERN_INFO DRIVER_NAME ": Using custom axes %d,%d,%d\n",
>> +		       lis3_dev.ac.x, lis3_dev.ac.y, lis3_dev.ac.z);
>> +	} else if (dmi_check_system(lis3lv02d_dmi_ids) == 0) {
>>  		printk(KERN_INFO DRIVER_NAME ": laptop model unknown, "
>>  				 "using default axes configuration\n");
>>  		lis3_dev.ac = lis3lv02d_axis_normal;
>> diff --git a/drivers/hwmon/lis3lv02d.c b/drivers/hwmon/lis3lv02d.c
>> index 6138f03..4b179d5 100644
>> --- a/drivers/hwmon/lis3lv02d.c
>> +++ b/drivers/hwmon/lis3lv02d.c
>> @@ -75,6 +75,51 @@ struct lis3lv02d lis3_dev = {
>>  
>>  EXPORT_SYMBOL_GPL(lis3_dev);
>>  
>> +static int axis_array[3];
>> +static void copy_to_axis_array(void)
>> +{
>> +	axis_array[0] = lis3_dev.ac.x;
>> +	axis_array[1] = lis3_dev.ac.y;
>> +	axis_array[2] = lis3_dev.ac.z;
>> +}
>> +
>> +static void copy_from_axis_array(void)
>> +{
>> +	lis3_dev.ac.x = axis_array[0];
>> +	lis3_dev.ac.y = axis_array[1];
>> +	lis3_dev.ac.z = axis_array[2];
>> +}
>> +
>> +static int param_set_axis(const char *val, const struct kernel_param *kp)
>> +{
>> +	int ret;
>> +	copy_to_axis_array();
>> +	ret = param_set_int(val, kp);
>> +	if (!ret) {
>> +		int val = *(int *)kp->arg;
>> +		if (val < 0)
>> +			val = -val;
>> +		if (!val || val > 3)
>> +			return -EINVAL;
>> +		copy_from_axis_array();
>> +	}
>> +	return ret;
>> +}
>> +
>> +static int param_get_axis(char *buffer, const struct kernel_param *kp)
>> +{
>> +	copy_to_axis_array();
>> +	return param_get_int(buffer, kp);
>> +}
>> +
>> +static struct kernel_param_ops param_ops_axis = {
>> +	.set = param_set_axis,
>> +	.get = param_get_axis,
>> +};
>> +
>> +module_param_array_named(axes, axis_array, axis, NULL, 0644);
>> +MODULE_PARM_DESC(axes, "Axis-mapping for x,y,z directions");
>> +
>>  static s16 lis3lv02d_read_8(struct lis3lv02d *lis3, int reg)
>>  {
>>  	s8 lo;

Re: [PATCH] lis3: Add axes module parameter for custom axis-mapping

[Takashi Iwai]

At Tue, 07 Sep 2010 16:25:02 +0200,
Éric Piel wrote:
> 
> Op 07-09-10 08:45, Takashi Iwai schreef:
> > At Mon, 23 Aug 2010 14:23:25 +0200,
> > Takashi Iwai wrote:
> >>
> >> The axis-mapping of lis3dev device on many (rather most) HP machines
> >> doesn't follow the standard.  When each new model appears, users need
> >> to adjust again.  Testing this requires the rebuild of kernel, thus
> >> it's not trivial for end-users.
> >>
> >> This patch adds a module parameter "axes" to allow a custom
> >> axis-mapping without patching and recompiling the kernel driver.
> >> User can pass the parameter such as axes=3,2,1.  Also it can be
> >> changed via sysfs.
> >>
> >> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > 
> > Can anyone review / take this?
> > Jean, should it be through hwmon tree? 
> Sorry for not answering before, but I'm very busy currently.
> 
> I like the idea, but the implementation seems weird. Maybe it's just
> that I don't understand well enough module_param_array_named() though.
>
> Each time the user changes the axis parameter, param_set_axis() is
> called three times (once per axis), right? Why always calling
> copy_to_axis_array() ? Does param_set_int(val, kp) return null only on
> the 3rd time? This function really looks like a kludge. Either you need
> to put more comments in order to make clear why things work, or rework it.

Yeah, the code is a bit tricky :)
The reason is that module parameter array calls the callback for each
array member.  But it doesn't pass which array index is being
accessed.

> Wouldn't it be much cleaner if lis3_dev.ac was an array instead of a
> struct? I don't mind that it involves changing more line if it makes
> things more understandable.

OK, then it makes things easier.  The revised patch is below.


thanks,

Takashi

===

>From 003c90d6b29b8ab80e076ceb1b92792aa95a2266 Mon Sep 17 00:00:00 2001
From: Takashi Iwai <tiwai@suse.de>
Date: Tue, 7 Sep 2010 16:40:59 +0200
Subject: [PATCH] lis3: Add axes module parameter for custom axis-mapping

The axis-mapping of lis3dev device on many (rather most) HP machines
doesn't follow the standard.  When each new model appears, users need
to adjust again.  Testing this requires the rebuild of kernel, thus
it's not trivial for end-users.

This patch adds a module parameter "axes" to allow a custom
axis-mapping without patching and recompiling the kernel driver.
User can pass the parameter such as axes=3,2,1.  Also it can be
changed via sysfs.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
v1->v2: change lis3_dev.ac to an int array to be written directly via
        module axes parameter


 drivers/hwmon/hp_accel.c      |   35 ++++++++++++++++++++---------------
 drivers/hwmon/lis3lv02d.c     |   15 +++++++++------
 drivers/hwmon/lis3lv02d.h     |    8 +-------
 drivers/hwmon/lis3lv02d_i2c.c |   12 +++++-------
 4 files changed, 35 insertions(+), 35 deletions(-)

diff --git a/drivers/hwmon/hp_accel.c b/drivers/hwmon/hp_accel.c
index 7580f55..b9dd63f 100644
--- a/drivers/hwmon/hp_accel.c
+++ b/drivers/hwmon/hp_accel.c
@@ -146,7 +146,8 @@ int lis3lv02d_acpi_write(struct lis3lv02d *lis3, int reg, u8 val)
 
 static int lis3lv02d_dmi_matched(const struct dmi_system_id *dmi)
 {
-	lis3_dev.ac = *((struct axis_conversion *)dmi->driver_data);
+	memcpy(lis3_dev.axis_map, (int *)dmi->driver_data,
+	       sizeof(lis3_dev.axis_map));
 	printk(KERN_INFO DRIVER_NAME ": hardware type %s found.\n", dmi->ident);
 
 	return 1;
@@ -154,16 +155,16 @@ static int lis3lv02d_dmi_matched(const struct dmi_system_id *dmi)
 
 /* Represents, for each axis seen by userspace, the corresponding hw axis (+1).
  * If the value is negative, the opposite of the hw value is used. */
-static struct axis_conversion lis3lv02d_axis_normal = {1, 2, 3};
-static struct axis_conversion lis3lv02d_axis_y_inverted = {1, -2, 3};
-static struct axis_conversion lis3lv02d_axis_x_inverted = {-1, 2, 3};
-static struct axis_conversion lis3lv02d_axis_z_inverted = {1, 2, -3};
-static struct axis_conversion lis3lv02d_axis_xy_swap = {2, 1, 3};
-static struct axis_conversion lis3lv02d_axis_xy_rotated_left = {-2, 1, 3};
-static struct axis_conversion lis3lv02d_axis_xy_rotated_left_usd = {-2, 1, -3};
-static struct axis_conversion lis3lv02d_axis_xy_swap_inverted = {-2, -1, 3};
-static struct axis_conversion lis3lv02d_axis_xy_rotated_right = {2, -1, 3};
-static struct axis_conversion lis3lv02d_axis_xy_swap_yz_inverted = {2, -1, -3};
+static int lis3lv02d_axis_normal[3] = {1, 2, 3};
+static int lis3lv02d_axis_y_inverted[3] = {1, -2, 3};
+static int lis3lv02d_axis_x_inverted[3] = {-1, 2, 3};
+static int lis3lv02d_axis_z_inverted[3] = {1, 2, -3};
+static int lis3lv02d_axis_xy_swap[3] = {2, 1, 3};
+static int lis3lv02d_axis_xy_rotated_left[3] = {-2, 1, 3};
+static int lis3lv02d_axis_xy_rotated_left_usd[3] = {-2, 1, -3};
+static int lis3lv02d_axis_xy_swap_inverted[3] = {-2, -1, 3};
+static int lis3lv02d_axis_xy_rotated_right[3] = {2, -1, 3};
+static int lis3lv02d_axis_xy_swap_yz_inverted[3] = {2, -1, -3};
 
 #define AXIS_DMI_MATCH(_ident, _name, _axis) {		\
 	.ident = _ident,				\
@@ -171,7 +172,7 @@ static struct axis_conversion lis3lv02d_axis_xy_swap_yz_inverted = {2, -1, -3};
 	.matches = {					\
 		DMI_MATCH(DMI_PRODUCT_NAME, _name)	\
 	},						\
-	.driver_data = &lis3lv02d_axis_##_axis		\
+	.driver_data = lis3lv02d_axis_##_axis		\
 }
 
 #define AXIS_DMI_MATCH2(_ident, _class1, _name1,	\
@@ -183,7 +184,7 @@ static struct axis_conversion lis3lv02d_axis_xy_swap_yz_inverted = {2, -1, -3};
 		DMI_MATCH(DMI_##_class1, _name1),	\
 		DMI_MATCH(DMI_##_class2, _name2),	\
 	},						\
-	.driver_data = &lis3lv02d_axis_##_axis		\
+	.driver_data = lis3lv02d_axis_##_axis		\
 }
 static struct dmi_system_id lis3lv02d_dmi_ids[] = {
 	/* product names are truncated to match all kinds of a same model */
@@ -297,10 +298,14 @@ static int lis3lv02d_add(struct acpi_device *device)
 	lis3lv02d_enum_resources(device);
 
 	/* If possible use a "standard" axes order */
-	if (dmi_check_system(lis3lv02d_dmi_ids) == 0) {
+	if (lis3_dev.axis_map[0] && lis3_dev.axis_map[1] && lis3_dev.axis_map[2]) {
+		printk(KERN_INFO DRIVER_NAME ": Using custom axes %d,%d,%d\n",
+		       lis3_dev.axis_map[0], lis3_dev.axis_map[1], lis3_dev.axis_map[2]);
+	} else if (dmi_check_system(lis3lv02d_dmi_ids) == 0) {
 		printk(KERN_INFO DRIVER_NAME ": laptop model unknown, "
 				 "using default axes configuration\n");
-		lis3_dev.ac = lis3lv02d_axis_normal;
+		memcpy(lis3_dev.axis_map, lis3lv02d_axis_normal,
+		       sizeof(lis3_dev.axis_map));
 	}
 
 	/* call the core layer do its init */
diff --git a/drivers/hwmon/lis3lv02d.c b/drivers/hwmon/lis3lv02d.c
index 6138f03..480fdba 100644
--- a/drivers/hwmon/lis3lv02d.c
+++ b/drivers/hwmon/lis3lv02d.c
@@ -75,6 +75,9 @@ struct lis3lv02d lis3_dev = {
 
 EXPORT_SYMBOL_GPL(lis3_dev);
 
+module_param_array_named(axes, lis3_dev.axis_map, int, NULL, 0644);
+MODULE_PARM_DESC(axes, "Axis-mapping for x,y,z directions");
+
 static s16 lis3lv02d_read_8(struct lis3lv02d *lis3, int reg)
 {
 	s8 lo;
@@ -130,9 +133,9 @@ static void lis3lv02d_get_xyz(struct lis3lv02d *lis3, int *x, int *y, int *z)
 	for (i = 0; i < 3; i++)
 		position[i] = (position[i] * lis3->scale) / LIS3_ACCURACY;
 
-	*x = lis3lv02d_get_axis(lis3->ac.x, position);
-	*y = lis3lv02d_get_axis(lis3->ac.y, position);
-	*z = lis3lv02d_get_axis(lis3->ac.z, position);
+	*x = lis3lv02d_get_axis(lis3->axis_map[0], position);
+	*y = lis3lv02d_get_axis(lis3->axis_map[1], position);
+	*z = lis3lv02d_get_axis(lis3->axis_map[2], position);
 }
 
 /* conversion btw sampling rate and the register values */
@@ -479,9 +482,9 @@ int lis3lv02d_joystick_enable(void)
 	input_set_abs_params(input_dev, ABS_Y, -max_val, max_val, fuzz, flat);
 	input_set_abs_params(input_dev, ABS_Z, -max_val, max_val, fuzz, flat);
 
-	lis3_dev.mapped_btns[0] = lis3lv02d_get_axis(abs(lis3_dev.ac.x), btns);
-	lis3_dev.mapped_btns[1] = lis3lv02d_get_axis(abs(lis3_dev.ac.y), btns);
-	lis3_dev.mapped_btns[2] = lis3lv02d_get_axis(abs(lis3_dev.ac.z), btns);
+	lis3_dev.mapped_btns[0] = lis3lv02d_get_axis(abs(lis3_dev.axis_map[0]), btns);
+	lis3_dev.mapped_btns[1] = lis3lv02d_get_axis(abs(lis3_dev.axis_map[1]), btns);
+	lis3_dev.mapped_btns[2] = lis3lv02d_get_axis(abs(lis3_dev.axis_map[2]), btns);
 
 	err = input_register_polled_device(lis3_dev.idev);
 	if (err) {
diff --git a/drivers/hwmon/lis3lv02d.h b/drivers/hwmon/lis3lv02d.h
index 8540913..51b9ba4 100644
--- a/drivers/hwmon/lis3lv02d.h
+++ b/drivers/hwmon/lis3lv02d.h
@@ -206,12 +206,6 @@ enum lis3lv02d_click_src_8b {
 	CLICK_IA	= 0x40,
 };
 
-struct axis_conversion {
-	s8	x;
-	s8	y;
-	s8	z;
-};
-
 struct lis3lv02d {
 	void			*bus_priv; /* used by the bus layer only */
 	int (*init) (struct lis3lv02d *lis3);
@@ -232,7 +226,7 @@ struct lis3lv02d {
 	struct input_polled_dev	*idev;     /* input device */
 	struct platform_device	*pdev;     /* platform device */
 	atomic_t		count;     /* interrupt count after last read */
-	struct axis_conversion	ac;        /* hw -> logical axis */
+	int			axis_map[3];  /* hw -> logical axis */
 	int			mapped_btns[3];
 
 	u32			irq;       /* IRQ number */
diff --git a/drivers/hwmon/lis3lv02d_i2c.c b/drivers/hwmon/lis3lv02d_i2c.c
index dc1f540..76ba42d 100644
--- a/drivers/hwmon/lis3lv02d_i2c.c
+++ b/drivers/hwmon/lis3lv02d_i2c.c
@@ -61,9 +61,7 @@ static int lis3_i2c_init(struct lis3lv02d *lis3)
 }
 
 /* Default axis mapping but it can be overwritten by platform data */
-static struct axis_conversion lis3lv02d_axis_map = { LIS3_DEV_X,
-						     LIS3_DEV_Y,
-						     LIS3_DEV_Z };
+static int lis3lv02d_axis_map[3] = { LIS3_DEV_X, LIS3_DEV_Y, LIS3_DEV_Z };
 
 static int __devinit lis3lv02d_i2c_probe(struct i2c_client *client,
 					const struct i2c_device_id *id)
@@ -73,13 +71,13 @@ static int __devinit lis3lv02d_i2c_probe(struct i2c_client *client,
 
 	if (pdata) {
 		if (pdata->axis_x)
-			lis3lv02d_axis_map.x = pdata->axis_x;
+			lis3lv02d_axis_map[0] = pdata->axis_x;
 
 		if (pdata->axis_y)
-			lis3lv02d_axis_map.y = pdata->axis_y;
+			lis3lv02d_axis_map[1] = pdata->axis_y;
 
 		if (pdata->axis_z)
-			lis3lv02d_axis_map.z = pdata->axis_z;
+			lis3lv02d_axis_map[2] = pdata->axis_z;
 
 		if (pdata->setup_resources)
 			ret = pdata->setup_resources();
@@ -94,7 +92,7 @@ static int __devinit lis3lv02d_i2c_probe(struct i2c_client *client,
 	lis3_dev.read	  = lis3_i2c_read;
 	lis3_dev.write	  = lis3_i2c_write;
 	lis3_dev.irq	  = client->irq;
-	lis3_dev.ac	  = lis3lv02d_axis_map;
+	memcpy(&lis3_dev.axis_map, &lis3lv02d_axis_map, sizeof(lis3_dev.axis_map));
 
 	i2c_set_clientdata(client, &lis3_dev);
 	ret = lis3lv02d_init_device(&lis3_dev);
-- 
1.7.2.1

Re: [PATCH] lis3: Add axes module parameter for custom axis-mapping

[Éric Piel]

Op 07-09-10 16:44, Takashi Iwai schreef:
:
>> Wouldn't it be much cleaner if lis3_dev.ac was an array instead of a
>> struct? I don't mind that it involves changing more line if it makes
>> things more understandable.
> 
> OK, then it makes things easier.  The revised patch is below.
Yes, I like it much better...

>  
> +module_param_array_named(axes, lis3_dev.axis_map, int, NULL, 0644);
> +MODULE_PARM_DESC(axes, "Axis-mapping for x,y,z directions");
But now there is no check at all about the value being between 1 and 3,
is there?

I think it's still necessary to have a small function which checks for
correct values, otherwise the user could read a bit anywhere in the memory.

Eric

Re: [PATCH] lis3: Add axes module parameter for custom axis-mapping

[Takashi Iwai]

At Tue, 07 Sep 2010 17:54:58 +0200,
Éric Piel wrote:
> 
> Op 07-09-10 16:44, Takashi Iwai schreef:
> :
> >> Wouldn't it be much cleaner if lis3_dev.ac was an array instead of a
> >> struct? I don't mind that it involves changing more line if it makes
> >> things more understandable.
> > 
> > OK, then it makes things easier.  The revised patch is below.
> Yes, I like it much better...
> 
> >  
> > +module_param_array_named(axes, lis3_dev.axis_map, int, NULL, 0644);
> > +MODULE_PARM_DESC(axes, "Axis-mapping for x,y,z directions");
> But now there is no check at all about the value being between 1 and 3,
> is there?
> I think it's still necessary to have a small function which checks for
> correct values, otherwise the user could read a bit anywhere in the memory.

Ah, right, it's accessing the array index.  This must be fixed indeed.
Another revised version below.


thanks,

Takashi

===
>From 003c90d6b29b8ab80e076ceb1b92792aa95a2266 Mon Sep 17 00:00:00 2001
From: Takashi Iwai <tiwai@suse.de>
Date: Tue, 7 Sep 2010 16:40:59 +0200
Subject: [PATCH] lis3: Add axes module parameter for custom axis-mapping

The axis-mapping of lis3dev device on many (rather most) HP machines
doesn't follow the standard.  When each new model appears, users need
to adjust again.  Testing this requires the rebuild of kernel, thus
it's not trivial for end-users.

This patch adds a module parameter "axes" to allow a custom
axis-mapping without patching and recompiling the kernel driver.
User can pass the parameter such as axes=3,2,1.  Also it can be
changed via sysfs.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
v2->v3: add the sanity-check of the axes parameter values
v1->v2: change lis3_dev.ac to an int array to be written directly via
        module axes parameter

 drivers/hwmon/hp_accel.c      |   35 ++++++++++++++++++++---------------
 drivers/hwmon/lis3lv02d.c     |   15 +++++++++------
 drivers/hwmon/lis3lv02d.h     |    8 +-------
 drivers/hwmon/lis3lv02d_i2c.c |   12 +++++-------
 4 files changed, 35 insertions(+), 35 deletions(-)

diff --git a/drivers/hwmon/hp_accel.c b/drivers/hwmon/hp_accel.c
index 7580f55..b9dd63f 100644
--- a/drivers/hwmon/hp_accel.c
+++ b/drivers/hwmon/hp_accel.c
@@ -146,7 +146,8 @@ int lis3lv02d_acpi_write(struct lis3lv02d *lis3, int reg, u8 val)
 
 static int lis3lv02d_dmi_matched(const struct dmi_system_id *dmi)
 {
-	lis3_dev.ac = *((struct axis_conversion *)dmi->driver_data);
+	memcpy(lis3_dev.axis_map, (int *)dmi->driver_data,
+	       sizeof(lis3_dev.axis_map));
 	printk(KERN_INFO DRIVER_NAME ": hardware type %s found.\n", dmi->ident);
 
 	return 1;
@@ -154,16 +155,16 @@ static int lis3lv02d_dmi_matched(const struct dmi_system_id *dmi)
 
 /* Represents, for each axis seen by userspace, the corresponding hw axis (+1).
  * If the value is negative, the opposite of the hw value is used. */
-static struct axis_conversion lis3lv02d_axis_normal = {1, 2, 3};
-static struct axis_conversion lis3lv02d_axis_y_inverted = {1, -2, 3};
-static struct axis_conversion lis3lv02d_axis_x_inverted = {-1, 2, 3};
-static struct axis_conversion lis3lv02d_axis_z_inverted = {1, 2, -3};
-static struct axis_conversion lis3lv02d_axis_xy_swap = {2, 1, 3};
-static struct axis_conversion lis3lv02d_axis_xy_rotated_left = {-2, 1, 3};
-static struct axis_conversion lis3lv02d_axis_xy_rotated_left_usd = {-2, 1, -3};
-static struct axis_conversion lis3lv02d_axis_xy_swap_inverted = {-2, -1, 3};
-static struct axis_conversion lis3lv02d_axis_xy_rotated_right = {2, -1, 3};
-static struct axis_conversion lis3lv02d_axis_xy_swap_yz_inverted = {2, -1, -3};
+static int lis3lv02d_axis_normal[3] = {1, 2, 3};
+static int lis3lv02d_axis_y_inverted[3] = {1, -2, 3};
+static int lis3lv02d_axis_x_inverted[3] = {-1, 2, 3};
+static int lis3lv02d_axis_z_inverted[3] = {1, 2, -3};
+static int lis3lv02d_axis_xy_swap[3] = {2, 1, 3};
+static int lis3lv02d_axis_xy_rotated_left[3] = {-2, 1, 3};
+static int lis3lv02d_axis_xy_rotated_left_usd[3] = {-2, 1, -3};
+static int lis3lv02d_axis_xy_swap_inverted[3] = {-2, -1, 3};
+static int lis3lv02d_axis_xy_rotated_right[3] = {2, -1, 3};
+static int lis3lv02d_axis_xy_swap_yz_inverted[3] = {2, -1, -3};
 
 #define AXIS_DMI_MATCH(_ident, _name, _axis) {		\
 	.ident = _ident,				\
@@ -171,7 +172,7 @@ static struct axis_conversion lis3lv02d_axis_xy_swap_yz_inverted = {2, -1, -3};
 	.matches = {					\
 		DMI_MATCH(DMI_PRODUCT_NAME, _name)	\
 	},						\
-	.driver_data = &lis3lv02d_axis_##_axis		\
+	.driver_data = lis3lv02d_axis_##_axis		\
 }
 
 #define AXIS_DMI_MATCH2(_ident, _class1, _name1,	\
@@ -183,7 +184,7 @@ static struct axis_conversion lis3lv02d_axis_xy_swap_yz_inverted = {2, -1, -3};
 		DMI_MATCH(DMI_##_class1, _name1),	\
 		DMI_MATCH(DMI_##_class2, _name2),	\
 	},						\
-	.driver_data = &lis3lv02d_axis_##_axis		\
+	.driver_data = lis3lv02d_axis_##_axis		\
 }
 static struct dmi_system_id lis3lv02d_dmi_ids[] = {
 	/* product names are truncated to match all kinds of a same model */
@@ -297,10 +298,14 @@ static int lis3lv02d_add(struct acpi_device *device)
 	lis3lv02d_enum_resources(device);
 
 	/* If possible use a "standard" axes order */
-	if (dmi_check_system(lis3lv02d_dmi_ids) == 0) {
+	if (lis3_dev.axis_map[0] && lis3_dev.axis_map[1] && lis3_dev.axis_map[2]) {
+		printk(KERN_INFO DRIVER_NAME ": Using custom axes %d,%d,%d\n",
+		       lis3_dev.axis_map[0], lis3_dev.axis_map[1], lis3_dev.axis_map[2]);
+	} else if (dmi_check_system(lis3lv02d_dmi_ids) == 0) {
 		printk(KERN_INFO DRIVER_NAME ": laptop model unknown, "
 				 "using default axes configuration\n");
-		lis3_dev.ac = lis3lv02d_axis_normal;
+		memcpy(lis3_dev.axis_map, lis3lv02d_axis_normal,
+		       sizeof(lis3_dev.axis_map));
 	}
 
 	/* call the core layer do its init */
diff --git a/drivers/hwmon/lis3lv02d.c b/drivers/hwmon/lis3lv02d.c
index 6138f03..480fdba 100644
--- a/drivers/hwmon/lis3lv02d.c
+++ b/drivers/hwmon/lis3lv02d.c
@@ -75,6 +75,9 @@ struct lis3lv02d lis3_dev = {
 
 EXPORT_SYMBOL_GPL(lis3_dev);
 
+module_param_array_named(axes, lis3_dev.axis_map, int, NULL, 0644);
+MODULE_PARM_DESC(axes, "Axis-mapping for x,y,z directions");
+
 static s16 lis3lv02d_read_8(struct lis3lv02d *lis3, int reg)
 {
 	s8 lo;
@@ -130,9 +133,9 @@ static void lis3lv02d_get_xyz(struct lis3lv02d *lis3, int *x, int *y, int *z)
 	for (i = 0; i < 3; i++)
 		position[i] = (position[i] * lis3->scale) / LIS3_ACCURACY;
 
-	*x = lis3lv02d_get_axis(lis3->ac.x, position);
-	*y = lis3lv02d_get_axis(lis3->ac.y, position);
-	*z = lis3lv02d_get_axis(lis3->ac.z, position);
+	*x = lis3lv02d_get_axis(lis3->axis_map[0], position);
+	*y = lis3lv02d_get_axis(lis3->axis_map[1], position);
+	*z = lis3lv02d_get_axis(lis3->axis_map[2], position);
 }
 
 /* conversion btw sampling rate and the register values */
@@ -479,9 +482,9 @@ int lis3lv02d_joystick_enable(void)
 	input_set_abs_params(input_dev, ABS_Y, -max_val, max_val, fuzz, flat);
 	input_set_abs_params(input_dev, ABS_Z, -max_val, max_val, fuzz, flat);
 
-	lis3_dev.mapped_btns[0] = lis3lv02d_get_axis(abs(lis3_dev.ac.x), btns);
-	lis3_dev.mapped_btns[1] = lis3lv02d_get_axis(abs(lis3_dev.ac.y), btns);
-	lis3_dev.mapped_btns[2] = lis3lv02d_get_axis(abs(lis3_dev.ac.z), btns);
+	lis3_dev.mapped_btns[0] = lis3lv02d_get_axis(abs(lis3_dev.axis_map[0]), btns);
+	lis3_dev.mapped_btns[1] = lis3lv02d_get_axis(abs(lis3_dev.axis_map[1]), btns);
+	lis3_dev.mapped_btns[2] = lis3lv02d_get_axis(abs(lis3_dev.axis_map[2]), btns);
 
 	err = input_register_polled_device(lis3_dev.idev);
 	if (err) {
diff --git a/drivers/hwmon/lis3lv02d.h b/drivers/hwmon/lis3lv02d.h
index 8540913..51b9ba4 100644
--- a/drivers/hwmon/lis3lv02d.h
+++ b/drivers/hwmon/lis3lv02d.h
@@ -206,12 +206,6 @@ enum lis3lv02d_click_src_8b {
 	CLICK_IA	= 0x40,
 };
 
-struct axis_conversion {
-	s8	x;
-	s8	y;
-	s8	z;
-};
-
 struct lis3lv02d {
 	void			*bus_priv; /* used by the bus layer only */
 	int (*init) (struct lis3lv02d *lis3);
@@ -232,7 +226,7 @@ struct lis3lv02d {
 	struct input_polled_dev	*idev;     /* input device */
 	struct platform_device	*pdev;     /* platform device */
 	atomic_t		count;     /* interrupt count after last read */
-	struct axis_conversion	ac;        /* hw -> logical axis */
+	int			axis_map[3];  /* hw -> logical axis */
 	int			mapped_btns[3];
 
 	u32			irq;       /* IRQ number */
diff --git a/drivers/hwmon/lis3lv02d_i2c.c b/drivers/hwmon/lis3lv02d_i2c.c
index dc1f540..76ba42d 100644
--- a/drivers/hwmon/lis3lv02d_i2c.c
+++ b/drivers/hwmon/lis3lv02d_i2c.c
@@ -61,9 +61,7 @@ static int lis3_i2c_init(struct lis3lv02d *lis3)
 }
 
 /* Default axis mapping but it can be overwritten by platform data */
-static struct axis_conversion lis3lv02d_axis_map = { LIS3_DEV_X,
-						     LIS3_DEV_Y,
-						     LIS3_DEV_Z };
+static int lis3lv02d_axis_map[3] = { LIS3_DEV_X, LIS3_DEV_Y, LIS3_DEV_Z };
 
 static int __devinit lis3lv02d_i2c_probe(struct i2c_client *client,
 					const struct i2c_device_id *id)
@@ -73,13 +71,13 @@ static int __devinit lis3lv02d_i2c_probe(struct i2c_client *client,
 
 	if (pdata) {
 		if (pdata->axis_x)
-			lis3lv02d_axis_map.x = pdata->axis_x;
+			lis3lv02d_axis_map[0] = pdata->axis_x;
 
 		if (pdata->axis_y)
-			lis3lv02d_axis_map.y = pdata->axis_y;
+			lis3lv02d_axis_map[1] = pdata->axis_y;
 
 		if (pdata->axis_z)
-			lis3lv02d_axis_map.z = pdata->axis_z;
+			lis3lv02d_axis_map[2] = pdata->axis_z;
 
 		if (pdata->setup_resources)
 			ret = pdata->setup_resources();
@@ -94,7 +92,7 @@ static int __devinit lis3lv02d_i2c_probe(struct i2c_client *client,
 	lis3_dev.read	  = lis3_i2c_read;
 	lis3_dev.write	  = lis3_i2c_write;
 	lis3_dev.irq	  = client->irq;
-	lis3_dev.ac	  = lis3lv02d_axis_map;
+	memcpy(&lis3_dev.axis_map, &lis3lv02d_axis_map, sizeof(lis3_dev.axis_map));
 
 	i2c_set_clientdata(client, &lis3_dev);
 	ret = lis3lv02d_init_device(&lis3_dev);
-- 
1.7.2.1

Re: [PATCH] lis3: Add axes module parameter for custom axis-mapping

[Éric Piel]

Op 07-09-10 18:10, Takashi Iwai schreef:
> At Tue, 07 Sep 2010 17:54:58 +0200,
:
>>>  
>>> +module_param_array_named(axes, lis3_dev.axis_map, int, NULL, 0644);
>>> +MODULE_PARM_DESC(axes, "Axis-mapping for x,y,z directions");
>> But now there is no check at all about the value being between 1 and 3,
>> is there?
>> I think it's still necessary to have a small function which checks for
>> correct values, otherwise the user could read a bit anywhere in the memory.
> 
> Ah, right, it's accessing the array index.  This must be fixed indeed.
> Another revised version below.
> 
It seems to be the same patch as the previous version. You probably
attached the wrong patch.

Cheers,
Eric

Re: [PATCH] lis3: Add axes module parameter for custom axis-mapping

[Takashi Iwai]

At Tue, 07 Sep 2010 21:17:20 +0200,
Éric Piel wrote:
> 
> Op 07-09-10 18:10, Takashi Iwai schreef:
> > At Tue, 07 Sep 2010 17:54:58 +0200,
> :
> >>>  
> >>> +module_param_array_named(axes, lis3_dev.axis_map, int, NULL, 0644);
> >>> +MODULE_PARM_DESC(axes, "Axis-mapping for x,y,z directions");
> >> But now there is no check at all about the value being between 1 and 3,
> >> is there?
> >> I think it's still necessary to have a small function which checks for
> >> correct values, otherwise the user could read a bit anywhere in the memory.
> > 
> > Ah, right, it's accessing the array index.  This must be fixed indeed.
> > Another revised version below.
> > 
> It seems to be the same patch as the previous version. You probably
> attached the wrong patch.

Oops, sorry.  Attached below.


Takashi

===

>From 0c5400d8214c9585e8eda18eb0fb8715e3d90e69 Mon Sep 17 00:00:00 2001
From: Takashi Iwai <tiwai@suse.de>
Date: Tue, 7 Sep 2010 16:40:59 +0200
Subject: [PATCH] lis3: Add axes module parameter for custom axis-mapping

The axis-mapping of lis3dev device on many (rather most) HP machines
doesn't follow the standard.  When each new model appears, users need
to adjust again.  Testing this requires the rebuild of kernel, thus
it's not trivial for end-users.

This patch adds a module parameter "axes" to allow a custom
axis-mapping without patching and recompiling the kernel driver.
User can pass the parameter such as axes=3,2,1.  Also it can be
changed via sysfs.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
v2->v3: add the sanity-check of the axes parameter values
v1->v2: change lis3_dev.ac to an int array to be written directly via
        module axes parameter

 drivers/hwmon/hp_accel.c      |   35 ++++++++++++++++++++---------------
 drivers/hwmon/lis3lv02d.c     |   36 ++++++++++++++++++++++++++++++------
 drivers/hwmon/lis3lv02d.h     |    8 +-------
 drivers/hwmon/lis3lv02d_i2c.c |   12 +++++-------
 4 files changed, 56 insertions(+), 35 deletions(-)

diff --git a/drivers/hwmon/hp_accel.c b/drivers/hwmon/hp_accel.c
index 7580f55..b9dd63f 100644
--- a/drivers/hwmon/hp_accel.c
+++ b/drivers/hwmon/hp_accel.c
@@ -146,7 +146,8 @@ int lis3lv02d_acpi_write(struct lis3lv02d *lis3, int reg, u8 val)
 
 static int lis3lv02d_dmi_matched(const struct dmi_system_id *dmi)
 {
-	lis3_dev.ac = *((struct axis_conversion *)dmi->driver_data);
+	memcpy(lis3_dev.axis_map, (int *)dmi->driver_data,
+	       sizeof(lis3_dev.axis_map));
 	printk(KERN_INFO DRIVER_NAME ": hardware type %s found.\n", dmi->ident);
 
 	return 1;
@@ -154,16 +155,16 @@ static int lis3lv02d_dmi_matched(const struct dmi_system_id *dmi)
 
 /* Represents, for each axis seen by userspace, the corresponding hw axis (+1).
  * If the value is negative, the opposite of the hw value is used. */
-static struct axis_conversion lis3lv02d_axis_normal = {1, 2, 3};
-static struct axis_conversion lis3lv02d_axis_y_inverted = {1, -2, 3};
-static struct axis_conversion lis3lv02d_axis_x_inverted = {-1, 2, 3};
-static struct axis_conversion lis3lv02d_axis_z_inverted = {1, 2, -3};
-static struct axis_conversion lis3lv02d_axis_xy_swap = {2, 1, 3};
-static struct axis_conversion lis3lv02d_axis_xy_rotated_left = {-2, 1, 3};
-static struct axis_conversion lis3lv02d_axis_xy_rotated_left_usd = {-2, 1, -3};
-static struct axis_conversion lis3lv02d_axis_xy_swap_inverted = {-2, -1, 3};
-static struct axis_conversion lis3lv02d_axis_xy_rotated_right = {2, -1, 3};
-static struct axis_conversion lis3lv02d_axis_xy_swap_yz_inverted = {2, -1, -3};
+static int lis3lv02d_axis_normal[3] = {1, 2, 3};
+static int lis3lv02d_axis_y_inverted[3] = {1, -2, 3};
+static int lis3lv02d_axis_x_inverted[3] = {-1, 2, 3};
+static int lis3lv02d_axis_z_inverted[3] = {1, 2, -3};
+static int lis3lv02d_axis_xy_swap[3] = {2, 1, 3};
+static int lis3lv02d_axis_xy_rotated_left[3] = {-2, 1, 3};
+static int lis3lv02d_axis_xy_rotated_left_usd[3] = {-2, 1, -3};
+static int lis3lv02d_axis_xy_swap_inverted[3] = {-2, -1, 3};
+static int lis3lv02d_axis_xy_rotated_right[3] = {2, -1, 3};
+static int lis3lv02d_axis_xy_swap_yz_inverted[3] = {2, -1, -3};
 
 #define AXIS_DMI_MATCH(_ident, _name, _axis) {		\
 	.ident = _ident,				\
@@ -171,7 +172,7 @@ static struct axis_conversion lis3lv02d_axis_xy_swap_yz_inverted = {2, -1, -3};
 	.matches = {					\
 		DMI_MATCH(DMI_PRODUCT_NAME, _name)	\
 	},						\
-	.driver_data = &lis3lv02d_axis_##_axis		\
+	.driver_data = lis3lv02d_axis_##_axis		\
 }
 
 #define AXIS_DMI_MATCH2(_ident, _class1, _name1,	\
@@ -183,7 +184,7 @@ static struct axis_conversion lis3lv02d_axis_xy_swap_yz_inverted = {2, -1, -3};
 		DMI_MATCH(DMI_##_class1, _name1),	\
 		DMI_MATCH(DMI_##_class2, _name2),	\
 	},						\
-	.driver_data = &lis3lv02d_axis_##_axis		\
+	.driver_data = lis3lv02d_axis_##_axis		\
 }
 static struct dmi_system_id lis3lv02d_dmi_ids[] = {
 	/* product names are truncated to match all kinds of a same model */
@@ -297,10 +298,14 @@ static int lis3lv02d_add(struct acpi_device *device)
 	lis3lv02d_enum_resources(device);
 
 	/* If possible use a "standard" axes order */
-	if (dmi_check_system(lis3lv02d_dmi_ids) == 0) {
+	if (lis3_dev.axis_map[0] && lis3_dev.axis_map[1] && lis3_dev.axis_map[2]) {
+		printk(KERN_INFO DRIVER_NAME ": Using custom axes %d,%d,%d\n",
+		       lis3_dev.axis_map[0], lis3_dev.axis_map[1], lis3_dev.axis_map[2]);
+	} else if (dmi_check_system(lis3lv02d_dmi_ids) == 0) {
 		printk(KERN_INFO DRIVER_NAME ": laptop model unknown, "
 				 "using default axes configuration\n");
-		lis3_dev.ac = lis3lv02d_axis_normal;
+		memcpy(lis3_dev.axis_map, lis3lv02d_axis_normal,
+		       sizeof(lis3_dev.axis_map));
 	}
 
 	/* call the core layer do its init */
diff --git a/drivers/hwmon/lis3lv02d.c b/drivers/hwmon/lis3lv02d.c
index 6138f03..5e15967 100644
--- a/drivers/hwmon/lis3lv02d.c
+++ b/drivers/hwmon/lis3lv02d.c
@@ -75,6 +75,30 @@ struct lis3lv02d lis3_dev = {
 
 EXPORT_SYMBOL_GPL(lis3_dev);
 
+/* just like param_set_int() but does sanity-check so that it won't point
+ * over the axis array size
+ */
+static int param_set_axis(const char *val, const struct kernel_param *kp)
+{
+	int ret = param_set_int(val, kp);
+	if (!ret) {
+		int val = *(int *)kp->arg;
+		if (val < 0)
+			val = -val;
+		if (!val || val > 3)
+			return -EINVAL;
+	}
+	return ret;
+}
+
+static struct kernel_param_ops param_ops_axis = {
+	.set = param_set_axis,
+	.get = param_get_int,
+};
+
+module_param_array_named(axes, lis3_dev.axis_map, axis, NULL, 0644);
+MODULE_PARM_DESC(axes, "Axis-mapping for x,y,z directions");
+
 static s16 lis3lv02d_read_8(struct lis3lv02d *lis3, int reg)
 {
 	s8 lo;
@@ -130,9 +154,9 @@ static void lis3lv02d_get_xyz(struct lis3lv02d *lis3, int *x, int *y, int *z)
 	for (i = 0; i < 3; i++)
 		position[i] = (position[i] * lis3->scale) / LIS3_ACCURACY;
 
-	*x = lis3lv02d_get_axis(lis3->ac.x, position);
-	*y = lis3lv02d_get_axis(lis3->ac.y, position);
-	*z = lis3lv02d_get_axis(lis3->ac.z, position);
+	*x = lis3lv02d_get_axis(lis3->axis_map[0], position);
+	*y = lis3lv02d_get_axis(lis3->axis_map[1], position);
+	*z = lis3lv02d_get_axis(lis3->axis_map[2], position);
 }
 
 /* conversion btw sampling rate and the register values */
@@ -479,9 +503,9 @@ int lis3lv02d_joystick_enable(void)
 	input_set_abs_params(input_dev, ABS_Y, -max_val, max_val, fuzz, flat);
 	input_set_abs_params(input_dev, ABS_Z, -max_val, max_val, fuzz, flat);
 
-	lis3_dev.mapped_btns[0] = lis3lv02d_get_axis(abs(lis3_dev.ac.x), btns);
-	lis3_dev.mapped_btns[1] = lis3lv02d_get_axis(abs(lis3_dev.ac.y), btns);
-	lis3_dev.mapped_btns[2] = lis3lv02d_get_axis(abs(lis3_dev.ac.z), btns);
+	lis3_dev.mapped_btns[0] = lis3lv02d_get_axis(abs(lis3_dev.axis_map[0]), btns);
+	lis3_dev.mapped_btns[1] = lis3lv02d_get_axis(abs(lis3_dev.axis_map[1]), btns);
+	lis3_dev.mapped_btns[2] = lis3lv02d_get_axis(abs(lis3_dev.axis_map[2]), btns);
 
 	err = input_register_polled_device(lis3_dev.idev);
 	if (err) {
diff --git a/drivers/hwmon/lis3lv02d.h b/drivers/hwmon/lis3lv02d.h
index 8540913..51b9ba4 100644
--- a/drivers/hwmon/lis3lv02d.h
+++ b/drivers/hwmon/lis3lv02d.h
@@ -206,12 +206,6 @@ enum lis3lv02d_click_src_8b {
 	CLICK_IA	= 0x40,
 };
 
-struct axis_conversion {
-	s8	x;
-	s8	y;
-	s8	z;
-};
-
 struct lis3lv02d {
 	void			*bus_priv; /* used by the bus layer only */
 	int (*init) (struct lis3lv02d *lis3);
@@ -232,7 +226,7 @@ struct lis3lv02d {
 	struct input_polled_dev	*idev;     /* input device */
 	struct platform_device	*pdev;     /* platform device */
 	atomic_t		count;     /* interrupt count after last read */
-	struct axis_conversion	ac;        /* hw -> logical axis */
+	int			axis_map[3];  /* hw -> logical axis */
 	int			mapped_btns[3];
 
 	u32			irq;       /* IRQ number */
diff --git a/drivers/hwmon/lis3lv02d_i2c.c b/drivers/hwmon/lis3lv02d_i2c.c
index dc1f540..76ba42d 100644
--- a/drivers/hwmon/lis3lv02d_i2c.c
+++ b/drivers/hwmon/lis3lv02d_i2c.c
@@ -61,9 +61,7 @@ static int lis3_i2c_init(struct lis3lv02d *lis3)
 }
 
 /* Default axis mapping but it can be overwritten by platform data */
-static struct axis_conversion lis3lv02d_axis_map = { LIS3_DEV_X,
-						     LIS3_DEV_Y,
-						     LIS3_DEV_Z };
+static int lis3lv02d_axis_map[3] = { LIS3_DEV_X, LIS3_DEV_Y, LIS3_DEV_Z };
 
 static int __devinit lis3lv02d_i2c_probe(struct i2c_client *client,
 					const struct i2c_device_id *id)
@@ -73,13 +71,13 @@ static int __devinit lis3lv02d_i2c_probe(struct i2c_client *client,
 
 	if (pdata) {
 		if (pdata->axis_x)
-			lis3lv02d_axis_map.x = pdata->axis_x;
+			lis3lv02d_axis_map[0] = pdata->axis_x;
 
 		if (pdata->axis_y)
-			lis3lv02d_axis_map.y = pdata->axis_y;
+			lis3lv02d_axis_map[1] = pdata->axis_y;
 
 		if (pdata->axis_z)
-			lis3lv02d_axis_map.z = pdata->axis_z;
+			lis3lv02d_axis_map[2] = pdata->axis_z;
 
 		if (pdata->setup_resources)
 			ret = pdata->setup_resources();
@@ -94,7 +92,7 @@ static int __devinit lis3lv02d_i2c_probe(struct i2c_client *client,
 	lis3_dev.read	  = lis3_i2c_read;
 	lis3_dev.write	  = lis3_i2c_write;
 	lis3_dev.irq	  = client->irq;
-	lis3_dev.ac	  = lis3lv02d_axis_map;
+	memcpy(&lis3_dev.axis_map, &lis3lv02d_axis_map, sizeof(lis3_dev.axis_map));
 
 	i2c_set_clientdata(client, &lis3_dev);
 	ret = lis3lv02d_init_device(&lis3_dev);
-- 
1.7.2.1

Re: [PATCH] lis3: Add axes module parameter for custom axis-mapping

[Éric Piel]

Op 07-09-10 21:31, Takashi Iwai schreef:
> At Tue, 07 Sep 2010 21:17:20 +0200,
> Éric Piel wrote:
>>
>> Op 07-09-10 18:10, Takashi Iwai schreef:
>>> At Tue, 07 Sep 2010 17:54:58 +0200,
>> :
>>>>>  
>>>>> +module_param_array_named(axes, lis3_dev.axis_map, int, NULL, 0644);
>>>>> +MODULE_PARM_DESC(axes, "Axis-mapping for x,y,z directions");
>>>> But now there is no check at all about the value being between 1 and 3,
>>>> is there?
>>>> I think it's still necessary to have a small function which checks for
>>>> correct values, otherwise the user could read a bit anywhere in the memory.
>>>
>>> Ah, right, it's accessing the array index.  This must be fixed indeed.
>>> Another revised version below.
>>>
>> It seems to be the same patch as the previous version. You probably
>> attached the wrong patch.
> 
> Oops, sorry.  Attached below.
> 
Looks good to me. Thanks Takashi.

Acked-by: Éric Piel <eric.piel@tremplin-utc.net>

Andrew, would you mind picking it up?
Eric