[v2 0/8] Add support for ast2600 ADC

[v2 0/8] Add support for ast2600 ADC

[Billy Tsai]

This patch serials make aspeed_adc.c can support ast2600.
In additional,
patch #6 is used to improve the adc accurate 
patch #7 is used to fix the clock issue in the original code.

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 (8):
  dt-bindings: iio: adc: rename the aspeed adc yaml
  dt-bindings: iio: adc: Binding ast2600 adc.
  iio: adc: aspeed: completes the bitfield declare.
  iio: adc: aspeed: Allow driver to support ast2600
  iio: adc: aspeed: Add func to set sampling rate.
  iio: adc: aspeed: Add compensation phase.
  iio: adc: aspeed: Fix the calculate error of clock.
  iio: adc: aspeed: Support battery sensing.

 .../bindings/iio/adc/aspeed,adc.yaml          |  79 ++++
 .../bindings/iio/adc/aspeed,ast2400-adc.yaml  |  55 ---
 drivers/iio/adc/aspeed_adc.c                  | 376 ++++++++++++++----
 3 files changed, 385 insertions(+), 125 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/aspeed,adc.yaml
 delete mode 100644 Documentation/devicetree/bindings/iio/adc/aspeed,ast2400-adc.yaml

-- 
2.25.1

[v2 1/8] dt-bindings: iio: adc: rename the aspeed adc yaml

[Billy Tsai]

The aspeed,ast2400-adc.yaml not only descriptor the bindings of ast2400.
Rename it to aspeed,adc.yaml for all of the aspeed adc bindings.

Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
---
 .../iio/adc/{aspeed,ast2400-adc.yaml => aspeed,adc.yaml}        | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
 rename Documentation/devicetree/bindings/iio/adc/{aspeed,ast2400-adc.yaml => aspeed,adc.yaml} (93%)

diff --git a/Documentation/devicetree/bindings/iio/adc/aspeed,ast2400-adc.yaml b/Documentation/devicetree/bindings/iio/adc/aspeed,adc.yaml
similarity index 93%
rename from Documentation/devicetree/bindings/iio/adc/aspeed,ast2400-adc.yaml
rename to Documentation/devicetree/bindings/iio/adc/aspeed,adc.yaml
index 7f534a933e92..23f3da1ffca3 100644
--- a/Documentation/devicetree/bindings/iio/adc/aspeed,ast2400-adc.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/aspeed,adc.yaml
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
 %YAML 1.2
 ---
-$id: http://devicetree.org/schemas/iio/adc/aspeed,ast2400-adc.yaml#
+$id: http://devicetree.org/schemas/iio/adc/aspeed,adc.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
 title: ADC that forms part of an ASPEED server management processor.
-- 
2.25.1

Re: [v2 1/8] dt-bindings: iio: adc: rename the aspeed adc yaml

[Jonathan Cameron]

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

> The aspeed,ast2400-adc.yaml not only descriptor the bindings of ast2400.
> Rename it to aspeed,adc.yaml for all of the aspeed adc bindings.
> 
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>

We try to avoid 'wild' card type namings most of the time and instead
name after a particular part number.  I say try because clearly
we let a few in over the years :(

It is very hard to know if this binding will apply to 'all' future
aspeed ADCs.

As such I'm not sure this particular rename makes sense.

Thanks,

Jonathan

> ---
>  .../iio/adc/{aspeed,ast2400-adc.yaml => aspeed,adc.yaml}        | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>  rename Documentation/devicetree/bindings/iio/adc/{aspeed,ast2400-adc.yaml => aspeed,adc.yaml} (93%)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/aspeed,ast2400-adc.yaml b/Documentation/devicetree/bindings/iio/adc/aspeed,adc.yaml
> similarity index 93%
> rename from Documentation/devicetree/bindings/iio/adc/aspeed,ast2400-adc.yaml
> rename to Documentation/devicetree/bindings/iio/adc/aspeed,adc.yaml
> index 7f534a933e92..23f3da1ffca3 100644
> --- a/Documentation/devicetree/bindings/iio/adc/aspeed,ast2400-adc.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/aspeed,adc.yaml
> @@ -1,7 +1,7 @@
>  # SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>  %YAML 1.2
>  ---
> -$id: http://devicetree.org/schemas/iio/adc/aspeed,ast2400-adc.yaml#
> +$id: http://devicetree.org/schemas/iio/adc/aspeed,adc.yaml#
>  $schema: http://devicetree.org/meta-schemas/core.yaml#
>  
>  title: ADC that forms part of an ASPEED server management processor.

Re: [v2 1/8] dt-bindings: iio: adc: rename the aspeed adc yaml

[Billy Tsai]

Hi Jonathan,

On 2021/7/23, 10:45 PM, "Jonathan Cameron" <Jonathan.Cameron@Huawei.com> wrote:

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

    >   > The aspeed,ast2400-adc.yaml not only descriptor the bindings of ast2400.
    >   > Rename it to aspeed,adc.yaml for all of the aspeed adc bindings.
    >   > 
    >   > Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>

    >   We try to avoid 'wild' card type namings most of the time and instead
    >   name after a particular part number.  I say try because clearly
    >   we let a few in over the years :(

    >   It is very hard to know if this binding will apply to 'all' future
    >   aspeed ADCs.

    >   As such I'm not sure this particular rename makes sense.

If I want to extend the yaml file to compatible more versions of the aspeed adc.
Would you suggest to add new files call aspeed,ast2600-adc.yaml or just append it
to the aspeed,ast2400-adc.yaml?

Thanks

Best Regards,
Billy Tsai

Re: [v2 1/8] dt-bindings: iio: adc: rename the aspeed adc yaml

[Rob Herring]

On Mon, Jul 26, 2021 at 06:53:07AM +0000, Billy Tsai wrote:
> Hi Jonathan,
> 
> On 2021/7/23, 10:45 PM, "Jonathan Cameron" <Jonathan.Cameron@Huawei.com> wrote:
> 
>     On Fri, 23 Jul 2021 16:16:14 +0800
>     Billy Tsai <billy_tsai@aspeedtech.com> wrote:
> 
>     >   > The aspeed,ast2400-adc.yaml not only descriptor the bindings of ast2400.
>     >   > Rename it to aspeed,adc.yaml for all of the aspeed adc bindings.
>     >   > 
>     >   > Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> 
>     >   We try to avoid 'wild' card type namings most of the time and instead
>     >   name after a particular part number.  I say try because clearly
>     >   we let a few in over the years :(
> 
>     >   It is very hard to know if this binding will apply to 'all' future
>     >   aspeed ADCs.
> 
>     >   As such I'm not sure this particular rename makes sense.
> 
> If I want to extend the yaml file to compatible more versions of the aspeed adc.
> Would you suggest to add new files call aspeed,ast2600-adc.yaml or just append it
> to the aspeed,ast2400-adc.yaml?

If 2600 is not backwards compatible with 2400, then probably a new 
schema file. Given you are adding new properties (which only apply to 
2600?), then most likely a new schema file. Depends at which point there 
are too many conditional (if/then/else) schemas.

Rob

Re: [v2 1/8] dt-bindings: iio: adc: rename the aspeed adc yaml

[Jonathan Cameron]

On Thu, 29 Jul 2021 14:31:35 -0600
Rob Herring <robh@kernel.org> wrote:

> On Mon, Jul 26, 2021 at 06:53:07AM +0000, Billy Tsai wrote:
> > Hi Jonathan,
> > 
> > On 2021/7/23, 10:45 PM, "Jonathan Cameron" <Jonathan.Cameron@Huawei.com> wrote:
> > 
> >     On Fri, 23 Jul 2021 16:16:14 +0800
> >     Billy Tsai <billy_tsai@aspeedtech.com> wrote:
> >   
> >     >   > The aspeed,ast2400-adc.yaml not only descriptor the bindings of ast2400.
> >     >   > Rename it to aspeed,adc.yaml for all of the aspeed adc bindings.
> >     >   > 
> >     >   > Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>  
> >   
> >     >   We try to avoid 'wild' card type namings most of the time and instead
> >     >   name after a particular part number.  I say try because clearly
> >     >   we let a few in over the years :(  
> >   
> >     >   It is very hard to know if this binding will apply to 'all' future
> >     >   aspeed ADCs.  
> >   
> >     >   As such I'm not sure this particular rename makes sense.  
> > 
> > If I want to extend the yaml file to compatible more versions of the aspeed adc.
> > Would you suggest to add new files call aspeed,ast2600-adc.yaml or just append it
> > to the aspeed,ast2400-adc.yaml?  
> 
> If 2600 is not backwards compatible with 2400, then probably a new 
> schema file. Given you are adding new properties (which only apply to 
> 2600?), then most likely a new schema file. Depends at which point there 
> are too many conditional (if/then/else) schemas.

Agreed.  It's a judgement call you need to make on when it is worth the new file.
Note that doesn't have anything to do with splitting the driver.  We have mulitple
binding files for single drivers and for that matter multiple drivers for single binding
files.

If it is 'compatible' enough to not make the file to complex, then add to the existing
ast2400 file without renaming.

Jonathan

> 
> Rob

[v2 2/8] dt-bindings: iio: adc: Binding ast2600 adc.

[Billy Tsai]

This patch add more description about aspeed adc and add two property
for ast2600:
- vref: used to configure reference voltage.
- battery-sensing: used to enable battery sensing mode for last channel.

Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
---
 .../bindings/iio/adc/aspeed,adc.yaml          | 28 +++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/aspeed,adc.yaml b/Documentation/devicetree/bindings/iio/adc/aspeed,adc.yaml
index 23f3da1ffca3..a562a7fbc30c 100644
--- a/Documentation/devicetree/bindings/iio/adc/aspeed,adc.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/aspeed,adc.yaml
@@ -10,14 +10,26 @@ maintainers:
   - Joel Stanley <joel@jms.id.au>
 
 description:
-  This device is a 10-bit converter for 16 voltage channels.  All inputs are
-  single ended.
+  • 10-bits resolution for 16 voltage channels.
+  At ast2400/ast2500 the device has only one engine with 16 voltage channels.
+  At ast2600 the device split into two individual engine and each contains 8 voltage channels.
+  • Channel scanning can be non-continuous.
+  • Programmable ADC clock frequency.
+  • Programmable upper and lower bound for each channels.
+  • Interrupt when larger or less than bounds for each channels.
+  • Support hysteresis for each channels.
+  • Buildin a compensating method.
+  Additional feature at ast2600
+  • Internal or External reference voltage.
+  • Support 2 Internal reference voltage 1.2v or 2.5v.
+  • Integrate dividing circuit for battery sensing.
 
 properties:
   compatible:
     enum:
       - aspeed,ast2400-adc
       - aspeed,ast2500-adc
+      - aspeed,ast2600-adc
 
   reg:
     maxItems: 1
@@ -33,6 +45,18 @@ properties:
   "#io-channel-cells":
     const: 1
 
+  vref:
+    minItems: 900
+    maxItems: 2700
+    default: 2500
+    description:
+      ADC Reference voltage in millivolts.
+
+  battery-sensing:
+    type: boolean
+    description:
+      Inform the driver that last channel will be used to sensor battery.
+
 required:
   - compatible
   - reg
-- 
2.25.1

Re: [v2 2/8] dt-bindings: iio: adc: Binding ast2600 adc.

[Jonathan Cameron]

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

> This patch add more description about aspeed adc and add two property
> for ast2600:
> - vref: used to configure reference voltage.
> - battery-sensing: used to enable battery sensing mode for last channel.
> 
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>

Hi Billy,

A few comments inline.

Thanks,

Jonathan

> ---
>  .../bindings/iio/adc/aspeed,adc.yaml          | 28 +++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/aspeed,adc.yaml b/Documentation/devicetree/bindings/iio/adc/aspeed,adc.yaml
> index 23f3da1ffca3..a562a7fbc30c 100644
> --- a/Documentation/devicetree/bindings/iio/adc/aspeed,adc.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/aspeed,adc.yaml
> @@ -10,14 +10,26 @@ maintainers:
>    - Joel Stanley <joel@jms.id.au>
>  
>  description:

I think you need a | after description if you want the formatting to be
maintained (otherwise it will undo the line breaks).

> -  This device is a 10-bit converter for 16 voltage channels.  All inputs are
> -  single ended.
> +  • 10-bits resolution for 16 voltage channels.
> +  At ast2400/ast2500 the device has only one engine with 16 voltage channels.
> +  At ast2600 the device split into two individual engine and each contains 8 voltage channels.

Please wrap lines at 80 chars unless it badly hurts readability.
engines

> +  • Channel scanning can be non-continuous.
> +  • Programmable ADC clock frequency.
> +  • Programmable upper and lower bound for each channels.

I would use threshold rather than bound.   A bound restricts the
value, and I think this is measuring it?

> +  • Interrupt when larger or less than bounds for each channels.
> +  • Support hysteresis for each channels.
> +  • Buildin a compensating method.

Built-in 

> +  Additional feature at ast2600

of ast2600

> +  • Internal or External reference voltage.
> +  • Support 2 Internal reference voltage 1.2v or 2.5v.
> +  • Integrate dividing circuit for battery sensing.
>  
>  properties:
>    compatible:
>      enum:
>        - aspeed,ast2400-adc
>        - aspeed,ast2500-adc
> +      - aspeed,ast2600-adc
>  
>    reg:
>      maxItems: 1
> @@ -33,6 +45,18 @@ properties:
>    "#io-channel-cells":
>      const: 1
>  
> +  vref:
> +    minItems: 900
> +    maxItems: 2700
> +    default: 2500
> +    description:
> +      ADC Reference voltage in millivolts.

I'm not clear from this description.  Is this describing an externally
connected voltage reference?  If so it needs to be done as a regulator.
If it's a classic high precision reference, the dts can just use
a fixed regulator.

> +
> +  battery-sensing:
> +    type: boolean
> +    description:
> +      Inform the driver that last channel will be used to sensor battery.

This isn't (I think?) a standard dt binding, so it needs a manufacturer
prefix.

aspeed,battery-sensing

> +
>  required:
>    - compatible
>    - reg

Re: [v2 2/8] dt-bindings: iio: adc: Binding ast2600 adc.

[Billy Tsai]

Hi Jonathan,

Thanks for your review. I will fix them.
About the vref I reply inline.

On 2021/7/23, 10:52 PM, "Jonathan Cameron" <Jonathan.Cameron@Huawei.com> wrote:

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

    >   > +  • Internal or External reference voltage.
    >   > +  • Support 2 Internal reference voltage 1.2v or 2.5v.
    >   > +  • Integrate dividing circuit for battery sensing.
    >   >  
    >   >  properties:
    >   >    compatible:
    >   >      enum:
    >   >        - aspeed,ast2400-adc
    >   >        - aspeed,ast2500-adc
    >   > +      - aspeed,ast2600-adc
    >   >  
    >   >    reg:
    >   >      maxItems: 1
    >   > @@ -33,6 +45,18 @@ properties:
    >   >    "#io-channel-cells":
    >   >      const: 1
    >   >  
    >   > +  vref:
    >   > +    minItems: 900
    >   > +    maxItems: 2700
    >   > +    default: 2500
    >   > +    description:
    >   > +      ADC Reference voltage in millivolts.

    >   I'm not clear from this description.  Is this describing an externally
    >   connected voltage reference?  If so it needs to be done as a regulator.
    >   If it's a classic high precision reference, the dts can just use
    >   a fixed regulator.

In the ast2600, the ADC supports two internal reference voltages of 1.2v or 2.5v,
as well as external voltages. When the user selects a voltage of 1.2v or 2.5v, my
driver will first select to use the internal voltage. 
As you mention at patch #4, you suggest to use two property to handle this feature.
vref: indicate the regulator handler. Like other dt-bindings used.
aspeed,int_vref_mv: indicate the chosen of 1.2v or 2.5v
and use "model_data->vref_fixed" to exclude ast2400 and ast2500
Is it right?

Thanks

    >   > +
    >   > +  battery-sensing:
    >   > +    type: boolean
    >   > +    description:
    >   > +      Inform the driver that last channel will be used to sensor battery.

    >   This isn't (I think?) a standard dt binding, so it needs a manufacturer
    >   prefix.

    >   aspeed,battery-sensing

Best Regards,
Billy Tsai

[v2 3/8] iio: adc: aspeed: completes the bitfield declare.

[Billy Tsai]

This patch completes the declare of adc register bitfields and uses the
same prefix ASPEED_ADC_* for these bitfields.

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

diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c
index 19efaa41bc34..99466a5924c7 100644
--- a/drivers/iio/adc/aspeed_adc.c
+++ b/drivers/iio/adc/aspeed_adc.c
@@ -16,6 +16,7 @@
 #include <linux/reset.h>
 #include <linux/spinlock.h>
 #include <linux/types.h>
+#include <linux/bitfield.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/driver.h>
@@ -28,15 +29,28 @@
 #define ASPEED_REG_INTERRUPT_CONTROL	0x04
 #define ASPEED_REG_VGA_DETECT_CONTROL	0x08
 #define ASPEED_REG_CLOCK_CONTROL	0x0C
-#define ASPEED_REG_MAX			0xC0
-
-#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)
-
-#define ASPEED_ADC_CTRL_INIT_RDY	BIT(8)
+#define ASPEED_REG_COMPENSATION_TRIM	0xC4
+#define ASPEED_REG_MAX			0xCC
+
+#define ASPEED_ADC_ENGINE_ENABLE		BIT(0)
+#define ASPEED_ADC_OPERATION_MODE		GENMASK(3, 1)
+#define ASPEED_ADC_OPERATION_MODE_POWER_DOWN	FIELD_PREP(ASPEED_ADC_OPERATION_MODE, 0)
+#define ASPEED_ADC_OPERATION_MODE_STANDBY	FIELD_PREP(ASPEED_ADC_OPERATION_MODE, 1)
+#define ASPEED_ADC_OPERATION_MODE_NORMAL	FIELD_PREP(ASPEED_ADC_OPERATION_MODE, 7)
+#define ASPEED_ADC_CTRL_COMPENSATION		BIT(4)
+#define ASPEED_ADC_AUTO_COMPENSATION		BIT(5)
+#define ASPEED_ADC_REF_VOLTAGE			GENMASK(7, 6)
+#define ASPEED_ADC_REF_VOLTAGE_2500mV		FIELD_PREP(ASPEED_ADC_REF_VOLTAGE, 0)
+#define ASPEED_ADC_REF_VOLTAGE_1200mV		FIELD_PREP(ASPEED_ADC_REF_VOLTAGE, 1)
+#define ASPEED_ADC_REF_VOLTAGE_EXT_HIGH		FIELD_PREP(ASPEED_ADC_REF_VOLTAGE, 2)
+#define ASPEED_ADC_REF_VOLTAGE_EXT_LOW		FIELD_PREP(ASPEED_ADC_REF_VOLTAGE, 3)
+#define ASPEED_ADC_CTRL_INIT_RDY		BIT(8)
+#define ASPEED_ADC_CH7_MODE			BIT(12)
+#define ASPEED_ADC_CH7_NORMAL			FIELD_PREP(ASPEED_ADC_CH7_MODE, 0)
+#define ASPEED_ADC_CH7_BATTERY			FIELD_PREP(ASPEED_ADC_CH7_MODE, 1)
+#define ASPEED_ADC_BATTERY_SENSING_ENABLE	BIT(13)
+#define ASPEED_ADC_CTRL_CHANNEL			GENMASK(31, 16)
+#define ASPEED_ADC_CTRL_CHANNEL_ENABLE(ch)	FIELD_PREP(ASPEED_ADC_CTRL_CHANNEL, BIT(ch))
 
 #define ASPEED_ADC_INIT_POLLING_TIME	500
 #define ASPEED_ADC_INIT_TIMEOUT		500000
@@ -226,7 +240,7 @@ static int aspeed_adc_probe(struct platform_device *pdev)
 
 	if (model_data->wait_init_sequence) {
 		/* Enable engine in normal mode. */
-		writel(ASPEED_OPERATION_MODE_NORMAL | ASPEED_ENGINE_ENABLE,
+		writel(ASPEED_ADC_OPERATION_MODE_NORMAL | ASPEED_ADC_ENGINE_ENABLE,
 		       data->base + ASPEED_REG_ENGINE_CONTROL);
 
 		/* Wait for initial sequence complete. */
@@ -246,7 +260,7 @@ static int aspeed_adc_probe(struct platform_device *pdev)
 		goto clk_enable_error;
 
 	adc_engine_control_reg_val = GENMASK(31, 16) |
-		ASPEED_OPERATION_MODE_NORMAL | ASPEED_ENGINE_ENABLE;
+		ASPEED_ADC_OPERATION_MODE_NORMAL | ASPEED_ADC_ENGINE_ENABLE;
 	writel(adc_engine_control_reg_val,
 		data->base + ASPEED_REG_ENGINE_CONTROL);
 
@@ -264,7 +278,7 @@ static int aspeed_adc_probe(struct platform_device *pdev)
 	return 0;
 
 iio_register_error:
-	writel(ASPEED_OPERATION_MODE_POWER_DOWN,
+	writel(ASPEED_ADC_OPERATION_MODE_POWER_DOWN,
 		data->base + ASPEED_REG_ENGINE_CONTROL);
 	clk_disable_unprepare(data->clk_scaler->clk);
 clk_enable_error:
@@ -283,7 +297,7 @@ static int aspeed_adc_remove(struct platform_device *pdev)
 	struct aspeed_adc_data *data = iio_priv(indio_dev);
 
 	iio_device_unregister(indio_dev);
-	writel(ASPEED_OPERATION_MODE_POWER_DOWN,
+	writel(ASPEED_ADC_OPERATION_MODE_POWER_DOWN,
 		data->base + ASPEED_REG_ENGINE_CONTROL);
 	clk_disable_unprepare(data->clk_scaler->clk);
 	reset_control_assert(data->rst);
-- 
2.25.1

Re: [v2 3/8] iio: adc: aspeed: completes the bitfield declare.

[Jonathan Cameron]

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

> This patch completes the declare of adc register bitfields and uses the
> same prefix ASPEED_ADC_* for these bitfields.
> 
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
Hi Billy

See inline,

Thanks,

Jonathan

> ---
>  drivers/iio/adc/aspeed_adc.c | 40 ++++++++++++++++++++++++------------
>  1 file changed, 27 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c
> index 19efaa41bc34..99466a5924c7 100644
> --- a/drivers/iio/adc/aspeed_adc.c
> +++ b/drivers/iio/adc/aspeed_adc.c
> @@ -16,6 +16,7 @@
>  #include <linux/reset.h>
>  #include <linux/spinlock.h>
>  #include <linux/types.h>
> +#include <linux/bitfield.h>
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/driver.h>
> @@ -28,15 +29,28 @@
>  #define ASPEED_REG_INTERRUPT_CONTROL	0x04
>  #define ASPEED_REG_VGA_DETECT_CONTROL	0x08
>  #define ASPEED_REG_CLOCK_CONTROL	0x0C
> -#define ASPEED_REG_MAX			0xC0
> -
> -#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)
> -
> -#define ASPEED_ADC_CTRL_INIT_RDY	BIT(8)
> +#define ASPEED_REG_COMPENSATION_TRIM	0xC4
> +#define ASPEED_REG_MAX			0xCC
> +
> +#define ASPEED_ADC_ENGINE_ENABLE		BIT(0)
> +#define ASPEED_ADC_OPERATION_MODE		GENMASK(3, 1)
> +#define ASPEED_ADC_OPERATION_MODE_POWER_DOWN	FIELD_PREP(ASPEED_ADC_OPERATION_MODE, 0)
It's more common to have the FIELD_PREP at the location where it
is used and just have defines here to be to the value of the field.

Perhaps also consider some abbreviations as I think we can safely
make them here without losing any meaning, given context.

ASPEED_ADC_OP_MODE
ASPEED_ADC_OP_MODE_PWR_DWN
ASPEED_ADC_OP_MODE_STANDBY
ASPEED_ADC_OP_MODE_NORMAL

etc.

> +#define ASPEED_ADC_OPERATION_MODE_STANDBY	FIELD_PREP(ASPEED_ADC_OPERATION_MODE, 1)
> +#define ASPEED_ADC_OPERATION_MODE_NORMAL	FIELD_PREP(ASPEED_ADC_OPERATION_MODE, 7)
> +#define ASPEED_ADC_CTRL_COMPENSATION		BIT(4)
> +#define ASPEED_ADC_AUTO_COMPENSATION		BIT(5)
> +#define ASPEED_ADC_REF_VOLTAGE			GENMASK(7, 6)
> +#define ASPEED_ADC_REF_VOLTAGE_2500mV		FIELD_PREP(ASPEED_ADC_REF_VOLTAGE, 0)
> +#define ASPEED_ADC_REF_VOLTAGE_1200mV		FIELD_PREP(ASPEED_ADC_REF_VOLTAGE, 1)
> +#define ASPEED_ADC_REF_VOLTAGE_EXT_HIGH		FIELD_PREP(ASPEED_ADC_REF_VOLTAGE, 2)
> +#define ASPEED_ADC_REF_VOLTAGE_EXT_LOW		FIELD_PREP(ASPEED_ADC_REF_VOLTAGE, 3)
> +#define ASPEED_ADC_CTRL_INIT_RDY		BIT(8)
> +#define ASPEED_ADC_CH7_MODE			BIT(12)
> +#define ASPEED_ADC_CH7_NORMAL			FIELD_PREP(ASPEED_ADC_CH7_MODE, 0)
> +#define ASPEED_ADC_CH7_BATTERY			FIELD_PREP(ASPEED_ADC_CH7_MODE, 1)
> +#define ASPEED_ADC_BATTERY_SENSING_ENABLE	BIT(13)
> +#define ASPEED_ADC_CTRL_CHANNEL			GENMASK(31, 16)
> +#define ASPEED_ADC_CTRL_CHANNEL_ENABLE(ch)	FIELD_PREP(ASPEED_ADC_CTRL_CHANNEL, BIT(ch))
>  
>  #define ASPEED_ADC_INIT_POLLING_TIME	500
>  #define ASPEED_ADC_INIT_TIMEOUT		500000
> @@ -226,7 +240,7 @@ static int aspeed_adc_probe(struct platform_device *pdev)
>  
>  	if (model_data->wait_init_sequence) {
>  		/* Enable engine in normal mode. */
> -		writel(ASPEED_OPERATION_MODE_NORMAL | ASPEED_ENGINE_ENABLE,
> +		writel(ASPEED_ADC_OPERATION_MODE_NORMAL | ASPEED_ADC_ENGINE_ENABLE,
>  		       data->base + ASPEED_REG_ENGINE_CONTROL);
>  
>  		/* Wait for initial sequence complete. */
> @@ -246,7 +260,7 @@ static int aspeed_adc_probe(struct platform_device *pdev)
>  		goto clk_enable_error;
>  
>  	adc_engine_control_reg_val = GENMASK(31, 16) |
> -		ASPEED_OPERATION_MODE_NORMAL | ASPEED_ENGINE_ENABLE;
> +		ASPEED_ADC_OPERATION_MODE_NORMAL | ASPEED_ADC_ENGINE_ENABLE;
>  	writel(adc_engine_control_reg_val,
>  		data->base + ASPEED_REG_ENGINE_CONTROL);
>  
> @@ -264,7 +278,7 @@ static int aspeed_adc_probe(struct platform_device *pdev)
>  	return 0;
>  
>  iio_register_error:
> -	writel(ASPEED_OPERATION_MODE_POWER_DOWN,
> +	writel(ASPEED_ADC_OPERATION_MODE_POWER_DOWN,
>  		data->base + ASPEED_REG_ENGINE_CONTROL);
>  	clk_disable_unprepare(data->clk_scaler->clk);
>  clk_enable_error:
> @@ -283,7 +297,7 @@ static int aspeed_adc_remove(struct platform_device *pdev)
>  	struct aspeed_adc_data *data = iio_priv(indio_dev);
>  
>  	iio_device_unregister(indio_dev);
> -	writel(ASPEED_OPERATION_MODE_POWER_DOWN,
> +	writel(ASPEED_ADC_OPERATION_MODE_POWER_DOWN,
>  		data->base + ASPEED_REG_ENGINE_CONTROL);
>  	clk_disable_unprepare(data->clk_scaler->clk);
>  	reset_control_assert(data->rst);

[v2 4/8] iio: adc: aspeed: Allow driver to support ast2600

[Billy Tsai]

The adc controller have some differents at ast2600:
1. Combine control register of clock divider to continuous bitfields.
2. Reference voltage becomes optional which are internal 2500mv/1200mv
and external range from 900mv to 2700mv
3. Divided into two engine, each one has 8 voltage sensing channels.

This patch handled these changes and compatible with old version.

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

diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c
index 99466a5924c7..84f079195375 100644
--- a/drivers/iio/adc/aspeed_adc.c
+++ b/drivers/iio/adc/aspeed_adc.c
@@ -1,8 +1,9 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /*
- * Aspeed AST2400/2500 ADC
+ * Aspeed AST2400/2500/2600 ADC
  *
  * Copyright (C) 2017 Google, Inc.
+ * Copyright (C) 2021 Aspeed Technology Inc.
  */
 
 #include <linux/clk.h>
@@ -55,12 +56,17 @@
 #define ASPEED_ADC_INIT_POLLING_TIME	500
 #define ASPEED_ADC_INIT_TIMEOUT		500000
 
+enum aspeed_adc_version {
+	aspeed_adc_ast2400,
+	aspeed_adc_ast2500,
+	aspeed_adc_ast2600,
+};
 struct aspeed_adc_model_data {
-	const char *model_name;
+	enum aspeed_adc_version version;
 	unsigned int min_sampling_rate;	// Hz
 	unsigned int max_sampling_rate;	// Hz
-	unsigned int vref_voltage;	// mV
 	bool wait_init_sequence;
+	unsigned int num_channels;
 };
 
 struct aspeed_adc_data {
@@ -70,6 +76,7 @@ struct aspeed_adc_data {
 	struct clk_hw		*clk_prescaler;
 	struct clk_hw		*clk_scaler;
 	struct reset_control	*rst;
+	int			vref;
 };
 
 #define ASPEED_CHAN(_idx, _data_reg_addr) {			\
@@ -106,8 +113,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:
@@ -115,7 +120,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;
 		*val2 = ASPEED_RESOLUTION_BITS;
 		return IIO_VAL_FRACTIONAL_LOG2;
 
@@ -182,6 +187,55 @@ static const struct iio_info aspeed_adc_iio_info = {
 	.debugfs_reg_access = aspeed_adc_reg_access,
 };
 
+static int aspeed_adc_vref_config(struct platform_device *pdev)
+{
+	const struct aspeed_adc_model_data *model_data;
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+	struct aspeed_adc_data *data = iio_priv(indio_dev);
+	int vref;
+	u32 adc_engine_control_reg_val =
+		readl(data->base + ASPEED_REG_ENGINE_CONTROL);
+
+	model_data = of_device_get_match_data(&pdev->dev);
+	switch (model_data->version) {
+	case aspeed_adc_ast2400:
+		vref = 2500;
+		break;
+	case aspeed_adc_ast2500:
+		vref = 1800;
+		break;
+	case aspeed_adc_ast2600:
+		if (of_property_read_u32(pdev->dev.of_node, "vref", &vref))
+			vref = 2500;
+		if (vref == 2500)
+			writel(adc_engine_control_reg_val |
+				       ASPEED_ADC_REF_VOLTAGE_2500mV,
+			       data->base + ASPEED_REG_ENGINE_CONTROL);
+		else if (vref == 1200)
+			writel(adc_engine_control_reg_val |
+				       ASPEED_ADC_REF_VOLTAGE_1200mV,
+			       data->base + ASPEED_REG_ENGINE_CONTROL);
+		else if ((vref >= 1550) && (vref <= 2700))
+			writel(adc_engine_control_reg_val |
+				       ASPEED_ADC_REF_VOLTAGE_EXT_HIGH,
+			       data->base + ASPEED_REG_ENGINE_CONTROL);
+		else if ((vref >= 900) && (vref <= 1650))
+			writel(adc_engine_control_reg_val |
+				       ASPEED_ADC_REF_VOLTAGE_EXT_LOW,
+			       data->base + ASPEED_REG_ENGINE_CONTROL);
+		else {
+			dev_err(&pdev->dev, "Vref not support");
+			return -EOPNOTSUPP;
+		}
+		break;
+	default:
+		dev_err(&pdev->dev, "ADC version not recognized");
+		return -EOPNOTSUPP;
+	}
+	data->vref = vref;
+	return 0;
+}
+
 static int aspeed_adc_probe(struct platform_device *pdev)
 {
 	struct iio_dev *indio_dev;
@@ -190,13 +244,16 @@ static int aspeed_adc_probe(struct platform_device *pdev)
 	const char *clk_parent_name;
 	int ret;
 	u32 adc_engine_control_reg_val;
+	char scaler_clk_name[32];
 
+	model_data = of_device_get_match_data(&pdev->dev);
 	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*data));
 	if (!indio_dev)
 		return -ENOMEM;
 
 	data = iio_priv(indio_dev);
 	data->dev = &pdev->dev;
+	dev_set_drvdata(data->dev, indio_dev);
 
 	data->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(data->base))
@@ -205,29 +262,39 @@ 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);
+	if (model_data->version <= aspeed_adc_ast2500) {
+		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);
 
-	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", "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;
+		}
+	} else {
+		snprintf(scaler_clk_name, sizeof(scaler_clk_name), "scaler-%s",
+			 pdev->name);
+		data->clk_scaler = clk_hw_register_divider(
+			&pdev->dev, scaler_clk_name, clk_parent_name, 0,
+			data->base + ASPEED_REG_CLOCK_CONTROL, 0, 16, 0,
+			&data->clk_lock);
+		if (IS_ERR(data->clk_scaler))
+			return PTR_ERR(data->clk_scaler);
 	}
 
-	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");
@@ -236,11 +303,17 @@ static int aspeed_adc_probe(struct platform_device *pdev)
 	}
 	reset_control_deassert(data->rst);
 
-	model_data = of_device_get_match_data(&pdev->dev);
+	ret = aspeed_adc_vref_config(pdev);
+	if (ret)
+		goto vref_config_error;
 
 	if (model_data->wait_init_sequence) {
+		adc_engine_control_reg_val =
+			readl(data->base + ASPEED_REG_ENGINE_CONTROL);
 		/* Enable engine in normal mode. */
-		writel(ASPEED_ADC_OPERATION_MODE_NORMAL | ASPEED_ADC_ENGINE_ENABLE,
+		writel(adc_engine_control_reg_val |
+			       ASPEED_ADC_OPERATION_MODE_NORMAL |
+			       ASPEED_ADC_ENGINE_ENABLE,
 		       data->base + ASPEED_REG_ENGINE_CONTROL);
 
 		/* Wait for initial sequence complete. */
@@ -254,22 +327,23 @@ static int aspeed_adc_probe(struct platform_device *pdev)
 			goto poll_timeout_error;
 	}
 
-	/* Start all channels in normal mode. */
 	ret = clk_prepare_enable(data->clk_scaler->clk);
 	if (ret)
 		goto clk_enable_error;
-
-	adc_engine_control_reg_val = GENMASK(31, 16) |
-		ASPEED_ADC_OPERATION_MODE_NORMAL | ASPEED_ADC_ENGINE_ENABLE;
+	adc_engine_control_reg_val =
+		readl(data->base + ASPEED_REG_ENGINE_CONTROL);
+	/* Start all channels in normal mode. */
+	adc_engine_control_reg_val |= ASPEED_ADC_CTRL_CHANNEL |
+				     ASPEED_ADC_OPERATION_MODE_NORMAL |
+				     ASPEED_ADC_ENGINE_ENABLE;
 	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->name = dev_name(&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)
@@ -281,13 +355,15 @@ static int aspeed_adc_probe(struct platform_device *pdev)
 	writel(ASPEED_ADC_OPERATION_MODE_POWER_DOWN,
 		data->base + ASPEED_REG_ENGINE_CONTROL);
 	clk_disable_unprepare(data->clk_scaler->clk);
+vref_config_error:
 clk_enable_error:
 poll_timeout_error:
 	reset_control_assert(data->rst);
 reset_error:
 	clk_hw_unregister_divider(data->clk_scaler);
 scaler_error:
-	clk_hw_unregister_divider(data->clk_prescaler);
+	if (model_data->version <= aspeed_adc_ast2500)
+		clk_hw_unregister_divider(data->clk_prescaler);
 	return ret;
 }
 
@@ -295,36 +371,48 @@ static int aspeed_adc_remove(struct platform_device *pdev)
 {
 	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
 	struct aspeed_adc_data *data = iio_priv(indio_dev);
+	const struct aspeed_adc_model_data *model_data;
 
+	model_data = of_device_get_match_data(&pdev->dev);
 	iio_device_unregister(indio_dev);
 	writel(ASPEED_ADC_OPERATION_MODE_POWER_DOWN,
 		data->base + ASPEED_REG_ENGINE_CONTROL);
 	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 (model_data->version <= aspeed_adc_ast2500)
+		clk_hw_unregister_divider(data->clk_prescaler);
 
 	return 0;
 }
 
 static const struct aspeed_adc_model_data ast2400_model_data = {
-	.model_name = "ast2400-adc",
-	.vref_voltage = 2500, // mV
+	.version = aspeed_adc_ast2400,
 	.min_sampling_rate = 10000,
 	.max_sampling_rate = 500000,
+	.num_channels = 16,
 };
 
 static const struct aspeed_adc_model_data ast2500_model_data = {
-	.model_name = "ast2500-adc",
-	.vref_voltage = 1800, // mV
-	.min_sampling_rate = 1,
-	.max_sampling_rate = 1000000,
+	.version = aspeed_adc_ast2500,
+	.min_sampling_rate = 10000,
+	.max_sampling_rate = 500000,
+	.wait_init_sequence = true,
+	.num_channels = 16,
+};
+
+static const struct aspeed_adc_model_data ast2600_model_data = {
+	.version = aspeed_adc_ast2600,
+	.min_sampling_rate = 10000,
+	.max_sampling_rate = 500000,
 	.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);
@@ -341,5 +429,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.25.1

Re: [v2 4/8] iio: adc: aspeed: Allow driver to support ast2600

[Jonathan Cameron]

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

> The adc controller have some differents at ast2600:
> 1. Combine control register of clock divider to continuous bitfields.
> 2. Reference voltage becomes optional which are internal 2500mv/1200mv
> and external range from 900mv to 2700mv
> 3. Divided into two engine, each one has 8 voltage sensing channels.
> 
> This patch handled these changes and compatible with old version.
> 
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>

Hi Billy,

Various comment inline.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/aspeed_adc.c | 176 ++++++++++++++++++++++++++---------
>  1 file changed, 132 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c
> index 99466a5924c7..84f079195375 100644
> --- a/drivers/iio/adc/aspeed_adc.c
> +++ b/drivers/iio/adc/aspeed_adc.c
> @@ -1,8 +1,9 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /*
> - * Aspeed AST2400/2500 ADC
> + * Aspeed AST2400/2500/2600 ADC
>   *
>   * Copyright (C) 2017 Google, Inc.
> + * Copyright (C) 2021 Aspeed Technology Inc.
>   */
>  
>  #include <linux/clk.h>
> @@ -55,12 +56,17 @@
>  #define ASPEED_ADC_INIT_POLLING_TIME	500
>  #define ASPEED_ADC_INIT_TIMEOUT		500000
>  
> +enum aspeed_adc_version {
> +	aspeed_adc_ast2400,
> +	aspeed_adc_ast2500,
> +	aspeed_adc_ast2600,
> +};

Nitpick! Blank line here.

>  struct aspeed_adc_model_data {
> -	const char *model_name;
> +	enum aspeed_adc_version version;

Currently we have a slightly odd mixture of
a device type enum and a device specific structure
containing that enum which is then used
to make additional decisions in the driver.

I would have a
	struct aspeed_adc_data *model_data;
pointer in here and make sure that has all the
relevant stuff so that we never need to
do special handling by 'version'.

>  	unsigned int min_sampling_rate;	// Hz
>  	unsigned int max_sampling_rate;	// Hz
> -	unsigned int vref_voltage;	// mV
>  	bool wait_init_sequence;
> +	unsigned int num_channels;
>  };
>  
>  struct aspeed_adc_data {
> @@ -70,6 +76,7 @@ struct aspeed_adc_data {
>  	struct clk_hw		*clk_prescaler;
>  	struct clk_hw		*clk_scaler;
>  	struct reset_control	*rst;
> +	int			vref;

Ideally I would break this into 2 patches.
1) Refactoring like this and the enum above.
2) Introduce the new part number using the result of that
   refactoring.

>  };
>  
>  #define ASPEED_CHAN(_idx, _data_reg_addr) {			\
> @@ -106,8 +113,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:
> @@ -115,7 +120,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;
>  		*val2 = ASPEED_RESOLUTION_BITS;
>  		return IIO_VAL_FRACTIONAL_LOG2;
>  
> @@ -182,6 +187,55 @@ static const struct iio_info aspeed_adc_iio_info = {
>  	.debugfs_reg_access = aspeed_adc_reg_access,
>  };
>  
> +static int aspeed_adc_vref_config(struct platform_device *pdev)

As mentioned below, this would be slightly simpler if you just passed
in the struct iio_dev rather than pdev.

> +{
> +	const struct aspeed_adc_model_data *model_data;
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct aspeed_adc_data *data = iio_priv(indio_dev);
> +	int vref;
> +	u32 adc_engine_control_reg_val =
> +		readl(data->base + ASPEED_REG_ENGINE_CONTROL);
> +
> +	model_data = of_device_get_match_data(&pdev->dev);
> +	switch (model_data->version) {
> +	case aspeed_adc_ast2400:
As mentioned above, instead of using a version enum, just put
everything required in the model data structure.
e.g. add a vref_fixed field and set it for these two devices.

	if (model_data->vref_fixed) {
		data->vref = model_data->vref_fixed;
		return 0;
	}

	data->reg = devm_regulator_get_optional(pdev->dev, "vref");
	if (!IS_ERR(data->reg)) {
		ret = regulator_enable(data->reg);
		if (ret)
			return ret;
		// need to provide a little function to turn the reg off.
		ret = devm_add_action_or_reset(aspeed_adc_regdisable, data->reg);
		if (ret)
			return ret;
		
		ret = regulator_get_voltage(data->reg);
		if (ret < 0)
			return ret;
		data->vref = ret;
		//Here do the register set depending on value.
	} else {
		if (PTR_ERR(data->reg != -ENODEV)
			return PTR_ERR(data->reg);

		ret = of_property_read_u32(pdev->dev.of_node, "vref_int_mv", data->vref);
		if (ret)
			return ret;

		data->vref = ret;
		//here to the register set to configure the internal regulator.		
	}

> +		vref = 2500;
> +		break;
> +	case aspeed_adc_ast2500:
> +		vref = 1800;
> +		break;
> +	case aspeed_adc_ast2600:
> +		if (of_property_read_u32(pdev->dev.of_node, "vref", &vref))
> +			vref = 2500;

As in the binding review, if it's an external reference it needs to be
specified as a regulator.

If that is not present then you can have something like
aspeed,int_vref_mv as an enum that can be either 2500 or 1200.
As this only applies to the ast2600 you should also have a condition
check in the binding yaml to enforce that (so neither external regulator
or aspeed,int_vref_mv is allowed for the wrong devices).  You can also
enforce only one of the regulator and internal vref is supplied in the
dts, but that is a little fiddly to do and so perhaps not worth it.

> +		if (vref == 2500)
> +			writel(adc_engine_control_reg_val |
> +				       ASPEED_ADC_REF_VOLTAGE_2500mV,
> +			       data->base + ASPEED_REG_ENGINE_CONTROL);
> +		else if (vref == 1200)
> +			writel(adc_engine_control_reg_val |
> +				       ASPEED_ADC_REF_VOLTAGE_1200mV,
> +			       data->base + ASPEED_REG_ENGINE_CONTROL);
> +		else if ((vref >= 1550) && (vref <= 2700))
> +			writel(adc_engine_control_reg_val |
> +				       ASPEED_ADC_REF_VOLTAGE_EXT_HIGH,
> +			       data->base + ASPEED_REG_ENGINE_CONTROL);
> +		else if ((vref >= 900) && (vref <= 1650))
> +			writel(adc_engine_control_reg_val |
> +				       ASPEED_ADC_REF_VOLTAGE_EXT_LOW,
> +			       data->base + ASPEED_REG_ENGINE_CONTROL);
> +		else {
> +			dev_err(&pdev->dev, "Vref not support");
> +			return -EOPNOTSUPP;
> +		}
> +		break;
> +	default:
> +		dev_err(&pdev->dev, "ADC version not recognized");
> +		return -EOPNOTSUPP;
> +	}
> +	data->vref = vref;
> +	return 0;
> +}
> +
>  static int aspeed_adc_probe(struct platform_device *pdev)
>  {
>  	struct iio_dev *indio_dev;
> @@ -190,13 +244,16 @@ static int aspeed_adc_probe(struct platform_device *pdev)
>  	const char *clk_parent_name;
>  	int ret;
>  	u32 adc_engine_control_reg_val;
> +	char scaler_clk_name[32];
>  
> +	model_data = of_device_get_match_data(&pdev->dev);
>  	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*data));
>  	if (!indio_dev)
>  		return -ENOMEM;
>  
>  	data = iio_priv(indio_dev);
>  	data->dev = &pdev->dev;
> +	dev_set_drvdata(data->dev, indio_dev);

You use platform_get_drvdata() below, so use platform_set_drvdata for this

>  
>  	data->base = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(data->base))
> @@ -205,29 +262,39 @@ 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);
> +	if (model_data->version <= aspeed_adc_ast2500) {

I'd like to see this based on a flag in the model_data not a version number match.

> +		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);
>  
> -	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", "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;
> +		}
> +	} else {
> +		snprintf(scaler_clk_name, sizeof(scaler_clk_name), "scaler-%s",
> +			 pdev->name);
> +		data->clk_scaler = clk_hw_register_divider(
> +			&pdev->dev, scaler_clk_name, clk_parent_name, 0,
> +			data->base + ASPEED_REG_CLOCK_CONTROL, 0, 16, 0,
> +			&data->clk_lock);
> +		if (IS_ERR(data->clk_scaler))
> +			return PTR_ERR(data->clk_scaler);
>  	}
>  
> -	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");
> @@ -236,11 +303,17 @@ static int aspeed_adc_probe(struct platform_device *pdev)
>  	}
>  	reset_control_deassert(data->rst);
>  
> -	model_data = of_device_get_match_data(&pdev->dev);
> +	ret = aspeed_adc_vref_config(pdev);

As we have the struct iio_dev available here I would pass it into this function.

> +	if (ret)
> +		goto vref_config_error;
>  
>  	if (model_data->wait_init_sequence) {
> +		adc_engine_control_reg_val =
> +			readl(data->base + ASPEED_REG_ENGINE_CONTROL);
>  		/* Enable engine in normal mode. */
> -		writel(ASPEED_ADC_OPERATION_MODE_NORMAL | ASPEED_ADC_ENGINE_ENABLE,
> +		writel(adc_engine_control_reg_val |
> +			       ASPEED_ADC_OPERATION_MODE_NORMAL |
> +			       ASPEED_ADC_ENGINE_ENABLE,
>  		       data->base + ASPEED_REG_ENGINE_CONTROL);
>  
>  		/* Wait for initial sequence complete. */
> @@ -254,22 +327,23 @@ static int aspeed_adc_probe(struct platform_device *pdev)
>  			goto poll_timeout_error;
>  	}
>  
> -	/* Start all channels in normal mode. */
>  	ret = clk_prepare_enable(data->clk_scaler->clk);
>  	if (ret)
>  		goto clk_enable_error;
> -
> -	adc_engine_control_reg_val = GENMASK(31, 16) |
> -		ASPEED_ADC_OPERATION_MODE_NORMAL | ASPEED_ADC_ENGINE_ENABLE;
> +	adc_engine_control_reg_val =
> +		readl(data->base + ASPEED_REG_ENGINE_CONTROL);
> +	/* Start all channels in normal mode. */
> +	adc_engine_control_reg_val |= ASPEED_ADC_CTRL_CHANNEL |
> +				     ASPEED_ADC_OPERATION_MODE_NORMAL |
> +				     ASPEED_ADC_ENGINE_ENABLE;
>  	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->name = dev_name(&pdev->dev);

This name needs to be the part number and I don't think that is true after
this change.  There are lots other ways to identify multiple instances.

>  	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)
> @@ -281,13 +355,15 @@ static int aspeed_adc_probe(struct platform_device *pdev)
>  	writel(ASPEED_ADC_OPERATION_MODE_POWER_DOWN,
>  		data->base + ASPEED_REG_ENGINE_CONTROL);
>  	clk_disable_unprepare(data->clk_scaler->clk);
> +vref_config_error:
>  clk_enable_error:
>  poll_timeout_error:
>  	reset_control_assert(data->rst);
>  reset_error:
>  	clk_hw_unregister_divider(data->clk_scaler);
>  scaler_error:
> -	clk_hw_unregister_divider(data->clk_prescaler);
> +	if (model_data->version <= aspeed_adc_ast2500)
> +		clk_hw_unregister_divider(data->clk_prescaler);

Consider a conversion to devm_add_action_or_reset() based cleanup
for these and potentially all the undwinding currently in remove.
It tends to end up simpler, particularly when there are parts of
probe that only happen for some supported devices.

>  	return ret;
>  }
>  
> @@ -295,36 +371,48 @@ static int aspeed_adc_remove(struct platform_device *pdev)
>  {
>  	struct iio_dev *indio_dev = platform_get_drvdata(pdev);

I'm curious.  Before this patch, this was never set, so did the driver
just crash on remove?

>  	struct aspeed_adc_data *data = iio_priv(indio_dev);
> +	const struct aspeed_adc_model_data *model_data;

I would suggest storing a pointer to this in the iio_priv() as you
reference it from multiple places.

>  
> +	model_data = of_device_get_match_data(&pdev->dev);
>  	iio_device_unregister(indio_dev);
>  	writel(ASPEED_ADC_OPERATION_MODE_POWER_DOWN,
>  		data->base + ASPEED_REG_ENGINE_CONTROL);
>  	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 (model_data->version <= aspeed_adc_ast2500)
> +		clk_hw_unregister_divider(data->clk_prescaler);
>  
>  	return 0;
>  }
>  
>  static const struct aspeed_adc_model_data ast2400_model_data = {
> -	.model_name = "ast2400-adc",
> -	.vref_voltage = 2500, // mV
> +	.version = aspeed_adc_ast2400,
>  	.min_sampling_rate = 10000,
>  	.max_sampling_rate = 500000,
> +	.num_channels = 16,
>  };
>  
>  static const struct aspeed_adc_model_data ast2500_model_data = {
> -	.model_name = "ast2500-adc",
> -	.vref_voltage = 1800, // mV
> -	.min_sampling_rate = 1,
> -	.max_sampling_rate = 1000000,
> +	.version = aspeed_adc_ast2500,
> +	.min_sampling_rate = 10000,
> +	.max_sampling_rate = 500000,
> +	.wait_init_sequence = true,
> +	.num_channels = 16,
> +};
> +
> +static const struct aspeed_adc_model_data ast2600_model_data = {
> +	.version = aspeed_adc_ast2600,
> +	.min_sampling_rate = 10000,
> +	.max_sampling_rate = 500000,
>  	.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);
> @@ -341,5 +429,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");

