Hi Jishnu, On Fri, 16 Feb 2024 at 12:39, Jishnu Prakash <quic_jprakash@xxxxxxxxxxx> wrote: Please disable sending HTML emails in your email client. It is generally frowned upon, it complicates replying, it breaks quotations, etc. > > Hi Krzysztof, > > On 1/4/2024 1:48 PM, Krzysztof Kozlowski wrote: > > On 31/12/2023 18:12, 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. > > 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 v2: > - Moved ADC5 Gen3 documentation into a separate new file. > > Changelog goes under ---. > > Why did you do this? What is the rationale? Sorry, this patchset goes > nowhere. > > > I'll elaborate this more in the next patchset. There are two main reasons for adding this documentation in a new file: > > 1.This device is not exactly like the existing QCOM VADC drivers as it now combines VADC functionality (reading ADC channel on client request) with ADC_TM functionality (thermal threshold monitoring). > > 2.Adding this device's bindings in the existing qcom,spmi-vadc.yaml file is not possible as it would require updating some of the existing top-level constraints. (for the older devices in that file, "reg" and "interrupts" can have at most one item, while this device can have more than one item under these properties.) > > > 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. > > > + per PMIC in the PMIC-specific files in include/dt-bindings/iio/adc. > + > + label: > + $ref: /schemas/types.yaml#/definitions/string > > Why do you need it in the first place? Don't you miss some $ref? > > > This is just meant to show the ADC channel name in DT for our reference. I'll check if I can use adc.yaml, which includes this property already, as a reference in this case. > > > + description: | > > Do not need '|' unless you need to preserve formatting. Applies everywhere. > > > > + ADC input of the platform as seen in the schematics. > + For thermistor inputs connected to generic AMUX or GPIO inputs > + these can vary across platform for the same pins. Hence select > + the platform schematics name for this channel. > + > > + qcom,adc-tm: > + description: | > + Indicates if ADC_TM monitoring is done on this channel. > + Defined for compatible property "qcom,spmi-adc5-gen3". You are describing qcom,spmi-adc5-gen3, are you not? So this phrase adds nothing. > + This is the same functionality as in the existing QCOM ADC_TM > + device, documented at devicetree/bindings/thermal/qcom-spmi-adc-tm5.yaml. > + type: boolean > + > > Why do you duplicate entire vadc file? Why it cannot be part of that > file? Oh wait, it was in v2. > > You now duplicated a lot of property definitions without clear reason. > If this is intention, then you need to put them in common schema. > > > Many of the properties used for earlier QCOM VADC devices will be used for this device too.....do you mean I can add a new schema file (named something like qcom,vadc.yaml) and move common properties into it (like qcom,hw-settle-time, qcom,decimation, etc) from this file and qcom,spmi-vadc.yaml? > > Can I do it in the same patch or should it be a separate patch coming before this one ? I'd say, separate patch. Move first, extend later. > > > > diff --git a/include/dt-bindings/iio/adc/qcom,spmi-adc7-smb139x.h b/include/dt-bindings/iio/adc/qcom,spmi-adc7-smb139x.h > > index c0680d1285cf..750a526af2c1 100644 > --- a/include/dt-bindings/iio/adc/qcom,spmi-adc7-smb139x.h > +++ b/include/dt-bindings/iio/adc/qcom,spmi-adc7-smb139x.h > @@ -6,7 +6,7 @@ > #ifndef _DT_BINDINGS_QCOM_SPMI_VADC_SMB139X_H > #define _DT_BINDINGS_QCOM_SPMI_VADC_SMB139X_H > > -#include <dt-bindings/iio/qcom,spmi-vadc.h> > +#include <dt-bindings/iio/adc/qcom,spmi-vadc.h> > > ? How is it related? > > > This should have gone into patch 1, I'll fix it in the next patch series. > > I'll address all your other comments in the next patchset. > > Thanks, > > Jishnu > > > > #define SMB139x_1_ADC7_SMB_TEMP (SMB139x_1_SID << 8 | ADC7_SMB_TEMP) > #define SMB139x_1_ADC7_ICHG_SMB (SMB139x_1_SID << 8 | ADC7_ICHG_SMB) > diff --git a/include/dt-bindings/iio/adc/qcom,spmi-vadc.h b/include/dt-bindings/iio/adc/qcom,spmi-vadc.h > index ef07ecd4d585..cfe653d945a4 100644 > --- a/include/dt-bindings/iio/adc/qcom,spmi-vadc.h > +++ b/include/dt-bindings/iio/adc/qcom,spmi-vadc.h > @@ -1,6 +1,8 @@ > /* SPDX-License-Identifier: GPL-2.0 */ > /* > * Copyright (c) 2012-2014,2018,2020 The Linux Foundation. All rights reserved. > + * > > Drop stray blank line > > Best regards, > Krzysztof > -- With best wishes Dmitry
