Re: [PATCH 01/11] iio: adc: Update bindings for ADC7 name used on QCOM PMICs

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

 



Hi Jonathan,

Sorry for the late reply, I could not get back earlier as I got occupied with other work till now. I have addressed your comments inline.

On 7/8/2023 8:28 PM, Jonathan Cameron wrote:
On Sat, 8 Jul 2023 12:58:25 +0530
Jishnu Prakash <quic_jprakash@xxxxxxxxxxx> wrote:

The name used initially for this version of Qualcomm Technologies, Inc.
PMIC ADC was ADC7, following the convention of calling the PMIC generation
PMIC7. However, the names were later amended internally to ADC5 Gen2 and
PMIC5 Gen2. In addition, the latest PMIC generation now is known as
PMIC5 Gen3 with ADC5 Gen3 supported on it. With this addition, it makes more
sense to correct the name for this version of ADCs to ADC5 Gen2 from ADC7.
Since this affects ADC devices across some PMICs, update the names accordingly.

In order to avoid breaking the existing implementations of ADC7, add
support for ADC5 Gen2 first now and remove the ADC7 support in a later
patch.

Signed-off-by: Jishnu Prakash <quic_jprakash@xxxxxxxxxxx>
Hi Jishnu.

Whilst I can appreciate why you've picked this particular approach to
deal with the renames I'm not sure it's the smoothest path - or the
easiest to review.

If doing a single patch for the complete rename was too much, perhaps
doing one header (or if it makes sense set of headers)
at a time would be easier to read?  With a final patch doing the compatible
addition.  Maybe let's see what other reviewers think though.


I don't completely understand what you mean here - but first let me briefly recap what I was trying to do here.

In patches 1-5 of this series, I intended to update all existing support for ADC7 by renaming it to ADC5 Gen2 to match the correct name used internally. In addition, since I am adding support for ADC5 Gen3 in patches 6 and 7, I thought it would make sense to rename this older peripheral, to make it more obvious to everyone that this version lies between ADC5 and ADC5 Gen3.

The patches were organized like  this:

Patch 1 - Update documentation to add gen2 compatible and update examples(without removing older compatible). Add new binding files equivalent to existing ADC7 files, just with macros and file names updated to use "adc5_gen2" instead of "adc7"

Patch 2 - Update driver files to replace usage of "adc7" with "adc5 gen2", adding new compatible for adc5 gen2 without removing exsiting one for adc7.

Patch 3 - Update compatible, macros and binding files included in all devicetree files, based on the earlier two changes.

Patch 4 - Delete all instances of adc7 compatible from documentation files. Delete all older binding files

Patch 5 - Delete the adc7 compatible from the driver


Based on the comments I got, I understand I cannot proceed as such with patches 4 and 5, I can amend/drop them. But to get back to your above point about my overall approach, how exactly would you like me to structure my patch series?

Should I make one big patch for documentation, bindings, driver and devicetree changes where I update the naming and deprecate adc7 usage? This may be straightforward but also hard to review.


Or a patch series like this:

One patch to update documentation

One patch to update the bindings (headers) (Or one patch per header file?)

One patch to update driver file (adding new compatible and comment to deprecate old one)

One patch to update all devicetree files (or separate patches?)

Please let me know what you think.

A few other comments inline,

Jonathan


properties:
@@ -27,6 +27,7 @@ properties:
            - qcom,spmi-adc5
            - qcom,spmi-adc-rev2
            - qcom,spmi-adc7
+          - qcom,spmi-adc5-gen2
Alphabetical order (roughly given currently list). So I'd stick
this after qcom,spmi-adc5


Will reorder them in the next patchset.


reg:
      description: VADC base address in the SPMI PMIC register map
@@ -71,7 +72,7 @@ patternProperties:
          description: |
            ADC channel number.
            See include/dt-bindings/iio/qcom,spmi-vadc.h
-          For PMIC7 ADC, the channel numbers are specified separately per PMIC
+          For PMIC5 Gen2 ADC, the channel numbers are specified separately per PMIC
            in the PMIC-specific files in include/dt-bindings/iio/.
label:
@@ -114,7 +115,7 @@ patternProperties:
                channel calibration. If property is not found, channel will be
                calibrated with 0.625V and 1.25V reference channels, also
                known as absolute calibration.
-            - For compatible property "qcom,spmi-adc5", "qcom,spmi-adc7" and
+            - For compatible property "qcom,spmi-adc5", "qcom,spmi-adc5-gen2" and
                "qcom,spmi-adc-rev2", if this property is specified VADC will use
                the VDD reference (1.875V) and GND for channel calibration. If
                property is not found, channel will be calibrated with 0V and 1.25V
@@ -213,7 +214,9 @@ allOf:
        properties:
          compatible:
            contains:
-            const: qcom,spmi-adc7
+            enum :
+                - qcom,spmi-adc7
There is a deprecated marking for dt-bindings. Might be good to use it here.


Thanks for your suggestion, I'll do this in the next patchset.



+                - qcom,spmi-adc5-gen2
then:

Thanks,

Jishnu






[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