iio: Support ROHM/Kionix kx022a

KX022A is a 3-axis Accelerometer from ROHM/Kionix. The sensor features include variable ODRs, I2C and SPI control, FIFO/LIFO with watermark IRQ, tap/motion detection, wake-up & back-to-sleep events, four acceleration ranges (2, 4, 8 and 16g) and probably some other cool features.

Add the basic device tree description for the accelerometer. Only basic accelerometer features are considered as of now - new properties may or may not be needed in the future when rest of the features are supported.

Signed-off-by Matti Vaittinen <[email protected]>

KX022A is a 3-axis accelerometer from ROHM/Kionix. The sensor features include variable ODRs, I2C and SPI control, FIFO/LIFO with watermark IRQ, tap/motion detection, wake-up & back-to-sleep events, four acceleration ranges (2, 4, 8 and 16g) and probably some other cool features.

Add support for the basic accelerometer features such as getting the acceleration data via IIO. (raw reads, triggered buffer [data-ready] or using the WMI IRQ).

Important things to be added include the double-tap, motion detection and wake-up as well as the runtime power management.

(quoted diff) +1 -0

+	}

(quoted diff) +1 -0

+	}

drivers/iio/accel/kionix-kx022a.c new file +4 -0

+static int kx022a_turn_on_off_unlocked(struct kx022a_data *data, bool on)
+{
+	int ret;
+

drivers/iio/accel/kionix-kx022a.c new file +3 -0

+	return ret;
+
+}

drivers/iio/accel/kionix-kx022a.c new file +11 -0

+				break;
+		if (n < 0) {
+			ret = -EINVAL;
+			goto unlock_out;
+		}
+		ret = kx022a_turn_off_lock(data);
+		if (ret)
+			break;
+
+		ret = regmap_update_bits(data->regmap,
+					 KX022A_REG_ODCNTL,

drivers/iio/accel/kionix-kx022a.c new file +19 -0

+				break;
+		if (n < 0) {
+			ret = -EINVAL;
+			goto unlock_out;
+		}
+
+		ret = kx022a_turn_off_lock(data);
+		if (ret)
+			break;
+
+		ret = regmap_update_bits(data->regmap, KX022A_REG_CNTL,
+					 KX022A_MASK_GSEL,
+					 n << KX022A_GSEL_SHIFT);
+		kx022a_turn_on_unlock(data);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}

(quoted diff) +2 -0

+	return IIO_VAL_INT;
+}

drivers/iio/accel/kionix-kx022a.c new file +6 -0

+	case IIO_CHAN_INFO_SAMP_FREQ:
+		ret = regmap_read(data->regmap, KX022A_REG_ODCNTL, &regval);
+		if (ret)
+			return ret;
+
+		if ((regval & KX022A_MASK_ODR) >

drivers/iio/accel/kionix-kx022a.c new file +4 -0

+		iio_push_to_buffers_with_timestamp(idev, &data->scan, tstamp);
+
+		tstamp += sample_period;
+	}

(quoted diff) +2 -0

+	if (ret)
+		goto unlock_out;

drivers/iio/accel/kionix-kx022a.c new file +4 -0

+	int ret = IRQ_NONE;
+
+	mutex_lock(&data->mutex);
+

drivers/iio/accel/kionix-kx022a.c new file +5 -0

+	}
+
+	mutex_unlock(&data->mutex);
+
+	return ret;

drivers/iio/accel/kionix-kx022a.c new file +6 -0

+		if (irq <= 0)
+			return dev_err_probe(dev, irq, "No suitable IRQ\n");
+
+		data->inc_reg = KX022A_REG_INC5;
+		data->ien_reg = KX022A_REG_INC6;
+	}

Quoted 11 lines · Matti Vaittinen

KX022A is a 3-axis accelerometer from ROHM/Kionix. The sensor features include variable ODRs, I2C and SPI control, FIFO/LIFO with watermark IRQ, tap/motion detection, wake-up & back-to-sleep events, four acceleration ranges (2, 4, 8 and 16g) and probably some other cool features.

Add support for the basic accelerometer features such as getting the acceleration data via IIO. (raw reads, triggered buffer [data-ready] or using the WMI IRQ).

Important things to be added include the double-tap, motion detection and wake-up as well as the runtime power management.

...

drivers/iio/accel/kionix-kx022a-i2c.c line 22

Quoted 3 lines

drivers/iio/accel/kionix-kx022a-i2c.c new file +3 -0

