Re: [PATCH v7 07/10] thermal: qcom: add support for adc-tm5 PMIC thermal monitor

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

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@linaro.org>
>> ---
>>   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