On Sun, 4 Nov 2018 18:47:04 +0100
Lorenzo Bianconi <lorenzo.bianconi@xxxxxxxxxx> wrote:
> >
> > On Sun, 4 Nov 2018 15:39:03 +0100
> > Lorenzo Bianconi <lorenzo.bianconi@xxxxxxxxxx> wrote:
> >
> > > Add ST_LSM6DSX_ID_EXT{0,1,2} sensor ids as reference for slave devices
> > > connected to st_lsm6dsx i2c controller. Moreover introduce odr dependency
> > > between accel and ext devices since accelerometer is used as trigger for
> > > i2c master controller read/write operations
> > >
> > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@xxxxxxxxxx>
> >
> > Hi Lorenzo,
> >
> > There looks to be an unrelated cleanup in here around set_enable so
> > please pull that out as a separate patch.
> >
> > Also, it seems the odr dependency is not as simple as they should be
> > the same? Perhaps you could add more here on what that dependency is?
> >
>
> I added the odr change in this patch since external sensors need to
> use accelerometer
> device as trigger for i2c operations, in other words the accelerometer
> sensor needs to
> be powered up in order to enable i2c master (I was thinking to a
> logical dependency here :)).
> Anyway I am fine to add just ext_id definitions in this patch and add
> odr dependency in a
> separate patch.
I was mostly just looking for some more explanation of the contraint
in the description rather than any splitting of the ODR change
and the ext_id part.
(the enable stuff does want to be a separate patch however!)
> Just one comment inline.
>
> Regards,
> Lorenzo
>
>
> > Thanks,
> >
> > Jonathan
> >
> > > ---
> > > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h | 9 +-
> > > .../iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 27 +++--
> > > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 101 ++++++++++++------
> > > 3 files changed, 95 insertions(+), 42 deletions(-)
> > >
> > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> > > index ac4cbbb0b3fb..2beb4f563892 100644
> > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> > > @@ -105,8 +105,11 @@ struct st_lsm6dsx_settings {
> > > };
> > >
> > > enum st_lsm6dsx_sensor_id {
> > > - ST_LSM6DSX_ID_ACC,
> > > ST_LSM6DSX_ID_GYRO,
> > > + ST_LSM6DSX_ID_ACC,
> > > + ST_LSM6DSX_ID_EXT0,
> > > + ST_LSM6DSX_ID_EXT1,
> > > + ST_LSM6DSX_ID_EXT2,
> > > ST_LSM6DSX_ID_MAX,
> > > };
> > >
> > > @@ -182,8 +185,8 @@ extern const struct dev_pm_ops st_lsm6dsx_pm_ops;
> > >
> > > int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
> > > struct regmap *regmap);
> > > -int st_lsm6dsx_sensor_enable(struct st_lsm6dsx_sensor *sensor);
> > > -int st_lsm6dsx_sensor_disable(struct st_lsm6dsx_sensor *sensor);
> > > +int st_lsm6dsx_sensor_set_enable(struct st_lsm6dsx_sensor *sensor,
> > > + bool enable);
> > Unrelated change?
> >
> > > int st_lsm6dsx_fifo_setup(struct st_lsm6dsx_hw *hw);
> > > int st_lsm6dsx_set_watermark(struct iio_dev *iio_dev, unsigned int val);
> > > int st_lsm6dsx_update_watermark(struct st_lsm6dsx_sensor *sensor,
> > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > > index 4b3ba0956b5a..96f7d56d3b6d 100644
> > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > > @@ -102,6 +102,9 @@ static void st_lsm6dsx_get_max_min_odr(struct st_lsm6dsx_hw *hw,
> > >
> > > *max_odr = 0, *min_odr = ~0;
> > > for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
> > > + if (!hw->iio_devs[i])
> > > + continue;
> > > +
> > > sensor = iio_priv(hw->iio_devs[i]);
> > >
> > > if (!(hw->enable_mask & BIT(sensor->id)))
> > > @@ -125,6 +128,9 @@ static int st_lsm6dsx_update_decimators(struct st_lsm6dsx_hw *hw)
> > > for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
> > > const struct st_lsm6dsx_reg *dec_reg;
> > >
> > > + if (!hw->iio_devs[i])
> > > + continue;
> > > +
> > > sensor = iio_priv(hw->iio_devs[i]);
> > > /* update fifo decimators and sample in pattern */
> > > if (hw->enable_mask & BIT(sensor->id)) {
> > > @@ -232,6 +238,9 @@ int st_lsm6dsx_update_watermark(struct st_lsm6dsx_sensor *sensor, u16 watermark)
> > > return 0;
> > >
> > > for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
> > > + if (!hw->iio_devs[i])
> > > + continue;
> > > +
> > > cur_sensor = iio_priv(hw->iio_devs[i]);
> > >
> > > if (!(hw->enable_mask & BIT(cur_sensor->id)))
> > > @@ -278,6 +287,9 @@ static int st_lsm6dsx_reset_hw_ts(struct st_lsm6dsx_hw *hw)
> > > return err;
> > >
> > > for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
> > > + if (!hw->iio_devs[i])
> > > + continue;
> > > +
> > > sensor = iio_priv(hw->iio_devs[i]);
> > > /*
> > > * store enable buffer timestamp as reference for
> > > @@ -562,15 +574,9 @@ static int st_lsm6dsx_update_fifo(struct iio_dev *iio_dev, bool enable)
> > > goto out;
> > > }
> > >
> > > - if (enable) {
> > > - err = st_lsm6dsx_sensor_enable(sensor);
> > > - if (err < 0)
> > > - goto out;
> > > - } else {
> > > - err = st_lsm6dsx_sensor_disable(sensor);
> > > - if (err < 0)
> > > - goto out;
> > > - }
> > > + err = st_lsm6dsx_sensor_set_enable(sensor, enable);
> > > + if (err < 0)
> > > + goto out;
> >
> > Another block of unrelated.
> >
> > >
> > > err = st_lsm6dsx_set_fifo_odr(sensor, enable);
> > > if (err < 0)
> > > @@ -690,6 +696,9 @@ int st_lsm6dsx_fifo_setup(struct st_lsm6dsx_hw *hw)
> > > }
> > >
> > > for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
> > > + if (!hw->iio_devs[i])
> > > + continue;
> > > +
> > > buffer = devm_iio_kfifo_allocate(hw->dev);
> > > if (!buffer)
> > > return -ENOMEM;
> > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > index 3433a5b6bf4d..f2549ddfee20 100644
> > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > @@ -450,7 +450,7 @@ int st_lsm6dsx_check_odr(struct st_lsm6dsx_sensor *sensor, u16 odr, u8 *val)
> > > int i;
> > >
> > > for (i = 0; i < ST_LSM6DSX_ODR_LIST_SIZE; i++)
> > > - if (st_lsm6dsx_odr_table[sensor->id].odr_avl[i].hz == odr)
> > > + if (st_lsm6dsx_odr_table[sensor->id].odr_avl[i].hz >= odr)
> >
> > Not sure why this change from the description...
>
> here st_lsm6dsx_odr_table is for lsm6dso accel sensor, while odr can
> be from external sensors and
> they can differ (e.g. lsm6dso accel device and lis2mdl conncted to i2c
> controller)
ah, I found the bit in the datasheet after sending this which says it is capped
by the lowest of the on chip devices so this makes more sense.
>
> >
> > > break;
> > >
> > > if (i == ST_LSM6DSX_ODR_LIST_SIZE)
> > > @@ -461,50 +461,82 @@ int st_lsm6dsx_check_odr(struct st_lsm6dsx_sensor *sensor, u16 odr, u8 *val)
> > > return 0;
> > > }
> > >
> > > -static int st_lsm6dsx_set_odr(struct st_lsm6dsx_sensor *sensor, u16 odr)
> > > +static u16 st_lsm6dsx_check_odr_dependency(struct st_lsm6dsx_hw *hw, u16 odr,
> > > + enum st_lsm6dsx_sensor_id id)
> > > {
> > > + struct st_lsm6dsx_sensor *ref = iio_priv(hw->iio_devs[id]);
> > > + u16 ret;
> > > +
> > > + if (odr > 0) {
> > > + if (hw->enable_mask & BIT(id))
> > > + ret = max_t(u16, ref->odr, odr);
> > > + else
> > > + ret = odr;
> > > + } else {
> > > + ret = (hw->enable_mask & BIT(id)) ? ref->odr : 0;
> > > + }
> > > + return ret;
> >
> > Cleaner just to return at all the places you set ret?
>
> ack fine, will do in v2
>
> >
> > > +}
> > > +
> > > +static int st_lsm6dsx_set_odr(struct st_lsm6dsx_sensor *sensor, u16 req_odr)
> > > +{
> > > + struct st_lsm6dsx_sensor *ref_sensor = sensor;
> > > struct st_lsm6dsx_hw *hw = sensor->hw;
> > > const struct st_lsm6dsx_reg *reg;
> > > unsigned int data;
> > > + u8 val = 0;
> > > int err;
> > > - u8 val;
> > >
> > > - err = st_lsm6dsx_check_odr(sensor, odr, &val);
> > > - if (err < 0)
> > > - return err;
> > > + switch (sensor->id) {
> > > + case ST_LSM6DSX_ID_EXT0:
> > > + case ST_LSM6DSX_ID_EXT1:
> > > + case ST_LSM6DSX_ID_EXT2:
> > > + case ST_LSM6DSX_ID_ACC: {
> > > + u16 odr;
> > > + int i;
> > > +
> > > + /* use acc as trigger for ext devices */
> > > + ref_sensor = iio_priv(hw->iio_devs[ST_LSM6DSX_ID_ACC]);
> > > + for (i = ST_LSM6DSX_ID_ACC; i < ST_LSM6DSX_ID_MAX; i++) {
> > > + if (!hw->iio_devs[i] || i == sensor->id)
> > > + continue;
> > > + odr = st_lsm6dsx_check_odr_dependency(hw, req_odr, i);
> > > + if (odr != req_odr)
> > > + /* device already configured */
> > > + return 0;
> > > + }
> > > + break;
> > > + }
> > > + default:
> > > + break;
> > > + }
> > > +
> > > + if (req_odr > 0) {
> > > + err = st_lsm6dsx_check_odr(ref_sensor, req_odr, &val);
> > > + if (err < 0)
> > > + return err;
> > > + }
> > >
> > > - reg = &st_lsm6dsx_odr_table[sensor->id].reg;
> > > + reg = &st_lsm6dsx_odr_table[ref_sensor->id].reg;
> > > data = ST_LSM6DSX_SHIFT_VAL(val, reg->mask);
> > > return st_lsm6dsx_update_bits_locked(hw, reg->addr, reg->mask, data);
> > > }
> > >
> > > -int st_lsm6dsx_sensor_enable(struct st_lsm6dsx_sensor *sensor)
> > > -{
> > > - int err;
> > > -
> > > - err = st_lsm6dsx_set_odr(sensor, sensor->odr);
> > > - if (err < 0)
> > > - return err;
> > > -
> > > - sensor->hw->enable_mask |= BIT(sensor->id);
> > > -
> > > - return 0;
> > > -}
> > > -
> > > -int st_lsm6dsx_sensor_disable(struct st_lsm6dsx_sensor *sensor)
> > > +int st_lsm6dsx_sensor_set_enable(struct st_lsm6dsx_sensor *sensor,
> > > + bool enable)
> > > {
> > > struct st_lsm6dsx_hw *hw = sensor->hw;
> > > - const struct st_lsm6dsx_reg *reg;
> > > - unsigned int data;
> > > + u16 odr = enable ? sensor->odr : 0;
> > > int err;
> > >
> > > - reg = &st_lsm6dsx_odr_table[sensor->id].reg;
> > > - data = ST_LSM6DSX_SHIFT_VAL(0, reg->mask);
> > > - err = st_lsm6dsx_update_bits_locked(hw, reg->addr, reg->mask, data);
> > > + err = st_lsm6dsx_set_odr(sensor, odr);
> > > if (err < 0)
> > > return err;
> > >
> > > - sensor->hw->enable_mask &= ~BIT(sensor->id);
> > > + if (enable)
> > > + hw->enable_mask |= BIT(sensor->id);
> > > + else
> > > + hw->enable_mask &= ~BIT(sensor->id);
> > >
> > > return 0;
> > > }
> > > @@ -516,7 +548,7 @@ static int st_lsm6dsx_read_oneshot(struct st_lsm6dsx_sensor *sensor,
> > > int err, delay;
> > > __le16 data;
> > >
> > > - err = st_lsm6dsx_sensor_enable(sensor);
> > > + err = st_lsm6dsx_sensor_set_enable(sensor, true);
> > > if (err < 0)
> > > return err;
> > >
> > > @@ -527,7 +559,7 @@ static int st_lsm6dsx_read_oneshot(struct st_lsm6dsx_sensor *sensor,
> > > if (err < 0)
> > > return err;
> > >
> > > - st_lsm6dsx_sensor_disable(sensor);
> > > + st_lsm6dsx_sensor_set_enable(sensor, false);
> > >
> > > *val = (s16)le16_to_cpu(data);
> > >
> > > @@ -892,7 +924,7 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
> > > if (err < 0)
> > > return err;
> > >
> > > - for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
> > > + for (i = 0; i < ST_LSM6DSX_ID_EXT0; i++) {
> > > hw->iio_devs[i] = st_lsm6dsx_alloc_iiodev(hw, i, name);
> > > if (!hw->iio_devs[i])
> > > return -ENOMEM;
> > > @@ -909,6 +941,9 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
> > > }
> > >
> > > for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
> > > + if (!hw->iio_devs[i])
> > > + continue;
> > > +
> > > err = devm_iio_device_register(hw->dev, hw->iio_devs[i]);
> > > if (err)
> > > return err;
> > > @@ -927,6 +962,9 @@ static int __maybe_unused st_lsm6dsx_suspend(struct device *dev)
> > > int i, err = 0;
> > >
> > > for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
> > > + if (!hw->iio_devs[i])
> > > + continue;
> > > +
> > > sensor = iio_priv(hw->iio_devs[i]);
> > > if (!(hw->enable_mask & BIT(sensor->id)))
> > > continue;
> > > @@ -952,6 +990,9 @@ static int __maybe_unused st_lsm6dsx_resume(struct device *dev)
> > > int i, err = 0;
> > >
> > > for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
> > > + if (!hw->iio_devs[i])
> > > + continue;
> > > +
> > > sensor = iio_priv(hw->iio_devs[i]);
> > > if (!(hw->enable_mask & BIT(sensor->id)))
> > > continue;
> >
