RE: [PATCH V1 2/6] iio: accel: sca3300: Add interface for operation modes.

[LI Qingwu]

Thanks a lot all of your inputs, I'm just back from long holiday and start to rework on the patches.

> From: Jonathan Cameron <jic23@xxxxxxxxxx>
> Sent: Sunday, January 30, 2022 7:40 PM
> To: LI Qingwu <qing-wu.li@xxxxxxxxxxxxxxxxxxxxxxx>
> Cc: lars@xxxxxxxxxx; robh+dt@xxxxxxxxxx; tomas.melin@xxxxxxxxxxx;
> andy.shevchenko@xxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH V1 2/6] iio: accel: sca3300: Add interface for operation
> modes.
> 
> This email is not from Hexagon’s Office 365 instance. Please be careful while
> clicking links, opening attachments, or replying to this email.
> 
> 
> On Mon, 24 Jan 2022 09:39:08 +0000
> LI Qingwu <Qing-wu.Li@xxxxxxxxxxxxxxxxxxxxxxx> wrote:
> 
> > The acceleration scale and the frequency were set via operation modes,
> > the scal and frequency are both non-uniqueness, this leads to logic
> > confusion for setting scale.and.frequency.
> > it getting worse if add more different sensor types into the driver.
> >
> > The commit add an interface for set and get the operation modes.
> > the following interfaces added:
> > in_accel_op_mode_available
> > in_op_mode
> >
> > SCA3300 operation modes table:
> > | Mode | Full-scale | low pass filter frequency |
> > | ---- | ---------- | ------------------------- |
> > | 1    | ± 3 g      | 70 Hz                     |
> > | 2    | ± 6 g      | 70 Hz                     |
> > | 3    | ± 1.5 g    | 70 Hz                     |
> > | 4    | ± 1.5 g    | 10 Hz                     |
> >
> > Signed-off-by: LI Qingwu <Qing-wu.Li@xxxxxxxxxxxxxxxxxxxxxxx>
> 
> While it may seem convenient to expose this to userspace, the reality is that
> generic userspace has no way to know how to use it.
> 
> That makes supplying this control a bad idea however convenient it may seem.
> It's not unusual to have these sorts of constraints on devices and so the ABI
> always assumes any setting may modify any other and / or change what is
> available for a given setting.
> 
> If you need a particular combination for your own userspace, then make the
> userspace aware of the constraints rather than exposing it as a 'mode' which
> the userspace will need to know about anyway.
> 
> Jonathan

Thanks a lot Jonathan, I couldn't agree with you more, the mode is not good for userspace,
I would like to ask you how to handle this.
Since the change for 'mode' was a prepare for support SCL3300,
For SCL3300, mode 3 and mode 4 are totally same for both scale and frequency.
The only different is mode 4 is low noise mode, but no difference from software point of view.
Then it's impossible to set to between mode 3/4, let's say normal noise and low noise mode, with index of frequency and scale.
Set between mode 3 and 4 is necessary, I have no idea how to handle it.

| Mode             | Full-scale   | frequency |
| -------------------      | ----------------- | ------------- |
| 1                 | ± 1.2 g     | 40 Hz    |
| 2                 | ± 2.4 g     | 70 Hz    |
| 3                 | ± 0.6 g     | 10 Hz    |
| 4 (Low noise mode)  | ± 0.6 g     | 10 Hz    |   
               
The link of the SCL3300 datasheet:
https://www.murata.com/-/media/webrenewal/products/sensor/pdf/datasheet/datasheet_scl3300-d01.ashx?la=en&cvid=20210316063715000000

> 
> 
> > ---
> >  drivers/iio/accel/sca3300.c | 55
> > +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 55 insertions(+)
> >
> > diff --git a/drivers/iio/accel/sca3300.c b/drivers/iio/accel/sca3300.c
> > index 083ae2a47ad9..e26b3175b3c6 100644
> > --- a/drivers/iio/accel/sca3300.c
> > +++ b/drivers/iio/accel/sca3300.c
> > @@ -42,6 +42,38 @@
> >  /* Device return status and mask */
> >  #define SCA3300_VALUE_RS_ERROR       0x3
> >  #define SCA3300_MASK_RS_STATUS       GENMASK(1, 0)
> > +enum sca3300_op_mode_indexes {
> > +     OP_MOD_1 = 0,
> > +     OP_MOD_2,
> > +     OP_MOD_3,
> > +     OP_MOD_4,
> > +     OP_MOD_CNT
> > +};
> > +
> > +static const char * const sca3300_op_modes[] = {
> > +     [OP_MOD_1] = "1",
> > +     [OP_MOD_2] = "2",
> > +     [OP_MOD_3] = "3",
> > +     [OP_MOD_4] = "4"
> > +};
> > +
> > +static int sca3300_get_op_mode(struct iio_dev *indio_dev,
> > +             const struct iio_chan_spec *chan); static int
> > +sca3300_set_op_mode(struct iio_dev *indio_dev,
> > +             const struct iio_chan_spec *chan, unsigned int mode);
> > +
> > +static const struct iio_enum sca3300_op_mode_enum = {
> > +     .items = sca3300_op_modes,
> > +     .num_items = ARRAY_SIZE(sca3300_op_modes),
> > +     .get = sca3300_get_op_mode,
> > +     .set = sca3300_set_op_mode,
> > +};
> > +
> > +static const struct iio_chan_spec_ext_info sca3300_ext_info[] = {
> > +     IIO_ENUM("op_mode", IIO_SHARED_BY_DIR,
> &sca3300_op_mode_enum),
> > +     IIO_ENUM_AVAILABLE("op_mode", &sca3300_op_mode_enum),
> > +     { }
> > +};
> >
> >  enum sca3300_scan_indexes {
> >       SCA3300_ACC_X = 0,
> > @@ -70,6 +102,7 @@ enum sca3300_scan_indexes {
> >               .storagebits = 16,
> \
> >               .endianness = IIO_CPU,
> \
> >       },
> \
> > +     .ext_info = sca3300_ext_info,
> \
> >  }
> >
> >  #define SCA3300_TEMP_CHANNEL(index, reg)
> {                           \
> > @@ -400,6 +433,28 @@ static int sca3300_read_avail(struct iio_dev
> *indio_dev,
> >       }
> >  }
> >
> > +static int sca3300_get_op_mode(struct iio_dev *indio_dev,
> > +             const struct iio_chan_spec *chan) {
> > +     int mode;
> > +     int ret;
> > +     struct sca3300_data *data = iio_priv(indio_dev);
> > +
> > +     ret = sca3300_read_reg(data, SCA3300_REG_MODE, &mode);
> > +     if (ret)
> > +             return ret;
> > +     return mode;
> > +
> > +}
> > +
> > +static int sca3300_set_op_mode(struct iio_dev *indio_dev,
> > +             const struct iio_chan_spec *chan, unsigned int mode) {
> > +     struct sca3300_data *data = iio_priv(indio_dev);
> > +
> > +     return sca3300_write_reg(data, SCA3300_REG_MODE, mode); }
> > +
> >  static const struct iio_info sca3300_info = {
> >       .read_raw = sca3300_read_raw,
> >       .write_raw = sca3300_write_raw,