Re: [RESEND v4 08/15] iio: adc: aspeed: Use model_data to set clk scaler.

Re: [RESEND v4 08/15] iio: adc: aspeed: Use model_data to set clk scaler.

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 6847 bytes --]

Hi Billy,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on iio/togreg]
[also build test WARNING on v5.14-rc7 next-20210824]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Billy-Tsai/Add-support-for-ast2600-ADC/20210825-082858
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: alpha-randconfig-r026-20210825 (attached as .config)
compiler: alpha-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/78a7c95363794cdf2453244b7e64b432d29d17f3
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Billy-Tsai/Add-support-for-ast2600-ADC/20210825-082858
        git checkout 78a7c95363794cdf2453244b7e64b432d29d17f3
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=alpha 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/iio/adc/aspeed_adc.c: In function 'aspeed_adc_probe':
>> drivers/iio/adc/aspeed_adc.c:225:9: warning: argument 3 null where non-null expected [-Wnonnull]
     225 |         snprintf(clk_parent_name, 32, of_clk_get_parent_name(pdev->dev.of_node, 0));
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   In file included from include/linux/clk.h:13,
                    from drivers/iio/adc/aspeed_adc.c:9:
   include/linux/kernel.h:204:5: note: in a call to function 'snprintf' declared 'nonnull'
     204 | int snprintf(char *buf, size_t size, const char *fmt, ...);
         |     ^~~~~~~~


vim +225 drivers/iio/adc/aspeed_adc.c

   200	
   201	static int aspeed_adc_probe(struct platform_device *pdev)
   202	{
   203		struct iio_dev *indio_dev;
   204		struct aspeed_adc_data *data;
   205		int ret;
   206		u32 adc_engine_control_reg_val;
   207		unsigned long scaler_flags = 0;
   208		char clk_name[32], clk_parent_name[32];
   209	
   210		indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*data));
   211		if (!indio_dev)
   212			return -ENOMEM;
   213	
   214		data = iio_priv(indio_dev);
   215		data->dev = &pdev->dev;
   216		data->model_data = of_device_get_match_data(&pdev->dev);
   217		platform_set_drvdata(pdev, indio_dev);
   218	
   219		data->base = devm_platform_ioremap_resource(pdev, 0);
   220		if (IS_ERR(data->base))
   221			return PTR_ERR(data->base);
   222	
   223		/* Register ADC clock prescaler with source specified by device tree. */
   224		spin_lock_init(&data->clk_lock);
 > 225		snprintf(clk_parent_name, 32, of_clk_get_parent_name(pdev->dev.of_node, 0));
   226		if (data->model_data->need_prescaler) {
   227			snprintf(clk_name, 32, "%s-prescaler",
   228				 data->model_data->model_name);
   229			data->clk_prescaler = clk_hw_register_divider(
   230				&pdev->dev, clk_name, clk_parent_name, 0,
   231				data->base + ASPEED_REG_CLOCK_CONTROL, 17, 15, 0,
   232				&data->clk_lock);
   233			if (IS_ERR(data->clk_prescaler))
   234				return PTR_ERR(data->clk_prescaler);
   235			snprintf(clk_parent_name, 32, clk_name);
   236			scaler_flags = CLK_SET_RATE_PARENT;
   237		}
   238		/*
   239		 * Register ADC clock scaler downstream from the prescaler. Allow rate
   240		 * setting to adjust the prescaler as well.
   241		 */
   242		snprintf(clk_name, 32, "%s-scaler", data->model_data->model_name);
   243		data->clk_scaler = clk_hw_register_divider(
   244			&pdev->dev, clk_name, clk_parent_name, scaler_flags,
   245			data->base + ASPEED_REG_CLOCK_CONTROL, 0,
   246			data->model_data->scaler_bit_width, 0, &data->clk_lock);
   247		if (IS_ERR(data->clk_scaler)) {
   248			ret = PTR_ERR(data->clk_scaler);
   249			goto scaler_error;
   250		}
   251	
   252		data->rst = devm_reset_control_get_exclusive(&pdev->dev, NULL);
   253		if (IS_ERR(data->rst)) {
   254			dev_err(&pdev->dev,
   255				"invalid or missing reset controller device tree entry");
   256			ret = PTR_ERR(data->rst);
   257			goto reset_error;
   258		}
   259		reset_control_deassert(data->rst);
   260	
   261		ret = aspeed_adc_vref_config(indio_dev);
   262		if (ret)
   263			goto vref_config_error;
   264	
   265		if (data->model_data->wait_init_sequence) {
   266			/* Enable engine in normal mode. */
   267			writel(FIELD_PREP(ASPEED_ADC_OP_MODE,
   268					  ASPEED_ADC_OP_MODE_NORMAL) |
   269				       ASPEED_ADC_ENGINE_ENABLE,
   270			       data->base + ASPEED_REG_ENGINE_CONTROL);
   271	
   272			/* Wait for initial sequence complete. */
   273			ret = readl_poll_timeout(data->base + ASPEED_REG_ENGINE_CONTROL,
   274						 adc_engine_control_reg_val,
   275						 adc_engine_control_reg_val &
   276						 ASPEED_ADC_CTRL_INIT_RDY,
   277						 ASPEED_ADC_INIT_POLLING_TIME,
   278						 ASPEED_ADC_INIT_TIMEOUT);
   279			if (ret)
   280				goto poll_timeout_error;
   281		}
   282	
   283		/* Start all channels in normal mode. */
   284		ret = clk_prepare_enable(data->clk_scaler->clk);
   285		if (ret)
   286			goto clk_enable_error;
   287	
   288		adc_engine_control_reg_val =
   289			ASPEED_ADC_CTRL_CHANNEL |
   290			FIELD_PREP(ASPEED_ADC_OP_MODE, ASPEED_ADC_OP_MODE_NORMAL) |
   291			ASPEED_ADC_ENGINE_ENABLE;
   292		writel(adc_engine_control_reg_val,
   293		       data->base + ASPEED_REG_ENGINE_CONTROL);
   294	
   295		indio_dev->name = data->model_data->model_name;
   296		indio_dev->info = &aspeed_adc_iio_info;
   297		indio_dev->modes = INDIO_DIRECT_MODE;
   298		indio_dev->channels = aspeed_adc_iio_channels;
   299		indio_dev->num_channels = data->model_data->num_channels;
   300	
   301		ret = iio_device_register(indio_dev);
   302		if (ret)
   303			goto iio_register_error;
   304	
   305		return 0;
   306	
   307	iio_register_error:
   308		writel(FIELD_PREP(ASPEED_ADC_OP_MODE, ASPEED_ADC_OP_MODE_PWR_DOWN),
   309		       data->base + ASPEED_REG_ENGINE_CONTROL);
   310		clk_disable_unprepare(data->clk_scaler->clk);
   311	clk_enable_error:
   312	poll_timeout_error:
   313	vref_config_error:
   314		reset_control_assert(data->rst);
   315	reset_error:
   316		clk_hw_unregister_divider(data->clk_scaler);
   317	scaler_error:
   318		if (data->model_data->need_prescaler)
   319			clk_hw_unregister_divider(data->clk_prescaler);
   320		return ret;
   321	}
   322	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27988 bytes --]

