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

[Jonathan Cameron <jic23@jic23.retrosnub.co.uk>]

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

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

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

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

...

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

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

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

...

>