[PATCH 0/3] Make driver compatible with ast2600

[PATCH 0/3] Make driver compatible with ast2600

[Billy Tsai]

The ast2600 is a new generation of SoC from ASPEED.
The adc device in this generation adds some changes and features.
This patch series handles the changes below:
1. Define the new register fields.
2. Split into two individual IPs and each contains 8 voltage channels.
3. Remove the pre-scaler and extend the field length of the scaler.
4. Ref_voltage becomes configurable.

Billy Tsai (3):
  iio: adc: aspeed: Orgnaize and add the define of adc
  iio: adc: aspeed: Make driver compatible with ast2600
  iio: adc: aspeed: Setting ref_voltage in probe

 .../bindings/iio/adc/aspeed_adc.txt           |  16 +-
 drivers/iio/adc/aspeed_adc.c                  | 168 ++++++++++++++----
 2 files changed, 148 insertions(+), 36 deletions(-)

-- 
2.17.1

[PATCH 1/3] iio: adc: aspeed: Orgnaize and add the define of adc

[Billy Tsai]

This patch organizes the define of adc to multiple partitions
and adds the new bit field define for ast2600 driver.

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

diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c
index 1e5375235cfe..ae400c4d6d40 100644
--- a/drivers/iio/adc/aspeed_adc.c
+++ b/drivers/iio/adc/aspeed_adc.c
@@ -21,23 +21,57 @@
 #include <linux/iio/driver.h>
 #include <linux/iopoll.h>
 
+/**********************************************************
+ * ADC feature define
+ *********************************************************/
 #define ASPEED_RESOLUTION_BITS		10
 #define ASPEED_CLOCKS_PER_SAMPLE	12
 
+/**********************************************************
+ * ADC HW register offset define
+ *********************************************************/
 #define ASPEED_REG_ENGINE_CONTROL	0x00
 #define ASPEED_REG_INTERRUPT_CONTROL	0x04
 #define ASPEED_REG_VGA_DETECT_CONTROL	0x08
 #define ASPEED_REG_CLOCK_CONTROL	0x0C
+#define ASPEED_REG_COMPENSATION_TRIM	0xC4
 #define ASPEED_REG_MAX			0xC0
 
+/**********************************************************
+ * ADC register Bit field
+ *********************************************************/
+/*ENGINE_CONTROL */
+/* [0] */
+#define ASPEED_ENGINE_ENABLE		BIT(0)
+/* [3:1] */
 #define ASPEED_OPERATION_MODE_POWER_DOWN	(0x0 << 1)
 #define ASPEED_OPERATION_MODE_STANDBY		(0x1 << 1)
 #define ASPEED_OPERATION_MODE_NORMAL		(0x7 << 1)
-
-#define ASPEED_ENGINE_ENABLE		BIT(0)
-
+/* [4] */
+#define ASPEED_CTRL_COMPENSATION	BIT(4)
+/* [5] */
+#define ASPEED_AUTOPENSATING		BIT(5)
+/* [7:6] */
+#define ASPEED_REF_VOLTAGE_2500mV	(0 << 6)
+#define ASPEED_REF_VOLTAGE_1200mV	(1 << 6)
+#define ASPEED_REF_VOLTAGE_EXT_HIGH	(2 << 6)
+#define ASPEED_REF_VOLTAGE_EXT_LOW	(3 << 6)
+#define ASPEED_BATTERY_SENSING_VOL_DIVIDE_2_3	(0 << 6)
+#define ASPEED_BATTERY_SENSING_VOL_DIVIDE_1_3	(1 << 6)
+/* [8] */
 #define ASPEED_ADC_CTRL_INIT_RDY	BIT(8)
-
+/* [12] */
+#define ASPEED_ADC_CH7_VOLTAGE_NORMAL	(0 << 12)
+#define ASPEED_ADC_CH7_VOLTAGE_BATTERY	(1 << 12)
+/* [13] */
+#define ASPEED_ADC_EN_BATTERY_SENSING	BIT(13)
+/* [31:16] */
+#define ASPEED_ADC_CTRL_CH_EN(n)	(1 << (16 + n))
+#define ASPEED_ADC_CTRL_CH_EN_ALL	GENMASK(31, 16)
+
+/**********************************************************
+ * Software setting
+ *********************************************************/
 #define ASPEED_ADC_INIT_POLLING_TIME	500
 #define ASPEED_ADC_INIT_TIMEOUT		500000
 
-- 
2.17.1

[PATCH 2/3] iio: adc: aspeed: Modify driver for ast2600

[Billy Tsai]

This patch is used to handle the difference between ast2600
and previous versions.

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

diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c
index ae400c4d6d40..fc4bbccf9348 100644
--- a/drivers/iio/adc/aspeed_adc.c
+++ b/drivers/iio/adc/aspeed_adc.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /*
- * Aspeed AST2400/2500 ADC
+ * Aspeed AST2400/2500/2600 ADC
  *
  * Copyright (C) 2017 Google, Inc.
  */
