[PATCH 0/7] meson-saradc: add chip temperature support

[PATCH 0/7] meson-saradc: add chip temperature support

[Martin Blumenstingl]

The meson-saradc allows switching the input of channel #6 between an
actual ADC pad and the SoC's internal temperature sensor.

On the 64-bit SoCs the SCPI firmware reads the chip temperature using
the SAR ADC. Thus the 64-bit SoCs simply expose the temperature through
the "arm,scpi-sensors" interface (and so Linux doesn't have to program
the SAR ADC for temperature readings).

The 32-bit SoCs however don't have any SCPI firmware. Thus we need to
configure the SAR ADC in Linux to get the chip temperature. To make it
work we need two changes to the existing dt-bindings:
- we have to get the TSC (temperature sensor calibration coefficient)
  from the eFuse (where it has been programmed to the correct value in
  the manufacturing process). the SAR ADC needs the TSC value, otherwise
  it returns only garbage readings
- we need to get a phandle to the new HHI syscon region because on
  Meson8b and Meson8m2 the TSC is five bits wide (on Meson8 it's only
  four bits wide) but only four bits are stored inside the SAR ADC's
  memory region. The upper most bit (not all five bits, only one out of
  five) is stored at a seemingly random register HHI_DPLL_TOP_0) in the
  HHI register area.

Dependencies: The SAR ADC changes are completely independent from
anything else. However, the arch/arm/boot/dts changes from this series
depend on my other series "Meson8/Meson8b: introduce a HHI syscon node"
from [0]. I decided to send them all together to give a better overview
how the end result looks like - I can split the dts changes into a
separate series when requested (in any case I would like Kevin to take
them through his linux-amlogic tree).


[0] https://patchwork.kernel.org/cover/10658527/


Martin Blumenstingl (7):
  dt-bindings: iio: adc: meson-saradc: add temperature sensor support
  iio: adc: meson-saradc: add support for the chip's temperature sensor
  ARM: dts: meson8: add the temperature calibration data for the SAR ADC
  ARM: dts: meson8b: add the temperature calibration data for the SAR
    ADC
  ARM: dts: meson8b: ec100: add iio-hwmon for the chip temperature
  ARM: dts: meson8b: odroidc1: add iio-hwmon for the chip temperature
  ARM: dts: meson8m2: mxiii-plus: add iio-hwmon for the chip temperature

 .../bindings/iio/adc/amlogic,meson-saradc.txt |  10 +
 arch/arm/boot/dts/meson8.dtsi                 |   8 +
 arch/arm/boot/dts/meson8b-ec100.dts           |   5 +
 arch/arm/boot/dts/meson8b-odroidc1.dts        |   5 +
 arch/arm/boot/dts/meson8b.dtsi                |   8 +
 arch/arm/boot/dts/meson8m2-mxiii-plus.dts     |   5 +
 drivers/iio/adc/meson_saradc.c                | 274 ++++++++++++++++--
 7 files changed, 289 insertions(+), 26 deletions(-)

-- 
2.19.1

[PATCH 0/7] meson-saradc: add chip temperature support

[Martin Blumenstingl]

The meson-saradc allows switching the input of channel #6 between an
actual ADC pad and the SoC's internal temperature sensor.

On the 64-bit SoCs the SCPI firmware reads the chip temperature using
the SAR ADC. Thus the 64-bit SoCs simply expose the temperature through
the "arm,scpi-sensors" interface (and so Linux doesn't have to program
the SAR ADC for temperature readings).

The 32-bit SoCs however don't have any SCPI firmware. Thus we need to
configure the SAR ADC in Linux to get the chip temperature. To make it
work we need two changes to the existing dt-bindings:
- we have to get the TSC (temperature sensor calibration coefficient)
  from the eFuse (where it has been programmed to the correct value in
  the manufacturing process). the SAR ADC needs the TSC value, otherwise
  it returns only garbage readings
- we need to get a phandle to the new HHI syscon region because on
  Meson8b and Meson8m2 the TSC is five bits wide (on Meson8 it's only
  four bits wide) but only four bits are stored inside the SAR ADC's
  memory region. The upper most bit (not all five bits, only one out of
  five) is stored at a seemingly random register HHI_DPLL_TOP_0) in the
  HHI register area.

Dependencies: The SAR ADC changes are completely independent from
anything else. However, the arch/arm/boot/dts changes from this series
depend on my other series "Meson8/Meson8b: introduce a HHI syscon node"
from [0]. I decided to send them all together to give a better overview
how the end result looks like - I can split the dts changes into a
separate series when requested (in any case I would like Kevin to take
them through his linux-amlogic tree).


[0] https://patchwork.kernel.org/cover/10658527/


Martin Blumenstingl (7):
  dt-bindings: iio: adc: meson-saradc: add temperature sensor support
  iio: adc: meson-saradc: add support for the chip's temperature sensor
  ARM: dts: meson8: add the temperature calibration data for the SAR ADC
  ARM: dts: meson8b: add the temperature calibration data for the SAR
    ADC
  ARM: dts: meson8b: ec100: add iio-hwmon for the chip temperature
  ARM: dts: meson8b: odroidc1: add iio-hwmon for the chip temperature
  ARM: dts: meson8m2: mxiii-plus: add iio-hwmon for the chip temperature

 .../bindings/iio/adc/amlogic,meson-saradc.txt |  10 +
 arch/arm/boot/dts/meson8.dtsi                 |   8 +
 arch/arm/boot/dts/meson8b-ec100.dts           |   5 +
 arch/arm/boot/dts/meson8b-odroidc1.dts        |   5 +
 arch/arm/boot/dts/meson8b.dtsi                |   8 +
 arch/arm/boot/dts/meson8m2-mxiii-plus.dts     |   5 +
 drivers/iio/adc/meson_saradc.c                | 274 ++++++++++++++++--
 7 files changed, 289 insertions(+), 26 deletions(-)

-- 
2.19.1

[PATCH 1/7] dt-bindings: iio: adc: meson-saradc: add temperature sensor support

[Martin Blumenstingl]

The 32-bit Meson SoCs Meson8, Meson8b and Meson8m2 can use the SAR ADC
to read the chip temperature. This requires a few new, optional
properties:
- nvmem-cells and nvmem-cell-names are needed because the temperature
  sensor requires calibration to work correctly. The calibration data is
  stored in the eFuse.
- amlogic,hhi-sysctrl is needed on Meson8b and Meson8m2 because the 5th
  bit of the TSC (temperature sensor calibration coefficient) is stored
  in the HHI register region (in the scratch register HHI_DPLL_TOP_0 at
  offset 0x318).

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 .../bindings/iio/adc/amlogic,meson-saradc.txt          | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/amlogic,meson-saradc.txt b/Documentation/devicetree/bindings/iio/adc/amlogic,meson-saradc.txt
index 54b823f3a453..75c775954102 100644
--- a/Documentation/devicetree/bindings/iio/adc/amlogic,meson-saradc.txt
+++ b/Documentation/devicetree/bindings/iio/adc/amlogic,meson-saradc.txt
@@ -22,6 +22,16 @@ Required properties:
 - vref-supply:	the regulator supply for the ADC reference voltage
 - #io-channel-cells: must be 1, see ../iio-bindings.txt
 
+Optional properties:
+- amlogic,hhi-sysctrl:	phandle to the syscon which contains the 5th bit
+			of the TSC (temperature sensor coefficient) on
+			Meson8b and Meson8m2 (which used to calibrate the
+			temperature sensor)
+- nvmem-cells:		phandle to the temperature_calib eFuse cells
+- nvmem-cell-names:	if present (to enable the temperature sensor
+			calibration) this must contain "temperature_calib"
+
+
 Example:
 	saradc: adc@8680 {
 		compatible = "amlogic,meson-gxl-saradc", "amlogic,meson-saradc";
-- 
2.19.1

[PATCH 1/7] dt-bindings: iio: adc: meson-saradc: add temperature sensor support

[Martin Blumenstingl]

The 32-bit Meson SoCs Meson8, Meson8b and Meson8m2 can use the SAR ADC
to read the chip temperature. This requires a few new, optional
properties:
- nvmem-cells and nvmem-cell-names are needed because the temperature
  sensor requires calibration to work correctly. The calibration data is
  stored in the eFuse.
- amlogic,hhi-sysctrl is needed on Meson8b and Meson8m2 because the 5th
  bit of the TSC (temperature sensor calibration coefficient) is stored
  in the HHI register region (in the scratch register HHI_DPLL_TOP_0 at
  offset 0x318).

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 .../bindings/iio/adc/amlogic,meson-saradc.txt          | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/amlogic,meson-saradc.txt b/Documentation/devicetree/bindings/iio/adc/amlogic,meson-saradc.txt
index 54b823f3a453..75c775954102 100644
--- a/Documentation/devicetree/bindings/iio/adc/amlogic,meson-saradc.txt
+++ b/Documentation/devicetree/bindings/iio/adc/amlogic,meson-saradc.txt
@@ -22,6 +22,16 @@ Required properties:
 - vref-supply:	the regulator supply for the ADC reference voltage
 - #io-channel-cells: must be 1, see ../iio-bindings.txt
 
+Optional properties:
+- amlogic,hhi-sysctrl:	phandle to the syscon which contains the 5th bit
+			of the TSC (temperature sensor coefficient) on
+			Meson8b and Meson8m2 (which used to calibrate the
+			temperature sensor)
+- nvmem-cells:		phandle to the temperature_calib eFuse cells
+- nvmem-cell-names:	if present (to enable the temperature sensor
+			calibration) this must contain "temperature_calib"
+
+
 Example:
 	saradc: adc at 8680 {
 		compatible = "amlogic,meson-gxl-saradc", "amlogic,meson-saradc";
-- 
2.19.1

[PATCH 2/7] iio: adc: meson-saradc: add support for the chip's temperature sensor

[Martin Blumenstingl]

Channel 6 of the SAR ADC can be switched between two inputs:
SAR_ADC_CH6 input (an actual pad on the SoC) and the signal from the
temperature sensor inside the SoC.

To get usable results from the temperature sensor we need to read the
corresponding calibration data from the eFuse and pass it to the SAR ADC
(bits [3:0]) and (on Meson8b and Meson8m2) to a scratch register in the
HHI region.
If the temperature sensor is not calibrated (the eFuse data contains a
bit for this) then the driver will return -ENOTSUPP when trying to read
the temperature.

This only enables the temperature sensor for the Meson8, Meson8b and
Meson8m2 SoCs because on the newer, 64-bit SoCs (GXBB, GXL and GXM) the
temperature sensor inside SAR ADC is firmware-controlled (by BL30, we
can simply use the SCPI hwmon driver to get the chip temperature).

To keep the devicetree interface backwards compatible we simply skip the
temperature sensor initialization if no eFuse nvmem cell is passed via
devicetree.

The public documentation for the SAR ADC IP block does not explain how
to use the registers to read the temperature. The logic from this patch
is based on reading and understanding Amlogic's GPL kernel sources.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/iio/adc/meson_saradc.c | 274 +++++++++++++++++++++++++++++----
 1 file changed, 248 insertions(+), 26 deletions(-)

diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
index 028ccd218f82..df1e45805c23 100644
--- a/drivers/iio/adc/meson_saradc.c
+++ b/drivers/iio/adc/meson_saradc.c
@@ -18,6 +18,7 @@
 #include <linux/io.h>
 #include <linux/iio/iio.h>
 #include <linux/module.h>
+#include <linux/nvmem-consumer.h>
 #include <linux/interrupt.h>
 #include <linux/of.h>
 #include <linux/of_irq.h>
@@ -25,6 +26,7 @@
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
+#include <linux/mfd/syscon.h>
 
 #define MESON_SAR_ADC_REG0					0x00
 	#define MESON_SAR_ADC_REG0_PANEL_DETECT			BIT(31)
@@ -165,6 +167,17 @@
 
 #define MESON_SAR_ADC_MAX_FIFO_SIZE				32
 #define MESON_SAR_ADC_TIMEOUT					100 /* ms */
+#define MESON_SAR_ADC_VOLTAGE_AND_TEMP_CHANNEL			6
+#define MESON_SAR_ADC_TEMP_OFFSET				27
+
+/* temperature sensor calibration information in eFuse */
+#define MESON_SAR_ADC_EFUSE_BYTES				4
+#define MESON_SARADC_EFUSE_BYTE3_UPPER_ADC_VAL			GENMASK(6, 0)
+#define MESON_SARADC_EFUSE_BYTE3_IS_CALIBRATED			BIT(7)
+
+#define MESON_HHI_DPLL_TOP_0					0x318
+#define MESON_HHI_DPLL_TOP_0_TSC_BIT4				BIT(9)
+
 /* for use with IIO_VAL_INT_PLUS_MICRO */
 #define MILLION							1000000
 
@@ -175,16 +188,27 @@
 	.address = _chan,						\
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
 				BIT(IIO_CHAN_INFO_AVERAGE_RAW),		\
-	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |		\
-				BIT(IIO_CHAN_INFO_CALIBBIAS) |		\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),		\
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_CALIBBIAS) |	\
 				BIT(IIO_CHAN_INFO_CALIBSCALE),		\
 	.datasheet_name = "SAR_ADC_CH"#_chan,				\
 }
 
-/*
- * TODO: the hardware supports IIO_TEMP for channel 6 as well which is
- * currently not supported by this driver.
- */
+#define MESON_SAR_ADC_TEMP_CHAN(_chan) {				\
+	.type = IIO_TEMP,						\
+	.indexed = 0,							\
+	.channel = _chan,						\
+	.address = MESON_SAR_ADC_VOLTAGE_AND_TEMP_CHANNEL,		\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
+				BIT(IIO_CHAN_INFO_AVERAGE_RAW),		\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET) |		\
+					BIT(IIO_CHAN_INFO_SCALE) |	\
+					BIT(IIO_CHAN_INFO_ENABLE),	\
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_CALIBBIAS) |	\
+				BIT(IIO_CHAN_INFO_CALIBSCALE),		\
+	.datasheet_name = "TEMP_SENSOR",				\
+}
+
 static const struct iio_chan_spec meson_sar_adc_iio_channels[] = {
 	MESON_SAR_ADC_CHAN(0),
 	MESON_SAR_ADC_CHAN(1),
@@ -197,6 +221,19 @@ static const struct iio_chan_spec meson_sar_adc_iio_channels[] = {
 	IIO_CHAN_SOFT_TIMESTAMP(8),
 };
 
+static const struct iio_chan_spec meson_sar_adc_and_temp_iio_channels[] = {
+	MESON_SAR_ADC_CHAN(0),
+	MESON_SAR_ADC_CHAN(1),
+	MESON_SAR_ADC_CHAN(2),
+	MESON_SAR_ADC_CHAN(3),
+	MESON_SAR_ADC_CHAN(4),
+	MESON_SAR_ADC_CHAN(5),
+	MESON_SAR_ADC_CHAN(6),
+	MESON_SAR_ADC_CHAN(7),
+	MESON_SAR_ADC_TEMP_CHAN(8),
+	IIO_CHAN_SOFT_TIMESTAMP(9),
+};
+
 enum meson_sar_adc_avg_mode {
 	NO_AVERAGING = 0x0,
 	MEAN_AVERAGING = 0x1,
@@ -225,6 +262,11 @@ struct meson_sar_adc_param {
 	u32					bandgap_reg;
 	unsigned int				resolution;
 	const struct regmap_config		*regmap_config;
+	u8					temperature_trimming_bits;
+	unsigned int				temperature_multiplier;
+	unsigned int				temperature_divider;
+	const struct iio_chan_spec		*channels;
+	unsigned int				num_channels;
 };
 
 struct meson_sar_adc_data {
@@ -246,6 +288,10 @@ struct meson_sar_adc_priv {
 	struct completion			done;
 	int					calibbias;
 	int					calibscale;
+	struct regmap				*tsc_regmap;
+	bool					temperature_sensor_calibrated;
+	u8					temperature_sensor_coefficient;
+	u16					temperature_sensor_adc_val;
 };
 
 static const struct regmap_config meson_sar_adc_regmap_config_gxbb = {
@@ -389,9 +435,17 @@ static void meson_sar_adc_enable_channel(struct iio_dev *indio_dev,
 			   MESON_SAR_ADC_DETECT_IDLE_SW_IDLE_MUX_SEL_MASK,
 			   regval);
 
-	if (chan->address == 6)
-		regmap_update_bits(priv->regmap, MESON_SAR_ADC_DELTA_10,
-				   MESON_SAR_ADC_DELTA_10_TEMP_SEL, 0);
+	if (chan->address == MESON_SAR_ADC_VOLTAGE_AND_TEMP_CHANNEL) {
+		if (chan->type == IIO_TEMP)
+			regmap_update_bits(priv->regmap,
+					   MESON_SAR_ADC_DELTA_10,
+					   MESON_SAR_ADC_DELTA_10_TEMP_SEL,
+					   MESON_SAR_ADC_DELTA_10_TEMP_SEL);
+		else
+			regmap_update_bits(priv->regmap,
+					   MESON_SAR_ADC_DELTA_10,
+					   MESON_SAR_ADC_DELTA_10_TEMP_SEL, 0);
+	}
 }
 
 static void meson_sar_adc_set_chan7_mux(struct iio_dev *indio_dev,
@@ -506,8 +560,12 @@ static int meson_sar_adc_get_sample(struct iio_dev *indio_dev,
 				    enum meson_sar_adc_num_samples avg_samples,
 				    int *val)
 {
+	struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
 	int ret;
 
+	if (chan->type == IIO_TEMP && !priv->temperature_sensor_calibrated)
+		return -ENOTSUPP;
+
 	ret = meson_sar_adc_lock(indio_dev);
 	if (ret)
 		return ret;
@@ -555,17 +613,31 @@ static int meson_sar_adc_iio_info_read_raw(struct iio_dev *indio_dev,
 		break;
 
 	case IIO_CHAN_INFO_SCALE:
-		ret = regulator_get_voltage(priv->vref);
-		if (ret < 0) {
-			dev_err(indio_dev->dev.parent,
-				"failed to get vref voltage: %d\n", ret);
-			return ret;
+		if (chan->type == IIO_VOLTAGE) {
+			ret = regulator_get_voltage(priv->vref);
+			if (ret < 0) {
+				dev_err(indio_dev->dev.parent,
+					"failed to get vref voltage: %d\n",
+					ret);
+				return ret;
+			}
+
+			*val = ret / 1000;
+			*val2 = priv->param->resolution;
+			return IIO_VAL_FRACTIONAL_LOG2;
+		} else if (chan->type == IIO_TEMP) {
+			/* SoC specific multiplier and divider */
+			*val = priv->param->temperature_multiplier;
+			*val2 = priv->param->temperature_divider;
+
+			/* celsius to millicelsius */
+			*val *= 1000;
+
+			return IIO_VAL_FRACTIONAL;
+		} else {
+			return -EINVAL;
 		}
 
-		*val = ret / 1000;
-		*val2 = priv->param->resolution;
-		return IIO_VAL_FRACTIONAL_LOG2;
-
 	case IIO_CHAN_INFO_CALIBBIAS:
 		*val = priv->calibbias;
 		return IIO_VAL_INT;
@@ -575,6 +647,17 @@ static int meson_sar_adc_iio_info_read_raw(struct iio_dev *indio_dev,
 		*val2 = priv->calibscale % MILLION;
 		return IIO_VAL_INT_PLUS_MICRO;
 
+	case IIO_CHAN_INFO_OFFSET:
+		*val = DIV_ROUND_CLOSEST(MESON_SAR_ADC_TEMP_OFFSET *
+					 priv->param->temperature_divider,
+					 priv->param->temperature_multiplier);
+		*val -= priv->temperature_sensor_adc_val;
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_ENABLE:
+		*val = priv->temperature_sensor_calibrated;
+		return IIO_VAL_INT;
+
 	default:
 		return -EINVAL;
 	}
@@ -625,6 +708,77 @@ static int meson_sar_adc_clk_init(struct iio_dev *indio_dev,
 	return 0;
 }
 
+static int meson_sar_adc_temp_sensor_init(struct iio_dev *indio_dev)
+{
+	struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
+	u8 *buf, trimming_bits, trimming_mask, upper_adc_val;
+	struct nvmem_cell *temperature_calib;
+	size_t read_len;
+	int ret;
+
+	temperature_calib = devm_nvmem_cell_get(&indio_dev->dev,
+						"temperature_calib");
+	if (IS_ERR(temperature_calib)) {
+		ret = PTR_ERR(temperature_calib);
+
+		/*
+		 * leave the temperature sensor disabled if no calibration data
+		 * was passed via nvmem-cells.
+		 */
+		if (ret == -ENODEV)
+			return 0;
+
+		if (ret != -EPROBE_DEFER)
+			dev_err(indio_dev->dev.parent,
+				"failed to get temperature_calib cell\n");
+
+		return ret;
+	}
+
+	priv->tsc_regmap = syscon_regmap_lookup_by_phandle(
+						indio_dev->dev.parent->of_node,
+						"amlogic,hhi-sysctrl");
+	if (IS_ERR(priv->tsc_regmap)) {
+		dev_err(indio_dev->dev.parent,
+			"failed to get amlogic,hhi-sysctrl regmap\n");
+		return PTR_ERR(priv->tsc_regmap);
+	}
+
+	read_len = MESON_SAR_ADC_EFUSE_BYTES;
+	buf = nvmem_cell_read(temperature_calib, &read_len);
+	if (IS_ERR(buf)) {
+		dev_err(indio_dev->dev.parent,
+			"failed to read temperature_calib cell\n");
+		return PTR_ERR(buf);
+	} else if (read_len != MESON_SAR_ADC_EFUSE_BYTES) {
+		kfree(buf);
+		dev_err(indio_dev->dev.parent,
+			"invalid read size of temperature_calib cell\n");
+		return -EINVAL;
+	}
+
+	trimming_bits = priv->param->temperature_trimming_bits;
+	trimming_mask = BIT(trimming_bits) - 1;
+
+	if (buf[3] & MESON_SARADC_EFUSE_BYTE3_IS_CALIBRATED)
+		priv->temperature_sensor_calibrated = true;
+	else
+		priv->temperature_sensor_calibrated = false;
+
+	priv->temperature_sensor_coefficient = buf[2] & trimming_mask;
+
+	upper_adc_val = FIELD_GET(MESON_SARADC_EFUSE_BYTE3_UPPER_ADC_VAL,
+				  buf[3]);
+
+	priv->temperature_sensor_adc_val = buf[2];
+	priv->temperature_sensor_adc_val |= upper_adc_val << BITS_PER_BYTE;
+	priv->temperature_sensor_adc_val >>= trimming_bits;
+
+	kfree(buf);
+
+	return 0;
+}
+
 static int meson_sar_adc_init(struct iio_dev *indio_dev)
 {
 	struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
@@ -649,10 +803,12 @@ static int meson_sar_adc_init(struct iio_dev *indio_dev)
 
 	meson_sar_adc_stop_sample_engine(indio_dev);
 
-	/* update the channel 6 MUX to select the temperature sensor */
+	/*
+	 * disable this bit as seems to be only relevant for Meson6 (based
+	 * on the vendor driver), which we don't support at the moment.
+	 */
 	regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG0,
-			MESON_SAR_ADC_REG0_ADC_TEMP_SEN_SEL,
-			MESON_SAR_ADC_REG0_ADC_TEMP_SEN_SEL);
+			MESON_SAR_ADC_REG0_ADC_TEMP_SEN_SEL, 0);
 
 	/* disable all channels by default */
 	regmap_write(priv->regmap, MESON_SAR_ADC_CHAN_LIST, 0x0);
@@ -709,6 +865,45 @@ static int meson_sar_adc_init(struct iio_dev *indio_dev)
 	regval |= MESON_SAR_ADC_AUX_SW_XP_DRIVE_SW;
 	regmap_write(priv->regmap, MESON_SAR_ADC_AUX_SW, regval);
 
+	if (priv->temperature_sensor_calibrated) {
+		regmap_update_bits(priv->regmap, MESON_SAR_ADC_DELTA_10,
+				   MESON_SAR_ADC_DELTA_10_TS_REVE1,
+				   MESON_SAR_ADC_DELTA_10_TS_REVE1);
+		regmap_update_bits(priv->regmap, MESON_SAR_ADC_DELTA_10,
+				   MESON_SAR_ADC_DELTA_10_TS_REVE0,
+				   MESON_SAR_ADC_DELTA_10_TS_REVE0);
+
+		/*
+		 * set bits [3:0] of the TSC (temperature sensor coefficient)
+		 * to get the correct values when reading the temperature.
+		 */
+		regval = FIELD_PREP(MESON_SAR_ADC_DELTA_10_TS_C_MASK,
+				    priv->temperature_sensor_coefficient);
+		regmap_update_bits(priv->regmap, MESON_SAR_ADC_DELTA_10,
+				   MESON_SAR_ADC_DELTA_10_TS_C_MASK, regval);
+
+		if (priv->param->temperature_trimming_bits == 5) {
+			if (priv->temperature_sensor_coefficient & BIT(4))
+				regval = MESON_HHI_DPLL_TOP_0_TSC_BIT4;
+			else
+				regval = 0;
+
+			/*
+			 * the 5th bit (4th when starting to count at 0) of the
+			 * TSC is located in another register area.
+			 */
+			regmap_update_bits(priv->tsc_regmap,
+					   MESON_HHI_DPLL_TOP_0,
+					   MESON_HHI_DPLL_TOP_0_TSC_BIT4,
+					   regval);
+		}
+	} else {
+		regmap_update_bits(priv->regmap, MESON_SAR_ADC_DELTA_10,
+				   MESON_SAR_ADC_DELTA_10_TS_REVE1, 0);
+		regmap_update_bits(priv->regmap, MESON_SAR_ADC_DELTA_10,
+				   MESON_SAR_ADC_DELTA_10_TS_REVE0, 0);
+	}
+
 	ret = clk_set_parent(priv->adc_sel_clk, priv->clkin);
 	if (ret) {
 		dev_err(indio_dev->dev.parent,
@@ -894,6 +1089,24 @@ static const struct meson_sar_adc_param meson_sar_adc_meson8_param = {
 	.bandgap_reg = MESON_SAR_ADC_DELTA_10,
 	.regmap_config = &meson_sar_adc_regmap_config_meson8,
 	.resolution = 10,
+	.temperature_trimming_bits = 4,
+	.temperature_multiplier = 18 * 10000,
+	.temperature_divider = 1024 * 10 * 85,
+	.channels = meson_sar_adc_and_temp_iio_channels,
+	.num_channels = ARRAY_SIZE(meson_sar_adc_and_temp_iio_channels),
+};
+
+static const struct meson_sar_adc_param meson_sar_adc_meson8b_param = {
+	.has_bl30_integration = false,
+	.clock_rate = 1150000,
+	.bandgap_reg = MESON_SAR_ADC_DELTA_10,
+	.regmap_config = &meson_sar_adc_regmap_config_meson8,
+	.resolution = 10,
+	.temperature_trimming_bits = 5,
+	.temperature_multiplier = 10,
+	.temperature_divider = 32,
+	.channels = meson_sar_adc_and_temp_iio_channels,
+	.num_channels = ARRAY_SIZE(meson_sar_adc_and_temp_iio_channels),
 };
 
 static const struct meson_sar_adc_param meson_sar_adc_gxbb_param = {
@@ -902,6 +1115,8 @@ static const struct meson_sar_adc_param meson_sar_adc_gxbb_param = {
 	.bandgap_reg = MESON_SAR_ADC_REG11,
 	.regmap_config = &meson_sar_adc_regmap_config_gxbb,
 	.resolution = 10,
+	.channels = meson_sar_adc_iio_channels,
+	.num_channels = ARRAY_SIZE(meson_sar_adc_iio_channels),
 };
 
 static const struct meson_sar_adc_param meson_sar_adc_gxl_param = {
@@ -910,6 +1125,8 @@ static const struct meson_sar_adc_param meson_sar_adc_gxl_param = {
 	.bandgap_reg = MESON_SAR_ADC_REG11,
 	.regmap_config = &meson_sar_adc_regmap_config_gxbb,
 	.resolution = 12,
+	.channels = meson_sar_adc_iio_channels,
+	.num_channels = ARRAY_SIZE(meson_sar_adc_iio_channels),
 };
 
 static const struct meson_sar_adc_data meson_sar_adc_meson8_data = {
@@ -918,12 +1135,12 @@ static const struct meson_sar_adc_data meson_sar_adc_meson8_data = {
 };
 
 static const struct meson_sar_adc_data meson_sar_adc_meson8b_data = {
-	.param = &meson_sar_adc_meson8_param,
+	.param = &meson_sar_adc_meson8b_param,
 	.name = "meson-meson8b-saradc",
 };
 
 static const struct meson_sar_adc_data meson_sar_adc_meson8m2_data = {
-	.param = &meson_sar_adc_meson8_param,
+	.param = &meson_sar_adc_meson8b_param,
 	.name = "meson-meson8m2-saradc",
 };
 
@@ -1008,9 +1225,8 @@ static int meson_sar_adc_probe(struct platform_device *pdev)
 	indio_dev->dev.of_node = pdev->dev.of_node;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &meson_sar_adc_iio_info;
-
-	indio_dev->channels = meson_sar_adc_iio_channels;
-	indio_dev->num_channels = ARRAY_SIZE(meson_sar_adc_iio_channels);
+	indio_dev->channels = priv->param->channels;
+	indio_dev->num_channels = priv->param->num_channels;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	base = devm_ioremap_resource(&pdev->dev, res);
@@ -1078,6 +1294,12 @@ static int meson_sar_adc_probe(struct platform_device *pdev)
 
 	priv->calibscale = MILLION;
 
+	if (priv->param->temperature_trimming_bits) {
+		ret = meson_sar_adc_temp_sensor_init(indio_dev);
+		if (ret)
+			return ret;
+	}
+
 	ret = meson_sar_adc_init(indio_dev);
 	if (ret)
 		goto err;
-- 
2.19.1

[PATCH 2/7] iio: adc: meson-saradc: add support for the chip's temperature sensor

[Martin Blumenstingl]

Channel 6 of the SAR ADC can be switched between two inputs:
SAR_ADC_CH6 input (an actual pad on the SoC) and the signal from the
temperature sensor inside the SoC.

To get usable results from the temperature sensor we need to read the
corresponding calibration data from the eFuse and pass it to the SAR ADC
(bits [3:0]) and (on Meson8b and Meson8m2) to a scratch register in the
HHI region.
If the temperature sensor is not calibrated (the eFuse data contains a
bit for this) then the driver will return -ENOTSUPP when trying to read
the temperature.

This only enables the temperature sensor for the Meson8, Meson8b and
Meson8m2 SoCs because on the newer, 64-bit SoCs (GXBB, GXL and GXM) the
temperature sensor inside SAR ADC is firmware-controlled (by BL30, we
can simply use the SCPI hwmon driver to get the chip temperature).

To keep the devicetree interface backwards compatible we simply skip the
temperature sensor initialization if no eFuse nvmem cell is passed via
devicetree.

The public documentation for the SAR ADC IP block does not explain how
to use the registers to read the temperature. The logic from this patch
is based on reading and understanding Amlogic's GPL kernel sources.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/iio/adc/meson_saradc.c | 274 +++++++++++++++++++++++++++++----
 1 file changed, 248 insertions(+), 26 deletions(-)

diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
index 028ccd218f82..df1e45805c23 100644
--- a/drivers/iio/adc/meson_saradc.c
+++ b/drivers/iio/adc/meson_saradc.c
@@ -18,6 +18,7 @@
 #include <linux/io.h>
 #include <linux/iio/iio.h>
 #include <linux/module.h>
+#include <linux/nvmem-consumer.h>
 #include <linux/interrupt.h>
 #include <linux/of.h>
 #include <linux/of_irq.h>
@@ -25,6 +26,7 @@
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
+#include <linux/mfd/syscon.h>
 
 #define MESON_SAR_ADC_REG0					0x00
 	#define MESON_SAR_ADC_REG0_PANEL_DETECT			BIT(31)
@@ -165,6 +167,17 @@
 
 #define MESON_SAR_ADC_MAX_FIFO_SIZE				32
 #define MESON_SAR_ADC_TIMEOUT					100 /* ms */
+#define MESON_SAR_ADC_VOLTAGE_AND_TEMP_CHANNEL			6
+#define MESON_SAR_ADC_TEMP_OFFSET				27
+
+/* temperature sensor calibration information in eFuse */
+#define MESON_SAR_ADC_EFUSE_BYTES				4
+#define MESON_SARADC_EFUSE_BYTE3_UPPER_ADC_VAL			GENMASK(6, 0)
+#define MESON_SARADC_EFUSE_BYTE3_IS_CALIBRATED			BIT(7)
+
+#define MESON_HHI_DPLL_TOP_0					0x318
+#define MESON_HHI_DPLL_TOP_0_TSC_BIT4				BIT(9)
+
 /* for use with IIO_VAL_INT_PLUS_MICRO */
 #define MILLION							1000000
 
@@ -175,16 +188,27 @@
 	.address = _chan,						\
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
 				BIT(IIO_CHAN_INFO_AVERAGE_RAW),		\
-	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |		\
-				BIT(IIO_CHAN_INFO_CALIBBIAS) |		\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),		\
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_CALIBBIAS) |	\
 				BIT(IIO_CHAN_INFO_CALIBSCALE),		\
 	.datasheet_name = "SAR_ADC_CH"#_chan,				\
 }
 
-/*
- * TODO: the hardware supports IIO_TEMP for channel 6 as well which is
- * currently not supported by this driver.
- */
+#define MESON_SAR_ADC_TEMP_CHAN(_chan) {				\
+	.type = IIO_TEMP,						\
+	.indexed = 0,							\
+	.channel = _chan,						\
+	.address = MESON_SAR_ADC_VOLTAGE_AND_TEMP_CHANNEL,		\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
+				BIT(IIO_CHAN_INFO_AVERAGE_RAW),		\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET) |		\
+					BIT(IIO_CHAN_INFO_SCALE) |	\
+					BIT(IIO_CHAN_INFO_ENABLE),	\
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_CALIBBIAS) |	\
+				BIT(IIO_CHAN_INFO_CALIBSCALE),		\
+	.datasheet_name = "TEMP_SENSOR",				\
+}
+
 static const struct iio_chan_spec meson_sar_adc_iio_channels[] = {
 	MESON_SAR_ADC_CHAN(0),
 	MESON_SAR_ADC_CHAN(1),
@@ -197,6 +221,19 @@ static const struct iio_chan_spec meson_sar_adc_iio_channels[] = {
 	IIO_CHAN_SOFT_TIMESTAMP(8),
 };
 
+static const struct iio_chan_spec meson_sar_adc_and_temp_iio_channels[] = {
+	MESON_SAR_ADC_CHAN(0),
+	MESON_SAR_ADC_CHAN(1),
+	MESON_SAR_ADC_CHAN(2),
+	MESON_SAR_ADC_CHAN(3),
+	MESON_SAR_ADC_CHAN(4),
+	MESON_SAR_ADC_CHAN(5),
+	MESON_SAR_ADC_CHAN(6),
+	MESON_SAR_ADC_CHAN(7),
+	MESON_SAR_ADC_TEMP_CHAN(8),
+	IIO_CHAN_SOFT_TIMESTAMP(9),
+};
+
 enum meson_sar_adc_avg_mode {
 	NO_AVERAGING = 0x0,
 	MEAN_AVERAGING = 0x1,
@@ -225,6 +262,11 @@ struct meson_sar_adc_param {
 	u32					bandgap_reg;
 	unsigned int				resolution;
 	const struct regmap_config		*regmap_config;
+	u8					temperature_trimming_bits;
+	unsigned int				temperature_multiplier;
+	unsigned int				temperature_divider;
+	const struct iio_chan_spec		*channels;
+	unsigned int				num_channels;
 };
 
 struct meson_sar_adc_data {
@@ -246,6 +288,10 @@ struct meson_sar_adc_priv {
 	struct completion			done;
 	int					calibbias;
 	int					calibscale;
+	struct regmap				*tsc_regmap;
+	bool					temperature_sensor_calibrated;
+	u8					temperature_sensor_coefficient;
+	u16					temperature_sensor_adc_val;
 };
 
 static const struct regmap_config meson_sar_adc_regmap_config_gxbb = {
@@ -389,9 +435,17 @@ static void meson_sar_adc_enable_channel(struct iio_dev *indio_dev,
 			   MESON_SAR_ADC_DETECT_IDLE_SW_IDLE_MUX_SEL_MASK,
 			   regval);
 
-	if (chan->address == 6)
-		regmap_update_bits(priv->regmap, MESON_SAR_ADC_DELTA_10,
-				   MESON_SAR_ADC_DELTA_10_TEMP_SEL, 0);
+	if (chan->address == MESON_SAR_ADC_VOLTAGE_AND_TEMP_CHANNEL) {
+		if (chan->type == IIO_TEMP)
+			regmap_update_bits(priv->regmap,
+					   MESON_SAR_ADC_DELTA_10,
+					   MESON_SAR_ADC_DELTA_10_TEMP_SEL,
+					   MESON_SAR_ADC_DELTA_10_TEMP_SEL);
+		else
+			regmap_update_bits(priv->regmap,
+					   MESON_SAR_ADC_DELTA_10,
+					   MESON_SAR_ADC_DELTA_10_TEMP_SEL, 0);
+	}
 }
 
 static void meson_sar_adc_set_chan7_mux(struct iio_dev *indio_dev,
@@ -506,8 +560,12 @@ static int meson_sar_adc_get_sample(struct iio_dev *indio_dev,
 				    enum meson_sar_adc_num_samples avg_samples,
 				    int *val)
 {
+	struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
 	int ret;
 
+	if (chan->type == IIO_TEMP && !priv->temperature_sensor_calibrated)
+		return -ENOTSUPP;
+
 	ret = meson_sar_adc_lock(indio_dev);
 	if (ret)
 		return ret;
@@ -555,17 +613,31 @@ static int meson_sar_adc_iio_info_read_raw(struct iio_dev *indio_dev,
 		break;
 
 	case IIO_CHAN_INFO_SCALE:
-		ret = regulator_get_voltage(priv->vref);
-		if (ret < 0) {
-			dev_err(indio_dev->dev.parent,
-				"failed to get vref voltage: %d\n", ret);
-			return ret;
+		if (chan->type == IIO_VOLTAGE) {
+			ret = regulator_get_voltage(priv->vref);
+			if (ret < 0) {
+				dev_err(indio_dev->dev.parent,
+					"failed to get vref voltage: %d\n",
+					ret);
+				return ret;
+			}
+
+			*val = ret / 1000;
+			*val2 = priv->param->resolution;
+			return IIO_VAL_FRACTIONAL_LOG2;
+		} else if (chan->type == IIO_TEMP) {
+			/* SoC specific multiplier and divider */
+			*val = priv->param->temperature_multiplier;
+			*val2 = priv->param->temperature_divider;
+
+			/* celsius to millicelsius */
+			*val *= 1000;
+
+			return IIO_VAL_FRACTIONAL;
+		} else {
+			return -EINVAL;
 		}
 
-		*val = ret / 1000;
-		*val2 = priv->param->resolution;
-		return IIO_VAL_FRACTIONAL_LOG2;
-
 	case IIO_CHAN_INFO_CALIBBIAS:
 		*val = priv->calibbias;
 		return IIO_VAL_INT;
@@ -575,6 +647,17 @@ static int meson_sar_adc_iio_info_read_raw(struct iio_dev *indio_dev,
 		*val2 = priv->calibscale % MILLION;
 		return IIO_VAL_INT_PLUS_MICRO;
 
+	case IIO_CHAN_INFO_OFFSET:
+		*val = DIV_ROUND_CLOSEST(MESON_SAR_ADC_TEMP_OFFSET *
+					 priv->param->temperature_divider,
+					 priv->param->temperature_multiplier);
+		*val -= priv->temperature_sensor_adc_val;
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_ENABLE:
+		*val = priv->temperature_sensor_calibrated;
+		return IIO_VAL_INT;
+
 	default:
 		return -EINVAL;
 	}
@@ -625,6 +708,77 @@ static int meson_sar_adc_clk_init(struct iio_dev *indio_dev,
 	return 0;
 }
 
+static int meson_sar_adc_temp_sensor_init(struct iio_dev *indio_dev)
+{
+	struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
+	u8 *buf, trimming_bits, trimming_mask, upper_adc_val;
+	struct nvmem_cell *temperature_calib;
+	size_t read_len;
+	int ret;
+
+	temperature_calib = devm_nvmem_cell_get(&indio_dev->dev,
+						"temperature_calib");
+	if (IS_ERR(temperature_calib)) {
+		ret = PTR_ERR(temperature_calib);
+
+		/*
+		 * leave the temperature sensor disabled if no calibration data
+		 * was passed via nvmem-cells.
+		 */
+		if (ret == -ENODEV)
+			return 0;
+
+		if (ret != -EPROBE_DEFER)
+			dev_err(indio_dev->dev.parent,
+				"failed to get temperature_calib cell\n");
+
+		return ret;
+	}
+
+	priv->tsc_regmap = syscon_regmap_lookup_by_phandle(
+						indio_dev->dev.parent->of_node,
+						"amlogic,hhi-sysctrl");
+	if (IS_ERR(priv->tsc_regmap)) {
+		dev_err(indio_dev->dev.parent,
+			"failed to get amlogic,hhi-sysctrl regmap\n");
+		return PTR_ERR(priv->tsc_regmap);
+	}
+
+	read_len = MESON_SAR_ADC_EFUSE_BYTES;
+	buf = nvmem_cell_read(temperature_calib, &read_len);
+	if (IS_ERR(buf)) {
+		dev_err(indio_dev->dev.parent,
+			"failed to read temperature_calib cell\n");
+		return PTR_ERR(buf);
+	} else if (read_len != MESON_SAR_ADC_EFUSE_BYTES) {
+		kfree(buf);
+		dev_err(indio_dev->dev.parent,
+			"invalid read size of temperature_calib cell\n");
+		return -EINVAL;
+	}
+
+	trimming_bits = priv->param->temperature_trimming_bits;
+	trimming_mask = BIT(trimming_bits) - 1;
+
+	if (buf[3] & MESON_SARADC_EFUSE_BYTE3_IS_CALIBRATED)
+		priv->temperature_sensor_calibrated = true;
+	else
+		priv->temperature_sensor_calibrated = false;
+
+	priv->temperature_sensor_coefficient = buf[2] & trimming_mask;
+
+	upper_adc_val = FIELD_GET(MESON_SARADC_EFUSE_BYTE3_UPPER_ADC_VAL,
+				  buf[3]);
+
+	priv->temperature_sensor_adc_val = buf[2];
+	priv->temperature_sensor_adc_val |= upper_adc_val << BITS_PER_BYTE;
+	priv->temperature_sensor_adc_val >>= trimming_bits;
+
+	kfree(buf);
+
+	return 0;
+}
+
 static int meson_sar_adc_init(struct iio_dev *indio_dev)
 {
 	struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
@@ -649,10 +803,12 @@ static int meson_sar_adc_init(struct iio_dev *indio_dev)
 
 	meson_sar_adc_stop_sample_engine(indio_dev);
 
-	/* update the channel 6 MUX to select the temperature sensor */
+	/*
+	 * disable this bit as seems to be only relevant for Meson6 (based
+	 * on the vendor driver), which we don't support at the moment.
+	 */
 	regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG0,
-			MESON_SAR_ADC_REG0_ADC_TEMP_SEN_SEL,
-			MESON_SAR_ADC_REG0_ADC_TEMP_SEN_SEL);
+			MESON_SAR_ADC_REG0_ADC_TEMP_SEN_SEL, 0);
 
 	/* disable all channels by default */
 	regmap_write(priv->regmap, MESON_SAR_ADC_CHAN_LIST, 0x0);
@@ -709,6 +865,45 @@ static int meson_sar_adc_init(struct iio_dev *indio_dev)
 	regval |= MESON_SAR_ADC_AUX_SW_XP_DRIVE_SW;
 	regmap_write(priv->regmap, MESON_SAR_ADC_AUX_SW, regval);
 
+	if (priv->temperature_sensor_calibrated) {
+		regmap_update_bits(priv->regmap, MESON_SAR_ADC_DELTA_10,
+				   MESON_SAR_ADC_DELTA_10_TS_REVE1,
+				   MESON_SAR_ADC_DELTA_10_TS_REVE1);
+		regmap_update_bits(priv->regmap, MESON_SAR_ADC_DELTA_10,
+				   MESON_SAR_ADC_DELTA_10_TS_REVE0,
+				   MESON_SAR_ADC_DELTA_10_TS_REVE0);
+
+		/*
+		 * set bits [3:0] of the TSC (temperature sensor coefficient)
+		 * to get the correct values when reading the temperature.
+		 */
+		regval = FIELD_PREP(MESON_SAR_ADC_DELTA_10_TS_C_MASK,
+				    priv->temperature_sensor_coefficient);
+		regmap_update_bits(priv->regmap, MESON_SAR_ADC_DELTA_10,
+				   MESON_SAR_ADC_DELTA_10_TS_C_MASK, regval);
+
+		if (priv->param->temperature_trimming_bits == 5) {
+			if (priv->temperature_sensor_coefficient & BIT(4))
+				regval = MESON_HHI_DPLL_TOP_0_TSC_BIT4;
+			else
+				regval = 0;
+
+			/*
+			 * the 5th bit (4th when starting to count at 0) of the
+			 * TSC is located in another register area.
+			 */
+			regmap_update_bits(priv->tsc_regmap,
+					   MESON_HHI_DPLL_TOP_0,
+					   MESON_HHI_DPLL_TOP_0_TSC_BIT4,
+					   regval);
+		}
+	} else {
+		regmap_update_bits(priv->regmap, MESON_SAR_ADC_DELTA_10,
+				   MESON_SAR_ADC_DELTA_10_TS_REVE1, 0);
+		regmap_update_bits(priv->regmap, MESON_SAR_ADC_DELTA_10,
+				   MESON_SAR_ADC_DELTA_10_TS_REVE0, 0);
+	}
+
 	ret = clk_set_parent(priv->adc_sel_clk, priv->clkin);
 	if (ret) {
 		dev_err(indio_dev->dev.parent,
@@ -894,6 +1089,24 @@ static const struct meson_sar_adc_param meson_sar_adc_meson8_param = {
 	.bandgap_reg = MESON_SAR_ADC_DELTA_10,
 	.regmap_config = &meson_sar_adc_regmap_config_meson8,
 	.resolution = 10,
+	.temperature_trimming_bits = 4,
+	.temperature_multiplier = 18 * 10000,
+	.temperature_divider = 1024 * 10 * 85,
+	.channels = meson_sar_adc_and_temp_iio_channels,
+	.num_channels = ARRAY_SIZE(meson_sar_adc_and_temp_iio_channels),
+};
+
+static const struct meson_sar_adc_param meson_sar_adc_meson8b_param = {
+	.has_bl30_integration = false,
+	.clock_rate = 1150000,
+	.bandgap_reg = MESON_SAR_ADC_DELTA_10,
+	.regmap_config = &meson_sar_adc_regmap_config_meson8,
+	.resolution = 10,
+	.temperature_trimming_bits = 5,
+	.temperature_multiplier = 10,
+	.temperature_divider = 32,
+	.channels = meson_sar_adc_and_temp_iio_channels,
+	.num_channels = ARRAY_SIZE(meson_sar_adc_and_temp_iio_channels),
 };
 
 static const struct meson_sar_adc_param meson_sar_adc_gxbb_param = {
@@ -902,6 +1115,8 @@ static const struct meson_sar_adc_param meson_sar_adc_gxbb_param = {
 	.bandgap_reg = MESON_SAR_ADC_REG11,
 	.regmap_config = &meson_sar_adc_regmap_config_gxbb,
 	.resolution = 10,
+	.channels = meson_sar_adc_iio_channels,
+	.num_channels = ARRAY_SIZE(meson_sar_adc_iio_channels),
 };
 
 static const struct meson_sar_adc_param meson_sar_adc_gxl_param = {
@@ -910,6 +1125,8 @@ static const struct meson_sar_adc_param meson_sar_adc_gxl_param = {
 	.bandgap_reg = MESON_SAR_ADC_REG11,
 	.regmap_config = &meson_sar_adc_regmap_config_gxbb,
 	.resolution = 12,
+	.channels = meson_sar_adc_iio_channels,
+	.num_channels = ARRAY_SIZE(meson_sar_adc_iio_channels),
 };
 
 static const struct meson_sar_adc_data meson_sar_adc_meson8_data = {
@@ -918,12 +1135,12 @@ static const struct meson_sar_adc_data meson_sar_adc_meson8_data = {
 };
 
 static const struct meson_sar_adc_data meson_sar_adc_meson8b_data = {
-	.param = &meson_sar_adc_meson8_param,
+	.param = &meson_sar_adc_meson8b_param,
 	.name = "meson-meson8b-saradc",
 };
 
 static const struct meson_sar_adc_data meson_sar_adc_meson8m2_data = {
-	.param = &meson_sar_adc_meson8_param,
+	.param = &meson_sar_adc_meson8b_param,
 	.name = "meson-meson8m2-saradc",
 };
 
@@ -1008,9 +1225,8 @@ static int meson_sar_adc_probe(struct platform_device *pdev)
 	indio_dev->dev.of_node = pdev->dev.of_node;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &meson_sar_adc_iio_info;
-
-	indio_dev->channels = meson_sar_adc_iio_channels;
-	indio_dev->num_channels = ARRAY_SIZE(meson_sar_adc_iio_channels);
+	indio_dev->channels = priv->param->channels;
+	indio_dev->num_channels = priv->param->num_channels;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	base = devm_ioremap_resource(&pdev->dev, res);
@@ -1078,6 +1294,12 @@ static int meson_sar_adc_probe(struct platform_device *pdev)
 
 	priv->calibscale = MILLION;
 
+	if (priv->param->temperature_trimming_bits) {
+		ret = meson_sar_adc_temp_sensor_init(indio_dev);
+		if (ret)
+			return ret;
+	}
+
 	ret = meson_sar_adc_init(indio_dev);
 	if (ret)
 		goto err;
-- 
2.19.1

[PATCH 3/7] ARM: dts: meson8: add the temperature calibration data for the SAR ADC

[Martin Blumenstingl]

The SAR ADC can measure the chip temperature of the SoC. This only
works if the chip is calibrated and if the calibration data is written
to the correct registers. The calibration data is stored in the upper
two bytes of eFuse offset 0x1f4.

This adds the eFuse cell for the temperature calibration data and
passes it to the SAR ADC. We also need to pass the HHI sysctrl node to
the SAR ADC because the 4th TSC (temperature sensor calibration
coefficient) bit is stored in the HHI region (unlike bits [3:0] which
are stored directly inside the SAR ADC's register area).

On boards that have the SAR ADC enabled channel 8 can be used to
measure the chip temperature.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 arch/arm/boot/dts/meson8.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/meson8.dtsi b/arch/arm/boot/dts/meson8.dtsi
index 3e3d9c54cddc..30d4406f63b3 100644
--- a/arch/arm/boot/dts/meson8.dtsi
+++ b/arch/arm/boot/dts/meson8.dtsi
@@ -304,6 +304,11 @@
 	compatible = "amlogic,meson8-efuse";
 	clocks = <&clkc CLKID_EFUSE>;
 	clock-names = "core";
+
+	temperature_calib: calib@1f4 {
+		/* only the upper two bytes are relevant */
+		reg = <0x1f4 0x4>;
+	};
 };
 
 &ethmac {
@@ -364,6 +369,9 @@
 	clocks = <&clkc CLKID_XTAL>,
 		<&clkc CLKID_SAR_ADC>;
 	clock-names = "clkin", "core";
+	amlogic,hhi-sysctrl = <&hhi>;
+	nvmem-cells = <&temperature_calib>;
+	nvmem-cell-names = "temperature_calib";
 };
 
 &sdio {
-- 
2.19.1

[PATCH 3/7] ARM: dts: meson8: add the temperature calibration data for the SAR ADC

[Martin Blumenstingl]

The SAR ADC can measure the chip temperature of the SoC. This only
works if the chip is calibrated and if the calibration data is written
to the correct registers. The calibration data is stored in the upper
two bytes of eFuse offset 0x1f4.

This adds the eFuse cell for the temperature calibration data and
passes it to the SAR ADC. We also need to pass the HHI sysctrl node to
the SAR ADC because the 4th TSC (temperature sensor calibration
coefficient) bit is stored in the HHI region (unlike bits [3:0] which
are stored directly inside the SAR ADC's register area).

On boards that have the SAR ADC enabled channel 8 can be used to
measure the chip temperature.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 arch/arm/boot/dts/meson8.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/meson8.dtsi b/arch/arm/boot/dts/meson8.dtsi
index 3e3d9c54cddc..30d4406f63b3 100644
--- a/arch/arm/boot/dts/meson8.dtsi
+++ b/arch/arm/boot/dts/meson8.dtsi
@@ -304,6 +304,11 @@
 	compatible = "amlogic,meson8-efuse";
 	clocks = <&clkc CLKID_EFUSE>;
 	clock-names = "core";
+
+	temperature_calib: calib at 1f4 {
+		/* only the upper two bytes are relevant */
+		reg = <0x1f4 0x4>;
+	};
 };
 
 &ethmac {
@@ -364,6 +369,9 @@
 	clocks = <&clkc CLKID_XTAL>,
 		<&clkc CLKID_SAR_ADC>;
 	clock-names = "clkin", "core";
+	amlogic,hhi-sysctrl = <&hhi>;
+	nvmem-cells = <&temperature_calib>;
+	nvmem-cell-names = "temperature_calib";
 };
 
 &sdio {
-- 
2.19.1

[PATCH 4/7] ARM: dts: meson8b: add the temperature calibration data for the SAR ADC

[Martin Blumenstingl]

The SAR ADC can measure the chip temperature of the SoC. This only
works if the chip is calibrated and if the calibration data is written
to the correct registers. The calibration data is stored in the upper
two bytes of eFuse offset 0x1f4.

This adds the eFuse cell for the temperature calibration data and
passes it to the SAR ADC. We also need to pass the HHI sysctrl node to
the SAR ADC because the 4th TSC (temperature sensor calibration
coefficient) bit is stored in the HHI region (unlike bits [3:0] which
are stored directly inside the SAR ADC's register area).

On boards that have the SAR ADC enabled channel 8 can be used to
measure the chip temperature.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 arch/arm/boot/dts/meson8b.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/meson8b.dtsi b/arch/arm/boot/dts/meson8b.dtsi
index f92aab0aa247..dd47af174c4d 100644
--- a/arch/arm/boot/dts/meson8b.dtsi
+++ b/arch/arm/boot/dts/meson8b.dtsi
@@ -283,6 +283,11 @@
 	compatible = "amlogic,meson8b-efuse";
 	clocks = <&clkc CLKID_EFUSE>;
 	clock-names = "core";
+
+	temperature_calib: calib@1f4 {
+		/* only the upper two bytes are relevant */
+		reg = <0x1f4 0x4>;
+	};
 };
 
 &ethmac {
@@ -354,6 +359,9 @@
 	clocks = <&clkc CLKID_XTAL>,
 		<&clkc CLKID_SAR_ADC>;
 	clock-names = "clkin", "core";
+	amlogic,hhi-sysctrl = <&hhi>;
+	nvmem-cells = <&temperature_calib>;
+	nvmem-cell-names = "temperature_calib";
 };
 
 &sdio {
-- 
2.19.1

[PATCH 4/7] ARM: dts: meson8b: add the temperature calibration data for the SAR ADC

[Martin Blumenstingl]

The SAR ADC can measure the chip temperature of the SoC. This only
works if the chip is calibrated and if the calibration data is written
to the correct registers. The calibration data is stored in the upper
two bytes of eFuse offset 0x1f4.

This adds the eFuse cell for the temperature calibration data and
passes it to the SAR ADC. We also need to pass the HHI sysctrl node to
the SAR ADC because the 4th TSC (temperature sensor calibration
coefficient) bit is stored in the HHI region (unlike bits [3:0] which
are stored directly inside the SAR ADC's register area).

On boards that have the SAR ADC enabled channel 8 can be used to
measure the chip temperature.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 arch/arm/boot/dts/meson8b.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/meson8b.dtsi b/arch/arm/boot/dts/meson8b.dtsi
index f92aab0aa247..dd47af174c4d 100644
--- a/arch/arm/boot/dts/meson8b.dtsi
+++ b/arch/arm/boot/dts/meson8b.dtsi
@@ -283,6 +283,11 @@
 	compatible = "amlogic,meson8b-efuse";
 	clocks = <&clkc CLKID_EFUSE>;
 	clock-names = "core";
+
+	temperature_calib: calib at 1f4 {
+		/* only the upper two bytes are relevant */
+		reg = <0x1f4 0x4>;
+	};
 };
 
 &ethmac {
@@ -354,6 +359,9 @@
 	clocks = <&clkc CLKID_XTAL>,
 		<&clkc CLKID_SAR_ADC>;
 	clock-names = "clkin", "core";
+	amlogic,hhi-sysctrl = <&hhi>;
+	nvmem-cells = <&temperature_calib>;
+	nvmem-cell-names = "temperature_calib";
 };
 
 &sdio {
-- 
2.19.1

[PATCH 5/7] ARM: dts: meson8b: ec100: add iio-hwmon for the chip temperature

[Martin Blumenstingl]

SAR ADC enabled channel 8 can be used to measure the chip temperature.
This can be made available to the hwmon subsystem by using iio-hwmon.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 arch/arm/boot/dts/meson8b-ec100.dts | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/meson8b-ec100.dts b/arch/arm/boot/dts/meson8b-ec100.dts
index 0872f6e3abf5..0cebe849a920 100644
--- a/arch/arm/boot/dts/meson8b-ec100.dts
+++ b/arch/arm/boot/dts/meson8b-ec100.dts
@@ -64,6 +64,11 @@
 		timeout-ms = <20000>;
 	};
 
+	iio-hwmon {
+		compatible = "iio-hwmon";
+		io-channels = <&saradc 8>;
+	};
+
 	leds {
 		compatible = "gpio-leds";
 
-- 
2.19.1

[PATCH 5/7] ARM: dts: meson8b: ec100: add iio-hwmon for the chip temperature

[Martin Blumenstingl]

SAR ADC enabled channel 8 can be used to measure the chip temperature.
This can be made available to the hwmon subsystem by using iio-hwmon.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 arch/arm/boot/dts/meson8b-ec100.dts | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/meson8b-ec100.dts b/arch/arm/boot/dts/meson8b-ec100.dts
index 0872f6e3abf5..0cebe849a920 100644
--- a/arch/arm/boot/dts/meson8b-ec100.dts
+++ b/arch/arm/boot/dts/meson8b-ec100.dts
@@ -64,6 +64,11 @@
 		timeout-ms = <20000>;
 	};
 
+	iio-hwmon {
+		compatible = "iio-hwmon";
+		io-channels = <&saradc 8>;
+	};
+
 	leds {
 		compatible = "gpio-leds";
 
-- 
2.19.1

[PATCH 6/7] ARM: dts: meson8b: odroidc1: add iio-hwmon for the chip temperature

[Martin Blumenstingl]

SAR ADC enabled channel 8 can be used to measure the chip temperature.
This can be made available to the hwmon subsystem by using iio-hwmon.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 arch/arm/boot/dts/meson8b-odroidc1.dts | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/meson8b-odroidc1.dts b/arch/arm/boot/dts/meson8b-odroidc1.dts
index 58669abda259..9251dc102fcc 100644
--- a/arch/arm/boot/dts/meson8b-odroidc1.dts
+++ b/arch/arm/boot/dts/meson8b-odroidc1.dts
@@ -118,6 +118,11 @@
 			  1800000 1>;
 	};
 
+	iio-hwmon {
+		compatible = "iio-hwmon";
+		io-channels = <&saradc 8>;
+	};
+
 	vcc_1v8: regulator-vcc-1v8 {
 		/*
 		 * RICHTEK RT9179 configured for a fixed output voltage of
-- 
2.19.1

[PATCH 6/7] ARM: dts: meson8b: odroidc1: add iio-hwmon for the chip temperature

[Martin Blumenstingl]

SAR ADC enabled channel 8 can be used to measure the chip temperature.
This can be made available to the hwmon subsystem by using iio-hwmon.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 arch/arm/boot/dts/meson8b-odroidc1.dts | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/meson8b-odroidc1.dts b/arch/arm/boot/dts/meson8b-odroidc1.dts
index 58669abda259..9251dc102fcc 100644
--- a/arch/arm/boot/dts/meson8b-odroidc1.dts
+++ b/arch/arm/boot/dts/meson8b-odroidc1.dts
@@ -118,6 +118,11 @@
 			  1800000 1>;
 	};
 
+	iio-hwmon {
+		compatible = "iio-hwmon";
+		io-channels = <&saradc 8>;
+	};
+
 	vcc_1v8: regulator-vcc-1v8 {
 		/*
 		 * RICHTEK RT9179 configured for a fixed output voltage of
-- 
2.19.1

[PATCH 7/7] ARM: dts: meson8m2: mxiii-plus: add iio-hwmon for the chip temperature

[Martin Blumenstingl]

SAR ADC enabled channel 8 can be used to measure the chip temperature.
This can be made available to the hwmon subsystem by using iio-hwmon.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 arch/arm/boot/dts/meson8m2-mxiii-plus.dts | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/meson8m2-mxiii-plus.dts b/arch/arm/boot/dts/meson8m2-mxiii-plus.dts
index f5853610b20b..d1d3c828c039 100644
--- a/arch/arm/boot/dts/meson8m2-mxiii-plus.dts
+++ b/arch/arm/boot/dts/meson8m2-mxiii-plus.dts
@@ -44,6 +44,11 @@
 		};
 	};
 
+	iio-hwmon {
+		compatible = "iio-hwmon";
+		io-channels = <&saradc 8>;
+	};
+
 	vcc_3v3: regulator-vcc3v3 {
 		compatible = "regulator-fixed";
 		regulator-name = "VCC3V3";
-- 
2.19.1

[PATCH 7/7] ARM: dts: meson8m2: mxiii-plus: add iio-hwmon for the chip temperature

[Martin Blumenstingl]

SAR ADC enabled channel 8 can be used to measure the chip temperature.
This can be made available to the hwmon subsystem by using iio-hwmon.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 arch/arm/boot/dts/meson8m2-mxiii-plus.dts | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/meson8m2-mxiii-plus.dts b/arch/arm/boot/dts/meson8m2-mxiii-plus.dts
index f5853610b20b..d1d3c828c039 100644
--- a/arch/arm/boot/dts/meson8m2-mxiii-plus.dts
+++ b/arch/arm/boot/dts/meson8m2-mxiii-plus.dts
@@ -44,6 +44,11 @@
 		};
 	};
 
+	iio-hwmon {
+		compatible = "iio-hwmon";
+		io-channels = <&saradc 8>;
+	};
+
 	vcc_3v3: regulator-vcc3v3 {
 		compatible = "regulator-fixed";
 		regulator-name = "VCC3V3";
-- 
2.19.1

Re: [PATCH 2/7] iio: adc: meson-saradc: add support for the chip's temperature sensor

[Peter Meerwald-Stadler]

On Sun, 28 Oct 2018, Martin Blumenstingl wrote:

comments below

> Channel 6 of the SAR ADC can be switched between two inputs:
> SAR_ADC_CH6 input (an actual pad on the SoC) and the signal from the
> temperature sensor inside the SoC.
> 
> To get usable results from the temperature sensor we need to read the
> corresponding calibration data from the eFuse and pass it to the SAR ADC
> (bits [3:0]) and (on Meson8b and Meson8m2) to a scratch register in the
> HHI region.
> If the temperature sensor is not calibrated (the eFuse data contains a
> bit for this) then the driver will return -ENOTSUPP when trying to read
> the temperature.
> 
> This only enables the temperature sensor for the Meson8, Meson8b and
> Meson8m2 SoCs because on the newer, 64-bit SoCs (GXBB, GXL and GXM) the
> temperature sensor inside SAR ADC is firmware-controlled (by BL30, we
> can simply use the SCPI hwmon driver to get the chip temperature).
> 
> To keep the devicetree interface backwards compatible we simply skip the
> temperature sensor initialization if no eFuse nvmem cell is passed via
> devicetree.
> 
> The public documentation for the SAR ADC IP block does not explain how
> to use the registers to read the temperature. The logic from this patch
> is based on reading and understanding Amlogic's GPL kernel sources.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/iio/adc/meson_saradc.c | 274 +++++++++++++++++++++++++++++----
>  1 file changed, 248 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
> index 028ccd218f82..df1e45805c23 100644
> --- a/drivers/iio/adc/meson_saradc.c
> +++ b/drivers/iio/adc/meson_saradc.c
> @@ -18,6 +18,7 @@
>  #include <linux/io.h>
>  #include <linux/iio/iio.h>
>  #include <linux/module.h>
> +#include <linux/nvmem-consumer.h>
>  #include <linux/interrupt.h>
>  #include <linux/of.h>
>  #include <linux/of_irq.h>
> @@ -25,6 +26,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/mfd/syscon.h>
>  
>  #define MESON_SAR_ADC_REG0					0x00
>  	#define MESON_SAR_ADC_REG0_PANEL_DETECT			BIT(31)
> @@ -165,6 +167,17 @@
>  
>  #define MESON_SAR_ADC_MAX_FIFO_SIZE				32
>  #define MESON_SAR_ADC_TIMEOUT					100 /* ms */
> +#define MESON_SAR_ADC_VOLTAGE_AND_TEMP_CHANNEL			6
> +#define MESON_SAR_ADC_TEMP_OFFSET				27
> +
> +/* temperature sensor calibration information in eFuse */
> +#define MESON_SAR_ADC_EFUSE_BYTES				4
> +#define MESON_SARADC_EFUSE_BYTE3_UPPER_ADC_VAL			GENMASK(6, 0)
> +#define MESON_SARADC_EFUSE_BYTE3_IS_CALIBRATED			BIT(7)

MESON_SAR_ADC_ prefix for consistency?

> +
> +#define MESON_HHI_DPLL_TOP_0					0x318
> +#define MESON_HHI_DPLL_TOP_0_TSC_BIT4				BIT(9)

I guess these do not belog to the SAR ADC at all?

> +
>  /* for use with IIO_VAL_INT_PLUS_MICRO */
>  #define MILLION							1000000
>  
> @@ -175,16 +188,27 @@
>  	.address = _chan,						\
>  	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
>  				BIT(IIO_CHAN_INFO_AVERAGE_RAW),		\
> -	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |		\
> -				BIT(IIO_CHAN_INFO_CALIBBIAS) |		\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),		\
> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_CALIBBIAS) |	\
>  				BIT(IIO_CHAN_INFO_CALIBSCALE),		\
>  	.datasheet_name = "SAR_ADC_CH"#_chan,				\
>  }
>  
> -/*
> - * TODO: the hardware supports IIO_TEMP for channel 6 as well which is
> - * currently not supported by this driver.
> - */
> +#define MESON_SAR_ADC_TEMP_CHAN(_chan) {				\
> +	.type = IIO_TEMP,						\
> +	.indexed = 0,							\

.indexed = 0 not needed

> +	.channel = _chan,						\
> +	.address = MESON_SAR_ADC_VOLTAGE_AND_TEMP_CHANNEL,		\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
> +				BIT(IIO_CHAN_INFO_AVERAGE_RAW),		\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET) |		\
> +					BIT(IIO_CHAN_INFO_SCALE) |	\
> +					BIT(IIO_CHAN_INFO_ENABLE),	\
> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_CALIBBIAS) |	\
> +				BIT(IIO_CHAN_INFO_CALIBSCALE),		\
> +	.datasheet_name = "TEMP_SENSOR",				\
> +}
> +
>  static const struct iio_chan_spec meson_sar_adc_iio_channels[] = {
>  	MESON_SAR_ADC_CHAN(0),
>  	MESON_SAR_ADC_CHAN(1),
> @@ -197,6 +221,19 @@ static const struct iio_chan_spec meson_sar_adc_iio_channels[] = {
>  	IIO_CHAN_SOFT_TIMESTAMP(8),
>  };
>  
> +static const struct iio_chan_spec meson_sar_adc_and_temp_iio_channels[] = {
> +	MESON_SAR_ADC_CHAN(0),
> +	MESON_SAR_ADC_CHAN(1),
> +	MESON_SAR_ADC_CHAN(2),
> +	MESON_SAR_ADC_CHAN(3),
> +	MESON_SAR_ADC_CHAN(4),
> +	MESON_SAR_ADC_CHAN(5),
> +	MESON_SAR_ADC_CHAN(6),
> +	MESON_SAR_ADC_CHAN(7),
> +	MESON_SAR_ADC_TEMP_CHAN(8),
> +	IIO_CHAN_SOFT_TIMESTAMP(9),
> +};
> +
>  enum meson_sar_adc_avg_mode {
>  	NO_AVERAGING = 0x0,
>  	MEAN_AVERAGING = 0x1,
> @@ -225,6 +262,11 @@ struct meson_sar_adc_param {
>  	u32					bandgap_reg;
>  	unsigned int				resolution;
>  	const struct regmap_config		*regmap_config;
> +	u8					temperature_trimming_bits;
> +	unsigned int				temperature_multiplier;
> +	unsigned int				temperature_divider;
> +	const struct iio_chan_spec		*channels;
> +	unsigned int				num_channels;
>  };
>  
>  struct meson_sar_adc_data {
> @@ -246,6 +288,10 @@ struct meson_sar_adc_priv {
>  	struct completion			done;
>  	int					calibbias;
>  	int					calibscale;
> +	struct regmap				*tsc_regmap;
> +	bool					temperature_sensor_calibrated;
> +	u8					temperature_sensor_coefficient;
> +	u16					temperature_sensor_adc_val;
>  };
>  
>  static const struct regmap_config meson_sar_adc_regmap_config_gxbb = {
> @@ -389,9 +435,17 @@ static void meson_sar_adc_enable_channel(struct iio_dev *indio_dev,
>  			   MESON_SAR_ADC_DETECT_IDLE_SW_IDLE_MUX_SEL_MASK,
>  			   regval);
>  
> -	if (chan->address == 6)
> -		regmap_update_bits(priv->regmap, MESON_SAR_ADC_DELTA_10,
> -				   MESON_SAR_ADC_DELTA_10_TEMP_SEL, 0);
> +	if (chan->address == MESON_SAR_ADC_VOLTAGE_AND_TEMP_CHANNEL) {

quite repetitive, maybe (just a suggestion)
		u8 val = chan->type == IIO_TEMP ? MESON_SAR_ADC_DELTA_10_TEMP_SEL : 0;
		regmap_update_bits(priv->regmap, MESON_SAR_ADC_DELTA_10,
				   MESON_SAR_ADC_DELTA_10_TEMP_SEL, val);

> +		if (chan->type == IIO_TEMP)
> +			regmap_update_bits(priv->regmap,
> +					   MESON_SAR_ADC_DELTA_10,
> +					   MESON_SAR_ADC_DELTA_10_TEMP_SEL,
> +					   MESON_SAR_ADC_DELTA_10_TEMP_SEL);
> +		else
> +			regmap_update_bits(priv->regmap,
> +					   MESON_SAR_ADC_DELTA_10,
> +					   MESON_SAR_ADC_DELTA_10_TEMP_SEL, 0);
> +	}
>  }
>  
>  static void meson_sar_adc_set_chan7_mux(struct iio_dev *indio_dev,
> @@ -506,8 +560,12 @@ static int meson_sar_adc_get_sample(struct iio_dev *indio_dev,
>  				    enum meson_sar_adc_num_samples avg_samples,
>  				    int *val)
>  {
> +	struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
>  	int ret;
>  
> +	if (chan->type == IIO_TEMP && !priv->temperature_sensor_calibrated)
> +		return -ENOTSUPP;
> +
>  	ret = meson_sar_adc_lock(indio_dev);
>  	if (ret)
>  		return ret;
> @@ -555,17 +613,31 @@ static int meson_sar_adc_iio_info_read_raw(struct iio_dev *indio_dev,
>  		break;
>  
>  	case IIO_CHAN_INFO_SCALE:
> -		ret = regulator_get_voltage(priv->vref);
> -		if (ret < 0) {
> -			dev_err(indio_dev->dev.parent,
> -				"failed to get vref voltage: %d\n", ret);
> -			return ret;
> +		if (chan->type == IIO_VOLTAGE) {
> +			ret = regulator_get_voltage(priv->vref);
> +			if (ret < 0) {
> +				dev_err(indio_dev->dev.parent,
> +					"failed to get vref voltage: %d\n",
> +					ret);
> +				return ret;
> +			}
> +
> +			*val = ret / 1000;
> +			*val2 = priv->param->resolution;
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		} else if (chan->type == IIO_TEMP) {
> +			/* SoC specific multiplier and divider */
> +			*val = priv->param->temperature_multiplier;
> +			*val2 = priv->param->temperature_divider;
> +
> +			/* celsius to millicelsius */
> +			*val *= 1000;
> +
> +			return IIO_VAL_FRACTIONAL;
> +		} else {
> +			return -EINVAL;
>  		}
>  
> -		*val = ret / 1000;
> -		*val2 = priv->param->resolution;
> -		return IIO_VAL_FRACTIONAL_LOG2;
> -
>  	case IIO_CHAN_INFO_CALIBBIAS:
>  		*val = priv->calibbias;
>  		return IIO_VAL_INT;
> @@ -575,6 +647,17 @@ static int meson_sar_adc_iio_info_read_raw(struct iio_dev *indio_dev,
>  		*val2 = priv->calibscale % MILLION;
>  		return IIO_VAL_INT_PLUS_MICRO;
>  
> +	case IIO_CHAN_INFO_OFFSET:
> +		*val = DIV_ROUND_CLOSEST(MESON_SAR_ADC_TEMP_OFFSET *
> +					 priv->param->temperature_divider,
> +					 priv->param->temperature_multiplier);
> +		*val -= priv->temperature_sensor_adc_val;
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_ENABLE:
> +		*val = priv->temperature_sensor_calibrated;
> +		return IIO_VAL_INT;
> +
>  	default:
>  		return -EINVAL;
>  	}
> @@ -625,6 +708,77 @@ static int meson_sar_adc_clk_init(struct iio_dev *indio_dev,
>  	return 0;
>  }
>  
> +static int meson_sar_adc_temp_sensor_init(struct iio_dev *indio_dev)
> +{
> +	struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
> +	u8 *buf, trimming_bits, trimming_mask, upper_adc_val;
> +	struct nvmem_cell *temperature_calib;
> +	size_t read_len;
> +	int ret;
> +
> +	temperature_calib = devm_nvmem_cell_get(&indio_dev->dev,
> +						"temperature_calib");
> +	if (IS_ERR(temperature_calib)) {
> +		ret = PTR_ERR(temperature_calib);
> +
> +		/*
> +		 * leave the temperature sensor disabled if no calibration data
> +		 * was passed via nvmem-cells.
> +		 */
> +		if (ret == -ENODEV)
> +			return 0;
> +
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(indio_dev->dev.parent,
> +				"failed to get temperature_calib cell\n");
> +
> +		return ret;
> +	}
> +
> +	priv->tsc_regmap = syscon_regmap_lookup_by_phandle(
> +						indio_dev->dev.parent->of_node,
> +						"amlogic,hhi-sysctrl");
> +	if (IS_ERR(priv->tsc_regmap)) {
> +		dev_err(indio_dev->dev.parent,
> +			"failed to get amlogic,hhi-sysctrl regmap\n");
> +		return PTR_ERR(priv->tsc_regmap);
> +	}
> +
> +	read_len = MESON_SAR_ADC_EFUSE_BYTES;
> +	buf = nvmem_cell_read(temperature_calib, &read_len);
> +	if (IS_ERR(buf)) {
> +		dev_err(indio_dev->dev.parent,
> +			"failed to read temperature_calib cell\n");
> +		return PTR_ERR(buf);
> +	} else if (read_len != MESON_SAR_ADC_EFUSE_BYTES) {
> +		kfree(buf);
> +		dev_err(indio_dev->dev.parent,
> +			"invalid read size of temperature_calib cell\n");
> +		return -EINVAL;
> +	}
> +
> +	trimming_bits = priv->param->temperature_trimming_bits;
> +	trimming_mask = BIT(trimming_bits) - 1;
> +
> +	if (buf[3] & MESON_SARADC_EFUSE_BYTE3_IS_CALIBRATED)
> +		priv->temperature_sensor_calibrated = true;
> +	else
> +		priv->temperature_sensor_calibrated = false;
> +
> +	priv->temperature_sensor_coefficient = buf[2] & trimming_mask;
> +
> +	upper_adc_val = FIELD_GET(MESON_SARADC_EFUSE_BYTE3_UPPER_ADC_VAL,
> +				  buf[3]);
> +
> +	priv->temperature_sensor_adc_val = buf[2];
> +	priv->temperature_sensor_adc_val |= upper_adc_val << BITS_PER_BYTE;
> +	priv->temperature_sensor_adc_val >>= trimming_bits;
> +
> +	kfree(buf);
> +
> +	return 0;
> +}
> +
>  static int meson_sar_adc_init(struct iio_dev *indio_dev)
>  {
>  	struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
> @@ -649,10 +803,12 @@ static int meson_sar_adc_init(struct iio_dev *indio_dev)
>  
>  	meson_sar_adc_stop_sample_engine(indio_dev);
>  
> -	/* update the channel 6 MUX to select the temperature sensor */
> +	/*
> +	 * disable this bit as seems to be only relevant for Meson6 (based
> +	 * on the vendor driver), which we don't support at the moment.
> +	 */
>  	regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG0,
> -			MESON_SAR_ADC_REG0_ADC_TEMP_SEN_SEL,
> -			MESON_SAR_ADC_REG0_ADC_TEMP_SEN_SEL);
> +			MESON_SAR_ADC_REG0_ADC_TEMP_SEN_SEL, 0);
>  
>  	/* disable all channels by default */
>  	regmap_write(priv->regmap, MESON_SAR_ADC_CHAN_LIST, 0x0);
> @@ -709,6 +865,45 @@ static int meson_sar_adc_init(struct iio_dev *indio_dev)
>  	regval |= MESON_SAR_ADC_AUX_SW_XP_DRIVE_SW;
>  	regmap_write(priv->regmap, MESON_SAR_ADC_AUX_SW, regval);
>  
> +	if (priv->temperature_sensor_calibrated) {
> +		regmap_update_bits(priv->regmap, MESON_SAR_ADC_DELTA_10,
> +				   MESON_SAR_ADC_DELTA_10_TS_REVE1,
> +				   MESON_SAR_ADC_DELTA_10_TS_REVE1);
> +		regmap_update_bits(priv->regmap, MESON_SAR_ADC_DELTA_10,
> +				   MESON_SAR_ADC_DELTA_10_TS_REVE0,
> +				   MESON_SAR_ADC_DELTA_10_TS_REVE0);
> +
> +		/*
> +		 * set bits [3:0] of the TSC (temperature sensor coefficient)
> +		 * to get the correct values when reading the temperature.
> +		 */
> +		regval = FIELD_PREP(MESON_SAR_ADC_DELTA_10_TS_C_MASK,
> +				    priv->temperature_sensor_coefficient);
> +		regmap_update_bits(priv->regmap, MESON_SAR_ADC_DELTA_10,
> +				   MESON_SAR_ADC_DELTA_10_TS_C_MASK, regval);
> +
> +		if (priv->param->temperature_trimming_bits == 5) {
> +			if (priv->temperature_sensor_coefficient & BIT(4))
> +				regval = MESON_HHI_DPLL_TOP_0_TSC_BIT4;
> +			else
> +				regval = 0;
> +
> +			/*
> +			 * the 5th bit (4th when starting to count at 0) of the
> +			 * TSC is located in another register area.
> +			 */
> +			regmap_update_bits(priv->tsc_regmap,
> +					   MESON_HHI_DPLL_TOP_0,
> +					   MESON_HHI_DPLL_TOP_0_TSC_BIT4,
> +					   regval);
> +		}
> +	} else {
> +		regmap_update_bits(priv->regmap, MESON_SAR_ADC_DELTA_10,
> +				   MESON_SAR_ADC_DELTA_10_TS_REVE1, 0);
> +		regmap_update_bits(priv->regmap, MESON_SAR_ADC_DELTA_10,
> +				   MESON_SAR_ADC_DELTA_10_TS_REVE0, 0);
> +	}
> +
>  	ret = clk_set_parent(priv->adc_sel_clk, priv->clkin);
>  	if (ret) {
>  		dev_err(indio_dev->dev.parent,
> @@ -894,6 +1089,24 @@ static const struct meson_sar_adc_param meson_sar_adc_meson8_param = {
>  	.bandgap_reg = MESON_SAR_ADC_DELTA_10,
>  	.regmap_config = &meson_sar_adc_regmap_config_meson8,
>  	.resolution = 10,
> +	.temperature_trimming_bits = 4,
> +	.temperature_multiplier = 18 * 10000,
> +	.temperature_divider = 1024 * 10 * 85,
> +	.channels = meson_sar_adc_and_temp_iio_channels,
> +	.num_channels = ARRAY_SIZE(meson_sar_adc_and_temp_iio_channels),
> +};
> +
> +static const struct meson_sar_adc_param meson_sar_adc_meson8b_param = {
> +	.has_bl30_integration = false,
> +	.clock_rate = 1150000,
> +	.bandgap_reg = MESON_SAR_ADC_DELTA_10,
> +	.regmap_config = &meson_sar_adc_regmap_config_meson8,
> +	.resolution = 10,
> +	.temperature_trimming_bits = 5,
> +	.temperature_multiplier = 10,
> +	.temperature_divider = 32,
> +	.channels = meson_sar_adc_and_temp_iio_channels,
> +	.num_channels = ARRAY_SIZE(meson_sar_adc_and_temp_iio_channels),
>  };
>  
>  static const struct meson_sar_adc_param meson_sar_adc_gxbb_param = {
> @@ -902,6 +1115,8 @@ static const struct meson_sar_adc_param meson_sar_adc_gxbb_param = {
>  	.bandgap_reg = MESON_SAR_ADC_REG11,
>  	.regmap_config = &meson_sar_adc_regmap_config_gxbb,
>  	.resolution = 10,
> +	.channels = meson_sar_adc_iio_channels,
> +	.num_channels = ARRAY_SIZE(meson_sar_adc_iio_channels),
>  };
>  
>  static const struct meson_sar_adc_param meson_sar_adc_gxl_param = {
> @@ -910,6 +1125,8 @@ static const struct meson_sar_adc_param meson_sar_adc_gxl_param = {
>  	.bandgap_reg = MESON_SAR_ADC_REG11,
>  	.regmap_config = &meson_sar_adc_regmap_config_gxbb,
>  	.resolution = 12,
> +	.channels = meson_sar_adc_iio_channels,
> +	.num_channels = ARRAY_SIZE(meson_sar_adc_iio_channels),
>  };
>  
>  static const struct meson_sar_adc_data meson_sar_adc_meson8_data = {
> @@ -918,12 +1135,12 @@ static const struct meson_sar_adc_data meson_sar_adc_meson8_data = {
>  };
>  
>  static const struct meson_sar_adc_data meson_sar_adc_meson8b_data = {
> -	.param = &meson_sar_adc_meson8_param,
> +	.param = &meson_sar_adc_meson8b_param,
>  	.name = "meson-meson8b-saradc",
>  };
>  
>  static const struct meson_sar_adc_data meson_sar_adc_meson8m2_data = {
> -	.param = &meson_sar_adc_meson8_param,
> +	.param = &meson_sar_adc_meson8b_param,
>  	.name = "meson-meson8m2-saradc",
>  };
>  
> @@ -1008,9 +1225,8 @@ static int meson_sar_adc_probe(struct platform_device *pdev)
>  	indio_dev->dev.of_node = pdev->dev.of_node;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->info = &meson_sar_adc_iio_info;
> -
> -	indio_dev->channels = meson_sar_adc_iio_channels;
> -	indio_dev->num_channels = ARRAY_SIZE(meson_sar_adc_iio_channels);
> +	indio_dev->channels = priv->param->channels;
> +	indio_dev->num_channels = priv->param->num_channels;
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	base = devm_ioremap_resource(&pdev->dev, res);
> @@ -1078,6 +1294,12 @@ static int meson_sar_adc_probe(struct platform_device *pdev)
>  
>  	priv->calibscale = MILLION;
>  
> +	if (priv->param->temperature_trimming_bits) {
> +		ret = meson_sar_adc_temp_sensor_init(indio_dev);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	ret = meson_sar_adc_init(indio_dev);
>  	if (ret)
>  		goto err;
> 

-- 

Peter Meerwald-Stadler
Mobile: +43 664 24 44 418

[PATCH 2/7] iio: adc: meson-saradc: add support for the chip's temperature sensor

[Peter Meerwald-Stadler]

On Sun, 28 Oct 2018, Martin Blumenstingl wrote:

comments below

> Channel 6 of the SAR ADC can be switched between two inputs:
> SAR_ADC_CH6 input (an actual pad on the SoC) and the signal from the
> temperature sensor inside the SoC.
> 
> To get usable results from the temperature sensor we need to read the
> corresponding calibration data from the eFuse and pass it to the SAR ADC
> (bits [3:0]) and (on Meson8b and Meson8m2) to a scratch register in the
> HHI region.
> If the temperature sensor is not calibrated (the eFuse data contains a
> bit for this) then the driver will return -ENOTSUPP when trying to read
> the temperature.
> 
> This only enables the temperature sensor for the Meson8, Meson8b and
> Meson8m2 SoCs because on the newer, 64-bit SoCs (GXBB, GXL and GXM) the
> temperature sensor inside SAR ADC is firmware-controlled (by BL30, we
> can simply use the SCPI hwmon driver to get the chip temperature).
> 
> To keep the devicetree interface backwards compatible we simply skip the
> temperature sensor initialization if no eFuse nvmem cell is passed via
> devicetree.
> 
> The public documentation for the SAR ADC IP block does not explain how
> to use the registers to read the temperature. The logic from this patch
> is based on reading and understanding Amlogic's GPL kernel sources.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/iio/adc/meson_saradc.c | 274 +++++++++++++++++++++++++++++----
>  1 file changed, 248 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
> index 028ccd218f82..df1e45805c23 100644
> --- a/drivers/iio/adc/meson_saradc.c
> +++ b/drivers/iio/adc/meson_saradc.c
> @@ -18,6 +18,7 @@
>  #include <linux/io.h>
>  #include <linux/iio/iio.h>
>  #include <linux/module.h>
> +#include <linux/nvmem-consumer.h>
>  #include <linux/interrupt.h>
>  #include <linux/of.h>
>  #include <linux/of_irq.h>
> @@ -25,6 +26,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/mfd/syscon.h>
>  
>  #define MESON_SAR_ADC_REG0					0x00
>  	#define MESON_SAR_ADC_REG0_PANEL_DETECT			BIT(31)
> @@ -165,6 +167,17 @@
>  
>  #define MESON_SAR_ADC_MAX_FIFO_SIZE				32
>  #define MESON_SAR_ADC_TIMEOUT					100 /* ms */
> +#define MESON_SAR_ADC_VOLTAGE_AND_TEMP_CHANNEL			6
> +#define MESON_SAR_ADC_TEMP_OFFSET				27
> +
> +/* temperature sensor calibration information in eFuse */
> +#define MESON_SAR_ADC_EFUSE_BYTES				4
> +#define MESON_SARADC_EFUSE_BYTE3_UPPER_ADC_VAL			GENMASK(6, 0)
> +#define MESON_SARADC_EFUSE_BYTE3_IS_CALIBRATED			BIT(7)

MESON_SAR_ADC_ prefix for consistency?

> +
> +#define MESON_HHI_DPLL_TOP_0					0x318
> +#define MESON_HHI_DPLL_TOP_0_TSC_BIT4				BIT(9)

I guess these do not belog to the SAR ADC at all?

> +
>  /* for use with IIO_VAL_INT_PLUS_MICRO */
>  #define MILLION							1000000
>  
> @@ -175,16 +188,27 @@
>  	.address = _chan,						\
>  	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
>  				BIT(IIO_CHAN_INFO_AVERAGE_RAW),		\
> -	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |		\
> -				BIT(IIO_CHAN_INFO_CALIBBIAS) |		\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),		\
> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_CALIBBIAS) |	\
>  				BIT(IIO_CHAN_INFO_CALIBSCALE),		\
>  	.datasheet_name = "SAR_ADC_CH"#_chan,				\
>  }
>  
> -/*
> - * TODO: the hardware supports IIO_TEMP for channel 6 as well which is
> - * currently not supported by this driver.
> - */
> +#define MESON_SAR_ADC_TEMP_CHAN(_chan) {				\
> +	.type = IIO_TEMP,						\
> +	.indexed = 0,							\

.indexed = 0 not needed

> +	.channel = _chan,						\
> +	.address = MESON_SAR_ADC_VOLTAGE_AND_TEMP_CHANNEL,		\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
> +				BIT(IIO_CHAN_INFO_AVERAGE_RAW),		\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET) |		\
> +					BIT(IIO_CHAN_INFO_SCALE) |	\
> +					BIT(IIO_CHAN_INFO_ENABLE),	\
> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_CALIBBIAS) |	\
> +				BIT(IIO_CHAN_INFO_CALIBSCALE),		\
> +	.datasheet_name = "TEMP_SENSOR",				\
> +}
> +
>  static const struct iio_chan_spec meson_sar_adc_iio_channels[] = {
>  	MESON_SAR_ADC_CHAN(0),
>  	MESON_SAR_ADC_CHAN(1),
> @@ -197,6 +221,19 @@ static const struct iio_chan_spec meson_sar_adc_iio_channels[] = {
>  	IIO_CHAN_SOFT_TIMESTAMP(8),
>  };
>  
> +static const struct iio_chan_spec meson_sar_adc_and_temp_iio_channels[] = {
> +	MESON_SAR_ADC_CHAN(0),
> +	MESON_SAR_ADC_CHAN(1),
> +	MESON_SAR_ADC_CHAN(2),
> +	MESON_SAR_ADC_CHAN(3),
> +	MESON_SAR_ADC_CHAN(4),
> +	MESON_SAR_ADC_CHAN(5),
> +	MESON_SAR_ADC_CHAN(6),
> +	MESON_SAR_ADC_CHAN(7),
> +	MESON_SAR_ADC_TEMP_CHAN(8),
> +	IIO_CHAN_SOFT_TIMESTAMP(9),
> +};
> +
>  enum meson_sar_adc_avg_mode {
>  	NO_AVERAGING = 0x0,
>  	MEAN_AVERAGING = 0x1,
> @@ -225,6 +262,11 @@ struct meson_sar_adc_param {
>  	u32					bandgap_reg;
>  	unsigned int				resolution;
>  	const struct regmap_config		*regmap_config;
> +	u8					temperature_trimming_bits;
> +	unsigned int				temperature_multiplier;
> +	unsigned int				temperature_divider;
> +	const struct iio_chan_spec		*channels;
> +	unsigned int				num_channels;
>  };
>  
>  struct meson_sar_adc_data {
> @@ -246,6 +288,10 @@ struct meson_sar_adc_priv {
>  	struct completion			done;
>  	int					calibbias;
>  	int					calibscale;
> +	struct regmap				*tsc_regmap;
> +	bool					temperature_sensor_calibrated;
> +	u8					temperature_sensor_coefficient;
> +	u16					temperature_sensor_adc_val;
>  };
>  
>  static const struct regmap_config meson_sar_adc_regmap_config_gxbb = {
> @@ -389,9 +435,17 @@ static void meson_sar_adc_enable_channel(struct iio_dev *indio_dev,
>  			   MESON_SAR_ADC_DETECT_IDLE_SW_IDLE_MUX_SEL_MASK,
>  			   regval);
>  
> -	if (chan->address == 6)
> -		regmap_update_bits(priv->regmap, MESON_SAR_ADC_DELTA_10,
> -				   MESON_SAR_ADC_DELTA_10_TEMP_SEL, 0);
> +	if (chan->address == MESON_SAR_ADC_VOLTAGE_AND_TEMP_CHANNEL) {

quite repetitive, maybe (just a suggestion)
		u8 val = chan->type == IIO_TEMP ? MESON_SAR_ADC_DELTA_10_TEMP_SEL : 0;
		regmap_update_bits(priv->regmap, MESON_SAR_ADC_DELTA_10,
				   MESON_SAR_ADC_DELTA_10_TEMP_SEL, val);

> +		if (chan->type == IIO_TEMP)
> +			regmap_update_bits(priv->regmap,
> +					   MESON_SAR_ADC_DELTA_10,
> +					   MESON_SAR_ADC_DELTA_10_TEMP_SEL,
> +					   MESON_SAR_ADC_DELTA_10_TEMP_SEL);
> +		else
> +			regmap_update_bits(priv->regmap,
> +					   MESON_SAR_ADC_DELTA_10,
> +					   MESON_SAR_ADC_DELTA_10_TEMP_SEL, 0);
> +	}
>  }
>  
>  static void meson_sar_adc_set_chan7_mux(struct iio_dev *indio_dev,
> @@ -506,8 +560,12 @@ static int meson_sar_adc_get_sample(struct iio_dev *indio_dev,
>  				    enum meson_sar_adc_num_samples avg_samples,
>  				    int *val)
>  {
> +	struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
>  	int ret;
>  
> +	if (chan->type == IIO_TEMP && !priv->temperature_sensor_calibrated)
> +		return -ENOTSUPP;
> +
>  	ret = meson_sar_adc_lock(indio_dev);
>  	if (ret)
>  		return ret;
> @@ -555,17 +613,31 @@ static int meson_sar_adc_iio_info_read_raw(struct iio_dev *indio_dev,
>  		break;
>  
>  	case IIO_CHAN_INFO_SCALE:
> -		ret = regulator_get_voltage(priv->vref);
> -		if (ret < 0) {
> -			dev_err(indio_dev->dev.parent,
> -				"failed to get vref voltage: %d\n", ret);
> -			return ret;
> +		if (chan->type == IIO_VOLTAGE) {
> +			ret = regulator_get_voltage(priv->vref);
> +			if (ret < 0) {
> +				dev_err(indio_dev->dev.parent,
> +					"failed to get vref voltage: %d\n",
> +					ret);
> +				return ret;
> +			}
> +
> +			*val = ret / 1000;
> +			*val2 = priv->param->resolution;
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		} else if (chan->type == IIO_TEMP) {
> +			/* SoC specific multiplier and divider */
> +			*val = priv->param->temperature_multiplier;
> +			*val2 = priv->param->temperature_divider;
> +
> +			/* celsius to millicelsius */
> +			*val *= 1000;
> +
> +			return IIO_VAL_FRACTIONAL;
> +		} else {
> +			return -EINVAL;
>  		}
>  
> -		*val = ret / 1000;
> -		*val2 = priv->param->resolution;
> -		return IIO_VAL_FRACTIONAL_LOG2;
> -
>  	case IIO_CHAN_INFO_CALIBBIAS:
>  		*val = priv->calibbias;
>  		return IIO_VAL_INT;
> @@ -575,6 +647,17 @@ static int meson_sar_adc_iio_info_read_raw(struct iio_dev *indio_dev,
>  		*val2 = priv->calibscale % MILLION;
>  		return IIO_VAL_INT_PLUS_MICRO;
>  
> +	case IIO_CHAN_INFO_OFFSET:
> +		*val = DIV_ROUND_CLOSEST(MESON_SAR_ADC_TEMP_OFFSET *
> +					 priv->param->temperature_divider,
> +					 priv->param->temperature_multiplier);
> +		*val -= priv->temperature_sensor_adc_val;
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_ENABLE:
> +		*val = priv->temperature_sensor_calibrated;
> +		return IIO_VAL_INT;
> +
>  	default:
>  		return -EINVAL;
>  	}
> @@ -625,6 +708,77 @@ static int meson_sar_adc_clk_init(struct iio_dev *indio_dev,
>  	return 0;
>  }
>  
> +static int meson_sar_adc_temp_sensor_init(struct iio_dev *indio_dev)
> +{
> +	struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
> +	u8 *buf, trimming_bits, trimming_mask, upper_adc_val;
> +	struct nvmem_cell *temperature_calib;
> +	size_t read_len;
> +	int ret;
> +
> +	temperature_calib = devm_nvmem_cell_get(&indio_dev->dev,
> +						"temperature_calib");
> +	if (IS_ERR(temperature_calib)) {
> +		ret = PTR_ERR(temperature_calib);
> +
> +		/*
> +		 * leave the temperature sensor disabled if no calibration data
> +		 * was passed via nvmem-cells.
> +		 */
> +		if (ret == -ENODEV)
> +			return 0;
> +
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(indio_dev->dev.parent,
> +				"failed to get temperature_calib cell\n");
> +
> +		return ret;
> +	}
> +
> +	priv->tsc_regmap = syscon_regmap_lookup_by_phandle(
> +						indio_dev->dev.parent->of_node,
> +						"amlogic,hhi-sysctrl");
> +	if (IS_ERR(priv->tsc_regmap)) {
> +		dev_err(indio_dev->dev.parent,
> +			"failed to get amlogic,hhi-sysctrl regmap\n");
> +		return PTR_ERR(priv->tsc_regmap);
> +	}
> +
> +	read_len = MESON_SAR_ADC_EFUSE_BYTES;
> +	buf = nvmem_cell_read(temperature_calib, &read_len);
> +	if (IS_ERR(buf)) {
> +		dev_err(indio_dev->dev.parent,
> +			"failed to read temperature_calib cell\n");
> +		return PTR_ERR(buf);
> +	} else if (read_len != MESON_SAR_ADC_EFUSE_BYTES) {
> +		kfree(buf);
> +		dev_err(indio_dev->dev.parent,
> +			"invalid read size of temperature_calib cell\n");
> +		return -EINVAL;
> +	}
> +
> +	trimming_bits = priv->param->temperature_trimming_bits;
> +	trimming_mask = BIT(trimming_bits) - 1;
> +
> +	if (buf[3] & MESON_SARADC_EFUSE_BYTE3_IS_CALIBRATED)
> +		priv->temperature_sensor_calibrated = true;
> +	else
> +		priv->temperature_sensor_calibrated = false;
> +
> +	priv->temperature_sensor_coefficient = buf[2] & trimming_mask;
> +
> +	upper_adc_val = FIELD_GET(MESON_SARADC_EFUSE_BYTE3_UPPER_ADC_VAL,
> +				  buf[3]);
> +
> +	priv->temperature_sensor_adc_val = buf[2];
> +	priv->temperature_sensor_adc_val |= upper_adc_val << BITS_PER_BYTE;
> +	priv->temperature_sensor_adc_val >>= trimming_bits;
> +
> +	kfree(buf);
> +
> +	return 0;
> +}
> +
>  static int meson_sar_adc_init(struct iio_dev *indio_dev)
>  {
>  	struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
> @@ -649,10 +803,12 @@ static int meson_sar_adc_init(struct iio_dev *indio_dev)
>  
>  	meson_sar_adc_stop_sample_engine(indio_dev);
>  
> -	/* update the channel 6 MUX to select the temperature sensor */
> +	/*
> +	 * disable this bit as seems to be only relevant for Meson6 (based
> +	 * on the vendor driver), which we don't support at the moment.
> +	 */
>  	regmap_update_bits(priv->regmap, MESON_SAR_ADC_REG0,
> -			MESON_SAR_ADC_REG0_ADC_TEMP_SEN_SEL,
> -			MESON_SAR_ADC_REG0_ADC_TEMP_SEN_SEL);
> +			MESON_SAR_ADC_REG0_ADC_TEMP_SEN_SEL, 0);
>  
>  	/* disable all channels by default */
>  	regmap_write(priv->regmap, MESON_SAR_ADC_CHAN_LIST, 0x0);
> @@ -709,6 +865,45 @@ static int meson_sar_adc_init(struct iio_dev *indio_dev)
>  	regval |= MESON_SAR_ADC_AUX_SW_XP_DRIVE_SW;
>  	regmap_write(priv->regmap, MESON_SAR_ADC_AUX_SW, regval);
>  
> +	if (priv->temperature_sensor_calibrated) {
> +		regmap_update_bits(priv->regmap, MESON_SAR_ADC_DELTA_10,
> +				   MESON_SAR_ADC_DELTA_10_TS_REVE1,
> +				   MESON_SAR_ADC_DELTA_10_TS_REVE1);
> +		regmap_update_bits(priv->regmap, MESON_SAR_ADC_DELTA_10,
> +				   MESON_SAR_ADC_DELTA_10_TS_REVE0,
> +				   MESON_SAR_ADC_DELTA_10_TS_REVE0);
> +
> +		/*
> +		 * set bits [3:0] of the TSC (temperature sensor coefficient)
> +		 * to get the correct values when reading the temperature.
> +		 */
> +		regval = FIELD_PREP(MESON_SAR_ADC_DELTA_10_TS_C_MASK,
> +				    priv->temperature_sensor_coefficient);
> +		regmap_update_bits(priv->regmap, MESON_SAR_ADC_DELTA_10,
> +				   MESON_SAR_ADC_DELTA_10_TS_C_MASK, regval);
> +
> +		if (priv->param->temperature_trimming_bits == 5) {
> +			if (priv->temperature_sensor_coefficient & BIT(4))
> +				regval = MESON_HHI_DPLL_TOP_0_TSC_BIT4;
> +			else
> +				regval = 0;
> +
> +			/*
> +			 * the 5th bit (4th when starting to count at 0) of the
> +			 * TSC is located in another register area.
> +			 */
> +			regmap_update_bits(priv->tsc_regmap,
> +					   MESON_HHI_DPLL_TOP_0,
> +					   MESON_HHI_DPLL_TOP_0_TSC_BIT4,
> +					   regval);
> +		}
> +	} else {
> +		regmap_update_bits(priv->regmap, MESON_SAR_ADC_DELTA_10,
> +				   MESON_SAR_ADC_DELTA_10_TS_REVE1, 0);
> +		regmap_update_bits(priv->regmap, MESON_SAR_ADC_DELTA_10,
> +				   MESON_SAR_ADC_DELTA_10_TS_REVE0, 0);
> +	}
> +
>  	ret = clk_set_parent(priv->adc_sel_clk, priv->clkin);
>  	if (ret) {
>  		dev_err(indio_dev->dev.parent,
> @@ -894,6 +1089,24 @@ static const struct meson_sar_adc_param meson_sar_adc_meson8_param = {
>  	.bandgap_reg = MESON_SAR_ADC_DELTA_10,
>  	.regmap_config = &meson_sar_adc_regmap_config_meson8,
>  	.resolution = 10,
> +	.temperature_trimming_bits = 4,
> +	.temperature_multiplier = 18 * 10000,
> +	.temperature_divider = 1024 * 10 * 85,
> +	.channels = meson_sar_adc_and_temp_iio_channels,
> +	.num_channels = ARRAY_SIZE(meson_sar_adc_and_temp_iio_channels),
> +};
> +
> +static const struct meson_sar_adc_param meson_sar_adc_meson8b_param = {
> +	.has_bl30_integration = false,
> +	.clock_rate = 1150000,
> +	.bandgap_reg = MESON_SAR_ADC_DELTA_10,
> +	.regmap_config = &meson_sar_adc_regmap_config_meson8,
> +	.resolution = 10,
> +	.temperature_trimming_bits = 5,
> +	.temperature_multiplier = 10,
> +	.temperature_divider = 32,
> +	.channels = meson_sar_adc_and_temp_iio_channels,
> +	.num_channels = ARRAY_SIZE(meson_sar_adc_and_temp_iio_channels),
>  };
>  
>  static const struct meson_sar_adc_param meson_sar_adc_gxbb_param = {
> @@ -902,6 +1115,8 @@ static const struct meson_sar_adc_param meson_sar_adc_gxbb_param = {
>  	.bandgap_reg = MESON_SAR_ADC_REG11,
>  	.regmap_config = &meson_sar_adc_regmap_config_gxbb,
>  	.resolution = 10,
> +	.channels = meson_sar_adc_iio_channels,
> +	.num_channels = ARRAY_SIZE(meson_sar_adc_iio_channels),
>  };
>  
>  static const struct meson_sar_adc_param meson_sar_adc_gxl_param = {
> @@ -910,6 +1125,8 @@ static const struct meson_sar_adc_param meson_sar_adc_gxl_param = {
>  	.bandgap_reg = MESON_SAR_ADC_REG11,
>  	.regmap_config = &meson_sar_adc_regmap_config_gxbb,
>  	.resolution = 12,
> +	.channels = meson_sar_adc_iio_channels,
> +	.num_channels = ARRAY_SIZE(meson_sar_adc_iio_channels),
>  };
>  
>  static const struct meson_sar_adc_data meson_sar_adc_meson8_data = {
> @@ -918,12 +1135,12 @@ static const struct meson_sar_adc_data meson_sar_adc_meson8_data = {
>  };
>  
>  static const struct meson_sar_adc_data meson_sar_adc_meson8b_data = {
> -	.param = &meson_sar_adc_meson8_param,
> +	.param = &meson_sar_adc_meson8b_param,
>  	.name = "meson-meson8b-saradc",
>  };
>  
>  static const struct meson_sar_adc_data meson_sar_adc_meson8m2_data = {
> -	.param = &meson_sar_adc_meson8_param,
> +	.param = &meson_sar_adc_meson8b_param,
>  	.name = "meson-meson8m2-saradc",
>  };
>  
> @@ -1008,9 +1225,8 @@ static int meson_sar_adc_probe(struct platform_device *pdev)
>  	indio_dev->dev.of_node = pdev->dev.of_node;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->info = &meson_sar_adc_iio_info;
> -
> -	indio_dev->channels = meson_sar_adc_iio_channels;
> -	indio_dev->num_channels = ARRAY_SIZE(meson_sar_adc_iio_channels);
> +	indio_dev->channels = priv->param->channels;
> +	indio_dev->num_channels = priv->param->num_channels;
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	base = devm_ioremap_resource(&pdev->dev, res);
> @@ -1078,6 +1294,12 @@ static int meson_sar_adc_probe(struct platform_device *pdev)
>  
>  	priv->calibscale = MILLION;
>  
> +	if (priv->param->temperature_trimming_bits) {
> +		ret = meson_sar_adc_temp_sensor_init(indio_dev);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	ret = meson_sar_adc_init(indio_dev);
>  	if (ret)
>  		goto err;
> 

-- 

Peter Meerwald-Stadler
Mobile: +43 664 24 44 418

Re: [PATCH 2/7] iio: adc: meson-saradc: add support for the chip's temperature sensor

[Jonathan Cameron]

On Sun, 28 Oct 2018 13:26:24 +0100
Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:

> Channel 6 of the SAR ADC can be switched between two inputs:
> SAR_ADC_CH6 input (an actual pad on the SoC) and the signal from the
> temperature sensor inside the SoC.
> 
> To get usable results from the temperature sensor we need to read the
> corresponding calibration data from the eFuse and pass it to the SAR ADC
> (bits [3:0]) and (on Meson8b and Meson8m2) to a scratch register in the
> HHI region.
> If the temperature sensor is not calibrated (the eFuse data contains a
> bit for this) then the driver will return -ENOTSUPP when trying to read
> the temperature.

If it is not supported I'd rather not see it at all.  Have two channel
sets and pick the one without the channel (by all means report
the lack of support in the kernel log).

> 
> This only enables the temperature sensor for the Meson8, Meson8b and
> Meson8m2 SoCs because on the newer, 64-bit SoCs (GXBB, GXL and GXM) the
> temperature sensor inside SAR ADC is firmware-controlled (by BL30, we
> can simply use the SCPI hwmon driver to get the chip temperature).
> 
> To keep the devicetree interface backwards compatible we simply skip the
> temperature sensor initialization if no eFuse nvmem cell is passed via
> devicetree.
> 
> The public documentation for the SAR ADC IP block does not explain how
> to use the registers to read the temperature. The logic from this patch
> is based on reading and understanding Amlogic's GPL kernel sources.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Only one significant comment on this (rest looks good to me),
what are you using the ENABLE element for in the info_mask?

I 'think' it's an indicator that the temperature sensor reads
will always return -ENOSUP?  This is non standard so I'd definitely
prefer us to do it the way I suggest above.

Thanks,

Jonathan
...
> +#define MESON_SAR_ADC_TEMP_CHAN(_chan) {				\
> +	.type = IIO_TEMP,						\
> +	.indexed = 0,							\
> +	.channel = _chan,						\
> +	.address = MESON_SAR_ADC_VOLTAGE_AND_TEMP_CHANNEL,		\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
> +				BIT(IIO_CHAN_INFO_AVERAGE_RAW),		\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET) |		\
> +					BIT(IIO_CHAN_INFO_SCALE) |	\
> +					BIT(IIO_CHAN_INFO_ENABLE),	\
ENABLE?

> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_CALIBBIAS) |	\
> +				BIT(IIO_CHAN_INFO_CALIBSCALE),		\
> +	.datasheet_name = "TEMP_SENSOR",				\
> +}
> +
>  static const struct iio_chan_spec meson_sar_adc_iio_channels[] = {
>  	MESON_SAR_ADC_CHAN(0),
>  	MESON_SAR_ADC_CHAN(1),
> @@ -197,6 +221,19 @@ static const struct iio_chan_spec meson_sar_adc_iio_channels[] = {
>  	IIO_CHAN_SOFT_TIMESTAMP(8),
>  };
>  
...
> @@ -555,17 +613,31 @@ static int meson_sar_adc_iio_info_read_raw(struct iio_dev *indio_dev,
>  		break;
>  
>  	case IIO_CHAN_INFO_SCALE:
> -		ret = regulator_get_voltage(priv->vref);
> -		if (ret < 0) {
> -			dev_err(indio_dev->dev.parent,
> -				"failed to get vref voltage: %d\n", ret);
> -			return ret;
> +		if (chan->type == IIO_VOLTAGE) {
> +			ret = regulator_get_voltage(priv->vref);
> +			if (ret < 0) {
> +				dev_err(indio_dev->dev.parent,
> +					"failed to get vref voltage: %d\n",
> +					ret);
> +				return ret;
> +			}
> +
> +			*val = ret / 1000;
> +			*val2 = priv->param->resolution;
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		} else if (chan->type == IIO_TEMP) {
> +			/* SoC specific multiplier and divider */
> +			*val = priv->param->temperature_multiplier;
> +			*val2 = priv->param->temperature_divider;
> +
> +			/* celsius to millicelsius */
> +			*val *= 1000;
> +
> +			return IIO_VAL_FRACTIONAL;
> +		} else {
> +			return -EINVAL;
>  		}
>  
> -		*val = ret / 1000;
> -		*val2 = priv->param->resolution;
> -		return IIO_VAL_FRACTIONAL_LOG2;
> -
>  	case IIO_CHAN_INFO_CALIBBIAS:
>  		*val = priv->calibbias;
>  		return IIO_VAL_INT;
> @@ -575,6 +647,17 @@ static int meson_sar_adc_iio_info_read_raw(struct iio_dev *indio_dev,
>  		*val2 = priv->calibscale % MILLION;
>  		return IIO_VAL_INT_PLUS_MICRO;
>  
> +	case IIO_CHAN_INFO_OFFSET:
> +		*val = DIV_ROUND_CLOSEST(MESON_SAR_ADC_TEMP_OFFSET *
> +					 priv->param->temperature_divider,
> +					 priv->param->temperature_multiplier);
> +		*val -= priv->temperature_sensor_adc_val;
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_ENABLE:

Hmm.  This is not a standard use of this attribute. It's normally only used
for things that are 'counting' where we want to start and stop
a cumulative counter.

> +		*val = priv->temperature_sensor_calibrated;
> +		return IIO_VAL_INT;
> +
>  	default:
>  		return -EINVAL;
>  	}
> @@ -625,6 +708,77 @@ static int meson_sar_adc_clk_init(struct iio_dev *indio_dev,
>  	return 0;
>  }
>  
> +static int meson_sar_adc_temp_sensor_init(struct iio_dev *indio_dev)
> +{
> +	struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
> +	u8 *buf, trimming_bits, trimming_mask, upper_adc_val;
> +	struct nvmem_cell *temperature_calib;
> +	size_t read_len;
> +	int ret;
> +
> +	temperature_calib = devm_nvmem_cell_get(&indio_dev->dev,
> +						"temperature_calib");
> +	if (IS_ERR(temperature_calib)) {
> +		ret = PTR_ERR(temperature_calib);
> +
> +		/*
> +		 * leave the temperature sensor disabled if no calibration data
> +		 * was passed via nvmem-cells.
> +		 */
> +		if (ret == -ENODEV)
> +			return 0;
> +
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(indio_dev->dev.parent,
> +				"failed to get temperature_calib cell\n");
> +
> +		return ret;
> +	}
> +
> +	priv->tsc_regmap = syscon_regmap_lookup_by_phandle(
> +						indio_dev->dev.parent->of_node,
> +						"amlogic,hhi-sysctrl");
> +	if (IS_ERR(priv->tsc_regmap)) {
> +		dev_err(indio_dev->dev.parent,
> +			"failed to get amlogic,hhi-sysctrl regmap\n");
> +		return PTR_ERR(priv->tsc_regmap);
> +	}
> +
> +	read_len = MESON_SAR_ADC_EFUSE_BYTES;
> +	buf = nvmem_cell_read(temperature_calib, &read_len);
> +	if (IS_ERR(buf)) {
> +		dev_err(indio_dev->dev.parent,
> +			"failed to read temperature_calib cell\n");
> +		return PTR_ERR(buf);
> +	} else if (read_len != MESON_SAR_ADC_EFUSE_BYTES) {
> +		kfree(buf);
> +		dev_err(indio_dev->dev.parent,
> +			"invalid read size of temperature_calib cell\n");
> +		return -EINVAL;
> +	}
> +
> +	trimming_bits = priv->param->temperature_trimming_bits;
> +	trimming_mask = BIT(trimming_bits) - 1;
> +
> +	if (buf[3] & MESON_SARADC_EFUSE_BYTE3_IS_CALIBRATED)
> +		priv->temperature_sensor_calibrated = true;
> +	else
> +		priv->temperature_sensor_calibrated = false;

Could just assign?
	priv->temperature_sensor_calibrated =
		buf[3] & MEESON_SARADC_EFUSE_BYTE3_IS_CALIBRATED;
> +
> +	priv->temperature_sensor_coefficient = buf[2] & trimming_mask;
> +
> +	upper_adc_val = FIELD_GET(MESON_SARADC_EFUSE_BYTE3_UPPER_ADC_VAL,
> +				  buf[3]);
> +
> +	priv->temperature_sensor_adc_val = buf[2];
> +	priv->temperature_sensor_adc_val |= upper_adc_val << BITS_PER_BYTE;
> +	priv->temperature_sensor_adc_val >>= trimming_bits;
> +
> +	kfree(buf);
> +
> +	return 0;
> +}
> +
...

[PATCH 2/7] iio: adc: meson-saradc: add support for the chip's temperature sensor

[Jonathan Cameron]

On Sun, 28 Oct 2018 13:26:24 +0100
Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:

> Channel 6 of the SAR ADC can be switched between two inputs:
> SAR_ADC_CH6 input (an actual pad on the SoC) and the signal from the
> temperature sensor inside the SoC.
> 
> To get usable results from the temperature sensor we need to read the
> corresponding calibration data from the eFuse and pass it to the SAR ADC
> (bits [3:0]) and (on Meson8b and Meson8m2) to a scratch register in the
> HHI region.
> If the temperature sensor is not calibrated (the eFuse data contains a
> bit for this) then the driver will return -ENOTSUPP when trying to read
> the temperature.

If it is not supported I'd rather not see it at all.  Have two channel
sets and pick the one without the channel (by all means report
the lack of support in the kernel log).

> 
> This only enables the temperature sensor for the Meson8, Meson8b and
> Meson8m2 SoCs because on the newer, 64-bit SoCs (GXBB, GXL and GXM) the
> temperature sensor inside SAR ADC is firmware-controlled (by BL30, we
> can simply use the SCPI hwmon driver to get the chip temperature).
> 
> To keep the devicetree interface backwards compatible we simply skip the
> temperature sensor initialization if no eFuse nvmem cell is passed via
> devicetree.
> 
> The public documentation for the SAR ADC IP block does not explain how
> to use the registers to read the temperature. The logic from this patch
> is based on reading and understanding Amlogic's GPL kernel sources.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Only one significant comment on this (rest looks good to me),
what are you using the ENABLE element for in the info_mask?

I 'think' it's an indicator that the temperature sensor reads
will always return -ENOSUP?  This is non standard so I'd definitely
prefer us to do it the way I suggest above.

Thanks,

Jonathan
...
> +#define MESON_SAR_ADC_TEMP_CHAN(_chan) {				\
> +	.type = IIO_TEMP,						\
> +	.indexed = 0,							\
> +	.channel = _chan,						\
> +	.address = MESON_SAR_ADC_VOLTAGE_AND_TEMP_CHANNEL,		\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
> +				BIT(IIO_CHAN_INFO_AVERAGE_RAW),		\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET) |		\
> +					BIT(IIO_CHAN_INFO_SCALE) |	\
> +					BIT(IIO_CHAN_INFO_ENABLE),	\
ENABLE?

> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_CALIBBIAS) |	\
> +				BIT(IIO_CHAN_INFO_CALIBSCALE),		\
> +	.datasheet_name = "TEMP_SENSOR",				\
> +}
> +
>  static const struct iio_chan_spec meson_sar_adc_iio_channels[] = {
>  	MESON_SAR_ADC_CHAN(0),
>  	MESON_SAR_ADC_CHAN(1),
> @@ -197,6 +221,19 @@ static const struct iio_chan_spec meson_sar_adc_iio_channels[] = {
>  	IIO_CHAN_SOFT_TIMESTAMP(8),
>  };
>  
...
> @@ -555,17 +613,31 @@ static int meson_sar_adc_iio_info_read_raw(struct iio_dev *indio_dev,
>  		break;
>  
>  	case IIO_CHAN_INFO_SCALE:
> -		ret = regulator_get_voltage(priv->vref);
> -		if (ret < 0) {
> -			dev_err(indio_dev->dev.parent,
> -				"failed to get vref voltage: %d\n", ret);
> -			return ret;
> +		if (chan->type == IIO_VOLTAGE) {
> +			ret = regulator_get_voltage(priv->vref);
> +			if (ret < 0) {
> +				dev_err(indio_dev->dev.parent,
> +					"failed to get vref voltage: %d\n",
> +					ret);
> +				return ret;
> +			}
> +
> +			*val = ret / 1000;
> +			*val2 = priv->param->resolution;
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		} else if (chan->type == IIO_TEMP) {
> +			/* SoC specific multiplier and divider */
> +			*val = priv->param->temperature_multiplier;
> +			*val2 = priv->param->temperature_divider;
> +
> +			/* celsius to millicelsius */
> +			*val *= 1000;
> +
> +			return IIO_VAL_FRACTIONAL;
> +		} else {
> +			return -EINVAL;
>  		}
>  
> -		*val = ret / 1000;
> -		*val2 = priv->param->resolution;
> -		return IIO_VAL_FRACTIONAL_LOG2;
> -
>  	case IIO_CHAN_INFO_CALIBBIAS:
>  		*val = priv->calibbias;
>  		return IIO_VAL_INT;
> @@ -575,6 +647,17 @@ static int meson_sar_adc_iio_info_read_raw(struct iio_dev *indio_dev,
>  		*val2 = priv->calibscale % MILLION;
>  		return IIO_VAL_INT_PLUS_MICRO;
>  
> +	case IIO_CHAN_INFO_OFFSET:
> +		*val = DIV_ROUND_CLOSEST(MESON_SAR_ADC_TEMP_OFFSET *
> +					 priv->param->temperature_divider,
> +					 priv->param->temperature_multiplier);
> +		*val -= priv->temperature_sensor_adc_val;
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_ENABLE:

Hmm.  This is not a standard use of this attribute. It's normally only used
for things that are 'counting' where we want to start and stop
a cumulative counter.

> +		*val = priv->temperature_sensor_calibrated;
> +		return IIO_VAL_INT;
> +
>  	default:
>  		return -EINVAL;
>  	}
> @@ -625,6 +708,77 @@ static int meson_sar_adc_clk_init(struct iio_dev *indio_dev,
>  	return 0;
>  }
>  
> +static int meson_sar_adc_temp_sensor_init(struct iio_dev *indio_dev)
> +{
> +	struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
> +	u8 *buf, trimming_bits, trimming_mask, upper_adc_val;
> +	struct nvmem_cell *temperature_calib;
> +	size_t read_len;
> +	int ret;
> +
> +	temperature_calib = devm_nvmem_cell_get(&indio_dev->dev,
> +						"temperature_calib");
> +	if (IS_ERR(temperature_calib)) {
> +		ret = PTR_ERR(temperature_calib);
> +
> +		/*
> +		 * leave the temperature sensor disabled if no calibration data
> +		 * was passed via nvmem-cells.
> +		 */
> +		if (ret == -ENODEV)
> +			return 0;
> +
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(indio_dev->dev.parent,
> +				"failed to get temperature_calib cell\n");
> +
> +		return ret;
> +	}
> +
> +	priv->tsc_regmap = syscon_regmap_lookup_by_phandle(
> +						indio_dev->dev.parent->of_node,
> +						"amlogic,hhi-sysctrl");
> +	if (IS_ERR(priv->tsc_regmap)) {
> +		dev_err(indio_dev->dev.parent,
> +			"failed to get amlogic,hhi-sysctrl regmap\n");
> +		return PTR_ERR(priv->tsc_regmap);
> +	}
> +
> +	read_len = MESON_SAR_ADC_EFUSE_BYTES;
> +	buf = nvmem_cell_read(temperature_calib, &read_len);
> +	if (IS_ERR(buf)) {
> +		dev_err(indio_dev->dev.parent,
> +			"failed to read temperature_calib cell\n");
> +		return PTR_ERR(buf);
> +	} else if (read_len != MESON_SAR_ADC_EFUSE_BYTES) {
> +		kfree(buf);
> +		dev_err(indio_dev->dev.parent,
> +			"invalid read size of temperature_calib cell\n");
> +		return -EINVAL;
> +	}
> +
> +	trimming_bits = priv->param->temperature_trimming_bits;
> +	trimming_mask = BIT(trimming_bits) - 1;
> +
> +	if (buf[3] & MESON_SARADC_EFUSE_BYTE3_IS_CALIBRATED)
> +		priv->temperature_sensor_calibrated = true;
> +	else
> +		priv->temperature_sensor_calibrated = false;

Could just assign?
	priv->temperature_sensor_calibrated =
		buf[3] & MEESON_SARADC_EFUSE_BYTE3_IS_CALIBRATED;
> +
> +	priv->temperature_sensor_coefficient = buf[2] & trimming_mask;
> +
> +	upper_adc_val = FIELD_GET(MESON_SARADC_EFUSE_BYTE3_UPPER_ADC_VAL,
> +				  buf[3]);
> +
> +	priv->temperature_sensor_adc_val = buf[2];
> +	priv->temperature_sensor_adc_val |= upper_adc_val << BITS_PER_BYTE;
> +	priv->temperature_sensor_adc_val >>= trimming_bits;
> +
> +	kfree(buf);
> +
> +	return 0;
> +}
> +
...

Re: [PATCH 2/7] iio: adc: meson-saradc: add support for the chip's temperature sensor

[Martin Blumenstingl]

Hi Peter,

thank you for taking the time to review my patch!

On Sun, Oct 28, 2018 at 5:19 PM Peter Meerwald-Stadler
<pmeerw@pmeerw.net> wrote:
>
> On Sun, 28 Oct 2018, Martin Blumenstingl wrote:
>
> comments below
>
> > Channel 6 of the SAR ADC can be switched between two inputs:
> > SAR_ADC_CH6 input (an actual pad on the SoC) and the signal from the
> > temperature sensor inside the SoC.
> >
> > To get usable results from the temperature sensor we need to read the
> > corresponding calibration data from the eFuse and pass it to the SAR ADC
> > (bits [3:0]) and (on Meson8b and Meson8m2) to a scratch register in the
> > HHI region.
> > If the temperature sensor is not calibrated (the eFuse data contains a
> > bit for this) then the driver will return -ENOTSUPP when trying to read
> > the temperature.
> >
> > This only enables the temperature sensor for the Meson8, Meson8b and
> > Meson8m2 SoCs because on the newer, 64-bit SoCs (GXBB, GXL and GXM) the
> > temperature sensor inside SAR ADC is firmware-controlled (by BL30, we
> > can simply use the SCPI hwmon driver to get the chip temperature).
> >
> > To keep the devicetree interface backwards compatible we simply skip the
> > temperature sensor initialization if no eFuse nvmem cell is passed via
> > devicetree.
> >
> > The public documentation for the SAR ADC IP block does not explain how
> > to use the registers to read the temperature. The logic from this patch
> > is based on reading and understanding Amlogic's GPL kernel sources.
> >
> > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> > ---
> >  drivers/iio/adc/meson_saradc.c | 274 +++++++++++++++++++++++++++++----
> >  1 file changed, 248 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
> > index 028ccd218f82..df1e45805c23 100644
> > --- a/drivers/iio/adc/meson_saradc.c
> > +++ b/drivers/iio/adc/meson_saradc.c
> > @@ -18,6 +18,7 @@
> >  #include <linux/io.h>
> >  #include <linux/iio/iio.h>
> >  #include <linux/module.h>
> > +#include <linux/nvmem-consumer.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/of.h>
> >  #include <linux/of_irq.h>
> > @@ -25,6 +26,7 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/regmap.h>
> >  #include <linux/regulator/consumer.h>
> > +#include <linux/mfd/syscon.h>
> >
> >  #define MESON_SAR_ADC_REG0                                   0x00
> >       #define MESON_SAR_ADC_REG0_PANEL_DETECT                 BIT(31)
> > @@ -165,6 +167,17 @@
> >
> >  #define MESON_SAR_ADC_MAX_FIFO_SIZE                          32
> >  #define MESON_SAR_ADC_TIMEOUT                                        100 /* ms */
> > +#define MESON_SAR_ADC_VOLTAGE_AND_TEMP_CHANNEL                       6
> > +#define MESON_SAR_ADC_TEMP_OFFSET                            27
> > +
> > +/* temperature sensor calibration information in eFuse */
> > +#define MESON_SAR_ADC_EFUSE_BYTES                            4
> > +#define MESON_SARADC_EFUSE_BYTE3_UPPER_ADC_VAL                       GENMASK(6, 0)
> > +#define MESON_SARADC_EFUSE_BYTE3_IS_CALIBRATED                       BIT(7)
>
> MESON_SAR_ADC_ prefix for consistency?
I missed that, thanks

> > +
> > +#define MESON_HHI_DPLL_TOP_0                                 0x318
> > +#define MESON_HHI_DPLL_TOP_0_TSC_BIT4                                BIT(9)
>
> I guess these do not belog to the SAR ADC at all?
yes and no
these register are part of the HHI register region which is shared the:
- clock controller
- HDMI controller
- IIRC the HDMI PHY as well
- one TSC bit for the SAR ADC
- ...possibly more

currently there's no header file for all the HHI includes because (as
far as I'm aware) each register is only used by one IP block

> > +
> >  /* for use with IIO_VAL_INT_PLUS_MICRO */
> >  #define MILLION                                                      1000000
> >
> > @@ -175,16 +188,27 @@
> >       .address = _chan,                                               \
> >       .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |                  \
> >                               BIT(IIO_CHAN_INFO_AVERAGE_RAW),         \
> > -     .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |          \
> > -                             BIT(IIO_CHAN_INFO_CALIBBIAS) |          \
> > +     .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),           \
> > +     .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_CALIBBIAS) |       \
> >                               BIT(IIO_CHAN_INFO_CALIBSCALE),          \
> >       .datasheet_name = "SAR_ADC_CH"#_chan,                           \
> >  }
> >
> > -/*
> > - * TODO: the hardware supports IIO_TEMP for channel 6 as well which is
> > - * currently not supported by this driver.
> > - */
> > +#define MESON_SAR_ADC_TEMP_CHAN(_chan) {                             \
> > +     .type = IIO_TEMP,                                               \
> > +     .indexed = 0,                                                   \
>
> .indexed = 0 not needed
OK, I will drop this

> > +     .channel = _chan,                                               \
> > +     .address = MESON_SAR_ADC_VOLTAGE_AND_TEMP_CHANNEL,              \
> > +     .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |                  \
> > +                             BIT(IIO_CHAN_INFO_AVERAGE_RAW),         \
> > +     .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET) |         \
> > +                                     BIT(IIO_CHAN_INFO_SCALE) |      \
> > +                                     BIT(IIO_CHAN_INFO_ENABLE),      \
> > +     .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_CALIBBIAS) |       \
> > +                             BIT(IIO_CHAN_INFO_CALIBSCALE),          \
> > +     .datasheet_name = "TEMP_SENSOR",                                \
> > +}
> > +
> >  static const struct iio_chan_spec meson_sar_adc_iio_channels[] = {
> >       MESON_SAR_ADC_CHAN(0),
> >       MESON_SAR_ADC_CHAN(1),
> > @@ -197,6 +221,19 @@ static const struct iio_chan_spec meson_sar_adc_iio_channels[] = {
> >       IIO_CHAN_SOFT_TIMESTAMP(8),
> >  };
> >
> > +static const struct iio_chan_spec meson_sar_adc_and_temp_iio_channels[] = {
> > +     MESON_SAR_ADC_CHAN(0),
> > +     MESON_SAR_ADC_CHAN(1),
> > +     MESON_SAR_ADC_CHAN(2),
> > +     MESON_SAR_ADC_CHAN(3),
> > +     MESON_SAR_ADC_CHAN(4),
> > +     MESON_SAR_ADC_CHAN(5),
> > +     MESON_SAR_ADC_CHAN(6),
> > +     MESON_SAR_ADC_CHAN(7),
> > +     MESON_SAR_ADC_TEMP_CHAN(8),
> > +     IIO_CHAN_SOFT_TIMESTAMP(9),
> > +};
> > +
> >  enum meson_sar_adc_avg_mode {
> >       NO_AVERAGING = 0x0,
> >       MEAN_AVERAGING = 0x1,
> > @@ -225,6 +262,11 @@ struct meson_sar_adc_param {
> >       u32                                     bandgap_reg;
> >       unsigned int                            resolution;
> >       const struct regmap_config              *regmap_config;
> > +     u8                                      temperature_trimming_bits;
> > +     unsigned int                            temperature_multiplier;
> > +     unsigned int                            temperature_divider;
> > +     const struct iio_chan_spec              *channels;
> > +     unsigned int                            num_channels;
> >  };
> >
> >  struct meson_sar_adc_data {
> > @@ -246,6 +288,10 @@ struct meson_sar_adc_priv {
> >       struct completion                       done;
> >       int                                     calibbias;
> >       int                                     calibscale;
> > +     struct regmap                           *tsc_regmap;
> > +     bool                                    temperature_sensor_calibrated;
> > +     u8                                      temperature_sensor_coefficient;
> > +     u16                                     temperature_sensor_adc_val;
> >  };
> >
> >  static const struct regmap_config meson_sar_adc_regmap_config_gxbb = {
> > @@ -389,9 +435,17 @@ static void meson_sar_adc_enable_channel(struct iio_dev *indio_dev,
> >                          MESON_SAR_ADC_DETECT_IDLE_SW_IDLE_MUX_SEL_MASK,
> >                          regval);
> >
> > -     if (chan->address == 6)
> > -             regmap_update_bits(priv->regmap, MESON_SAR_ADC_DELTA_10,
> > -                                MESON_SAR_ADC_DELTA_10_TEMP_SEL, 0);
> > +     if (chan->address == MESON_SAR_ADC_VOLTAGE_AND_TEMP_CHANNEL) {
>
> quite repetitive, maybe (just a suggestion)
>                 u8 val = chan->type == IIO_TEMP ? MESON_SAR_ADC_DELTA_10_TEMP_SEL : 0;
>                 regmap_update_bits(priv->regmap, MESON_SAR_ADC_DELTA_10,
>                                    MESON_SAR_ADC_DELTA_10_TEMP_SEL, val);
thanks for the suggestion!
I already have a "regval" variable which we can use here
I'll simplify this in the next version - I'll keep the basic if/else
though instead of using a ternary operator (because "regval =
chan->type == IIO_TEMP ? MESON_SAR_ADC_DELTA_10_TEMP_SEL : 0;" would
exceed the line length limit of 80 chars)

> > +             if (chan->type == IIO_TEMP)
> > +                     regmap_update_bits(priv->regmap,
> > +                                        MESON_SAR_ADC_DELTA_10,
> > +                                        MESON_SAR_ADC_DELTA_10_TEMP_SEL,
> > +                                        MESON_SAR_ADC_DELTA_10_TEMP_SEL);
> > +             else
> > +                     regmap_update_bits(priv->regmap,
> > +                                        MESON_SAR_ADC_DELTA_10,
> > +                                        MESON_SAR_ADC_DELTA_10_TEMP_SEL, 0);
> > +     }
> >  }
[snip]


Regards
Martin

[PATCH 2/7] iio: adc: meson-saradc: add support for the chip's temperature sensor

[Martin Blumenstingl]

Hi Peter,

thank you for taking the time to review my patch!

On Sun, Oct 28, 2018 at 5:19 PM Peter Meerwald-Stadler
<pmeerw@pmeerw.net> wrote:
>
> On Sun, 28 Oct 2018, Martin Blumenstingl wrote:
>
> comments below
>
> > Channel 6 of the SAR ADC can be switched between two inputs:
> > SAR_ADC_CH6 input (an actual pad on the SoC) and the signal from the
> > temperature sensor inside the SoC.
> >
> > To get usable results from the temperature sensor we need to read the
> > corresponding calibration data from the eFuse and pass it to the SAR ADC
> > (bits [3:0]) and (on Meson8b and Meson8m2) to a scratch register in the
> > HHI region.
> > If the temperature sensor is not calibrated (the eFuse data contains a
> > bit for this) then the driver will return -ENOTSUPP when trying to read
> > the temperature.
> >
> > This only enables the temperature sensor for the Meson8, Meson8b and
> > Meson8m2 SoCs because on the newer, 64-bit SoCs (GXBB, GXL and GXM) the
> > temperature sensor inside SAR ADC is firmware-controlled (by BL30, we
> > can simply use the SCPI hwmon driver to get the chip temperature).
> >
> > To keep the devicetree interface backwards compatible we simply skip the
> > temperature sensor initialization if no eFuse nvmem cell is passed via
> > devicetree.
> >
> > The public documentation for the SAR ADC IP block does not explain how
> > to use the registers to read the temperature. The logic from this patch
> > is based on reading and understanding Amlogic's GPL kernel sources.
> >
> > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> > ---
> >  drivers/iio/adc/meson_saradc.c | 274 +++++++++++++++++++++++++++++----
> >  1 file changed, 248 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c
> > index 028ccd218f82..df1e45805c23 100644
> > --- a/drivers/iio/adc/meson_saradc.c
> > +++ b/drivers/iio/adc/meson_saradc.c
> > @@ -18,6 +18,7 @@
> >  #include <linux/io.h>
> >  #include <linux/iio/iio.h>
> >  #include <linux/module.h>
> > +#include <linux/nvmem-consumer.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/of.h>
> >  #include <linux/of_irq.h>
> > @@ -25,6 +26,7 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/regmap.h>
> >  #include <linux/regulator/consumer.h>
> > +#include <linux/mfd/syscon.h>
> >
> >  #define MESON_SAR_ADC_REG0                                   0x00
> >       #define MESON_SAR_ADC_REG0_PANEL_DETECT                 BIT(31)
> > @@ -165,6 +167,17 @@
> >
> >  #define MESON_SAR_ADC_MAX_FIFO_SIZE                          32
> >  #define MESON_SAR_ADC_TIMEOUT                                        100 /* ms */
> > +#define MESON_SAR_ADC_VOLTAGE_AND_TEMP_CHANNEL                       6
> > +#define MESON_SAR_ADC_TEMP_OFFSET                            27
> > +
> > +/* temperature sensor calibration information in eFuse */
> > +#define MESON_SAR_ADC_EFUSE_BYTES                            4
> > +#define MESON_SARADC_EFUSE_BYTE3_UPPER_ADC_VAL                       GENMASK(6, 0)
> > +#define MESON_SARADC_EFUSE_BYTE3_IS_CALIBRATED                       BIT(7)
>
> MESON_SAR_ADC_ prefix for consistency?
I missed that, thanks

> > +
> > +#define MESON_HHI_DPLL_TOP_0                                 0x318
> > +#define MESON_HHI_DPLL_TOP_0_TSC_BIT4                                BIT(9)
>
> I guess these do not belog to the SAR ADC at all?
yes and no
these register are part of the HHI register region which is shared the:
- clock controller
- HDMI controller
- IIRC the HDMI PHY as well
- one TSC bit for the SAR ADC
- ...possibly more

currently there's no header file for all the HHI includes because (as
far as I'm aware) each register is only used by one IP block

> > +
> >  /* for use with IIO_VAL_INT_PLUS_MICRO */
> >  #define MILLION                                                      1000000
> >
> > @@ -175,16 +188,27 @@
> >       .address = _chan,                                               \
> >       .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |                  \
> >                               BIT(IIO_CHAN_INFO_AVERAGE_RAW),         \
> > -     .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |          \
> > -                             BIT(IIO_CHAN_INFO_CALIBBIAS) |          \
> > +     .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),           \
> > +     .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_CALIBBIAS) |       \
> >                               BIT(IIO_CHAN_INFO_CALIBSCALE),          \
> >       .datasheet_name = "SAR_ADC_CH"#_chan,                           \
> >  }
> >
> > -/*
> > - * TODO: the hardware supports IIO_TEMP for channel 6 as well which is
> > - * currently not supported by this driver.
> > - */
> > +#define MESON_SAR_ADC_TEMP_CHAN(_chan) {                             \
> > +     .type = IIO_TEMP,                                               \
> > +     .indexed = 0,                                                   \
>
> .indexed = 0 not needed
OK, I will drop this

> > +     .channel = _chan,                                               \
> > +     .address = MESON_SAR_ADC_VOLTAGE_AND_TEMP_CHANNEL,              \
> > +     .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |                  \
> > +                             BIT(IIO_CHAN_INFO_AVERAGE_RAW),         \
> > +     .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET) |         \
> > +                                     BIT(IIO_CHAN_INFO_SCALE) |      \
> > +                                     BIT(IIO_CHAN_INFO_ENABLE),      \
> > +     .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_CALIBBIAS) |       \
> > +                             BIT(IIO_CHAN_INFO_CALIBSCALE),          \
> > +     .datasheet_name = "TEMP_SENSOR",                                \
> > +}
> > +
> >  static const struct iio_chan_spec meson_sar_adc_iio_channels[] = {
> >       MESON_SAR_ADC_CHAN(0),
> >       MESON_SAR_ADC_CHAN(1),
> > @@ -197,6 +221,19 @@ static const struct iio_chan_spec meson_sar_adc_iio_channels[] = {
> >       IIO_CHAN_SOFT_TIMESTAMP(8),
> >  };
> >
> > +static const struct iio_chan_spec meson_sar_adc_and_temp_iio_channels[] = {
> > +     MESON_SAR_ADC_CHAN(0),
> > +     MESON_SAR_ADC_CHAN(1),
> > +     MESON_SAR_ADC_CHAN(2),
> > +     MESON_SAR_ADC_CHAN(3),
> > +     MESON_SAR_ADC_CHAN(4),
> > +     MESON_SAR_ADC_CHAN(5),
> > +     MESON_SAR_ADC_CHAN(6),
> > +     MESON_SAR_ADC_CHAN(7),
> > +     MESON_SAR_ADC_TEMP_CHAN(8),
> > +     IIO_CHAN_SOFT_TIMESTAMP(9),
> > +};
> > +
> >  enum meson_sar_adc_avg_mode {
> >       NO_AVERAGING = 0x0,
> >       MEAN_AVERAGING = 0x1,
> > @@ -225,6 +262,11 @@ struct meson_sar_adc_param {
> >       u32                                     bandgap_reg;
> >       unsigned int                            resolution;
> >       const struct regmap_config              *regmap_config;
> > +     u8                                      temperature_trimming_bits;
> > +     unsigned int                            temperature_multiplier;
> > +     unsigned int                            temperature_divider;
> > +     const struct iio_chan_spec              *channels;
> > +     unsigned int                            num_channels;
> >  };
> >
> >  struct meson_sar_adc_data {
> > @@ -246,6 +288,10 @@ struct meson_sar_adc_priv {
> >       struct completion                       done;
> >       int                                     calibbias;
> >       int                                     calibscale;
> > +     struct regmap                           *tsc_regmap;
> > +     bool                                    temperature_sensor_calibrated;
> > +     u8                                      temperature_sensor_coefficient;
> > +     u16                                     temperature_sensor_adc_val;
> >  };
> >
> >  static const struct regmap_config meson_sar_adc_regmap_config_gxbb = {
> > @@ -389,9 +435,17 @@ static void meson_sar_adc_enable_channel(struct iio_dev *indio_dev,
> >                          MESON_SAR_ADC_DETECT_IDLE_SW_IDLE_MUX_SEL_MASK,
> >                          regval);
> >
> > -     if (chan->address == 6)
> > -             regmap_update_bits(priv->regmap, MESON_SAR_ADC_DELTA_10,
> > -                                MESON_SAR_ADC_DELTA_10_TEMP_SEL, 0);
> > +     if (chan->address == MESON_SAR_ADC_VOLTAGE_AND_TEMP_CHANNEL) {
>
> quite repetitive, maybe (just a suggestion)
>                 u8 val = chan->type == IIO_TEMP ? MESON_SAR_ADC_DELTA_10_TEMP_SEL : 0;
>                 regmap_update_bits(priv->regmap, MESON_SAR_ADC_DELTA_10,
>                                    MESON_SAR_ADC_DELTA_10_TEMP_SEL, val);
thanks for the suggestion!
I already have a "regval" variable which we can use here
I'll simplify this in the next version - I'll keep the basic if/else
though instead of using a ternary operator (because "regval =
chan->type == IIO_TEMP ? MESON_SAR_ADC_DELTA_10_TEMP_SEL : 0;" would
exceed the line length limit of 80 chars)

> > +             if (chan->type == IIO_TEMP)
> > +                     regmap_update_bits(priv->regmap,
> > +                                        MESON_SAR_ADC_DELTA_10,
> > +                                        MESON_SAR_ADC_DELTA_10_TEMP_SEL,
> > +                                        MESON_SAR_ADC_DELTA_10_TEMP_SEL);
> > +             else
> > +                     regmap_update_bits(priv->regmap,
> > +                                        MESON_SAR_ADC_DELTA_10,
> > +                                        MESON_SAR_ADC_DELTA_10_TEMP_SEL, 0);
> > +     }
> >  }
[snip]


Regards
Martin

Re: [PATCH 2/7] iio: adc: meson-saradc: add support for the chip's temperature sensor

[Martin Blumenstingl]

Hi Jonathan,

On Sun, Oct 28, 2018 at 5:35 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sun, 28 Oct 2018 13:26:24 +0100
> Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:
>
> > Channel 6 of the SAR ADC can be switched between two inputs:
> > SAR_ADC_CH6 input (an actual pad on the SoC) and the signal from the
> > temperature sensor inside the SoC.
> >
> > To get usable results from the temperature sensor we need to read the
> > corresponding calibration data from the eFuse and pass it to the SAR ADC
> > (bits [3:0]) and (on Meson8b and Meson8m2) to a scratch register in the
> > HHI region.
> > If the temperature sensor is not calibrated (the eFuse data contains a
> > bit for this) then the driver will return -ENOTSUPP when trying to read
> > the temperature.
>
> If it is not supported I'd rather not see it at all.  Have two channel
> sets and pick the one without the channel (by all means report
> the lack of support in the kernel log).
noted, I'll change the code then

> >
> > This only enables the temperature sensor for the Meson8, Meson8b and
> > Meson8m2 SoCs because on the newer, 64-bit SoCs (GXBB, GXL and GXM) the
> > temperature sensor inside SAR ADC is firmware-controlled (by BL30, we
> > can simply use the SCPI hwmon driver to get the chip temperature).
> >
> > To keep the devicetree interface backwards compatible we simply skip the
> > temperature sensor initialization if no eFuse nvmem cell is passed via
> > devicetree.
> >
> > The public documentation for the SAR ADC IP block does not explain how
> > to use the registers to read the temperature. The logic from this patch
> > is based on reading and understanding Amlogic's GPL kernel sources.
> >
> > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>
> Only one significant comment on this (rest looks good to me),
> what are you using the ENABLE element for in the info_mask?
>
> I 'think' it's an indicator that the temperature sensor reads
> will always return -ENOSUP?  This is non standard so I'd definitely
> prefer us to do it the way I suggest above.
yeah, I thought it was a good idea to have an "enable" element if the
temperature sensor may or may not be supported
I'll remove it when using two separate channel sets

> Thanks,
>
> Jonathan
> ...
> > +#define MESON_SAR_ADC_TEMP_CHAN(_chan) {                             \
> > +     .type = IIO_TEMP,                                               \
> > +     .indexed = 0,                                                   \
> > +     .channel = _chan,                                               \
> > +     .address = MESON_SAR_ADC_VOLTAGE_AND_TEMP_CHANNEL,              \
> > +     .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |                  \
> > +                             BIT(IIO_CHAN_INFO_AVERAGE_RAW),         \
> > +     .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET) |         \
> > +                                     BIT(IIO_CHAN_INFO_SCALE) |      \
> > +                                     BIT(IIO_CHAN_INFO_ENABLE),      \
> ENABLE?
>
> > +     .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_CALIBBIAS) |       \
> > +                             BIT(IIO_CHAN_INFO_CALIBSCALE),          \
> > +     .datasheet_name = "TEMP_SENSOR",                                \
> > +}
> > +
> >  static const struct iio_chan_spec meson_sar_adc_iio_channels[] = {
> >       MESON_SAR_ADC_CHAN(0),
> >       MESON_SAR_ADC_CHAN(1),
> > @@ -197,6 +221,19 @@ static const struct iio_chan_spec meson_sar_adc_iio_channels[] = {
> >       IIO_CHAN_SOFT_TIMESTAMP(8),
> >  };
> >
> ...
> > @@ -555,17 +613,31 @@ static int meson_sar_adc_iio_info_read_raw(struct iio_dev *indio_dev,
> >               break;
> >
> >       case IIO_CHAN_INFO_SCALE:
> > -             ret = regulator_get_voltage(priv->vref);
> > -             if (ret < 0) {
> > -                     dev_err(indio_dev->dev.parent,
> > -                             "failed to get vref voltage: %d\n", ret);
> > -                     return ret;
> > +             if (chan->type == IIO_VOLTAGE) {
> > +                     ret = regulator_get_voltage(priv->vref);
> > +                     if (ret < 0) {
> > +                             dev_err(indio_dev->dev.parent,
> > +                                     "failed to get vref voltage: %d\n",
> > +                                     ret);
> > +                             return ret;
> > +                     }
> > +
> > +                     *val = ret / 1000;
> > +                     *val2 = priv->param->resolution;
> > +                     return IIO_VAL_FRACTIONAL_LOG2;
> > +             } else if (chan->type == IIO_TEMP) {
> > +                     /* SoC specific multiplier and divider */
> > +                     *val = priv->param->temperature_multiplier;
> > +                     *val2 = priv->param->temperature_divider;
> > +
> > +                     /* celsius to millicelsius */
> > +                     *val *= 1000;
> > +
> > +                     return IIO_VAL_FRACTIONAL;
> > +             } else {
> > +                     return -EINVAL;
> >               }
> >
> > -             *val = ret / 1000;
> > -             *val2 = priv->param->resolution;
> > -             return IIO_VAL_FRACTIONAL_LOG2;
> > -
> >       case IIO_CHAN_INFO_CALIBBIAS:
> >               *val = priv->calibbias;
> >               return IIO_VAL_INT;
> > @@ -575,6 +647,17 @@ static int meson_sar_adc_iio_info_read_raw(struct iio_dev *indio_dev,
> >               *val2 = priv->calibscale % MILLION;
> >               return IIO_VAL_INT_PLUS_MICRO;
> >
> > +     case IIO_CHAN_INFO_OFFSET:
> > +             *val = DIV_ROUND_CLOSEST(MESON_SAR_ADC_TEMP_OFFSET *
> > +                                      priv->param->temperature_divider,
> > +                                      priv->param->temperature_multiplier);
> > +             *val -= priv->temperature_sensor_adc_val;
> > +             return IIO_VAL_INT;
> > +
> > +     case IIO_CHAN_INFO_ENABLE:
>
> Hmm.  This is not a standard use of this attribute. It's normally only used
> for things that are 'counting' where we want to start and stop
> a cumulative counter.
indeed, as you suggested I'll drop this

> > +             *val = priv->temperature_sensor_calibrated;
> > +             return IIO_VAL_INT;
> > +
> >       default:
> >               return -EINVAL;
> >       }
> > @@ -625,6 +708,77 @@ static int meson_sar_adc_clk_init(struct iio_dev *indio_dev,
> >       return 0;
> >  }
> >
> > +static int meson_sar_adc_temp_sensor_init(struct iio_dev *indio_dev)
> > +{
> > +     struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
> > +     u8 *buf, trimming_bits, trimming_mask, upper_adc_val;
> > +     struct nvmem_cell *temperature_calib;
> > +     size_t read_len;
> > +     int ret;
> > +
> > +     temperature_calib = devm_nvmem_cell_get(&indio_dev->dev,
> > +                                             "temperature_calib");
> > +     if (IS_ERR(temperature_calib)) {
> > +             ret = PTR_ERR(temperature_calib);
> > +
> > +             /*
> > +              * leave the temperature sensor disabled if no calibration data
> > +              * was passed via nvmem-cells.
> > +              */
> > +             if (ret == -ENODEV)
> > +                     return 0;
> > +
> > +             if (ret != -EPROBE_DEFER)
> > +                     dev_err(indio_dev->dev.parent,
> > +                             "failed to get temperature_calib cell\n");
> > +
> > +             return ret;
> > +     }
> > +
> > +     priv->tsc_regmap = syscon_regmap_lookup_by_phandle(
> > +                                             indio_dev->dev.parent->of_node,
> > +                                             "amlogic,hhi-sysctrl");
> > +     if (IS_ERR(priv->tsc_regmap)) {
> > +             dev_err(indio_dev->dev.parent,
> > +                     "failed to get amlogic,hhi-sysctrl regmap\n");
> > +             return PTR_ERR(priv->tsc_regmap);
> > +     }
> > +
> > +     read_len = MESON_SAR_ADC_EFUSE_BYTES;
> > +     buf = nvmem_cell_read(temperature_calib, &read_len);
> > +     if (IS_ERR(buf)) {
> > +             dev_err(indio_dev->dev.parent,
> > +                     "failed to read temperature_calib cell\n");
> > +             return PTR_ERR(buf);
> > +     } else if (read_len != MESON_SAR_ADC_EFUSE_BYTES) {
> > +             kfree(buf);
> > +             dev_err(indio_dev->dev.parent,
> > +                     "invalid read size of temperature_calib cell\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     trimming_bits = priv->param->temperature_trimming_bits;
> > +     trimming_mask = BIT(trimming_bits) - 1;
> > +
> > +     if (buf[3] & MESON_SARADC_EFUSE_BYTE3_IS_CALIBRATED)
> > +             priv->temperature_sensor_calibrated = true;
> > +     else
> > +             priv->temperature_sensor_calibrated = false;
>
> Could just assign?
>         priv->temperature_sensor_calibrated =
>                 buf[3] & MEESON_SARADC_EFUSE_BYTE3_IS_CALIBRATED;
I could change this but (personal preference here) my eyes are not
trained to read variable assignments where the value is broken into a
new line
looking back at it I could also remove the whole "else" block as
"false" is obviously the default value

let me know whether you'd like me to change this, then I'll do that for v2

> > +
> > +     priv->temperature_sensor_coefficient = buf[2] & trimming_mask;
> > +
> > +     upper_adc_val = FIELD_GET(MESON_SARADC_EFUSE_BYTE3_UPPER_ADC_VAL,
> > +                               buf[3]);
> > +
> > +     priv->temperature_sensor_adc_val = buf[2];
> > +     priv->temperature_sensor_adc_val |= upper_adc_val << BITS_PER_BYTE;
> > +     priv->temperature_sensor_adc_val >>= trimming_bits;
> > +
> > +     kfree(buf);
> > +
> > +     return 0;
> > +}
> > +
> ...


Regards
Martin

[PATCH 2/7] iio: adc: meson-saradc: add support for the chip's temperature sensor

[Martin Blumenstingl]

Hi Jonathan,

On Sun, Oct 28, 2018 at 5:35 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sun, 28 Oct 2018 13:26:24 +0100
> Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:
>
> > Channel 6 of the SAR ADC can be switched between two inputs:
> > SAR_ADC_CH6 input (an actual pad on the SoC) and the signal from the
> > temperature sensor inside the SoC.
> >
> > To get usable results from the temperature sensor we need to read the
> > corresponding calibration data from the eFuse and pass it to the SAR ADC
> > (bits [3:0]) and (on Meson8b and Meson8m2) to a scratch register in the
> > HHI region.
> > If the temperature sensor is not calibrated (the eFuse data contains a
> > bit for this) then the driver will return -ENOTSUPP when trying to read
> > the temperature.
>
> If it is not supported I'd rather not see it at all.  Have two channel
> sets and pick the one without the channel (by all means report
> the lack of support in the kernel log).
noted, I'll change the code then

> >
> > This only enables the temperature sensor for the Meson8, Meson8b and
> > Meson8m2 SoCs because on the newer, 64-bit SoCs (GXBB, GXL and GXM) the
> > temperature sensor inside SAR ADC is firmware-controlled (by BL30, we
> > can simply use the SCPI hwmon driver to get the chip temperature).
> >
> > To keep the devicetree interface backwards compatible we simply skip the
> > temperature sensor initialization if no eFuse nvmem cell is passed via
> > devicetree.
> >
> > The public documentation for the SAR ADC IP block does not explain how
> > to use the registers to read the temperature. The logic from this patch
> > is based on reading and understanding Amlogic's GPL kernel sources.
> >
> > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>
> Only one significant comment on this (rest looks good to me),
> what are you using the ENABLE element for in the info_mask?
>
> I 'think' it's an indicator that the temperature sensor reads
> will always return -ENOSUP?  This is non standard so I'd definitely
> prefer us to do it the way I suggest above.
yeah, I thought it was a good idea to have an "enable" element if the
temperature sensor may or may not be supported
I'll remove it when using two separate channel sets

> Thanks,
>
> Jonathan
> ...
> > +#define MESON_SAR_ADC_TEMP_CHAN(_chan) {                             \
> > +     .type = IIO_TEMP,                                               \
> > +     .indexed = 0,                                                   \
> > +     .channel = _chan,                                               \
> > +     .address = MESON_SAR_ADC_VOLTAGE_AND_TEMP_CHANNEL,              \
> > +     .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |                  \
> > +                             BIT(IIO_CHAN_INFO_AVERAGE_RAW),         \
> > +     .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET) |         \
> > +                                     BIT(IIO_CHAN_INFO_SCALE) |      \
> > +                                     BIT(IIO_CHAN_INFO_ENABLE),      \
> ENABLE?
>
> > +     .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_CALIBBIAS) |       \
> > +                             BIT(IIO_CHAN_INFO_CALIBSCALE),          \
> > +     .datasheet_name = "TEMP_SENSOR",                                \
> > +}
> > +
> >  static const struct iio_chan_spec meson_sar_adc_iio_channels[] = {
> >       MESON_SAR_ADC_CHAN(0),
> >       MESON_SAR_ADC_CHAN(1),
> > @@ -197,6 +221,19 @@ static const struct iio_chan_spec meson_sar_adc_iio_channels[] = {
> >       IIO_CHAN_SOFT_TIMESTAMP(8),
> >  };
> >
> ...
> > @@ -555,17 +613,31 @@ static int meson_sar_adc_iio_info_read_raw(struct iio_dev *indio_dev,
> >               break;
> >
> >       case IIO_CHAN_INFO_SCALE:
> > -             ret = regulator_get_voltage(priv->vref);
> > -             if (ret < 0) {
> > -                     dev_err(indio_dev->dev.parent,
> > -                             "failed to get vref voltage: %d\n", ret);
> > -                     return ret;
> > +             if (chan->type == IIO_VOLTAGE) {
> > +                     ret = regulator_get_voltage(priv->vref);
> > +                     if (ret < 0) {
> > +                             dev_err(indio_dev->dev.parent,
> > +                                     "failed to get vref voltage: %d\n",
> > +                                     ret);
> > +                             return ret;
> > +                     }
> > +
> > +                     *val = ret / 1000;
> > +                     *val2 = priv->param->resolution;
> > +                     return IIO_VAL_FRACTIONAL_LOG2;
> > +             } else if (chan->type == IIO_TEMP) {
> > +                     /* SoC specific multiplier and divider */
> > +                     *val = priv->param->temperature_multiplier;
> > +                     *val2 = priv->param->temperature_divider;
> > +
> > +                     /* celsius to millicelsius */
> > +                     *val *= 1000;
> > +
> > +                     return IIO_VAL_FRACTIONAL;
> > +             } else {
> > +                     return -EINVAL;
> >               }
> >
> > -             *val = ret / 1000;
> > -             *val2 = priv->param->resolution;
> > -             return IIO_VAL_FRACTIONAL_LOG2;
> > -
> >       case IIO_CHAN_INFO_CALIBBIAS:
> >               *val = priv->calibbias;
> >               return IIO_VAL_INT;
> > @@ -575,6 +647,17 @@ static int meson_sar_adc_iio_info_read_raw(struct iio_dev *indio_dev,
> >               *val2 = priv->calibscale % MILLION;
> >               return IIO_VAL_INT_PLUS_MICRO;
> >
> > +     case IIO_CHAN_INFO_OFFSET:
> > +             *val = DIV_ROUND_CLOSEST(MESON_SAR_ADC_TEMP_OFFSET *
> > +                                      priv->param->temperature_divider,
> > +                                      priv->param->temperature_multiplier);
> > +             *val -= priv->temperature_sensor_adc_val;
> > +             return IIO_VAL_INT;
> > +
> > +     case IIO_CHAN_INFO_ENABLE:
>
> Hmm.  This is not a standard use of this attribute. It's normally only used
> for things that are 'counting' where we want to start and stop
> a cumulative counter.
indeed, as you suggested I'll drop this

> > +             *val = priv->temperature_sensor_calibrated;
> > +             return IIO_VAL_INT;
> > +
> >       default:
> >               return -EINVAL;
> >       }
> > @@ -625,6 +708,77 @@ static int meson_sar_adc_clk_init(struct iio_dev *indio_dev,
> >       return 0;
> >  }
> >
> > +static int meson_sar_adc_temp_sensor_init(struct iio_dev *indio_dev)
> > +{
> > +     struct meson_sar_adc_priv *priv = iio_priv(indio_dev);
> > +     u8 *buf, trimming_bits, trimming_mask, upper_adc_val;
> > +     struct nvmem_cell *temperature_calib;
> > +     size_t read_len;
> > +     int ret;
> > +
> > +     temperature_calib = devm_nvmem_cell_get(&indio_dev->dev,
> > +                                             "temperature_calib");
> > +     if (IS_ERR(temperature_calib)) {
> > +             ret = PTR_ERR(temperature_calib);
> > +
> > +             /*
> > +              * leave the temperature sensor disabled if no calibration data
> > +              * was passed via nvmem-cells.
> > +              */
> > +             if (ret == -ENODEV)
> > +                     return 0;
> > +
> > +             if (ret != -EPROBE_DEFER)
> > +                     dev_err(indio_dev->dev.parent,
> > +                             "failed to get temperature_calib cell\n");
> > +
> > +             return ret;
> > +     }
> > +
> > +     priv->tsc_regmap = syscon_regmap_lookup_by_phandle(
> > +                                             indio_dev->dev.parent->of_node,
> > +                                             "amlogic,hhi-sysctrl");
> > +     if (IS_ERR(priv->tsc_regmap)) {
> > +             dev_err(indio_dev->dev.parent,
> > +                     "failed to get amlogic,hhi-sysctrl regmap\n");
> > +             return PTR_ERR(priv->tsc_regmap);
> > +     }
> > +
> > +     read_len = MESON_SAR_ADC_EFUSE_BYTES;
> > +     buf = nvmem_cell_read(temperature_calib, &read_len);
> > +     if (IS_ERR(buf)) {
> > +             dev_err(indio_dev->dev.parent,
> > +                     "failed to read temperature_calib cell\n");
> > +             return PTR_ERR(buf);
> > +     } else if (read_len != MESON_SAR_ADC_EFUSE_BYTES) {
> > +             kfree(buf);
> > +             dev_err(indio_dev->dev.parent,
> > +                     "invalid read size of temperature_calib cell\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     trimming_bits = priv->param->temperature_trimming_bits;
> > +     trimming_mask = BIT(trimming_bits) - 1;
> > +
> > +     if (buf[3] & MESON_SARADC_EFUSE_BYTE3_IS_CALIBRATED)
> > +             priv->temperature_sensor_calibrated = true;
> > +     else
> > +             priv->temperature_sensor_calibrated = false;
>
> Could just assign?
>         priv->temperature_sensor_calibrated =
>                 buf[3] & MEESON_SARADC_EFUSE_BYTE3_IS_CALIBRATED;
I could change this but (personal preference here) my eyes are not
trained to read variable assignments where the value is broken into a
new line
looking back at it I could also remove the whole "else" block as
"false" is obviously the default value

let me know whether you'd like me to change this, then I'll do that for v2

> > +
> > +     priv->temperature_sensor_coefficient = buf[2] & trimming_mask;
> > +
> > +     upper_adc_val = FIELD_GET(MESON_SARADC_EFUSE_BYTE3_UPPER_ADC_VAL,
> > +                               buf[3]);
> > +
> > +     priv->temperature_sensor_adc_val = buf[2];
> > +     priv->temperature_sensor_adc_val |= upper_adc_val << BITS_PER_BYTE;
> > +     priv->temperature_sensor_adc_val >>= trimming_bits;
> > +
> > +     kfree(buf);
> > +
> > +     return 0;
> > +}
> > +
> ...


Regards
Martin

Re: [PATCH 2/7] iio: adc: meson-saradc: add support for the chip's temperature sensor

[Jonathan Cameron]

On Sun, 28 Oct 2018 18:02:28 +0100
Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:

Hi Martin,
..

> > > +     if (buf[3] & MESON_SARADC_EFUSE_BYTE3_IS_CALIBRATED)
> > > +             priv->temperature_sensor_calibrated = true;
> > > +     else
> > > +             priv->temperature_sensor_calibrated = false;  
> >
> > Could just assign?
> >         priv->temperature_sensor_calibrated =
> >                 buf[3] & MEESON_SARADC_EFUSE_BYTE3_IS_CALIBRATED;  
> I could change this but (personal preference here) my eyes are not
> trained to read variable assignments where the value is broken into a
> new line
> looking back at it I could also remove the whole "else" block as
> "false" is obviously the default value
> 
> let me know whether you'd like me to change this, then I'll do that for v2

I don't feel strongly on any of the options.. Take your pick!

> 
> > > +
> > > +     priv->temperature_sensor_coefficient = buf[2] & trimming_mask;
> > > +
> > > +     upper_adc_val = FIELD_GET(MESON_SARADC_EFUSE_BYTE3_UPPER_ADC_VAL,
> > > +                               buf[3]);
> > > +
> > > +     priv->temperature_sensor_adc_val = buf[2];
> > > +     priv->temperature_sensor_adc_val |= upper_adc_val << BITS_PER_BYTE;
> > > +     priv->temperature_sensor_adc_val >>= trimming_bits;
> > > +
> > > +     kfree(buf);
> > > +
> > > +     return 0;
> > > +}
> > > +  
> > ...  
> 
> 
> Regards
> Martin

[PATCH 2/7] iio: adc: meson-saradc: add support for the chip's temperature sensor

[Jonathan Cameron]

On Sun, 28 Oct 2018 18:02:28 +0100
Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:

Hi Martin,
..

> > > +     if (buf[3] & MESON_SARADC_EFUSE_BYTE3_IS_CALIBRATED)
> > > +             priv->temperature_sensor_calibrated = true;
> > > +     else
> > > +             priv->temperature_sensor_calibrated = false;  
> >
> > Could just assign?
> >         priv->temperature_sensor_calibrated =
> >                 buf[3] & MEESON_SARADC_EFUSE_BYTE3_IS_CALIBRATED;  
> I could change this but (personal preference here) my eyes are not
> trained to read variable assignments where the value is broken into a
> new line
> looking back at it I could also remove the whole "else" block as
> "false" is obviously the default value
> 
> let me know whether you'd like me to change this, then I'll do that for v2

I don't feel strongly on any of the options.. Take your pick!

> 
> > > +
> > > +     priv->temperature_sensor_coefficient = buf[2] & trimming_mask;
> > > +
> > > +     upper_adc_val = FIELD_GET(MESON_SARADC_EFUSE_BYTE3_UPPER_ADC_VAL,
> > > +                               buf[3]);
> > > +
> > > +     priv->temperature_sensor_adc_val = buf[2];
> > > +     priv->temperature_sensor_adc_val |= upper_adc_val << BITS_PER_BYTE;
> > > +     priv->temperature_sensor_adc_val >>= trimming_bits;
> > > +
> > > +     kfree(buf);
> > > +
> > > +     return 0;
> > > +}
> > > +  
> > ...  
> 
> 
> Regards
> Martin