diff mbox series

[v3,2/3] iio: accel: Support Kionix/ROHM KX022A accelerometer

Message ID 5000bd61650554658d13619c8244f02cedbc182a.1666263249.git.mazziesaccount@gmail.com (mailing list archive)
State Changes Requested
Headers show
Series [v3,1/3] dt-bindings: iio: Add KX022A accelerometer | expand

Commit Message

Matti Vaittinen Oct. 20, 2022, 11:37 a.m. UTC
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.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

---
v2 => v3 Mostly fixes suggested by Andy
- styling issues
- spell-checks
- use namespaces for exported symbols
- drop module param
- document the lock
- change value written when clearing fifo
- drop WARN_ON()
- correctly check the return value from fwnode_irq_get_byname()
- do not emphasize config Y over M
- reorder struct kx022a_data from potential optimization
- IIO_DEVICE_ATTR_RO instead of IIO_DEVICE_ATTR where applicable
- directly include bits.h for BIT()
- use sysfs_emit() for sysfs
- use unique name for IRQ
- convert read_raw() values to CPU endianess
- fix HW-fifo size to 258 bytes
- kx022a-spi, Fix kconfig dependency
- disable irq (to protect timestamp / sample amount calculation) when the
  fifo flush is iniriated by user-space

RFCv1 => v2 (mostly based on feedback from Jonathan):
- Fix bunch of typos from the commit message.
- Add missing break; to the kx022a_write_raw()
- Fix SPI driver to use of_match_table
- Fix indentiation in I2C driver
- Drop struct kx022a_trigger
- Drop cross references from Kconfig
- Use /* */ also in file header comments
- Misc minor styling
- Do sensor-reset at probe
- Support both IRQ pins
- Implement read_avail callback
- Use dma aligned buffers for bulk-reads
- Use iio_trigger_poll_chained()
- Use devm consistently
- Drop inclusion of device.h
- Add SPI device ID for module loading
- Add module param for hw fifo / watermark IRQ usage
- Fix io-vdd-supply name to match one in the bindings
---
 drivers/iio/accel/Kconfig             |   21 +
 drivers/iio/accel/Makefile            |    3 +
 drivers/iio/accel/kionix-kx022a-i2c.c |   52 ++
 drivers/iio/accel/kionix-kx022a-spi.c |   59 ++
 drivers/iio/accel/kionix-kx022a.c     | 1147 +++++++++++++++++++++++++
 drivers/iio/accel/kionix-kx022a.h     |   82 ++
 6 files changed, 1364 insertions(+)
 create mode 100644 drivers/iio/accel/kionix-kx022a-i2c.c
 create mode 100644 drivers/iio/accel/kionix-kx022a-spi.c
 create mode 100644 drivers/iio/accel/kionix-kx022a.c
 create mode 100644 drivers/iio/accel/kionix-kx022a.h

Comments

Andy Shevchenko Oct. 20, 2022, 2:34 p.m. UTC | #1
On Thu, Oct 20, 2022 at 02:37:15PM +0300, Matti Vaittinen wrote:
> 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.

...

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

> +	}

...

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

> +	}

...

> +	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
for robustness, but I don't think it worth it. What we need is to provide
IIO specific type for these tables and use it.

...

> +static int kx022a_turn_on_off_unlocked(struct kx022a_data *data, bool on)
> +{
> +	int ret;
> +
> +	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() ?

> +	return ret;
> +
> +}

...

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

> +				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,
> +					 KX022A_MASK_ODR, n);
> +		data->odr_ns = kx022a_odrs[n];
> +		kx022a_turn_on_unlock(data);
> +		break;
> +	case IIO_CHAN_INFO_SCALE:
> +		n = ARRAY_SIZE(kx022a_scale_table);
> +
> +		while (n-- > 0)
> +			if (val == kx022a_scale_table[n][0] &&
> +			    kx022a_scale_table[n][1] == val2)

Ditto.

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

...

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

> +	return IIO_VAL_INT;
> +}


...

> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		ret = regmap_read(data->regmap, KX022A_REG_ODCNTL, &regval);
> +		if (ret)
> +			return ret;
> +
> +		if ((regval & KX022A_MASK_ODR) >
> +		    ARRAY_SIZE(kx022a_accel_samp_freq_table)) {
> +			dev_err(data->dev, "Invalid ODR\n");
> +			return -EINVAL;
> +		}
> +
> +		kx022a_reg2freq(regval, val, val2);

> +		ret = IIO_VAL_INT_PLUS_MICRO;
> +
> +		break;

return IIO_VAL_INT_PLUS_MICRO;

> +
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = regmap_read(data->regmap, KX022A_REG_CNTL, &regval);
> +		if (ret < 0)
> +			return ret;
> +
> +		kx022a_reg2scale(regval, val, val2);
> +
> +		ret = IIO_VAL_INT_PLUS_MICRO;
> +		break;

Ditto.

...

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

Would simple '0' suffice?

...

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

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

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

...

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

Maybe other lines also need to be revised?

> +	if (ret)
> +		goto unlock_out;

...

> +	int ret = IRQ_NONE;
> +
> +	mutex_lock(&data->mutex);
> +
> +	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);

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

...

> +	if (!dev)
> +		return -ENODEV;

Do you really need this check?

...

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

...

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