@@ -81,6 +81,7 @@ struct aspeed_adc_model_data {
 	unsigned int max_sampling_rate;	// Hz
 	unsigned int vref_voltage;	// mV
 	bool wait_init_sequence;
+	int num_channels;
 };
 
 struct aspeed_adc_data {
@@ -90,6 +91,7 @@ struct aspeed_adc_data {
 	struct clk_hw		*clk_prescaler;
 	struct clk_hw		*clk_scaler;
 	struct reset_control	*rst;
+	unsigned int vref_voltage;	// mV
 };
 
 #define ASPEED_CHAN(_idx, _data_reg_addr) {			\
@@ -126,8 +128,6 @@ static int aspeed_adc_read_raw(struct iio_dev *indio_dev,
 			       int *val, int *val2, long mask)
 {
 	struct aspeed_adc_data *data = iio_priv(indio_dev);
-	const struct aspeed_adc_model_data *model_data =
-			of_device_get_match_data(data->dev);
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
@@ -135,7 +135,7 @@ static int aspeed_adc_read_raw(struct iio_dev *indio_dev,
 		return IIO_VAL_INT;
 
 	case IIO_CHAN_INFO_SCALE:
-		*val = model_data->vref_voltage;
+		*val = data->vref_voltage;
 		*val2 = ASPEED_RESOLUTION_BITS;
 		return IIO_VAL_FRACTIONAL_LOG2;
 
@@ -208,8 +208,13 @@ static int aspeed_adc_probe(struct platform_device *pdev)
 	struct aspeed_adc_data *data;
 	const struct aspeed_adc_model_data *model_data;
 	const char *clk_parent_name;
+	char prescaler_clk_name[32];
+	char scaler_clk_name[32];
 	int ret;
 	u32 adc_engine_control_reg_val;
+	u32 ref_voltage_cfg = 0;
+
+	model_data = of_device_get_match_data(&pdev->dev);
 
 	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*data));
 	if (!indio_dev)
@@ -225,29 +230,75 @@ 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);
+	snprintf(prescaler_clk_name, sizeof(prescaler_clk_name),
+			"prescaler-%s", pdev->name);
+	snprintf(scaler_clk_name, sizeof(scaler_clk_name),
+			"scaler-%s", pdev->name);
+	if (!strcmp(model_data->model_name, "ast2400-adc") ||
+	    !strcmp(model_data->model_name, "ast2500-adc")) {
+		/* Divider config */
+		data->clk_prescaler = clk_hw_register_divider(
+				&pdev->dev, prescaler_clk_name, clk_parent_name,
+				0,
+				data->base + ASPEED_REG_CLOCK_CONTROL, 17, 15,
+				CLK_DIVIDER_ONE_BASED, &data->clk_lock);
+		if (IS_ERR(data->clk_prescaler))
+			return PTR_ERR(data->clk_prescaler);
 
-	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);
-
-	/*
-	 * Register ADC clock scaler downstream from the prescaler. Allow rate
-	 * setting to adjust the prescaler as well.
-	 */
-	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);
-	if (IS_ERR(data->clk_scaler)) {
-		ret = PTR_ERR(data->clk_scaler);
-		goto scaler_error;
+		/*
+		 * Register ADC clock scaler downstream from the prescaler. Allow rate
+		 * setting to adjust the prescaler as well.
+		 */
+		data->clk_scaler = clk_hw_register_divider(
+					&pdev->dev, scaler_clk_name, prescaler_clk_name,
+					CLK_SET_RATE_PARENT,
+					data->base + ASPEED_REG_CLOCK_CONTROL, 0, 10,
+					CLK_DIVIDER_ONE_BASED, &data->clk_lock);
+		if (IS_ERR(data->clk_scaler)) {
+			ret = PTR_ERR(data->clk_scaler);
+			goto scaler_error;
+		}
+		/* Get ref_voltage */
+		data->vref_voltage = model_data->vref_voltage;
+	} else if (!strcmp(model_data->model_name, "ast2600-adc")) {
+		/* Divider config */
+		data->clk_scaler = clk_hw_register_divider(
+			&pdev->dev, scaler_clk_name, clk_parent_name,
+			CLK_SET_RATE_UNGATE,
+			data->base + ASPEED_REG_CLOCK_CONTROL, 0, 16,
+			CLK_DIVIDER_ONE_BASED, &data->clk_lock);
+		if (IS_ERR(data->clk_scaler))
+			return PTR_ERR(data->clk_scaler);
+		/*
+		 * Get ref_voltage:
+		 * If reference voltage is between 1550~1650mv, we can set
+		 * fields either ASPEED_REF_VOLTAGE_EXT_HIGH or ASPEED_REF_VOLTAGE_EXT_LOW.
+		 * In this place, we select ASPEED_REF_VOLTAGE_EXT_HIGH as higher priority.
+		 */
+		if (!of_property_read_u32(pdev->dev.of_node, "ref_voltage",
+					  &data->vref_voltage)) {
+			if (data->vref_voltage == 2500)
+				ref_voltage_cfg = ASPEED_REF_VOLTAGE_2500mV;
+			else if (data->vref_voltage == 1200)
+				ref_voltage_cfg = ASPEED_REF_VOLTAGE_1200mV;
+			else if ((data->vref_voltage >= 1550) &&
+					(data->vref_voltage <= 2700))
+				ref_voltage_cfg = ASPEED_REF_VOLTAGE_EXT_HIGH;
+			else if ((data->vref_voltage >= 900) &&
+					(data->vref_voltage <= 1650))
+				ref_voltage_cfg = ASPEED_REF_VOLTAGE_EXT_LOW;
+			else {
+				dev_err(&pdev->dev, "ref_voltage property is out of range: %d\n",
+					data->vref_voltage);
+				return -EINVAL;
+			}
+		} else {
+			dev_err(&pdev->dev, "Couldn't read ref_voltage property\n");
+			return -EINVAL;
+		}
 	}
 
