Accelerometer driver for STMicroeletronics-LIS331DL-three-axis-digital

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Thanks Jonathan.
  Please find my comments inline.

Thanks
Kalhan

-----Original Message-----
From: Jonathan Cameron [mailto:jic23 at cam.ac.uk]
Sent: Tuesday, August 11, 2009 6:35 PM
To: Trisal, Kalhan
Cc: lm-sensors at lm-sensors.org; alan at linux.intel.com
Subject: Re:  Accelerometer driver for STMicroeletronics-LIS331DL-three-axis-digital

Kalhan Trisal wrote:
>>From 0d9b356e19170242fcbb746744b7ccb5156211eb Mon Sep 17 00:00:00 2001
> From: Kalhan Trisal <kalhan.trisal at intel.com>
> Date: Tue, 11 Aug 2009 11:12:02 -0400
> Subject: [PATCH] STMicroeletronics LIS331DL three-axis digital accelerometer
>
> This driver provides support for the LIS3331DL accelerometer
> accelerometer, connected to I2C. The accelerometer data is readable via
> sys/class/hwmon.
>
> Signed-off-by: Kalhan Trisal <kalhan.trisal at intel.com>
Please read the various previous threads on where accelerometers should
go in the kernel.  One of the biggest elements of them is that hwmon
is not the right place. They typically aren't being used for low
update rate monitoring of hardware.  One option is IIO but thats still
waiting for someone to have time to review the core.  Take a look
at the lis3l02dq driver posted to lkml to see how this sort of device
would be handled (including the interrupts etc).

Anyhow, other than the where to put it issue, I have a few nitpicking
comments and queries below.

> ---
>  drivers/hwmon/Kconfig    |    9 ++
>  drivers/hwmon/Makefile   |    1 +
>  drivers/hwmon/lis331dl.c |  283 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 293 insertions(+), 0 deletions(-)
>  create mode 100755 drivers/hwmon/lis331dl.c
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 2d50166..ac6117a 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -975,6 +975,15 @@ config SENSORS_LIS3LV02D
>         Say Y here if you have an applicable laptop and want to experience
>         the awesome power of lis3lv02d.
>
> +config SENSORS_LIS331DL
> +     tristate "STMicroeletronics LIS331DL three-axis digital accelerometer"
> +     depends on I2C_MRST
> +     default n
> +     help
> +       This driver provides support for the LIS3331DL accelerometer
> +       accelerometer, connected or I2C. The accelerometer data is readable via
> +       sys/class/hwmon.
> +
>  config SENSORS_LIS3_SPI
>       tristate "STMicroeletronics LIS3LV02Dx three-axis digital accelerometer (SPI)"
>       depends on !ACPI && SPI_MASTER && INPUT
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index b793dce..3b1e424 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -89,6 +89,7 @@ obj-$(CONFIG_SENSORS_VT8231)        += vt8231.o
>  obj-$(CONFIG_SENSORS_W83627EHF)      += w83627ehf.o
>  obj-$(CONFIG_SENSORS_W83L785TS)      += w83l785ts.o
>  obj-$(CONFIG_SENSORS_W83L786NG)      += w83l786ng.o
> +obj-$(CONFIG_SENSORS_LIS331DL)       += lis331dl.o
>
>  ifeq ($(CONFIG_HWMON_DEBUG_CHIP),y)
>  EXTRA_CFLAGS += -DDEBUG
> diff --git a/drivers/hwmon/lis331dl.c b/drivers/hwmon/lis331dl.c
> new file mode 100755
> index 0000000..9b999a3
> --- /dev/null
> +++ b/drivers/hwmon/lis331dl.c
> @@ -0,0 +1,283 @@
> +/*
> + * lis331dl.c - ST LIS331DL  Accelerometer Driver
> + *
> + * Copyright (C) 2009 Intel Corp
> + *
> + *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, write to the Free Software Foundation, Inc.,
> + * 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/hwmon-vid.h>
> +#include <linux/err.h>
> +#include <linux/delay.h>
> +#include <linux/mutex.h>
> +#include <linux/sysfs.h>
> +
> +
> +MODULE_AUTHOR("Kalhan Trisal <kalhan.trisal at intel.com");
> +MODULE_DESCRIPTION("STMacroelectronics LIS331DL Accelerometer Driver");
> +MODULE_LICENSE("GPL v2");
> +
> +#define ACCEL_DATA_RATE_100HZ 0
> +#define ACCEL_DATA_RATE_400HZ 1
> +#define ACCEL_POWER_MODE_DOWN 0
> +#define ACCEL_POWER_MODE_ACTIVE 1
> +#define ACCEL_NORMAL_MODE 0
> +#define ACCEL_MEMORY_REBOOT 1
> +
> +/* internal return values */
> +
> +struct acclero_data {
> +     struct device *hwmon_dev;
> +     struct mutex update_lock;
> +};
The above structure has rather a general name, likely to lead to a clash
in the future. The same is true of a number of the functions.