[RESEND v4 00/15] Add support for ast2600 ADC

[Billy Tsai]

This patch serials make aspeed_adc.c can support ast2600 and backward
compatible.

RESEND due to miss some patches when sent patch v4.
Change since v3:
dt-bindings:
  - Fix properties:aspeed,int_vref_mv type error.

Change since v2:
dt-bindings:
  - Create a new dt-bindings for ast2600 adc
aspeed_adc.c:
  - Splits the patch for more details
  - Remove version enum and use the flags in model data to distinguish
  hardware feature
  - Support trimming data get and set.
  - Use devm_add_action_or_reset to simplify probe error handling.

Changes since v1:
dt-bindings:
  - Fix the aspeed,adc.yaml check error.
  - Add battery-sensing property.
aspeed_adc.c:
  - Change the init flow:
    Clock and reference voltage setting should be completed before adc
    engine enable.
  - Change the default sampling rate to meet most user case.
  - Add patch #8 to suppoert battery sensing mode.

Billy Tsai (15):
  dt-bindings: iio: adc: Add ast2600-adc bindings
  iio: adc: aspeed: completes the bitfield declare.
  iio: adc: aspeed: set driver data when adc probe.
  iio: adc: aspeed: Keep model data to driver data.
  iio: adc: aspeed: Refactory model data structure
  iio: adc: aspeed: Add vref config function
  iio: adc: aspeed: Set num_channels with model data
  iio: adc: aspeed: Use model_data to set clk scaler.
  iio: adc: aspeed: Use devm_add_action_or_reset.
  iio: adc: aspeed: Support ast2600 adc.
  iio: adc: aspeed: Fix the calculate error of clock.
  iio: adc: aspeed: Add func to set sampling rate.
  iio: adc: aspeed: Add compensation phase.
  iio: adc: aspeed: Support battery sensing.
  iio: adc: aspeed: Get and set trimming data.

 .../bindings/iio/adc/aspeed,ast2600-adc.yaml  |  97 +++
 drivers/iio/adc/aspeed_adc.c                  | 562 +++++++++++++++---
 2 files changed, 569 insertions(+), 90 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/aspeed,ast2600-adc.yaml