-	data->rst = devm_reset_control_get_exclusive(&pdev->dev, NULL);
+	data->rst = devm_reset_control_get_shared(&pdev->dev, NULL);
 	if (IS_ERR(data->rst)) {
 		dev_err(&pdev->dev,
 			"invalid or missing reset controller device tree entry");
@@ -256,13 +307,14 @@ static int aspeed_adc_probe(struct platform_device *pdev)
 	}
 	reset_control_deassert(data->rst);
 
-	model_data = of_device_get_match_data(&pdev->dev);
+	adc_engine_control_reg_val = readl(data->base + ASPEED_REG_ENGINE_CONTROL);
+	/* Enable engine in normal mode and set ref_voltage */
+	adc_engine_control_reg_val |= (ASPEED_OPERATION_MODE_NORMAL |
+				ASPEED_ENGINE_ENABLE | ref_voltage_cfg);
+	writel(adc_engine_control_reg_val,
+			data->base + ASPEED_REG_ENGINE_CONTROL);
 
 	if (model_data->wait_init_sequence) {
-		/* Enable engine in normal mode. */
-		writel(ASPEED_OPERATION_MODE_NORMAL | ASPEED_ENGINE_ENABLE,
-		       data->base + ASPEED_REG_ENGINE_CONTROL);
-
 		/* Wait for initial sequence complete. */
 		ret = readl_poll_timeout(data->base + ASPEED_REG_ENGINE_CONTROL,
 					 adc_engine_control_reg_val,
@@ -279,18 +331,16 @@ static int aspeed_adc_probe(struct platform_device *pdev)
 	if (ret)
 		goto clk_enable_error;
 
-	adc_engine_control_reg_val = GENMASK(31, 16) |
-		ASPEED_OPERATION_MODE_NORMAL | ASPEED_ENGINE_ENABLE;
+	adc_engine_control_reg_val |= ASPEED_ADC_CTRL_CH_EN_ALL;
 	writel(adc_engine_control_reg_val,
 		data->base + ASPEED_REG_ENGINE_CONTROL);
 
-	model_data = of_device_get_match_data(&pdev->dev);
 	indio_dev->name = model_data->model_name;
 	indio_dev->dev.parent = &pdev->dev;
 	indio_dev->info = &aspeed_adc_iio_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->channels = aspeed_adc_iio_channels;
-	indio_dev->num_channels = ARRAY_SIZE(aspeed_adc_iio_channels);
+	indio_dev->num_channels = model_data->num_channels;
 
 	ret = iio_device_register(indio_dev);
 	if (ret)
@@ -333,6 +383,7 @@ static const struct aspeed_adc_model_data ast2400_model_data = {
 	.vref_voltage = 2500, // mV
 	.min_sampling_rate = 10000,
 	.max_sampling_rate = 500000,
+	.num_channels = 16,
 };
 
 static const struct aspeed_adc_model_data ast2500_model_data = {
@@ -341,11 +392,21 @@ static const struct aspeed_adc_model_data ast2500_model_data = {
 	.min_sampling_rate = 1,
 	.max_sampling_rate = 1000000,
 	.wait_init_sequence = true,
+	.num_channels = 16,
+};
+
+static const struct aspeed_adc_model_data ast2600_model_data = {
+	.model_name = "ast2600-adc",
+	.min_sampling_rate = 1,
+	.max_sampling_rate = 1000000,
+	.wait_init_sequence = true,
+	.num_channels = 8,
 };
 
 static const struct of_device_id aspeed_adc_matches[] = {
 	{ .compatible = "aspeed,ast2400-adc", .data = &ast2400_model_data },
 	{ .compatible = "aspeed,ast2500-adc", .data = &ast2500_model_data },
+	{ .compatible = "aspeed,ast2600-adc", .data = &ast2600_model_data },
 	{},
 };
 MODULE_DEVICE_TABLE(of, aspeed_adc_matches);
@@ -362,5 +423,5 @@ static struct platform_driver aspeed_adc_driver = {
 module_platform_driver(aspeed_adc_driver);
 
 MODULE_AUTHOR("Rick Altherr <raltherr@google.com>");
-MODULE_DESCRIPTION("Aspeed AST2400/2500 ADC Driver");
+MODULE_DESCRIPTION("Aspeed AST2400/2500/2600 ADC Driver");
 MODULE_LICENSE("GPL");
-- 
2.17.1

[PATCH 3/3] iio: adc: aspeed: Setting ref_voltage in probe

[Billy Tsai]

At ast2600 ref_voltage becomes configurable and this property is board
dependency.

Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
---
 .../devicetree/bindings/iio/adc/aspeed_adc.txt   | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
index 034fc2ba100e..0ba1980c4e06 100644
--- a/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
+++ b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
@@ -3,8 +3,11 @@ Aspeed ADC
 This device is a 10-bit converter for 16 voltage channels.  All inputs are
 single ended.
 
+At ast2600, this device split into two individual IPs and each contains 8 voltage channels.
+
+Chip level dtsi:
 Required properties:
-- compatible: Should be "aspeed,ast2400-adc" or "aspeed,ast2500-adc"
+- compatible: Should be "aspeed,ast2400-adc" or "aspeed,ast2500-adc" or "aspeed,ast2600-adc"
 - reg: memory window mapping address and length
 - clocks: Input clock used to derive the sample clock. Expected to be the
           SoC's APB clock.
@@ -20,3 +23,14 @@ Example:
 		resets = <&syscon ASPEED_RESET_ADC>;
 		#io-channel-cells = <1>;
 	};
+
+Board level dts:
+Required properties:
+- ref_voltage: (ast2600 only)
+	- Reference voltage in millivolts for the conversions.
+	- The range of value is 900 to 2700 mv.
+
+Example:
+&adc0 {
+	ref_voltage = <2500>;
+};
\ No newline at end of file
-- 
2.17.1

Re: [PATCH 3/3] iio: adc: aspeed: Setting ref_voltage in probe

[Rob Herring]

On Tue, Oct 13, 2020 at 06:32:45PM +0800, Billy Tsai wrote:
> At ast2600 ref_voltage becomes configurable and this property is board
> dependency.
> 
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> ---
>  .../devicetree/bindings/iio/adc/aspeed_adc.txt   | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
> index 034fc2ba100e..0ba1980c4e06 100644
> --- a/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
> +++ b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
> @@ -3,8 +3,11 @@ Aspeed ADC
>  This device is a 10-bit converter for 16 voltage channels.  All inputs are
>  single ended.
>  
> +At ast2600, this device split into two individual IPs and each contains 8 voltage channels.
> +
> +Chip level dtsi:
>  Required properties:
> -- compatible: Should be "aspeed,ast2400-adc" or "aspeed,ast2500-adc"
> +- compatible: Should be "aspeed,ast2400-adc" or "aspeed,ast2500-adc" or "aspeed,ast2600-adc"
>  - reg: memory window mapping address and length
>  - clocks: Input clock used to derive the sample clock. Expected to be the
>            SoC's APB clock.
> @@ -20,3 +23,14 @@ Example:
>  		resets = <&syscon ASPEED_RESET_ADC>;
>  		#io-channel-cells = <1>;
>  	};
> +
> +Board level dts:

This split is convention, but not relevant to the binding.

> +Required properties:
> +- ref_voltage: (ast2600 only)

s/_/-/

And needs a vendor prefix.

> +	- Reference voltage in millivolts for the conversions.
> +	- The range of value is 900 to 2700 mv.
> +
> +Example:
> +&adc0 {
> +	ref_voltage = <2500>;
> +};
> \ No newline at end of file

Fix this.

> -- 
> 2.17.1
> 

Re: [PATCH 1/3] iio: adc: aspeed: Orgnaize and add the define of adc

[Jonathan Cameron]

On Tue, 13 Oct 2020 18:32:43 +0800
Billy Tsai <billy_tsai@aspeedtech.com> wrote:

> This patch organizes the define of adc to multiple partitions
> and adds the new bit field define for ast2600 driver.

Should be 2 patch patches.  If you need to do a reorg,
do it first, then add new bits in a second patch.  That way
we are reviewing one non functional change, and one that actually
does something.

As a general rule, I'd also prefer to just see the additional defines
added as and when they are used (in the patch that first uses them).

> 
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> ---
>  drivers/iio/adc/aspeed_adc.c | 42 ++++++++++++++++++++++++++++++++----
>  1 file changed, 38 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c
> index 1e5375235cfe..ae400c4d6d40 100644
> --- a/drivers/iio/adc/aspeed_adc.c
> +++ b/drivers/iio/adc/aspeed_adc.c
> @@ -21,23 +21,57 @@
>  #include <linux/iio/driver.h>
>  #include <linux/iopoll.h>
>  
> +/**********************************************************
> + * ADC feature define
> + *********************************************************/

I'm generally of the view that block comments like this
normally imply the defines are not sufficiently self
identifying.   It should be possible to know what sort of thing
they are at the point of use without having to go find a comment
in the header.
So if you think these are needed, perhaps reconsider the naming
of of the defines?  Personally I just don't seem them as necessary.
Like all comments, they tend to 'rot' over time so if they
aren't adding information, better not to have them!

>  #define ASPEED_RESOLUTION_BITS		10
>  #define ASPEED_CLOCKS_PER_SAMPLE	12
>  
> +/**********************************************************
> + * ADC HW register offset define
> + *********************************************************/
>  #define ASPEED_REG_ENGINE_CONTROL	0x00
>  #define ASPEED_REG_INTERRUPT_CONTROL	0x04
>  #define ASPEED_REG_VGA_DETECT_CONTROL	0x08
>  #define ASPEED_REG_CLOCK_CONTROL	0x0C
> +#define ASPEED_REG_COMPENSATION_TRIM	0xC4
>  #define ASPEED_REG_MAX			0xC0
>  
> +/**********************************************************
> + * ADC register Bit field
> + *********************************************************/
> +/*ENGINE_CONTROL */
Inconsistent spacing after /* 
> +/* [0] */
> +#define ASPEED_ENGINE_ENABLE		BIT(0)
> +/* [3:1] */

If the docs are needed, then better to use FIELD_PREP and GENMASK as that is
going to be self documenting at the actual point of the defines.

>  #define ASPEED_OPERATION_MODE_POWER_DOWN	(0x0 << 1)
>  #define ASPEED_OPERATION_MODE_STANDBY		(0x1 << 1)
>  #define ASPEED_OPERATION_MODE_NORMAL		(0x7 << 1)
> -
> -#define ASPEED_ENGINE_ENABLE		BIT(0)
> -
> +/* [4] */
> +#define ASPEED_CTRL_COMPENSATION	BIT(4)
> +/* [5] */
> +#define ASPEED_AUTOPENSATING		BIT(5)
> +/* [7:6] */
> +#define ASPEED_REF_VOLTAGE_2500mV	(0 << 6)
> +#define ASPEED_REF_VOLTAGE_1200mV	(1 << 6)
> +#define ASPEED_REF_VOLTAGE_EXT_HIGH	(2 << 6)
> +#define ASPEED_REF_VOLTAGE_EXT_LOW	(3 << 6)
> +#define ASPEED_BATTERY_SENSING_VOL_DIVIDE_2_3	(0 << 6)
> +#define ASPEED_BATTERY_SENSING_VOL_DIVIDE_1_3	(1 << 6)
> +/* [8] */
>  #define ASPEED_ADC_CTRL_INIT_RDY	BIT(8)
> -
> +/* [12] */
> +#define ASPEED_ADC_CH7_VOLTAGE_NORMAL	(0 << 12)
> +#define ASPEED_ADC_CH7_VOLTAGE_BATTERY	(1 << 12)
> +/* [13] */
> +#define ASPEED_ADC_EN_BATTERY_SENSING	BIT(13)
> +/* [31:16] */
> +#define ASPEED_ADC_CTRL_CH_EN(n)	(1 << (16 + n))
> +#define ASPEED_ADC_CTRL_CH_EN_ALL	GENMASK(31, 16)
> +
> +/**********************************************************
> + * Software setting
> + *********************************************************/
>  #define ASPEED_ADC_INIT_POLLING_TIME	500
>  #define ASPEED_ADC_INIT_TIMEOUT		500000
>  

Re: [PATCH 2/3] iio: adc: aspeed: Modify driver for ast2600

[Jonathan Cameron]

On Tue, 13 Oct 2020 18:32:44 +0800
Billy Tsai <billy_tsai@aspeedtech.com> wrote:

Hi Billy,

> This patch is used to handle the difference between ast2600
> and previous versions.
Good to mention what they are. Not all of them are obvious (such
as the reset change to being requested as shared).

> 
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>

Patch is a mixture of adding the new support, and adding various
'hooks' to enable the differences.  I'd rather see it as two patches.
First one adds the hooks but makes no functional change, second patch
adds the new device support.  That makes for easier reviewing for me!

> ---
>  drivers/iio/adc/aspeed_adc.c | 129 ++++++++++++++++++++++++++---------
>  1 file changed, 95 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c
> index ae400c4d6d40..fc4bbccf9348 100644
> --- a/drivers/iio/adc/aspeed_adc.c
> +++ b/drivers/iio/adc/aspeed_adc.c
> @@ -1,6 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /*
> - * Aspeed AST2400/2500 ADC
> + * Aspeed AST2400/2500/2600 ADC
>   *
>   * Copyright (C) 2017 Google, Inc.
>   */
> @@ -81,6 +81,7 @@ struct aspeed_adc_model_data {
>  	unsigned int max_sampling_rate;	// Hz
>  	unsigned int vref_voltage;	// mV

Add a comment to this to say that not all devices use a fixed
vref_voltage.

>  	bool wait_init_sequence;
> +	int num_channels;
>  };
>  
>  struct aspeed_adc_data {
> @@ -90,6 +91,7 @@ struct aspeed_adc_data {
>  	struct clk_hw		*clk_prescaler;
>  	struct clk_hw		*clk_scaler;
>  	struct reset_control	*rst;
> +	unsigned int vref_voltage;	// mV

If the naming can include the unit, then we both don't need the
comment and the units are apparent at the point of use.

vref_mV; for example.  A good cleanup would be to do similar
for the other cases in the driver.


>  };
>  
>  #define ASPEED_CHAN(_idx, _data_reg_addr) {			\
> @@ -126,8 +128,6 @@ static int aspeed_adc_read_raw(struct iio_dev *indio_dev,
>  			       int *val, int *val2, long mask)
>  {
>  	struct aspeed_adc_data *data = iio_priv(indio_dev);
> -	const struct aspeed_adc_model_data *model_data =
> -			of_device_get_match_data(data->dev);
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> @@ -135,7 +135,7 @@ static int aspeed_adc_read_raw(struct iio_dev *indio_dev,
>  		return IIO_VAL_INT;
>  
>  	case IIO_CHAN_INFO_SCALE:
> -		*val = model_data->vref_voltage;
> +		*val = data->vref_voltage;
>  		*val2 = ASPEED_RESOLUTION_BITS;
>  		return IIO_VAL_FRACTIONAL_LOG2;
>  
> @@ -208,8 +208,13 @@ static int aspeed_adc_probe(struct platform_device *pdev)
>  	struct aspeed_adc_data *data;
>  	const struct aspeed_adc_model_data *model_data;
>  	const char *clk_parent_name;
> +	char prescaler_clk_name[32];
> +	char scaler_clk_name[32];
>  	int ret;
>  	u32 adc_engine_control_reg_val;
> +	u32 ref_voltage_cfg = 0;
> +
> +	model_data = of_device_get_match_data(&pdev->dev);
>  
>  	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*data));
>  	if (!indio_dev)
> @@ -225,29 +230,75 @@ 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);
> +	snprintf(prescaler_clk_name, sizeof(prescaler_clk_name),
> +			"prescaler-%s", pdev->name);
> +	snprintf(scaler_clk_name, sizeof(scaler_clk_name),
> +			"scaler-%s", pdev->name);
> +	if (!strcmp(model_data->model_name, "ast2400-adc") ||
> +	    !strcmp(model_data->model_name, "ast2500-adc")) {

It would be nice to avoid all of these string comparisons if possible.
Various options come to mind:
1) Put a flag for each possible thing in the mode_data structures
   so we just check that to make decisions in probe.
2) Compare against the model_data pointers (bit ugly)
3) Do comparisons just once to convert to an enum.
4) Put an enum value in each mode_data structure to identify it without
   a string comparison.

