On Wed, Aug 18, 2021 at 12:29 PM Sean Nyekjaer <sean@xxxxxxxxxx> 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, ®);
> + if (ret | (reg < 0))
This is weird and shadows an actual error.
> + return -EINVAL;
--
With Best Regards,
Andy Shevchenko
