Skip to contentSkip to conversation

iio: adc: sc27xx: adjust structure and add PMIC's support

PATCH Superseded Cixi Geng 6 participants 26 messages Jan 6, 2022 – Mar 1, 2022

Subsystems (3)

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Cixi Geng

author
#0 lore raw

cover iio: adc: sc27xx: adjust structure and add PMIC's support

From: Cixi Geng <[email protected]>

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(-)
Signature · 1 line
2.25.1
author
#1 lore raw

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

From: Cixi Geng <[email protected]>

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

---
 .../bindings/iio/adc/sprd,sc2720-adc.yaml     | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
Documentation/devicetree/bindings/iio/adc/sprd,sc2720-adc.yaml +19 -0
index caa3ee0b4b8c..cd20ff17e58c 100644
@@ -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
Signature · 1 line
2.25.1
maintainer
#2 lore raw

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

Quoted 11 lines · Cixi Geng

From: Cixi Geng <[email protected]>

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

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

author
#3 lore raw

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

In reply to Cixi Geng

From: Cixi Geng <[email protected]>

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

---
 drivers/iio/adc/sc27xx_adc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/iio/adc/sc27xx_adc.c +2 -2
index 00098caf6d9e..aee076c8e2b1 100644
@@ -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)
Signature · 1 line
2.25.1
maintainer Reviewed-by on v1 2/7
#4 lore raw

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

Quoted 8 lines · Cixi Geng

From: Cixi Geng <[email protected]>

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

Quoted 22 lines
---
 drivers/iio/adc/sc27xx_adc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/iio/adc/sc27xx_adc.c +2 -3
index 00098caf6d9e..aee076c8e2b1 100644
@@ -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

Signature · 1 line
Baolin Wang
#5 lore raw

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

Quoted 11 lines · On Fri, 7 Jan 2022 14:55:15 +0800 Baolin Wang <[email protected]> wrote:
Quoted 8 lines · Cixi Geng

From: Cixi Geng <[email protected]>

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

Fixes tag for backports?

or is this having no visible result today?

Jonathan

Quoted 25 lines
Quoted 23 lines
---
 drivers/iio/adc/sc27xx_adc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/iio/adc/sc27xx_adc.c +2 -3
index 00098caf6d9e..aee076c8e2b1 100644
@@ -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

author
#6 lore raw

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

In reply to Cixi Geng

From: Cixi Geng <[email protected]>

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

---
 drivers/iio/adc/sc27xx_adc.c | 94 ++++++++++++++++++++++++++++++------
 1 file changed, 79 insertions(+), 15 deletions(-)
drivers/iio/adc/sc27xx_adc.c +79 -15
index aee076c8e2b1..d2712e54ee79 100644
@@ -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);
Signature · 1 line
2.25.1
maintainer
#7 lore raw

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

Quoted 56 lines · Cixi Geng

From: Cixi Geng <[email protected]>

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

---
 drivers/iio/adc/sc27xx_adc.c | 94 ++++++++++++++++++++++++++++++------
 1 file changed, 79 insertions(+), 15 deletions(-)
drivers/iio/adc/sc27xx_adc.c +19 -2
index aee076c8e2b1..d2712e54ee79 100644
@@ -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,
 };

drivers/iio/adc/sc27xx_adc.c static struct sc27xx_adc_linear_graph small_scale_graph = { · line 131

Quoted 10 lines
drivers/iio/adc/sc27xx_adc.c +9 -0
 
+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?

Quoted 35 lines
drivers/iio/adc/sc27xx_adc.c +8 -3
+
 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)

drivers/iio/adc/sc27xx_adc.c static int sc27xx_adc_get_ratio(int channel, int scale) · line 228

Quoted 10 lines
drivers/iio/adc/sc27xx_adc.c +10 -0
+{
+       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.

Quoted 121 lines
drivers/iio/adc/sc27xx_adc.c +33 -11
+
 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

Signature · 1 line
Baolin Wang
author
#8 lore raw

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

Quoted 112 lines · Baolin Wang <[email protected]> 于2022年1月7日周五 15:03写道:
Quoted 56 lines · Cixi Geng

From: Cixi Geng <[email protected]>

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

---
 drivers/iio/adc/sc27xx_adc.c | 94 ++++++++++++++++++++++++++++++------
 1 file changed, 79 insertions(+), 15 deletions(-)
drivers/iio/adc/sc27xx_adc.c +19 -2
index aee076c8e2b1..d2712e54ee79 100644
@@ -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,
 };

drivers/iio/adc/sc27xx_adc.c static struct sc27xx_adc_linear_graph small_scale_graph = { · line 131

Quoted 10 lines
drivers/iio/adc/sc27xx_adc.c +9 -0
 
+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?

Quoted 40 lines
drivers/iio/adc/sc27xx_adc.c +13 -3
+
 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)
Quoted 8 lines

drivers/iio/adc/sc27xx_adc.c static int sc27xx_adc_get_ratio(int channel, int scale) · line 228

Quoted 5 lines
drivers/iio/adc/sc27xx_adc.c +5 -0
+                       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

Quoted 126 lines
Quoted 120 lines
drivers/iio/adc/sc27xx_adc.c +33 -11
+
 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);
--
Quoted 2 lines

2.25.1

--
Baolin Wang

maintainer
#9 lore raw

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

Quoted 122 lines · Cixi Geng
Quoted 112 lines · Baolin Wang <[email protected]> 于2022年1月7日周五 15:03写道:
Quoted 56 lines · Cixi Geng

From: Cixi Geng <[email protected]>

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

---
 drivers/iio/adc/sc27xx_adc.c | 94 ++++++++++++++++++++++++++++++------
 1 file changed, 79 insertions(+), 15 deletions(-)
drivers/iio/adc/sc27xx_adc.c +19 -2
index aee076c8e2b1..d2712e54ee79 100644
@@ -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,
 };

drivers/iio/adc/sc27xx_adc.c static struct sc27xx_adc_linear_graph small_scale_graph = { · line 131

Quoted 10 lines
drivers/iio/adc/sc27xx_adc.c +9 -0
 
+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?

Quoted 40 lines
drivers/iio/adc/sc27xx_adc.c +13 -3
+
 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)
Quoted 8 lines

drivers/iio/adc/sc27xx_adc.c static int sc27xx_adc_get_ratio(int channel, int scale) · line 228

Quoted 5 lines
drivers/iio/adc/sc27xx_adc.c +5 -0
+                       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.

Quoted 2 lines

Did you mean that all the PMIC's scale_init function should put into the sc27xx_adc_write_raw?

No.

Quoted 3 lines

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.

Signature · 1 line
Baolin Wang
author
#10 lore raw

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

Quoted 129 lines · Baolin Wang <[email protected]> 于2022年1月17日周一 14:15写道:
Quoted 122 lines · Cixi Geng
Quoted 112 lines · Baolin Wang <[email protected]> 于2022年1月7日周五 15:03写道:
Quoted 56 lines · Cixi Geng

From: Cixi Geng <[email protected]>

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

---
 drivers/iio/adc/sc27xx_adc.c | 94 ++++++++++++++++++++++++++++++------
 1 file changed, 79 insertions(+), 15 deletions(-)
drivers/iio/adc/sc27xx_adc.c +19 -2
index aee076c8e2b1..d2712e54ee79 100644
@@ -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,
 };

drivers/iio/adc/sc27xx_adc.c static struct sc27xx_adc_linear_graph small_scale_graph = { · line 131

Quoted 10 lines
drivers/iio/adc/sc27xx_adc.c +9 -0
 
+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?

Quoted 40 lines
drivers/iio/adc/sc27xx_adc.c +13 -3
+
 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)
Quoted 8 lines

drivers/iio/adc/sc27xx_adc.c static int sc27xx_adc_get_ratio(int channel, int scale) · line 228

Quoted 5 lines
drivers/iio/adc/sc27xx_adc.c +5 -0
+                       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.

Quoted 2 lines

Did you mean that all the PMIC's scale_init function should put into the sc27xx_adc_write_raw?

No.

Quoted 7 lines
Quoted 3 lines

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.

Quoted 3 lines

--
Baolin Wang

maintainer
#11 lore raw

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

Quoted 138 lines · Cixi Geng
Quoted 129 lines · Baolin Wang <[email protected]> 于2022年1月17日周一 14:15写道:
Quoted 122 lines · Cixi Geng
Quoted 112 lines · Baolin Wang <[email protected]> 于2022年1月7日周五 15:03写道:
Quoted 56 lines · Cixi Geng

From: Cixi Geng <[email protected]>

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

---
 drivers/iio/adc/sc27xx_adc.c | 94 ++++++++++++++++++++++++++++++------
 1 file changed, 79 insertions(+), 15 deletions(-)
drivers/iio/adc/sc27xx_adc.c +19 -2
index aee076c8e2b1..d2712e54ee79 100644
@@ -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,
 };

drivers/iio/adc/sc27xx_adc.c static struct sc27xx_adc_linear_graph small_scale_graph = { · line 131

Quoted 10 lines
drivers/iio/adc/sc27xx_adc.c +9 -0
 
+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?

Quoted 40 lines
drivers/iio/adc/sc27xx_adc.c +13 -3
+
 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)
Quoted 8 lines

drivers/iio/adc/sc27xx_adc.c static int sc27xx_adc_get_ratio(int channel, int scale) · line 228

Quoted 5 lines
drivers/iio/adc/sc27xx_adc.c +5 -0
+                       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.

Quoted 2 lines

Did you mean that all the PMIC's scale_init function should put into the sc27xx_adc_write_raw?

No.

Quoted 7 lines
Quoted 3 lines

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.

Quoted 6 lines

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.

Signature · 1 line
Baolin Wang
author
#12 lore raw

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

Quoted 147 lines · Baolin Wang <[email protected]> 于2022年2月10日周四 16:07写道:
Quoted 138 lines · Cixi Geng
Quoted 129 lines · Baolin Wang <[email protected]> 于2022年1月17日周一 14:15写道:
Quoted 122 lines · Cixi Geng
Quoted 112 lines · Baolin Wang <[email protected]> 于2022年1月7日周五 15:03写道:
Quoted 56 lines · Cixi Geng

From: Cixi Geng <[email protected]>

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

---
 drivers/iio/adc/sc27xx_adc.c | 94 ++++++++++++++++++++++++++++++------
 1 file changed, 79 insertions(+), 15 deletions(-)
drivers/iio/adc/sc27xx_adc.c +19 -2
index aee076c8e2b1..d2712e54ee79 100644
@@ -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,
 };

drivers/iio/adc/sc27xx_adc.c static struct sc27xx_adc_linear_graph small_scale_graph = { · line 131

Quoted 10 lines
drivers/iio/adc/sc27xx_adc.c +9 -0
 
+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?

Quoted 40 lines
drivers/iio/adc/sc27xx_adc.c +13 -3
+
 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)
Quoted 8 lines

drivers/iio/adc/sc27xx_adc.c static int sc27xx_adc_get_ratio(int channel, int scale) · line 228

Quoted 5 lines
drivers/iio/adc/sc27xx_adc.c +5 -0
+                       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.

Quoted 2 lines

Did you mean that all the PMIC's scale_init function should put into the sc27xx_adc_write_raw?

No.

Quoted 7 lines
Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 5 lines

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.

Quoted 2 lines

--
Baolin Wang

#13 lore raw

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

Quoted 148 lines · On Wed, 23 Feb 2022 20:46:08 +0800 Cixi Geng <[email protected]> wrote:
Quoted 147 lines · Baolin Wang <[email protected]> 于2022年2月10日周四 16:07写道:
Quoted 144 lines · Cixi Geng
Quoted 129 lines · Baolin Wang <[email protected]> 于2022年1月17日周一 14:15写道:
Quoted 122 lines · Cixi Geng
Quoted 112 lines · Baolin Wang <[email protected]> 于2022年1月7日周五 15:03写道:
Quoted 56 lines · Cixi Geng

From: Cixi Geng <[email protected]>

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

---
 drivers/iio/adc/sc27xx_adc.c | 94 ++++++++++++++++++++++++++++++------
 1 file changed, 79 insertions(+), 15 deletions(-)
drivers/iio/adc/sc27xx_adc.c +19 -2
index aee076c8e2b1..d2712e54ee79 100644
@@ -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,
 };

drivers/iio/adc/sc27xx_adc.c static struct sc27xx_adc_linear_graph small_scale_graph = { · line 131

Quoted 10 lines
drivers/iio/adc/sc27xx_adc.c +9 -0
 
+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?

Quoted 40 lines
drivers/iio/adc/sc27xx_adc.c +13 -3
+
 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)
Quoted 8 lines

drivers/iio/adc/sc27xx_adc.c static int sc27xx_adc_get_ratio(int channel, int scale) · line 228

Quoted 5 lines
drivers/iio/adc/sc27xx_adc.c +5 -0
+                       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.

Quoted 2 lines

Did you mean that all the PMIC's scale_init function should put into the sc27xx_adc_write_raw?

No.

Quoted 7 lines
Quoted 3 lines

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.

Quoted 8 lines
Quoted 5 lines

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

Quoted 2 lines
Quoted 2 lines

--
Baolin Wang

author
#14 lore raw

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

Quoted 177 lines · Jonathan Cameron <[email protected]> 于2022年2月25日周五 18:19写道:
Quoted 148 lines · On Wed, 23 Feb 2022 20:46:08 +0800 Cixi Geng <[email protected]> wrote:
Quoted 147 lines · Baolin Wang <[email protected]> 于2022年2月10日周四 16:07写道:
Quoted 144 lines · Cixi Geng
Quoted 129 lines · Baolin Wang <[email protected]> 于2022年1月17日周一 14:15写道:
Quoted 122 lines · Cixi Geng
Quoted 112 lines · Baolin Wang <[email protected]> 于2022年1月7日周五 15:03写道:
Quoted 56 lines · Cixi Geng

From: Cixi Geng <[email protected]>

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

---
 drivers/iio/adc/sc27xx_adc.c | 94 ++++++++++++++++++++++++++++++------
 1 file changed, 79 insertions(+), 15 deletions(-)
drivers/iio/adc/sc27xx_adc.c +19 -2
index aee076c8e2b1..d2712e54ee79 100644
@@ -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,
 };

drivers/iio/adc/sc27xx_adc.c static struct sc27xx_adc_linear_graph small_scale_graph = { · line 131

Quoted 10 lines
drivers/iio/adc/sc27xx_adc.c +9 -0
 
+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?

Quoted 40 lines
drivers/iio/adc/sc27xx_adc.c +13 -3
+
 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)
Quoted 8 lines

drivers/iio/adc/sc27xx_adc.c static int sc27xx_adc_get_ratio(int channel, int scale) · line 228

Quoted 5 lines
drivers/iio/adc/sc27xx_adc.c +5 -0
+                       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.

Quoted 2 lines

Did you mean that all the PMIC's scale_init function should put into the sc27xx_adc_write_raw?

No.

Quoted 7 lines
Quoted 3 lines

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.

Quoted 8 lines
Quoted 5 lines

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.

Quoted 2 lines

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!

Quoted 3 lines
Quoted 2 lines
Quoted 2 lines

--
Baolin Wang

author
#15 lore raw

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

In reply to Cixi Geng

From: Cixi Geng <[email protected]>

sc2720 and sc2721 is the product of sc27xx series.

---
 drivers/iio/adc/sc27xx_adc.c | 198 +++++++++++++++++++++++++++++++++++
 1 file changed, 198 insertions(+)
drivers/iio/adc/sc27xx_adc.c +198 -0
index d2712e54ee79..7b5c66660ac9 100644
@@ -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);
Signature · 1 line
2.25.1
maintainer
#16 lore raw

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

Quoted 46 lines · Cixi Geng

From: Cixi Geng <[email protected]>

sc2720 and sc2721 is the product of sc27xx series.

---
 drivers/iio/adc/sc27xx_adc.c | 198 +++++++++++++++++++++++++++++++++++
 1 file changed, 198 insertions(+)
drivers/iio/adc/sc27xx_adc.c +7 -0
index d2712e54ee79..7b5c66660ac9 100644
@@ -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
+

drivers/iio/adc/sc27xx_adc.c line 86

Quoted 10 lines
drivers/iio/adc/sc27xx_adc.c +9 -0
+/* 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?

Quoted 129 lines
drivers/iio/adc/sc27xx_adc.c +107 -0
        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:

drivers/iio/adc/sc27xx_adc.c static int sc2731_adc_get_ratio(int channel, int scale) · line 350

Quoted 10 lines
drivers/iio/adc/sc27xx_adc.c +10 -0
+               case 30:
+               case 31:
+                       data->channel_scale[i] = 3;
+                       break;
+               default:
+                       data->channel_scale[i] = 0;
+                       break;
+               }
+       }
+}

Like previous comments, this is not needed.

Quoted 5 lines
drivers/iio/adc/sc27xx_adc.c +1 -0
+
 static void sc2731_adc_scale_init(struct sc27xx_adc_data *data)
 {
        int i;

drivers/iio/adc/sc27xx_adc.c static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel, · line 382

Quoted 10 lines
drivers/iio/adc/sc27xx_adc.c +7 -0
@@ -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.

Quoted 71 lines
drivers/iio/adc/sc27xx_adc.c +46 -0
+                       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;

drivers/iio/adc/sc27xx_adc.c static int sc27xx_adc_probe(struct platform_device *pdev) · line 774

Quoted 6 lines
drivers/iio/adc/sc27xx_adc.c +3 -0
@@ -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().

drivers/iio/adc/sc27xx_adc.c static int sc27xx_adc_probe(struct platform_device *pdev) · line 775

Quoted 1 line
drivers/iio/adc/sc27xx_adc.c +1 -0
+                       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.

drivers/iio/adc/sc27xx_adc.c static int sc27xx_adc_probe(struct platform_device *pdev) · line 776

Quoted 1 line
drivers/iio/adc/sc27xx_adc.c +1 -0
+                       dev_err(dev, "err! ADC volref, err: %d\n", ret);

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

Quoted 19 lines
drivers/iio/adc/sc27xx_adc.c +6 -1
+                       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

Signature · 1 line
Baolin Wang
#17 lore raw

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

Quoted 44 lines · On Fri, 7 Jan 2022 15:16:15 +0800 Baolin Wang <[email protected]> wrote:
Quoted 43 lines · Cixi Geng

From: Cixi Geng <[email protected]>

sc2720 and sc2721 is the product of sc27xx series.

---
 drivers/iio/adc/sc27xx_adc.c | 198 +++++++++++++++++++++++++++++++++++
 1 file changed, 198 insertions(+)
drivers/iio/adc/sc27xx_adc.c +4 -0
index d2712e54ee79..7b5c66660ac9 100644
@@ -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)
 
Quoted 15 lines

drivers/iio/adc/sc27xx_adc.c line 86

Quoted 13 lines
drivers/iio/adc/sc27xx_adc.c +12 -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 :)

Quoted 9 lines
Quoted 9 lines
drivers/iio/adc/sc27xx_adc.c +1 -0
        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.
  */

...

Quoted 9 lines
Quoted 9 lines
drivers/iio/adc/sc27xx_adc.c +1 -0
+
 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;
        }
 
Quoted 4 lines

drivers/iio/adc/sc27xx_adc.c static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel, · line 379

Quoted 4 lines
drivers/iio/adc/sc27xx_adc.c +4 -0
+       /*
+        * 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...

Quoted 39 lines

drivers/iio/adc/sc27xx_adc.c static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel, · line 382

Quoted 3 lines
drivers/iio/adc/sc27xx_adc.c +3 -0
+        */
+       if (data->var_data->pmic_type == SC2721_ADC) {
+               if ((channel == 30) || (channel == 31)) {  

Combine the two branches please.

Quoted 33 lines
drivers/iio/adc/sc27xx_adc.c +21 -0
+                       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)
 }

...

Quoted 1 line
build bot
#18 lore raw

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

In reply to Cixi Geng
CC: kbuild-all(a)lists.01.org In-Reply-To: <[email protected]> References: <[email protected]> TO: Cixi Geng <[email protected]> TO…

CC: kbuild-all(a)lists.01.org
In-Reply-To: <[email protected]>
References: <[email protected]>
TO: Cixi Geng <[email protected]>
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

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

author
#19 lore raw

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

In reply to Cixi Geng

From: Cixi Geng <[email protected]>

sc2730 is the product of sc27xx series.

---
 drivers/iio/adc/sc27xx_adc.c | 105 +++++++++++++++++++++++++++++++++++
 1 file changed, 105 insertions(+)
drivers/iio/adc/sc27xx_adc.c +105 -0
index 7b5c66660ac9..195f44cf61e1 100644
@@ -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},
 	{ }
Signature · 1 line
2.25.1
author
#20 lore raw

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

In reply to Cixi Geng

From: Cixi Geng <[email protected]>

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

---
 drivers/iio/adc/sc27xx_adc.c | 263 +++++++++++++++++++++++++++++++++--
 1 file changed, 254 insertions(+), 9 deletions(-)
drivers/iio/adc/sc27xx_adc.c +254 -9
index 195f44cf61e1..68b967f32498 100644
@@ -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);
Signature · 1 line
2.25.1
maintainer
#21 lore raw

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

Quoted 78 lines · Cixi Geng

From: Cixi Geng <[email protected]>

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

---
 drivers/iio/adc/sc27xx_adc.c | 263 +++++++++++++++++++++++++++++++++--
 1 file changed, 254 insertions(+), 9 deletions(-)
drivers/iio/adc/sc27xx_adc.c +32 -0
index 195f44cf61e1..68b967f32498 100644
@@ -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)) {

drivers/iio/adc/sc27xx_adc.c static int sc27xx_adc_get_calib_data(u32 calib_data, int calib_adc) · line 209

Quoted 10 lines
drivers/iio/adc/sc27xx_adc.c +10 -0
+               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.

Quoted 137 lines
drivers/iio/adc/sc27xx_adc.c +111 -0
+
 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;

drivers/iio/adc/sc27xx_adc.c static int sc27xx_adc_to_volt(struct sc27xx_adc_linear_graph *graph, · line 741

Quoted 10 lines
drivers/iio/adc/sc27xx_adc.c +8 -0
 }
 
+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.

Quoted 157 lines
drivers/iio/adc/sc27xx_adc.c +93 -10
+
+       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

Signature · 1 line
Baolin Wang
build bot
#22 lore raw

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

In reply to Cixi Geng
CC: kbuild-all(a)lists.01.org In-Reply-To: <[email protected]> References: <[email protected]> TO: Cixi Geng <[email protected]> TO…

CC: kbuild-all(a)lists.01.org
In-Reply-To: <[email protected]>
References: <[email protected]>
TO: Cixi Geng <[email protected]>
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

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

author
#23 lore raw

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

In reply to Cixi Geng

From: Cixi Geng <[email protected]>

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

---
 drivers/iio/adc/sc27xx_adc.c | 103 ++++++++++++++++++++++++++++++++++-
 1 file changed, 102 insertions(+), 1 deletion(-)
drivers/iio/adc/sc27xx_adc.c +102 -1
index 68b967f32498..cecda8d53474 100644
@@ -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,
 	},
 };
 
Signature · 1 line
2.25.1
maintainer
#24 lore raw

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

Quoted 68 lines · Cixi Geng

From: Cixi Geng <[email protected]>

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

---
 drivers/iio/adc/sc27xx_adc.c | 103 ++++++++++++++++++++++++++++++++++-
 1 file changed, 102 insertions(+), 1 deletion(-)
drivers/iio/adc/sc27xx_adc.c +16 -0
index 68b967f32498..cecda8d53474 100644
@@ -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 */

drivers/iio/adc/sc27xx_adc.c static int sc27xx_adc_enable(struct sc27xx_adc_data *data) · line 1010

Quoted 7 lines
drivers/iio/adc/sc27xx_adc.c +4 -0
@@ -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?

Quoted 50 lines
drivers/iio/adc/sc27xx_adc.c +17 -1
+
        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)

drivers/iio/adc/sc27xx_adc.c static int sc27xx_adc_probe(struct platform_device *pdev) · line 1196

Quoted 10 lines
drivers/iio/adc/sc27xx_adc.c +9 -0
+       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?

Quoted 6 lines
drivers/iio/adc/sc27xx_adc.c +2 -0
+               pm_runtime_disable(dev);
+       }
 
        return ret;
 }

drivers/iio/adc/sc27xx_adc.c static const struct of_device_id sc27xx_adc_of_match[] = { · line 1219

Quoted 10 lines
drivers/iio/adc/sc27xx_adc.c +7 -0
@@ -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?

Quoted 15 lines
drivers/iio/adc/sc27xx_adc.c +15 -0
+               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 */

drivers/iio/adc/sc27xx_adc.c static const struct of_device_id sc27xx_adc_of_match[] = { · line 1244

Quoted 10 lines
drivers/iio/adc/sc27xx_adc.c +10 -0
+       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.

Quoted 9 lines
drivers/iio/adc/sc27xx_adc.c +9 -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;

drivers/iio/adc/sc27xx_adc.c static const struct of_device_id sc27xx_adc_of_match[] = { · line 1263

Quoted 10 lines
drivers/iio/adc/sc27xx_adc.c +10 -0
+               }
+       }
+
+       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.

Quoted 14 lines
drivers/iio/adc/sc27xx_adc.c +3 -1
+
 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

Signature · 1 line
Baolin Wang
#25 lore raw

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

Quoted 9 lines · On Fri, 7 Jan 2022 15:34:32 +0800 Baolin Wang <[email protected]> wrote:
Quoted 8 lines · Cixi Geng

From: Cixi Geng <[email protected]>

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

A few additional comments from me inline,

Thanks,

Jonathan

Quoted 93 lines
Quoted 60 lines
---
 drivers/iio/adc/sc27xx_adc.c | 103 ++++++++++++++++++++++++++++++++++-
 1 file changed, 102 insertions(+), 1 deletion(-)
drivers/iio/adc/sc27xx_adc.c +16 -0
index 68b967f32498..cecda8d53474 100644
@@ -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 */

drivers/iio/adc/sc27xx_adc.c static int sc27xx_adc_enable(struct sc27xx_adc_data *data) · line 1010

Quoted 7 lines
drivers/iio/adc/sc27xx_adc.c +4 -0
@@ -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?

Quoted 23 lines
drivers/iio/adc/sc27xx_adc.c +6 -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);
 
Quoted 6 lines

drivers/iio/adc/sc27xx_adc.c static int sc27xx_adc_probe(struct platform_device *pdev) · line 1166

Quoted 6 lines
drivers/iio/adc/sc27xx_adc.c +1 -0
        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.

Quoted 26 lines
Quoted 26 lines
drivers/iio/adc/sc27xx_adc.c +14 -1
                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;
+       }
+
Quoted 8 lines

drivers/iio/adc/sc27xx_adc.c static int sc27xx_adc_probe(struct platform_device *pdev) · line 1196

Quoted 5 lines
drivers/iio/adc/sc27xx_adc.c +5 -0
+       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.

Quoted 10 lines
Quoted 10 lines
drivers/iio/adc/sc27xx_adc.c +2 -0
+               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);
 
Quoted 15 lines

drivers/iio/adc/sc27xx_adc.c static const struct of_device_id sc27xx_adc_of_match[] = { · line 1219

Quoted 7 lines
drivers/iio/adc/sc27xx_adc.c +7 -0
+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?

Quoted 5 lines
drivers/iio/adc/sc27xx_adc.c +5 -0
+               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.

Quoted 62 lines
Quoted 10 lines
drivers/iio/adc/sc27xx_adc.c +10 -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 */

drivers/iio/adc/sc27xx_adc.c static const struct of_device_id sc27xx_adc_of_match[] = { · line 1244

Quoted 10 lines
drivers/iio/adc/sc27xx_adc.c +10 -0
+       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.

Quoted 10 lines
drivers/iio/adc/sc27xx_adc.c +9 -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;

drivers/iio/adc/sc27xx_adc.c static const struct of_device_id sc27xx_adc_of_match[] = { · line 1263

Quoted 10 lines
drivers/iio/adc/sc27xx_adc.c +10 -0
+               }
+       }
+
+       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.

Quoted 14 lines
drivers/iio/adc/sc27xx_adc.c +3 -1
+
 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