-- 
2.25.1

[RESEND v4 08/15] iio: adc: aspeed: Use model_data to set clk scaler.

[Billy Tsai]

This patch use need_prescaler and scaler_bit_width to set the adc clock
scaler.

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

diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c
index 2d6215a91f99..52db38be9699 100644
--- a/drivers/iio/adc/aspeed_adc.c
+++ b/drivers/iio/adc/aspeed_adc.c
@@ -202,9 +202,10 @@ static int aspeed_adc_probe(struct platform_device *pdev)
 {
 	struct iio_dev *indio_dev;
 	struct aspeed_adc_data *data;
-	const char *clk_parent_name;
 	int ret;
 	u32 adc_engine_control_reg_val;
+	unsigned long scaler_flags = 0;
+	char clk_name[32], clk_parent_name[32];
 
 	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*data));
 	if (!indio_dev)
@@ -221,24 +222,28 @@ static int aspeed_adc_probe(struct platform_device *pdev)
 
 	/* Register ADC clock prescaler with source specified by device tree. */
 	spin_lock_init(&data->clk_lock);
-	clk_parent_name = of_clk_get_parent_name(pdev->dev.of_node, 0);
-
-	data->clk_prescaler = clk_hw_register_divider(
-				&pdev->dev, "prescaler", clk_parent_name, 0,
-				data->base + ASPEED_REG_CLOCK_CONTROL,
-				17, 15, 0, &data->clk_lock);
-	if (IS_ERR(data->clk_prescaler))
-		return PTR_ERR(data->clk_prescaler);
-
+	snprintf(clk_parent_name, 32, of_clk_get_parent_name(pdev->dev.of_node, 0));
+	if (data->model_data->need_prescaler) {
+		snprintf(clk_name, 32, "%s-prescaler",
+			 data->model_data->model_name);
+		data->clk_prescaler = clk_hw_register_divider(
+			&pdev->dev, clk_name, clk_parent_name, 0,
+			data->base + ASPEED_REG_CLOCK_CONTROL, 17, 15, 0,
+			&data->clk_lock);
+		if (IS_ERR(data->clk_prescaler))
+			return PTR_ERR(data->clk_prescaler);
+		snprintf(clk_parent_name, 32, clk_name);
+		scaler_flags = CLK_SET_RATE_PARENT;
+	}
 	/*
 	 * Register ADC clock scaler downstream from the prescaler. Allow rate
 	 * setting to adjust the prescaler as well.
 	 */
+	snprintf(clk_name, 32, "%s-scaler", data->model_data->model_name);
 	data->clk_scaler = clk_hw_register_divider(
-				&pdev->dev, "scaler", "prescaler",
-				CLK_SET_RATE_PARENT,
-				data->base + ASPEED_REG_CLOCK_CONTROL,
-				0, 10, 0, &data->clk_lock);
+		&pdev->dev, clk_name, clk_parent_name, scaler_flags,
+		data->base + ASPEED_REG_CLOCK_CONTROL, 0,
+		data->model_data->scaler_bit_width, 0, &data->clk_lock);
 	if (IS_ERR(data->clk_scaler)) {
 		ret = PTR_ERR(data->clk_scaler);
 		goto scaler_error;
@@ -310,7 +315,8 @@ static int aspeed_adc_probe(struct platform_device *pdev)
 reset_error:
 	clk_hw_unregister_divider(data->clk_scaler);
 scaler_error:
-	clk_hw_unregister_divider(data->clk_prescaler);
+	if (data->model_data->need_prescaler)
+		clk_hw_unregister_divider(data->clk_prescaler);
 	return ret;
 }
 
@@ -325,7 +331,8 @@ static int aspeed_adc_remove(struct platform_device *pdev)
 	clk_disable_unprepare(data->clk_scaler->clk);
 	reset_control_assert(data->rst);
 	clk_hw_unregister_divider(data->clk_scaler);
-	clk_hw_unregister_divider(data->clk_prescaler);
+	if (data->model_data->need_prescaler)
+		clk_hw_unregister_divider(data->clk_prescaler);
 
 	return 0;
 }
-- 
2.25.1

