diff mbox series

[RESEND,v4,12/15] iio: adc: aspeed: Add func to set sampling rate.

Message ID 202108250003.17P03KRU092474@twspam01.aspeedtech.com (mailing list archive)
State Changes Requested
Headers show
Series Add support for ast2600 ADC | expand

Commit Message

Billy Tsai Aug. 24, 2021, 9:12 a.m. UTC 
Add the function to set the sampling rate and keep the sampling period
for a driver used to wait the lastest value.

Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
---
 drivers/iio/adc/aspeed_adc.c | 58 +++++++++++++++++++++++++-----------
 1 file changed, 40 insertions(+), 18 deletions(-)

Comments

Jonathan Cameron Aug. 29, 2021, 3:36 p.m. UTC | #1 
On Tue, 24 Aug 2021 17:12:40 +0800
Billy Tsai <billy_tsai@aspeedtech.com> wrote:

> Add the function to set the sampling rate and keep the sampling period
> for a driver used to wait the lastest value.
> 
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>

Why move the code as well as factoring out the setter function?
I doubt it does any harm, but I'd like to understand why you did it.

Jonathan


> ---
>  drivers/iio/adc/aspeed_adc.c | 58 +++++++++++++++++++++++++-----------
>  1 file changed, 40 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c
> index 8fe7da1a651f..4d979dd7fe88 100644
> --- a/drivers/iio/adc/aspeed_adc.c
> +++ b/drivers/iio/adc/aspeed_adc.c
> @@ -65,6 +65,12 @@
>  
>  #define ASPEED_ADC_INIT_POLLING_TIME	500
>  #define ASPEED_ADC_INIT_TIMEOUT		500000
> +/*
> + * When the sampling rate is too high, the ADC may not have enough charging
> + * time, resulting in a low voltage value. Thus, default use slow sampling
> + * rate for most user case.
> + */
> +#define ASPEED_ADC_DEF_SAMPLING_RATE	65000
>  
>  struct aspeed_adc_model_data {
>  	const char *model_name;
> @@ -88,6 +94,7 @@ struct aspeed_adc_data {
>  	struct clk_hw		*clk_scaler;
>  	struct reset_control	*rst;
>  	int			vref;
> +	u32			sample_period_ns;
>  };
>  
>  #define ASPEED_CHAN(_idx, _data_reg_addr) {			\
> @@ -119,6 +126,24 @@ static const struct iio_chan_spec aspeed_adc_iio_channels[] = {
>  	ASPEED_CHAN(15, 0x2E),
>  };
>  
> +static int aspeed_adc_set_sampling_rate(struct iio_dev *indio_dev, u32 rate)
> +{
> +	struct aspeed_adc_data *data = iio_priv(indio_dev);
> +
> +	if (rate < data->model_data->min_sampling_rate ||
> +	    rate > data->model_data->max_sampling_rate)
> +		return -EINVAL;
> +	/* Each sampling needs 12 clocks to covert.*/

convert.  Please run a spell checker over these patches.

> +	clk_set_rate(data->clk_scaler->clk, rate * ASPEED_CLOCKS_PER_SAMPLE);
> +	rate = clk_get_rate(data->clk_scaler->clk);
> +	data->sample_period_ns = DIV_ROUND_UP_ULL(
> +		(u64)NSEC_PER_SEC * ASPEED_CLOCKS_PER_SAMPLE, rate);
> +	dev_dbg(data->dev, "Adc clock = %d sample period = %d ns", rate,
> +		data->sample_period_ns);
> +
> +	return 0;
> +}
> +
>  static int aspeed_adc_read_raw(struct iio_dev *indio_dev,
>  			       struct iio_chan_spec const *chan,
>  			       int *val, int *val2, long mask)
> @@ -149,17 +174,10 @@ static int aspeed_adc_write_raw(struct iio_dev *indio_dev,
>  				struct iio_chan_spec const *chan,
>  				int val, int val2, long mask)
>  {
> -	struct aspeed_adc_data *data = iio_priv(indio_dev);
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SAMP_FREQ:
> -		if (val < data->model_data->min_sampling_rate ||
> -			val > data->model_data->max_sampling_rate)
> -			return -EINVAL;
> -
> -		clk_set_rate(data->clk_scaler->clk,
> -				val * ASPEED_CLOCKS_PER_SAMPLE);
> -		return 0;
> +		return aspeed_adc_set_sampling_rate(indio_dev, val);
>  
>  	case IIO_CHAN_INFO_SCALE:
>  	case IIO_CHAN_INFO_RAW:
> @@ -386,6 +404,20 @@ static int aspeed_adc_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	ret = clk_prepare_enable(data->clk_scaler->clk);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_add_action_or_reset(data->dev,
> +				       aspeed_adc_clk_disable_unprepare,
> +				       data->clk_scaler->clk);
> +	if (ret)
> +		return ret;
> +
> +	ret = aspeed_adc_set_sampling_rate(indio_dev, ASPEED_ADC_DEF_SAMPLING_RATE);
> +	if (ret)
> +		return ret;
> +
>  	ret = aspeed_adc_vref_config(indio_dev);
>  	if (ret)
>  		return ret;
> @@ -413,16 +445,6 @@ static int aspeed_adc_probe(struct platform_device *pdev)
>  	}
>  
>  	/* Start all channels in normal mode. */

Why move this code up?

> -	ret = clk_prepare_enable(data->clk_scaler->clk);
> -	if (ret)
> -		return ret;
> -
> -	ret = devm_add_action_or_reset(data->dev,
> -				       aspeed_adc_clk_disable_unprepare,
> -				       data->clk_scaler->clk);
> -	if (ret)
> -		return ret;
> -
>  	adc_engine_control_reg_val =
>  		readl(data->base + ASPEED_REG_ENGINE_CONTROL);
>  	adc_engine_control_reg_val |=
Billy Tsai Aug. 30, 2021, 8:35 a.m. UTC | #2 
Hi Jonathan,

On 2021/8/29, 11:33 PM, "Jonathan Cameron" <jic23@kernel.org> wrote:

    On Tue, 24 Aug 2021 17:12:40 +0800
    Billy Tsai <billy_tsai@aspeedtech.com> wrote:

    >> Add the function to set the sampling rate and keep the sampling period
    >> for a driver used to wait the lastest value.
    >> 
    >> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>

    > Why move the code as well as factoring out the setter function?
    > I doubt it does any harm, but I'd like to understand why you did it.

    > Jonathan

    >> +	ret = clk_prepare_enable(data->clk_scaler->clk);
    >> +	if (ret)
    >> +		return ret;
    >> +
    >> +	ret = devm_add_action_or_reset(data->dev,
    >> +				       aspeed_adc_clk_disable_unprepare,
    >> +				       data->clk_scaler->clk);
    >> +	if (ret)
    >> +		return ret;
    >> +
    >> +	ret = aspeed_adc_set_sampling_rate(indio_dev, ASPEED_ADC_DEF_SAMPLING_RATE);
    >> +	if (ret)
    >> +		return ret;
    >> +
    >>  	ret = aspeed_adc_vref_config(indio_dev);
    >>  	if (ret)
    >>  		return ret;
    >> @@ -413,16 +445,6 @@ static int aspeed_adc_probe(struct platform_device *pdev)
    >>  	}
    >>  
    >>  	/* Start all channels in normal mode. */

    > Why move this code up?