> +			dev_warn(dev, "Only one IRQ supported\n");
> +	} else {
> +		irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT2");

Ditto.

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

...

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

...

> +	/*
> +	 * 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.
Matti Vaittinen Oct. 21, 2022, 7:10 a.m. UTC | #2
Hi Andy & All,

Thanks again for the review.

On 10/20/22 17:34, Andy Shevchenko wrote:
> On Thu, Oct 20, 2022 at 02:37:15PM +0300, Matti Vaittinen wrote:
>> 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.
> 
> ...
> 
>> +	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.

I think we discussed this already (and disagreed). To me it looks plain 
ugly to have hard-coded return value in dev_err_probe(). That's why I 
prefer the

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

when -EINVAL is hard-coded. On the other hand, your comment below is 
very valid...

> 
>> +	}
> 
> ...
> 
>> +	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.

...Yes. This is different from the case above, and I agree the 
dev_err_probe() should be used here. Thanks for pointing it out. I'll 
change this and the one in SPI driver as well.

> 
>> +	}
> 
> ...
> 
>> +	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

To me this sounds good. I'll see how it would look like.

> 
> ...
> 
>> +static int kx022a_turn_on_off_unlocked(struct kx022a_data *data, bool on)
>> +{
>> +	int ret;
>> +
>> +	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() ?

Never heard of that before. Seems we have all kinds of gadgets in kernel 
:) I deeply dislike how ternary looks like so I will gladly hide it in 
str_on_off() - thanks!

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

As to why I didn't - because for me the order does not matter regarding 
how hard it is to catch the meaning here. However, there is no problem 
changing this because the order does not matter for me :)
> 
> ...
> 
>> +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.
> 

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.

>> +	return IIO_VAL_INT;
>> +}
> 
> 
> ...
> 
>> +	return regmap_write(data->regmap, KX022A_REG_BUF_CLEAR, 0x0);
> 
> Would simple '0' suffice? 

Technically, yes. I however prefer having the register values in hex - 
unless they directly map to some meaningful physical world entity that 
is easier to understand when in decimal. (For example, a register which 
content would directly represent millivolts or divider or any such 
meaningful physical entity).

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

Well, you know my opinion but Ok ;)

Now I also noticed that the name of the block scoped variable 'samples' 
collides with the function scoped variable of same name. I'll rename the 
block scoped 'samples' just to avoid confusion.

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

Because, again, using 0 is clearer to me. It leaves zero room for 
wondering :)

> 
> Also might be good to have a temporary for channels:
> 
> 		... *chs = data->scan.channels;

I think we have discussed this too previously somewhere. I do dislike 
hiding things in temporary variables. I like seeing that we are really 
using the driver private data and not some stack variable and only use 
temporary variables when they significantly reduce the line count.

However, in this particular case I can scope the temporary variable in 
this smallish block of code - which makes it pretty easy to spot we are 
using the data->scan.channel underneath (as the chs is assigned just a 
row above). And it helps us avoid line split so ... Ok.

> 
> 
> 		for_each_set_bit(bit, idev->active_scan_mask, AXIS_MAX)
> 			memcpy(&chs[bit], &samples[bit], sizeof(chs[bit]));
> 
>> +		iio_push_to_buffers_with_timestamp(idev, &data->scan, tstamp);
>> +
>> +		tstamp += sample_period;
>> +	}
> 
> ...
> 
>> +	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.

Good catch. Thanks.

> ...
> 
>> +	int ret = IRQ_NONE;
>> +
>> +	mutex_lock(&data->mutex);
>> +
>> +	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);

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'.

> 
>> +	}
>> +
>> +	mutex_unlock(&data->mutex);
>> +
>> +	return ret;
> 
> ...
> 
>> +	if (!dev)
>> +		return -ENODEV;
> 
> Do you really need this check?

Good question. In principle I do like checking the parameters of 
exported calls. OTOH, this export is now done using the driver namespace 
so yes, I think we can drop the check.

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

I just drop the check for !dev. But even if I didn't, I wouldn't use 
ternary here - to me it is _much_ harder to read compared to two 
separate ifs while giving no obvious benefits.

> And please, call it fwnode.

Ok. I personally like fw more - probably because I'm used to that - but 
I guess the 'fwnode' is used in number of other places. Thanks.

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

I think I've beeen distracted while writing this part :) I guess I have 
added the temporary variable 'fw' just for the purpose of being able to 
call the dev_fwnode() just once. So, Thanks!

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

Thanks!

> 
> ...
> 
>> +	/*
>> +	 * 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.
> 

Because for example just "spi0,0" is much less informative compared to 
"spi0.0-kx022a". As an user I like seeing the device generating the IRQ. 
I don't wan't to dig out details like which bus/chipselect my device is 
connected to - especially if I have only one accelerometer connected. 
The dev_name() is used just to make this unique for cases where we could 
have multiple similar devices connected to the system (as you suggested 
in previous review).


Once again, thanks for the review! I appreciate your help/suggestions.

Yours
	-- Matti
Andy Shevchenko Oct. 21, 2022, 10:49 a.m. UTC | #3
On Fri, Oct 21, 2022 at 10:10:08AM +0300, Matti Vaittinen wrote:
> On 10/20/22 17:34, Andy Shevchenko wrote:
> > On Thu, Oct 20, 2022 at 02:37:15PM +0300, Matti Vaittinen wrote:

...

> > > +	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.
> > 
> 
> 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()?

...

> > > +	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);
> 
> 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'.

You assigning semantically different values to the same variable inside the
function.
Matti Vaittinen Oct. 21, 2022, 11:02 a.m. UTC | #4
On 10/21/22 13:49, Andy Shevchenko wrote:
> On Fri, Oct 21, 2022 at 10:10:08AM +0300, Matti Vaittinen wrote:
>> On 10/20/22 17:34, Andy Shevchenko wrote:
>>> On Thu, Oct 20, 2022 at 02:37:15PM +0300, Matti Vaittinen wrote:
> 
> ...
> 
>>>> +	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.
>>>
>>
>> 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()?

Yes! Thanks.

> 
> ...
> 
>>>> +	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);
>>
>> 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'.
> 
> You assigning semantically different values to the same variable inside the
> function.

Ah, yes! This was really a bug making it way in. I guess you may just 
have saved me from some not-that-funny debugging session... Well spotted!

I still don't like hiding the IRQ_HANDLED / IRQ_NONE. I'll just go for

         if (data->state & KX022A_STATE_FIFO) { 

                 int ok; 

 

                 ok = __kx022a_fifo_flush(idev, KX022A_FIFO_LENGTH, 
true);
                 if (ok > 0) 

                         ret = IRQ_HANDLED; 

         }

for v4. (Which I try to send still today before my memory is flushed by 
the weekend :])

Thanks a lot!

Yours
	-- Matti
diff mbox series

Patch

diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
index ffac66db7ac9..b7fd054819d2 100644
--- a/drivers/iio/accel/Kconfig
+++ b/drivers/iio/accel/Kconfig
@@ -409,6 +409,27 @@  config IIO_ST_ACCEL_SPI_3AXIS
 	  To compile this driver as a module, choose M here. The module
 	  will be called st_accel_spi.
 
+config IIO_KX022A
+	tristate
+
+config IIO_KX022A_SPI
+	tristate "Kionix KX022A tri-axis digital accelerometer"
+	depends on SPI
+	select IIO_KX022A
+	select REGMAP_SPI
+	help
+	  Enable support for the Kionix KX022A digital tri-axis
+	  accelerometer connected to I2C interface.
+
+config IIO_KX022A_I2C
+	tristate "Kionix KX022A tri-axis digital accelerometer"
+	depends on I2C
+	select IIO_KX022A
+	select REGMAP_I2C
+	help
+	  Enable support for the Kionix KX022A digital tri-axis
+	  accelerometer connected to I2C interface.
+
 config KXSD9
 	tristate "Kionix KXSD9 Accelerometer Driver"
 	select IIO_BUFFER
diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile
index 5e45b5fa5ab5..311ead9c3ef1 100644
--- a/drivers/iio/accel/Makefile
+++ b/drivers/iio/accel/Makefile
@@ -40,6 +40,9 @@  obj-$(CONFIG_FXLS8962AF)	+= fxls8962af-core.o
 obj-$(CONFIG_FXLS8962AF_I2C)	+= fxls8962af-i2c.o
 obj-$(CONFIG_FXLS8962AF_SPI)	+= fxls8962af-spi.o
 obj-$(CONFIG_HID_SENSOR_ACCEL_3D) += hid-sensor-accel-3d.o
+obj-$(CONFIG_IIO_KX022A)	+= kionix-kx022a.o
+obj-$(CONFIG_IIO_KX022A_I2C)	+= kionix-kx022a-i2c.o
+obj-$(CONFIG_IIO_KX022A_SPI)	+= kionix-kx022a-spi.o
 obj-$(CONFIG_KXCJK1013) += kxcjk-1013.o
 obj-$(CONFIG_KXSD9)	+= kxsd9.o
 obj-$(CONFIG_KXSD9_SPI)	+= kxsd9-spi.o
diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c
new file mode 100644
index 000000000000..2fdae9f1925e
--- /dev/null
+++ b/drivers/iio/accel/kionix-kx022a-i2c.c
@@ -0,0 +1,52 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2022 ROHM Semiconductors
+ *
+ * ROHM/KIONIX KX022A accelerometer driver
+ */
+
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+#include "kionix-kx022a.h"
+
+static int kx022a_i2c_probe(struct i2c_client *i2c)
+{
+	struct device *dev = &i2c->dev;
+	struct regmap *regmap;
+
+	if (!i2c->irq) {
+		dev_err(dev, "No IRQ configured\n");
+		return -EINVAL;
+	}
+
+	regmap = devm_regmap_init_i2c(i2c, &kx022a_regmap);
+	if (IS_ERR(regmap)) {
+		dev_err(dev, "Failed to initialize Regmap\n");
+		return PTR_ERR(regmap);
+	}
+
+	return kx022a_probe_internal(dev);
+}
+
+static const struct of_device_id kx022a_of_match[] = {
+	{ .compatible = "kionix,kx022a", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, kx022a_of_match);
+
+static struct i2c_driver kx022a_i2c_driver = {
+	.driver = {
+		.name  = "kx022a-i2c",
+		.of_match_table = kx022a_of_match,
+	  },
+	.probe_new    = kx022a_i2c_probe,
+};
+module_i2c_driver(kx022a_i2c_driver);
+
+MODULE_DESCRIPTION("ROHM/Kionix KX022A accelerometer driver");
+MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(KIONIX_ACCEL);
diff --git a/drivers/iio/accel/kionix-kx022a-spi.c b/drivers/iio/accel/kionix-kx022a-spi.c
new file mode 100644
index 000000000000..dbb5409d05f3
--- /dev/null
+++ b/drivers/iio/accel/kionix-kx022a-spi.c
@@ -0,0 +1,59 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2022 ROHM Semiconductors
+ *
+ * ROHM/KIONIX KX022A accelerometer driver
+ */
+
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/spi/spi.h>
+
+#include "kionix-kx022a.h"
+
+static int kx022a_spi_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct regmap *regmap;
+
+	if (!spi->irq) {
+		dev_err(dev, "No IRQ configured\n");
+		return -EINVAL;
+	}
+
+	regmap = devm_regmap_init_spi(spi, &kx022a_regmap);
+	if (IS_ERR(regmap)) {
+		dev_err(dev, "Failed to initialize Regmap\n");
+		return PTR_ERR(regmap);
+	}
+
+	return kx022a_probe_internal(dev);
+}
+
+static const struct spi_device_id kx022a_id[] = {
+	{ "kx022a" },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, kx022a_id);
+
+static const struct of_device_id kx022a_of_match[] = {
+	{ .compatible = "kionix,kx022a", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, kx022a_of_match);
+
+static struct spi_driver kx022a_spi_driver = {
+	.driver = {
+		.name   = "kx022a-spi",
+		.of_match_table = kx022a_of_match,
+	},
+	.probe = kx022a_spi_probe,
+	.id_table = kx022a_id,
+};
+module_spi_driver(kx022a_spi_driver);
+
+MODULE_DESCRIPTION("ROHM/Kionix kx022A accelerometer driver");
+MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(KIONIX_ACCEL);
diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c
new file mode 100644
index 000000000000..d292bc70120d
--- /dev/null
+++ b/drivers/iio/accel/kionix-kx022a.c
@@ -0,0 +1,1147 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2022 ROHM Semiconductors
+ *
+ * ROHM/KIONIX KX022A accelerometer driver
+ */
+
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/mutex.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+#include <linux/units.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+#include "kionix-kx022a.h"
+
+/*
+ * The KX022A has FIFO which can store 43 samples of HiRes data from 2
+ * channels. This equals to 43 (samples) * 3 (channels) * 2 (bytes/sample) to
+ * 258 bytes of sample data. The quirk to know is that the amount of bytes in
+ * the FIFO is advertised via 8 bit register (max value 255). The thing to note
+ * is that full 258 bytes of data is indicated using the max value 255.
+ */
+#define KX022A_FIFO_LENGTH			43
+#define KX022A_FIFO_FULL_VALUE			255
+#define KX022A_SOFT_RESET_WAIT_TIME_US		(5 * USEC_PER_MSEC)
+#define KX022A_SOFT_RESET_TOTAL_WAIT_TIME_US	(500 * USEC_PER_MSEC)
+
+/* 3 axis, 2 bytes of data for each of the axis */
+#define KX022A_FIFO_SAMPLES_SIZE_BYTES		6
+#define KX022A_FIFO_MAX_BYTES					\
+	(KX022A_FIFO_LENGTH * KX022A_FIFO_SAMPLES_SIZE_BYTES)
+
+enum {
+	KX022A_STATE_SAMPLE,
+	KX022A_STATE_FIFO,
+};
+
+/* Regmap configs */
+static const struct regmap_range kx022a_volatile_ranges[] = {
+	{
+		.range_min = KX022A_REG_XHP_L,
+		.range_max = KX022A_REG_COTR,
+	}, {
+		.range_min = KX022A_REG_TSCP,
+		.range_max = KX022A_REG_INT_REL,
+	}, {
+		/* The reset bit will be cleared by sensor */
+		.range_min = KX022A_REG_CNTL2,
+		.range_max = KX022A_REG_CNTL2,
+	}, {
+		.range_min = KX022A_REG_BUF_STATUS_1,
+		.range_max = KX022A_REG_BUF_READ,
+	},
+};
+
+static const struct regmap_access_table kx022a_volatile_regs = {
+	.yes_ranges = &kx022a_volatile_ranges[0],
+	.n_yes_ranges = ARRAY_SIZE(kx022a_volatile_ranges),
+};
+
+static const struct regmap_range kx022a_precious_ranges[] = {
+	{
+		.range_min = KX022A_REG_INT_REL,
+		.range_max = KX022A_REG_INT_REL,
+	},
+};
+
+static const struct regmap_access_table kx022a_precious_regs = {
+	.yes_ranges = &kx022a_precious_ranges[0],
+	.n_yes_ranges = ARRAY_SIZE(kx022a_precious_ranges),
+};
+
+/*
+ * The HW does not set WHO_AM_I reg as read-only but we don't want to write it
+ * so we still include it in the read-only ranges.
+ */
+static const struct regmap_range kx022a_read_only_ranges[] = {
+	{
+		.range_min = KX022A_REG_XHP_L,
+		.range_max = KX022A_REG_INT_REL,
+	}, {
+		.range_min = KX022A_REG_BUF_STATUS_1,
+		.range_max = KX022A_REG_BUF_STATUS_2,
+	}, {
+		.range_min = KX022A_REG_BUF_READ,
+		.range_max = KX022A_REG_BUF_READ,
+	},
+};
+
+static const struct regmap_access_table kx022a_ro_regs = {
+	.no_ranges = &kx022a_read_only_ranges[0],
+	.n_no_ranges = ARRAY_SIZE(kx022a_read_only_ranges),
+};
+
+static const struct regmap_range kx022a_write_only_ranges[] = {
+	{
+		.range_min = KX022A_REG_BTS_WUF_TH,
+		.range_max = KX022A_REG_BTS_WUF_TH,
+	}, {
+		.range_min = KX022A_REG_MAN_WAKE,
+		.range_max = KX022A_REG_MAN_WAKE,
+	}, {
+		.range_min = KX022A_REG_SELF_TEST,
+		.range_max = KX022A_REG_SELF_TEST,
+	}, {
+		.range_min = KX022A_REG_BUF_CLEAR,
+		.range_max = KX022A_REG_BUF_CLEAR,
+	},
+};
+
+static const struct regmap_access_table kx022a_wo_regs = {
+	.no_ranges = &kx022a_write_only_ranges[0],
+	.n_no_ranges = ARRAY_SIZE(kx022a_write_only_ranges),
+};
+
+static const struct regmap_range kx022a_noinc_read_ranges[] = {
+	{
+		.range_min = KX022A_REG_BUF_READ,
+		.range_max = KX022A_REG_BUF_READ,
+	},
+};
+
+static const struct regmap_access_table kx022a_nir_regs = {
+	.yes_ranges = &kx022a_noinc_read_ranges[0],
+	.n_yes_ranges = ARRAY_SIZE(kx022a_noinc_read_ranges),
+};
+
+const struct regmap_config kx022a_regmap = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.volatile_table = &kx022a_volatile_regs,
+	.rd_table = &kx022a_wo_regs,
+	.wr_table = &kx022a_ro_regs,
+	.rd_noinc_table = &kx022a_nir_regs,
+	.precious_table = &kx022a_precious_regs,
+	.max_register = KX022A_MAX_REGISTER,
+	.cache_type = REGCACHE_RBTREE,
+};
+EXPORT_SYMBOL_NS_GPL(kx022a_regmap, KIONIX_ACCEL);
+
+struct kx022a_data {
+	struct regmap *regmap;
+	struct iio_trigger *trig;
+	struct device *dev;
+	struct iio_mount_matrix orientation;
+	int64_t timestamp, old_timestamp;
+
+	int irq;
+	int inc_reg;
+	int ien_reg;
+
+	unsigned int g_range;
+	unsigned int state;
+	unsigned int odr_ns;
+
+	bool trigger_enabled;
+	/*
+	 * Prevent toggling the sensor stby/active state (PC1 bit) in the
+	 * middle of a configuration, or when the fifo is enabled. Also,
+	 * protect the data stored/retrieved from this structure from
+	 * concurrent accesses.
+	 */
+	struct mutex mutex;
+	u8 watermark;
+
+	/* 3 x 16bit accel data + timestamp */
+	__le16 buffer[8] __aligned(IIO_DMA_MINALIGN);
+	struct {
+		__le16 channels[3];
+		s64 ts __aligned(8);
+	} scan;
+};
+
+static const struct iio_mount_matrix *
+kx022a_get_mount_matrix(const struct iio_dev *idev,
+			const struct iio_chan_spec *chan)
+{
+	struct kx022a_data *data = iio_priv(idev);
+
+	return &data->orientation;
+}
+
+enum {
+	AXIS_X,
+	AXIS_Y,
+	AXIS_Z,
+	AXIS_MAX
+};
+
+static const unsigned long kx022a_scan_masks[] = {
+	BIT(AXIS_X) | BIT(AXIS_Y) | BIT(AXIS_Z), 0
+};
+
+static const struct iio_chan_spec_ext_info kx022a_ext_info[] = {
+	IIO_MOUNT_MATRIX(IIO_SHARED_BY_TYPE, kx022a_get_mount_matrix),
+	{ }
+};
+
+#define KX022A_ACCEL_CHAN(axis, index)						\
+{								\
+	.type = IIO_ACCEL,					\
+	.modified = 1,						\
+	.channel2 = IIO_MOD_##axis,				\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |	\
+				BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
+	.info_mask_shared_by_type_available =			\
+				BIT(IIO_CHAN_INFO_SCALE) |	\
+				BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
+	.ext_info = kx022a_ext_info,				\
+	.address = KX022A_REG_##axis##OUT_L,			\
+	.scan_index = index,					\
+	.scan_type = {                                          \
+		.sign = 's',					\
+		.realbits = 16,					\
+		.storagebits = 16,				\
+		.endianness = IIO_LE,				\
+	},							\
+}
+
+static const struct iio_chan_spec kx022a_channels[] = {
+	KX022A_ACCEL_CHAN(X, 0),
+	KX022A_ACCEL_CHAN(Y, 1),
+	KX022A_ACCEL_CHAN(Z, 2),
+	IIO_CHAN_SOFT_TIMESTAMP(3),
+};
+
+/*
+ * The sensor HW can support ODR up to 1600 Hz, which is beyond what most of the
+ * Linux CPUs can handle without dropping samples. Also, the low power mode is
+ * not available for higher sample rates. Thus, the driver only supports 200 Hz
+ * and slower ODRs. The slowest is 0.78 Hz.
+ */
+static const int kx022a_accel_samp_freq_table[][2] = {
+	{ 0, 780000 },
+	{ 1, 563000 },
+	{ 3, 125000 },
+	{ 6, 250000 },
+	{ 12, 500000 },
+	{ 25, 0 },
+	{ 50, 0 },
+	{ 100, 0 },
+	{ 200, 0 },
+};
+
+static const unsigned int kx022a_odrs[] = {
+	1282051282,
+	639795266,
+	320 * MEGA,
+	160 * MEGA,
+	80 * MEGA,
+	40 * MEGA,
+	20 * MEGA,
+	10 * MEGA,
+	5 * MEGA,
+};
+
+/*
+ * range is typically +-2G/4G/8G/16G, distributed over the amount of bits.
+ * The scale table can be calculated using
+ *	(range / 2^bits) * g = (range / 2^bits) * 9.80665 m/s^2
+ *	=> KX022A uses 16 bit (HiRes mode - assume the low 8 bits are zeroed
+ *	in low-power mode(?) )
+ *	=> +/-2G  => 4 / 2^16 * 9,80665 * 10^6 (to scale to micro)
+ *	=> +/-2G  - 598.550415
+ *	   +/-4G  - 1197.10083
+ *	   +/-8G  - 2394.20166
+ *	   +/-16G - 4788.40332
+ */
+static const int kx022a_scale_table[][2] = {
+	{ 598, 550415 },
+	{ 1197, 100830 },
+	{ 2394, 201660 },
+	{ 4788, 403320 },
+};
+
+static int kx022a_read_avail(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     const int **vals, int *type, int *length,
+			     long mask)
+{
+	switch (mask) {
+	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;
+	default:
+		return -EINVAL;
+	}
+}
+
+#define KX022A_DEFAULT_PERIOD_NS (20 * NSEC_PER_MSEC)
+
+static void kx022a_reg2freq(unsigned int val,  int *val1, int *val2)
+{
+	*val1 = kx022a_accel_samp_freq_table[val & KX022A_MASK_ODR][0];
+	*val2 = kx022a_accel_samp_freq_table[val & KX022A_MASK_ODR][1];
+}
+
+static void kx022a_reg2scale(unsigned int val, unsigned int *val1,
+			     unsigned int *val2)
+{
+	val &= KX022A_MASK_GSEL;
+	val >>= KX022A_GSEL_SHIFT;
+
+	*val1 = kx022a_scale_table[val][0];
+	*val2 = kx022a_scale_table[val][1];
+}
+
+static int kx022a_turn_on_off_unlocked(struct kx022a_data *data, bool on)
+{
+	int ret;
+
+	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);
+
+	return ret;
+
+}
+
+static int kx022a_turn_off_lock(struct kx022a_data *data)
+{
+	int ret;
+
+	mutex_lock(&data->mutex);
+	ret = kx022a_turn_on_off_unlocked(data, false);
+	if (ret)
+		mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int kx022a_turn_on_unlock(struct kx022a_data *data)
+{
+	int ret;
+
+	ret = kx022a_turn_on_off_unlocked(data, true);
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int kx022a_write_raw(struct iio_dev *idev,
+			    struct iio_chan_spec const *chan,
+			    int val, int val2, long mask)
+{
+	struct kx022a_data *data = iio_priv(idev);
+	int ret, n;
+
+	/*
+	 * We should not allow changing scale or frequency when FIFO is running
+	 * as it will mess the timestamp/scale for samples existing in the
+	 * buffer. If this turns out to be an issue we can later change logic
+	 * to internally flush the fifo before reconfiguring so the samples in
+	 * fifo keep matching the freq/scale settings. (Such setup could cause
+	 * issues if users trust the watermark to be reached within known
+	 * time-limit).
+	 */
+	ret = iio_device_claim_direct_mode(idev);
+	if (ret)
+		return ret;
+
+	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)
+				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,
+					 KX022A_MASK_ODR, n);
+		data->odr_ns = kx022a_odrs[n];
+		kx022a_turn_on_unlock(data);
+		break;
+	case IIO_CHAN_INFO_SCALE:
+		n = ARRAY_SIZE(kx022a_scale_table);
+
+		while (n-- > 0)
+			if (val == kx022a_scale_table[n][0] &&
+			    kx022a_scale_table[n][1] == val2)
+				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;
+	}
+
+unlock_out:
+	iio_device_release_direct_mode(idev);
+
+	return ret;
+}
+
+static int kx022a_fifo_set_wmi(struct kx022a_data *data)
+{
+	u8 threshold;
+
+	threshold = data->watermark;
+
+	return regmap_update_bits(data->regmap, KX022A_REG_BUF_CNTL1,
+				  KX022A_MASK_WM_TH, threshold);
+}
+
+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]);
+
+	return IIO_VAL_INT;
+}
+
+static int kx022a_read_raw(struct iio_dev *idev,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long mask)
+{
+	struct kx022a_data *data = iio_priv(idev);
+	unsigned int regval;
+	int ret = -EINVAL;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = iio_device_claim_direct_mode(idev);
+		if (ret)
+			return ret;
+
+		mutex_lock(&data->mutex);
+		ret = kx022a_get_axis(data, chan, val);
+		mutex_unlock(&data->mutex);
+
+		iio_device_release_direct_mode(idev);
+		break;
+
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		ret = regmap_read(data->regmap, KX022A_REG_ODCNTL, &regval);
+		if (ret)
+			return ret;
+
+		if ((regval & KX022A_MASK_ODR) >
+		    ARRAY_SIZE(kx022a_accel_samp_freq_table)) {
+			dev_err(data->dev, "Invalid ODR\n");
+			return -EINVAL;
+		}
+
+		kx022a_reg2freq(regval, val, val2);
+		ret = IIO_VAL_INT_PLUS_MICRO;
+
+		break;
+
+	case IIO_CHAN_INFO_SCALE:
+		ret = regmap_read(data->regmap, KX022A_REG_CNTL, &regval);
+		if (ret < 0)
+			return ret;
+
+		kx022a_reg2scale(regval, val, val2);
+
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+	}
+
+	return ret;
+};
+
+static int kx022a_validate_trigger(struct iio_dev *idev,
+				   struct iio_trigger *trig)
+{
+	struct kx022a_data *data = iio_priv(idev);
+
+	if (data->trig != trig)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int kx022a_set_watermark(struct iio_dev *idev, unsigned int val)
+{
+	struct kx022a_data *data = iio_priv(idev);
+
+	if (val > KX022A_FIFO_LENGTH)
+		val = KX022A_FIFO_LENGTH;
+
+	mutex_lock(&data->mutex);
+	data->watermark = val;
+	mutex_unlock(&data->mutex);
+
+	return 0;
+}
+
+static ssize_t hwfifo_enabled_show(struct device *dev,
+				   struct device_attribute *attr,
+				   char *buf)
+{
+	struct iio_dev *idev = dev_to_iio_dev(dev);
+	struct kx022a_data *data = iio_priv(idev);
+	bool state;
+
+	mutex_lock(&data->mutex);
+	state = data->state;
+	mutex_unlock(&data->mutex);
+
+	return sysfs_emit(buf, "%d\n", state);
+}
+
+static ssize_t hwfifo_watermark_show(struct device *dev,
+				     struct device_attribute *attr,
+				     char *buf)
+{
+	struct iio_dev *idev = dev_to_iio_dev(dev);
+	struct kx022a_data *data = iio_priv(idev);
+	int wm;
+
+	mutex_lock(&data->mutex);
+	wm = data->watermark;
+	mutex_unlock(&data->mutex);
+
+	return sysfs_emit(buf, "%d\n", wm);
+}
+
+static IIO_DEVICE_ATTR_RO(hwfifo_enabled, 0);
+static IIO_DEVICE_ATTR_RO(hwfifo_watermark, 0);
+
+static const struct attribute *kx022a_fifo_attributes[] = {
+	&iio_dev_attr_hwfifo_watermark.dev_attr.attr,
+	&iio_dev_attr_hwfifo_enabled.dev_attr.attr,
+	NULL
+};
+
+static int kx022a_drop_fifo_contents(struct kx022a_data *data)
+{
+	/*
+	 * We must clear the old time-stamp to avoid computing the timestamps
+	 * based on samples acquired when buffer was last enabled.
+	 *
+	 * We don't need to protect the timestamp as long as we are only
+	 * called from fifo-disable where we can guarantee the sensor is not
+	 * triggering interrupts and where the mutex is locked to prevent the
+	 * user-space access.
+	 */
+	data->timestamp = 0;
+
+	return regmap_write(data->regmap, KX022A_REG_BUF_CLEAR, 0x0);
+}
+
+static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples,
+			       bool irq)
+{
+	struct kx022a_data *data = iio_priv(idev);
+	struct device *dev = regmap_get_device(data->regmap);
+	u16 buffer[KX022A_FIFO_LENGTH * 3];
+	uint64_t sample_period;
+	int count, fifo_bytes;
+	bool renable = false;
+	int64_t tstamp;
+	int ret, i;
+
+	ret = regmap_read(data->regmap, KX022A_REG_BUF_STATUS_1, &fifo_bytes);
+	if (ret) {
+		dev_err(dev, "Error reading buffer status\n");
+		return ret;
+	}
+
+	/* Let's not overflow if we for some reason get bogus value from i2c */
+	if (fifo_bytes == KX022A_FIFO_FULL_VALUE)
+		fifo_bytes = KX022A_FIFO_MAX_BYTES;
+
+	if (fifo_bytes % KX022A_FIFO_SAMPLES_SIZE_BYTES)
+		dev_warn(data->dev, "Bad FIFO alignment. Data may be corrupt\n");
+
+	count = fifo_bytes / KX022A_FIFO_SAMPLES_SIZE_BYTES;
+	if (!count)
+		return 0;
+
+	/*
+	 * If we are being called from IRQ handler we know the stored timestamp
+	 * is fairly accurate for the last stored sample. Otherwise, if we are
+	 * called as a result of a read operation from userspace and hence
+	 * before the watermark interrupt was triggered, take a timestamp
+	 * now. We can fall anywhere in between two samples so the error in this
+	 * case is at most one sample period.
+	 */
+	if (!irq) {
+		/*
+		 * We need to have the IRQ disabled or we risk of messing-up
+		 * the timestamps. If we are ran from IRQ, then the
+		 * IRQF_ONESHOT has us covered - but if we are ran by the
+		 * user-space read we need to disable the IRQ to be on a safe
+		 * side. We do this usng synchronous disable so that if the
+		 * IRQ thread is being ran on other CPU we wait for it to be
+		 * finished.
+		 */
+		disable_irq(data->irq);
+		renable = true;
+
+		data->old_timestamp = data->timestamp;
+		data->timestamp = iio_get_time_ns(idev);
+	}
+
+	/*
+	 * Approximate timestamps for each of the sample based on the sampling
+	 * frequency, timestamp for last sample and number of samples.
+	 *
+	 * We'd better not use the current bandwidth settings to compute the
+	 * sample period. The real sample rate varies with the device and
+	 * small variation adds when we store a large number of samples.
+	 *
+	 * To avoid this issue we compute the actual sample period ourselves
+	 * based on the timestamp delta between the last two flush operations.
+	 */
+	if (data->old_timestamp) {
+		sample_period = data->timestamp - data->old_timestamp;
+		do_div(sample_period, count);
+	} else {
+		sample_period = data->odr_ns;
+	}
+	tstamp = data->timestamp - (count - 1) * sample_period;
+
+	if (samples && count > samples) {
+		/*
+		 * Here we leave some old samples to the buffer. We need to
+		 * adjust the timestamp to match the first sample in the buffer
+		 * or we will miscalculate the sample_period at next round.
+		 */
+		data->timestamp -= (count - samples) * sample_period;
+		count = samples;
+	}
+
+	fifo_bytes = count * KX022A_FIFO_SAMPLES_SIZE_BYTES;
+	ret = regmap_noinc_read(data->regmap, KX022A_REG_BUF_READ,
+				buffer, fifo_bytes);
+	if (ret)
+		goto renable_out;
+
+	for (i = 0; i < count; i++) {
+		int bit;
+		u16 *samples = &buffer[i * 3];
+
+		for_each_set_bit(bit, idev->active_scan_mask, AXIS_MAX)
+			memcpy(&data->scan.channels[bit], &samples[bit],
+			       sizeof(data->scan.channels[0]));
+
+		iio_push_to_buffers_with_timestamp(idev, &data->scan, tstamp);
+
+		tstamp += sample_period;
+	}
+
+	ret = count;
+
+renable_out:
+	if (renable)
+		enable_irq(data->irq);
+
+	return ret;
+}
+
+static int kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples)
+{
+	struct kx022a_data *data = iio_priv(idev);
+	int ret;
+
+	mutex_lock(&data->mutex);
+	ret = __kx022a_fifo_flush(idev, samples, false);
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static const struct iio_info kx022a_info = {
+	.read_raw = &kx022a_read_raw,
+	.write_raw = &kx022a_write_raw,
+	.read_avail = &kx022a_read_avail,
+
+	.validate_trigger	= kx022a_validate_trigger,
+	.hwfifo_set_watermark	= kx022a_set_watermark,
+	.hwfifo_flush_to_buffer	= kx022a_fifo_flush,
+};
+
+static int kx022a_set_drdy_irq(struct kx022a_data *data, bool en)
+{
+	if (en)
+		return regmap_set_bits(data->regmap, KX022A_REG_CNTL,
+				       KX022A_MASK_DRDY);
+
+	return regmap_clear_bits(data->regmap, KX022A_REG_CNTL,
+				 KX022A_MASK_DRDY);
+}
+
+static int kx022a_prepare_irq_pin(struct kx022a_data *data)
+{
+	/* Enable IRQ1 pin. Set polarity to active low */
+	int mask = KX022A_MASK_IEN | KX022A_MASK_IPOL |
+		   KX022A_MASK_ITYP;
+	int val = KX022A_MASK_IEN | KX022A_IPOL_LOW |
+		  KX022A_ITYP_LEVEL;
+	int ret;
+
+	ret = regmap_update_bits(data->regmap, data->inc_reg, mask, val);
+	if (ret)
+		return ret;
+
+	/* We enable WMI to IRQ pin only at buffer_enable */
+	mask = KX022A_MASK_INS2_DRDY;
+
+	return regmap_set_bits(data->regmap, data->ien_reg, mask);
+}
+
+static int kx022a_fifo_disable(struct kx022a_data *data)
+{
+	int ret = 0;
+
+	ret = kx022a_turn_off_lock(data);
+	if (ret)
+		return ret;
+
+	ret = regmap_clear_bits(data->regmap, data->ien_reg,
+				KX022A_MASK_WMI);
+	if (ret)
+		goto unlock_out;
+
+	ret = regmap_clear_bits(data->regmap, KX022A_REG_BUF_CNTL2,
+				KX022A_MASK_BUF_EN);
+	if (ret)
+		goto unlock_out;
+
+	data->state &= (~KX022A_STATE_FIFO);
+
+	kx022a_drop_fifo_contents(data);
+
+	return kx022a_turn_on_unlock(data);
+
+unlock_out:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int kx022a_buffer_predisable(struct iio_dev *idev)
+{
+	struct kx022a_data *data = iio_priv(idev);
+
+	if (iio_device_get_current_mode(idev) == INDIO_BUFFER_TRIGGERED)
+		return 0;
+
+	return kx022a_fifo_disable(data);
+}
+
+static int kx022a_fifo_enable(struct kx022a_data *data)
+{
+	int ret;
+
+	ret = kx022a_turn_off_lock(data);
+	if (ret)
+		return ret;
+
+	/* Update watermark to HW */
+	ret = kx022a_fifo_set_wmi(data);
+	if (ret)
+		goto unlock_out;
+
+	/* Enable buffer */
+	ret = regmap_set_bits(data->regmap, KX022A_REG_BUF_CNTL2,
+			      KX022A_MASK_BUF_EN);
+	if (ret)
+		goto unlock_out;
+
+	data->state |= KX022A_STATE_FIFO;
+	ret = regmap_set_bits(data->regmap, data->ien_reg,
+			      KX022A_MASK_WMI);
+	if (ret)
+		goto unlock_out;
+
+	return kx022a_turn_on_unlock(data);
+
+unlock_out:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int kx022a_buffer_postenable(struct iio_dev *idev)
+{
+	struct kx022a_data *data = iio_priv(idev);
+
+	/*
+	 * If we use data-ready trigger, then the IRQ masks should be handled by
+	 * trigger enable and the hardware buffer is not used but we just update
+	 * results to the IIO fifo when data-ready triggers.
+	 */
+	if (iio_device_get_current_mode(idev) == INDIO_BUFFER_TRIGGERED)
+		return 0;
+
+	return kx022a_fifo_enable(data);
+}
+
+static const struct iio_buffer_setup_ops kx022a_buffer_ops = {
+	.postenable = kx022a_buffer_postenable,
+	.predisable = kx022a_buffer_predisable,
+};
+
+static irqreturn_t kx022a_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *idev = pf->indio_dev;
+	struct kx022a_data *data = iio_priv(idev);
+	int ret;
+
+	ret = regmap_bulk_read(data->regmap, KX022A_REG_XOUT_L, data->buffer,
+			       KX022A_FIFO_SAMPLES_SIZE_BYTES);
+	if (ret < 0)
+		goto err_read;
+
+	iio_push_to_buffers_with_timestamp(idev, data->buffer, pf->timestamp);
+err_read:
+	iio_trigger_notify_done(idev->trig);
+
+	return IRQ_HANDLED;
+}
+
+/* Get timestamps and wake the thread if we need to read data */
+static irqreturn_t kx022a_irq_handler(int irq, void *private)
+{
+	struct iio_dev *idev = private;
+	struct kx022a_data *data = iio_priv(idev);
+
+	data->old_timestamp = data->timestamp;
+	data->timestamp = iio_get_time_ns(idev);
+
+	if (data->state & KX022A_STATE_FIFO || data->trigger_enabled)
+		return IRQ_WAKE_THREAD;
+
+	return IRQ_NONE;
+}
+
+/*
+ * WMI and data-ready IRQs are acked when results are read. If we add
+ * TILT/WAKE or other IRQs - then we may need to implement the acking
+ * (which is racy).
+ */
+static irqreturn_t kx022a_irq_thread_handler(int irq, void *private)
+{
+	struct iio_dev *idev = private;
+	struct kx022a_data *data = iio_priv(idev);
+	int ret = IRQ_NONE;
+
+	mutex_lock(&data->mutex);
+
+	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;
+	}
+
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int kx022a_trigger_set_state(struct iio_trigger *trig,
+				    bool state)
+{
+	struct kx022a_data *data = iio_trigger_get_drvdata(trig);
+	int ret = 0;
+
+	mutex_lock(&data->mutex);
+
+	if (data->trigger_enabled == state)
+		goto unlock_out;
+
+	if (data->state & KX022A_STATE_FIFO) {
+		dev_warn(data->dev, "Can't set trigger when FIFO enabled\n");
+		ret = -EBUSY;
+		goto unlock_out;
+	}
+
+	ret = kx022a_turn_on_off_unlocked(data, false);
+	if (ret)
+		goto unlock_out;
+
+	data->trigger_enabled = state;
+	ret = kx022a_set_drdy_irq(data, state);
+	if (ret)
+		goto unlock_out;
+
+	ret = kx022a_turn_on_off_unlocked(data, true);
+
+unlock_out:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static const struct iio_trigger_ops kx022a_trigger_ops = {
+	.set_trigger_state = kx022a_trigger_set_state,
+};
+
+static int kx022a_chip_init(struct kx022a_data *data)
+{
+	int ret, val;
+
+	/* Reset the senor */
+	ret = regmap_write(data->regmap, KX022A_REG_CNTL2, KX022A_MASK_SRST);
+	if (ret)
+		return ret;
+
+	/*
+	 * I've seen I2C read failures if we poll too fast after the sensor
+	 * reset. Slight delay gives I2C block the time to recover.
+	 */
+	msleep(1);
+
+	ret = regmap_read_poll_timeout(data->regmap, KX022A_REG_CNTL2, val,
+				       !(val & KX022A_MASK_SRST),
+				       KX022A_SOFT_RESET_WAIT_TIME_US,
+				       KX022A_SOFT_RESET_TOTAL_WAIT_TIME_US);
+	if (ret) {
+		dev_err(data->dev, "Sensor reset %s\n",
+			val & KX022A_MASK_SRST ? "timeout" : "fail#");
+		return ret;
+	}
+
+	ret = regmap_reinit_cache(data->regmap, &kx022a_regmap);
+	if (ret) {
+		dev_err(data->dev, "Failed to reinit reg cache\n");
+		return ret;
+	}
+
+	/* set data res 16bit */
+	ret = regmap_set_bits(data->regmap, KX022A_REG_BUF_CNTL2,
+			      KX022A_MASK_BRES16);
+	if (ret) {
+		dev_err(data->dev, "Failed to set data resolution\n");
+		return ret;
+	}
+
+	return kx022a_prepare_irq_pin(data);
+}
+
+int kx022a_probe_internal(struct device *dev)
+{
+	static const char * const regulator_names[] = {"io-vdd", "vdd"};
+	struct iio_trigger *indio_trig;
+	struct kx022a_data *data;
+	struct fwnode_handle *fw;
+	struct regmap *regmap;
+	unsigned int chip_id;
+	struct iio_dev *idev;
+	int ret, irq;
+	char *name;
+
+	if (!dev)
+		return -ENODEV;
+
+	regmap = dev_get_regmap(dev, NULL);
+	if (!regmap) {
+		dev_err(dev, "no regmap\n");
+		return -EINVAL;
+	}
+
+	idev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!idev)
+		return -ENOMEM;
+
+	data = iio_priv(idev);
+
+	/*
+	 * VDD is the analog and digital domain voltage supply and
+	 * IO_VDD is the digital I/O voltage supply.
+	 */
+	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulator_names),
+					     regulator_names);
+	if (ret && ret != -ENODEV)
+		return dev_err_probe(dev, ret, "failed to enable regulator\n");
+
+	ret = regmap_read(regmap, KX022A_REG_WHO, &chip_id);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to access sensor\n");
+
+	if (chip_id != KX022A_ID) {
+		dev_err(dev, "unsupported device 0x%x\n", chip_id);
+		return -EINVAL;
+	}
+
+	fw = dev_fwnode(dev);
+	if (!fw)
+		return -ENODEV;
+
+	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)
+			dev_warn(dev, "Only one IRQ supported\n");
+	} else {
+		irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT2");
+		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;
+	}
+
+	data->regmap = regmap;
+	data->dev = dev;
+	data->irq = irq;
+	data->odr_ns = KX022A_DEFAULT_PERIOD_NS;
+	mutex_init(&data->mutex);
+
+	idev->channels = kx022a_channels;
+	idev->num_channels = ARRAY_SIZE(kx022a_channels);
+	idev->name = "kx022-accel";
+	idev->info = &kx022a_info;
+	idev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
+	idev->available_scan_masks = kx022a_scan_masks;
+
+	/* Read the mounting matrix, if present */
+	ret = iio_read_mount_matrix(dev, &data->orientation);
+	if (ret)
+		return ret;
+
+	/* The sensor must be turned off for configuration */
+	ret = kx022a_turn_off_lock(data);
+	if (ret)
+		return ret;
+
+	ret = kx022a_chip_init(data);
+	if (ret) {
+		mutex_unlock(&data->mutex);
+		return ret;
+	}
+
+	ret = kx022a_turn_on_unlock(data);
+	if (ret)
+		return ret;
+
+	udelay(100);
+	ret = devm_iio_triggered_buffer_setup_ext(dev, idev,
+						  &iio_pollfunc_store_time,
+						  kx022a_trigger_handler,
+						  IIO_BUFFER_DIRECTION_IN,
+						  &kx022a_buffer_ops,
+						  kx022a_fifo_attributes);
+
+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "iio_triggered_buffer_setup_ext FAIL %d\n",
+				     ret);
+	indio_trig = devm_iio_trigger_alloc(dev, "%sdata-rdy-dev%d", idev->name,
+					    iio_device_id(idev));
+	if (!indio_trig)
+		return -ENOMEM;
+
+	data->trig = indio_trig;
+
+	indio_trig->ops = &kx022a_trigger_ops;
+	iio_trigger_set_drvdata(indio_trig, data);
+
+	ret = devm_iio_trigger_register(dev, indio_trig);
+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "Trigger registration failed\n");
+
+	ret = devm_iio_device_register(data->dev, idev);
+	if (ret < 0)
+		return dev_err_probe(dev, ret,
+				     "Unable to register iio device\n");
+
+	/*
+	 * 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));
+
+	ret = devm_request_threaded_irq(data->dev, irq, kx022a_irq_handler,
+					&kx022a_irq_thread_handler,
+					IRQF_ONESHOT, name, idev);
+	if (ret)
+		return dev_err_probe(data->dev, ret, "Could not request IRQ\n");
+
+	return ret;
+}
+EXPORT_SYMBOL_NS_GPL(kx022a_probe_internal, KIONIX_ACCEL);
+
+MODULE_DESCRIPTION("ROHM/Kionix KX022A accelerometer driver");
+MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/iio/accel/kionix-kx022a.h b/drivers/iio/accel/kionix-kx022a.h
new file mode 100644
index 000000000000..12424649d438
--- /dev/null
+++ b/drivers/iio/accel/kionix-kx022a.h
@@ -0,0 +1,82 @@ 
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2022 ROHM Semiconductors
+ *
+ * ROHM/KIONIX KX022A accelerometer driver
+ */
+
+#ifndef _KX022A_H_
+#define _KX022A_H_
+
+#include <linux/bits.h>
+#include <linux/regmap.h>
+
+#define KX022A_REG_WHO		0x0f
+#define KX022A_ID		0xc8
+
+#define KX022A_REG_CNTL2	0x19
+#define KX022A_MASK_SRST	BIT(7)
+#define KX022A_REG_CNTL		0x18
+#define KX022A_MASK_PC1		BIT(7)
+#define KX022A_MASK_RES		BIT(6)
+#define KX022A_MASK_DRDY	BIT(5)
+#define KX022A_MASK_GSEL	GENMASK(4, 3)
+#define KX022A_GSEL_SHIFT	3
+#define KX022A_GSEL_2		0x0
+#define KX022A_GSEL_4		BIT(3)
+#define KX022A_GSEL_8		BIT(4)
+#define KX022A_GSEL_16		GENMASK(4, 3)
+
+#define KX022A_REG_INS2		0x13
+#define KX022A_MASK_INS2_DRDY	BIT(4)
+#define KX122_MASK_INS2_WMI	BIT(5)
+
+#define KX022A_REG_XHP_L	0x0
+#define KX022A_REG_XOUT_L	0x06
+#define KX022A_REG_YOUT_L	0x08
+#define KX022A_REG_ZOUT_L	0x0a
+#define KX022A_REG_COTR		0x0c
+#define KX022A_REG_TSCP		0x10
+#define KX022A_REG_INT_REL	0x17
+
+#define KX022A_REG_ODCNTL	0x1b
+
+#define KX022A_REG_BTS_WUF_TH	0x31
+#define KX022A_REG_MAN_WAKE	0x2c
+
+#define KX022A_REG_BUF_CNTL1	0x3a
+#define KX022A_MASK_WM_TH	GENMASK(6, 0)
+#define KX022A_REG_BUF_CNTL2	0x3b
+#define KX022A_MASK_BUF_EN	BIT(7)
+#define KX022A_MASK_BRES16	BIT(6)
+#define KX022A_REG_BUF_STATUS_1	0x3c
+#define KX022A_REG_BUF_STATUS_2	0x3d
+#define KX022A_REG_BUF_CLEAR	0x3e
+#define KX022A_REG_BUF_READ	0x3f
+#define KX022A_MASK_ODR		GENMASK(3, 0)
+#define KX022A_ODR_SHIFT	3
+#define KX022A_FIFO_MAX_WMI_TH	41
+
+#define KX022A_REG_INC1		0x1c
+#define KX022A_REG_INC5		0x20
+#define KX022A_REG_INC6		0x21
+#define KX022A_MASK_IEN		BIT(5)
+#define KX022A_MASK_IPOL	BIT(4)
+#define KX022A_IPOL_LOW		0
+#define KX022A_IPOL_HIGH	KX022A_MASK_IPOL1
+#define KX022A_MASK_ITYP	BIT(3)
+#define KX022A_ITYP_PULSE	KX022A_MASK_ITYP
+#define KX022A_ITYP_LEVEL	0
+
+#define KX022A_REG_INC4		0x1f
+#define KX022A_MASK_WMI		BIT(5)
+
+#define KX022A_REG_SELF_TEST	0x60
+#define KX022A_MAX_REGISTER	0x60
+
+struct device;
+
+int kx022a_probe_internal(struct device *dev);
+extern const struct regmap_config kx022a_regmap;
+
+#endif