On Tue, 26 Aug 2025 14:06:57 +0530
Jishnu Prakash <jishnu.prakash@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx>
Hi Jishnu,
A few comment inline from a fresh read
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..9ec0d4e058b8
> --- /dev/null
> +++ b/drivers/thermal/qcom/qcom-spmi-adc-tm5-gen3.c
> @@ -0,0 +1,535 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + */
> +
> +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] = {0};
> + u8 buf[16] = {0};
Small preference for { };
which is effectively the same but for structures (so not relevant here) that is
also defined by newer c specs to initialize holes which the {0}; version is not
(but actually does in compilers with the settings the kernel uses).
> + int i, ret = 0, sdam_index = -1;
> +
> + for (i = 0; i < adc_tm5->nchannels; i++) {
> + bool upper_set = false, lower_set = false;
> + 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;
> + }
> + }
> +
> + if ((tm_status[0] & BIT(offset)) && chan_prop->high_thr_en)
> + upper_set = true;
upper_set = ((tm_status[0] & BIT(offset)) && chan_prop->high_thr_en;
seems as clear to me and avoid need to initialize above.
The
for (i...) {
if (x)
b = true;
}
pattern made me thing this was a check that built up over iterations, but it's
not so avoiding that is probably a good thing as well!
> +
> + if ((tm_status[1] & BIT(offset)) && chan_prop->low_thr_en)
> + lower_set = true;
> + 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_configure(struct adc_tm5_gen3_channel_props *prop,
> + int low_temp, int high_temp)
> +{
> + struct adc_tm5_gen3_chip *adc_tm5 = prop->chip;
> + u8 conv_req = 0, buf[ADC_TM5_GEN3_CONFIG_REGS];
Spit these sort of complex mix of types of declaration up.
u8 buf[*];
u8 conv_reg = 0;
etc as it helps readability. Generally I wouldn't mix assignment and
non assignment and also not arrays or pointers and non pointers etc.
> + u16 adc_code;
> + int ret;
> +
> + /* 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 */
> + if (high_temp != INT_MAX) {
> + prop->low_thr_en = true;
Perhaps neater as a assignment then use of the bool
prop->low_thr_en = (hightemp != INT_MAX);
if (prp->low_thr_en) {
adc_code = qcom_adc_tm5_gen2_temp_res_scale(high_temp);
put_unaligned_le16(adc_code, &buf[8]);
}
Applies to below similar case as well.
> + adc_code = qcom_adc_tm5_gen2_temp_res_scale(high_temp);
> + put_unaligned_le16(adc_code, &buf[8]);
> + } else {
> + prop->low_thr_en = false;
> + }
> +
> + /* Low temperature corresponds to high voltage threshold */
> + if (low_temp != -INT_MAX) {
> + prop->high_thr_en = true;
> + adc_code = qcom_adc_tm5_gen2_temp_res_scale(low_temp);
> + put_unaligned_le16(adc_code, &buf[10]);
> + } else {
> + prop->high_thr_en = false;
> + }
> +
> + 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_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;
> + }
> +
> + ret = devm_add_action_or_reset(dev, adc5_gen3_disable, adc_tm5);
I'd normally expect a pairing of a devm action with whatever it is undoing.
If not add a comment for why that isn't the case here.
> + if (ret)
> + return ret;
> +
> + INIT_WORK(&adc_tm5->tm_handler_work, tm_handler_work);
> +}
> +
> +static const struct auxiliary_device_id adctm5_auxiliary_id_table[] = {
> + { .name = "qcom_spmi_adc5_gen3.adc5_tm_gen3", },
> + {}
For IIO drivers I'm trying to slowly standardize some formatting choices.
For these I picked (for no particular reason)
{ }
> +};
