Re: [PATCH v5 7/9] thermal: qcom: add support for adc-tm5 PMIC thermal monitor

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Dmitry,

Recently I was testing the ADC_TM driver added with these changes on SC7180 and I could see that this patch needs a few changes, which I'll mention below.

On 9/14/2020 9:18 PM, Dmitry Baryshkov wrote:
Add support for Thermal Monitoring part of PMIC5. This part is closely
coupled with ADC, using it's channels directly. ADC-TM support
generating interrupts on ADC value crossing low or high voltage bounds,
which is used to support thermal trip points.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
---
  drivers/iio/adc/qcom-vadc-common.c       |  62 +++
  drivers/iio/adc/qcom-vadc-common.h       |   3 +
  drivers/thermal/qcom/Kconfig             |  11 +
  drivers/thermal/qcom/Makefile            |   1 +
  drivers/thermal/qcom/qcom-spmi-adc-tm5.c | 583 +++++++++++++++++++++++
  5 files changed, 660 insertions(+)
  create mode 100644 drivers/thermal/qcom/qcom-spmi-adc-tm5.c

diff --git a/drivers/iio/adc/qcom-vadc-common.c b/drivers/iio/adc/qcom-vadc-common.c
index 40d77b3af1bb..e58e393b8713 100644
--- a/drivers/iio/adc/qcom-vadc-common.c
+++ b/drivers/iio/adc/qcom-vadc-common.c
@@ -377,6 +377,42 @@ static int qcom_vadc_map_voltage_temp(const struct vadc_map_pt *pts,
  	return 0;
  }


+static irqreturn_t adc_tm5_isr(int irq, void *data)
+{
+	struct adc_tm5_chip *chip = data;
+	u8 status_low, status_high, ctl;
+	int ret = 0, i = 0;
+
+	ret = adc_tm5_read(chip, ADC_TM5_STATUS_LOW, &status_low, 1);
+	if (ret) {
+		dev_err(chip->dev, "read status low failed with %d\n", ret);
+		return IRQ_HANDLED;
+	}
+
+	ret = adc_tm5_read(chip, ADC_TM5_STATUS_HIGH, &status_high, 1);
+	if (ret) {
+		dev_err(chip->dev, "read status high failed with %d\n", ret);
+		return IRQ_HANDLED;
+	}
+
+	for (i = 0; i < chip->nchannels; i++) {
+		bool upper_set = false, lower_set = false;
+		unsigned int ch = chip->channels[i].channel;
+
+		if (!chip->channels[i].tzd) {
+			dev_err_once(chip->dev, "thermal device not found\n");
+			continue;
+		}
+
+		ret = adc_tm5_read(chip, ADC_TM5_M_EN(ch), &ctl, 1);
+
+		if (ret) {
+			dev_err(chip->dev, "ctl read failed with %d\n", ret);
+			continue;
+		}
+
+		lower_set = (status_low & BIT(ch)) &&
+			(ctl & ADC_TM5_M_MEAS_EN) &&
+			(ctl & ADC_TM5_M_LOW_THR_INT_EN);
+
+		upper_set = (status_high & BIT(ch)) &&
+			(ctl & ADC_TM5_M_MEAS_EN) &&
+			(ctl & ADC_TM5_M_HIGH_THR_INT_EN);
+
+		if (upper_set || lower_set)
+			thermal_zone_device_update(chip->channels[i].tzd,
+						   THERMAL_EVENT_UNSPECIFIED);

When using thermal_zone_device_update here, it internally calls tz->ops->get_temp, which maps to adc_tm5_get_temp defined just below. This in turn calls iio_read_channel_processed, which internally calls a mutex and this results in a mutex being called from atomic context.

To avoid this, the interrupt should be requested as a threaded IRQ.

+	}
+
+	return IRQ_HANDLED;
+}
+
+static int adc_tm5_get_temp(void *data, int *temp)
+{
+	struct adc_tm5_channel *channel = data;
+	int ret, milli_celsius;
+
+	if (!channel || !channel->iio)
+		return -EINVAL;
+
+	ret = iio_read_channel_processed(channel->iio, &milli_celsius);
+	if (ret < 0)
+		return ret;
+
+	*temp = milli_celsius;
+
+	return 0;
+}
+
+static int adc_tm5_disable_channel(struct adc_tm5_channel *channel)
+{
+	struct adc_tm5_chip *chip = channel->chip;
+	unsigned int reg = ADC_TM5_M_EN(channel->channel);
+
+	return adc_tm5_reg_update(chip, reg,
+			ADC_TM5_M_MEAS_EN | ADC_TM5_M_HIGH_THR_INT_EN | ADC_TM5_M_LOW_THR_INT_EN,
+			0);
+}
+
+static int adc_tm5_configure(struct adc_tm5_channel *channel, int low_temp, int high_temp)
+{
+	struct adc_tm5_chip *chip = channel->chip;
+	u8 buf[8];
+	u16 reg = ADC_TM5_M_ADC_CH_SEL_CTL(channel->channel);
+	int ret = 0;
+
+	ret = adc_tm5_read(chip, reg, buf, sizeof(buf));
+	if (ret) {
+		dev_err(chip->dev, "block read failed with %d\n", ret);
+		return ret;
+	}
+
+	/* Update ADC channel select */
+	buf[0] = channel->adc_channel;
+
+	/* Warm temperature corresponds to low voltage threshold */
+	if (high_temp != INT_MAX) {
+		u16 adc_code = qcom_adc_tm5_temp_volt_scale(channel->prescale,
+				chip->data->full_scale_code_volt, high_temp);
+
+		buf[1] = adc_code & 0xff;
+		buf[2] = adc_code >> 8;
+		buf[7] |= ADC_TM5_M_LOW_THR_INT_EN;
+	}
+
+	/* Cool temperature corresponds to high voltage threshold */
+	if (low_temp != -INT_MAX) {
+		u16 adc_code = qcom_adc_tm5_temp_volt_scale(channel->prescale,
+				chip->data->full_scale_code_volt, low_temp);
+
+		buf[3] = adc_code & 0xff;
+		buf[4] = adc_code >> 8;
+		buf[7] |= ADC_TM5_M_HIGH_THR_INT_EN;
+	}
+
+	/* Update timer select */
+	buf[5] = ADC5_TIMER_SEL_2;
+
+	/* Set calibration select, hw_settle delay */
+	buf[6] &= ~ADC_TM5_M_CTL_HW_SETTLE_DELAY_MASK;
+	buf[6] |= FIELD_PREP(ADC_TM5_M_CTL_HW_SETTLE_DELAY_MASK, channel->hw_settle_time);
+	buf[6] &= ~ADC_TM5_M_CTL_CAL_SEL_MASK;
+	buf[6] |= FIELD_PREP(ADC_TM5_M_CTL_CAL_SEL_MASK, channel->cal_method);
+
+	buf[7] |= ADC_TM5_M_MEAS_EN;
+
+	ret = adc_tm5_write(chip, reg, buf, sizeof(buf));
+	if (ret)
+		dev_err(chip->dev, "buf write failed\n");
+
+	return ret;
+}
+
+static int adc_tm5_set_trips(void *data, int low_temp, int high_temp)
+{
+	struct adc_tm5_channel *channel = data;
+	struct adc_tm5_chip *chip;
+	int ret;
+
+	if (!channel)
+		return -EINVAL;
+
+	chip = channel->chip;
+	dev_dbg(chip->dev, "%d:low_temp(mdegC):%d, high_temp(mdegC):%d\n",
+		channel->channel, low_temp, high_temp);
+
+	if (high_temp == INT_MAX && low_temp <= -INT_MAX)
+		ret = adc_tm5_disable_channel(channel);
+	else
+		ret = adc_tm5_configure(channel, low_temp, high_temp);
In addition to the configurations done in adc_tm5_configure, you also need to write to the registers at 0x3546 (write 0x80 to enable the ADC_TM peripheral overall) and 0x3547 (this is the conversion request strobe, you need to write to bit 7 here too, to initiate the recurring measurements).
+
+	return ret;
+}
+



