Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220106125947.139523-1-gengcixi@gmail.com> <20220106125947.139523-4-gengcixi@gmail.com>
 <CADBw62pBCdrbRspTV9Yck4DP8DE=ECGmEtD74NOtm1YRT3DM8w@mail.gmail.com>
 <CAF12kFu6O-gfiqp4j24zxC_GqCwJ2Q5KGYYaCtnagmUFB_bsVg@mail.gmail.com>
 <CADBw62rSdWN-L8HbnyMrUNp=x0pDdKR6MyKO4yfu00MnrN4L-g@mail.gmail.com>
 <CAF12kFvUfykKfeRAJACFRk31pmEBQEPw402x0JN4i1uv0EK1zg@mail.gmail.com>
 <CADBw62pmtbzr78c9J20cFbNRuTrntGg_E8TH_g=LciCVGYrYqQ@mail.gmail.com>
 <CAF12kFtV_dpHukd2v0UwSoAFsDbNXZLPSnSSK9dqq7hnoJh9UQ@mail.gmail.com> <20220225101942.00002f43@Huawei.com>
In-Reply-To: <20220225101942.00002f43@Huawei.com>
From:   Cixi Geng <gengcixi@gmail.com>
Date:   Tue, 1 Mar 2022 14:27:50 +0800
Message-ID: <CAF12kFtQH3_EsPBnDgBL59tYZ6wintgt9yceS2tUiB1Pv8qjgQ@mail.gmail.com>
Subject: Re: [PATCH 3/7] iio: adc: sc27xx: structure adjuststment and optimization
To:     Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc:     Baolin Wang <baolin.wang7@gmail.com>,
        Orson Zhai <orsonzhai@gmail.com>,
        Chunyan Zhang <zhang.lyra@gmail.com>, jic23@kernel.org,
        Lars-Peter Clausen <lars@metafoo.de>,
        Rob Herring <robh+dt@kernel.org>, lgirdwood@gmail.com,
        Mark Brown <broonie@kernel.org>,
        =?UTF-8?B?5pyx546J5piOIChZdW1pbmcgWmh1LzExNDU3KQ==?= 
        <yuming.zhu1@unisoc.com>, linux-iio@vger.kernel.org,
        Devicetree List <devicetree@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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