mbox series

[V2,0/3] iio: adc: Add support for QCOM SPMI PMIC5 Gen3 ADC

Message ID 20231116032530.753192-1-quic_jprakash@quicinc.com
Headers show
Series iio: adc: Add support for QCOM SPMI PMIC5 Gen3 ADC | expand

Message

Jishnu Prakash Nov. 16, 2023, 3:25 a.m. UTC
PMIC5 Gen3 has a similar ADC architecture to that on PMIC5 Gen2,
with all SW communication to ADC going through PMK8550 which
communicates with other PMICs through PBS. The major difference is
that the register interface used here is that of an SDAM present on
PMK8550, rather than a dedicated ADC peripheral. There may be more than one
SDAM used for ADC5 Gen3. Each ADC SDAM has eight channels, each of which may
be used for either immediate reads (same functionality as previous PMIC5 and
PMIC5 Gen2 ADC peripherals) or recurring measurements (same as PMIC5 and PMIC5
Gen2 ADC_TM functionality). In this case, we have VADC and ADC_TM functionality
combined into the same driver.

Patches 1 adds bindings for ADC5 Gen3 peripheral.

Patches 2 adds driver support for ADC5 Gen3.

Patch 3 is a cleanup, to move the QCOM ADC dt-bindings files from
dt-bindings/iio to dt-bindings/iio/adc folder, as they are
specifically for ADC devices. It also fixes all compilation errors
with this change in driver and devicetree files and similar errors
in documentation for dtbinding check.

Changes since v1:
- Dropped patches 1-5 for changing 'ADC7' peripheral name to 'ADC5 Gen2'.
- Addressed reviewer comments for binding and driver patches for ADC5 Gen3.
- Combined patches 8-11 into a single patch as requested by reviewers to make
  the change clearer and made all fixes required in same patch.

