| Message ID | fe1088cebb0de70bcb99af517004c1816b696825.1663760018.git.mazziesaccount@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: Support ROHM/Kionix kx022a | expand |
On 9/21/22 14:45, Matti Vaittinen wrote: > KX022A is a 3-axis Accelerometer from ROHM/Kionix. The senor 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 fatures. > > 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. > > NOTE: Filling-up the hardware FIFO should be avoided. During my testing > I noticed that filling up the hardware FIFO might mess-up the sample > count. My sensor ended up in a state where amount of data in FIFO was > reported to be 0xff bytes, which equals to 42,5 samples. Specification > says the FIFO can hold maximum of 41 samples in HiRes mode. Also, at > least once the FIFO was stuck in a state where reading data from > hwardware FIFO did not decrease the amount of data reported to be in the > FIFO - eg. FIFO was "stuck". The code has now an error count and 10 > reads with invalid FIFO data count will cause the fifo contents to be > dropped. > > Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> > --- //snip > diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c //snip > +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; > + > + /* > + * 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). > + */ > + mutex_lock(&data->mutex); > + if (iio_buffer_enabled(idev)) { > + ret = -EBUSY; > + goto unlock_out; > + } > + switch (mask) { > + case IIO_CHAN_INFO_SAMP_FREQ: > + ret = kx022a_find_tuple_index(&kx022a_accel_samp_freq_table[0], > + ARRAY_SIZE(kx022a_accel_samp_freq_table), > + val, val2); > + /* Configure if we found valid ODR */ > + if (ret >= 0) { > + int odr = ret; > + > + ret = __kx022a_turn_off_unlocked(data); > + if (ret) > + goto unlock_out; > + > + ret = regmap_update_bits(data->regmap, KX022A_REG_ODCNTL, > + KX022A_MASK_ODR, odr); > + data->odr_ns = kx022a_odrs[odr]; > + __kx022a_turn_on_unlocked(data); > + } > + break; > + case IIO_CHAN_INFO_SCALE: > + ret = kx022a_find_tuple_index(&kx022a_scale_table[0], > + ARRAY_SIZE(kx022a_scale_table), > + val, val2); > + /* Configure if we found valid scale */ > + if (ret >= 0) { > + ret = __kx022a_turn_off_unlocked(data); > + if (ret) > + goto unlock_out; > + > + ret = regmap_update_bits(data->regmap, KX022A_REG_CNTL, > + KX022A_MASK_GSEL, > + ret << KX022A_GSEL_SHIFT); > + __kx022a_turn_on_unlocked(data); > + } I did a last minute change to the locking here. As a result I missed the break causing -EINVAL to be returned for scale setting. The build bot pinged me about it :) Will be fixed when I respin this! > + default: > + ret = -EINVAL; > + } > + > +unlock_out: > + mutex_unlock(&data->mutex); > + > + return ret; > +} > + Yours, --Matti
On Wed, 21 Sep 2022 14:45:35 +0300 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > KX022A is a 3-axis Accelerometer from ROHM/Kionix. The senor 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 fatures. > > 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. > > NOTE: Filling-up the hardware FIFO should be avoided. During my testing > I noticed that filling up the hardware FIFO might mess-up the sample > count. My sensor ended up in a state where amount of data in FIFO was > reported to be 0xff bytes, which equals to 42,5 samples. Specification > says the FIFO can hold maximum of 41 samples in HiRes mode. Also, at > least once the FIFO was stuck in a state where reading data from > hwardware FIFO did not decrease the amount of data reported to be in the spell check this. > FIFO - eg. FIFO was "stuck". The code has now an error count and 10 > reads with invalid FIFO data count will cause the fifo contents to be > dropped. Ouch - that's nasty. > > Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> Hi Matti, A somewhat superficial review as I'm short on time, but wanted to get some initial comments out to you as I'd started reviewing this yesterday and not sure when I'd get back to it. Jonathan > --- > drivers/iio/accel/Kconfig | 23 + > drivers/iio/accel/Makefile | 3 + > drivers/iio/accel/kionix-kx022a-i2c.c | 52 ++ > drivers/iio/accel/kionix-kx022a-spi.c | 50 ++ > drivers/iio/accel/kionix-kx022a.c | 1149 +++++++++++++++++++++++++ > drivers/iio/accel/kionix-kx022a.h | 76 ++ > 6 files changed, 1353 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 > > diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig > index 35798712f811..12ad6bcb2c76 100644 > --- a/drivers/iio/accel/Kconfig > +++ b/drivers/iio/accel/Kconfig > @@ -409,6 +409,29 @@ 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 I2C > + select IIO_KX022A > + select REGMAP_SPI > + help > + Say Y here to enable support for the Kionix KX022A digital tri-axis > + accelerometer connected to I2C interface. See IIO_KX022A_I2C if > + you want to enable support for the KX022A connected via I2C. I would not bother with the cross reference to the I2C config element. > + > +config IIO_KX022A_I2C > + tristate "Kionix KX022A tri-axis digital accelerometer" > + depends on I2C > + select IIO_KX022A > + select REGMAP_I2C > + help > + Say Y here to enable support for the Kionix KX022A digital tri-axis > + accelerometer connected to I2C interface. See IIO_KX022A_SPI if > + you want to enable support for the KX022A connected via SPI. > + > diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c > new file mode 100644 > index 000000000000..2dbf8a33b84b > --- /dev/null > +++ b/drivers/iio/accel/kionix-kx022a-i2c.c > @@ -0,0 +1,52 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +// Other than where required for SPDX use the /* * Copyright... */ syntax. > +// 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 regmap *regmap; > + struct device *dev = &i2c->dev; > + > + if (!i2c->irq) { There look to be 2 interrupt lines, so you need to know which one this is. fwnode_irq_get_byname() trying first int1 then int2 (as only a single one might be mapped). > + 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, i2c->irq); > +} > + > +static const struct of_device_id kx022a_of_match[] = { > + { .compatible = "kionix,kx022a", }, > + { }, No comma on null terminators like this. > +}; > +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, I'd avoid tab alignment like this. It breaks far to often and doesn't really help readability. > +}; > + > +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"); > diff --git a/drivers/iio/accel/kionix-kx022a-spi.c b/drivers/iio/accel/kionix-kx022a-spi.c > new file mode 100644 > index 000000000000..007f4b726d8f > --- /dev/null > +++ b/drivers/iio/accel/kionix-kx022a-spi.c > @@ -0,0 +1,50 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +// > +// Copyright (C) 2022 ROHM Semiconductors > +// > +// ROHM/KIONIX KX022A accelerometer driver Pretty much same comments as for the i2c part. > + > +#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, spi->irq); > +} > + > +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", > + }, > + .probe = kx022a_spi_probe, > +}; > + > +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"); > diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c > new file mode 100644 > index 000000000000..9f9e0d7efc09 > --- /dev/null > +++ b/drivers/iio/accel/kionix-kx022a.c > @@ -0,0 +1,1149 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +// > +// Copyright (C) 2022 ROHM Semiconductors > +// > +// ROHM/KIONIX KX022A accelerometer driver > + > +#include <linux/delay.h> > +#include <linux/gpio.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 <linux/interrupt.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/of.h> > +#include <linux/of_gpio.h> > +#include <linux/regmap.h> > +#include <linux/regulator/consumer.h> > +#include <linux/slab.h> > + > +#include "kionix-kx022a.h" > + > +/* > + * Threshold for deciding our HW fifo is unrecoverably corrupt and should be > + * cleared > + */ > +#define KXO22A_FIFO_ERR_THRESHOLD 10 > +#define KX022A_FIFO_LENGTH 41 > +/* 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) > + > +#define MIN_ODR_INTERVAL_MS 5 > +#define MAX_ODR_INTERVAL_MS 1280 > +#define NUM_SUPPORTED_ODR 9 > + > +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, > + }, { > + .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_GPL(kx022a_regmap); > + > +struct kx022a_data; > + > +struct kx022a_trigger { Unusual structure. IIRC we normally just squash this stuff into the iio_priv structure and set the trigger devdata to the iio_dev. I haven't really thought about the advantages of this way around but can see a disadvantage is you get circular pointer loops this way. > + struct kx022a_data *data; Looks like you can get to the data pointer via a container_of() so do that instead of carrying a pointer to the containing structure. > + struct iio_trigger *indio_trig; > + bool enabled; > + const char *name; > +}; > + > +struct kx022a_data { > + int irq; > + struct regmap *regmap; > + struct kx022a_trigger trigger; > + struct device *dev; > + unsigned int g_range; > + struct mutex mutex; > + unsigned int state; > + > + int64_t timestamp, old_timestamp; > + unsigned int odr_ns; > + > + struct iio_mount_matrix orientation; > + u8 watermark; > + /* 3 x 16bit accel data + timestamp */ > + s16 buffer[8]; > + 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), \ > + .ext_info = kx022a_ext_info, \ > + .address = KX022A_REG_##axis##OUT_L, \ > + .scan_index = index, \ > + .scan_type = { \ > + .sign = 's', \ > + .realbits = 16, \ > + .storagebits = 16, \ > + .shift = 0, \ shift = 0 is 'obvious' default, so don't bother providing it explicitly. > + .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 > + * Linux CPUs can handle w/o dropping samples. Also, the low power mode is not > + * available for higher sample rates. Thus the driver only supports 200 Hz and > + * slower ODRs. Slowest being 0.78 Hz > + */ > +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("0.78 1.563 3.125 6.25 12.5 25 50 100 200"); > +static IIO_CONST_ATTR(scale_available, > + "598.550415 1197.10083 2394.20166 4788.40332"); > + > +static struct attribute *kx022a_attributes[] = { > + &iio_const_attr_sampling_frequency_available.dev_attr.attr, > + &iio_const_attr_scale_available.dev_attr.attr, Use the read_avail() callback instead of doing these as attributes. That makes the values available to consumer drivers... > + NULL, > +}; > + > +static const struct attribute_group kx022a_attrs_group = { > + .attrs = kx022a_attributes, > +}; > + > +struct kx022a_tuple { > + int val; > + int val2; > +}; > + > +/* > + * 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 struct kx022a_tuple kx022a_scale_table[] = { > + { 598, 550415 }, { 1197, 100830 }, { 2394, 201660 }, { 4788, 403320 } Burn some lines with one pair per line as it is more readable. > +}; > + > +/* > + * ODR (output data rate) table. First value represents the integer portion of > + * frequency (Hz), and the second value is the decimal part (uHz). > + * Eg, 0.78 Hz, 1.563 Hz, ... > + */ > +#define KX922A_DEFAULT_PERIOD 20000000 > +static const struct kx022a_tuple kx022a_accel_samp_freq_table[] = { > + { 0, 780000 }, { 1, 563000 }, { 3, 125000 }, { 6, 250000 }, > + { 12, 500000 }, { 25, 0 }, { 50, 0 }, { 100, 0 }, { 200, 0 } > +}; Spend the lines and have one entry pair per line as more readable. > +static const unsigned int kx022a_odrs[] = { 1282051282, 639795266, 320000000, > + 160000000, 80000000, 40000000, 20000000, 10000000, 5000000 }; where we have lots of zeros useful to use the MEGA etc defines in units.h > + > +/* Find index of tuple matching the given values */ > +static int kx022a_find_tuple_index(const struct kx022a_tuple *tuples, int n, > + int val, int val2) > +{ > + while (n-- > 0) > + if (val == tuples[n].val && tuples[n].val2 == val2) > + return n; > + > + return -EINVAL; > +} > + > +static void kx022a_reg2freq(unsigned int val, int *val1, int *val2) > +{ > + *val1 = kx022a_accel_samp_freq_table[val & KX022A_MASK_ODR].val; > + *val2 = kx022a_accel_samp_freq_table[val & KX022A_MASK_ODR].val2; > +} > + > +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].val; > + *val2 = kx022a_scale_table[val].val2; > +} > + > +static int __kx022a_turn_on_unlocked(struct kx022a_data *data) > +{ > + int ret; > + > + ret = regmap_set_bits(data->regmap, KX022A_REG_CNTL, KX022A_MASK_PC1); > + if (ret) > + dev_err(data->dev, "Turn ON fail %d\n", ret); > + > + return ret; > +} > + > +static int __kx022a_turn_off_unlocked(struct kx022a_data *data) > +{ > + int ret; > + > + ret = regmap_clear_bits(data->regmap, KX022A_REG_CNTL, KX022A_MASK_PC1); > + if (ret) > + dev_err(data->dev, "Turn OFF fail %d\n", ret); > + > + return ret; > +} > + > +static int kx022a_turn_off_lock(struct kx022a_data *data) > +{ > + int ret; As below, I'm not convinced this wrapper is worthwhile > + > + mutex_lock(&data->mutex); > + ret = __kx022a_turn_off_unlocked(data); > + if (ret) > + mutex_unlock(&data->mutex); > + > + return ret; > +} > + > +static int kx022a_turn_on_unlock(struct kx022a_data *data) > +{ > + int ret; > + This is not used enough that I can see a strong reason for the wrapper. Just put the two calls inline and rename the unlocked case. > + ret = __kx022a_turn_on_unlocked(data); > + 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; > + > + /* > + * 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). > + */ > + mutex_lock(&data->mutex); > + if (iio_buffer_enabled(idev)) { As below - do this with iio_device_claim_direct_mode() before taking the mutex. It's the standard way to avoid the races and prevent access when buffers are enabled (if they are - you can't claim direct mode). > + ret = -EBUSY; > + goto unlock_out; > + } > + switch (mask) { > + case IIO_CHAN_INFO_SAMP_FREQ: > + ret = kx022a_find_tuple_index(&kx022a_accel_samp_freq_table[0], > + ARRAY_SIZE(kx022a_accel_samp_freq_table), > + val, val2); > + /* Configure if we found valid ODR */ > + if (ret >= 0) { > + int odr = ret; > + > + ret = __kx022a_turn_off_unlocked(data); > + if (ret) > + goto unlock_out; > + > + ret = regmap_update_bits(data->regmap, KX022A_REG_ODCNTL, > + KX022A_MASK_ODR, odr); > + data->odr_ns = kx022a_odrs[odr]; > + __kx022a_turn_on_unlocked(data); > + } > + break; > + case IIO_CHAN_INFO_SCALE: > + ret = kx022a_find_tuple_index(&kx022a_scale_table[0], > + ARRAY_SIZE(kx022a_scale_table), > + val, val2); > + /* Configure if we found valid scale */ > + if (ret >= 0) { > + ret = __kx022a_turn_off_unlocked(data); > + if (ret) > + goto unlock_out; > + > + ret = regmap_update_bits(data->regmap, KX022A_REG_CNTL, > + KX022A_MASK_GSEL, > + ret << KX022A_GSEL_SHIFT); > + __kx022a_turn_on_unlocked(data); > + } > + default: > + ret = -EINVAL; > + } > + > +unlock_out: > + mutex_unlock(&data->mutex); > + > + 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_fifo_report_data(struct kx022a_data *data, void *buffer, > + int samples) > +{ I would just put this code inline at the single call site. > + int ret, fifo_bytes; > + > + fifo_bytes = samples * KX022A_FIFO_SAMPLES_SIZE_BYTES; > + ret = regmap_noinc_read(data->regmap, KX022A_REG_BUF_READ, > + buffer, fifo_bytes); > + if (ret) > + dev_err(data->dev, "FIFO read failed %d\n", ret); > + > + return ret; > +} > + > +static int kx022a_get_axis(struct kx022a_data *data, > + struct iio_chan_spec const *chan, > + int *val) > +{ > + u16 raw_val; > + int ret; > + > + ret = regmap_bulk_read(data->regmap, chan->address, &raw_val, > + sizeof(raw_val)); Bulk reads for SPI still require dma safe buffers. Long story but The short version is today regmap happens to always use bounce buffers for SPI (or it did last time I looked at it). However there is no guarantee on that in future. As such you need to use a buffer that is __aligned(IIO_DMA_MINALIGN); Easiest option is to put one at the end of the iio_priv() structure. > + if (ret) > + return ret; > + *val = raw_val; > + > + 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; > + > + mutex_lock(&data->mutex); > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + if (iio_buffer_enabled(idev)) { Claim direct mode as standard way to avoid racing in read_raw. Note you should be very careful on locking order as well. Claim direct mode first then take any other locks. > + ret = -EBUSY; > + goto error_ret; > + } > + > + ret = kx022a_get_axis(data, chan, val); > + break; > + case IIO_CHAN_INFO_SAMP_FREQ: > + { Brackets don't add anything here as not defining scope of anything. > + ret = regmap_read(data->regmap, KX022A_REG_ODCNTL, ®val); > + if (ret) > + goto error_ret; > + > + if ((regval & KX022A_MASK_ODR) > > + ARRAY_SIZE(kx022a_accel_samp_freq_table)) { > + dev_err(data->dev, "Invalid ODR\n"); > + ret = -EINVAL; > + goto error_ret; > + } > + > + 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, ®val); > + if (ret < 0) > + goto error_ret; > + > + kx022a_reg2scale(regval, val, val2); > + > + ret = IIO_VAL_INT_PLUS_MICRO; > + break; > + } > + > +error_ret: > + mutex_unlock(&data->mutex); > + > + return ret; > +}; > + > +static int kx022a_validate_trigger(struct iio_dev *idev, > + struct iio_trigger *trig) > +{ > + struct kx022a_data *data = iio_priv(idev); > + > + if (data->trigger.indio_trig == trig) > + return 0; > + > + return -EINVAL; > +} > + > +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 kx022a_get_fifo_state(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 sprintf(buf, "%d\n", state); > +} > + > +static ssize_t kx022a_get_fifo_watermark(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 sprintf(buf, "%d\n", wm); > +} > + > +static IIO_DEVICE_ATTR(hwfifo_enabled, 0444, > + kx022a_get_fifo_state, NULL, 0); > +static IIO_DEVICE_ATTR(hwfifo_watermark, 0444, > + kx022a_get_fifo_watermark, NULL, 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 > + */ > + data->timestamp = 0; > + > + return regmap_write(data->regmap, KX022A_REG_BUF_CLEAR, 0xff); > +} > + > +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); > + int ret, i; > + int count, fifo_bytes; > + u16 buffer[KX022A_FIFO_LENGTH * 3]; > + int64_t tstamp; > + uint64_t sample_period; > + static int err_ctr; > + > + ret = regmap_read(data->regmap, KX022A_REG_BUF_STATUS_1, &fifo_bytes); > + if (ret < 0) { > + dev_err(dev, "Error reading buffer status\n"); > + return ret; > + } > + > + /* Let's not owerflow if we for some reason get bogus value from i2c */ overflow. Make sure to spell check. > + if (fifo_bytes > KX022A_FIFO_MAX_BYTES) { > + /* > + * I've observed a strange behaviour where FIFO may get stuck if > + * samples are not read out fast enough. By 'stuck' I mean > + * situation where amount of data adverticed by the STATUS_1 > + * reg is 255 - which equals to 42,5 (sic!) samples and by > + * my experimenting there are situations where reading the > + * FIFO buffer does not decrease the data count but the same > + * fifo sample level (255 bytes of data) is reported > + */ > + err_ctr++; > + dev_warn(data->dev, "Bad amount of data %u\n", fifo_bytes); > + fifo_bytes = KX022A_FIFO_MAX_BYTES; > + } else if (fifo_bytes % KX022A_FIFO_SAMPLES_SIZE_BYTES) { > + err_ctr++; > + dev_err(data->dev, "Bad FIFO alignment. Data may be corrupt\n"); > + } else { > + err_ctr = 0; > + } > + > + if (err_ctr > KXO22A_FIFO_ERR_THRESHOLD) { > + __kx022a_turn_off_unlocked(data); > + kx022a_drop_fifo_contents(data); > + __kx022a_turn_on_unlocked(data); > + > + err_ctr = 0; > + > + return -EINVAL; > + } > + > + 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) { > + 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; > + } > + > + ret = kx022a_fifo_report_data(data, buffer, count); > + if (ret) > + return ret; > + > + 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; > + } > + > + return count; > +} > + > +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, > + .attrs = &kx022a_attrs_group, > + > + .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 */ Must either handle both pins or at least know if it is irq2 and treat that as no irq for now. > + int mask = KX022A_MASK_IEN1 | KX022A_MASK_IPOL1 | > + KX022A_MASK_ITYP; > + int val = KX022A_MASK_IEN1 | KX022A_IPOL_LOW | > + KX022A_ITYP_LEVEL; > + int ret; > + > + ret = regmap_update_bits(data->regmap, KX022A_REG_INC1, mask, val); > + if (ret) > + return ret; > + > + mask = KX022A_MASK_INS2_DRDY | KX122_MASK_INS2_WMI; > + > + return regmap_set_bits(data->regmap, KX022A_REG_INC4, mask); > +} > + > +static int kx022a_fifo_disable(struct kx022a_data *data) > +{ > + int ret = 0; > + > + /* PC1 to 0 */ ? No idea what that means... > + ret = kx022a_turn_off_lock(data); > + if (ret) > + return ret; > + > + ret = regmap_clear_bits(data->regmap, KX022A_REG_INC4, > + KX022A_MASK_WMI1); > + if (ret) > + goto unlock_out; > + > + /* disable buffer */ > + 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 = 0; > + > + ret = kx022a_turn_off_lock(data); > + if (ret) > + return ret; > + > + /* Write WMI to HW */ Where WMI is? > + 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, KX022A_REG_INC4, > + KX022A_MASK_WMI1); > + 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 triggers, then the IRQs should be handled by trigger > + * enable and buffer is not used but we just add results to buffer > + * when data-ready triggers. That comment needs rewriting given you have two buffers and I think they are very different things.. > + */ > + 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; > + > + mutex_lock(&data->mutex); > + 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: > + mutex_unlock(&data->mutex); > + 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); > + bool ack = false; > + > + data->old_timestamp = data->timestamp; > + data->timestamp = iio_get_time_ns(idev); > + > + if (data->trigger.enabled) { > + iio_trigger_poll(data->trigger.indio_trig); > + ack = true; > + } > + > + if (data->state & KX022A_STATE_FIFO) > + return IRQ_WAKE_THREAD; > + > + if (ack) { > + /* > + * The IRQ is not acked until data is read. We need to disable > + * the IRQ in order to schedule the trigger thread. Enabling > + * is done in reenable. I'm a little confused at what is going on here, but a common solution to situations like this is to handling to use iio_trigger_poll_chained() from the thread and a IRQF_ONESHOT irq. Thus the interrupt will remain masked until the trigger consumers are all done. > + * > + * It would be possible to set the IRQ to 50uS pulse. If we are > + * losing data due to the disabled IRQ we can evaluate the > + * option of using edge triggered IRQs with the pulse mode. > + */ > + disable_irq_nosync(irq); > + return IRQ_HANDLED; > + } > + > + return IRQ_NONE; > +} > + > +/* Read the data from the fifo and fill IIO buffers */ > +static irqreturn_t kx022a_irq_thread_handler(int irq, void *private) > +{ > + struct iio_dev *idev = private; > + struct kx022a_data *data = iio_priv(idev); > + bool ack = false; > + int ret; > + > + mutex_lock(&data->mutex); > + > + if (data->state & KX022A_STATE_FIFO) { > + ret = __kx022a_fifo_flush(idev, KX022A_FIFO_LENGTH, true); > + if (ret > 0) > + ack = true; > + } > + /* > + * 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). I'm not even going to ask for more info at this stage but that sound worrying. > + */ > + if (ack) > + ret = IRQ_HANDLED; > + else > + ret = IRQ_NONE; > + > + mutex_unlock(&data->mutex); > + > + return ret; > +} > + > +static int kx022a_trigger_set_state(struct iio_trigger *trig, > + bool state) > +{ > + struct kx022a_trigger *t = iio_trigger_get_drvdata(trig); > + struct kx022a_data *data = t->data; > + int ret = 0; > + > + mutex_lock(&data->mutex); > + > + if (t->enabled == state) > + goto unlock_out; > + > + ret = __kx022a_turn_off_unlocked(data); > + if (ret) > + goto unlock_out; > + > + t->enabled = state; > + ret = kx022a_set_drdy_irq(data, state); > + if (ret) > + goto unlock_out; > + > + ret = __kx022a_turn_on_unlocked(data); > + > +unlock_out: > + mutex_unlock(&data->mutex); > + > + return ret; > +} blank line between functions. > +static void kx022a_trig_reen(struct iio_trigger *trig) > +{ > + struct kx022a_trigger *t = iio_trigger_get_drvdata(trig); Mentioned above, but I really don't like the pointer loops between the trigger and main _data structure. > + struct kx022a_data *data = t->data; > + > + enable_irq(data->irq); > +} > + > +static const struct iio_trigger_ops kx022a_trigger_ops = { > + .set_trigger_state = kx022a_trigger_set_state, > + .reenable = kx022a_trig_reen, > +}; > + > +static int kx022a_chip_init(struct kx022a_data *data) > +{ > + int ret, dummy; > + > + /* > + * Disable IRQs because if the IRQs are left on (for example by > + * a shutdown which did not deactivate the accelerometer) we do > + * most probably end up flooding the system with unhandled IRQs > + * and get the line disabled from SOC side. > + */ > + ret = regmap_write(data->regmap, KX022A_REG_INC4, 0); Unusual to do this rather than a reset. Quick look suggests there is a suitable software reset (CNTL2) > + if (ret) { > + dev_err(data->dev, "Failed to init IRQ states\n"); > + return ret; > + } > + > + ret = kx022a_set_drdy_irq(data, false); > + if (ret) { > + dev_err(data->dev, "Failed to init DRDY\n"); > + return ret; > + } > + > + /* Clear any pending IRQs */ > + ret = regmap_read(data->regmap, KX022A_REG_INT_REL, &dummy); > + if (ret) { > + dev_err(data->dev, "Failed to ACK IRQs\n"); > + return ret; > + } > + /* set data res 16bit */ > + ret = regmap_set_bits(data->regmap, KX022A_REG_BUF_CNTL2, > + KX022A_MASK_BRES); Can we have a _BRES_16 to make this self documenting? > + if (ret) { > + dev_err(data->dev, "Failed to set data resolution\n"); > + return ret; > + } > + > + ret = kx022a_prepare_irq_pin(data); > + if (ret) { > + dev_err(data->dev, "Failed to configure IRQ pin\n"); > + return ret; > + } > + > + /* disable buffer */ consistent capitalisation of comments... > + ret = regmap_clear_bits(data->regmap, KX022A_REG_BUF_CNTL2, > + KX022A_MASK_BUF_EN); > + if (ret) > + return ret; > + > + return kx022a_drop_fifo_contents(data); > +} > + > +int kx022a_probe_internal(struct device *dev, int irq) > +{ > + static const char * const regulator_names[] = {"io_vdd", "vdd"}; > + struct iio_trigger *indio_trig; > + struct kx022a_data *data; > + struct regmap *regmap; > + unsigned int chip_id; > + struct iio_dev *idev; > + int ret; > + > + if (WARN_ON(!dev)) > + return -ENODEV; > + > + regmap = dev_get_regmap(dev, NULL); > + if (!regmap) { > + dev_err(dev, "no regmap\n"); Use dev_err_probe() for all dev_err() stuff in probe paths. It ends up cleaner and we don't care about the tiny overhead of checking for deferred. > + > + 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 > + * 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) { > + dev_err(dev, "Failed to access sensor\n"); > + return ret; > + } > + > + if (chip_id != KX022A_ID) { > + dev_err(dev, "unsupported device 0x%x\n", chip_id); > + return -EINVAL; > + } > + > + data->regmap = regmap; > + data->dev = dev; > + data->irq = irq; > + data->odr_ns = KX922A_DEFAULT_PERIOD; > + 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) > + return ret; > + > + ret = kx022a_turn_on_unlock(data); > + if (ret) > + return ret; > + > + udelay(100); > + > + ret = iio_triggered_buffer_setup_ext(idev, devm_ > + &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->trigger.indio_trig = indio_trig; > + > + indio_trig->ops = &kx022a_trigger_ops; > + data->trigger.data = data; > + iio_trigger_set_drvdata(indio_trig, &data->trigger); > + > + ret = iio_trigger_register(indio_trig); Don't mix devm and non devm allocation / registration. > + 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"); > + > + ret = devm_request_threaded_irq(data->dev, irq, kx022a_irq_handler, > + &kx022a_irq_thread_handler, > + IRQF_ONESHOT, "kx022", idev); > + if (ret) > + return dev_err_probe(data->dev, ret, "Could not request IRQ\n"); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(kx022a_probe_internal); > + > +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..ec89e46c6ca8 > --- /dev/null > +++ b/drivers/iio/accel/kionix-kx022a.h > @@ -0,0 +1,76 @@ ... > +#define KX022A_REG_INC4 0x1f > +#define KX022A_MASK_WMI1 BIT(5) > + > +#define KX022A_REG_SELF_TEST 0x60 > +#define KX022A_MAX_REGISTER 0x60 > + Use a forward definition for struct device rather than including the header. > +int kx022a_probe_internal(struct device *dev, int irq); > +extern const struct regmap_config kx022a_regmap; > + > +#endif
On 9/22/22 20:03, Jonathan Cameron wrote: > On Wed, 21 Sep 2022 14:45:35 +0300 > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > >> KX022A is a 3-axis Accelerometer from ROHM/Kionix. The senor 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 fatures. >> >> 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. >> >> NOTE: Filling-up the hardware FIFO should be avoided. During my testing >> I noticed that filling up the hardware FIFO might mess-up the sample >> count. My sensor ended up in a state where amount of data in FIFO was >> reported to be 0xff bytes, which equals to 42,5 samples. Specification >> says the FIFO can hold maximum of 41 samples in HiRes mode. Also, at >> least once the FIFO was stuck in a state where reading data from >> hwardware FIFO did not decrease the amount of data reported to be in the > spell check this. > >> FIFO - eg. FIFO was "stuck". The code has now an error count and 10 >> reads with invalid FIFO data count will cause the fifo contents to be >> dropped. > Ouch - that's nasty. Indeed it is. As this commit states, this is pretty initial support for the accelerometer. I want to enable people to do basic experimenting and also use the component to some slow ODR solutions. Besides, having even a basic support in-tree enable people to add further improvements :) So, I am hoping / expecting to see improvements added also by other people using this. I think that after this initial support many people still _need_ for example the runtime PM. Maybe we will also end up with some nicer solution to the FIFO issues. Oh, please note that the patch is no longer complete. I've "snipped" unrelated pieces when replying to Jonathan. I did also just silently ignore the comments which I simply agree with. >> >> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> > > Hi Matti, > > A somewhat superficial review as I'm short on time, but wanted to get some > initial comments out to you as I'd started reviewing this yesterday and not > sure when I'd get back to it. No problem Jonathan. I appreciate the feedback - and even if the review has not been completed the initial feedback will already allow me to rework :) I won't go anywhere so you can for sure send me more comments during the coming new iterations. Besides, I appreciate your engagement even more as some maintainers do not have the time to look at the RFC patches at all. Getting even initial feedback this quickly is much more than I expected! So, thanks! >> +config IIO_KX022A_SPI >> + tristate "Kionix KX022A tri-axis digital accelerometer" >> + depends on I2C >> + select IIO_KX022A >> + select REGMAP_SPI >> + help >> + Say Y here to enable support for the Kionix KX022A digital tri-axis >> + accelerometer connected to I2C interface. See IIO_KX022A_I2C if >> + you want to enable support for the KX022A connected via I2C. > > I would not bother with the cross reference to the I2C config element. I thought it might help avoid some confusion - but can be dropped. >> +// SPDX-License-Identifier: GPL-2.0-only >> +// > > Other than where required for SPDX use the > /* > * Copyright... > */ > syntax. No problem. Some maintainers prefer having the top of the file comment block looking coherent and it also pleases my eye more. Still, can be changed to suit the IIO-style. > >> +// Copyright (C) 2022 ROHM Semiconductors >> +// >> +// ROHM/KIONIX KX022A accelerometer driver >> + >> +static int kx022a_i2c_probe(struct i2c_client *i2c) >> +{ >> + struct regmap *regmap; >> + struct device *dev = &i2c->dev; >> + >> + if (!i2c->irq) { > > There look to be 2 interrupt lines, so you need to know which > one this is. fwnode_irq_get_byname() trying first int1 then > int2 (as only a single one might be mapped). Seems like you know the Kionix component(s). Cool! My idea is initially to only support a fixed configuration with IRQ connected to the IRQ1 pin. The support for the second IRQ can be added when needed. I think it's better to have at first something working and add features iteratively when needed. I do also believe that people who add support to IRQ2-pin should also be testing this. I have currently only IRQ1 connected. Would a comment explaining this do? You made me think that this should probably be explained in the dt-bindings already. I need to add the irq-names and mention that if the irq-names is not populated it is assumed (only) the IRQ1 is used. >> +static struct i2c_driver kx022a_i2c_driver = { >> + .driver = { >> + .name = "kx022a-i2c", >> + .of_match_table = kx022a_of_match, >> + }, >> + .probe_new = kx022a_i2c_probe, > > I'd avoid tab alignment like this. It breaks far to often > and doesn't really help readability. Yep. I already fixed this in my local tree yesterday :) Will carry the fix to the v2 :) >> + >> +struct kx022a_data; >> + >> +struct kx022a_trigger { > > Unusual structure. IIRC we normally just squash this stuff into > the iio_priv structure and set the trigger devdata to the iio_dev. > I haven't really thought about the advantages of this way around > but can see a disadvantage is you get circular pointer loops > this way. I'd better admit I didn't invent this myself. I stole big parts of the logic from the bmc150-accel-core.c which supports multiple triggers. (By the way, I should probably metion that in the commit message). All triggers are in an array - so if you only get the trigger pointer to one of the triggers in the array, then calculating the start of the containing struct becomes non trivial. This does not apply to this driver now as we only support the data-ready trigger so I can change this. OTOH, reading the list of features KX022A hardware provides - I wouldn't be surprized to see a new trigger(s) in the future. It'd be nice to get support for different modes like for example capturing N events that were stored prior certain movement. These might be best implemented with triggers. What is your adviece - is it worth trying to keep adding new trigger(s) as a low-hanging fruit - or is it just better to make the current (limited) implementation as simple as possible? >> + struct kx022a_data *data; > > Looks like you can get to the data pointer via a container_of() > so do that instead of carrying a pointer to the containing structure. True. As long as I have just one trigger... (and yes, we have just one right now). >> +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("0.78 1.563 3.125 6.25 12.5 25 50 100 200"); >> +static IIO_CONST_ATTR(scale_available, >> + "598.550415 1197.10083 2394.20166 4788.40332"); >> + >> +static struct attribute *kx022a_attributes[] = { >> + &iio_const_attr_sampling_frequency_available.dev_attr.attr, >> + &iio_const_attr_scale_available.dev_attr.attr, > > Use the read_avail() callback instead of doing these as attributes. > That makes the values available to consumer drivers... Thanks for the guidance. My first IIO driver => all help much appreciated :) >> +static const unsigned int kx022a_odrs[] = { 1282051282, 639795266, 320000000, >> + 160000000, 80000000, 40000000, 20000000, 10000000, 5000000 }; > > where we have lots of zeros useful to use the MEGA etc defines in units.h One of the best things in upstream collaboration is always learning new (to me) things :) I'll check then out. Thanks! >> + >> +static int kx022a_turn_off_lock(struct kx022a_data *data) >> +{ >> + int ret; > > As below, I'm not convinced this wrapper is worthwhile > >> + >> + mutex_lock(&data->mutex); >> + ret = __kx022a_turn_off_unlocked(data); >> + if (ret) >> + mutex_unlock(&data->mutex); >> + >> + return ret; >> +} >> + >> +static int kx022a_turn_on_unlock(struct kx022a_data *data) >> +{ >> + int ret; >> + > This is not used enough that I can see a strong reason for the > wrapper. Just put the two calls inline and rename the unlocked case. > I kind of see the value of the wrappers as it documents the purpose of locking. It's kind of a wake-up that we really want to keep the access locked during sensor 'shut-down'. I don't know how other sensors work but for me the Kionix sensors requiring the sensor to be "turned off" for the duration of pretty much _any_ configurations was a surprize... Again, I think this will be required for _all_ new configuration code we add in the driver. >> + ret = __kx022a_turn_on_unlocked(data); >> + mutex_unlock(&data->mutex); >> + >> + return ret; >> +} >> + >> + /* >> + * 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). >> + */ >> + mutex_lock(&data->mutex); >> + if (iio_buffer_enabled(idev)) { > > As below - do this with iio_device_claim_direct_mode() before taking the mutex. > It's the standard way to avoid the races and prevent access when buffers are enabled > (if they are - you can't claim direct mode). Thanks! I'll see the iio_device_claim_direct_mode() - again, appreciate your advices! >> + >> +static int kx022a_fifo_report_data(struct kx022a_data *data, void *buffer, >> + int samples) >> +{ > > I would just put this code inline at the single call site. Eg, meld this in the fifo flushing function? Ok. > >> + int ret, fifo_bytes; >> + >> + fifo_bytes = samples * KX022A_FIFO_SAMPLES_SIZE_BYTES; >> + ret = regmap_noinc_read(data->regmap, KX022A_REG_BUF_READ, >> + buffer, fifo_bytes); >> + if (ret) >> + dev_err(data->dev, "FIFO read failed %d\n", ret); >> + >> + return ret; >> +} >> + >> +static int kx022a_get_axis(struct kx022a_data *data, >> + struct iio_chan_spec const *chan, >> + int *val) >> +{ >> + u16 raw_val; >> + int ret; >> + >> + ret = regmap_bulk_read(data->regmap, chan->address, &raw_val, >> + sizeof(raw_val)); > > Bulk reads for SPI still require dma safe buffers. Long story but > The short version is today regmap happens to always use bounce buffers > for SPI (or it did last time I looked at it). However there is no > guarantee on that in future. As such you need to use a buffer > that is __aligned(IIO_DMA_MINALIGN); > Easiest option is to put one at the end of the iio_priv() structure. > Thanks. That's a bummer. I would prefer buffer from the stack which would be a simple way of avoiding races. OTOH, the access is serialized so there should be no problem with your suggestion of using buffer from iio_priv. I'll fix this. >> + if (ret) >> + return ret; >> + *val = raw_val; >> + >> + 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; >> + >> + mutex_lock(&data->mutex); >> + switch (mask) { >> + case IIO_CHAN_INFO_RAW: >> + if (iio_buffer_enabled(idev)) { > > Claim direct mode as standard way to avoid racing in read_raw. > Note you should be very careful on locking order as well. > Claim direct mode first then take any other locks. > Thanks again. >> + if (fifo_bytes > KX022A_FIFO_MAX_BYTES) { >> + /* >> + * I've observed a strange behaviour where FIFO may get stuck if >> + * samples are not read out fast enough. By 'stuck' I mean >> + * situation where amount of data adverticed by the STATUS_1 >> + * reg is 255 - which equals to 42,5 (sic!) samples and by >> + * my experimenting there are situations where reading the >> + * FIFO buffer does not decrease the data count but the same >> + * fifo sample level (255 bytes of data) is reported >> + */ >> + err_ctr++; >> + dev_warn(data->dev, "Bad amount of data %u\n", fifo_bytes); >> + fifo_bytes = KX022A_FIFO_MAX_BYTES; >> + } else if (fifo_bytes % KX022A_FIFO_SAMPLES_SIZE_BYTES) { >> + err_ctr++; >> + dev_err(data->dev, "Bad FIFO alignment. Data may be corrupt\n"); >> + } else { >> + err_ctr = 0; >> + } >> + >> + if (err_ctr > KXO22A_FIFO_ERR_THRESHOLD) { >> + __kx022a_turn_off_unlocked(data); >> + kx022a_drop_fifo_contents(data); >> + __kx022a_turn_on_unlocked(data); >> + >> + err_ctr = 0; >> + >> + return -EINVAL; >> + } >> + >> + 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) { >> + 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; Just a note not directly related to this driver. I borrowed (stoled) the timestamp logic from the bmc150-accel-core.c. The bmc150-core did not have this timestamp adjustment which I think is necessary in case the fifo is not read empty. Someone familiar with the bmc150-accel-core.c might want to check this. >> + } >> + >> + ret = kx022a_fifo_report_data(data, buffer, count); >> + if (ret) >> + return ret; >> + >> + 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; >> + } >> + >> + return count; >> +} >> + >> +static int kx022a_prepare_irq_pin(struct kx022a_data *data) >> +{ >> + /* Enable IRQ1 pin. Set polarity to active low */ > > Must either handle both pins or at least know if it is irq2 and > treat that as no irq for now. I don't want to try supporting both pins for now. It makes this somewhat more complex - especially if we want to support using two IRQs. That will require some thorough thinking which I don't have time to do right now :( I can add the irq-names to the bindings and add check to the probe so that if the irq2 is used we error out with nice 'not supported' message. > >> + int mask = KX022A_MASK_IEN1 | KX022A_MASK_IPOL1 | >> + KX022A_MASK_ITYP; >> + int val = KX022A_MASK_IEN1 | KX022A_IPOL_LOW | >> + KX022A_ITYP_LEVEL; >> + int ret; >> + >> + ret = regmap_update_bits(data->regmap, KX022A_REG_INC1, mask, val); >> + if (ret) >> + return ret; >> + >> + mask = KX022A_MASK_INS2_DRDY | KX122_MASK_INS2_WMI; >> + >> + return regmap_set_bits(data->regmap, KX022A_REG_INC4, mask); >> +} >> + >> +static int kx022a_fifo_disable(struct kx022a_data *data) >> +{ >> + int ret = 0; >> + >> + /* PC1 to 0 */ > ? No idea what that means... Sorry about confusing print. The sensor state control bit is named as PC1 in the data-sheet and almost every single register table refers to the "PC1 bit" because pretty much nothing can be configured when sensor is running. I still agree this comment is terrible for anyone not reading the data-sheet. Thanks for pointing it out - I'll either drop it or make it more obvious... > >> + ret = kx022a_turn_off_lock(data); >> + if (ret) >> + return ret; >> + >> + ret = regmap_clear_bits(data->regmap, KX022A_REG_INC4, >> + KX022A_MASK_WMI1); >> + if (ret) >> + goto unlock_out; >> + >> + /* disable buffer */ >> + 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_fifo_enable(struct kx022a_data *data) >> +{ >> + int ret = 0; >> + >> + ret = kx022a_turn_off_lock(data); >> + if (ret) >> + return ret; >> + >> + /* Write WMI to HW */ > > Where WMI is? Stored in the iio_priv. data->watermark. The kx022a_set_watermark() only updates the value in iio_priv() and we take it in use only when the buffer is enabled. >> + >> +static int kx022a_buffer_postenable(struct iio_dev *idev) >> +{ >> + struct kx022a_data *data = iio_priv(idev); >> + >> + /* >> + * If we use triggers, then the IRQs should be handled by trigger >> + * enable and buffer is not used but we just add results to buffer >> + * when data-ready triggers. > > That comment needs rewriting given you have two buffers and I think they > are very different things.. True. Sometimes it just requires someone else to read your comments to really see the obfuscations like this one. Thanks for pointing it out. >> + >> +/* 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); >> + bool ack = false; >> + >> + data->old_timestamp = data->timestamp; >> + data->timestamp = iio_get_time_ns(idev); >> + >> + if (data->trigger.enabled) { >> + iio_trigger_poll(data->trigger.indio_trig); >> + ack = true; >> + } >> + >> + if (data->state & KX022A_STATE_FIFO) >> + return IRQ_WAKE_THREAD; >> + >> + if (ack) { >> + /* >> + * The IRQ is not acked until data is read. We need to disable >> + * the IRQ in order to schedule the trigger thread. Enabling >> + * is done in reenable. > > I'm a little confused at what is going on here, but a common solution to situations > like this is to handling to use iio_trigger_poll_chained() from the thread and > a IRQF_ONESHOT irq. Thus the interrupt will remain masked until the trigger > consumers are all done. Oh, right. This is again probably because of my lack of experience with IIO. I did just use the iio_trigger_poll() from the handler - which in turn ends up to separate thread for handling. Eg, handing was not done in the context of the irq thread => IRQF_ONESHOT did not keep irq disabled for the duration of the thread scheduled by the iio_trigger_poll(). Thus I disabled the IRQ so that the thread scheduled by iio_trigger_poll() was able to run and read the sample. I will revise this with the iio_trigger_poll_chained() - hopefully I can make this nicer with it! Thanks > >> + * >> + * It would be possible to set the IRQ to 50uS pulse. If we are >> + * losing data due to the disabled IRQ we can evaluate the >> + * option of using edge triggered IRQs with the pulse mode. >> + */ >> + disable_irq_nosync(irq); >> + return IRQ_HANDLED; >> + } >> + >> + return IRQ_NONE; >> +} >> + >> +/* Read the data from the fifo and fill IIO buffers */ >> +static irqreturn_t kx022a_irq_thread_handler(int irq, void *private) >> +{ >> + struct iio_dev *idev = private; >> + struct kx022a_data *data = iio_priv(idev); >> + bool ack = false; >> + int ret; >> + >> + mutex_lock(&data->mutex); >> + >> + if (data->state & KX022A_STATE_FIFO) { >> + ret = __kx022a_fifo_flush(idev, KX022A_FIFO_LENGTH, true); >> + if (ret > 0) >> + ack = true; >> + } >> + /* >> + * 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). > > I'm not even going to ask for more info at this stage but that sound worrying. Yep, you didn't ask but I can still babble a bit :) It's just about the fact that this (and many other) Kionix sensors clear all IRQs when a single bit of a single register is written. This naturally is racy because there will always be a time-window for new IRQs to trigger between reading the active IRQs and writing the ack. So with this acking mechanism we do potentially lose IRQs. It might be there were separate ack registers for both of the IRQ pins - meaning we might be able to use separate pins for separate (ackable) IRQs - and be able to support two IRQs without an acking race. This however would require some logic for which pin to use for which IRQ - and potentially this selection could be HW dependant. Currently I am not supporting anything but watermark and data-ready, both of which get cleaned when data is read. It's still worth mentioning that care must be taken if ackable IRQs are to be added... > >> + */ >> + if (ack) >> + ret = IRQ_HANDLED; >> + else >> + ret = IRQ_NONE; >> + >> + mutex_unlock(&data->mutex); >> + >> + return ret; >> +} >> + >> +static void kx022a_trig_reen(struct iio_trigger *trig) >> +{ >> + struct kx022a_trigger *t = iio_trigger_get_drvdata(trig); > > Mentioned above, but I really don't like the pointer loops between > the trigger and main _data structure. > Yep. This is also something that remained from supporting an array of triggers. >> + struct kx022a_data *data = t->data; >> + >> + enable_irq(data->irq); >> +} >> + >> +static const struct iio_trigger_ops kx022a_trigger_ops = { >> + .set_trigger_state = kx022a_trigger_set_state, >> + .reenable = kx022a_trig_reen, >> +}; >> + >> +static int kx022a_chip_init(struct kx022a_data *data) >> +{ >> + int ret, dummy; >> + >> + /* >> + * Disable IRQs because if the IRQs are left on (for example by >> + * a shutdown which did not deactivate the accelerometer) we do >> + * most probably end up flooding the system with unhandled IRQs >> + * and get the line disabled from SOC side. >> + */ >> + ret = regmap_write(data->regmap, KX022A_REG_INC4, 0); > > Unusual to do this rather than a reset. Quick look suggests there is > a suitable software reset (CNTL2) Same thing was just told me by a colleague of mine yesterday. Reset simplifies this quite a bit. (I just wonder if we can trust the reset always initializes the IC to same defaults or if there may be OTP differencies. I am new to these sensors). >> + if (ret) { >> + dev_err(data->dev, "Failed to init IRQ states\n"); >> + return ret; >> + } >> + >> + ret = kx022a_set_drdy_irq(data, false); >> + if (ret) { >> + dev_err(data->dev, "Failed to init DRDY\n"); >> + return ret; >> + } >> + >> + /* Clear any pending IRQs */ >> + ret = regmap_read(data->regmap, KX022A_REG_INT_REL, &dummy); >> + if (ret) { >> + dev_err(data->dev, "Failed to ACK IRQs\n"); >> + return ret; >> + } >> + /* set data res 16bit */ >> + ret = regmap_set_bits(data->regmap, KX022A_REG_BUF_CNTL2, >> + KX022A_MASK_BRES); > > Can we have a _BRES_16 to make this self documenting? Yes we can :) >> + >> + ret = iio_triggered_buffer_setup_ext(idev, > > devm_ Ouch. Yes. > >> + &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->trigger.indio_trig = indio_trig; >> + >> + indio_trig->ops = &kx022a_trigger_ops; >> + data->trigger.data = data; >> + iio_trigger_set_drvdata(indio_trig, &data->trigger); >> + >> + ret = iio_trigger_register(indio_trig); > > Don't mix devm and non devm allocation / registration. Ouch2. I know and should have seen this. Thanks for pointing it out. > >> + 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"); >> + >> + ret = devm_request_threaded_irq(data->dev, irq, kx022a_irq_handler, >> + &kx022a_irq_thread_handler, >> + IRQF_ONESHOT, "kx022", idev); >> + if (ret) >> + return dev_err_probe(data->dev, ret, "Could not request IRQ\n"); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(kx022a_probe_internal); Best Regards -- Matti
On Fri, 23 Sep 2022 09:31:12 +0300 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > On 9/22/22 20:03, Jonathan Cameron wrote: > > On Wed, 21 Sep 2022 14:45:35 +0300 > > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > > > >> KX022A is a 3-axis Accelerometer from ROHM/Kionix. The senor 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 fatures. > >> > >> 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. > >> > >> NOTE: Filling-up the hardware FIFO should be avoided. During my testing > >> I noticed that filling up the hardware FIFO might mess-up the sample > >> count. My sensor ended up in a state where amount of data in FIFO was > >> reported to be 0xff bytes, which equals to 42,5 samples. Specification > >> says the FIFO can hold maximum of 41 samples in HiRes mode. Also, at > >> least once the FIFO was stuck in a state where reading data from > >> hwardware FIFO did not decrease the amount of data reported to be in the > > spell check this. > > > >> FIFO - eg. FIFO was "stuck". The code has now an error count and 10 > >> reads with invalid FIFO data count will cause the fifo contents to be > >> dropped. > > Ouch - that's nasty. > > Indeed it is. As this commit states, this is pretty initial support for > the accelerometer. I want to enable people to do basic experimenting and > also use the component to some slow ODR solutions. Besides, having even > a basic support in-tree enable people to add further improvements :) So, > I am hoping / expecting to see improvements added also by other people > using this. I think that after this initial support many people still > _need_ for example the runtime PM. Maybe we will also end up with some > nicer solution to the FIFO issues. My initial gut feeling is that, if that fifo issue doesn't have a clean solution, we don't don't support the fifo (by default anyway - could have a module parameter or something to turn it on). Late handling of interrupts is something that can happen for lots of reasons. It should never be fatal! > > Oh, please note that the patch is no longer complete. I've "snipped" > unrelated pieces when replying to Jonathan. I did also just silently > ignore the comments which I simply agree with. Exactly as it should be! Thanks for taking the time to do that. > > >> > >> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> > > > > Hi Matti, > > > > A somewhat superficial review as I'm short on time, but wanted to get some > > initial comments out to you as I'd started reviewing this yesterday and not > > sure when I'd get back to it. > > No problem Jonathan. I appreciate the feedback - and even if the review > has not been completed the initial feedback will already allow me to > rework :) I won't go anywhere so you can for sure send me more comments > during the coming new iterations. > > Besides, I appreciate your engagement even more as some maintainers do > not have the time to look at the RFC patches at all. Getting even > initial feedback this quickly is much more than I expected! So, thanks! I only get to RFCs because of the help others give with review of IIO patches which means I'm not always completely snowed under! > >> +// SPDX-License-Identifier: GPL-2.0-only > >> +// > > > > Other than where required for SPDX use the > > /* > > * Copyright... > > */ > > syntax. > > No problem. Some maintainers prefer having the top of the file comment > block looking coherent and it also pleases my eye more. Still, can be > changed to suit the IIO-style. IIO style well predates SPDX in the kernel (and maybe in general!) :) > > > > >> +// Copyright (C) 2022 ROHM Semiconductors > >> +// > >> +// ROHM/KIONIX KX022A accelerometer driver > >> + > > >> +static int kx022a_i2c_probe(struct i2c_client *i2c) > >> +{ > >> + struct regmap *regmap; > >> + struct device *dev = &i2c->dev; > >> + > >> + if (!i2c->irq) { > > > > There look to be 2 interrupt lines, so you need to know which > > one this is. fwnode_irq_get_byname() trying first int1 then > > int2 (as only a single one might be mapped). > > Seems like you know the Kionix component(s). Last one I had was a kxsd9 which was over 10 years ago! Just get used to looking through datasheets for things that we have gotten wrong before :) > Cool! My idea is initially > to only support a fixed configuration with IRQ connected to the IRQ1 > pin. The support for the second IRQ can be added when needed. I think > it's better to have at first something working and add features > iteratively when needed. I do also believe that people who add support > to IRQ2-pin should also be testing this. I have currently only IRQ1 > connected. > > Would a comment explaining this do? Hmm. Binding still needs to support either interrupt, but if the driver doesn't that's just a feature to add. Normally it's pretty simple to do irq1 or irq2 though... irq1 and irq2 is of course much harder! > > You made me think that this should probably be explained in the > dt-bindings already. I need to add the irq-names and mention that if the > irq-names is not populated it is assumed (only) the IRQ1 is used. New binding. Just insist that interrupt-names is provided :) Makes for simpler code and would be good practice anyway I think. > ... > > >> + > >> +struct kx022a_data; > >> + > >> +struct kx022a_trigger { > > > > Unusual structure. IIRC we normally just squash this stuff into > > the iio_priv structure and set the trigger devdata to the iio_dev. > > I haven't really thought about the advantages of this way around > > but can see a disadvantage is you get circular pointer loops > > this way. > > I'd better admit I didn't invent this myself. I stole big parts of the > logic from the bmc150-accel-core.c which supports multiple triggers. (By > the way, I should probably metion that in the commit message). All > triggers are in an array - so if you only get the trigger pointer to one > of the triggers in the array, then calculating the start of the > containing struct becomes non trivial. > > This does not apply to this driver now as we only support the data-ready > trigger so I can change this. OTOH, reading the list of features KX022A > hardware provides - I wouldn't be surprized to see a new trigger(s) in > the future. It'd be nice to get support for different modes like for > example capturing N events that were stored prior certain movement. That stuff is hard to do in a generic enough way that standard software can support it. Still I'd be interested to see a proposal. It's a subset of what impact sensors tend to do. > These might be best implemented with triggers. What is your adviece - is > it worth trying to keep adding new trigger(s) as a low-hanging fruit - > or is it just better to make the current (limited) implementation as > simple as possible? Simple first. Different matter if you add the other triggers later in the same patch series, but history says half the interesting features we plan to add never get added. > > >> +static int kx022a_turn_on_unlock(struct kx022a_data *data) > >> +{ > >> + int ret; > >> + > > This is not used enough that I can see a strong reason for the > > wrapper. Just put the two calls inline and rename the unlocked case. > > > > I kind of see the value of the wrappers as it documents the purpose of > locking. It's kind of a wake-up that we really want to keep the access > locked during sensor 'shut-down'. I don't know how other sensors work > but for me the Kionix sensors requiring the sensor to be "turned off" > for the duration of pretty much _any_ configurations was a surprize... That's unusual in remotely modern devices. It was more common some years back to have restrictions like that. Often I suspect the device was fine but no one wanted to test all the corner cases of making changes whilst it was running so they made it impossible to hit them. > Again, I think this will be required for _all_ new configuration code we > add in the driver. ok. Maybe rename the mutex to make it self describing? Mind you I can't think of a good name. interlock would probably be a bit confusing though that's what it would be called if it was on a heavy bit of machinery! > > >> + ret = __kx022a_turn_on_unlocked(data); > >> + mutex_unlock(&data->mutex); > >> + > >> + return ret; > >> +} ... ... > >> + /* > >> + * 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; > > Just a note not directly related to this driver. I borrowed (stoled) the > timestamp logic from the bmc150-accel-core.c. The bmc150-core did not > have this timestamp adjustment which I think is necessary in case the > fifo is not read empty. Someone familiar with the bmc150-accel-core.c > might want to check this. Interesting point. Probably worth sending a separate email to query this as quite likely no one who is that position is reading this thread! > > >> + } .... > >> + > >> +static int kx022a_prepare_irq_pin(struct kx022a_data *data) > >> +{ > >> + /* Enable IRQ1 pin. Set polarity to active low */ > > > > Must either handle both pins or at least know if it is irq2 and > > treat that as no irq for now. > > I don't want to try supporting both pins for now. It makes this somewhat > more complex - especially if we want to support using two IRQs. That > will require some thorough thinking which I don't have time to do right > now :( > > I can add the irq-names to the bindings and add check to the probe so > that if the irq2 is used we error out with nice 'not supported' message. > A slightly nicer thing to do is support one irq, but let it be either irq1 or irq2. If both are supplied just ignore the second one. People have an 'amusing' habit of only wiring up irq2 on boards. ... > >> +static int kx022a_fifo_enable(struct kx022a_data *data) > >> +{ > >> + int ret = 0; > >> + > >> + ret = kx022a_turn_off_lock(data); > >> + if (ret) > >> + return ret; > >> + > >> + /* Write WMI to HW */ > > > > Where WMI is? > > Stored in the iio_priv. data->watermark. The kx022a_set_watermark() only > updates the value in iio_priv() and we take it in use only when the > buffer is enabled. /* Write watermark to HW */ ? > > > > >> +/* Read the data from the fifo and fill IIO buffers */ > >> +static irqreturn_t kx022a_irq_thread_handler(int irq, void *private) > >> +{ > >> + struct iio_dev *idev = private; > >> + struct kx022a_data *data = iio_priv(idev); > >> + bool ack = false; > >> + int ret; > >> + > >> + mutex_lock(&data->mutex); > >> + > >> + if (data->state & KX022A_STATE_FIFO) { > >> + ret = __kx022a_fifo_flush(idev, KX022A_FIFO_LENGTH, true); > >> + if (ret > 0) > >> + ack = true; > >> + } > >> + /* > >> + * 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). > > > > I'm not even going to ask for more info at this stage but that sound worrying. > > Yep, you didn't ask but I can still babble a bit :) It's just about the > fact that this (and many other) Kionix sensors clear all IRQs when a > single bit of a single register is written. This naturally is racy > because there will always be a time-window for new IRQs to trigger > between reading the active IRQs and writing the ack. So with this acking > mechanism we do potentially lose IRQs. > > It might be there were separate ack registers for both of the IRQ pins - > meaning we might be able to use separate pins for separate (ackable) > IRQs - and be able to support two IRQs without an acking race. This > however would require some logic for which pin to use for which IRQ - > and potentially this selection could be HW dependant. Currently I am not > supporting anything but watermark and data-ready, both of which get > cleaned when data is read. It's still worth mentioning that care must be > taken if ackable IRQs are to be added... Fair enough. I 'love' stupid designs like this. If you want another silly interrupt example, take the PCI Data Object Exchanges spec that has caused me much woe over the last year :) For this one, probably a case of only one interrupt source at a time (or one per pin as you suggest). > > >> + struct kx022a_data *data = t->data; > >> + > >> + enable_irq(data->irq); > >> +} > >> + > >> +static const struct iio_trigger_ops kx022a_trigger_ops = { > >> + .set_trigger_state = kx022a_trigger_set_state, > >> + .reenable = kx022a_trig_reen, > >> +}; > >> + > >> +static int kx022a_chip_init(struct kx022a_data *data) > >> +{ > >> + int ret, dummy; > >> + > >> + /* > >> + * Disable IRQs because if the IRQs are left on (for example by > >> + * a shutdown which did not deactivate the accelerometer) we do > >> + * most probably end up flooding the system with unhandled IRQs > >> + * and get the line disabled from SOC side. > >> + */ > >> + ret = regmap_write(data->regmap, KX022A_REG_INC4, 0); > > > > Unusual to do this rather than a reset. Quick look suggests there is > > a suitable software reset (CNTL2) > > Same thing was just told me by a colleague of mine yesterday. Reset > simplifies this quite a bit. (I just wonder if we can trust the reset > always initializes the IC to same defaults or if there may be OTP > differencies. I am new to these sensors). > I really hope we can rely on any documented reset values! Jonathan
On 9/24/22 18:17, Jonathan Cameron wrote: > On Fri, 23 Sep 2022 09:31:12 +0300 > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > >> On 9/22/22 20:03, Jonathan Cameron wrote: >>> On Wed, 21 Sep 2022 14:45:35 +0300 >>> Matti Vaittinen <mazziesaccount@gmail.com> wrote: >>> >>>> KX022A is a 3-axis Accelerometer from ROHM/Kionix. The senor 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 fatures. >>>> >>>> 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. >>>> >>>> NOTE: Filling-up the hardware FIFO should be avoided. During my testing >>>> I noticed that filling up the hardware FIFO might mess-up the sample >>>> count. My sensor ended up in a state where amount of data in FIFO was >>>> reported to be 0xff bytes, which equals to 42,5 samples. Specification >>>> says the FIFO can hold maximum of 41 samples in HiRes mode. Also, at >>>> least once the FIFO was stuck in a state where reading data from >>>> hwardware FIFO did not decrease the amount of data reported to be in the >>> spell check this. >>> >>>> FIFO - eg. FIFO was "stuck". The code has now an error count and 10 >>>> reads with invalid FIFO data count will cause the fifo contents to be >>>> dropped. >>> Ouch - that's nasty. >> >> Indeed it is. As this commit states, this is pretty initial support for >> the accelerometer. I want to enable people to do basic experimenting and >> also use the component to some slow ODR solutions. Besides, having even >> a basic support in-tree enable people to add further improvements :) So, >> I am hoping / expecting to see improvements added also by other people >> using this. I think that after this initial support many people still >> _need_ for example the runtime PM. Maybe we will also end up with some >> nicer solution to the FIFO issues. > > My initial gut feeling is that, if that fifo issue doesn't have a clean > solution, we don't don't support the fifo (by default anyway - > could have a module parameter or something to turn it on). Late handling > of interrupts is something that can happen for lots of reasons. It should > never be fatal! Tell me about it... More than 10-years ago I "inherited" maintenance of a timing code which was built on periodic 10msec IRQ which incremented a counter. (And after that there were many new generations of Linux based devices with various "rt"-requirements). Missing an IRQ was fatal as there were no hardware where we could read the correct timestamp and other units in the system were no longer in sync if the counter was wrong. Only recovery was complete system restart for all units - which in that use case was really bad. I learned to absolutely hate the serial prints over slow UART - and I learned to love irqsoff tracer. I don't work for that company anymore and I believe the product has already retired. Yet, I still get *shrugs* when I see hard time limits for serving IRQs on Linux. (Other than that I kind of like pondering the rt-challenges). Anyways, I don't see a real risk for example when using the ODRs < 2 Hz and setting the watermark to somewhere near 20 samples. It's really unfortunate the HW has these "hickups" - but I think it is still perfectly usable for many cases - we just really need to document the corner cases somewhere. > Simple first. Different matter if you add the other triggers later in > the same patch series, but history says half the interesting features > we plan to add never get added. ack. >>> Must either handle both pins or at least know if it is irq2 and >>> treat that as no irq for now. >> >> I don't want to try supporting both pins for now. It makes this somewhat >> more complex - especially if we want to support using two IRQs. That >> will require some thorough thinking which I don't have time to do right >> now :( >> >> I can add the irq-names to the bindings and add check to the probe so >> that if the irq2 is used we error out with nice 'not supported' message. >> > > A slightly nicer thing to do is support one irq, but let it be either irq1 or > irq2. If both are supplied just ignore the second one. People have > an 'amusing' habit of only wiring up irq2 on boards. > Ok. It shouldn't be such a big change for the code. I'll see what it will look like. >>>> + >>>> +static int kx022a_chip_init(struct kx022a_data *data) >>>> +{ >>>> + int ret, dummy; >>>> + >>>> + /* >>>> + * Disable IRQs because if the IRQs are left on (for example by >>>> + * a shutdown which did not deactivate the accelerometer) we do >>>> + * most probably end up flooding the system with unhandled IRQs >>>> + * and get the line disabled from SOC side. >>>> + */ >>>> + ret = regmap_write(data->regmap, KX022A_REG_INC4, 0); >>> >>> Unusual to do this rather than a reset. Quick look suggests there is >>> a suitable software reset (CNTL2) >> >> Same thing was just told me by a colleague of mine yesterday. Reset >> simplifies this quite a bit. (I just wonder if we can trust the reset >> always initializes the IC to same defaults or if there may be OTP >> differencies. I am new to these sensors). >> > > I really hope we can rely on any documented reset values! That is a sane assumption to do - but in my experience we don't live in a sane world ;) I've seen way too many cases where the defaults are changed for ICs for example by changing OTP. And sometimes the OTP change has not been visible to the drivers from any revision registers or such. I'm not talking about Kionix sensors though as I've never worked with Kionix sensors before - so let's just try out the reset and fix things if problems emerge. I am probably just a bit paranoid :) Thanks for all the help! Yours, -- Matti
Hi Jonathan, On 9/22/22 20:03, Jonathan Cameron wrote: > On Wed, 21 Sep 2022 14:45:35 +0300 > Matti Vaittinen <mazziesaccount@gmail.com> wrote: >> + >> +/* >> + * The sensor HW can support ODR up to 1600 Hz - which is beyond what most of >> + * Linux CPUs can handle w/o dropping samples. Also, the low power mode is not >> + * available for higher sample rates. Thus the driver only supports 200 Hz and >> + * slower ODRs. Slowest being 0.78 Hz >> + */ >> +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("0.78 1.563 3.125 6.25 12.5 25 50 100 200"); >> +static IIO_CONST_ATTR(scale_available, >> + "598.550415 1197.10083 2394.20166 4788.40332"); >> + >> +static struct attribute *kx022a_attributes[] = { >> + &iio_const_attr_sampling_frequency_available.dev_attr.attr, >> + &iio_const_attr_scale_available.dev_attr.attr, > > Use the read_avail() callback instead of doing these as attributes. > That makes the values available to consumer drivers... Am I correct that populating the read_avail() does not add sysfs entries for available scale/frequency? Eg, if I wish to expose the supported values via sysfs I still need these attributes? Implementing the read_avail() as well is not a problem though. >> +static int kx022a_turn_on_unlock(struct kx022a_data *data) >> +{ >> + int ret; >> + > This is not used enough that I can see a strong reason for the > wrapper. Just put the two calls inline and rename the unlocked case. In my opinion the kx022a_turn_on_unlock() and kx022a_turn_off_lock() do simplify functions. Especially after I started using the iio_device_claim_direct_mode() :) Thus I will leave these for the v2 - please ping me again if you still want to see them removed (but I think the usage of iio_device_claim_direct_mode() changed this to favour the kx022a_turn_on_unlock() and kx022a_turn_off_lock()). >> +static int kx022a_chip_init(struct kx022a_data *data) >> +{ >> + int ret, dummy; >> + >> + /* >> + * Disable IRQs because if the IRQs are left on (for example by >> + * a shutdown which did not deactivate the accelerometer) we do >> + * most probably end up flooding the system with unhandled IRQs >> + * and get the line disabled from SOC side. >> + */ >> + ret = regmap_write(data->regmap, KX022A_REG_INC4, 0); > > Unusual to do this rather than a reset. Quick look suggests there is > a suitable software reset (CNTL2) I switched to the software reset as you suggested. I am not really convinced it is a better way. It seems the software reset requires us to re-init the regmap cache. Well, I don't think it is a bid geal though - just something worth noticing I guess. >> + >> +int kx022a_probe_internal(struct device *dev, int irq) >> +{ >> + static const char * const regulator_names[] = {"io_vdd", "vdd"}; >> + struct iio_trigger *indio_trig; >> + struct kx022a_data *data; >> + struct regmap *regmap; >> + unsigned int chip_id; >> + struct iio_dev *idev; >> + int ret; >> + >> + if (WARN_ON(!dev)) >> + return -ENODEV; >> + >> + regmap = dev_get_regmap(dev, NULL); >> + if (!regmap) { >> + dev_err(dev, "no regmap\n"); > > Use dev_err_probe() for all dev_err() stuff in probe paths. > It ends up cleaner and we don't care about the tiny overhead > of checking for deferred. This one bothers me a bit. It just does not feel correct to pass -EINVAL for the dev_err_probe() so the dev_err_probe() can check if -EINVAL != -EPROBE_DEFER. I do understand perfectly well the consistent use of dev_err_probe() for all cases where we get an error-code from a function and return it - but using dev_err_probe() when we hard-code the return value in code calling the dev_err_probe() does not feel like "the right thing to do" (tm). Eg, I agree that return dev_err_probe(dev, ret, "bar"); is nice even if we know the function that gave us the "ret" never requests defer (as that can change some day). However, I don't like issuing: return dev_err_probe(dev, -EINVAL, "bar"); Well, please let me know if you think the dev_err_probe() should be used even in cases where we hard code the return to something... For v2 I do change the other prints (like the one about failed regmap read below). > >> + >> + 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 >> + * 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) { >> + dev_err(dev, "Failed to access sensor\n"); Yours, -- Matti Vaittinen
On Wed, Sep 28, 2022 at 02:14:14PM +0300, Matti Vaittinen wrote: > On 9/22/22 20:03, Jonathan Cameron wrote: > > On Wed, 21 Sep 2022 14:45:35 +0300 ... > > > + dev_err(dev, "no regmap\n"); > > > > Use dev_err_probe() for all dev_err() stuff in probe paths. > > It ends up cleaner and we don't care about the tiny overhead > > of checking for deferred. > > This one bothers me a bit. It just does not feel correct to pass -EINVAL for > the dev_err_probe() so the dev_err_probe() can check if -EINVAL != > -EPROBE_DEFER. I do understand perfectly well the consistent use of > dev_err_probe() for all cases where we get an error-code from a function and > return it - but using dev_err_probe() when we hard-code the return value in > code calling the dev_err_probe() does not feel like "the right thing to do" > (tm). > > Eg, I agree that > return dev_err_probe(dev, ret, "bar"); > is nice even if we know the function that gave us the "ret" never requests > defer (as that can change some day). > > However, I don't like issuing: > return dev_err_probe(dev, -EINVAL, "bar"); This case specifically was added into documentation by 7065f92255bb ("driver core: Clarify that dev_err_probe() is OK even w/out -EPROBE_DEFER"). > Well, please let me know if you think the dev_err_probe() should be used > even in cases where we hard code the return to something... And this should be, of course, maintainer's decision.
Hi Andy, On 9/28/22 17:06, Andy Shevchenko wrote: > On Wed, Sep 28, 2022 at 02:14:14PM +0300, Matti Vaittinen wrote: >> On 9/22/22 20:03, Jonathan Cameron wrote: >>> On Wed, 21 Sep 2022 14:45:35 +0300 > > ... > >>>> + dev_err(dev, "no regmap\n"); >>> >>> Use dev_err_probe() for all dev_err() stuff in probe paths. >>> It ends up cleaner and we don't care about the tiny overhead >>> of checking for deferred. >> >> This one bothers me a bit. It just does not feel correct to pass -EINVAL for >> the dev_err_probe() so the dev_err_probe() can check if -EINVAL != >> -EPROBE_DEFER. I do understand perfectly well the consistent use of >> dev_err_probe() for all cases where we get an error-code from a function and >> return it - but using dev_err_probe() when we hard-code the return value in >> code calling the dev_err_probe() does not feel like "the right thing to do" >> (tm). >> >> Eg, I agree that >> return dev_err_probe(dev, ret, "bar"); >> is nice even if we know the function that gave us the "ret" never requests >> defer (as that can change some day). >> >> However, I don't like issuing: >> return dev_err_probe(dev, -EINVAL, "bar"); > > This case specifically was added into documentation by 7065f92255bb ("driver > core: Clarify that dev_err_probe() is OK even w/out -EPROBE_DEFER"). Yes. And this is exactly what I meant with: >> Eg, I agree that >> return dev_err_probe(dev, ret, "bar"); >> is nice even if we know the function that gave us the "ret" never requests >> defer There is still (in my opinion) a significant difference if we call: >> return dev_err_probe(dev, -EINVAL, "bar"); - where we really hard-code the -EINVAL as a parameter to the dev_err_probe() >> Well, please let me know if you think the dev_err_probe() should be used >> even in cases where we hard code the return to something... > > And this should be, of course, maintainer's decision. Ultimately, yes. Best Regards --Matti
On Mon, 26 Sep 2022 05:02:44 +0000 "Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com> wrote: > On 9/24/22 18:17, Jonathan Cameron wrote: > > On Fri, 23 Sep 2022 09:31:12 +0300 > > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > > > >> On 9/22/22 20:03, Jonathan Cameron wrote: > >>> On Wed, 21 Sep 2022 14:45:35 +0300 > >>> Matti Vaittinen <mazziesaccount@gmail.com> wrote: > >>> > >>>> KX022A is a 3-axis Accelerometer from ROHM/Kionix. The senor 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 fatures. > >>>> > >>>> 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. > >>>> > >>>> NOTE: Filling-up the hardware FIFO should be avoided. During my testing > >>>> I noticed that filling up the hardware FIFO might mess-up the sample > >>>> count. My sensor ended up in a state where amount of data in FIFO was > >>>> reported to be 0xff bytes, which equals to 42,5 samples. Specification > >>>> says the FIFO can hold maximum of 41 samples in HiRes mode. Also, at > >>>> least once the FIFO was stuck in a state where reading data from > >>>> hwardware FIFO did not decrease the amount of data reported to be in the > >>> spell check this. > >>> > >>>> FIFO - eg. FIFO was "stuck". The code has now an error count and 10 > >>>> reads with invalid FIFO data count will cause the fifo contents to be > >>>> dropped. > >>> Ouch - that's nasty. > >> > >> Indeed it is. As this commit states, this is pretty initial support for > >> the accelerometer. I want to enable people to do basic experimenting and > >> also use the component to some slow ODR solutions. Besides, having even > >> a basic support in-tree enable people to add further improvements :) So, > >> I am hoping / expecting to see improvements added also by other people > >> using this. I think that after this initial support many people still > >> _need_ for example the runtime PM. Maybe we will also end up with some > >> nicer solution to the FIFO issues. > > > > My initial gut feeling is that, if that fifo issue doesn't have a clean > > solution, we don't don't support the fifo (by default anyway - > > could have a module parameter or something to turn it on). Late handling > > of interrupts is something that can happen for lots of reasons. It should > > never be fatal! > > Tell me about it... More than 10-years ago I "inherited" maintenance of > a timing code which was built on periodic 10msec IRQ which incremented a > counter. (And after that there were many new generations of Linux based > devices with various "rt"-requirements). Missing an IRQ was fatal as > there were no hardware where we could read the correct timestamp and > other units in the system were no longer in sync if the counter was > wrong. Only recovery was complete system restart for all units - which > in that use case was really bad. I learned to absolutely hate the serial > prints over slow UART - and I learned to love irqsoff tracer. I don't > work for that company anymore and I believe the product has already > retired. Yet, I still get *shrugs* when I see hard time limits for > serving IRQs on Linux. (Other than that I kind of like pondering the > rt-challenges). > > Anyways, I don't see a real risk for example when using the ODRs < 2 Hz > and setting the watermark to somewhere near 20 samples. > > It's really unfortunate the HW has these "hickups" - but I think it is > still perfectly usable for many cases - we just really need to document > the corner cases somewhere. I'd rather we gated it - so by default the fifo mode isn't there and people who are really confident they want it set a module parameter or similar. Cuts down on the bug reports. > > > Simple first. Different matter if you add the other triggers later in > > the same patch series, but history says half the interesting features > > we plan to add never get added. > > ack. > > >>> Must either handle both pins or at least know if it is irq2 and > >>> treat that as no irq for now. > >> > >> I don't want to try supporting both pins for now. It makes this somewhat > >> more complex - especially if we want to support using two IRQs. That > >> will require some thorough thinking which I don't have time to do right > >> now :( > >> > >> I can add the irq-names to the bindings and add check to the probe so > >> that if the irq2 is used we error out with nice 'not supported' message. > >> > > > > A slightly nicer thing to do is support one irq, but let it be either irq1 or > > irq2. If both are supplied just ignore the second one. People have > > an 'amusing' habit of only wiring up irq2 on boards. > > > > Ok. It shouldn't be such a big change for the code. I'll see what it > will look like. > > >>>> + > >>>> +static int kx022a_chip_init(struct kx022a_data *data) > >>>> +{ > >>>> + int ret, dummy; > >>>> + > >>>> + /* > >>>> + * Disable IRQs because if the IRQs are left on (for example by > >>>> + * a shutdown which did not deactivate the accelerometer) we do > >>>> + * most probably end up flooding the system with unhandled IRQs > >>>> + * and get the line disabled from SOC side. > >>>> + */ > >>>> + ret = regmap_write(data->regmap, KX022A_REG_INC4, 0); > >>> > >>> Unusual to do this rather than a reset. Quick look suggests there is > >>> a suitable software reset (CNTL2) > >> > >> Same thing was just told me by a colleague of mine yesterday. Reset > >> simplifies this quite a bit. (I just wonder if we can trust the reset > >> always initializes the IC to same defaults or if there may be OTP > >> differencies. I am new to these sensors). > >> > > > > I really hope we can rely on any documented reset values! > > That is a sane assumption to do - but in my experience we don't live in > a sane world ;) I've seen way too many cases where the defaults are > changed for ICs for example by changing OTP. And sometimes the OTP > change has not been visible to the drivers from any revision registers > or such. > > I'm not talking about Kionix sensors though as I've never worked with > Kionix sensors before - so let's just try out the reset and fix things > if problems emerge. I am probably just a bit paranoid :) Entirely reasonable! Jonathan > > Thanks for all the help! > > Yours, > -- Matti >
On Wed, 28 Sep 2022 19:23:06 +0300 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > Hi Andy, > > On 9/28/22 17:06, Andy Shevchenko wrote: > > On Wed, Sep 28, 2022 at 02:14:14PM +0300, Matti Vaittinen wrote: > >> On 9/22/22 20:03, Jonathan Cameron wrote: > >>> On Wed, 21 Sep 2022 14:45:35 +0300 > > > > ... > > > >>>> + dev_err(dev, "no regmap\n"); > >>> > >>> Use dev_err_probe() for all dev_err() stuff in probe paths. > >>> It ends up cleaner and we don't care about the tiny overhead > >>> of checking for deferred. > >> > >> This one bothers me a bit. It just does not feel correct to pass -EINVAL for > >> the dev_err_probe() so the dev_err_probe() can check if -EINVAL != > >> -EPROBE_DEFER. I do understand perfectly well the consistent use of > >> dev_err_probe() for all cases where we get an error-code from a function and > >> return it - but using dev_err_probe() when we hard-code the return value in > >> code calling the dev_err_probe() does not feel like "the right thing to do" > >> (tm). > >> > >> Eg, I agree that > >> return dev_err_probe(dev, ret, "bar"); > >> is nice even if we know the function that gave us the "ret" never requests > >> defer (as that can change some day). > >> > >> However, I don't like issuing: > >> return dev_err_probe(dev, -EINVAL, "bar"); > > > > This case specifically was added into documentation by 7065f92255bb ("driver > > core: Clarify that dev_err_probe() is OK even w/out -EPROBE_DEFER"). > > Yes. And this is exactly what I meant with: > >> Eg, I agree that > >> return dev_err_probe(dev, ret, "bar"); > >> is nice even if we know the function that gave us the "ret" never > requests > >> defer > > There is still (in my opinion) a significant difference if we call: > >> return dev_err_probe(dev, -EINVAL, "bar"); > > - where we really hard-code the -EINVAL as a parameter to the > dev_err_probe() > > >> Well, please let me know if you think the dev_err_probe() should be used > >> even in cases where we hard code the return to something... > > > > And this should be, of course, maintainer's decision. > > Ultimately, yes. I'm not that fussed. So happy to accept code taking either view where it is hard coded in the call like this. I'd love a dev_err() that took and returned the error value though just for all those single lines of code saved. J > > Best Regards > --Matti >
On Wed, 28 Sep 2022 14:14:14 +0300 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > Hi Jonathan, > > On 9/22/22 20:03, Jonathan Cameron wrote: > > On Wed, 21 Sep 2022 14:45:35 +0300 > > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > >> + > >> +/* > >> + * The sensor HW can support ODR up to 1600 Hz - which is beyond what most of > >> + * Linux CPUs can handle w/o dropping samples. Also, the low power mode is not > >> + * available for higher sample rates. Thus the driver only supports 200 Hz and > >> + * slower ODRs. Slowest being 0.78 Hz > >> + */ > >> +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("0.78 1.563 3.125 6.25 12.5 25 50 100 200"); > >> +static IIO_CONST_ATTR(scale_available, > >> + "598.550415 1197.10083 2394.20166 4788.40332"); > >> + > >> +static struct attribute *kx022a_attributes[] = { > >> + &iio_const_attr_sampling_frequency_available.dev_attr.attr, > >> + &iio_const_attr_scale_available.dev_attr.attr, > > > > Use the read_avail() callback instead of doing these as attributes. > > That makes the values available to consumer drivers... > > Am I correct that populating the read_avail() does not add sysfs entries > for available scale/frequency? Eg, if I wish to expose the supported > values via sysfs I still need these attributes? Implementing the > read_avail() as well is not a problem though. Need to also set the relevant bit in info_mask_shared_by_xxx_avail in the channels for the sysfs files to be created by calling the read_avail() callback. When I introduced those I thought about making it mandatory to introduce them for all the info_mask_shared_by_xxx entries and not having the extra bitmap but that meant figuring out the relevant entries for a mass of stuff whenever a driver was converted from the old approach like you've used here. > > >> +static int kx022a_turn_on_unlock(struct kx022a_data *data) > >> +{ > >> + int ret; > >> + > > This is not used enough that I can see a strong reason for the > > wrapper. Just put the two calls inline and rename the unlocked case. > > In my opinion the kx022a_turn_on_unlock() and kx022a_turn_off_lock() do > simplify functions. Especially after I started using the > iio_device_claim_direct_mode() :) Thus I will leave these for the v2 - > please ping me again if you still want to see them removed (but I think > the usage of iio_device_claim_direct_mode() changed this to favour the > kx022a_turn_on_unlock() and kx022a_turn_off_lock()). Let's see how it looks in v2. > > >> +static int kx022a_chip_init(struct kx022a_data *data) > >> +{ > >> + int ret, dummy; > >> + > >> + /* > >> + * Disable IRQs because if the IRQs are left on (for example by > >> + * a shutdown which did not deactivate the accelerometer) we do > >> + * most probably end up flooding the system with unhandled IRQs > >> + * and get the line disabled from SOC side. > >> + */ > >> + ret = regmap_write(data->regmap, KX022A_REG_INC4, 0); > > > > Unusual to do this rather than a reset. Quick look suggests there is > > a suitable software reset (CNTL2) > > I switched to the software reset as you suggested. I am not really > convinced it is a better way. It seems the software reset requires us to > re-init the regmap cache. Yup, though if you've provided the reset defaults that should be quick. Jonathan
On 10/2/22 14:18, Jonathan Cameron wrote: > On Wed, 28 Sep 2022 14:14:14 +0300 > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > >> Hi Jonathan, >> >> On 9/22/22 20:03, Jonathan Cameron wrote: >>> On Wed, 21 Sep 2022 14:45:35 +0300 >>> Matti Vaittinen <mazziesaccount@gmail.com> wrote: >>>> + >>>> +/* >>>> + * The sensor HW can support ODR up to 1600 Hz - which is beyond what most of >>>> + * Linux CPUs can handle w/o dropping samples. Also, the low power mode is not >>>> + * available for higher sample rates. Thus the driver only supports 200 Hz and >>>> + * slower ODRs. Slowest being 0.78 Hz >>>> + */ >>>> +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("0.78 1.563 3.125 6.25 12.5 25 50 100 200"); >>>> +static IIO_CONST_ATTR(scale_available, >>>> + "598.550415 1197.10083 2394.20166 4788.40332"); >>>> + >>>> +static struct attribute *kx022a_attributes[] = { >>>> + &iio_const_attr_sampling_frequency_available.dev_attr.attr, >>>> + &iio_const_attr_scale_available.dev_attr.attr, >>> >>> Use the read_avail() callback instead of doing these as attributes. >>> That makes the values available to consumer drivers... >> >> Am I correct that populating the read_avail() does not add sysfs entries >> for available scale/frequency? Eg, if I wish to expose the supported >> values via sysfs I still need these attributes? Implementing the >> read_avail() as well is not a problem though. > > Need to also set the relevant bit in > info_mask_shared_by_xxx_avail in the channels for the sysfs files to be created Thanks for the help! I missed this. I'll try that :) Yours -- Matti
diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig index 35798712f811..12ad6bcb2c76 100644 --- a/drivers/iio/accel/Kconfig +++ b/drivers/iio/accel/Kconfig @@ -409,6 +409,29 @@ 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 I2C + select IIO_KX022A + select REGMAP_SPI + help + Say Y here to enable support for the Kionix KX022A digital tri-axis + accelerometer connected to I2C interface. See IIO_KX022A_I2C if + you want to enable support for the KX022A connected via I2C. + +config IIO_KX022A_I2C + tristate "Kionix KX022A tri-axis digital accelerometer" + depends on I2C + select IIO_KX022A + select REGMAP_I2C + help + Say Y here to enable support for the Kionix KX022A digital tri-axis + accelerometer connected to I2C interface. See IIO_KX022A_SPI if + you want to enable support for the KX022A connected via SPI. + config KXSD9 tristate "Kionix KXSD9 Accelerometer Driver" select IIO_BUFFER diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile index 4d8792668838..7bd654b74f42 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..2dbf8a33b84b --- /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 regmap *regmap; + struct device *dev = &i2c->dev; + + 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, i2c->irq); +} + +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"); diff --git a/drivers/iio/accel/kionix-kx022a-spi.c b/drivers/iio/accel/kionix-kx022a-spi.c new file mode 100644 index 000000000000..007f4b726d8f --- /dev/null +++ b/drivers/iio/accel/kionix-kx022a-spi.c @@ -0,0 +1,50 @@ +// 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, spi->irq); +} + +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", + }, + .probe = kx022a_spi_probe, +}; + +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"); diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c new file mode 100644 index 000000000000..9f9e0d7efc09 --- /dev/null +++ b/drivers/iio/accel/kionix-kx022a.c @@ -0,0 +1,1149 @@ +// SPDX-License-Identifier: GPL-2.0-only +// +// Copyright (C) 2022 ROHM Semiconductors +// +// ROHM/KIONIX KX022A accelerometer driver + +#include <linux/delay.h> +#include <linux/gpio.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 <linux/interrupt.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/of.h> +#include <linux/of_gpio.h> +#include <linux/regmap.h> +#include <linux/regulator/consumer.h> +#include <linux/slab.h> + +#include "kionix-kx022a.h" + +/* + * Threshold for deciding our HW fifo is unrecoverably corrupt and should be + * cleared + */ +#define KXO22A_FIFO_ERR_THRESHOLD 10 +#define KX022A_FIFO_LENGTH 41 +/* 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) + +#define MIN_ODR_INTERVAL_MS 5 +#define MAX_ODR_INTERVAL_MS 1280 +#define NUM_SUPPORTED_ODR 9 + +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, + }, { + .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_GPL(kx022a_regmap); + +struct kx022a_data; + +struct kx022a_trigger { + struct kx022a_data *data; + struct iio_trigger *indio_trig; + bool enabled; + const char *name; +}; + +struct kx022a_data { + int irq; + struct regmap *regmap; + struct kx022a_trigger trigger; + struct device *dev; + unsigned int g_range; + struct mutex mutex; + unsigned int state; + + int64_t timestamp, old_timestamp; + unsigned int odr_ns; + + struct iio_mount_matrix orientation; + u8 watermark; + /* 3 x 16bit accel data + timestamp */ + s16 buffer[8]; + 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), \ + .ext_info = kx022a_ext_info, \ + .address = KX022A_REG_##axis##OUT_L, \ + .scan_index = index, \ + .scan_type = { \ + .sign = 's', \ + .realbits = 16, \ + .storagebits = 16, \ + .shift = 0, \ + .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 + * Linux CPUs can handle w/o dropping samples. Also, the low power mode is not + * available for higher sample rates. Thus the driver only supports 200 Hz and + * slower ODRs. Slowest being 0.78 Hz + */ +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("0.78 1.563 3.125 6.25 12.5 25 50 100 200"); +static IIO_CONST_ATTR(scale_available, + "598.550415 1197.10083 2394.20166 4788.40332"); + +static struct attribute *kx022a_attributes[] = { + &iio_const_attr_sampling_frequency_available.dev_attr.attr, + &iio_const_attr_scale_available.dev_attr.attr, + NULL, +}; + +static const struct attribute_group kx022a_attrs_group = { + .attrs = kx022a_attributes, +}; + +struct kx022a_tuple { + int val; + int val2; +}; + +/* + * 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 struct kx022a_tuple kx022a_scale_table[] = { + { 598, 550415 }, { 1197, 100830 }, { 2394, 201660 }, { 4788, 403320 } +}; + +/* + * ODR (output data rate) table. First value represents the integer portion of + * frequency (Hz), and the second value is the decimal part (uHz). + * Eg, 0.78 Hz, 1.563 Hz, ... + */ +#define KX922A_DEFAULT_PERIOD 20000000 +static const struct kx022a_tuple kx022a_accel_samp_freq_table[] = { + { 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, 320000000, + 160000000, 80000000, 40000000, 20000000, 10000000, 5000000 }; + +/* Find index of tuple matching the given values */ +static int kx022a_find_tuple_index(const struct kx022a_tuple *tuples, int n, + int val, int val2) +{ + while (n-- > 0) + if (val == tuples[n].val && tuples[n].val2 == val2) + return n; + + return -EINVAL; +} + +static void kx022a_reg2freq(unsigned int val, int *val1, int *val2) +{ + *val1 = kx022a_accel_samp_freq_table[val & KX022A_MASK_ODR].val; + *val2 = kx022a_accel_samp_freq_table[val & KX022A_MASK_ODR].val2; +} + +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].val; + *val2 = kx022a_scale_table[val].val2; +} + +static int __kx022a_turn_on_unlocked(struct kx022a_data *data) +{ + int ret; + + ret = regmap_set_bits(data->regmap, KX022A_REG_CNTL, KX022A_MASK_PC1); + if (ret) + dev_err(data->dev, "Turn ON fail %d\n", ret); + + return ret; +} + +static int __kx022a_turn_off_unlocked(struct kx022a_data *data) +{ + int ret; + + ret = regmap_clear_bits(data->regmap, KX022A_REG_CNTL, KX022A_MASK_PC1); + if (ret) + dev_err(data->dev, "Turn OFF fail %d\n", ret); + + return ret; +} + +static int kx022a_turn_off_lock(struct kx022a_data *data) +{ + int ret; + + mutex_lock(&data->mutex); + ret = __kx022a_turn_off_unlocked(data); + if (ret) + mutex_unlock(&data->mutex); + + return ret; +} + +static int kx022a_turn_on_unlock(struct kx022a_data *data) +{ + int ret; + + ret = __kx022a_turn_on_unlocked(data); + 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; + + /* + * 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). + */ + mutex_lock(&data->mutex); + if (iio_buffer_enabled(idev)) { + ret = -EBUSY; + goto unlock_out; + } + switch (mask) { + case IIO_CHAN_INFO_SAMP_FREQ: + ret = kx022a_find_tuple_index(&kx022a_accel_samp_freq_table[0], + ARRAY_SIZE(kx022a_accel_samp_freq_table), + val, val2); + /* Configure if we found valid ODR */ + if (ret >= 0) { + int odr = ret; + + ret = __kx022a_turn_off_unlocked(data); + if (ret) + goto unlock_out; + + ret = regmap_update_bits(data->regmap, KX022A_REG_ODCNTL, + KX022A_MASK_ODR, odr); + data->odr_ns = kx022a_odrs[odr]; + __kx022a_turn_on_unlocked(data); + } + break; + case IIO_CHAN_INFO_SCALE: + ret = kx022a_find_tuple_index(&kx022a_scale_table[0], + ARRAY_SIZE(kx022a_scale_table), + val, val2); + /* Configure if we found valid scale */ + if (ret >= 0) { + ret = __kx022a_turn_off_unlocked(data); + if (ret) + goto unlock_out; + + ret = regmap_update_bits(data->regmap, KX022A_REG_CNTL, + KX022A_MASK_GSEL, + ret << KX022A_GSEL_SHIFT); + __kx022a_turn_on_unlocked(data); + } + default: + ret = -EINVAL; + } + +unlock_out: + mutex_unlock(&data->mutex); + + 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_fifo_report_data(struct kx022a_data *data, void *buffer, + int samples) +{ + int ret, fifo_bytes; + + fifo_bytes = samples * KX022A_FIFO_SAMPLES_SIZE_BYTES; + ret = regmap_noinc_read(data->regmap, KX022A_REG_BUF_READ, + buffer, fifo_bytes); + if (ret) + dev_err(data->dev, "FIFO read failed %d\n", ret); + + return ret; +} + +static int kx022a_get_axis(struct kx022a_data *data, + struct iio_chan_spec const *chan, + int *val) +{ + u16 raw_val; + int ret; + + ret = regmap_bulk_read(data->regmap, chan->address, &raw_val, + sizeof(raw_val)); + if (ret) + return ret; + *val = raw_val; + + 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; + + mutex_lock(&data->mutex); + switch (mask) { + case IIO_CHAN_INFO_RAW: + if (iio_buffer_enabled(idev)) { + ret = -EBUSY; + goto error_ret; + } + + ret = kx022a_get_axis(data, chan, val); + break; + case IIO_CHAN_INFO_SAMP_FREQ: + { + ret = regmap_read(data->regmap, KX022A_REG_ODCNTL, ®val); + if (ret) + goto error_ret; + + if ((regval & KX022A_MASK_ODR) > + ARRAY_SIZE(kx022a_accel_samp_freq_table)) { + dev_err(data->dev, "Invalid ODR\n"); + ret = -EINVAL; + goto error_ret; + } + + 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, ®val); + if (ret < 0) + goto error_ret; + + kx022a_reg2scale(regval, val, val2); + + ret = IIO_VAL_INT_PLUS_MICRO; + break; + } + +error_ret: + mutex_unlock(&data->mutex); + + return ret; +}; + +static int kx022a_validate_trigger(struct iio_dev *idev, + struct iio_trigger *trig) +{ + struct kx022a_data *data = iio_priv(idev); + + if (data->trigger.indio_trig == trig) + return 0; + + return -EINVAL; +} + +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 kx022a_get_fifo_state(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 sprintf(buf, "%d\n", state); +} + +static ssize_t kx022a_get_fifo_watermark(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 sprintf(buf, "%d\n", wm); +} + +static IIO_DEVICE_ATTR(hwfifo_enabled, 0444, + kx022a_get_fifo_state, NULL, 0); +static IIO_DEVICE_ATTR(hwfifo_watermark, 0444, + kx022a_get_fifo_watermark, NULL, 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 + */ + data->timestamp = 0; + + return regmap_write(data->regmap, KX022A_REG_BUF_CLEAR, 0xff); +} + +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); + int ret, i; + int count, fifo_bytes; + u16 buffer[KX022A_FIFO_LENGTH * 3]; + int64_t tstamp; + uint64_t sample_period; + static int err_ctr; + + ret = regmap_read(data->regmap, KX022A_REG_BUF_STATUS_1, &fifo_bytes); + if (ret < 0) { + dev_err(dev, "Error reading buffer status\n"); + return ret; + } + + /* Let's not owerflow if we for some reason get bogus value from i2c */ + if (fifo_bytes > KX022A_FIFO_MAX_BYTES) { + /* + * I've observed a strange behaviour where FIFO may get stuck if + * samples are not read out fast enough. By 'stuck' I mean + * situation where amount of data adverticed by the STATUS_1 + * reg is 255 - which equals to 42,5 (sic!) samples and by + * my experimenting there are situations where reading the + * FIFO buffer does not decrease the data count but the same + * fifo sample level (255 bytes of data) is reported + */ + err_ctr++; + dev_warn(data->dev, "Bad amount of data %u\n", fifo_bytes); + fifo_bytes = KX022A_FIFO_MAX_BYTES; + } else if (fifo_bytes % KX022A_FIFO_SAMPLES_SIZE_BYTES) { + err_ctr++; + dev_err(data->dev, "Bad FIFO alignment. Data may be corrupt\n"); + } else { + err_ctr = 0; + } + + if (err_ctr > KXO22A_FIFO_ERR_THRESHOLD) { + __kx022a_turn_off_unlocked(data); + kx022a_drop_fifo_contents(data); + __kx022a_turn_on_unlocked(data); + + err_ctr = 0; + + return -EINVAL; + } + + 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) { + 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; + } + + ret = kx022a_fifo_report_data(data, buffer, count); + if (ret) + return ret; + + 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; + } + + return count; +} + +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, + .attrs = &kx022a_attrs_group, + + .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_IEN1 | KX022A_MASK_IPOL1 | + KX022A_MASK_ITYP; + int val = KX022A_MASK_IEN1 | KX022A_IPOL_LOW | + KX022A_ITYP_LEVEL; + int ret; + + ret = regmap_update_bits(data->regmap, KX022A_REG_INC1, mask, val); + if (ret) + return ret; + + mask = KX022A_MASK_INS2_DRDY | KX122_MASK_INS2_WMI; + + return regmap_set_bits(data->regmap, KX022A_REG_INC4, mask); +} + +static int kx022a_fifo_disable(struct kx022a_data *data) +{ + int ret = 0; + + /* PC1 to 0 */ + ret = kx022a_turn_off_lock(data); + if (ret) + return ret; + + ret = regmap_clear_bits(data->regmap, KX022A_REG_INC4, + KX022A_MASK_WMI1); + if (ret) + goto unlock_out; + + /* disable buffer */ + 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 = 0; + + ret = kx022a_turn_off_lock(data); + if (ret) + return ret; + + /* Write WMI 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, KX022A_REG_INC4, + KX022A_MASK_WMI1); + 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 triggers, then the IRQs should be handled by trigger + * enable and buffer is not used but we just add results to buffer + * 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; + + mutex_lock(&data->mutex); + 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: + mutex_unlock(&data->mutex); + 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); + bool ack = false; + + data->old_timestamp = data->timestamp; + data->timestamp = iio_get_time_ns(idev); + + if (data->trigger.enabled) { + iio_trigger_poll(data->trigger.indio_trig); + ack = true; + } + + if (data->state & KX022A_STATE_FIFO) + return IRQ_WAKE_THREAD; + + if (ack) { + /* + * The IRQ is not acked until data is read. We need to disable + * the IRQ in order to schedule the trigger thread. Enabling + * is done in reenable. + * + * It would be possible to set the IRQ to 50uS pulse. If we are + * losing data due to the disabled IRQ we can evaluate the + * option of using edge triggered IRQs with the pulse mode. + */ + disable_irq_nosync(irq); + return IRQ_HANDLED; + } + + return IRQ_NONE; +} + +/* Read the data from the fifo and fill IIO buffers */ +static irqreturn_t kx022a_irq_thread_handler(int irq, void *private) +{ + struct iio_dev *idev = private; + struct kx022a_data *data = iio_priv(idev); + bool ack = false; + int ret; + + mutex_lock(&data->mutex); + + if (data->state & KX022A_STATE_FIFO) { + ret = __kx022a_fifo_flush(idev, KX022A_FIFO_LENGTH, true); + if (ret > 0) + ack = true; + } + /* + * 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). + */ + if (ack) + ret = IRQ_HANDLED; + else + ret = IRQ_NONE; + + mutex_unlock(&data->mutex); + + return ret; +} + +static int kx022a_trigger_set_state(struct iio_trigger *trig, + bool state) +{ + struct kx022a_trigger *t = iio_trigger_get_drvdata(trig); + struct kx022a_data *data = t->data; + int ret = 0; + + mutex_lock(&data->mutex); + + if (t->enabled == state) + goto unlock_out; + + ret = __kx022a_turn_off_unlocked(data); + if (ret) + goto unlock_out; + + t->enabled = state; + ret = kx022a_set_drdy_irq(data, state); + if (ret) + goto unlock_out; + + ret = __kx022a_turn_on_unlocked(data); + +unlock_out: + mutex_unlock(&data->mutex); + + return ret; +} +static void kx022a_trig_reen(struct iio_trigger *trig) +{ + struct kx022a_trigger *t = iio_trigger_get_drvdata(trig); + struct kx022a_data *data = t->data; + + enable_irq(data->irq); +} + +static const struct iio_trigger_ops kx022a_trigger_ops = { + .set_trigger_state = kx022a_trigger_set_state, + .reenable = kx022a_trig_reen, +}; + +static int kx022a_chip_init(struct kx022a_data *data) +{ + int ret, dummy; + + /* + * Disable IRQs because if the IRQs are left on (for example by + * a shutdown which did not deactivate the accelerometer) we do + * most probably end up flooding the system with unhandled IRQs + * and get the line disabled from SOC side. + */ + ret = regmap_write(data->regmap, KX022A_REG_INC4, 0); + if (ret) { + dev_err(data->dev, "Failed to init IRQ states\n"); + return ret; + } + + ret = kx022a_set_drdy_irq(data, false); + if (ret) { + dev_err(data->dev, "Failed to init DRDY\n"); + return ret; + } + + /* Clear any pending IRQs */ + ret = regmap_read(data->regmap, KX022A_REG_INT_REL, &dummy); + if (ret) { + dev_err(data->dev, "Failed to ACK IRQs\n"); + return ret; + } + /* set data res 16bit */ + ret = regmap_set_bits(data->regmap, KX022A_REG_BUF_CNTL2, + KX022A_MASK_BRES); + if (ret) { + dev_err(data->dev, "Failed to set data resolution\n"); + return ret; + } + + ret = kx022a_prepare_irq_pin(data); + if (ret) { + dev_err(data->dev, "Failed to configure IRQ pin\n"); + return ret; + } + + /* disable buffer */ + ret = regmap_clear_bits(data->regmap, KX022A_REG_BUF_CNTL2, + KX022A_MASK_BUF_EN); + if (ret) + return ret; + + return kx022a_drop_fifo_contents(data); +} + +int kx022a_probe_internal(struct device *dev, int irq) +{ + static const char * const regulator_names[] = {"io_vdd", "vdd"}; + struct iio_trigger *indio_trig; + struct kx022a_data *data; + struct regmap *regmap; + unsigned int chip_id; + struct iio_dev *idev; + int ret; + + if (WARN_ON(!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 + * 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) { + dev_err(dev, "Failed to access sensor\n"); + return ret; + } + + if (chip_id != KX022A_ID) { + dev_err(dev, "unsupported device 0x%x\n", chip_id); + return -EINVAL; + } + + data->regmap = regmap; + data->dev = dev; + data->irq = irq; + data->odr_ns = KX922A_DEFAULT_PERIOD; + 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) + return ret; + + ret = kx022a_turn_on_unlock(data); + if (ret) + return ret; + + udelay(100); + + ret = iio_triggered_buffer_setup_ext(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->trigger.indio_trig = indio_trig; + + indio_trig->ops = &kx022a_trigger_ops; + data->trigger.data = data; + iio_trigger_set_drvdata(indio_trig, &data->trigger); + + ret = iio_trigger_register(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"); + + ret = devm_request_threaded_irq(data->dev, irq, kx022a_irq_handler, + &kx022a_irq_thread_handler, + IRQF_ONESHOT, "kx022", idev); + if (ret) + return dev_err_probe(data->dev, ret, "Could not request IRQ\n"); + + return ret; +} +EXPORT_SYMBOL_GPL(kx022a_probe_internal); + +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..ec89e46c6ca8 --- /dev/null +++ b/drivers/iio/accel/kionix-kx022a.h @@ -0,0 +1,76 @@ +/* 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/device.h> +#include <linux/regmap.h> + +#define KX022A_REG_WHO 0x0f +#define KX022A_ID 0xc8 + +#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_BRES 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_MASK_IEN1 BIT(5) +#define KX022A_MASK_IPOL1 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_WMI1 BIT(5) + +#define KX022A_REG_SELF_TEST 0x60 +#define KX022A_MAX_REGISTER 0x60 + +int kx022a_probe_internal(struct device *dev, int irq); +extern const struct regmap_config kx022a_regmap; + +#endif
KX022A is a 3-axis Accelerometer from ROHM/Kionix. The senor 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 fatures. 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. NOTE: Filling-up the hardware FIFO should be avoided. During my testing I noticed that filling up the hardware FIFO might mess-up the sample count. My sensor ended up in a state where amount of data in FIFO was reported to be 0xff bytes, which equals to 42,5 samples. Specification says the FIFO can hold maximum of 41 samples in HiRes mode. Also, at least once the FIFO was stuck in a state where reading data from hwardware FIFO did not decrease the amount of data reported to be in the FIFO - eg. FIFO was "stuck". The code has now an error count and 10 reads with invalid FIFO data count will cause the fifo contents to be dropped. Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> --- drivers/iio/accel/Kconfig | 23 + drivers/iio/accel/Makefile | 3 + drivers/iio/accel/kionix-kx022a-i2c.c | 52 ++ drivers/iio/accel/kionix-kx022a-spi.c | 50 ++ drivers/iio/accel/kionix-kx022a.c | 1149 +++++++++++++++++++++++++ drivers/iio/accel/kionix-kx022a.h | 76 ++ 6 files changed, 1353 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