[v2 5/8] iio: adc: aspeed: Add func to set sampling rate.

[Billy Tsai]

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 | 48 +++++++++++++++++++++++++-----------
 1 file changed, 33 insertions(+), 15 deletions(-)

diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c
index 84f079195375..bb6100228cae 100644
--- a/drivers/iio/adc/aspeed_adc.c
+++ b/drivers/iio/adc/aspeed_adc.c
@@ -55,6 +55,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
 
 enum aspeed_adc_version {
 	aspeed_adc_ast2400,
@@ -77,6 +83,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) {			\
@@ -108,6 +115,26 @@ 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);
+	const struct aspeed_adc_model_data *model_data =
+		of_device_get_match_data(data->dev);
+
+	if (rate < model_data->min_sampling_rate ||
+	    rate > 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)
@@ -138,19 +165,9 @@ 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);
-	const struct aspeed_adc_model_data *model_data =
-			of_device_get_match_data(data->dev);
-
 	switch (mask) {
 	case IIO_CHAN_INFO_SAMP_FREQ:
-		if (val < model_data->min_sampling_rate ||
-			val > 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:
@@ -302,6 +319,10 @@ static int aspeed_adc_probe(struct platform_device *pdev)
 		goto reset_error;
 	}
 	reset_control_deassert(data->rst);
+	ret = clk_prepare_enable(data->clk_scaler->clk);
+	if (ret)
+		goto clk_enable_error;
+	aspeed_adc_set_sampling_rate(indio_dev, ASPEED_ADC_DEF_SAMPLING_RATE);
 
 	ret = aspeed_adc_vref_config(pdev);
 	if (ret)
@@ -327,9 +348,6 @@ static int aspeed_adc_probe(struct platform_device *pdev)
 			goto poll_timeout_error;
 	}
 
