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

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

[Cixi Geng]

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

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

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

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

-- 
2.25.1

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

[Cixi Geng]

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

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

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

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

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

[Cixi Geng]

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

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

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

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

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

[Cixi Geng]

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

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

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

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

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

[Cixi Geng]

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

sc2720 and sc2721 is the product of sc27xx series.

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

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

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

[Cixi Geng]

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

sc2730 is the product of sc27xx series.

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

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

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

[Cixi Geng]

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

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

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

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

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

[Cixi Geng]

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

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

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

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

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

[Rob Herring]

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

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

yamllint warnings/errors:

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

doc reference errors (make refcheckdocs):

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

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

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

pip3 install dtschema --upgrade

Please check and re-submit.

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

[Baolin Wang]

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

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

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


-- 
Baolin Wang

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

[Baolin Wang]

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

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

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

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

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


-- 
Baolin Wang

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

[Baolin Wang]

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

Why add an unused member?

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

Like previous comments, this is not needed.

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

Combine the two branches please.

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

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

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

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

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

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

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


-- 
Baolin Wang

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

[Baolin Wang]

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

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

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

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

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


-- 
Baolin Wang

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

[Baolin Wang]

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

Can you hide this into the pm runtime callbacks?

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

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

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

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

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

no need to initialize it.

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

Please use SET_RUNTIME_PM_OPS macro.

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


-- 
Baolin Wang

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

[Jonathan Cameron]

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

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

Fixes: tag for backports?

or is this having no visible result today?

Jonathan

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

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

[Jonathan Cameron]

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

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

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

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

...

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

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

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

...

> 

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

[Jonathan Cameron]

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

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

Thanks,

Jonathan

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

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

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

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

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


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

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

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

[Cixi Geng]

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

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

[Baolin Wang]

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

No.

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

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

-- 
Baolin Wang

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

[Cixi Geng]

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

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

[Baolin Wang]

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

Hi Jonathan,

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

-- 
Baolin Wang

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

[Cixi Geng]

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

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

[Jonathan Cameron]

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

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

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

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

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

Jonathan

> > --
> > Baolin Wang  

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

[Cixi Geng]

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