Jishnu Prakash (3):
  dt-bindings: iio: adc: Add QCOM PMIC5 Gen3 ADC bindings
  iio: adc: Add support for QCOM PMIC5 Gen3 ADC
  dt-bindings: iio/adc: Move QCOM ADC bindings to iio/adc folder

 .../bindings/iio/adc/qcom,spmi-vadc.yaml      |  185 ++-
 .../bindings/mfd/qcom,spmi-pmic.yaml          |    2 +-
 .../bindings/thermal/qcom-spmi-adc-tm-hc.yaml |    2 +-
 .../bindings/thermal/qcom-spmi-adc-tm5.yaml   |    6 +-
 arch/arm64/boot/dts/qcom/pm2250.dtsi          |    2 +-
 arch/arm64/boot/dts/qcom/pm6125.dtsi          |    2 +-
 arch/arm64/boot/dts/qcom/pm6150.dtsi          |    2 +-
 arch/arm64/boot/dts/qcom/pm6150l.dtsi         |    2 +-
 arch/arm64/boot/dts/qcom/pm660.dtsi           |    2 +-
 arch/arm64/boot/dts/qcom/pm660l.dtsi          |    2 +-
 arch/arm64/boot/dts/qcom/pm7250b.dtsi         |    2 +-
 arch/arm64/boot/dts/qcom/pm8150.dtsi          |    2 +-
 arch/arm64/boot/dts/qcom/pm8150b.dtsi         |    2 +-
 arch/arm64/boot/dts/qcom/pm8150l.dtsi         |    2 +-
 arch/arm64/boot/dts/qcom/pm8916.dtsi          |    2 +-
 arch/arm64/boot/dts/qcom/pm8950.dtsi          |    2 +-
 arch/arm64/boot/dts/qcom/pm8953.dtsi          |    2 +-
 arch/arm64/boot/dts/qcom/pm8994.dtsi          |    2 +-
 arch/arm64/boot/dts/qcom/pm8998.dtsi          |    2 +-
 arch/arm64/boot/dts/qcom/pmi632.dtsi          |    2 +-
 arch/arm64/boot/dts/qcom/pmi8950.dtsi         |    2 +-
 arch/arm64/boot/dts/qcom/pmm8155au_1.dtsi     |    2 +-
 arch/arm64/boot/dts/qcom/pmp8074.dtsi         |    2 +-
 arch/arm64/boot/dts/qcom/pms405.dtsi          |    2 +-
 arch/arm64/boot/dts/qcom/sc7280-idp.dts       |    2 +-
 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi      |    2 +-
 arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi    |    4 +-
 arch/arm64/boot/dts/qcom/sc8180x-pmics.dtsi   |    2 +-
 .../qcom/sc8280xp-lenovo-thinkpad-x13s.dts    |    6 +-
 .../boot/dts/qcom/sm7225-fairphone-fp4.dts    |    2 +-
 arch/arm64/boot/dts/qcom/sm8450-hdk.dts       |    8 +-
 drivers/iio/adc/Kconfig                       |   25 +
 drivers/iio/adc/Makefile                      |    1 +
 drivers/iio/adc/qcom-spmi-adc5-gen3.c         | 1189 +++++++++++++++++
 drivers/iio/adc/qcom-spmi-adc5.c              |    2 +-
 drivers/iio/adc/qcom-spmi-vadc.c              |    2 +-
 .../iio/adc/qcom,spmi-adc5-gen3-pm8550.h      |   50 +
 .../iio/adc/qcom,spmi-adc5-gen3-pm8550b.h     |   89 ++
 .../iio/adc/qcom,spmi-adc5-gen3-pm8550vx.h    |   22 +
 .../iio/adc/qcom,spmi-adc5-gen3-pmk8550.h     |   56 +
 .../iio/{ => adc}/qcom,spmi-adc7-pm8350.h     |    2 +-
 .../iio/{ => adc}/qcom,spmi-adc7-pm8350b.h    |    2 +-
 .../iio/{ => adc}/qcom,spmi-adc7-pmk8350.h    |    2 +-
 .../iio/{ => adc}/qcom,spmi-adc7-pmr735a.h    |    2 +-
 .../iio/{ => adc}/qcom,spmi-adc7-pmr735b.h    |    0
 .../iio/{ => adc}/qcom,spmi-vadc.h            |   81 ++
 46 files changed, 1725 insertions(+), 61 deletions(-)
 create mode 100644 drivers/iio/adc/qcom-spmi-adc5-gen3.c
 create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550.h
 create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550b.h
 create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550vx.h
 create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pmk8550.h
 rename include/dt-bindings/iio/{ => adc}/qcom,spmi-adc7-pm8350.h (98%)
 rename include/dt-bindings/iio/{ => adc}/qcom,spmi-adc7-pm8350b.h (99%)
 rename include/dt-bindings/iio/{ => adc}/qcom,spmi-adc7-pmk8350.h (97%)
 rename include/dt-bindings/iio/{ => adc}/qcom,spmi-adc7-pmr735a.h (95%)
 rename include/dt-bindings/iio/{ => adc}/qcom,spmi-adc7-pmr735b.h (100%)
 rename include/dt-bindings/iio/{ => adc}/qcom,spmi-vadc.h (77%)

Comments

Jishnu Prakash Nov. 16, 2023, 6:29 a.m. UTC | #1
Hi Dmitry,

On 11/16/2023 10:52 AM, Dmitry Baryshkov wrote:
> On Thu, 16 Nov 2023 at 05:26, Jishnu Prakash <quic_jprakash@quicinc.com> wrote:
>> PMIC5 Gen3 has a similar ADC architecture to that on PMIC5 Gen2,
>> with all SW communication to ADC going through PMK8550 which
>> communicates with other PMICs through PBS. The major difference is
>> that the register interface used here is that of an SDAM present on
>> PMK8550, rather than a dedicated ADC peripheral. There may be more than one
>> SDAM used for ADC5 Gen3. Each ADC SDAM has eight channels, each of which may
>> be used for either immediate reads (same functionality as previous PMIC5 and
>> PMIC5 Gen2 ADC peripherals) or recurring measurements (same as PMIC5 and PMIC5
>> Gen2 ADC_TM functionality). In this case, we have VADC and ADC_TM functionality
>> combined into the same driver.
>>
>> Patches 1 adds bindings for ADC5 Gen3 peripheral.
>>
>> Patches 2 adds driver support for ADC5 Gen3.
> For some reason I don't see this patch in my inbox. Maybe it will
> arrive later. Immediate response: please add
> devm_thermal_add_hwmon_sysfs().


