All of lore.kernel.org
 help / color / mirror / Atom feed
From: Eduardo Valentin <edubezval@gmail.com>
To: Amit Kucheria <amit.kucheria@linaro.org>
Cc: linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	bjorn.andersson@linaro.org, andy.gross@linaro.org,
	vkoul@kernel.org, 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
Date: Thu, 29 Nov 2018 08:56:08 -0800	[thread overview]
Message-ID: <20181129165606.GE2688@localhost.localdomain> (raw)
In-Reply-To: <578f79ce10c51bbb7bd6f44395e10a3369a050f4.1543335819.git.amit.kucheria@linaro.org>

On Tue, Nov 27, 2018 at 09:59:05PM +0530, Amit Kucheria wrote:
> qcs404 has a single TSENS IP block with 10 sensors. It uses version 1.4
> of the TSENS IP, functionality for which is encapsulated inside
> qcom,tsens-v1 compatible.
> 
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> Reviewed-by: Vinod Koul <vkoul@kernel.org>
> Tested-by: Vinod Koul <vkoul@kernel.org>
> ---
>  drivers/thermal/qcom/Makefile       |   2 +-
>  drivers/thermal/qcom/tsens-common.c |   2 +-
>  drivers/thermal/qcom/tsens-v1.c     | 196 ++++++++++++++++++++++++++++
>  drivers/thermal/qcom/tsens.c        |   3 +
>  drivers/thermal/qcom/tsens.h        |   3 +-
>  5 files changed, 203 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/thermal/qcom/tsens-v1.c
> 
> diff --git a/drivers/thermal/qcom/Makefile b/drivers/thermal/qcom/Makefile
> index a821929ede0b..60269ee90c43 100644
> --- a/drivers/thermal/qcom/Makefile
> +++ b/drivers/thermal/qcom/Makefile
> @@ -1,2 +1,2 @@
>  obj-$(CONFIG_QCOM_TSENS)	+= qcom_tsens.o
> -qcom_tsens-y			+= tsens.o tsens-common.o tsens-8916.o tsens-8974.o tsens-8960.o tsens-v2.o
> +qcom_tsens-y			+= tsens.o tsens-common.o tsens-8916.o tsens-8974.o tsens-8960.o tsens-v2.o tsens-v1.o
> diff --git a/drivers/thermal/qcom/tsens-common.c b/drivers/thermal/qcom/tsens-common.c
> index 3be4be2e0465..98f77401bc12 100644
> --- a/drivers/thermal/qcom/tsens-common.c
> +++ b/drivers/thermal/qcom/tsens-common.c
> @@ -76,7 +76,7 @@ void compute_intercept_slope(struct tsens_device *tmdev, u32 *p1,
>  	}
>  }
>  
> -static inline int code_to_degc(u32 adc_code, const struct tsens_sensor *s)
> +int code_to_degc(u32 adc_code, const struct tsens_sensor *s)
>  {
>  	int degc, num, den;
>  
> diff --git a/drivers/thermal/qcom/tsens-v1.c b/drivers/thermal/qcom/tsens-v1.c
> new file mode 100644
> index 000000000000..1dbf4fde6da8
> --- /dev/null
> +++ b/drivers/thermal/qcom/tsens-v1.c
> @@ -0,0 +1,196 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, Linaro Limited
> + */
> +
> +#include <linux/regmap.h>
> +#include <linux/bitops.h>
> +#include "tsens.h"
> +
> +/* eeprom layout data for qcs404 (v1) */
> +#define BASE0_MASK	0x000007f8
> +#define BASE1_MASK	0x0007f800
> +#define BASE0_SHIFT	3
> +#define BASE1_SHIFT	11
> +
> +#define S0_P1_MASK	0x0000003f
> +#define S1_P1_MASK	0x0003f000
> +#define S2_P1_MASK	0x3f000000
> +#define S3_P1_MASK	0x000003f0
> +#define S4_P1_MASK	0x003f0000
> +#define S5_P1_MASK	0x0000003f
> +#define S6_P1_MASK	0x0003f000
> +#define S7_P1_MASK	0x3f000000
> +#define S8_P1_MASK	0x000003f0
> +#define S9_P1_MASK	0x003f0000
> +
> +#define S0_P2_MASK	0x00000fc0
> +#define S1_P2_MASK	0x00fc0000
> +#define S2_P2_MASK_1_0	0xc0000000
> +#define S2_P2_MASK_5_2	0x0000000f
> +#define S3_P2_MASK	0x0000fc00
> +#define S4_P2_MASK	0x0fc00000
> +#define S5_P2_MASK	0x00000fc0
> +#define S6_P2_MASK	0x00fc0000
> +#define S7_P2_MASK_1_0	0xc0000000
> +#define S7_P2_MASK_5_2	0x0000000f
> +#define S8_P2_MASK	0x0000fc00
> +#define S9_P2_MASK	0x0fc00000
> +
> +#define S0_P1_SHIFT	0
> +#define S0_P2_SHIFT	6
> +#define S1_P1_SHIFT	12
> +#define S1_P2_SHIFT	18
> +#define S2_P1_SHIFT	24
> +#define S2_P2_SHIFT_1_0	30
> +
> +#define S2_P2_SHIFT_5_2	0
> +#define S3_P1_SHIFT	4
> +#define S3_P2_SHIFT	10
> +#define S4_P1_SHIFT	16
> +#define S4_P2_SHIFT	22
> +
> +#define S5_P1_SHIFT	0
> +#define S5_P2_SHIFT	6
> +#define S6_P1_SHIFT	12
> +#define S6_P2_SHIFT	18
> +#define S7_P1_SHIFT	24
> +#define S7_P2_SHIFT_1_0	30
> +
> +#define S7_P2_SHIFT_5_2	0
> +#define S8_P1_SHIFT	4
> +#define S8_P2_SHIFT	10
> +#define S9_P1_SHIFT	16
> +#define S9_P2_SHIFT	22
> +
> +#define CAL_SEL_MASK	7
> +#define CAL_SEL_SHIFT	0
> +
> +static int calibrate_v1(struct tsens_device *tmdev)
> +{
> +	u32 base0 = 0, base1 = 0;
> +	u32 p1[10], p2[10];
> +	u32 mode = 0, lsb = 0, msb = 0;
> +	u32 *qfprom_cdata;
> +	int i;
> +
> +	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?
Why these defaults? Do we expect to have further patches to keep
updating these?

> +		}
> +		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?

Also, the goto's dont really help, as we dont really have any
resource to rollback here..

> +
> +	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
> 

  reply	other threads:[~2018-11-29 16:56 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-27 16:29 [PATCH v3 0/4] thermal: tsens: Add support for QCS404 platform Amit Kucheria
2018-11-27 16:29 ` [PATCH v3 1/4] dt: thermal: tsens: Add bindings for qcs404 Amit Kucheria
2018-11-27 16:29 ` [PATCH v3 2/4] drivers: thermal: tsens: Add generic support for TSENS v1 IP Amit Kucheria
2018-11-29 16:56   ` Eduardo Valentin [this message]
2018-12-04 11:24     ` Amit Kucheria
2018-12-04 16:43       ` Eduardo Valentin
2018-11-27 16:29 ` [PATCH v3 3/4] arm64: dts: qcom: qcs404: Add tsens controller Amit Kucheria
2018-11-27 16:29 ` [PATCH v3 4/4] arm64: dts: qcom: qcs404: Add thermal zones for each sensor Amit Kucheria
2018-11-29 16:57   ` Eduardo Valentin
2018-12-04 11:26     ` Amit Kucheria
2018-12-04 16:36       ` Eduardo Valentin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181129165606.GE2688@localhost.localdomain \
    --to=edubezval@gmail.com \
    --cc=amit.kucheria@linaro.org \
    --cc=andy.gross@linaro.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=daniel.lezcano@linaro.org \
    --cc=khasim.mohammed@linaro.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=rui.zhang@intel.com \
    --cc=vkoul@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.