-	ret = clk_prepare_enable(data->clk_scaler->clk);
-	if (ret)
-		goto clk_enable_error;
 	adc_engine_control_reg_val =
 		readl(data->base + ASPEED_REG_ENGINE_CONTROL);
 	/* Start all channels in normal mode. */
@@ -354,8 +372,8 @@ static int aspeed_adc_probe(struct platform_device *pdev)
 iio_register_error:
 	writel(ASPEED_ADC_OPERATION_MODE_POWER_DOWN,
 		data->base + ASPEED_REG_ENGINE_CONTROL);
-	clk_disable_unprepare(data->clk_scaler->clk);
 vref_config_error:
+	clk_disable_unprepare(data->clk_scaler->clk);
 clk_enable_error:
 poll_timeout_error:
 	reset_control_assert(data->rst);
-- 
2.25.1

Re: [v2 5/8] iio: adc: aspeed: Add func to set sampling rate.

[Jonathan Cameron]

On Fri, 23 Jul 2021 16:16:18 +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>
A few follow on comments from suggestion in early patch, but otherwise this
looks good to me.

Jonathan

> ---
>  drivers/iio/adc/aspeed_adc.c | 48 +++++++++++++++++++++++++-----------
>  1 file changed, 33 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c
> index 84f079195375..bb6100228cae 100644
> --- a/drivers/iio/adc/aspeed_adc.c
> +++ b/drivers/iio/adc/aspeed_adc.c
> @@ -55,6 +55,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
>  
>  enum aspeed_adc_version {
>  	aspeed_adc_ast2400,
> @@ -77,6 +83,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) {			\
> @@ -108,6 +115,26 @@ 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);
> +	const struct aspeed_adc_model_data *model_data =
> +		of_device_get_match_data(data->dev);

As in previous patch, this would be cleaner if there was a cached copy of
the model_data pointer in data rather than looking it up each time.

> +
> +	if (rate < model_data->min_sampling_rate ||
> +	    rate > 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);

blank line here.

> +	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)
> @@ -138,19 +165,9 @@ 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);
> -	const struct aspeed_adc_model_data *model_data =
> -			of_device_get_match_data(data->dev);
> -
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SAMP_FREQ:
> -		if (val < model_data->min_sampling_rate ||
> -			val > 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:
> @@ -302,6 +319,10 @@ static int aspeed_adc_probe(struct platform_device *pdev)
>  		goto reset_error;
>  	}
>  	reset_control_deassert(data->rst);
> +	ret = clk_prepare_enable(data->clk_scaler->clk);
> +	if (ret)
> +		goto clk_enable_error;
> +	aspeed_adc_set_sampling_rate(indio_dev, ASPEED_ADC_DEF_SAMPLING_RATE);
>  
>  	ret = aspeed_adc_vref_config(pdev);
>  	if (ret)
> @@ -327,9 +348,6 @@ static int aspeed_adc_probe(struct platform_device *pdev)
>  			goto poll_timeout_error;
>  	}
>  
> -	ret = clk_prepare_enable(data->clk_scaler->clk);
> -	if (ret)
> -		goto clk_enable_error;
>  	adc_engine_control_reg_val =
>  		readl(data->base + ASPEED_REG_ENGINE_CONTROL);
>  	/* Start all channels in normal mode. */
> @@ -354,8 +372,8 @@ static int aspeed_adc_probe(struct platform_device *pdev)
>  iio_register_error:
>  	writel(ASPEED_ADC_OPERATION_MODE_POWER_DOWN,
>  		data->base + ASPEED_REG_ENGINE_CONTROL);
> -	clk_disable_unprepare(data->clk_scaler->clk);
>  vref_config_error:
> +	clk_disable_unprepare(data->clk_scaler->clk);
>  clk_enable_error:
>  poll_timeout_error:
>  	reset_control_assert(data->rst);

