Messages in this thread | | | Date | Sun, 23 Jan 2022 20:16:14 +0530 | Subject | Re: [PATCH V3 4/4] thermal: qcom: add support for PMIC5 Gen2 ADCTM | From | Jishnu Prakash <> |
| |
Hi Jonathan,
On 11/27/2021 12:16 AM, Jonathan Cameron wrote: > On Tue, 23 Nov 2021 11:27:04 +0530 > Jishnu Prakash <quic_jprakash@quicinc.com> wrote: > >> Add support for PMIC5 Gen2 ADC_TM, used on PMIC7 chips. It is a >> close counterpart of PMIC7 ADC and has the same functionality as >> PMIC5 ADC_TM, for threshold monitoring and interrupt generation. >> It is present on PMK8350 alone, like PMIC7 ADC and can be used >> to monitor up to 8 ADC channels, from any of the PMIC7 PMICs >> having ADC on a target, through PBS(Programmable Boot Sequence). >> >> Signed-off-by: Jishnu Prakash <quic_jprakash@quicinc.com> > Just one note on using put_unaligned_le16() below. Otherwise, from > a drive by review point of view it looks fine to someone not that > familiar with the driver or thermal :) > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > >> --- >> drivers/thermal/qcom/qcom-spmi-adc-tm5.c | 375 ++++++++++++++++++++++++++++++- >> 1 file changed, 372 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/thermal/qcom/qcom-spmi-adc-tm5.c b/drivers/thermal/qcom/qcom-spmi-adc-tm5.c >> index fc8cd45..a7b33a8 100644 >> --- a/drivers/thermal/qcom/qcom-spmi-adc-tm5.c >> +++ b/drivers/thermal/qcom/qcom-spmi-adc-tm5.c >> @@ -4,7 +4,10 @@ >> * >> * Based on original driver: >> * Copyright (c) 2012-2020, The Linux Foundation. All rights reserved. >> + * >> + * Copyright (c) 2021 Qualcomm Innovation Center, Inc. All rights reserved. >> */
>> + >> + /* Low temperature corresponds to high voltage threshold */ >> + if (low != -INT_MAX) { >> + channel->high_thr_en = true; >> + adc_code = qcom_adc_tm5_gen2_temp_res_scale(low); >> + >> + buf[11] = adc_code & 0xff; >> + buf[12] = adc_code >> 8; > looks like a little endian put though not necessarily aligned so > put_unaligned_le16() preferred to open coding it. Same in similar places. > Not my area though so maintainer may not care as much.
I'll use put_unaligned_le16 as suggested, in similar places in the next post.
>> + } else { >> + channel->high_thr_en = false; >> + } >> + >> + buf[13] = ADC_TM_GEN2_MEAS_EN; >> + if (channel->high_thr_en) >> + buf[13] |= ADC_TM5_GEN2_HIGH_THR_INT_EN;
Thanks,
Jishnu
| |