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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.


'#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.

Thanks,

Jishnu


Best regards,
Krzysztof





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux