Re: [PATCH 06/11] iio: adc: Add QCOM PMIC5 Gen3 ADC bindings

[Krzysztof Kozlowski]

On 16/11/2023 04:23, Jishnu Prakash wrote:
> Hi Krzysztof,
> 
> On 10/23/2023 12:06 PM, Krzysztof Kozlowski wrote:
>> On 23/10/2023 08:14, Jishnu Prakash wrote:
>>> Hi Krzysztof,
>>>
>>> On 7/9/2023 10:53 PM, Krzysztof Kozlowski wrote:
>>>
>>>>>      reg:
>>>>>        description: VADC base address in the SPMI PMIC register map
>>>>> -    maxItems: 1
>>>>> +    minItems: 1
>>>> Why? This does not make any sense. With previous patches it looks like
>>>> random set of changes.
>>> The idea here is to convey that reg can have multiple values for ADC5
>>> Gen3 as there can be more than one peripheral used for ADC, so there can
>>> be multiple base addresses. I'll try to make this more clear in the next
>>> patchset.
>> You cannot remove constraints from an entry.
> 
> 
> In this case, minItems: 1 will remain true for all other ADC devices 
> documented here, but it will not be true for ADC5 Gen3, as this one can 
> have multiple base addresses if more than one SDAM is used for ADC. I'll 
> update this separately for each compatible, keeping it the same for the 
> older ones, hope that should work.

You responded like you disagree with me, so just in case:  my comment
stays. If you resend the same, I will NAK it.

> 
> 
>>>>>    
>>>>>      '#address-cells':
>>>>>        const: 1
>>>>> @@ -38,6 +39,12 @@ properties:
>>>>>      '#size-cells':
>>>>>        const: 0
>>>>>    
>>>>>    
>>>>> +      qcom,adc-tm-type:
>>>>> +        description: |
>>>>> +            Indicates if ADC_TM monitoring is done on this channel.
>>>> Description does not match property name.
>>> You mean it sounds more like an enum which can take several values
>>> rather than just a boolean? I can update it to "qcom,adc-tm" if that
>>> looks better.
>> The property name suggests this is type of monitoring. Property
>> description says this will enable ADC_TM monitoring. These two do not match.
>>
>> Except that I wonder now whether this is a property of hardware at
>> all... What is this monitoring? By the driver?
> 
> 
> The property description is right, this property is used to indicate 
> that one of the configurable channels on the ADC SDAMs will be used for 
> ADC_TM functionality, for periodically monitoring this particular ADC 
> channel . This is the exact same functionality as in the existing QCOM 
> ADC_TM device, documented at 
> devicetree/bindings/thermal/qcom-spmi-adc-tm5.yaml. I'll mention this 
> too in the description.
> 
> It can be considered a property of the hardware as the monitoring is 
> done by a sequence under PBS (Programmable Boot Sequence, can be 
> considered firmware), which periodically gets the channel reading and 
> checks it against upper/lower thresholds set by clients of this driver, 
> for threshold violations.
> 
> 
>> ...
>>
>>>>>        then:
>>>>>          patternProperties:
>>>>> @@ -299,7 +315,7 @@ examples:
>>>>>                    label = "xo_therm";
>>>>>                };
>>>>>    
> 
>>>>> diff --git a/include/dt-bindings/iio/qcom,spmi-adc5-gen3-pm8550.h b/include/dt-bindings/iio/qcom,spmi-adc5-gen3-pm8550.h
>>>>> new file mode 100644
>>>>> index 000000000000..74e6e2f6f9ed
>>>>> --- /dev/null
>>>>> +++ b/include/dt-bindings/iio/qcom,spmi-adc5-gen3-pm8550.h
>>>>> @@ -0,0 +1,48 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>>> Dual license.
>>> I think we do have an internal rule by which we do have to add these two
>>> licenses....I'll check again and update them if required.
>> Just to be clear: your internal rules are your internal affair. We
>> expect here dual license.
> 
> 
> I misunderstood what you meant earlier, I understand now that 
> "GPL-2.0-only" is wrong, I'll update it.

If only you run checkpatch before sending patches...

Best regards,
Krzysztof