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

[Jonathan Cameron <jic23@kernel.org>]

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  
> 
> 
>