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

[Takashi Iwai <tiwai@suse.de>]

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