[PATCH V0 0/2] Add support for UMP serise pmic adc

[PATCH V0 0/2] Add support for UMP serise pmic adc

[Mingjin Yang]

Patch 1, add UMP518 pmic adc description and sample in dt-bindings.
Patch 2, add UMP serise pmic adc driver, support both voltage and current mode.

Mingjin Yang (2):
  dt-bindings: iio: adc: Add support for ump518 pmic adc
  iio: adc: sprd_pmic_adc: Add support for UMP serise pmic adc

 .../bindings/iio/adc/sprd,sc2720-adc.yaml     |    2 +
 drivers/iio/adc/Kconfig                       |   10 +
 drivers/iio/adc/Makefile                      |    1 +
 drivers/iio/adc/sprd_pmic_adc.c               | 1298 +++++++++++++++++
 4 files changed, 1311 insertions(+)
 create mode 100644 drivers/iio/adc/sprd_pmic_adc.c

-- 
2.17.1

[PATCH V0 1/2] dt-bindings: iio: adc: Add support for ump518 pmic adc

[Mingjin Yang]

Add ump518 pmic adc description and sample in dt-bindings.

Signed-off-by: Mingjin Yang <mingjin.yang@unisoc.com>
---
 Documentation/devicetree/bindings/iio/adc/sprd,sc2720-adc.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/sprd,sc2720-adc.yaml b/Documentation/devicetree/bindings/iio/adc/sprd,sc2720-adc.yaml
index 8181cf9a8e07..f28d188d83da 100644
--- a/Documentation/devicetree/bindings/iio/adc/sprd,sc2720-adc.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/sprd,sc2720-adc.yaml
@@ -21,6 +21,7 @@ properties:
       - sprd,sc2730-adc
       - sprd,sc2731-adc
       - sprd,ump9620-adc
+      - sprd,ump518-adc
 
   reg:
     maxItems: 1
@@ -46,6 +47,7 @@ allOf:
             contains:
               enum:
                 - sprd,ump9620-adc
+                - sprd,ump518-adc
     then:
       properties:
         nvmem-cells:
-- 
2.17.1

Re: [PATCH V0 1/2] dt-bindings: iio: adc: Add support for ump518 pmic adc

[Conor Dooley]

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

On Wed, Aug 16, 2023 at 04:02:24PM +0800, Mingjin Yang wrote:
> Add ump518 pmic adc description and sample in dt-bindings.
> 
> Signed-off-by: Mingjin Yang <mingjin.yang@unisoc.com>
> ---
>  Documentation/devicetree/bindings/iio/adc/sprd,sc2720-adc.yaml | 2 ++

Acked-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/sprd,sc2720-adc.yaml b/Documentation/devicetree/bindings/iio/adc/sprd,sc2720-adc.yaml
> index 8181cf9a8e07..f28d188d83da 100644
> --- a/Documentation/devicetree/bindings/iio/adc/sprd,sc2720-adc.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/sprd,sc2720-adc.yaml
> @@ -21,6 +21,7 @@ properties:
>        - sprd,sc2730-adc
>        - sprd,sc2731-adc
>        - sprd,ump9620-adc
> +      - sprd,ump518-adc
>  
>    reg:
>      maxItems: 1
> @@ -46,6 +47,7 @@ allOf:
>              contains:
>                enum:
>                  - sprd,ump9620-adc
> +                - sprd,ump518-adc
>      then:
>        properties:
>          nvmem-cells:
> -- 
> 2.17.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH V0 1/2] dt-bindings: iio: adc: Add support for ump518 pmic adc

[Jonathan Cameron]

On Wed, 16 Aug 2023 15:44:55 +0100
Conor Dooley <conor@kernel.org> wrote:

> On Wed, Aug 16, 2023 at 04:02:24PM +0800, Mingjin Yang wrote:
> > Add ump518 pmic adc description and sample in dt-bindings.
ADC

Description?  I'm seeing the compatible only which doesn't seem like
a description to me.



> > 
> > Signed-off-by: Mingjin Yang <mingjin.yang@unisoc.com>
> > ---
> >  Documentation/devicetree/bindings/iio/adc/sprd,sc2720-adc.yaml | 2 ++  
> 
> Acked-by: Conor Dooley <conor.dooley@microchip.com>
> 
> Thanks,
> Conor.
> 
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/sprd,sc2720-adc.yaml b/Documentation/devicetree/bindings/iio/adc/sprd,sc2720-adc.yaml
> > index 8181cf9a8e07..f28d188d83da 100644
> > --- a/Documentation/devicetree/bindings/iio/adc/sprd,sc2720-adc.yaml
> > +++ b/Documentation/devicetree/bindings/iio/adc/sprd,sc2720-adc.yaml
> > @@ -21,6 +21,7 @@ properties:
> >        - sprd,sc2730-adc
> >        - sprd,sc2731-adc
> >        - sprd,ump9620-adc
> > +      - sprd,ump518-adc

alpha-numeric ordering probably worth retaining.
So I'd move this and the case below up one line.

Thanks,

Jonathan

> >  
> >    reg:
> >      maxItems: 1
> > @@ -46,6 +47,7 @@ allOf:
> >              contains:
> >                enum:
> >                  - sprd,ump9620-adc
> > +                - sprd,ump518-adc
> >      then:
> >        properties:
> >          nvmem-cells:
> > -- 
> > 2.17.1
> >   

[PATCH V0 2/2] iio: adc: sprd_pmic_adc: Add support for UMP serise pmic adc

[Mingjin Yang]

This patch adds support for UNISOC UMP family
of ADC driver. The ADC peripheral can support both
voltage and current mode whose input signal is
connected to the PMIC ADC AMUX.

Signed-off-by: Mingjin Yang <mingjin.yang@unisoc.com>
---
 drivers/iio/adc/Kconfig         |   10 +
 drivers/iio/adc/Makefile        |    1 +
 drivers/iio/adc/sprd_pmic_adc.c | 1298 +++++++++++++++++++++++++++++++
 3 files changed, 1309 insertions(+)
 create mode 100644 drivers/iio/adc/sprd_pmic_adc.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index dc14bde31ac1..39476abfbd8a 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1375,6 +1375,16 @@ config TWL6030_GPADC
 	  This driver can also be built as a module. If so, the module will be
 	  called twl6030-gpadc.
 
+config SPRD_PMIC_ADC
+	tristate "Unisoc PMIC ADC"
+	depends on MFD_SC27XX_PMIC || COMPILE_TEST
+	help
+	  Say yes here to build support for the integrated ADC inside the
+	  Unisoc PMICs.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called sprd_pmic_adc.
+
 config VF610_ADC
 	tristate "Freescale vf610 ADC driver"
 	depends on HAS_IOMEM
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index eb6e891790fb..1956dea9d543 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -95,6 +95,7 @@ obj-$(CONFIG_RICHTEK_RTQ6056) += rtq6056.o
 obj-$(CONFIG_RZG2L_ADC) += rzg2l_adc.o
 obj-$(CONFIG_SC27XX_ADC) += sc27xx_adc.o
 obj-$(CONFIG_SPEAR_ADC) += spear_adc.o
+obj-$(CONFIG_SPRD_PMIC_ADC) += sprd_pmic_adc.o
 obj-$(CONFIG_SUN4I_GPADC) += sun4i-gpadc-iio.o
 obj-$(CONFIG_STM32_ADC_CORE) += stm32-adc-core.o
 obj-$(CONFIG_STM32_ADC) += stm32-adc.o
diff --git a/drivers/iio/adc/sprd_pmic_adc.c b/drivers/iio/adc/sprd_pmic_adc.c
new file mode 100644
index 000000000000..6113bd84cd77
--- /dev/null
+++ b/drivers/iio/adc/sprd_pmic_adc.c
@@ -0,0 +1,1298 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2020 Unisoc Inc.
+
+#include <linux/hwspinlock.h>
+#include <linux/iio/iio.h>
+#include <linux/math64.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/nvmem-consumer.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+#include <linux/sort.h>
+
+/* ADC controller registers definition */
+#define SPRD_ADC_CTL			0x0
+
+#define SPRD_ADC_CH_CFG			0x4
+#define SPRD_ADC_CHN_ID_MASK		GENMASK(4, 0)
+
+#define SPRD_ADC_DATA			0x4c
+#define SPRD_ADC_DATA_MASK		GENMASK(11, 0)
+
+#define SPRD_ADC_INT_EN			0x50
+#define SPRD_ADC_IRQ_EN			BIT(0)
+
+#define SPRD_ADC_INT_CLR		0x54
+#define SPRD_ADC_IRQ_CLR		BIT(0)
+
+#define SPRD_ADC_INT_STS		0x58
+
+#define SPRD_ADC_INT_RAW		0x5c
+#define SPRD_ADC_IRQ_RAW		BIT(0)
+
+/* Bits and mask definition for SPRD_ADC_CTL register */
+#define SPRD_ADC_EN			BIT(0)
+#define SPRD_ADC_CHN_RUN		BIT(1)
+#define SPRD_ADC_12BIT_MODE		BIT(2)
+#define SPRD_ADC_RUN_NUM_MASK		GENMASK(7, 4)
+#define SPRD_ADC_RUN_NUM_SHIFT		4
+#define SPRD_ADC_AVERAGE_SHIFT		8
+#define SPRD_ADC_AVERAGE_MASK		GENMASK(10, 8)
+
+/* Timeout (ms) for the trylock of hardware spinlocks */
+#define SPRD_ADC_HWLOCK_TIMEOUT		5000
+
+/* Maximum ADC channel number */
+#define SPRD_ADC_CHANNEL_MAX		32
+
+/* Timeout (us) for ADC data conversion according to ADC datasheet */
+#define SPRD_ADC_RDY_TIMEOUT		1000000
+#define SPRD_ADC_POLL_RAW_STATUS	500
+
+/* ADC voltage ratio definition */
+#define SPRD_RATIO_NUMERATOR_OFFSET	16
+#define SPRD_RATIO_DENOMINATOR_MASK	GENMASK(15, 0)
+#define RATIO(n, d)			(((n) << SPRD_RATIO_NUMERATOR_OFFSET) | (d))
+#define R_INVALID			(0xffffffff)
+#define RATIO_DEF			RATIO(1, 1)
+
+/* ADC specific channel reference voltage 3.5V */
+#define SPRD_ADC_REFVOL_VDD35		3500000
+
+/* ADC default channel reference voltage is 2.8V */
+#define SPRD_ADC_REFVOL_VDD28		2800000
+
+#define SPRD_ADC_CELL_MAX		(2)
+#define SPRD_ADC_INVALID_DATA		(0XFFFFFFFF)
+#define SPRD_ADC_INIT_MAGIC		(0xa7a77a7a)
+#define ADC_MESURE_NUMBER_SW		(15)
+#define ADC_MESURE_NUMBER_HW_DEF	(3) /* 2 << 3 = 8 times */
+
+#define CH_DATA_FMT "ch%d_d[(%d %d 0x%x 0x%x) | (%u/%u), (%u/%u), (%u/%u), (%u/%u), (%u/%u)]\n"
+
+#define SPRD_ADC_CHANNEL(index, mask) {				\
+	.type = IIO_VOLTAGE,					\
+	.channel = index,					\
+	.info_mask_separate = mask | BIT(IIO_CHAN_INFO_SCALE),	\
+	.datasheet_name = "CH##index",				\
+	.indexed = 1,						\
+}
+
+#define CH_DATA_INIT(sl, filter, isen, ip_r)		\
+{							\
+	.scale = sl,					\
+	.isen_info = isen,				\
+	.filter_info = filter,				\
+	.ip_ratio = ip_r,				\
+	.ch_aux_ratio = {0, 0, 0, 0},			\
+	.inited = SPRD_ADC_INIT_MAGIC,			\
+}
+
+#define CALIB_INFO_INIT(ch, sl, gph)	\
+{					\
+	.channel = ch,			\
+	.scale = sl,			\
+	.graph = gph,			\
+	.inited = SPRD_ADC_INIT_MAGIC,	\
+}
+
+#define GRAPH_POINT_INIT(v0, a0, v1, a1)\
+					\
+	.volt0 = v0,			\
+	.adc0 = a0,			\
+	.volt1 = v1,			\
+	.adc1 = a1,			\
+	.inited = SPRD_ADC_INIT_MAGIC
+
+#define REG_BIT_INIT(b_base, reg_address, b_mask, b_offset, set, clr, func, verse)	\
+{										\
+	.base = b_base,								\
+	.reg_addr = reg_address,						\
+	.mask = b_mask,								\
+	.offset = b_offset,							\
+	.val_set = set,								\
+	.val_clr = clr,								\
+	.inited = SPRD_ADC_INIT_MAGIC,						\
+	.get_val = func,							\
+	.reverse = verse							\
+}
+
+enum SPRD_PMIC_TYPE {
+	UMP9620_ADC,
+	UMP518_ADC,
+};
+
+enum SPRD_ADC_GRAPH_TYPE {
+	TWO_CELL_GRAPH,
+	SPRD_ADC_GRAPH_TYPE_MAX
+};
+
+enum SPRD_ADC_GRAPH_OFFSET {
+	GRAPH_BIG,
+	GRAPH_SMALL,
+	GRAPH_VBAT_DET,
+	GRAPH_MAX
+};
+
+enum SPRD_ADC_SCALE {
+	SCALE_00,
+	SCALE_01,
+	SCALE_10,
+	SCALE_11,
+	SPRD_ADC_SCALE_MAX
+};
+
+enum SPRD_ADC_REG_TYPE {
+	REG_MODULE_EN,
+	REG_CLK_EN,
+	REG_SOFT_RST,
+	REG_XTL_CTRL,
+	REG_SCALE,
+	REG_CFG_FIXED_ST = 11,/* FIXED CFG START */
+	REG_CAL_EN,
+	REG_NTC_MUX,
+	REG_CFG_FIXED_END,/* FIXED CFG END */
+	REG_ISEN_ST = 21,/* CURRENT MODE START */
+	REG_ISEN0,
+	REG_ISEN1,
+	REG_ISEN2,
+	REG_ISEN3,
+	REG_ISEN_END,/* CURRENT MODE END */
+	SPRD_ADC_REG_TYPE_MAX
+};
+
+enum SPRD_ADC_REG_BASE {
+	BASE_GLB,
+	BASE_ANA
+};
+
+struct sprd_adc_pm_data {
+	struct regmap *clk_regmap;
+	u32 clk_reg;/* adc clk26 vote reg */
+	u32 clk_reg_mask;/* adc clk26 vote reg mask */
+	bool dev_suspended;
+};
+
+/*bit[0-7]: scale
+ *bit[8-15]: filter_info(bit8: sw filter support, bit[9-15]: hw filter val(2<<n))
+ *bit[16-23]: isen_info (bit16: isen support, bit[17-23]: isen val)
+ */
+struct sprd_adc_channel_data {
+	int scale;
+	int graph;
+	int isen_info;
+	int filter_info;
+	int ip_ratio;
+	int ch_aux_ratio[SPRD_ADC_SCALE_MAX];
+	int inited;
+};
+
+struct calib_info {
+	int channel;
+	int scale;
+	int graph;
+	int ch_aux_ratio[SPRD_ADC_SCALE_MAX];
+	int inited;
+};
+
+struct reg_bit {
+	u32 base;
+	u32 reg_addr;
+	u32 mask;
+	u32 offset;
+	u32 val_set;
+	u32 val_clr;
+	u32 inited;
+	bool reverse;
+	u32 (*get_val)(void *pri, int ch, bool set);
+};
+
+struct sprd_adc_data {
+	struct iio_dev *indio_dev;
+	struct device *dev;
+	struct regmap *regmap;
+	/* lock to protect against multiple access to the device */
+	struct mutex mutex;
+	/*
+	 * One hardware spinlock to synchronize between the multiple
+	 * subsystems which will access the unique ADC controller.
+	 */
+	struct hwspinlock *hwlock;
+	u32 base;
+	int irq;
+	struct sprd_adc_channel_data ch_data[SPRD_ADC_CHANNEL_MAX];
+	const struct sprd_adc_variant_data *var_data;
+	struct sprd_adc_pm_data pm_data;
+};
+
+struct sprd_adc_variant_data {
+	const int pmic_type;
+	const struct reg_bit *const reg_list;
+	const u32 glb_reg_base;
+	const u32 adc_reg_base_offset;
+	const u32 graph_index;
+	const struct calib_info calib_infos[GRAPH_MAX];
+	const int aux_ratio_comm[SPRD_ADC_SCALE_MAX];
+	void (*const ch_data_init)(struct sprd_adc_data *data);
+};
+
+struct sprd_adc_linear_graph {
+	const char *cell_names[SPRD_ADC_CELL_MAX+1];/* must end with NULL point */
+	int cell_value[SPRD_ADC_CELL_MAX+1];
+	void (*const calibrate)(struct sprd_adc_linear_graph *graph);
+	const int volt0;
+	int adc0;
+	const int volt1;
+	int adc1;
+	const int inited;
+};
+
+struct sprd_adc_graphs {
+	struct sprd_adc_linear_graph adc_graphs[GRAPH_MAX];
+};
+
+static void sprd_adc_calib_with_two_cell(struct sprd_adc_linear_graph *graph);
+static u32 get_isen(void *pri, int ch, bool enable);
+static int sprd_adc_soft_rst(struct sprd_adc_data *data);
+static int sprd_adc_hw_enable(struct sprd_adc_data *data);
+static inline u32 GET_REG_ADDR(struct sprd_adc_data *data, int index)
+{
+	u32 base = ((data->var_data->reg_list[index].base == BASE_GLB)
+		    ? (data->var_data->glb_reg_base)
+		    : (data->base - data->var_data->adc_reg_base_offset));
+	return (base + data->var_data->reg_list[index].reg_addr);
+}
+
+static inline void ratio_to_u32_fract(u32 ratio, struct u32_fract *fract)
+{
+	fract->numerator = ratio >> SPRD_RATIO_NUMERATOR_OFFSET;
+	fract->denominator = ratio & SPRD_RATIO_DENOMINATOR_MASK;
+}
+
+static struct sprd_adc_graphs sprd_adc_graphs_array[] = {
+	[TWO_CELL_GRAPH] = {
+		.adc_graphs = {
+			[GRAPH_BIG] = {
+				.cell_names = {"big_scale_calib1", "big_scale_calib2", NULL},
+				.calibrate = sprd_adc_calib_with_two_cell,
+				GRAPH_POINT_INIT(4200, 3310, 3600, 2832),
+			},
+			[GRAPH_SMALL] = {
+				.cell_names = {"small_scale_calib1", "small_scale_calib2", NULL},
+				.calibrate = sprd_adc_calib_with_two_cell,
+				GRAPH_POINT_INIT(1000, 3413, 100, 341),
+			},
+			[GRAPH_VBAT_DET] = {
+				.cell_names = {"vbat_det_cal1", "vbat_det_cal2", NULL},
+				.calibrate = sprd_adc_calib_with_two_cell,
+				GRAPH_POINT_INIT(1400, 3482, 200, 476),
+			},
+		},
+	},
+};
+
+static const struct reg_bit regs_ump9620[] = {
+	[REG_MODULE_EN] = REG_BIT_INIT(BASE_GLB, 0x08, BIT(5), 5, 1, 0, NULL, false),
+	[REG_CLK_EN] = REG_BIT_INIT(BASE_GLB, 0x0c, GENMASK(6, 5), 5, 0x3, 0, NULL, false),
+	[REG_SOFT_RST] = REG_BIT_INIT(BASE_GLB, 0x14, BIT(6), 6, 1, 0, NULL, false),
+	[REG_XTL_CTRL] = REG_BIT_INIT(BASE_GLB, 0x378, BIT(8), 8, 1, 0, NULL, false),
+	[REG_SCALE] = REG_BIT_INIT(BASE_ANA, 0xffff, GENMASK(10, 9), 9, -1, -1, NULL, false),
+	[REG_CAL_EN] = REG_BIT_INIT(BASE_ANA, 0x4, BIT(12), 12, 1, 0, NULL, false),
+	[REG_ISEN0] = REG_BIT_INIT(BASE_GLB, 0x384, BIT(0), 0, 1, 0, NULL, true),
+	[REG_ISEN1] = REG_BIT_INIT(BASE_GLB, 0x384, BIT(13), 13, 1, 0, NULL, false),
+	[REG_ISEN2] = REG_BIT_INIT(BASE_GLB, 0x384, GENMASK(11, 9), 9, -1, -1, get_isen, false),
+	[REG_ISEN3] = REG_BIT_INIT(BASE_ANA, 0xB0, BIT(0), 0, 1, 0, NULL, false),
+};
+
+static const struct iio_chan_spec sprd_channels[] = {
+	SPRD_ADC_CHANNEL(0, BIT(IIO_CHAN_INFO_PROCESSED)),
+	SPRD_ADC_CHANNEL(1, BIT(IIO_CHAN_INFO_PROCESSED)),
+	SPRD_ADC_CHANNEL(2, BIT(IIO_CHAN_INFO_PROCESSED)),
+	SPRD_ADC_CHANNEL(3, BIT(IIO_CHAN_INFO_PROCESSED)),
+	SPRD_ADC_CHANNEL(4, BIT(IIO_CHAN_INFO_PROCESSED)),
+	SPRD_ADC_CHANNEL(5, BIT(IIO_CHAN_INFO_PROCESSED)),
+	SPRD_ADC_CHANNEL(6, BIT(IIO_CHAN_INFO_PROCESSED)),
+	SPRD_ADC_CHANNEL(7, BIT(IIO_CHAN_INFO_PROCESSED)),
+	SPRD_ADC_CHANNEL(8, BIT(IIO_CHAN_INFO_PROCESSED)),
+	SPRD_ADC_CHANNEL(9, BIT(IIO_CHAN_INFO_PROCESSED)),
+	SPRD_ADC_CHANNEL(10, BIT(IIO_CHAN_INFO_PROCESSED)),
+	SPRD_ADC_CHANNEL(11, BIT(IIO_CHAN_INFO_PROCESSED)),
+	SPRD_ADC_CHANNEL(12, BIT(IIO_CHAN_INFO_PROCESSED)),
+	SPRD_ADC_CHANNEL(13, BIT(IIO_CHAN_INFO_PROCESSED)),
+	SPRD_ADC_CHANNEL(14, BIT(IIO_CHAN_INFO_PROCESSED)),
+	SPRD_ADC_CHANNEL(15, BIT(IIO_CHAN_INFO_PROCESSED)),
+	SPRD_ADC_CHANNEL(16, BIT(IIO_CHAN_INFO_PROCESSED)),
+	SPRD_ADC_CHANNEL(17, BIT(IIO_CHAN_INFO_PROCESSED)),
+	SPRD_ADC_CHANNEL(18, BIT(IIO_CHAN_INFO_PROCESSED)),
+	SPRD_ADC_CHANNEL(19, BIT(IIO_CHAN_INFO_PROCESSED)),
+	SPRD_ADC_CHANNEL(20, BIT(IIO_CHAN_INFO_RAW)),
+	SPRD_ADC_CHANNEL(21, BIT(IIO_CHAN_INFO_PROCESSED)),
+	SPRD_ADC_CHANNEL(22, BIT(IIO_CHAN_INFO_PROCESSED)),
+	SPRD_ADC_CHANNEL(23, BIT(IIO_CHAN_INFO_PROCESSED)),
+	SPRD_ADC_CHANNEL(24, BIT(IIO_CHAN_INFO_PROCESSED)),
+	SPRD_ADC_CHANNEL(25, BIT(IIO_CHAN_INFO_PROCESSED)),
+	SPRD_ADC_CHANNEL(26, BIT(IIO_CHAN_INFO_PROCESSED)),
+	SPRD_ADC_CHANNEL(27, BIT(IIO_CHAN_INFO_PROCESSED)),
+	SPRD_ADC_CHANNEL(28, BIT(IIO_CHAN_INFO_PROCESSED)),
+	SPRD_ADC_CHANNEL(29, BIT(IIO_CHAN_INFO_PROCESSED)),
+	SPRD_ADC_CHANNEL(30, BIT(IIO_CHAN_INFO_PROCESSED)),
+	SPRD_ADC_CHANNEL(31, BIT(IIO_CHAN_INFO_PROCESSED)),
+};
+
+static void sprd_adc_calib_with_two_cell(struct sprd_adc_linear_graph *graph)
+{
+	int adc_calib_data0 = graph->cell_value[0];
+	int adc_calib_data1 = graph->cell_value[1];
+	int adc0_calib, adc1_calib;
+
+	pr_debug("calib before: adc0: %d:, adc1:%d, calib_data0: %d, calib_data1: %d\n",
+		 graph->adc0, graph->adc1, adc_calib_data0, adc_calib_data1);
+
+	adc0_calib = (adc_calib_data0 & 0xfff0) >> 4;
+	adc1_calib = (adc_calib_data1 & 0xfff0) >> 4;
+
+	if (adc0_calib > 0 && adc1_calib > 0) {
+		graph->adc0 = adc0_calib;
+		graph->adc1 = adc1_calib;
+	}
+
+	pr_debug("calib aft: adc0: %d:, adc1:%d, calib_data0: %d, calib_data1: %d\n",
+		 graph->adc0, graph->adc1, adc_calib_data0, adc_calib_data1);
+}
+
+static const struct calib_info *sprd_adc_calib_info_get(struct sprd_adc_data *data, int ch)
+{
+	int calib_index, calib_small = 1;
+	const struct calib_info *calib_info;
+
+	for (calib_index = 0; calib_index < GRAPH_MAX; calib_index++) {
+		calib_info = &data->var_data->calib_infos[calib_index];
+		if (calib_info->inited != SPRD_ADC_INIT_MAGIC)
+			continue;
+
+		if (calib_info->graph == GRAPH_SMALL)
+			calib_small = calib_index;
+
+		if (calib_info->channel == ch)
+			return calib_info;
+	}
+
+	return &data->var_data->calib_infos[calib_small];
+}
+
+static void sprd_adc_aux_ratio_init(struct sprd_adc_data *data, int ch)
+{
+	u64 final_ratio;
+	int scale, *aux_ratio_ch, aux_ratio_tmp[SPRD_ADC_SCALE_MAX];
+	const struct calib_info *calib_match = sprd_adc_calib_info_get(data, ch);
+	const int *aux_ratio_calib;
+	struct u32_fract c_fract, aux_fract;
+
+	data->ch_data[ch].graph = calib_match->graph;
+	aux_ratio_ch = data->ch_data[ch].ch_aux_ratio;
+
+	aux_ratio_calib = &data->var_data->aux_ratio_comm[0];
+	memcpy(&aux_ratio_tmp[0], aux_ratio_calib, sizeof(int) * SPRD_ADC_SCALE_MAX);
+
+	ratio_to_u32_fract(aux_ratio_calib[calib_match->scale], &c_fract);
+	for (scale = 0; scale < SPRD_ADC_SCALE_MAX; scale++) {
+
+		if (ch == calib_match->channel && scale == calib_match->scale) {
+			aux_ratio_ch[scale] = RATIO_DEF;
+			continue;
+		}
+
+		if (aux_ratio_tmp[scale] == R_INVALID || aux_ratio_calib[scale] == R_INVALID) {
+			aux_ratio_ch[scale] = RATIO_DEF;
+			continue;
+		}
+
+		ratio_to_u32_fract(aux_ratio_tmp[scale], &aux_fract);
+		if (scale > calib_match->scale) {
+			final_ratio =
+				div_u64((u64)aux_fract.denominator * c_fract.numerator * 1000,
+					c_fract.denominator * aux_fract.numerator);
+			aux_ratio_ch[scale] = RATIO(1000, (int)final_ratio);
+		} else {
+			final_ratio =
+				div_u64((u64)c_fract.denominator * aux_fract.numerator * 1000,
+					aux_fract.denominator * c_fract.numerator);
+			aux_ratio_ch[scale] = RATIO((int)final_ratio, 1000);
+		}
+	}
+}
+
+static inline void sprd_adc_ch_data_show(struct sprd_adc_data *data, int ch)
+{
+	struct sprd_adc_channel_data *ch_data = &data->ch_data[ch];
+	struct u32_fract ip_fract, aux_fract0, aux_fract1, aux_fract2, aux_fract3;
+
+	ratio_to_u32_fract(ch_data->ip_ratio, &ip_fract);
+	ratio_to_u32_fract(ch_data->ch_aux_ratio[0], &aux_fract0);
+	ratio_to_u32_fract(ch_data->ch_aux_ratio[1], &aux_fract1);
+	ratio_to_u32_fract(ch_data->ch_aux_ratio[2], &aux_fract2);
+	ratio_to_u32_fract(ch_data->ch_aux_ratio[3], &aux_fract3);
+	dev_dbg(data->dev, CH_DATA_FMT,
+		ch, ch_data->scale, ch_data->graph, ch_data->isen_info, ch_data->filter_info,
+		ip_fract.numerator, ip_fract.denominator,
+		aux_fract0.numerator, aux_fract0.denominator,
+		aux_fract1.numerator, aux_fract1.denominator,
+		aux_fract2.numerator, aux_fract2.denominator,
+		aux_fract3.numerator, aux_fract3.denominator);
+}
+
+static void sprd_adc_ch_data_merge(struct sprd_adc_data *data, struct sprd_adc_channel_data *d_diff,
+				   struct sprd_adc_channel_data *d_def)
+{
+	struct sprd_adc_channel_data *ch_data;
+	int ch;
+
+	for (ch = 0; ch < SPRD_ADC_CHANNEL_MAX; ch++) {
+		ch_data = &data->ch_data[ch];
+		*ch_data = ((d_diff[ch].inited == SPRD_ADC_INIT_MAGIC) ? d_diff[ch] : *d_def);
+	}
+
+	for (ch = 0; ch < SPRD_ADC_CHANNEL_MAX; ch++) {
+		sprd_adc_aux_ratio_init(data, ch);
+		sprd_adc_ch_data_show(data, ch);
+	}
+}
+
+static int sprd_adc_graphs_calibrate(struct sprd_adc_data *data)
+{
+	int i, j, ret;
+	struct sprd_adc_graphs *graphs;
+	struct sprd_adc_linear_graph *linear_graph;
+	u16 cell_value = 0;
+
+	if (data->var_data->graph_index >= SPRD_ADC_GRAPH_TYPE_MAX)
+		return -EINVAL;
+
+	graphs = &sprd_adc_graphs_array[data->var_data->graph_index];
+	for (i = 0; i < GRAPH_MAX; i++) {
+
+		if (graphs->adc_graphs[i].inited != SPRD_ADC_INIT_MAGIC)
+			continue;
+
+		linear_graph = &graphs->adc_graphs[i];
+		for (j = 0; linear_graph->cell_names[j] != NULL; j++) {
+			ret = nvmem_cell_read_u16(data->dev, linear_graph->cell_names[j],
+						  &cell_value);
+			if (ret) {
+				dev_err(data->dev, "calibrate err! name: %s: ret %d\n",
+					linear_graph->cell_names[j], ret);
+				return ret;
+			}
+			linear_graph->cell_value[j] = cell_value;
+		}
+	}
+
+	for (i = 0; i < GRAPH_MAX; i++) {
+
+		if (graphs->adc_graphs[i].inited != SPRD_ADC_INIT_MAGIC)
+			continue;
+
+		linear_graph = &graphs->adc_graphs[i];
+		linear_graph->calibrate(linear_graph);
+	}
+
+	return 0;
+}
+
+static void ump9620_ch_data_init(struct sprd_adc_data *data)
+{
+	struct sprd_adc_channel_data ch_data_def = CH_DATA_INIT(SCALE_00, 0, 0, RATIO_DEF);
+	struct sprd_adc_channel_data ch_data[SPRD_ADC_CHANNEL_MAX] = {
+		[0] = CH_DATA_INIT(SCALE_01, 0, 0, RATIO_DEF),
+		[5] = CH_DATA_INIT(SCALE_00, 0x1, 0x9, RATIO_DEF),
+		[6] = CH_DATA_INIT(SCALE_00, 0x1, 0x9, RATIO_DEF),
+		[7] = CH_DATA_INIT(SCALE_10, 0, 0, RATIO_DEF),
+		[9] = CH_DATA_INIT(SCALE_10, 0, 0, RATIO_DEF),
+		[10] = CH_DATA_INIT(SCALE_11, 0, 0, RATIO_DEF),
+		[11] = CH_DATA_INIT(SCALE_00, 0, 0, RATIO_DEF),
+		[13] = CH_DATA_INIT(SCALE_01, 0, 0, RATIO_DEF),
+		[14] = CH_DATA_INIT(SCALE_00, 0, 0, RATIO(68, 900)),
+		[15] = CH_DATA_INIT(SCALE_00, 0, 0, RATIO(1, 3)),
+		[19] = CH_DATA_INIT(SCALE_11, 0, 0, RATIO_DEF),
+		[21] = CH_DATA_INIT(SCALE_00, 0, 0, RATIO(3, 8)),
+		[22] = CH_DATA_INIT(SCALE_00, 0, 0, RATIO(3, 8)),
+		[23] = CH_DATA_INIT(SCALE_00, 0, 0, RATIO(3, 8)),
+		[30] = CH_DATA_INIT(SCALE_11, 0, 0, RATIO_DEF),
+		[31] = CH_DATA_INIT(SCALE_11, 0, 0, RATIO_DEF),
+	};
+
+	sprd_adc_ch_data_merge(data, ch_data, &ch_data_def);
+}
+
+static void sprd_adc_regs_dump(struct sprd_adc_data *data, int ch, int scale, const char *tag)
+{
+	u32 mod_en = 0, clk_en = 0, int_ctl = 0, int_raw = 0, adc_ctl = 0;
+	u32 ch_cfg = 0, pm_clk_reg = 0, xtl_ctl = 0;
+	int ret;
+
+	ret = regmap_read(data->regmap, GET_REG_ADDR(data, REG_MODULE_EN), &mod_en);
+	ret |= regmap_read(data->regmap, GET_REG_ADDR(data, REG_CLK_EN), &clk_en);
+	ret |= regmap_read(data->regmap, GET_REG_ADDR(data, REG_XTL_CTRL), &xtl_ctl);
+	ret |= regmap_read(data->regmap, data->base + SPRD_ADC_INT_CLR, &int_ctl);
+	ret |= regmap_read(data->regmap, data->base + SPRD_ADC_INT_RAW, &int_raw);
+	ret |= regmap_read(data->regmap, data->base + SPRD_ADC_CTL, &adc_ctl);
+	ret |= regmap_read(data->regmap, data->base + SPRD_ADC_CH_CFG, &ch_cfg);
+	if (data->pm_data.clk_regmap)
+		ret |= regmap_read(data->pm_data.clk_regmap,  data->pm_data.clk_reg, &pm_clk_reg);
+
+	if (ret) {
+		dev_err(data->dev, "regs_dump err: ret %d\n", ret);
+		return;
+	}
+
+	dev_err(data->dev, "[%s]: ch %d, sl %d, clk_en 0x%x, xtl_ctl 0x%x, pm_clk 0x%x\n",
+		tag, ch, scale, clk_en, xtl_ctl, pm_clk_reg);
+	dev_err(data->dev, "mod_en 0x%x, int_ctl 0x%x, int_raw 0x%x, adc_ctl 0x%x, ch_cfg 0x%x\n",
+		mod_en, int_ctl, int_raw, adc_ctl, ch_cfg);
+}
+
+static u32 get_isen(void *pri, int ch, bool enable)
+{
+	struct sprd_adc_data *data = (struct sprd_adc_data *)pri;
+
+	if (!enable)
+		return 0;
+
+	return (data->ch_data[ch].isen_info >> 1);
+}
+
+static int sprd_adc_reg_cfg(struct sprd_adc_data *data, int ch, int index, bool set)
+{
+	int ret;
+	u32 reg_addr, val, read_val = 0;
+	const struct reg_bit *const reg_inf = &data->var_data->reg_list[index];
+	bool set_act = ((set && !reg_inf->reverse) || (!set && reg_inf->reverse));
+
+	if (data->var_data->reg_list[index].inited != SPRD_ADC_INIT_MAGIC)
+		return 0;
+
+	reg_addr = GET_REG_ADDR(data, index);
+	if (!reg_inf->get_val)
+		val = (set_act ? reg_inf->val_set : reg_inf->val_clr);
+	else
+		val = reg_inf->get_val(data, ch, set_act);
+	val = ((val << reg_inf->offset) & reg_inf->mask);
+	ret = regmap_update_bits(data->regmap, reg_addr, reg_inf->mask, val);
+	ret |= regmap_read(data->regmap, reg_addr, &read_val);
+	if (ret) {
+		dev_err(data->dev, "reg_cfg err: reg[%d], ret %d\n", reg_addr, ret);
+		return ret;
+	}
+
+	dev_dbg(data->dev, "reg_cfg[%d %d]: reg 0x%x, mask: 0x%x, val: 0x%x, read_val: 0x%x\n",
+		set, reg_inf->reverse, reg_addr, reg_inf->mask, val, read_val);
+
+	return 0;
+}
+
+static void sprd_adc_regs_set_order(struct sprd_adc_data *data, int ch, int start, int end)
+{
+	int i;
+
+	for (i = start + 1; i < end; i++)
+		sprd_adc_reg_cfg(data, ch, i, true);
+}
+
+static void sprd_adc_regs_clr_order_reverse(struct sprd_adc_data *data, int ch, int start, int end)
+{
+	int i;
+
+	for (i = end - 1; i > start; i--)
+		sprd_adc_reg_cfg(data, ch, i, false);
+}
+
+static int sprd_adc_isen_enable(struct sprd_adc_data *data, int channel)
+{
+	bool isen_support = data->ch_data[channel].isen_info & 0x1;
+
+	if (!isen_support)
+		return 0;
+
+	sprd_adc_regs_set_order(data, channel, REG_ISEN_ST, REG_ISEN_END);
+	usleep_range(500, 1000);
+
+	return 0;
+}
+
+static int sprd_adc_isen_disable(struct sprd_adc_data *data, int channel)
+{
+	bool isen_support = data->ch_data[channel].isen_info & 0x1;
+
+	if (!isen_support)
+		return 0;
+
+	sprd_adc_regs_clr_order_reverse(data, channel, REG_ISEN_ST, REG_ISEN_END);
+	usleep_range(500, 1000);
+
+	return 0;
+}
+
+static void sprd_adc_config_fixed(struct sprd_adc_data *data)
+{
+	sprd_adc_regs_set_order(data, -1, REG_CFG_FIXED_ST, REG_CFG_FIXED_END);
+}
+
+static int sprd_adc_data_average(int *vals, int len)
+{
+	int i, sum = 0;
+
+	for (i = 0; i < len; i++)
+		sum += vals[i];
+	return DIV_ROUND_CLOSEST(sum, len);
+}
+
+static int compare_val(const void *a, const void *b)
+{
+	return *(int *)a - *(int *)b;
+}
+
+static int sprd_adc_get_val_with_sw_filter(struct sprd_adc_data *data, int ch)
+{
+	int data_buf[ADC_MESURE_NUMBER_SW] = { 0 }, ret = 0, num = ADC_MESURE_NUMBER_SW;
+	int count, result;
+	unsigned int  rawdata;
+
+	for (count = 0; count < ADC_MESURE_NUMBER_SW; count++) {
+		ret |= regmap_read(data->regmap, data->base + SPRD_ADC_DATA, &rawdata);
+		rawdata &= SPRD_ADC_DATA_MASK;
+		data_buf[count] = rawdata;
+		udelay(10);
+		if (ret)
+			return -EINVAL;
+	}
+
+	sort(data_buf, ADC_MESURE_NUMBER_SW, sizeof(int), compare_val, NULL);
+	result = sprd_adc_data_average(&data_buf[num / 5], (num - num * 2 / 5));
+
+	for (count = 0; count < ADC_MESURE_NUMBER_SW; count++)
+		dev_dbg(data->dev, "data_buf[%d]=%d ", count, data_buf[count]);
+	dev_dbg(data->dev, "result=%d\n", result);
+
+	return result;
+}
+
+static int sprd_adc_enable(struct sprd_adc_data *data, int channel)
+{
+	int ret = 0;
+	u32 reg_read = 0;
+
+	if (data->pm_data.clk_regmap) {
+		ret = regmap_update_bits(data->pm_data.clk_regmap, data->pm_data.clk_reg,
+					 data->pm_data.clk_reg_mask,
+					 data->pm_data.clk_reg_mask);
+		ret |= regmap_read(data->pm_data.clk_regmap, data->pm_data.clk_reg, &reg_read);
+		if (ret) {
+			dev_err(data->dev, "failed to enable clk26m, channel %d\n", channel);
+			return ret;
+		}
+		dev_dbg(data->dev, "enable clk26m: ch %d, reg_read 0x%x\n", channel, reg_read);
+	}
+
+	ret = sprd_adc_isen_enable(data, channel);
+	if (ret) {
+		dev_err(data->dev, "failed to enable isen\n");
+		return ret;
+	}
+
+	ret = regmap_update_bits(data->regmap, data->base + SPRD_ADC_CTL,
+				 SPRD_ADC_EN, SPRD_ADC_EN);
+	if (ret) {
+		dev_err(data->dev, "failed to set SPRD_ADC_EN\n");
+		return ret;
+	}
+
+	return ret;
+}
+
+static int sprd_adc_disable(struct sprd_adc_data *data, int channel)
+{
+	int ret = 0;
+	u32 reg_read = 0;
+
+	ret = regmap_update_bits(data->regmap, data->base + SPRD_ADC_CTL, SPRD_ADC_EN, 0);
+	if (ret) {
+		dev_err(data->dev, "failed to reset SPRD_ADC_EN\n");
+		return ret;
+	}
+
+	ret = sprd_adc_isen_disable(data, channel);
+	if (ret) {
+		dev_err(data->dev, "failed to disable isen\n");
+		return ret;
+	}
+
+	if (data->pm_data.clk_regmap) {
+		ret = regmap_update_bits(data->pm_data.clk_regmap, data->pm_data.clk_reg,
+					 data->pm_data.clk_reg_mask, 0);
+		ret |= regmap_read(data->pm_data.clk_regmap, data->pm_data.clk_reg, &reg_read);
+		if (ret) {
+			dev_err(data->dev, "failed to disable clk26m, channel %d\n", channel);
+			return ret;
+		}
+		dev_dbg(data->dev, "disable clk26m: ch %d, reg_read 0x%x\n", channel, reg_read);
+	}
+
+	return ret;
+}
+
+static int sprd_adc_read(struct sprd_adc_data *data, int channel, int scale, int *val)
+{
+	int ret = 0, sample_num_sw;
+	u32 rawdata = 0, tmp, status, scale_shift, scale_mask;
+	bool filter_sw = data->ch_data[channel].filter_info & 0x1;
+	int sample_num_hw = data->ch_data[channel].filter_info >> 1;
+
+	if (data->pm_data.dev_suspended) {
+		dev_err(data->dev, "adc_exp: adc bas been suspended, ignore.\n");
+		return -EBUSY;
+	}
+
+	dev_dbg(data->dev, "ch_data[%d]: scale %d, graph %d, filter_info 0x%x, isen_info 0x%x\n",
+		channel, data->ch_data[channel].scale, data->ch_data[channel].graph,
+		data->ch_data[channel].filter_info, data->ch_data[channel].isen_info);
+
+	ret = hwspin_lock_timeout_raw(data->hwlock, SPRD_ADC_HWLOCK_TIMEOUT);
+	if (ret) {
+		dev_err(data->dev, "timeout to get the hwspinlock\n");
+		return ret;
+	}
+
+	ret = sprd_adc_enable(data, channel);
+	if (ret)
+		goto disable_adc;
+
+	ret = regmap_update_bits(data->regmap, data->base + SPRD_ADC_INT_CLR,
+				 SPRD_ADC_IRQ_CLR, SPRD_ADC_IRQ_CLR);
+	if (ret)
+		goto disable_adc;
+
+	/* Configure the channel id and scale */
+	scale_shift = data->var_data->reg_list[REG_SCALE].offset;
+	scale_mask = data->var_data->reg_list[REG_SCALE].mask;
+	tmp = (scale << scale_shift) & scale_mask;
+	tmp |= channel & SPRD_ADC_CHN_ID_MASK;
+	ret = regmap_update_bits(data->regmap, data->base + SPRD_ADC_CH_CFG,
+				 SPRD_ADC_CHN_ID_MASK | scale_mask, tmp);
+	if (ret)
+		goto disable_adc;
+
+	/* Select 12bit conversion mode, and only sample 1 time */
+	tmp = SPRD_ADC_12BIT_MODE;
+	sample_num_sw = (filter_sw ? ADC_MESURE_NUMBER_SW - 1 : 0);
+	sample_num_hw = ((sample_num_hw > 0) ? sample_num_hw : ADC_MESURE_NUMBER_HW_DEF);
+	tmp |= ((unsigned long)sample_num_sw << SPRD_ADC_RUN_NUM_SHIFT) & SPRD_ADC_RUN_NUM_MASK;
+	tmp |= ((unsigned long)sample_num_hw << SPRD_ADC_AVERAGE_SHIFT) & SPRD_ADC_AVERAGE_MASK;
+	ret = regmap_update_bits(data->regmap, data->base + SPRD_ADC_CTL,
+				 SPRD_ADC_RUN_NUM_MASK | SPRD_ADC_12BIT_MODE |
+				 SPRD_ADC_AVERAGE_MASK, tmp);
+	if (ret)
+		goto disable_adc;
+
+	ret = regmap_update_bits(data->regmap, data->base + SPRD_ADC_CTL,
+				 SPRD_ADC_CHN_RUN, SPRD_ADC_CHN_RUN);
+	if (ret)
+		goto disable_adc;
+
+	ret = regmap_read_poll_timeout(data->regmap, data->base + SPRD_ADC_INT_RAW, status,
+				       (status & SPRD_ADC_IRQ_RAW), SPRD_ADC_POLL_RAW_STATUS,
+				       SPRD_ADC_RDY_TIMEOUT);
+	if (ret) {
+		dev_err(data->dev, "read adc timeout 0x%x\n", status);
+		sprd_adc_regs_dump(data, channel, scale, "t_bef");
+		sprd_adc_hw_enable(data);
+		sprd_adc_soft_rst(data);
+		sprd_adc_regs_dump(data, channel, scale, "t_aft");
+		goto disable_adc;
+	}
+
+	if (filter_sw) {
+		rawdata = sprd_adc_get_val_with_sw_filter(data, channel);
+	} else {
+		ret = regmap_read(data->regmap, data->base + SPRD_ADC_DATA, &rawdata);
+		rawdata &= SPRD_ADC_DATA_MASK;
+	}
+disable_adc:
+	ret = sprd_adc_disable(data, channel);
+
+	hwspin_unlock_raw(data->hwlock);
+
+	if (!ret)
+		*val = rawdata;
+
+	return ret;
+}
+
+static int sprd_adc_calculate_volt_by_graph(struct sprd_adc_data *data, int channel,
+					    int scale, int raw_adc)
+{
+	int tmp;
+	struct sprd_adc_graphs *graphs = &sprd_adc_graphs_array[data->var_data->graph_index];
+	int graph_offset = data->ch_data[channel].graph;
+	struct sprd_adc_linear_graph *linear_graph = &graphs->adc_graphs[graph_offset];
+
+	tmp = (linear_graph->volt0 - linear_graph->volt1) * (raw_adc - linear_graph->adc1);
+	tmp /= (linear_graph->adc0 - linear_graph->adc1);
+	tmp += linear_graph->volt1;
+	tmp = (tmp < 0 ? 0 : tmp);
+
+	dev_dbg(data->dev, "by_graph_c%d: v0 %d a0 %d, v1 %d a1 %d, raw_adc 0x%x, vol_graph %d\n",
+		channel, linear_graph->volt0, linear_graph->adc0, linear_graph->volt1,
+		linear_graph->adc1, raw_adc, tmp);
+
+	return tmp;
+}
+
+static int sprd_adc_calculate_volt_by_ratio(struct sprd_adc_data *data, int channel,
+					    int scale, int vol_graph)
+{
+	struct u32_fract ip_fract, aux_fract;
+	int pmic_type = data->var_data->pmic_type;
+	u32 vol_final = vol_graph;
+
+	ratio_to_u32_fract(data->ch_data[channel].ip_ratio, &ip_fract);
+	vol_final = DIV_ROUND_CLOSEST(vol_final * ip_fract.denominator, ip_fract.numerator);
+
+	ratio_to_u32_fract(data->ch_data[channel].ch_aux_ratio[scale], &aux_fract);
+	vol_final = DIV_ROUND_CLOSEST(vol_final * aux_fract.denominator, aux_fract.numerator);
+
+	dev_dbg(data->dev, "by_ratio_c%d: id %d, scale %d, ip_r[%d/%d], aux_r[%d/%d], vol_f %d\n",
+		channel, pmic_type, scale, ip_fract.numerator, ip_fract.denominator,
+		aux_fract.numerator, aux_fract.denominator, vol_final);
+
+	return vol_final;
+}
+
+static int sprd_adc_read_processed(struct sprd_adc_data *data, int channel, int scale, int *val)
+{
+	int ret, raw_adc, vol_graph;
+
+	ret = sprd_adc_read(data, channel, scale, &raw_adc);
+
+	if (ret)
+		return ret;
+
+	vol_graph = sprd_adc_calculate_volt_by_graph(data, channel, scale, raw_adc);
+	*val = sprd_adc_calculate_volt_by_ratio(data, channel, scale, vol_graph);
+
+	return 0;
+}
+
+static int sprd_adc_ch_data_encode(struct sprd_adc_data *data, int ch)
+{
+	int scale = data->ch_data[ch].scale & 0xff;
+	int isen_info = data->ch_data[ch].isen_info & 0xff;
+	int filter_info = data->ch_data[ch].filter_info & 0xff;
+
+	return (scale | (filter_info << 8) | (isen_info << 16));
+}
+
+static void sprd_adc_ch_data_decode(struct sprd_adc_data *data, int ch, int val)
+{
+	int scale_override, filter_override, isen_override;
+
+	scale_override = (val & 0xff);
+	filter_override = ((val >> 8) & 0xff);
+	isen_override = ((val >> 16) & 0xff);
+
+	if (scale_override < SPRD_ADC_SCALE_MAX && scale_override != data->ch_data[ch].scale)
+		data->ch_data[ch].scale = scale_override;
+
+	if (data->ch_data[ch].filter_info != filter_override)
+		data->ch_data[ch].filter_info = filter_override;
+
+	if (data->ch_data[ch].isen_info != isen_override)
+		data->ch_data[ch].isen_info = isen_override;
+}
+
+static int sprd_adc_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
+			     int *val, int *val2, long mask)
+{
+	struct sprd_adc_data *data = iio_priv(indio_dev);
+	int scale = data->ch_data[chan->channel].scale;
+	int ret, tmp;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		mutex_lock(&data->mutex);
+		ret = sprd_adc_read(data, chan->channel, scale, &tmp);
+		mutex_unlock(&data->mutex);
+
+		if (ret)
+			return ret;
+
+		*val = tmp;
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_PROCESSED:
+		mutex_lock(&data->mutex);
+		ret = sprd_adc_read_processed(data, chan->channel, scale, &tmp);
+		mutex_unlock(&data->mutex);
+
+		if (ret)
+			return ret;
+
+		*val = tmp;
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		*val = sprd_adc_ch_data_encode(data, chan->channel);
+		return IIO_VAL_INT;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int sprd_adc_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
+			      int val, int val2, long mask)
+{
+	struct sprd_adc_data *data = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		sprd_adc_ch_data_decode(data, chan->channel, val);
+		return 0;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int sprd_adc_soft_rst(struct sprd_adc_data *data)
+{
+	int ret;
+	u32 reg_addr, mask;
+
+	reg_addr = GET_REG_ADDR(data, REG_SOFT_RST);
+	mask = data->var_data->reg_list[REG_SOFT_RST].mask;
+	ret = regmap_update_bits(data->regmap, reg_addr, mask, mask);
+	if (ret)
+		return ret;
+
+	udelay(10);
+
+	reg_addr = GET_REG_ADDR(data, REG_SOFT_RST);
+	mask = data->var_data->reg_list[REG_SOFT_RST].mask;
+	ret = regmap_update_bits(data->regmap, reg_addr, mask, 0);
+	if (ret)
+		return ret;
+
+	sprd_adc_config_fixed(data);
+
+	return 0;
+}
+static int sprd_adc_hw_enable(struct sprd_adc_data *data)
+{
+	int ret;
+	u32 reg_addr, mask;
+
+	reg_addr = GET_REG_ADDR(data, REG_XTL_CTRL);
+	mask = data->var_data->reg_list[REG_XTL_CTRL].mask;
+	ret = regmap_update_bits(data->regmap, reg_addr, mask, mask);
+	if (ret)
+		return ret;
+
+	reg_addr = GET_REG_ADDR(data, REG_MODULE_EN);
+	mask = data->var_data->reg_list[REG_MODULE_EN].mask;
+	ret = regmap_update_bits(data->regmap, reg_addr, mask, mask);
+	if (ret)
+		return ret;
+
+	/* Enable ADC work clock */
+	reg_addr = GET_REG_ADDR(data, REG_CLK_EN);
+	mask = data->var_data->reg_list[REG_CLK_EN].mask;
+	ret = regmap_update_bits(data->regmap, reg_addr, mask, mask);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static void sprd_adc_hw_disable(void *_data)
+{
+	struct sprd_adc_data *data = _data;
+	u32 reg_addr, mask;
+
+	/* Disable ADC work clock and controller clock */
+	reg_addr = GET_REG_ADDR(data, REG_CLK_EN);
+	mask = data->var_data->reg_list[REG_CLK_EN].mask;
+	regmap_update_bits(data->regmap, reg_addr, mask, 0);
+
+	reg_addr = GET_REG_ADDR(data, REG_MODULE_EN);
+	mask = data->var_data->reg_list[REG_MODULE_EN].mask;
+	regmap_update_bits(data->regmap, reg_addr, mask, 0);
+}
+
+static void sprd_adc_free_lock(void *_data)
+{
+	struct sprd_adc_data *sprd_data = _data;
+
+	hwspin_lock_free(sprd_data->hwlock);
+	mutex_destroy(&sprd_data->mutex);
+}
+
+static int sprd_adc_ch_data_init(struct sprd_adc_data *data)
+{
+	struct device_node *np = data->dev->of_node;
+	int size, ret, ch, ch_data_val, i;
+	u32 *ch_data_override;
+
+	data->var_data->ch_data_init(data);
+
+	size = of_property_count_elems_of_size(np, "ch_data_override", sizeof(u32));
+	if (size <= 0)
+		return 0;
+
+	if (size % 2) {
+		dev_err(data->dev, "Pair of ch data err!\n");
+		return -EINVAL;
+	}
+
+	ch_data_override = kcalloc(size, sizeof(u32), GFP_KERNEL);
+	if (!ch_data_override)
+		return -ENOMEM;
+
+	ret = of_property_read_u32_array(np, "ch_data_override", ch_data_override, size);
+	if (ret < 0) {
+		dev_err(data->dev, "Failed to read ch data from dt: %d\n", ret);
+		kfree(ch_data_override);
+		return ret;
+	}
+
+	for (i = 0; i < size; i += 2) {
+		ch = ch_data_override[i];
+		ch_data_val = ch_data_override[i+1];
+		sprd_adc_ch_data_decode(data, ch, ch_data_val);
+		sprd_adc_ch_data_show(data, ch);
+	}
+
+	kfree(ch_data_override);
+
+	return 0;
+}
+
+static int sprd_adc_pm_init(struct sprd_adc_data *data)
+{
+	unsigned int pm_args[2];
+	u32 reg, mask;
+	struct device_node *np = data->dev->of_node;
+
+	data->pm_data.clk_regmap =
+		syscon_regmap_lookup_by_phandle_args(np, "sprd_adc_pm_reg", 2, pm_args);
+	if (!IS_ERR_OR_NULL(data->pm_data.clk_regmap)) {
+		data->pm_data.clk_reg = reg = pm_args[0];
+		data->pm_data.clk_reg_mask = mask = pm_args[1];
+	} else {
+		data->pm_data.clk_regmap = NULL;
+	}
+
+	data->pm_data.dev_suspended = false;
+
+	return 0;
+}
+
+static const struct iio_info sprd_info = {
+	.read_raw = &sprd_adc_read_raw,
+	.write_raw = &sprd_adc_write_raw,
+};
+
+static int sprd_adc_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct sprd_adc_data *sprd_data;
+	const struct sprd_adc_variant_data *pdata;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	pdata = of_device_get_match_data(&pdev->dev);
+	if (!pdata) {
+		dev_err(&pdev->dev, "No matching driver data found\n");
+		return -EINVAL;
+	}
+
+	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*sprd_data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	sprd_data = iio_priv(indio_dev);
+
+	sprd_data->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!sprd_data->regmap) {
+		dev_err(&pdev->dev, "failed to get ADC regmap\n");
+		return -ENODEV;
+	}
+
+	ret = of_property_read_u32(np, "reg", &sprd_data->base);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to get ADC base address\n");
+		return ret;
+	}
+
+	sprd_data->irq = platform_get_irq(pdev, 0);
+	if (sprd_data->irq < 0) {
+		dev_err(&pdev->dev, "failed to get ADC irq number\n");
+		return sprd_data->irq;
+	}
+
+	ret = of_hwspin_lock_get_id(np, 0);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to get hwspinlock id\n");
+		return ret;
+	}
+
+	mutex_init(&sprd_data->mutex);
+	sprd_data->hwlock = hwspin_lock_request_specific(ret);
+	if (!sprd_data->hwlock) {
+		dev_err(&pdev->dev, "failed to request hwspinlock\n");
+		return -ENXIO;
+	}
+
+	ret = devm_add_action(&pdev->dev, sprd_adc_free_lock, sprd_data);
+	if (ret) {
+		sprd_adc_free_lock(sprd_data);
+		dev_err(&pdev->dev, "failed to add free lock action\n");
+		return ret;
+	}
+
+	sprd_data->dev = &pdev->dev;
+	sprd_data->var_data = pdata;
+	sprd_data->indio_dev = indio_dev;
+
+	/* ADC channel scales calibration from nvmem device */
+	ret = sprd_adc_graphs_calibrate(sprd_data);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to calib graphs from nvmem\n");
+		return ret;
+	}
+
+	ret = sprd_adc_pm_init(sprd_data);
+	if (ret) {
+		dev_err(&pdev->dev, "adc pm init err.\n");
+		return ret;
+	}
+
+	ret = sprd_adc_ch_data_init(sprd_data);
+	if (ret) {
+		dev_err(&pdev->dev, "ch data init err.\n");
+		return ret;
+	}
+
+	ret = sprd_adc_hw_enable(sprd_data);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to enable ADC module\n");
+		return ret;
+	}
+
+	ret = devm_add_action(&pdev->dev, sprd_adc_hw_disable, sprd_data);
+	if (ret) {
+		sprd_adc_hw_disable(sprd_data);
+		dev_err(&pdev->dev, "failed to add ADC disable action\n");
+		return ret;
+	}
+
+	ret = sprd_adc_soft_rst(sprd_data);
+	if (ret) {
+		dev_err(&pdev->dev, "adc soft rst failed\n");
+		return ret;
+	}
+
+	indio_dev->dev.parent = &pdev->dev;
+	indio_dev->name = dev_name(&pdev->dev);
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &sprd_info;
+	indio_dev->channels = sprd_channels;
+	indio_dev->num_channels = ARRAY_SIZE(sprd_channels);
+	ret = devm_iio_device_register(&pdev->dev, indio_dev);
+	if (ret)
+		dev_err(&pdev->dev, "could not register iio (ADC)");
+
+	platform_set_drvdata(pdev, indio_dev);
+
+	return ret;
+}
+
+static int sprd_adc_pm_suspend(struct device *dev)
+{
+	struct sprd_adc_data *sprd_data = iio_priv(dev_get_drvdata(dev));
+
+	mutex_lock(&sprd_data->mutex);
+	sprd_data->pm_data.dev_suspended = true;
+	mutex_unlock(&sprd_data->mutex);
+
+	return 0;
+}
+
+static int sprd_adc_pm_resume(struct device *dev)
+{
+	struct sprd_adc_data *sprd_data = iio_priv(dev_get_drvdata(dev));
+
+	mutex_lock(&sprd_data->mutex);
+	sprd_data->pm_data.dev_suspended = false;
+	mutex_unlock(&sprd_data->mutex);
+
+	return 0;
+}
+
+static const struct sprd_adc_variant_data ump9620_data = {
+	.pmic_type = UMP9620_ADC,
+	.glb_reg_base = 0x2000,
+	.adc_reg_base_offset = 0x4,
+	.reg_list = regs_ump9620,
+	.graph_index = TWO_CELL_GRAPH,
+	.calib_infos = {
+		CALIB_INFO_INIT(11, SCALE_00, GRAPH_BIG),
+		CALIB_INFO_INIT(1, SCALE_00, GRAPH_SMALL),
+		CALIB_INFO_INIT(0, SCALE_01, GRAPH_VBAT_DET)
+	},
+	.aux_ratio_comm = {RATIO_DEF, RATIO(100, 133), RATIO(1000, 2600), RATIO(1000, 4060)},
+	.ch_data_init = ump9620_ch_data_init,
+};
+
+static const struct sprd_adc_variant_data ump518_data = {
+	.pmic_type = UMP518_ADC,
+	.glb_reg_base = 0x1800,
+	.adc_reg_base_offset = 0x4,
+	.reg_list = regs_ump9620,
+	.graph_index = TWO_CELL_GRAPH,
+	.calib_infos = {
+		CALIB_INFO_INIT(11, SCALE_00, GRAPH_BIG),
+		CALIB_INFO_INIT(1, SCALE_00, GRAPH_SMALL),
+		CALIB_INFO_INIT(0, SCALE_01, GRAPH_VBAT_DET)
+	},
+	.aux_ratio_comm = {RATIO_DEF, RATIO(100, 133), RATIO(1000, 2600), RATIO(1000, 4060)},
+	.ch_data_init = ump9620_ch_data_init,
+};
+
+static const struct of_device_id sprd_adc_of_match[] = {
+	{ .compatible = "sprd,ump9620-adc", .data = &ump9620_data},
+	{ .compatible = "sprd,ump518-adc", .data = &ump518_data},
+	{ }
+};
+
+static const struct dev_pm_ops sprd_adc_pm_ops = {
+	.suspend_noirq = sprd_adc_pm_suspend,
+	.resume_noirq = sprd_adc_pm_resume,
+};
+
+static struct platform_driver sprd_adc_driver = {
+	.probe = sprd_adc_probe,
+	.driver = {
+		.name = "sprd-adc",
+		.of_match_table = sprd_adc_of_match,
+		.pm	= &sprd_adc_pm_ops,
+	},
+};
+
+module_platform_driver(sprd_adc_driver);
+MODULE_DESCRIPTION("SPRD PMIC ADC Driver");
+MODULE_LICENSE("GPL");
-- 
2.17.1

Re: [PATCH V0 2/2] iio: adc: sprd_pmic_adc: Add support for UMP serise pmic adc

[kernel test robot]

Hi Mingjin,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on linus/master v6.5-rc6 next-20230816]
[cannot apply to jic23-iio/togreg]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Mingjin-Yang/dt-bindings-iio-adc-Add-support-for-ump518-pmic-adc/20230816-160754
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20230816080225.21482-3-mingjin.yang%40unisoc.com
patch subject: [PATCH V0 2/2] iio: adc: sprd_pmic_adc: Add support for UMP serise pmic adc
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20230816/202308162143.UjXhwiRf-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230816/202308162143.UjXhwiRf-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308162143.UjXhwiRf-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/iio/adc/sprd_pmic_adc.c: In function 'ump9620_ch_data_init':
>> drivers/iio/adc/sprd_pmic_adc.c:530:1: warning: the frame size of 1320 bytes is larger than 1024 bytes [-Wframe-larger-than=]
     530 | }
         | ^


vim +530 drivers/iio/adc/sprd_pmic_adc.c

   506	
   507	static void ump9620_ch_data_init(struct sprd_adc_data *data)
   508	{
   509		struct sprd_adc_channel_data ch_data_def = CH_DATA_INIT(SCALE_00, 0, 0, RATIO_DEF);
   510		struct sprd_adc_channel_data ch_data[SPRD_ADC_CHANNEL_MAX] = {
   511			[0] = CH_DATA_INIT(SCALE_01, 0, 0, RATIO_DEF),
   512			[5] = CH_DATA_INIT(SCALE_00, 0x1, 0x9, RATIO_DEF),
   513			[6] = CH_DATA_INIT(SCALE_00, 0x1, 0x9, RATIO_DEF),
   514			[7] = CH_DATA_INIT(SCALE_10, 0, 0, RATIO_DEF),
   515			[9] = CH_DATA_INIT(SCALE_10, 0, 0, RATIO_DEF),
   516			[10] = CH_DATA_INIT(SCALE_11, 0, 0, RATIO_DEF),
   517			[11] = CH_DATA_INIT(SCALE_00, 0, 0, RATIO_DEF),
   518			[13] = CH_DATA_INIT(SCALE_01, 0, 0, RATIO_DEF),
   519			[14] = CH_DATA_INIT(SCALE_00, 0, 0, RATIO(68, 900)),
   520			[15] = CH_DATA_INIT(SCALE_00, 0, 0, RATIO(1, 3)),
   521			[19] = CH_DATA_INIT(SCALE_11, 0, 0, RATIO_DEF),
   522			[21] = CH_DATA_INIT(SCALE_00, 0, 0, RATIO(3, 8)),
   523			[22] = CH_DATA_INIT(SCALE_00, 0, 0, RATIO(3, 8)),
   524			[23] = CH_DATA_INIT(SCALE_00, 0, 0, RATIO(3, 8)),
   525			[30] = CH_DATA_INIT(SCALE_11, 0, 0, RATIO_DEF),
   526			[31] = CH_DATA_INIT(SCALE_11, 0, 0, RATIO_DEF),
   527		};
   528	
   529		sprd_adc_ch_data_merge(data, ch_data, &ch_data_def);
 > 530	}
   531	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH V0 2/2] iio: adc: sprd_pmic_adc: Add support for UMP serise pmic adc

