* [PATCH] iio: accel: st_accel: add SPI-3wire support
@ 2017-07-03 18:07 Lorenzo Bianconi
2017-07-04 19:26 ` Jonathan Cameron
0 siblings, 1 reply; 4+ messages in thread
From: Lorenzo Bianconi @ 2017-07-03 18:07 UTC (permalink / raw)
To: jic23; +Cc: linux-iio, lorenzo.bianconi
Add sensor interface mode (SIM) lookup table to support devices
(like LSM303AGR accel sensor) that support just SPI-3wire
communication mode. SIM mode has to be configured before any
other operation since it is not enabled by default and the driver
is not able to read without that configuration
Fixes: ddc05fa28606 (iio: st-accel: add support for lsm303agr accel)
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@st.com>
---
drivers/iio/accel/st_accel_core.c | 37 +++++++++++++++++++++++++
drivers/iio/common/st_sensors/st_sensors_core.c | 36 ++++++++++++++++++++++++
include/linux/iio/common/st_sensors.h | 11 ++++++++
include/linux/platform_data/st_sensors_pdata.h | 2 ++
4 files changed, 86 insertions(+)
diff --git a/drivers/iio/accel/st_accel_core.c b/drivers/iio/accel/st_accel_core.c
index 07d1489cd457..195966e82516 100644
--- a/drivers/iio/accel/st_accel_core.c
+++ b/drivers/iio/accel/st_accel_core.c
@@ -619,6 +619,38 @@ static const struct st_sensor_settings st_accel_sensors_settings[] = {
},
};
+static const struct st_sensor_sim st_accel_sim_table[] = {
+ {
+ .sensors = {
+ [0] = LSM303AGR_ACCEL_DEV_NAME,
+ [1] = LIS3DH_ACCEL_DEV_NAME,
+ [2] = LIS2DH12_ACCEL_DEV_NAME,
+ [3] = LIS331DLH_ACCEL_DEV_NAME,
+ [4] = LNG2DM_ACCEL_DEV_NAME,
+ [5] = LSM330D_ACCEL_DEV_NAME,
+ [6] = LSM330DL_ACCEL_DEV_NAME,
+ [7] = LSM330DLC_ACCEL_DEV_NAME,
+ },
+ .addr = 0x23,
+ .val = BIT(0),
+ },
+ {
+ .sensors = {
+ [0] = LSM330_ACCEL_DEV_NAME,
+ },
+ .addr = 0x24,
+ .val = BIT(0),
+ },
+ {
+ .sensors = {
+ [0] = LIS3L02DQ_ACCEL_DEV_NAME,
+ [1] = LIS3LV02DL_ACCEL_DEV_NAME,
+ },
+ .addr = 0x21,
+ .val = BIT(1),
+ },
+};
+
static int st_accel_read_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *ch, int *val,
int *val2, long mask)
@@ -719,6 +751,11 @@ int st_accel_common_probe(struct iio_dev *indio_dev)
indio_dev->info = &accel_info;
mutex_init(&adata->tb.buf_lock);
+ err = st_sensors_init_interface_mode(indio_dev, st_accel_sim_table,
+ ARRAY_SIZE(st_accel_sim_table));
+ if (err < 0)
+ return err;
+
err = st_sensors_power_enable(indio_dev);
if (err)
return err;
diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
index 274868100fd0..50c281b6286d 100644
--- a/drivers/iio/common/st_sensors/st_sensors_core.c
+++ b/drivers/iio/common/st_sensors/st_sensors_core.c
@@ -384,6 +384,42 @@ static struct st_sensors_platform_data *st_sensors_of_probe(struct device *dev,
}
#endif
+int st_sensors_init_interface_mode(struct iio_dev *indio_dev,
+ const struct st_sensor_sim *sim_table,
+ int sim_len)
+{
+ struct st_sensor_data *sdata = iio_priv(indio_dev);
+ struct device_node *np = sdata->dev->of_node;
+ struct st_sensors_platform_data *pdata;
+ int err = 0;
+
+ pdata = (struct st_sensors_platform_data *)sdata->dev->platform_data;
+ if ((np && of_property_read_bool(np, "spi-3wire")) ||
+ (pdata && pdata->spi_3wire)) {
+ int i, j;
+
+ for (i = 0; i < sim_len; i++) {
+ for (j = 0; j < ST_SENSORS_MAX_SIM; j++) {
+ if (!strcmp(sim_table[i].sensors[j],
+ indio_dev->name))
+ break;
+ }
+ if (j < ST_SENSORS_MAX_SIM)
+ break;
+ }
+
+ if (i == sim_len)
+ return -EINVAL;
+
+ err = sdata->tf->write_byte(&sdata->tb, sdata->dev,
+ sim_table[i].addr,
+ sim_table[i].val);
+ }
+
+ return err;
+}
+EXPORT_SYMBOL(st_sensors_init_interface_mode);
+
int st_sensors_init_sensor(struct iio_dev *indio_dev,
struct st_sensors_platform_data *pdata)
{
diff --git a/include/linux/iio/common/st_sensors.h b/include/linux/iio/common/st_sensors.h
index 1f8211b6438b..15ce0cb360d7 100644
--- a/include/linux/iio/common/st_sensors.h
+++ b/include/linux/iio/common/st_sensors.h
@@ -41,6 +41,7 @@
#define ST_SENSORS_MAX_NAME 17
#define ST_SENSORS_MAX_4WAI 7
+#define ST_SENSORS_MAX_SIM 9
#define ST_SENSORS_LSM_CHANNELS(device_type, mask, index, mod, \
ch2, s, endian, rbits, sbits, addr) \
@@ -105,6 +106,12 @@ struct st_sensor_fullscale {
struct st_sensor_fullscale_avl fs_avl[ST_SENSORS_FULLSCALE_AVL_MAX];
};
+struct st_sensor_sim {
+ char sensors[ST_SENSORS_MAX_SIM][ST_SENSORS_MAX_NAME];
+ u8 addr;
+ u8 val;
+};
+
/**
* struct st_sensor_bdu - ST sensor device block data update
* @addr: address of the register.
@@ -325,6 +332,10 @@ ssize_t st_sensors_sysfs_sampling_frequency_avail(struct device *dev,
ssize_t st_sensors_sysfs_scale_avail(struct device *dev,
struct device_attribute *attr, char *buf);
+int st_sensors_init_interface_mode(struct iio_dev *indio_dev,
+ const struct st_sensor_sim *sim_table,
+ int sim_len);
+
#ifdef CONFIG_OF
void st_sensors_of_name_probe(struct device *dev,
const struct of_device_id *match,
diff --git a/include/linux/platform_data/st_sensors_pdata.h b/include/linux/platform_data/st_sensors_pdata.h
index 79b0e4cdb814..f8274b0c6888 100644
--- a/include/linux/platform_data/st_sensors_pdata.h
+++ b/include/linux/platform_data/st_sensors_pdata.h
@@ -17,10 +17,12 @@
* Available only for accelerometer and pressure sensors.
* Accelerometer DRDY on LSM330 available only on pin 1 (see datasheet).
* @open_drain: set the interrupt line to be open drain if possible.
+ * @spi_3wire: enable spi-3wire mode.
*/
struct st_sensors_platform_data {
u8 drdy_int_pin;
bool open_drain;
+ bool spi_3wire;
};
#endif /* ST_SENSORS_PDATA_H */
--
2.13.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] iio: accel: st_accel: add SPI-3wire support
2017-07-03 18:07 [PATCH] iio: accel: st_accel: add SPI-3wire support Lorenzo Bianconi
@ 2017-07-04 19:26 ` Jonathan Cameron
2017-07-05 8:27 ` Lorenzo Bianconi
0 siblings, 1 reply; 4+ messages in thread
From: Jonathan Cameron @ 2017-07-04 19:26 UTC (permalink / raw)
To: Lorenzo Bianconi; +Cc: linux-iio, lorenzo.bianconi
On Mon, 3 Jul 2017 20:07:11 +0200
Lorenzo Bianconi <lorenzo.bianconi83@gmail.com> wrote:
> Add sensor interface mode (SIM) lookup table to support devices
> (like LSM303AGR accel sensor) that support just SPI-3wire
> communication mode. SIM mode has to be configured before any
> other operation since it is not enabled by default and the driver
> is not able to read without that configuration
>
> Fixes: ddc05fa28606 (iio: st-accel: add support for lsm303agr accel)
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@st.com>
Just checked and the 3-wire device tree property is documented in spi-bus.txt
So, that's all fine. However, I'm not keen on introducing a special look up
table for this when we already have a convenient one covering all the other
per device registers etc.
Another approach suggested inline...
> ---
> drivers/iio/accel/st_accel_core.c | 37 +++++++++++++++++++++++++
> drivers/iio/common/st_sensors/st_sensors_core.c | 36 ++++++++++++++++++++++++
> include/linux/iio/common/st_sensors.h | 11 ++++++++
> include/linux/platform_data/st_sensors_pdata.h | 2 ++
> 4 files changed, 86 insertions(+)
>
> diff --git a/drivers/iio/accel/st_accel_core.c b/drivers/iio/accel/st_accel_core.c
> index 07d1489cd457..195966e82516 100644
> --- a/drivers/iio/accel/st_accel_core.c
> +++ b/drivers/iio/accel/st_accel_core.c
> @@ -619,6 +619,38 @@ static const struct st_sensor_settings st_accel_sensors_settings[] = {
> },
> };
>
> +static const struct st_sensor_sim st_accel_sim_table[] = {
> + {
> + .sensors = {
> + [0] = LSM303AGR_ACCEL_DEV_NAME,
> + [1] = LIS3DH_ACCEL_DEV_NAME,
> + [2] = LIS2DH12_ACCEL_DEV_NAME,
> + [3] = LIS331DLH_ACCEL_DEV_NAME,
> + [4] = LNG2DM_ACCEL_DEV_NAME,
> + [5] = LSM330D_ACCEL_DEV_NAME,
> + [6] = LSM330DL_ACCEL_DEV_NAME,
> + [7] = LSM330DLC_ACCEL_DEV_NAME,
The index has no meaning as we really have an unordered
list so perhaps drop it and let it automatically place them.
Doing it by name is particularly ugly given we already
have a big look up table for device differences.
One option would be to split the st_sensors_check_device_support
function into two parts - the first does the match by name
and the second does the wai check. That way you could
then stick your configuration in between the two and
put this info in with all the other device specific
characteristics.
To make the transition easy you could add the two split
functions and call them from the original. Then you can
introduce the split as needed into the various st sensor
drivers.
> + },
> + .addr = 0x23,
> + .val = BIT(0),
> + },
> + {
> + .sensors = {
> + [0] = LSM330_ACCEL_DEV_NAME,
> + },
> + .addr = 0x24,
> + .val = BIT(0),
> + },
> + {
> + .sensors = {
> + [0] = LIS3L02DQ_ACCEL_DEV_NAME,
> + [1] = LIS3LV02DL_ACCEL_DEV_NAME,
> + },
> + .addr = 0x21,
> + .val = BIT(1),
> + },
> +};
> +
> static int st_accel_read_raw(struct iio_dev *indio_dev,
> struct iio_chan_spec const *ch, int *val,
> int *val2, long mask)
> @@ -719,6 +751,11 @@ int st_accel_common_probe(struct iio_dev *indio_dev)
> indio_dev->info = &accel_info;
> mutex_init(&adata->tb.buf_lock);
>
> + err = st_sensors_init_interface_mode(indio_dev, st_accel_sim_table,
> + ARRAY_SIZE(st_accel_sim_table));
> + if (err < 0)
> + return err;
> +
> err = st_sensors_power_enable(indio_dev);
> if (err)
> return err;
> diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
> index 274868100fd0..50c281b6286d 100644
> --- a/drivers/iio/common/st_sensors/st_sensors_core.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_core.c
> @@ -384,6 +384,42 @@ static struct st_sensors_platform_data *st_sensors_of_probe(struct device *dev,
> }
> #endif
>
> +int st_sensors_init_interface_mode(struct iio_dev *indio_dev,
> + const struct st_sensor_sim *sim_table,
> + int sim_len)
> +{
> + struct st_sensor_data *sdata = iio_priv(indio_dev);
> + struct device_node *np = sdata->dev->of_node;
> + struct st_sensors_platform_data *pdata;
> + int err = 0;
> +
> + pdata = (struct st_sensors_platform_data *)sdata->dev->platform_data;
> + if ((np && of_property_read_bool(np, "spi-3wire")) ||
> + (pdata && pdata->spi_3wire)) {
> + int i, j;
> +
> + for (i = 0; i < sim_len; i++) {
> + for (j = 0; j < ST_SENSORS_MAX_SIM; j++) {
> + if (!strcmp(sim_table[i].sensors[j],
> + indio_dev->name))
> + break;
> + }
> + if (j < ST_SENSORS_MAX_SIM)
> + break;
> + }
> +
> + if (i == sim_len)
> + return -EINVAL;
> +
> + err = sdata->tf->write_byte(&sdata->tb, sdata->dev,
> + sim_table[i].addr,
> + sim_table[i].val);
> + }
> +
> + return err;
> +}
> +EXPORT_SYMBOL(st_sensors_init_interface_mode);
> +
> int st_sensors_init_sensor(struct iio_dev *indio_dev,
> struct st_sensors_platform_data *pdata)
> {
> diff --git a/include/linux/iio/common/st_sensors.h b/include/linux/iio/common/st_sensors.h
> index 1f8211b6438b..15ce0cb360d7 100644
> --- a/include/linux/iio/common/st_sensors.h
> +++ b/include/linux/iio/common/st_sensors.h
> @@ -41,6 +41,7 @@
>
> #define ST_SENSORS_MAX_NAME 17
> #define ST_SENSORS_MAX_4WAI 7
> +#define ST_SENSORS_MAX_SIM 9
>
> #define ST_SENSORS_LSM_CHANNELS(device_type, mask, index, mod, \
> ch2, s, endian, rbits, sbits, addr) \
> @@ -105,6 +106,12 @@ struct st_sensor_fullscale {
> struct st_sensor_fullscale_avl fs_avl[ST_SENSORS_FULLSCALE_AVL_MAX];
> };
>
> +struct st_sensor_sim {
> + char sensors[ST_SENSORS_MAX_SIM][ST_SENSORS_MAX_NAME];
> + u8 addr;
> + u8 val;
> +};
> +
> /**
> * struct st_sensor_bdu - ST sensor device block data update
> * @addr: address of the register.
> @@ -325,6 +332,10 @@ ssize_t st_sensors_sysfs_sampling_frequency_avail(struct device *dev,
> ssize_t st_sensors_sysfs_scale_avail(struct device *dev,
> struct device_attribute *attr, char *buf);
>
> +int st_sensors_init_interface_mode(struct iio_dev *indio_dev,
> + const struct st_sensor_sim *sim_table,
> + int sim_len);
> +
> #ifdef CONFIG_OF
> void st_sensors_of_name_probe(struct device *dev,
> const struct of_device_id *match,
> diff --git a/include/linux/platform_data/st_sensors_pdata.h b/include/linux/platform_data/st_sensors_pdata.h
> index 79b0e4cdb814..f8274b0c6888 100644
> --- a/include/linux/platform_data/st_sensors_pdata.h
> +++ b/include/linux/platform_data/st_sensors_pdata.h
> @@ -17,10 +17,12 @@
> * Available only for accelerometer and pressure sensors.
> * Accelerometer DRDY on LSM330 available only on pin 1 (see datasheet).
> * @open_drain: set the interrupt line to be open drain if possible.
> + * @spi_3wire: enable spi-3wire mode.
> */
> struct st_sensors_platform_data {
> u8 drdy_int_pin;
> bool open_drain;
> + bool spi_3wire;
> };
>
> #endif /* ST_SENSORS_PDATA_H */
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] iio: accel: st_accel: add SPI-3wire support
2017-07-04 19:26 ` Jonathan Cameron
@ 2017-07-05 8:27 ` Lorenzo Bianconi
2017-07-05 8:41 ` Jonathan Cameron
0 siblings, 1 reply; 4+ messages in thread
From: Lorenzo Bianconi @ 2017-07-05 8:27 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: linux-iio, Lorenzo BIANCONI
> On Mon, 3 Jul 2017 20:07:11 +0200
> Lorenzo Bianconi <lorenzo.bianconi83@gmail.com> wrote:
>
>> Add sensor interface mode (SIM) lookup table to support devices
>> (like LSM303AGR accel sensor) that support just SPI-3wire
>> communication mode. SIM mode has to be configured before any
>> other operation since it is not enabled by default and the driver
>> is not able to read without that configuration
>>
>> Fixes: ddc05fa28606 (iio: st-accel: add support for lsm303agr accel)
>> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@st.com>
> Just checked and the 3-wire device tree property is documented in spi-bus.txt
>
> So, that's all fine. However, I'm not keen on introducing a special look up
> table for this when we already have a convenient one covering all the other
> per device registers etc.
Hi Jonathan,
thanks for the review
>
> Another approach suggested inline...
>> ---
>> drivers/iio/accel/st_accel_core.c | 37 +++++++++++++++++++++++++
>> drivers/iio/common/st_sensors/st_sensors_core.c | 36 ++++++++++++++++++++++++
>> include/linux/iio/common/st_sensors.h | 11 ++++++++
>> include/linux/platform_data/st_sensors_pdata.h | 2 ++
>> 4 files changed, 86 insertions(+)
>>
>> diff --git a/drivers/iio/accel/st_accel_core.c b/drivers/iio/accel/st_accel_core.c
>> index 07d1489cd457..195966e82516 100644
>> --- a/drivers/iio/accel/st_accel_core.c
>> +++ b/drivers/iio/accel/st_accel_core.c
>> @@ -619,6 +619,38 @@ static const struct st_sensor_settings st_accel_sensors_settings[] = {
>> },
>> };
>>
>> +static const struct st_sensor_sim st_accel_sim_table[] = {
>> + {
>> + .sensors = {
>> + [0] = LSM303AGR_ACCEL_DEV_NAME,
>> + [1] = LIS3DH_ACCEL_DEV_NAME,
>> + [2] = LIS2DH12_ACCEL_DEV_NAME,
>> + [3] = LIS331DLH_ACCEL_DEV_NAME,
>> + [4] = LNG2DM_ACCEL_DEV_NAME,
>> + [5] = LSM330D_ACCEL_DEV_NAME,
>> + [6] = LSM330DL_ACCEL_DEV_NAME,
>> + [7] = LSM330DLC_ACCEL_DEV_NAME,
> The index has no meaning as we really have an unordered
> list so perhaps drop it and let it automatically place them.
Ack
>
> Doing it by name is particularly ugly given we already
> have a big look up table for device differences.
>
> One option would be to split the st_sensors_check_device_support
> function into two parts - the first does the match by name
> and the second does the wai check. That way you could
> then stick your configuration in between the two and
> put this info in with all the other device specific
> characteristics.
>
> To make the transition easy you could add the two split
> functions and call them from the original. Then you can
> introduce the split as needed into the various st sensor
> drivers.
Fine. I guess it is enough to add a static version of
st_sensors_init_interface_mode
with a reference to the right entry in st_sensor_settings from the top
half of st_sensors_check_device_support.
If the everything is ok with SIM configuration we will go forward to
wai check in st_sensors_check_device_support bottom half.
Maybe it is not necessary to split st_sensors_check_device_support in
two separate functions. Do you agree?
Regards,
Lorenzo
>> + },
>> + .addr = 0x23,
>> + .val = BIT(0),
>> + },
>> + {
>> + .sensors = {
>> + [0] = LSM330_ACCEL_DEV_NAME,
>> + },
>> + .addr = 0x24,
>> + .val = BIT(0),
>> + },
>> + {
>> + .sensors = {
>> + [0] = LIS3L02DQ_ACCEL_DEV_NAME,
>> + [1] = LIS3LV02DL_ACCEL_DEV_NAME,
>> + },
>> + .addr = 0x21,
>> + .val = BIT(1),
>> + },
>> +};
>> +
>> static int st_accel_read_raw(struct iio_dev *indio_dev,
>> struct iio_chan_spec const *ch, int *val,
>> int *val2, long mask)
>> @@ -719,6 +751,11 @@ int st_accel_common_probe(struct iio_dev *indio_dev)
>> indio_dev->info = &accel_info;
>> mutex_init(&adata->tb.buf_lock);
>>
>> + err = st_sensors_init_interface_mode(indio_dev, st_accel_sim_table,
>> + ARRAY_SIZE(st_accel_sim_table));
>> + if (err < 0)
>> + return err;
>> +
>> err = st_sensors_power_enable(indio_dev);
>> if (err)
>> return err;
>> diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
>> index 274868100fd0..50c281b6286d 100644
>> --- a/drivers/iio/common/st_sensors/st_sensors_core.c
>> +++ b/drivers/iio/common/st_sensors/st_sensors_core.c
>> @@ -384,6 +384,42 @@ static struct st_sensors_platform_data *st_sensors_of_probe(struct device *dev,
>> }
>> #endif
>>
>> +int st_sensors_init_interface_mode(struct iio_dev *indio_dev,
>> + const struct st_sensor_sim *sim_table,
>> + int sim_len)
>> +{
>> + struct st_sensor_data *sdata = iio_priv(indio_dev);
>> + struct device_node *np = sdata->dev->of_node;
>> + struct st_sensors_platform_data *pdata;
>> + int err = 0;
>> +
>> + pdata = (struct st_sensors_platform_data *)sdata->dev->platform_data;
>> + if ((np && of_property_read_bool(np, "spi-3wire")) ||
>> + (pdata && pdata->spi_3wire)) {
>> + int i, j;
>> +
>> + for (i = 0; i < sim_len; i++) {
>> + for (j = 0; j < ST_SENSORS_MAX_SIM; j++) {
>> + if (!strcmp(sim_table[i].sensors[j],
>> + indio_dev->name))
>> + break;
>> + }
>> + if (j < ST_SENSORS_MAX_SIM)
>> + break;
>> + }
>> +
>> + if (i == sim_len)
>> + return -EINVAL;
>> +
>> + err = sdata->tf->write_byte(&sdata->tb, sdata->dev,
>> + sim_table[i].addr,
>> + sim_table[i].val);
>> + }
>> +
>> + return err;
>> +}
>> +EXPORT_SYMBOL(st_sensors_init_interface_mode);
>> +
>> int st_sensors_init_sensor(struct iio_dev *indio_dev,
>> struct st_sensors_platform_data *pdata)
>> {
>> diff --git a/include/linux/iio/common/st_sensors.h b/include/linux/iio/common/st_sensors.h
>> index 1f8211b6438b..15ce0cb360d7 100644
>> --- a/include/linux/iio/common/st_sensors.h
>> +++ b/include/linux/iio/common/st_sensors.h
>> @@ -41,6 +41,7 @@
>>
>> #define ST_SENSORS_MAX_NAME 17
>> #define ST_SENSORS_MAX_4WAI 7
>> +#define ST_SENSORS_MAX_SIM 9
>>
>> #define ST_SENSORS_LSM_CHANNELS(device_type, mask, index, mod, \
>> ch2, s, endian, rbits, sbits, addr) \
>> @@ -105,6 +106,12 @@ struct st_sensor_fullscale {
>> struct st_sensor_fullscale_avl fs_avl[ST_SENSORS_FULLSCALE_AVL_MAX];
>> };
>>
>> +struct st_sensor_sim {
>> + char sensors[ST_SENSORS_MAX_SIM][ST_SENSORS_MAX_NAME];
>> + u8 addr;
>> + u8 val;
>> +};
>> +
>> /**
>> * struct st_sensor_bdu - ST sensor device block data update
>> * @addr: address of the register.
>> @@ -325,6 +332,10 @@ ssize_t st_sensors_sysfs_sampling_frequency_avail(struct device *dev,
>> ssize_t st_sensors_sysfs_scale_avail(struct device *dev,
>> struct device_attribute *attr, char *buf);
>>
>> +int st_sensors_init_interface_mode(struct iio_dev *indio_dev,
>> + const struct st_sensor_sim *sim_table,
>> + int sim_len);
>> +
>> #ifdef CONFIG_OF
>> void st_sensors_of_name_probe(struct device *dev,
>> const struct of_device_id *match,
>> diff --git a/include/linux/platform_data/st_sensors_pdata.h b/include/linux/platform_data/st_sensors_pdata.h
>> index 79b0e4cdb814..f8274b0c6888 100644
>> --- a/include/linux/platform_data/st_sensors_pdata.h
>> +++ b/include/linux/platform_data/st_sensors_pdata.h
>> @@ -17,10 +17,12 @@
>> * Available only for accelerometer and pressure sensors.
>> * Accelerometer DRDY on LSM330 available only on pin 1 (see datasheet).
>> * @open_drain: set the interrupt line to be open drain if possible.
>> + * @spi_3wire: enable spi-3wire mode.
>> */
>> struct st_sensors_platform_data {
>> u8 drdy_int_pin;
>> bool open_drain;
>> + bool spi_3wire;
>> };
>>
>> #endif /* ST_SENSORS_PDATA_H */
>
--
UNIX is Sexy: who | grep -i blonde | talk; cd ~; wine; talk; touch;
unzip; touch; strip; gasp; finger; gasp; mount; fsck; more; yes; gasp;
umount; make clean; sleep
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] iio: accel: st_accel: add SPI-3wire support
2017-07-05 8:27 ` Lorenzo Bianconi
@ 2017-07-05 8:41 ` Jonathan Cameron
0 siblings, 0 replies; 4+ messages in thread
From: Jonathan Cameron @ 2017-07-05 8:41 UTC (permalink / raw)
To: Lorenzo Bianconi; +Cc: Jonathan Cameron, linux-iio, Lorenzo BIANCONI
On Wed, 5 Jul 2017 10:27:17 +0200
Lorenzo Bianconi <lorenzo.bianconi83@gmail.com> wrote:
> > On Mon, 3 Jul 2017 20:07:11 +0200
> > Lorenzo Bianconi <lorenzo.bianconi83@gmail.com> wrote:
> >
> >> Add sensor interface mode (SIM) lookup table to support devices
> >> (like LSM303AGR accel sensor) that support just SPI-3wire
> >> communication mode. SIM mode has to be configured before any
> >> other operation since it is not enabled by default and the driver
> >> is not able to read without that configuration
> >>
> >> Fixes: ddc05fa28606 (iio: st-accel: add support for lsm303agr
> >> accel) Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@st.com>
> > Just checked and the 3-wire device tree property is documented in
> > spi-bus.txt
> >
> > So, that's all fine. However, I'm not keen on introducing a
> > special look up table for this when we already have a convenient
> > one covering all the other per device registers etc.
>
> Hi Jonathan,
>
> thanks for the review
>
> >
> > Another approach suggested inline...
> >> ---
> >> drivers/iio/accel/st_accel_core.c | 37
> >> +++++++++++++++++++++++++
> >> drivers/iio/common/st_sensors/st_sensors_core.c | 36
> >> ++++++++++++++++++++++++
> >> include/linux/iio/common/st_sensors.h | 11 ++++++++
> >> include/linux/platform_data/st_sensors_pdata.h | 2 ++ 4 files
> >> changed, 86 insertions(+)
> >>
> >> diff --git a/drivers/iio/accel/st_accel_core.c
> >> b/drivers/iio/accel/st_accel_core.c index
> >> 07d1489cd457..195966e82516 100644 ---
> >> a/drivers/iio/accel/st_accel_core.c +++
> >> b/drivers/iio/accel/st_accel_core.c @@ -619,6 +619,38 @@ static
> >> const struct st_sensor_settings st_accel_sensors_settings[] = { },
> >> };
> >>
> >> +static const struct st_sensor_sim st_accel_sim_table[] = {
> >> + {
> >> + .sensors = {
> >> + [0] = LSM303AGR_ACCEL_DEV_NAME,
> >> + [1] = LIS3DH_ACCEL_DEV_NAME,
> >> + [2] = LIS2DH12_ACCEL_DEV_NAME,
> >> + [3] = LIS331DLH_ACCEL_DEV_NAME,
> >> + [4] = LNG2DM_ACCEL_DEV_NAME,
> >> + [5] = LSM330D_ACCEL_DEV_NAME,
> >> + [6] = LSM330DL_ACCEL_DEV_NAME,
> >> + [7] = LSM330DLC_ACCEL_DEV_NAME,
> > The index has no meaning as we really have an unordered
> > list so perhaps drop it and let it automatically place them.
>
> Ack
>
> >
> > Doing it by name is particularly ugly given we already
> > have a big look up table for device differences.
> >
> > One option would be to split the st_sensors_check_device_support
> > function into two parts - the first does the match by name
> > and the second does the wai check. That way you could
> > then stick your configuration in between the two and
> > put this info in with all the other device specific
> > characteristics.
> >
> > To make the transition easy you could add the two split
> > functions and call them from the original. Then you can
> > introduce the split as needed into the various st sensor
> > drivers.
>
> Fine. I guess it is enough to add a static version of
> st_sensors_init_interface_mode
> with a reference to the right entry in st_sensor_settings from the top
> half of st_sensors_check_device_support.
> If the everything is ok with SIM configuration we will go forward to
> wai check in st_sensors_check_device_support bottom half.
> Maybe it is not necessary to split st_sensors_check_device_support in
> two separate functions. Do you agree?
As long as the logic ends up as
1) Look up table entry to get the address to configure 3 wire if
necessary.
2) Configure
3) Check WAI
Then I'm fine with any code arrangement that you want to use.
Jonathan
>
> Regards,
> Lorenzo
>
> >> + },
> >> + .addr = 0x23,
> >> + .val = BIT(0),
> >> + },
> >> + {
> >> + .sensors = {
> >> + [0] = LSM330_ACCEL_DEV_NAME,
> >> + },
> >> + .addr = 0x24,
> >> + .val = BIT(0),
> >> + },
> >> + {
> >> + .sensors = {
> >> + [0] = LIS3L02DQ_ACCEL_DEV_NAME,
> >> + [1] = LIS3LV02DL_ACCEL_DEV_NAME,
> >> + },
> >> + .addr = 0x21,
> >> + .val = BIT(1),
> >> + },
> >> +};
> >> +
> >> static int st_accel_read_raw(struct iio_dev *indio_dev,
> >> struct iio_chan_spec const *ch, int *val,
> >> int *val2,
> >> long mask) @@ -719,6 +751,11 @@ int st_accel_common_probe(struct
> >> iio_dev *indio_dev) indio_dev->info = &accel_info;
> >> mutex_init(&adata->tb.buf_lock);
> >>
> >> + err = st_sensors_init_interface_mode(indio_dev,
> >> st_accel_sim_table,
> >> +
> >> ARRAY_SIZE(st_accel_sim_table));
> >> + if (err < 0)
> >> + return err;
> >> +
> >> err = st_sensors_power_enable(indio_dev);
> >> if (err)
> >> return err;
> >> diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c
> >> b/drivers/iio/common/st_sensors/st_sensors_core.c index
> >> 274868100fd0..50c281b6286d 100644 ---
> >> a/drivers/iio/common/st_sensors/st_sensors_core.c +++
> >> b/drivers/iio/common/st_sensors/st_sensors_core.c @@ -384,6
> >> +384,42 @@ static struct st_sensors_platform_data
> >> *st_sensors_of_probe(struct device *dev, } #endif
> >>
> >> +int st_sensors_init_interface_mode(struct iio_dev *indio_dev,
> >> + const struct st_sensor_sim
> >> *sim_table,
> >> + int sim_len)
> >> +{
> >> + struct st_sensor_data *sdata = iio_priv(indio_dev);
> >> + struct device_node *np = sdata->dev->of_node;
> >> + struct st_sensors_platform_data *pdata;
> >> + int err = 0;
> >> +
> >> + pdata = (struct st_sensors_platform_data
> >> *)sdata->dev->platform_data;
> >> + if ((np && of_property_read_bool(np, "spi-3wire")) ||
> >> + (pdata && pdata->spi_3wire)) {
> >> + int i, j;
> >> +
> >> + for (i = 0; i < sim_len; i++) {
> >> + for (j = 0; j < ST_SENSORS_MAX_SIM; j++) {
> >> + if (!strcmp(sim_table[i].sensors[j],
> >> + indio_dev->name))
> >> + break;
> >> + }
> >> + if (j < ST_SENSORS_MAX_SIM)
> >> + break;
> >> + }
> >> +
> >> + if (i == sim_len)
> >> + return -EINVAL;
> >> +
> >> + err = sdata->tf->write_byte(&sdata->tb, sdata->dev,
> >> + sim_table[i].addr,
> >> + sim_table[i].val);
> >> + }
> >> +
> >> + return err;
> >> +}
> >> +EXPORT_SYMBOL(st_sensors_init_interface_mode);
> >> +
> >> int st_sensors_init_sensor(struct iio_dev *indio_dev,
> >> struct
> >> st_sensors_platform_data *pdata) {
> >> diff --git a/include/linux/iio/common/st_sensors.h
> >> b/include/linux/iio/common/st_sensors.h index
> >> 1f8211b6438b..15ce0cb360d7 100644 ---
> >> a/include/linux/iio/common/st_sensors.h +++
> >> b/include/linux/iio/common/st_sensors.h @@ -41,6 +41,7 @@
> >>
> >> #define ST_SENSORS_MAX_NAME 17
> >> #define ST_SENSORS_MAX_4WAI 7
> >> +#define ST_SENSORS_MAX_SIM 9
> >>
> >> #define ST_SENSORS_LSM_CHANNELS(device_type, mask, index, mod, \
> >> ch2, s, endian, rbits,
> >> sbits, addr) \ @@ -105,6 +106,12 @@ struct st_sensor_fullscale {
> >> struct st_sensor_fullscale_avl
> >> fs_avl[ST_SENSORS_FULLSCALE_AVL_MAX]; };
> >>
> >> +struct st_sensor_sim {
> >> + char sensors[ST_SENSORS_MAX_SIM][ST_SENSORS_MAX_NAME];
> >> + u8 addr;
> >> + u8 val;
> >> +};
> >> +
> >> /**
> >> * struct st_sensor_bdu - ST sensor device block data update
> >> * @addr: address of the register.
> >> @@ -325,6 +332,10 @@ ssize_t
> >> st_sensors_sysfs_sampling_frequency_avail(struct device *dev,
> >> ssize_t st_sensors_sysfs_scale_avail(struct device *dev, struct
> >> device_attribute *attr, char *buf);
> >>
> >> +int st_sensors_init_interface_mode(struct iio_dev *indio_dev,
> >> + const struct st_sensor_sim
> >> *sim_table,
> >> + int sim_len);
> >> +
> >> #ifdef CONFIG_OF
> >> void st_sensors_of_name_probe(struct device *dev,
> >> const struct of_device_id *match,
> >> diff --git a/include/linux/platform_data/st_sensors_pdata.h
> >> b/include/linux/platform_data/st_sensors_pdata.h index
> >> 79b0e4cdb814..f8274b0c6888 100644 ---
> >> a/include/linux/platform_data/st_sensors_pdata.h +++
> >> b/include/linux/platform_data/st_sensors_pdata.h @@ -17,10 +17,12
> >> @@
> >> * Available only for accelerometer and pressure sensors.
> >> * Accelerometer DRDY on LSM330 available only on pin 1 (see
> >> datasheet).
> >> * @open_drain: set the interrupt line to be open drain if
> >> possible.
> >> + * @spi_3wire: enable spi-3wire mode.
> >> */
> >> struct st_sensors_platform_data {
> >> u8 drdy_int_pin;
> >> bool open_drain;
> >> + bool spi_3wire;
> >> };
> >>
> >> #endif /* ST_SENSORS_PDATA_H */
> >
>
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-07-05 8:42 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-07-03 18:07 [PATCH] iio: accel: st_accel: add SPI-3wire support Lorenzo Bianconi
2017-07-04 19:26 ` Jonathan Cameron
2017-07-05 8:27 ` Lorenzo Bianconi
2017-07-05 8:41 ` Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox