[PATCH 0/7] iio: adc: sc27xx: adjust structure and add PMIC's support

[PATCH 0/7] iio: adc: sc27xx: adjust structure and add PMIC's support

[Cixi Geng]

From: Cixi Geng <cixi.geng1@unisoc.com>

this patchset add a sc27xx_adc_variant_data structure
and add sc272*,sc273* and ump9620 PMIC support.
also add ump9620 PMIC suspend and resume pm implement.

Cixi Geng (7):
  dt-bindings:iio:adc: add sprd,ump9620-adc dtbindings
  iio: adc: sc27xx: fix read big scale voltage not right
  iio: adc: sc27xx: structure adjuststment and optimization
  iio: adc: sc27xx: add support for PMIC sc2720 and sc2721
  iio: adc: sc27xx: add support for PMIC sc2730
  iio: adc: sc27xx: add support for PMIC ump9620
  iio: adc: sc27xx: add Ump9620 ADC suspend and resume pm support

 .../bindings/iio/adc/sprd,sc2720-adc.yaml     |  19 +
 drivers/iio/adc/sc27xx_adc.c                  | 767 +++++++++++++++++-
 2 files changed, 759 insertions(+), 27 deletions(-)

-- 
2.25.1

[PATCH 1/7] dt-bindings:iio:adc: add sprd,ump9620-adc dtbindings

[Cixi Geng]

From: Cixi Geng <cixi.geng1@unisoc.com>

sprd,ump9620-adc is one variant of sc27xx series, add ump9620 in
dtbindings.

Signed-off-by: Chunyan Zhang <zhang.lyra@gmail.com>
Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com>
---
 .../bindings/iio/adc/sprd,sc2720-adc.yaml     | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/sprd,sc2720-adc.yaml b/Documentation/devicetree/bindings/iio/adc/sprd,sc2720-adc.yaml
index caa3ee0b4b8c..cd20ff17e58c 100644
--- a/Documentation/devicetree/bindings/iio/adc/sprd,sc2720-adc.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/sprd,sc2720-adc.yaml
@@ -20,6 +20,7 @@ properties:
       - sprd,sc2723-adc
       - sprd,sc2730-adc
       - sprd,sc2731-adc
+      - sprd,ump9620-adc
 
   reg:
     maxItems: 1
@@ -36,11 +37,29 @@ properties:
   nvmem-cells:
     maxItems: 2
 
+if:
+  not:
+    properties:
+      compatible:
+        contains:
+          enum:
+            - sprd,ump9620-adc
+then:
   nvmem-cell-names:
     items:
       - const: big_scale_calib
       - const: small_scale_calib
 
+else:
+  nvmem-cell-names:
+    items:
+      - const: big_scale_calib1
+      - const: big_scale_calib2
+      - const: small_scale_calib1
+      - const: small_scale_calib2
+      - const: vbat_det_cal1
+      - const: vbat_det_cal2
+
 required:
   - compatible
   - reg
-- 
2.25.1

Re: [PATCH 1/7] dt-bindings:iio:adc: add sprd,ump9620-adc dtbindings

[Rob Herring]

On Thu, 06 Jan 2022 20:59:41 +0800, Cixi Geng wrote:
> From: Cixi Geng <cixi.geng1@unisoc.com>
> 
> sprd,ump9620-adc is one variant of sc27xx series, add ump9620 in
> dtbindings.
> 
> Signed-off-by: Chunyan Zhang <zhang.lyra@gmail.com>
> Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com>
> ---
>  .../bindings/iio/adc/sprd,sc2720-adc.yaml     | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/sprd,sc2720-adc.yaml: else: 'nvmem-cell-names' is not one of ['$ref', 'additionalItems', 'additionalProperties', 'allOf', 'anyOf', 'const', 'contains', 'default', 'dependencies', 'dependentRequired', 'dependentSchemas', 'deprecated', 'description', 'else', 'enum', 'exclusiveMaximum', 'exclusiveMinimum', 'items', 'if', 'minItems', 'minimum', 'maxItems', 'maximum', 'multipleOf', 'not', 'oneOf', 'pattern', 'patternProperties', 'properties', 'required', 'then', 'type', 'typeSize', 'unevaluatedProperties', 'uniqueItems']
	from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/sprd,sc2720-adc.yaml: then: 'nvmem-cell-names' is not one of ['$ref', 'additionalItems', 'additionalProperties', 'allOf', 'anyOf', 'const', 'contains', 'default', 'dependencies', 'dependentRequired', 'dependentSchemas', 'deprecated', 'description', 'else', 'enum', 'exclusiveMaximum', 'exclusiveMinimum', 'items', 'if', 'minItems', 'minimum', 'maxItems', 'maximum', 'multipleOf', 'not', 'oneOf', 'pattern', 'patternProperties', 'properties', 'required', 'then', 'type', 'typeSize', 'unevaluatedProperties', 'uniqueItems']
	from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/sprd,sc2720-adc.yaml: ignoring, error in schema: else
Documentation/devicetree/bindings/iio/adc/sprd,sc2720-adc.example.dt.yaml:0:0: /example-0/pmic/adc@480: failed to match any schema with compatible: ['sprd,sc2731-adc']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1576074

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

[PATCH 2/7] iio: adc: sc27xx: fix read big scale voltage not right

[Cixi Geng]

From: Cixi Geng <cixi.geng1@unisoc.com>

Fix wrong configuration value of SC27XX_ADC_SCALE_MASK and
SC27XX_ADC_SCALE_SHIFT by spec documetation.

Signed-off-by: Yuming Zhu <yuming.zhu1@unisoc.com>
Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com>
---
 drivers/iio/adc/sc27xx_adc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c
index 00098caf6d9e..aee076c8e2b1 100644
--- a/drivers/iio/adc/sc27xx_adc.c
+++ b/drivers/iio/adc/sc27xx_adc.c
@@ -36,8 +36,8 @@
 
 /* Bits and mask definition for SC27XX_ADC_CH_CFG register */
 #define SC27XX_ADC_CHN_ID_MASK		GENMASK(4, 0)
-#define SC27XX_ADC_SCALE_MASK		GENMASK(10, 8)
-#define SC27XX_ADC_SCALE_SHIFT		8
+#define SC27XX_ADC_SCALE_MASK		GENMASK(10, 9)
+#define SC27XX_ADC_SCALE_SHIFT		9
 
 /* Bits definitions for SC27XX_ADC_INT_EN registers */
 #define SC27XX_ADC_IRQ_EN		BIT(0)
-- 
2.25.1

Re: [PATCH 2/7] iio: adc: sc27xx: fix read big scale voltage not right

[Baolin Wang]

On Thu, Jan 6, 2022 at 9:00 PM Cixi Geng <gengcixi@gmail.com> wrote:
>
> From: Cixi Geng <cixi.geng1@unisoc.com>
>
> Fix wrong configuration value of SC27XX_ADC_SCALE_MASK and
> SC27XX_ADC_SCALE_SHIFT by spec documetation.
>
> Signed-off-by: Yuming Zhu <yuming.zhu1@unisoc.com>
> Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com>

Reviewed-by: Baolin Wang <baolin.wang7@gmail.com>

> ---
>  drivers/iio/adc/sc27xx_adc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c
> index 00098caf6d9e..aee076c8e2b1 100644
> --- a/drivers/iio/adc/sc27xx_adc.c
> +++ b/drivers/iio/adc/sc27xx_adc.c
> @@ -36,8 +36,8 @@
>
>  /* Bits and mask definition for SC27XX_ADC_CH_CFG register */
>  #define SC27XX_ADC_CHN_ID_MASK         GENMASK(4, 0)
> -#define SC27XX_ADC_SCALE_MASK          GENMASK(10, 8)
> -#define SC27XX_ADC_SCALE_SHIFT         8
> +#define SC27XX_ADC_SCALE_MASK          GENMASK(10, 9)
> +#define SC27XX_ADC_SCALE_SHIFT         9
>
>  /* Bits definitions for SC27XX_ADC_INT_EN registers */
>  #define SC27XX_ADC_IRQ_EN              BIT(0)
> --
> 2.25.1
>


-- 
Baolin Wang

Re: [PATCH 2/7] iio: adc: sc27xx: fix read big scale voltage not right

[Jonathan Cameron]

On Fri, 7 Jan 2022 14:55:15 +0800
Baolin Wang <baolin.wang7@gmail.com> wrote:

> On Thu, Jan 6, 2022 at 9:00 PM Cixi Geng <gengcixi@gmail.com> wrote:
> >
> > From: Cixi Geng <cixi.geng1@unisoc.com>
> >
> > Fix wrong configuration value of SC27XX_ADC_SCALE_MASK and
> > SC27XX_ADC_SCALE_SHIFT by spec documetation.
> >
> > Signed-off-by: Yuming Zhu <yuming.zhu1@unisoc.com>
> > Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com>  
> 
> Reviewed-by: Baolin Wang <baolin.wang7@gmail.com>

Fixes: tag for backports?

or is this having no visible result today?

Jonathan

> 
> > ---
> >  drivers/iio/adc/sc27xx_adc.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c
> > index 00098caf6d9e..aee076c8e2b1 100644
> > --- a/drivers/iio/adc/sc27xx_adc.c
> > +++ b/drivers/iio/adc/sc27xx_adc.c
> > @@ -36,8 +36,8 @@
> >
> >  /* Bits and mask definition for SC27XX_ADC_CH_CFG register */
> >  #define SC27XX_ADC_CHN_ID_MASK         GENMASK(4, 0)
> > -#define SC27XX_ADC_SCALE_MASK          GENMASK(10, 8)
> > -#define SC27XX_ADC_SCALE_SHIFT         8
> > +#define SC27XX_ADC_SCALE_MASK          GENMASK(10, 9)
> > +#define SC27XX_ADC_SCALE_SHIFT         9
> >
> >  /* Bits definitions for SC27XX_ADC_INT_EN registers */
> >  #define SC27XX_ADC_IRQ_EN              BIT(0)
> > --
> > 2.25.1
> >  
> 
> 

[PATCH 3/7] iio: adc: sc27xx: structure adjuststment and optimization

[Cixi Geng]

From: Cixi Geng <cixi.geng1@unisoc.com>

Introduce one variant device data structure to be compatible
with SC2731 PMIC since it has different scale and ratio calculation
and so on.

Signed-off-by: Yuming Zhu <yuming.zhu1@unisoc.com>
Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com>
---
 drivers/iio/adc/sc27xx_adc.c | 94 ++++++++++++++++++++++++++++++------
 1 file changed, 79 insertions(+), 15 deletions(-)

diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c
index aee076c8e2b1..d2712e54ee79 100644
--- a/drivers/iio/adc/sc27xx_adc.c
+++ b/drivers/iio/adc/sc27xx_adc.c
@@ -12,9 +12,9 @@
 #include <linux/slab.h>
 
 /* PMIC global registers definition */
-#define SC27XX_MODULE_EN		0xc08
+#define SC2731_MODULE_EN		0xc08
 #define SC27XX_MODULE_ADC_EN		BIT(5)
-#define SC27XX_ARM_CLK_EN		0xc10
+#define SC2731_ARM_CLK_EN		0xc10
 #define SC27XX_CLK_ADC_EN		BIT(5)
 #define SC27XX_CLK_ADC_CLK_EN		BIT(6)
 
@@ -78,6 +78,23 @@ struct sc27xx_adc_data {
 	int channel_scale[SC27XX_ADC_CHANNEL_MAX];
 	u32 base;
 	int irq;
+	const struct sc27xx_adc_variant_data *var_data;
+};
+
+/*
+ * Since different PMICs of SC27xx series can have different
+ * address and ratio, we should save ratio config and base
+ * in the device data structure.
+ */
+struct sc27xx_adc_variant_data {
+	u32 module_en;
+	u32 clk_en;
+	u32 scale_shift;
+	u32 scale_mask;
+	const struct sc27xx_adc_linear_graph *bscale_cal;
+	const struct sc27xx_adc_linear_graph *sscale_cal;
+	void (*init_scale)(struct sc27xx_adc_data *data);
+	int (*get_ratio)(int channel, int scale);
 };
 
 struct sc27xx_adc_linear_graph {
@@ -103,6 +120,16 @@ static struct sc27xx_adc_linear_graph small_scale_graph = {
 	100, 341,
 };
 
+static const struct sc27xx_adc_linear_graph sc2731_big_scale_graph_calib = {
+	4200, 850,
+	3600, 728,
+};
+
+static const struct sc27xx_adc_linear_graph sc2731_small_scale_graph_calib = {
+	1000, 838,
+	100, 84,
+};
+
 static const struct sc27xx_adc_linear_graph big_scale_graph_calib = {
 	4200, 856,
 	3600, 733,
@@ -130,11 +157,11 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data,
 	size_t len;
 
 	if (big_scale) {
-		calib_graph = &big_scale_graph_calib;
+		calib_graph = data->var_data->bscale_cal;
 		graph = &big_scale_graph;
 		cell_name = "big_scale_calib";
 	} else {
-		calib_graph = &small_scale_graph_calib;
+		calib_graph = data->var_data->sscale_cal;
 		graph = &small_scale_graph;
 		cell_name = "small_scale_calib";
 	}
@@ -160,7 +187,7 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data,
 	return 0;
 }
 
-static int sc27xx_adc_get_ratio(int channel, int scale)
+static int sc2731_adc_get_ratio(int channel, int scale)
 {
 	switch (channel) {
 	case 1:
@@ -185,6 +212,21 @@ static int sc27xx_adc_get_ratio(int channel, int scale)
 	return SC27XX_VOLT_RATIO(1, 1);
 }
 
+/*
+ * According to the datasheet set specific value on some channel.
+ */
+static void sc2731_adc_scale_init(struct sc27xx_adc_data *data)
+{
+	int i;
+
+	for (i = 0; i < SC27XX_ADC_CHANNEL_MAX; i++) {
+		if (i == 5)
+			data->channel_scale[i] = 1;
+		else
+			data->channel_scale[i] = 0;
+	}
+}
+
 static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
 			   int scale, int *val)
 {
@@ -208,10 +250,11 @@ static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
 		goto disable_adc;
 
 	/* Configure the channel id and scale */
-	tmp = (scale << SC27XX_ADC_SCALE_SHIFT) & SC27XX_ADC_SCALE_MASK;
+	tmp = (scale << data->var_data->scale_shift) & data->var_data->scale_mask;
 	tmp |= channel & SC27XX_ADC_CHN_ID_MASK;
 	ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CH_CFG,
-				 SC27XX_ADC_CHN_ID_MASK | SC27XX_ADC_SCALE_MASK,
+				 SC27XX_ADC_CHN_ID_MASK |
+				 data->var_data->scale_mask,
 				 tmp);
 	if (ret)
 		goto disable_adc;
@@ -262,8 +305,9 @@ static void sc27xx_adc_volt_ratio(struct sc27xx_adc_data *data,
 				  int channel, int scale,
 				  u32 *div_numerator, u32 *div_denominator)
 {
-	u32 ratio = sc27xx_adc_get_ratio(channel, scale);
+	u32 ratio;
 
+	ratio = data->var_data->get_ratio(channel, scale);
 	*div_numerator = ratio >> SC27XX_RATIO_NUMERATOR_OFFSET;
 	*div_denominator = ratio & SC27XX_RATIO_DENOMINATOR_MASK;
 }
@@ -432,13 +476,13 @@ static int sc27xx_adc_enable(struct sc27xx_adc_data *data)
 {
 	int ret;
 
-	ret = regmap_update_bits(data->regmap, SC27XX_MODULE_EN,
+	ret = regmap_update_bits(data->regmap, data->var_data->module_en,
 				 SC27XX_MODULE_ADC_EN, SC27XX_MODULE_ADC_EN);
 	if (ret)
 		return ret;
 
 	/* Enable ADC work clock and controller clock */
-	ret = regmap_update_bits(data->regmap, SC27XX_ARM_CLK_EN,
+	ret = regmap_update_bits(data->regmap, data->var_data->clk_en,
 				 SC27XX_CLK_ADC_EN | SC27XX_CLK_ADC_CLK_EN,
 				 SC27XX_CLK_ADC_EN | SC27XX_CLK_ADC_CLK_EN);
 	if (ret)
@@ -456,10 +500,10 @@ static int sc27xx_adc_enable(struct sc27xx_adc_data *data)
 	return 0;
 
 disable_clk:
-	regmap_update_bits(data->regmap, SC27XX_ARM_CLK_EN,
+	regmap_update_bits(data->regmap, data->var_data->clk_en,
 			   SC27XX_CLK_ADC_EN | SC27XX_CLK_ADC_CLK_EN, 0);
 disable_adc:
-	regmap_update_bits(data->regmap, SC27XX_MODULE_EN,
+	regmap_update_bits(data->regmap, data->var_data->module_en,
 			   SC27XX_MODULE_ADC_EN, 0);
 
 	return ret;
@@ -470,21 +514,39 @@ static void sc27xx_adc_disable(void *_data)
 	struct sc27xx_adc_data *data = _data;
 
 	/* Disable ADC work clock and controller clock */
-	regmap_update_bits(data->regmap, SC27XX_ARM_CLK_EN,
+	regmap_update_bits(data->regmap, data->var_data->clk_en,
 			   SC27XX_CLK_ADC_EN | SC27XX_CLK_ADC_CLK_EN, 0);
 
-	regmap_update_bits(data->regmap, SC27XX_MODULE_EN,
+	regmap_update_bits(data->regmap, data->var_data->module_en,
 			   SC27XX_MODULE_ADC_EN, 0);
 }
 
+static const struct sc27xx_adc_variant_data sc2731_data = {
+	.module_en = SC2731_MODULE_EN,
+	.clk_en = SC2731_ARM_CLK_EN,
+	.scale_shift = SC27XX_ADC_SCALE_SHIFT,
+	.scale_mask = SC27XX_ADC_SCALE_MASK,
+	.bscale_cal = &sc2731_big_scale_graph_calib,
+	.sscale_cal = &sc2731_small_scale_graph_calib,
+	.init_scale = sc2731_adc_scale_init,
+	.get_ratio = sc2731_adc_get_ratio,
+};
+
 static int sc27xx_adc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
 	struct sc27xx_adc_data *sc27xx_data;
+	const struct sc27xx_adc_variant_data *pdata;
 	struct iio_dev *indio_dev;
 	int ret;
 
+	pdata = of_device_get_match_data(dev);
+	if (!pdata) {
+		dev_err(dev, "No matching driver data found\n");
+		return -EINVAL;
+	}
+
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*sc27xx_data));
 	if (!indio_dev)
 		return -ENOMEM;
@@ -520,6 +582,8 @@ static int sc27xx_adc_probe(struct platform_device *pdev)
 	}
 
 	sc27xx_data->dev = dev;
+	sc27xx_data->var_data = pdata;
+	sc27xx_data->var_data->init_scale(sc27xx_data);
 
 	ret = sc27xx_adc_enable(sc27xx_data);
 	if (ret) {
@@ -546,7 +610,7 @@ static int sc27xx_adc_probe(struct platform_device *pdev)
 }
 
 static const struct of_device_id sc27xx_adc_of_match[] = {
-	{ .compatible = "sprd,sc2731-adc", },
+	{ .compatible = "sprd,sc2731-adc", .data = &sc2731_data},
 	{ }
 };
 MODULE_DEVICE_TABLE(of, sc27xx_adc_of_match);
-- 
2.25.1

Re: [PATCH 3/7] iio: adc: sc27xx: structure adjuststment and optimization

[Baolin Wang]

On Thu, Jan 6, 2022 at 9:00 PM Cixi Geng <gengcixi@gmail.com> wrote:
>
> From: Cixi Geng <cixi.geng1@unisoc.com>
>
> Introduce one variant device data structure to be compatible
> with SC2731 PMIC since it has different scale and ratio calculation
> and so on.
>
> Signed-off-by: Yuming Zhu <yuming.zhu1@unisoc.com>
> Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com>
> ---
>  drivers/iio/adc/sc27xx_adc.c | 94 ++++++++++++++++++++++++++++++------
>  1 file changed, 79 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c
> index aee076c8e2b1..d2712e54ee79 100644
> --- a/drivers/iio/adc/sc27xx_adc.c
> +++ b/drivers/iio/adc/sc27xx_adc.c
> @@ -12,9 +12,9 @@
>  #include <linux/slab.h>
>
>  /* PMIC global registers definition */
> -#define SC27XX_MODULE_EN               0xc08
> +#define SC2731_MODULE_EN               0xc08
>  #define SC27XX_MODULE_ADC_EN           BIT(5)
> -#define SC27XX_ARM_CLK_EN              0xc10
> +#define SC2731_ARM_CLK_EN              0xc10
>  #define SC27XX_CLK_ADC_EN              BIT(5)
>  #define SC27XX_CLK_ADC_CLK_EN          BIT(6)
>
> @@ -78,6 +78,23 @@ struct sc27xx_adc_data {
>         int channel_scale[SC27XX_ADC_CHANNEL_MAX];
>         u32 base;
>         int irq;
> +       const struct sc27xx_adc_variant_data *var_data;
> +};
> +
> +/*
> + * Since different PMICs of SC27xx series can have different
> + * address and ratio, we should save ratio config and base
> + * in the device data structure.
> + */
> +struct sc27xx_adc_variant_data {
> +       u32 module_en;
> +       u32 clk_en;
> +       u32 scale_shift;
> +       u32 scale_mask;
> +       const struct sc27xx_adc_linear_graph *bscale_cal;
> +       const struct sc27xx_adc_linear_graph *sscale_cal;
> +       void (*init_scale)(struct sc27xx_adc_data *data);
> +       int (*get_ratio)(int channel, int scale);
>  };
>
>  struct sc27xx_adc_linear_graph {
> @@ -103,6 +120,16 @@ static struct sc27xx_adc_linear_graph small_scale_graph = {
>         100, 341,
>  };
>
> +static const struct sc27xx_adc_linear_graph sc2731_big_scale_graph_calib = {
> +       4200, 850,
> +       3600, 728,
> +};
> +
> +static const struct sc27xx_adc_linear_graph sc2731_small_scale_graph_calib = {
> +       1000, 838,
> +       100, 84,
> +};

The original big_scale_graph_calib and small_scale_graph_calib are for
SC2731 PMIC, why add new structure definition for SC2731?

> +
>  static const struct sc27xx_adc_linear_graph big_scale_graph_calib = {
>         4200, 856,
>         3600, 733,
> @@ -130,11 +157,11 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data,
>         size_t len;
>
>         if (big_scale) {
> -               calib_graph = &big_scale_graph_calib;
> +               calib_graph = data->var_data->bscale_cal;
>                 graph = &big_scale_graph;
>                 cell_name = "big_scale_calib";
>         } else {
> -               calib_graph = &small_scale_graph_calib;
> +               calib_graph = data->var_data->sscale_cal;
>                 graph = &small_scale_graph;
>                 cell_name = "small_scale_calib";
>         }
> @@ -160,7 +187,7 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data,
>         return 0;
>  }
>
> -static int sc27xx_adc_get_ratio(int channel, int scale)
> +static int sc2731_adc_get_ratio(int channel, int scale)
>  {
>         switch (channel) {
>         case 1:
> @@ -185,6 +212,21 @@ static int sc27xx_adc_get_ratio(int channel, int scale)
>         return SC27XX_VOLT_RATIO(1, 1);
>  }
>
> +/*
> + * According to the datasheet set specific value on some channel.
> + */
> +static void sc2731_adc_scale_init(struct sc27xx_adc_data *data)
> +{
> +       int i;
> +
> +       for (i = 0; i < SC27XX_ADC_CHANNEL_MAX; i++) {
> +               if (i == 5)
> +                       data->channel_scale[i] = 1;
> +               else
> +                       data->channel_scale[i] = 0;
> +       }
> +}

This is unnecessary I think, please see sc27xx_adc_write_raw() that
can set the channel scale.

> +
>  static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
>                            int scale, int *val)
>  {
> @@ -208,10 +250,11 @@ static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
>                 goto disable_adc;
>
>         /* Configure the channel id and scale */
> -       tmp = (scale << SC27XX_ADC_SCALE_SHIFT) & SC27XX_ADC_SCALE_MASK;
> +       tmp = (scale << data->var_data->scale_shift) & data->var_data->scale_mask;
>         tmp |= channel & SC27XX_ADC_CHN_ID_MASK;
>         ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CH_CFG,
> -                                SC27XX_ADC_CHN_ID_MASK | SC27XX_ADC_SCALE_MASK,
> +                                SC27XX_ADC_CHN_ID_MASK |
> +                                data->var_data->scale_mask,
>                                  tmp);
>         if (ret)
>                 goto disable_adc;
> @@ -262,8 +305,9 @@ static void sc27xx_adc_volt_ratio(struct sc27xx_adc_data *data,
>                                   int channel, int scale,
>                                   u32 *div_numerator, u32 *div_denominator)
>  {
> -       u32 ratio = sc27xx_adc_get_ratio(channel, scale);
> +       u32 ratio;
>
> +       ratio = data->var_data->get_ratio(channel, scale);
>         *div_numerator = ratio >> SC27XX_RATIO_NUMERATOR_OFFSET;
>         *div_denominator = ratio & SC27XX_RATIO_DENOMINATOR_MASK;
>  }
> @@ -432,13 +476,13 @@ static int sc27xx_adc_enable(struct sc27xx_adc_data *data)
>  {
>         int ret;
>
> -       ret = regmap_update_bits(data->regmap, SC27XX_MODULE_EN,
> +       ret = regmap_update_bits(data->regmap, data->var_data->module_en,
>                                  SC27XX_MODULE_ADC_EN, SC27XX_MODULE_ADC_EN);
>         if (ret)
>                 return ret;
>
>         /* Enable ADC work clock and controller clock */
> -       ret = regmap_update_bits(data->regmap, SC27XX_ARM_CLK_EN,
> +       ret = regmap_update_bits(data->regmap, data->var_data->clk_en,
>                                  SC27XX_CLK_ADC_EN | SC27XX_CLK_ADC_CLK_EN,
>                                  SC27XX_CLK_ADC_EN | SC27XX_CLK_ADC_CLK_EN);
>         if (ret)
> @@ -456,10 +500,10 @@ static int sc27xx_adc_enable(struct sc27xx_adc_data *data)
>         return 0;
>
>  disable_clk:
> -       regmap_update_bits(data->regmap, SC27XX_ARM_CLK_EN,
> +       regmap_update_bits(data->regmap, data->var_data->clk_en,
>                            SC27XX_CLK_ADC_EN | SC27XX_CLK_ADC_CLK_EN, 0);
>  disable_adc:
> -       regmap_update_bits(data->regmap, SC27XX_MODULE_EN,
> +       regmap_update_bits(data->regmap, data->var_data->module_en,
>                            SC27XX_MODULE_ADC_EN, 0);
>
>         return ret;
> @@ -470,21 +514,39 @@ static void sc27xx_adc_disable(void *_data)
>         struct sc27xx_adc_data *data = _data;
>
>         /* Disable ADC work clock and controller clock */
> -       regmap_update_bits(data->regmap, SC27XX_ARM_CLK_EN,
> +       regmap_update_bits(data->regmap, data->var_data->clk_en,
>                            SC27XX_CLK_ADC_EN | SC27XX_CLK_ADC_CLK_EN, 0);
>
> -       regmap_update_bits(data->regmap, SC27XX_MODULE_EN,
> +       regmap_update_bits(data->regmap, data->var_data->module_en,
>                            SC27XX_MODULE_ADC_EN, 0);
>  }
>
> +static const struct sc27xx_adc_variant_data sc2731_data = {
> +       .module_en = SC2731_MODULE_EN,
> +       .clk_en = SC2731_ARM_CLK_EN,
> +       .scale_shift = SC27XX_ADC_SCALE_SHIFT,
> +       .scale_mask = SC27XX_ADC_SCALE_MASK,
> +       .bscale_cal = &sc2731_big_scale_graph_calib,
> +       .sscale_cal = &sc2731_small_scale_graph_calib,
> +       .init_scale = sc2731_adc_scale_init,
> +       .get_ratio = sc2731_adc_get_ratio,
> +};
> +
>  static int sc27xx_adc_probe(struct platform_device *pdev)
>  {
>         struct device *dev = &pdev->dev;
>         struct device_node *np = dev->of_node;
>         struct sc27xx_adc_data *sc27xx_data;
> +       const struct sc27xx_adc_variant_data *pdata;
>         struct iio_dev *indio_dev;
>         int ret;
>
> +       pdata = of_device_get_match_data(dev);
> +       if (!pdata) {
> +               dev_err(dev, "No matching driver data found\n");
> +               return -EINVAL;
> +       }
> +
>         indio_dev = devm_iio_device_alloc(dev, sizeof(*sc27xx_data));
>         if (!indio_dev)
>                 return -ENOMEM;
> @@ -520,6 +582,8 @@ static int sc27xx_adc_probe(struct platform_device *pdev)
>         }
>
>         sc27xx_data->dev = dev;
> +       sc27xx_data->var_data = pdata;
> +       sc27xx_data->var_data->init_scale(sc27xx_data);
>
>         ret = sc27xx_adc_enable(sc27xx_data);
>         if (ret) {
> @@ -546,7 +610,7 @@ static int sc27xx_adc_probe(struct platform_device *pdev)
>  }
>
>  static const struct of_device_id sc27xx_adc_of_match[] = {
> -       { .compatible = "sprd,sc2731-adc", },
> +       { .compatible = "sprd,sc2731-adc", .data = &sc2731_data},
>         { }
>  };
>  MODULE_DEVICE_TABLE(of, sc27xx_adc_of_match);
> --
> 2.25.1
>


-- 
Baolin Wang

Re: [PATCH 3/7] iio: adc: sc27xx: structure adjuststment and optimization

[Cixi Geng]

Baolin Wang <baolin.wang7@gmail.com> 于2022年1月7日周五 15:03写道：
>
> On Thu, Jan 6, 2022 at 9:00 PM Cixi Geng <gengcixi@gmail.com> wrote:
> >
> > From: Cixi Geng <cixi.geng1@unisoc.com>
> >
> > Introduce one variant device data structure to be compatible
> > with SC2731 PMIC since it has different scale and ratio calculation
> > and so on.
> >
> > Signed-off-by: Yuming Zhu <yuming.zhu1@unisoc.com>
> > Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com>
> > ---
> >  drivers/iio/adc/sc27xx_adc.c | 94 ++++++++++++++++++++++++++++++------
> >  1 file changed, 79 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c
> > index aee076c8e2b1..d2712e54ee79 100644
> > --- a/drivers/iio/adc/sc27xx_adc.c
> > +++ b/drivers/iio/adc/sc27xx_adc.c
> > @@ -12,9 +12,9 @@
> >  #include <linux/slab.h>
> >
> >  /* PMIC global registers definition */
> > -#define SC27XX_MODULE_EN               0xc08
> > +#define SC2731_MODULE_EN               0xc08
> >  #define SC27XX_MODULE_ADC_EN           BIT(5)
> > -#define SC27XX_ARM_CLK_EN              0xc10
> > +#define SC2731_ARM_CLK_EN              0xc10
> >  #define SC27XX_CLK_ADC_EN              BIT(5)
> >  #define SC27XX_CLK_ADC_CLK_EN          BIT(6)
> >
> > @@ -78,6 +78,23 @@ struct sc27xx_adc_data {
> >         int channel_scale[SC27XX_ADC_CHANNEL_MAX];
> >         u32 base;
> >         int irq;
> > +       const struct sc27xx_adc_variant_data *var_data;
> > +};
> > +
> > +/*
> > + * Since different PMICs of SC27xx series can have different
> > + * address and ratio, we should save ratio config and base
> > + * in the device data structure.
> > + */
> > +struct sc27xx_adc_variant_data {
> > +       u32 module_en;
> > +       u32 clk_en;
> > +       u32 scale_shift;
> > +       u32 scale_mask;
> > +       const struct sc27xx_adc_linear_graph *bscale_cal;
> > +       const struct sc27xx_adc_linear_graph *sscale_cal;
> > +       void (*init_scale)(struct sc27xx_adc_data *data);
> > +       int (*get_ratio)(int channel, int scale);
> >  };
> >
> >  struct sc27xx_adc_linear_graph {
> > @@ -103,6 +120,16 @@ static struct sc27xx_adc_linear_graph small_scale_graph = {
> >         100, 341,
> >  };
> >
> > +static const struct sc27xx_adc_linear_graph sc2731_big_scale_graph_calib = {
> > +       4200, 850,
> > +       3600, 728,
> > +};
> > +
> > +static const struct sc27xx_adc_linear_graph sc2731_small_scale_graph_calib = {
> > +       1000, 838,
> > +       100, 84,
> > +};
>
> The original big_scale_graph_calib and small_scale_graph_calib are for
> SC2731 PMIC, why add new structure definition for SC2731?
>
> > +
> >  static const struct sc27xx_adc_linear_graph big_scale_graph_calib = {
> >         4200, 856,
> >         3600, 733,
> > @@ -130,11 +157,11 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data,
> >         size_t len;
> >
> >         if (big_scale) {
> > -               calib_graph = &big_scale_graph_calib;
> > +               calib_graph = data->var_data->bscale_cal;
> >                 graph = &big_scale_graph;
> >                 cell_name = "big_scale_calib";
> >         } else {
> > -               calib_graph = &small_scale_graph_calib;
> > +               calib_graph = data->var_data->sscale_cal;
> >                 graph = &small_scale_graph;
> >                 cell_name = "small_scale_calib";
> >         }
> > @@ -160,7 +187,7 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data,
> >         return 0;
> >  }
> >
> > -static int sc27xx_adc_get_ratio(int channel, int scale)
> > +static int sc2731_adc_get_ratio(int channel, int scale)
> >  {
> >         switch (channel) {
> >         case 1:
> > @@ -185,6 +212,21 @@ static int sc27xx_adc_get_ratio(int channel, int scale)
> >         return SC27XX_VOLT_RATIO(1, 1);
> >  }
> >
> > +/*
> > + * According to the datasheet set specific value on some channel.
> > + */
> > +static void sc2731_adc_scale_init(struct sc27xx_adc_data *data)
> > +{
> > +       int i;
> > +
> > +       for (i = 0; i < SC27XX_ADC_CHANNEL_MAX; i++) {
> > +               if (i == 5)
> > +                       data->channel_scale[i] = 1;
> > +               else
> > +                       data->channel_scale[i] = 0;
> > +       }
> > +}
>
> This is unnecessary I think, please see sc27xx_adc_write_raw() that
> can set the channel scale.
Did you mean that all the PMIC's scale_init function should put into
the sc27xx_adc_write_raw?
but the scale_init is all different by each PMIC, if implemented in
the write_raw, will add a lot of
if or switch_case branch
>
> > +
> >  static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
> >                            int scale, int *val)
> >  {
> > @@ -208,10 +250,11 @@ static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
> >                 goto disable_adc;
> >
> >         /* Configure the channel id and scale */
> > -       tmp = (scale << SC27XX_ADC_SCALE_SHIFT) & SC27XX_ADC_SCALE_MASK;
> > +       tmp = (scale << data->var_data->scale_shift) & data->var_data->scale_mask;
> >         tmp |= channel & SC27XX_ADC_CHN_ID_MASK;
> >         ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CH_CFG,
> > -                                SC27XX_ADC_CHN_ID_MASK | SC27XX_ADC_SCALE_MASK,
> > +                                SC27XX_ADC_CHN_ID_MASK |
> > +                                data->var_data->scale_mask,
> >                                  tmp);
> >         if (ret)
> >                 goto disable_adc;
> > @@ -262,8 +305,9 @@ static void sc27xx_adc_volt_ratio(struct sc27xx_adc_data *data,
> >                                   int channel, int scale,
> >                                   u32 *div_numerator, u32 *div_denominator)
> >  {
> > -       u32 ratio = sc27xx_adc_get_ratio(channel, scale);
> > +       u32 ratio;
> >
> > +       ratio = data->var_data->get_ratio(channel, scale);
> >         *div_numerator = ratio >> SC27XX_RATIO_NUMERATOR_OFFSET;
> >         *div_denominator = ratio & SC27XX_RATIO_DENOMINATOR_MASK;
> >  }
> > @@ -432,13 +476,13 @@ static int sc27xx_adc_enable(struct sc27xx_adc_data *data)
> >  {
> >         int ret;
> >
> > -       ret = regmap_update_bits(data->regmap, SC27XX_MODULE_EN,
> > +       ret = regmap_update_bits(data->regmap, data->var_data->module_en,
> >                                  SC27XX_MODULE_ADC_EN, SC27XX_MODULE_ADC_EN);
> >         if (ret)
> >                 return ret;
> >
> >         /* Enable ADC work clock and controller clock */
> > -       ret = regmap_update_bits(data->regmap, SC27XX_ARM_CLK_EN,
> > +       ret = regmap_update_bits(data->regmap, data->var_data->clk_en,
> >                                  SC27XX_CLK_ADC_EN | SC27XX_CLK_ADC_CLK_EN,
> >                                  SC27XX_CLK_ADC_EN | SC27XX_CLK_ADC_CLK_EN);
> >         if (ret)
> > @@ -456,10 +500,10 @@ static int sc27xx_adc_enable(struct sc27xx_adc_data *data)
> >         return 0;
> >
> >  disable_clk:
> > -       regmap_update_bits(data->regmap, SC27XX_ARM_CLK_EN,
> > +       regmap_update_bits(data->regmap, data->var_data->clk_en,
> >                            SC27XX_CLK_ADC_EN | SC27XX_CLK_ADC_CLK_EN, 0);
> >  disable_adc:
> > -       regmap_update_bits(data->regmap, SC27XX_MODULE_EN,
> > +       regmap_update_bits(data->regmap, data->var_data->module_en,
> >                            SC27XX_MODULE_ADC_EN, 0);
> >
> >         return ret;
> > @@ -470,21 +514,39 @@ static void sc27xx_adc_disable(void *_data)
> >         struct sc27xx_adc_data *data = _data;
> >
> >         /* Disable ADC work clock and controller clock */
> > -       regmap_update_bits(data->regmap, SC27XX_ARM_CLK_EN,
> > +       regmap_update_bits(data->regmap, data->var_data->clk_en,
> >                            SC27XX_CLK_ADC_EN | SC27XX_CLK_ADC_CLK_EN, 0);
> >
> > -       regmap_update_bits(data->regmap, SC27XX_MODULE_EN,
> > +       regmap_update_bits(data->regmap, data->var_data->module_en,
> >                            SC27XX_MODULE_ADC_EN, 0);
> >  }
> >
> > +static const struct sc27xx_adc_variant_data sc2731_data = {
> > +       .module_en = SC2731_MODULE_EN,
> > +       .clk_en = SC2731_ARM_CLK_EN,
> > +       .scale_shift = SC27XX_ADC_SCALE_SHIFT,
> > +       .scale_mask = SC27XX_ADC_SCALE_MASK,
> > +       .bscale_cal = &sc2731_big_scale_graph_calib,
> > +       .sscale_cal = &sc2731_small_scale_graph_calib,
> > +       .init_scale = sc2731_adc_scale_init,
> > +       .get_ratio = sc2731_adc_get_ratio,
> > +};
> > +
> >  static int sc27xx_adc_probe(struct platform_device *pdev)
> >  {
> >         struct device *dev = &pdev->dev;
> >         struct device_node *np = dev->of_node;
> >         struct sc27xx_adc_data *sc27xx_data;
> > +       const struct sc27xx_adc_variant_data *pdata;
> >         struct iio_dev *indio_dev;
> >         int ret;
> >
> > +       pdata = of_device_get_match_data(dev);
> > +       if (!pdata) {
> > +               dev_err(dev, "No matching driver data found\n");
> > +               return -EINVAL;
> > +       }
> > +
> >         indio_dev = devm_iio_device_alloc(dev, sizeof(*sc27xx_data));
> >         if (!indio_dev)
> >                 return -ENOMEM;
> > @@ -520,6 +582,8 @@ static int sc27xx_adc_probe(struct platform_device *pdev)
> >         }
> >
> >         sc27xx_data->dev = dev;
> > +       sc27xx_data->var_data = pdata;
> > +       sc27xx_data->var_data->init_scale(sc27xx_data);
> >
> >         ret = sc27xx_adc_enable(sc27xx_data);
> >         if (ret) {
> > @@ -546,7 +610,7 @@ static int sc27xx_adc_probe(struct platform_device *pdev)
> >  }
> >
> >  static const struct of_device_id sc27xx_adc_of_match[] = {
> > -       { .compatible = "sprd,sc2731-adc", },
> > +       { .compatible = "sprd,sc2731-adc", .data = &sc2731_data},
> >         { }
> >  };
> >  MODULE_DEVICE_TABLE(of, sc27xx_adc_of_match);
> > --
> > 2.25.1
> >
>
>
> --
> Baolin Wang

Re: [PATCH 3/7] iio: adc: sc27xx: structure adjuststment and optimization

[Baolin Wang]

On Thu, Jan 13, 2022 at 9:54 AM Cixi Geng <gengcixi@gmail.com> wrote:
>
> Baolin Wang <baolin.wang7@gmail.com> 于2022年1月7日周五 15:03写道：
> >
> > On Thu, Jan 6, 2022 at 9:00 PM Cixi Geng <gengcixi@gmail.com> wrote:
> > >
> > > From: Cixi Geng <cixi.geng1@unisoc.com>
> > >
> > > Introduce one variant device data structure to be compatible
> > > with SC2731 PMIC since it has different scale and ratio calculation
> > > and so on.
> > >
> > > Signed-off-by: Yuming Zhu <yuming.zhu1@unisoc.com>
> > > Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com>
> > > ---
> > >  drivers/iio/adc/sc27xx_adc.c | 94 ++++++++++++++++++++++++++++++------
> > >  1 file changed, 79 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c
> > > index aee076c8e2b1..d2712e54ee79 100644
> > > --- a/drivers/iio/adc/sc27xx_adc.c
> > > +++ b/drivers/iio/adc/sc27xx_adc.c
> > > @@ -12,9 +12,9 @@
> > >  #include <linux/slab.h>
> > >
> > >  /* PMIC global registers definition */
> > > -#define SC27XX_MODULE_EN               0xc08
> > > +#define SC2731_MODULE_EN               0xc08
> > >  #define SC27XX_MODULE_ADC_EN           BIT(5)
> > > -#define SC27XX_ARM_CLK_EN              0xc10
> > > +#define SC2731_ARM_CLK_EN              0xc10
> > >  #define SC27XX_CLK_ADC_EN              BIT(5)
> > >  #define SC27XX_CLK_ADC_CLK_EN          BIT(6)
> > >
> > > @@ -78,6 +78,23 @@ struct sc27xx_adc_data {
> > >         int channel_scale[SC27XX_ADC_CHANNEL_MAX];
> > >         u32 base;
> > >         int irq;
> > > +       const struct sc27xx_adc_variant_data *var_data;
> > > +};
> > > +
> > > +/*
> > > + * Since different PMICs of SC27xx series can have different
> > > + * address and ratio, we should save ratio config and base
> > > + * in the device data structure.
> > > + */
> > > +struct sc27xx_adc_variant_data {
> > > +       u32 module_en;
> > > +       u32 clk_en;
> > > +       u32 scale_shift;
> > > +       u32 scale_mask;
> > > +       const struct sc27xx_adc_linear_graph *bscale_cal;
> > > +       const struct sc27xx_adc_linear_graph *sscale_cal;
> > > +       void (*init_scale)(struct sc27xx_adc_data *data);
> > > +       int (*get_ratio)(int channel, int scale);
> > >  };
> > >
> > >  struct sc27xx_adc_linear_graph {
> > > @@ -103,6 +120,16 @@ static struct sc27xx_adc_linear_graph small_scale_graph = {
> > >         100, 341,
> > >  };
> > >
> > > +static const struct sc27xx_adc_linear_graph sc2731_big_scale_graph_calib = {
> > > +       4200, 850,
> > > +       3600, 728,
> > > +};
> > > +
> > > +static const struct sc27xx_adc_linear_graph sc2731_small_scale_graph_calib = {
> > > +       1000, 838,
> > > +       100, 84,
> > > +};
> >
> > The original big_scale_graph_calib and small_scale_graph_calib are for
> > SC2731 PMIC, why add new structure definition for SC2731?
> >
> > > +
> > >  static const struct sc27xx_adc_linear_graph big_scale_graph_calib = {
> > >         4200, 856,
> > >         3600, 733,
> > > @@ -130,11 +157,11 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data,
> > >         size_t len;
> > >
> > >         if (big_scale) {
> > > -               calib_graph = &big_scale_graph_calib;
> > > +               calib_graph = data->var_data->bscale_cal;
> > >                 graph = &big_scale_graph;
> > >                 cell_name = "big_scale_calib";
> > >         } else {
> > > -               calib_graph = &small_scale_graph_calib;
> > > +               calib_graph = data->var_data->sscale_cal;
> > >                 graph = &small_scale_graph;
> > >                 cell_name = "small_scale_calib";
> > >         }
> > > @@ -160,7 +187,7 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data,
> > >         return 0;
> > >  }
> > >
> > > -static int sc27xx_adc_get_ratio(int channel, int scale)
> > > +static int sc2731_adc_get_ratio(int channel, int scale)
> > >  {
> > >         switch (channel) {
> > >         case 1:
> > > @@ -185,6 +212,21 @@ static int sc27xx_adc_get_ratio(int channel, int scale)
> > >         return SC27XX_VOLT_RATIO(1, 1);
> > >  }
> > >
> > > +/*
> > > + * According to the datasheet set specific value on some channel.
> > > + */
> > > +static void sc2731_adc_scale_init(struct sc27xx_adc_data *data)
> > > +{
> > > +       int i;
> > > +
> > > +       for (i = 0; i < SC27XX_ADC_CHANNEL_MAX; i++) {
> > > +               if (i == 5)
> > > +                       data->channel_scale[i] = 1;
> > > +               else
> > > +                       data->channel_scale[i] = 0;
> > > +       }
> > > +}
> >
> > This is unnecessary I think, please see sc27xx_adc_write_raw() that
> > can set the channel scale.
> Did you mean that all the PMIC's scale_init function should put into
> the sc27xx_adc_write_raw?

No.

> but the scale_init is all different by each PMIC, if implemented in
> the write_raw, will add a lot of
> if or switch_case branch

What I mean is we should follow the original method to set the channel
scale by iio_info. Please also refer to other drivers how ot handle
the channel scale.

-- 
Baolin Wang

Re: [PATCH 3/7] iio: adc: sc27xx: structure adjuststment and optimization

[Cixi Geng]

Baolin Wang <baolin.wang7@gmail.com> 于2022年1月17日周一 14:15写道：
>
> On Thu, Jan 13, 2022 at 9:54 AM Cixi Geng <gengcixi@gmail.com> wrote:
> >
> > Baolin Wang <baolin.wang7@gmail.com> 于2022年1月7日周五 15:03写道：
> > >
> > > On Thu, Jan 6, 2022 at 9:00 PM Cixi Geng <gengcixi@gmail.com> wrote:
> > > >
> > > > From: Cixi Geng <cixi.geng1@unisoc.com>
> > > >
> > > > Introduce one variant device data structure to be compatible
> > > > with SC2731 PMIC since it has different scale and ratio calculation
> > > > and so on.
> > > >
> > > > Signed-off-by: Yuming Zhu <yuming.zhu1@unisoc.com>
> > > > Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com>
> > > > ---
> > > >  drivers/iio/adc/sc27xx_adc.c | 94 ++++++++++++++++++++++++++++++------
> > > >  1 file changed, 79 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c
> > > > index aee076c8e2b1..d2712e54ee79 100644
> > > > --- a/drivers/iio/adc/sc27xx_adc.c
> > > > +++ b/drivers/iio/adc/sc27xx_adc.c
> > > > @@ -12,9 +12,9 @@
> > > >  #include <linux/slab.h>
> > > >
> > > >  /* PMIC global registers definition */
> > > > -#define SC27XX_MODULE_EN               0xc08
> > > > +#define SC2731_MODULE_EN               0xc08
> > > >  #define SC27XX_MODULE_ADC_EN           BIT(5)
> > > > -#define SC27XX_ARM_CLK_EN              0xc10
> > > > +#define SC2731_ARM_CLK_EN              0xc10
> > > >  #define SC27XX_CLK_ADC_EN              BIT(5)
> > > >  #define SC27XX_CLK_ADC_CLK_EN          BIT(6)
> > > >
> > > > @@ -78,6 +78,23 @@ struct sc27xx_adc_data {
> > > >         int channel_scale[SC27XX_ADC_CHANNEL_MAX];
> > > >         u32 base;
> > > >         int irq;
> > > > +       const struct sc27xx_adc_variant_data *var_data;
> > > > +};
> > > > +
> > > > +/*
> > > > + * Since different PMICs of SC27xx series can have different
> > > > + * address and ratio, we should save ratio config and base
> > > > + * in the device data structure.
> > > > + */
> > > > +struct sc27xx_adc_variant_data {
> > > > +       u32 module_en;
> > > > +       u32 clk_en;
> > > > +       u32 scale_shift;
> > > > +       u32 scale_mask;
> > > > +       const struct sc27xx_adc_linear_graph *bscale_cal;
> > > > +       const struct sc27xx_adc_linear_graph *sscale_cal;
> > > > +       void (*init_scale)(struct sc27xx_adc_data *data);
> > > > +       int (*get_ratio)(int channel, int scale);
> > > >  };
> > > >
> > > >  struct sc27xx_adc_linear_graph {
> > > > @@ -103,6 +120,16 @@ static struct sc27xx_adc_linear_graph small_scale_graph = {
> > > >         100, 341,
> > > >  };
> > > >
> > > > +static const struct sc27xx_adc_linear_graph sc2731_big_scale_graph_calib = {
> > > > +       4200, 850,
> > > > +       3600, 728,
> > > > +};
> > > > +
> > > > +static const struct sc27xx_adc_linear_graph sc2731_small_scale_graph_calib = {
> > > > +       1000, 838,
> > > > +       100, 84,
> > > > +};
> > >
> > > The original big_scale_graph_calib and small_scale_graph_calib are for
> > > SC2731 PMIC, why add new structure definition for SC2731?
> > >
> > > > +
> > > >  static const struct sc27xx_adc_linear_graph big_scale_graph_calib = {
> > > >         4200, 856,
> > > >         3600, 733,
> > > > @@ -130,11 +157,11 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data,
> > > >         size_t len;
> > > >
> > > >         if (big_scale) {
> > > > -               calib_graph = &big_scale_graph_calib;
> > > > +               calib_graph = data->var_data->bscale_cal;
> > > >                 graph = &big_scale_graph;
> > > >                 cell_name = "big_scale_calib";
> > > >         } else {
> > > > -               calib_graph = &small_scale_graph_calib;
> > > > +               calib_graph = data->var_data->sscale_cal;
> > > >                 graph = &small_scale_graph;
> > > >                 cell_name = "small_scale_calib";
> > > >         }
> > > > @@ -160,7 +187,7 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data,
> > > >         return 0;
> > > >  }
> > > >
> > > > -static int sc27xx_adc_get_ratio(int channel, int scale)
> > > > +static int sc2731_adc_get_ratio(int channel, int scale)
> > > >  {
> > > >         switch (channel) {
> > > >         case 1:
> > > > @@ -185,6 +212,21 @@ static int sc27xx_adc_get_ratio(int channel, int scale)
> > > >         return SC27XX_VOLT_RATIO(1, 1);
> > > >  }
> > > >
> > > > +/*
> > > > + * According to the datasheet set specific value on some channel.
> > > > + */
> > > > +static void sc2731_adc_scale_init(struct sc27xx_adc_data *data)
> > > > +{
> > > > +       int i;
> > > > +
> > > > +       for (i = 0; i < SC27XX_ADC_CHANNEL_MAX; i++) {
> > > > +               if (i == 5)
> > > > +                       data->channel_scale[i] = 1;
> > > > +               else
> > > > +                       data->channel_scale[i] = 0;
> > > > +       }
> > > > +}
> > >
> > > This is unnecessary I think, please see sc27xx_adc_write_raw() that
> > > can set the channel scale.
> > Did you mean that all the PMIC's scale_init function should put into
> > the sc27xx_adc_write_raw?
>
> No.
>
> > but the scale_init is all different by each PMIC, if implemented in
> > the write_raw, will add a lot of
> > if or switch_case branch
>
> What I mean is we should follow the original method to set the channel
> scale by iio_info. Please also refer to other drivers how ot handle
> the channel scale.
Hi Baolin,  I understand the adc_write_raw() function is the method to set
channal scale for the userspace, we can change the channel scale by write
a value on a user code. did i understand right?
out  scale_init is to set scale value when the driver probe stage, and I also
did not found other adc driver use the adc_write_raw() during the driver
 initialization phase.
>
> --
> Baolin Wang

Re: [PATCH 3/7] iio: adc: sc27xx: structure adjuststment and optimization

[Baolin Wang]

On Mon, Jan 24, 2022 at 4:07 PM Cixi Geng <gengcixi@gmail.com> wrote:
>
> Baolin Wang <baolin.wang7@gmail.com> 于2022年1月17日周一 14:15写道：
> >
> > On Thu, Jan 13, 2022 at 9:54 AM Cixi Geng <gengcixi@gmail.com> wrote:
> > >
> > > Baolin Wang <baolin.wang7@gmail.com> 于2022年1月7日周五 15:03写道：
> > > >
> > > > On Thu, Jan 6, 2022 at 9:00 PM Cixi Geng <gengcixi@gmail.com> wrote:
> > > > >
> > > > > From: Cixi Geng <cixi.geng1@unisoc.com>
> > > > >
> > > > > Introduce one variant device data structure to be compatible
> > > > > with SC2731 PMIC since it has different scale and ratio calculation
> > > > > and so on.
> > > > >
> > > > > Signed-off-by: Yuming Zhu <yuming.zhu1@unisoc.com>
> > > > > Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com>
> > > > > ---
> > > > >  drivers/iio/adc/sc27xx_adc.c | 94 ++++++++++++++++++++++++++++++------
> > > > >  1 file changed, 79 insertions(+), 15 deletions(-)
> > > > >
> > > > > diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c
> > > > > index aee076c8e2b1..d2712e54ee79 100644
> > > > > --- a/drivers/iio/adc/sc27xx_adc.c
> > > > > +++ b/drivers/iio/adc/sc27xx_adc.c
> > > > > @@ -12,9 +12,9 @@
> > > > >  #include <linux/slab.h>
> > > > >
> > > > >  /* PMIC global registers definition */
> > > > > -#define SC27XX_MODULE_EN               0xc08
> > > > > +#define SC2731_MODULE_EN               0xc08
> > > > >  #define SC27XX_MODULE_ADC_EN           BIT(5)
> > > > > -#define SC27XX_ARM_CLK_EN              0xc10
> > > > > +#define SC2731_ARM_CLK_EN              0xc10
> > > > >  #define SC27XX_CLK_ADC_EN              BIT(5)
> > > > >  #define SC27XX_CLK_ADC_CLK_EN          BIT(6)
> > > > >
> > > > > @@ -78,6 +78,23 @@ struct sc27xx_adc_data {
> > > > >         int channel_scale[SC27XX_ADC_CHANNEL_MAX];
> > > > >         u32 base;
> > > > >         int irq;
> > > > > +       const struct sc27xx_adc_variant_data *var_data;
> > > > > +};
> > > > > +
> > > > > +/*
> > > > > + * Since different PMICs of SC27xx series can have different
> > > > > + * address and ratio, we should save ratio config and base
> > > > > + * in the device data structure.
> > > > > + */
> > > > > +struct sc27xx_adc_variant_data {
> > > > > +       u32 module_en;
> > > > > +       u32 clk_en;
> > > > > +       u32 scale_shift;
> > > > > +       u32 scale_mask;
> > > > > +       const struct sc27xx_adc_linear_graph *bscale_cal;
> > > > > +       const struct sc27xx_adc_linear_graph *sscale_cal;
> > > > > +       void (*init_scale)(struct sc27xx_adc_data *data);
> > > > > +       int (*get_ratio)(int channel, int scale);
> > > > >  };
> > > > >
> > > > >  struct sc27xx_adc_linear_graph {
> > > > > @@ -103,6 +120,16 @@ static struct sc27xx_adc_linear_graph small_scale_graph = {
> > > > >         100, 341,
> > > > >  };
> > > > >
> > > > > +static const struct sc27xx_adc_linear_graph sc2731_big_scale_graph_calib = {
> > > > > +       4200, 850,
> > > > > +       3600, 728,
> > > > > +};
> > > > > +
> > > > > +static const struct sc27xx_adc_linear_graph sc2731_small_scale_graph_calib = {
> > > > > +       1000, 838,
> > > > > +       100, 84,
> > > > > +};
> > > >
> > > > The original big_scale_graph_calib and small_scale_graph_calib are for
> > > > SC2731 PMIC, why add new structure definition for SC2731?
> > > >
> > > > > +
> > > > >  static const struct sc27xx_adc_linear_graph big_scale_graph_calib = {
> > > > >         4200, 856,
> > > > >         3600, 733,
> > > > > @@ -130,11 +157,11 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data,
> > > > >         size_t len;
> > > > >
> > > > >         if (big_scale) {
> > > > > -               calib_graph = &big_scale_graph_calib;
> > > > > +               calib_graph = data->var_data->bscale_cal;
> > > > >                 graph = &big_scale_graph;
> > > > >                 cell_name = "big_scale_calib";
> > > > >         } else {
> > > > > -               calib_graph = &small_scale_graph_calib;
> > > > > +               calib_graph = data->var_data->sscale_cal;
> > > > >                 graph = &small_scale_graph;
> > > > >                 cell_name = "small_scale_calib";
> > > > >         }
> > > > > @@ -160,7 +187,7 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data,
> > > > >         return 0;
> > > > >  }
> > > > >
> > > > > -static int sc27xx_adc_get_ratio(int channel, int scale)
> > > > > +static int sc2731_adc_get_ratio(int channel, int scale)
> > > > >  {
> > > > >         switch (channel) {
> > > > >         case 1:
> > > > > @@ -185,6 +212,21 @@ static int sc27xx_adc_get_ratio(int channel, int scale)
> > > > >         return SC27XX_VOLT_RATIO(1, 1);
> > > > >  }
> > > > >
> > > > > +/*
> > > > > + * According to the datasheet set specific value on some channel.
> > > > > + */
> > > > > +static void sc2731_adc_scale_init(struct sc27xx_adc_data *data)
> > > > > +{
> > > > > +       int i;
> > > > > +
> > > > > +       for (i = 0; i < SC27XX_ADC_CHANNEL_MAX; i++) {
> > > > > +               if (i == 5)
> > > > > +                       data->channel_scale[i] = 1;
> > > > > +               else
> > > > > +                       data->channel_scale[i] = 0;
> > > > > +       }
> > > > > +}
> > > >
> > > > This is unnecessary I think, please see sc27xx_adc_write_raw() that
> > > > can set the channel scale.
> > > Did you mean that all the PMIC's scale_init function should put into
> > > the sc27xx_adc_write_raw?
> >
> > No.
> >
> > > but the scale_init is all different by each PMIC, if implemented in
> > > the write_raw, will add a lot of
> > > if or switch_case branch
> >
> > What I mean is we should follow the original method to set the channel
> > scale by iio_info. Please also refer to other drivers how ot handle
> > the channel scale.
> Hi Baolin,  I understand the adc_write_raw() function is the method to set
> channal scale for the userspace, we can change the channel scale by write
> a value on a user code. did i understand right?
> out  scale_init is to set scale value when the driver probe stage, and I also
> did not found other adc driver use the adc_write_raw() during the driver
>  initialization phase.

Hi Jonathan,

How do you think about the method in this patch to set the channel
scale? Thanks.

-- 
Baolin Wang

Re: [PATCH 3/7] iio: adc: sc27xx: structure adjuststment and optimization

[Cixi Geng]

Baolin Wang <baolin.wang7@gmail.com> 于2022年2月10日周四 16:07写道：
>
> On Mon, Jan 24, 2022 at 4:07 PM Cixi Geng <gengcixi@gmail.com> wrote:
> >
> > Baolin Wang <baolin.wang7@gmail.com> 于2022年1月17日周一 14:15写道：
> > >
> > > On Thu, Jan 13, 2022 at 9:54 AM Cixi Geng <gengcixi@gmail.com> wrote:
> > > >
> > > > Baolin Wang <baolin.wang7@gmail.com> 于2022年1月7日周五 15:03写道：
> > > > >
> > > > > On Thu, Jan 6, 2022 at 9:00 PM Cixi Geng <gengcixi@gmail.com> wrote:
> > > > > >
> > > > > > From: Cixi Geng <cixi.geng1@unisoc.com>
> > > > > >
> > > > > > Introduce one variant device data structure to be compatible
> > > > > > with SC2731 PMIC since it has different scale and ratio calculation
> > > > > > and so on.
> > > > > >
> > > > > > Signed-off-by: Yuming Zhu <yuming.zhu1@unisoc.com>
> > > > > > Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com>
> > > > > > ---
> > > > > >  drivers/iio/adc/sc27xx_adc.c | 94 ++++++++++++++++++++++++++++++------
> > > > > >  1 file changed, 79 insertions(+), 15 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c
> > > > > > index aee076c8e2b1..d2712e54ee79 100644
> > > > > > --- a/drivers/iio/adc/sc27xx_adc.c
> > > > > > +++ b/drivers/iio/adc/sc27xx_adc.c
> > > > > > @@ -12,9 +12,9 @@
> > > > > >  #include <linux/slab.h>
> > > > > >
> > > > > >  /* PMIC global registers definition */
> > > > > > -#define SC27XX_MODULE_EN               0xc08
> > > > > > +#define SC2731_MODULE_EN               0xc08
> > > > > >  #define SC27XX_MODULE_ADC_EN           BIT(5)
> > > > > > -#define SC27XX_ARM_CLK_EN              0xc10
> > > > > > +#define SC2731_ARM_CLK_EN              0xc10
> > > > > >  #define SC27XX_CLK_ADC_EN              BIT(5)
> > > > > >  #define SC27XX_CLK_ADC_CLK_EN          BIT(6)
> > > > > >
> > > > > > @@ -78,6 +78,23 @@ struct sc27xx_adc_data {
> > > > > >         int channel_scale[SC27XX_ADC_CHANNEL_MAX];
> > > > > >         u32 base;
> > > > > >         int irq;
> > > > > > +       const struct sc27xx_adc_variant_data *var_data;
> > > > > > +};
> > > > > > +
> > > > > > +/*
> > > > > > + * Since different PMICs of SC27xx series can have different
> > > > > > + * address and ratio, we should save ratio config and base
> > > > > > + * in the device data structure.
> > > > > > + */
> > > > > > +struct sc27xx_adc_variant_data {
> > > > > > +       u32 module_en;
> > > > > > +       u32 clk_en;
> > > > > > +       u32 scale_shift;
> > > > > > +       u32 scale_mask;
> > > > > > +       const struct sc27xx_adc_linear_graph *bscale_cal;
> > > > > > +       const struct sc27xx_adc_linear_graph *sscale_cal;
> > > > > > +       void (*init_scale)(struct sc27xx_adc_data *data);
> > > > > > +       int (*get_ratio)(int channel, int scale);
> > > > > >  };
> > > > > >
> > > > > >  struct sc27xx_adc_linear_graph {
> > > > > > @@ -103,6 +120,16 @@ static struct sc27xx_adc_linear_graph small_scale_graph = {
> > > > > >         100, 341,
> > > > > >  };
> > > > > >
> > > > > > +static const struct sc27xx_adc_linear_graph sc2731_big_scale_graph_calib = {
> > > > > > +       4200, 850,
> > > > > > +       3600, 728,
> > > > > > +};
> > > > > > +
> > > > > > +static const struct sc27xx_adc_linear_graph sc2731_small_scale_graph_calib = {
> > > > > > +       1000, 838,
> > > > > > +       100, 84,
> > > > > > +};
> > > > >
> > > > > The original big_scale_graph_calib and small_scale_graph_calib are for
> > > > > SC2731 PMIC, why add new structure definition for SC2731?
> > > > >
> > > > > > +
> > > > > >  static const struct sc27xx_adc_linear_graph big_scale_graph_calib = {
> > > > > >         4200, 856,
> > > > > >         3600, 733,
> > > > > > @@ -130,11 +157,11 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data,
> > > > > >         size_t len;
> > > > > >
> > > > > >         if (big_scale) {
> > > > > > -               calib_graph = &big_scale_graph_calib;
> > > > > > +               calib_graph = data->var_data->bscale_cal;
> > > > > >                 graph = &big_scale_graph;
> > > > > >                 cell_name = "big_scale_calib";
> > > > > >         } else {
> > > > > > -               calib_graph = &small_scale_graph_calib;
> > > > > > +               calib_graph = data->var_data->sscale_cal;
> > > > > >                 graph = &small_scale_graph;
> > > > > >                 cell_name = "small_scale_calib";
> > > > > >         }
> > > > > > @@ -160,7 +187,7 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data,
> > > > > >         return 0;
> > > > > >  }
> > > > > >
> > > > > > -static int sc27xx_adc_get_ratio(int channel, int scale)
> > > > > > +static int sc2731_adc_get_ratio(int channel, int scale)
> > > > > >  {
> > > > > >         switch (channel) {
> > > > > >         case 1:
> > > > > > @@ -185,6 +212,21 @@ static int sc27xx_adc_get_ratio(int channel, int scale)
> > > > > >         return SC27XX_VOLT_RATIO(1, 1);
> > > > > >  }
> > > > > >
> > > > > > +/*
> > > > > > + * According to the datasheet set specific value on some channel.
> > > > > > + */
> > > > > > +static void sc2731_adc_scale_init(struct sc27xx_adc_data *data)
> > > > > > +{
> > > > > > +       int i;
> > > > > > +
> > > > > > +       for (i = 0; i < SC27XX_ADC_CHANNEL_MAX; i++) {
> > > > > > +               if (i == 5)
> > > > > > +                       data->channel_scale[i] = 1;
> > > > > > +               else
> > > > > > +                       data->channel_scale[i] = 0;
> > > > > > +       }
> > > > > > +}
> > > > >
> > > > > This is unnecessary I think, please see sc27xx_adc_write_raw() that
> > > > > can set the channel scale.
> > > > Did you mean that all the PMIC's scale_init function should put into
> > > > the sc27xx_adc_write_raw?
> > >
> > > No.
> > >
> > > > but the scale_init is all different by each PMIC, if implemented in
> > > > the write_raw, will add a lot of
> > > > if or switch_case branch
> > >
> > > What I mean is we should follow the original method to set the channel
> > > scale by iio_info. Please also refer to other drivers how ot handle
> > > the channel scale.
> > Hi Baolin,  I understand the adc_write_raw() function is the method to set
> > channal scale for the userspace, we can change the channel scale by write
> > a value on a user code. did i understand right?
> > out  scale_init is to set scale value when the driver probe stage, and I also
> > did not found other adc driver use the adc_write_raw() during the driver
> >  initialization phase.
>
> Hi Jonathan,
>
> How do you think about the method in this patch to set the channel
> scale? Thanks.
>
Hi Jonathan,
Could you have a loot at this patch ,and give some advice about the
method to set the channel scale? Thanks very much.
> --
> Baolin Wang

Re: [PATCH 3/7] iio: adc: sc27xx: structure adjuststment and optimization

[Jonathan Cameron]

On Wed, 23 Feb 2022 20:46:08 +0800
Cixi Geng <gengcixi@gmail.com> wrote:

> Baolin Wang <baolin.wang7@gmail.com> 于2022年2月10日周四 16:07写道：
> >
> > On Mon, Jan 24, 2022 at 4:07 PM Cixi Geng <gengcixi@gmail.com> wrote:  
> > >
> > > Baolin Wang <baolin.wang7@gmail.com> 于2022年1月17日周一 14:15写道：  
> > > >
> > > > On Thu, Jan 13, 2022 at 9:54 AM Cixi Geng <gengcixi@gmail.com> wrote:  
> > > > >
> > > > > Baolin Wang <baolin.wang7@gmail.com> 于2022年1月7日周五 15:03写道：  
> > > > > >
> > > > > > On Thu, Jan 6, 2022 at 9:00 PM Cixi Geng <gengcixi@gmail.com> wrote:  
> > > > > > >
> > > > > > > From: Cixi Geng <cixi.geng1@unisoc.com>
> > > > > > >
> > > > > > > Introduce one variant device data structure to be compatible
> > > > > > > with SC2731 PMIC since it has different scale and ratio calculation
> > > > > > > and so on.
> > > > > > >
> > > > > > > Signed-off-by: Yuming Zhu <yuming.zhu1@unisoc.com>
> > > > > > > Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com>
> > > > > > > ---
> > > > > > >  drivers/iio/adc/sc27xx_adc.c | 94 ++++++++++++++++++++++++++++++------
> > > > > > >  1 file changed, 79 insertions(+), 15 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c
> > > > > > > index aee076c8e2b1..d2712e54ee79 100644
> > > > > > > --- a/drivers/iio/adc/sc27xx_adc.c
> > > > > > > +++ b/drivers/iio/adc/sc27xx_adc.c
> > > > > > > @@ -12,9 +12,9 @@
> > > > > > >  #include <linux/slab.h>
> > > > > > >
> > > > > > >  /* PMIC global registers definition */
> > > > > > > -#define SC27XX_MODULE_EN               0xc08
> > > > > > > +#define SC2731_MODULE_EN               0xc08
> > > > > > >  #define SC27XX_MODULE_ADC_EN           BIT(5)
> > > > > > > -#define SC27XX_ARM_CLK_EN              0xc10
> > > > > > > +#define SC2731_ARM_CLK_EN              0xc10
> > > > > > >  #define SC27XX_CLK_ADC_EN              BIT(5)
> > > > > > >  #define SC27XX_CLK_ADC_CLK_EN          BIT(6)
> > > > > > >
> > > > > > > @@ -78,6 +78,23 @@ struct sc27xx_adc_data {
> > > > > > >         int channel_scale[SC27XX_ADC_CHANNEL_MAX];
> > > > > > >         u32 base;
> > > > > > >         int irq;
> > > > > > > +       const struct sc27xx_adc_variant_data *var_data;
> > > > > > > +};
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * Since different PMICs of SC27xx series can have different
> > > > > > > + * address and ratio, we should save ratio config and base
> > > > > > > + * in the device data structure.
> > > > > > > + */
> > > > > > > +struct sc27xx_adc_variant_data {
> > > > > > > +       u32 module_en;
> > > > > > > +       u32 clk_en;
> > > > > > > +       u32 scale_shift;
> > > > > > > +       u32 scale_mask;
> > > > > > > +       const struct sc27xx_adc_linear_graph *bscale_cal;
> > > > > > > +       const struct sc27xx_adc_linear_graph *sscale_cal;
> > > > > > > +       void (*init_scale)(struct sc27xx_adc_data *data);
> > > > > > > +       int (*get_ratio)(int channel, int scale);
> > > > > > >  };
> > > > > > >
> > > > > > >  struct sc27xx_adc_linear_graph {
> > > > > > > @@ -103,6 +120,16 @@ static struct sc27xx_adc_linear_graph small_scale_graph = {
> > > > > > >         100, 341,
> > > > > > >  };
> > > > > > >
> > > > > > > +static const struct sc27xx_adc_linear_graph sc2731_big_scale_graph_calib = {
> > > > > > > +       4200, 850,
> > > > > > > +       3600, 728,
> > > > > > > +};
> > > > > > > +
> > > > > > > +static const struct sc27xx_adc_linear_graph sc2731_small_scale_graph_calib = {
> > > > > > > +       1000, 838,
> > > > > > > +       100, 84,
> > > > > > > +};  
> > > > > >
> > > > > > The original big_scale_graph_calib and small_scale_graph_calib are for
> > > > > > SC2731 PMIC, why add new structure definition for SC2731?
> > > > > >  
> > > > > > > +
> > > > > > >  static const struct sc27xx_adc_linear_graph big_scale_graph_calib = {
> > > > > > >         4200, 856,
> > > > > > >         3600, 733,
> > > > > > > @@ -130,11 +157,11 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data,
> > > > > > >         size_t len;
> > > > > > >
> > > > > > >         if (big_scale) {
> > > > > > > -               calib_graph = &big_scale_graph_calib;
> > > > > > > +               calib_graph = data->var_data->bscale_cal;
> > > > > > >                 graph = &big_scale_graph;
> > > > > > >                 cell_name = "big_scale_calib";
> > > > > > >         } else {
> > > > > > > -               calib_graph = &small_scale_graph_calib;
> > > > > > > +               calib_graph = data->var_data->sscale_cal;
> > > > > > >                 graph = &small_scale_graph;
> > > > > > >                 cell_name = "small_scale_calib";
> > > > > > >         }
> > > > > > > @@ -160,7 +187,7 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data,
> > > > > > >         return 0;
> > > > > > >  }
> > > > > > >
> > > > > > > -static int sc27xx_adc_get_ratio(int channel, int scale)
> > > > > > > +static int sc2731_adc_get_ratio(int channel, int scale)
> > > > > > >  {
> > > > > > >         switch (channel) {
> > > > > > >         case 1:
> > > > > > > @@ -185,6 +212,21 @@ static int sc27xx_adc_get_ratio(int channel, int scale)
> > > > > > >         return SC27XX_VOLT_RATIO(1, 1);
> > > > > > >  }
> > > > > > >
> > > > > > > +/*
> > > > > > > + * According to the datasheet set specific value on some channel.
> > > > > > > + */
> > > > > > > +static void sc2731_adc_scale_init(struct sc27xx_adc_data *data)
> > > > > > > +{
> > > > > > > +       int i;
> > > > > > > +
> > > > > > > +       for (i = 0; i < SC27XX_ADC_CHANNEL_MAX; i++) {
> > > > > > > +               if (i == 5)
> > > > > > > +                       data->channel_scale[i] = 1;
> > > > > > > +               else
> > > > > > > +                       data->channel_scale[i] = 0;
> > > > > > > +       }
> > > > > > > +}  
> > > > > >
> > > > > > This is unnecessary I think, please see sc27xx_adc_write_raw() that
> > > > > > can set the channel scale.  
> > > > > Did you mean that all the PMIC's scale_init function should put into
> > > > > the sc27xx_adc_write_raw?  
> > > >
> > > > No.
> > > >  
> > > > > but the scale_init is all different by each PMIC, if implemented in
> > > > > the write_raw, will add a lot of
> > > > > if or switch_case branch  
> > > >
> > > > What I mean is we should follow the original method to set the channel
> > > > scale by iio_info. Please also refer to other drivers how ot handle
> > > > the channel scale.  
> > > Hi Baolin,  I understand the adc_write_raw() function is the method to set
> > > channal scale for the userspace, we can change the channel scale by write
> > > a value on a user code. did i understand right?
> > > out  scale_init is to set scale value when the driver probe stage, and I also
> > > did not found other adc driver use the adc_write_raw() during the driver
> > >  initialization phase.  
> >
> > Hi Jonathan,
> >
> > How do you think about the method in this patch to set the channel
> > scale? Thanks.
> >  
> Hi Jonathan,
> Could you have a loot at this patch ,and give some advice about the
> method to set the channel scale? Thanks very much.

Hi, thanks for poking me on this - I'd missed the question buried deep in the thread!

Anyhow, I don't quite follow the discussion but think it could be focused
on one of 2 questions...

1) Does setting an initial default make sense?  
   Yes, this is an acceptable thing to do if there is a particular set of defaults
   and there is no risk of regressions (i.e. the device wasn't previously supported
   with different defaults).
2) Should you use the write_raw callback to actually do the setting?
   Probably not as it has a set of parameters that don't make as much sense from within
   the driver.  It 'might' make sense to have a common _set() function for this
   feature which is called both in this initialization case and from the write_raw()
   function however as that could do bounds checking etc in one common place.
   However, it is very simple here, so perhaps not necessary.

Jonathan

> > --
> > Baolin Wang  

Re: [PATCH 3/7] iio: adc: sc27xx: structure adjuststment and optimization

[Cixi Geng]

Jonathan Cameron <Jonathan.Cameron@huawei.com> 于2022年2月25日周五 18:19写道：
>
> On Wed, 23 Feb 2022 20:46:08 +0800
> Cixi Geng <gengcixi@gmail.com> wrote:
>
> > Baolin Wang <baolin.wang7@gmail.com> 于2022年2月10日周四 16:07写道：
> > >
> > > On Mon, Jan 24, 2022 at 4:07 PM Cixi Geng <gengcixi@gmail.com> wrote:
> > > >
> > > > Baolin Wang <baolin.wang7@gmail.com> 于2022年1月17日周一 14:15写道：
> > > > >
> > > > > On Thu, Jan 13, 2022 at 9:54 AM Cixi Geng <gengcixi@gmail.com> wrote:
> > > > > >
> > > > > > Baolin Wang <baolin.wang7@gmail.com> 于2022年1月7日周五 15:03写道：
> > > > > > >
> > > > > > > On Thu, Jan 6, 2022 at 9:00 PM Cixi Geng <gengcixi@gmail.com> wrote:
> > > > > > > >
> > > > > > > > From: Cixi Geng <cixi.geng1@unisoc.com>
> > > > > > > >
> > > > > > > > Introduce one variant device data structure to be compatible
> > > > > > > > with SC2731 PMIC since it has different scale and ratio calculation
> > > > > > > > and so on.
> > > > > > > >
> > > > > > > > Signed-off-by: Yuming Zhu <yuming.zhu1@unisoc.com>
> > > > > > > > Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com>
> > > > > > > > ---
> > > > > > > >  drivers/iio/adc/sc27xx_adc.c | 94 ++++++++++++++++++++++++++++++------
> > > > > > > >  1 file changed, 79 insertions(+), 15 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c
> > > > > > > > index aee076c8e2b1..d2712e54ee79 100644
> > > > > > > > --- a/drivers/iio/adc/sc27xx_adc.c
> > > > > > > > +++ b/drivers/iio/adc/sc27xx_adc.c
> > > > > > > > @@ -12,9 +12,9 @@
> > > > > > > >  #include <linux/slab.h>
> > > > > > > >
> > > > > > > >  /* PMIC global registers definition */
> > > > > > > > -#define SC27XX_MODULE_EN               0xc08
> > > > > > > > +#define SC2731_MODULE_EN               0xc08
> > > > > > > >  #define SC27XX_MODULE_ADC_EN           BIT(5)
> > > > > > > > -#define SC27XX_ARM_CLK_EN              0xc10
> > > > > > > > +#define SC2731_ARM_CLK_EN              0xc10
> > > > > > > >  #define SC27XX_CLK_ADC_EN              BIT(5)
> > > > > > > >  #define SC27XX_CLK_ADC_CLK_EN          BIT(6)
> > > > > > > >
> > > > > > > > @@ -78,6 +78,23 @@ struct sc27xx_adc_data {
> > > > > > > >         int channel_scale[SC27XX_ADC_CHANNEL_MAX];
> > > > > > > >         u32 base;
> > > > > > > >         int irq;
> > > > > > > > +       const struct sc27xx_adc_variant_data *var_data;
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +/*
> > > > > > > > + * Since different PMICs of SC27xx series can have different
> > > > > > > > + * address and ratio, we should save ratio config and base
> > > > > > > > + * in the device data structure.
> > > > > > > > + */
> > > > > > > > +struct sc27xx_adc_variant_data {
> > > > > > > > +       u32 module_en;
> > > > > > > > +       u32 clk_en;
> > > > > > > > +       u32 scale_shift;
> > > > > > > > +       u32 scale_mask;
> > > > > > > > +       const struct sc27xx_adc_linear_graph *bscale_cal;
> > > > > > > > +       const struct sc27xx_adc_linear_graph *sscale_cal;
> > > > > > > > +       void (*init_scale)(struct sc27xx_adc_data *data);
> > > > > > > > +       int (*get_ratio)(int channel, int scale);
> > > > > > > >  };
> > > > > > > >
> > > > > > > >  struct sc27xx_adc_linear_graph {
> > > > > > > > @@ -103,6 +120,16 @@ static struct sc27xx_adc_linear_graph small_scale_graph = {
> > > > > > > >         100, 341,
> > > > > > > >  };
> > > > > > > >
> > > > > > > > +static const struct sc27xx_adc_linear_graph sc2731_big_scale_graph_calib = {
> > > > > > > > +       4200, 850,
> > > > > > > > +       3600, 728,
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +static const struct sc27xx_adc_linear_graph sc2731_small_scale_graph_calib = {
> > > > > > > > +       1000, 838,
> > > > > > > > +       100, 84,
> > > > > > > > +};
> > > > > > >
> > > > > > > The original big_scale_graph_calib and small_scale_graph_calib are for
> > > > > > > SC2731 PMIC, why add new structure definition for SC2731?
> > > > > > >
> > > > > > > > +
> > > > > > > >  static const struct sc27xx_adc_linear_graph big_scale_graph_calib = {
> > > > > > > >         4200, 856,
> > > > > > > >         3600, 733,
> > > > > > > > @@ -130,11 +157,11 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data,
> > > > > > > >         size_t len;
> > > > > > > >
> > > > > > > >         if (big_scale) {
> > > > > > > > -               calib_graph = &big_scale_graph_calib;
> > > > > > > > +               calib_graph = data->var_data->bscale_cal;
> > > > > > > >                 graph = &big_scale_graph;
> > > > > > > >                 cell_name = "big_scale_calib";
> > > > > > > >         } else {
> > > > > > > > -               calib_graph = &small_scale_graph_calib;
> > > > > > > > +               calib_graph = data->var_data->sscale_cal;
> > > > > > > >                 graph = &small_scale_graph;
> > > > > > > >                 cell_name = "small_scale_calib";
> > > > > > > >         }
> > > > > > > > @@ -160,7 +187,7 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data,
> > > > > > > >         return 0;
> > > > > > > >  }
> > > > > > > >
> > > > > > > > -static int sc27xx_adc_get_ratio(int channel, int scale)
> > > > > > > > +static int sc2731_adc_get_ratio(int channel, int scale)
> > > > > > > >  {
> > > > > > > >         switch (channel) {
> > > > > > > >         case 1:
> > > > > > > > @@ -185,6 +212,21 @@ static int sc27xx_adc_get_ratio(int channel, int scale)
> > > > > > > >         return SC27XX_VOLT_RATIO(1, 1);
> > > > > > > >  }
> > > > > > > >
> > > > > > > > +/*
> > > > > > > > + * According to the datasheet set specific value on some channel.
> > > > > > > > + */
> > > > > > > > +static void sc2731_adc_scale_init(struct sc27xx_adc_data *data)
> > > > > > > > +{
> > > > > > > > +       int i;
> > > > > > > > +
> > > > > > > > +       for (i = 0; i < SC27XX_ADC_CHANNEL_MAX; i++) {
> > > > > > > > +               if (i == 5)
> > > > > > > > +                       data->channel_scale[i] = 1;
> > > > > > > > +               else
> > > > > > > > +                       data->channel_scale[i] = 0;
> > > > > > > > +       }
> > > > > > > > +}
> > > > > > >
> > > > > > > This is unnecessary I think, please see sc27xx_adc_write_raw() that
> > > > > > > can set the channel scale.
> > > > > > Did you mean that all the PMIC's scale_init function should put into
> > > > > > the sc27xx_adc_write_raw?
> > > > >
> > > > > No.
> > > > >
> > > > > > but the scale_init is all different by each PMIC, if implemented in
> > > > > > the write_raw, will add a lot of
> > > > > > if or switch_case branch
> > > > >
> > > > > What I mean is we should follow the original method to set the channel
> > > > > scale by iio_info. Please also refer to other drivers how ot handle
> > > > > the channel scale.
> > > > Hi Baolin,  I understand the adc_write_raw() function is the method to set
> > > > channal scale for the userspace, we can change the channel scale by write
> > > > a value on a user code. did i understand right?
> > > > out  scale_init is to set scale value when the driver probe stage, and I also
> > > > did not found other adc driver use the adc_write_raw() during the driver
> > > >  initialization phase.
> > >
> > > Hi Jonathan,
> > >
> > > How do you think about the method in this patch to set the channel
> > > scale? Thanks.
> > >
> > Hi Jonathan,
> > Could you have a loot at this patch ,and give some advice about the
> > method to set the channel scale? Thanks very much.
>
> Hi, thanks for poking me on this - I'd missed the question buried deep in the thread!
>
> Anyhow, I don't quite follow the discussion but think it could be focused
> on one of 2 questions...
>
> 1) Does setting an initial default make sense?
>    Yes, this is an acceptable thing to do if there is a particular set of defaults
>    and there is no risk of regressions (i.e. the device wasn't previously supported
>    with different defaults).
> 2) Should you use the write_raw callback to actually do the setting?
>    Probably not as it has a set of parameters that don't make as much sense from within
>    the driver.  It 'might' make sense to have a common _set() function for this
>    feature which is called both in this initialization case and from the write_raw()
>    function however as that could do bounds checking etc in one common place.
>    However, it is very simple here, so perhaps not necessary.
>
> Jonathan
>
Hi Jonathan, thanks for your comment !
And Baolin, I will send a new verision for the patches tto keep the
scale_init and
fix other issues . thanks!
> > > --
> > > Baolin Wang
>

[PATCH 4/7] iio: adc: sc27xx: add support for PMIC sc2720 and sc2721

[Cixi Geng]

From: Cixi Geng <cixi.geng1@unisoc.com>

sc2720 and sc2721 is the product of sc27xx series.

Signed-off-by: Yuming Zhu <yuming.zhu1@unisoc.com>
Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com>
---
 drivers/iio/adc/sc27xx_adc.c | 198 +++++++++++++++++++++++++++++++++++
 1 file changed, 198 insertions(+)

diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c
index d2712e54ee79..7b5c66660ac9 100644
--- a/drivers/iio/adc/sc27xx_adc.c
+++ b/drivers/iio/adc/sc27xx_adc.c
@@ -9,11 +9,13 @@
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 
 /* PMIC global registers definition */
 #define SC2731_MODULE_EN		0xc08
 #define SC27XX_MODULE_ADC_EN		BIT(5)
+#define SC2721_ARM_CLK_EN		0xc0c
 #define SC2731_ARM_CLK_EN		0xc10
 #define SC27XX_CLK_ADC_EN		BIT(5)
 #define SC27XX_CLK_ADC_CLK_EN		BIT(6)
@@ -37,7 +39,9 @@
 /* Bits and mask definition for SC27XX_ADC_CH_CFG register */
 #define SC27XX_ADC_CHN_ID_MASK		GENMASK(4, 0)
 #define SC27XX_ADC_SCALE_MASK		GENMASK(10, 9)
+#define SC2721_ADC_SCALE_MASK		BIT(5)
 #define SC27XX_ADC_SCALE_SHIFT		9
+#define SC2721_ADC_SCALE_SHIFT		5
 
 /* Bits definitions for SC27XX_ADC_INT_EN registers */
 #define SC27XX_ADC_IRQ_EN		BIT(0)
@@ -67,8 +71,21 @@
 #define SC27XX_RATIO_NUMERATOR_OFFSET	16
 #define SC27XX_RATIO_DENOMINATOR_MASK	GENMASK(15, 0)
 
+/* ADC specific channel reference voltage 3.5V */
+#define SC27XX_ADC_REFVOL_VDD35		3500000
+
+/* ADC default channel reference voltage is 2.8V */
+#define SC27XX_ADC_REFVOL_VDD28		2800000
+
+enum sc27xx_pmic_type {
+	SC27XX_ADC,
+	SC2721_ADC,
+};
+
 struct sc27xx_adc_data {
+	struct iio_dev *indio_dev;
 	struct device *dev;
+	struct regulator *volref;
 	struct regmap *regmap;
 	/*
 	 * One hardware spinlock to synchronize between the multiple
@@ -87,6 +104,7 @@ struct sc27xx_adc_data {
  * in the device data structure.
  */
 struct sc27xx_adc_variant_data {
+	enum sc27xx_pmic_type pmic_type;
 	u32 module_en;
 	u32 clk_en;
 	u32 scale_shift;
@@ -187,6 +205,94 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data,
 	return 0;
 }
 
+static int sc2720_adc_get_ratio(int channel, int scale)
+{
+	switch (channel) {
+	case 14:
+		switch (scale) {
+		case 0:
+			return SC27XX_VOLT_RATIO(68, 900);
+		case 1:
+			return SC27XX_VOLT_RATIO(68, 1760);
+		case 2:
+			return SC27XX_VOLT_RATIO(68, 2327);
+		case 3:
+			return SC27XX_VOLT_RATIO(68, 3654);
+		default:
+			return SC27XX_VOLT_RATIO(1, 1);
+		}
+	case 16:
+		switch (scale) {
+		case 0:
+			return SC27XX_VOLT_RATIO(48, 100);
+		case 1:
+			return SC27XX_VOLT_RATIO(480, 1955);
+		case 2:
+			return SC27XX_VOLT_RATIO(480, 2586);
+		case 3:
+			return SC27XX_VOLT_RATIO(48, 406);
+		default:
+			return SC27XX_VOLT_RATIO(1, 1);
+		}
+	case 21:
+	case 22:
+	case 23:
+		switch (scale) {
+		case 0:
+			return SC27XX_VOLT_RATIO(3, 8);
+		case 1:
+			return SC27XX_VOLT_RATIO(375, 1955);
+		case 2:
+			return SC27XX_VOLT_RATIO(375, 2586);
+		case 3:
+			return SC27XX_VOLT_RATIO(300, 3248);
+		default:
+			return SC27XX_VOLT_RATIO(1, 1);
+		}
+	default:
+		switch (scale) {
+		case 0:
+			return SC27XX_VOLT_RATIO(1, 1);
+		case 1:
+			return SC27XX_VOLT_RATIO(1000, 1955);
+		case 2:
+			return SC27XX_VOLT_RATIO(1000, 2586);
+		case 3:
+			return SC27XX_VOLT_RATIO(100, 406);
+		default:
+			return SC27XX_VOLT_RATIO(1, 1);
+		}
+	}
+	return SC27XX_VOLT_RATIO(1, 1);
+}
+
+static int sc2721_adc_get_ratio(int channel, int scale)
+{
+	switch (channel) {
+	case 1:
+	case 2:
+	case 3:
+	case 4:
+		return scale ? SC27XX_VOLT_RATIO(400, 1025) :
+			SC27XX_VOLT_RATIO(1, 1);
+	case 5:
+		return SC27XX_VOLT_RATIO(7, 29);
+	case 7:
+	case 9:
+		return scale ? SC27XX_VOLT_RATIO(100, 125) :
+			SC27XX_VOLT_RATIO(1, 1);
+	case 14:
+		return SC27XX_VOLT_RATIO(68, 900);
+	case 16:
+		return SC27XX_VOLT_RATIO(48, 100);
+	case 19:
+		return SC27XX_VOLT_RATIO(1, 3);
+	default:
+		return SC27XX_VOLT_RATIO(1, 1);
+	}
+	return SC27XX_VOLT_RATIO(1, 1);
+}
+
 static int sc2731_adc_get_ratio(int channel, int scale)
 {
 	switch (channel) {
@@ -215,6 +321,34 @@ static int sc2731_adc_get_ratio(int channel, int scale)
 /*
  * According to the datasheet set specific value on some channel.
  */
+static void sc2720_adc_scale_init(struct sc27xx_adc_data *data)
+{
+	int i;
+
+	for (i = 0; i < SC27XX_ADC_CHANNEL_MAX; i++) {
+		switch (i) {
+		case 5:
+			data->channel_scale[i] = 3;
+			break;
+		case 7:
+		case 9:
+			data->channel_scale[i] = 2;
+			break;
+		case 13:
+			data->channel_scale[i] = 1;
+			break;
+		case 19:
+		case 30:
+		case 31:
+			data->channel_scale[i] = 3;
+			break;
+		default:
+			data->channel_scale[i] = 0;
+			break;
+		}
+	}
+}
+
 static void sc2731_adc_scale_init(struct sc27xx_adc_data *data)
 {
 	int i;
@@ -239,6 +373,24 @@ static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
 		return ret;
 	}
 
+	/*
+	 * According to the sc2721 chip data sheet, the reference voltage of
+	 * specific channel 30 and channel 31 in ADC module needs to be set from
+	 * the default 2.8v to 3.5v.
+	 */
+	if (data->var_data->pmic_type == SC2721_ADC) {
+		if ((channel == 30) || (channel == 31)) {
+			ret = regulator_set_voltage(data->volref,
+						SC27XX_ADC_REFVOL_VDD35,
+						SC27XX_ADC_REFVOL_VDD35);
+			if (ret) {
+				dev_err(data->dev, "failed to set the volref 3.5V\n");
+				hwspin_unlock_raw(data->hwlock);
+				return ret;
+			}
+		}
+	}
+
 	ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CTL,
 				 SC27XX_ADC_EN, SC27XX_ADC_EN);
 	if (ret)
@@ -293,6 +445,16 @@ static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
 	regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CTL,
 			   SC27XX_ADC_EN, 0);
 unlock_adc:
+	if (data->var_data->pmic_type == SC2721_ADC) {
+		if ((channel == 30) || (channel == 31)) {
+			ret = regulator_set_voltage(data->volref,
+						    SC27XX_ADC_REFVOL_VDD28,
+						    SC27XX_ADC_REFVOL_VDD28);
+			if (ret)
+				dev_err(data->dev, "failed to set the volref 2.8V\n");
+		}
+	}
+
 	hwspin_unlock_raw(data->hwlock);
 
 	if (!ret)
@@ -522,6 +684,7 @@ static void sc27xx_adc_disable(void *_data)
 }
 
 static const struct sc27xx_adc_variant_data sc2731_data = {
+	.pmic_type = SC27XX_ADC,
 	.module_en = SC2731_MODULE_EN,
 	.clk_en = SC2731_ARM_CLK_EN,
 	.scale_shift = SC27XX_ADC_SCALE_SHIFT,
@@ -532,6 +695,30 @@ static const struct sc27xx_adc_variant_data sc2731_data = {
 	.get_ratio = sc2731_adc_get_ratio,
 };
 
+static const struct sc27xx_adc_variant_data sc2721_data = {
+	.pmic_type = SC2721_ADC,
+	.module_en = SC2731_MODULE_EN,
+	.clk_en = SC2721_ARM_CLK_EN,
+	.scale_shift = SC2721_ADC_SCALE_SHIFT,
+	.scale_mask = SC2721_ADC_SCALE_MASK,
+	.bscale_cal = &sc2731_big_scale_graph_calib,
+	.sscale_cal = &sc2731_small_scale_graph_calib,
+	.init_scale = sc2731_adc_scale_init,
+	.get_ratio = sc2721_adc_get_ratio,
+};
+
+static const struct sc27xx_adc_variant_data sc2720_data = {
+	.pmic_type = SC27XX_ADC,
+	.module_en = SC2731_MODULE_EN,
+	.clk_en = SC2721_ARM_CLK_EN,
+	.scale_shift = SC27XX_ADC_SCALE_SHIFT,
+	.scale_mask = SC27XX_ADC_SCALE_MASK,
+	.bscale_cal = &big_scale_graph_calib,
+	.sscale_cal = &small_scale_graph_calib,
+	.init_scale = sc2720_adc_scale_init,
+	.get_ratio = sc2720_adc_get_ratio,
+};
+
 static int sc27xx_adc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -582,6 +769,15 @@ static int sc27xx_adc_probe(struct platform_device *pdev)
 	}
 
 	sc27xx_data->dev = dev;
+	if (pdata->pmic_type == SC2721_ADC) {
+		sc27xx_data->volref = devm_regulator_get_optional(dev, "vref");
+		if (IS_ERR_OR_NULL(sc27xx_data->volref)) {
+			ret = PTR_ERR(sc27xx_data->volref);
+			dev_err(dev, "err! ADC volref, err: %d\n", ret);
+			return ret;
+		}
+	}
+
 	sc27xx_data->var_data = pdata;
 	sc27xx_data->var_data->init_scale(sc27xx_data);
 
@@ -611,6 +807,8 @@ static int sc27xx_adc_probe(struct platform_device *pdev)
 
 static const struct of_device_id sc27xx_adc_of_match[] = {
 	{ .compatible = "sprd,sc2731-adc", .data = &sc2731_data},
+	{ .compatible = "sprd,sc2721-adc", .data = &sc2721_data},
+	{ .compatible = "sprd,sc2720-adc", .data = &sc2720_data},
 	{ }
 };
 MODULE_DEVICE_TABLE(of, sc27xx_adc_of_match);
-- 
2.25.1

Re: [PATCH 4/7] iio: adc: sc27xx: add support for PMIC sc2720 and sc2721

[Baolin Wang]

On Thu, Jan 6, 2022 at 9:00 PM Cixi Geng <gengcixi@gmail.com> wrote:
>
> From: Cixi Geng <cixi.geng1@unisoc.com>
>
> sc2720 and sc2721 is the product of sc27xx series.
>
> Signed-off-by: Yuming Zhu <yuming.zhu1@unisoc.com>
> Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com>
> ---
>  drivers/iio/adc/sc27xx_adc.c | 198 +++++++++++++++++++++++++++++++++++
>  1 file changed, 198 insertions(+)
>
> diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c
> index d2712e54ee79..7b5c66660ac9 100644
> --- a/drivers/iio/adc/sc27xx_adc.c
> +++ b/drivers/iio/adc/sc27xx_adc.c
> @@ -9,11 +9,13 @@
>  #include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
>
>  /* PMIC global registers definition */
>  #define SC2731_MODULE_EN               0xc08
>  #define SC27XX_MODULE_ADC_EN           BIT(5)
> +#define SC2721_ARM_CLK_EN              0xc0c
>  #define SC2731_ARM_CLK_EN              0xc10
>  #define SC27XX_CLK_ADC_EN              BIT(5)
>  #define SC27XX_CLK_ADC_CLK_EN          BIT(6)
> @@ -37,7 +39,9 @@
>  /* Bits and mask definition for SC27XX_ADC_CH_CFG register */
>  #define SC27XX_ADC_CHN_ID_MASK         GENMASK(4, 0)
>  #define SC27XX_ADC_SCALE_MASK          GENMASK(10, 9)
> +#define SC2721_ADC_SCALE_MASK          BIT(5)
>  #define SC27XX_ADC_SCALE_SHIFT         9
> +#define SC2721_ADC_SCALE_SHIFT         5
>
>  /* Bits definitions for SC27XX_ADC_INT_EN registers */
>  #define SC27XX_ADC_IRQ_EN              BIT(0)
> @@ -67,8 +71,21 @@
>  #define SC27XX_RATIO_NUMERATOR_OFFSET  16
>  #define SC27XX_RATIO_DENOMINATOR_MASK  GENMASK(15, 0)
>
> +/* ADC specific channel reference voltage 3.5V */
> +#define SC27XX_ADC_REFVOL_VDD35                3500000
> +
> +/* ADC default channel reference voltage is 2.8V */
> +#define SC27XX_ADC_REFVOL_VDD28                2800000
> +
> +enum sc27xx_pmic_type {
> +       SC27XX_ADC,
> +       SC2721_ADC,
> +};
> +
>  struct sc27xx_adc_data {
> +       struct iio_dev *indio_dev;

Why add an unused member?

>         struct device *dev;
> +       struct regulator *volref;
>         struct regmap *regmap;
>         /*
>          * One hardware spinlock to synchronize between the multiple
> @@ -87,6 +104,7 @@ struct sc27xx_adc_data {
>   * in the device data structure.
>   */
>  struct sc27xx_adc_variant_data {
> +       enum sc27xx_pmic_type pmic_type;
>         u32 module_en;
>         u32 clk_en;
>         u32 scale_shift;
> @@ -187,6 +205,94 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data,
>         return 0;
>  }
>
> +static int sc2720_adc_get_ratio(int channel, int scale)
> +{
> +       switch (channel) {
> +       case 14:
> +               switch (scale) {
> +               case 0:
> +                       return SC27XX_VOLT_RATIO(68, 900);
> +               case 1:
> +                       return SC27XX_VOLT_RATIO(68, 1760);
> +               case 2:
> +                       return SC27XX_VOLT_RATIO(68, 2327);
> +               case 3:
> +                       return SC27XX_VOLT_RATIO(68, 3654);
> +               default:
> +                       return SC27XX_VOLT_RATIO(1, 1);
> +               }
> +       case 16:
> +               switch (scale) {
> +               case 0:
> +                       return SC27XX_VOLT_RATIO(48, 100);
> +               case 1:
> +                       return SC27XX_VOLT_RATIO(480, 1955);
> +               case 2:
> +                       return SC27XX_VOLT_RATIO(480, 2586);
> +               case 3:
> +                       return SC27XX_VOLT_RATIO(48, 406);
> +               default:
> +                       return SC27XX_VOLT_RATIO(1, 1);
> +               }
> +       case 21:
> +       case 22:
> +       case 23:
> +               switch (scale) {
> +               case 0:
> +                       return SC27XX_VOLT_RATIO(3, 8);
> +               case 1:
> +                       return SC27XX_VOLT_RATIO(375, 1955);
> +               case 2:
> +                       return SC27XX_VOLT_RATIO(375, 2586);
> +               case 3:
> +                       return SC27XX_VOLT_RATIO(300, 3248);
> +               default:
> +                       return SC27XX_VOLT_RATIO(1, 1);
> +               }
> +       default:
> +               switch (scale) {
> +               case 0:
> +                       return SC27XX_VOLT_RATIO(1, 1);
> +               case 1:
> +                       return SC27XX_VOLT_RATIO(1000, 1955);
> +               case 2:
> +                       return SC27XX_VOLT_RATIO(1000, 2586);
> +               case 3:
> +                       return SC27XX_VOLT_RATIO(100, 406);
> +               default:
> +                       return SC27XX_VOLT_RATIO(1, 1);
> +               }
> +       }
> +       return SC27XX_VOLT_RATIO(1, 1);
> +}
> +
> +static int sc2721_adc_get_ratio(int channel, int scale)
> +{
> +       switch (channel) {
> +       case 1:
> +       case 2:
> +       case 3:
> +       case 4:
> +               return scale ? SC27XX_VOLT_RATIO(400, 1025) :
> +                       SC27XX_VOLT_RATIO(1, 1);
> +       case 5:
> +               return SC27XX_VOLT_RATIO(7, 29);
> +       case 7:
> +       case 9:
> +               return scale ? SC27XX_VOLT_RATIO(100, 125) :
> +                       SC27XX_VOLT_RATIO(1, 1);
> +       case 14:
> +               return SC27XX_VOLT_RATIO(68, 900);
> +       case 16:
> +               return SC27XX_VOLT_RATIO(48, 100);
> +       case 19:
> +               return SC27XX_VOLT_RATIO(1, 3);
> +       default:
> +               return SC27XX_VOLT_RATIO(1, 1);
> +       }
> +       return SC27XX_VOLT_RATIO(1, 1);
> +}
> +
>  static int sc2731_adc_get_ratio(int channel, int scale)
>  {
>         switch (channel) {
> @@ -215,6 +321,34 @@ static int sc2731_adc_get_ratio(int channel, int scale)
>  /*
>   * According to the datasheet set specific value on some channel.
>   */
> +static void sc2720_adc_scale_init(struct sc27xx_adc_data *data)
> +{
> +       int i;
> +
> +       for (i = 0; i < SC27XX_ADC_CHANNEL_MAX; i++) {
> +               switch (i) {
> +               case 5:
> +                       data->channel_scale[i] = 3;
> +                       break;
> +               case 7:
> +               case 9:
> +                       data->channel_scale[i] = 2;
> +                       break;
> +               case 13:
> +                       data->channel_scale[i] = 1;
> +                       break;
> +               case 19:
> +               case 30:
> +               case 31:
> +                       data->channel_scale[i] = 3;
> +                       break;
> +               default:
> +                       data->channel_scale[i] = 0;
> +                       break;
> +               }
> +       }
> +}

Like previous comments, this is not needed.

> +
>  static void sc2731_adc_scale_init(struct sc27xx_adc_data *data)
>  {
>         int i;
> @@ -239,6 +373,24 @@ static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
>                 return ret;
>         }
>
> +       /*
> +        * According to the sc2721 chip data sheet, the reference voltage of
> +        * specific channel 30 and channel 31 in ADC module needs to be set from
> +        * the default 2.8v to 3.5v.
> +        */
> +       if (data->var_data->pmic_type == SC2721_ADC) {
> +               if ((channel == 30) || (channel == 31)) {

Combine the two branches please.

> +                       ret = regulator_set_voltage(data->volref,
> +                                               SC27XX_ADC_REFVOL_VDD35,
> +                                               SC27XX_ADC_REFVOL_VDD35);
> +                       if (ret) {
> +                               dev_err(data->dev, "failed to set the volref 3.5V\n");
> +                               hwspin_unlock_raw(data->hwlock);
> +                               return ret;
> +                       }
> +               }
> +       }
> +
>         ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CTL,
>                                  SC27XX_ADC_EN, SC27XX_ADC_EN);
>         if (ret)
> @@ -293,6 +445,16 @@ static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
>         regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CTL,
>                            SC27XX_ADC_EN, 0);
>  unlock_adc:
> +       if (data->var_data->pmic_type == SC2721_ADC) {
> +               if ((channel == 30) || (channel == 31)) {
> +                       ret = regulator_set_voltage(data->volref,
> +                                                   SC27XX_ADC_REFVOL_VDD28,
> +                                                   SC27XX_ADC_REFVOL_VDD28);
> +                       if (ret)
> +                               dev_err(data->dev, "failed to set the volref 2.8V\n");
> +               }
> +       }
> +
>         hwspin_unlock_raw(data->hwlock);
>
>         if (!ret)
> @@ -522,6 +684,7 @@ static void sc27xx_adc_disable(void *_data)
>  }
>
>  static const struct sc27xx_adc_variant_data sc2731_data = {
> +       .pmic_type = SC27XX_ADC,
>         .module_en = SC2731_MODULE_EN,
>         .clk_en = SC2731_ARM_CLK_EN,
>         .scale_shift = SC27XX_ADC_SCALE_SHIFT,
> @@ -532,6 +695,30 @@ static const struct sc27xx_adc_variant_data sc2731_data = {
>         .get_ratio = sc2731_adc_get_ratio,
>  };
>
> +static const struct sc27xx_adc_variant_data sc2721_data = {
> +       .pmic_type = SC2721_ADC,
> +       .module_en = SC2731_MODULE_EN,
> +       .clk_en = SC2721_ARM_CLK_EN,
> +       .scale_shift = SC2721_ADC_SCALE_SHIFT,
> +       .scale_mask = SC2721_ADC_SCALE_MASK,
> +       .bscale_cal = &sc2731_big_scale_graph_calib,
> +       .sscale_cal = &sc2731_small_scale_graph_calib,
> +       .init_scale = sc2731_adc_scale_init,
> +       .get_ratio = sc2721_adc_get_ratio,
> +};
> +
> +static const struct sc27xx_adc_variant_data sc2720_data = {
> +       .pmic_type = SC27XX_ADC,
> +       .module_en = SC2731_MODULE_EN,
> +       .clk_en = SC2721_ARM_CLK_EN,
> +       .scale_shift = SC27XX_ADC_SCALE_SHIFT,
> +       .scale_mask = SC27XX_ADC_SCALE_MASK,
> +       .bscale_cal = &big_scale_graph_calib,
> +       .sscale_cal = &small_scale_graph_calib,
> +       .init_scale = sc2720_adc_scale_init,
> +       .get_ratio = sc2720_adc_get_ratio,
> +};
> +
>  static int sc27xx_adc_probe(struct platform_device *pdev)
>  {
>         struct device *dev = &pdev->dev;
> @@ -582,6 +769,15 @@ static int sc27xx_adc_probe(struct platform_device *pdev)
>         }
>
>         sc27xx_data->dev = dev;
> +       if (pdata->pmic_type == SC2721_ADC) {
> +               sc27xx_data->volref = devm_regulator_get_optional(dev, "vref");
> +               if (IS_ERR_OR_NULL(sc27xx_data->volref)) {

devm_regulator_get_optional() never return NULL, please use IS_ERR().

> +                       ret = PTR_ERR(sc27xx_data->volref);

Should check ret == -ENODEV, since -ENODEV means the regulator is not
supplied which is not a error for 'OPTIONAL_GET' type.

> +                       dev_err(dev, "err! ADC volref, err: %d\n", ret);

Can you elaborate on the error message like other places in this driver?

> +                       return ret;
> +               }
> +       }
> +
>         sc27xx_data->var_data = pdata;
>         sc27xx_data->var_data->init_scale(sc27xx_data);
>
> @@ -611,6 +807,8 @@ static int sc27xx_adc_probe(struct platform_device *pdev)
>
>  static const struct of_device_id sc27xx_adc_of_match[] = {
>         { .compatible = "sprd,sc2731-adc", .data = &sc2731_data},
> +       { .compatible = "sprd,sc2721-adc", .data = &sc2721_data},
> +       { .compatible = "sprd,sc2720-adc", .data = &sc2720_data},
>         { }
>  };
>  MODULE_DEVICE_TABLE(of, sc27xx_adc_of_match);
> --
> 2.25.1
>


-- 
Baolin Wang

Re: [PATCH 4/7] iio: adc: sc27xx: add support for PMIC sc2720 and sc2721

[Jonathan Cameron]

On Fri, 7 Jan 2022 15:16:15 +0800
Baolin Wang <baolin.wang7@gmail.com> wrote:

> On Thu, Jan 6, 2022 at 9:00 PM Cixi Geng <gengcixi@gmail.com> wrote:
> >
> > From: Cixi Geng <cixi.geng1@unisoc.com>
> >
> > sc2720 and sc2721 is the product of sc27xx series.
> >
> > Signed-off-by: Yuming Zhu <yuming.zhu1@unisoc.com>
> > Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com>
> > ---
> >  drivers/iio/adc/sc27xx_adc.c | 198 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 198 insertions(+)
> >
> > diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c
> > index d2712e54ee79..7b5c66660ac9 100644
> > --- a/drivers/iio/adc/sc27xx_adc.c
> > +++ b/drivers/iio/adc/sc27xx_adc.c
> > @@ -9,11 +9,13 @@
> >  #include <linux/of_device.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h>
> >  #include <linux/slab.h>
> >
> >  /* PMIC global registers definition */
> >  #define SC2731_MODULE_EN               0xc08
> >  #define SC27XX_MODULE_ADC_EN           BIT(5)
> > +#define SC2721_ARM_CLK_EN              0xc0c
> >  #define SC2731_ARM_CLK_EN              0xc10
> >  #define SC27XX_CLK_ADC_EN              BIT(5)
> >  #define SC27XX_CLK_ADC_CLK_EN          BIT(6)
> > @@ -37,7 +39,9 @@
> >  /* Bits and mask definition for SC27XX_ADC_CH_CFG register */
> >  #define SC27XX_ADC_CHN_ID_MASK         GENMASK(4, 0)
> >  #define SC27XX_ADC_SCALE_MASK          GENMASK(10, 9)
> > +#define SC2721_ADC_SCALE_MASK          BIT(5)
> >  #define SC27XX_ADC_SCALE_SHIFT         9
> > +#define SC2721_ADC_SCALE_SHIFT         5
> >
> >  /* Bits definitions for SC27XX_ADC_INT_EN registers */
> >  #define SC27XX_ADC_IRQ_EN              BIT(0)
> > @@ -67,8 +71,21 @@
> >  #define SC27XX_RATIO_NUMERATOR_OFFSET  16
> >  #define SC27XX_RATIO_DENOMINATOR_MASK  GENMASK(15, 0)
> >
> > +/* ADC specific channel reference voltage 3.5V */
> > +#define SC27XX_ADC_REFVOL_VDD35                3500000
> > +
> > +/* ADC default channel reference voltage is 2.8V */
> > +#define SC27XX_ADC_REFVOL_VDD28                2800000
> > +
> > +enum sc27xx_pmic_type {
> > +       SC27XX_ADC,
> > +       SC2721_ADC,
> > +};
> > +
> >  struct sc27xx_adc_data {
> > +       struct iio_dev *indio_dev;  
> 
> Why add an unused member?
It's very very rarely a good architecture structure to have
the data stored in iio_priv() have a pointer back to the indio_dev.
Normally it implies somewhere the wrong level of structure is being
passed to a function.

So I'm glad it's not used :)

> 
> >         struct device *dev;
> > +       struct regulator *volref;
> >         struct regmap *regmap;
> >         /*
> >          * One hardware spinlock to synchronize between the multiple
> > @@ -87,6 +104,7 @@ struct sc27xx_adc_data {
> >   * in the device data structure.
> >   */

...

> 
> > +
> >  static void sc2731_adc_scale_init(struct sc27xx_adc_data *data)
> >  {
> >         int i;
> > @@ -239,6 +373,24 @@ static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
> >                 return ret;
> >         }
> >
> > +       /*
> > +        * According to the sc2721 chip data sheet, the reference voltage of
> > +        * specific channel 30 and channel 31 in ADC module needs to be set from
> > +        * the default 2.8v to 3.5v.

That's horrible... :) Ah well...

> > +        */
> > +       if (data->var_data->pmic_type == SC2721_ADC) {
> > +               if ((channel == 30) || (channel == 31)) {  
> 
> Combine the two branches please.
> 
> > +                       ret = regulator_set_voltage(data->volref,
> > +                                               SC27XX_ADC_REFVOL_VDD35,
> > +                                               SC27XX_ADC_REFVOL_VDD35);
> > +                       if (ret) {
> > +                               dev_err(data->dev, "failed to set the volref 3.5V\n");
> > +                               hwspin_unlock_raw(data->hwlock);
> > +                               return ret;
> > +                       }
> > +               }
> > +       }
> > +
> >         ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CTL,
> >                                  SC27XX_ADC_EN, SC27XX_ADC_EN);
> >         if (ret)
> > @@ -293,6 +445,16 @@ static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
> >         regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CTL,
> >                            SC27XX_ADC_EN, 0);
> >  unlock_adc:
> > +       if (data->var_data->pmic_type == SC2721_ADC) {
> > +               if ((channel == 30) || (channel == 31)) {
> > +                       ret = regulator_set_voltage(data->volref,
> > +                                                   SC27XX_ADC_REFVOL_VDD28,
> > +                                                   SC27XX_ADC_REFVOL_VDD28);
> > +                       if (ret)
> > +                               dev_err(data->dev, "failed to set the volref 2.8V\n");
> > +               }
> > +       }
> > +
> >         hwspin_unlock_raw(data->hwlock);
> >
> >         if (!ret)
> > @@ -522,6 +684,7 @@ static void sc27xx_adc_disable(void *_data)
> >  }

...

> 

[PATCH 5/7] iio: adc: sc27xx: add support for PMIC sc2730

[Cixi Geng]

From: Cixi Geng <cixi.geng1@unisoc.com>

sc2730 is the product of sc27xx series.

Signed-off-by: Yuming Zhu <yuming.zhu1@unisoc.com>
Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com>
---
 drivers/iio/adc/sc27xx_adc.c | 105 +++++++++++++++++++++++++++++++++++
 1 file changed, 105 insertions(+)

diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c
index 7b5c66660ac9..195f44cf61e1 100644
--- a/drivers/iio/adc/sc27xx_adc.c
+++ b/drivers/iio/adc/sc27xx_adc.c
@@ -13,9 +13,11 @@
 #include <linux/slab.h>
 
 /* PMIC global registers definition */
+#define SC2730_MODULE_EN		0x1808
 #define SC2731_MODULE_EN		0xc08
 #define SC27XX_MODULE_ADC_EN		BIT(5)
 #define SC2721_ARM_CLK_EN		0xc0c
+#define SC2730_ARM_CLK_EN		0x180c
 #define SC2731_ARM_CLK_EN		0xc10
 #define SC27XX_CLK_ADC_EN		BIT(5)
 #define SC27XX_CLK_ADC_CLK_EN		BIT(6)
@@ -293,6 +295,80 @@ static int sc2721_adc_get_ratio(int channel, int scale)
 	return SC27XX_VOLT_RATIO(1, 1);
 }
 
+static int sc2730_adc_get_ratio(int channel, int scale)
+{
+	switch (channel) {
+	case 14:
+		switch (scale) {
+		case 0:
+			return SC27XX_VOLT_RATIO(68, 900);
+		case 1:
+			return SC27XX_VOLT_RATIO(68, 1760);
+		case 2:
+			return SC27XX_VOLT_RATIO(68, 2327);
+		case 3:
+			return SC27XX_VOLT_RATIO(68, 3654);
+		default:
+			return SC27XX_VOLT_RATIO(1, 1);
+		}
+	case 15:
+		switch (scale) {
+		case 0:
+			return SC27XX_VOLT_RATIO(1, 3);
+		case 1:
+			return SC27XX_VOLT_RATIO(1000, 5865);
+		case 2:
+			return SC27XX_VOLT_RATIO(500, 3879);
+		case 3:
+			return SC27XX_VOLT_RATIO(500, 6090);
+		default:
+			return SC27XX_VOLT_RATIO(1, 1);
+		}
+	case 16:
+		switch (scale) {
+		case 0:
+			return SC27XX_VOLT_RATIO(48, 100);
+		case 1:
+			return SC27XX_VOLT_RATIO(480, 1955);
+		case 2:
+			return SC27XX_VOLT_RATIO(480, 2586);
+		case 3:
+			return SC27XX_VOLT_RATIO(48, 406);
+		default:
+			return SC27XX_VOLT_RATIO(1, 1);
+		}
+	case 21:
+	case 22:
+	case 23:
+		switch (scale) {
+		case 0:
+			return SC27XX_VOLT_RATIO(3, 8);
+		case 1:
+			return SC27XX_VOLT_RATIO(375, 1955);
+		case 2:
+			return SC27XX_VOLT_RATIO(375, 2586);
+		case 3:
+			return SC27XX_VOLT_RATIO(300, 3248);
+		default:
+			return SC27XX_VOLT_RATIO(1, 1);
+		}
+	default:
+		switch (scale) {
+		case 0:
+			return SC27XX_VOLT_RATIO(1, 1);
+		case 1:
+			return SC27XX_VOLT_RATIO(1000, 1955);
+		case 2:
+			return SC27XX_VOLT_RATIO(1000, 2586);
+		case 3:
+			return SC27XX_VOLT_RATIO(1000, 4060);
+		default:
+			return SC27XX_VOLT_RATIO(1, 1);
+		}
+	}
+	return SC27XX_VOLT_RATIO(1, 1);
+}
+
 static int sc2731_adc_get_ratio(int channel, int scale)
 {
 	switch (channel) {
@@ -349,6 +425,22 @@ static void sc2720_adc_scale_init(struct sc27xx_adc_data *data)
 	}
 }
 
+static void sc2730_adc_scale_init(struct sc27xx_adc_data *data)
+{
+	int i;
+
+	for (i = 0; i < SC27XX_ADC_CHANNEL_MAX; i++) {
+		if (i == 5 || i == 10 || i == 19 || i == 30 || i == 31)
+			data->channel_scale[i] = 3;
+		else if (i == 7 || i == 9)
+			data->channel_scale[i] = 2;
+		else if (i == 13)
+			data->channel_scale[i] = 1;
+		else
+			data->channel_scale[i] = 0;
+	}
+}
+
 static void sc2731_adc_scale_init(struct sc27xx_adc_data *data)
 {
 	int i;
@@ -695,6 +787,18 @@ static const struct sc27xx_adc_variant_data sc2731_data = {
 	.get_ratio = sc2731_adc_get_ratio,
 };
 
+static const struct sc27xx_adc_variant_data sc2730_data = {
+	.pmic_type = SC27XX_ADC,
+	.module_en = SC2730_MODULE_EN,
+	.clk_en = SC2730_ARM_CLK_EN,
+	.scale_shift = SC27XX_ADC_SCALE_SHIFT,
+	.scale_mask = SC27XX_ADC_SCALE_MASK,
+	.bscale_cal = &big_scale_graph_calib,
+	.sscale_cal = &small_scale_graph_calib,
+	.init_scale = sc2730_adc_scale_init,
+	.get_ratio = sc2730_adc_get_ratio,
+};
+
 static const struct sc27xx_adc_variant_data sc2721_data = {
 	.pmic_type = SC2721_ADC,
 	.module_en = SC2731_MODULE_EN,
@@ -807,6 +911,7 @@ static int sc27xx_adc_probe(struct platform_device *pdev)
 
 static const struct of_device_id sc27xx_adc_of_match[] = {
 	{ .compatible = "sprd,sc2731-adc", .data = &sc2731_data},
+	{ .compatible = "sprd,sc2730-adc", .data = &sc2730_data},
 	{ .compatible = "sprd,sc2721-adc", .data = &sc2721_data},
 	{ .compatible = "sprd,sc2720-adc", .data = &sc2720_data},
 	{ }
-- 
2.25.1

[PATCH 6/7] iio: adc: sc27xx: add support for PMIC ump9620

[Cixi Geng]

From: Cixi Geng <cixi.geng1@unisoc.com>

The ump9620 is variant from sc27xx chip, add it in here.

Signed-off-by: Yuming Zhu <yuming.zhu1@unisoc.com>
Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com>
---
 drivers/iio/adc/sc27xx_adc.c | 263 +++++++++++++++++++++++++++++++++--
 1 file changed, 254 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c
index 195f44cf61e1..68b967f32498 100644
--- a/drivers/iio/adc/sc27xx_adc.c
+++ b/drivers/iio/adc/sc27xx_adc.c
@@ -15,12 +15,16 @@
 /* PMIC global registers definition */
 #define SC2730_MODULE_EN		0x1808
 #define SC2731_MODULE_EN		0xc08
+#define UMP9620_MODULE_EN		0x2008
 #define SC27XX_MODULE_ADC_EN		BIT(5)
 #define SC2721_ARM_CLK_EN		0xc0c
 #define SC2730_ARM_CLK_EN		0x180c
 #define SC2731_ARM_CLK_EN		0xc10
+#define UMP9620_ARM_CLK_EN		0x200c
+#define UMP9620_XTL_WAIT_CTRL0		0x2378
 #define SC27XX_CLK_ADC_EN		BIT(5)
 #define SC27XX_CLK_ADC_CLK_EN		BIT(6)
+#define UMP9620_XTL_WAIT_CTRL0_EN	BIT(8)
 
 /* ADC controller registers definition */
 #define SC27XX_ADC_CTL			0x0
@@ -82,6 +86,13 @@
 enum sc27xx_pmic_type {
 	SC27XX_ADC,
 	SC2721_ADC,
+	UMP9620_ADC,
+};
+
+enum ump96xx_scale_cal {
+	UMP96XX_VBAT_SENSES_CAL,
+	UMP96XX_VBAT_DET_CAL,
+	UMP96XX_CH1_CAL,
 };
 
 struct sc27xx_adc_data {
@@ -140,6 +151,11 @@ static struct sc27xx_adc_linear_graph small_scale_graph = {
 	100, 341,
 };
 
+static struct sc27xx_adc_linear_graph ump9620_bat_det_graph = {
+	1400, 3482,
+	200, 476,
+};
+
 static const struct sc27xx_adc_linear_graph sc2731_big_scale_graph_calib = {
 	4200, 850,
 	3600, 728,
@@ -165,6 +181,33 @@ static int sc27xx_adc_get_calib_data(u32 calib_data, int calib_adc)
 	return ((calib_data & 0xff) + calib_adc - 128) * 4;
 }
 
+static int adc_nvmem_cell_calib_data(struct sc27xx_adc_data *data, const char *cell_name)
+{
+	struct nvmem_cell *cell;
+	void *buf;
+	u32 calib_data = 0;
+	size_t len = 0;
+
+	if (!data)
+		return -EINVAL;
+
+	cell = nvmem_cell_get(data->dev, cell_name);
+	if (IS_ERR_OR_NULL(cell))
+		return PTR_ERR(cell);
+
+	buf = nvmem_cell_read(cell, &len);
+	if (IS_ERR_OR_NULL(buf)) {
+		nvmem_cell_put(cell);
+		return PTR_ERR(buf);
+	}
+
+	memcpy(&calib_data, buf, min(len, sizeof(u32)));
+
+	kfree(buf);
+	nvmem_cell_put(cell);
+	return calib_data;
+}
+
 static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data,
 					bool big_scale)
 {
@@ -207,6 +250,56 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data,
 	return 0;
 }
 
+static int ump96xx_adc_scale_cal(struct sc27xx_adc_data *data,
+					enum ump96xx_scale_cal cal_type)
+{
+	struct sc27xx_adc_linear_graph *graph = NULL;
+	const char *cell_name1 = NULL, *cell_name2 = NULL;
+	int adc_calib_data1 = 0, adc_calib_data2 = 0;
+
+	if (!data)
+		return -EINVAL;
+
+	if (cal_type == UMP96XX_VBAT_DET_CAL) {
+		graph = &ump9620_bat_det_graph;
+		cell_name1 = "vbat_det_cal1";
+		cell_name2 = "vbat_det_cal2";
+	} else if (cal_type == UMP96XX_VBAT_SENSES_CAL) {
+		graph = &big_scale_graph;
+		cell_name1 = "big_scale_calib1";
+		cell_name2 = "big_scale_calib2";
+	} else if (cal_type == UMP96XX_CH1_CAL) {
+		graph = &small_scale_graph;
+		cell_name1 = "small_scale_calib1";
+		cell_name2 = "small_scale_calib2";
+	} else {
+		graph = &small_scale_graph;
+		cell_name1 = "small_scale_calib1";
+		cell_name2 = "small_scale_calib2";
+	}
+
+	adc_calib_data1 = adc_nvmem_cell_calib_data(data, cell_name1);
+	if (adc_calib_data1 < 0) {
+		dev_err(data->dev, "err! %s:%d\n", cell_name1, adc_calib_data1);
+		return adc_calib_data1;
+	}
+
+	adc_calib_data2 = adc_nvmem_cell_calib_data(data, cell_name2);
+	if (adc_calib_data2 < 0) {
+		dev_err(data->dev, "err! %s:%d\n", cell_name2, adc_calib_data2);
+		return adc_calib_data2;
+	}
+
+	/*
+	 *Read the data in the two blocks of efuse and convert them into the
+	 *calibration value in the ump9620 adc linear graph.
+	 */
+	graph->adc0 = (adc_calib_data1 & 0xfff0) >> 4;
+	graph->adc1 = (adc_calib_data2 & 0xfff0) >> 4;
+
+	return 0;
+}
+
 static int sc2720_adc_get_ratio(int channel, int scale)
 {
 	switch (channel) {
@@ -394,6 +487,50 @@ static int sc2731_adc_get_ratio(int channel, int scale)
 	return SC27XX_VOLT_RATIO(1, 1);
 }
 
+static int ump9620_adc_get_ratio(int channel, int scale)
+{
+	switch (channel) {
+	case 11:
+		return SC27XX_VOLT_RATIO(1, 1);
+	case 14:
+		switch (scale) {
+		case 0:
+			return SC27XX_VOLT_RATIO(68, 900);
+		default:
+			return SC27XX_VOLT_RATIO(1, 1);
+		}
+	case 15:
+		switch (scale) {
+		case 0:
+			return SC27XX_VOLT_RATIO(1, 3);
+		default:
+			return SC27XX_VOLT_RATIO(1, 1);
+		}
+	case 21:
+	case 22:
+	case 23:
+		switch (scale) {
+		case 0:
+			return SC27XX_VOLT_RATIO(3, 8);
+		default:
+			return SC27XX_VOLT_RATIO(1, 1);
+		}
+	default:
+		switch (scale) {
+		case 0:
+			return SC27XX_VOLT_RATIO(1, 1);
+		case 1:
+			return SC27XX_VOLT_RATIO(1000, 1955);
+		case 2:
+			return SC27XX_VOLT_RATIO(1000, 2600);
+		case 3:
+			return SC27XX_VOLT_RATIO(1000, 4060);
+		default:
+			return SC27XX_VOLT_RATIO(1, 1);
+		}
+	}
+}
+
 /*
  * According to the datasheet set specific value on some channel.
  */
@@ -453,6 +590,22 @@ static void sc2731_adc_scale_init(struct sc27xx_adc_data *data)
 	}
 }
 
+static void ump9620_adc_scale_init(struct sc27xx_adc_data *data)
+{
+	int i;
+
+	for (i = 0; i < SC27XX_ADC_CHANNEL_MAX; i++) {
+		if (i == 10 || i == 19 || i == 30 || i == 31)
+			data->channel_scale[i] = 3;
+		else if (i == 7 || i == 9)
+			data->channel_scale[i] = 2;
+		else if (i == 0 || i == 13)
+			data->channel_scale[i] = 1;
+		else
+			data->channel_scale[i] = 0;
+	}
+}
+
 static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
 			   int scale, int *val)
 {
@@ -578,6 +731,23 @@ static int sc27xx_adc_to_volt(struct sc27xx_adc_linear_graph *graph,
 	return tmp < 0 ? 0 : tmp;
 }
 
+static int ump96xx_adc_to_volt(struct sc27xx_adc_linear_graph *graph, int scale,
+			      int raw_adc)
+{
+	int tmp;
+
+	tmp = (graph->volt0 - graph->volt1) * (raw_adc - graph->adc1);
+	tmp /= (graph->adc0 - graph->adc1);
+	tmp += graph->volt1;
+
+	if (scale == 2)
+		tmp = tmp * 2600 / 1000;
+	else if (scale == 3)
+		tmp = tmp * 4060 / 1000;
+
+	return tmp < 0 ? 0 : tmp;
+}
+
 static int sc27xx_adc_convert_volt(struct sc27xx_adc_data *data, int channel,
 				   int scale, int raw_adc)
 {
@@ -608,6 +778,39 @@ static int sc27xx_adc_convert_volt(struct sc27xx_adc_data *data, int channel,
 	return DIV_ROUND_CLOSEST(volt * denominator, numerator);
 }
 
+static int ump96xx_adc_convert_volt(struct sc27xx_adc_data *data, int channel,
+				   int scale, int raw_adc)
+{
+	u32 numerator, denominator;
+	u32 volt;
+
+	switch (channel) {
+	case 0:
+		if (scale == 1)
+			volt = sc27xx_adc_to_volt(&ump9620_bat_det_graph, raw_adc);
+		else
+			volt = ump96xx_adc_to_volt(&small_scale_graph, scale, raw_adc);
+		break;
+	case 11:
+		volt = sc27xx_adc_to_volt(&big_scale_graph, raw_adc);
+		break;
+	default:
+		if (scale == 1)
+			volt = sc27xx_adc_to_volt(&ump9620_bat_det_graph, raw_adc);
+		else
+			volt = ump96xx_adc_to_volt(&small_scale_graph, scale, raw_adc);
+		break;
+	}
+
+	if (channel == 0 && scale == 1)
+		return volt;
+
+	sc27xx_adc_volt_ratio(data, channel, scale, &numerator, &denominator);
+
+	return DIV_ROUND_CLOSEST(volt * denominator, numerator);
+}
+
+
 static int sc27xx_adc_read_processed(struct sc27xx_adc_data *data,
 				     int channel, int scale, int *val)
 {
@@ -617,7 +820,11 @@ static int sc27xx_adc_read_processed(struct sc27xx_adc_data *data,
 	if (ret)
 		return ret;
 
-	*val = sc27xx_adc_convert_volt(data, channel, scale, raw_adc);
+	if (data->var_data->pmic_type == UMP9620_ADC)
+		*val = ump96xx_adc_convert_volt(data, channel, scale, raw_adc);
+	else
+		*val = sc27xx_adc_convert_volt(data, channel, scale, raw_adc);
+
 	return 0;
 }
 
@@ -735,21 +942,42 @@ static int sc27xx_adc_enable(struct sc27xx_adc_data *data)
 	if (ret)
 		return ret;
 
-	/* Enable ADC work clock and controller clock */
+	/* Enable 26MHz crvstal oscillator wait cycles for UMP9620 ADC */
+	if (data->var_data->pmic_type == UMP9620_ADC) {
+		ret = regmap_update_bits(data->regmap, UMP9620_XTL_WAIT_CTRL0,
+					 UMP9620_XTL_WAIT_CTRL0_EN,
+					 UMP9620_XTL_WAIT_CTRL0_EN);
+	}
+
+	/* Enable ADC work clock */
 	ret = regmap_update_bits(data->regmap, data->var_data->clk_en,
 				 SC27XX_CLK_ADC_EN | SC27XX_CLK_ADC_CLK_EN,
 				 SC27XX_CLK_ADC_EN | SC27XX_CLK_ADC_CLK_EN);
 	if (ret)
 		goto disable_adc;
 
-	/* ADC channel scales' calibration from nvmem device */
-	ret = sc27xx_adc_scale_calibration(data, true);
-	if (ret)
-		goto disable_clk;
+	/* ADC channel scales calibration from nvmem device */
+	if (data->var_data->pmic_type == UMP9620_ADC) {
+		ret = ump96xx_adc_scale_cal(data, UMP96XX_VBAT_SENSES_CAL);
+		if (ret)
+			goto disable_clk;
 
-	ret = sc27xx_adc_scale_calibration(data, false);
-	if (ret)
-		goto disable_clk;
+		ret = ump96xx_adc_scale_cal(data, UMP96XX_VBAT_DET_CAL);
+		if (ret)
+			goto disable_clk;
+
+		ret = ump96xx_adc_scale_cal(data, UMP96XX_CH1_CAL);
+		if (ret)
+			goto disable_clk;
+	} else {
+		ret = sc27xx_adc_scale_calibration(data, true);
+		if (ret)
+			goto disable_clk;
+
+		ret = sc27xx_adc_scale_calibration(data, false);
+		if (ret)
+			goto disable_clk;
+	}
 
 	return 0;
 
@@ -773,6 +1001,10 @@ static void sc27xx_adc_disable(void *_data)
 
 	regmap_update_bits(data->regmap, data->var_data->module_en,
 			   SC27XX_MODULE_ADC_EN, 0);
+
+	if (data->var_data->pmic_type == UMP9620_ADC)
+		regmap_update_bits(data->regmap, UMP9620_XTL_WAIT_CTRL0,
+				UMP9620_XTL_WAIT_CTRL0_EN, 0);
 }
 
 static const struct sc27xx_adc_variant_data sc2731_data = {
@@ -823,6 +1055,18 @@ static const struct sc27xx_adc_variant_data sc2720_data = {
 	.get_ratio = sc2720_adc_get_ratio,
 };
 
+static const struct sc27xx_adc_variant_data ump9620_data = {
+	.pmic_type = UMP9620_ADC,
+	.module_en = UMP9620_MODULE_EN,
+	.clk_en = UMP9620_ARM_CLK_EN,
+	.scale_shift = SC27XX_ADC_SCALE_SHIFT,
+	.scale_mask = SC27XX_ADC_SCALE_MASK,
+	.bscale_cal = &big_scale_graph,
+	.sscale_cal = &small_scale_graph,
+	.init_scale = ump9620_adc_scale_init,
+	.get_ratio = ump9620_adc_get_ratio,
+};
+
 static int sc27xx_adc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -914,6 +1158,7 @@ static const struct of_device_id sc27xx_adc_of_match[] = {
 	{ .compatible = "sprd,sc2730-adc", .data = &sc2730_data},
 	{ .compatible = "sprd,sc2721-adc", .data = &sc2721_data},
 	{ .compatible = "sprd,sc2720-adc", .data = &sc2720_data},
+	{ .compatible = "sprd,ump9620-adc", .data = &ump9620_data},
 	{ }
 };
 MODULE_DEVICE_TABLE(of, sc27xx_adc_of_match);
-- 
2.25.1

Re: [PATCH 6/7] iio: adc: sc27xx: add support for PMIC ump9620

[Baolin Wang]

On Thu, Jan 6, 2022 at 9:00 PM Cixi Geng <gengcixi@gmail.com> wrote:
>
> From: Cixi Geng <cixi.geng1@unisoc.com>
>
> The ump9620 is variant from sc27xx chip, add it in here.
>
> Signed-off-by: Yuming Zhu <yuming.zhu1@unisoc.com>
> Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com>
> ---
>  drivers/iio/adc/sc27xx_adc.c | 263 +++++++++++++++++++++++++++++++++--
>  1 file changed, 254 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c
> index 195f44cf61e1..68b967f32498 100644
> --- a/drivers/iio/adc/sc27xx_adc.c
> +++ b/drivers/iio/adc/sc27xx_adc.c
> @@ -15,12 +15,16 @@
>  /* PMIC global registers definition */
>  #define SC2730_MODULE_EN               0x1808
>  #define SC2731_MODULE_EN               0xc08
> +#define UMP9620_MODULE_EN              0x2008
>  #define SC27XX_MODULE_ADC_EN           BIT(5)
>  #define SC2721_ARM_CLK_EN              0xc0c
>  #define SC2730_ARM_CLK_EN              0x180c
>  #define SC2731_ARM_CLK_EN              0xc10
> +#define UMP9620_ARM_CLK_EN             0x200c
> +#define UMP9620_XTL_WAIT_CTRL0         0x2378
>  #define SC27XX_CLK_ADC_EN              BIT(5)
>  #define SC27XX_CLK_ADC_CLK_EN          BIT(6)
> +#define UMP9620_XTL_WAIT_CTRL0_EN      BIT(8)
>
>  /* ADC controller registers definition */
>  #define SC27XX_ADC_CTL                 0x0
> @@ -82,6 +86,13 @@
>  enum sc27xx_pmic_type {
>         SC27XX_ADC,
>         SC2721_ADC,
> +       UMP9620_ADC,
> +};
> +
> +enum ump96xx_scale_cal {
> +       UMP96XX_VBAT_SENSES_CAL,
> +       UMP96XX_VBAT_DET_CAL,
> +       UMP96XX_CH1_CAL,
>  };
>
>  struct sc27xx_adc_data {
> @@ -140,6 +151,11 @@ static struct sc27xx_adc_linear_graph small_scale_graph = {
>         100, 341,
>  };
>
> +static struct sc27xx_adc_linear_graph ump9620_bat_det_graph = {
> +       1400, 3482,
> +       200, 476,
> +};
> +
>  static const struct sc27xx_adc_linear_graph sc2731_big_scale_graph_calib = {
>         4200, 850,
>         3600, 728,
> @@ -165,6 +181,33 @@ static int sc27xx_adc_get_calib_data(u32 calib_data, int calib_adc)
>         return ((calib_data & 0xff) + calib_adc - 128) * 4;
>  }
>
> +static int adc_nvmem_cell_calib_data(struct sc27xx_adc_data *data, const char *cell_name)
> +{
> +       struct nvmem_cell *cell;
> +       void *buf;
> +       u32 calib_data = 0;
> +       size_t len = 0;
> +
> +       if (!data)
> +               return -EINVAL;
> +
> +       cell = nvmem_cell_get(data->dev, cell_name);
> +       if (IS_ERR_OR_NULL(cell))
> +               return PTR_ERR(cell);
> +
> +       buf = nvmem_cell_read(cell, &len);
> +       if (IS_ERR_OR_NULL(buf)) {
> +               nvmem_cell_put(cell);
> +               return PTR_ERR(buf);
> +       }
> +
> +       memcpy(&calib_data, buf, min(len, sizeof(u32)));
> +
> +       kfree(buf);
> +       nvmem_cell_put(cell);
> +       return calib_data;
> +}

These are some duplicated code in sc27xx_adc_scale_calibration(),
please factor out the sc27xx_adc_scale_calibration() firstly.

> +
>  static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data,
>                                         bool big_scale)
>  {
> @@ -207,6 +250,56 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data,
>         return 0;
>  }
>
> +static int ump96xx_adc_scale_cal(struct sc27xx_adc_data *data,
> +                                       enum ump96xx_scale_cal cal_type)
> +{
> +       struct sc27xx_adc_linear_graph *graph = NULL;
> +       const char *cell_name1 = NULL, *cell_name2 = NULL;
> +       int adc_calib_data1 = 0, adc_calib_data2 = 0;
> +
> +       if (!data)
> +               return -EINVAL;
> +
> +       if (cal_type == UMP96XX_VBAT_DET_CAL) {
> +               graph = &ump9620_bat_det_graph;
> +               cell_name1 = "vbat_det_cal1";
> +               cell_name2 = "vbat_det_cal2";
> +       } else if (cal_type == UMP96XX_VBAT_SENSES_CAL) {
> +               graph = &big_scale_graph;
> +               cell_name1 = "big_scale_calib1";
> +               cell_name2 = "big_scale_calib2";
> +       } else if (cal_type == UMP96XX_CH1_CAL) {
> +               graph = &small_scale_graph;
> +               cell_name1 = "small_scale_calib1";
> +               cell_name2 = "small_scale_calib2";
> +       } else {
> +               graph = &small_scale_graph;
> +               cell_name1 = "small_scale_calib1";
> +               cell_name2 = "small_scale_calib2";
> +       }
> +
> +       adc_calib_data1 = adc_nvmem_cell_calib_data(data, cell_name1);
> +       if (adc_calib_data1 < 0) {
> +               dev_err(data->dev, "err! %s:%d\n", cell_name1, adc_calib_data1);
> +               return adc_calib_data1;
> +       }
> +
> +       adc_calib_data2 = adc_nvmem_cell_calib_data(data, cell_name2);
> +       if (adc_calib_data2 < 0) {
> +               dev_err(data->dev, "err! %s:%d\n", cell_name2, adc_calib_data2);
> +               return adc_calib_data2;
> +       }
> +
> +       /*
> +        *Read the data in the two blocks of efuse and convert them into the
> +        *calibration value in the ump9620 adc linear graph.
> +        */
> +       graph->adc0 = (adc_calib_data1 & 0xfff0) >> 4;
> +       graph->adc1 = (adc_calib_data2 & 0xfff0) >> 4;
> +
> +       return 0;
> +}
> +
>  static int sc2720_adc_get_ratio(int channel, int scale)
>  {
>         switch (channel) {
> @@ -394,6 +487,50 @@ static int sc2731_adc_get_ratio(int channel, int scale)
>         return SC27XX_VOLT_RATIO(1, 1);
>  }
>
> +static int ump9620_adc_get_ratio(int channel, int scale)
> +{
> +       switch (channel) {
> +       case 11:
> +               return SC27XX_VOLT_RATIO(1, 1);
> +       case 14:
> +               switch (scale) {
> +               case 0:
> +                       return SC27XX_VOLT_RATIO(68, 900);
> +               default:
> +                       return SC27XX_VOLT_RATIO(1, 1);
> +               }
> +       case 15:
> +               switch (scale) {
> +               case 0:
> +                       return SC27XX_VOLT_RATIO(1, 3);
> +               default:
> +                       return SC27XX_VOLT_RATIO(1, 1);
> +               }
> +       case 21:
> +       case 22:
> +       case 23:
> +               switch (scale) {
> +               case 0:
> +                       return SC27XX_VOLT_RATIO(3, 8);
> +               default:
> +                       return SC27XX_VOLT_RATIO(1, 1);
> +               }
> +       default:
> +               switch (scale) {
> +               case 0:
> +                       return SC27XX_VOLT_RATIO(1, 1);
> +               case 1:
> +                       return SC27XX_VOLT_RATIO(1000, 1955);
> +               case 2:
> +                       return SC27XX_VOLT_RATIO(1000, 2600);
> +               case 3:
> +                       return SC27XX_VOLT_RATIO(1000, 4060);
> +               default:
> +                       return SC27XX_VOLT_RATIO(1, 1);
> +               }
> +       }
> +}
> +
>  /*
>   * According to the datasheet set specific value on some channel.
>   */
> @@ -453,6 +590,22 @@ static void sc2731_adc_scale_init(struct sc27xx_adc_data *data)
>         }
>  }
>
> +static void ump9620_adc_scale_init(struct sc27xx_adc_data *data)
> +{
> +       int i;
> +
> +       for (i = 0; i < SC27XX_ADC_CHANNEL_MAX; i++) {
> +               if (i == 10 || i == 19 || i == 30 || i == 31)
> +                       data->channel_scale[i] = 3;
> +               else if (i == 7 || i == 9)
> +                       data->channel_scale[i] = 2;
> +               else if (i == 0 || i == 13)
> +                       data->channel_scale[i] = 1;
> +               else
> +                       data->channel_scale[i] = 0;
> +       }
> +}
> +
>  static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
>                            int scale, int *val)
>  {
> @@ -578,6 +731,23 @@ static int sc27xx_adc_to_volt(struct sc27xx_adc_linear_graph *graph,
>         return tmp < 0 ? 0 : tmp;
>  }
>
> +static int ump96xx_adc_to_volt(struct sc27xx_adc_linear_graph *graph, int scale,
> +                             int raw_adc)
> +{
> +       int tmp;
> +
> +       tmp = (graph->volt0 - graph->volt1) * (raw_adc - graph->adc1);
> +       tmp /= (graph->adc0 - graph->adc1);
> +       tmp += graph->volt1;

These are also copy-paste from sc27xx_adc_to_volt(), please avoid
duplicate code.

> +
> +       if (scale == 2)
> +               tmp = tmp * 2600 / 1000;
> +       else if (scale == 3)
> +               tmp = tmp * 4060 / 1000;
> +
> +       return tmp < 0 ? 0 : tmp;
> +}
> +
>  static int sc27xx_adc_convert_volt(struct sc27xx_adc_data *data, int channel,
>                                    int scale, int raw_adc)
>  {
> @@ -608,6 +778,39 @@ static int sc27xx_adc_convert_volt(struct sc27xx_adc_data *data, int channel,
>         return DIV_ROUND_CLOSEST(volt * denominator, numerator);
>  }
>
> +static int ump96xx_adc_convert_volt(struct sc27xx_adc_data *data, int channel,
> +                                  int scale, int raw_adc)
> +{
> +       u32 numerator, denominator;
> +       u32 volt;
> +
> +       switch (channel) {
> +       case 0:
> +               if (scale == 1)
> +                       volt = sc27xx_adc_to_volt(&ump9620_bat_det_graph, raw_adc);
> +               else
> +                       volt = ump96xx_adc_to_volt(&small_scale_graph, scale, raw_adc);
> +               break;
> +       case 11:
> +               volt = sc27xx_adc_to_volt(&big_scale_graph, raw_adc);
> +               break;
> +       default:
> +               if (scale == 1)
> +                       volt = sc27xx_adc_to_volt(&ump9620_bat_det_graph, raw_adc);
> +               else
> +                       volt = ump96xx_adc_to_volt(&small_scale_graph, scale, raw_adc);
> +               break;
> +       }
> +
> +       if (channel == 0 && scale == 1)
> +               return volt;
> +
> +       sc27xx_adc_volt_ratio(data, channel, scale, &numerator, &denominator);
> +
> +       return DIV_ROUND_CLOSEST(volt * denominator, numerator);
> +}
> +
> +
>  static int sc27xx_adc_read_processed(struct sc27xx_adc_data *data,
>                                      int channel, int scale, int *val)
>  {
> @@ -617,7 +820,11 @@ static int sc27xx_adc_read_processed(struct sc27xx_adc_data *data,
>         if (ret)
>                 return ret;
>
> -       *val = sc27xx_adc_convert_volt(data, channel, scale, raw_adc);
> +       if (data->var_data->pmic_type == UMP9620_ADC)
> +               *val = ump96xx_adc_convert_volt(data, channel, scale, raw_adc);
> +       else
> +               *val = sc27xx_adc_convert_volt(data, channel, scale, raw_adc);
> +
>         return 0;
>  }
>
> @@ -735,21 +942,42 @@ static int sc27xx_adc_enable(struct sc27xx_adc_data *data)
>         if (ret)
>                 return ret;
>
> -       /* Enable ADC work clock and controller clock */
> +       /* Enable 26MHz crvstal oscillator wait cycles for UMP9620 ADC */
> +       if (data->var_data->pmic_type == UMP9620_ADC) {
> +               ret = regmap_update_bits(data->regmap, UMP9620_XTL_WAIT_CTRL0,
> +                                        UMP9620_XTL_WAIT_CTRL0_EN,
> +                                        UMP9620_XTL_WAIT_CTRL0_EN);
> +       }
> +
> +       /* Enable ADC work clock */
>         ret = regmap_update_bits(data->regmap, data->var_data->clk_en,
>                                  SC27XX_CLK_ADC_EN | SC27XX_CLK_ADC_CLK_EN,
>                                  SC27XX_CLK_ADC_EN | SC27XX_CLK_ADC_CLK_EN);
>         if (ret)
>                 goto disable_adc;
>
> -       /* ADC channel scales' calibration from nvmem device */
> -       ret = sc27xx_adc_scale_calibration(data, true);
> -       if (ret)
> -               goto disable_clk;
> +       /* ADC channel scales calibration from nvmem device */
> +       if (data->var_data->pmic_type == UMP9620_ADC) {
> +               ret = ump96xx_adc_scale_cal(data, UMP96XX_VBAT_SENSES_CAL);
> +               if (ret)
> +                       goto disable_clk;
>
> -       ret = sc27xx_adc_scale_calibration(data, false);
> -       if (ret)
> -               goto disable_clk;
> +               ret = ump96xx_adc_scale_cal(data, UMP96XX_VBAT_DET_CAL);
> +               if (ret)
> +                       goto disable_clk;
> +
> +               ret = ump96xx_adc_scale_cal(data, UMP96XX_CH1_CAL);
> +               if (ret)
> +                       goto disable_clk;
> +       } else {
> +               ret = sc27xx_adc_scale_calibration(data, true);
> +               if (ret)
> +                       goto disable_clk;
> +
> +               ret = sc27xx_adc_scale_calibration(data, false);
> +               if (ret)
> +                       goto disable_clk;
> +       }
>
>         return 0;
>
> @@ -773,6 +1001,10 @@ static void sc27xx_adc_disable(void *_data)
>
>         regmap_update_bits(data->regmap, data->var_data->module_en,
>                            SC27XX_MODULE_ADC_EN, 0);
> +
> +       if (data->var_data->pmic_type == UMP9620_ADC)
> +               regmap_update_bits(data->regmap, UMP9620_XTL_WAIT_CTRL0,
> +                               UMP9620_XTL_WAIT_CTRL0_EN, 0);
>  }
>
>  static const struct sc27xx_adc_variant_data sc2731_data = {
> @@ -823,6 +1055,18 @@ static const struct sc27xx_adc_variant_data sc2720_data = {
>         .get_ratio = sc2720_adc_get_ratio,
>  };
>
> +static const struct sc27xx_adc_variant_data ump9620_data = {
> +       .pmic_type = UMP9620_ADC,
> +       .module_en = UMP9620_MODULE_EN,
> +       .clk_en = UMP9620_ARM_CLK_EN,
> +       .scale_shift = SC27XX_ADC_SCALE_SHIFT,
> +       .scale_mask = SC27XX_ADC_SCALE_MASK,
> +       .bscale_cal = &big_scale_graph,
> +       .sscale_cal = &small_scale_graph,
> +       .init_scale = ump9620_adc_scale_init,
> +       .get_ratio = ump9620_adc_get_ratio,
> +};
> +
>  static int sc27xx_adc_probe(struct platform_device *pdev)
>  {
>         struct device *dev = &pdev->dev;
> @@ -914,6 +1158,7 @@ static const struct of_device_id sc27xx_adc_of_match[] = {
>         { .compatible = "sprd,sc2730-adc", .data = &sc2730_data},
>         { .compatible = "sprd,sc2721-adc", .data = &sc2721_data},
>         { .compatible = "sprd,sc2720-adc", .data = &sc2720_data},
> +       { .compatible = "sprd,ump9620-adc", .data = &ump9620_data},
>         { }
>  };
>  MODULE_DEVICE_TABLE(of, sc27xx_adc_of_match);
> --
> 2.25.1
>


-- 
Baolin Wang

[PATCH 7/7] iio: adc: sc27xx: add Ump9620 ADC suspend and resume pm support

[Cixi Geng]

From: Cixi Geng <cixi.geng1@unisoc.com>

Ump9620 ADC suspend and resume pm optimization, configuration
0x6490_ 0350(PAD_ CLK26M_ SINOUT_ PMIC_ 1P8 ) bit 8.

Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com>
Signed-off-by: Yuming Zhu <yuming.zhu1@unisoc.com>
---
 drivers/iio/adc/sc27xx_adc.c | 103 ++++++++++++++++++++++++++++++++++-
 1 file changed, 102 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c
index 68b967f32498..cecda8d53474 100644
--- a/drivers/iio/adc/sc27xx_adc.c
+++ b/drivers/iio/adc/sc27xx_adc.c
@@ -11,6 +11,7 @@
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
+#include <linux/pm_runtime.h>
 
 /* PMIC global registers definition */
 #define SC2730_MODULE_EN		0x1808
@@ -83,6 +84,9 @@
 /* ADC default channel reference voltage is 2.8V */
 #define SC27XX_ADC_REFVOL_VDD28		2800000
 
+/* 10s delay before suspending the ADC IP */
+#define SC27XX_ADC_AUTOSUSPEND_DELAY	10000
+
 enum sc27xx_pmic_type {
 	SC27XX_ADC,
 	SC2721_ADC,
@@ -618,6 +622,9 @@ static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
 		return ret;
 	}
 
+	if (data->var_data->pmic_type == UMP9620_ADC)
+		pm_runtime_get_sync(data->indio_dev->dev.parent);
+
 	/*
 	 * According to the sc2721 chip data sheet, the reference voltage of
 	 * specific channel 30 and channel 31 in ADC module needs to be set from
@@ -700,6 +707,11 @@ static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
 		}
 	}
 
+	if (data->var_data->pmic_type == UMP9620_ADC) {
+		pm_runtime_mark_last_busy(data->indio_dev->dev.parent);
+		pm_runtime_put_autosuspend(data->indio_dev->dev.parent);
+	}
+
 	hwspin_unlock_raw(data->hwlock);
 
 	if (!ret)
@@ -947,6 +959,10 @@ static int sc27xx_adc_enable(struct sc27xx_adc_data *data)
 		ret = regmap_update_bits(data->regmap, UMP9620_XTL_WAIT_CTRL0,
 					 UMP9620_XTL_WAIT_CTRL0_EN,
 					 UMP9620_XTL_WAIT_CTRL0_EN);
+		if (ret) {
+			dev_err(data->dev, "failed to set the UMP9620 ADC clk26m bit8 on IP\n");
+			goto clean_adc_clk26m_bit8;
+		}
 	}
 
 	/* Enable ADC work clock */
@@ -988,6 +1004,11 @@ static int sc27xx_adc_enable(struct sc27xx_adc_data *data)
 	regmap_update_bits(data->regmap, data->var_data->module_en,
 			   SC27XX_MODULE_ADC_EN, 0);
 
+clean_adc_clk26m_bit8:
+	if (data->var_data->pmic_type == UMP9620_ADC)
+		regmap_update_bits(data->regmap, UMP9620_XTL_WAIT_CTRL0,
+				UMP9620_XTL_WAIT_CTRL0_EN, 0);
+
 	return ret;
 }
 
@@ -1086,6 +1107,8 @@ static int sc27xx_adc_probe(struct platform_device *pdev)
 	if (!indio_dev)
 		return -ENOMEM;
 
+	platform_set_drvdata(pdev, indio_dev);
+
 	sc27xx_data = iio_priv(indio_dev);
 
 	sc27xx_data->regmap = dev_get_regmap(dev->parent, NULL);
@@ -1126,7 +1149,10 @@ static int sc27xx_adc_probe(struct platform_device *pdev)
 		}
 	}
 
+	sc27xx_data->dev = dev;
 	sc27xx_data->var_data = pdata;
+	sc27xx_data->indio_dev = indio_dev;
+
 	sc27xx_data->var_data->init_scale(sc27xx_data);
 
 	ret = sc27xx_adc_enable(sc27xx_data);
@@ -1137,18 +1163,39 @@ static int sc27xx_adc_probe(struct platform_device *pdev)
 
 	ret = devm_add_action_or_reset(dev, sc27xx_adc_disable, sc27xx_data);
 	if (ret) {
+		sc27xx_adc_disable(sc27xx_data);
 		dev_err(dev, "failed to add ADC disable action\n");
 		return ret;
 	}
 
+	indio_dev->dev.parent = dev;
 	indio_dev->name = dev_name(dev);
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &sc27xx_info;
 	indio_dev->channels = sc27xx_channels;
 	indio_dev->num_channels = ARRAY_SIZE(sc27xx_channels);
+
+	if (sc27xx_data->var_data->pmic_type == UMP9620_ADC) {
+		pm_runtime_set_autosuspend_delay(dev,
+						 SC27XX_ADC_AUTOSUSPEND_DELAY);
+		pm_runtime_use_autosuspend(dev);
+		pm_runtime_set_suspended(dev);
+		pm_runtime_enable(dev);
+	}
+
 	ret = devm_iio_device_register(dev, indio_dev);
-	if (ret)
+	if (ret) {
 		dev_err(dev, "could not register iio (ADC)");
+		goto err_iio_register;
+	}
+
+	return 0;
+
+err_iio_register:
+	if (sc27xx_data->var_data->pmic_type == UMP9620_ADC) {
+		pm_runtime_put(dev);
+		pm_runtime_disable(dev);
+	}
 
 	return ret;
 }
@@ -1163,11 +1210,65 @@ static const struct of_device_id sc27xx_adc_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, sc27xx_adc_of_match);
 
+static int sc27xx_adc_remove(struct platform_device *pdev)
+{
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+	struct sc27xx_adc_data *sc27xx_data = iio_priv(indio_dev);
+
+	if (sc27xx_data->var_data->pmic_type == UMP9620_ADC) {
+		pm_runtime_put(&pdev->dev);
+		pm_runtime_disable(&pdev->dev);
+
+		/* set the UMP9620 ADC clk26m bit8 on IP */
+		regmap_update_bits(sc27xx_data->regmap, UMP9620_XTL_WAIT_CTRL0,
+				UMP9620_XTL_WAIT_CTRL0_EN, 0);
+	}
+
+	return 0;
+}
+
+static int sc27xx_adc_runtime_suspend(struct device *dev)
+{
+	struct sc27xx_adc_data *sc27xx_data = iio_priv(dev_get_drvdata(dev));
+
+	/* clean the UMP9620 ADC clk26m bit8 on IP */
+	if (sc27xx_data->var_data->pmic_type == UMP9620_ADC)
+		regmap_update_bits(sc27xx_data->regmap, UMP9620_XTL_WAIT_CTRL0,
+				UMP9620_XTL_WAIT_CTRL0_EN, 0);
+
+	return 0;
+}
+
+static int sc27xx_adc_runtime_resume(struct device *dev)
+{
+	int ret = 0;
+	struct sc27xx_adc_data *sc27xx_data = iio_priv(dev_get_drvdata(dev));
+
+	/* set the UMP9620 ADC clk26m bit8 on IP */
+	if (sc27xx_data->var_data->pmic_type == UMP9620_ADC) {
+		ret = regmap_update_bits(sc27xx_data->regmap, UMP9620_XTL_WAIT_CTRL0,
+				UMP9620_XTL_WAIT_CTRL0_EN, UMP9620_XTL_WAIT_CTRL0_EN);
+		if (ret) {
+			dev_err(dev, "failed to set the UMP9620 ADC clk26m bit8 on IP\n");
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static const struct dev_pm_ops sc27xx_adc_pm_ops = {
+	.runtime_suspend = &sc27xx_adc_runtime_suspend,
+	.runtime_resume = &sc27xx_adc_runtime_resume,
+};
+
 static struct platform_driver sc27xx_adc_driver = {
 	.probe = sc27xx_adc_probe,
+	.remove = sc27xx_adc_remove,
 	.driver = {
 		.name = "sc27xx-adc",
 		.of_match_table = sc27xx_adc_of_match,
+		.pm	= &sc27xx_adc_pm_ops,
 	},
 };
 
-- 
2.25.1

Re: [PATCH 7/7] iio: adc: sc27xx: add Ump9620 ADC suspend and resume pm support

[Baolin Wang]

On Thu, Jan 6, 2022 at 9:00 PM Cixi Geng <gengcixi@gmail.com> wrote:
>
> From: Cixi Geng <cixi.geng1@unisoc.com>
>
> Ump9620 ADC suspend and resume pm optimization, configuration
> 0x6490_ 0350(PAD_ CLK26M_ SINOUT_ PMIC_ 1P8 ) bit 8.
>
> Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com>
> Signed-off-by: Yuming Zhu <yuming.zhu1@unisoc.com>
> ---
>  drivers/iio/adc/sc27xx_adc.c | 103 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 102 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c
> index 68b967f32498..cecda8d53474 100644
> --- a/drivers/iio/adc/sc27xx_adc.c
> +++ b/drivers/iio/adc/sc27xx_adc.c
> @@ -11,6 +11,7 @@
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
> +#include <linux/pm_runtime.h>
>
>  /* PMIC global registers definition */
>  #define SC2730_MODULE_EN               0x1808
> @@ -83,6 +84,9 @@
>  /* ADC default channel reference voltage is 2.8V */
>  #define SC27XX_ADC_REFVOL_VDD28                2800000
>
> +/* 10s delay before suspending the ADC IP */
> +#define SC27XX_ADC_AUTOSUSPEND_DELAY   10000
> +
>  enum sc27xx_pmic_type {
>         SC27XX_ADC,
>         SC2721_ADC,
> @@ -618,6 +622,9 @@ static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
>                 return ret;
>         }
>
> +       if (data->var_data->pmic_type == UMP9620_ADC)
> +               pm_runtime_get_sync(data->indio_dev->dev.parent);
> +
>         /*
>          * According to the sc2721 chip data sheet, the reference voltage of
>          * specific channel 30 and channel 31 in ADC module needs to be set from
> @@ -700,6 +707,11 @@ static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
>                 }
>         }
>
> +       if (data->var_data->pmic_type == UMP9620_ADC) {
> +               pm_runtime_mark_last_busy(data->indio_dev->dev.parent);
> +               pm_runtime_put_autosuspend(data->indio_dev->dev.parent);
> +       }
> +
>         hwspin_unlock_raw(data->hwlock);
>
>         if (!ret)
> @@ -947,6 +959,10 @@ static int sc27xx_adc_enable(struct sc27xx_adc_data *data)
>                 ret = regmap_update_bits(data->regmap, UMP9620_XTL_WAIT_CTRL0,
>                                          UMP9620_XTL_WAIT_CTRL0_EN,
>                                          UMP9620_XTL_WAIT_CTRL0_EN);
> +               if (ret) {
> +                       dev_err(data->dev, "failed to set the UMP9620 ADC clk26m bit8 on IP\n");
> +                       goto clean_adc_clk26m_bit8;
> +               }
>         }
>
>         /* Enable ADC work clock */
> @@ -988,6 +1004,11 @@ static int sc27xx_adc_enable(struct sc27xx_adc_data *data)
>         regmap_update_bits(data->regmap, data->var_data->module_en,
>                            SC27XX_MODULE_ADC_EN, 0);
>
> +clean_adc_clk26m_bit8:
> +       if (data->var_data->pmic_type == UMP9620_ADC)
> +               regmap_update_bits(data->regmap, UMP9620_XTL_WAIT_CTRL0,
> +                               UMP9620_XTL_WAIT_CTRL0_EN, 0);

Can you hide this into the pm runtime callbacks?

> +
>         return ret;
>  }
>
> @@ -1086,6 +1107,8 @@ static int sc27xx_adc_probe(struct platform_device *pdev)
>         if (!indio_dev)
>                 return -ENOMEM;
>
> +       platform_set_drvdata(pdev, indio_dev);
> +
>         sc27xx_data = iio_priv(indio_dev);
>
>         sc27xx_data->regmap = dev_get_regmap(dev->parent, NULL);
> @@ -1126,7 +1149,10 @@ static int sc27xx_adc_probe(struct platform_device *pdev)
>                 }
>         }
>
> +       sc27xx_data->dev = dev;
>         sc27xx_data->var_data = pdata;
> +       sc27xx_data->indio_dev = indio_dev;
> +
>         sc27xx_data->var_data->init_scale(sc27xx_data);
>
>         ret = sc27xx_adc_enable(sc27xx_data);
> @@ -1137,18 +1163,39 @@ static int sc27xx_adc_probe(struct platform_device *pdev)
>
>         ret = devm_add_action_or_reset(dev, sc27xx_adc_disable, sc27xx_data);
>         if (ret) {
> +               sc27xx_adc_disable(sc27xx_data);
>                 dev_err(dev, "failed to add ADC disable action\n");
>                 return ret;
>         }
>
> +       indio_dev->dev.parent = dev;
>         indio_dev->name = dev_name(dev);
>         indio_dev->modes = INDIO_DIRECT_MODE;
>         indio_dev->info = &sc27xx_info;
>         indio_dev->channels = sc27xx_channels;
>         indio_dev->num_channels = ARRAY_SIZE(sc27xx_channels);
> +
> +       if (sc27xx_data->var_data->pmic_type == UMP9620_ADC) {
> +               pm_runtime_set_autosuspend_delay(dev,
> +                                                SC27XX_ADC_AUTOSUSPEND_DELAY);
> +               pm_runtime_use_autosuspend(dev);
> +               pm_runtime_set_suspended(dev);
> +               pm_runtime_enable(dev);
> +       }
> +
>         ret = devm_iio_device_register(dev, indio_dev);
> -       if (ret)
> +       if (ret) {
>                 dev_err(dev, "could not register iio (ADC)");
> +               goto err_iio_register;
> +       }
> +
> +       return 0;
> +
> +err_iio_register:
> +       if (sc27xx_data->var_data->pmic_type == UMP9620_ADC) {
> +               pm_runtime_put(dev);

I don't think the pm_runtime_put() is needed, since you did not get
the counter before, right?

> +               pm_runtime_disable(dev);
> +       }
>
>         return ret;
>  }
> @@ -1163,11 +1210,65 @@ static const struct of_device_id sc27xx_adc_of_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, sc27xx_adc_of_match);
>
> +static int sc27xx_adc_remove(struct platform_device *pdev)
> +{
> +       struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +       struct sc27xx_adc_data *sc27xx_data = iio_priv(indio_dev);
> +
> +       if (sc27xx_data->var_data->pmic_type == UMP9620_ADC) {
> +               pm_runtime_put(&pdev->dev);

You did not get the pm count, why put it firstly?

> +               pm_runtime_disable(&pdev->dev);
> +
> +               /* set the UMP9620 ADC clk26m bit8 on IP */
> +               regmap_update_bits(sc27xx_data->regmap, UMP9620_XTL_WAIT_CTRL0,
> +                               UMP9620_XTL_WAIT_CTRL0_EN, 0);
> +       }
> +
> +       return 0;
> +}
> +
> +static int sc27xx_adc_runtime_suspend(struct device *dev)
> +{
> +       struct sc27xx_adc_data *sc27xx_data = iio_priv(dev_get_drvdata(dev));
> +
> +       /* clean the UMP9620 ADC clk26m bit8 on IP */
> +       if (sc27xx_data->var_data->pmic_type == UMP9620_ADC)
> +               regmap_update_bits(sc27xx_data->regmap, UMP9620_XTL_WAIT_CTRL0,
> +                               UMP9620_XTL_WAIT_CTRL0_EN, 0);
> +
> +       return 0;
> +}
> +
> +static int sc27xx_adc_runtime_resume(struct device *dev)
> +{
> +       int ret = 0;

no need to initialize it.

> +       struct sc27xx_adc_data *sc27xx_data = iio_priv(dev_get_drvdata(dev));
> +
> +       /* set the UMP9620 ADC clk26m bit8 on IP */
> +       if (sc27xx_data->var_data->pmic_type == UMP9620_ADC) {
> +               ret = regmap_update_bits(sc27xx_data->regmap, UMP9620_XTL_WAIT_CTRL0,
> +                               UMP9620_XTL_WAIT_CTRL0_EN, UMP9620_XTL_WAIT_CTRL0_EN);
> +               if (ret) {
> +                       dev_err(dev, "failed to set the UMP9620 ADC clk26m bit8 on IP\n");
> +                       return ret;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
> +static const struct dev_pm_ops sc27xx_adc_pm_ops = {
> +       .runtime_suspend = &sc27xx_adc_runtime_suspend,
> +       .runtime_resume = &sc27xx_adc_runtime_resume,
> +};

Please use SET_RUNTIME_PM_OPS macro.

> +
>  static struct platform_driver sc27xx_adc_driver = {
>         .probe = sc27xx_adc_probe,
> +       .remove = sc27xx_adc_remove,
>         .driver = {
>                 .name = "sc27xx-adc",
>                 .of_match_table = sc27xx_adc_of_match,
> +               .pm     = &sc27xx_adc_pm_ops,
>         },
>  };
>
> --
> 2.25.1
>


-- 
Baolin Wang

Re: [PATCH 7/7] iio: adc: sc27xx: add Ump9620 ADC suspend and resume pm support

[Jonathan Cameron]

On Fri, 7 Jan 2022 15:34:32 +0800
Baolin Wang <baolin.wang7@gmail.com> wrote:

> On Thu, Jan 6, 2022 at 9:00 PM Cixi Geng <gengcixi@gmail.com> wrote:
> >
> > From: Cixi Geng <cixi.geng1@unisoc.com>
> >
> > Ump9620 ADC suspend and resume pm optimization, configuration
> > 0x6490_ 0350(PAD_ CLK26M_ SINOUT_ PMIC_ 1P8 ) bit 8.
> >
> > Signed-off-by: Cixi Geng <cixi.geng1@unisoc.com>
> > Signed-off-by: Yuming Zhu <yuming.zhu1@unisoc.com>
A few additional comments from me inline,

Thanks,

Jonathan

> > ---
> >  drivers/iio/adc/sc27xx_adc.c | 103 ++++++++++++++++++++++++++++++++++-
> >  1 file changed, 102 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c
> > index 68b967f32498..cecda8d53474 100644
> > --- a/drivers/iio/adc/sc27xx_adc.c
> > +++ b/drivers/iio/adc/sc27xx_adc.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/regmap.h>
> >  #include <linux/regulator/consumer.h>
> >  #include <linux/slab.h>
> > +#include <linux/pm_runtime.h>
> >
> >  /* PMIC global registers definition */
> >  #define SC2730_MODULE_EN               0x1808
> > @@ -83,6 +84,9 @@
> >  /* ADC default channel reference voltage is 2.8V */
> >  #define SC27XX_ADC_REFVOL_VDD28                2800000
> >
> > +/* 10s delay before suspending the ADC IP */
> > +#define SC27XX_ADC_AUTOSUSPEND_DELAY   10000
> > +
> >  enum sc27xx_pmic_type {
> >         SC27XX_ADC,
> >         SC2721_ADC,
> > @@ -618,6 +622,9 @@ static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
> >                 return ret;
> >         }
> >
> > +       if (data->var_data->pmic_type == UMP9620_ADC)
> > +               pm_runtime_get_sync(data->indio_dev->dev.parent);
> > +
> >         /*
> >          * According to the sc2721 chip data sheet, the reference voltage of
> >          * specific channel 30 and channel 31 in ADC module needs to be set from
> > @@ -700,6 +707,11 @@ static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
> >                 }
> >         }
> >
> > +       if (data->var_data->pmic_type == UMP9620_ADC) {
> > +               pm_runtime_mark_last_busy(data->indio_dev->dev.parent);
> > +               pm_runtime_put_autosuspend(data->indio_dev->dev.parent);
> > +       }
> > +
> >         hwspin_unlock_raw(data->hwlock);
> >
> >         if (!ret)
> > @@ -947,6 +959,10 @@ static int sc27xx_adc_enable(struct sc27xx_adc_data *data)
> >                 ret = regmap_update_bits(data->regmap, UMP9620_XTL_WAIT_CTRL0,
> >                                          UMP9620_XTL_WAIT_CTRL0_EN,
> >                                          UMP9620_XTL_WAIT_CTRL0_EN);
> > +               if (ret) {
> > +                       dev_err(data->dev, "failed to set the UMP9620 ADC clk26m bit8 on IP\n");
> > +                       goto clean_adc_clk26m_bit8;
> > +               }
> >         }
> >
> >         /* Enable ADC work clock */
> > @@ -988,6 +1004,11 @@ static int sc27xx_adc_enable(struct sc27xx_adc_data *data)
> >         regmap_update_bits(data->regmap, data->var_data->module_en,
> >                            SC27XX_MODULE_ADC_EN, 0);
> >
> > +clean_adc_clk26m_bit8:
> > +       if (data->var_data->pmic_type == UMP9620_ADC)
> > +               regmap_update_bits(data->regmap, UMP9620_XTL_WAIT_CTRL0,
> > +                               UMP9620_XTL_WAIT_CTRL0_EN, 0);  
> 
> Can you hide this into the pm runtime callbacks?
> 
> > +
> >         return ret;
> >  }
> >
> > @@ -1086,6 +1107,8 @@ static int sc27xx_adc_probe(struct platform_device *pdev)
> >         if (!indio_dev)
> >                 return -ENOMEM;
> >
> > +       platform_set_drvdata(pdev, indio_dev);
> > +
> >         sc27xx_data = iio_priv(indio_dev);
> >
> >         sc27xx_data->regmap = dev_get_regmap(dev->parent, NULL);
> > @@ -1126,7 +1149,10 @@ static int sc27xx_adc_probe(struct platform_device *pdev)
> >                 }
> >         }
> >
> > +       sc27xx_data->dev = dev;
> >         sc27xx_data->var_data = pdata;
> > +       sc27xx_data->indio_dev = indio_dev;
> > +
> >         sc27xx_data->var_data->init_scale(sc27xx_data);
> >
> >         ret = sc27xx_adc_enable(sc27xx_data);
> > @@ -1137,18 +1163,39 @@ static int sc27xx_adc_probe(struct platform_device *pdev)
> >
> >         ret = devm_add_action_or_reset(dev, sc27xx_adc_disable, sc27xx_data);
> >         if (ret) {
> > +               sc27xx_adc_disable(sc27xx_data);

No. That's what the _or_reset() bit of the above call is about. It will have already
called this if the devm registration failed.

> >                 dev_err(dev, "failed to add ADC disable action\n");
> >                 return ret;
> >         }
> >
> > +       indio_dev->dev.parent = dev;
> >         indio_dev->name = dev_name(dev);
> >         indio_dev->modes = INDIO_DIRECT_MODE;
> >         indio_dev->info = &sc27xx_info;
> >         indio_dev->channels = sc27xx_channels;
> >         indio_dev->num_channels = ARRAY_SIZE(sc27xx_channels);
> > +
> > +       if (sc27xx_data->var_data->pmic_type == UMP9620_ADC) {
> > +               pm_runtime_set_autosuspend_delay(dev,
> > +                                                SC27XX_ADC_AUTOSUSPEND_DELAY);
> > +               pm_runtime_use_autosuspend(dev);
> > +               pm_runtime_set_suspended(dev);
> > +               pm_runtime_enable(dev);
> > +       }
> > +
> >         ret = devm_iio_device_register(dev, indio_dev);
> > -       if (ret)
> > +       if (ret) {
> >                 dev_err(dev, "could not register iio (ADC)");
> > +               goto err_iio_register;
> > +       }
> > +
> > +       return 0;
> > +
> > +err_iio_register:
> > +       if (sc27xx_data->var_data->pmic_type == UMP9620_ADC) {
> > +               pm_runtime_put(dev);  
> 
> I don't think the pm_runtime_put() is needed, since you did not get
> the counter before, right?

Please try to avoid mixing up devm_ managed cleanup and manual cleanup.
devm_add_action_or_reset() can be used to ensure the pm_runtime_disable
occurs on error and in remove function.
> 
> > +               pm_runtime_disable(dev);
> > +       }
> >
> >         return ret;
> >  }
> > @@ -1163,11 +1210,65 @@ static const struct of_device_id sc27xx_adc_of_match[] = {
> >  };
> >  MODULE_DEVICE_TABLE(of, sc27xx_adc_of_match);
> >
> > +static int sc27xx_adc_remove(struct platform_device *pdev)
> > +{
> > +       struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> > +       struct sc27xx_adc_data *sc27xx_data = iio_priv(indio_dev);
> > +
> > +       if (sc27xx_data->var_data->pmic_type == UMP9620_ADC) {
> > +               pm_runtime_put(&pdev->dev);  
> 
> You did not get the pm count, why put it firstly?
> 
> > +               pm_runtime_disable(&pdev->dev);
> > +
> > +               /* set the UMP9620 ADC clk26m bit8 on IP */
> > +               regmap_update_bits(sc27xx_data->regmap, UMP9620_XTL_WAIT_CTRL0,
> > +                               UMP9620_XTL_WAIT_CTRL0_EN, 0);

Why is this not called in error path of the probe() function?
I suspect because it also doesn't need to be called here as you have it automatically
called in the sc27xx_adc_disable() call during device managed cleanup.


> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int sc27xx_adc_runtime_suspend(struct device *dev)
> > +{
> > +       struct sc27xx_adc_data *sc27xx_data = iio_priv(dev_get_drvdata(dev));
> > +
> > +       /* clean the UMP9620 ADC clk26m bit8 on IP */
> > +       if (sc27xx_data->var_data->pmic_type == UMP9620_ADC)
> > +               regmap_update_bits(sc27xx_data->regmap, UMP9620_XTL_WAIT_CTRL0,
> > +                               UMP9620_XTL_WAIT_CTRL0_EN, 0);
> > +
> > +       return 0;
> > +}
> > +
> > +static int sc27xx_adc_runtime_resume(struct device *dev)
> > +{
> > +       int ret = 0;  
> 
> no need to initialize it.
> 
> > +       struct sc27xx_adc_data *sc27xx_data = iio_priv(dev_get_drvdata(dev));
> > +
> > +       /* set the UMP9620 ADC clk26m bit8 on IP */
> > +       if (sc27xx_data->var_data->pmic_type == UMP9620_ADC) {
> > +               ret = regmap_update_bits(sc27xx_data->regmap, UMP9620_XTL_WAIT_CTRL0,
> > +                               UMP9620_XTL_WAIT_CTRL0_EN, UMP9620_XTL_WAIT_CTRL0_EN);

> > +               if (ret) {
> > +                       dev_err(dev, "failed to set the UMP9620 ADC clk26m bit8 on IP\n");
> > +                       return ret;
> > +               }
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct dev_pm_ops sc27xx_adc_pm_ops = {
> > +       .runtime_suspend = &sc27xx_adc_runtime_suspend,
> > +       .runtime_resume = &sc27xx_adc_runtime_resume,
> > +};  
> 
> Please use SET_RUNTIME_PM_OPS macro.
> 
> > +
> >  static struct platform_driver sc27xx_adc_driver = {
> >         .probe = sc27xx_adc_probe,
> > +       .remove = sc27xx_adc_remove,
> >         .driver = {
> >                 .name = "sc27xx-adc",
> >                 .of_match_table = sc27xx_adc_of_match,
> > +               .pm     = &sc27xx_adc_pm_ops,
> >         },
> >  };
> >
> > --
> > 2.25.1
> >  
> 
> 

Re: [PATCH 4/7] iio: adc: sc27xx: add support for PMIC sc2720 and sc2721

[kernel test robot]

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

CC: kbuild-all(a)lists.01.org
In-Reply-To: <20220106125947.139523-5-gengcixi@gmail.com>
References: <20220106125947.139523-5-gengcixi@gmail.com>
TO: Cixi Geng <gengcixi@gmail.com>
TO: orsonzhai(a)gmail.com
TO: baolin.wang7(a)gmail.com
TO: zhang.lyra(a)gmail.com
TO: jic23(a)kernel.org
TO: lars(a)metafoo.de
TO: robh+dt(a)kernel.org
TO: lgirdwood(a)gmail.com
TO: broonie(a)kernel.org
CC: yuming.zhu1(a)unisoc.com
CC: linux-iio(a)vger.kernel.org

Hi Cixi,

Thank you for the patch! Perhaps something to improve:

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

url:    https://github.com/0day-ci/linux/commits/Cixi-Geng/iio-adc-sc27xx-adjust-structure-and-add-PMIC-s-support/20220106-210151
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
:::::: branch date: 27 hours ago
:::::: commit date: 27 hours ago
config: openrisc-randconfig-m031-20220106 (https://download.01.org/0day-ci/archive/20220108/202201080030.L51zYw0G-lkp(a)intel.com/config)
compiler: or1k-linux-gcc (GCC) 11.2.0

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

smatch warnings:
drivers/iio/adc/sc27xx_adc.c:461 sc27xx_adc_read() error: uninitialized symbol 'value'.
drivers/iio/adc/sc27xx_adc.c:775 sc27xx_adc_probe() warn: passing zero to 'PTR_ERR'

vim +/value +461 drivers/iio/adc/sc27xx_adc.c

b39db3bcbc9a96 Cixi Geng   2022-01-06  363  
5df362a6cf49ca Freeman Liu 2018-06-21  364  static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
5df362a6cf49ca Freeman Liu 2018-06-21  365  			   int scale, int *val)
5df362a6cf49ca Freeman Liu 2018-06-21  366  {
5df362a6cf49ca Freeman Liu 2018-06-21  367  	int ret;
8de877d2bba5c3 Freeman Liu 2019-07-25  368  	u32 tmp, value, status;
5df362a6cf49ca Freeman Liu 2018-06-21  369  
5df362a6cf49ca Freeman Liu 2018-06-21  370  	ret = hwspin_lock_timeout_raw(data->hwlock, SC27XX_ADC_HWLOCK_TIMEOUT);
5df362a6cf49ca Freeman Liu 2018-06-21  371  	if (ret) {
5df362a6cf49ca Freeman Liu 2018-06-21  372  		dev_err(data->dev, "timeout to get the hwspinlock\n");
5df362a6cf49ca Freeman Liu 2018-06-21  373  		return ret;
5df362a6cf49ca Freeman Liu 2018-06-21  374  	}
5df362a6cf49ca Freeman Liu 2018-06-21  375  
cd6ab0edd81be2 Cixi Geng   2022-01-06  376  	/*
cd6ab0edd81be2 Cixi Geng   2022-01-06  377  	 * According to the sc2721 chip data sheet, the reference voltage of
cd6ab0edd81be2 Cixi Geng   2022-01-06  378  	 * specific channel 30 and channel 31 in ADC module needs to be set from
cd6ab0edd81be2 Cixi Geng   2022-01-06  379  	 * the default 2.8v to 3.5v.
cd6ab0edd81be2 Cixi Geng   2022-01-06  380  	 */
cd6ab0edd81be2 Cixi Geng   2022-01-06  381  	if (data->var_data->pmic_type == SC2721_ADC) {
cd6ab0edd81be2 Cixi Geng   2022-01-06  382  		if ((channel == 30) || (channel == 31)) {
cd6ab0edd81be2 Cixi Geng   2022-01-06  383  			ret = regulator_set_voltage(data->volref,
cd6ab0edd81be2 Cixi Geng   2022-01-06  384  						SC27XX_ADC_REFVOL_VDD35,
cd6ab0edd81be2 Cixi Geng   2022-01-06  385  						SC27XX_ADC_REFVOL_VDD35);
cd6ab0edd81be2 Cixi Geng   2022-01-06  386  			if (ret) {
cd6ab0edd81be2 Cixi Geng   2022-01-06  387  				dev_err(data->dev, "failed to set the volref 3.5V\n");
cd6ab0edd81be2 Cixi Geng   2022-01-06  388  				hwspin_unlock_raw(data->hwlock);
cd6ab0edd81be2 Cixi Geng   2022-01-06  389  				return ret;
cd6ab0edd81be2 Cixi Geng   2022-01-06  390  			}
cd6ab0edd81be2 Cixi Geng   2022-01-06  391  		}
cd6ab0edd81be2 Cixi Geng   2022-01-06  392  	}
cd6ab0edd81be2 Cixi Geng   2022-01-06  393  
5df362a6cf49ca Freeman Liu 2018-06-21  394  	ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CTL,
5df362a6cf49ca Freeman Liu 2018-06-21  395  				 SC27XX_ADC_EN, SC27XX_ADC_EN);
5df362a6cf49ca Freeman Liu 2018-06-21  396  	if (ret)
5df362a6cf49ca Freeman Liu 2018-06-21  397  		goto unlock_adc;
5df362a6cf49ca Freeman Liu 2018-06-21  398  
8de877d2bba5c3 Freeman Liu 2019-07-25  399  	ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_INT_CLR,
8de877d2bba5c3 Freeman Liu 2019-07-25  400  				 SC27XX_ADC_IRQ_CLR, SC27XX_ADC_IRQ_CLR);
8de877d2bba5c3 Freeman Liu 2019-07-25  401  	if (ret)
8de877d2bba5c3 Freeman Liu 2019-07-25  402  		goto disable_adc;
8de877d2bba5c3 Freeman Liu 2019-07-25  403  
5df362a6cf49ca Freeman Liu 2018-06-21  404  	/* Configure the channel id and scale */
b39db3bcbc9a96 Cixi Geng   2022-01-06  405  	tmp = (scale << data->var_data->scale_shift) & data->var_data->scale_mask;
5df362a6cf49ca Freeman Liu 2018-06-21  406  	tmp |= channel & SC27XX_ADC_CHN_ID_MASK;
5df362a6cf49ca Freeman Liu 2018-06-21  407  	ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CH_CFG,
b39db3bcbc9a96 Cixi Geng   2022-01-06  408  				 SC27XX_ADC_CHN_ID_MASK |
b39db3bcbc9a96 Cixi Geng   2022-01-06  409  				 data->var_data->scale_mask,
5df362a6cf49ca Freeman Liu 2018-06-21  410  				 tmp);
5df362a6cf49ca Freeman Liu 2018-06-21  411  	if (ret)
5df362a6cf49ca Freeman Liu 2018-06-21  412  		goto disable_adc;
5df362a6cf49ca Freeman Liu 2018-06-21  413  
5df362a6cf49ca Freeman Liu 2018-06-21  414  	/* Select 12bit conversion mode, and only sample 1 time */
5df362a6cf49ca Freeman Liu 2018-06-21  415  	tmp = SC27XX_ADC_12BIT_MODE;
5df362a6cf49ca Freeman Liu 2018-06-21  416  	tmp |= (0 << SC27XX_ADC_RUN_NUM_SHIFT) & SC27XX_ADC_RUN_NUM_MASK;
5df362a6cf49ca Freeman Liu 2018-06-21  417  	ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CTL,
5df362a6cf49ca Freeman Liu 2018-06-21  418  				 SC27XX_ADC_RUN_NUM_MASK | SC27XX_ADC_12BIT_MODE,
5df362a6cf49ca Freeman Liu 2018-06-21  419  				 tmp);
5df362a6cf49ca Freeman Liu 2018-06-21  420  	if (ret)
5df362a6cf49ca Freeman Liu 2018-06-21  421  		goto disable_adc;
5df362a6cf49ca Freeman Liu 2018-06-21  422  
5df362a6cf49ca Freeman Liu 2018-06-21  423  	ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CTL,
5df362a6cf49ca Freeman Liu 2018-06-21  424  				 SC27XX_ADC_CHN_RUN, SC27XX_ADC_CHN_RUN);
5df362a6cf49ca Freeman Liu 2018-06-21  425  	if (ret)
5df362a6cf49ca Freeman Liu 2018-06-21  426  		goto disable_adc;
5df362a6cf49ca Freeman Liu 2018-06-21  427  
8de877d2bba5c3 Freeman Liu 2019-07-25  428  	ret = regmap_read_poll_timeout(data->regmap,
8de877d2bba5c3 Freeman Liu 2019-07-25  429  				       data->base + SC27XX_ADC_INT_RAW,
8de877d2bba5c3 Freeman Liu 2019-07-25  430  				       status, (status & SC27XX_ADC_IRQ_RAW),
8de877d2bba5c3 Freeman Liu 2019-07-25  431  				       SC27XX_ADC_POLL_RAW_STATUS,
8de877d2bba5c3 Freeman Liu 2019-07-25  432  				       SC27XX_ADC_RDY_TIMEOUT);
8de877d2bba5c3 Freeman Liu 2019-07-25  433  	if (ret) {
8de877d2bba5c3 Freeman Liu 2019-07-25  434  		dev_err(data->dev, "read adc timeout, status = 0x%x\n", status);
8de877d2bba5c3 Freeman Liu 2019-07-25  435  		goto disable_adc;
750ac07eb2c856 Freeman Liu 2018-11-09  436  	}
5df362a6cf49ca Freeman Liu 2018-06-21  437  
8de877d2bba5c3 Freeman Liu 2019-07-25  438  	ret = regmap_read(data->regmap, data->base + SC27XX_ADC_DATA, &value);
8de877d2bba5c3 Freeman Liu 2019-07-25  439  	if (ret)
8de877d2bba5c3 Freeman Liu 2019-07-25  440  		goto disable_adc;
8de877d2bba5c3 Freeman Liu 2019-07-25  441  
8de877d2bba5c3 Freeman Liu 2019-07-25  442  	value &= SC27XX_ADC_DATA_MASK;
8de877d2bba5c3 Freeman Liu 2019-07-25  443  
5df362a6cf49ca Freeman Liu 2018-06-21  444  disable_adc:
5df362a6cf49ca Freeman Liu 2018-06-21  445  	regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CTL,
5df362a6cf49ca Freeman Liu 2018-06-21  446  			   SC27XX_ADC_EN, 0);
5df362a6cf49ca Freeman Liu 2018-06-21  447  unlock_adc:
cd6ab0edd81be2 Cixi Geng   2022-01-06  448  	if (data->var_data->pmic_type == SC2721_ADC) {
cd6ab0edd81be2 Cixi Geng   2022-01-06  449  		if ((channel == 30) || (channel == 31)) {
cd6ab0edd81be2 Cixi Geng   2022-01-06  450  			ret = regulator_set_voltage(data->volref,
cd6ab0edd81be2 Cixi Geng   2022-01-06  451  						    SC27XX_ADC_REFVOL_VDD28,
cd6ab0edd81be2 Cixi Geng   2022-01-06  452  						    SC27XX_ADC_REFVOL_VDD28);
cd6ab0edd81be2 Cixi Geng   2022-01-06  453  			if (ret)
cd6ab0edd81be2 Cixi Geng   2022-01-06  454  				dev_err(data->dev, "failed to set the volref 2.8V\n");
cd6ab0edd81be2 Cixi Geng   2022-01-06  455  		}
cd6ab0edd81be2 Cixi Geng   2022-01-06  456  	}
cd6ab0edd81be2 Cixi Geng   2022-01-06  457  
5df362a6cf49ca Freeman Liu 2018-06-21  458  	hwspin_unlock_raw(data->hwlock);
5df362a6cf49ca Freeman Liu 2018-06-21  459  
5df362a6cf49ca Freeman Liu 2018-06-21  460  	if (!ret)
8de877d2bba5c3 Freeman Liu 2019-07-25 @461  		*val = value;
5df362a6cf49ca Freeman Liu 2018-06-21  462  
5df362a6cf49ca Freeman Liu 2018-06-21  463  	return ret;
5df362a6cf49ca Freeman Liu 2018-06-21  464  }
5df362a6cf49ca Freeman Liu 2018-06-21  465  

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

Re: [PATCH 4/7] iio: adc: sc27xx: add support for PMIC sc2720 and sc2721

[Dan Carpenter]

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

Hi Cixi,

url:    https://github.com/0day-ci/linux/commits/Cixi-Geng/iio-adc-sc27xx-adjust-structure-and-add-PMIC-s-support/20220106-210151
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: openrisc-randconfig-m031-20220106 (https://download.01.org/0day-ci/archive/20220108/202201080030.L51zYw0G-lkp(a)intel.com/config)
compiler: or1k-linux-gcc (GCC) 11.2.0

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

smatch warnings:
drivers/iio/adc/sc27xx_adc.c:461 sc27xx_adc_read() error: uninitialized symbol 'value'.

vim +/value +461 drivers/iio/adc/sc27xx_adc.c

5df362a6cf49ca Freeman Liu 2018-06-21  364  static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
5df362a6cf49ca Freeman Liu 2018-06-21  365  			   int scale, int *val)
5df362a6cf49ca Freeman Liu 2018-06-21  366  {
5df362a6cf49ca Freeman Liu 2018-06-21  367  	int ret;
8de877d2bba5c3 Freeman Liu 2019-07-25  368  	u32 tmp, value, status;
5df362a6cf49ca Freeman Liu 2018-06-21  369  
5df362a6cf49ca Freeman Liu 2018-06-21  370  	ret = hwspin_lock_timeout_raw(data->hwlock, SC27XX_ADC_HWLOCK_TIMEOUT);
5df362a6cf49ca Freeman Liu 2018-06-21  371  	if (ret) {
5df362a6cf49ca Freeman Liu 2018-06-21  372  		dev_err(data->dev, "timeout to get the hwspinlock\n");
5df362a6cf49ca Freeman Liu 2018-06-21  373  		return ret;
5df362a6cf49ca Freeman Liu 2018-06-21  374  	}
5df362a6cf49ca Freeman Liu 2018-06-21  375  
cd6ab0edd81be2 Cixi Geng   2022-01-06  376  	/*
cd6ab0edd81be2 Cixi Geng   2022-01-06  377  	 * According to the sc2721 chip data sheet, the reference voltage of
cd6ab0edd81be2 Cixi Geng   2022-01-06  378  	 * specific channel 30 and channel 31 in ADC module needs to be set from
cd6ab0edd81be2 Cixi Geng   2022-01-06  379  	 * the default 2.8v to 3.5v.
cd6ab0edd81be2 Cixi Geng   2022-01-06  380  	 */
cd6ab0edd81be2 Cixi Geng   2022-01-06  381  	if (data->var_data->pmic_type == SC2721_ADC) {
cd6ab0edd81be2 Cixi Geng   2022-01-06  382  		if ((channel == 30) || (channel == 31)) {
cd6ab0edd81be2 Cixi Geng   2022-01-06  383  			ret = regulator_set_voltage(data->volref,
cd6ab0edd81be2 Cixi Geng   2022-01-06  384  						SC27XX_ADC_REFVOL_VDD35,
cd6ab0edd81be2 Cixi Geng   2022-01-06  385  						SC27XX_ADC_REFVOL_VDD35);
cd6ab0edd81be2 Cixi Geng   2022-01-06  386  			if (ret) {
cd6ab0edd81be2 Cixi Geng   2022-01-06  387  				dev_err(data->dev, "failed to set the volref 3.5V\n");
cd6ab0edd81be2 Cixi Geng   2022-01-06  388  				hwspin_unlock_raw(data->hwlock);
cd6ab0edd81be2 Cixi Geng   2022-01-06  389  				return ret;
cd6ab0edd81be2 Cixi Geng   2022-01-06  390  			}
cd6ab0edd81be2 Cixi Geng   2022-01-06  391  		}
cd6ab0edd81be2 Cixi Geng   2022-01-06  392  	}
cd6ab0edd81be2 Cixi Geng   2022-01-06  393  
5df362a6cf49ca Freeman Liu 2018-06-21  394  	ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CTL,
5df362a6cf49ca Freeman Liu 2018-06-21  395  				 SC27XX_ADC_EN, SC27XX_ADC_EN);
5df362a6cf49ca Freeman Liu 2018-06-21  396  	if (ret)
5df362a6cf49ca Freeman Liu 2018-06-21  397  		goto unlock_adc;
5df362a6cf49ca Freeman Liu 2018-06-21  398  
8de877d2bba5c3 Freeman Liu 2019-07-25  399  	ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_INT_CLR,
8de877d2bba5c3 Freeman Liu 2019-07-25  400  				 SC27XX_ADC_IRQ_CLR, SC27XX_ADC_IRQ_CLR);
8de877d2bba5c3 Freeman Liu 2019-07-25  401  	if (ret)
8de877d2bba5c3 Freeman Liu 2019-07-25  402  		goto disable_adc;

Assume we hit a goto.

8de877d2bba5c3 Freeman Liu 2019-07-25  403  
5df362a6cf49ca Freeman Liu 2018-06-21  404  	/* Configure the channel id and scale */
b39db3bcbc9a96 Cixi Geng   2022-01-06  405  	tmp = (scale << data->var_data->scale_shift) & data->var_data->scale_mask;
5df362a6cf49ca Freeman Liu 2018-06-21  406  	tmp |= channel & SC27XX_ADC_CHN_ID_MASK;
5df362a6cf49ca Freeman Liu 2018-06-21  407  	ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CH_CFG,
b39db3bcbc9a96 Cixi Geng   2022-01-06  408  				 SC27XX_ADC_CHN_ID_MASK |
b39db3bcbc9a96 Cixi Geng   2022-01-06  409  				 data->var_data->scale_mask,
5df362a6cf49ca Freeman Liu 2018-06-21  410  				 tmp);
5df362a6cf49ca Freeman Liu 2018-06-21  411  	if (ret)
5df362a6cf49ca Freeman Liu 2018-06-21  412  		goto disable_adc;
5df362a6cf49ca Freeman Liu 2018-06-21  413  
5df362a6cf49ca Freeman Liu 2018-06-21  414  	/* Select 12bit conversion mode, and only sample 1 time */
5df362a6cf49ca Freeman Liu 2018-06-21  415  	tmp = SC27XX_ADC_12BIT_MODE;
5df362a6cf49ca Freeman Liu 2018-06-21  416  	tmp |= (0 << SC27XX_ADC_RUN_NUM_SHIFT) & SC27XX_ADC_RUN_NUM_MASK;
5df362a6cf49ca Freeman Liu 2018-06-21  417  	ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CTL,
5df362a6cf49ca Freeman Liu 2018-06-21  418  				 SC27XX_ADC_RUN_NUM_MASK | SC27XX_ADC_12BIT_MODE,
5df362a6cf49ca Freeman Liu 2018-06-21  419  				 tmp);
5df362a6cf49ca Freeman Liu 2018-06-21  420  	if (ret)
5df362a6cf49ca Freeman Liu 2018-06-21  421  		goto disable_adc;
5df362a6cf49ca Freeman Liu 2018-06-21  422  
5df362a6cf49ca Freeman Liu 2018-06-21  423  	ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CTL,
5df362a6cf49ca Freeman Liu 2018-06-21  424  				 SC27XX_ADC_CHN_RUN, SC27XX_ADC_CHN_RUN);
5df362a6cf49ca Freeman Liu 2018-06-21  425  	if (ret)
5df362a6cf49ca Freeman Liu 2018-06-21  426  		goto disable_adc;
5df362a6cf49ca Freeman Liu 2018-06-21  427  
8de877d2bba5c3 Freeman Liu 2019-07-25  428  	ret = regmap_read_poll_timeout(data->regmap,
8de877d2bba5c3 Freeman Liu 2019-07-25  429  				       data->base + SC27XX_ADC_INT_RAW,
8de877d2bba5c3 Freeman Liu 2019-07-25  430  				       status, (status & SC27XX_ADC_IRQ_RAW),
8de877d2bba5c3 Freeman Liu 2019-07-25  431  				       SC27XX_ADC_POLL_RAW_STATUS,
8de877d2bba5c3 Freeman Liu 2019-07-25  432  				       SC27XX_ADC_RDY_TIMEOUT);
8de877d2bba5c3 Freeman Liu 2019-07-25  433  	if (ret) {
8de877d2bba5c3 Freeman Liu 2019-07-25  434  		dev_err(data->dev, "read adc timeout, status = 0x%x\n", status);
8de877d2bba5c3 Freeman Liu 2019-07-25  435  		goto disable_adc;
750ac07eb2c856 Freeman Liu 2018-11-09  436  	}
5df362a6cf49ca Freeman Liu 2018-06-21  437  
8de877d2bba5c3 Freeman Liu 2019-07-25  438  	ret = regmap_read(data->regmap, data->base + SC27XX_ADC_DATA, &value);

value is initialized here.

8de877d2bba5c3 Freeman Liu 2019-07-25  439  	if (ret)
8de877d2bba5c3 Freeman Liu 2019-07-25  440  		goto disable_adc;
8de877d2bba5c3 Freeman Liu 2019-07-25  441  
8de877d2bba5c3 Freeman Liu 2019-07-25  442  	value &= SC27XX_ADC_DATA_MASK;
8de877d2bba5c3 Freeman Liu 2019-07-25  443  
5df362a6cf49ca Freeman Liu 2018-06-21  444  disable_adc:
5df362a6cf49ca Freeman Liu 2018-06-21  445  	regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CTL,
5df362a6cf49ca Freeman Liu 2018-06-21  446  			   SC27XX_ADC_EN, 0);
5df362a6cf49ca Freeman Liu 2018-06-21  447  unlock_adc:
cd6ab0edd81be2 Cixi Geng   2022-01-06  448  	if (data->var_data->pmic_type == SC2721_ADC) {
cd6ab0edd81be2 Cixi Geng   2022-01-06  449  		if ((channel == 30) || (channel == 31)) {
cd6ab0edd81be2 Cixi Geng   2022-01-06  450  			ret = regulator_set_voltage(data->volref,
cd6ab0edd81be2 Cixi Geng   2022-01-06  451  						    SC27XX_ADC_REFVOL_VDD28,
cd6ab0edd81be2 Cixi Geng   2022-01-06  452  						    SC27XX_ADC_REFVOL_VDD28);
cd6ab0edd81be2 Cixi Geng   2022-01-06  453  			if (ret)
cd6ab0edd81be2 Cixi Geng   2022-01-06  454  				dev_err(data->dev, "failed to set the volref 2.8V\n");

ret is reset here.

cd6ab0edd81be2 Cixi Geng   2022-01-06  455  		}
cd6ab0edd81be2 Cixi Geng   2022-01-06  456  	}
cd6ab0edd81be2 Cixi Geng   2022-01-06  457  
5df362a6cf49ca Freeman Liu 2018-06-21  458  	hwspin_unlock_raw(data->hwlock);
5df362a6cf49ca Freeman Liu 2018-06-21  459  
5df362a6cf49ca Freeman Liu 2018-06-21  460  	if (!ret)
8de877d2bba5c3 Freeman Liu 2019-07-25 @461  		*val = value;

Since ret is reset, it is no long connected to value.


5df362a6cf49ca Freeman Liu 2018-06-21  462  
5df362a6cf49ca Freeman Liu 2018-06-21  463  	return ret;
5df362a6cf49ca Freeman Liu 2018-06-21  464  }

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

Re: [PATCH 4/7] iio: adc: sc27xx: add support for PMIC sc2720 and sc2721

[Dan Carpenter]

Hi Cixi,

url:    https://github.com/0day-ci/linux/commits/Cixi-Geng/iio-adc-sc27xx-adjust-structure-and-add-PMIC-s-support/20220106-210151
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: openrisc-randconfig-m031-20220106 (https://download.01.org/0day-ci/archive/20220108/202201080030.L51zYw0G-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 11.2.0

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

smatch warnings:
drivers/iio/adc/sc27xx_adc.c:461 sc27xx_adc_read() error: uninitialized symbol 'value'.

vim +/value +461 drivers/iio/adc/sc27xx_adc.c

5df362a6cf49ca Freeman Liu 2018-06-21  364  static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
5df362a6cf49ca Freeman Liu 2018-06-21  365  			   int scale, int *val)
5df362a6cf49ca Freeman Liu 2018-06-21  366  {
5df362a6cf49ca Freeman Liu 2018-06-21  367  	int ret;
8de877d2bba5c3 Freeman Liu 2019-07-25  368  	u32 tmp, value, status;
5df362a6cf49ca Freeman Liu 2018-06-21  369  
5df362a6cf49ca Freeman Liu 2018-06-21  370  	ret = hwspin_lock_timeout_raw(data->hwlock, SC27XX_ADC_HWLOCK_TIMEOUT);
5df362a6cf49ca Freeman Liu 2018-06-21  371  	if (ret) {
5df362a6cf49ca Freeman Liu 2018-06-21  372  		dev_err(data->dev, "timeout to get the hwspinlock\n");
5df362a6cf49ca Freeman Liu 2018-06-21  373  		return ret;
5df362a6cf49ca Freeman Liu 2018-06-21  374  	}
5df362a6cf49ca Freeman Liu 2018-06-21  375  
cd6ab0edd81be2 Cixi Geng   2022-01-06  376  	/*
cd6ab0edd81be2 Cixi Geng   2022-01-06  377  	 * According to the sc2721 chip data sheet, the reference voltage of
cd6ab0edd81be2 Cixi Geng   2022-01-06  378  	 * specific channel 30 and channel 31 in ADC module needs to be set from
cd6ab0edd81be2 Cixi Geng   2022-01-06  379  	 * the default 2.8v to 3.5v.
cd6ab0edd81be2 Cixi Geng   2022-01-06  380  	 */
cd6ab0edd81be2 Cixi Geng   2022-01-06  381  	if (data->var_data->pmic_type == SC2721_ADC) {
cd6ab0edd81be2 Cixi Geng   2022-01-06  382  		if ((channel == 30) || (channel == 31)) {
cd6ab0edd81be2 Cixi Geng   2022-01-06  383  			ret = regulator_set_voltage(data->volref,
cd6ab0edd81be2 Cixi Geng   2022-01-06  384  						SC27XX_ADC_REFVOL_VDD35,
cd6ab0edd81be2 Cixi Geng   2022-01-06  385  						SC27XX_ADC_REFVOL_VDD35);
cd6ab0edd81be2 Cixi Geng   2022-01-06  386  			if (ret) {
cd6ab0edd81be2 Cixi Geng   2022-01-06  387  				dev_err(data->dev, "failed to set the volref 3.5V\n");
cd6ab0edd81be2 Cixi Geng   2022-01-06  388  				hwspin_unlock_raw(data->hwlock);
cd6ab0edd81be2 Cixi Geng   2022-01-06  389  				return ret;
cd6ab0edd81be2 Cixi Geng   2022-01-06  390  			}
cd6ab0edd81be2 Cixi Geng   2022-01-06  391  		}
cd6ab0edd81be2 Cixi Geng   2022-01-06  392  	}
cd6ab0edd81be2 Cixi Geng   2022-01-06  393  
5df362a6cf49ca Freeman Liu 2018-06-21  394  	ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CTL,
5df362a6cf49ca Freeman Liu 2018-06-21  395  				 SC27XX_ADC_EN, SC27XX_ADC_EN);
5df362a6cf49ca Freeman Liu 2018-06-21  396  	if (ret)
5df362a6cf49ca Freeman Liu 2018-06-21  397  		goto unlock_adc;
5df362a6cf49ca Freeman Liu 2018-06-21  398  
8de877d2bba5c3 Freeman Liu 2019-07-25  399  	ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_INT_CLR,
8de877d2bba5c3 Freeman Liu 2019-07-25  400  				 SC27XX_ADC_IRQ_CLR, SC27XX_ADC_IRQ_CLR);
8de877d2bba5c3 Freeman Liu 2019-07-25  401  	if (ret)
8de877d2bba5c3 Freeman Liu 2019-07-25  402  		goto disable_adc;

Assume we hit a goto.

8de877d2bba5c3 Freeman Liu 2019-07-25  403  
5df362a6cf49ca Freeman Liu 2018-06-21  404  	/* Configure the channel id and scale */
b39db3bcbc9a96 Cixi Geng   2022-01-06  405  	tmp = (scale << data->var_data->scale_shift) & data->var_data->scale_mask;
5df362a6cf49ca Freeman Liu 2018-06-21  406  	tmp |= channel & SC27XX_ADC_CHN_ID_MASK;
5df362a6cf49ca Freeman Liu 2018-06-21  407  	ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CH_CFG,
b39db3bcbc9a96 Cixi Geng   2022-01-06  408  				 SC27XX_ADC_CHN_ID_MASK |
b39db3bcbc9a96 Cixi Geng   2022-01-06  409  				 data->var_data->scale_mask,
5df362a6cf49ca Freeman Liu 2018-06-21  410  				 tmp);
5df362a6cf49ca Freeman Liu 2018-06-21  411  	if (ret)
5df362a6cf49ca Freeman Liu 2018-06-21  412  		goto disable_adc;
5df362a6cf49ca Freeman Liu 2018-06-21  413  
5df362a6cf49ca Freeman Liu 2018-06-21  414  	/* Select 12bit conversion mode, and only sample 1 time */
5df362a6cf49ca Freeman Liu 2018-06-21  415  	tmp = SC27XX_ADC_12BIT_MODE;
5df362a6cf49ca Freeman Liu 2018-06-21  416  	tmp |= (0 << SC27XX_ADC_RUN_NUM_SHIFT) & SC27XX_ADC_RUN_NUM_MASK;
5df362a6cf49ca Freeman Liu 2018-06-21  417  	ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CTL,
5df362a6cf49ca Freeman Liu 2018-06-21  418  				 SC27XX_ADC_RUN_NUM_MASK | SC27XX_ADC_12BIT_MODE,
5df362a6cf49ca Freeman Liu 2018-06-21  419  				 tmp);
5df362a6cf49ca Freeman Liu 2018-06-21  420  	if (ret)
5df362a6cf49ca Freeman Liu 2018-06-21  421  		goto disable_adc;
5df362a6cf49ca Freeman Liu 2018-06-21  422  
5df362a6cf49ca Freeman Liu 2018-06-21  423  	ret = regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CTL,
5df362a6cf49ca Freeman Liu 2018-06-21  424  				 SC27XX_ADC_CHN_RUN, SC27XX_ADC_CHN_RUN);
5df362a6cf49ca Freeman Liu 2018-06-21  425  	if (ret)
5df362a6cf49ca Freeman Liu 2018-06-21  426  		goto disable_adc;
5df362a6cf49ca Freeman Liu 2018-06-21  427  
8de877d2bba5c3 Freeman Liu 2019-07-25  428  	ret = regmap_read_poll_timeout(data->regmap,
8de877d2bba5c3 Freeman Liu 2019-07-25  429  				       data->base + SC27XX_ADC_INT_RAW,
8de877d2bba5c3 Freeman Liu 2019-07-25  430  				       status, (status & SC27XX_ADC_IRQ_RAW),
8de877d2bba5c3 Freeman Liu 2019-07-25  431  				       SC27XX_ADC_POLL_RAW_STATUS,
8de877d2bba5c3 Freeman Liu 2019-07-25  432  				       SC27XX_ADC_RDY_TIMEOUT);
8de877d2bba5c3 Freeman Liu 2019-07-25  433  	if (ret) {
8de877d2bba5c3 Freeman Liu 2019-07-25  434  		dev_err(data->dev, "read adc timeout, status = 0x%x\n", status);
8de877d2bba5c3 Freeman Liu 2019-07-25  435  		goto disable_adc;
750ac07eb2c856 Freeman Liu 2018-11-09  436  	}
5df362a6cf49ca Freeman Liu 2018-06-21  437  
8de877d2bba5c3 Freeman Liu 2019-07-25  438  	ret = regmap_read(data->regmap, data->base + SC27XX_ADC_DATA, &value);

value is initialized here.

8de877d2bba5c3 Freeman Liu 2019-07-25  439  	if (ret)
8de877d2bba5c3 Freeman Liu 2019-07-25  440  		goto disable_adc;
8de877d2bba5c3 Freeman Liu 2019-07-25  441  
8de877d2bba5c3 Freeman Liu 2019-07-25  442  	value &= SC27XX_ADC_DATA_MASK;
8de877d2bba5c3 Freeman Liu 2019-07-25  443  
5df362a6cf49ca Freeman Liu 2018-06-21  444  disable_adc:
5df362a6cf49ca Freeman Liu 2018-06-21  445  	regmap_update_bits(data->regmap, data->base + SC27XX_ADC_CTL,
5df362a6cf49ca Freeman Liu 2018-06-21  446  			   SC27XX_ADC_EN, 0);
5df362a6cf49ca Freeman Liu 2018-06-21  447  unlock_adc:
cd6ab0edd81be2 Cixi Geng   2022-01-06  448  	if (data->var_data->pmic_type == SC2721_ADC) {
cd6ab0edd81be2 Cixi Geng   2022-01-06  449  		if ((channel == 30) || (channel == 31)) {
cd6ab0edd81be2 Cixi Geng   2022-01-06  450  			ret = regulator_set_voltage(data->volref,
cd6ab0edd81be2 Cixi Geng   2022-01-06  451  						    SC27XX_ADC_REFVOL_VDD28,
cd6ab0edd81be2 Cixi Geng   2022-01-06  452  						    SC27XX_ADC_REFVOL_VDD28);
cd6ab0edd81be2 Cixi Geng   2022-01-06  453  			if (ret)
cd6ab0edd81be2 Cixi Geng   2022-01-06  454  				dev_err(data->dev, "failed to set the volref 2.8V\n");

ret is reset here.

cd6ab0edd81be2 Cixi Geng   2022-01-06  455  		}
cd6ab0edd81be2 Cixi Geng   2022-01-06  456  	}
cd6ab0edd81be2 Cixi Geng   2022-01-06  457  
5df362a6cf49ca Freeman Liu 2018-06-21  458  	hwspin_unlock_raw(data->hwlock);
5df362a6cf49ca Freeman Liu 2018-06-21  459  
5df362a6cf49ca Freeman Liu 2018-06-21  460  	if (!ret)
8de877d2bba5c3 Freeman Liu 2019-07-25 @461  		*val = value;

Since ret is reset, it is no long connected to value.


5df362a6cf49ca Freeman Liu 2018-06-21  462  
5df362a6cf49ca Freeman Liu 2018-06-21  463  	return ret;
5df362a6cf49ca Freeman Liu 2018-06-21  464  }

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

Re: [PATCH 6/7] iio: adc: sc27xx: add support for PMIC ump9620

[kernel test robot]

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

CC: kbuild-all(a)lists.01.org
In-Reply-To: <20220106125947.139523-7-gengcixi@gmail.com>
References: <20220106125947.139523-7-gengcixi@gmail.com>
TO: Cixi Geng <gengcixi@gmail.com>
TO: orsonzhai(a)gmail.com
TO: baolin.wang7(a)gmail.com
TO: zhang.lyra(a)gmail.com
TO: jic23(a)kernel.org
TO: lars(a)metafoo.de
TO: robh+dt(a)kernel.org
TO: lgirdwood(a)gmail.com
TO: broonie(a)kernel.org
CC: yuming.zhu1(a)unisoc.com
CC: linux-iio(a)vger.kernel.org

Hi Cixi,

Thank you for the patch! Perhaps something to improve:

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

url:    https://github.com/0day-ci/linux/commits/Cixi-Geng/iio-adc-sc27xx-adjust-structure-and-add-PMIC-s-support/20220106-210151
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
:::::: branch date: 30 hours ago
:::::: commit date: 30 hours ago
config: openrisc-randconfig-m031-20220106 (https://download.01.org/0day-ci/archive/20220108/202201080313.70ZrhufR-lkp(a)intel.com/config)
compiler: or1k-linux-gcc (GCC) 11.2.0

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

New smatch warnings:
drivers/iio/adc/sc27xx_adc.c:196 adc_nvmem_cell_calib_data() warn: passing zero to 'PTR_ERR'

Old smatch warnings:
drivers/iio/adc/sc27xx_adc.c:201 adc_nvmem_cell_calib_data() warn: passing zero to 'PTR_ERR'
drivers/iio/adc/sc27xx_adc.c:706 sc27xx_adc_read() error: uninitialized symbol 'value'.
drivers/iio/adc/sc27xx_adc.c:1123 sc27xx_adc_probe() warn: passing zero to 'PTR_ERR'

vim +/PTR_ERR +196 drivers/iio/adc/sc27xx_adc.c

8ba0dbfd07a358 Baolin Wang 2018-08-29  183  
87da8dcc76b4aa Cixi Geng   2022-01-06  184  static int adc_nvmem_cell_calib_data(struct sc27xx_adc_data *data, const char *cell_name)
87da8dcc76b4aa Cixi Geng   2022-01-06  185  {
87da8dcc76b4aa Cixi Geng   2022-01-06  186  	struct nvmem_cell *cell;
87da8dcc76b4aa Cixi Geng   2022-01-06  187  	void *buf;
87da8dcc76b4aa Cixi Geng   2022-01-06  188  	u32 calib_data = 0;
87da8dcc76b4aa Cixi Geng   2022-01-06  189  	size_t len = 0;
87da8dcc76b4aa Cixi Geng   2022-01-06  190  
87da8dcc76b4aa Cixi Geng   2022-01-06  191  	if (!data)
87da8dcc76b4aa Cixi Geng   2022-01-06  192  		return -EINVAL;
87da8dcc76b4aa Cixi Geng   2022-01-06  193  
87da8dcc76b4aa Cixi Geng   2022-01-06  194  	cell = nvmem_cell_get(data->dev, cell_name);
87da8dcc76b4aa Cixi Geng   2022-01-06  195  	if (IS_ERR_OR_NULL(cell))
87da8dcc76b4aa Cixi Geng   2022-01-06 @196  		return PTR_ERR(cell);
87da8dcc76b4aa Cixi Geng   2022-01-06  197  
87da8dcc76b4aa Cixi Geng   2022-01-06  198  	buf = nvmem_cell_read(cell, &len);
87da8dcc76b4aa Cixi Geng   2022-01-06  199  	if (IS_ERR_OR_NULL(buf)) {
87da8dcc76b4aa Cixi Geng   2022-01-06  200  		nvmem_cell_put(cell);
87da8dcc76b4aa Cixi Geng   2022-01-06  201  		return PTR_ERR(buf);
87da8dcc76b4aa Cixi Geng   2022-01-06  202  	}
87da8dcc76b4aa Cixi Geng   2022-01-06  203  
87da8dcc76b4aa Cixi Geng   2022-01-06  204  	memcpy(&calib_data, buf, min(len, sizeof(u32)));
87da8dcc76b4aa Cixi Geng   2022-01-06  205  
87da8dcc76b4aa Cixi Geng   2022-01-06  206  	kfree(buf);
87da8dcc76b4aa Cixi Geng   2022-01-06  207  	nvmem_cell_put(cell);
87da8dcc76b4aa Cixi Geng   2022-01-06  208  	return calib_data;
87da8dcc76b4aa Cixi Geng   2022-01-06  209  }
87da8dcc76b4aa Cixi Geng   2022-01-06  210  

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

Re: [PATCH 6/7] iio: adc: sc27xx: add support for PMIC ump9620

[Dan Carpenter]

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

Hi Cixi,

url:    https://github.com/0day-ci/linux/commits/Cixi-Geng/iio-adc-sc27xx-adjust-structure-and-add-PMIC-s-support/20220106-210151
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: openrisc-randconfig-m031-20220106 (https://download.01.org/0day-ci/archive/20220108/202201080313.70ZrhufR-lkp(a)intel.com/config)
compiler: or1k-linux-gcc (GCC) 11.2.0

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

New smatch warnings:
drivers/iio/adc/sc27xx_adc.c:196 adc_nvmem_cell_calib_data() warn: passing zero to 'PTR_ERR'

vim +/PTR_ERR +196 drivers/iio/adc/sc27xx_adc.c

87da8dcc76b4aa Cixi Geng   2022-01-06  184  static int adc_nvmem_cell_calib_data(struct sc27xx_adc_data *data, const char *cell_name)
87da8dcc76b4aa Cixi Geng   2022-01-06  185  {
87da8dcc76b4aa Cixi Geng   2022-01-06  186  	struct nvmem_cell *cell;
87da8dcc76b4aa Cixi Geng   2022-01-06  187  	void *buf;
87da8dcc76b4aa Cixi Geng   2022-01-06  188  	u32 calib_data = 0;
87da8dcc76b4aa Cixi Geng   2022-01-06  189  	size_t len = 0;
87da8dcc76b4aa Cixi Geng   2022-01-06  190  
87da8dcc76b4aa Cixi Geng   2022-01-06  191  	if (!data)
87da8dcc76b4aa Cixi Geng   2022-01-06  192  		return -EINVAL;
87da8dcc76b4aa Cixi Geng   2022-01-06  193  
87da8dcc76b4aa Cixi Geng   2022-01-06  194  	cell = nvmem_cell_get(data->dev, cell_name);
87da8dcc76b4aa Cixi Geng   2022-01-06  195  	if (IS_ERR_OR_NULL(cell))
87da8dcc76b4aa Cixi Geng   2022-01-06 @196  		return PTR_ERR(cell);

When functions return a both error pointers and NULL, then the NULL
return means that the feature has been deliberately disabled by the
admin.

nvmem_cell_get() cannot be disabled and never returns NULL so this code
should just be:

	if (IS_ERR(cell))
		return PTR_ERR(cell);

Otherwise we have to create the whole infrastructure to disable it.  We
can't just create random sprinklings of dead code to half way support
disabling the feature.

87da8dcc76b4aa Cixi Geng   2022-01-06  197  
87da8dcc76b4aa Cixi Geng   2022-01-06  198  	buf = nvmem_cell_read(cell, &len);
87da8dcc76b4aa Cixi Geng   2022-01-06  199  	if (IS_ERR_OR_NULL(buf)) {
87da8dcc76b4aa Cixi Geng   2022-01-06  200  		nvmem_cell_put(cell);
87da8dcc76b4aa Cixi Geng   2022-01-06  201  		return PTR_ERR(buf);

Same.

87da8dcc76b4aa Cixi Geng   2022-01-06  202  	}
87da8dcc76b4aa Cixi Geng   2022-01-06  203  
87da8dcc76b4aa Cixi Geng   2022-01-06  204  	memcpy(&calib_data, buf, min(len, sizeof(u32)));
87da8dcc76b4aa Cixi Geng   2022-01-06  205  
87da8dcc76b4aa Cixi Geng   2022-01-06  206  	kfree(buf);
87da8dcc76b4aa Cixi Geng   2022-01-06  207  	nvmem_cell_put(cell);
87da8dcc76b4aa Cixi Geng   2022-01-06  208  	return calib_data;
87da8dcc76b4aa Cixi Geng   2022-01-06  209  }

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

Re: [PATCH 6/7] iio: adc: sc27xx: add support for PMIC ump9620

[Dan Carpenter]

Hi Cixi,

url:    https://github.com/0day-ci/linux/commits/Cixi-Geng/iio-adc-sc27xx-adjust-structure-and-add-PMIC-s-support/20220106-210151
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: openrisc-randconfig-m031-20220106 (https://download.01.org/0day-ci/archive/20220108/202201080313.70ZrhufR-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 11.2.0

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

New smatch warnings:
drivers/iio/adc/sc27xx_adc.c:196 adc_nvmem_cell_calib_data() warn: passing zero to 'PTR_ERR'

vim +/PTR_ERR +196 drivers/iio/adc/sc27xx_adc.c

87da8dcc76b4aa Cixi Geng   2022-01-06  184  static int adc_nvmem_cell_calib_data(struct sc27xx_adc_data *data, const char *cell_name)
87da8dcc76b4aa Cixi Geng   2022-01-06  185  {
87da8dcc76b4aa Cixi Geng   2022-01-06  186  	struct nvmem_cell *cell;
87da8dcc76b4aa Cixi Geng   2022-01-06  187  	void *buf;
87da8dcc76b4aa Cixi Geng   2022-01-06  188  	u32 calib_data = 0;
87da8dcc76b4aa Cixi Geng   2022-01-06  189  	size_t len = 0;
87da8dcc76b4aa Cixi Geng   2022-01-06  190  
87da8dcc76b4aa Cixi Geng   2022-01-06  191  	if (!data)
87da8dcc76b4aa Cixi Geng   2022-01-06  192  		return -EINVAL;
87da8dcc76b4aa Cixi Geng   2022-01-06  193  
87da8dcc76b4aa Cixi Geng   2022-01-06  194  	cell = nvmem_cell_get(data->dev, cell_name);
87da8dcc76b4aa Cixi Geng   2022-01-06  195  	if (IS_ERR_OR_NULL(cell))
87da8dcc76b4aa Cixi Geng   2022-01-06 @196  		return PTR_ERR(cell);

When functions return a both error pointers and NULL, then the NULL
return means that the feature has been deliberately disabled by the
admin.

nvmem_cell_get() cannot be disabled and never returns NULL so this code
should just be:

	if (IS_ERR(cell))
		return PTR_ERR(cell);

Otherwise we have to create the whole infrastructure to disable it.  We
can't just create random sprinklings of dead code to half way support
disabling the feature.

87da8dcc76b4aa Cixi Geng   2022-01-06  197  
87da8dcc76b4aa Cixi Geng   2022-01-06  198  	buf = nvmem_cell_read(cell, &len);
87da8dcc76b4aa Cixi Geng   2022-01-06  199  	if (IS_ERR_OR_NULL(buf)) {
87da8dcc76b4aa Cixi Geng   2022-01-06  200  		nvmem_cell_put(cell);
87da8dcc76b4aa Cixi Geng   2022-01-06  201  		return PTR_ERR(buf);

Same.

87da8dcc76b4aa Cixi Geng   2022-01-06  202  	}
87da8dcc76b4aa Cixi Geng   2022-01-06  203  
87da8dcc76b4aa Cixi Geng   2022-01-06  204  	memcpy(&calib_data, buf, min(len, sizeof(u32)));
87da8dcc76b4aa Cixi Geng   2022-01-06  205  
87da8dcc76b4aa Cixi Geng   2022-01-06  206  	kfree(buf);
87da8dcc76b4aa Cixi Geng   2022-01-06  207  	nvmem_cell_put(cell);
87da8dcc76b4aa Cixi Geng   2022-01-06  208  	return calib_data;
87da8dcc76b4aa Cixi Geng   2022-01-06  209  }

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