Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 4 Dec 2018 08:43:42 -0800
From:   Eduardo Valentin <edubezval@gmail.com>
To:     Amit Kucheria <amit.kucheria@linaro.org>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-arm-msm <linux-arm-msm@vger.kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Andy Gross <andy.gross@linaro.org>, vkoul@kernel.org,
        Khasim Syed Mohammed <khasim.mohammed@linaro.org>,
        Zhang Rui <rui.zhang@intel.com>,
        Daniel Lezcano <daniel.lezcano@linaro.org>
Subject: Re: [PATCH v3 2/4] drivers: thermal: tsens: Add generic support for
 TSENS v1 IP
Message-ID: <20181204164340.GA3146@localhost.localdomain>
References: <cover.1543335819.git.amit.kucheria@linaro.org>
 <578f79ce10c51bbb7bd6f44395e10a3369a050f4.1543335819.git.amit.kucheria@linaro.org>
 <20181129165606.GE2688@localhost.localdomain>
 <CAP245DXT7L_HnfeVNcY0ko3JP-kDnNuhaZPy664Z-sAm4SUVXg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAP245DXT7L_HnfeVNcY0ko3JP-kDnNuhaZPy664Z-sAm4SUVXg@mail.gmail.com>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Dec 04, 2018 at 04:54:21PM +0530, Amit Kucheria wrote:
