Hi Jonathan, On 12/23/2013 12:49 AM, Jonathan Cameron wrote: >To: Duan Fugang-B38611 >Cc: mark.rutland@xxxxxxx; otavio@xxxxxxxxxxxxxxxx; pmeerw@xxxxxxxxxx; >lars@xxxxxxxxxx; linux-iio@xxxxxxxxxxxxxxx >Subject: Re: [PATCH v4 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver > >On 12/16/13 08:17, fugang.duan@xxxxxxxxxxxxx wrote: >> Hi, Jonathan, >> >> On Sunday, December 08, 2013 2:55 AM, Jonathan Cameron <jic23@xxxxxxxxxx> >wrote: >>> To: Duan Fugang-B38611 >>> Cc: shawn.guo@xxxxxxxxxx; Li Frank-B20596; mark.rutland@xxxxxxx; >>> otavio@xxxxxxxxxxxxxxxx; pmeerw@xxxxxxxxxx; lars@xxxxxxxxxx; linux- >>> iio@xxxxxxxxxxxxxxx >>> Subject: Re: [PATCH v4 2/3] iio:adc:imx: add Freescale Vybrid vf610 >>> adc driver >>> >>> On 12/04/13 10:00, Fugang Duan wrote: >>>> Add Freescale Vybrid vf610 adc driver. The driver only support ADC >>>> software trigger. >>>> >>>> CC: Shawn Guo <shawn.guo@xxxxxxxxxx> >>>> CC: Jonathan Cameron <jic23@xxxxxxxxxx> >>>> CC: Mark Rutland <mark.rutland@xxxxxxx> >>>> CC: Otavio Salvador <otavio@xxxxxxxxxxxxxxxx> >>>> CC: Peter Meerwald <pmeerw@xxxxxxxxxx> >>>> CC: Lars-Peter Clausen <lars@xxxxxxxxxx> >>>> Signed-off-by: Fugang Duan <B38611@xxxxxxxxxxxxx> >>> >>> Mostly looking good, but a fair number of comments inline, >>> particularly on the proposed userspace interfaces (which should have >>> been clearly documented) >>> >>> Also, it is often helpful to provide a link to device documentation >>> if it is available? >>> I think : >>> >>> http://cache.freescale.com/files/32bit/doc/ref_manual/VYBRIDRM.pdf?fp >>> sp=1&WT_TY >>> PE=Reference%20Manuals&WT_VENDOR=FREESCALE&WT_FILE_FORMAT=pdf&WT_ASSE >>> T=Document >>> ation >>> >>> is probably the right one - chapter 37. >>> >>> For some of the interfaces proposed, I would like to get a better >>> grasp on whether they actualy want to be exposed to userspace or >>> whether there are 'right' options given other constraints. >>> >>> >>> >>>> --- >>>> drivers/iio/adc/Kconfig | 10 + >>>> drivers/iio/adc/Makefile | 1 + >>>> drivers/iio/adc/vf610_adc.c | 954 >>>> +++++++++++++++++++++++++++++++++++++++++++ >>>> 3 files changed, 965 insertions(+), 0 deletions(-) >> [...] >> >>>> + >>>> +/* >>>> + * Here, just list sample frequency available for >>>> + * software trigger. >>>> + */ >>>> +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("28 32 36 38 42 46 50 54 >>>> +58"); >>>> + >>>> +static IIO_DEVICE_ATTR(resolution_mode, S_IWUSR | S_IRUGO, >>>> + vf610_resolution_mode_show, >>>> + vf610_resolution_mode_store, 0); >>>> + >>>> +static IIO_DEVICE_ATTR(low_power_mode, S_IWUSR | S_IRUGO, >>>> + vf610_low_power_mode_show, >>>> + vf610_low_power_mode_store, 0); >>>> + >>>> +static IIO_DEV_ATTR_SAMP_FREQ(S_IRUGO, >>>> + vf610_read_sample_frequency, >>>> + NULL); >>>> + >>>> +static IIO_CONST_ATTR(clk_divider_available, "1 2 4 8 16"); static >>>> +IIO_DEVICE_ATTR(adc_internal_clk, S_IWUSR | S_IRUGO, >>>> + vf610_adc_internal_clk_show, >>>> + vf610_adc_internal_clk_store, 0); >>> I am guessing that the two above are related to the same control? >>> That isn't implied by there names! >>>> + >>>> +static struct attribute *vf610_attributes[] = { >>>> + &iio_dev_attr_sampling_frequency.dev_attr.attr, >>> >>> Please use the info_mask elements for the relevant channel and add >>> support to read_raw for samp_freq rather than hand coding it here. >>> I'm in the process of pulling all of these out in favour of that >>> support as if it is done like this, then there is no access to these >>> values for drivers within the kernel. >>> >>>> + &iio_const_attr_sampling_frequency_available.dev_attr.attr, >>> Whilst I have proposed a framework to handle *_available attributes >>> via the core, it isn't ready to merge yet so this is currently the >>> right way to do this one! >>> >>>> + &iio_dev_attr_resolution_mode.dev_attr.attr, >>> I raised this question before - is there a use case for polled acess >>> (as provided so far in this driver) which is already inherently slow >>> in which one would want to vary the resolution? >>> If there is then this should be added as a new info_mask element and >>> provided by the read_raw/write_raw callbacks. >>> >> Since the ADC support 8-bit, 10-bit, 12-bit, maybe user change it in runtime, >so add the sys interface. >> You mean that the driver only enable one resolution in default ? >That's what I was thinking. Whilst a user might of course change the >resolution is there any real reason for them to do so whilst using the >relatively slow interface that is implemented here (sysfs access) Agree it, try to simply it as much as possible. >> >>>> + &iio_dev_attr_low_power_mode.dev_attr.attr, >>> Some comments on this above. >>> >> I will remove the interface. >> >>>> + &iio_dev_attr_adc_internal_clk.dev_attr.attr, >>> These are very low level interfaces that won't make much sense to >>> expose to userspace. There is no documentation on when one would >>> change this that I can see. It probably effects a number of things... >>> >>> The possibly related long sample configuration is interesting.. >>> >>>From the datasheet, these would typically be changed to cope with high >>>impedances or to allow for fast operation / lower power operation >>>when the impedance of the connected source is low.... Given this is >>>about coping with input impedances I'd argue that this is perhaps >>>something that does belong in device tree >>> perhaps as a 'hint' on the input impedance. The driver when >>> then only offer possible sampling frequencies given the restrictions >>>applied whatever is being measured (reflected in the clk divider). >>> >>>> + &iio_const_attr_clk_divider_available.dev_attr.attr, >>> Anything here that is no currently documented in >>> Documentation/bindings/testing/sysfs-bus-iio* needs to be proposed >>> formally as a new ABI element with documentation. >>> >> The ADC sample frequency: >> IPG clk -> divide 1 -> ADC conversion time = SFCAdder + AverageNum x (BCT + >LSTAdder) >> divide 2 >> divide 4 >> divide 8 >> divide 16 >> >> >> * SFCAdder: fix to 8 ADCK cycles >> * AverageNum: 1, 4, 32 samples for hardware average. >> * BCT (Base Conversion Time): >> * - 17 ADCK cycles for 8 bit mode >> * - 21 ADCK cycles for 10 bit mode >> * - 25 ADCK cycles for 12 bit mode >> *LSTAdder(Long Sample Time): >> * - 3 ADCK cycles >> * - 13 ADCK cycles >> * - 25 ADCK cycles >> >> >> Base on above formula, we can get one frequency group: >> static const u16 vf610_sample_freq_avail[3][9] = { >> {28, 38, 50, 88, 128, 176, 648, 968, 1352}, // for 8-bit >> {32, 42, 54, 104, 144, 192, 776, 1096, 1480}, // for 10-bit >> {36, 46, 58, 120, 160, 208, 904, 1224, 1608}, // for 12-bit }; >> >> * For each frequency group: >> * Entry 0,1,2 -> 1 sample for each conversion >> * Entry 3,4,5 -> hardware average 4 samples >> * Entry 6,7,8 -> hardware average 32 samples >> >> >> For ADC sample frequency, unit is ADCK cycles, which is not Hz. >> So, the driver use info_mask elements "IIO_CHAN_INFO_SAMP_FREQ" to read/write >avail frequency, unit is ADCK cycles. > >No. Can't do that. The unit of sampling frequency is Hz. There is no >flexibility on this at all. We simply cannot have different drivers working in >different units. > You are right, all driver must be generalized. >> User can read "adc_in_voltage_sampling_frequency" to get the value. >> The driver use sys interface " adc_internal_clk " to read/write clock >after divider. clk_divider_available: 1 2 4 8 16. >> The driver use sys interface " sampling_frequency " to read the >> real ADC frequency, which is adc_internel_clk / >> in_voltage_sampling_frequency >> Exp: >> Resolution is 8 bit mode, 1 sample for each conversion, IPG clock is >66Mhz, divider is 2, we can get: >> Read sys "adc_in_voltage_sampling_frequency", which must be 28 ADCK >cycles. >> Read sys "adc_internal_clk", which must be 66Mhz/2=33Mhz. >> Read sys "sampling_frequency", which is the real ADC sample >> frequency: 33Mhz/28=1.18Mhz >> >> >> If re-calculate the static avail frequency with adding clock divider array to >make the avail values are real frequency (hz), it may be more complex. >> This is the current implementation, so you see the driver added some non- >standard iio sys interfaces. >> Do you have any other suggestion ? > >Firstly, what I'm always primarily interested in is generic interfaces that can >cover any similar devices. What we are describing here will be too device >specific if we are not careful. > >So the things being controlled are: > >1) The sampling frequency (how often we can measure the channel). > >2) A filter applied to the samples taken (averaging). > >3) The resolution. > >4) The sample time - effectively a front end filter rather than a digital one I >think.... > >5) the clock speed used to driver the above. > > >The sampling frequency clearly describes in Hz the rate at which input samples >are taken at the front end of any filtering. Here that is a rather complex >thing to compute but it needs to be done! > >For the averaging filter, we do have it described the other way around already >using oversampling, but reconsidering that I'm not entirely sure that is the >best way of describing it and presumably the filter for a typical oversampling >adc is rather more complex than a single average. Perhaps we need something >like in_filter_subsample (values of 1, 4 and 32 here) in_filter_mean_window >(values of 1, 4, and 32 as well) > >This would allow us to subsample more complex filters and to have moving >average filtering without subsampling described rather than the case here where >they are linked (note that changing any IIO attribute can change any other so >having a linked pair - in this case - isn't a problem). Note that the setting >of this averaging filter will effect the output data rate, but as this is after >sampling it will NOT effect the sampling_frequency attribute. > >The input clock divider is an odd one. It is no different to an externally >provided clock (and we have one of those here driving it). Hence we need to >treat that more generally. Do we need a way for IIO to >a) Query what the input clocks can be so as to provide _available attributes >b) Request an input clock to allow for particular sampling frequencies etc? >I have no particular problem with doing this, but it needs to be generalized. >We should not care that this clock divider is part of the ADC element of the >SoC from the point of view of the interface. > >If you want a quick 'reasonable' solution, I'd suggest implementing it as a >normal clock chip provided by and used by the ADC. Thus any future interface >would control it just fine. If you really want this functionality then we need >the generalized version, not something device specific. > >Resolution is an interesting one. Right now we have this reasonably well >defined (though not as yet writable!) for buffered output, but have taken the >view that if people are using sysfs to read a channel then they aren't that >fussed about how quickly the read it and will hence want the highest possible >resolution. Unless there are very clear reasons why we need to change this I >think it will prove a messy nightmare given that resolution changes tend to >make every single other element (scale, offset, sampling frequency etc) change. > >According to the datasheet, the long sample time option is all about handling >high impedance inputs. Sounds like a wiring question for me (hence device >tree) rather than something where a userspace interface makes sense. > >How many of the above want to be controlled on a per channel basis? Worth >thinking about now, before you get stuck with an ABI where they are all >shared... > Sys interface is slow speed interface, so resolution, clock divider are not necessary, just Use normal clock for ADC modules. From above of your suggestion, I think I need to simply the implemention. >I'm growing to really dislike SoC adcs ;) Every piece of hardware does >something new making generic interfaces (which are a must if anyone is ever >going to >use them) harder! It seems that with discrete parts, things are simpler >as people chose an appropriate one for their application, whereas when someone >sticks and ADC on the side of a SoC they decide to make it possible to >everything but in a way that makes it very hard to have variable in a clean >fashion. > > Yes, I'am also dislike SOC adcs. But it is a trend for future ADCs market. There will have more specific function/features, which are hard for software to implement in generalized interfaces. Thanks for your suggestion! Wish you a merry Christmas day! 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