[Jonathan Cameron]

On Wed, 16 Aug 2023 22:07:15 +0800
kernel test robot <lkp@intel.com> wrote:

> Hi Mingjin,
> 
> kernel test robot noticed the following build warnings:
> 
> [auto build test WARNING on robh/for-next]
> [also build test WARNING on linus/master v6.5-rc6 next-20230816]
> [cannot apply to jic23-iio/togreg]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Mingjin-Yang/dt-bindings-iio-adc-Add-support-for-ump518-pmic-adc/20230816-160754
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
> patch link:    https://lore.kernel.org/r/20230816080225.21482-3-mingjin.yang%40unisoc.com
> patch subject: [PATCH V0 2/2] iio: adc: sprd_pmic_adc: Add support for UMP serise pmic adc
> config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20230816/202308162143.UjXhwiRf-lkp@intel.com/config)
> compiler: m68k-linux-gcc (GCC) 12.3.0
> reproduce: (https://download.01.org/0day-ci/archive/20230816/202308162143.UjXhwiRf-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202308162143.UjXhwiRf-lkp@intel.com/
> 
> All warnings (new ones prefixed by >>):
> 
>    drivers/iio/adc/sprd_pmic_adc.c: In function 'ump9620_ch_data_init':
> >> drivers/iio/adc/sprd_pmic_adc.c:530:1: warning: the frame size of 1320 bytes is larger than 1024 bytes [-Wframe-larger-than=]  
>      530 | }
>          | ^
> 
> 
> vim +530 drivers/iio/adc/sprd_pmic_adc.c
> 
>    506	
>    507	static void ump9620_ch_data_init(struct sprd_adc_data *data)
>    508	{
>    509		struct sprd_adc_channel_data ch_data_def = CH_DATA_INIT(SCALE_00, 0, 0, RATIO_DEF);
>    510		struct sprd_adc_channel_data ch_data[SPRD_ADC_CHANNEL_MAX] = {

Make these both const and I think the compiler will move these off the stack thus
solving the issue seen.

>    511			[0] = CH_DATA_INIT(SCALE_01, 0, 0, RATIO_DEF),
>    512			[5] = CH_DATA_INIT(SCALE_00, 0x1, 0x9, RATIO_DEF),
>    513			[6] = CH_DATA_INIT(SCALE_00, 0x1, 0x9, RATIO_DEF),
>    514			[7] = CH_DATA_INIT(SCALE_10, 0, 0, RATIO_DEF),
>    515			[9] = CH_DATA_INIT(SCALE_10, 0, 0, RATIO_DEF),
>    516			[10] = CH_DATA_INIT(SCALE_11, 0, 0, RATIO_DEF),
>    517			[11] = CH_DATA_INIT(SCALE_00, 0, 0, RATIO_DEF),
>    518			[13] = CH_DATA_INIT(SCALE_01, 0, 0, RATIO_DEF),
>    519			[14] = CH_DATA_INIT(SCALE_00, 0, 0, RATIO(68, 900)),
>    520			[15] = CH_DATA_INIT(SCALE_00, 0, 0, RATIO(1, 3)),
>    521			[19] = CH_DATA_INIT(SCALE_11, 0, 0, RATIO_DEF),
>    522			[21] = CH_DATA_INIT(SCALE_00, 0, 0, RATIO(3, 8)),
>    523			[22] = CH_DATA_INIT(SCALE_00, 0, 0, RATIO(3, 8)),
>    524			[23] = CH_DATA_INIT(SCALE_00, 0, 0, RATIO(3, 8)),
>    525			[30] = CH_DATA_INIT(SCALE_11, 0, 0, RATIO_DEF),
>    526			[31] = CH_DATA_INIT(SCALE_11, 0, 0, RATIO_DEF),
>    527		};
>    528	
>    529		sprd_adc_ch_data_merge(data, ch_data, &ch_data_def);

>  > 530	}  
>    531	
> 

Re: [PATCH V0 2/2] iio: adc: sprd_pmic_adc: Add support for UMP serise pmic adc

[Jonathan Cameron]

On Wed, 16 Aug 2023 16:02:25 +0800
Mingjin Yang <mingjin.yang@unisoc.com> wrote:

> This patch adds support for UNISOC UMP family
> of ADC driver. The ADC peripheral can support both
> voltage and current mode whose input signal is
> connected to the PMIC ADC AMUX.
> 
> Signed-off-by: Mingjin Yang <mingjin.yang@unisoc.com>

Hi Mingjin,

Thanks for your driver - it's in pretty good shape for a v0.

Comments inline.

Jonathan


> diff --git a/drivers/iio/adc/sprd_pmic_adc.c b/drivers/iio/adc/sprd_pmic_adc.c
> new file mode 100644
> index 000000000000..6113bd84cd77
> --- /dev/null
> +++ b/drivers/iio/adc/sprd_pmic_adc.c
> @@ -0,0 +1,1298 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2020 Unisoc Inc.
> +
> +#include <linux/hwspinlock.h>
> +#include <linux/iio/iio.h>
> +#include <linux/math64.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/nvmem-consumer.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +#include <linux/sort.h>
> +
> +/* ADC controller registers definition */
> +#define SPRD_ADC_CTL			0x0
> +
> +#define SPRD_ADC_CH_CFG			0x4
> +#define SPRD_ADC_CHN_ID_MASK		GENMASK(4, 0)
> +
> +#define SPRD_ADC_DATA			0x4c
> +#define SPRD_ADC_DATA_MASK		GENMASK(11, 0)
> +
> +#define SPRD_ADC_INT_EN			0x50
> +#define SPRD_ADC_IRQ_EN			BIT(0)
> +
> +#define SPRD_ADC_INT_CLR		0x54
> +#define SPRD_ADC_IRQ_CLR		BIT(0)
> +
> +#define SPRD_ADC_INT_STS		0x58
> +
> +#define SPRD_ADC_INT_RAW		0x5c
> +#define SPRD_ADC_IRQ_RAW		BIT(0)
> +
> +/* Bits and mask definition for SPRD_ADC_CTL register */
> +#define SPRD_ADC_EN			BIT(0)
> +#define SPRD_ADC_CHN_RUN		BIT(1)
> +#define SPRD_ADC_12BIT_MODE		BIT(2)
> +#define SPRD_ADC_RUN_NUM_MASK		GENMASK(7, 4)
> +#define SPRD_ADC_RUN_NUM_SHIFT		4
> +#define SPRD_ADC_AVERAGE_SHIFT		8
> +#define SPRD_ADC_AVERAGE_MASK		GENMASK(10, 8)
> +
> +/* Timeout (ms) for the trylock of hardware spinlocks */
> +#define SPRD_ADC_HWLOCK_TIMEOUT		5000
Name it so the units are CLAR
	SPRD_ADC_HWLOCK_TIMEOUT_MS
then drop teh comment that tells us nothing.
> +
> +/* Maximum ADC channel number */
Obvious comment: drop it.
> +#define SPRD_ADC_CHANNEL_MAX		32
> +
> +/* Timeout (us) for ADC data conversion according to ADC datasheet */
Reference specific part of datasheet or drop the comment.
> +#define SPRD_ADC_RDY_TIMEOUT		1000000
Name it so the unit is obviously
	TIMEOUT_US

> +#define SPRD_ADC_POLL_RAW_STATUS	500
What's this one?
> +
> +/* ADC voltage ratio definition */
> +#define SPRD_RATIO_NUMERATOR_OFFSET	16
> +#define SPRD_RATIO_DENOMINATOR_MASK	GENMASK(15, 0)
> +#define RATIO(n, d)			(((n) << SPRD_RATIO_NUMERATOR_OFFSET) | (d))
> +#define R_INVALID			(0xffffffff)
> +#define RATIO_DEF			RATIO(1, 1)

Prefix every define..

> +
> +/* ADC specific channel reference voltage 3.5V */
> +#define SPRD_ADC_REFVOL_VDD35		3500000
> +
> +/* ADC default channel reference voltage is 2.8V */
> +#define SPRD_ADC_REFVOL_VDD28		2800000
> +
> +#define SPRD_ADC_CELL_MAX		(2)
> +#define SPRD_ADC_INVALID_DATA		(0XFFFFFFFF)
> +#define SPRD_ADC_INIT_MAGIC		(0xa7a77a7a)
> +#define ADC_MESURE_NUMBER_SW		(15)
> +#define ADC_MESURE_NUMBER_HW_DEF	(3) /* 2 << 3 = 8 times */

Prefix with SPRD and also the brackets around integer values have little purpose
so drop them.  Note the ones around variables are good.

> +
> +#define CH_DATA_FMT "ch%d_d[(%d %d 0x%x 0x%x) | (%u/%u), (%u/%u), (%u/%u), (%u/%u), (%u/%u)]\n"
This define hurts readability.  Just put in inline in the one place it's used for debug.
> +
> +#define SPRD_ADC_CHANNEL(index, mask) {				\
> +	.type = IIO_VOLTAGE,					\
> +	.channel = index,					\
> +	.info_mask_separate = mask | BIT(IIO_CHAN_INFO_SCALE),	\
> +	.datasheet_name = "CH##index",				\
> +	.indexed = 1,						\
> +}
> +
> +#define CH_DATA_INIT(sl, filter, isen, ip_r)		\
> +{							\
> +	.scale = sl,					\
> +	.isen_info = isen,				\
> +	.filter_info = filter,				\
> +	.ip_ratio = ip_r,				\
> +	.ch_aux_ratio = {0, 0, 0, 0},			\
> +	.inited = SPRD_ADC_INIT_MAGIC,			\
> +}
> +
> +#define CALIB_INFO_INIT(ch, sl, gph)	\
> +{					\
> +	.channel = ch,			\
> +	.scale = sl,			\
> +	.graph = gph,			\
> +	.inited = SPRD_ADC_INIT_MAGIC,	\
> +}
> +
> +#define GRAPH_POINT_INIT(v0, a0, v1, a1)\

space before \
also why the blank line here?

> +					\
> +	.volt0 = v0,			\
> +	.adc0 = a0,			\
> +	.volt1 = v1,			\
> +	.adc1 = a1,			\
> +	.inited = SPRD_ADC_INIT_MAGIC
> +

> +
> +enum SPRD_PMIC_TYPE {
> +	UMP9620_ADC,
> +	UMP518_ADC,
> +};
> +
> +enum SPRD_ADC_GRAPH_TYPE {
> +	TWO_CELL_GRAPH,
> +	SPRD_ADC_GRAPH_TYPE_MAX
> +};
> +
> +enum SPRD_ADC_GRAPH_OFFSET {
> +	GRAPH_BIG,
> +	GRAPH_SMALL,
> +	GRAPH_VBAT_DET,
> +	GRAPH_MAX
> +};
> +
> +enum SPRD_ADC_SCALE {
> +	SCALE_00,
> +	SCALE_01,
> +	SCALE_10,
> +	SCALE_11,
> +	SPRD_ADC_SCALE_MAX
> +};
> +
> +enum SPRD_ADC_REG_TYPE {
> +	REG_MODULE_EN,
> +	REG_CLK_EN,
> +	REG_SOFT_RST,
> +	REG_XTL_CTRL,
> +	REG_SCALE,
> +	REG_CFG_FIXED_ST = 11,/* FIXED CFG START */
> +	REG_CAL_EN,
> +	REG_NTC_MUX,
> +	REG_CFG_FIXED_END,/* FIXED CFG END */
> +	REG_ISEN_ST = 21,/* CURRENT MODE START */
> +	REG_ISEN0,
> +	REG_ISEN1,
> +	REG_ISEN2,
> +	REG_ISEN3,
> +	REG_ISEN_END,/* CURRENT MODE END */
> +	SPRD_ADC_REG_TYPE_MAX
> +};
> +
> +enum SPRD_ADC_REG_BASE {
> +	BASE_GLB,
> +	BASE_ANA
> +};
> +
> +struct sprd_adc_pm_data {
> +	struct regmap *clk_regmap;
> +	u32 clk_reg;/* adc clk26 vote reg */
> +	u32 clk_reg_mask;/* adc clk26 vote reg mask */
> +	bool dev_suspended;
> +};
> +
> +/*bit[0-7]: scale
/*
 * bit[0-7]: scale
...
 */

However, what is it referring to?

> + *bit[8-15]: filter_info(bit8: sw filter support, bit[9-15]: hw filter val(2<<n))
> + *bit[16-23]: isen_info (bit16: isen support, bit[17-23]: isen val)
> + */
> +struct sprd_adc_channel_data {
> +	int scale;
> +	int graph;
> +	int isen_info;
> +	int filter_info;
> +	int ip_ratio;
> +	int ch_aux_ratio[SPRD_ADC_SCALE_MAX];
> +	int inited;
> +};
> +
> +struct calib_info {
> +	int channel;
> +	int scale;
> +	int graph;
> +	int ch_aux_ratio[SPRD_ADC_SCALE_MAX];
> +	int inited;
> +};
> +
> +struct reg_bit {

sprd_adc_reg_bit etc
to reduce chance of a clash on naming in future.

> +	u32 base;
> +	u32 reg_addr;
> +	u32 mask;
> +	u32 offset;
> +	u32 val_set;
> +	u32 val_clr;
> +	u32 inited;
> +	bool reverse;
> +	u32 (*get_val)(void *pri, int ch, bool set);
> +};



> +
> +static void sprd_adc_calib_with_two_cell(struct sprd_adc_linear_graph *graph);
> +static u32 get_isen(void *pri, int ch, bool enable);
> +static int sprd_adc_soft_rst(struct sprd_adc_data *data);
> +static int sprd_adc_hw_enable(struct sprd_adc_data *data);

blank line here.

> +static inline u32 GET_REG_ADDR(struct sprd_adc_data *data, int index)
Unless you have a very strong reason don't tell the compiler what to inline.
These hints are often limit what it can optimise somewhat and can hurt
the actual performance.

Generic naming without a prefix tends to break long term. Also, this is
just a function, shouldn't be in capitals.

sprd_adc_get_reg_addr()


> +{
> +	u32 base = ((data->var_data->reg_list[index].base == BASE_GLB)
> +		    ? (data->var_data->glb_reg_base)
> +		    : (data->base - data->var_data->adc_reg_base_offset));
> +	return (base + data->var_data->reg_list[index].reg_addr);
> +}


