From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [PATCH 1/2] dt-bindings: iio: adc: Add DT binding document for PMIC5 ADC References: <1525815501-27588-1-git-send-email-smohanad@codeaurora.org> <20180512111543.4f65dddd@archlinux> From: Siddartha Mohanadoss Message-ID: <18857b67-0865-7794-d981-87beb3f9c22f@codeaurora.org> Date: Tue, 15 May 2018 16:57:03 -0700 MIME-Version: 1.0 In-Reply-To: <20180512111543.4f65dddd@archlinux> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US To: Jonathan Cameron Cc: linux-iio@vger.kernel.org, devicetree@vger.kernel.org, Hartmut Knaack , Lars-Peter Clausen , Peter Meerwald-Stadler , Rob Herring , cdevired@codeaurora.org, rphani@codeaurora.org, sivaa@codeaurora.org List-ID: Hi Jonathan, Thanks for the comments. On 05/12/2018 03:15 AM, Jonathan Cameron wrote: > On Tue, 8 May 2018 14:38:21 -0700 > Siddartha Mohanadoss wrote: > >> PMIC5 ADC has support for clients to measure voltage and current >> on inputs connected to the PMIC. Clients include reading voltage >> phone power and on board system thermistors for thermal management. >> ADC5 on certain PMIC has support to read battery current. >> >> This change adds documentation. >> >> Signed-off-by: Siddartha Mohanadoss > Hi Siddartha, > > Some complexity in here! Anyhow, a few comments inline and we will definitely > be wanting guidance from the devicetree people for this one. > > Jonathan > >> --- >> .../devicetree/bindings/iio/adc/qcom,spmi-adc5.txt | 137 +++++++++++++++++++++ >> 1 file changed, 137 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5.txt >> >> diff --git a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5.txt b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5.txt >> new file mode 100644 >> index 0000000..c9268ba >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5.txt >> @@ -0,0 +1,137 @@ >> +Qualcomm Technologies Inc. SPMI PMIC5 voltage and current ADC >> + >> +SPMI PMIC5 voltage ADC (ADC) provides interface to clients to read >> +voltage. The VADC is a 16-bit sigma-delta ADC. >> + >> +ADC node: >> + >> +- compatible: >> + Usage: required >> + Value type: >> + Definition: Should contain "qcom,spmi-adc5" for PMIC5 ADC driver. >> + Should contain "qcom,spmi-adc-rev2" for PMIC refresh ADC driver. >> + >> +- reg: >> + Usage: required >> + Value type: >> + Definition: VADC base address and length in the SPMI PMIC register map. >> + >> +- #address-cells: >> + Usage: required >> + Value type: >> + Definition: Must be one. Child node 'reg' property should define ADC >> + channel number. >> + >> +- #size-cells: >> + Usage: required >> + Value type: >> + Definition: Must be zero. >> + >> +- #io-channel-cells: >> + Usage: required >> + Value type: >> + Definition: Must be one. For details about IIO bindings see: >> + Documentation/devicetree/bindings/iio/iio-bindings.txt >> + >> +- interrupts: >> + Usage: optional >> + Value type: >> + Definition: End of conversion interrupt. >> + >> +Channel node properties: >> + >> +- reg: >> + Usage: required >> + Value type: >> + Definition: ADC channel number. >> + See include/dt-bindings/iio/qcom,spmi-vadc.h >> + >> +- label: >> + Usage: required >> + Value type: >> + Definition: ADC datasheet channel name. >> + For thermistor inputs connected to generic AMUX or GPIO inputs >> + these can vary across platform for the same pins. Hence select >> + the datasheet name for this channel. >> + >> +- qcom,pre-scaling: >> + Usage: required >> + Value type: >> + Definition: Used for scaling the channel input signal before the signal is >> + fed to VADC. The configuration for this node is to know the >> + pre-determined ratio and use it for post scaling. Select one from >> + the following options. >> + <1 1>, <1 3>, <1 4>, <1 6>, <1 20>, <1 8>, <10 81>, <1 10> >> + If property is not found default value depending on chip will be used. >> + >> +- qcom,decimation: >> + Usage: optional >> + Value type: >> + Definition: This parameter is used to decrease ADC sampling rate. >> + Quicker measurements can be made by reducing decimation ratio. >> + For PMIC5 ADC, combined two step decimation values are 250, 420 and 840. >> + If property is not found, default value of 840 will be used. > The odd indenting here needs sorting. Mixture of spaces and tabs at the moment. Ok, will take a look. > Hmm. In someways this is a policy decision so should be pushed up to userspace, > but given the 'right' value will be somewhat dependent on what you are doing > with the channel and what is wired to it, it could arguably have a 'right' value > for a given circuit. This is really just the sampling frequency wrapped > up in decimation of something, I'm guessing some input clock? Yes. It's number of samples collected over a 4.8MHz clock. The only reason to update this value would be if client wants to get the conversion results back sooner. Hence left this as an optional property. > > Let's see what the Device-tree people think on this one! Personally I have > never really minded devicetree providing sensible defaults. We can put > control on these things later, if there is a usecase for changing them. > >> + For PMIC refresh ADC, supported decimation values are 256, 512, 1024. >> + If property is not found, default value of 1024 will be used. >> + >> +- qcom,ratiometric: >> + Usage: optional >> + Value type: >> + Definition: Channel calibration type. 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 reference channels, also >> + known as absolute calibration. >> + >> +- qcom,hw-settle-time: >> + Usage: optional >> + Value type: >> + Definition: Time between AMUX getting configured and the ADC starting >> + conversion. >> + For PMIC5, delay = 15us for value 0, >> + 100us * (value) for values 0 < value < 11, and >> + 2ms * (value - 10) otherwise. >> + Valid values are: 15, 100, 200, 300, 400, 500, 600, 700, 800, > This description is very confusing given the different uses of 'value' > None of the values you have allowed is less than 11 so the first condition > doesn't apply. The 'value' is an index programmed in the hardware to achieve the hardware settling delay specified under valid values. I will update the documentation here. > >> + 900 us and 1, 2, 4, 6, 8, 10 ms >> + If property is not found, channel will use 15us. >> + For PMIC rev2, delay = 100us * (value) for values 0 < value < 11, and >> + 2ms * (value - 10) otherwise. >> + Valid values are: 0, 100, 200, 300, 400, 500, 600, 700, 800, >> + 900 us and 1, 2, 4, 6, 8, 10 ms >> + If property is not found, channel will use 0 us. >> + >> +- qcom,avg-samples: >> + Usage: optional >> + Value type: >> + Definition: Number of samples to be used for measurement. >> + Averaging provides the option to obtain a single measurement >> + from the ADC that is an average of multiple samples. The value >> + selected is 2^(value). >> + Valid values are: 1, 2, 4, 8, 16 >> + If property is not found, 1 sample will be used. > As with decimation, this is arguably not a feature of the hardware, but > a software decision... > >> + >> +Example: >> + >> + /* VADC node */ >> + pmic_vadc: vadc@3100 { >> + compatible = "qcom,spmi-adc5"; >> + reg = <0x3100 0x100>; >> + interrupts = <0x0 0x31 0x0 IRQ_TYPE_EDGE_RISING>; >> + #address-cells = <1>; >> + #size-cells = <0>; >> + #io-channel-cells = <1>; >> + io-channel-ranges; >> + >> + /* Channel node */ >> + vph_pwr { >> + reg = ; >> + label = "vph_pwr"; >> + qcom,pre-scaling = <1 3>; >> + }; >> + }; >> + >> + /* IIO client node */ >> + usb { >> + io-channels = <&pmic_vadc ADC_VPH_PWR>; >> + io-channel-names = "vadc"; >> + };