>> will rename to device specific structure.

> +static unsigned int i2c_write_current_data(struct i2c_client *client,
> +                                     unsigned int reg, unsigned int value)
> +{
> +     int ret_val;
> +
> +     ret_val = i2c_smbus_write_byte_data(client, reg, value);
> +     return ret_val;
> +}
Why bother with above. Doesn't seem to being anything?  Presumably
it was to allow for later inclusion of spi support, but as is you
might as well call the smbus command directly.


>> yes can be done but for future support/enhancement  let's keep it as modular as possible.

> +static ssize_t rate_show(struct device *dev,
> +                     struct device_attribute *attr, char *buf)
> +{
> +     struct i2c_client *client = to_i2c_client(dev);
> +     int ret_val, val;
> +
> +     val = i2c_smbus_read_byte_data(client, 0x20);
> +     ret_val = (val & 0x80); /* 1= 400HZ 0= 100HZ */
> +     if (ret_val == 0x80)
> +             ret_val = 1;
Having carefully added definitions for this above, why not use them?


>> will do this change in next patch.

> +     return sprintf(buf, "%d\n", ret_val);
> +
odd spacing.
>> will rectify this.
> +}

Not clear what state is refering to. Looking at the data sheet I
can see that it is power down control. Do you want userspace access
to this, or should it be done via the fine grained power management
stuff that is working its way into the kernel?

>> in mids the Apps. manager are intelligent to check if no apps are going to use the device they can power down the device,
>> power management can do this, but we decided that exposing this to app manager will be more effective as we can put the
>>device in active mode only when needed.

> +static ssize_t state_show(struct device *dev,
> +                     struct device_attribute *attr, char *buf)
> +{
> +     struct i2c_client *client = to_i2c_client(dev);
> +     int ret_val, val;
> +
Definitions for register addresses would make this easier to read.

>> taken will do the change in next patch

> +     val = i2c_smbus_read_byte_data(client, 0x20);
> +     ret_val = (val & 0x40);
> +     if (ret_val == 0x40)
> +             ret_val = 1;
> +     return sprintf(buf, "%d\n", ret_val);
> +}
> +

Up to you whether you do a single access point for all 3 readings (scan
across them) or individual reads.  Guess which makes sense depends on the
application.

>> thought about this but apps want all the data at a given time stamp, this also many not be accurate but by calling individual x,y,z
>>this seems to be more accurate at given time stamp what is the current position.