1 is probably the best of these options as more extensible to additional
models as it provides fine grained feature information in one location
rather than many.



> +		/* Divider config */
> +		data->clk_prescaler = clk_hw_register_divider(
> +				&pdev->dev, prescaler_clk_name, clk_parent_name,
> +				0,
> +				data->base + ASPEED_REG_CLOCK_CONTROL, 17, 15,
> +				CLK_DIVIDER_ONE_BASED, &data->clk_lock);
> +		if (IS_ERR(data->clk_prescaler))
> +			return PTR_ERR(data->clk_prescaler);
>  
> -	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);
> -
> -	/*
> -	 * Register ADC clock scaler downstream from the prescaler. Allow rate
> -	 * setting to adjust the prescaler as well.
> -	 */
> -	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);
> -	if (IS_ERR(data->clk_scaler)) {
> -		ret = PTR_ERR(data->clk_scaler);
> -		goto scaler_error;
> +		/*
> +		 * Register ADC clock scaler downstream from the prescaler. Allow rate
> +		 * setting to adjust the prescaler as well.
> +		 */
> +		data->clk_scaler = clk_hw_register_divider(
> +					&pdev->dev, scaler_clk_name, prescaler_clk_name,
> +					CLK_SET_RATE_PARENT,
> +					data->base + ASPEED_REG_CLOCK_CONTROL, 0, 10,
> +					CLK_DIVIDER_ONE_BASED, &data->clk_lock);
> +		if (IS_ERR(data->clk_scaler)) {
> +			ret = PTR_ERR(data->clk_scaler);
> +			goto scaler_error;
> +		}
> +		/* Get ref_voltage */

If you do the 'feature flag' suggestion I make above, clearly this is a
separate feature to the clock stuff and so needs it's own feature flag.

> +		data->vref_voltage = model_data->vref_voltage;
> +	} else if (!strcmp(model_data->model_name, "ast2600-adc")) {
> +		/* Divider config */
> +		data->clk_scaler = clk_hw_register_divider(
> +			&pdev->dev, scaler_clk_name, clk_parent_name,
> +			CLK_SET_RATE_UNGATE,
> +			data->base + ASPEED_REG_CLOCK_CONTROL, 0, 16,
> +			CLK_DIVIDER_ONE_BASED, &data->clk_lock);
> +		if (IS_ERR(data->clk_scaler))
> +			return PTR_ERR(data->clk_scaler);
> +		/*
> +		 * Get ref_voltage:
> +		 * If reference voltage is between 1550~1650mv, we can set
> +		 * fields either ASPEED_REF_VOLTAGE_EXT_HIGH or ASPEED_REF_VOLTAGE_EXT_LOW.
> +		 * In this place, we select ASPEED_REF_VOLTAGE_EXT_HIGH as higher priority.
> +		 */
> +		if (!of_property_read_u32(pdev->dev.of_node, "ref_voltage",
> +					  &data->vref_voltage)) {
> +			if (data->vref_voltage == 2500)
> +				ref_voltage_cfg = ASPEED_REF_VOLTAGE_2500mV;
> +			else if (data->vref_voltage == 1200)
> +				ref_voltage_cfg = ASPEED_REF_VOLTAGE_1200mV;
> +			else if ((data->vref_voltage >= 1550) &&
> +					(data->vref_voltage <= 2700))
> +				ref_voltage_cfg = ASPEED_REF_VOLTAGE_EXT_HIGH;
> +			else if ((data->vref_voltage >= 900) &&
> +					(data->vref_voltage <= 1650))
> +				ref_voltage_cfg = ASPEED_REF_VOLTAGE_EXT_LOW;
> +			else {
> +				dev_err(&pdev->dev, "ref_voltage property is out of range: %d\n",
> +					data->vref_voltage);
> +				return -EINVAL;
> +			}
> +		} else {
Don't eat the error value from of_property_read_u32.  It might potentially
provide information on 'why' we couldn't read it.

Also would reduce indentation (improve readability) to do
		ret = of_property_read_u32(pdev->dev.of_node, "ref_voltage", ...
		if (ret) {
			dev_err(..)
			return ret;
		}
		if (data->vref_voltage) ...

> +			dev_err(&pdev->dev, "Couldn't read ref_voltage property\n");
> +			return -EINVAL;
> +		}
>  	}
>  
> -	data->rst = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> +	data->rst = devm_reset_control_get_shared(&pdev->dev, NULL);

I'd like a comment in the patch description saying why you made this change.

>  	if (IS_ERR(data->rst)) {
>  		dev_err(&pdev->dev,
>  			"invalid or missing reset controller device tree entry");
> @@ -256,13 +307,14 @@ static int aspeed_adc_probe(struct platform_device *pdev)
>  	}
>  	reset_control_deassert(data->rst);
>  
> -	model_data = of_device_get_match_data(&pdev->dev);
> +	adc_engine_control_reg_val = readl(data->base + ASPEED_REG_ENGINE_CONTROL);

Given this is during probe, I'd like to see an explanation comment on why we are
leaving the other bits of the register in whatever state we find them in.
i.e. why are we leaving some channels enabled?  We weren't previously
doing so...

> +	/* Enable engine in normal mode and set ref_voltage */
> +	adc_engine_control_reg_val |= (ASPEED_OPERATION_MODE_NORMAL |
> +				ASPEED_ENGINE_ENABLE | ref_voltage_cfg);
> +	writel(adc_engine_control_reg_val,
> +			data->base + ASPEED_REG_ENGINE_CONTROL);
>  
>  	if (model_data->wait_init_sequence) {
> -		/* Enable engine in normal mode. */
> -		writel(ASPEED_OPERATION_MODE_NORMAL | ASPEED_ENGINE_ENABLE,
> -		       data->base + ASPEED_REG_ENGINE_CONTROL);
> -
>  		/* Wait for initial sequence complete. */
>  		ret = readl_poll_timeout(data->base + ASPEED_REG_ENGINE_CONTROL,
>  					 adc_engine_control_reg_val,
> @@ -279,18 +331,16 @@ static int aspeed_adc_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto clk_enable_error;
>  
> -	adc_engine_control_reg_val = GENMASK(31, 16) |
> -		ASPEED_OPERATION_MODE_NORMAL | ASPEED_ENGINE_ENABLE;
> +	adc_engine_control_reg_val |= ASPEED_ADC_CTRL_CH_EN_ALL;
>  	writel(adc_engine_control_reg_val,
>  		data->base + ASPEED_REG_ENGINE_CONTROL);
>  
> -	model_data = of_device_get_match_data(&pdev->dev);
>  	indio_dev->name = model_data->model_name;
>  	indio_dev->dev.parent = &pdev->dev;
>  	indio_dev->info = &aspeed_adc_iio_info;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->channels = aspeed_adc_iio_channels;
> -	indio_dev->num_channels = ARRAY_SIZE(aspeed_adc_iio_channels);
> +	indio_dev->num_channels = model_data->num_channels;
>  
>  	ret = iio_device_register(indio_dev);
>  	if (ret)
> @@ -333,6 +383,7 @@ static const struct aspeed_adc_model_data ast2400_model_data = {
>  	.vref_voltage = 2500, // mV
>  	.min_sampling_rate = 10000,
>  	.max_sampling_rate = 500000,
> +	.num_channels = 16,
>  };
>  
>  static const struct aspeed_adc_model_data ast2500_model_data = {
> @@ -341,11 +392,21 @@ static const struct aspeed_adc_model_data ast2500_model_data = {
>  	.min_sampling_rate = 1,
>  	.max_sampling_rate = 1000000,
>  	.wait_init_sequence = true,
> +	.num_channels = 16,
> +};
> +
> +static const struct aspeed_adc_model_data ast2600_model_data = {
> +	.model_name = "ast2600-adc",
> +	.min_sampling_rate = 1,
> +	.max_sampling_rate = 1000000,
> +	.wait_init_sequence = true,
> +	.num_channels = 8,
>  };
>  
>  static const struct of_device_id aspeed_adc_matches[] = {
>  	{ .compatible = "aspeed,ast2400-adc", .data = &ast2400_model_data },
>  	{ .compatible = "aspeed,ast2500-adc", .data = &ast2500_model_data },
> +	{ .compatible = "aspeed,ast2600-adc", .data = &ast2600_model_data },
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, aspeed_adc_matches);
> @@ -362,5 +423,5 @@ static struct platform_driver aspeed_adc_driver = {
>  module_platform_driver(aspeed_adc_driver);
>  
>  MODULE_AUTHOR("Rick Altherr <raltherr@google.com>");
> -MODULE_DESCRIPTION("Aspeed AST2400/2500 ADC Driver");
> +MODULE_DESCRIPTION("Aspeed AST2400/2500/2600 ADC Driver");
>  MODULE_LICENSE("GPL");

Re: [PATCH 3/3] iio: adc: aspeed: Setting ref_voltage in probe

[Jonathan Cameron]

On Tue, 13 Oct 2020 18:32:45 +0800
Billy Tsai <billy_tsai@aspeedtech.com> wrote:

> At ast2600 ref_voltage becomes configurable and this property is board
> dependency.
> 
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>

dt-bindings: etc for the patch title.  Look at naming of similar patches
for a reference.


> ---
>  .../devicetree/bindings/iio/adc/aspeed_adc.txt   | 16 +++++++++++++++-

This has been converted to yaml. 
It is now in Linus' tree (though only very recently!)

You will need to add the relevant logic to make the yaml binding express that this
only exists and is required for ast600

The binding is making me wonder a bit on what this voltage actually is...

I 'think' we have a setup where the reference voltage is either picked
from 2 available internal references or uses an external regulator (presumably
provided to a pin on the chip?)

As such, the binding should be an optional regulator.  If the regulator is present
we use that, even if it matches the internal regulator votlage.  Someone put it
down on the board, so presumably  they want to use it.

If it's not present, then have to provide either of the two internal regulator
voltages via a devicetree binding such as you have here.



>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
> index 034fc2ba100e..0ba1980c4e06 100644
> --- a/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
> +++ b/Documentation/devicetree/bindings/iio/adc/aspeed_adc.txt
> @@ -3,8 +3,11 @@ Aspeed ADC
>  This device is a 10-bit converter for 16 voltage channels.  All inputs are
>  single ended.
>  
> +At ast2600, this device split into two individual IPs and each contains 8 voltage channels.
> +
> +Chip level dtsi:
>  Required properties:
> -- compatible: Should be "aspeed,ast2400-adc" or "aspeed,ast2500-adc"
> +- compatible: Should be "aspeed,ast2400-adc" or "aspeed,ast2500-adc" or "aspeed,ast2600-adc"
>  - reg: memory window mapping address and length
>  - clocks: Input clock used to derive the sample clock. Expected to be the
>            SoC's APB clock.
> @@ -20,3 +23,14 @@ Example:
>  		resets = <&syscon ASPEED_RESET_ADC>;
>  		#io-channel-cells = <1>;
>  	};
> +
> +Board level dts:
> +Required properties:
> +- ref_voltage: (ast2600 only)
> +	- Reference voltage in millivolts for the conversions.
> +	- The range of value is 900 to 2700 mv.

See above for suggestion on on this. For the internal case, will need
a name that expresses both what it is and what the units are.

int_ref_voltage_mv perhaps?

Yaml binding should check that either we have a regulator, or
we have this element.  That way we'll ensure we don't have any confusion
over which one to use.

Thanks,

Jonathan

> +
> +Example:
> +&adc0 {
> +	ref_voltage = <2500>;
> +};
> \ No newline at end of file

Re: [PATCH 1/3] iio: adc: aspeed: Orgnaize and add the define of adc

[Andy Shevchenko]

On Sun, Oct 18, 2020 at 1:32 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Tue, 13 Oct 2020 18:32:43 +0800
> Billy Tsai <billy_tsai@aspeedtech.com> wrote:

...

> > +/* [31:16] */

Useless comment.

> > +#define ASPEED_ADC_CTRL_CH_EN(n)     (1 << (16 + n))
> > +#define ASPEED_ADC_CTRL_CH_EN_ALL    GENMASK(31, 16)

But the main point is here.
First of all you may use BIT() in above.
Second, it's a good practice to put variables in the additional
parentheses in macros in case you are doing some operations with.

So, something like BIT(16 + (n)) would be nice to have at the end.

-- 
With Best Regards,
Andy Shevchenko