LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Billy Tsai <billy_tsai@aspeedtech.com>
Cc: <jic23@kernel.org>, <lars@metafoo.de>, <pmeerw@pmeerw.net>,
	<robh+dt@kernel.org>, <joel@jms.id.au>, <andrew@aj.id.au>,
	<p.zabel@pengutronix.de>, <linux-iio@vger.kernel.org>,
	<devicetree@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-aspeed@lists.ozlabs.org>, <linux-kernel@vger.kernel.org>,
	<BMC-SW@aspeedtech.com>
Subject: Re: [v2 6/8] iio: adc: aspeed: Add compensation phase.
Date: Fri, 23 Jul 2021 16:44:13 +0100	[thread overview]
Message-ID: <20210723164413.00003de8@Huawei.com> (raw)
In-Reply-To: <20210723081621.29477-7-billy_tsai@aspeedtech.com>

On Fri, 23 Jul 2021 16:16:19 +0800
Billy Tsai <billy_tsai@aspeedtech.com> wrote:

> This patch adds a compensation phase to improve the accurate of adc

ADC

> measurement. This is the builtin function though input half of the

built-in 

> reference voltage to get the adc offset.

ADC

> 
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> ---
>  drivers/iio/adc/aspeed_adc.c | 52 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 50 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c
> index bb6100228cae..0153b28b83b7 100644
> --- a/drivers/iio/adc/aspeed_adc.c
> +++ b/drivers/iio/adc/aspeed_adc.c
> @@ -61,6 +61,7 @@
>   * rate for most user case.
>   */
>  #define ASPEED_ADC_DEF_SAMPLING_RATE	65000
> +#define ASPEED_ADC_MAX_RAW_DATA		GENMASK(9, 0)
>  
>  enum aspeed_adc_version {
>  	aspeed_adc_ast2400,
> @@ -84,6 +85,7 @@ struct aspeed_adc_data {
>  	struct reset_control	*rst;
>  	int			vref;
>  	u32			sample_period_ns;
> +	int			cv;
>  };
>  
>  #define ASPEED_CHAN(_idx, _data_reg_addr) {			\
> @@ -115,6 +117,48 @@ static const struct iio_chan_spec aspeed_adc_iio_channels[] = {
>  	ASPEED_CHAN(15, 0x2E),
>  };
>  
> +static int aspeed_adc_compensation(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);

Same comment as previous patches.  pdev doesn't seem to be the best thing
to pass into these functions.

> +	struct aspeed_adc_data *data = iio_priv(indio_dev);
> +	u32 index, adc_raw = 0;
> +	u32 adc_engine_control_reg_val =
> +		readl(data->base + ASPEED_REG_ENGINE_CONTROL);

blank line here. In this case I would suggest

	u32 adc_engine_control_reg_val;

	adc_engine_control_reg_val = readl(...)
	
	adc_engine_control_reg_val |= ...

Whilst we are hear, I'd normally also expect to see a mask to ensure that
we have no stray bits set.  In this particular case MODE_NORMAL is the mask
but the reviewer shoudn't need to check that!

> +	adc_engine_control_reg_val |=
> +		(ASPEED_ADC_OPERATION_MODE_NORMAL | ASPEED_ADC_ENGINE_ENABLE);
> +
> +	/*
> +	 * Enable compensating sensing:
> +	 * After that, the input voltage of adc will force to half of the reference

"ADC" in all places it appears in comments.

> +	 * voltage. So the expected reading raw data will become half of the max
> +	 * value. We can get compensating value = 0x200 - adc read raw value.
> +	 * It is recommended to average at least 10 samples to get a final CV.
> +	 */
> +	writel(adc_engine_control_reg_val | ASPEED_ADC_CTRL_COMPENSATION |
> +		       ASPEED_ADC_CTRL_CHANNEL_ENABLE(0),
> +	       data->base + ASPEED_REG_ENGINE_CONTROL);
> +	/*
> +	 * After enable compensating sensing mode need to wait some time for adc stable
> +	 * Experiment result is 1ms.
> +	 */
> +	mdelay(1);
> +
> +	for (index = 0; index < 16; index++) {
> +		/*
> +		 * Waiting for the sampling period ensures that the value acquired
> +		 * is fresh each time.
> +		 */
> +		ndelay(data->sample_period_ns);
> +		adc_raw += readw(data->base + aspeed_adc_iio_channels[0].address);
> +	}
> +	adc_raw >>= 4;
> +	data->cv = BIT(ASPEED_RESOLUTION_BITS - 1) - adc_raw;
> +	writel(adc_engine_control_reg_val,
> +	       data->base + ASPEED_REG_ENGINE_CONTROL);
> +	dev_dbg(data->dev, "compensating value = %d\n", data->cv);

Blank line here.

> +	return 0;
> +}
> +
>  static int aspeed_adc_set_sampling_rate(struct iio_dev *indio_dev, u32 rate)
>  {
>  	struct aspeed_adc_data *data = iio_priv(indio_dev);
> @@ -143,7 +187,11 @@ static int aspeed_adc_read_raw(struct iio_dev *indio_dev,
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> -		*val = readw(data->base + chan->address);
> +		*val = readw(data->base + chan->address) + data->cv;

We would normally express this as IIO_CHAN_INFO_OFFSET, thus allowing
userspace to see and apply the compensation offset.  It could also modify
it if necessary (perhaps some long term drift effect or temperature effect
might mean userspace has more info than the kernel).

> +		if (*val < 0)
> +			*val = 0;
> +		else if (*val >= ASPEED_ADC_MAX_RAW_DATA)
> +			*val = ASPEED_ADC_MAX_RAW_DATA;

Why clamp the value like this? I'm not sure I follow the logic. Is it
because some userspace might rely on the existing range?

>  		return IIO_VAL_INT;
>  
>  	case IIO_CHAN_INFO_SCALE:
> @@ -347,7 +395,7 @@ static int aspeed_adc_probe(struct platform_device *pdev)
>  		if (ret)
>  			goto poll_timeout_error;
>  	}
> -
Keep the blank line.

> +	aspeed_adc_compensation(pdev);
>  	adc_engine_control_reg_val =
>  		readl(data->base + ASPEED_REG_ENGINE_CONTROL);
>  	/* Start all channels in normal mode. */


  reply	other threads:[~2021-07-23 15:44 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-23  8:16 [v2 0/8] Add support for ast2600 ADC Billy Tsai
2021-07-23  8:16 ` [v2 1/8] dt-bindings: iio: adc: rename the aspeed adc yaml Billy Tsai
2021-07-23 14:44   ` Jonathan Cameron
2021-07-26  6:53     ` Billy Tsai
2021-07-29 20:31       ` Rob Herring
2021-07-31 17:27         ` Jonathan Cameron
2021-07-23  8:16 ` [v2 2/8] dt-bindings: iio: adc: Binding ast2600 adc Billy Tsai
2021-07-23 14:51   ` Jonathan Cameron
2021-07-26  7:21     ` Billy Tsai
2021-07-23  8:16 ` [v2 3/8] iio: adc: aspeed: completes the bitfield declare Billy Tsai
2021-07-23 14:55   ` Jonathan Cameron
2021-07-23  8:16 ` [v2 4/8] iio: adc: aspeed: Allow driver to support ast2600 Billy Tsai
2021-07-23 15:29   ` Jonathan Cameron
2021-07-23  8:16 ` [v2 5/8] iio: adc: aspeed: Add func to set sampling rate Billy Tsai
2021-07-23 15:37   ` Jonathan Cameron
2021-07-23  8:16 ` [v2 6/8] iio: adc: aspeed: Add compensation phase Billy Tsai
2021-07-23 15:44   ` Jonathan Cameron [this message]
2021-07-23  8:16 ` [v2 7/8] iio: adc: aspeed: Fix the calculate error of clock Billy Tsai
2021-07-23  8:16 ` [v2 8/8] iio: adc: aspeed: Support battery sensing Billy Tsai
2021-07-23 15:56   ` Jonathan Cameron
2021-07-26  6:54     ` Billy Tsai

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=20210723164413.00003de8@Huawei.com \
    --to=jonathan.cameron@huawei.com \
    --cc=BMC-SW@aspeedtech.com \
    --cc=andrew@aj.id.au \
    --cc=billy_tsai@aspeedtech.com \
    --cc=devicetree@vger.kernel.org \
    --cc=jic23@kernel.org \
    --cc=joel@jms.id.au \
    --cc=lars@metafoo.de \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-aspeed@lists.ozlabs.org \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=p.zabel@pengutronix.de \
    --cc=pmeerw@pmeerw.net \
    --cc=robh+dt@kernel.org \
    --subject='Re: [v2 6/8] iio: adc: aspeed: Add compensation phase.' \
    /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

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).