> +static ssize_t xyz_pos_show(struct device *dev,
> +                             struct device_attribute *attr, char *buf)
> +{
> +     int x, y, z;
> +     struct i2c_client *client = to_i2c_client(dev);
I haven't read the data sheet thoroughly, but is the device capable of
doing more complex reads? (would be nice to cut down the bus load
of this given the device is running at maybe 400 Hz).
Table 14 of the data sheet seems to hint this can be done.
I guess it would also be dependant on what your i2c adapter supports.

>> Agree It is adapter dependent, will put the note here saying that if the adapter support multiple
>> register read simultaneously then that also can be performed.

> +     x = i2c_smbus_read_byte_data(client, 0x29);
> +     y = i2c_smbus_read_byte_data(client, 0x2B);
> +     z = i2c_smbus_read_byte_data(client, 0x2D);
> +     return sprintf(buf, "%d, %d, %d \n", x, y, z);
> +}
> +
> +static ssize_t rate_store(struct device *dev,
> +             struct device_attribute *attr, const char *buf, size_t count)
> +{
> +     struct i2c_client *client = to_i2c_client(dev);
> +     struct acclero_data *data = i2c_get_clientdata(client);
> +     unsigned int ret_val, set_val;
> +     unsigned long val;
> +
> +     if (strict_strtoul(buf, 10, &val))
> +             return -EINVAL;
Why bother locking for updates without protecting the reads as well?
Given I'm guessing a given i2c write is atomic, I'm not sure you gain
anything.


 > > will remove mutex

> +     ret_val = i2c_smbus_read_byte_data(client, 0x20);
> +
> +     mutex_lock(&data->update_lock);
> +     if (val == ACCEL_DATA_RATE_100HZ)
> +             set_val = (ret_val & 0x7F); /* setting the 8th bit to 0 */
> +     else if (val == ACCEL_DATA_RATE_400HZ)
> +             set_val = (ret_val | (1 << 7));
> +     else
> +             goto invarg;
> +
> +     i2c_write_current_data(client, 0x20, set_val);
> +     mutex_unlock(&data->update_lock);
> +     return count;
> +invarg:
> +     mutex_unlock(&data->update_lock);
> +     return -EINVAL;
> +}
> +
> +static ssize_t state_store(struct device *dev,
> +             struct device_attribute *attr, const  char *buf, size_t count)
> +{
> +     struct i2c_client *client = to_i2c_client(dev);
> +     struct acclero_data *data = i2c_get_clientdata(client);
> +     unsigned int ret_val, set_val;
> +     unsigned long val;
> +
> +     if (strict_strtoul(buf, 10, &val))
> +             return -EINVAL;
> +     ret_val = i2c_smbus_read_byte_data(client, 0x20);
> +
> +     mutex_lock(&data->update_lock);
> +     if (val == ACCEL_POWER_MODE_DOWN)
> +             set_val = ret_val & 0xBF; /* if value id 0 */
> +     else if (val == ACCEL_POWER_MODE_ACTIVE)
> +             set_val = (ret_val | (1<<6)); /* if value is 1 */
> +     else
> +             goto invarg;
> +
> +     i2c_write_current_data(client, 0x20, set_val);
> +     mutex_unlock(&data->update_lock);
> +     return count;
> +invarg:
> +     mutex_unlock(&data->update_lock);
> +     return -EINVAL;
> +}

Why do you want this exposed to userspace? To my reading
the primary purpose in doing this is to reset any changes
to the various calibrated parameters. Exporting to userspace
might make sense if you were also exporting interfaces for them
(HPcoeff for example) Even then, I'm not sure providing access
to this isn't more confusing than ideal.

>> this is exposed to user space if the application manager is sensing that the values
>> from the device are inconsistent, then best way is to reboot the device. This option is
>> only for sensor application manager. I felt it will be useful when device misbehaves and can be rectified
>> dynamically by rebooting the device.

> +static ssize_t reboot_mem_store(struct device *dev,
> +             struct device_attribute *attr, const char *buf, size_t count)
> +{
> +     struct i2c_client *client = to_i2c_client(dev);
> +     struct acclero_data *data = i2c_get_clientdata(client);
> +     unsigned int ret_val, set_val;
> +     unsigned long val;
> +
> +     if (strict_strtoul(buf, 10, &val))
> +             return -EINVAL;
> +     ret_val = i2c_smbus_read_byte_data(client, 0x21);
> +     if (val == ACCEL_MEMORY_REBOOT) {
> +             mutex_lock(&data->update_lock);
> +             set_val = (ret_val | (1 << 6)); /* setting the 6th  bit */
> +             i2c_write_current_data(client, 0x21, set_val);
> +             mutex_unlock(&data->update_lock);
> +     } else
> +             return -EINVAL;
> +     return count;
> +}
> +
> +static DEVICE_ATTR(data_rate, S_IRUGO | S_IWUSR, rate_show, rate_store);
> +static DEVICE_ATTR(power_state, S_IRUGO | S_IWUSR, state_show, state_store);
> +static DEVICE_ATTR(reboot_mem, S_IWUSR, NULL, reboot_mem_store);
> +static DEVICE_ATTR(position, S_IRUGO, xyz_pos_show, NULL);
> +
> +static struct attribute *mid_att_acclero[] = {
> +     &dev_attr_data_rate.attr,
> +     &dev_attr_power_state.attr,
> +     &dev_attr_reboot_mem.attr,
> +     &dev_attr_position.attr,
> +     NULL
> +};
This is a little odd, why have a device with an attribute group
of the same name? I think this will lead to a somewhat unusual
structure in sysfs.

