On Tue, Aug 24, 2021 at 2:38 PM Sean Nyekjaer <sean@xxxxxxxxxx> 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
