Hello Matti,
> > +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.
my bad here, I should have made a separate patch and explained more ...
buf_smp_lvl_mask is essential because kionix products use different
number of bits to report "the number of data bytes that have been stored in the
sample buffer" using the registers BUF_STATUS_1 and BUF_STATUS_2
kx022a: 8bits
kx132: 10bits
kx12x: 11bits
kx126: 12bits
I think this function is quite generic and can be used for different
kionix devices:
- It reads BUF_STATUS_1 and BUF_STATUS_2 and then uses a chip specific
mask
- It takes care of the quirk of kx022a which is just a simple if statement
>
> > +
> > 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?
I was stupid enough to ignore the warnings...
I will take care of it in the v2
>
>
> > @@ -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!
Thank you for the feedback :)
--
Kind Regards
Mehdi Djait
