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

[Jishnu Prakash]

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

> >