+static int adc_tm5_probe(struct platform_device *pdev)
+{
+	struct device_node *node = pdev->dev.of_node;
+	struct device *dev = &pdev->dev;
+	struct adc_tm5_chip *adc_tm;
+	struct regmap *regmap;
+	int ret, irq;
+	u32 reg;
+
+	regmap = dev_get_regmap(dev->parent, NULL);
+	if (!regmap)
+		return -ENODEV;
+
+	ret = of_property_read_u32(node, "reg", &reg);
+	if (ret)
+		return ret;
+
+	adc_tm = devm_kzalloc(&pdev->dev, sizeof(*adc_tm), GFP_KERNEL);
+	if (!adc_tm)
+		return -ENOMEM;
+
+	adc_tm->regmap = regmap;
+	adc_tm->dev = dev;
+	adc_tm->base = reg;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(dev, "get_irq failed: %d\n", irq);
+		return irq;
+	}
+
+	ret = adc_tm5_get_dt_data(adc_tm, node);
+	if (ret) {
+		dev_err(dev, "get dt data failed: %d\n", ret);
+		return ret;
+	}
+
+	ret = adc_tm5_init(adc_tm);
+	if (ret) {
+		dev_err(dev, "adc-tm init failed\n");
+		return ret;
+	}
+
+	ret = adc_tm5_register_tzd(adc_tm);
+	if (ret) {
+		dev_err(dev, "tzd register failed\n");
+		return ret;
+	}
+
+	return devm_request_irq(dev, irq, adc_tm5_isr, 0, "pm-adc-tm5", adc_tm);
The interrupt should be requested with devm_request_threaded_irq, with the existing interrupt handler being given as the threaded function, for the reason mentioned above earlier.
+}
+




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux