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 <[email protected]>
---
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_i2c_probe(struct i2c_client *i2c)
{
struct device *dev = &i2c->dev;
+ const struct kx022a_chip_info *chip_info;
struct regmap *regmap;
if (!i2c->irq) {
@@ -22,22 +23,28 @@ static int kx022a_i2c_probe(struct i2c_client *i2c)
return -EINVAL;
}
- regmap = devm_regmap_init_i2c(i2c, &kx022a_regmap);
+ chip_info = device_get_match_data(&i2c->dev);
+ if (!chip_info) {
+ const struct i2c_device_id *id = i2c_client_get_device_id(i2c);
+ chip_info = (const struct kx022a_chip_info *)id->driver_data;
+ }
+
+ regmap = devm_regmap_init_i2c(i2c, chip_info->regmap_config);
if (IS_ERR(regmap))
return dev_err_probe(dev, PTR_ERR(regmap),
"Failed to initialize Regmap\n");
- return kx022a_probe_internal(dev);
+ return kx022a_probe_internal(dev, chip_info);
}
static const struct i2c_device_id kx022a_i2c_id[] = {
- { .name = "kx022a", 0 },
+ { .name = "kx022a", .driver_data = (kernel_ulong_t)&kx022a_chip_info },
{ }
};
MODULE_DEVICE_TABLE(i2c, kx022a_i2c_id);
static const struct of_device_id kx022a_of_match[] = {
- { .compatible = "kionix,kx022a", },
+ { .compatible = "kionix,kx022a", .data = &kx022a_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 9cd047f7b346..b84503e24510 100644
--- a/drivers/iio/accel/kionix-kx022a-spi.c
+++ b/drivers/iio/accel/kionix-kx022a-spi.c
@@ -15,6 +15,7 @@
static int kx022a_spi_probe(struct spi_device *spi)
{
struct device *dev = &spi->dev;
+ const struct kx022a_chip_info *chip_info;
struct regmap *regmap;
if (!spi->irq) {
@@ -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;
+ }
+
+ regmap = devm_regmap_init_spi(spi, chip_info->regmap_config);
if (IS_ERR(regmap))
return dev_err_probe(dev, PTR_ERR(regmap),
"Failed to initialize Regmap\n");
- return kx022a_probe_internal(dev);
+ return kx022a_probe_internal(dev, chip_info);
}
static const struct spi_device_id kx022a_id[] = {
- { "kx022a" },
+ { .name = "kx022a", .driver_data = (kernel_ulong_t)&kx022a_chip_info },
{ }
};
MODULE_DEVICE_TABLE(spi, kx022a_id);
static const struct of_device_id kx022a_of_match[] = {
- { .compatible = "kionix,kx022a", },
+ { .compatible = "kionix,kx022a", .data = &kx022a_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 494e81ba1da9..66da3c8c3fd0 100644
--- a/drivers/iio/accel/kionix-kx022a.c
+++ b/drivers/iio/accel/kionix-kx022a.c
@@ -48,7 +48,7 @@ enum {
KX022A_STATE_FIFO,
};
-/* Regmap configs */
+/* kx022a Regmap configs */
static const struct regmap_range kx022a_volatile_ranges[] = {
{
.range_min = KX022A_REG_XHP_L,
@@ -138,7 +138,7 @@ static const struct regmap_access_table kx022a_nir_regs = {
.n_yes_ranges = ARRAY_SIZE(kx022a_noinc_read_ranges),
};
-const struct regmap_config kx022a_regmap = {
+static const struct regmap_config kx022a_regmap_config = {
.reg_bits = 8,
.val_bits = 8,
.volatile_table = &kx022a_volatile_regs,
@@ -149,9 +149,9 @@ const struct regmap_config kx022a_regmap = {
.max_register = KX022A_MAX_REGISTER,
.cache_type = REGCACHE_RBTREE,
};
-EXPORT_SYMBOL_NS_GPL(kx022a_regmap, IIO_KX022A);
struct kx022a_data {
+ const struct kx022a_chip_info *chip_info;
struct regmap *regmap;
struct iio_trigger *trig;
struct device *dev;
@@ -208,7 +208,7 @@ static const struct iio_chan_spec_ext_info kx022a_ext_info[] = {
{ }
};
-#define KX022A_ACCEL_CHAN(axis, index) \
+#define KX022A_ACCEL_CHAN(axis, reg, index) \
{ \
.type = IIO_ACCEL, \
.modified = 1, \
@@ -220,7 +220,7 @@ static const struct iio_chan_spec_ext_info kx022a_ext_info[] = {
BIT(IIO_CHAN_INFO_SCALE) | \
BIT(IIO_CHAN_INFO_SAMP_FREQ), \
.ext_info = kx022a_ext_info, \
- .address = KX022A_REG_##axis##OUT_L, \
+ .address = reg, \
.scan_index = index, \
.scan_type = { \
.sign = 's', \
@@ -231,9 +231,9 @@ static const struct iio_chan_spec_ext_info kx022a_ext_info[] = {
}
static const struct iio_chan_spec kx022a_channels[] = {
- KX022A_ACCEL_CHAN(X, 0),
- KX022A_ACCEL_CHAN(Y, 1),
- KX022A_ACCEL_CHAN(Z, 2),
+ KX022A_ACCEL_CHAN(X, KX022A_REG_XOUT_L, 0),
+ KX022A_ACCEL_CHAN(Y, KX022A_REG_YOUT_L, 1),
+ KX022A_ACCEL_CHAN(Z, KX022A_REG_ZOUT_L, 2),
IIO_CHAN_SOFT_TIMESTAMP(3),
};
@@ -332,10 +332,10 @@ static int kx022a_turn_on_off_unlocked(struct kx022a_data *data, bool on)
int ret;
if (on)
- ret = regmap_set_bits(data->regmap, KX022A_REG_CNTL,
+ ret = regmap_set_bits(data->regmap, data->chip_info->cntl,
KX022A_MASK_PC1);
else
- ret = regmap_clear_bits(data->regmap, KX022A_REG_CNTL,
+ ret = regmap_clear_bits(data->regmap, data->chip_info->cntl,
KX022A_MASK_PC1);
if (ret)
dev_err(data->dev, "Turn %s fail %d\n", str_on_off(on), ret);
@@ -402,7 +402,7 @@ static int kx022a_write_raw(struct iio_dev *idev,
break;
ret = regmap_update_bits(data->regmap,
- KX022A_REG_ODCNTL,
+ data->chip_info->odcntl,
KX022A_MASK_ODR, n);
data->odr_ns = kx022a_odrs[n];
kx022a_turn_on_unlock(data);
@@ -423,7 +423,7 @@ static int kx022a_write_raw(struct iio_dev *idev,
if (ret)
break;
- ret = regmap_update_bits(data->regmap, KX022A_REG_CNTL,
+ ret = regmap_update_bits(data->regmap, data->chip_info->cntl,
KX022A_MASK_GSEL,
n << KX022A_GSEL_SHIFT);
kx022a_turn_on_unlock(data);
@@ -445,7 +445,7 @@ static int kx022a_fifo_set_wmi(struct kx022a_data *data)
threshold = data->watermark;
- return regmap_update_bits(data->regmap, KX022A_REG_BUF_CNTL1,
+ return regmap_update_bits(data->regmap, data->chip_info->buf_cntl1,
KX022A_MASK_WM_TH, threshold);
}
@@ -488,7 +488,7 @@ static int kx022a_read_raw(struct iio_dev *idev,
return ret;
case IIO_CHAN_INFO_SAMP_FREQ:
- ret = regmap_read(data->regmap, KX022A_REG_ODCNTL, ®val);
+ ret = regmap_read(data->regmap, data->chip_info->odcntl, ®val);
if (ret)
return ret;
@@ -503,7 +503,7 @@ static int kx022a_read_raw(struct iio_dev *idev,
return IIO_VAL_INT_PLUS_MICRO;
case IIO_CHAN_INFO_SCALE:
- ret = regmap_read(data->regmap, KX022A_REG_CNTL, ®val);
+ ret = regmap_read(data->regmap, data->chip_info->cntl, ®val);
if (ret < 0)
return ret;
@@ -530,8 +530,8 @@ 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;
+ if (val > data->chip_info->fifo_length)
+ val = data->chip_info->fifo_length;
mutex_lock(&data->mutex);
data->watermark = val;
@@ -592,7 +592,7 @@ static int kx022a_drop_fifo_contents(struct kx022a_data *data)
*/
data->timestamp = 0;
- return regmap_write(data->regmap, KX022A_REG_BUF_CLEAR, 0x0);
+ return regmap_write(data->regmap, data->chip_info->buf_clear, 0x0);
}
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;
+
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;
}
@@ -732,10 +739,10 @@ static const struct iio_info kx022a_info = {
static int kx022a_set_drdy_irq(struct kx022a_data *data, bool en)
{
if (en)
- return regmap_set_bits(data->regmap, KX022A_REG_CNTL,
+ return regmap_set_bits(data->regmap, data->chip_info->cntl,
KX022A_MASK_DRDY);
- return regmap_clear_bits(data->regmap, KX022A_REG_CNTL,
+ return regmap_clear_bits(data->regmap, data->chip_info->cntl,
KX022A_MASK_DRDY);
}
@@ -770,7 +777,7 @@ static int kx022a_fifo_disable(struct kx022a_data *data)
if (ret)
goto unlock_out;
- ret = regmap_clear_bits(data->regmap, KX022A_REG_BUF_CNTL2,
+ ret = regmap_clear_bits(data->regmap, data->chip_info->buf_cntl2,
KX022A_MASK_BUF_EN);
if (ret)
goto unlock_out;
@@ -811,7 +818,7 @@ static int kx022a_fifo_enable(struct kx022a_data *data)
goto unlock_out;
/* Enable buffer */
- ret = regmap_set_bits(data->regmap, KX022A_REG_BUF_CNTL2,
+ ret = regmap_set_bits(data->regmap, data->chip_info->buf_cntl2,
KX022A_MASK_BUF_EN);
if (ret)
goto unlock_out;
@@ -857,7 +864,7 @@ static irqreturn_t kx022a_trigger_handler(int irq, void *p)
struct kx022a_data *data = iio_priv(idev);
int ret;
- ret = regmap_bulk_read(data->regmap, KX022A_REG_XOUT_L, data->buffer,
+ ret = regmap_bulk_read(data->regmap, data->chip_info->xout_l, data->buffer,
KX022A_FIFO_SAMPLES_SIZE_BYTES);
if (ret < 0)
goto err_read;
@@ -905,7 +912,7 @@ static irqreturn_t kx022a_irq_thread_handler(int irq, void *private)
if (data->state & KX022A_STATE_FIFO) {
int ok;
- ok = __kx022a_fifo_flush(idev, KX022A_FIFO_LENGTH, true);
+ ok = __kx022a_fifo_flush(idev, data->chip_info->fifo_length, true);
if (ok > 0)
ret = IRQ_HANDLED;
}
@@ -958,7 +965,7 @@ static int kx022a_chip_init(struct kx022a_data *data)
int ret, val;
/* Reset the senor */
- ret = regmap_write(data->regmap, KX022A_REG_CNTL2, KX022A_MASK_SRST);
+ ret = regmap_write(data->regmap, data->chip_info->cntl2, KX022A_MASK_SRST);
if (ret)
return ret;
@@ -968,7 +975,7 @@ static int kx022a_chip_init(struct kx022a_data *data)
*/
msleep(1);
- ret = regmap_read_poll_timeout(data->regmap, KX022A_REG_CNTL2, val,
+ ret = regmap_read_poll_timeout(data->regmap, data->chip_info->cntl2, val,
!(val & KX022A_MASK_SRST),
KX022A_SOFT_RESET_WAIT_TIME_US,
KX022A_SOFT_RESET_TOTAL_WAIT_TIME_US);
@@ -978,14 +985,14 @@ static int kx022a_chip_init(struct kx022a_data *data)
return ret;
}
- ret = regmap_reinit_cache(data->regmap, &kx022a_regmap);
+ ret = regmap_reinit_cache(data->regmap, data->chip_info->regmap_config);
if (ret) {
dev_err(data->dev, "Failed to reinit reg cache\n");
return ret;
}
/* set data res 16bit */
- ret = regmap_set_bits(data->regmap, KX022A_REG_BUF_CNTL2,
+ ret = regmap_set_bits(data->regmap, data->chip_info->buf_cntl2,
KX022A_MASK_BRES16);
if (ret) {
dev_err(data->dev, "Failed to set data resolution\n");
@@ -995,7 +1002,31 @@ static int kx022a_chip_init(struct kx022a_data *data)
return kx022a_prepare_irq_pin(data);
}
-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);
+
+int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info)
{
static const char * const regulator_names[] = {"io-vdd", "vdd"};
struct iio_trigger *indio_trig;
@@ -1022,6 +1053,7 @@ int kx022a_probe_internal(struct device *dev)
return -ENOMEM;
data = iio_priv(idev);
+ data->chip_info = chip_info;
/*
* VDD is the analog and digital domain voltage supply and
@@ -1032,24 +1064,24 @@ int kx022a_probe_internal(struct device *dev)
if (ret && ret != -ENODEV)
return dev_err_probe(dev, ret, "failed to enable regulator\n");
- ret = regmap_read(regmap, KX022A_REG_WHO, &chip_id);
+ ret = regmap_read(regmap, chip_info->who, &chip_id);
if (ret)
return dev_err_probe(dev, ret, "Failed to access sensor\n");
- if (chip_id != KX022A_ID)
+ if (chip_id != chip_info->id)
dev_warn(dev, "unknown device 0x%x\n", chip_id);
irq = fwnode_irq_get_byname(fwnode, "INT1");
if (irq > 0) {
- data->inc_reg = KX022A_REG_INC1;
- data->ien_reg = KX022A_REG_INC4;
+ data->inc_reg = chip_info->inc1;
+ data->ien_reg = chip_info->inc4;
} else {
irq = fwnode_irq_get_byname(fwnode, "INT2");
if (irq <= 0)
return dev_err_probe(dev, irq, "No suitable IRQ\n");
- data->inc_reg = KX022A_REG_INC5;
- data->ien_reg = KX022A_REG_INC6;
+ data->inc_reg = chip_info->inc5;
+ data->ien_reg = chip_info->inc6;
}
data->regmap = regmap;
@@ -1058,9 +1090,9 @@ int kx022a_probe_internal(struct device *dev)
data->odr_ns = KX022A_DEFAULT_PERIOD_NS;
mutex_init(&data->mutex);
- idev->channels = kx022a_channels;
- idev->num_channels = ARRAY_SIZE(kx022a_channels);
- idev->name = "kx022-accel";
+ idev->channels = chip_info->channels;
+ idev->num_channels = chip_info->num_channels;
+ idev->name = chip_info->name;
idev->info = &kx022a_info;
idev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
idev->available_scan_masks = kx022a_scan_masks;
diff --git a/drivers/iio/accel/kionix-kx022a.h b/drivers/iio/accel/kionix-kx022a.h
index 12424649d438..3c31e7d88f78 100644
--- a/drivers/iio/accel/kionix-kx022a.h
+++ b/drivers/iio/accel/kionix-kx022a.h
@@ -76,7 +76,57 @@
struct device;
-int kx022a_probe_internal(struct device *dev);
-extern const struct regmap_config kx022a_regmap;
+/**
+ * struct kx022a_chip_info - Kionix accelerometer chip specific information
+ *
+ * @name: name of the device
+ * @regmap_config: pointer to register map configuration
+ * @channels: pointer to iio_chan_spec array
+ * @num_channels: number of iio_chan_spec channels
+ * @fifo_length: number of 16-bit samples in a full buffer
+ * @who: WHO_AM_I register
+ * @id: WHO_AM_I register value
+ * @cntl: control register 1
+ * @cntl2: control register 2
+ * @odcntl: output data control register
+ * @buf_cntl1: buffer control register 1
+ * @buf_cntl2: buffer control register 2
+ * @buf_clear: buffer clear register
+ * @buf_status1: buffer status register 1
+ * @buf_smp_lvl_mask: buffer sample level mask
+ * @buf_read: buffer read register
+ * @inc1: interrupt control register 1
+ * @inc4: interrupt control register 4
+ * @inc5: interrupt control register 5
+ * @inc6: interrupt control register 6
+ * @xout_l: x-axis output least significant byte
+ */
+struct kx022a_chip_info {
+ const char *name;
+ const struct regmap_config *regmap_config;
+ const struct iio_chan_spec *channels;
+ unsigned int num_channels;
+ unsigned int fifo_length;
+ u8 who;
+ u8 id;
+ u8 cntl;
+ u8 cntl2;
+ u8 odcntl;
+ u8 buf_cntl1;
+ u8 buf_cntl2;
+ u8 buf_clear;
+ u8 buf_status1;
+ u16 buf_smp_lvl_mask;
+ u8 buf_read;
+ u8 inc1;
+ u8 inc4;
+ u8 inc5;
+ u8 inc6;
+ u8 xout_l;
+};
+
+int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info);
+
+extern const struct kx022a_chip_info kx022a_chip_info;
#endif
--
2.30.2
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 <[email protected]>
> ---
> 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?
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.
> +
> 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.
All in all, looks nice and clean to me! Good job.
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
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 <[email protected]>
> > ---
> > 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 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 <[email protected]>
>>> ---
>>> 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
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
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 <[email protected]>
> > > > ---
> > > > 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 <[email protected]>
>>>>> ---
>>>>> 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
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On Sat, 29 Apr 2023 15:07:46 +0200
Mehdi Djait <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >>>>> ---
> >>>>> 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 <[email protected]> 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.
--
With Best Regards,
Andy Shevchenko
Hello Andy,
On Tue, May 02, 2023 at 10:41:31PM +0300, Andy Shevchenko wrote:
> On Sun, Apr 30, 2023 at 06:49:10PM +0100, Jonathan Cameron wrote:
> > On Sat, 29 Apr 2023 15:07:46 +0200
> > Mehdi Djait <[email protected]> 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.
I will change it in the next version
--
Kind Regards
Mehdi Djait
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
On 5/7/23 23:45, Mehdi Djait wrote:
> 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
I think that for the clarity and re-usability we would have fields for
functions. We could for example have separate fields for buffer enable
and resolution even though it means assigning the same register to both.
This, however, is somewhat wasteful (memory vise).
Problem with buf_cntl1 and buf_cntl2 is that the 'cntl' is too broad to
really tell what the control is. Also, what is the difference of
buf_cntl1 and buf_cntl2?
> 4. who, id, xout_l, odcntl, buf_{clear, status, read} are quite straightforward
I agree. These look pretty clear to me, although 'status' is also
somewhat ambiguous. (Is it sample level? Is it some corruption
detection? Can the buffer be 'busy'?).
> Anyway this is my opinion, what do you think ?
I can currently live with both of these approaches. We may need to
review this if/when adding support to other sensor(s) though. I will
leave the decision to you - just make the code best you can ;)
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~