> +
> +static inline void sprd_adc_ch_data_show(struct sprd_adc_data *data, int ch)
> +{
> +	struct sprd_adc_channel_data *ch_data = &data->ch_data[ch];
> +	struct u32_fract ip_fract, aux_fract0, aux_fract1, aux_fract2, aux_fract3;
> +
> +	ratio_to_u32_fract(ch_data->ip_ratio, &ip_fract);
> +	ratio_to_u32_fract(ch_data->ch_aux_ratio[0], &aux_fract0);
> +	ratio_to_u32_fract(ch_data->ch_aux_ratio[1], &aux_fract1);
> +	ratio_to_u32_fract(ch_data->ch_aux_ratio[2], &aux_fract2);
> +	ratio_to_u32_fract(ch_data->ch_aux_ratio[3], &aux_fract3);
> +	dev_dbg(data->dev, CH_DATA_FMT,
> +		ch, ch_data->scale, ch_data->graph, ch_data->isen_info, ch_data->filter_info,
> +		ip_fract.numerator, ip_fract.denominator,
> +		aux_fract0.numerator, aux_fract0.denominator,
> +		aux_fract1.numerator, aux_fract1.denominator,
> +		aux_fract2.numerator, aux_fract2.denominator,
> +		aux_fract3.numerator, aux_fract3.denominator);

A lot of code in this driver is debug.  Is it actually useful once you've finished
development?  I'd consider dropping at least some of it to reduce the amount of
code you need to maintain long term.

> +}
> +
> +static void sprd_adc_ch_data_merge(struct sprd_adc_data *data, struct sprd_adc_channel_data *d_diff,

Long line - generally try to stay under 80 chars if it doesn't hurt readability
and under 100 unless it's a message someone might want to grep for.


> +				   struct sprd_adc_channel_data *d_def)
> +{
> +	struct sprd_adc_channel_data *ch_data;
> +	int ch;
> +
> +	for (ch = 0; ch < SPRD_ADC_CHANNEL_MAX; ch++) {
> +		ch_data = &data->ch_data[ch];
> +		*ch_data = ((d_diff[ch].inited == SPRD_ADC_INIT_MAGIC) ? d_diff[ch] : *d_def);
> +	}
> +
> +	for (ch = 0; ch < SPRD_ADC_CHANNEL_MAX; ch++) {
> +		sprd_adc_aux_ratio_init(data, ch);
> +		sprd_adc_ch_data_show(data, ch);
> +	}
> +}


> +static void ump9620_ch_data_init(struct sprd_adc_data *data)
> +{
> +	struct sprd_adc_channel_data ch_data_def = CH_DATA_INIT(SCALE_00, 0, 0, RATIO_DEF);
> +	struct sprd_adc_channel_data ch_data[SPRD_ADC_CHANNEL_MAX] = {

const

> +		[0] = CH_DATA_INIT(SCALE_01, 0, 0, RATIO_DEF),
> +		[5] = CH_DATA_INIT(SCALE_00, 0x1, 0x9, RATIO_DEF),
> +		[6] = CH_DATA_INIT(SCALE_00, 0x1, 0x9, RATIO_DEF),
> +		[7] = CH_DATA_INIT(SCALE_10, 0, 0, RATIO_DEF),
> +		[9] = CH_DATA_INIT(SCALE_10, 0, 0, RATIO_DEF),
> +		[10] = CH_DATA_INIT(SCALE_11, 0, 0, RATIO_DEF),
> +		[11] = CH_DATA_INIT(SCALE_00, 0, 0, RATIO_DEF),
> +		[13] = CH_DATA_INIT(SCALE_01, 0, 0, RATIO_DEF),
> +		[14] = CH_DATA_INIT(SCALE_00, 0, 0, RATIO(68, 900)),
> +		[15] = CH_DATA_INIT(SCALE_00, 0, 0, RATIO(1, 3)),
> +		[19] = CH_DATA_INIT(SCALE_11, 0, 0, RATIO_DEF),
> +		[21] = CH_DATA_INIT(SCALE_00, 0, 0, RATIO(3, 8)),
> +		[22] = CH_DATA_INIT(SCALE_00, 0, 0, RATIO(3, 8)),
> +		[23] = CH_DATA_INIT(SCALE_00, 0, 0, RATIO(3, 8)),
> +		[30] = CH_DATA_INIT(SCALE_11, 0, 0, RATIO_DEF),
> +		[31] = CH_DATA_INIT(SCALE_11, 0, 0, RATIO_DEF),
> +	};
> +
> +	sprd_adc_ch_data_merge(data, ch_data, &ch_data_def);
> +}
> +

> +
> +static int sprd_adc_isen_enable(struct sprd_adc_data *data, int channel)
> +{
> +	bool isen_support = data->ch_data[channel].isen_info & 0x1;

As below.

> +
> +	if (!isen_support)
> +		return 0;
> +
> +	sprd_adc_regs_set_order(data, channel, REG_ISEN_ST, REG_ISEN_END);
> +	usleep_range(500, 1000);
> +
> +	return 0;
> +}
> +
> +static int sprd_adc_isen_disable(struct sprd_adc_data *data, int channel)
> +{
> +	bool isen_support = data->ch_data[channel].isen_info & 0x1;

FIELD_GET()

> +
> +	if (!isen_support)
as isen_support not otherwise used.
	if (!FIELD_GET(...., ....))

> +		return 0;
> +
> +	sprd_adc_regs_clr_order_reverse(data, channel, REG_ISEN_ST, REG_ISEN_END);
> +	usleep_range(500, 1000);
> +
> +	return 0;
> +}
> +

> +
> +static int sprd_adc_enable(struct sprd_adc_data *data, int channel)
> +{
> +	int ret = 0;
> +	u32 reg_read = 0;
> +
> +	if (data->pm_data.clk_regmap) {
> +		ret = regmap_update_bits(data->pm_data.clk_regmap, data->pm_data.clk_reg,
> +					 data->pm_data.clk_reg_mask,
> +					 data->pm_data.clk_reg_mask);
> +		ret |= regmap_read(data->pm_data.clk_regmap, data->pm_data.clk_reg, &reg_read);
> +		if (ret) {
> +			dev_err(data->dev, "failed to enable clk26m, channel %d\n", channel);
> +			return ret;
> +		}
> +		dev_dbg(data->dev, "enable clk26m: ch %d, reg_read 0x%x\n", channel, reg_read);

Directly accessing the regmap of a clock seems unusual. Why not provide generic clock interfaces
for this?

> +	}
> +
> +	ret = sprd_adc_isen_enable(data, channel);
> +	if (ret) {
> +		dev_err(data->dev, "failed to enable isen\n");
> +		return ret;
> +	}
> +
> +	ret = regmap_update_bits(data->regmap, data->base + SPRD_ADC_CTL,
> +				 SPRD_ADC_EN, SPRD_ADC_EN);
> +	if (ret) {
> +		dev_err(data->dev, "failed to set SPRD_ADC_EN\n");
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> +
> +static int sprd_adc_disable(struct sprd_adc_data *data, int channel)
> +{
> +	int ret = 0;

Set just below here. Check for other instances of this.

> +	u32 reg_read = 0;
> +
> +	ret = regmap_update_bits(data->regmap, data->base + SPRD_ADC_CTL, SPRD_ADC_EN, 0);
> +	if (ret) {
> +		dev_err(data->dev, "failed to reset SPRD_ADC_EN\n");
> +		return ret;
> +	}
> +
> +	ret = sprd_adc_isen_disable(data, channel);
> +	if (ret) {
> +		dev_err(data->dev, "failed to disable isen\n");
> +		return ret;
> +	}
> +
> +	if (data->pm_data.clk_regmap) {
> +		ret = regmap_update_bits(data->pm_data.clk_regmap, data->pm_data.clk_reg,
> +					 data->pm_data.clk_reg_mask, 0);
> +		ret |= regmap_read(data->pm_data.clk_regmap, data->pm_data.clk_reg, &reg_read);

Don't combine ret values. Can result in complete garbage.  Just handle each error
separately.  It costs a few lines, but always works!

> +		if (ret) {
> +			dev_err(data->dev, "failed to disable clk26m, channel %d\n", channel);
> +			return ret;
> +		}
> +		dev_dbg(data->dev, "disable clk26m: ch %d, reg_read 0x%x\n", channel, reg_read);
> +	}
> +
> +	return ret;

return 0;
Can't get here with any other value that I can see.

> +}
> +
> +static int sprd_adc_read(struct sprd_adc_data *data, int channel, int scale, int *val)
> +{
> +	int ret = 0, sample_num_sw;
> +	u32 rawdata = 0, tmp, status, scale_shift, scale_mask;
> +	bool filter_sw = data->ch_data[channel].filter_info & 0x1;
> +	int sample_num_hw = data->ch_data[channel].filter_info >> 1;

FIELD_GET() for both of these.

> +
> +	if (data->pm_data.dev_suspended) {
> +		dev_err(data->dev, "adc_exp: adc bas been suspended, ignore.\n");

has

> +		return -EBUSY;
> +	}
> +
> +	dev_dbg(data->dev, "ch_data[%d]: scale %d, graph %d, filter_info 0x%x, isen_info 0x%x\n",
> +		channel, data->ch_data[channel].scale, data->ch_data[channel].graph,
> +		data->ch_data[channel].filter_info, data->ch_data[channel].isen_info);
> +
> +	ret = hwspin_lock_timeout_raw(data->hwlock, SPRD_ADC_HWLOCK_TIMEOUT);
> +	if (ret) {
> +		dev_err(data->dev, "timeout to get the hwspinlock\n");

timeout getting the 
or
timeout trying to get 

> +		return ret;
> +	}
> +
> +	ret = sprd_adc_enable(data, channel);
> +	if (ret)
> +		goto disable_adc;
> +
> +	ret = regmap_update_bits(data->regmap, data->base + SPRD_ADC_INT_CLR,
> +				 SPRD_ADC_IRQ_CLR, SPRD_ADC_IRQ_CLR);
> +	if (ret)
> +		goto disable_adc;
> +
> +	/* Configure the channel id and scale */
> +	scale_shift = data->var_data->reg_list[REG_SCALE].offset;
> +	scale_mask = data->var_data->reg_list[REG_SCALE].mask;
> +	tmp = (scale << scale_shift) & scale_mask;
> +	tmp |= channel & SPRD_ADC_CHN_ID_MASK;
> +	ret = regmap_update_bits(data->regmap, data->base + SPRD_ADC_CH_CFG,
> +				 SPRD_ADC_CHN_ID_MASK | scale_mask, tmp);
> +	if (ret)
> +		goto disable_adc;
> +
> +	/* Select 12bit conversion mode, and only sample 1 time */
> +	tmp = SPRD_ADC_12BIT_MODE;
> +	sample_num_sw = (filter_sw ? ADC_MESURE_NUMBER_SW - 1 : 0);
> +	sample_num_hw = ((sample_num_hw > 0) ? sample_num_hw : ADC_MESURE_NUMBER_HW_DEF);
> +	tmp |= ((unsigned long)sample_num_sw << SPRD_ADC_RUN_NUM_SHIFT) & SPRD_ADC_RUN_NUM_MASK;
> +	tmp |= ((unsigned long)sample_num_hw << SPRD_ADC_AVERAGE_SHIFT) & SPRD_ADC_AVERAGE_MASK;
> +	ret = regmap_update_bits(data->regmap, data->base + SPRD_ADC_CTL,
> +				 SPRD_ADC_RUN_NUM_MASK | SPRD_ADC_12BIT_MODE |
> +				 SPRD_ADC_AVERAGE_MASK, tmp);
> +	if (ret)
> +		goto disable_adc;
> +
> +	ret = regmap_update_bits(data->regmap, data->base + SPRD_ADC_CTL,
> +				 SPRD_ADC_CHN_RUN, SPRD_ADC_CHN_RUN);
> +	if (ret)
> +		goto disable_adc;
> +
> +	ret = regmap_read_poll_timeout(data->regmap, data->base + SPRD_ADC_INT_RAW, status,
> +				       (status & SPRD_ADC_IRQ_RAW), SPRD_ADC_POLL_RAW_STATUS,
> +				       SPRD_ADC_RDY_TIMEOUT);
> +	if (ret) {
> +		dev_err(data->dev, "read adc timeout 0x%x\n", status);
> +		sprd_adc_regs_dump(data, channel, scale, "t_bef");
> +		sprd_adc_hw_enable(data);
> +		sprd_adc_soft_rst(data);
> +		sprd_adc_regs_dump(data, channel, scale, "t_aft");
> +		goto disable_adc;
> +	}
> +
> +	if (filter_sw) {
> +		rawdata = sprd_adc_get_val_with_sw_filter(data, channel);
> +	} else {
> +		ret = regmap_read(data->regmap, data->base + SPRD_ADC_DATA, &rawdata);
check ret.
> +		rawdata &= SPRD_ADC_DATA_MASK;
> +	}
> +disable_adc:
> +	ret = sprd_adc_disable(data, channel);
> +
> +	hwspin_unlock_raw(data->hwlock);
> +
> +	if (!ret)
> +		*val = rawdata;
I think val is valid even if the disable fails. So do it up a few lines unconditionally.

> +
> +	return ret;
> +}
> +
> +static int sprd_adc_calculate_volt_by_graph(struct sprd_adc_data *data, int channel,
> +					    int scale, int raw_adc)
> +{
> +	int tmp;
> +	struct sprd_adc_graphs *graphs = &sprd_adc_graphs_array[data->var_data->graph_index];
> +	int graph_offset = data->ch_data[channel].graph;
> +	struct sprd_adc_linear_graph *linear_graph = &graphs->adc_graphs[graph_offset];
> +
> +	tmp = (linear_graph->volt0 - linear_graph->volt1) * (raw_adc - linear_graph->adc1);
> +	tmp /= (linear_graph->adc0 - linear_graph->adc1);
> +	tmp += linear_graph->volt1;
> +	tmp = (tmp < 0 ? 0 : tmp);

	tmp = max(0, tmp) perhaps makes it clearer this is clamping a small negative (I think)

> +
> +	dev_dbg(data->dev, "by_graph_c%d: v0 %d a0 %d, v1 %d a1 %d, raw_adc 0x%x, vol_graph %d\n",
> +		channel, linear_graph->volt0, linear_graph->adc0, linear_graph->volt1,
> +		linear_graph->adc1, raw_adc, tmp);
> +
> +	return tmp;
> +}
> +
> +static int sprd_adc_calculate_volt_by_ratio(struct sprd_adc_data *data, int channel,
> +					    int scale, int vol_graph)
> +{
> +	struct u32_fract ip_fract, aux_fract;
> +	int pmic_type = data->var_data->pmic_type;
> +	u32 vol_final = vol_graph;
> +
> +	ratio_to_u32_fract(data->ch_data[channel].ip_ratio, &ip_fract);
> +	vol_final = DIV_ROUND_CLOSEST(vol_final * ip_fract.denominator, ip_fract.numerator);
> +
> +	ratio_to_u32_fract(data->ch_data[channel].ch_aux_ratio[scale], &aux_fract);
> +	vol_final = DIV_ROUND_CLOSEST(vol_final * aux_fract.denominator, aux_fract.numerator);
> +
> +	dev_dbg(data->dev, "by_ratio_c%d: id %d, scale %d, ip_r[%d/%d], aux_r[%d/%d], vol_f %d\n",
> +		channel, pmic_type, scale, ip_fract.numerator, ip_fract.denominator,
> +		aux_fract.numerator, aux_fract.denominator, vol_final);
> +
> +	return vol_final;
> +}
> +
> +static int sprd_adc_read_processed(struct sprd_adc_data *data, int channel, int scale, int *val)
> +{
> +	int ret, raw_adc, vol_graph;
> +
> +	ret = sprd_adc_read(data, channel, scale, &raw_adc);
> +
> +	if (ret)
> +		return ret;
> +
> +	vol_graph = sprd_adc_calculate_volt_by_graph(data, channel, scale, raw_adc);
> +	*val = sprd_adc_calculate_volt_by_ratio(data, channel, scale, vol_graph);
> +
> +	return 0;
> +}
> +
> +static int sprd_adc_ch_data_encode(struct sprd_adc_data *data, int ch)
> +{
> +	int scale = data->ch_data[ch].scale & 0xff;
> +	int isen_info = data->ch_data[ch].isen_info & 0xff;
> +	int filter_info = data->ch_data[ch].filter_info & 0xff;
> +
> +	return (scale | (filter_info << 8) | (isen_info << 16));

As below - looks like an array of u8. Treat it as such or use FIELD_PREP()
and treat it as a u32


> +}
> +
> +static void sprd_adc_ch_data_decode(struct sprd_adc_data *data, int ch, int val)
> +{
> +	int scale_override, filter_override, isen_override;
> +
> +	scale_override = (val & 0xff);
> +	filter_override = ((val >> 8) & 0xff);
> +	isen_override = ((val >> 16) & 0xff);

FIELD_GET() with appropriate mask defines.
Or just convert it to an array of u8 with appropriate endian conversion.


> +
> +	if (scale_override < SPRD_ADC_SCALE_MAX && scale_override != data->ch_data[ch].scale)
> +		data->ch_data[ch].scale = scale_override;
> +
> +	if (data->ch_data[ch].filter_info != filter_override)
> +		data->ch_data[ch].filter_info = filter_override;
> +
> +	if (data->ch_data[ch].isen_info != isen_override)
> +		data->ch_data[ch].isen_info = isen_override;
> +}
> +

...

> +static int sprd_adc_hw_enable(struct sprd_adc_data *data)
> +{
> +	int ret;
> +	u32 reg_addr, mask;
> +
> +	reg_addr = GET_REG_ADDR(data, REG_XTL_CTRL);

I'm not sure the local variables much help readability.
So I'd just put them inline instead perhaps providing a
local variabel for the reglist.
	u32 *reg_list = data->var_data->reg_list;

	ret = regmap_update_bits(data->regmap,
				 GET_REG_ADDR(data, REG_XTL_CTRL),
				 reg_list[REG_XTL_CTRL]);

etc.  May make sense for other similar cases.


> +	mask = data->var_data->reg_list[REG_XTL_CTRL].mask;
> +	ret = regmap_update_bits(data->regmap, reg_addr, mask, mask);
> +	if (ret)
> +		return ret;
> +
> +	reg_addr = GET_REG_ADDR(data, REG_MODULE_EN);
> +	mask = data->var_data->reg_list[REG_MODULE_EN].mask;
> +	ret = regmap_update_bits(data->regmap, reg_addr, mask, mask);
> +	if (ret)
> +		return ret;
> +
> +	/* Enable ADC work clock */
> +	reg_addr = GET_REG_ADDR(data, REG_CLK_EN);
> +	mask = data->var_data->reg_list[REG_CLK_EN].mask;
> +	ret = regmap_update_bits(data->regmap, reg_addr, mask, mask);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

	return regmap_update_bits()

> +}
> +

> +
> +static int sprd_adc_ch_data_init(struct sprd_adc_data *data)
> +{
> +	struct device_node *np = data->dev->of_node;
> +	int size, ret, ch, ch_data_val, i;
> +	u32 *ch_data_override;
> +
> +	data->var_data->ch_data_init(data);
> +
> +	size = of_property_count_elems_of_size(np, "ch_data_override", sizeof(u32));
> +	if (size <= 0)
> +		return 0;
> +
> +	if (size % 2) {
> +		dev_err(data->dev, "Pair of ch data err!\n");
> +		return -EINVAL;
> +	}
> +
> +	ch_data_override = kcalloc(size, sizeof(u32), GFP_KERNEL);
> +	if (!ch_data_override)
> +		return -ENOMEM;
> +
> +	ret = of_property_read_u32_array(np, "ch_data_override", ch_data_override, size);
> +	if (ret < 0) {
> +		dev_err(data->dev, "Failed to read ch data from dt: %d\n", ret);
> +		kfree(ch_data_override);

Probably better to use a goto for this cleanup so it can be shared with the normal
exit path. Of if you want to have fun (and don't need to backport this),
try the new __free() infrastructure to do scope based cleanup found in

linux/cleanup.h

This sort of pattern is exactly what that code is for.


> +		return ret;
> +	}
> +
> +	for (i = 0; i < size; i += 2) {
> +		ch = ch_data_override[i];
> +		ch_data_val = ch_data_override[i+1];
kernel style always has spaces around an operator with two parameters.
[i + 1]
> +		sprd_adc_ch_data_decode(data, ch, ch_data_val);
> +		sprd_adc_ch_data_show(data, ch);
> +	}
> +
> +	kfree(ch_data_override);
> +
> +	return 0;
> +}
> +

> +
> +static int sprd_adc_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct sprd_adc_data *sprd_data;
> +	const struct sprd_adc_variant_data *pdata;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	pdata = of_device_get_match_data(&pdev->dev);

device_get_match_data()


> +	if (!pdata) {
> +		dev_err(&pdev->dev, "No matching driver data found\n");
> +		return -EINVAL;
> +	}
> +
> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*sprd_data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	sprd_data = iio_priv(indio_dev);
> +
> +	sprd_data->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> +	if (!sprd_data->regmap) {
> +		dev_err(&pdev->dev, "failed to get ADC regmap\n");
> +		return -ENODEV;
> +	}
> +
> +	ret = of_property_read_u32(np, "reg", &sprd_data->base);

Even though some elements of this (of_hwspin...) don't have generic firmware
interfaces, I would prefer to see those from linux/property.h used
wherever possible.  It will take us a long time to make that a subsystem
wide change, but good not to have more unnecessary instances of device tree
specific property reading.

> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to get ADC base address\n");
> +		return ret;
> +	}
> +
> +	sprd_data->irq = platform_get_irq(pdev, 0);
> +	if (sprd_data->irq < 0) {
> +		dev_err(&pdev->dev, "failed to get ADC irq number\n");
> +		return sprd_data->irq;
> +	}
> +
> +	ret = of_hwspin_lock_get_id(np, 0);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to get hwspinlock id\n");
> +		return ret;
> +	}
> +
> +	mutex_init(&sprd_data->mutex);
> +	sprd_data->hwlock = hwspin_lock_request_specific(ret);
> +	if (!sprd_data->hwlock) {
> +		dev_err(&pdev->dev, "failed to request hwspinlock\n");
> +		return -ENXIO;
> +	}
> +
> +	ret = devm_add_action(&pdev->dev, sprd_adc_free_lock, sprd_data);
> +	if (ret) {
> +		sprd_adc_free_lock(sprd_data);

devm_add_action_or_reset() as below.  Same for any more cases of this, I won't
mention it again (not I review up the driver as they make more logical sense
to me that way around ;)

> +		dev_err(&pdev->dev, "failed to add free lock action\n");
> +		return ret;
> +	}
> +
> +	sprd_data->dev = &pdev->dev;
> +	sprd_data->var_data = pdata;
> +	sprd_data->indio_dev = indio_dev;
> +
> +	/* ADC channel scales calibration from nvmem device */
> +	ret = sprd_adc_graphs_calibrate(sprd_data);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to calib graphs from nvmem\n");
> +		return ret;
> +	}
> +
> +	ret = sprd_adc_pm_init(sprd_data);
> +	if (ret) {
> +		dev_err(&pdev->dev, "adc pm init err.\n");
> +		return ret;
> +	}
> +
> +	ret = sprd_adc_ch_data_init(sprd_data);
> +	if (ret) {
> +		dev_err(&pdev->dev, "ch data init err.\n");
> +		return ret;
> +	}
> +
> +	ret = sprd_adc_hw_enable(sprd_data);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to enable ADC module\n");
> +		return ret;
> +	}
> +
> +	ret = devm_add_action(&pdev->dev, sprd_adc_hw_disable, sprd_data);

ret = devm_add_action_or_reset() and...

> +	if (ret) {
> +		sprd_adc_hw_disable(sprd_data);

drop this.

This is such a common case that we have the or_reset() form for it.

> +		dev_err(&pdev->dev, "failed to add ADC disable action\n");
> +		return ret;

return dev_err_probe(&pdev->dev, ret, "...");
Same for all similar error handling in probe, or functions that are only
called from probe.

> +	}
> +
> +	ret = sprd_adc_soft_rst(sprd_data);
> +	if (ret) {
> +		dev_err(&pdev->dev, "adc soft rst failed\n");
> +		return ret;
		return dev_err_probe(...);

> +	}
> +
> +	indio_dev->dev.parent = &pdev->dev;
> +	indio_dev->name = dev_name(&pdev->dev);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &sprd_info;
> +	indio_dev->channels = sprd_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(sprd_channels);
> +	ret = devm_iio_device_register(&pdev->dev, indio_dev);
> +	if (ret)
> +		dev_err(&pdev->dev, "could not register iio (ADC)");

Prefer return dev_err_probe() in all cases.
Also should not carry on to set drvdata if this failed as we should never
need it.

> +
> +	platform_set_drvdata(pdev, indio_dev);
> +
> +	return ret;
> +}

> +
> +static const struct sprd_adc_variant_data ump9620_data = {
> +	.pmic_type = UMP9620_ADC,
> +	.glb_reg_base = 0x2000,
> +	.adc_reg_base_offset = 0x4,
> +	.reg_list = regs_ump9620,
> +	.graph_index = TWO_CELL_GRAPH,
> +	.calib_infos = {
> +		CALIB_INFO_INIT(11, SCALE_00, GRAPH_BIG),
> +		CALIB_INFO_INIT(1, SCALE_00, GRAPH_SMALL),
> +		CALIB_INFO_INIT(0, SCALE_01, GRAPH_VBAT_DET)
> +	},
> +	.aux_ratio_comm = {RATIO_DEF, RATIO(100, 133), RATIO(1000, 2600), RATIO(1000, 4060)},
> +	.ch_data_init = ump9620_ch_data_init,
> +};
> +
> +static const struct sprd_adc_variant_data ump518_data = {
> +	.pmic_type = UMP518_ADC,
> +	.glb_reg_base = 0x1800,
> +	.adc_reg_base_offset = 0x4,
> +	.reg_list = regs_ump9620,
> +	.graph_index = TWO_CELL_GRAPH,
> +	.calib_infos = {
> +		CALIB_INFO_INIT(11, SCALE_00, GRAPH_BIG),
> +		CALIB_INFO_INIT(1, SCALE_00, GRAPH_SMALL),
> +		CALIB_INFO_INIT(0, SCALE_01, GRAPH_VBAT_DET)
> +	},
> +	.aux_ratio_comm = {RATIO_DEF, RATIO(100, 133), RATIO(1000, 2600), RATIO(1000, 4060)},
> +	.ch_data_init = ump9620_ch_data_init,
> +};
> +
> +static const struct of_device_id sprd_adc_of_match[] = {
> +	{ .compatible = "sprd,ump9620-adc", .data = &ump9620_data},
> +	{ .compatible = "sprd,ump518-adc", .data = &ump518_data},

Space before } for consistency with the opening bracket.

Numerical order preferred as for the bindings.

> +	{ }
> +};
> +
> +static const struct dev_pm_ops sprd_adc_pm_ops = {
> +	.suspend_noirq = sprd_adc_pm_suspend,
> +	.resume_noirq = sprd_adc_pm_resume,
> +};
> +
> +static struct platform_driver sprd_adc_driver = {
> +	.probe = sprd_adc_probe,
> +	.driver = {
> +		.name = "sprd-adc",
> +		.of_match_table = sprd_adc_of_match,
> +		.pm	= &sprd_adc_pm_ops,

Just space before = not a tab.

Also what stops this building without pm_sleep support?
You probably should either map to one of the macros that set the pm
ops up or add pm_sleep_ptr() protection to allow the sprd_adc_pm_ops
structure to be removed if appropriate.

> +	},
> +};
> +
> +module_platform_driver(sprd_adc_driver);
> +MODULE_DESCRIPTION("SPRD PMIC ADC Driver");
> +MODULE_LICENSE("GPL");

Re: [PATCH V0 2/2] iio: adc: sprd_pmic_adc: Add support for UMP serise pmic adc

[杨明金]

