From: Mehdi Djait <mehdi.djait.k@gmail.com>
To: Matti Vaittinen <mazziesaccount@gmail.com>
Cc: jic23@kernel.org, krzysztof.kozlowski+dt@linaro.org,
andriy.shevchenko@linux.intel.com, linux-iio@vger.kernel.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure
Date: Tue, 21 Mar 2023 16:56:43 +0100 [thread overview]
Message-ID: <ZBnTuykAqse5vBhO@carbian> (raw)
In-Reply-To: <4c28925d-c07c-61b7-8863-9c00e6846687@gmail.com>
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
next prev parent reply other threads:[~2023-03-21 15:56 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-16 23:48 [PATCH 0/3] iio: accel: Add support for Kionix/ROHM KX132 accelerometer Mehdi Djait
2023-03-16 23:48 ` [PATCH 1/3] dt-bindings: iio: Add " Mehdi Djait
2023-03-19 15:54 ` Jonathan Cameron
2023-03-21 13:22 ` Mehdi Djait
2023-03-16 23:48 ` [PATCH 2/3] iio: accel: kionix-kx022a: Add chip_info structure Mehdi Djait
2023-03-17 1:01 ` kernel test robot
2023-03-17 4:57 ` kernel test robot
2023-03-17 12:06 ` Andy Shevchenko
2023-03-18 16:12 ` Mehdi Djait
2023-03-19 16:20 ` Jonathan Cameron
2023-03-20 7:17 ` Matti Vaittinen
2023-03-20 12:24 ` Jonathan Cameron
2023-03-21 15:39 ` Mehdi Djait
2023-03-20 9:35 ` Matti Vaittinen
2023-03-20 12:02 ` Andy Shevchenko
2023-03-20 12:34 ` Jonathan Cameron
2023-03-20 12:52 ` Matti Vaittinen
2023-03-21 15:56 ` Mehdi Djait [this message]
2023-03-22 6:37 ` Matti Vaittinen
2023-03-21 1:05 ` kernel test robot
2023-03-16 23:48 ` [PATCH 3/3] iio: accel: Add support for Kionix/ROHM KX132 accelerometer Mehdi Djait
2023-03-19 16:22 ` Jonathan Cameron
2023-03-21 16:34 ` Mehdi Djait
2023-03-25 18:12 ` Jonathan Cameron
2023-03-20 9:57 ` Matti Vaittinen
2023-03-17 12:07 ` [PATCH 0/3] " Andy Shevchenko
2023-03-18 15:55 ` Mehdi Djait
2023-03-19 7:43 ` Matti Vaittinen
2023-03-21 13:16 ` Mehdi Djait
2023-03-22 7:47 ` Matti Vaittinen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZBnTuykAqse5vBhO@carbian \
--to=mehdi.djait.k@gmail.com \
--cc=andriy.shevchenko@linux.intel.com \
--cc=devicetree@vger.kernel.org \
--cc=jic23@kernel.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mazziesaccount@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).