Messages in this thread Patch in this message | | | Date | Mon, 20 Mar 2023 11:35:06 +0200 | From | Matti Vaittinen <> | Subject | Re: [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure |
| |
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@gmail.com> > --- > 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! ~~
| |