Yes, I'll check and add this in the next patch series, I'll wait for 
some more comments on the existing patches for now.

I ran into some error after sending the first two mails (cover letter 
and patch 1), so I sent patches 2 and 3 separately after it, I think you 
may have received them separately.


>
>> Patch 3 is a cleanup, to move the QCOM ADC dt-bindings files from
>> dt-bindings/iio to dt-bindings/iio/adc folder, as they are
>> specifically for ADC devices. It also fixes all compilation errors
>> with this change in driver and devicetree files and similar errors
>> in documentation for dtbinding check.
> NAK. The kernel is expected to build and work after each commit.
> Otherwise git-bisecting the kernel becomes impossible.
> So, please rework your series in a way that there are no compilation
> errors after any of the patches. The easiest way would be to rearrange
> your patches in 3-1-2 order.


I think you may have misunderstood the meaning here, I had verified 
compilation works each time after applying each of the three patches in 
this series. It's not that this last patch fixes compilation errors 
caused by the first two, this is a completely separate patch which 
affects existing QCOM ADC code (driver and devicetree) including ADC5 Gen3.


This patch does two things mainly:

Move the ADC binding files from dt-bindings/iio folder to 
dt-bindings/iio/adc folder (this would naturally cause some errors in 
driver and devicetree code due to path update)

Fix all compilation and dtbinding errors generated by the move


I added this change at the end of the series as I was not completely 
sure if it could get picked, just wanted to make it easier to drop if 
that is the final decision.


Thanks,

Jishnu


>
>
>> Changes since v1:
>> - Dropped patches 1-5 for changing 'ADC7' peripheral name to 'ADC5 Gen2'.
>> - Addressed reviewer comments for binding and driver patches for ADC5 Gen3.
>> - Combined patches 8-11 into a single patch as requested by reviewers to make
>>    the change clearer and made all fixes required in same patch.
>>
>>   .../iio/{ => adc}/qcom,spmi-adc7-pm8350b.h    |    2 +-
>>   .../iio/{ => adc}/qcom,spmi-adc7-pmk8350.h    |    2 +-
>>   .../iio/{ => adc}/qcom,spmi-adc7-pmr735a.h    |    2 +-
>>   .../iio/{ => adc}/qcom,spmi-adc7-pmr735b.h    |    0
>>   .../iio/{ => adc}/qcom,spmi-vadc.h            |   81 ++
>>   46 files changed, 1725 insertions(+), 61 deletions(-)
>>   create mode 100644 drivers/iio/adc/qcom-spmi-adc5-gen3.c
>>   create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550.h
>>   create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550b.h
>>   create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550vx.h
>>   create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pmk8550.h
>>   rename include/dt-bindings/iio/{ => adc}/qcom,spmi-adc7-pm8350.h (98%)
>>   rename include/dt-bindings/iio/{ => adc}/qcom,spmi-adc7-pm8350b.h (99%)
>>   rename include/dt-bindings/iio/{ => adc}/qcom,spmi-adc7-pmk8350.h (97%)
>>   rename include/dt-bindings/iio/{ => adc}/qcom,spmi-adc7-pmr735a.h (95%)
>>   rename include/dt-bindings/iio/{ => adc}/qcom,spmi-adc7-pmr735b.h (100%)
>>   rename include/dt-bindings/iio/{ => adc}/qcom,spmi-vadc.h (77%)
>>
>> --
>> 2.25.1
>>
>
> --
> With best wishes
> Dmitry
Dmitry Baryshkov Nov. 16, 2023, 6:58 a.m. UTC | #2
On Thu, 16 Nov 2023 at 08:30, Jishnu Prakash <quic_jprakash@quicinc.com> wrote:
>
> Hi Dmitry,
>
> On 11/16/2023 10:52 AM, Dmitry Baryshkov wrote:
> > On Thu, 16 Nov 2023 at 05:26, Jishnu Prakash <quic_jprakash@quicinc.com> wrote:
> >> PMIC5 Gen3 has a similar ADC architecture to that on PMIC5 Gen2,
> >> with all SW communication to ADC going through PMK8550 which
> >> communicates with other PMICs through PBS. The major difference is
> >> that the register interface used here is that of an SDAM present on
> >> PMK8550, rather than a dedicated ADC peripheral. There may be more than one
> >> SDAM used for ADC5 Gen3. Each ADC SDAM has eight channels, each of which may
> >> be used for either immediate reads (same functionality as previous PMIC5 and
> >> PMIC5 Gen2 ADC peripherals) or recurring measurements (same as PMIC5 and PMIC5
> >> Gen2 ADC_TM functionality). In this case, we have VADC and ADC_TM functionality
> >> combined into the same driver.
> >>
> >> Patches 1 adds bindings for ADC5 Gen3 peripheral.
> >>
> >> Patches 2 adds driver support for ADC5 Gen3.
> > For some reason I don't see this patch in my inbox. Maybe it will
> > arrive later. Immediate response: please add
> > devm_thermal_add_hwmon_sysfs().
>
>
> Yes, I'll check and add this in the next patch series, I'll wait for
> some more comments on the existing patches for now.
>
> I ran into some error after sending the first two mails (cover letter
> and patch 1), so I sent patches 2 and 3 separately after it, I think you
> may have received them separately.
>
>
> >
> >> Patch 3 is a cleanup, to move the QCOM ADC dt-bindings files from
> >> dt-bindings/iio to dt-bindings/iio/adc folder, as they are
> >> specifically for ADC devices. It also fixes all compilation errors
> >> with this change in driver and devicetree files and similar errors
> >> in documentation for dtbinding check.
> > NAK. The kernel is expected to build and work after each commit.
> > Otherwise git-bisecting the kernel becomes impossible.
> > So, please rework your series in a way that there are no compilation
> > errors after any of the patches. The easiest way would be to rearrange
> > your patches in 3-1-2 order.
>
>
> I think you may have misunderstood the meaning here, I had verified
> compilation works each time after applying each of the three patches in
> this series. It's not that this last patch fixes compilation errors
> caused by the first two, this is a completely separate patch which
> affects existing QCOM ADC code (driver and devicetree) including ADC5 Gen3.
>
>
> This patch does two things mainly:
>
> Move the ADC binding files from dt-bindings/iio folder to
> dt-bindings/iio/adc folder (this would naturally cause some errors in
> driver and devicetree code due to path update)
>
> Fix all compilation and dtbinding errors generated by the move
>
>
> I added this change at the end of the series as I was not completely
> sure if it could get picked, just wanted to make it easier to drop if
> that is the final decision.

Ah, so patch 1 adds new files to <dt-bindings/iio/adc>, while
retaining old files in the old directory. I'd say, this is
counterintuitive.
Please reorder patches into 3-1-2 order. dt-binding changes anyway
should come first.

