* [PATCH 2/7] iio: adc: meson-saradc: add support for the chip's temperature sensor
@ 2018-10-28 12:26 ` Martin Blumenstingl
0 siblings, 0 replies; 26+ messages in thread
From: Martin Blumenstingl @ 2018-10-28 12:26 UTC (permalink / raw)
To: linus-amlogic
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
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH 2/7] iio: adc: meson-saradc: add support for the chip's temperature sensor
2018-10-28 12:26 ` Martin Blumenstingl
@ 2018-10-28 16:19 ` Peter Meerwald-Stadler
-1 siblings, 0 replies; 26+ messages in thread
From: Peter Meerwald-Stadler @ 2018-10-28 16:19 UTC (permalink / raw)
To: Martin Blumenstingl; +Cc: linux-amlogic, linux-iio, lars, jic23
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
^ permalink raw reply [flat|nested] 26+ messages in thread* [PATCH 2/7] iio: adc: meson-saradc: add support for the chip's temperature sensor
@ 2018-10-28 16:19 ` Peter Meerwald-Stadler
0 siblings, 0 replies; 26+ messages in thread
From: Peter Meerwald-Stadler @ 2018-10-28 16:19 UTC (permalink / raw)
To: linus-amlogic
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
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 2/7] iio: adc: meson-saradc: add support for the chip's temperature sensor
2018-10-28 16:19 ` Peter Meerwald-Stadler
@ 2018-10-28 16:47 ` Martin Blumenstingl
-1 siblings, 0 replies; 26+ messages in thread
From: Martin Blumenstingl @ 2018-10-28 16:47 UTC (permalink / raw)
To: pmeerw; +Cc: linux-amlogic, linux-iio, lars, jic23
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
^ permalink raw reply [flat|nested] 26+ messages in thread* [PATCH 2/7] iio: adc: meson-saradc: add support for the chip's temperature sensor
@ 2018-10-28 16:47 ` Martin Blumenstingl
0 siblings, 0 replies; 26+ messages in thread
From: Martin Blumenstingl @ 2018-10-28 16:47 UTC (permalink / raw)
To: linus-amlogic
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/7] iio: adc: meson-saradc: add support for the chip's temperature sensor
2018-10-28 12:26 ` Martin Blumenstingl
@ 2018-10-28 16:35 ` Jonathan Cameron
-1 siblings, 0 replies; 26+ messages in thread
From: Jonathan Cameron @ 2018-10-28 16:35 UTC (permalink / raw)
To: Martin Blumenstingl
Cc: linux-amlogic, devicetree, linux-iio, robh+dt, pmeerw, lars,
knaack.h, khilman, carlo
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;
> +}
> +
...
^ permalink raw reply [flat|nested] 26+ messages in thread* [PATCH 2/7] iio: adc: meson-saradc: add support for the chip's temperature sensor
@ 2018-10-28 16:35 ` Jonathan Cameron
0 siblings, 0 replies; 26+ messages in thread
From: Jonathan Cameron @ 2018-10-28 16:35 UTC (permalink / raw)
To: linus-amlogic
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;
> +}
> +
...
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 2/7] iio: adc: meson-saradc: add support for the chip's temperature sensor
2018-10-28 16:35 ` Jonathan Cameron
@ 2018-10-28 17:02 ` Martin Blumenstingl
-1 siblings, 0 replies; 26+ messages in thread
From: Martin Blumenstingl @ 2018-10-28 17:02 UTC (permalink / raw)
To: jic23
Cc: linux-amlogic, devicetree, linux-iio, robh+dt, pmeerw, lars,
knaack.h, khilman, carlo
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
^ permalink raw reply [flat|nested] 26+ messages in thread* [PATCH 2/7] iio: adc: meson-saradc: add support for the chip's temperature sensor
@ 2018-10-28 17:02 ` Martin Blumenstingl
0 siblings, 0 replies; 26+ messages in thread
From: Martin Blumenstingl @ 2018-10-28 17:02 UTC (permalink / raw)
To: linus-amlogic
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
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 2/7] iio: adc: meson-saradc: add support for the chip's temperature sensor
2018-10-28 17:02 ` Martin Blumenstingl
@ 2018-10-28 19:02 ` Jonathan Cameron
-1 siblings, 0 replies; 26+ messages in thread
From: Jonathan Cameron @ 2018-10-28 19:02 UTC (permalink / raw)
To: Martin Blumenstingl
Cc: linux-amlogic, devicetree, linux-iio, robh+dt, pmeerw, lars,
knaack.h, khilman, carlo
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 2/7] iio: adc: meson-saradc: add support for the chip's temperature sensor
@ 2018-10-28 19:02 ` Jonathan Cameron
0 siblings, 0 replies; 26+ messages in thread
From: Jonathan Cameron @ 2018-10-28 19:02 UTC (permalink / raw)
To: linus-amlogic
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
^ permalink raw reply [flat|nested] 26+ messages in thread