Add support for ADC_TM part of PMIC5 Gen3.
This is an auxiliary driver under the Gen3 ADC driver, which implements the
threshold setting and interrupt generating functionalities of QCOM ADC_TM
drivers, used to support thermal trip points.
Signed-off-by: Jishnu Prakash <jishnu.prakash@oss.qualcomm.com>
---
Changes since v7:
- Addressed following comments from Jonathan:
- Replaced {0} with { } in tm_handler_work()
- Simplified logic for setting upper_set and lower_set into
a single line each, in tm_handler_work()
- Cleaned up local variable declarations and high/low threshold
check in adc_tm5_gen3_configure()
- Moved cleanup action to disable all ADC_TM channels to probe
end and added comment to describe it.
- Fixed { } formatting in adctm5_auxiliary_id_table[].
Changes since v6:
- Addressed following comments from Jonathan:
- Added error check for devm_thermal_add_hwmon_sysfs() call.
- Used local variable `dev` in multiple places in adc_tm5_probe().
in place of `&aux_dev->dev` and `adc_tm5->dev`.
- Added a comment to explain cleanup action calling adc5_gen3_clear_work()
near probe end.
- Fixed return statement at probe end to return last called API's
return value directly.
Changes since v5:
- Addressed following comments from Jonathan:
- Corrected all files to follow kernel-doc formatting fully.
- Cleaned up formatting in struct definitions.
- Used sizeof() to specify length in register read/write calls
instead of using integers directly.
- Added comments in adc_tm5_probe() for skipping first SDAM for
IRQ request and for usage of auxiliary_set_drvdata().
- Corrected line wrap length driver file.
- Moved INIT_WORK() and auxiliary_set_drvdata() to earlier
locations to ensure they are ready when needed.
Changes since v4:
- Fixed a compilation error and updated dependencies in config as suggested
by Krzysztof.
drivers/thermal/qcom/Kconfig | 9 +
drivers/thermal/qcom/Makefile | 1 +
drivers/thermal/qcom/qcom-spmi-adc-tm5-gen3.c | 530 ++++++++++++++++++
3 files changed, 540 insertions(+)
create mode 100644 drivers/thermal/qcom/qcom-spmi-adc-tm5-gen3.c
diff --git a/drivers/thermal/qcom/Kconfig b/drivers/thermal/qcom/Kconfig
index a6bb01082ec6..1acb11e4ac80 100644
--- a/drivers/thermal/qcom/Kconfig
+++ b/drivers/thermal/qcom/Kconfig
@@ -21,6 +21,15 @@ config QCOM_SPMI_ADC_TM5
Thermal client sets threshold temperature for both warm and cool and
gets updated when a threshold is reached.
+config QCOM_SPMI_ADC_TM5_GEN3
+ tristate "Qualcomm SPMI PMIC Thermal Monitor ADC5 Gen3"
+ depends on QCOM_SPMI_ADC5_GEN3
+ help
+ This enables the auxiliary thermal driver for the ADC5 Gen3 thermal
+ monitoring device. It shows up as a thermal zone with multiple trip points.
+ Thermal client sets threshold temperature for both warm and cool and
+ gets updated when a threshold is reached.
+
config QCOM_SPMI_TEMP_ALARM
tristate "Qualcomm SPMI PMIC Temperature Alarm"
depends on OF && SPMI && IIO
diff --git a/drivers/thermal/qcom/Makefile b/drivers/thermal/qcom/Makefile
index 0fa2512042e7..828d9e7bc797 100644
--- a/drivers/thermal/qcom/Makefile
+++ b/drivers/thermal/qcom/Makefile
@@ -4,5 +4,6 @@ obj-$(CONFIG_QCOM_TSENS) += qcom_tsens.o
qcom_tsens-y += tsens.o tsens-v2.o tsens-v1.o tsens-v0_1.o \
tsens-8960.o
obj-$(CONFIG_QCOM_SPMI_ADC_TM5) += qcom-spmi-adc-tm5.o
+obj-$(CONFIG_QCOM_SPMI_ADC_TM5_GEN3) += qcom-spmi-adc-tm5-gen3.o
obj-$(CONFIG_QCOM_SPMI_TEMP_ALARM) += qcom-spmi-temp-alarm.o
obj-$(CONFIG_QCOM_LMH) += lmh.o
diff --git a/drivers/thermal/qcom/qcom-spmi-adc-tm5-gen3.c b/drivers/thermal/qcom/qcom-spmi-adc-tm5-gen3.c
new file mode 100644
index 000000000000..c6cc8ef76f7e
--- /dev/null
+++ b/drivers/thermal/qcom/qcom-spmi-adc-tm5-gen3.c
@@ -0,0 +1,530 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/iio/adc/qcom-adc5-gen3-common.h>
+#include <linux/iio/consumer.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/thermal.h>
+#include <linux/unaligned.h>
+
+#include "../thermal_hwmon.h"
+
+struct adc_tm5_gen3_chip;
+
+/**
+ * struct adc_tm5_gen3_channel_props - ADC_TM channel structure
+ * @timer: time period of recurring TM measurement.
+ * @tm_chan_index: TM channel number used (ranging from 1-7).
+ * @sdam_index: SDAM on which this TM channel lies.
+ * @common_props: structure withcommon ADC channel properties.
+ * @high_thr_en: TM high threshold crossing detection enabled.
+ * @low_thr_en: TM low threshold crossing detection enabled.
+ * @chip: ADC TM device.
+ * @tzd: pointer to thermal device corresponding to TM channel.
+ * @last_temp: last temperature that caused threshold violation,
+ * or a thermal TM channel.
+ * @last_temp_set: indicates if last_temp is stored.
+ */
+struct adc_tm5_gen3_channel_props {
+ unsigned int timer;
+ unsigned int tm_chan_index;
+ unsigned int sdam_index;
+ struct adc5_channel_common_prop common_props;
+ bool high_thr_en;
+ bool low_thr_en;
+ struct adc_tm5_gen3_chip *chip;
+ struct thermal_zone_device *tzd;
+ int last_temp;
+ bool last_temp_set;
+};
+
+/**
+ * struct adc_tm5_gen3_chip - ADC Thermal Monitoring device structure
+ * @dev_data: Top-level ADC device data.
+ * @chan_props: Array of ADC_TM channel structures.
+ * @nchannels: number of TM channels allocated
+ * @dev: SPMI ADC5 Gen3 device.
+ * @tm_handler_work: handler for TM interrupt for threshold violation.
+ */
+struct adc_tm5_gen3_chip {
+ struct adc5_device_data *dev_data;
+ struct adc_tm5_gen3_channel_props *chan_props;
+ unsigned int nchannels;
+ struct device *dev;
+ struct work_struct tm_handler_work;
+};
+
+static int get_sdam_from_irq(struct adc_tm5_gen3_chip *adc_tm5, int irq)
+{
+ int i;
+
+ for (i = 0; i < adc_tm5->dev_data->num_sdams; i++) {
+ if (adc_tm5->dev_data->base[i].irq == irq)
+ return i;
+ }
+ return -ENOENT;
+}
+
+static irqreturn_t adctm5_gen3_isr(int irq, void *dev_id)
+{
+ struct adc_tm5_gen3_chip *adc_tm5 = dev_id;
+ int ret, sdam_num;
+ u8 tm_status[2];
+ u8 status, val;
+
+ sdam_num = get_sdam_from_irq(adc_tm5, irq);
+ if (sdam_num < 0) {
+ dev_err(adc_tm5->dev, "adc irq %d not associated with an sdam\n",
+ irq);
+ return IRQ_HANDLED;
+ }
+
+ ret = adc5_gen3_read(adc_tm5->dev_data, sdam_num, ADC5_GEN3_STATUS1,
+ &status, sizeof(status));
+ if (ret) {
+ dev_err(adc_tm5->dev, "adc read status1 failed with %d\n", ret);
+ return IRQ_HANDLED;
+ }
+
+ if (status & ADC5_GEN3_STATUS1_CONV_FAULT) {
+ dev_err_ratelimited(adc_tm5->dev,
+ "Unexpected conversion fault, status:%#x\n",
+ status);
+ val = ADC5_GEN3_CONV_ERR_CLR_REQ;
+ adc5_gen3_status_clear(adc_tm5->dev_data, sdam_num,
+ ADC5_GEN3_CONV_ERR_CLR, &val, 1);
+ return IRQ_HANDLED;
+ }
+
+ ret = adc5_gen3_read(adc_tm5->dev_data, sdam_num, ADC5_GEN3_TM_HIGH_STS,
+ tm_status, sizeof(tm_status));
+ if (ret) {
+ dev_err(adc_tm5->dev, "adc read TM status failed with %d\n", ret);
+ return IRQ_HANDLED;
+ }
+
+ if (tm_status[0] || tm_status[1])
+ schedule_work(&adc_tm5->tm_handler_work);
+
+ dev_dbg(adc_tm5->dev, "Interrupt status:%#x, high:%#x, low:%#x\n",
+ status, tm_status[0], tm_status[1]);
+
+ return IRQ_HANDLED;
+}
+
+static int adc5_gen3_tm_status_check(struct adc_tm5_gen3_chip *adc_tm5,
+ int sdam_index, u8 *tm_status, u8 *buf)
+{
+ int ret;
+
+ ret = adc5_gen3_read(adc_tm5->dev_data, sdam_index, ADC5_GEN3_TM_HIGH_STS,
+ tm_status, 2);
+ if (ret) {
+ dev_err(adc_tm5->dev, "adc read TM status failed with %d\n", ret);
+ return ret;
+ }
+
+ ret = adc5_gen3_status_clear(adc_tm5->dev_data, sdam_index, ADC5_GEN3_TM_HIGH_STS_CLR,
+ tm_status, 2);
+ if (ret) {
+ dev_err(adc_tm5->dev, "adc status clear conv_req failed with %d\n",
+ ret);
+ return ret;
+ }
+
+ ret = adc5_gen3_read(adc_tm5->dev_data, sdam_index, ADC5_GEN3_CH_DATA0(0),
+ buf, 16);
+ if (ret)
+ dev_err(adc_tm5->dev, "adc read data failed with %d\n", ret);
+
+ return ret;
+}
+
+static void tm_handler_work(struct work_struct *work)
+{
+ struct adc_tm5_gen3_chip *adc_tm5 = container_of(work, struct adc_tm5_gen3_chip,
+ tm_handler_work);
+ struct adc_tm5_gen3_channel_props *chan_prop;
+ u8 tm_status[2] = { };
+ u8 buf[16] = { };
+ int sdam_index = -1;
+ int i, ret;
+
+ for (i = 0; i < adc_tm5->nchannels; i++) {
+ bool upper_set, lower_set;
+ int temp, offset;
+ u16 code = 0;
+
+ chan_prop = &adc_tm5->chan_props[i];
+ offset = chan_prop->tm_chan_index;
+
+ adc5_gen3_mutex_lock(adc_tm5->dev);
+ if (chan_prop->sdam_index != sdam_index) {
+ sdam_index = chan_prop->sdam_index;
+ ret = adc5_gen3_tm_status_check(adc_tm5, sdam_index,
+ tm_status, buf);
+ if (ret) {
+ adc5_gen3_mutex_unlock(adc_tm5->dev);
+ break;
+ }
+ }
+
+ upper_set = ((tm_status[0] & BIT(offset)) && chan_prop->high_thr_en);
+ lower_set = ((tm_status[1] & BIT(offset)) && chan_prop->low_thr_en);
+ adc5_gen3_mutex_unlock(adc_tm5->dev);
+
+ if (!(upper_set || lower_set))
+ continue;
+
+ code = get_unaligned_le16(&buf[2 * offset]);
+ pr_debug("ADC_TM threshold code:%#x\n", code);
+
+ ret = adc5_gen3_therm_code_to_temp(adc_tm5->dev,
+ &chan_prop->common_props,
+ code, &temp);
+ if (ret) {
+ dev_err(adc_tm5->dev,
+ "Invalid temperature reading, ret = %d, code=%#x\n",
+ ret, code);
+ continue;
+ }
+
+ chan_prop->last_temp = temp;
+ chan_prop->last_temp_set = true;
+ thermal_zone_device_update(chan_prop->tzd, THERMAL_TRIP_VIOLATED);
+ }
+}
+
+static int adc_tm5_gen3_get_temp(struct thermal_zone_device *tz, int *temp)
+{
+ struct adc_tm5_gen3_channel_props *prop = thermal_zone_device_priv(tz);
+ struct adc_tm5_gen3_chip *adc_tm5;
+
+ if (!prop || !prop->chip)
+ return -EINVAL;
+
+ adc_tm5 = prop->chip;
+
+ if (prop->last_temp_set) {
+ pr_debug("last_temp: %d\n", prop->last_temp);
+ prop->last_temp_set = false;
+ *temp = prop->last_temp;
+ return 0;
+ }
+
+ return adc5_gen3_get_scaled_reading(adc_tm5->dev, &prop->common_props,
+ temp);
+}
+
+static int _adc_tm5_gen3_disable_channel(struct adc_tm5_gen3_channel_props *prop)
+{
+ struct adc_tm5_gen3_chip *adc_tm5 = prop->chip;
+ int ret;
+ u8 val;
+
+ prop->high_thr_en = false;
+ prop->low_thr_en = false;
+
+ ret = adc5_gen3_poll_wait_hs(adc_tm5->dev_data, prop->sdam_index);
+ if (ret)
+ return ret;
+
+ val = BIT(prop->tm_chan_index);
+ ret = adc5_gen3_write(adc_tm5->dev_data, prop->sdam_index,
+ ADC5_GEN3_TM_HIGH_STS_CLR, &val, sizeof(val));
+ if (ret)
+ return ret;
+
+ val = MEAS_INT_DISABLE;
+ ret = adc5_gen3_write(adc_tm5->dev_data, prop->sdam_index,
+ ADC5_GEN3_TIMER_SEL, &val, sizeof(val));
+ if (ret)
+ return ret;
+
+ /* To indicate there is an actual conversion request */
+ val = ADC5_GEN3_CHAN_CONV_REQ | prop->tm_chan_index;
+ ret = adc5_gen3_write(adc_tm5->dev_data, prop->sdam_index,
+ ADC5_GEN3_PERPH_CH, &val, sizeof(val));
+ if (ret)
+ return ret;
+
+ val = ADC5_GEN3_CONV_REQ_REQ;
+ return adc5_gen3_write(adc_tm5->dev_data, prop->sdam_index,
+ ADC5_GEN3_CONV_REQ, &val, sizeof(val));
+}
+
+static int adc_tm5_gen3_disable_channel(struct adc_tm5_gen3_channel_props *prop)
+{
+ return _adc_tm5_gen3_disable_channel(prop);
+}
+
+#define ADC_TM5_GEN3_CONFIG_REGS 12
+
+static int adc_tm5_gen3_configure(struct adc_tm5_gen3_channel_props *prop,
+ int low_temp, int high_temp)
+{
+ struct adc_tm5_gen3_chip *adc_tm5 = prop->chip;
+ u8 buf[ADC_TM5_GEN3_CONFIG_REGS];
+ u8 conv_req;
+ u16 adc_code;
+ int ret;
+
+ ret = adc5_gen3_poll_wait_hs(adc_tm5->dev_data, prop->sdam_index);
+ if (ret < 0)
+ return ret;
+
+ ret = adc5_gen3_read(adc_tm5->dev_data, prop->sdam_index,
+ ADC5_GEN3_SID, buf, sizeof(buf));
+ if (ret < 0)
+ return ret;
+
+ /* Write SID */
+ buf[0] = FIELD_PREP(ADC5_GEN3_SID_MASK, prop->common_props.sid);
+
+ /* Select TM channel and indicate there is an actual conversion request */
+ buf[1] = ADC5_GEN3_CHAN_CONV_REQ | prop->tm_chan_index;
+
+ buf[2] = prop->timer;
+
+ /* Digital param selection */
+ adc5_gen3_update_dig_param(&prop->common_props, &buf[3]);
+
+ /* Update fast average sample value */
+ buf[4] &= ~ADC5_GEN3_FAST_AVG_CTL_SAMPLES_MASK;
+ buf[4] |= prop->common_props.avg_samples | ADC5_GEN3_FAST_AVG_CTL_EN;
+
+ /* Select ADC channel */
+ buf[5] = prop->common_props.channel;
+
+ /* Select HW settle delay for channel */
+ buf[6] = FIELD_PREP(ADC5_GEN3_HW_SETTLE_DELAY_MASK,
+ prop->common_props.hw_settle_time_us);
+
+ /* High temperature corresponds to low voltage threshold */
+ prop->low_thr_en = (high_temp != INT_MAX);
+ if (prop->low_thr_en) {
+ adc_code = qcom_adc_tm5_gen2_temp_res_scale(high_temp);
+ put_unaligned_le16(adc_code, &buf[8]);
+ }
+
+ /* Low temperature corresponds to high voltage threshold */
+ prop->high_thr_en = (low_temp != -INT_MAX);
+ if (prop->high_thr_en) {
+ adc_code = qcom_adc_tm5_gen2_temp_res_scale(low_temp);
+ put_unaligned_le16(adc_code, &buf[10]);
+ }
+
+ buf[7] = 0;
+ if (prop->high_thr_en)
+ buf[7] |= ADC5_GEN3_HIGH_THR_INT_EN;
+ if (prop->low_thr_en)
+ buf[7] |= ADC5_GEN3_LOW_THR_INT_EN;
+
+ ret = adc5_gen3_write(adc_tm5->dev_data, prop->sdam_index, ADC5_GEN3_SID,
+ buf, sizeof(buf));
+ if (ret < 0)
+ return ret;
+
+ conv_req = ADC5_GEN3_CONV_REQ_REQ;
+ return adc5_gen3_write(adc_tm5->dev_data, prop->sdam_index,
+ ADC5_GEN3_CONV_REQ, &conv_req, sizeof(conv_req));
+}
+
+static int adc_tm5_gen3_set_trip_temp(struct thermal_zone_device *tz,
+ int low_temp, int high_temp)
+{
+ struct adc_tm5_gen3_channel_props *prop = thermal_zone_device_priv(tz);
+ struct adc_tm5_gen3_chip *adc_tm5;
+ int ret;
+
+ if (!prop || !prop->chip)
+ return -EINVAL;
+
+ adc_tm5 = prop->chip;
+
+ dev_dbg(adc_tm5->dev, "channel:%s, low_temp(mdegC):%d, high_temp(mdegC):%d\n",
+ prop->common_props.label, low_temp, high_temp);
+
+ adc5_gen3_mutex_lock(adc_tm5->dev);
+ if (high_temp == INT_MAX && low_temp <= -INT_MAX)
+ ret = adc_tm5_gen3_disable_channel(prop);
+ else
+ ret = adc_tm5_gen3_configure(prop, low_temp, high_temp);
+ adc5_gen3_mutex_unlock(adc_tm5->dev);
+
+ return ret;
+}
+
+static const struct thermal_zone_device_ops adc_tm_ops = {
+ .get_temp = adc_tm5_gen3_get_temp,
+ .set_trips = adc_tm5_gen3_set_trip_temp,
+};
+
+static int adc_tm5_register_tzd(struct adc_tm5_gen3_chip *adc_tm5)
+{
+ unsigned int i, channel;
+ struct thermal_zone_device *tzd;
+ int ret;
+
+ for (i = 0; i < adc_tm5->nchannels; i++) {
+ channel = ADC5_GEN3_V_CHAN(adc_tm5->chan_props[i].common_props);
+ tzd = devm_thermal_of_zone_register(adc_tm5->dev, channel,
+ &adc_tm5->chan_props[i],
+ &adc_tm_ops);
+
+ if (IS_ERR(tzd)) {
+ if (PTR_ERR(tzd) == -ENODEV) {
+ dev_warn(adc_tm5->dev,
+ "thermal sensor on channel %d is not used\n",
+ channel);
+ continue;
+ }
+ return dev_err_probe(adc_tm5->dev, PTR_ERR(tzd),
+ "Error registering TZ zone:%ld for channel:%d\n",
+ PTR_ERR(tzd), channel);
+ }
+ adc_tm5->chan_props[i].tzd = tzd;
+ ret = devm_thermal_add_hwmon_sysfs(adc_tm5->dev, tzd);
+ if (ret)
+ return ret;
+ }
+ return 0;
+}
+
+static void adc5_gen3_clear_work(void *data)
+{
+ struct adc_tm5_gen3_chip *adc_tm5 = data;
+
+ cancel_work_sync(&adc_tm5->tm_handler_work);
+}
+
+static void adc5_gen3_disable(void *data)
+{
+ struct adc_tm5_gen3_chip *adc_tm5 = data;
+ int i;
+
+ adc5_gen3_mutex_lock(adc_tm5->dev);
+ /* Disable all available TM channels */
+ for (i = 0; i < adc_tm5->nchannels; i++)
+ _adc_tm5_gen3_disable_channel(&adc_tm5->chan_props[i]);
+
+ adc5_gen3_mutex_unlock(adc_tm5->dev);
+}
+
+static void adctm_event_handler(struct auxiliary_device *adev)
+{
+ struct adc_tm5_gen3_chip *adc_tm5 = auxiliary_get_drvdata(adev);
+
+ schedule_work(&adc_tm5->tm_handler_work);
+}
+
+static int adc_tm5_probe(struct auxiliary_device *aux_dev,
+ const struct auxiliary_device_id *id)
+{
+ struct adc_tm5_gen3_chip *adc_tm5;
+ struct tm5_aux_dev_wrapper *aux_dev_wrapper;
+ struct device *dev = &aux_dev->dev;
+ int i, ret;
+
+ adc_tm5 = devm_kzalloc(dev, sizeof(*adc_tm5), GFP_KERNEL);
+ if (!adc_tm5)
+ return -ENOMEM;
+
+ aux_dev_wrapper = container_of(aux_dev, struct tm5_aux_dev_wrapper,
+ aux_dev);
+
+ adc_tm5->dev = dev;
+ adc_tm5->dev_data = aux_dev_wrapper->dev_data;
+ adc_tm5->nchannels = aux_dev_wrapper->n_tm_channels;
+ adc_tm5->chan_props = devm_kcalloc(dev, aux_dev_wrapper->n_tm_channels,
+ sizeof(*adc_tm5->chan_props), GFP_KERNEL);
+ if (!adc_tm5->chan_props)
+ return -ENOMEM;
+
+ for (i = 0; i < adc_tm5->nchannels; i++) {
+ adc_tm5->chan_props[i].common_props = aux_dev_wrapper->tm_props[i];
+ adc_tm5->chan_props[i].timer = MEAS_INT_1S;
+ adc_tm5->chan_props[i].sdam_index = (i + 1) / 8;
+ adc_tm5->chan_props[i].tm_chan_index = (i + 1) % 8;
+ adc_tm5->chan_props[i].chip = adc_tm5;
+ }
+
+ INIT_WORK(&adc_tm5->tm_handler_work, tm_handler_work);
+
+ /*
+ * Skipping first SDAM IRQ as it is requested in parent driver.
+ * If there is a TM violation on that IRQ, the parent driver calls
+ * the notifier (tm_event_notify) exposed from this driver to handle it.
+ */
+ for (i = 1; i < adc_tm5->dev_data->num_sdams; i++) {
+ ret = devm_request_threaded_irq(dev,
+ adc_tm5->dev_data->base[i].irq,
+ NULL, adctm5_gen3_isr, IRQF_ONESHOT,
+ adc_tm5->dev_data->base[i].irq_name,
+ adc_tm5);
+ if (ret < 0)
+ return ret;
+ }
+
+ /*
+ * This drvdata is only used in the function (adctm_event_handler)
+ * called by parent ADC driver in case of TM violation on the first SDAM.
+ */
+ auxiliary_set_drvdata(aux_dev, adc_tm5);
+
+ /*
+ * This is to cancel any instances of tm_handler_work scheduled by
+ * TM interrupt, at the time of module removal.
+ */
+
+ ret = devm_add_action(dev, adc5_gen3_clear_work, adc_tm5);
+ if (ret)
+ return ret;
+
+ ret = adc_tm5_register_tzd(adc_tm5);
+ if (ret)
+ return ret;
+
+ /* This is to disable all ADC_TM channels in case of probe failure. */
+
+ return devm_add_action(dev, adc5_gen3_disable, adc_tm5);
+}
+
+static const struct auxiliary_device_id adctm5_auxiliary_id_table[] = {
+ { .name = "qcom_spmi_adc5_gen3.adc5_tm_gen3", },
+ { }
+};
+
+MODULE_DEVICE_TABLE(auxiliary, adctm5_auxiliary_id_table);
+
+static struct adc_tm5_auxiliary_drv adctm5gen3_auxiliary_drv = {
+ .adrv = {
+ .id_table = adctm5_auxiliary_id_table,
+ .probe = adc_tm5_probe,
+ },
+ .tm_event_notify = adctm_event_handler,
+};
+
+static int __init adctm5_init_module(void)
+{
+ return auxiliary_driver_register(&adctm5gen3_auxiliary_drv.adrv);
+}
+
+static void __exit adctm5_exit_module(void)
+{
+ auxiliary_driver_unregister(&adctm5gen3_auxiliary_drv.adrv);
+}
+
+module_init(adctm5_init_module);
+module_exit(adctm5_exit_module);
+
+MODULE_DESCRIPTION("SPMI PMIC Thermal Monitor ADC driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS("QCOM_SPMI_ADC5_GEN3");
--
2.25.1
On Thu, 27 Nov 2025 19:10:36 +0530
Jishnu Prakash <jishnu.prakash@oss.qualcomm.com> wrote:
> Add support for ADC_TM part of PMIC5 Gen3.
>
> This is an auxiliary driver under the Gen3 ADC driver, which implements the
> threshold setting and interrupt generating functionalities of QCOM ADC_TM
> drivers, used to support thermal trip points.
>
> Signed-off-by: Jishnu Prakash <jishnu.prakash@oss.qualcomm.com>
> ---
Hi Jishnu
Fresh read threw up a few more comments from me.
See inline
Thanks,
Jonathan
> diff --git a/drivers/thermal/qcom/qcom-spmi-adc-tm5-gen3.c b/drivers/thermal/qcom/qcom-spmi-adc-tm5-gen3.c
> new file mode 100644
> index 000000000000..c6cc8ef76f7e
> --- /dev/null
> +++ b/drivers/thermal/qcom/qcom-spmi-adc-tm5-gen3.c
> +
> +static void tm_handler_work(struct work_struct *work)
> +{
> + struct adc_tm5_gen3_chip *adc_tm5 = container_of(work, struct adc_tm5_gen3_chip,
> + tm_handler_work);
> + struct adc_tm5_gen3_channel_props *chan_prop;
Why not declare in the reduced scope below? Then could probably combine
declaration and assignment for this, and offset.
> + u8 tm_status[2] = { };
> + u8 buf[16] = { };
> + int sdam_index = -1;
> + int i, ret;
> +
> + for (i = 0; i < adc_tm5->nchannels; i++) {
It's considered fine in new kernel code to declare the loop variable
as
for (int i = 0;
> + bool upper_set, lower_set;
> + int temp, offset;
> + u16 code = 0;
> +
> + chan_prop = &adc_tm5->chan_props[i];
> + offset = chan_prop->tm_chan_index;
> +
> + adc5_gen3_mutex_lock(adc_tm5->dev);
> + if (chan_prop->sdam_index != sdam_index) {
> + sdam_index = chan_prop->sdam_index;
> + ret = adc5_gen3_tm_status_check(adc_tm5, sdam_index,
> + tm_status, buf);
> + if (ret) {
> + adc5_gen3_mutex_unlock(adc_tm5->dev);
If you had the guard() below, could perhaps use scoped_guard() here
to avoid need for unlocking in error paths.
That would be at the cost of increased indent however, so may not be worth it
or that may suggest factoring out some of this code as a helper.
> + break;
> + }
> + }
> +
> + upper_set = ((tm_status[0] & BIT(offset)) && chan_prop->high_thr_en);
> + lower_set = ((tm_status[1] & BIT(offset)) && chan_prop->low_thr_en);
> + adc5_gen3_mutex_unlock(adc_tm5->dev);
> +
> + if (!(upper_set || lower_set))
> + continue;
> +
> + code = get_unaligned_le16(&buf[2 * offset]);
> + pr_debug("ADC_TM threshold code:%#x\n", code);
> +
> + ret = adc5_gen3_therm_code_to_temp(adc_tm5->dev,
> + &chan_prop->common_props,
> + code, &temp);
> + if (ret) {
> + dev_err(adc_tm5->dev,
> + "Invalid temperature reading, ret = %d, code=%#x\n",
> + ret, code);
> + continue;
> + }
> +
> + chan_prop->last_temp = temp;
> + chan_prop->last_temp_set = true;
> + thermal_zone_device_update(chan_prop->tzd, THERMAL_TRIP_VIOLATED);
> + }
> +}
> +static int adc_tm5_gen3_set_trip_temp(struct thermal_zone_device *tz,
> + int low_temp, int high_temp)
> +{
> + struct adc_tm5_gen3_channel_props *prop = thermal_zone_device_priv(tz);
> + struct adc_tm5_gen3_chip *adc_tm5;
> + int ret;
> +
> + if (!prop || !prop->chip)
> + return -EINVAL;
> +
> + adc_tm5 = prop->chip;
> +
> + dev_dbg(adc_tm5->dev, "channel:%s, low_temp(mdegC):%d, high_temp(mdegC):%d\n",
> + prop->common_props.label, low_temp, high_temp);
> +
> + adc5_gen3_mutex_lock(adc_tm5->dev);
> + if (high_temp == INT_MAX && low_temp <= -INT_MAX)
How is low temp lower than the min value that fits in an integer?
> + ret = adc_tm5_gen3_disable_channel(prop);
> + else
> + ret = adc_tm5_gen3_configure(prop, low_temp, high_temp);
> + adc5_gen3_mutex_unlock(adc_tm5->dev);
Might be worth a DEFINE_GUARD() so you can do
guard(adc5_gen3)(adc_tm5->dev);
if (high_temp = INT_MAX && low_temp <= -INT_MAX)
return adc_tm5_gen3_disable_channel(prop);
return adc_tm5...
I haven't looked to see if this is useful elsewhere in these drivers.
> +
> + return ret;
> +}
Hi Jonathan,
On 12/7/2025 10:34 PM, Jonathan Cameron wrote:
> On Thu, 27 Nov 2025 19:10:36 +0530
> Jishnu Prakash <jishnu.prakash@oss.qualcomm.com> wrote:
>
>> Add support for ADC_TM part of PMIC5 Gen3.
>>
>> This is an auxiliary driver under the Gen3 ADC driver, which implements the
>> threshold setting and interrupt generating functionalities of QCOM ADC_TM
>> drivers, used to support thermal trip points.
>>
>> Signed-off-by: Jishnu Prakash <jishnu.prakash@oss.qualcomm.com>
>> ---
> Hi Jishnu
>
> Fresh read threw up a few more comments from me.
>
> See inline
>
> Thanks,
>
> Jonathan
>
>> diff --git a/drivers/thermal/qcom/qcom-spmi-adc-tm5-gen3.c b/drivers/thermal/qcom/qcom-spmi-adc-tm5-gen3.c
>> new file mode 100644
>> index 000000000000..c6cc8ef76f7e
>> --- /dev/null
>> +++ b/drivers/thermal/qcom/qcom-spmi-adc-tm5-gen3.c
>
>> +
>> +static void tm_handler_work(struct work_struct *work)
>> +{
>> + struct adc_tm5_gen3_chip *adc_tm5 = container_of(work, struct adc_tm5_gen3_chip,
>> + tm_handler_work);
>> + struct adc_tm5_gen3_channel_props *chan_prop;
>
> Why not declare in the reduced scope below? Then could probably combine
> declaration and assignment for this, and offset.
OK, I'll just keep the following lines here:
struct adc_tm5_gen3_chip *adc_tm5 = container_of(work, struct adc_tm5_gen3_chip, tm_handler_work);
int sdam_index = -1;
and declare the remaining variables in the loop.
>
>
>> + u8 tm_status[2] = { };
>> + u8 buf[16] = { };
>> + int sdam_index = -1;
>> + int i, ret;
>> +
>> + for (i = 0; i < adc_tm5->nchannels; i++) {
> It's considered fine in new kernel code to declare the loop variable
> as
> for (int i = 0;
>
>> + bool upper_set, lower_set;
>> + int temp, offset;
>> + u16 code = 0;
>> +
>> + chan_prop = &adc_tm5->chan_props[i];
>> + offset = chan_prop->tm_chan_index;
>> +
>> + adc5_gen3_mutex_lock(adc_tm5->dev);
>> + if (chan_prop->sdam_index != sdam_index) {
>> + sdam_index = chan_prop->sdam_index;
>> + ret = adc5_gen3_tm_status_check(adc_tm5, sdam_index,
>> + tm_status, buf);
>> + if (ret) {
>> + adc5_gen3_mutex_unlock(adc_tm5->dev);
>
> If you had the guard() below, could perhaps use scoped_guard() here
> to avoid need for unlocking in error paths.
> That would be at the cost of increased indent however, so may not be worth it
> or that may suggest factoring out some of this code as a helper.
The mutex is meant to guard register writes, so it might be sufficient to
have it around adc5_gen3_tm_status_check() alone. I'll make this change.
>
>> + break;
>> + }
>> + }
>> +
>> + upper_set = ((tm_status[0] & BIT(offset)) && chan_prop->high_thr_en);
>> + lower_set = ((tm_status[1] & BIT(offset)) && chan_prop->low_thr_en);
>> + adc5_gen3_mutex_unlock(adc_tm5->dev);
>> +
>> + if (!(upper_set || lower_set))
>> + continue;
>> +
>> + code = get_unaligned_le16(&buf[2 * offset]);
>> + pr_debug("ADC_TM threshold code:%#x\n", code);
>> +
>> + ret = adc5_gen3_therm_code_to_temp(adc_tm5->dev,
>> + &chan_prop->common_props,
>> + code, &temp);
>> + if (ret) {
>> + dev_err(adc_tm5->dev,
>> + "Invalid temperature reading, ret = %d, code=%#x\n",
>> + ret, code);
>> + continue;
>> + }
>> +
>> + chan_prop->last_temp = temp;
>> + chan_prop->last_temp_set = true;
>> + thermal_zone_device_update(chan_prop->tzd, THERMAL_TRIP_VIOLATED);
>> + }
>> +}
>
>> +static int adc_tm5_gen3_set_trip_temp(struct thermal_zone_device *tz,
>> + int low_temp, int high_temp)
>> +{
>> + struct adc_tm5_gen3_channel_props *prop = thermal_zone_device_priv(tz);
>> + struct adc_tm5_gen3_chip *adc_tm5;
>> + int ret;
>> +
>> + if (!prop || !prop->chip)
>> + return -EINVAL;
>> +
>> + adc_tm5 = prop->chip;
>> +
>> + dev_dbg(adc_tm5->dev, "channel:%s, low_temp(mdegC):%d, high_temp(mdegC):%d\n",
>> + prop->common_props.label, low_temp, high_temp);
>> +
>> + adc5_gen3_mutex_lock(adc_tm5->dev);
>> + if (high_temp == INT_MAX && low_temp <= -INT_MAX)
>
> How is low temp lower than the min value that fits in an integer?
Yes, that check should be (low_temp == -INT_MAX), I'll fix it.
>
>> + ret = adc_tm5_gen3_disable_channel(prop);
>> + else
>> + ret = adc_tm5_gen3_configure(prop, low_temp, high_temp);
>> + adc5_gen3_mutex_unlock(adc_tm5->dev);
> Might be worth a DEFINE_GUARD() so you can do
> guard(adc5_gen3)(adc_tm5->dev);
> if (high_temp = INT_MAX && low_temp <= -INT_MAX)
> return adc_tm5_gen3_disable_channel(prop);
>
> return adc_tm5...
>
> I haven't looked to see if this is useful elsewhere in these drivers.
>
I'll add this and address your other comments.
Thanks,
Jishnu
>> +
>> + return ret;
>> +}
On Thu, Nov 27, 2025 at 07:10:36PM +0530, Jishnu Prakash wrote:
> Add support for ADC_TM part of PMIC5 Gen3.
>
> This is an auxiliary driver under the Gen3 ADC driver, which implements the
> threshold setting and interrupt generating functionalities of QCOM ADC_TM
> drivers, used to support thermal trip points.
>
> Signed-off-by: Jishnu Prakash <jishnu.prakash@oss.qualcomm.com>
> ---
> Changes since v7:
> - Addressed following comments from Jonathan:
> - Replaced {0} with { } in tm_handler_work()
> - Simplified logic for setting upper_set and lower_set into
> a single line each, in tm_handler_work()
> - Cleaned up local variable declarations and high/low threshold
> check in adc_tm5_gen3_configure()
> - Moved cleanup action to disable all ADC_TM channels to probe
> end and added comment to describe it.
> - Fixed { } formatting in adctm5_auxiliary_id_table[].
>
> Changes since v6:
> - Addressed following comments from Jonathan:
> - Added error check for devm_thermal_add_hwmon_sysfs() call.
> - Used local variable `dev` in multiple places in adc_tm5_probe().
> in place of `&aux_dev->dev` and `adc_tm5->dev`.
> - Added a comment to explain cleanup action calling adc5_gen3_clear_work()
> near probe end.
> - Fixed return statement at probe end to return last called API's
> return value directly.
>
> Changes since v5:
> - Addressed following comments from Jonathan:
> - Corrected all files to follow kernel-doc formatting fully.
> - Cleaned up formatting in struct definitions.
> - Used sizeof() to specify length in register read/write calls
> instead of using integers directly.
> - Added comments in adc_tm5_probe() for skipping first SDAM for
> IRQ request and for usage of auxiliary_set_drvdata().
> - Corrected line wrap length driver file.
> - Moved INIT_WORK() and auxiliary_set_drvdata() to earlier
> locations to ensure they are ready when needed.
>
> Changes since v4:
> - Fixed a compilation error and updated dependencies in config as suggested
> by Krzysztof.
>
> drivers/thermal/qcom/Kconfig | 9 +
> drivers/thermal/qcom/Makefile | 1 +
> drivers/thermal/qcom/qcom-spmi-adc-tm5-gen3.c | 530 ++++++++++++++++++
> 3 files changed, 540 insertions(+)
> create mode 100644 drivers/thermal/qcom/qcom-spmi-adc-tm5-gen3.c
>
> diff --git a/drivers/thermal/qcom/Kconfig b/drivers/thermal/qcom/Kconfig
> index a6bb01082ec6..1acb11e4ac80 100644
> --- a/drivers/thermal/qcom/Kconfig
> +++ b/drivers/thermal/qcom/Kconfig
> @@ -21,6 +21,15 @@ config QCOM_SPMI_ADC_TM5
> Thermal client sets threshold temperature for both warm and cool and
> gets updated when a threshold is reached.
>
> +config QCOM_SPMI_ADC_TM5_GEN3
> + tristate "Qualcomm SPMI PMIC Thermal Monitor ADC5 Gen3"
> + depends on QCOM_SPMI_ADC5_GEN3
This module depends directly on the Gen3 ADC driver. I think you can
drop a separate "common" submodule.
> + help
> + This enables the auxiliary thermal driver for the ADC5 Gen3 thermal
> + monitoring device. It shows up as a thermal zone with multiple trip points.
> + Thermal client sets threshold temperature for both warm and cool and
> + gets updated when a threshold is reached.
> +
> config QCOM_SPMI_TEMP_ALARM
> tristate "Qualcomm SPMI PMIC Temperature Alarm"
> depends on OF && SPMI && IIO
> +
> +static struct adc_tm5_auxiliary_drv adctm5gen3_auxiliary_drv = {
> + .adrv = {
> + .id_table = adctm5_auxiliary_id_table,
> + .probe = adc_tm5_probe,
> + },
> + .tm_event_notify = adctm_event_handler,
> +};
> +
> +static int __init adctm5_init_module(void)
> +{
> + return auxiliary_driver_register(&adctm5gen3_auxiliary_drv.adrv);
> +}
> +
> +static void __exit adctm5_exit_module(void)
> +{
> + auxiliary_driver_unregister(&adctm5gen3_auxiliary_drv.adrv);
> +}
> +
> +module_init(adctm5_init_module);
> +module_exit(adctm5_exit_module);
We really need to make this work with module_auxiliary_driver-like
macro.
> +
> +MODULE_DESCRIPTION("SPMI PMIC Thermal Monitor ADC driver");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS("QCOM_SPMI_ADC5_GEN3");
> --
> 2.25.1
>
--
With best wishes
Dmitry
Hi Dmitry,
On 12/6/2025 7:54 AM, Dmitry Baryshkov wrote:
> On Thu, Nov 27, 2025 at 07:10:36PM +0530, Jishnu Prakash wrote:
>> Add support for ADC_TM part of PMIC5 Gen3.
>>
>> This is an auxiliary driver under the Gen3 ADC driver, which implements the
>> threshold setting and interrupt generating functionalities of QCOM ADC_TM
>> drivers, used to support thermal trip points.
>>
>> Signed-off-by: Jishnu Prakash <jishnu.prakash@oss.qualcomm.com>
>> ---
...
>>
>> drivers/thermal/qcom/Kconfig | 9 +
>> drivers/thermal/qcom/Makefile | 1 +
>> drivers/thermal/qcom/qcom-spmi-adc-tm5-gen3.c | 530 ++++++++++++++++++
>> 3 files changed, 540 insertions(+)
>> create mode 100644 drivers/thermal/qcom/qcom-spmi-adc-tm5-gen3.c
>>
>> diff --git a/drivers/thermal/qcom/Kconfig b/drivers/thermal/qcom/Kconfig
>> index a6bb01082ec6..1acb11e4ac80 100644
>> --- a/drivers/thermal/qcom/Kconfig
>> +++ b/drivers/thermal/qcom/Kconfig
>> @@ -21,6 +21,15 @@ config QCOM_SPMI_ADC_TM5
>> Thermal client sets threshold temperature for both warm and cool and
>> gets updated when a threshold is reached.
>>
>> +config QCOM_SPMI_ADC_TM5_GEN3
>> + tristate "Qualcomm SPMI PMIC Thermal Monitor ADC5 Gen3"
>> + depends on QCOM_SPMI_ADC5_GEN3
>
> This module depends directly on the Gen3 ADC driver. I think you can
> drop a separate "common" submodule.
>
Yes, I can do this in the next patch series.
>> + help
>> + This enables the auxiliary thermal driver for the ADC5 Gen3 thermal
>> + monitoring device. It shows up as a thermal zone with multiple trip points.
>> + Thermal client sets threshold temperature for both warm and cool and
>> + gets updated when a threshold is reached.
>> +
>> config QCOM_SPMI_TEMP_ALARM
>> tristate "Qualcomm SPMI PMIC Temperature Alarm"
>> depends on OF && SPMI && IIO
>
>
>> +
>> +static struct adc_tm5_auxiliary_drv adctm5gen3_auxiliary_drv = {
>> + .adrv = {
>> + .id_table = adctm5_auxiliary_id_table,
>> + .probe = adc_tm5_probe,
>> + },
>> + .tm_event_notify = adctm_event_handler,
>> +};
>> +
>> +static int __init adctm5_init_module(void)
>> +{
>> + return auxiliary_driver_register(&adctm5gen3_auxiliary_drv.adrv);
>> +}
>> +
>> +static void __exit adctm5_exit_module(void)
>> +{
>> + auxiliary_driver_unregister(&adctm5gen3_auxiliary_drv.adrv);
>> +}
>> +
>> +module_init(adctm5_init_module);
>> +module_exit(adctm5_exit_module);
>
> We really need to make this work with module_auxiliary_driver-like
> macro.
>
I tried doing this again now, but I'm not sure if the way I found is fine.
Just to recap, the main issue with using module_auxiliary_driver() directly,
which I discussed with Jonathan here earlier: (https://lore.kernel.org/all/20250301032901.7b38fed4@jic23-huawei/)
was that it is a macro definition which uses its input argument to
generate function names. So if I have a line like this:
module_auxiliary_driver(adctm5gen3_auxiliary_drv.adrv);
it will generate function definitions like this, due to text substitutions:
static int __init adctm5gen3_auxiliary_drv.adrv_init(void)
which will fail compilation.
I see that in other drivers where module_auxiliary_driver() is used, there is a
"struct auxiliary_driver" initialization just before that initialized variable
is passed to module_auxiliary_driver(). I tried making a similar change here now:
-static struct adc_tm5_auxiliary_drv adctm5gen3_auxiliary_drv = {
- .adrv = {
- .id_table = adctm5_auxiliary_id_table,
- .probe = adc_tm5_probe,
- },
- .tm_event_notify = adctm_event_handler,
+static struct auxiliary_driver adctm5gen3_auxiliary_driver = {
+ .id_table = adctm5_auxiliary_id_table,
+ .probe = adc_tm5_probe,
};
-static int __init adctm5_init_module(void)
-{
- return auxiliary_driver_register(&adctm5gen3_auxiliary_drv.adrv);
-}
-
-static void __exit adctm5_exit_module(void)
-{
- auxiliary_driver_unregister(&adctm5gen3_auxiliary_drv.adrv);
-}
+struct adc_tm5_auxiliary_drv adctm5gen3_auxiliary_drv = {
+ .adrv = adctm5gen3_auxiliary_driver,
+ .tm_event_notify = adctm_event_handler,
+};
-module_init(adctm5_init_module);
-module_exit(adctm5_exit_module);
+module_auxiliary_driver(adctm5gen3_auxiliary_driver);
With this, I get the following error:
drivers/thermal/qcom/qcom-spmi-adc-tm5-gen3.c:513:10: error: initializer element is not constant
513 | .adrv = adctm5gen3_auxiliary_driver,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
It looks like the above definiton of adctm5gen3_auxiliary_driver is not considered
a compile-time constant. I made the following modification to fix this:
-static struct auxiliary_driver adctm5gen3_auxiliary_driver = {
+static const struct auxiliary_driver adctm5gen3_auxiliary_driver = {
And with this, the code does get built, but with warnings like this:
drivers/thermal/qcom/qcom-spmi-adc-tm5-gen3.c: In function ‘adctm5gen3_auxiliary_driver_init’:
./include/linux/device/driver.h:260:20: warning: passing argument 1 of ‘__auxiliary_driver_register’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
260 | return __register(&(__driver) , ##__VA_ARGS__); \
| ^~~~~~~~~~~
./include/linux/auxiliary_bus.h:253:30: note: in definition of macro ‘auxiliary_driver_register’
253 | __auxiliary_driver_register(auxdrv, THIS_MODULE, KBUILD_MODNAME)
| ^~~~~~
./include/linux/auxiliary_bus.h:287:2: note: in expansion of macro ‘module_driver’
287 | module_driver(__auxiliary_driver, auxiliary_driver_register, auxiliary_driver_unregister)
| ^~~~~~~~~~~~~
drivers/thermal/qcom/qcom-spmi-adc-tm5-gen3.c:517:1: note: in expansion of macro ‘module_auxiliary_driver’
517 | module_auxiliary_driver(adctm5gen3_auxiliary_driver);
| ^~~~~~~~~~~~~~~~~~~~~~~
./include/linux/auxiliary_bus.h:250:58: note: expected ‘struct auxiliary_driver *’ but argument is of type ‘const struct auxiliary_driver *’
250 | int __auxiliary_driver_register(struct auxiliary_driver *auxdrv, struct module *owner,
| ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~
Is it acceptable to have the above code changes to use module_auxiliary_driver(),
even with the warnings generated?
Or do you have any other suggestions?
Thanks,
Jishnu
>> +
>> +MODULE_DESCRIPTION("SPMI PMIC Thermal Monitor ADC driver");
>> +MODULE_LICENSE("GPL");
>> +MODULE_IMPORT_NS("QCOM_SPMI_ADC5_GEN3");
>> --
>> 2.25.1
>>
>
© 2016 - 2026 Red Hat, Inc.