Re: [PATCH v2 1/2] iio: accel: fxls8962af: add threshold event handling

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Tue, Aug 24, 2021 at 2:38 PM Sean Nyekjaer <sean@geanix.com> wrote:

...

> Do we have some helper functions to do the 12 bit 2-complement numbers?

Probably not, look around where sign_extend32() is defined. More on this below.

...

> +       return regmap_update_bits(data->regmap, FXLS8962AF_INT_EN,
> +                                mask,
> +                                value);

One line?

...

> +       /*
> +        * Add the same value to the lower-threshold register with a reversed sign
> +        * in 2-complement 12 bit format.
> +        */
> +       data->lower_thres = (~val & GENMASK(11, 0)) + 1;

This looks suspicious.

0 => 0xfff + 1 => 0x1000. Is it what is wanted?
I thought that -val & mask is what you need.

Can you explain more in the comment (maybe with examples) on what is
coming and what is expected?

> +       data->upper_thres = val & GENMASK(10, 0);

...

> +       is_active = fxls8962af_is_active(data);
> +       if (is_active) {
> +               ret = fxls8962af_standby(data);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       ret = regmap_bulk_write(data->regmap, FXLS8962AF_SDCD_LTHS_LSB,
> +                               &data->lower_thres, chan->scan_type.storagebits / 8);
> +       if (ret)
> +               return ret;
> +
> +       ret = regmap_bulk_write(data->regmap, FXLS8962AF_SDCD_UTHS_LSB,
> +                               &data->upper_thres, chan->scan_type.storagebits / 8);
> +       if (ret)
> +               return ret;
> +
> +       if (is_active)
> +               ret = fxls8962af_active(data);

I would rewrite it with a helper

if (..._is_active(...)) {
  ret = ..._standby(...);
  ...
  ret = _set_thresholds(...);
  ...
  ret = _active(...);
} else {
  ret = _set_thresholds(...);
}
return ret;

or something closer to it.

> +       return ret;
> +}

...

> +       int ret;

Useless

> +       if (type != IIO_EV_TYPE_THRESH)
> +               return -EINVAL;
> +
> +       switch (chan->channel2) {
> +       case IIO_MOD_X:
> +               ret = FXLS8962AF_SDCD_CONFIG1_X_OT_EN & data->enable_event;
> +               break;
> +       case IIO_MOD_Y:
> +               ret = FXLS8962AF_SDCD_CONFIG1_Y_OT_EN & data->enable_event;
> +               break;
> +       case IIO_MOD_Z:
> +               ret = FXLS8962AF_SDCD_CONFIG1_Z_OT_EN & data->enable_event;
> +               break;
> +       default:
> +               return -EINVAL;
> +       }

> +       return ret;

Just return directly from the cases.

...

> +       ret = regmap_write(data->regmap, FXLS8962AF_SDCD_CONFIG2, enable_event ? 0xC0 : 0x00);

0xc0

> +       if (ret)
> +               return ret;

....

> +       .mask_separate = BIT(IIO_EV_INFO_VALUE) |
> +                        BIT(IIO_EV_INFO_ENABLE),

One line?

-- 
With Best Regards,
Andy Shevchenko