Message-ID: <CAA8EJpq+2cu4pyWRGm_DVQe7_6NJAssT=HWD6UieyXkAgncwMA@mail.gmail.com>
Date:   Thu, 16 Nov 2023 08:58:03 +0200
From:   Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To:     Jishnu Prakash <quic_jprakash@...cinc.com>
Cc:     jic23@...nel.org, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
        agross@...nel.org, andersson@...nel.org, konrad.dybcio@...aro.org,
        daniel.lezcano@...aro.org, linus.walleij@...aro.org,
        linux-arm-msm@...r.kernel.org, andriy.shevchenko@...ux.intel.com,
        quic_subbaram@...cinc.com, quic_collinsd@...cinc.com,
        quic_amelende@...cinc.com, quic_kamalw@...cinc.com,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        marijn.suijten@...ainline.org, lars@...afoo.de, luca@...tu.xyz,
        linux-iio@...r.kernel.org, lee@...nel.org, rafael@...nel.org,
        rui.zhang@...el.com, lukasz.luba@....com,
        cros-qcom-dts-watchers@...omium.org, sboyd@...nel.org,
        linux-pm@...r.kernel.org, linux-arm-msm-owner@...r.kernel.org,
        kernel@...cinc.com
Subject: Re: [PATCH V2 0/3] iio: adc: Add support for QCOM SPMI PMIC5 Gen3 ADC

On Thu, 16 Nov 2023 at 08:30, Jishnu Prakash <quic_jprakash@...cinc.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@...cinc.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



-- 
With best wishes
Dmitry

Powered by blists - more mailing lists