On Wed, 5 Jul 2017 10:27:17 +0200
Lorenzo Bianconi <lorenzo.bianconi83@xxxxxxxxx> wrote:
> > On Mon, 3 Jul 2017 20:07:11 +0200
> > Lorenzo Bianconi <lorenzo.bianconi83@xxxxxxxxx> 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@xxxxxx>
> > 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 */
> >
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
