From: Jonathan Cameron <jic23@kernel.org>
To: Denis CIOCCA <denis.ciocca@st.com>
Cc: lars@metafoo.de, linux-iio@vger.kernel.org
Subject: Re: [PATCH 1/4] iio:common: Add STMicroelectronics common library
Date: Sun, 16 Dec 2012 12:15:21 +0000 [thread overview]
Message-ID: <50CDBB59.2070906@kernel.org> (raw)
In-Reply-To: <1355404302-2239-2-git-send-email-denis.ciocca@st.com>
On 12/13/2012 01:11 PM, Denis CIOCCA wrote:
> This patch add generic library for STMicroelectronics 3-axis sensors.
Nice piece of work. Couple of little bits and bobs, but basically there
as far as I am concerned...
(just the spi buffer alignment stuff and it really is just a case of
making sure they are the last element in the chunk of memory created
as mallocs are always a whole number of cachelines.)
> ---
> drivers/iio/common/Kconfig | 1 +
> drivers/iio/common/Makefile | 1 +
> drivers/iio/common/st_sensors/Kconfig | 30 ++
> drivers/iio/common/st_sensors/Makefile | 9 +
> drivers/iio/common/st_sensors/st_sensors_buffer.c | 115 +++++++
> drivers/iio/common/st_sensors/st_sensors_core.c | 335 ++++++++++++++++++++
> drivers/iio/common/st_sensors/st_sensors_i2c.c | 78 +++++
> drivers/iio/common/st_sensors/st_sensors_spi.c | 124 +++++++
> drivers/iio/common/st_sensors/st_sensors_trigger.c | 76 +++++
> include/linux/iio/common/st_sensors.h | 246 ++++++++++++++
> 10 files changed, 1015 insertions(+), 0 deletions(-)
> create mode 100644 drivers/iio/common/st_sensors/Kconfig
> create mode 100644 drivers/iio/common/st_sensors/Makefile
> create mode 100644 drivers/iio/common/st_sensors/st_sensors_buffer.c
> create mode 100644 drivers/iio/common/st_sensors/st_sensors_core.c
> create mode 100644 drivers/iio/common/st_sensors/st_sensors_i2c.c
> create mode 100644 drivers/iio/common/st_sensors/st_sensors_spi.c
> create mode 100644 drivers/iio/common/st_sensors/st_sensors_trigger.c
> create mode 100644 include/linux/iio/common/st_sensors.h
>
> diff --git a/drivers/iio/common/Kconfig b/drivers/iio/common/Kconfig
> index ed45ee5..0b6e97d 100644
> --- a/drivers/iio/common/Kconfig
> +++ b/drivers/iio/common/Kconfig
> @@ -3,3 +3,4 @@
> #
>
> source "drivers/iio/common/hid-sensors/Kconfig"
> +source "drivers/iio/common/st_sensors/Kconfig"
> diff --git a/drivers/iio/common/Makefile b/drivers/iio/common/Makefile
> index 8158400..c2352be 100644
> --- a/drivers/iio/common/Makefile
> +++ b/drivers/iio/common/Makefile
> @@ -7,3 +7,4 @@
> #
>
> obj-y += hid-sensors/
> +obj-y += st_sensors/
> diff --git a/drivers/iio/common/st_sensors/Kconfig b/drivers/iio/common/st_sensors/Kconfig
> new file mode 100644
> index 0000000..ddcfcc6
> --- /dev/null
> +++ b/drivers/iio/common/st_sensors/Kconfig
> @@ -0,0 +1,30 @@
> +#
> +# Hid Sensor common modules
> +#
> +menu "STMicroelectronics sensors library"
> +
> +config ST_SENSORS_CORE
> + tristate "Common modules for all ST sensors IIO drivers"
> + help
> + Say yes here to build support for ST sensors to use
> + common core functions.
> +
> +config ST_SENSORS_I2C
> + tristate "I2C common library for ST Sensor IIO drivers"
> + help
> + Say yes here to build support for ST sensors to use
> + common i2c functions.
> +
> +config ST_SENSORS_SPI
> + tristate "SPI common library for ST Sensor IIO drivers"
> + help
> + Say yes here to build support for ST sensors to use
> + common spi functions.
> +
> +config ST_SENSORS_TRIGGERED_BUFFER
> + tristate "trigger and buffer common library for ST Sensor IIO drivers"
> + help
> + Say yes here to build support for ST sensors to use
> + common trigger and buffer functions.
> +
> +endmenu
> diff --git a/drivers/iio/common/st_sensors/Makefile b/drivers/iio/common/st_sensors/Makefile
> new file mode 100644
> index 0000000..a191e65
> --- /dev/null
> +++ b/drivers/iio/common/st_sensors/Makefile
> @@ -0,0 +1,9 @@
> +#
> +# Makefile for the STMicroelectronics sensor common modules.
> +#
> +
> +obj-$(CONFIG_ST_SENSORS_CORE) += st_sensors_core.o
> +obj-$(CONFIG_ST_SENSORS_I2C) += st_sensors_i2c.o
> +obj-$(CONFIG_ST_SENSORS_SPI) += st_sensors_spi.o
> +obj-$(CONFIG_ST_SENSORS_TRIGGERED_BUFFER) += st_sensors_triggered_buffer.o
> +st_sensors_triggered_buffer-y := st_sensors_trigger.o st_sensors_buffer.o
> diff --git a/drivers/iio/common/st_sensors/st_sensors_buffer.c b/drivers/iio/common/st_sensors/st_sensors_buffer.c
> new file mode 100644
> index 0000000..b636751
> --- /dev/null
> +++ b/drivers/iio/common/st_sensors/st_sensors_buffer.c
> @@ -0,0 +1,115 @@
> +/*
> + * STMicroelectronics sensors buffer library driver
> + *
> + * Copyright 2012 STMicroelectronics Inc.
> + *
> + * Denis Ciocca <denis.ciocca@st.com>
> + *
> + * Licensed under the GPL-2.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/interrupt.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/irqreturn.h>
> +
> +#include <linux/iio/common/st_sensors.h>
> +
> +int st_sensors_get_buffer_element(struct iio_dev *indio_dev, u8 *buf)
> +{
> + int i, n = 0, len;
> + u8 addr[ST_SENSORS_NUMBER_DATA_CHANNELS];
> + struct st_sensor_data *sdata = iio_priv(indio_dev);
> +
> + for (i = 0; i < ST_SENSORS_NUMBER_DATA_CHANNELS; i++) {
> + if (test_bit(i, indio_dev->active_scan_mask)) {
> + addr[n] = indio_dev->channels[i].address;
> + n++;
> + }
> + }
> + switch (n) {
> + case 1:
> + len = sdata->tf.read_multiple_byte(&sdata->tf, sdata->dev,
> + addr[0], ST_SENSORS_BYTE_FOR_CHANNEL, buf,
> + sdata->multiread_bit);
> + break;
> + case 2:
> + if ((addr[1] - addr[0]) == ST_SENSORS_BYTE_FOR_CHANNEL) {
> + len = sdata->tf.read_multiple_byte(&sdata->tf,
> + sdata->dev, addr[0],
> + ST_SENSORS_BYTE_FOR_CHANNEL*n,
> + buf, sdata->multiread_bit);
> + } else {
> + u8 rx_array[ST_SENSORS_BYTE_FOR_CHANNEL*
> + ST_SENSORS_NUMBER_DATA_CHANNELS];
> + len = sdata->tf.read_multiple_byte(&sdata->tf,
> + sdata->dev, addr[0],
> + ST_SENSORS_BYTE_FOR_CHANNEL*
> + ST_SENSORS_NUMBER_DATA_CHANNELS,
> + rx_array, sdata->multiread_bit);
> + if (len < 0)
> + goto read_data_channels_error;
> +
> + for (i = 0; i < n * ST_SENSORS_NUMBER_DATA_CHANNELS;
> + i++) {
> + if (i < n)
> + buf[i] = rx_array[i];
> + else
> + buf[i] = rx_array[n + i];
> + }
> + len = ST_SENSORS_BYTE_FOR_CHANNEL*n;
> + }
> + break;
> + case 3:
> + len = sdata->tf.read_multiple_byte(&sdata->tf, sdata->dev,
> + addr[0], ST_SENSORS_BYTE_FOR_CHANNEL*
> + ST_SENSORS_NUMBER_DATA_CHANNELS,
> + buf, sdata->multiread_bit);
> + break;
> + default:
> + len = -EINVAL;
> + goto read_data_channels_error;
> + }
> + if (len != ST_SENSORS_BYTE_FOR_CHANNEL*n) {
> + len = -EIO;
> + goto read_data_channels_error;
> + }
> +
> +read_data_channels_error:
> + return len;
> +}
> +EXPORT_SYMBOL(st_sensors_get_buffer_element);
> +
> +irqreturn_t st_sensors_trigger_handler(int irq, void *p)
> +{
> + int len;
> + struct iio_poll_func *pf = p;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct st_sensor_data *sdata = iio_priv(indio_dev);
> +
> + len = st_sensors_get_buffer_element(indio_dev, sdata->buffer_data);
> + if (len < 0)
> + goto st_sensors_get_buffer_element_error;
> +
> + if (indio_dev->scan_timestamp)
> + *(s64 *)((u8 *)sdata->buffer_data +
> + ALIGN(len, sizeof(s64))) = pf->timestamp;
> +
> + iio_push_to_buffers(indio_dev, sdata->buffer_data);
> +
> +st_sensors_get_buffer_element_error:
> + iio_trigger_notify_done(indio_dev->trig);
> +
> + return IRQ_HANDLED;
> +}
> +EXPORT_SYMBOL(st_sensors_trigger_handler);
> +
> +MODULE_AUTHOR("Denis Ciocca <denis.ciocca@st.com>");
> +MODULE_DESCRIPTION("STMicroelectronics ST-sensors buffer");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
> new file mode 100644
> index 0000000..2b00ed7
> --- /dev/null
> +++ b/drivers/iio/common/st_sensors/st_sensors_core.c
> @@ -0,0 +1,335 @@
> +/*
> + * STMicroelectronics sensors core library driver
> + *
> + * Copyright 2012 STMicroelectronics Inc.
> + *
> + * Denis Ciocca <denis.ciocca@st.com>
> + *
> + * Licensed under the GPL-2.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/iio/iio.h>
> +
> +#include <linux/iio/common/st_sensors.h>
> +
> +
> +#define ST_SENSORS_WAI_ADDRESS 0x0f
> +
> +int st_sensors_write_data_with_mask(struct iio_dev *indio_dev, u8 reg_addr,
> + u8 mask, short num_bit, u8 data)
> +{
> + int err;
> + u8 new_data;
> + struct st_sensor_data *sdata = iio_priv(indio_dev);
> +
> + err = sdata->tf.read_byte(&sdata->tf, sdata->dev, reg_addr, &new_data);
> + if (err < 0)
> + goto st_sensors_write_data_with_mask_error;
> +
> + new_data = ((new_data & (~mask)) | ((data << __ffs(mask)) & mask));
> + err = sdata->tf.write_byte(&sdata->tf, sdata->dev, reg_addr, new_data);
> +
> +st_sensors_write_data_with_mask_error:
> + return err;
> +}
> +EXPORT_SYMBOL(st_sensors_write_data_with_mask);
> +
> +int st_sensors_get_sampling_frequency_avl(struct iio_dev *indio_dev,
> + const struct st_sensor_odr_avl *odr_avl, char *buf)
> +{
> + int i, len = 0;
> +
> + mutex_lock(&indio_dev->mlock);
> + for (i = 0; i < ST_SENSORS_ODR_LIST_MAX; i++) {
> + if (odr_avl[i].hz == 0)
> + break;
> +
> + len += sprintf(buf + len, "%d ", odr_avl[i].hz);
> + }
> + mutex_unlock(&indio_dev->mlock);
> + buf[len - 1] = '\n';
> +
> + return len;
> +}
> +EXPORT_SYMBOL(st_sensors_get_sampling_frequency_avl);
> +
> +int st_sensors_get_fullscale_avl(struct iio_dev *indio_dev,
> + const struct st_sensor_fullscale_avl *fullscale_avl, char *buf)
> +{
> + int i, len = 0;
> +
> + mutex_lock(&indio_dev->mlock);
> + for (i = 0; i < ST_SENSORS_FULLSCALE_AVL_MAX; i++) {
> + if (fullscale_avl[i].num == 0)
> + break;
> +
> + len += sprintf(buf + len, "0.%06u ", fullscale_avl[i].gain);
> + }
> + mutex_unlock(&indio_dev->mlock);
> + buf[len - 1] = '\n';
> +
> + return len;
> +}
> +EXPORT_SYMBOL(st_sensors_get_fullscale_avl);
> +
> +static int st_sensors_match_odr(const struct st_sensors *sensor,
> + unsigned int odr, struct st_sensor_odr_avl *odr_out)
> +{
> + int i, ret = -EINVAL;
> +
> + for (i = 0; i < ST_SENSORS_ODR_LIST_MAX; i++) {
> + if ((sensor->odr.odr_avl[i].hz == odr) &&
> + (sensor->odr.odr_avl[i].hz != 0)) {
> + odr_out->hz = sensor->odr.odr_avl[i].hz;
> + odr_out->value = sensor->odr.odr_avl[i].value;
> + ret = 0;
> + break;
> + }
> + }
> +
> + return ret;
> +}
> +
> +int st_sensors_set_odr(struct iio_dev *indio_dev,
> + const struct st_sensors *sensor, unsigned int odr)
> +{
> + int err;
> + struct st_sensor_data *sdata = iio_priv(indio_dev);
> + struct st_sensor_odr_avl odr_out;
> +
> + err = st_sensors_match_odr(sensor, odr, &odr_out);
> + if (err < 0)
> + goto st_sensors_match_odr_error;
> +
> + if ((sensor->odr.addr == sensor->pw.addr) &&
> + (sensor->odr.mask == sensor->pw.mask)) {
> + if (sdata->enabled == true) {
> + err = st_sensors_write_data_with_mask(indio_dev,
> + sensor->odr.addr, sensor->odr.mask,
> + sensor->odr.num_bit,
> + odr_out.value);
> + } else {
> + err = 0;
> + }
> + } else {
> + err = st_sensors_write_data_with_mask(indio_dev,
> + sensor->odr.addr, sensor->odr.mask,
> + sensor->odr.num_bit, odr_out.value);
> + }
> + if (err >= 0)
> + sdata->odr = odr_out.hz;
> +
> +st_sensors_match_odr_error:
> + return err;
> +}
> +EXPORT_SYMBOL(st_sensors_set_odr);
> +
> +static int st_sensors_match_fs(const struct st_sensors *sensor,
> + unsigned int fs, struct st_sensor_fullscale_avl *fs_out)
> +{
> + int i, ret = -EINVAL;
> +
> + for (i = 0; i < ST_SENSORS_FULLSCALE_AVL_MAX; i++) {
> + if ((sensor->fs.fs_avl[i].num == fs) &&
> + (sensor->fs.fs_avl[i].num != 0)) {
> + fs_out->num = sensor->fs.fs_avl[i].num;
> + fs_out->gain = sensor->fs.fs_avl[i].gain;
> + fs_out->value = sensor->fs.fs_avl[i].value;
why not
fs_out = &sensor->fs.fs_avl[i];
(obviously with the relevant changes to the call sites.)
> + ret = 0;
> + break;
> + }
> + }
> +
> + return ret;
> +}
> +
> +int st_sensors_set_fullscale(struct iio_dev *indio_dev,
> + const struct st_sensors *sensor, unsigned int fs)
> +{
> + int err;
> + struct st_sensor_data *sdata = iio_priv(indio_dev);
> + struct st_sensor_fullscale_avl fs_out;
> +
> + err = st_sensors_match_fs(sensor, fs, &fs_out);
> + if (err < 0)
> + goto st_accel_set_fullscale_error;
> +
> + err = st_sensors_write_data_with_mask(indio_dev, sensor->fs.addr,
> + sensor->fs.mask, sensor->fs.num_bit, sensor->fs.fs_avl->value);
> + if (err < 0)
> + goto st_accel_set_fullscale_error;
> +
> + sdata->fullscale = sensor->fs.fs_avl->num;
> + sdata->gain = sensor->fs.fs_avl->gain;
> + sdata->gain2 = sensor->fs.fs_avl->gain2;
Is there any reason not to just have fs_avl in sdata
so have a struct st_sensor_fullscale_avl *fs_avl in sdata and
then just set the pointer here?
> + return err;
> +
> +st_accel_set_fullscale_error:
> + dev_err(&indio_dev->dev, "failed to set new fullscale.\n");
> + return err;
> +}
> +EXPORT_SYMBOL(st_sensors_set_fullscale);
> +
> +int st_sensors_set_enable(struct iio_dev *indio_dev,
> + const struct st_sensors *sensor, bool enable)
> +{
> + int err = -EINVAL;
> + bool found;
> + u8 tmp_value;
> + struct st_sensor_odr_avl odr_out;
> + struct st_sensor_data *sdata = iio_priv(indio_dev);
> +
> + if (enable) {
> + found = false;
> + tmp_value = sensor->pw.value_on;
> + if ((sensor->odr.addr == sensor->pw.addr) &&
> + (sensor->odr.mask == sensor->pw.mask)) {
> + err = st_sensors_match_odr(sensor,
> + sdata->odr, &odr_out);
> + if (err < 0)
> + goto set_enable_error;
> + tmp_value = odr_out.value;
> + found = true;
> + }
> + err = st_sensors_write_data_with_mask(indio_dev,
> + sensor->pw.addr, sensor->pw.mask,
> + sensor->pw.num_bit, tmp_value);
> + if (err < 0)
> + goto set_enable_error;
> + sdata->enabled = true;
> + if (found)
> + sdata->odr = odr_out.hz;
> + } else {
> + err = st_sensors_write_data_with_mask(indio_dev,
> + sensor->pw.addr, sensor->pw.mask,
> + sensor->pw.num_bit, sensor->pw.value_off);
> + if (err < 0)
> + goto set_enable_error;
> + sdata->enabled = false;
> + }
> +
> +set_enable_error:
> + return err;
> +}
> +EXPORT_SYMBOL(st_sensors_set_enable);
> +
> +int st_sensors_set_axis_enable(struct iio_dev *indio_dev,
> + const struct st_sensors *sensor, u8 axis_enable)
> +{
> + return st_sensors_write_data_with_mask(indio_dev,
> + sensor->enable_axis.addr, sensor->enable_axis.mask,
> + 3, axis_enable);
> +}
> +EXPORT_SYMBOL(st_sensors_set_axis_enable);
> +
> +int st_sensors_init_sensor(struct iio_dev *indio_dev,
> + const struct st_sensors *sensor)
> +{
> + int err;
> + struct st_sensor_data *sdata = iio_priv(indio_dev);
> +
> + err = st_sensors_set_enable(indio_dev, sensor, false);
> + if (err < 0)
> + goto init_error;
> +
> + err = st_sensors_set_fullscale(indio_dev, sensor, sdata->fullscale);
> + if (err < 0)
> + goto init_error;
> +
> + err = st_sensors_set_odr(indio_dev, sensor, sdata->odr);
> + if (err < 0)
> + goto init_error;
> +
> + /* set BDU */
> + err = st_sensors_write_data_with_mask(indio_dev, sensor->bdu.addr,
> + sensor->bdu.mask, 1, true);
> + if (err < 0)
> + goto init_error;
> +
> + err = st_sensors_set_axis_enable(indio_dev, sensor,
> + ST_SENSORS_ENABLE_ALL_CHANNELS);
> +
> +init_error:
> + return err;
> +}
> +EXPORT_SYMBOL(st_sensors_init_sensor);
> +
> +int st_sensors_set_dataready_irq(struct iio_dev *indio_dev,
> + const struct st_sensors *sensor, bool enable)
> +{
> + int err;
> +
I'd like to see some comments in here as the flow / naming is not entirely
obvious.
> + if (sensor->drdy_irq.ig1.en_addr > 0) {
> + err = st_sensors_write_data_with_mask(indio_dev,
> + sensor->drdy_irq.ig1.en_addr,
> + sensor->drdy_irq.ig1.en_mask, 1, (int)enable);
> + if (err < 0)
> + goto st_accel_set_dataready_irq_error;
> + }
> +
> + if (sensor->drdy_irq.ig1.latch_mask_addr > 0) {
> + err = st_sensors_write_data_with_mask(indio_dev,
> + sensor->drdy_irq.ig1.latch_mask_addr,
> + sensor->drdy_irq.ig1.latching_mask, 1,
> + (int)enable);
> + if (err < 0)
> + goto st_accel_set_dataready_irq_error;
> + }
> +
> + err = st_sensors_write_data_with_mask(indio_dev,
> + sensor->drdy_irq.addr,
> + sensor->drdy_irq.mask, 1, (int)enable);
> +
> +st_accel_set_dataready_irq_error:
> + return err;
> +}
> +EXPORT_SYMBOL(st_sensors_set_dataready_irq);
> +
> +int st_sensors_set_fullscale_by_gain(struct iio_dev *indio_dev,
> + const struct st_sensors *sensor, int scale)
> +{
> + int err = -EINVAL, i;
> +
> + for (i = 0; i < ST_SENSORS_FULLSCALE_AVL_MAX; i++) {
> + if ((sensor->fs.fs_avl[i].gain == scale) &&
> + (sensor->fs.fs_avl[i].gain != 0)) {
> + err = 0;
> + break;
> + }
> + }
> + if (err < 0)
> + goto st_sensors_match_scale_error;
> +
> + err = st_sensors_set_fullscale(indio_dev,
> + sensor, sensor->fs.fs_avl[i].num);
> +
> +st_sensors_match_scale_error:
> + return err;
> +}
> +EXPORT_SYMBOL(st_sensors_set_fullscale_by_gain);
> +
> +int st_sensors_read_axis_data(struct iio_dev *indio_dev, u8 ch_addr, int *data)
> +{
> + int err;
> + u8 outdata[ST_SENSORS_BYTE_FOR_CHANNEL];
> + struct st_sensor_data *sdata = iio_priv(indio_dev);
> +
> + err = sdata->tf.read_multiple_byte(&sdata->tf, sdata->dev,
> + ch_addr, ST_SENSORS_BYTE_FOR_CHANNEL,
> + outdata, sdata->multiread_bit);
> + if (err < 0)
> + goto read_error;
> +
> + *data = ((s16)le16_to_cpup((u16 *)outdata));
> +
> +read_error:
> + return err;
> +}
> +EXPORT_SYMBOL(st_sensors_read_axis_data);
> +
> +MODULE_AUTHOR("Denis Ciocca <denis.ciocca@st.com>");
> +MODULE_DESCRIPTION("STMicroelectronics ST-sensors core");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/iio/common/st_sensors/st_sensors_i2c.c b/drivers/iio/common/st_sensors/st_sensors_i2c.c
> new file mode 100644
> index 0000000..fa4bc7d
> --- /dev/null
> +++ b/drivers/iio/common/st_sensors/st_sensors_i2c.c
> @@ -0,0 +1,78 @@
> +/*
> + * STMicroelectronics sensors i2c library driver
> + *
> + * Copyright 2012 STMicroelectronics Inc.
> + *
> + * Denis Ciocca <denis.ciocca@st.com>
> + *
> + * Licensed under the GPL-2.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/iio/iio.h>
> +
> +#include <linux/iio/common/st_sensors.h>
> +
> +
> +#define ST_SENSORS_I2C_MULTIREAD 0x80
> +
> +unsigned int st_sensors_i2c_get_irq(struct iio_dev *indio_dev)
> +{
> + struct st_sensor_data *sdata = iio_priv(indio_dev);
> + struct i2c_client *client = to_i2c_client(sdata->dev);
> +
> + return client->irq;
> +}
> +
> +static int st_sensors_i2c_read_byte(struct st_sensor_transfer *st,
> + struct device *dev, u8 reg_addr, u8 *res_byte)
> +{
> + int err;
> +
> + err = i2c_smbus_read_byte_data(to_i2c_client(dev), reg_addr);
> + if (err < 0)
> + goto st_accel_i2c_read_byte_error;
> +
> + *res_byte = err & 0xff;
> +
> +st_accel_i2c_read_byte_error:
> + return err < 0 ? err : 0;
> +}
> +
> +static int st_sensors_i2c_read_multiple_byte(struct st_sensor_transfer *st,
> + struct device *dev, u8 reg_addr, int len, u8 *data, bool multiread_bit)
> +{
> + if (multiread_bit == true)
> + reg_addr |= ST_SENSORS_I2C_MULTIREAD;
> +
> + return i2c_smbus_read_i2c_block_data(to_i2c_client(dev),
> + reg_addr, len, data);
> +}
> +
> +static int st_sensors_i2c_write_byte(struct st_sensor_transfer *st,
> + struct device *dev, u8 reg_addr, u8 data)
> +{
> + return i2c_smbus_write_byte_data(to_i2c_client(dev), reg_addr, data);
> +}
> +
> +void st_sensors_i2c_configure(struct iio_dev *indio_dev,
> + struct i2c_client *client, struct st_sensor_data *sdata)
> +{
> + i2c_set_clientdata(client, indio_dev);
> +
> + indio_dev->dev.parent = &client->dev;
> + indio_dev->name = client->name;
> +
> + mutex_init(&sdata->tf.buf_lock);
> + sdata->tf.read_byte = st_sensors_i2c_read_byte;
> + sdata->tf.write_byte = st_sensors_i2c_write_byte;
> + sdata->tf.read_multiple_byte = st_sensors_i2c_read_multiple_byte;
> + sdata->get_irq_data_ready = st_sensors_i2c_get_irq;
I review backwards through code so the same comments as written for the
spi equivalent apply here...
> +}
> +EXPORT_SYMBOL(st_sensors_i2c_configure);
> +
> +MODULE_AUTHOR("Denis Ciocca <denis.ciocca@st.com>");
> +MODULE_DESCRIPTION("STMicroelectronics ST-sensors i2c driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/iio/common/st_sensors/st_sensors_spi.c b/drivers/iio/common/st_sensors/st_sensors_spi.c
> new file mode 100644
> index 0000000..82fa633
> --- /dev/null
> +++ b/drivers/iio/common/st_sensors/st_sensors_spi.c
> @@ -0,0 +1,124 @@
> +/*
> + * STMicroelectronics sensors spi library driver
> + *
> + * Copyright 2012 STMicroelectronics Inc.
> + *
> + * Denis Ciocca <denis.ciocca@st.com>
> + *
> + * Licensed under the GPL-2.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/iio/iio.h>
> +
> +#include <linux/iio/common/st_sensors.h>
> +
> +
> +#define ST_SENSORS_SPI_MULTIREAD 0xc0
> +#define ST_SENSORS_SPI_READ 0x80
> +
> +unsigned int st_sensors_spi_get_irq(struct iio_dev *indio_dev)
> +{
> + struct st_sensor_data *sdata = iio_priv(indio_dev);
> + struct spi_device *spi = to_spi_device(sdata->dev);
> +
> + return spi->irq;
I'd roll this up slightly and drop the intermediate variable.
return to_spi_device(sdata->dev)->irq;
> +}
> +
> +static int st_sensors_spi_read(struct st_sensor_transfer *st,
> + struct device *dev, u8 reg_addr, int len, u8 *data, bool multiread_bit)
> +{
> + struct spi_message msg;
> + int err;
> +
> + struct spi_transfer xfers[] = {
> + {
> + .tx_buf = st->tx_buf,
> + .bits_per_word = 8,
> + .len = 1,
> + },
> + {
> + .rx_buf = st->rx_buf,
> + .bits_per_word = 8,
> + .len = len,
> + }
> + };
> +
> + mutex_lock(&st->buf_lock);
> + if ((multiread_bit == true) && (len > 1))
> + st->tx_buf[0] = reg_addr | ST_SENSORS_SPI_MULTIREAD;
> + else
> + st->tx_buf[0] = reg_addr | ST_SENSORS_SPI_READ;
> +
> + spi_message_init(&msg);
> + spi_message_add_tail(&xfers[0], &msg);
> + spi_message_add_tail(&xfers[1], &msg);
> + err = spi_sync(to_spi_device(dev), &msg);
> + if (err)
> + goto acc_spi_read_error;
> +
> + memcpy(data, st->rx_buf, len*sizeof(u8));
> + mutex_unlock(&st->buf_lock);
> + return len;
> +
> +acc_spi_read_error:
> + return err;
> +}
> +
> +static int st_sensors_spi_read_byte(struct st_sensor_transfer *st,
> + struct device *dev, u8 reg_addr, u8 *res_byte)
> +{
> + return st_sensors_spi_read(st, dev, reg_addr, 1, res_byte, false);
> +}
> +
> +static int st_sensors_spi_read_multiple_byte(struct st_sensor_transfer *st,
> + struct device *dev, u8 reg_addr, int len, u8 *data, bool multiread_bit)
> +{
> + return st_sensors_spi_read(st, dev, reg_addr, len, data, multiread_bit);
> +}
> +
> +static int st_sensors_spi_write_byte(struct st_sensor_transfer *st,
> + struct device *dev, u8 reg_addr, u8 data)
> +{
> + struct spi_message msg;
> + int err;
> +
> + struct spi_transfer xfers = {
> + .tx_buf = st->tx_buf,
> + .bits_per_word = 8,
> + .len = 2,
> + };
> +
> + mutex_lock(&st->buf_lock);
> + st->tx_buf[0] = reg_addr;
> + st->tx_buf[1] = data;
> +
> + spi_message_init(&msg);
> + spi_message_add_tail(&xfers, &msg);
> + err = spi_sync(to_spi_device(dev), &msg);
> + mutex_unlock(&st->buf_lock);
> +
> + return err;
> +}
> +
> +void st_sensors_spi_configure(struct iio_dev *indio_dev,
> + struct spi_device *spi, struct st_sensor_data *sdata)
> +{
> + spi_set_drvdata(spi, indio_dev);
> +
> + indio_dev->dev.parent = &spi->dev;
> + indio_dev->name = spi->modalias;
> +
> + mutex_init(&sdata->tf.buf_lock);
Silly though it may seem I thik I'd prefer the mutex init in
st_sensors_init_sensor as it isn't spi or i2c specific whereas
everything else in here is...
> + sdata->tf.read_byte = st_sensors_spi_read_byte;
> + sdata->tf.write_byte = st_sensors_spi_write_byte;
> + sdata->tf.read_multiple_byte = st_sensors_spi_read_multiple_byte;
Might be worth wrapping these callbacks up in a const static structure rather
than along side the dynamic data in tf. (like we do with struct iio_info within
struct iio_dev)
> + sdata->get_irq_data_ready = st_sensors_spi_get_irq;
> +}
> +EXPORT_SYMBOL(st_sensors_spi_configure);
> +
> +MODULE_AUTHOR("Denis Ciocca <denis.ciocca@st.com>");
> +MODULE_DESCRIPTION("STMicroelectronics ST-sensors spi driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/iio/common/st_sensors/st_sensors_trigger.c b/drivers/iio/common/st_sensors/st_sensors_trigger.c
> new file mode 100644
> index 0000000..6d43b13
> --- /dev/null
> +++ b/drivers/iio/common/st_sensors/st_sensors_trigger.c
> @@ -0,0 +1,76 @@
> +/*
> + * STMicroelectronics sensors trigger library driver
> + *
> + * Copyright 2012 STMicroelectronics Inc.
> + *
> + * Denis Ciocca <denis.ciocca@st.com>
> + *
> + * Licensed under the GPL-2.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/interrupt.h>
> +
> +#include <linux/iio/common/st_sensors.h>
> +
> +int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
> + const struct iio_trigger_ops *trig_ops)
> +{
> + int err;
> + struct st_sensor_data *sdata = iio_priv(indio_dev);
> +
> + sdata->trig = iio_trigger_alloc("%s-trigger", indio_dev->name);
> + if (sdata->trig == NULL) {
> + err = -ENOMEM;
> + dev_err(&indio_dev->dev, "failed to allocate iio trigger.\n");
> + goto iio_trigger_alloc_error;
> + }
> +
> + err = request_threaded_irq(sdata->get_irq_data_ready(indio_dev),
> + iio_trigger_generic_data_rdy_poll,
> + NULL,
> + IRQF_TRIGGER_RISING,
> + sdata->trig->name,
> + sdata->trig);
> + if (err)
> + goto request_irq_error;
> +
> + sdata->trig->private_data = indio_dev;
> + sdata->trig->ops = trig_ops;
> + sdata->trig->dev.parent = sdata->dev;
> +
> + err = iio_trigger_register(sdata->trig);
> + if (err < 0) {
> + dev_err(&indio_dev->dev, "failed to register iio trigger.\n");
> + goto iio_trigger_register_error;
> + }
> + indio_dev->trig = sdata->trig;
> +
> + return 0;
> +
> +iio_trigger_register_error:
> + free_irq(sdata->get_irq_data_ready(indio_dev), sdata->trig);
> +request_irq_error:
> + iio_trigger_free(sdata->trig);
> +iio_trigger_alloc_error:
> + return err;
> +}
> +EXPORT_SYMBOL(st_sensors_allocate_trigger);
> +
> +void st_sensors_deallocate_trigger(struct iio_dev *indio_dev)
> +{
> + struct st_sensor_data *sdata = iio_priv(indio_dev);
> +
> + iio_trigger_unregister(sdata->trig);
> + free_irq(sdata->get_irq_data_ready(indio_dev), sdata->trig);
> + iio_trigger_free(sdata->trig);
> +}
> +EXPORT_SYMBOL(st_sensors_deallocate_trigger);
> +
> +MODULE_AUTHOR("Denis Ciocca <denis.ciocca@st.com>");
> +MODULE_DESCRIPTION("STMicroelectronics ST-sensors trigger");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/iio/common/st_sensors.h b/include/linux/iio/common/st_sensors.h
> new file mode 100644
> index 0000000..c0f2acc
> --- /dev/null
> +++ b/include/linux/iio/common/st_sensors.h
> @@ -0,0 +1,246 @@
> +/*
> + * STMicroelectronics sensors library driver
> + *
> + * Copyright 2012 STMicroelectronics Inc.
> + *
> + * Denis Ciocca <denis.ciocca@st.com>
> + *
> + * Licensed under the GPL-2.
> + */
> +
> +#ifndef ST_SENSORS_H
> +#define ST_SENSORS_H
> +
> +#include <linux/i2c.h>
> +#include <linux/spi/spi.h>
> +#include <linux/irqreturn.h>
> +#include <linux/iio/trigger.h>
> +
> +
> +#define ST_SENSORS_TX_MAX_LENGHT 2
> +#define ST_SENSORS_RX_MAX_LENGHT 6
> +
> +#define ST_SENSORS_ODR_LIST_MAX 10
> +#define ST_SENSORS_FULLSCALE_AVL_MAX 10
> +
> +#define ST_SENSORS_NUMBER_ALL_CHANNELS 4
> +#define ST_SENSORS_NUMBER_DATA_CHANNELS 3
> +#define ST_SENSORS_ENABLE_ALL_CHANNELS 0x07
> +#define ST_SENSORS_BYTE_FOR_CHANNEL 2
> +#define ST_SENSORS_SCAN_X 0
> +#define ST_SENSORS_SCAN_Y 1
> +#define ST_SENSORS_SCAN_Z 2
> +#define ST_SENSORS_DEFAULT_12_REALBITS 12
> +#define ST_SENSORS_DEFAULT_16_REALBITS 16
> +#define ST_SENSORS_DEFAULT_POWER_ON_VALUE 0x01
> +#define ST_SENSORS_DEFAULT_POWER_OFF_VALUE 0x00
> +#define ST_SENSORS_DEFAULT_WAI_ADDRESS 0x0f
> +#define ST_SENSORS_DEFAULT_AXIS_ADDR 0x20
> +#define ST_SENSORS_DEFAULT_AXIS_MASK 0x07
> +#define ST_SENSORS_DEFAULT_AXIS_N_BIT 3
> +
> +#define ST_SENSORS_LSM_CHANNELS(device_type, index, mod, endian, bits, addr) \
> +{ \
> + .type = device_type, \
> + .modified = 1, \
> + .info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT | \
> + IIO_CHAN_INFO_SCALE_SEPARATE_BIT, \
> + .scan_index = index, \
> + .channel2 = mod, \
> + .address = addr, \
> + .scan_type = { \
> + .sign = 's', \
> + .realbits = bits, \
> + .shift = 16 - bits, \
> + .storagebits = 16, \
> + .endianness = endian, \
> + }, \
> +}
> +
> +struct st_sensor_odr_avl {
> + unsigned int hz;
> + u8 value;
> +};
> +
> +struct st_sensor_odr {
> + u8 addr;
> + u8 mask;
> + short num_bit;
> + struct st_sensor_odr_avl odr_avl[ST_SENSORS_ODR_LIST_MAX];
> +};
> +
> +struct st_sensor_power {
> + u8 addr;
> + u8 mask;
> + unsigned short num_bit;
> + u8 value_off;
> + u8 value_on;
> +};
> +
> +struct st_sensor_axis {
> + u8 addr;
> + u8 mask;
> +};
> +
> +struct st_sensor_fullscale_avl {
> + unsigned int num;
> + u8 value;
> + unsigned int gain;
> + unsigned int gain2;
> +};
> +
> +struct st_sensor_fullscale {
> + u8 addr;
> + u8 mask;
> + unsigned short num_bit;
> + struct st_sensor_fullscale_avl fs_avl[ST_SENSORS_FULLSCALE_AVL_MAX];
> +};
> +
> +struct st_sensor_bdu {
> + u8 addr;
> + u8 mask;
> +};
> +
> +struct st_sensor_interrupt_generator {
> + u8 en_addr;
> + u8 latch_mask_addr;
> + u8 en_mask;
> + u8 latching_mask;
> +};
> +
> +struct st_sensor_data_ready_irq {
> + u8 addr;
> + u8 mask;
> + struct st_sensor_interrupt_generator ig1;
I'd just embed the definition in here
i.e. (assuming I remember the syntax correctly...)
struct st_sensor_data_read_irq {
u8 addr;
u8 mask;
struct {
u8 en_addr;
u8 latch_mask_addr;
u8 en_mask;
u8 latching_mask;
} ig1;
};
> +};
> +
> +/**
> + * struct st_sensor_transfer - ST sensor device I/O struct
> + * @buf_lock: Mutex to protect rx and tx buffers.
> + * @tx_buf: Buffer used by SPI transfer function to send data to the sensors.
> + * This buffer is used to avoid DMA not-aligned issue.
> + * @rx_buf: Buffer used by SPI transfer to receive data from sensors.
> + * This buffer is used to avoid DMA not-aligned issue.
> + * @read_byte: Function used to read one byte.
> + * @write_byte: Function used to write one byte.
> + * @read_multiple_byte: Function used to read multiple byte.
> + */
> +struct st_sensor_transfer {
> + struct mutex buf_lock;
Move these to the end of the structure and all is fine
(as long as this is the last structure in any containing one as well -
see below).
Also only need to force alignment of the tx_buf
(as the rx_buf will pack after it and any hardware
that corrupts that is plain mad)
> + u8 tx_buf[ST_SENSORS_TX_MAX_LENGHT] ____cacheline_aligned;
> + u8 rx_buf[ST_SENSORS_RX_MAX_LENGHT] ____cacheline_aligned;
> + int (*read_byte) (struct st_sensor_transfer *st, struct device *dev,
> + u8 reg_addr, u8 *res_byte);
> + int (*write_byte) (struct st_sensor_transfer *st, struct device *dev,
> + u8 reg_addr, u8 data);
> + int (*read_multiple_byte) (struct st_sensor_transfer *st,
> + struct device *dev, u8 reg_addr, int len, u8 *data,
> + bool multiread_bit);
> +};
> +
> +/**
> + * struct st_sensor_data - ST sensor device status
> + * @dev: Pointer to instance of struct device (I2C or SPI).
> + * @trig: The trigger in use by the core driver.
> + * @enabled: Status of the sensor (false->off, true->on).
> + * @multiread_bit: Use or not particular bit for [I2C/SPI] multiread.
> + * @index: Number used to point the sensor being used in the st_sensors struct.
> + * @buffer_data: Data used by buffer part.
> + * @fullscale: Maximum range of measure by the sensor.
> + * @gain: Sensitivity of the sensor [m/s^2/LSB].
> + * @odr: Output data rate of the sensor [Hz].
> + * @tf: Transfer function structure used by I/O operations.
> + */
> +struct st_sensor_data {
> + struct device *dev;
> + struct iio_trigger *trig;
> +
> + bool enabled;
> + bool multiread_bit;
> +
> + short index;
> +
> + char *buffer_data;
> +
> + unsigned int fullscale;
> + unsigned int gain;
> + unsigned int gain2;
> + unsigned int odr;
> +
This will need to also be at the end of this structure to ensure
that the below callback pointer doesn't end up on the same cacheline as
the tx_buf and rx_buf. A comment saying that this is a requirement here
will also make future bugs much less likely.
> + struct st_sensor_transfer tf;
> +
> + unsigned int (*get_irq_data_ready) (struct iio_dev *indio_dev);
> +};
> +
> +/**
> + * struct st_sensors - ST sensors list
> + * @wai: Contents of WhoAmI register.
> + * @ch: IIO channels for the sensor.
> + * @odr: Output data rate register and odr list available.
> + * @pw: Power register of the sensor.
> + * @enable_axis: Enable one or more axis of the sensor.
> + * @fs: Full scale register and fs list available.
> + * @bdu: Block data update register.
> + * @drdy_irq: Data ready register of the sensor.
> + * @multi_read_bit: Use or not particular bit for [I2C/SPI] multiread.
> + */
> +struct st_sensors {
> + u8 wai;
> + struct iio_chan_spec *ch;
> + struct st_sensor_odr odr;
> + struct st_sensor_power pw;
> + struct st_sensor_axis enable_axis;
> + struct st_sensor_fullscale fs;
> + struct st_sensor_bdu bdu;
> + struct st_sensor_data_ready_irq drdy_irq;
> + bool multi_read_bit;
> +};
> +
Either break these out to separate headers or deal with the fact that
spi or i2c might not be present in a given build with ifdef fun and
games.
> +void st_sensors_spi_configure(struct iio_dev *indio_dev,
> + struct spi_device *spi, struct st_sensor_data *sdata);
> +
> +void st_sensors_i2c_configure(struct iio_dev *indio_dev,
> + struct i2c_client *client, struct st_sensor_data *sdata);
> +
> +int st_sensors_write_data_with_mask(struct iio_dev *indio_dev, u8 reg_addr,
> + u8 mask, short num_bit, u8 data);
> +
> +int st_sensors_get_sampling_frequency_avl(struct iio_dev *indio_dev,
> + const struct st_sensor_odr_avl *odr_avl, char *buf);
> +
> +int st_sensors_get_fullscale_avl(struct iio_dev *indio_dev,
> + const struct st_sensor_fullscale_avl *fullscale_avl, char *buf);
> +
> +int st_sensors_set_odr(struct iio_dev *indio_dev,
> + const struct st_sensors *sensor, unsigned int odr);
> +
> +int st_sensors_set_fullscale(struct iio_dev *indio_dev,
> + const struct st_sensors *sensor, unsigned int fs);
> +
> +int st_sensors_set_enable(struct iio_dev *indio_dev,
> + const struct st_sensors *sensor, bool enable);
> +
> +int st_sensors_set_axis_enable(struct iio_dev *indio_dev,
> + const struct st_sensors *sensor, u8 axis_enable);
> +
> +int st_sensors_init_sensor(struct iio_dev *indio_dev,
> + const struct st_sensors *sensor);
> +
> +int st_sensors_set_dataready_irq(struct iio_dev *indio_dev,
> + const struct st_sensors *sensor, bool enable);
> +
> +int st_sensors_set_fullscale_by_gain(struct iio_dev *indio_dev,
> + const struct st_sensors *sensor, int scale);
> +
> +int st_sensors_read_axis_data(struct iio_dev *indio_dev, u8 ch_addr, int *data);
> +
> +int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
> + const struct iio_trigger_ops *trig_ops);
> +
> +void st_sensors_deallocate_trigger(struct iio_dev *indio_dev);
> +
> +int st_sensors_get_buffer_element(struct iio_dev *indio_dev, u8 *buf);
> +
> +irqreturn_t st_sensors_trigger_handler(int irq, void *p);
> +
> +#endif /* ST_SENSORS_H */
>
next prev parent reply other threads:[~2012-12-16 12:15 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-12-13 13:11 STMicroelectronics sensors driver Denis CIOCCA
2012-12-13 13:11 ` [PATCH 1/4] iio:common: Add STMicroelectronics common library Denis CIOCCA
2012-12-16 12:15 ` Jonathan Cameron [this message]
2012-12-13 13:11 ` [PATCH 2/4] iio:accel: Add STMicroelectronics accelerometers driver Denis CIOCCA
2012-12-16 11:51 ` Jonathan Cameron
2012-12-13 13:11 ` [PATCH 3/4] iio:gyro: Add STMicroelectronics gyroscopes driver Denis CIOCCA
2012-12-13 13:11 ` [PATCH 4/4] iio:magnetometer: Add STMicroelectronics magnetometers driver Denis CIOCCA
2012-12-16 11:28 ` STMicroelectronics sensors driver Jonathan Cameron
-- strict thread matches above, loose matches on Subject: below --
2013-01-08 16:30 iio: STMicroelectronics iio drivers Denis CIOCCA
2013-01-08 16:30 ` [PATCH 1/4] iio:common: Add STMicroelectronics common library Denis CIOCCA
2013-01-09 12:06 iio: STMicroelectronics iio drivers bugfix Denis CIOCCA
2013-01-09 12:06 ` [PATCH 1/4] iio:common: Add STMicroelectronics common library Denis CIOCCA
2013-01-12 21:59 ` Jonathan Cameron
2013-01-18 9:31 STMicroelectronics drivers bugfix Denis CIOCCA
2013-01-18 9:31 ` [PATCH 1/4] iio:common: Add STMicroelectronics common library Denis CIOCCA
2013-01-18 16:58 STMicroelectronics drivers Denis CIOCCA
2013-01-18 16:58 ` [PATCH 1/4] iio:common: Add STMicroelectronics common library Denis CIOCCA
2013-01-21 8:36 STMicroelectronics driver kconfig-makefile bugfix Denis CIOCCA
2013-01-21 8:36 ` [PATCH 1/4] iio:common: Add STMicroelectronics common library Denis CIOCCA
2013-01-22 16:15 ` Jonathan Cameron
2013-01-23 9:33 ` Lars-Peter Clausen
2013-01-25 23:44 STMicroelectronics IIO driver Denis Ciocca
2013-01-25 23:44 ` [PATCH 1/4] iio:common: Add STMicroelectronics common library Denis Ciocca
2013-01-29 21:45 ` Lars-Peter Clausen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=50CDBB59.2070906@kernel.org \
--to=jic23@kernel.org \
--cc=denis.ciocca@st.com \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.