Re: [PATCH v3 2/3] dt-bindings: iio: adc: Add support for QCOM PMIC5 Gen3 ADC

[Jishnu Prakash <quic_jprakash@quicinc.com>]

Hi Krzysztof,

(Resending this mail for tracking on mailing lists, as it got rejected 
from lists the first time due to HTML)

On 1/4/2024 1:48 PM, Krzysztof Kozlowski wrote:
> On 31/12/2023 18:12, Jishnu Prakash wrote:
>> For the PMIC5-Gen3 type PMICs, ADC peripheral is present in HW for the
>> following PMICs: PMK8550, PM8550, PM8550B and PM8550VX PMICs.
>>
>> It is similar to PMIC5-Gen2, with SW communication to ADCs on all PMICs
>> going through PBS(Programmable Boot Sequence) firmware through a single
>> register interface. This interface is implemented on an SDAM (Shared
>> Direct Access Memory) peripheral on the master PMIC PMK8550 rather
>> than a dedicated ADC peripheral.
>>
>> Add documentation for PMIC5 Gen3 ADC and macro definitions for ADC
>> channels and virtual channels (combination of ADC channel number and
>> PMIC SID number) per PMIC, to be used by clients of this device.
>>
>> Changes since v2:
>> - Moved ADC5 Gen3 documentation into a separate new file.
> 
> Changelog goes under ---.
> 
> Why did you do this? What is the rationale? Sorry, this patchset goes
> nowhere.

I'll elaborate this more in the next patchset. There are two main 
reasons for adding this documentation in a new file:

1.This device is not exactly like the existing QCOM VADC drivers as it 
now combines VADC functionality (reading ADC channel on client request) 
with ADC_TM functionality (thermal threshold monitoring).

2.Adding this device's bindings in the existing qcom,spmi-vadc.yaml file 
is not possible as it would require updating some of the existing 
top-level constraints. (for the older devices in that file, "reg" and 
"interrupts" can have at most one item, while this device can have more 
than one item under these properties.)


> 
>>
>> Changes since v1:
>> - Updated properties separately for all compatibles to clarify usage
>>    of new properties and updates in usage of old properties for ADC5 Gen3.
>> - Avoided updating 'adc7' name to 'adc5 gen2' and just left a comment
>>    mentioning this convention.
>> - Used predefined channel IDs in individual PMIC channel definitions
>>    instead of numeric IDs.
>> - Addressed other comments from reviewers.
>>

>> +          per PMIC in the PMIC-specific files in include/dt-bindings/iio/adc.
>> +
>> +      label:
>> +        $ref: /schemas/types.yaml#/definitions/string
> 
> Why do you need it in the first place? Don't you miss some $ref?

This is just meant to show the ADC channel name in DT for our reference. 
I'll check if I can use adc.yaml, which includes this property already, 
as a reference in this case.

> 
>> +        description: |
> 
> Do not need '|' unless you need to preserve formatting. Applies everywhere.
> 
> 
> 
>> +            ADC input of the platform as seen in the schematics.
>> +            For thermistor inputs connected to generic AMUX or GPIO inputs
>> +            these can vary across platform for the same pins. Hence select
>> +            the platform schematics name for this channel.
>> +


>> +
>> +      qcom,adc-tm:
>> +        description: |
>> +            Indicates if ADC_TM monitoring is done on this channel.
>> +            Defined for compatible property "qcom,spmi-adc5-gen3".
>> +            This is the same functionality as in the existing QCOM ADC_TM
>> +            device, documented at devicetree/bindings/thermal/qcom-spmi-adc-tm5.yaml.
>> +        type: boolean
>> +
> 
> Why do you duplicate entire vadc file? Why it cannot be part of that
> file? Oh wait, it was in v2.
> 
> You now duplicated a lot of property definitions without clear reason.
> If this is intention, then you need to put them in common schema.

Many of the properties used for earlier QCOM VADC devices will be used 
for this device too.....do you mean I can add a new schema file (named 
something like qcom,vadc.yaml) and move common properties into it (like 
qcom,hw-settle-time, qcom,decimation, etc) from this file and 
qcom,spmi-vadc.yaml?


Dmitry suggests doing it in a separate patch, I'll follow his suggestion 
if there's no issue.


>> diff --git a/include/dt-bindings/iio/adc/qcom,spmi-adc7-smb139x.h b/include/dt-bindings/iio/adc/qcom,spmi-adc7-smb139x.h
>> index c0680d1285cf..750a526af2c1 100644
>> --- a/include/dt-bindings/iio/adc/qcom,spmi-adc7-smb139x.h
>> +++ b/include/dt-bindings/iio/adc/qcom,spmi-adc7-smb139x.h
>> @@ -6,7 +6,7 @@
>>   #ifndef _DT_BINDINGS_QCOM_SPMI_VADC_SMB139X_H
>>   #define _DT_BINDINGS_QCOM_SPMI_VADC_SMB139X_H
>>   
>> -#include <dt-bindings/iio/qcom,spmi-vadc.h>
>> +#include <dt-bindings/iio/adc/qcom,spmi-vadc.h>
> 
> ? How is it related?

This should have gone into patch 1, I'll fix it in the next patch series.

I'll address all your other comments in the next patchset.

Thanks,

Jishnu

> 
>>   
>>   #define SMB139x_1_ADC7_SMB_TEMP			(SMB139x_1_SID << 8 | ADC7_SMB_TEMP)
>>   #define SMB139x_1_ADC7_ICHG_SMB			(SMB139x_1_SID << 8 | ADC7_ICHG_SMB)
>> diff --git a/include/dt-bindings/iio/adc/qcom,spmi-vadc.h b/include/dt-bindings/iio/adc/qcom,spmi-vadc.h
>> index ef07ecd4d585..cfe653d945a4 100644
>> --- a/include/dt-bindings/iio/adc/qcom,spmi-vadc.h
>> +++ b/include/dt-bindings/iio/adc/qcom,spmi-vadc.h
>> @@ -1,6 +1,8 @@
>>   /* SPDX-License-Identifier: GPL-2.0 */
>>   /*
>>    * Copyright (c) 2012-2014,2018,2020 The Linux Foundation. All rights reserved.
>> + *
> 
> Drop stray blank line
> 
> Best regards,
> Krzysztof
>