Re: [PATCH V1 5/6] iio: accel: sca3300: Add inclination channels.

Re: [PATCH V1 5/6] iio: accel: sca3300: Add inclination channels.

[Jonathan Cameron]

On Mon, 24 Jan 2022 15:19:09 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Mon, Jan 24, 2022 at 11:39 AM LI Qingwu
> <Qing-wu.Li@leica-geosystems.com.cn> wrote:
> >
> > Different with SCA3300, SCL3300 can output inclination angles.
> > Angles are formed from acceleration with following equations:
> > ANG_X = atan2(accx / √(accy^2 + accz^2)),
> > ANG_Y = atan2(accy / √(accx^2 + accz^2)),
> > ANG_Z = atan2(accz / √(accx^2 + accy^2)),
> >
> > The commit add output of the raw value,scale
> > and scale_available of angles.
> > add interface for enable/disable of the angle output.
> >
> > new interfaces:  
> 
> New
> 
> > in_incli_en

Why?  There are only a few reasons to have an enable for a
channel and they don't include something that we might only
sometimes read (tend to be temporal channels such as step
counters where we want to be able to pause their counting,
or output channels).

> > in_incli_scale
> > in_incli_scale_available
> > in_incli_x_raw
> > in_incli_y_raw
> > in_incli_z_raw  
> 
> Indent them by 2 spaces.
> 
> Wondering if these need to be described in ABI documentation.
It's standard ABI, though we don't give much description of what
exactly these ease. It might be possible to add more information
to the generic docs, but that would require looking very carefully
at the current supporting devices.

"Inclination raw reading about axis x, y or z (may be
Arbitrarily assigned). Data converted by application of offset
and scale to degrees."

> 
> ...
> 
> >         SCA3300_ACCEL_CHANNEL(SCA3300_ACC_Y, 0x2, Y),
> >         SCA3300_ACCEL_CHANNEL(SCA3300_ACC_Z, 0x3, Z),
> >         SCA3300_TEMP_CHANNEL(SCA3300_TEMP, 0x05),
> > -       IIO_CHAN_SOFT_TIMESTAMP(4)  
> 
> > +       IIO_CHAN_SOFT_TIMESTAMP(SCA3300_TIMESTAMP)  
> 
> + Comma (while at it)?
> 
> ...
> 
> > -       IIO_CHAN_SOFT_TIMESTAMP(4),
> > +       SCA3300_INCLI_CHANNEL(SCA3300_INCLI_X, 0x09, X),
> > +       SCA3300_INCLI_CHANNEL(SCA3300_INCLI_Y, 0x0A, Y),
> > +       SCA3300_INCLI_CHANNEL(SCA3300_INCLI_Z, 0x0B, Z),
> > +       IIO_CHAN_SOFT_TIMESTAMP(SCA3300_TIMESTAMP)  
> 
> Ditto.
> 
> > +static const int sca3300_incli_scale[CHIP_CNT][OP_MOD_CNT][2] = {
> > +       [CHIP_SCA3300] = {{0, 0}, {0, 0}, {0, 0}, {0, 0}},  
> 
> > +       [CHIP_SCL3300] = {{0, 5495}, {0, 5495}, {0, 5495}, {0, 5495}}  
> 
> + Comma.
> 
> > +};  
> 
> ...
> 
> >         struct {
> > -               s16 channels[4];
> > +               s16 channels[SCA3300_TIMESTAMP-1];  
> 
> Missed spaces around the arithmetic operator.
> 
> >                 s64 ts __aligned(sizeof(s64));
> >         } scan;
> >         const struct sca3300_chip_info *chip_info;
> >         u8 txbuf[4] ____cacheline_aligned;
> >         u8 rxbuf[4];  
> 
> > -  
> 
> Stray change.
> 
> >  };  
> 
> ...
> 
> > +               /*Inclination scale info tied to accel scale.*/
> > +               /*not allowed to set separately.      */  
> 
> Please, follow the proper style for multi-line comments, including
> necessary spaces, periods, starting and ending lines.
> 
> ...
> 
> > +       case IIO_CHAN_INFO_ENABLE:
> > +               if (data->chip_info->chip_type == CHIP_SCL3300) {  
> 
> > +                       if (chan->type == IIO_INCLI) {  
> 
> See below.
> 
> > +                               if (val != 0)  
> 
>    if (val)
> 
> > +                                       reg_val = 0x1F;
> > +                               else
> > +                                       reg_val = 0x00;
> > +                               return sca3300_write_reg(data, SCA3300_REG_ANG_CTRL, reg_val);
> > +                       }
> > +               }  
> 
> ...
> 
> > -               if (chan->type == IIO_ACCEL) {
> > +
> > +               if (chan->type == IIO_INCLI) {  
> 
> > +               } else if (chan->type == IIO_ACCEL) {  
> 
> I would recommend using switch-case for channel type as well.
> 
> ...
> 
> > +       case IIO_CHAN_INFO_ENABLE:
> > +               if (chan->type == IIO_INCLI) {  
> 
> > +                       ret = sca3300_read_reg(data, SCA3300_REG_ANG_CTRL, &reg_val);  
> 
> How is ret supposed to be used?
> 
> > +                       *val = reg_val;
> > +                       return IIO_VAL_INT;
> > +               }
> > +               return -EINVAL;  
> 

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

[Jonathan Cameron]

On Thu, 10 Feb 2022 10:08:53 +0000
LI Qingwu <qing-wu.li@leica-geosystems.com.cn> wrote:

> 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@kernel.org>
> > Sent: Sunday, January 30, 2022 7:40 PM
> > To: LI Qingwu <qing-wu.li@leica-geosystems.com.cn>
> > Cc: lars@metafoo.de; robh+dt@kernel.org; tomas.melin@vaisala.com;
> > andy.shevchenko@gmail.com; devicetree@vger.kernel.org;
> > linux-kernel@vger.kernel.org
> > 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@leica-geosystems.com.cn> 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@leica-geosystems.com.cn>  
> > 
> > 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  
> 

+cc linux-iio@vger.kernel.org

> 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.

Why would a user ever select the 'high noise' option?
My guess is power saving?  Probably not enough to be relevant
in a system running Linux.

I would suggest just not supporting that option.
It is not uncommon for some modes to make limited sense and
to just be there as an artefact of the underlying hardware
architecture.  Doesn't mean we have to support them :)

Jonathan





> 
> | 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,  
>