>> I am not clear here, does it mean that we should rename it as accelerometer.

> +static struct attribute_group m_acclero_gr = {
> +     .name = "lis331dl",
> +     .attrs = mid_att_acclero
> +};
> +
> +static void accel_set_default_config(struct i2c_client *client)
> +{
This could at very least do with a comment explaining what default
you have selected.


>> Accepted will do that.

> +     i2c_write_current_data(client, 0x20, 0x47);
> +}
> +
> +static int  lis331dl_probe(struct i2c_client *client,
> +                                     const struct i2c_device_id *id)
> +{
> +     int res;
> +     struct acclero_data *data;
> +
> +     data = kzalloc(sizeof(struct acclero_data), GFP_KERNEL);
> +     if (data == NULL) {
> +             printk(KERN_WARNING "lis331dl: Memory initi failed \n");
> +             return -ENOMEM;
> +     }
> +     mutex_init(&data->update_lock);
> +     i2c_set_clientdata(client, data);
> +
> +     res = sysfs_create_group(&client->dev.kobj, &m_acclero_gr);
> +     if (res) {
> +             printk(KERN_WARNING "lis331dl: Sysfs  group failed!!\n");
> +             goto acclero_error1;
> +     }
> +     data->hwmon_dev = hwmon_device_register(&client->dev);
> +     if (IS_ERR(data->hwmon_dev)) {
> +             res = PTR_ERR(data->hwmon_dev);
> +             data->hwmon_dev = NULL;
> +             sysfs_remove_group(&client->dev.kobj, &m_acclero_gr);
> +             printk(KERN_WARNING "lis331dl: unable to register \
> +                                             hwmon device\n");
> +             goto acclero_error1;
> +     }
> +     accel_set_default_config(client);
> +
> +     dev_info(&client->dev, "%s lis331dl:  Accelerometer chip \
> +                                                     foundn", client->name);
> +     return res;
> +
> +acclero_error1:
> +     i2c_set_clientdata(client, NULL);
> +     kfree(data);
> +     return res;
> +}
> +
> +static int lis331dl_remove(struct i2c_client *client)
> +{
> +     struct acclero_data *data = i2c_get_clientdata(client);
> +
> +     hwmon_device_unregister(data->hwmon_dev);
> +     sysfs_remove_group(&client->dev.kobj, &m_acclero_gr);
> +     kfree(data);
> +     return 0;
> +}
That's a very odd name for i2c device.  Shouldn't it be lis33ld1
or similar?

>> Accepted will do the change.

> +static struct i2c_device_id lis331dl_id[] = {
> +     { "i2c_accel", 0 },
> +     { }
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, lis331dl_id);
> +
> +static struct i2c_driver lis331dl_driver = {
> +     .driver = {
> +     .name = "lis331dl",
> +     },
> +     .probe = lis331dl_probe,
> +     .remove = lis331dl_remove,
> +     .id_table = lis331dl_id,
> +};
> +
> +static int __init sensor_lis331dl_init(void)
> +{
> +     int res;
> +
> +     res = i2c_add_driver(&lis331dl_driver);
> +     return res;
> +}
> +
> +static void  __exit sensor_lis331dl_exit(void)
> +{
> +     i2c_del_driver(&lis331dl_driver);
> +}
> +
> +module_init(sensor_lis331dl_init);
> +module_exit(sensor_lis331dl_exit);




[Index of Archives]     [Linux Kernel]     [Linux Hardware Monitoring]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux