On Tue, May 15, 2018 at 04:57:03PM -0700, Siddartha Mohanadoss wrote: > 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 <smohanad@xxxxxxxxxxxxxx> 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 <smohanad@xxxxxxxxxxxxxx> > > 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: <string> > > > + Definition: Should contain "qcom,spmi-adc5" for PMIC5 ADC driver. > > > + Should contain "qcom,spmi-adc-rev2" for PMIC refresh ADC driver. Chip specific compatible strings please unless you convince me there are a large number of chips per above compatible. Bindings are for hardware, not drivers. > > > + > > > +- reg: > > > + Usage: required > > > + Value type: <prop-encoded-array> > > > + Definition: VADC base address and length in the SPMI PMIC register map. > > > + > > > +- #address-cells: > > > + Usage: required > > > + Value type: <u32> > > > + Definition: Must be one. Child node 'reg' property should define ADC > > > + channel number. > > > + > > > +- #size-cells: > > > + Usage: required > > > + Value type: <u32> > > > + Definition: Must be zero. > > > + > > > +- #io-channel-cells: > > > + Usage: required > > > + Value type: <u32> > > > + Definition: Must be one. For details about IIO bindings see: > > > + Documentation/devicetree/bindings/iio/iio-bindings.txt > > > + > > > +- interrupts: > > > + Usage: optional > > > + Value type: <prop-encoded-array> > > > + Definition: End of conversion interrupt. > > > + > > > +Channel node properties: > > > + > > > +- reg: > > > + Usage: required > > > + Value type: <u32> > > > + Definition: ADC channel number. > > > + See include/dt-bindings/iio/qcom,spmi-vadc.h > > > + > > > +- label: > > > + Usage: required > > > + Value type: <empty> > > > + 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. Why do you need this? If the name comes from a datasheet list, then perhaps you should list the exact string here. Otherwise, there's a lot left to the user in terms of capitalization, etc. > > > + > > > +- qcom,pre-scaling: > > > + Usage: required > > > + Value type: <u32 array> > > > + 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: <u32> > > > + 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. I don't have a problem with this in DT, though my first thought was it should be common. Then after reading some on decimation, I'm not sure it would always just be a single 32-bit value? > > > > > + 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: <empty> > > > + 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: <u32> > > > + 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: <u32> > > > + 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... We already have a common property for touchscreens and vendor properties for a few ADCs, so we should define a common one. Now, sadly, I've just found that all these properties are already defined in bindings/iio/adc/qcom,spmi-vadc.txt. Why didn't you add these compatibles to the existing binding. Then we're not reviewing the same thing again... Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