[v2 6/8] iio: adc: aspeed: Add compensation phase.

[Billy Tsai]

This patch adds a compensation phase to improve the accurate of adc
measurement. This is the builtin function though input half of the
reference voltage to get the adc offset.

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);
+	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);
+	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
+	 * 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);
+	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;
+		if (*val < 0)
+			*val = 0;
+		else if (*val >= ASPEED_ADC_MAX_RAW_DATA)
+			*val = ASPEED_ADC_MAX_RAW_DATA;
 		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;
 	}
-
+	aspeed_adc_compensation(pdev);
 	adc_engine_control_reg_val =
 		readl(data->base + ASPEED_REG_ENGINE_CONTROL);
 	/* Start all channels in normal mode. */
-- 
2.25.1

Re: [v2 6/8] iio: adc: aspeed: Add compensation phase.

[Jonathan Cameron]

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. */

[v2 7/8] iio: adc: aspeed: Fix the calculate error of clock.

[Billy Tsai]

The adc clcok formula is
ast2400/2500:
ADC clock period = PCLK * 2 * (ADC0C[31:17] + 1) * (ADC0C[9:0] + 1)
ast2600:
ADC clock period = PCLK * 2 * (ADC0C[15:0] + 1)
They all have one fixed divided 2 and the legacy driver didn't handle it.
This patch register the fixed factory clock device as the parent of adc
clock scaler to fix this issue.

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

diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c
index 0153b28b83b7..7e674b607e36 100644
--- a/drivers/iio/adc/aspeed_adc.c
+++ b/drivers/iio/adc/aspeed_adc.c
@@ -80,6 +80,7 @@ struct aspeed_adc_data {
 	struct device		*dev;
 	void __iomem		*base;
 	spinlock_t		clk_lock;
+	struct clk_hw		*fixed_div_clk;
 	struct clk_hw		*clk_prescaler;
 	struct clk_hw		*clk_scaler;
 	struct reset_control	*rst;
@@ -310,6 +311,7 @@ static int aspeed_adc_probe(struct platform_device *pdev)
 	int ret;
 	u32 adc_engine_control_reg_val;
 	char scaler_clk_name[32];
+	char fixed_div_clk_name[32];
 
 	model_data = of_device_get_match_data(&pdev->dev);
 	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*data));
@@ -328,10 +330,15 @@ static int aspeed_adc_probe(struct platform_device *pdev)
 	spin_lock_init(&data->clk_lock);
 	clk_parent_name = of_clk_get_parent_name(pdev->dev.of_node, 0);
 	if (model_data->version <= aspeed_adc_ast2500) {
+		/* ADC clock period = PCLK * 2 * (ADC0C[31:17] + 1) * (ADC0C[9:0] + 1) */
+		data->fixed_div_clk = clk_hw_register_fixed_factor(
+			&pdev->dev, "fixed-div", clk_parent_name, 0, 1, 2);
+		if (IS_ERR(data->fixed_div_clk))
+			return PTR_ERR(data->fixed_div_clk);
 		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);
+			&pdev->dev, "prescaler", "fixed-div", 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);
 