Because the ADC clock is required when initializing the ADC device.
In our system, the clock is always on. Thus, the legacy driver won't encounter any issues.
I move the clk_prepare_enable ahead of initializing phase for making the driver probe logically closer to the hardware. 

    >> -	ret = clk_prepare_enable(data->clk_scaler->clk);
    >> -	if (ret)
    >> -		return ret;
    >> -
    >> -	ret = devm_add_action_or_reset(data->dev,
    >> -				       aspeed_adc_clk_disable_unprepare,
    >> -				       data->clk_scaler->clk);
    >> -	if (ret)
    >> -		return ret;
    >> -
    >>  	adc_engine_control_reg_val =
    >>  		readl(data->base + ASPEED_REG_ENGINE_CONTROL);
    >>  	adc_engine_control_reg_val |=


Best Regards,
Billy Tsai
Jonathan Cameron Aug. 30, 2021, 9:52 a.m. UTC | #3 
On Mon, 30 Aug 2021 08:35:53 +0000
Billy Tsai <billy_tsai@aspeedtech.com> wrote:

> Hi Jonathan,
> 
> On 2021/8/29, 11:33 PM, "Jonathan Cameron" <jic23@kernel.org> wrote:
> 
>     On Tue, 24 Aug 2021 17:12:40 +0800
>     Billy Tsai <billy_tsai@aspeedtech.com> wrote:
> 
>     >> Add the function to set the sampling rate and keep the sampling period
>     >> for a driver used to wait the lastest value.
>     >> 
>     >> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>  
> 
>     > Why move the code as well as factoring out the setter function?
>     > I doubt it does any harm, but I'd like to understand why you did it.  
> 
>     > Jonathan  
> 
>     >> +	ret = clk_prepare_enable(data->clk_scaler->clk);
>     >> +	if (ret)
>     >> +		return ret;
>     >> +
>     >> +	ret = devm_add_action_or_reset(data->dev,
>     >> +				       aspeed_adc_clk_disable_unprepare,
>     >> +				       data->clk_scaler->clk);
>     >> +	if (ret)
>     >> +		return ret;
>     >> +
>     >> +	ret = aspeed_adc_set_sampling_rate(indio_dev, ASPEED_ADC_DEF_SAMPLING_RATE);
>     >> +	if (ret)
>     >> +		return ret;
>     >> +
>     >>  	ret = aspeed_adc_vref_config(indio_dev);
>     >>  	if (ret)
>     >>  		return ret;
>     >> @@ -413,16 +445,6 @@ static int aspeed_adc_probe(struct platform_device *pdev)
>     >>  	}
>     >>  
>     >>  	/* Start all channels in normal mode. */  
> 
>     > Why move this code up?  
> 
> Because the ADC clock is required when initializing the ADC device.
> In our system, the clock is always on. Thus, the legacy driver won't encounter any issues.
> I move the clk_prepare_enable ahead of initializing phase for making the driver probe logically closer to the hardware. 

