Messages in this thread | | | Date | Sun, 23 Jan 2022 20:26:30 +0530 | Subject | Re: [PATCH V3 4/4] thermal: qcom: add support for PMIC5 Gen2 ADCTM | From | Jishnu Prakash <> |
| |
Hi Dmitry,
On 11/29/2021 8:02 AM, Dmitry Baryshkov wrote: > On 23/11/2021 08:57, Jishnu Prakash 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> >> --- >> 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
>> static int adc_tm5_set_trips(void *data, int low, int high) >> { >> struct adc_tm5_channel *channel = data; >> @@ -422,6 +739,11 @@ static int adc_tm5_init(struct adc_tm5_chip *chip) >> } >> } >> + mutex_init(&chip->adc_mutex_lock); > > Minor issue. This way, the mutex is left uninitialized for ADC_TM5_HC > devices. I'd move the mutex_init() call to the _probe function itself.
The mutex is needed only for Gen2 ADC_TM devices, I have mentioned this in the adc_tm5_chip struct description. I'll keep it in the Gen2 ADC_TM init function.
> >> + >> + if (chip->data->gen == ADC_TM5_GEN2) >> + return ret; >> + > > Please do not do this. Create a separate adc_tm5_gen2_init function. > Add init() callback to adc_tm5_data structure.
Will make this change in next post.
> >> buf[0] = chip->decimation; >> buf[1] = chip->avg_samples | ADC_TM5_FAST_AVG_EN; >> buf[2] = ADC_TM5_TIMER1; >> @@ -442,7 +764,7 @@ static int adc_tm5_get_dt_channel_data(struct >> adc_tm5_chip *adc_tm, >> struct device_node *node) >> { >> const char *name = node->name; >> - u32 chan, value, varr[2]; >> + u32 chan, value, adc_channel, varr[2]; >> int ret; >> struct device *dev = adc_tm->dev; >> struct of_phandle_args args; >> @@ -472,7 +794,11 @@ static int adc_tm5_get_dt_channel_data(struct >> adc_tm5_chip *adc_tm, >> } >> of_node_put(args.np); >> - if (args.args_count != 1 || args.args[0] >= ADC5_MAX_CHANNEL) { >> + adc_channel = args.args[0]; >> + if (adc_tm->data->gen == ADC_TM5_GEN2) >> + adc_channel &= 0xff; >> + >> + if (args.args_count != 1 || adc_channel >= ADC5_MAX_CHANNEL) { > > Here you read the data (args.args[0]) before checking that it is > actually available (args.args_count is not zero). Please correct the > sequence.
Will correct this in next post.
> >> dev_err(dev, "%s: invalid ADC channel number %d\n", name, >> chan); >> return -EINVAL; >> } >> @@ -518,6 +844,32 @@ static int adc_tm5_get_dt_channel_data(struct >> adc_tm5_chip *adc_tm, >> else >> channel->cal_method = ADC_TM5_ABSOLUTE_CAL; >> + if (adc_tm->data->gen == ADC_TM5_GEN2) { >> + ret = of_property_read_u32(node, "qcom,decimation", &value); >> + if (!ret) { >> + ret = qcom_adc5_decimation_from_dt(value, >> adc_tm->data->decimation); >> + if (ret < 0) { >> + dev_err(dev, "invalid decimation %d\n", value); >> + return ret;
Thanks,
Jishnu
| |