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

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

On Wed, Aug 18, 2021 at 12:29 PM Sean Nyekjaer <sean@geanix.com> wrote:
>
> Add event channels that controls the creation of motion events.

'channel' or 'control' decide which one should be plural.

> +static int fxls8962af_event_setup(struct fxls8962af_data *data, int state)
> +{
> +       int ret;
> +
> +       /* Enable wakeup interrupt */
> +       ret = regmap_update_bits(data->regmap, FXLS8962AF_INT_EN,
> +                                FXLS8962AF_INT_EN_SDCD_OT_EN,
> +                                state ? FXLS8962AF_INT_EN_SDCD_OT_EN : 0);
> +
> +       return ret;

Redundant ret. Besides that use simply

int mask = FXLS8962AF_INT_EN_SDCD_OT_EN;
int value = state ? mask : 0;

return regmap(..., mask, value);

> +}

...

> +       ret = regmap_bulk_read(data->regmap, FXLS8962AF_SDCD_UTHS_LSB,
> +                              &raw_val, (chan->scan_type.storagebits / 8));

Too many parentheses.
Check also the rest of the code for all comments I have given in this email.

> +       if (ret)
> +               return ret;

...

> +       if (val < 0 || val > 2047)
> +               return -EINVAL;

Can be performed as (val >> 11), but the above is more explicit I think.

...

> +       /* Add the same value to the lower-threshold register with a reversed sign */
> +       data->lower_thres = (-val & 0x80000000) >> 20 | (val & 0x7FF);
> +       data->upper_thres = (val & 0x80000000) >> 20 | (val & 0x7FF);

Why is it so complicated?

Wouldn't lower = -val & GENMASK(10, 0); work?
The upper, btw, has a dead code.

...

> +       ret = regmap_bulk_write(data->regmap, FXLS8962AF_SDCD_LTHS_LSB,
> +                               &data->lower_thres, (chan->scan_type.storagebits / 8));

Missed error check.

> +       ret = regmap_bulk_write(data->regmap, FXLS8962AF_SDCD_UTHS_LSB,
> +                               &data->upper_thres, (chan->scan_type.storagebits / 8));

Ditto.

> +       if (is_active)
> +               ret = fxls8962af_active(data);
> +
> +       return ret;

...

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

This is strange.

...

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

Redundant ' > 0'

> +       if (ret)
> +               return ret;

...

> +       if (data->enable_event > 0) {

Ditto.

> +               fxls8962af_active(data);
> +               ret = fxls8962af_power_on(data);
> +       } else {
> +               if (!iio_buffer_enabled(indio_dev))
> +                       ret = fxls8962af_power_off(data);
> +       }

...

> +static const struct iio_event_spec fxls8962af_event = {
> +       .type = IIO_EV_TYPE_THRESH,
> +       .dir = IIO_EV_DIR_EITHER,
> +       .mask_separate = BIT(IIO_EV_INFO_VALUE) |
> +                        BIT(IIO_EV_INFO_ENABLE)

+ comma

> +};

...

> +       if (data->enable_event == 0)

if (!enable_event)

> +               fxls8962af_power_off(data);

...

> +       ret = regmap_read(data->regmap, FXLS8962AF_SDCD_INT_SRC1, &reg);
> +       if (ret | (reg < 0))

This is weird and shadows an actual error.

> +               return -EINVAL;

-- 
With Best Regards,
Andy Shevchenko