From: Jonathan Cameron <jic23@xxxxxxxxxx> Data: Wednesday, November 27, 2013 4:10 PM >To: Duan Fugang-B38611; Mark Rutland >Cc: sachin.kamat@xxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; shawn.guo@xxxxxxxxxx; >Li Frank-B20596; linux-iio@xxxxxxxxxxxxxxx >Subject: RE: [PATCH v3 3/3] Documentation: add the binding file for Freescale >vf610 ADC driver > > > >Fugang Duan <fugang.duan@xxxxxxxxxxxxx> wrote: >>From: Mark Rutland [mailto:mark.rutland@xxxxxxx] >>Data: Tuesday, November 26, 2013 10:09 PM >> >>>To: Duan Fugang-B38611 >>>Cc: jic23@xxxxxxxxxx; sachin.kamat@xxxxxxxxxx; >>devicetree@xxxxxxxxxxxxxxx; >>>shawn.guo@xxxxxxxxxx; Li Frank-B20596; linux-iio@xxxxxxxxxxxxxxx >>>Subject: Re: [PATCH v3 3/3] Documentation: add the binding file for >>Freescale >>>vf610 ADC driver >>> >>>On Tue, Nov 26, 2013 at 10:56:34AM +0000, Fugang Duan wrote: >>>> The patch adds the binding file for Freescale vf610 ADC driver. >>>> >>>> Signed-off-by: Fugang Duan <B38611@xxxxxxxxxxxxx> >>>> --- >>>> .../devicetree/bindings/iio/adc/vf610-adc.txt | 57 >>>++++++++++++++++++++ >>>> 1 files changed, 57 insertions(+), 0 deletions(-) >>>> >>>> diff --git a/Documentation/devicetree/bindings/iio/adc/vf610-adc.txt >>>> b/Documentation/devicetree/bindings/iio/adc/vf610-adc.txt >>>> new file mode 100644 >>>> index 0000000..4101516 >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/iio/adc/vf610-adc.txt >>>> @@ -0,0 +1,57 @@ >>>> +Freescale vf610 Analog to Digital Converter bindings >>>> + >>>> +The devicetree bindings are for the new ADC driver written for >>>> +vf610/i.MX6slx and upward SoCs from Freescale. >>>> + >>>> +Required properties: >>>> +- compatible: Should be "fsl,vf610-adc" >>> >>>s/be/contain/ >>> >>>> +- reg: Offset and length of the register set for the device >>>> +- interrupts: Should contain the interrupt for the device >>>> +- clocks: The clocks needed by the ADC controller >>> >>>How many? Which ones? >>> >>>> +- clock-names: the name of the clocks >>> >>>Either define the set of names, or don't use clock-names. It's useless >>if it >>>doesn't tell you anything. >>> >>>> + >>>> +Optional properties: >>>> +- fsl,adc-io-pinctl: Enable field for the I/O port control of MCU >>pins used >>>as analog inputs. >>>> +- fsl,adc-vref: ADC refrence voltage value, unit is uV. >>> >>>Can you not query the regulator to figure this out? >> >>This is optional, some board design have no fixed regulator, just >>supply fixed voltage from MCU digital power which Cannot handled by the >>module. >Fixed regulators are used when things either supplied by an actual fixed reg or >when directly connected to another supply. It does not have to correspond to a >bit of real hardware though that MCU digital power is coming from somewhere! > >Hence this definitely wants to be a regulator Good suggestion, thanks ! >> >>> >>>> +- fsl,adc-clk-div: Current clock divider value, such as 1,2,4,8,16 >>and so on. >>> >>>Could you elaborate on this? What's it used for and why is it needed? >> >>Exp: ADC clock source is ipg bus clock, the clk divider can control the >>ADC sample speed while other setting don't change: >> The sample speed that clock divider is equal to 1 is twice than the >>divider is equal to 2. >> >>Anyway, it can control sample speed by user. >If left in device tree then this might be best implemented as a very simple >clock. >I would prefer it to be controlled from user space as sampling_frequency. >> >>> >>>> +- fsl,adc-res: ADC conversion mode selection, such as 8 for 8-bit, >>10 for >>>10-bit, 12 for 12-bit mode. >>> >>>This sounds like something that could be changed at runtime. Why does >>this need >>>to be configured in the DT? >>> >>>> +- fsl,adc-sam-time: ADC sample time duration, number of ADC clocks, >>>> +such as 2, 4, 6, 8, 12, 16, 20, 24 >>> >>>Likewise. >>> >>>Please don't poinltessly abbreviate, "sample" is much better than >>"sam" >>>here... >>> >>>> +- fsl,adc-aver-sam-sel: Determines how many ADC conversions will be >>averaged >>>to create the ADC average result. >>>> + The Optional value is 4, 8, 16, 32. >>> >>>Likewise. >> >> >>For above ADC configuration, ADC conversion mode, sample time duration, >>hardware average sample selection, and so on. >>All of them can changed at runtime. The only method is to introduce the >>ioctl interface for user to change them at runtime. >>How about you think ? >No it is not. Like all similar controls these Should be via sysfs attributes >created via The standard channel structures in IIO. >Sample duration might be integration_time or sampling_frequency depending on >its exact semantics. Averaging is a form of low pass filter and should be >specified as such. > >If it is not already in the abi then propose something new. > >The only one here that I can see an issue with doing that way is the resolution. >So far it hasn't actually made sense to make this controllable.. >Couple of questions on that. >1. What effect on speed of sampling does changing this have? Note that we >probably do not care about this if using sysfs reading. >2. What other disadvantages are there to always running in 12 bit mode? > >I have no problem with adding write support for the type of buffer elements >though it would require some IIO core infrastructure. >However I do want a well described reason to bother with the added complexity. > Thanks very much! Your suggestion is very useful for me. I will use the standard sys interface for ADC configuration. And for your question: 1. effect on speed of sampling: - ADC sample time duration, number of ADC clocks, such as 2, 4, 6, 8, 12, 16, 20, 24 - clock, the clock source is ipg bus clock, current clock divider value for ADC, such as 1,2,4,8,16 and so on - if hardware average function enable, The ADC average sample number also affect speed, determines how many ADC conversions will be averaged to create the ADC average result. All of above setting affect the speed of sampling. If use the sysfs reading, the speed don't need care. So I have some questions: a. For high speed sampling, IIO subsystem how to handle it if there no ioctl interface for user ? it is iio buffer/fifo ? b. For software trigger, the speed of sampling also don't need to care, is it right ? c. If use case need high speed sampling, there must have hareware trigger and iio buffer for it, right ? 2. no disadvantage, I have validated ADC can work fine at 8-bit, 10-bit, 12-bit mode. >> >>> >>>> +- fsl,adc-hw-aver-en: Bool type to decide enable hardware average >>function. >>> >>>When would you wnat this and when wouldn't you? >>> >>>Similarly, "averages" is far clearer than "aver". >> >>Ok, I see. Thanks. >>> >>>> +- fsl,adc-low-power-mode: Bool type to decide enable ADC low power >>mode. >>> >>>Similarly? >This one is interesting. What effects does it have in this device? Sometimes it >is a sampling frequency restriction... >>> >>>> +- fsl,adc-high-speed-conv: Bool type to decide enable ADC high >>speed mode. >>> >>>Similarly? >>> >>>> +- vref: The regulator to support ADC refrence voltage. >>> >>>s/vref/vref-supply/ >>> >>>Thanks, >>>Mark. >> >>Thanks for your review. >>Summary, there have many comments why don't let some ADC configuration >>change at runtime. >>The driver still don't add ioctl interface for user to configurate the >>ADC setting. So the setting are got from DT. >> >>Do you have one good method to change the configuration at runtime ? >See above :) > >In short...Add relevant bits to info_mask_* in all the iio_Chan_spec structures >then put the relevant handling in the read_raw and write_raw iio_info >callbacks. >> >>Thanks, >>Andy >> >>-- >>To unsubscribe from this list: send the line "unsubscribe linux-iio" in >>the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info >>at http://vger.kernel.org/majordomo-info.html > >-- >Sent from my Android phone with K-9 Mail. Please excuse my brevity. Thanks for your review and answers. Andy ��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
