From: Jonathan Cameron <jic23@kernel.org>
To: Julien Stephan <jstephan@baylibre.com>
Cc: "Lars-Peter Clausen" <lars@metafoo.de>,
"Michael Hennerich" <Michael.Hennerich@analog.com>,
"Nuno Sá" <nuno.sa@analog.com>,
"David Lechner" <dlechner@baylibre.com>,
"Rob Herring" <robh@kernel.org>,
"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
"Conor Dooley" <conor+dt@kernel.org>,
"Liam Girdwood" <lgirdwood@gmail.com>,
"Mark Brown" <broonie@kernel.org>,
"kernel test robot" <lkp@intel.com>,
linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH RFC v6 10/10] iio: adc: ad7380: add support for resolution boost
Date: Mon, 6 May 2024 15:29:46 +0100 [thread overview]
Message-ID: <20240506152946.74f1438c@jic23-huawei> (raw)
In-Reply-To: <20240501-adding-new-ad738x-driver-v6-10-3c0741154728@baylibre.com>
On Wed, 01 May 2024 16:55:43 +0200
Julien Stephan <jstephan@baylibre.com> wrote:
> ad738x chips are able to use an additional 2 bits of resolution when
> using oversampling. The 14-bits chips can have up to 16 bits of
> resolution and the 16-bits chips can have up to 18 bits of resolution.
>
> In order to dynamically allow to enable/disable the resolution boost
> feature, we have to set iio realbits/storagebits to the maximum per chips.
> This means that for iio, data will always be on the higher resolution
> available, and to cope with that we adjust the iio scale and iio offset
> depending on the resolution boost status.
>
> The available scales can be displayed using the regular _scale_available
> attributes and can be set by writing the regular _scale attribute.
> The available scales depend on the oversampling status.
>
> Signed-off-by: Julien Stephan <jstephan@baylibre.com>
>
> ---
>
> In order to support the resolution boost (additional 2 bits of resolution)
> we need to set realbits/storagebits to the maximum value i.e :
> - realbits = 16 + 2 = 18, and storagebits = 32 for 16-bits chips
> - realbits = 14 + 2 = 16, and storagebits = 16 for 14-bits chips
>
> For 14-bits chips this does not have a major impact, but this
> has the drawback of forcing 16-bits chip users to always use 32-bits
> words in buffers even if they are not using oversampling and resolution
> boost. Is there a better way of implementing this? For example
> implementing dynamic scan_type?
>
> Another issue is the location of the timestamps. I understood the need
> for ts to be consistent between chips, but right now I do not have a
> better solution.. I was thinking of maybe adding the timestamps at the
> beginning? That would imply to change the iio_chan_spec struct and maybe
> add a iio_push_to_buffers_with_timestamp_first() wrapper function? Is
> that an option?
Questions discussed in another branch of the thread.
Jonathan
>
> Any suggestion would be very much appreciated.
> ---
> drivers/iio/adc/ad7380.c | 226 ++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 194 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
> index 7b021bb9cf87..e240098708e9 100644
> --- a/drivers/iio/adc/ad7380.c
> +++ b/drivers/iio/adc/ad7380.c
> @@ -20,6 +20,7 @@
> #include <linux/err.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> +#include <linux/units.h>
> #include <linux/regmap.h>
> #include <linux/regulator/consumer.h>
> #include <linux/slab.h>
> @@ -58,6 +59,8 @@
> #define AD7380_CONFIG1_CRC_R BIT(4)
> #define AD7380_CONFIG1_ALERTEN BIT(3)
> #define AD7380_CONFIG1_RES BIT(2)
> +#define RESOLUTION_BOOST_DISABLE 0
> +#define RESOLUTION_BOOST_ENABLE 1
If the field is defined, the values should be obvious.
Also you used this as a boolean where simply passing in true
or false would be less confusing.
> #define AD7380_CONFIG1_REFSEL BIT(1)
> #define AD7380_CONFIG1_PMODE BIT(0)
>
> @@ -86,6 +89,14 @@ struct ad7380_chip_info {
> const struct ad7380_timing_specs *timing_specs;
> };
> @@ -259,6 +271,8 @@ struct ad7380_state {
> struct spi_device *spi;
> unsigned int oversampling_mode;
> unsigned int oversampling_ratio;
> + unsigned int scales[2][2];
> + bool resolution_boost_enable;
> struct regmap *regmap;
> unsigned int vref_mv;
> unsigned int vcm_mv[MAX_NUM_CHANNELS];
> @@ -270,7 +284,10 @@ struct ad7380_state {
> * As MAX_NUM_CHANNELS is 4 the layout of the structure is the same for all parts
> */
> struct {
> - u16 raw[MAX_NUM_CHANNELS];
> + union {
> + u16 u16[MAX_NUM_CHANNELS];
> + u32 u32[MAX_NUM_CHANNELS];
> + } raw;
>
> s64 ts __aligned(8);
As per earlier comments, roll this timestamp into the union as well
because it will move around.
> } scan_data __aligned(IIO_DMA_MINALIGN);
> @@ -359,23 +376,67 @@ static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg,
> unreachable();
> }
>
> +static int ad7380_set_resolution_boost(struct iio_dev *indio_dev, bool enable)
You pass 1 or 0 in here rather than true or false which would make more sense.
> +{
> + struct ad7380_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + if (st->resolution_boost_enable == enable)
> + return 0;
> +
> + ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG1,
> + AD7380_CONFIG1_RES,
> + FIELD_PREP(AD7380_CONFIG1_RES, enable));
Mapping true / false to 1 / 0 whilst correct doesn't give particularly readable
code. So useful to just have an
enable ? 1 : 0
in there to make the mapping more obvious.
> +
> + if (ret)
> + return ret;
> +
> + st->resolution_boost_enable = enable;
Trivial: blank line here.
> + return 0;
> +}
>
> static int ad7380_init(struct ad7380_state *st, struct regulator *vref)
> {
> int ret;
> @@ -691,12 +849,16 @@ static int ad7380_init(struct ad7380_state *st, struct regulator *vref)
> if (ret < 0)
> return ret;
>
> - /* Disable oversampling by default.
> - * This is the default value after reset,
> + /* Disable oversampling and resolution boost by default.
Follow through from earlier. Wrong comment syntax + wrap lines nearer 80 chars.
> + * This are the default values after reset,
> * so just initialize internal data
> */
next prev parent reply other threads:[~2024-05-06 14:30 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-01 14:55 [PATCH RFC v6 00/10] iio: adc: add new ad7380 driver Julien Stephan
2024-05-01 14:55 ` [PATCH RFC v6 01/10] dt-bindings: iio: adc: Add binding for AD7380 ADCs Julien Stephan
2024-05-01 14:55 ` [PATCH RFC v6 02/10] iio: adc: ad7380: new driver " Julien Stephan
2024-05-06 13:56 ` Jonathan Cameron
2024-05-01 14:55 ` [PATCH RFC v6 03/10] dt-bindings: iio: adc: ad7380: add pseudo-differential parts Julien Stephan
2024-05-01 14:55 ` [PATCH RFC v6 04/10] iio: adc: ad7380: add support for " Julien Stephan
2024-05-01 14:55 ` [PATCH RFC v6 05/10] iio: adc: ad7380: prepare for parts with more channels Julien Stephan
2024-05-01 14:55 ` [PATCH RFC v6 06/10] dt-bindings: iio: adc: ad7380: add support for ad738x-4 4 channels variants Julien Stephan
2024-05-01 14:55 ` [PATCH RFC v6 07/10] " Julien Stephan
2024-05-01 14:55 ` [PATCH RFC v6 08/10] iio: adc: ad7380: add oversampling support Julien Stephan
2024-05-06 8:20 ` Nuno Sá
2024-05-06 14:05 ` Jonathan Cameron
2024-05-01 14:55 ` [PATCH RFC v6 09/10] iio: adc: ad7380: add support for rolling average oversampling mode Julien Stephan
2024-05-06 8:33 ` Nuno Sá
2024-05-06 14:17 ` Jonathan Cameron
2024-05-06 15:04 ` David Lechner
2024-05-08 11:25 ` Jonathan Cameron
2024-05-09 22:01 ` David Lechner
2024-05-11 11:47 ` Jonathan Cameron
2024-05-01 14:55 ` [PATCH RFC v6 10/10] iio: adc: ad7380: add support for resolution boost Julien Stephan
2024-05-06 8:55 ` Nuno Sá
2024-05-06 13:46 ` Jonathan Cameron
2024-05-06 14:20 ` Jonathan Cameron
2024-05-06 14:44 ` David Lechner
2024-05-08 11:32 ` Jonathan Cameron
2024-05-06 15:09 ` David Lechner
2024-05-06 14:29 ` Jonathan Cameron [this message]
2024-05-06 21:45 ` David Lechner
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=20240506152946.74f1438c@jic23-huawei \
--to=jic23@kernel.org \
--cc=Michael.Hennerich@analog.com \
--cc=broonie@kernel.org \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=dlechner@baylibre.com \
--cc=jstephan@baylibre.com \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=lars@metafoo.de \
--cc=lgirdwood@gmail.com \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=lkp@intel.com \
--cc=nuno.sa@analog.com \
--cc=robh@kernel.org \
/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 read-only IMAP folder(s) and NNTP newsgroup(s).