Re: [PATCH] iio: accel: st_accel: add SPI-3wire support

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

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 */  
> >  
> 
> 
>