Jonathan Cameron <jic23@xxxxxxxxxx> 于2023年8月28日周一 23:56写道：
>
> On Wed, 16 Aug 2023 16:02:25 +0800
> Mingjin Yang <mingjin.yang@xxxxxxxxxx> 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@xxxxxxxxxx>
>
> 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, ®_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, ®_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");
>