Re: [RESEND v4 08/15] iio: adc: aspeed: Use model_data to set clk scaler.

[Jonathan Cameron]

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

> This patch use need_prescaler and scaler_bit_width to set the adc clock
> scaler.
> 
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
Hi Billy,

One minor comment inline.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/aspeed_adc.c | 39 +++++++++++++++++++++---------------
>  1 file changed, 23 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c
> index 2d6215a91f99..52db38be9699 100644
> --- a/drivers/iio/adc/aspeed_adc.c
> +++ b/drivers/iio/adc/aspeed_adc.c
> @@ -202,9 +202,10 @@ static int aspeed_adc_probe(struct platform_device *pdev)
>  {
>  	struct iio_dev *indio_dev;
>  	struct aspeed_adc_data *data;
> -	const char *clk_parent_name;
>  	int ret;
>  	u32 adc_engine_control_reg_val;
> +	unsigned long scaler_flags = 0;
> +	char clk_name[32], clk_parent_name[32];
>  
>  	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*data));
>  	if (!indio_dev)
> @@ -221,24 +222,28 @@ static int aspeed_adc_probe(struct platform_device *pdev)
>  
>  	/* Register ADC clock prescaler with source specified by device tree. */
>  	spin_lock_init(&data->clk_lock);
> -	clk_parent_name = of_clk_get_parent_name(pdev->dev.of_node, 0);
> -
> -	data->clk_prescaler = clk_hw_register_divider(
> -				&pdev->dev, "prescaler", clk_parent_name, 0,
> -				data->base + ASPEED_REG_CLOCK_CONTROL,
> -				17, 15, 0, &data->clk_lock);
> -	if (IS_ERR(data->clk_prescaler))
> -		return PTR_ERR(data->clk_prescaler);
> -
> +	snprintf(clk_parent_name, 32, of_clk_get_parent_name(pdev->dev.of_node, 0));

ARRAY_SIZE(clk_parent_name) instead of 32.
Same for other places this pattern occurs.


> +	if (data->model_data->need_prescaler) {
> +		snprintf(clk_name, 32, "%s-prescaler",
> +			 data->model_data->model_name);
> +		data->clk_prescaler = clk_hw_register_divider(
> +			&pdev->dev, clk_name, clk_parent_name, 0,
> +			data->base + ASPEED_REG_CLOCK_CONTROL, 17, 15, 0,
> +			&data->clk_lock);
> +		if (IS_ERR(data->clk_prescaler))
> +			return PTR_ERR(data->clk_prescaler);
> +		snprintf(clk_parent_name, 32, clk_name);
> +		scaler_flags = CLK_SET_RATE_PARENT;
> +	}
>  	/*
>  	 * Register ADC clock scaler downstream from the prescaler. Allow rate
>  	 * setting to adjust the prescaler as well.
>  	 */
> +	snprintf(clk_name, 32, "%s-scaler", data->model_data->model_name);
>  	data->clk_scaler = clk_hw_register_divider(
> -				&pdev->dev, "scaler", "prescaler",
> -				CLK_SET_RATE_PARENT,
> -				data->base + ASPEED_REG_CLOCK_CONTROL,
> -				0, 10, 0, &data->clk_lock);
> +		&pdev->dev, clk_name, clk_parent_name, scaler_flags,
> +		data->base + ASPEED_REG_CLOCK_CONTROL, 0,
> +		data->model_data->scaler_bit_width, 0, &data->clk_lock);
>  	if (IS_ERR(data->clk_scaler)) {
>  		ret = PTR_ERR(data->clk_scaler);
>  		goto scaler_error;
> @@ -310,7 +315,8 @@ static int aspeed_adc_probe(struct platform_device *pdev)
>  reset_error:
>  	clk_hw_unregister_divider(data->clk_scaler);
>  scaler_error:
> -	clk_hw_unregister_divider(data->clk_prescaler);
> +	if (data->model_data->need_prescaler)
> +		clk_hw_unregister_divider(data->clk_prescaler);
>  	return ret;
>  }
>  
> @@ -325,7 +331,8 @@ static int aspeed_adc_remove(struct platform_device *pdev)
>  	clk_disable_unprepare(data->clk_scaler->clk);
>  	reset_control_assert(data->rst);
>  	clk_hw_unregister_divider(data->clk_scaler);
> -	clk_hw_unregister_divider(data->clk_prescaler);
> +	if (data->model_data->need_prescaler)
> +		clk_hw_unregister_divider(data->clk_prescaler);
>  
>  	return 0;
>  }