Re: [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 3/17/23 01:48, Mehdi Djait wrote:
Refactor the kx022a driver implementation to make it more
generic and extensible.
Add the chip_info structure will to the driver's private
data to hold all the device specific infos.
Move the enum, struct and constants definitions to the header
file.

Signed-off-by: Mehdi Djait <mehdi.djait.k@xxxxxxxxx>
---
  drivers/iio/accel/kionix-kx022a-i2c.c |  19 +-
  drivers/iio/accel/kionix-kx022a-spi.c |  22 +-
  drivers/iio/accel/kionix-kx022a.c     | 289 ++++++++++++--------------
  drivers/iio/accel/kionix-kx022a.h     | 128 ++++++++++--
  4 files changed, 274 insertions(+), 184 deletions(-)

diff --git a/drivers/iio/accel/kionix-kx022a-i2c.c b/drivers/iio/accel/kionix-kx022a-i2c.c
index e6fd02d931b6..21c4c0ae1a68 100644
--- a/drivers/iio/accel/kionix-kx022a-i2c.c
+++ b/drivers/iio/accel/kionix-kx022a-i2c.c
@@ -15,23 +15,35 @@
  static int kx022a_i2c_probe(struct i2c_client *i2c)
  {
  	struct device *dev = &i2c->dev;
+	struct kx022a_chip_info *chip_info;
  	struct regmap *regmap;
+	const struct i2c_device_id *id = i2c_client_get_device_id(i2c);
if (!i2c->irq) {
  		dev_err(dev, "No IRQ configured\n");
  		return -EINVAL;
  	}
- regmap = devm_regmap_init_i2c(i2c, &kx022a_regmap);
+	chip_info = device_get_match_data(&i2c->dev);
+	if (!chip_info)
+		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");

Hm. I would like to pull the regmap_config out of the chip_info struct. As far as I see, the regmap_config is only needed in these bus specific files. On the other hand, the chip-info is only needed in the kionix-kx022a.c file, right?

So, maybe you could here just get the regmap_config based on the chip-id (enum value you added - the data pointer in match tables could be just the enum value indicating the IC type). Then, you could pass this enum value to kx022a_probe_internal() - and the chip-info struct could be selected in the kionix-kx022a.c based on it. That way you would not need the struct chip-info here or regmap_config in kionix-kx022a.c. Same in the *-spi.c

Something like:

enum {
	KIONIX_IC_KX022A,
	KIONIX_IC_KX132_xxx, /* xxx denotes accurate model suffix */
};
	
static const struct of_device_id kx022a_of_match[] = {
	{ .compatible = "kionix,kx022a", .data = KIONIX_IC_KX022A },
	...

chip_id = device_get_match_data(&i2c->dev);

regmap_cfg = kx022a_kx_regmap_cfg[chip_id];
regmap = devm_regmap_init_i2c(i2c, regmap_cfg);
...
return kx022a_probe_internal(dev, chip_id);

Do you think that would work?

OTOH, to really benefit from this we should probably pull out the regmap-configs from the kionix-kx022a.c. I am not really sure where we should put it then though. Hence, if there is no good ideas how to split the config and chip-info so they are only available/used where needed - then I am also Ok with the current approach.

-
-struct kx022a_data {
-	struct regmap *regmap;
-	struct iio_trigger *trig;
-	struct device *dev;
-	struct iio_mount_matrix orientation;
-	int64_t timestamp, old_timestamp;
-
-	int irq;
-	int inc_reg;
-	int ien_reg;
-
-	unsigned int state;
-	unsigned int odr_ns;
-
-	bool trigger_enabled;
-	/*
-	 * Prevent toggling the sensor stby/active state (PC1 bit) in the
-	 * middle of a configuration, or when the fifo is enabled. Also,
-	 * protect the data stored/retrieved from this structure from
-	 * concurrent accesses.
-	 */
-	struct mutex mutex;
-	u8 watermark;
-
-	/* 3 x 16bit accel data + timestamp */
-	__le16 buffer[8] __aligned(IIO_DMA_MINALIGN);
-	struct {
-		__le16 channels[3];
-		s64 ts __aligned(8);
-	} scan;
-};

As mentioned by Jonathan - It'd be better to keep this struct in C-file.

+const struct kx022a_chip_info kx_chip_info[] = {
+	[KX022A] = {
+		.name		  = "kx022a",
+		.type		  = KX022A,
+		.regmap_config	  = &kx022a_regmap_config,

As mentioned above, the regmap config is not really needed after the regmap is initialized. Id prefer this not being part of the chip info.

+		.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_smp_lvl_mask = KX022A_MASK_BUF_SMP_LVL,
+		.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(kx_chip_info, IIO_KX022A);
+
  static int kx022a_read_avail(struct iio_dev *indio_dev,
  			     struct iio_chan_spec const *chan,
  			     const int **vals, int *type, int *length,
@@ -309,19 +275,17 @@ static int kx022a_read_avail(struct iio_dev *indio_dev,
  	}
  }
-#define KX022A_DEFAULT_PERIOD_NS (20 * NSEC_PER_MSEC)
-
  static void kx022a_reg2freq(unsigned int val,  int *val1, int *val2)
  {
-	*val1 = kx022a_accel_samp_freq_table[val & KX022A_MASK_ODR][0];
-	*val2 = kx022a_accel_samp_freq_table[val & KX022A_MASK_ODR][1];
+	*val1 = kx022a_accel_samp_freq_table[val & KX_MASK_ODR][0];
+	*val2 = kx022a_accel_samp_freq_table[val & KX_MASK_ODR][1];
  }

As mentioned elsewhere, doing the renaming separately from the functional changes will ease the reviewing.


+static int kx022a_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;
+	}
+
+	buf_status &= data->chip_info->buf_smp_lvl_mask;
+	fifo_bytes = le16_to_cpu(buf_status);
+
+	/*
+	 * The KX022A has FIFO which can store 43 samples of HiRes data from 2
+	 * channels. This equals to 43 (samples) * 3 (channels) * 2 (bytes/sample) to
+	 * 258 bytes of sample data. The quirk to know is that the amount of bytes in
+	 * the FIFO is advertised via 8 bit register (max value 255). The thing to note
+	 * is that full 258 bytes of data is indicated using the max value 255.
+	 */
+	if (data->chip_info->type == KX022A && fifo_bytes == KX022A_FIFO_FULL_VALUE)
+		fifo_bytes = KX022A_FIFO_MAX_BYTES;
+
+	if (fifo_bytes % KX_FIFO_SAMPLES_SIZE_BYTES)
+		dev_warn(data->dev, "Bad FIFO alignment. Data may be corrupt\n");
+
+	return fifo_bytes;
+}

I like adding this function. Here I agree with Jonathan - having a device specific functions would clarify this a bit. The KX022A "quirk" is a bit confusing. You could then get rid of the buf_smp_lvl_mask.

+
  static int kx022a_drop_fifo_contents(struct kx022a_data *data)
  {
  	/*
@@ -593,35 +588,22 @@ 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,
  			       bool irq)
  {
  	struct kx022a_data *data = iio_priv(idev);
-	struct device *dev = regmap_get_device(data->regmap);
-	__le16 buffer[KX022A_FIFO_LENGTH * 3];
+	__le16 buffer[data->chip_info->fifo_length * 3];

I don't like this. Having the length of an array decided at run-time is not something I appreciate. Maybe you could just always reserve the memory so that the largest FIFO gets supported. I am just wondering how large arrays we can safely allocate from the stack?


@@ -812,14 +792,14 @@ static int kx022a_fifo_enable(struct kx022a_data *data)
  		goto unlock_out;
/* Enable buffer */
-	ret = regmap_set_bits(data->regmap, KX022A_REG_BUF_CNTL2,
-			      KX022A_MASK_BUF_EN);
+	ret = regmap_set_bits(data->regmap, data->chip_info->buf_cntl2,
+			      KX_MASK_BUF_EN);
  	if (ret)
  		goto unlock_out;
- data->state |= KX022A_STATE_FIFO;
+	data->state |= KX_STATE_FIFO;
  	ret = regmap_set_bits(data->regmap, data->ien_reg,
-			      KX022A_MASK_WMI);
+			      KX_MASK_WMI);

I think this fits to one line now. (even on my screen)

  	if (ret)
  		goto unlock_out;

-int kx022a_probe_internal(struct device *dev)
+int kx022a_probe_internal(struct device *dev, const struct kx022a_chip_info *chip_info)

As mentioned elsewhere, this might also work if the chip-type enum was passed here as parameter. That way the bus specific part would not need to know about the struct chip_info...

  {
  	static const char * const regulator_names[] = {"io-vdd", "vdd"};
  	struct iio_trigger *indio_trig;
@@ -1023,6 +1003,7 @@ int kx022a_probe_internal(struct device *dev)
  		return -ENOMEM;
data = iio_priv(idev);
+	data->chip_info = chip_info;

...Here you could then pick the correct chip_info based on the chip-type enum. In that case I'd like to get the regmap_config(s) in own file. Not sure how that would look like though.

All in all, I like how this looks like. Nice job!

Yours,
	-- Matti

--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux