| Message ID | cover.1682373451.git.mehdi.djait.k@gmail.com |
|---|---|
| Headers | show |
| Series | iio: accel: Add support for Kionix/ROHM KX132-1211 accelerometer | expand |
Hi Matti, On Tue, Apr 25, 2023 at 09:50:11AM +0300, Matti Vaittinen wrote: > On 4/25/23 01:22, Mehdi Djait wrote: > > Add the chip_info structure to the driver's private data to hold all > > the device specific infos. > > Refactor the kx022a driver implementation to make it more generic and > > extensible. > > > > Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com> > > --- > > v3: > > - added the change of the buffer's allocation in the __kx022a_fifo_flush > > to this patch > > - added the chip_info to the struct kx022a_data > > > > v2: > > - mentioned the introduction of the i2c_device_id table in the commit > > - get i2c_/spi_get_device_id only when device get match fails > > - removed the generic KX_define > > - removed the kx022a_device_type enum > > - added comments for the chip_info struct elements > > - fixed errors pointed out by the kernel test robot > > > > drivers/iio/accel/kionix-kx022a-i2c.c | 15 +++- > > drivers/iio/accel/kionix-kx022a-spi.c | 15 +++- > > drivers/iio/accel/kionix-kx022a.c | 114 +++++++++++++++++--------- > > drivers/iio/accel/kionix-kx022a.h | 54 +++++++++++- > > 4 files changed, 147 insertions(+), 51 deletions(-) > > > > diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c > > index 8f23631a1fd3..ce299d0446f7 100644 > > --- a/drivers/iio/accel/kionix-kx022a-i2c.c > > +++ b/drivers/iio/accel/kionix-kx022a-i2c.c > > @@ -15,6 +15,7 @@ > > ... > > > > static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples, > > @@ -600,13 +600,17 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples, > > { > > struct kx022a_data *data = iio_priv(idev); > > struct device *dev = regmap_get_device(data->regmap); > > - __le16 buffer[KX022A_FIFO_LENGTH * 3]; > > + __le16 *buffer; > > uint64_t sample_period; > > int count, fifo_bytes; > > bool renable = false; > > int64_t tstamp; > > int ret, i; > > + buffer = kmalloc(data->chip_info->fifo_length * KX022A_FIFO_SAMPLES_SIZE_BYTES, GFP_KERNEL); > > + if (!buffer) > > + return -ENOMEM; > > Do you think we could get rid of allocating and freeing the buffer for each > flush? I feel it is a bit wasteful, and with high sampling frequencies this > function can be called quite often. Do you think there would be a way to > either use stack (always reserve big enough buffer no matter which chip we > have - or is the buffer too big to be safely taken from the stack?), or a > buffer stored in private data and allocated at probe or buffer enable? I tried using the same allocation as before but a device like the KX127 has a fifo_length of 342 (compared to 86 for kx132, and 43 for kx022a). Allocating this much using the stack will result in a Warning. > > Also, please avoid such long lines. I know many people don't care about the > line length - but for example I tend to have 3 terminal windows open > side-by-side on my laptop screen. Hence long lines tend to be harder to read > for me. That is the case for me also, but Jonathan asked me to change "fifo_length * 6" and the KX022A_FIFO_SAMPLES_SIZE_BYTES is already defined. > > > + > > ret = regmap_read(data->regmap, KX022A_REG_BUF_STATUS_1, &fifo_bytes); > > if (ret) { > > dev_err(dev, "Error reading buffer status\n"); > > @@ -621,8 +625,10 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples, > > dev_warn(data->dev, "Bad FIFO alignment. Data may be corrupt\n"); > > count = fifo_bytes / KX022A_FIFO_SAMPLES_SIZE_BYTES; > > - if (!count) > > + if (!count) { > > + kfree(buffer); > > return 0; > > + } > > /* > > * If we are being called from IRQ handler we know the stored timestamp > > @@ -679,7 +685,7 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples, > > } > > fifo_bytes = count * KX022A_FIFO_SAMPLES_SIZE_BYTES; > > - ret = regmap_noinc_read(data->regmap, KX022A_REG_BUF_READ, > > + ret = regmap_noinc_read(data->regmap, data->chip_info->buf_read, > > &buffer[0], fifo_bytes); > > if (ret) > > goto renable_out; > > @@ -704,6 +710,7 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples, > > if (renable) > > enable_irq(data->irq); > > + kfree(buffer); > > return ret; > > } > > > ... > > > -int kx022a_probe_internal(struct device *dev) > > +const struct kx022a_chip_info kx022a_chip_info = { > > + .name = "kx022-accel", > > + .regmap_config = &kx022a_regmap_config, > > + .channels = kx022a_channels, > > + .num_channels = ARRAY_SIZE(kx022a_channels), > > + .fifo_length = KX022A_FIFO_LENGTH, > > + .who = KX022A_REG_WHO, > > + .id = KX022A_ID, > > + .cntl = KX022A_REG_CNTL, > > + .cntl2 = KX022A_REG_CNTL2, > > + .odcntl = KX022A_REG_ODCNTL, > > + .buf_cntl1 = KX022A_REG_BUF_CNTL1, > > + .buf_cntl2 = KX022A_REG_BUF_CNTL2, > > + .buf_clear = KX022A_REG_BUF_CLEAR, > > + .buf_status1 = KX022A_REG_BUF_STATUS_1, > > + .buf_read = KX022A_REG_BUF_READ, > > + .inc1 = KX022A_REG_INC1, > > + .inc4 = KX022A_REG_INC4, > > + .inc5 = KX022A_REG_INC5, > > + .inc6 = KX022A_REG_INC6, > > + .xout_l = KX022A_REG_XOUT_L, > > +}; > > +EXPORT_SYMBOL_NS_GPL(kx022a_chip_info, IIO_KX022A); > > Do you think the fields (or at least some of them) in this struct could be > named based on the (main) functionality being used, not based on the > register name? Something like "watermark_reg", "buf_en_reg", "reset_reg", > "output_rate_reg", "int1_pinconf_reg", "int1_src_reg", "int2_pinconf_reg", > "int1_src_reg" ... > > I would not be at all surprized to see for example some IRQ control to be > shifted from INC<X> to INC<Y> or cntl<X> / buf_cntl<X> stuff to be moved to > cntl<Y> or to buf_cntl<Y> for next sensor we want to support. Especially > when new cool feature is added to next sensor, resulting also adding a new > cntl<Z> or buf_cntl<Z> or INC<Z>. > > I, however, believe the _functionality_ will be there (in some register) - > at least for the ICs for which we can re-use this driver. Hence, it might be > nice - and if you can think of better names for these fields - to rename > them based on the _functionality_ we use. > > Another benefit would be added clarity to the code. Writing a value to > "buf_en_reg", "watermark_reg" or to "int1_src_reg" is much clearer (to me) > than writing a value to "buf_cntl1", "buf_cntl2" or "INC4". Especially if > you don't have a datasheet at your hands. > > I am not "demanding" this (at least not for now :]) because it seems these > two Kionix sensors have been pretty consistent what comes to maintaining the > same functionality in the registers with same naming - but I believe this is > something that may in any case be lurking around the corner. I agree, this seems to be the better solution. I will look into this. > > > > All in all, looks nice and clean to me! Good job. Thank you :) -- Kind Regards Mehdi Djait
On 4/25/23 01:22, Mehdi Djait wrote: > Kionix KX132-1211 is a tri-axis 16-bit accelerometer that can support > ranges from ±2G to ±16G, digital output through I²C/SPI. > Add support for basic accelerometer features such as reading acceleration > via IIO using raw reads, triggered buffer (data-ready), or the WMI IRQ. > > Datasheet: https://kionixfs.azureedge.net/en/document/KX132-1211-Technical-Reference-Manual-Rev-5.0.pdf > Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com> > --- > drivers/iio/accel/Kconfig | 8 +- > drivers/iio/accel/kionix-kx022a-i2c.c | 2 + > drivers/iio/accel/kionix-kx022a-spi.c | 2 + > drivers/iio/accel/kionix-kx022a.c | 147 ++++++++++++++++++++++++++ > drivers/iio/accel/kionix-kx022a.h | 52 +++++++++ > 5 files changed, 207 insertions(+), 4 deletions(-) > > diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig > index b6b45d359f28..d8cc6e6f2bb9 100644 > --- a/drivers/iio/accel/Kconfig > +++ b/drivers/iio/accel/Kconfig > @@ -418,8 +418,8 @@ config IIO_KX022A_SPI > select IIO_KX022A > select REGMAP_SPI > help > - Enable support for the Kionix KX022A digital tri-axis > - accelerometer connected to I2C interface. > + Enable support for the Kionix KX022A, KX132-1211 digital tri-axis > + accelerometers connected to SPI interface. > > config IIO_KX022A_I2C > tristate "Kionix KX022A tri-axis digital accelerometer I2C interface" > @@ -427,8 +427,8 @@ config IIO_KX022A_I2C > select IIO_KX022A > select REGMAP_I2C > help > - Enable support for the Kionix KX022A digital tri-axis > - accelerometer connected to I2C interface. > + Enable support for the Kionix KX022A, KX132-1211 digital tri-axis > + accelerometers connected to I2C interface. > > config KXSD9 > tristate "Kionix KXSD9 Accelerometer Driver" > diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c > index ce299d0446f7..4ea28d2482ec 100644 > --- a/drivers/iio/accel/kionix-kx022a-i2c.c > +++ b/drivers/iio/accel/kionix-kx022a-i2c.c > @@ -39,12 +39,14 @@ static int kx022a_i2c_probe(struct i2c_client *i2c) > > static const struct i2c_device_id kx022a_i2c_id[] = { > { .name = "kx022a", .driver_data = (kernel_ulong_t)&kx022a_chip_info }, > + { .name = "kx132-1211", .driver_data = (kernel_ulong_t)&kx132_chip_info }, > { } > }; > MODULE_DEVICE_TABLE(i2c, kx022a_i2c_id); > > static const struct of_device_id kx022a_of_match[] = { > { .compatible = "kionix,kx022a", .data = &kx022a_chip_info }, > + { .compatible = "kionix,kx132-1211", .data = &kx132_chip_info }, > { } > }; > MODULE_DEVICE_TABLE(of, kx022a_of_match); > diff --git a/drivers/iio/accel/kionix-kx022a-spi.c b/drivers/iio/accel/kionix-kx022a-spi.c > index b84503e24510..b755b2b395ed 100644 > --- a/drivers/iio/accel/kionix-kx022a-spi.c > +++ b/drivers/iio/accel/kionix-kx022a-spi.c > @@ -39,12 +39,14 @@ static int kx022a_spi_probe(struct spi_device *spi) > > static const struct spi_device_id kx022a_id[] = { > { .name = "kx022a", .driver_data = (kernel_ulong_t)&kx022a_chip_info }, > + { .name = "kx132-1211", .driver_data = (kernel_ulong_t)&kx132_chip_info }, > { } > }; > MODULE_DEVICE_TABLE(spi, kx022a_id); > > static const struct of_device_id kx022a_of_match[] = { > { .compatible = "kionix,kx022a", .data = &kx022a_chip_info }, > + { .compatible = "kionix,kx132-1211", .data = &kx132_chip_info }, > { } > }; > MODULE_DEVICE_TABLE(of, kx022a_of_match); > diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c > index 4a31d17c1f22..a6808ab12162 100644 > --- a/drivers/iio/accel/kionix-kx022a.c > +++ b/drivers/iio/accel/kionix-kx022a.c > @@ -150,6 +150,101 @@ static const struct regmap_config kx022a_regmap_config = { > .cache_type = REGCACHE_RBTREE, > }; > > +/* Regmap configs kx132 */ > +static const struct regmap_range kx132_volatile_ranges[] = { > + { > + .range_min = KX132_REG_XADP_L, > + .range_max = KX132_REG_COTR, > + }, { > + .range_min = KX132_REG_TSCP, > + .range_max = KX132_REG_INT_REL, > + }, { > + /* The reset bit will be cleared by sensor */ > + .range_min = KX132_REG_CNTL2, > + .range_max = KX132_REG_CNTL2, > + }, { > + .range_min = KX132_REG_BUF_STATUS_1, > + .range_max = KX132_REG_BUF_READ, > + }, > +}; Maybe the CNTL5 should also be added to volatile table as it has "self clearing" bits? I didn't go through all the registers to see if there are more. > + > +static const struct regmap_access_table kx132_volatile_regs = { > + .yes_ranges = &kx132_volatile_ranges[0], > + .n_yes_ranges = ARRAY_SIZE(kx132_volatile_ranges), > +}; > + > +static const struct regmap_range kx132_precious_ranges[] = { > + { > + .range_min = KX132_REG_INT_REL, > + .range_max = KX132_REG_INT_REL, > + }, > +}; > + > +static const struct regmap_access_table kx132_precious_regs = { > + .yes_ranges = &kx132_precious_ranges[0], > + .n_yes_ranges = ARRAY_SIZE(kx132_precious_ranges), > +}; > + > +static const struct regmap_range kx132_read_only_ranges[] = { > + { > + .range_min = KX132_REG_XADP_L, > + .range_max = KX132_REG_INT_REL, > + }, { > + .range_min = KX132_REG_BUF_STATUS_1, > + .range_max = KX132_REG_BUF_STATUS_2, > + }, { > + .range_min = KX132_REG_BUF_READ, > + .range_max = KX132_REG_BUF_READ, > + }, > +}; Do you think adding the "Kionix reserved" registers to read-only ranges would make things safer or is there a reason to keep them writable? I think the data-sheet states these should not be written to. > + > +static const struct regmap_access_table kx132_ro_regs = { > + .no_ranges = &kx132_read_only_ranges[0], > + .n_no_ranges = ARRAY_SIZE(kx132_read_only_ranges), > +}; > + > +static const struct regmap_range kx132_write_only_ranges[] = { > + { > + .range_min = KX132_REG_MAN_WAKE, > + .range_max = KX132_REG_MAN_WAKE, Why the WUFC is write-only? Also, I don't think the KX022A_REG_MAN_WAKE and KX132_REG_MAN_WAKE reflect same functionality. The naming of the define may be slightly misleading. > + }, { > + .range_min = KX132_REG_SELF_TEST, > + .range_max = KX132_REG_SELF_TEST, > + }, { > + .range_min = KX132_REG_BUF_CLEAR, > + .range_max = KX132_REG_BUF_CLEAR, > + }, > +}; > + > +static const struct regmap_access_table kx132_wo_regs = { > + .no_ranges = &kx132_write_only_ranges[0], > + .n_no_ranges = ARRAY_SIZE(kx132_write_only_ranges), > +}; > + > +static const struct regmap_range kx132_noinc_read_ranges[] = { > + { > + .range_min = KX132_REG_BUF_READ, > + .range_max = KX132_REG_BUF_READ, > + }, > +}; > + > +static const struct regmap_access_table kx132_nir_regs = { > + .yes_ranges = &kx132_noinc_read_ranges[0], > + .n_yes_ranges = ARRAY_SIZE(kx132_noinc_read_ranges), > +}; > + > +static const struct regmap_config kx132_regmap_config = { > + .reg_bits = 8, > + .val_bits = 8, > + .volatile_table = &kx132_volatile_regs, > + .rd_table = &kx132_wo_regs, > + .wr_table = &kx132_ro_regs, > + .rd_noinc_table = &kx132_nir_regs, > + .precious_table = &kx132_precious_regs, > + .max_register = KX132_MAX_REGISTER, > + .cache_type = REGCACHE_RBTREE, > +}; > + > struct kx022a_data { > const struct kx022a_chip_info *chip_info; > struct regmap *regmap; > @@ -237,6 +332,13 @@ static const struct iio_chan_spec kx022a_channels[] = { > IIO_CHAN_SOFT_TIMESTAMP(3), > }; > > +static const struct iio_chan_spec kx132_channels[] = { > + KX022A_ACCEL_CHAN(X, KX132_REG_XOUT_L, 0), > + KX022A_ACCEL_CHAN(Y, KX132_REG_YOUT_L, 1), > + KX022A_ACCEL_CHAN(Z, KX132_REG_ZOUT_L, 2), > + IIO_CHAN_SOFT_TIMESTAMP(3), > +}; > + > /* > * The sensor HW can support ODR up to 1600 Hz, which is beyond what most of the > * Linux CPUs can handle without dropping samples. Also, the low power mode is > @@ -613,6 +715,25 @@ static int kx022a_get_fifo_bytes(struct kx022a_data *data) > return fifo_bytes; > } > > +static int kx132_get_fifo_bytes(struct kx022a_data *data) > +{ > + struct device *dev = regmap_get_device(data->regmap); > + __le16 buf_status; > + int ret, fifo_bytes; > + > + ret = regmap_bulk_read(data->regmap, data->chip_info->buf_status1, > + &buf_status, sizeof(buf_status)); > + if (ret) { > + dev_err(dev, "Error reading buffer status\n"); > + return ret; > + } > + > + fifo_bytes = le16_to_cpu(buf_status); > + fifo_bytes &= data->chip_info->buf_smp_lvl_mask; This is probably just my limitation but I've hard time thinking how this works out on BE machines. It'd be much easier for me to understand this if the data was handled as two u8 values and mask was applied before endianes conversion. (Eg - untested pseudo code follows; __le16 buf_status; u8 *reg_data; ... ret = regmap_bulk_read(data->regmap, data->chip_info->buf_status1, &buf_status, sizeof(buf_status)); ... reg_data = (u8 *)&buf_status; /* Clear the unused bits form 2.nd reg */ reg_data[1] = reg_data[i] & MASK_SMP_LVL_REG_HIGH_BITS; /* Convert to CPU endianess */ fifo_bytes = le16_to_cpu(buf_status); Well, others may have different view on this :) > + > + return fifo_bytes; > +} > + > static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples, > bool irq) > { > @@ -1036,6 +1157,32 @@ const struct kx022a_chip_info kx022a_chip_info = { > }; > EXPORT_SYMBOL_NS_GPL(kx022a_chip_info, IIO_KX022A); > > +const struct kx022a_chip_info kx132_chip_info = { > + .name = "kx132-1211", > + .regmap_config = &kx132_regmap_config, > + .channels = kx132_channels, > + .num_channels = ARRAY_SIZE(kx132_channels), > + .fifo_length = KX132_FIFO_LENGTH, > + .who = KX132_REG_WHO, > + .id = KX132_ID, > + .cntl = KX132_REG_CNTL, > + .cntl2 = KX132_REG_CNTL2, > + .odcntl = KX132_REG_ODCNTL, > + .buf_cntl1 = KX132_REG_BUF_CNTL1, > + .buf_cntl2 = KX132_REG_BUF_CNTL2, > + .buf_clear = KX132_REG_BUF_CLEAR, > + .buf_status1 = KX132_REG_BUF_STATUS_1, > + .buf_smp_lvl_mask = KX132_MASK_BUF_SMP_LVL, > + .buf_read = KX132_REG_BUF_READ, > + .inc1 = KX132_REG_INC1, > + .inc4 = KX132_REG_INC4, > + .inc5 = KX132_REG_INC5, > + .inc6 = KX132_REG_INC6, > + .xout_l = KX132_REG_XOUT_L, > + .get_fifo_bytes = kx132_get_fifo_bytes, > +}; > +EXPORT_SYMBOL_NS_GPL(kx132_chip_info, IIO_KX022A); > + > int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info) > { > static const char * const regulator_names[] = {"io-vdd", "vdd"}; > diff --git a/drivers/iio/accel/kionix-kx022a.h b/drivers/iio/accel/kionix-kx022a.h > index f043767067b7..1f4135cf20eb 100644 > --- a/drivers/iio/accel/kionix-kx022a.h > +++ b/drivers/iio/accel/kionix-kx022a.h > @@ -74,6 +74,57 @@ > #define KX022A_REG_SELF_TEST 0x60 > #define KX022A_MAX_REGISTER 0x60 > > +#define KX132_REG_WHO 0x13 > +#define KX132_ID 0x3d > + > +#define KX132_FIFO_LENGTH 86 > + > +#define KX132_REG_CNTL 0x1b > +#define KX132_REG_CNTL2 0x1c > +#define KX132_MASK_RES BIT(6) > +#define KX132_GSEL_2 0x0 > +#define KX132_GSEL_4 BIT(3) > +#define KX132_GSEL_8 BIT(4) > +#define KX132_GSEL_16 GENMASK(4, 3) > + > +#define KX132_REG_INS2 0x17 > +#define KX132_MASK_INS2_WMI BIT(5) > + > +#define KX132_REG_XADP_L 0x02 > +#define KX132_REG_XOUT_L 0x08 > +#define KX132_REG_YOUT_L 0x0a > +#define KX132_REG_ZOUT_L 0x0c > +#define KX132_REG_COTR 0x12 > +#define KX132_REG_TSCP 0x14 > +#define KX132_REG_INT_REL 0x1a > + > +#define KX132_REG_ODCNTL 0x21 > + > +#define KX132_REG_BTS_WUF_TH 0x4a > +#define KX132_REG_MAN_WAKE 0x4d As mentioned elsewhere, I don't think this is functionally same as KX022A_REG_MAN_WAKE. > + > +#define KX132_REG_BUF_CNTL1 0x5e > +#define KX132_REG_BUF_CNTL2 0x5f > +#define KX132_REG_BUF_STATUS_1 0x60 > +#define KX132_REG_BUF_STATUS_2 0x61 > +#define KX132_MASK_BUF_SMP_LVL GENMASK(9, 0) > +#define KX132_REG_BUF_CLEAR 0x62 > +#define KX132_REG_BUF_READ 0x63 > +#define KX132_ODR_SHIFT 3 > +#define KX132_FIFO_MAX_WMI_TH 86 > + > +#define KX132_REG_INC1 0x22 > +#define KX132_REG_INC5 0x26 > +#define KX132_REG_INC6 0x27 > +#define KX132_IPOL_LOW 0 > +#define KX132_IPOL_HIGH KX022A_MASK_IPOL > +#define KX132_ITYP_PULSE KX022A_MASK_ITYP > + > +#define KX132_REG_INC4 0x25 > + > +#define KX132_REG_SELF_TEST 0x5d > +#define KX132_MAX_REGISTER 0x76 > + > struct device; > > struct kx022a_data; > @@ -132,5 +183,6 @@ struct kx022a_chip_info { > int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info); > > extern const struct kx022a_chip_info kx022a_chip_info; > +extern const struct kx022a_chip_info kx132_chip_info; > > #endif Thanks for adding this support! Yours, -- Matti
On 4/25/23 10:24, Mehdi Djait wrote: > Hi Matti, > > On Tue, Apr 25, 2023 at 09:50:11AM +0300, Matti Vaittinen wrote: >> On 4/25/23 01:22, Mehdi Djait wrote: >>> Add the chip_info structure to the driver's private data to hold all >>> the device specific infos. >>> Refactor the kx022a driver implementation to make it more generic and >>> extensible. >>> >>> Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com> >>> --- >>> v3: >>> - added the change of the buffer's allocation in the __kx022a_fifo_flush >>> to this patch >>> - added the chip_info to the struct kx022a_data >>> >>> v2: >>> - mentioned the introduction of the i2c_device_id table in the commit >>> - get i2c_/spi_get_device_id only when device get match fails >>> - removed the generic KX_define >>> - removed the kx022a_device_type enum >>> - added comments for the chip_info struct elements >>> - fixed errors pointed out by the kernel test robot >>> >>> drivers/iio/accel/kionix-kx022a-i2c.c | 15 +++- >>> drivers/iio/accel/kionix-kx022a-spi.c | 15 +++- >>> drivers/iio/accel/kionix-kx022a.c | 114 +++++++++++++++++--------- >>> drivers/iio/accel/kionix-kx022a.h | 54 +++++++++++- >>> 4 files changed, 147 insertions(+), 51 deletions(-) >>> >>> diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c >>> index 8f23631a1fd3..ce299d0446f7 100644 >>> --- a/drivers/iio/accel/kionix-kx022a-i2c.c >>> +++ b/drivers/iio/accel/kionix-kx022a-i2c.c >>> @@ -15,6 +15,7 @@ >> >> ... >> >> >>> static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples, >>> @@ -600,13 +600,17 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples, >>> { >>> struct kx022a_data *data = iio_priv(idev); >>> struct device *dev = regmap_get_device(data->regmap); >>> - __le16 buffer[KX022A_FIFO_LENGTH * 3]; >>> + __le16 *buffer; >>> uint64_t sample_period; >>> int count, fifo_bytes; >>> bool renable = false; >>> int64_t tstamp; >>> int ret, i; >>> + buffer = kmalloc(data->chip_info->fifo_length * KX022A_FIFO_SAMPLES_SIZE_BYTES, GFP_KERNEL); >>> + if (!buffer) >>> + return -ENOMEM; >> >> Do you think we could get rid of allocating and freeing the buffer for each >> flush? I feel it is a bit wasteful, and with high sampling frequencies this >> function can be called quite often. Do you think there would be a way to >> either use stack (always reserve big enough buffer no matter which chip we >> have - or is the buffer too big to be safely taken from the stack?), or a >> buffer stored in private data and allocated at probe or buffer enable? > > I tried using the same allocation as before but a device like the KX127 > has a fifo_length of 342 (compared to 86 for kx132, and 43 for kx022a). > Allocating this much using the stack will result in a Warning. > Right. Maybe you could then have the buffer in private-data and allocate it in buffer pre-enable? Do you think that would work? >> >> Also, please avoid such long lines. I know many people don't care about the >> line length - but for example I tend to have 3 terminal windows open >> side-by-side on my laptop screen. Hence long lines tend to be harder to read >> for me. > > That is the case for me also, but Jonathan asked me to change > "fifo_length * 6" and the KX022A_FIFO_SAMPLES_SIZE_BYTES is already > defined. then please maybe split the line from appropriate point like: buffer = kmalloc(data->chip_info->fifo_length * KX022A_FIFO_SAMPLES_SIZE_BYTES, GFP_KERNEL); > >> >>> + >>> ret = regmap_read(data->regmap, KX022A_REG_BUF_STATUS_1, &fifo_bytes); >>> if (ret) { >>> dev_err(dev, "Error reading buffer status\n"); >>> @@ -621,8 +625,10 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples, >>> dev_warn(data->dev, "Bad FIFO alignment. Data may be corrupt\n"); >>> count = fifo_bytes / KX022A_FIFO_SAMPLES_SIZE_BYTES; >>> - if (!count) >>> + if (!count) { >>> + kfree(buffer); >>> return 0; >>> + } >>> /* >>> * If we are being called from IRQ handler we know the stored timestamp >>> @@ -679,7 +685,7 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples, >>> } >>> fifo_bytes = count * KX022A_FIFO_SAMPLES_SIZE_BYTES; >>> - ret = regmap_noinc_read(data->regmap, KX022A_REG_BUF_READ, >>> + ret = regmap_noinc_read(data->regmap, data->chip_info->buf_read, >>> &buffer[0], fifo_bytes); >>> if (ret) >>> goto renable_out; >>> @@ -704,6 +710,7 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples, >>> if (renable) >>> enable_irq(data->irq); >>> + kfree(buffer); >>> return ret; >>> } >>> >> ... >> >>> -int kx022a_probe_internal(struct device *dev) >>> +const struct kx022a_chip_info kx022a_chip_info = { >>> + .name = "kx022-accel", >>> + .regmap_config = &kx022a_regmap_config, >>> + .channels = kx022a_channels, >>> + .num_channels = ARRAY_SIZE(kx022a_channels), >>> + .fifo_length = KX022A_FIFO_LENGTH, >>> + .who = KX022A_REG_WHO, >>> + .id = KX022A_ID, >>> + .cntl = KX022A_REG_CNTL, >>> + .cntl2 = KX022A_REG_CNTL2, >>> + .odcntl = KX022A_REG_ODCNTL, >>> + .buf_cntl1 = KX022A_REG_BUF_CNTL1, >>> + .buf_cntl2 = KX022A_REG_BUF_CNTL2, >>> + .buf_clear = KX022A_REG_BUF_CLEAR, >>> + .buf_status1 = KX022A_REG_BUF_STATUS_1, >>> + .buf_read = KX022A_REG_BUF_READ, >>> + .inc1 = KX022A_REG_INC1, >>> + .inc4 = KX022A_REG_INC4, >>> + .inc5 = KX022A_REG_INC5, >>> + .inc6 = KX022A_REG_INC6, >>> + .xout_l = KX022A_REG_XOUT_L, >>> +}; >>> +EXPORT_SYMBOL_NS_GPL(kx022a_chip_info, IIO_KX022A); >> >> Do you think the fields (or at least some of them) in this struct could be >> named based on the (main) functionality being used, not based on the >> register name? Something like "watermark_reg", "buf_en_reg", "reset_reg", >> "output_rate_reg", "int1_pinconf_reg", "int1_src_reg", "int2_pinconf_reg", >> "int1_src_reg" ... >> >> I would not be at all surprized to see for example some IRQ control to be >> shifted from INC<X> to INC<Y> or cntl<X> / buf_cntl<X> stuff to be moved to >> cntl<Y> or to buf_cntl<Y> for next sensor we want to support. Especially >> when new cool feature is added to next sensor, resulting also adding a new >> cntl<Z> or buf_cntl<Z> or INC<Z>. >> >> I, however, believe the _functionality_ will be there (in some register) - >> at least for the ICs for which we can re-use this driver. Hence, it might be >> nice - and if you can think of better names for these fields - to rename >> them based on the _functionality_ we use. >> >> Another benefit would be added clarity to the code. Writing a value to >> "buf_en_reg", "watermark_reg" or to "int1_src_reg" is much clearer (to me) >> than writing a value to "buf_cntl1", "buf_cntl2" or "INC4". Especially if >> you don't have a datasheet at your hands. >> >> I am not "demanding" this (at least not for now :]) because it seems these >> two Kionix sensors have been pretty consistent what comes to maintaining the >> same functionality in the registers with same naming - but I believe this is >> something that may in any case be lurking around the corner. > > I agree, this seems to be the better solution. I will look into this. > Thanks for going the extra mile :) Yours, -- Matti
Hi Mehdi, On Tue, Apr 25, 2023 at 12:22:25AM +0200, Mehdi Djait wrote: > Add the chip_info structure to the driver's private data to hold all > the device specific infos. > Refactor the kx022a driver implementation to make it more generic and > extensible. Could you please split this in different patches? Add id in one patch and refactor in a different patch. Please, also the refactorings need to be split. I see here that this is a general code cleanup, plus some other stuff. [...] > @@ -22,22 +23,28 @@ static int kx022a_spi_probe(struct spi_device *spi) > return -EINVAL; > } > > - regmap = devm_regmap_init_spi(spi, &kx022a_regmap); > + chip_info = device_get_match_data(&spi->dev); > + if (!chip_info) { > + const struct spi_device_id *id = spi_get_device_id(spi); > + chip_info = (const struct kx022a_chip_info *)id->driver_data; you don't need the cast here... if you don't find it messy, I wouldn't mind this form... some hate it, I find it easier to read: chip_info = spi_get_device_id(spi)->driver_data; your choice. Andi
Hi Matti, On Tue, Apr 25, 2023 at 11:12:11AM +0300, Matti Vaittinen wrote: > On 4/25/23 10:24, Mehdi Djait wrote: > > Hi Matti, > > > > On Tue, Apr 25, 2023 at 09:50:11AM +0300, Matti Vaittinen wrote: > > > On 4/25/23 01:22, Mehdi Djait wrote: > > > > Add the chip_info structure to the driver's private data to hold all > > > > the device specific infos. > > > > Refactor the kx022a driver implementation to make it more generic and > > > > extensible. > > > > > > > > Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com> > > > > --- > > > > v3: > > > > - added the change of the buffer's allocation in the __kx022a_fifo_flush > > > > to this patch > > > > - added the chip_info to the struct kx022a_data > > > > > > > > v2: > > > > - mentioned the introduction of the i2c_device_id table in the commit > > > > - get i2c_/spi_get_device_id only when device get match fails > > > > - removed the generic KX_define > > > > - removed the kx022a_device_type enum > > > > - added comments for the chip_info struct elements > > > > - fixed errors pointed out by the kernel test robot > > > > > > > > drivers/iio/accel/kionix-kx022a-i2c.c | 15 +++- > > > > drivers/iio/accel/kionix-kx022a-spi.c | 15 +++- > > > > drivers/iio/accel/kionix-kx022a.c | 114 +++++++++++++++++--------- > > > > drivers/iio/accel/kionix-kx022a.h | 54 +++++++++++- > > > > 4 files changed, 147 insertions(+), 51 deletions(-) > > > > > > > > diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c > > > > index 8f23631a1fd3..ce299d0446f7 100644 > > > > --- a/drivers/iio/accel/kionix-kx022a-i2c.c > > > > +++ b/drivers/iio/accel/kionix-kx022a-i2c.c > > > > @@ -15,6 +15,7 @@ > > > > > > ... > > > > > > > > > > static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples, > > > > @@ -600,13 +600,17 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples, > > > > { > > > > struct kx022a_data *data = iio_priv(idev); > > > > struct device *dev = regmap_get_device(data->regmap); > > > > - __le16 buffer[KX022A_FIFO_LENGTH * 3]; > > > > + __le16 *buffer; > > > > uint64_t sample_period; > > > > int count, fifo_bytes; > > > > bool renable = false; > > > > int64_t tstamp; > > > > int ret, i; > > > > + buffer = kmalloc(data->chip_info->fifo_length * KX022A_FIFO_SAMPLES_SIZE_BYTES, GFP_KERNEL); > > > > + if (!buffer) > > > > + return -ENOMEM; > > > > > > Do you think we could get rid of allocating and freeing the buffer for each > > > flush? I feel it is a bit wasteful, and with high sampling frequencies this > > > function can be called quite often. Do you think there would be a way to > > > either use stack (always reserve big enough buffer no matter which chip we > > > have - or is the buffer too big to be safely taken from the stack?), or a > > > buffer stored in private data and allocated at probe or buffer enable? > > > > I tried using the same allocation as before but a device like the KX127 > > has a fifo_length of 342 (compared to 86 for kx132, and 43 for kx022a). > > Allocating this much using the stack will result in a Warning. > > > > Right. Maybe you could then have the buffer in private-data and allocate it > in buffer pre-enable? Do you think that would work? Do you mean add a new function kx022a_buffer_preenable to iio_buffer_setup_ops ? Would adding the allocation to kx022a_fifo_enable and the free to kx022a_fifo_disable be a good option also ? > > > > > > Also, please avoid such long lines. I know many people don't care about the > > > line length - but for example I tend to have 3 terminal windows open > > > side-by-side on my laptop screen. Hence long lines tend to be harder to read > > > for me. > > > > That is the case for me also, but Jonathan asked me to change > > "fifo_length * 6" and the KX022A_FIFO_SAMPLES_SIZE_BYTES is already > > defined. > > then please maybe split the line from appropriate point like: > buffer = kmalloc(data->chip_info->fifo_length * > KX022A_FIFO_SAMPLES_SIZE_BYTES, GFP_KERNEL); I will split it > > > > > > > > > > + > > > > ret = regmap_read(data->regmap, KX022A_REG_BUF_STATUS_1, &fifo_bytes); > > > > if (ret) { > > > > dev_err(dev, "Error reading buffer status\n"); > > > > @@ -621,8 +625,10 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples, > > > > dev_warn(data->dev, "Bad FIFO alignment. Data may be corrupt\n"); > > > > count = fifo_bytes / KX022A_FIFO_SAMPLES_SIZE_BYTES; > > > > - if (!count) > > > > + if (!count) { > > > > + kfree(buffer); > > > > return 0; > > > > + } > > > > /* > > > > * If we are being called from IRQ handler we know the stored timestamp > > > > @@ -679,7 +685,7 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples, > > > > } > > > > fifo_bytes = count * KX022A_FIFO_SAMPLES_SIZE_BYTES; > > > > - ret = regmap_noinc_read(data->regmap, KX022A_REG_BUF_READ, > > > > + ret = regmap_noinc_read(data->regmap, data->chip_info->buf_read, > > > > &buffer[0], fifo_bytes); > > > > if (ret) > > > > goto renable_out; > > > > @@ -704,6 +710,7 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples, > > > > if (renable) > > > > enable_irq(data->irq); > > > > + kfree(buffer); > > > > return ret; > > > > } > > > > > > > ... > > > > > > > -int kx022a_probe_internal(struct device *dev) > > > > +const struct kx022a_chip_info kx022a_chip_info = { > > > > + .name = "kx022-accel", > > > > + .regmap_config = &kx022a_regmap_config, > > > > + .channels = kx022a_channels, > > > > + .num_channels = ARRAY_SIZE(kx022a_channels), > > > > + .fifo_length = KX022A_FIFO_LENGTH, > > > > + .who = KX022A_REG_WHO, > > > > + .id = KX022A_ID, > > > > + .cntl = KX022A_REG_CNTL, > > > > + .cntl2 = KX022A_REG_CNTL2, > > > > + .odcntl = KX022A_REG_ODCNTL, > > > > + .buf_cntl1 = KX022A_REG_BUF_CNTL1, > > > > + .buf_cntl2 = KX022A_REG_BUF_CNTL2, > > > > + .buf_clear = KX022A_REG_BUF_CLEAR, > > > > + .buf_status1 = KX022A_REG_BUF_STATUS_1, > > > > + .buf_read = KX022A_REG_BUF_READ, > > > > + .inc1 = KX022A_REG_INC1, > > > > + .inc4 = KX022A_REG_INC4, > > > > + .inc5 = KX022A_REG_INC5, > > > > + .inc6 = KX022A_REG_INC6, > > > > + .xout_l = KX022A_REG_XOUT_L, > > > > +}; > > > > +EXPORT_SYMBOL_NS_GPL(kx022a_chip_info, IIO_KX022A); > > > > > > Do you think the fields (or at least some of them) in this struct could be > > > named based on the (main) functionality being used, not based on the > > > register name? Something like "watermark_reg", "buf_en_reg", "reset_reg", > > > "output_rate_reg", "int1_pinconf_reg", "int1_src_reg", "int2_pinconf_reg", > > > "int1_src_reg" ... > > > > > > I would not be at all surprized to see for example some IRQ control to be > > > shifted from INC<X> to INC<Y> or cntl<X> / buf_cntl<X> stuff to be moved to > > > cntl<Y> or to buf_cntl<Y> for next sensor we want to support. Especially > > > when new cool feature is added to next sensor, resulting also adding a new > > > cntl<Z> or buf_cntl<Z> or INC<Z>. > > > > > > I, however, believe the _functionality_ will be there (in some register) - > > > at least for the ICs for which we can re-use this driver. Hence, it might be > > > nice - and if you can think of better names for these fields - to rename > > > them based on the _functionality_ we use. > > > > > > Another benefit would be added clarity to the code. Writing a value to > > > "buf_en_reg", "watermark_reg" or to "int1_src_reg" is much clearer (to me) > > > than writing a value to "buf_cntl1", "buf_cntl2" or "INC4". Especially if > > > you don't have a datasheet at your hands. > > > > > > I am not "demanding" this (at least not for now :]) because it seems these > > > two Kionix sensors have been pretty consistent what comes to maintaining the > > > same functionality in the registers with same naming - but I believe this is > > > something that may in any case be lurking around the corner. > > > > I agree, this seems to be the better solution. I will look into this. > > > > Thanks for going the extra mile :) Thank you for the review -- Kind Regards Mehdi Djait
Hi Andi, Thank you for the review. On Tue, Apr 25, 2023 at 05:57:34PM +0200, Andi Shyti wrote: > Hi Mehdi, > > On Tue, Apr 25, 2023 at 12:22:25AM +0200, Mehdi Djait wrote: > > Add the chip_info structure to the driver's private data to hold all > > the device specific infos. > > Refactor the kx022a driver implementation to make it more generic and > > extensible. > > Could you please split this in different patches? Add id in one > patch and refactor in a different patch. Please, also the > refactorings need to be split. > > I see here that this is a general code cleanup, plus some other > stuff. Looking at the diff and considering the comments from Jonathan in the previous versions, the only thing that can separated from this patch would be the changes related to: -#define KX022A_ACCEL_CHAN(axis, index) \ +#define KX022A_ACCEL_CHAN(axis, reg, index) \ > > [...] > > > @@ -22,22 +23,28 @@ static int kx022a_spi_probe(struct spi_device *spi) > > return -EINVAL; > > } > > > > - regmap = devm_regmap_init_spi(spi, &kx022a_regmap); > > + chip_info = device_get_match_data(&spi->dev); > > + if (!chip_info) { > > + const struct spi_device_id *id = spi_get_device_id(spi); > > + chip_info = (const struct kx022a_chip_info *)id->driver_data; > > you don't need the cast here... if you don't find it messy, I > wouldn't mind this form... some hate it, I find it easier to > read: > > chip_info = spi_get_device_id(spi)->driver_data; > > your choice. I don't really have any strong opinion about this other than keeping the same style used in iio drivers Again thank you for the review -- Kind Regards Mehdi Djait
On 4/29/23 15:59, Mehdi Djait wrote: > Hi Matti, > > On Tue, Apr 25, 2023 at 11:12:11AM +0300, Matti Vaittinen wrote: >> On 4/25/23 10:24, Mehdi Djait wrote: >>> Hi Matti, >>> >>> On Tue, Apr 25, 2023 at 09:50:11AM +0300, Matti Vaittinen wrote: >>>> On 4/25/23 01:22, Mehdi Djait wrote: >>>>> Add the chip_info structure to the driver's private data to hold all >>>>> the device specific infos. >>>>> Refactor the kx022a driver implementation to make it more generic and >>>>> extensible. >>>>> >>>>> Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com> >>>>> --- >>>>> v3: >>>>> - added the change of the buffer's allocation in the __kx022a_fifo_flush >>>>> to this patch >>>>> - added the chip_info to the struct kx022a_data >>>>> >>>>> v2: >>>>> - mentioned the introduction of the i2c_device_id table in the commit >>>>> - get i2c_/spi_get_device_id only when device get match fails >>>>> - removed the generic KX_define >>>>> - removed the kx022a_device_type enum >>>>> - added comments for the chip_info struct elements >>>>> - fixed errors pointed out by the kernel test robot >>>>> >>>>> drivers/iio/accel/kionix-kx022a-i2c.c | 15 +++- >>>>> drivers/iio/accel/kionix-kx022a-spi.c | 15 +++- >>>>> drivers/iio/accel/kionix-kx022a.c | 114 +++++++++++++++++--------- >>>>> drivers/iio/accel/kionix-kx022a.h | 54 +++++++++++- >>>>> 4 files changed, 147 insertions(+), 51 deletions(-) >>>>> >>>>> diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c >>>>> index 8f23631a1fd3..ce299d0446f7 100644 >>>>> --- a/drivers/iio/accel/kionix-kx022a-i2c.c >>>>> +++ b/drivers/iio/accel/kionix-kx022a-i2c.c >>>>> @@ -15,6 +15,7 @@ >>>> >>>> ... >>>> >>>> >>>>> static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples, >>>>> @@ -600,13 +600,17 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples, >>>>> { >>>>> struct kx022a_data *data = iio_priv(idev); >>>>> struct device *dev = regmap_get_device(data->regmap); >>>>> - __le16 buffer[KX022A_FIFO_LENGTH * 3]; >>>>> + __le16 *buffer; >>>>> uint64_t sample_period; >>>>> int count, fifo_bytes; >>>>> bool renable = false; >>>>> int64_t tstamp; >>>>> int ret, i; >>>>> + buffer = kmalloc(data->chip_info->fifo_length * KX022A_FIFO_SAMPLES_SIZE_BYTES, GFP_KERNEL); >>>>> + if (!buffer) >>>>> + return -ENOMEM; >>>> >>>> Do you think we could get rid of allocating and freeing the buffer for each >>>> flush? I feel it is a bit wasteful, and with high sampling frequencies this >>>> function can be called quite often. Do you think there would be a way to >>>> either use stack (always reserve big enough buffer no matter which chip we >>>> have - or is the buffer too big to be safely taken from the stack?), or a >>>> buffer stored in private data and allocated at probe or buffer enable? >>> >>> I tried using the same allocation as before but a device like the KX127 >>> has a fifo_length of 342 (compared to 86 for kx132, and 43 for kx022a). >>> Allocating this much using the stack will result in a Warning. >>> >> >> Right. Maybe you could then have the buffer in private-data and allocate it >> in buffer pre-enable? Do you think that would work? > > Do you mean add a new function kx022a_buffer_preenable to iio_buffer_setup_ops ? Sorry. I thought the kx022a already implemented the pre-enable callback but it was the postenable. I was mistaken. > Would adding the allocation to kx022a_fifo_enable and the free to > kx022a_fifo_disable be a good option also ? Yes. I think that should work! Yours, -- Matti
On Sat, 29 Apr 2023 15:07:46 +0200 Mehdi Djait <mehdi.djait.k@gmail.com> wrote: > Hi Andi, > > Thank you for the review. > > On Tue, Apr 25, 2023 at 05:57:34PM +0200, Andi Shyti wrote: > > Hi Mehdi, > > > > On Tue, Apr 25, 2023 at 12:22:25AM +0200, Mehdi Djait wrote: > > > Add the chip_info structure to the driver's private data to hold all > > > the device specific infos. > > > Refactor the kx022a driver implementation to make it more generic and > > > extensible. > > > > Could you please split this in different patches? Add id in one > > patch and refactor in a different patch. Please, also the > > refactorings need to be split. > > > > I see here that this is a general code cleanup, plus some other > > stuff. > > Looking at the diff and considering the comments from Jonathan in the > previous versions, the only thing that can separated from this patch > would be the changes related to: > -#define KX022A_ACCEL_CHAN(axis, index) \ > +#define KX022A_ACCEL_CHAN(axis, reg, index) \ > > > > > [...] > > > > > @@ -22,22 +23,28 @@ static int kx022a_spi_probe(struct spi_device *spi) > > > return -EINVAL; > > > } > > > > > > - regmap = devm_regmap_init_spi(spi, &kx022a_regmap); > > > + chip_info = device_get_match_data(&spi->dev); > > > + if (!chip_info) { > > > + const struct spi_device_id *id = spi_get_device_id(spi); > > > + chip_info = (const struct kx022a_chip_info *)id->driver_data; > > > > you don't need the cast here... if you don't find it messy, I > > wouldn't mind this form... some hate it, I find it easier to > > read: > > > > chip_info = spi_get_device_id(spi)->driver_data; > > > > your choice. > > I don't really have any strong opinion about this other than keeping the > same style used in iio drivers > > Again thank you for the review I'm fairly sure the cast is needed because driver_data is (via defines) an unsigned long, which you cannot implicitly cast to a pointer without various warnings being generated. Jonathan > > -- > Kind Regards > Mehdi Djait >
On Sat, 29 Apr 2023 16:56:38 +0300 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > On 4/29/23 15:59, Mehdi Djait wrote: > > Hi Matti, > > > > On Tue, Apr 25, 2023 at 11:12:11AM +0300, Matti Vaittinen wrote: > >> On 4/25/23 10:24, Mehdi Djait wrote: > >>> Hi Matti, > >>> > >>> On Tue, Apr 25, 2023 at 09:50:11AM +0300, Matti Vaittinen wrote: > >>>> On 4/25/23 01:22, Mehdi Djait wrote: > >>>>> Add the chip_info structure to the driver's private data to hold all > >>>>> the device specific infos. > >>>>> Refactor the kx022a driver implementation to make it more generic and > >>>>> extensible. > >>>>> > >>>>> Signed-off-by: Mehdi Djait <mehdi.djait.k@gmail.com> > >>>>> --- > >>>>> v3: > >>>>> - added the change of the buffer's allocation in the __kx022a_fifo_flush > >>>>> to this patch > >>>>> - added the chip_info to the struct kx022a_data > >>>>> > >>>>> v2: > >>>>> - mentioned the introduction of the i2c_device_id table in the commit > >>>>> - get i2c_/spi_get_device_id only when device get match fails > >>>>> - removed the generic KX_define > >>>>> - removed the kx022a_device_type enum > >>>>> - added comments for the chip_info struct elements > >>>>> - fixed errors pointed out by the kernel test robot > >>>>> > >>>>> drivers/iio/accel/kionix-kx022a-i2c.c | 15 +++- > >>>>> drivers/iio/accel/kionix-kx022a-spi.c | 15 +++- > >>>>> drivers/iio/accel/kionix-kx022a.c | 114 +++++++++++++++++--------- > >>>>> drivers/iio/accel/kionix-kx022a.h | 54 +++++++++++- > >>>>> 4 files changed, 147 insertions(+), 51 deletions(-) > >>>>> > >>>>> diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c > >>>>> index 8f23631a1fd3..ce299d0446f7 100644 > >>>>> --- a/drivers/iio/accel/kionix-kx022a-i2c.c > >>>>> +++ b/drivers/iio/accel/kionix-kx022a-i2c.c > >>>>> @@ -15,6 +15,7 @@ > >>>> > >>>> ... > >>>> > >>>> > >>>>> static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples, > >>>>> @@ -600,13 +600,17 @@ static int __kx022a_fifo_flush(struct iio_dev *idev, unsigned int samples, > >>>>> { > >>>>> struct kx022a_data *data = iio_priv(idev); > >>>>> struct device *dev = regmap_get_device(data->regmap); > >>>>> - __le16 buffer[KX022A_FIFO_LENGTH * 3]; > >>>>> + __le16 *buffer; > >>>>> uint64_t sample_period; > >>>>> int count, fifo_bytes; > >>>>> bool renable = false; > >>>>> int64_t tstamp; > >>>>> int ret, i; > >>>>> + buffer = kmalloc(data->chip_info->fifo_length * KX022A_FIFO_SAMPLES_SIZE_BYTES, GFP_KERNEL); > >>>>> + if (!buffer) > >>>>> + return -ENOMEM; > >>>> > >>>> Do you think we could get rid of allocating and freeing the buffer for each > >>>> flush? I feel it is a bit wasteful, and with high sampling frequencies this > >>>> function can be called quite often. Do you think there would be a way to > >>>> either use stack (always reserve big enough buffer no matter which chip we > >>>> have - or is the buffer too big to be safely taken from the stack?), or a > >>>> buffer stored in private data and allocated at probe or buffer enable? > >>> > >>> I tried using the same allocation as before but a device like the KX127 > >>> has a fifo_length of 342 (compared to 86 for kx132, and 43 for kx022a). > >>> Allocating this much using the stack will result in a Warning. > >>> > >> > >> Right. Maybe you could then have the buffer in private-data and allocate it > >> in buffer pre-enable? Do you think that would work? > > > > Do you mean add a new function kx022a_buffer_preenable to iio_buffer_setup_ops ? > > Sorry. I thought the kx022a already implemented the pre-enable callback > but it was the postenable. I was mistaken. Separation between what should be done in preenable and postenable has been vague for a long time. These days only really matters if you need to order them wrt updating the scan mode I think. > > > Would adding the allocation to kx022a_fifo_enable and the free to > > kx022a_fifo_disable be a good option also ? > Yes. I think that should work! Agreed that these allocations should be taken out of this hot path. Either of these options should work so up to you. > > Yours, > -- Matti > >
On Sun, Apr 30, 2023 at 06:49:10PM +0100, Jonathan Cameron wrote: > On Sat, 29 Apr 2023 15:07:46 +0200 > Mehdi Djait <mehdi.djait.k@gmail.com> wrote: > > On Tue, Apr 25, 2023 at 05:57:34PM +0200, Andi Shyti wrote: > > > On Tue, Apr 25, 2023 at 12:22:25AM +0200, Mehdi Djait wrote: ... > > > > + chip_info = device_get_match_data(&spi->dev); > > > > + if (!chip_info) { > > > > + const struct spi_device_id *id = spi_get_device_id(spi); > > > > + chip_info = (const struct kx022a_chip_info *)id->driver_data; > > > > > > you don't need the cast here... if you don't find it messy, I > > > wouldn't mind this form... some hate it, I find it easier to > > > read: > > > > > > chip_info = spi_get_device_id(spi)->driver_data; > > > > > > your choice. > > > > I don't really have any strong opinion about this other than keeping the > > same style used in iio drivers > > > > Again thank you for the review > > I'm fairly sure the cast is needed because driver_data is (via defines) > an unsigned long, which you cannot implicitly cast to a pointer without > various warnings being generated. Instead we have a specific SPI provided helper for the above, please use it instead of open coded stuff.
Hello Matti, On Tue, Apr 25, 2023 at 11:12:11AM +0300, Matti Vaittinen wrote: > > > > +const struct kx022a_chip_info kx022a_chip_info = { > > > > + .name = "kx022-accel", > > > > + .regmap_config = &kx022a_regmap_config, > > > > + .channels = kx022a_channels, > > > > + .num_channels = ARRAY_SIZE(kx022a_channels), > > > > + .fifo_length = KX022A_FIFO_LENGTH, > > > > + .who = KX022A_REG_WHO, > > > > + .id = KX022A_ID, > > > > + .cntl = KX022A_REG_CNTL, > > > > + .cntl2 = KX022A_REG_CNTL2, > > > > + .odcntl = KX022A_REG_ODCNTL, > > > > + .buf_cntl1 = KX022A_REG_BUF_CNTL1, > > > > + .buf_cntl2 = KX022A_REG_BUF_CNTL2, > > > > + .buf_clear = KX022A_REG_BUF_CLEAR, > > > > + .buf_status1 = KX022A_REG_BUF_STATUS_1, > > > > + .buf_read = KX022A_REG_BUF_READ, > > > > + .inc1 = KX022A_REG_INC1, > > > > + .inc4 = KX022A_REG_INC4, > > > > + .inc5 = KX022A_REG_INC5, > > > > + .inc6 = KX022A_REG_INC6, > > > > + .xout_l = KX022A_REG_XOUT_L, > > > > +}; > > > > +EXPORT_SYMBOL_NS_GPL(kx022a_chip_info, IIO_KX022A); > > > > > > Do you think the fields (or at least some of them) in this struct could be > > > named based on the (main) functionality being used, not based on the > > > register name? Something like "watermark_reg", "buf_en_reg", "reset_reg", > > > "output_rate_reg", "int1_pinconf_reg", "int1_src_reg", "int2_pinconf_reg", > > > "int1_src_reg" ... > > > > > > I would not be at all surprized to see for example some IRQ control to be > > > shifted from INC<X> to INC<Y> or cntl<X> / buf_cntl<X> stuff to be moved to > > > cntl<Y> or to buf_cntl<Y> for next sensor we want to support. Especially > > > when new cool feature is added to next sensor, resulting also adding a new > > > cntl<Z> or buf_cntl<Z> or INC<Z>. > > > > > > I, however, believe the _functionality_ will be there (in some register) - > > > at least for the ICs for which we can re-use this driver. Hence, it might be > > > nice - and if you can think of better names for these fields - to rename > > > them based on the _functionality_ we use. > > > > > > Another benefit would be added clarity to the code. Writing a value to > > > "buf_en_reg", "watermark_reg" or to "int1_src_reg" is much clearer (to me) > > > than writing a value to "buf_cntl1", "buf_cntl2" or "INC4". Especially if > > > you don't have a datasheet at your hands. > > > > > > I am not "demanding" this (at least not for now :]) because it seems these > > > two Kionix sensors have been pretty consistent what comes to maintaining the > > > same functionality in the registers with same naming - but I believe this is > > > something that may in any case be lurking around the corner. > > > > I agree, this seems to be the better solution. I will look into this. > > > > Thanks for going the extra mile :) I am reconsidering the renaming of the fields 1. inc{1,4,5,6} get assigned once to data->{ien_reg,inc_reg} in the probe function and then never used again 2. buf_cntl2 is used for enabling the buffer and changing the resolution to 16bits, so which name is better than buf_cntl ? 3. cntl seems the most appropriate name, again different functions and the same reg 4. who, id, xout_l, odcntl, buf_{clear, status, read} are quite straightforward Anyway this is my opinion, what do you think ? -- Kind Regards, Mehdi Djait
Hello everyone, Version 3 for adding support for the kx132-1211 accelerometer KX132 accelerometer is a sensor which: - supports G-ranges of (+/-) 2, 4, 8, and 16G - can be connected to I2C or SPI - has internal HW FIFO buffer - supports various ODRs (output data rates) The KX132 accelerometer is very similair to the KX022A. One key difference is number of bits to report the number of data bytes that have been stored in the buffer: 8 bits for KX022A vs 10 bits for KX132-1211. Changes in v3: - added two new patches by separating the addition of the i2c_device_id table and the removal of blank lines from other unrelated changes - fixes a warning detected by the kernel test robot - made all the changes related the chip_info in one patch Changes in v2: - added a new patch for warning when the device_id match fails in the probe function - added a new patch for the function that retrieves the number of bytes in the buffer - added a change to the Kconfig file in the patch adding the support for the kx132-1211 - various fixes and modifications listed under each patch Mehdi Djait (7): dt-bindings: iio: Add KX132-1211 accelerometer iio: accel: kionix-kx022a: Remove blank lines iio: accel: kionix-kx022a: Warn on failed matches and assume compatibility iio: accel: kionix-kx022a: Add an i2c_device_id table iio: accel: kionix-kx022a: Refactor driver and add chip_info structure iio: accel: kionix-kx022a: Add a function to retrieve number of bytes in buffer iio: accel: Add support for Kionix/ROHM KX132-1211 accelerometer .../bindings/iio/accel/kionix,kx022a.yaml | 12 +- drivers/iio/accel/Kconfig | 8 +- drivers/iio/accel/kionix-kx022a-i2c.c | 22 +- drivers/iio/accel/kionix-kx022a-spi.c | 17 +- drivers/iio/accel/kionix-kx022a.c | 295 ++++++++++++++---- drivers/iio/accel/kionix-kx022a.h | 110 ++++++- 6 files changed, 391 insertions(+), 73 deletions(-)