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,
