Message-ID: <CAF12kFsWVQin76KvWnfRszo+w033yjoL5DKc7ttXvywBcZPh5A@mail.gmail.com>
Date:   Sat, 2 Apr 2022 18:55:21 +0800
From:   Cixi Geng <gengcixi@...il.com>
To:     Jonathan Cameron <jic23@...nel.org>
Cc:     Lars-Peter Clausen <lars@...afoo.de>,
        Rob Herring <robh+dt@...nel.org>,
        Orson Zhai <orsonzhai@...il.com>,
        "baolin.wang7@...il.com" <baolin.wang7@...il.com>,
        Chunyan Zhang <zhang.lyra@...il.com>, lgirdwood@...il.com,
        Mark Brown <broonie@...nel.org>,
        朱玉明 (Yuming Zhu/11457) 
        <yuming.zhu1@...soc.com>, linux-iio@...r.kernel.org,
        Devicetree List <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V2 6/7] iio: adc: sc27xx: add support for PMIC ump9620

Jonathan Cameron <jic23@...nel.org> 于2022年3月20日周日 22:34写道：
>
> On Sat, 12 Mar 2022 00:46:27 +0800
> Cixi Geng <gengcixi@...il.com> wrote:
>
> > From: Cixi Geng <cixi.geng1@...soc.com>
> >
> > The ump9620 is variant from sc27xx chip, add it in here.
> >
> > Signed-off-by: Yuming Zhu <yuming.zhu1@...soc.com>
> > Signed-off-by: Cixi Geng <cixi.geng1@...soc.com>
> Hi,
>
> Same comments as made earlier apply here.
> * Should there be a co-developed-by marking here or should
> the from reflect Yuming Zhu as the patch author with you in
> the role of being on the path to upstream?
> * change log below the ---
> * No blank lines or non tag lines in the tag block - use
> comments at the end of the lines if you want to give
> reported-by for particularly things.
>
> Also, I'm not immediately spotting where Baolin gave
> a tag.
>
> Comments inline.
>
> There are a few places in here where you have significant deviation between
> the code that runs for this new device and previously supported parts.
> Doing that via if / else tends not to scale as yet more parts are added in
> future. Perhaps it would be better to move to some device type specific
> callbacks for more of these cases.  This would be similar to the init_scale
> and get_ratio function pointers you already have in the variant data.
>
> Thanks,
>
> Jonathan
>
> >
> > V2 changes:
> > 1. remove duplicated function
> > Reviewed-by: Baolin Wang <baolin.wang7@...il.com>
> >
> > 2. fix the smatch warnings
> > Reported-by: kernel test robot <lkp@...el.com>
> > Reported-by: Dan Carpenter <dan.carpenter@...cle.com>
> > ---
> >  drivers/iio/adc/sc27xx_adc.c | 305 ++++++++++++++++++++++++++++++-----
> >  1 file changed, 266 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c
> > index b89637c051ac..e9b680e9c275 100644
> > --- a/drivers/iio/adc/sc27xx_adc.c
> > +++ b/drivers/iio/adc/sc27xx_adc.c
> > @@ -15,12 +15,16 @@
> >  /* PMIC global registers definition */
> >  #define SC2730_MODULE_EN             0x1808
> >  #define SC2731_MODULE_EN             0xc08
> > +#define UMP9620_MODULE_EN            0x2008
> >  #define SC27XX_MODULE_ADC_EN         BIT(5)
> >  #define SC2721_ARM_CLK_EN            0xc0c
> >  #define SC2730_ARM_CLK_EN            0x180c
> >  #define SC2731_ARM_CLK_EN            0xc10
> > +#define UMP9620_ARM_CLK_EN           0x200c
> > +#define UMP9620_XTL_WAIT_CTRL0               0x2378
> >  #define SC27XX_CLK_ADC_EN            BIT(5)
> >  #define SC27XX_CLK_ADC_CLK_EN                BIT(6)
> > +#define UMP9620_XTL_WAIT_CTRL0_EN    BIT(8)
> >
> >  /* ADC controller registers definition */
> >  #define SC27XX_ADC_CTL                       0x0
> > @@ -82,6 +86,13 @@
> >  enum sc27xx_pmic_type {
> >       SC27XX_ADC,
> >       SC2721_ADC,
> > +     UMP9620_ADC,
> > +};
> > +
> > +enum ump96xx_scale_cal {
> > +     UMP96XX_VBAT_SENSES_CAL,
> > +     UMP96XX_VBAT_DET_CAL,
> > +     UMP96XX_CH1_CAL,
> >  };
> >
> >  struct sc27xx_adc_data {
> > @@ -139,6 +150,11 @@ static struct sc27xx_adc_linear_graph small_scale_graph = {
> >       100, 341,
> >  };
> >
> > +static struct sc27xx_adc_linear_graph ump9620_bat_det_graph = {
> > +     1400, 3482,
> > +     200, 476,
> > +};
> > +
> >  /* Add these for sc2731 pmic, and the [big|small]_scale_graph_calib for common's */
> >  static const struct sc27xx_adc_linear_graph sc2731_big_scale_graph_calib = {
> >       4200, 850,
> > @@ -165,16 +181,41 @@ static int sc27xx_adc_get_calib_data(u32 calib_data, int calib_adc)
> >       return ((calib_data & 0xff) + calib_adc - 128) * 4;
> >  }
> >
> > +/* get the adc nvmem cell calibration data */
> > +static int adc_nvmem_cell_calib_data(struct sc27xx_adc_data *data, const char *cell_name)
> > +{
>
> This looks to be a bit of refactoring that could be sensibly pulled out before this
> patch.
Ok, I will do pull out the refactor part in a single patch
>
> > +     struct nvmem_cell *cell;
> > +     void *buf;
> > +     u32 origin_calib_data = 0;
>
> Why initialise the above?   I don't see it being used
> in any paths where it isn't already initialised.

Hi Jonathan:
It used origin_calib_data in memcpy,and the length of data
may less than sizeof(u32). we expect the other bits keeps 0;
So we initialise 0 in here.
>
> > +     size_t len = 0;
> > +
> > +     if (!data)
> > +             return -EINVAL;
> > +
> > +     cell = nvmem_cell_get(data->dev, cell_name);
> > +     if (IS_ERR(cell))
> > +             return PTR_ERR(cell);
> > +
> > +     buf = nvmem_cell_read(cell, &len);
> > +     if (IS_ERR(buf)) {
> > +             nvmem_cell_put(cell);
> > +             return PTR_ERR(buf);
> > +     }
> > +
> > +     memcpy(&origin_calib_data, buf, min(len, sizeof(u32)));
> > +
> > +     kfree(buf);
> > +     nvmem_cell_put(cell);
> > +     return origin_calib_data;
> > +}
> > +
> >  static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data,
> >                                       bool big_scale)
> >  {
> >       const struct sc27xx_adc_linear_graph *calib_graph;
> >       struct sc27xx_adc_linear_graph *graph;
> > -     struct nvmem_cell *cell;
> >       const char *cell_name;
> >       u32 calib_data = 0;
> > -     void *buf;
> > -     size_t len;
> >
> >       if (big_scale) {
> >               calib_graph = data->var_data->bscale_cal;
> > @@ -186,24 +227,63 @@ static int sc27xx_adc_scale_calibration(struct sc27xx_adc_data *data,
> >               cell_name = "small_scale_calib";
> >       }
> >
> > -     cell = nvmem_cell_get(data->dev, cell_name);
> > -     if (IS_ERR(cell))
> > -             return PTR_ERR(cell);
> > -
> > -     buf = nvmem_cell_read(cell, &len);
> > -     nvmem_cell_put(cell);
> > -
> > -     if (IS_ERR(buf))
> > -             return PTR_ERR(buf);
> > -
> > -     memcpy(&calib_data, buf, min(len, sizeof(u32)));
> > +     calib_data = adc_nvmem_cell_calib_data(data, cell_name);
> >
> >       /* Only need to calibrate the adc values in the linear graph. */
> >       graph->adc0 = sc27xx_adc_get_calib_data(calib_data, calib_graph->adc0);
> >       graph->adc1 = sc27xx_adc_get_calib_data(calib_data >> 8,
> >                                               calib_graph->adc1);
> >
> > -     kfree(buf);
> > +     return 0;
> > +}
> > +
> > +static int ump96xx_adc_scale_cal(struct sc27xx_adc_data *data,
> > +                                     enum ump96xx_scale_cal cal_type)
> > +{
> > +     struct sc27xx_adc_linear_graph *graph = NULL;
>
> Always set below, so don't initialize here. Same for other
> local variables.
here do make no sense, remove the initalise in next version.
>
> > +     const char *cell_name1 = NULL, *cell_name2 = NULL;
> > +     int adc_calib_data1 = 0, adc_calib_data2 = 0;
> > +
> > +     if (!data)
> > +             return -EINVAL;
> > +
> > +     if (cal_type == UMP96XX_VBAT_DET_CAL) {
> > +             graph = &ump9620_bat_det_graph;
> > +             cell_name1 = "vbat_det_cal1";
> > +             cell_name2 = "vbat_det_cal2";
> > +     } else if (cal_type == UMP96XX_VBAT_SENSES_CAL) {
> > +             graph = &big_scale_graph;
> > +             cell_name1 = "big_scale_calib1";
> > +             cell_name2 = "big_scale_calib2";
> > +     } else if (cal_type == UMP96XX_CH1_CAL) {
> > +             graph = &small_scale_graph;
> > +             cell_name1 = "small_scale_calib1";
> > +             cell_name2 = "small_scale_calib2";
> > +     } else {
> > +             graph = &small_scale_graph;
> > +             cell_name1 = "small_scale_calib1";
> > +             cell_name2 = "small_scale_calib2";
> > +     }
> > +
> > +     adc_calib_data1 = adc_nvmem_cell_calib_data(data, cell_name1);
> > +     if (adc_calib_data1 < 0) {
> > +             dev_err(data->dev, "err! %s:%d\n", cell_name1, adc_calib_data1);
> > +             return adc_calib_data1;
> > +     }
> > +
> > +     adc_calib_data2 = adc_nvmem_cell_calib_data(data, cell_name2);
> > +     if (adc_calib_data2 < 0) {
> > +             dev_err(data->dev, "err! %s:%d\n", cell_name2, adc_calib_data2);
> > +             return adc_calib_data2;
> > +     }
> > +
> > +     /*
> > +      *Read the data in the two blocks of efuse and convert them into the
> > +      *calibration value in the ump9620 adc linear graph.
> > +      */
> > +     graph->adc0 = (adc_calib_data1 & 0xfff0) >> 4;
> > +     graph->adc1 = (adc_calib_data2 & 0xfff0) >> 4;
> > +
> >       return 0;
> >  }
> >
> > @@ -394,6 +474,50 @@ static int sc2731_adc_get_ratio(int channel, int scale)
> >       return SC27XX_VOLT_RATIO(1, 1);
> >  }
> >
> > +static int ump9620_adc_get_ratio(int channel, int scale)
> > +{
> > +     switch (channel) {
> > +     case 11:
> > +             return SC27XX_VOLT_RATIO(1, 1);
> > +     case 14:
> > +             switch (scale) {
> > +             case 0:
> > +                     return SC27XX_VOLT_RATIO(68, 900);
> > +             default:
> > +                     return SC27XX_VOLT_RATIO(1, 1);
> > +             }
> > +     case 15:
> > +             switch (scale) {
> > +             case 0:
> > +                     return SC27XX_VOLT_RATIO(1, 3);
> > +             default:
> > +                     return SC27XX_VOLT_RATIO(1, 1);
> > +             }
> > +     case 21:
> > +     case 22:
> > +     case 23:
> > +             switch (scale) {
> > +             case 0:
> > +                     return SC27XX_VOLT_RATIO(3, 8);
> > +             default:
> > +                     return SC27XX_VOLT_RATIO(1, 1);
> > +             }
> > +     default:
> > +             switch (scale) {
> > +             case 0:
> > +                     return SC27XX_VOLT_RATIO(1, 1);
> > +             case 1:
> > +                     return SC27XX_VOLT_RATIO(1000, 1955);
> > +             case 2:
> > +                     return SC27XX_VOLT_RATIO(1000, 2600);
> > +             case 3:
> > +                     return SC27XX_VOLT_RATIO(1000, 4060);
> > +             default:
> > +                     return SC27XX_VOLT_RATIO(1, 1);
> > +             }
> > +     }
> > +}
> > +
> >  /*
> >   * According to the datasheet set specific value on some channel.
> >   */
> > @@ -453,6 +577,22 @@ static void sc2731_adc_scale_init(struct sc27xx_adc_data *data)
> >       }
> >  }
> >
> > +static void ump9620_adc_scale_init(struct sc27xx_adc_data *data)
> > +{
> > +     int i;
> > +
> > +     for (i = 0; i < SC27XX_ADC_CHANNEL_MAX; i++) {
> > +             if (i == 10 || i == 19 || i == 30 || i == 31)
> > +                     data->channel_scale[i] = 3;
> > +             else if (i == 7 || i == 9)
> > +                     data->channel_scale[i] = 2;
> > +             else if (i == 0 || i == 13)
> > +                     data->channel_scale[i] = 1;
> > +             else
> > +                     data->channel_scale[i] = 0;
> > +     }
> > +}
> > +
> >  static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
> >                          int scale, int *val)
> >  {
> > @@ -567,7 +707,7 @@ static void sc27xx_adc_volt_ratio(struct sc27xx_adc_data *data,
> >       *div_denominator = ratio & SC27XX_RATIO_DENOMINATOR_MASK;
> >  }
> >
> > -static int sc27xx_adc_to_volt(struct sc27xx_adc_linear_graph *graph,
> > +static int adc_to_volt(struct sc27xx_adc_linear_graph *graph,
> >                             int raw_adc)
> >  {
> >       int tmp;
> > @@ -576,6 +716,31 @@ static int sc27xx_adc_to_volt(struct sc27xx_adc_linear_graph *graph,
> >       tmp /= (graph->adc0 - graph->adc1);
> >       tmp += graph->volt1;
> >
> > +     return tmp;
> > +}
> > +
> > +static int sc27xx_adc_to_volt(struct sc27xx_adc_linear_graph *graph,
> > +                           int raw_adc)
> > +{
> > +     int tmp;
> > +
> > +     tmp = adc_to_volt(graph, raw_adc);
> > +
> > +     return tmp < 0 ? 0 : tmp;
> > +}
> > +
> > +static int ump96xx_adc_to_volt(struct sc27xx_adc_linear_graph *graph, int scale,
> > +                           int raw_adc)
> > +{
> > +     int tmp;
> > +
> > +     tmp = adc_to_volt(graph, raw_adc);
> > +
> > +     if (scale == 2)
> > +             tmp = tmp * 2600 / 1000;
> > +     else if (scale == 3)
> > +             tmp = tmp * 4060 / 1000;
> > +
> >       return tmp < 0 ? 0 : tmp;
> >  }
> >
> > @@ -585,23 +750,46 @@ static int sc27xx_adc_convert_volt(struct sc27xx_adc_data *data, int channel,
> >       u32 numerator, denominator;
> >       u32 volt;
> >
> > -     /*
> > -      * Convert ADC values to voltage values according to the linear graph,
> > -      * and channel 5 and channel 1 has been calibrated, so we can just
> > -      * return the voltage values calculated by the linear graph. But other
> > -      * channels need be calculated to the real voltage values with the
> > -      * voltage ratio.
> > -      */
> > -     switch (channel) {
> > -     case 5:
> > -             return sc27xx_adc_to_volt(&big_scale_graph, raw_adc);
> > +     if (data->var_data->pmic_type == UMP9620_ADC) {
> > +             switch (channel) {
> > +             case 0:
> > +                     if (scale == 1)
> > +                             volt = sc27xx_adc_to_volt(&ump9620_bat_det_graph, raw_adc);
> > +                     else
> > +                             volt = ump96xx_adc_to_volt(&small_scale_graph, scale, raw_adc);
> > +                     break;
> > +             case 11:
> > +                     volt = sc27xx_adc_to_volt(&big_scale_graph, raw_adc);
> > +                     break;
> > +             default:
> > +                     if (scale == 1)
> > +                             volt = sc27xx_adc_to_volt(&ump9620_bat_det_graph, raw_adc);
> > +                     else
> > +                             volt = ump96xx_adc_to_volt(&small_scale_graph, scale, raw_adc);
> > +                     break;
> > +             }
> >
> > -     case 1:
> > -             return sc27xx_adc_to_volt(&small_scale_graph, raw_adc);
> > +             if (channel == 0 && scale == 1)
> > +                     return volt;
> > +     } else {
> > +             /*
> > +              * Convert ADC values to voltage values according to the linear graph,
> > +              * and channel 5 and channel 1 has been calibrated, so we can just
> > +              * return the voltage values calculated by the linear graph. But other
> > +              * channels need be calculated to the real voltage values with the
> > +              * voltage ratio.
> > +              */
> > +             switch (channel) {
> > +             case 5:
> > +                     return sc27xx_adc_to_volt(&big_scale_graph, raw_adc);
> >
> > -     default:
> > -             volt = sc27xx_adc_to_volt(&small_scale_graph, raw_adc);
> > -             break;
> > +             case 1:
> > +                     return sc27xx_adc_to_volt(&small_scale_graph, raw_adc);
> > +
> > +             default:
> > +                     volt = sc27xx_adc_to_volt(&small_scale_graph, raw_adc);
> > +                     break;
> > +             }
> >       }
> >
> >       sc27xx_adc_volt_ratio(data, channel, scale, &numerator, &denominator);
> > @@ -619,6 +807,7 @@ static int sc27xx_adc_read_processed(struct sc27xx_adc_data *data,
> >               return ret;
> >
> >       *val = sc27xx_adc_convert_volt(data, channel, scale, raw_adc);
> > +
> >       return 0;
> >  }
> >
> > @@ -736,21 +925,42 @@ static int sc27xx_adc_enable(struct sc27xx_adc_data *data)
> >       if (ret)
> >               return ret;
> >
> > -     /* Enable ADC work clock and controller clock */
> > +     /* Enable 26MHz crvstal oscillator wait cycles for UMP9620 ADC */
> > +     if (data->var_data->pmic_type == UMP9620_ADC) {
> > +             ret = regmap_update_bits(data->regmap, UMP9620_XTL_WAIT_CTRL0,
> > +                                      UMP9620_XTL_WAIT_CTRL0_EN,
> > +                                      UMP9620_XTL_WAIT_CTRL0_EN);
> > +     }
> > +
> > +     /* Enable ADC work clock */
> >       ret = regmap_update_bits(data->regmap, data->var_data->clk_en,
> >                                SC27XX_CLK_ADC_EN | SC27XX_CLK_ADC_CLK_EN,
> >                                SC27XX_CLK_ADC_EN | SC27XX_CLK_ADC_CLK_EN);
> >       if (ret)
> >               goto disable_adc;
> >
> > -     /* ADC channel scales' calibration from nvmem device */
> > -     ret = sc27xx_adc_scale_calibration(data, true);
> > -     if (ret)
> > -             goto disable_clk;
> > +     /* ADC channel scales calibration from nvmem device */
> > +     if (data->var_data->pmic_type == UMP9620_ADC) {
> > +             ret = ump96xx_adc_scale_cal(data, UMP96XX_VBAT_SENSES_CAL);
> > +             if (ret)
> > +                     goto disable_clk;
> >
> > -     ret = sc27xx_adc_scale_calibration(data, false);
> > -     if (ret)
> > -             goto disable_clk;
> > +             ret = ump96xx_adc_scale_cal(data, UMP96XX_VBAT_DET_CAL);
> > +             if (ret)
> > +                     goto disable_clk;
> > +
> > +             ret = ump96xx_adc_scale_cal(data, UMP96XX_CH1_CAL);
> > +             if (ret)
> > +                     goto disable_clk;
> > +     } else {
> > +             ret = sc27xx_adc_scale_calibration(data, true);
> > +             if (ret)
> > +                     goto disable_clk;
> > +
> > +             ret = sc27xx_adc_scale_calibration(data, false);
> > +             if (ret)
> > +                     goto disable_clk;
> > +     }
> >
> >       return 0;
> >
> > @@ -774,6 +984,10 @@ static void sc27xx_adc_disable(void *_data)
> >
> >       regmap_update_bits(data->regmap, data->var_data->module_en,
> >                          SC27XX_MODULE_ADC_EN, 0);
> > +
> > +     if (data->var_data->pmic_type == UMP9620_ADC)
> > +             regmap_update_bits(data->regmap, UMP9620_XTL_WAIT_CTRL0,
> > +                             UMP9620_XTL_WAIT_CTRL0_EN, 0);
> >  }
> >
> >  static const struct sc27xx_adc_variant_data sc2731_data = {
> > @@ -824,6 +1038,18 @@ static const struct sc27xx_adc_variant_data sc2720_data = {
> >       .get_ratio = sc2720_adc_get_ratio,
> >  };
> >
> > +static const struct sc27xx_adc_variant_data ump9620_data = {
> > +     .pmic_type = UMP9620_ADC,
> > +     .module_en = UMP9620_MODULE_EN,
> > +     .clk_en = UMP9620_ARM_CLK_EN,
> > +     .scale_shift = SC27XX_ADC_SCALE_SHIFT,
> > +     .scale_mask = SC27XX_ADC_SCALE_MASK,
> > +     .bscale_cal = &big_scale_graph,
> > +     .sscale_cal = &small_scale_graph,
> > +     .init_scale = ump9620_adc_scale_init,
> > +     .get_ratio = ump9620_adc_get_ratio,
> > +};
> > +
> >  static int sc27xx_adc_probe(struct platform_device *pdev)
> >  {
> >       struct device *dev = &pdev->dev;
> > @@ -917,6 +1143,7 @@ static const struct of_device_id sc27xx_adc_of_match[] = {
> >       { .compatible = "sprd,sc2730-adc", .data = &sc2730_data},
> >       { .compatible = "sprd,sc2721-adc", .data = &sc2721_data},
> >       { .compatible = "sprd,sc2720-adc", .data = &sc2720_data},
> > +     { .compatible = "sprd,ump9620-adc", .data = &ump9620_data},
> >       { }
> >  };
> >  MODULE_DEVICE_TABLE(of, sc27xx_adc_of_match);
>

Powered by blists - more mailing lists