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

Matti Vaittinen
2023-03-20T09:35:33Z

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! ~~