>
>
> Thanks,
>
> Jishnu
>
>
> >
> >
> >> Changes since v1:
> >> - Dropped patches 1-5 for changing 'ADC7' peripheral name to 'ADC5 Gen2'.
> >> - Addressed reviewer comments for binding and driver patches for ADC5 Gen3.
> >> - Combined patches 8-11 into a single patch as requested by reviewers to make
> >>    the change clearer and made all fixes required in same patch.
> >>
> >>   .../iio/{ => adc}/qcom,spmi-adc7-pm8350b.h    |    2 +-
> >>   .../iio/{ => adc}/qcom,spmi-adc7-pmk8350.h    |    2 +-
> >>   .../iio/{ => adc}/qcom,spmi-adc7-pmr735a.h    |    2 +-
> >>   .../iio/{ => adc}/qcom,spmi-adc7-pmr735b.h    |    0
> >>   .../iio/{ => adc}/qcom,spmi-vadc.h            |   81 ++
> >>   46 files changed, 1725 insertions(+), 61 deletions(-)
> >>   create mode 100644 drivers/iio/adc/qcom-spmi-adc5-gen3.c
> >>   create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550.h
> >>   create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550b.h
> >>   create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550vx.h
> >>   create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pmk8550.h
> >>   rename include/dt-bindings/iio/{ => adc}/qcom,spmi-adc7-pm8350.h (98%)
> >>   rename include/dt-bindings/iio/{ => adc}/qcom,spmi-adc7-pm8350b.h (99%)
> >>   rename include/dt-bindings/iio/{ => adc}/qcom,spmi-adc7-pmk8350.h (97%)
> >>   rename include/dt-bindings/iio/{ => adc}/qcom,spmi-adc7-pmr735a.h (95%)
> >>   rename include/dt-bindings/iio/{ => adc}/qcom,spmi-adc7-pmr735b.h (100%)
> >>   rename include/dt-bindings/iio/{ => adc}/qcom,spmi-vadc.h (77%)
> >>
> >> --
> >> 2.25.1
> >>
> >
> > --
> > With best wishes
> > Dmitry
Krzysztof Kozlowski Nov. 16, 2023, 11:43 a.m. UTC | #3
On 16/11/2023 04:25, Jishnu Prakash wrote:
> For the PMIC5-Gen3 type PMICs, ADC peripheral is present in HW for the
> following PMICs: PMK8550, PM8550, PM8550B and PM8550VX PMICs.
> 

A nit, subject: drop second/last, redundant "bindings". The
"dt-bindings" prefix is already stating that these are bindings.

> It is similar to PMIC5-Gen2, with SW communication to ADCs on all PMICs
> going through PBS(Programmable Boot Sequence) firmware through a single
> register interface. This interface is implemented on an SDAM (Shared
> Direct Access Memory) peripheral on the master PMIC PMK8550 rather
> than a dedicated ADC peripheral.
> 
> Add documentation for PMIC5 Gen3 ADC and macro definitions for ADC
> channels and virtual channels (combination of ADC channel number and
> PMIC SID number) per PMIC, to be used by clients of this device.
> 
> Changes since v1:
> - Updated properties separately for all compatibles to clarify usage
>   of new properties and updates in usage of old properties for ADC5 Gen3.
> - Avoided updating 'adc7' name to 'adc5 gen2' and just left a comment
>   mentioning this convention.
> - Used predefined channel IDs in individual PMIC channel definitions
>   instead of numeric IDs.
> - Addressed other comments from reviewers.
> 
> Co-developed-by: Anjelique Melendez <quic_amelende@quicinc.com>
> Signed-off-by: Anjelique Melendez <quic_amelende@quicinc.com>
> Signed-off-by: Jishnu Prakash <quic_jprakash@quicinc.com>
> ---
>  .../bindings/iio/adc/qcom,spmi-vadc.yaml      | 181 ++++++++++++++++--
>  .../iio/adc/qcom,spmi-adc5-gen3-pm8550.h      |  50 +++++
>  .../iio/adc/qcom,spmi-adc5-gen3-pm8550b.h     |  89 +++++++++
>  .../iio/adc/qcom,spmi-adc5-gen3-pm8550vx.h    |  22 +++
>  .../iio/adc/qcom,spmi-adc5-gen3-pmk8550.h     |  56 ++++++
>  include/dt-bindings/iio/qcom,spmi-vadc.h      |  81 ++++++++
>  6 files changed, 464 insertions(+), 15 deletions(-)
>  create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550.h
>  create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550b.h
>  create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550vx.h
>  create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pmk8550.h
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.yaml b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.yaml
> index ad7d6fc49de5..c988bd74a247 100644
> --- a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.yaml
> @@ -13,8 +13,10 @@ maintainers:
>  description: |
>    SPMI PMIC voltage ADC (VADC) provides interface to clients to read
>    voltage. The VADC is a 15-bit sigma-delta ADC.
> -  SPMI PMIC5/PMIC7 voltage ADC (ADC) provides interface to clients to read
> -  voltage. The VADC is a 16-bit sigma-delta ADC.
> +  SPMI PMIC5/PMIC7/PMIC5 Gen3 voltage ADC (ADC) provides interface to
> +  clients to read voltage. The VADC is a 16-bit sigma-delta ADC.
> +  Note that PMIC7 ADC is the generation between PMIC5 and PMIC5 Gen3 ADC,
> +  it can be considered like PMIC5 Gen2.
>  
>  properties:
>    compatible:
> @@ -23,14 +25,18 @@ properties:
>            - const: qcom,pms405-adc
>            - const: qcom,spmi-adc-rev2
>        - enum:
> -          - qcom,spmi-vadc
> -          - qcom,spmi-adc5
>            - qcom,spmi-adc-rev2
> +          - qcom,spmi-adc5
> +          - qcom,spmi-adc5-gen3
>            - qcom,spmi-adc7
> +          - qcom,spmi-vadc
>  
>    reg:
> -    description: VADC base address in the SPMI PMIC register map
> -    maxItems: 1

NAK.

I wrote it multiple times. You canno remove the widest constraints from
top-level property.

> +    description: |
> +      - For compatible properties "qcom,spmi-vadc", "qcom,spmi-adc5", "qcom,spmi-adc-rev2"
> +        and "qcom,spmi-adc7", reg is the VADC base address in the SPMI PMIC register map.
> +      - For compatible property "qcom,spmi-adc5-gen3", each reg corresponds
> +        to an SDAM peripheral base address that is being used for ADC.
>  
>    '#address-cells':
>      const: 1
> @@ -38,13 +44,26 @@ properties:
>    '#size-cells':
>      const: 0
>  
> +  "#thermal-sensor-cells":
> +    const: 1
> +    description:
> +      Number of cells required to uniquely identify the thermal sensors. Since
> +      we have multiple sensors this is set to 1. For compatible property
> +      "qcom,spmi-adc5-gen3", this property is required for ADC_TM device.
> +
>    '#io-channel-cells':
>      const: 1
>  
>    interrupts:
> -    maxItems: 1

No, srsly. We went through it.


> -    description:
> +    description: |
>        End of conversion interrupt.
> +      - For compatible property "qcom,spmi-adc5-gen3", interrupts are defined
> +        for each SDAM being used.
> +
> +  interrupt-names:
> +    description: |

You must describe the names which also provides constraints.

I am not going to review the rest of the file.

Best regards,
Krzysztof
Jonathan Cameron Nov. 25, 2023, 7:35 p.m. UTC | #4
On Thu, 16 Nov 2023 08:58:03 +0200
Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:

