On 08/10/2020 19:22, Daniel Lezcano wrote:
On 07/10/2020 15:54, 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>
---
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 | 622 +++++++++++++++++++++++
5 files changed, 699 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 s32 qcom_vadc_map_temp_voltage(const struct vadc_map_pt *pts,
+ u32 tablesize, int input)
+{
+ bool descending = 1;
+ u32 i = 0;
+
The code seems like a bit
Could you please clarify, what do you mean?
+ /* Check if table is descending or ascending */
+ if (tablesize > 1) {
+ if (pts[0].y < pts[1].y)
+ descending = 0;
+ }
+
+ while (i < tablesize) {
+ if (descending && pts[i].y < input) {
+ /* table entry is less than measured*/
+ /* value and table is descending, stop */
+ break;
+ } else if ((!descending) && pts[i].y > input) {
+ /* table entry is greater than measured*/
+ /*value and table is ascending, stop */
+ break;
+ }
+ i++;
+ }
+
+ if (i == 0)
+ return pts[0].x;
+ if (i == tablesize)
+ return pts[tablesize - 1].x;
+
+ /* result is between search_index and search_index-1 */
+ /* interpolate linearly */
+ return fixp_linear_interpolate(pts[i - 1].y, pts[i - 1].x,
+ pts[i].y, pts[i].x, input);
+}
+
static void qcom_vadc_scale_calib(const struct vadc_linear_graph *calib_graph,
u16 adc_code,
bool absolute,
[....]
diff --git a/drivers/thermal/qcom/qcom-spmi-adc-tm5.c b/drivers/thermal/qcom/qcom-spmi-adc-tm5.c
new file mode 100644
index 000000000000..c09a50f59053
--- /dev/null
+++ b/drivers/thermal/qcom/qcom-spmi-adc-tm5.c
@@ -0,0 +1,622 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2012-2020, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2020 Linaro Limited
+ */
If it is possible, please give a description of this sensor, the
different register mapping, etc ... So it will be easier to review and
debug in the future.
In which form? I don't often see such descriptions in the code.
+#include <linux/bitfield.h>
+#include <linux/iio/consumer.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/thermal.h>
+
+#include "../../iio/adc/qcom-vadc-common.h"
Do not use this form of inclusion.
Fixed.
[...]
+ if (ret) {
+ dev_err(chip->dev, "read status low failed with %d\n", ret);
+ return IRQ_HANDLED;
+ }
Can you identify the reasons those reads can fail? If it is not supposed
to happen it is fine but otherwise we don't want to be flooded with
error messages on the console.
Changed to use unlikely(ret) as way to show that this is not supposed to
happen.
+ 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);
Is the check (ctl & ADC_TM5_M_[HIGH|LOW]_THR_INT_EN) necessary if
status_high or status_low is true ?
Isn't possible to simplify that with:
eg.
if (!(ctl & ADC_TM5_M_MEAS_EN)
continue;
if (!(status_high & BIT(ch)) && !(status_low & BIT(ch))
continue;
thermal_zone_device_update(chip->channels[i].tzd,
THERMAL_EVENT_UNSPECIFIED);
??
I'd prefer to leave the check as is, having no information if status bit
can be updated without actually triggering IRQ.
I've moved ADC_TM5_MEAS_EN check upwards to simplify this.
+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;
+ } else {
+ buf[7] &= ~ADC_TM5_M_LOW_THR_INT_EN;
+ }
+
+ /* Cool temperature corresponds to high voltage threshold */
+ if (low_temp != -INT_MAX) {
Is it really -INT_MAX ? or INT_MIN
-2147483647 vs -2147483648 ?
It is really -INT_MAX, see thermal_zone_set_trips().
[...]
+
+ for (i = 0; i < chip->nchannels; i++) {
+ if (chip->channels[i].channel >= channels_available) {
+ dev_err(chip->dev, "Invalid channel %d\n", chip->channels[i].channel);
+ return -EINVAL;
+ }
Is it a sanity check to make sure the hardware and the DT are compatible ?
Yes.
--
With best wishes
Dmitry
