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

[Jonathan Cameron <jic23@kernel.org>]

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