Jonathan Cameron <jic23@kernel.org> 于2023年8月28日周一 23:56写道：
>
> On Wed, 16 Aug 2023 16:02:25 +0800
> Mingjin Yang <mingjin.yang@unisoc.com> wrote:
>
> > This patch adds support for UNISOC UMP family
> > of ADC driver. The ADC peripheral can support both
> > voltage and current mode whose input signal is
> > connected to the PMIC ADC AMUX.
> >
> > Signed-off-by: Mingjin Yang <mingjin.yang@unisoc.com>
>
> Hi Mingjin,
>
> Thanks for your driver - it's in pretty good shape for a v0.
>
> Comments inline.
>
> Jonathan
>
>
> > diff --git a/drivers/iio/adc/sprd_pmic_adc.c b/drivers/iio/adc/sprd_pmic_adc.c
> > new file mode 100644
> > index 000000000000..6113bd84cd77
> > --- /dev/null
> > +++ b/drivers/iio/adc/sprd_pmic_adc.c
> > @@ -0,0 +1,1298 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (C) 2020 Unisoc Inc.
> > +
> > +#include <linux/hwspinlock.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/math64.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/module.h>
> > +#include <linux/nvmem-consumer.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/slab.h>
> > +#include <linux/sort.h>
> > +
> > +/* ADC controller registers definition */
> > +#define SPRD_ADC_CTL                 0x0
> > +
> > +#define SPRD_ADC_CH_CFG                      0x4
> > +#define SPRD_ADC_CHN_ID_MASK         GENMASK(4, 0)
> > +
> > +#define SPRD_ADC_DATA                        0x4c
> > +#define SPRD_ADC_DATA_MASK           GENMASK(11, 0)
> > +
> > +#define SPRD_ADC_INT_EN                      0x50
> > +#define SPRD_ADC_IRQ_EN                      BIT(0)
> > +
> > +#define SPRD_ADC_INT_CLR             0x54
> > +#define SPRD_ADC_IRQ_CLR             BIT(0)
> > +
> > +#define SPRD_ADC_INT_STS             0x58
> > +
> > +#define SPRD_ADC_INT_RAW             0x5c
> > +#define SPRD_ADC_IRQ_RAW             BIT(0)
> > +
> > +/* Bits and mask definition for SPRD_ADC_CTL register */
> > +#define SPRD_ADC_EN                  BIT(0)
> > +#define SPRD_ADC_CHN_RUN             BIT(1)
> > +#define SPRD_ADC_12BIT_MODE          BIT(2)
> > +#define SPRD_ADC_RUN_NUM_MASK                GENMASK(7, 4)
> > +#define SPRD_ADC_RUN_NUM_SHIFT               4
> > +#define SPRD_ADC_AVERAGE_SHIFT               8
> > +#define SPRD_ADC_AVERAGE_MASK                GENMASK(10, 8)
> > +
> > +/* Timeout (ms) for the trylock of hardware spinlocks */
> > +#define SPRD_ADC_HWLOCK_TIMEOUT              5000
> Name it so the units are CLAR
>         SPRD_ADC_HWLOCK_TIMEOUT_MS
> then drop teh comment that tells us nothing.
> > +
> > +/* Maximum ADC channel number */
> Obvious comment: drop it.
> > +#define SPRD_ADC_CHANNEL_MAX         32
> > +
> > +/* Timeout (us) for ADC data conversion according to ADC datasheet */
> Reference specific part of datasheet or drop the comment.
> > +#define SPRD_ADC_RDY_TIMEOUT         1000000
> Name it so the unit is obviously
>         TIMEOUT_US
>
> > +#define SPRD_ADC_POLL_RAW_STATUS     500
> What's this one?
> > +
> > +/* ADC voltage ratio definition */
> > +#define SPRD_RATIO_NUMERATOR_OFFSET  16
> > +#define SPRD_RATIO_DENOMINATOR_MASK  GENMASK(15, 0)
> > +#define RATIO(n, d)                  (((n) << SPRD_RATIO_NUMERATOR_OFFSET) | (d))
> > +#define R_INVALID                    (0xffffffff)
> > +#define RATIO_DEF                    RATIO(1, 1)
>
> Prefix every define..
>
> > +
> > +/* ADC specific channel reference voltage 3.5V */
> > +#define SPRD_ADC_REFVOL_VDD35                3500000
> > +
> > +/* ADC default channel reference voltage is 2.8V */
> > +#define SPRD_ADC_REFVOL_VDD28                2800000
> > +
> > +#define SPRD_ADC_CELL_MAX            (2)
> > +#define SPRD_ADC_INVALID_DATA                (0XFFFFFFFF)
> > +#define SPRD_ADC_INIT_MAGIC          (0xa7a77a7a)
> > +#define ADC_MESURE_NUMBER_SW         (15)
> > +#define ADC_MESURE_NUMBER_HW_DEF     (3) /* 2 << 3 = 8 times */
>
> Prefix with SPRD and also the brackets around integer values have little purpose
> so drop them.  Note the ones around variables are good.
>
> > +
> > +#define CH_DATA_FMT "ch%d_d[(%d %d 0x%x 0x%x) | (%u/%u), (%u/%u), (%u/%u), (%u/%u), (%u/%u)]\n"
> This define hurts readability.  Just put in inline in the one place it's used for debug.
> > +
> > +#define SPRD_ADC_CHANNEL(index, mask) {                              \
> > +     .type = IIO_VOLTAGE,                                    \
> > +     .channel = index,                                       \
> > +     .info_mask_separate = mask | BIT(IIO_CHAN_INFO_SCALE),  \
> > +     .datasheet_name = "CH##index",                          \
> > +     .indexed = 1,                                           \
> > +}
> > +
> > +#define CH_DATA_INIT(sl, filter, isen, ip_r)         \
> > +{                                                    \
> > +     .scale = sl,                                    \
> > +     .isen_info = isen,                              \
> > +     .filter_info = filter,                          \
> > +     .ip_ratio = ip_r,                               \
> > +     .ch_aux_ratio = {0, 0, 0, 0},                   \
> > +     .inited = SPRD_ADC_INIT_MAGIC,                  \
> > +}
> > +
> > +#define CALIB_INFO_INIT(ch, sl, gph) \
> > +{                                    \
> > +     .channel = ch,                  \
> > +     .scale = sl,                    \
> > +     .graph = gph,                   \
> > +     .inited = SPRD_ADC_INIT_MAGIC,  \
> > +}
> > +
> > +#define GRAPH_POINT_INIT(v0, a0, v1, a1)\
>
> space before \
> also why the blank line here?
>
> > +                                     \
> > +     .volt0 = v0,                    \
> > +     .adc0 = a0,                     \
> > +     .volt1 = v1,                    \
> > +     .adc1 = a1,                     \
> > +     .inited = SPRD_ADC_INIT_MAGIC
> > +
>
> > +
> > +enum SPRD_PMIC_TYPE {
> > +     UMP9620_ADC,
> > +     UMP518_ADC,
> > +};
> > +
> > +enum SPRD_ADC_GRAPH_TYPE {
> > +     TWO_CELL_GRAPH,
> > +     SPRD_ADC_GRAPH_TYPE_MAX
> > +};
> > +
> > +enum SPRD_ADC_GRAPH_OFFSET {
> > +     GRAPH_BIG,
> > +     GRAPH_SMALL,
> > +     GRAPH_VBAT_DET,
> > +     GRAPH_MAX
> > +};
> > +
> > +enum SPRD_ADC_SCALE {
> > +     SCALE_00,
> > +     SCALE_01,
> > +     SCALE_10,
> > +     SCALE_11,
> > +     SPRD_ADC_SCALE_MAX
> > +};
> > +
> > +enum SPRD_ADC_REG_TYPE {
> > +     REG_MODULE_EN,
> > +     REG_CLK_EN,
> > +     REG_SOFT_RST,
> > +     REG_XTL_CTRL,
> > +     REG_SCALE,
> > +     REG_CFG_FIXED_ST = 11,/* FIXED CFG START */
> > +     REG_CAL_EN,
> > +     REG_NTC_MUX,
> > +     REG_CFG_FIXED_END,/* FIXED CFG END */
> > +     REG_ISEN_ST = 21,/* CURRENT MODE START */
> > +     REG_ISEN0,
> > +     REG_ISEN1,
> > +     REG_ISEN2,
> > +     REG_ISEN3,
> > +     REG_ISEN_END,/* CURRENT MODE END */
> > +     SPRD_ADC_REG_TYPE_MAX
> > +};
> > +
> > +enum SPRD_ADC_REG_BASE {
> > +     BASE_GLB,
> > +     BASE_ANA
> > +};
> > +
> > +struct sprd_adc_pm_data {
> > +     struct regmap *clk_regmap;
> > +     u32 clk_reg;/* adc clk26 vote reg */
> > +     u32 clk_reg_mask;/* adc clk26 vote reg mask */
> > +     bool dev_suspended;
> > +};
> > +
> > +/*bit[0-7]: scale
> /*
>  * bit[0-7]: scale
> ...
>  */
>
> However, what is it referring to?
>
> > + *bit[8-15]: filter_info(bit8: sw filter support, bit[9-15]: hw filter val(2<<n))
> > + *bit[16-23]: isen_info (bit16: isen support, bit[17-23]: isen val)
> > + */
> > +struct sprd_adc_channel_data {
> > +     int scale;
> > +     int graph;
> > +     int isen_info;
> > +     int filter_info;
> > +     int ip_ratio;
> > +     int ch_aux_ratio[SPRD_ADC_SCALE_MAX];
> > +     int inited;
> > +};
> > +
> > +struct calib_info {
> > +     int channel;
> > +     int scale;
> > +     int graph;
> > +     int ch_aux_ratio[SPRD_ADC_SCALE_MAX];
> > +     int inited;
> > +};
> > +
> > +struct reg_bit {
>
> sprd_adc_reg_bit etc
> to reduce chance of a clash on naming in future.
>
> > +     u32 base;
> > +     u32 reg_addr;
> > +     u32 mask;
> > +     u32 offset;
> > +     u32 val_set;
> > +     u32 val_clr;
> > +     u32 inited;
> > +     bool reverse;
> > +     u32 (*get_val)(void *pri, int ch, bool set);
> > +};
>
>
>
> > +
> > +static void sprd_adc_calib_with_two_cell(struct sprd_adc_linear_graph *graph);
> > +static u32 get_isen(void *pri, int ch, bool enable);
> > +static int sprd_adc_soft_rst(struct sprd_adc_data *data);
> > +static int sprd_adc_hw_enable(struct sprd_adc_data *data);
>
> blank line here.
>
> > +static inline u32 GET_REG_ADDR(struct sprd_adc_data *data, int index)
> Unless you have a very strong reason don't tell the compiler what to inline.
> These hints are often limit what it can optimise somewhat and can hurt
> the actual performance.
>
> Generic naming without a prefix tends to break long term. Also, this is
> just a function, shouldn't be in capitals.
>
> sprd_adc_get_reg_addr()
>
>
> > +{
> > +     u32 base = ((data->var_data->reg_list[index].base == BASE_GLB)
> > +                 ? (data->var_data->glb_reg_base)
> > +                 : (data->base - data->var_data->adc_reg_base_offset));
> > +     return (base + data->var_data->reg_list[index].reg_addr);
> > +}
>
>
> > +
> > +static inline void sprd_adc_ch_data_show(struct sprd_adc_data *data, int ch)
> > +{
> > +     struct sprd_adc_channel_data *ch_data = &data->ch_data[ch];
> > +     struct u32_fract ip_fract, aux_fract0, aux_fract1, aux_fract2, aux_fract3;
> > +
> > +     ratio_to_u32_fract(ch_data->ip_ratio, &ip_fract);
> > +     ratio_to_u32_fract(ch_data->ch_aux_ratio[0], &aux_fract0);
> > +     ratio_to_u32_fract(ch_data->ch_aux_ratio[1], &aux_fract1);
> > +     ratio_to_u32_fract(ch_data->ch_aux_ratio[2], &aux_fract2);
> > +     ratio_to_u32_fract(ch_data->ch_aux_ratio[3], &aux_fract3);
> > +     dev_dbg(data->dev, CH_DATA_FMT,
> > +             ch, ch_data->scale, ch_data->graph, ch_data->isen_info, ch_data->filter_info,
> > +             ip_fract.numerator, ip_fract.denominator,
> > +             aux_fract0.numerator, aux_fract0.denominator,
> > +             aux_fract1.numerator, aux_fract1.denominator,
> > +             aux_fract2.numerator, aux_fract2.denominator,
> > +             aux_fract3.numerator, aux_fract3.denominator);
>
> A lot of code in this driver is debug.  Is it actually useful once you've finished
> development?  I'd consider dropping at least some of it to reduce the amount of
> code you need to maintain long term.
>
> > +}
> > +
> > +static void sprd_adc_ch_data_merge(struct sprd_adc_data *data, struct sprd_adc_channel_data *d_diff,
>
> Long line - generally try to stay under 80 chars if it doesn't hurt readability
> and under 100 unless it's a message someone might want to grep for.
>
>
> > +                                struct sprd_adc_channel_data *d_def)
> > +{
> > +     struct sprd_adc_channel_data *ch_data;
> > +     int ch;
> > +
> > +     for (ch = 0; ch < SPRD_ADC_CHANNEL_MAX; ch++) {
> > +             ch_data = &data->ch_data[ch];
> > +             *ch_data = ((d_diff[ch].inited == SPRD_ADC_INIT_MAGIC) ? d_diff[ch] : *d_def);
> > +     }
> > +
> > +     for (ch = 0; ch < SPRD_ADC_CHANNEL_MAX; ch++) {
> > +             sprd_adc_aux_ratio_init(data, ch);
> > +             sprd_adc_ch_data_show(data, ch);
> > +     }
> > +}
>
>
> > +static void ump9620_ch_data_init(struct sprd_adc_data *data)
> > +{
> > +     struct sprd_adc_channel_data ch_data_def = CH_DATA_INIT(SCALE_00, 0, 0, RATIO_DEF);
> > +     struct sprd_adc_channel_data ch_data[SPRD_ADC_CHANNEL_MAX] = {
>
> const
>
> > +             [0] = CH_DATA_INIT(SCALE_01, 0, 0, RATIO_DEF),
> > +             [5] = CH_DATA_INIT(SCALE_00, 0x1, 0x9, RATIO_DEF),
> > +             [6] = CH_DATA_INIT(SCALE_00, 0x1, 0x9, RATIO_DEF),
> > +             [7] = CH_DATA_INIT(SCALE_10, 0, 0, RATIO_DEF),
> > +             [9] = CH_DATA_INIT(SCALE_10, 0, 0, RATIO_DEF),
> > +             [10] = CH_DATA_INIT(SCALE_11, 0, 0, RATIO_DEF),
> > +             [11] = CH_DATA_INIT(SCALE_00, 0, 0, RATIO_DEF),
> > +             [13] = CH_DATA_INIT(SCALE_01, 0, 0, RATIO_DEF),
> > +             [14] = CH_DATA_INIT(SCALE_00, 0, 0, RATIO(68, 900)),
> > +             [15] = CH_DATA_INIT(SCALE_00, 0, 0, RATIO(1, 3)),
> > +             [19] = CH_DATA_INIT(SCALE_11, 0, 0, RATIO_DEF),
> > +             [21] = CH_DATA_INIT(SCALE_00, 0, 0, RATIO(3, 8)),
> > +             [22] = CH_DATA_INIT(SCALE_00, 0, 0, RATIO(3, 8)),
> > +             [23] = CH_DATA_INIT(SCALE_00, 0, 0, RATIO(3, 8)),
> > +             [30] = CH_DATA_INIT(SCALE_11, 0, 0, RATIO_DEF),
> > +             [31] = CH_DATA_INIT(SCALE_11, 0, 0, RATIO_DEF),
> > +     };
> > +
> > +     sprd_adc_ch_data_merge(data, ch_data, &ch_data_def);
> > +}
> > +
>
> > +
> > +static int sprd_adc_isen_enable(struct sprd_adc_data *data, int channel)
> > +{
> > +     bool isen_support = data->ch_data[channel].isen_info & 0x1;
>
> As below.
>
> > +
> > +     if (!isen_support)
> > +             return 0;
> > +
> > +     sprd_adc_regs_set_order(data, channel, REG_ISEN_ST, REG_ISEN_END);
> > +     usleep_range(500, 1000);
> > +
> > +     return 0;
> > +}
> > +
> > +static int sprd_adc_isen_disable(struct sprd_adc_data *data, int channel)
> > +{
> > +     bool isen_support = data->ch_data[channel].isen_info & 0x1;
>
> FIELD_GET()
>
> > +
> > +     if (!isen_support)
> as isen_support not otherwise used.
>         if (!FIELD_GET(...., ....))
>
> > +             return 0;
> > +
> > +     sprd_adc_regs_clr_order_reverse(data, channel, REG_ISEN_ST, REG_ISEN_END);
> > +     usleep_range(500, 1000);
> > +
> > +     return 0;
> > +}
> > +
>
> > +
> > +static int sprd_adc_enable(struct sprd_adc_data *data, int channel)
> > +{
> > +     int ret = 0;
> > +     u32 reg_read = 0;
> > +
> > +     if (data->pm_data.clk_regmap) {
> > +             ret = regmap_update_bits(data->pm_data.clk_regmap, data->pm_data.clk_reg,
> > +                                      data->pm_data.clk_reg_mask,
> > +                                      data->pm_data.clk_reg_mask);
> > +             ret |= regmap_read(data->pm_data.clk_regmap, data->pm_data.clk_reg, &reg_read);
> > +             if (ret) {
> > +                     dev_err(data->dev, "failed to enable clk26m, channel %d\n", channel);
> > +                     return ret;
> > +             }
> > +             dev_dbg(data->dev, "enable clk26m: ch %d, reg_read 0x%x\n", channel, reg_read);
>
> Directly accessing the regmap of a clock seems unusual. Why not provide generic clock interfaces
> for this?

This register is used to vote to enable/disable the pmic 26m clk which
is provided to modules like audio, typec and adc.
Therefore, this clk cannot be disabled or enabled directly.

> > +     }
> > +
> > +     ret = sprd_adc_isen_enable(data, channel);
> > +     if (ret) {
> > +             dev_err(data->dev, "failed to enable isen\n");
> > +             return ret;
> > +     }
> > +
> > +     ret = regmap_update_bits(data->regmap, data->base + SPRD_ADC_CTL,
> > +                              SPRD_ADC_EN, SPRD_ADC_EN);
> > +     if (ret) {
> > +             dev_err(data->dev, "failed to set SPRD_ADC_EN\n");
> > +             return ret;
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> > +static int sprd_adc_disable(struct sprd_adc_data *data, int channel)
> > +{
> > +     int ret = 0;
>
> Set just below here. Check for other instances of this.
>
> > +     u32 reg_read = 0;
> > +
> > +     ret = regmap_update_bits(data->regmap, data->base + SPRD_ADC_CTL, SPRD_ADC_EN, 0);
> > +     if (ret) {
> > +             dev_err(data->dev, "failed to reset SPRD_ADC_EN\n");
> > +             return ret;
> > +     }
> > +
> > +     ret = sprd_adc_isen_disable(data, channel);
> > +     if (ret) {
> > +             dev_err(data->dev, "failed to disable isen\n");
> > +             return ret;
> > +     }
> > +
> > +     if (data->pm_data.clk_regmap) {
> > +             ret = regmap_update_bits(data->pm_data.clk_regmap, data->pm_data.clk_reg,
> > +                                      data->pm_data.clk_reg_mask, 0);
> > +             ret |= regmap_read(data->pm_data.clk_regmap, data->pm_data.clk_reg, &reg_read);
>
> Don't combine ret values. Can result in complete garbage.  Just handle each error
> separately.  It costs a few lines, but always works!
>
> > +             if (ret) {
> > +                     dev_err(data->dev, "failed to disable clk26m, channel %d\n", channel);
> > +                     return ret;
> > +             }
> > +             dev_dbg(data->dev, "disable clk26m: ch %d, reg_read 0x%x\n", channel, reg_read);
> > +     }
> > +
> > +     return ret;
>
> return 0;
> Can't get here with any other value that I can see.
>
> > +}
> > +
> > +static int sprd_adc_read(struct sprd_adc_data *data, int channel, int scale, int *val)
> > +{
> > +     int ret = 0, sample_num_sw;
> > +     u32 rawdata = 0, tmp, status, scale_shift, scale_mask;
> > +     bool filter_sw = data->ch_data[channel].filter_info & 0x1;
> > +     int sample_num_hw = data->ch_data[channel].filter_info >> 1;
>
> FIELD_GET() for both of these.
>
> > +
> > +     if (data->pm_data.dev_suspended) {
> > +             dev_err(data->dev, "adc_exp: adc bas been suspended, ignore.\n");
>
> has
>
> > +             return -EBUSY;
> > +     }
> > +
> > +     dev_dbg(data->dev, "ch_data[%d]: scale %d, graph %d, filter_info 0x%x, isen_info 0x%x\n",
> > +             channel, data->ch_data[channel].scale, data->ch_data[channel].graph,
> > +             data->ch_data[channel].filter_info, data->ch_data[channel].isen_info);
> > +
> > +     ret = hwspin_lock_timeout_raw(data->hwlock, SPRD_ADC_HWLOCK_TIMEOUT);
> > +     if (ret) {
> > +             dev_err(data->dev, "timeout to get the hwspinlock\n");
>
> timeout getting the
> or
> timeout trying to get
>
> > +             return ret;
> > +     }
> > +
> > +     ret = sprd_adc_enable(data, channel);
> > +     if (ret)
> > +             goto disable_adc;
> > +
> > +     ret = regmap_update_bits(data->regmap, data->base + SPRD_ADC_INT_CLR,
> > +                              SPRD_ADC_IRQ_CLR, SPRD_ADC_IRQ_CLR);
> > +     if (ret)
> > +             goto disable_adc;
> > +
> > +     /* Configure the channel id and scale */
> > +     scale_shift = data->var_data->reg_list[REG_SCALE].offset;
> > +     scale_mask = data->var_data->reg_list[REG_SCALE].mask;
> > +     tmp = (scale << scale_shift) & scale_mask;
> > +     tmp |= channel & SPRD_ADC_CHN_ID_MASK;
> > +     ret = regmap_update_bits(data->regmap, data->base + SPRD_ADC_CH_CFG,
> > +                              SPRD_ADC_CHN_ID_MASK | scale_mask, tmp);
> > +     if (ret)
> > +             goto disable_adc;
> > +
> > +     /* Select 12bit conversion mode, and only sample 1 time */
> > +     tmp = SPRD_ADC_12BIT_MODE;
> > +     sample_num_sw = (filter_sw ? ADC_MESURE_NUMBER_SW - 1 : 0);
> > +     sample_num_hw = ((sample_num_hw > 0) ? sample_num_hw : ADC_MESURE_NUMBER_HW_DEF);
> > +     tmp |= ((unsigned long)sample_num_sw << SPRD_ADC_RUN_NUM_SHIFT) & SPRD_ADC_RUN_NUM_MASK;
> > +     tmp |= ((unsigned long)sample_num_hw << SPRD_ADC_AVERAGE_SHIFT) & SPRD_ADC_AVERAGE_MASK;
> > +     ret = regmap_update_bits(data->regmap, data->base + SPRD_ADC_CTL,
> > +                              SPRD_ADC_RUN_NUM_MASK | SPRD_ADC_12BIT_MODE |
> > +                              SPRD_ADC_AVERAGE_MASK, tmp);
> > +     if (ret)
> > +             goto disable_adc;
> > +
> > +     ret = regmap_update_bits(data->regmap, data->base + SPRD_ADC_CTL,
> > +                              SPRD_ADC_CHN_RUN, SPRD_ADC_CHN_RUN);
> > +     if (ret)
> > +             goto disable_adc;
> > +
> > +     ret = regmap_read_poll_timeout(data->regmap, data->base + SPRD_ADC_INT_RAW, status,
> > +                                    (status & SPRD_ADC_IRQ_RAW), SPRD_ADC_POLL_RAW_STATUS,
> > +                                    SPRD_ADC_RDY_TIMEOUT);
> > +     if (ret) {
> > +             dev_err(data->dev, "read adc timeout 0x%x\n", status);
> > +             sprd_adc_regs_dump(data, channel, scale, "t_bef");
> > +             sprd_adc_hw_enable(data);
> > +             sprd_adc_soft_rst(data);
> > +             sprd_adc_regs_dump(data, channel, scale, "t_aft");
> > +             goto disable_adc;
> > +     }
> > +
> > +     if (filter_sw) {
> > +             rawdata = sprd_adc_get_val_with_sw_filter(data, channel);
> > +     } else {
> > +             ret = regmap_read(data->regmap, data->base + SPRD_ADC_DATA, &rawdata);
> check ret.
> > +             rawdata &= SPRD_ADC_DATA_MASK;
> > +     }
> > +disable_adc:
> > +     ret = sprd_adc_disable(data, channel);
> > +
> > +     hwspin_unlock_raw(data->hwlock);
> > +
> > +     if (!ret)
> > +             *val = rawdata;
> I think val is valid even if the disable fails. So do it up a few lines unconditionally.
>
> > +
> > +     return ret;
> > +}
> > +
> > +static int sprd_adc_calculate_volt_by_graph(struct sprd_adc_data *data, int channel,
> > +                                         int scale, int raw_adc)
> > +{
> > +     int tmp;
> > +     struct sprd_adc_graphs *graphs = &sprd_adc_graphs_array[data->var_data->graph_index];
> > +     int graph_offset = data->ch_data[channel].graph;
> > +     struct sprd_adc_linear_graph *linear_graph = &graphs->adc_graphs[graph_offset];
> > +
> > +     tmp = (linear_graph->volt0 - linear_graph->volt1) * (raw_adc - linear_graph->adc1);
> > +     tmp /= (linear_graph->adc0 - linear_graph->adc1);
> > +     tmp += linear_graph->volt1;
> > +     tmp = (tmp < 0 ? 0 : tmp);
>
>         tmp = max(0, tmp) perhaps makes it clearer this is clamping a small negative (I think)
>
> > +
> > +     dev_dbg(data->dev, "by_graph_c%d: v0 %d a0 %d, v1 %d a1 %d, raw_adc 0x%x, vol_graph %d\n",
> > +             channel, linear_graph->volt0, linear_graph->adc0, linear_graph->volt1,
> > +             linear_graph->adc1, raw_adc, tmp);
> > +
> > +     return tmp;
> > +}
> > +
> > +static int sprd_adc_calculate_volt_by_ratio(struct sprd_adc_data *data, int channel,
> > +                                         int scale, int vol_graph)
> > +{
> > +     struct u32_fract ip_fract, aux_fract;
> > +     int pmic_type = data->var_data->pmic_type;
> > +     u32 vol_final = vol_graph;
> > +
> > +     ratio_to_u32_fract(data->ch_data[channel].ip_ratio, &ip_fract);
> > +     vol_final = DIV_ROUND_CLOSEST(vol_final * ip_fract.denominator, ip_fract.numerator);
> > +
> > +     ratio_to_u32_fract(data->ch_data[channel].ch_aux_ratio[scale], &aux_fract);
> > +     vol_final = DIV_ROUND_CLOSEST(vol_final * aux_fract.denominator, aux_fract.numerator);
> > +
> > +     dev_dbg(data->dev, "by_ratio_c%d: id %d, scale %d, ip_r[%d/%d], aux_r[%d/%d], vol_f %d\n",
> > +             channel, pmic_type, scale, ip_fract.numerator, ip_fract.denominator,
> > +             aux_fract.numerator, aux_fract.denominator, vol_final);
> > +
> > +     return vol_final;
> > +}
> > +
> > +static int sprd_adc_read_processed(struct sprd_adc_data *data, int channel, int scale, int *val)
> > +{
> > +     int ret, raw_adc, vol_graph;
> > +
> > +     ret = sprd_adc_read(data, channel, scale, &raw_adc);
> > +
> > +     if (ret)
> > +             return ret;
> > +
> > +     vol_graph = sprd_adc_calculate_volt_by_graph(data, channel, scale, raw_adc);
> > +     *val = sprd_adc_calculate_volt_by_ratio(data, channel, scale, vol_graph);
> > +
> > +     return 0;
> > +}
> > +
> > +static int sprd_adc_ch_data_encode(struct sprd_adc_data *data, int ch)
> > +{
> > +     int scale = data->ch_data[ch].scale & 0xff;
> > +     int isen_info = data->ch_data[ch].isen_info & 0xff;
> > +     int filter_info = data->ch_data[ch].filter_info & 0xff;
> > +
> > +     return (scale | (filter_info << 8) | (isen_info << 16));
>
> As below - looks like an array of u8. Treat it as such or use FIELD_PREP()
> and treat it as a u32
>
>
> > +}
> > +
> > +static void sprd_adc_ch_data_decode(struct sprd_adc_data *data, int ch, int val)
> > +{
> > +     int scale_override, filter_override, isen_override;
> > +
> > +     scale_override = (val & 0xff);
> > +     filter_override = ((val >> 8) & 0xff);
> > +     isen_override = ((val >> 16) & 0xff);
>
> FIELD_GET() with appropriate mask defines.
> Or just convert it to an array of u8 with appropriate endian conversion.
>
>
> > +
> > +     if (scale_override < SPRD_ADC_SCALE_MAX && scale_override != data->ch_data[ch].scale)
> > +             data->ch_data[ch].scale = scale_override;
> > +
> > +     if (data->ch_data[ch].filter_info != filter_override)
> > +             data->ch_data[ch].filter_info = filter_override;
> > +
> > +     if (data->ch_data[ch].isen_info != isen_override)
> > +             data->ch_data[ch].isen_info = isen_override;
> > +}
> > +
>
> ...
>
> > +static int sprd_adc_hw_enable(struct sprd_adc_data *data)
> > +{
> > +     int ret;
> > +     u32 reg_addr, mask;
> > +
> > +     reg_addr = GET_REG_ADDR(data, REG_XTL_CTRL);
>
> I'm not sure the local variables much help readability.
> So I'd just put them inline instead perhaps providing a
> local variabel for the reglist.
>         u32 *reg_list = data->var_data->reg_list;
>
>         ret = regmap_update_bits(data->regmap,
>                                  GET_REG_ADDR(data, REG_XTL_CTRL),
>                                  reg_list[REG_XTL_CTRL]);
>
> etc.  May make sense for other similar cases.
>
>
> > +     mask = data->var_data->reg_list[REG_XTL_CTRL].mask;
> > +     ret = regmap_update_bits(data->regmap, reg_addr, mask, mask);
> > +     if (ret)
> > +             return ret;
> > +
> > +     reg_addr = GET_REG_ADDR(data, REG_MODULE_EN);
> > +     mask = data->var_data->reg_list[REG_MODULE_EN].mask;
> > +     ret = regmap_update_bits(data->regmap, reg_addr, mask, mask);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Enable ADC work clock */
> > +     reg_addr = GET_REG_ADDR(data, REG_CLK_EN);
> > +     mask = data->var_data->reg_list[REG_CLK_EN].mask;
> > +     ret = regmap_update_bits(data->regmap, reg_addr, mask, mask);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return 0;
>
>         return regmap_update_bits()
>
> > +}
> > +
>
> > +
> > +static int sprd_adc_ch_data_init(struct sprd_adc_data *data)
> > +{
> > +     struct device_node *np = data->dev->of_node;
> > +     int size, ret, ch, ch_data_val, i;
> > +     u32 *ch_data_override;
> > +
> > +     data->var_data->ch_data_init(data);
> > +
> > +     size = of_property_count_elems_of_size(np, "ch_data_override", sizeof(u32));
> > +     if (size <= 0)
> > +             return 0;
> > +
> > +     if (size % 2) {
> > +             dev_err(data->dev, "Pair of ch data err!\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     ch_data_override = kcalloc(size, sizeof(u32), GFP_KERNEL);
> > +     if (!ch_data_override)
> > +             return -ENOMEM;
> > +
> > +     ret = of_property_read_u32_array(np, "ch_data_override", ch_data_override, size);
> > +     if (ret < 0) {
> > +             dev_err(data->dev, "Failed to read ch data from dt: %d\n", ret);
> > +             kfree(ch_data_override);
>
> Probably better to use a goto for this cleanup so it can be shared with the normal
> exit path. Of if you want to have fun (and don't need to backport this),
> try the new __free() infrastructure to do scope based cleanup found in
>
> linux/cleanup.h
>
> This sort of pattern is exactly what that code is for.
>
>
> > +             return ret;
> > +     }
> > +
> > +     for (i = 0; i < size; i += 2) {
> > +             ch = ch_data_override[i];
> > +             ch_data_val = ch_data_override[i+1];
> kernel style always has spaces around an operator with two parameters.
> [i + 1]
> > +             sprd_adc_ch_data_decode(data, ch, ch_data_val);
> > +             sprd_adc_ch_data_show(data, ch);
> > +     }
> > +
> > +     kfree(ch_data_override);
> > +
> > +     return 0;
> > +}
> > +
>
> > +
> > +static int sprd_adc_probe(struct platform_device *pdev)
> > +{
> > +     struct device_node *np = pdev->dev.of_node;
> > +     struct sprd_adc_data *sprd_data;
> > +     const struct sprd_adc_variant_data *pdata;
> > +     struct iio_dev *indio_dev;
> > +     int ret;
> > +
> > +     pdata = of_device_get_match_data(&pdev->dev);
>
> device_get_match_data()
>
>
> > +     if (!pdata) {
> > +             dev_err(&pdev->dev, "No matching driver data found\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*sprd_data));
> > +     if (!indio_dev)
> > +             return -ENOMEM;
> > +
> > +     sprd_data = iio_priv(indio_dev);
> > +
> > +     sprd_data->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> > +     if (!sprd_data->regmap) {
> > +             dev_err(&pdev->dev, "failed to get ADC regmap\n");
> > +             return -ENODEV;
> > +     }
> > +
> > +     ret = of_property_read_u32(np, "reg", &sprd_data->base);
>
> Even though some elements of this (of_hwspin...) don't have generic firmware
> interfaces, I would prefer to see those from linux/property.h used
> wherever possible.  It will take us a long time to make that a subsystem
> wide change, but good not to have more unnecessary instances of device tree
> specific property reading.

Sorry, I don't understand what needs to be modified. Can you provide
more information or give an example?
Do you mean that the "reg"  property reading is unnecessary?

> > +     if (ret) {
> > +             dev_err(&pdev->dev, "failed to get ADC base address\n");
> > +             return ret;
> > +     }
> > +
> > +     sprd_data->irq = platform_get_irq(pdev, 0);
> > +     if (sprd_data->irq < 0) {
> > +             dev_err(&pdev->dev, "failed to get ADC irq number\n");
> > +             return sprd_data->irq;
> > +     }
> > +
> > +     ret = of_hwspin_lock_get_id(np, 0);
> > +     if (ret < 0) {
> > +             dev_err(&pdev->dev, "failed to get hwspinlock id\n");
> > +             return ret;
> > +     }
> > +
> > +     mutex_init(&sprd_data->mutex);
> > +     sprd_data->hwlock = hwspin_lock_request_specific(ret);
> > +     if (!sprd_data->hwlock) {
> > +             dev_err(&pdev->dev, "failed to request hwspinlock\n");
> > +             return -ENXIO;
> > +     }
> > +
> > +     ret = devm_add_action(&pdev->dev, sprd_adc_free_lock, sprd_data);
> > +     if (ret) {
> > +             sprd_adc_free_lock(sprd_data);
>
> devm_add_action_or_reset() as below.  Same for any more cases of this, I won't
> mention it again (not I review up the driver as they make more logical sense
> to me that way around ;)
>
> > +             dev_err(&pdev->dev, "failed to add free lock action\n");
> > +             return ret;
> > +     }
> > +
> > +     sprd_data->dev = &pdev->dev;
> > +     sprd_data->var_data = pdata;
> > +     sprd_data->indio_dev = indio_dev;
> > +
> > +     /* ADC channel scales calibration from nvmem device */
> > +     ret = sprd_adc_graphs_calibrate(sprd_data);
> > +     if (ret) {
> > +             dev_err(&pdev->dev, "failed to calib graphs from nvmem\n");
> > +             return ret;
> > +     }
> > +
> > +     ret = sprd_adc_pm_init(sprd_data);
> > +     if (ret) {
> > +             dev_err(&pdev->dev, "adc pm init err.\n");
> > +             return ret;
> > +     }
> > +
> > +     ret = sprd_adc_ch_data_init(sprd_data);
> > +     if (ret) {
> > +             dev_err(&pdev->dev, "ch data init err.\n");
> > +             return ret;
> > +     }
> > +
> > +     ret = sprd_adc_hw_enable(sprd_data);
> > +     if (ret) {
> > +             dev_err(&pdev->dev, "failed to enable ADC module\n");
> > +             return ret;
> > +     }
> > +
> > +     ret = devm_add_action(&pdev->dev, sprd_adc_hw_disable, sprd_data);
>
> ret = devm_add_action_or_reset() and...
>
> > +     if (ret) {
> > +             sprd_adc_hw_disable(sprd_data);
>
> drop this.
>
> This is such a common case that we have the or_reset() form for it.
>
> > +             dev_err(&pdev->dev, "failed to add ADC disable action\n");
> > +             return ret;
>
> return dev_err_probe(&pdev->dev, ret, "...");
> Same for all similar error handling in probe, or functions that are only
> called from probe.
>
> > +     }
> > +
> > +     ret = sprd_adc_soft_rst(sprd_data);
> > +     if (ret) {
> > +             dev_err(&pdev->dev, "adc soft rst failed\n");
> > +             return ret;
>                 return dev_err_probe(...);
>
> > +     }
> > +
> > +     indio_dev->dev.parent = &pdev->dev;
> > +     indio_dev->name = dev_name(&pdev->dev);
> > +     indio_dev->modes = INDIO_DIRECT_MODE;
> > +     indio_dev->info = &sprd_info;
> > +     indio_dev->channels = sprd_channels;
> > +     indio_dev->num_channels = ARRAY_SIZE(sprd_channels);
> > +     ret = devm_iio_device_register(&pdev->dev, indio_dev);
> > +     if (ret)
> > +             dev_err(&pdev->dev, "could not register iio (ADC)");
>
> Prefer return dev_err_probe() in all cases.
> Also should not carry on to set drvdata if this failed as we should never
> need it.
>
> > +
> > +     platform_set_drvdata(pdev, indio_dev);
> > +
> > +     return ret;
> > +}
>
> > +
> > +static const struct sprd_adc_variant_data ump9620_data = {
> > +     .pmic_type = UMP9620_ADC,
> > +     .glb_reg_base = 0x2000,
> > +     .adc_reg_base_offset = 0x4,
> > +     .reg_list = regs_ump9620,
> > +     .graph_index = TWO_CELL_GRAPH,
> > +     .calib_infos = {
> > +             CALIB_INFO_INIT(11, SCALE_00, GRAPH_BIG),
> > +             CALIB_INFO_INIT(1, SCALE_00, GRAPH_SMALL),
> > +             CALIB_INFO_INIT(0, SCALE_01, GRAPH_VBAT_DET)
> > +     },
> > +     .aux_ratio_comm = {RATIO_DEF, RATIO(100, 133), RATIO(1000, 2600), RATIO(1000, 4060)},
> > +     .ch_data_init = ump9620_ch_data_init,
> > +};
> > +
> > +static const struct sprd_adc_variant_data ump518_data = {
> > +     .pmic_type = UMP518_ADC,
> > +     .glb_reg_base = 0x1800,
> > +     .adc_reg_base_offset = 0x4,
> > +     .reg_list = regs_ump9620,
> > +     .graph_index = TWO_CELL_GRAPH,
> > +     .calib_infos = {
> > +             CALIB_INFO_INIT(11, SCALE_00, GRAPH_BIG),
> > +             CALIB_INFO_INIT(1, SCALE_00, GRAPH_SMALL),
> > +             CALIB_INFO_INIT(0, SCALE_01, GRAPH_VBAT_DET)
> > +     },
> > +     .aux_ratio_comm = {RATIO_DEF, RATIO(100, 133), RATIO(1000, 2600), RATIO(1000, 4060)},
> > +     .ch_data_init = ump9620_ch_data_init,
> > +};
> > +
> > +static const struct of_device_id sprd_adc_of_match[] = {
> > +     { .compatible = "sprd,ump9620-adc", .data = &ump9620_data},
> > +     { .compatible = "sprd,ump518-adc", .data = &ump518_data},
>
> Space before } for consistency with the opening bracket.
>
> Numerical order preferred as for the bindings.
>
> > +     { }
> > +};
> > +
> > +static const struct dev_pm_ops sprd_adc_pm_ops = {
> > +     .suspend_noirq = sprd_adc_pm_suspend,
> > +     .resume_noirq = sprd_adc_pm_resume,
> > +};
> > +
> > +static struct platform_driver sprd_adc_driver = {
> > +     .probe = sprd_adc_probe,
> > +     .driver = {
> > +             .name = "sprd-adc",
> > +             .of_match_table = sprd_adc_of_match,
> > +             .pm     = &sprd_adc_pm_ops,
>
> Just space before = not a tab.
>
> Also what stops this building without pm_sleep support?
> You probably should either map to one of the macros that set the pm
> ops up or add pm_sleep_ptr() protection to allow the sprd_adc_pm_ops
> structure to be removed if appropriate.
>
> > +     },
> > +};
> > +
> > +module_platform_driver(sprd_adc_driver);
> > +MODULE_DESCRIPTION("SPRD PMIC ADC Driver");
> > +MODULE_LICENSE("GPL");
>

Re: [PATCH V0 2/2] iio: adc: sprd_pmic_adc: Add support for UMP serise pmic adc

[Jonathan Cameron]

On Wed, 30 Aug 2023 15:15:12 +0800
杨明金 <magicyangmingjin@gmail.com> wrote:

> Jonathan Cameron <jic23@kernel.org> 于2023年8月28日周一 23:56写道：
> >
Hi,

Please crop replies to relevant part only.  Hopefully I found it!


> > > +static int sprd_adc_enable(struct sprd_adc_data *data, int channel)
> > > +{
> > > +     int ret = 0;
> > > +     u32 reg_read = 0;
> > > +
> > > +     if (data->pm_data.clk_regmap) {
> > > +             ret = regmap_update_bits(data->pm_data.clk_regmap, data->pm_data.clk_reg,
> > > +                                      data->pm_data.clk_reg_mask,
> > > +                                      data->pm_data.clk_reg_mask);
> > > +             ret |= regmap_read(data->pm_data.clk_regmap, data->pm_data.clk_reg, &reg_read);
> > > +             if (ret) {
> > > +                     dev_err(data->dev, "failed to enable clk26m, channel %d\n", channel);
> > > +                     return ret;
> > > +             }
> > > +             dev_dbg(data->dev, "enable clk26m: ch %d, reg_read 0x%x\n", channel, reg_read);  
> >
> > Directly accessing the regmap of a clock seems unusual. Why not provide generic clock interfaces
> > for this?  
> 
> This register is used to vote to enable/disable the pmic 26m clk which
> is provided to modules like audio, typec and adc.
> Therefore, this clk cannot be disabled or enabled directly.

clk_enable() and friends support reference counted enable and disable
so I don't understand why this needs something unusual.


> 

> > > +static int sprd_adc_probe(struct platform_device *pdev)
> > > +{
> > > +     struct device_node *np = pdev->dev.of_node;
> > > +     struct sprd_adc_data *sprd_data;
> > > +     const struct sprd_adc_variant_data *pdata;
> > > +     struct iio_dev *indio_dev;
> > > +     int ret;
> > > +
> > > +     pdata = of_device_get_match_data(&pdev->dev);  
> >
> > device_get_match_data()
> >
> >  
> > > +     if (!pdata) {
> > > +             dev_err(&pdev->dev, "No matching driver data found\n");
> > > +             return -EINVAL;
> > > +     }
> > > +
> > > +     indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*sprd_data));
> > > +     if (!indio_dev)
> > > +             return -ENOMEM;
> > > +
> > > +     sprd_data = iio_priv(indio_dev);
> > > +
> > > +     sprd_data->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> > > +     if (!sprd_data->regmap) {
> > > +             dev_err(&pdev->dev, "failed to get ADC regmap\n");
> > > +             return -ENODEV;
> > > +     }
> > > +
> > > +     ret = of_property_read_u32(np, "reg", &sprd_data->base);  
> >
> > Even though some elements of this (of_hwspin...) don't have generic firmware
> > interfaces, I would prefer to see those from linux/property.h used
> > wherever possible.  It will take us a long time to make that a subsystem
> > wide change, but good not to have more unnecessary instances of device tree
> > specific property reading.  
> 
> Sorry, I don't understand what needs to be modified. Can you provide
> more information or give an example?
> Do you mean that the "reg"  property reading is unnecessary?

No.  Where possibly use
	device_property_read_u32(dev, "reg".. etc
and similar functions from
include/linux/property.h rather than device tree specific ones.
The generic property handling deals with various different types of firmware
without needing drivers to be aware of it.

Some elements that you need here do not have generic property handling so
for those you will need to continue using the of_ variants.
Note that this is to support long term move of everything to the generic
firmware framework.  Even if we drivers in IIO etc that are really device
tree only there are benefits for maintenance in using one framework
for all drivers. As some IIO drivers do support other firmware types
(ACPI for example) the generic version is the preferred choice.

Thanks,

Jonathan

Re: [PATCH V0 2/2] iio: adc: sprd_pmic_adc: Add support for UMP serise pmic adc

[杨明金]

> > > > +static int sprd_adc_enable(struct sprd_adc_data *data, int channel)
> > > > +{
> > > > +     int ret = 0;
> > > > +     u32 reg_read = 0;
> > > > +
> > > > +     if (data->pm_data.clk_regmap) {
> > > > +             ret = regmap_update_bits(data->pm_data.clk_regmap, data->pm_data.clk_reg,
> > > > +                                      data->pm_data.clk_reg_mask,
> > > > +                                      data->pm_data.clk_reg_mask);
> > > > +             ret |= regmap_read(data->pm_data.clk_regmap, data->pm_data.clk_reg, &reg_read);
> > > > +             if (ret) {
> > > > +                     dev_err(data->dev, "failed to enable clk26m, channel %d\n", channel);
> > > > +                     return ret;
> > > > +             }
> > > > +             dev_dbg(data->dev, "enable clk26m: ch %d, reg_read 0x%x\n", channel, reg_read);
> > >
> > > Directly accessing the regmap of a clock seems unusual. Why not provide generic clock interfaces
> > > for this?
> >
> > This register is used to vote to enable/disable the pmic 26m clk which
> > is provided to modules like audio, typec and adc.
> > Therefore, this clk cannot be disabled or enabled directly.
>
> clk_enable() and friends support reference counted enable and disable
> so I don't understand why this needs something unusual.

Through communication with internal clk colleagues,
I learned that this register is not a traditional eb register,
so the current clk driver is not configured to support this.

Jonathan Cameron <jic23@kernel.org> 于2023年9月3日周日 19:14写道：
>
> On Wed, 30 Aug 2023 15:15:12 +0800
> 杨明金 <magicyangmingjin@gmail.com> wrote:
>
> > Jonathan Cameron <jic23@kernel.org> 于2023年8月28日周一 23:56写道：
> > >
> Hi,
>
> Please crop replies to relevant part only.  Hopefully I found it!
>
>
> > > > +static int sprd_adc_enable(struct sprd_adc_data *data, int channel)
> > > > +{
> > > > +     int ret = 0;
> > > > +     u32 reg_read = 0;
> > > > +
> > > > +     if (data->pm_data.clk_regmap) {
> > > > +             ret = regmap_update_bits(data->pm_data.clk_regmap, data->pm_data.clk_reg,
> > > > +                                      data->pm_data.clk_reg_mask,
> > > > +                                      data->pm_data.clk_reg_mask);
> > > > +             ret |= regmap_read(data->pm_data.clk_regmap, data->pm_data.clk_reg, &reg_read);
> > > > +             if (ret) {
> > > > +                     dev_err(data->dev, "failed to enable clk26m, channel %d\n", channel);
> > > > +                     return ret;
> > > > +             }
> > > > +             dev_dbg(data->dev, "enable clk26m: ch %d, reg_read 0x%x\n", channel, reg_read);
> > >
> > > Directly accessing the regmap of a clock seems unusual. Why not provide generic clock interfaces
> > > for this?
> >
> > This register is used to vote to enable/disable the pmic 26m clk which
> > is provided to modules like audio, typec and adc.
> > Therefore, this clk cannot be disabled or enabled directly.
>
> clk_enable() and friends support reference counted enable and disable
> so I don't understand why this needs something unusual.
>
>
> >
>
> > > > +static int sprd_adc_probe(struct platform_device *pdev)
> > > > +{
> > > > +     struct device_node *np = pdev->dev.of_node;
> > > > +     struct sprd_adc_data *sprd_data;
> > > > +     const struct sprd_adc_variant_data *pdata;
> > > > +     struct iio_dev *indio_dev;
> > > > +     int ret;
> > > > +
> > > > +     pdata = of_device_get_match_data(&pdev->dev);
> > >
> > > device_get_match_data()
> > >
> > >
> > > > +     if (!pdata) {
> > > > +             dev_err(&pdev->dev, "No matching driver data found\n");
> > > > +             return -EINVAL;
> > > > +     }
> > > > +
> > > > +     indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*sprd_data));
> > > > +     if (!indio_dev)
> > > > +             return -ENOMEM;
> > > > +
> > > > +     sprd_data = iio_priv(indio_dev);
> > > > +
> > > > +     sprd_data->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> > > > +     if (!sprd_data->regmap) {
> > > > +             dev_err(&pdev->dev, "failed to get ADC regmap\n");
> > > > +             return -ENODEV;
> > > > +     }
> > > > +
> > > > +     ret = of_property_read_u32(np, "reg", &sprd_data->base);
> > >
> > > Even though some elements of this (of_hwspin...) don't have generic firmware
> > > interfaces, I would prefer to see those from linux/property.h used
> > > wherever possible.  It will take us a long time to make that a subsystem
> > > wide change, but good not to have more unnecessary instances of device tree
> > > specific property reading.
> >
> > Sorry, I don't understand what needs to be modified. Can you provide
> > more information or give an example?
> > Do you mean that the "reg"  property reading is unnecessary?
>
> No.  Where possibly use
>         device_property_read_u32(dev, "reg".. etc
> and similar functions from
> include/linux/property.h rather than device tree specific ones.
> The generic property handling deals with various different types of firmware
> without needing drivers to be aware of it.
>
> Some elements that you need here do not have generic property handling so
> for those you will need to continue using the of_ variants.
> Note that this is to support long term move of everything to the generic
> firmware framework.  Even if we drivers in IIO etc that are really device
> tree only there are benefits for maintenance in using one framework
> for all drivers. As some IIO drivers do support other firmware types
> (ACPI for example) the generic version is the preferred choice.
>
> Thanks,
>
> Jonathan

Re: [PATCH V0 2/2] iio: adc: sprd_pmic_adc: Add support for UMP serise pmic adc

[Jonathan Cameron]

On Tue, 5 Sep 2023 10:59:33 +0800
杨明金 <magicyangmingjin@gmail.com> wrote:

> > > > > +static int sprd_adc_enable(struct sprd_adc_data *data, int channel)
> > > > > +{
> > > > > +     int ret = 0;
> > > > > +     u32 reg_read = 0;
> > > > > +
> > > > > +     if (data->pm_data.clk_regmap) {
> > > > > +             ret = regmap_update_bits(data->pm_data.clk_regmap, data->pm_data.clk_reg,
> > > > > +                                      data->pm_data.clk_reg_mask,
> > > > > +                                      data->pm_data.clk_reg_mask);
> > > > > +             ret |= regmap_read(data->pm_data.clk_regmap, data->pm_data.clk_reg, &reg_read);
> > > > > +             if (ret) {
> > > > > +                     dev_err(data->dev, "failed to enable clk26m, channel %d\n", channel);
> > > > > +                     return ret;
> > > > > +             }
> > > > > +             dev_dbg(data->dev, "enable clk26m: ch %d, reg_read 0x%x\n", channel, reg_read);  
> > > >
> > > > Directly accessing the regmap of a clock seems unusual. Why not provide generic clock interfaces
> > > > for this?  
> > >
> > > This register is used to vote to enable/disable the pmic 26m clk which
> > > is provided to modules like audio, typec and adc.
> > > Therefore, this clk cannot be disabled or enabled directly.  
> >
> > clk_enable() and friends support reference counted enable and disable
> > so I don't understand why this needs something unusual.  
> 
> Through communication with internal clk colleagues,
> I learned that this register is not a traditional eb register,

eb?

Sorry, I'm not familiar with the term.

> so the current clk driver is not configured to support this.
> 
> Jonathan Cameron <jic23@kernel.org> 于2023年9月3日周日 19:14写道：
> >
> > On Wed, 30 Aug 2023 15:15:12 +0800
> > 杨明金 <magicyangmingjin@gmail.com> wrote:
> >