> On Thu, 16 Nov 2023 at 08:30, Jishnu Prakash <quic_jprakash@quicinc.com> wrote:
> >
> > Hi Dmitry,
> >
> > On 11/16/2023 10:52 AM, Dmitry Baryshkov wrote:  
> > > On Thu, 16 Nov 2023 at 05:26, Jishnu Prakash <quic_jprakash@quicinc.com> wrote:  
> > >> PMIC5 Gen3 has a similar ADC architecture to that on PMIC5 Gen2,
> > >> with all SW communication to ADC going through PMK8550 which
> > >> communicates with other PMICs through PBS. The major difference is
> > >> that the register interface used here is that of an SDAM present on
> > >> PMK8550, rather than a dedicated ADC peripheral. There may be more than one
> > >> SDAM used for ADC5 Gen3. Each ADC SDAM has eight channels, each of which may
> > >> be used for either immediate reads (same functionality as previous PMIC5 and
> > >> PMIC5 Gen2 ADC peripherals) or recurring measurements (same as PMIC5 and PMIC5
> > >> Gen2 ADC_TM functionality). In this case, we have VADC and ADC_TM functionality
> > >> combined into the same driver.
> > >>
> > >> Patches 1 adds bindings for ADC5 Gen3 peripheral.
> > >>
> > >> Patches 2 adds driver support for ADC5 Gen3.  
> > > For some reason I don't see this patch in my inbox. Maybe it will
> > > arrive later. Immediate response: please add
> > > devm_thermal_add_hwmon_sysfs().  
> >
> >
> > Yes, I'll check and add this in the next patch series, I'll wait for
> > some more comments on the existing patches for now.
> >
> > I ran into some error after sending the first two mails (cover letter
> > and patch 1), so I sent patches 2 and 3 separately after it, I think you
> > may have received them separately.
> >
> >  
> > >  
> > >> Patch 3 is a cleanup, to move the QCOM ADC dt-bindings files from
> > >> dt-bindings/iio to dt-bindings/iio/adc folder, as they are
> > >> specifically for ADC devices. It also fixes all compilation errors
> > >> with this change in driver and devicetree files and similar errors
> > >> in documentation for dtbinding check.  
> > > NAK. The kernel is expected to build and work after each commit.
> > > Otherwise git-bisecting the kernel becomes impossible.
> > > So, please rework your series in a way that there are no compilation
> > > errors after any of the patches. The easiest way would be to rearrange
> > > your patches in 3-1-2 order.  
> >
> >
> > I think you may have misunderstood the meaning here, I had verified
> > compilation works each time after applying each of the three patches in
> > this series. It's not that this last patch fixes compilation errors
> > caused by the first two, this is a completely separate patch which
> > affects existing QCOM ADC code (driver and devicetree) including ADC5 Gen3.
> >
> >
> > This patch does two things mainly:
> >
> > Move the ADC binding files from dt-bindings/iio folder to
> > dt-bindings/iio/adc folder (this would naturally cause some errors in
> > driver and devicetree code due to path update)
> >
> > Fix all compilation and dtbinding errors generated by the move
> >
> >
> > I added this change at the end of the series as I was not completely
> > sure if it could get picked, just wanted to make it easier to drop if
> > that is the final decision.  
> 
> Ah, so patch 1 adds new files to <dt-bindings/iio/adc>, while
> retaining old files in the old directory. I'd say, this is
> counterintuitive.
> Please reorder patches into 3-1-2 order. dt-binding changes anyway
> should come first.

Absolutely agree.  Refactors, cleanup etc should precede the new stuff
in a series.  That way they can get picked up by anyone who wants to backport
without having to first figure out if they want the new stuff.

Jonathan

> 
> >
> >
> > Thanks,
> >
> > Jishnu
> >
> >  
> > >
> > >  
> > >> Changes since v1:
> > >> - Dropped patches 1-5 for changing 'ADC7' peripheral name to 'ADC5 Gen2'.
> > >> - Addressed reviewer comments for binding and driver patches for ADC5 Gen3.
> > >> - Combined patches 8-11 into a single patch as requested by reviewers to make
> > >>    the change clearer and made all fixes required in same patch.
> > >>
> > >>   .../iio/{ => adc}/qcom,spmi-adc7-pm8350b.h    |    2 +-
> > >>   .../iio/{ => adc}/qcom,spmi-adc7-pmk8350.h    |    2 +-
> > >>   .../iio/{ => adc}/qcom,spmi-adc7-pmr735a.h    |    2 +-
> > >>   .../iio/{ => adc}/qcom,spmi-adc7-pmr735b.h    |    0
> > >>   .../iio/{ => adc}/qcom,spmi-vadc.h            |   81 ++
> > >>   46 files changed, 1725 insertions(+), 61 deletions(-)
> > >>   create mode 100644 drivers/iio/adc/qcom-spmi-adc5-gen3.c
> > >>   create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550.h
> > >>   create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550b.h
> > >>   create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550vx.h
> > >>   create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pmk8550.h
> > >>   rename include/dt-bindings/iio/{ => adc}/qcom,spmi-adc7-pm8350.h (98%)
> > >>   rename include/dt-bindings/iio/{ => adc}/qcom,spmi-adc7-pm8350b.h (99%)
> > >>   rename include/dt-bindings/iio/{ => adc}/qcom,spmi-adc7-pmk8350.h (97%)
> > >>   rename include/dt-bindings/iio/{ => adc}/qcom,spmi-adc7-pmr735a.h (95%)
> > >>   rename include/dt-bindings/iio/{ => adc}/qcom,spmi-adc7-pmr735b.h (100%)
> > >>   rename include/dt-bindings/iio/{ => adc}/qcom,spmi-vadc.h (77%)
> > >>
> > >> --
> > >> 2.25.1
> > >>  
> > >
> > > --
> > > With best wishes
> > > Dmitry  
> 
> 
>
Jishnu Prakash Dec. 21, 2023, 8 a.m. UTC | #5
Hi Krzysztof,

