by Eduardo Valentin

[permalink] [raw]

Subject: Re: [PATCH v3 2/4] drivers: thermal: tsens: Add generic support for TSENS v1 IP

On Tue, Dec 04, 2018 at 04:54:21PM +0530, Amit Kucheria wrote:
> On Thu, Nov 29, 2018 at 10:26 PM Eduardo Valentin <[email protected]> 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
> > >