+	if (!i2c->irq) {
+		dev_err(dev, "No IRQ configured\n");
+		return -EINVAL;

At least

	return dev_err_probe(...);

for know error codes (or when we know that there won't be EPROBE_DEFER), takes less LoCs in the source file.

Review

Quoted 2 lines

(quoted diff) +2 -0

+		dev_err(dev, "No IRQ configured\n");
+		return -EINVAL;

Quoted 2 lines

(quoted diff) +1 -0

+	}

...

Quoted 5 lines

drivers/iio/accel/kionix-kx022a-i2c.c new file +4 -0

+	regmap = devm_regmap_init_i2c(i2c, &kx022a_regmap);
+	if (IS_ERR(regmap)) {
+		dev_err(dev, "Failed to initialize Regmap\n");
+		return PTR_ERR(regmap);

Ditto here and anywhere else for the similar cases.

Quoted 2 lines

(quoted diff) +1 -0

+	}

...

drivers/iio/accel/kionix-kx022a.c line 304

Quoted 10 lines

drivers/iio/accel/kionix-kx022a.c new file +10 -0

+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*vals = (const int *)kx022a_accel_samp_freq_table;
+		*length = ARRAY_SIZE(kx022a_accel_samp_freq_table) * 2;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_SCALE:
+		*vals = (const int *)kx022a_scale_table;
+		*length = ARRAY_SIZE(kx022a_scale_table) * 2;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		return IIO_AVAIL_LIST;

These ' * 2' can be replaced with respective ARRAY_SIZE() of nested element

...

Quoted 4 lines

drivers/iio/accel/kionix-kx022a.c new file +4 -0

+static int kx022a_turn_on_off_unlocked(struct kx022a_data *data, bool on)
+{
+	int ret;
+

drivers/iio/accel/kionix-kx022a.c line 341

Quoted 10 lines

drivers/iio/accel/kionix-kx022a.c new file +10 -0

+	if (on)
+		ret = regmap_set_bits(data->regmap, KX022A_REG_CNTL,
+				      KX022A_MASK_PC1);
+	else
+		ret = regmap_clear_bits(data->regmap, KX022A_REG_CNTL,
+					KX022A_MASK_PC1);
+
+	if (ret)
+		dev_err(data->dev, "Turn %s fail %d\n", (on) ? "ON" : "OFF",
+			ret);

str_on_off() ?

drivers/iio/accel/kionix-kx022a.c line 395

Quoted 7 lines

drivers/iio/accel/kionix-kx022a.c new file +7 -0

+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		n = ARRAY_SIZE(kx022a_accel_samp_freq_table);
+
+		while (n--)
+			if (val == kx022a_accel_samp_freq_table[n][0] &&
+			    kx022a_accel_samp_freq_table[n][1] == val2)

Why not to use the same kind of l and r arguments in == lines? In current form it's a bit harder to see what the catch here.

...

drivers/iio/accel/kionix-kx022a.c line 464

Quoted 12 lines

drivers/iio/accel/kionix-kx022a.c new file +12 -0

+static int kx022a_get_axis(struct kx022a_data *data,
+			   struct iio_chan_spec const *chan,
+			   int *val)
+{
+	int ret;
+
+	ret = regmap_bulk_read(data->regmap, chan->address, &data->buffer,
+			       sizeof(__le16));
+	if (ret)
+		return ret;
+
+	*val = le16_to_cpu(data->buffer[0]);

'p'-variant of the above would look better

	*val = le16_to_cpup(data->buffer);

since it will be the same as above address without any additional arithmetics.

Quoted 2 lines

(quoted diff) +2 -0

+	return IIO_VAL_INT;
+}

...

drivers/iio/accel/kionix-kx022a.c line 597

Quoted 1 line

drivers/iio/accel/kionix-kx022a.c new file +1 -0

+	return regmap_write(data->regmap, KX022A_REG_BUF_CLEAR, 0x0);

Would simple '0' suffice?

...

drivers/iio/accel/kionix-kx022a.c line 691

Quoted 3 lines

drivers/iio/accel/kionix-kx022a.c new file +3 -0

+	for (i = 0; i < count; i++) {
+		int bit;
+		u16 *samples = &buffer[i * 3];

I would put it as

		u16 *samples = &buffer[i * 3];
		int bit;

drivers/iio/accel/kionix-kx022a.c line 695

Quoted 4 lines

drivers/iio/accel/kionix-kx022a.c new file +3 -0

+		for_each_set_bit(bit, idev->active_scan_mask, AXIS_MAX)
+			memcpy(&data->scan.channels[bit], &samples[bit],
+			       sizeof(data->scan.channels[0]));

Why not use bit instead of 0 for the sake of consistency?

Also might be good to have a temporary for channels:

		... *chs = data->scan.channels;

		for_each_set_bit(bit, idev->active_scan_mask, AXIS_MAX)
			memcpy(&chs[bit], &samples[bit], sizeof(chs[bit]));

Quoted 4 lines

drivers/iio/accel/kionix-kx022a.c new file +4 -0

+		iio_push_to_buffers_with_timestamp(idev, &data->scan, tstamp);
+
+		tstamp += sample_period;
+	}

...

Review

Quoted 2 lines

(quoted diff) +2 -0

+	ret = regmap_clear_bits(data->regmap, data->ien_reg,
+				KX022A_MASK_WMI);

I don't see why it's not on a single line. Even if you are a conservative adept of 80.

...

Quoted 4 lines

drivers/iio/accel/kionix-kx022a.c new file +4 -0

+	int ret = IRQ_NONE;
+
+	mutex_lock(&data->mutex);
+

drivers/iio/accel/kionix-kx022a.c line 910

Quoted 10 lines

drivers/iio/accel/kionix-kx022a.c new file +9 -0

+	if (data->trigger_enabled) {
+		iio_trigger_poll_chained(data->trig);
+		ret = IRQ_HANDLED;
+	}
+
+	if (data->state & KX022A_STATE_FIFO) {
 
+		ret = __kx022a_fifo_flush(idev, KX022A_FIFO_LENGTH, true);
+		if (ret > 0)
+			ret = IRQ_HANDLED;

I don't like it. Perhaps

	bool handled = false;
	int ret;

	...
		ret = ...
		if (ret > 0)
			handled = true;
	...

	return IRQ_RETVAL(handled);

Quoted 6 lines

drivers/iio/accel/kionix-kx022a.c new file +5 -0

+	}
+
+	mutex_unlock(&data->mutex);
+
+	return ret;

...

Review

Quoted 2 lines

(quoted diff) +2 -0

+	if (!dev)
+		return -ENODEV;

Do you really need this check?

...

drivers/iio/accel/kionix-kx022a.c line 1045

Quoted 3 lines

drivers/iio/accel/kionix-kx022a.c new file +3 -0

+	fw = dev_fwnode(dev);
+	if (!fw)
+		return -ENODEV;

You may combine these two in one.

	struct fwnode_handle *fwnode;


	fwnode = dev ? dev_fwnode(dev) : NULL;
	if (!fwnode)
		return -ENODEV;

And please, call it fwnode.

...

drivers/iio/accel/kionix-kx022a.c line 1052

Quoted 6 lines

drivers/iio/accel/kionix-kx022a.c new file +6 -0

+	irq = fwnode_irq_get_byname(fw, "INT1");
+	if (irq > 0) {
+		data->inc_reg = KX022A_REG_INC1;
+		data->ien_reg = KX022A_REG_INC4;
+
+		if (fwnode_irq_get_byname(dev_fwnode(dev), "INT2") > 0)

Why not use fwnode again

...

drivers/iio/accel/kionix-kx022a.c line 1107

Quoted 4 lines

drivers/iio/accel/kionix-kx022a.c new file +4 -0

+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "iio_triggered_buffer_setup_ext FAIL %d\n",
+				     ret);

Drop dup ret at the end, dev_err_probe() has been adding it to each message.

...

drivers/iio/accel/kionix-kx022a.c line 1133

Quoted 6 lines

drivers/iio/accel/kionix-kx022a.c new file +6 -0

+	/*
+	 * No need to check for NULL. request_threadedI_irq() defaults to
+	 * dev_name() should the alloc fail.
+	 */
+	name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-kx022a",
+			      dev_name(data->dev));

It's not clear why do you need a suffix here.

Quoted 1 line · Andy Shevchenko

On Thu, Oct 20, 2022 at 02:37:15PM +0300, Matti Vaittinen wrote:

Quoted 9 lines

drivers/iio/accel/kionix-kx022a.c line 464

Quoted 6 lines

drivers/iio/accel/kionix-kx022a.c new file +6 -0

+	ret = regmap_bulk_read(data->regmap, chan->address, &data->buffer,
+			       sizeof(__le16));
+	if (ret)
+		return ret;
+
+	*val = le16_to_cpu(data->buffer[0]);

'p'-variant of the above would look better

Quoted 4 lines

	*val = le16_to_cpup(data->buffer);

since it will be the same as above address without any additional arithmetics.

I guess there is no significant performance difference? To my eye the le16_to_cpu(data->buffer[0]) is much more clear. I see right from the call that we have an array here and use the first member. If there is no obvious technical merit for using le16_to_cpup(data->buffer) over le16_to_cpu(data->buffer[0]), then I do really prefer the latter for clarity.

Quoted 13 lines

drivers/iio/accel/kionix-kx022a.c line 910

Quoted 10 lines

drivers/iio/accel/kionix-kx022a.c new file +9 -0

+	if (data->trigger_enabled) {
+		iio_trigger_poll_chained(data->trig);
+		ret = IRQ_HANDLED;
+	}
+
+	if (data->state & KX022A_STATE_FIFO) {
 
+		ret = __kx022a_fifo_flush(idev, KX022A_FIFO_LENGTH, true);
+		if (ret > 0)
+			ret = IRQ_HANDLED;

I don't like it. Perhaps

Quoted 10 lines

	bool handled = false;
	int ret;

	...
		ret = ...
		if (ret > 0)
			handled = true;
	...

	return IRQ_RETVAL(handled);

I don't see the benefit of adding another variable 'handled'. If I understand correctly, it just introduces one extra 'if' in IRQ thread handling while hiding the return value in IRQ_RETVAL() - macro.

I do like seeing the IRQ_NONE being returned by default and IRQ_HANDLED only when "handlers" are successfully executed. Adding extra variable just obfuscates this (from my eyes) while adding also the additional 'if'.

Quoted 2 lines · Matti Vaittinen

Quoted 1 line · Andy Shevchenko

On Thu, Oct 20, 2022 at 02:37:15PM +0300, Matti Vaittinen wrote:

...

Quoted 14 lines

Quoted 9 lines

drivers/iio/accel/kionix-kx022a.c line 464

Quoted 6 lines

drivers/iio/accel/kionix-kx022a.c new file +6 -0

+	ret = regmap_bulk_read(data->regmap, chan->address, &data->buffer,
+			       sizeof(__le16));
+	if (ret)
+		return ret;
+
+	*val = le16_to_cpu(data->buffer[0]);

'p'-variant of the above would look better

Quoted 4 lines

	*val = le16_to_cpup(data->buffer);

since it will be the same as above address without any additional arithmetics.

Quoted 6 lines

I guess there is no significant performance difference? To my eye the le16_to_cpu(data->buffer[0]) is much more clear. I see right from the call that we have an array here and use the first member. If there is no obvious technical merit for using le16_to_cpup(data->buffer) over le16_to_cpu(data->buffer[0]), then I do really prefer the latter for clarity.

Then you probably wanted to have &data->buffer[0] as a parameter to regmap_bulk_read()?

...

Quoted 28 lines

Quoted 13 lines

drivers/iio/accel/kionix-kx022a.c line 910

Quoted 10 lines

drivers/iio/accel/kionix-kx022a.c new file +9 -0

+	if (data->trigger_enabled) {
+		iio_trigger_poll_chained(data->trig);
+		ret = IRQ_HANDLED;
+	}
+
+	if (data->state & KX022A_STATE_FIFO) {
 
+		ret = __kx022a_fifo_flush(idev, KX022A_FIFO_LENGTH, true);
+		if (ret > 0)
+			ret = IRQ_HANDLED;

I don't like it. Perhaps

Quoted 10 lines

	bool handled = false;
	int ret;

	...
		ret = ...
		if (ret > 0)
			handled = true;
	...

	return IRQ_RETVAL(handled);

I don't see the benefit of adding another variable 'handled'. If I understand correctly, it just introduces one extra 'if' in IRQ thread handling while hiding the return value in IRQ_RETVAL() - macro.

Quoted 3 lines

I do like seeing the IRQ_NONE being returned by default and IRQ_HANDLED only when "handlers" are successfully executed. Adding extra variable just obfuscates this (from my eyes) while adding also the additional 'if'.

You assigning semantically different values to the same variable inside the function.

Add initial support for ROHM/Kionix kx022a accelerometer

About the HW:
KX022A accelerometer is a sensor which:
- supports G-ranges of (+/-) 2, 4, 8, and 16G
- can be connected to I2C or SPI
- has internal HW FIFO buffer
- supports various ODRs (output data rates)
- support detecting special events like double tap or motion
- can be configured to wake-up system when events are detected.

About the series:

This series adds support for only getting the accelerometer data and configuring the G-range / ODR via IIO. Motion detection or double-tap detection are not supported by the series. The other quite important but still missing piece is the runtime PM. Nevertheless, the driver should be usable and brings the basic support for getting accelerometer data.

base-commit: 568035b01cfb107af8d2e4bd2fb9aea22cf5b868
prerequisite-patch-id: f692b7ef9ce0a0b016fb5ac1c5733e562baa4ae2
prerequisite-patch-id: cc2e849e2b3fe553fe955f21a4b995681e04383f
prerequisite-patch-id: f136637f7a8fe92dc4f60b908e2e7aa24aac3f43

Quoted 2 lines · Matti Vaittinen

Add initial support for ROHM/Kionix kx022a accelerometer

Sorry folks! I don't know what happened with my format-patch :(