On 11/16/2023 5:13 PM, Krzysztof Kozlowski wrote:
> On 16/11/2023 04:25, Jishnu Prakash wrote:
>> For the PMIC5-Gen3 type PMICs, ADC peripheral is present in HW for the
>> following PMICs: PMK8550, PM8550, PM8550B and PM8550VX PMICs.
>>
> A nit, subject: drop second/last, redundant "bindings". The
> "dt-bindings" prefix is already stating that these are bindings.
>
>>   
>>     reg:
>> -    description: VADC base address in the SPMI PMIC register map
>> -    maxItems: 1
> NAK.
>
> I wrote it multiple times. You canno remove the widest constraints from
> top-level property.
>

>>     '#io-channel-cells':
>>       const: 1
>>   
>>     interrupts:
>> -    maxItems: 1
> No, srsly. We went through it.


Is it fine if I add the bindings for ADC5 Gen3 in a new file? It's not 
just for the reg and interrupts properties, I think it would make sense 
to have a new file as ADC5 Gen3 is a new device combining the functions 
of the existing QCOM VADC and ADC_TM devices.


>
>
>> -    description:
>> +    description: |
>>         End of conversion interrupt.
>> +      - For compatible property "qcom,spmi-adc5-gen3", interrupts are defined
>> +        for each SDAM being used.
>> +
>> +  interrupt-names:
>> +    description: |
> You must describe the names which also provides constraints.


I'll update this in the next patchset.

Thanks,

Jishnu


>
> I am not going to review the rest of the file.
>
> Best regards,
> Krzysztof
>
Krzysztof Kozlowski Dec. 21, 2023, 8:04 a.m. UTC | #6
On 21/12/2023 09:00, Jishnu Prakash wrote:
> Hi Krzysztof,
> 
> On 11/16/2023 5:13 PM, Krzysztof Kozlowski wrote:
>> On 16/11/2023 04:25, Jishnu Prakash wrote:
>>> For the PMIC5-Gen3 type PMICs, ADC peripheral is present in HW for the
>>> following PMICs: PMK8550, PM8550, PM8550B and PM8550VX PMICs.
>>>
>> A nit, subject: drop second/last, redundant "bindings". The
>> "dt-bindings" prefix is already stating that these are bindings.
>>
>>>   
>>>     reg:
>>> -    description: VADC base address in the SPMI PMIC register map
>>> -    maxItems: 1
>> NAK.
>>
>> I wrote it multiple times. You canno remove the widest constraints from
>> top-level property.
>>
> 
>>>     '#io-channel-cells':
>>>       const: 1
>>>   
>>>     interrupts:
>>> -    maxItems: 1
>> No, srsly. We went through it.
> 
> 
> Is it fine if I add the bindings for ADC5 Gen3 in a new file? It's not 
> just for the reg and interrupts properties, I think it would make sense 
> to have a new file as ADC5 Gen3 is a new device combining the functions 
> of the existing QCOM VADC and ADC_TM devices.
> 

The patch is no longer in my mailbox, that was more than a month ago.

Best regards,
Krzysztof