Thanks. Please add something to the patch description to say this.

Jonathan

> 
>     >> -	ret = clk_prepare_enable(data->clk_scaler->clk);
>     >> -	if (ret)
>     >> -		return ret;
>     >> -
>     >> -	ret = devm_add_action_or_reset(data->dev,
>     >> -				       aspeed_adc_clk_disable_unprepare,
>     >> -				       data->clk_scaler->clk);
>     >> -	if (ret)
>     >> -		return ret;
>     >> -
>     >>  	adc_engine_control_reg_val =
>     >>  		readl(data->base + ASPEED_REG_ENGINE_CONTROL);
>     >>  	adc_engine_control_reg_val |=  
> 
> 
> Best Regards,
> Billy Tsai
>
diff mbox series

Patch

diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c
index 8fe7da1a651f..4d979dd7fe88 100644
--- a/drivers/iio/adc/aspeed_adc.c
+++ b/drivers/iio/adc/aspeed_adc.c
@@ -65,6 +65,12 @@ 
 
 #define ASPEED_ADC_INIT_POLLING_TIME	500
 #define ASPEED_ADC_INIT_TIMEOUT		500000
+/*
+ * When the sampling rate is too high, the ADC may not have enough charging
+ * time, resulting in a low voltage value. Thus, default use slow sampling
+ * rate for most user case.
+ */
+#define ASPEED_ADC_DEF_SAMPLING_RATE	65000
 
 struct aspeed_adc_model_data {
 	const char *model_name;
@@ -88,6 +94,7 @@  struct aspeed_adc_data {
 	struct clk_hw		*clk_scaler;
 	struct reset_control	*rst;
 	int			vref;
+	u32			sample_period_ns;
 };
 
 #define ASPEED_CHAN(_idx, _data_reg_addr) {			\
@@ -119,6 +126,24 @@  static const struct iio_chan_spec aspeed_adc_iio_channels[] = {
 	ASPEED_CHAN(15, 0x2E),
 };
 
+static int aspeed_adc_set_sampling_rate(struct iio_dev *indio_dev, u32 rate)
+{
+	struct aspeed_adc_data *data = iio_priv(indio_dev);
+
+	if (rate < data->model_data->min_sampling_rate ||
+	    rate > data->model_data->max_sampling_rate)
+		return -EINVAL;
+	/* Each sampling needs 12 clocks to covert.*/
+	clk_set_rate(data->clk_scaler->clk, rate * ASPEED_CLOCKS_PER_SAMPLE);
+	rate = clk_get_rate(data->clk_scaler->clk);
+	data->sample_period_ns = DIV_ROUND_UP_ULL(
+		(u64)NSEC_PER_SEC * ASPEED_CLOCKS_PER_SAMPLE, rate);
+	dev_dbg(data->dev, "Adc clock = %d sample period = %d ns", rate,
+		data->sample_period_ns);
+
+	return 0;
+}
+
 static int aspeed_adc_read_raw(struct iio_dev *indio_dev,
 			       struct iio_chan_spec const *chan,
 			       int *val, int *val2, long mask)
@@ -149,17 +174,10 @@  static int aspeed_adc_write_raw(struct iio_dev *indio_dev,
 				struct iio_chan_spec const *chan,
 				int val, int val2, long mask)
 {
-	struct aspeed_adc_data *data = iio_priv(indio_dev);
 
 	switch (mask) {
 	case IIO_CHAN_INFO_SAMP_FREQ:
-		if (val < data->model_data->min_sampling_rate ||
-			val > data->model_data->max_sampling_rate)
-			return -EINVAL;
-
-		clk_set_rate(data->clk_scaler->clk,
-				val * ASPEED_CLOCKS_PER_SAMPLE);
-		return 0;
+		return aspeed_adc_set_sampling_rate(indio_dev, val);
 
 	case IIO_CHAN_INFO_SCALE:
 	case IIO_CHAN_INFO_RAW:
@@ -386,6 +404,20 @@  static int aspeed_adc_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ret = clk_prepare_enable(data->clk_scaler->clk);
+	if (ret)
+		return ret;
+
+	ret = devm_add_action_or_reset(data->dev,
+				       aspeed_adc_clk_disable_unprepare,
+				       data->clk_scaler->clk);
+	if (ret)
+		return ret;
+
+	ret = aspeed_adc_set_sampling_rate(indio_dev, ASPEED_ADC_DEF_SAMPLING_RATE);
+	if (ret)
+		return ret;
+
 	ret = aspeed_adc_vref_config(indio_dev);
 	if (ret)
 		return ret;
@@ -413,16 +445,6 @@  static int aspeed_adc_probe(struct platform_device *pdev)
 	}
 
 	/* Start all channels in normal mode. */
-	ret = clk_prepare_enable(data->clk_scaler->clk);
-	if (ret)
-		return ret;
-
-	ret = devm_add_action_or_reset(data->dev,
-				       aspeed_adc_clk_disable_unprepare,
-				       data->clk_scaler->clk);
-	if (ret)
-		return ret;
-
 	adc_engine_control_reg_val =
 		readl(data->base + ASPEED_REG_ENGINE_CONTROL);
 	adc_engine_control_reg_val |=