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