@@ -349,14 +356,23 @@ static int aspeed_adc_probe(struct platform_device *pdev)
 			goto scaler_error;
 		}
 	} else {
+		/* ADC clock period = period of PCLK * 2 * (ADC0C[15:0] + 1) */
+		snprintf(fixed_div_clk_name, sizeof(fixed_div_clk_name), "fixed-div-%s",
+			 pdev->name);
+		data->fixed_div_clk = clk_hw_register_fixed_factor(
+			&pdev->dev, fixed_div_clk_name, clk_parent_name, 0, 1, 2);
+		if (IS_ERR(data->fixed_div_clk))
+			return PTR_ERR(data->fixed_div_clk);
 		snprintf(scaler_clk_name, sizeof(scaler_clk_name), "scaler-%s",
 			 pdev->name);
 		data->clk_scaler = clk_hw_register_divider(
 			&pdev->dev, scaler_clk_name, clk_parent_name, 0,
 			data->base + ASPEED_REG_CLOCK_CONTROL, 0, 16, 0,
 			&data->clk_lock);
-		if (IS_ERR(data->clk_scaler))
-			return PTR_ERR(data->clk_scaler);
+		if (IS_ERR(data->clk_scaler)) {
+			ret = PTR_ERR(data->clk_scaler);
+			goto scaler_error;
+		}
 	}
 
 	data->rst = devm_reset_control_get_shared(&pdev->dev, NULL);
@@ -430,6 +446,7 @@ static int aspeed_adc_probe(struct platform_device *pdev)
 scaler_error:
 	if (model_data->version <= aspeed_adc_ast2500)
 		clk_hw_unregister_divider(data->clk_prescaler);
+	clk_hw_unregister_fixed_factor(data->fixed_div_clk);
 	return ret;
 }
 
@@ -448,6 +465,7 @@ static int aspeed_adc_remove(struct platform_device *pdev)
 	clk_hw_unregister_divider(data->clk_scaler);
 	if (model_data->version <= aspeed_adc_ast2500)
 		clk_hw_unregister_divider(data->clk_prescaler);
+	clk_hw_unregister_fixed_factor(data->fixed_div_clk);
 
 	return 0;
 }
-- 
2.25.1

[v2 8/8] iio: adc: aspeed: Support battery sensing.

[Billy Tsai]

In ast2600, ADC integrate dividing circuit at last input channel for
battery sensing. This patch use the dts property "battery-sensing" to
enable this feature makes the last channel of each adc can tolerance
higher voltage than reference voltage.

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

diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c
index 7e674b607e36..6c7e2bb7b1ac 100644
--- a/drivers/iio/adc/aspeed_adc.c
+++ b/drivers/iio/adc/aspeed_adc.c
@@ -45,6 +45,9 @@
 #define ASPEED_ADC_REF_VOLTAGE_1200mV		FIELD_PREP(ASPEED_ADC_REF_VOLTAGE, 1)
 #define ASPEED_ADC_REF_VOLTAGE_EXT_HIGH		FIELD_PREP(ASPEED_ADC_REF_VOLTAGE, 2)
 #define ASPEED_ADC_REF_VOLTAGE_EXT_LOW		FIELD_PREP(ASPEED_ADC_REF_VOLTAGE, 3)
+#define ASPEED_ADC_BATTERY_SENSING_DIV		BIT(6)
+#define ASPEED_ADC_BATTERY_SENSING_DIV_2_3	FIELD_PREP(ASPEED_ADC_BATTERY_SENSING_DIV, 0)
+#define ASPEED_ADC_BATTERY_SENSING_DIV_1_3	FIELD_PREP(ASPEED_ADC_BATTERY_SENSING_DIV, 1)
 #define ASPEED_ADC_CTRL_INIT_RDY		BIT(8)
 #define ASPEED_ADC_CH7_MODE			BIT(12)
 #define ASPEED_ADC_CH7_NORMAL			FIELD_PREP(ASPEED_ADC_CH7_MODE, 0)
@@ -76,6 +79,11 @@ struct aspeed_adc_model_data {
 	unsigned int num_channels;
 };
 
+struct adc_gain {
+	u8 mult;
+	u8 div;
+};
+
 struct aspeed_adc_data {
 	struct device		*dev;
 	void __iomem		*base;
@@ -87,6 +95,8 @@ struct aspeed_adc_data {
 	int			vref;
 	u32			sample_period_ns;
 	int			cv;
+	bool			battery_sensing;
+	struct adc_gain		battery_mode_gain;
 };
 
 #define ASPEED_CHAN(_idx, _data_reg_addr) {			\
@@ -185,14 +195,38 @@ 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);
+	u32 adc_engine_control_reg_val;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		*val = readw(data->base + chan->address) + data->cv;
-		if (*val < 0)
-			*val = 0;
-		else if (*val >= ASPEED_ADC_MAX_RAW_DATA)
-			*val = ASPEED_ADC_MAX_RAW_DATA;
+		if (data->battery_sensing && chan->channel == 7) {
+			adc_engine_control_reg_val =
+				readl(data->base + ASPEED_REG_ENGINE_CONTROL);
+			writel(adc_engine_control_reg_val |
+				       ASPEED_ADC_CH7_BATTERY |
+				       ASPEED_ADC_BATTERY_SENSING_ENABLE,
+			       data->base + ASPEED_REG_ENGINE_CONTROL);
+			/*
+			 * After enable battery sensing mode need to wait some time for adc stable
+			 * Experiment result is 1ms.
+			 */
+			mdelay(1);
+			*val = readw(data->base + chan->address) + data->cv;
+			if (*val < 0)
+				*val = 0;
+			else if (*val >= ASPEED_ADC_MAX_RAW_DATA)
+				*val = ASPEED_ADC_MAX_RAW_DATA;
+			*val = (*val * data->battery_mode_gain.mult) /
+			       data->battery_mode_gain.div;
+			writel(adc_engine_control_reg_val,
+			       data->base + ASPEED_REG_ENGINE_CONTROL);
+		} else {
+			*val = readw(data->base + chan->address) + data->cv;
+			if (*val < 0)
+				*val = 0;
+			else if (*val >= ASPEED_ADC_MAX_RAW_DATA)
+				*val = ASPEED_ADC_MAX_RAW_DATA;
+		}
 		return IIO_VAL_INT;
 
 	case IIO_CHAN_INFO_SCALE:
@@ -392,6 +426,22 @@ static int aspeed_adc_probe(struct platform_device *pdev)
 	if (ret)
 		goto vref_config_error;
 
+	if (of_find_property(data->dev->of_node, "battery-sensing", NULL)) {
+		if (model_data->version >= aspeed_adc_ast2600) {
+			data->battery_sensing = 1;
+			if (readl(data->base + ASPEED_REG_ENGINE_CONTROL) &
+			    ASPEED_ADC_BATTERY_SENSING_DIV_1_3) {
+				data->battery_mode_gain.mult = 3;
+				data->battery_mode_gain.div = 1;
+			} else {
+				data->battery_mode_gain.mult = 3;
+				data->battery_mode_gain.div = 2;
+			}
+		} else
+			dev_warn(&pdev->dev,
+				 "Failed to enable battey-sensing mode\n");
+	}
+
 	if (model_data->wait_init_sequence) {
 		adc_engine_control_reg_val =
 			readl(data->base + ASPEED_REG_ENGINE_CONTROL);
-- 
2.25.1

Re: [v2 8/8] iio: adc: aspeed: Support battery sensing.

[Jonathan Cameron]

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

> In ast2600, ADC integrate dividing circuit at last input channel for
> battery sensing. This patch use the dts property "battery-sensing" to
> enable this feature makes the last channel of each adc can tolerance

this feature allows the last channel of each ADC to tolerate a higher
voltage than the reference voltage.

> higher voltage than reference voltage.
> 
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
This looks fine otherwise, but one more general question inline.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/aspeed_adc.c | 60 +++++++++++++++++++++++++++++++++---
>  1 file changed, 55 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c
> index 7e674b607e36..6c7e2bb7b1ac 100644
> --- a/drivers/iio/adc/aspeed_adc.c
> +++ b/drivers/iio/adc/aspeed_adc.c
> @@ -45,6 +45,9 @@
>  #define ASPEED_ADC_REF_VOLTAGE_1200mV		FIELD_PREP(ASPEED_ADC_REF_VOLTAGE, 1)
>  #define ASPEED_ADC_REF_VOLTAGE_EXT_HIGH		FIELD_PREP(ASPEED_ADC_REF_VOLTAGE, 2)
>  #define ASPEED_ADC_REF_VOLTAGE_EXT_LOW		FIELD_PREP(ASPEED_ADC_REF_VOLTAGE, 3)
> +#define ASPEED_ADC_BATTERY_SENSING_DIV		BIT(6)
> +#define ASPEED_ADC_BATTERY_SENSING_DIV_2_3	FIELD_PREP(ASPEED_ADC_BATTERY_SENSING_DIV, 0)
> +#define ASPEED_ADC_BATTERY_SENSING_DIV_1_3	FIELD_PREP(ASPEED_ADC_BATTERY_SENSING_DIV, 1)
>  #define ASPEED_ADC_CTRL_INIT_RDY		BIT(8)
>  #define ASPEED_ADC_CH7_MODE			BIT(12)
>  #define ASPEED_ADC_CH7_NORMAL			FIELD_PREP(ASPEED_ADC_CH7_MODE, 0)
> @@ -76,6 +79,11 @@ struct aspeed_adc_model_data {
>  	unsigned int num_channels;
>  };
>  
> +struct adc_gain {
> +	u8 mult;
> +	u8 div;
> +};
> +
>  struct aspeed_adc_data {
>  	struct device		*dev;
>  	void __iomem		*base;
> @@ -87,6 +95,8 @@ struct aspeed_adc_data {
>  	int			vref;
>  	u32			sample_period_ns;
>  	int			cv;
> +	bool			battery_sensing;
> +	struct adc_gain		battery_mode_gain;
>  };
>  
>  #define ASPEED_CHAN(_idx, _data_reg_addr) {			\
> @@ -185,14 +195,38 @@ 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);
> +	u32 adc_engine_control_reg_val;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> -		*val = readw(data->base + chan->address) + data->cv;
> -		if (*val < 0)
> -			*val = 0;
> -		else if (*val >= ASPEED_ADC_MAX_RAW_DATA)
> -			*val = ASPEED_ADC_MAX_RAW_DATA;

Not related to this patch, but can the device support a per channel
reference selection? I.e. some channels use different internal references or
external references from others?

If so we should consider if it is necessary to expose that functionality
in the dt-binding.

> +		if (data->battery_sensing && chan->channel == 7) {
> +			adc_engine_control_reg_val =
> +				readl(data->base + ASPEED_REG_ENGINE_CONTROL);
> +			writel(adc_engine_control_reg_val |
> +				       ASPEED_ADC_CH7_BATTERY |
> +				       ASPEED_ADC_BATTERY_SENSING_ENABLE,
> +			       data->base + ASPEED_REG_ENGINE_CONTROL);
> +			/*
> +			 * After enable battery sensing mode need to wait some time for adc stable
> +			 * Experiment result is 1ms.
> +			 */
> +			mdelay(1);
> +			*val = readw(data->base + chan->address) + data->cv;
> +			if (*val < 0)
> +				*val = 0;
> +			else if (*val >= ASPEED_ADC_MAX_RAW_DATA)
> +				*val = ASPEED_ADC_MAX_RAW_DATA;
> +			*val = (*val * data->battery_mode_gain.mult) /
> +			       data->battery_mode_gain.div;
> +			writel(adc_engine_control_reg_val,
> +			       data->base + ASPEED_REG_ENGINE_CONTROL);
> +		} else {
> +			*val = readw(data->base + chan->address) + data->cv;
> +			if (*val < 0)
> +				*val = 0;
> +			else if (*val >= ASPEED_ADC_MAX_RAW_DATA)
> +				*val = ASPEED_ADC_MAX_RAW_DATA;
> +		}
>  		return IIO_VAL_INT;
>  
>  	case IIO_CHAN_INFO_SCALE:
> @@ -392,6 +426,22 @@ static int aspeed_adc_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto vref_config_error;
>  
> +	if (of_find_property(data->dev->of_node, "battery-sensing", NULL)) {
> +		if (model_data->version >= aspeed_adc_ast2600) {
> +			data->battery_sensing = 1;
> +			if (readl(data->base + ASPEED_REG_ENGINE_CONTROL) &
> +			    ASPEED_ADC_BATTERY_SENSING_DIV_1_3) {
> +				data->battery_mode_gain.mult = 3;
> +				data->battery_mode_gain.div = 1;
> +			} else {
> +				data->battery_mode_gain.mult = 3;
> +				data->battery_mode_gain.div = 2;
> +			}
> +		} else
> +			dev_warn(&pdev->dev,
> +				 "Failed to enable battey-sensing mode\n");
> +	}
> +
>  	if (model_data->wait_init_sequence) {
>  		adc_engine_control_reg_val =
>  			readl(data->base + ASPEED_REG_ENGINE_CONTROL);

Re: [v2 8/8] iio: adc: aspeed: Support battery sensing.

[Billy Tsai]

On 2021/7/23, 11:57 PM, "Jonathan Cameron" <Jonathan.Cameron@Huawei.com> wrote:

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

    >   > In ast2600, ADC integrate dividing circuit at last input channel for
    >   > battery sensing. This patch use the dts property "battery-sensing" to
    >   > enable this feature makes the last channel of each adc can tolerance

    >   this feature allows the last channel of each ADC to tolerate a higher
    >   voltage than the reference voltage.

    >   > higher voltage than reference voltage.
    >   > 
    >   > Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
    >   This looks fine otherwise, but one more general question inline.

Hi Jonathan,

I reply the question inline.

Thanks,

Best Regards,
Billy Tsai

    >   Thanks,

    >   Jonathan

    >   > ---
    >   >  drivers/iio/adc/aspeed_adc.c | 60 +++++++++++++++++++++++++++++++++---
    >   >  1 file changed, 55 insertions(+), 5 deletions(-)
    >   > 
    >   > diff --git a/drivers/iio/adc/aspeed_adc.c b/drivers/iio/adc/aspeed_adc.c
    >   > index 7e674b607e36..6c7e2bb7b1ac 100644
    >   > --- a/drivers/iio/adc/aspeed_adc.c
    >   > +++ b/drivers/iio/adc/aspeed_adc.c
    >   > @@ -45,6 +45,9 @@
    >   >  #define ASPEED_ADC_REF_VOLTAGE_1200mV		FIELD_PREP(ASPEED_ADC_REF_VOLTAGE, 1)
    >   >  #define ASPEED_ADC_REF_VOLTAGE_EXT_HIGH		FIELD_PREP(ASPEED_ADC_REF_VOLTAGE, 2)
    >   >  #define ASPEED_ADC_REF_VOLTAGE_EXT_LOW		FIELD_PREP(ASPEED_ADC_REF_VOLTAGE, 3)
    >   > +#define ASPEED_ADC_BATTERY_SENSING_DIV		BIT(6)
    >   > +#define ASPEED_ADC_BATTERY_SENSING_DIV_2_3	FIELD_PREP(ASPEED_ADC_BATTERY_SENSING_DIV, 0)
    >   > +#define ASPEED_ADC_BATTERY_SENSING_DIV_1_3	FIELD_PREP(ASPEED_ADC_BATTERY_SENSING_DIV, 1)
    >   >  #define ASPEED_ADC_CTRL_INIT_RDY		BIT(8)
    >   >  #define ASPEED_ADC_CH7_MODE			BIT(12)
    >   >  #define ASPEED_ADC_CH7_NORMAL			FIELD_PREP(ASPEED_ADC_CH7_MODE, 0)
    >   > @@ -76,6 +79,11 @@ struct aspeed_adc_model_data {
    >   >  	unsigned int num_channels;
    >   >  };
    >   >  
    >   > +struct adc_gain {
    >   > +	u8 mult;
    >   > +	u8 div;
    >   > +};
    >   > +
    >   >  struct aspeed_adc_data {
    >   >  	struct device		*dev;
    >   >  	void __iomem		*base;
    >   > @@ -87,6 +95,8 @@ struct aspeed_adc_data {
    >   >  	int			vref;
    >   >  	u32			sample_period_ns;
    >   >  	int			cv;
    >   > +	bool			battery_sensing;
    >   > +	struct adc_gain		battery_mode_gain;
    >   >  };
    >   >  
    >   >  #define ASPEED_CHAN(_idx, _data_reg_addr) {			\
    >   > @@ -185,14 +195,38 @@ 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);
    >   > +	u32 adc_engine_control_reg_val;
    >   >  
    >   >  	switch (mask) {
    >   >  	case IIO_CHAN_INFO_RAW:
    >   > -		*val = readw(data->base + chan->address) + data->cv;
    >   > -		if (*val < 0)
    >   > -			*val = 0;
    >   > -		else if (*val >= ASPEED_ADC_MAX_RAW_DATA)
    >   > -			*val = ASPEED_ADC_MAX_RAW_DATA;

    >   Not related to this patch, but can the device support a per channel
    >   reference selection? I.e. some channels use different internal references or
    >   external references from others?

No, the device can only choice one reference voltage for all of the channels.

    >   If so we should consider if it is necessary to expose that functionality
    >   in the dt-binding.

    >   > +		if (data->battery_sensing && chan->channel == 7) {
    >   > +			adc_engine_control_reg_val =
    >   > +				readl(data->base + ASPEED_REG_ENGINE_CONTROL);
    >   > +			writel(adc_engine_control_reg_val |
    >   > +				       ASPEED_ADC_CH7_BATTERY |
    >   > +				       ASPEED_ADC_BATTERY_SENSING_ENABLE,
    >   > +			       data->base + ASPEED_REG_ENGINE_CONTROL);
    >   > +			/*
    >   > +			 * After enable battery sensing mode need to wait some time for adc stable
    >   > +			 * Experiment result is 1ms.
    >   > +			 */
    >   > +			mdelay(1);
    >   > +			*val = readw(data->base + chan->address) + data->cv;
    >   > +			if (*val < 0)
    >   > +				*val = 0;
    >   > +			else if (*val >= ASPEED_ADC_MAX_RAW_DATA)
    >   > +				*val = ASPEED_ADC_MAX_RAW_DATA;
    >   > +			*val = (*val * data->battery_mode_gain.mult) /
    >   > +			       data->battery_mode_gain.div;
    >   > +			writel(adc_engine_control_reg_val,
    >   > +			       data->base + ASPEED_REG_ENGINE_CONTROL);
    >   > +		} else {
    >   > +			*val = readw(data->base + chan->address) + data->cv;
    >   > +			if (*val < 0)
    >   > +				*val = 0;
    >   > +			else if (*val >= ASPEED_ADC_MAX_RAW_DATA)
    >   > +				*val = ASPEED_ADC_MAX_RAW_DATA;
    >   > +		}
    >   >  		return IIO_VAL_INT;
    >   >  
    >   >  	case IIO_CHAN_INFO_SCALE:
    >   > @@ -392,6 +426,22 @@ static int aspeed_adc_probe(struct platform_device *pdev)
    >   >  	if (ret)
    >   >  		goto vref_config_error;
    >   >  
    >   > +	if (of_find_property(data->dev->of_node, "battery-sensing", NULL)) {
    >   > +		if (model_data->version >= aspeed_adc_ast2600) {
    >   > +			data->battery_sensing = 1;
    >   > +			if (readl(data->base + ASPEED_REG_ENGINE_CONTROL) &
    >   > +			    ASPEED_ADC_BATTERY_SENSING_DIV_1_3) {
    >   > +				data->battery_mode_gain.mult = 3;
    >   > +				data->battery_mode_gain.div = 1;
    >   > +			} else {
    >   > +				data->battery_mode_gain.mult = 3;
    >   > +				data->battery_mode_gain.div = 2;
    >   > +			}
    >   > +		} else
    >   > +			dev_warn(&pdev->dev,
    >   > +				 "Failed to enable battey-sensing mode\n");
    >   > +	}
    >   > +
    >   >  	if (model_data->wait_init_sequence) {
    >   >  		adc_engine_control_reg_val =
    >   >  			readl(data->base + ASPEED_REG_ENGINE_CONTROL);