Re: [PATCH v3 7/7] iio: accel: Add support for Kionix/ROHM KX132-1211 accelerometer

Jonathan Cameron
2023-05-01T14:41:10Z

On Tue, 25 Apr 2023 00:22:27 +0200
Mehdi Djait <mehdi.djait.k@gmail.com> wrote:

> Kionix KX132-1211 is a tri-axis 16-bit accelerometer that can support
> ranges from ±2G to ±16G, digital output through I²C/SPI.
> Add support for basic accelerometer features such as reading acceleration
> via IIO using raw reads, triggered buffer (data-ready), or the WMI IRQ.
>
> Datasheet: https://kionixfs.azureedge.net/en/document/KX132-1211-Technical-Reference-Manual-Rev-5.0.pdf
> Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com>

Two tiny things inline.

> +static int kx132_get_fifo_bytes(struct kx022a_data *data)
> +{
> + struct device *dev = regmap_get_device(data->regmap);
> + __le16 buf_status;
> + int ret, fifo_bytes;
> +
> + ret = regmap_bulk_read(data->regmap, data->chip_info->buf_status1,
> + &buf_status, sizeof(buf_status));
> + if (ret) {
> + dev_err(dev, "Error reading buffer status\n");
> + return ret;
> + }
> +
> + fifo_bytes = le16_to_cpu(buf_status);
> + fifo_bytes &= data->chip_info->buf_smp_lvl_mask;

Slight preference for FIELD_GET() as it saves me checking the mask includes
lowest bits.

> +
> + return fifo_bytes;
> +}
> +
> static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
> bool irq)
> {
> @@ -1036,6 +1157,32 @@ const struct kx022a_chip_info kx022a_chip_info = {
> };
> EXPORT_SYMBOL_NS_GPL(kx022a_chip_info, IIO_KX022A);
>
> +const struct kx022a_chip_info kx132_chip_info = {
> + .name = "kx132-1211",
> + .regmap_config = &kx132_regmap_config,
> + .channels = kx132_channels,
> + .num_channels = ARRAY_SIZE(kx132_channels),
> + .fifo_length = KX132_FIFO_LENGTH,
> + .who = KX132_REG_WHO,
> + .id = KX132_ID,
> + .cntl = KX132_REG_CNTL,
> + .cntl2 = KX132_REG_CNTL2,
> + .odcntl = KX132_REG_ODCNTL,
> + .buf_cntl1 = KX132_REG_BUF_CNTL1,
> + .buf_cntl2 = KX132_REG_BUF_CNTL2,
> + .buf_clear = KX132_REG_BUF_CLEAR,
> + .buf_status1 = KX132_REG_BUF_STATUS_1,
> + .buf_smp_lvl_mask = KX132_MASK_BUF_SMP_LVL,

There are some things in here (typically where the define isn't used
anywhere else) where I think it would be easier to follow if the
value was listed here. Masks and IDs for example.

> + .buf_read = KX132_REG_BUF_READ,
> + .inc1 = KX132_REG_INC1,
> + .inc4 = KX132_REG_INC4,
> + .inc5 = KX132_REG_INC5,
> + .inc6 = KX132_REG_INC6,
> + .xout_l = KX132_REG_XOUT_L,
> + .get_fifo_bytes = kx132_get_fifo_bytes,
> +};
> +EXPORT_SYMBOL_NS_GPL(kx132_chip_info, IIO_KX022A);
> +
> int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info)
> {
> static const char * const regulator_names[] = {"io-vdd", "vdd"};