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

From: Éric Piel
Date: Tue Sep 07 2010 - 10:50:14 EST


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@xxxxxxx>
>
> 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;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/