> On Thu, Nov 29, 2018 at 10:26 PM Eduardo Valentin <edubezval@gmail.com> wrote:
> >
> > On Tue, Nov 27, 2018 at 09:59:05PM +0530, Amit Kucheria wrote:
> 
> <snip>
> 
> > > +     qfprom_cdata = (u32 *)qfprom_read(tmdev->dev, "calib");
> > > +     if (IS_ERR(qfprom_cdata))
> > > +             return PTR_ERR(qfprom_cdata);
> > > +
> > > +     mode = (qfprom_cdata[4] & CAL_SEL_MASK) >> CAL_SEL_SHIFT;
> > > +     dev_dbg(tmdev->dev, "calibration mode is %d\n", mode);
> > > +
> > > +     switch (mode) {
> > > +     case TWO_PT_CALIB:
> > > +             base1 = (qfprom_cdata[4] & BASE1_MASK) >> BASE1_SHIFT;
> > > +             p2[0] = (qfprom_cdata[0] & S0_P2_MASK) >> S0_P2_SHIFT;
> > > +             p2[1] = (qfprom_cdata[0] & S1_P2_MASK) >> S1_P2_SHIFT;
> > > +             /* This value is split over two registers, 2 bits and 4 bits */
> > > +             lsb   = (qfprom_cdata[0] & S2_P2_MASK_1_0) >> S2_P2_SHIFT_1_0;
> > > +             msb   = (qfprom_cdata[1] & S2_P2_MASK_5_2) >> S2_P2_SHIFT_5_2;
> > > +             p2[2] = msb << 2 | lsb;
> > > +             p2[3] = (qfprom_cdata[1] & S3_P2_MASK) >> S3_P2_SHIFT;
> > > +             p2[4] = (qfprom_cdata[1] & S4_P2_MASK) >> S4_P2_SHIFT;
> > > +             p2[5] = (qfprom_cdata[2] & S5_P2_MASK) >> S5_P2_SHIFT;
> > > +             p2[6] = (qfprom_cdata[2] & S6_P2_MASK) >> S6_P2_SHIFT;
> > > +             /* This value is split over two registers, 2 bits and 4 bits */
> > > +             lsb   = (qfprom_cdata[2] & S7_P2_MASK_1_0) >> S7_P2_SHIFT_1_0;
> > > +             msb   = (qfprom_cdata[3] & S7_P2_MASK_5_2) >> S7_P2_SHIFT_5_2;
> > > +             p2[7] = msb << 2 | lsb;
> > > +             p2[8] = (qfprom_cdata[3] & S8_P2_MASK) >> S8_P2_SHIFT;
> > > +             p2[9] = (qfprom_cdata[3] & S9_P2_MASK) >> S9_P2_SHIFT;
> > > +             for (i = 0; i < tmdev->num_sensors; i++)
> > > +                     p2[i] = ((base1 + p2[i]) << 2);
> > > +             /* Fall through */
> > > +     case ONE_PT_CALIB2:
> > > +             base0 = (qfprom_cdata[4] & BASE0_MASK) >> BASE0_SHIFT;
> > > +             p1[0] = (qfprom_cdata[0] & S0_P1_MASK) >> S0_P1_SHIFT;
> > > +             p1[1] = (qfprom_cdata[0] & S1_P1_MASK) >> S1_P1_SHIFT;
> > > +             p1[2] = (qfprom_cdata[0] & S2_P1_MASK) >> S2_P1_SHIFT;
> > > +             p1[3] = (qfprom_cdata[1] & S3_P1_MASK) >> S3_P1_SHIFT;
> > > +             p1[4] = (qfprom_cdata[1] & S4_P1_MASK) >> S4_P1_SHIFT;
> > > +             p1[5] = (qfprom_cdata[2] & S5_P1_MASK) >> S5_P1_SHIFT;
> > > +             p1[6] = (qfprom_cdata[2] & S6_P1_MASK) >> S6_P1_SHIFT;
> > > +             p1[7] = (qfprom_cdata[2] & S7_P1_MASK) >> S7_P1_SHIFT;
> > > +             p1[8] = (qfprom_cdata[3] & S8_P1_MASK) >> S8_P1_SHIFT;
> > > +             p1[9] = (qfprom_cdata[3] & S9_P1_MASK) >> S9_P1_SHIFT;
> > > +             for (i = 0; i < tmdev->num_sensors; i++)
> > > +                     p1[i] = (((base0) + p1[i]) << 2);
> > > +             break;
> > > +     default:
> > > +             for (i = 0; i < tmdev->num_sensors; i++) {
> > > +                     p1[i] = 500;
> > > +                     p2[i] = 780;
> > ... Wouldn't be nice to know what 500 or 780 stands for here?
> 
> Indeed, however I haven't found a suitable explanations in the
> downstream trees or documentation yet. tsens-8974.c and tsens-8916.c
> seem to have similar magic numbers.
> 

Well, it does not need to be exactly like it is in downstream, does it?
:-)

> > Why these defaults? Do we expect to have further patches to keep
> > updating these?
> 
> No we don't expect them to change. I wish I had a better answer, but
> honestly I don't. I'll keep looking for a explanation to replace these
> magic numbers.
> 

Yeah, I see, the problem that I have is that these are slope and
offsets, and these constants change from chip version/rev, essentially
due to sensor location, process, etc, so hardcoding in the probe seams
to be asking for future patching.

> > > +             }
> > > +             break;
> > > +     }
> > > +
> > > +     compute_intercept_slope(tmdev, p1, p2, mode);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +#define STATUS_OFFSET                0x44
> > > +#define LAST_TEMP_MASK               0x3ff
> > > +#define STATUS_VALID_BIT     BIT(14)
> > > +
> > > +static int get_temp_tsens_v1(struct tsens_device *tmdev, int id, int *temp)
> > > +{
> > > +     struct tsens_sensor *s = &tmdev->sensor[id];
> > > +     u32 code;
> > > +     unsigned int status_reg;
> > > +     u32 last_temp = 0, last_temp2 = 0, last_temp3 = 0;
> > > +     int ret;
> > > +
> > > +     status_reg = tmdev->tm_offset + STATUS_OFFSET + s->hw_id * 4;
> > > +     ret = regmap_read(tmdev->tm_map, status_reg, &code);
> > > +     if (ret)
> > > +             return ret;
> > > +     last_temp = code & LAST_TEMP_MASK;
> > > +     if (code & STATUS_VALID_BIT)
> > > +             goto done;
> > > +
> > > +     /* Try a second time */
> > > +     ret = regmap_read(tmdev->tm_map, status_reg, &code);
> > > +     if (ret)
> > > +             return ret;
> > > +     if (code & STATUS_VALID_BIT) {
> > > +             last_temp = code & LAST_TEMP_MASK;
> > > +             goto done;
> > > +     } else {
> > > +             last_temp2 = code & LAST_TEMP_MASK;
> > > +     }
> > > +
> > > +     /* Try a third/last time */
> > > +     ret = regmap_read(tmdev->tm_map, status_reg, &code);
> > > +     if (ret)
> > > +             return ret;
> > > +     if (code & STATUS_VALID_BIT) {
> > > +             last_temp = code & LAST_TEMP_MASK;
> > > +             goto done;
> > > +     } else {
> > > +             last_temp3 = code & LAST_TEMP_MASK;
> > > +     }
> > > +
> > > +     if (last_temp == last_temp2)
> > > +             last_temp = last_temp2;
> > > +     else if (last_temp2 == last_temp3)
> > > +             last_temp = last_temp3;
> > > +done:
> > > +     /* Convert temperature from ADC code to milliCelsius */
> > > +     *temp = code_to_degc(last_temp, s) * 1000;
> >
> > This three strikes/read approach seams awkward. Is this a broken
> > sensor or are you missing the programming steps? Maybe you need to
> > either wait for a IRQ on temp ready, or maybe you need simply wait
> > some amount of cycles before the sensor is ready with new temp/ADC
> > conversion, no?
> 
> This "algorithm" for reading data is specified in the Tsens IP HW
> documentation almost word for word. The status_reg contains both the
> VALID_BIT and the temperature, so we don't know if this is valid data
> until we read the register. I'll be posting IRQ support soon.
> 
> Again, this algorithm is used for tsens-v2.c and tsens-8916.c so I
> haven't tried to clean this up, yet. I have an experimental branch to
> hide IP-specific register changes behind the regmap interface. This
> should get rid of copies of these functions across the various files
> but that isn't yet ready to post yet.
> 
> > Also, the goto's dont really help, as we dont really have any
> > resource to rollback here..
> 
> Agreed. :-) This function could be made much clearer.
> 
> If I fix this, should I fix it in the other files right away, or can
> we wait a bit more to clean this up as part of the move to regmap? I
> think it'd be logically simpier to clean this up in a separate series
> than introducing changes to the known working function as part of new
> machine enablement.
> 
> Thanks for the review.
> 
> Regards,
> Amit
> 
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static const struct tsens_ops ops_generic_v1 = {
> > > +     .init           = init_common,
> > > +     .calibrate      = calibrate_v1,
> > > +     .get_temp       = get_temp_tsens_v1,
> > > +};
> > > +
> > > +const struct tsens_data data_tsens_v1 = {
> > > +     .ops            = &ops_generic_v1,
> > > +     .reg_offsets    = { [SROT_CTRL_OFFSET] = 0x4 },
> > > +};
> > > diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
> > > index f1ec9bbe4717..d0cc0c09894a 100644
> > > --- a/drivers/thermal/qcom/tsens.c
> > > +++ b/drivers/thermal/qcom/tsens.c
> > > @@ -63,6 +63,9 @@ static const struct of_device_id tsens_table[] = {
> > >       }, {
> > >               .compatible = "qcom,msm8996-tsens",
> > >               .data = &data_8996,
> > > +     }, {
> > > +             .compatible = "qcom,tsens-v1",
> > > +             .data = &data_tsens_v1,
> > >       }, {
> > >               .compatible = "qcom,tsens-v2",
> > >               .data = &data_tsens_v2,
> > > diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
> > > index 7b7feee5dc46..f8dc96c42b94 100644
> > > --- a/drivers/thermal/qcom/tsens.h
> > > +++ b/drivers/thermal/qcom/tsens.h
> > > @@ -90,9 +90,10 @@ char *qfprom_read(struct device *, const char *);
> > >  void compute_intercept_slope(struct tsens_device *, u32 *, u32 *, u32);
> > >  int init_common(struct tsens_device *);
> > >  int get_temp_common(struct tsens_device *, int, int *);
> > > +int code_to_degc(u32 adc_code, const struct tsens_sensor *s);
> > >
> > >  /* TSENS v1 targets */
> > > -extern const struct tsens_data data_8916, data_8974, data_8960;
> > > +extern const struct tsens_data data_8916, data_8974, data_8960, data_tsens_v1;
> > >  /* TSENS v2 targets */
> > >  extern const struct tsens_data data_8996, data_tsens_v2;
> > >
> > > --
> > > 2.17.1
> > >