On 15/04/15 20:11, Thomas Betker wrote: > This patch series fixes some issues found by tests on our Zynq-7020 > board; the issues usually result in incorrect voltage readings: > > [1/5] iio: adc: xilinx: Fix register addresses > [2/5] iio: adc: xilinx: Fix "vccaux" channel .address > [3/5] iio: adc: xilinx: Fix VREFP scale > [4/5] iio: adc: xilinx: Fix VREFN sign > [5/5] iio: adc: xilinx: Set .datasheet_name instead of .extend_name > > About [1]: The register addresses in question are currently not used by > the driver, but we do use them for extended tests. (I might provide a > patch to support lowest/highest values another time; see also [5].) > > About [3], [4]: The transfer functions for VREFP and VREFN are > vrefp = unsigned(code) * 3.0 / 4096 > vrefn = signed(code) * 1.0 / 4096 > I know it's not consistent, but that's what UG480 says ... VREFP = 1.25 > definitely needs the factor 3.0 to get reasonable values (a factor 1.0 > will never get you more than 1.0V). As for VREFN = 0V, we did actually > see negative values on our boards. > > About [5]: This may be a matter of debate on the IIO list, but I didn't > get the impression that extensions are intended to be used that way. > .extend_name = "vccint" means that the sysfs node names are > in_voltage0_vccint_raw/_scale, which is a bit unexpected. I would > rather see them called just in_voltage0_*, and reserve extensions for > things like in_voltage0_lowest_* and in_voltage0_highest_* (MIN_VCCINT, > MAX_VCCINT) -- that's what we do in our project. It was always intended for labelling of channels with well defined purposes. That is ones that are hard wired to something rather than simply exposed for general purpose use. I'm guessing vccint is the internal supply voltage, in which case that was pretty much the intended use. The datasheet name is only really there for binding consumers to individual channels. The thought being that you'd probably be sat there with a circuit diagram in front of you specifying what is connected to which precise pin of the ADC... So I think the original approach is more in keeping with the intent. The lowest/highest versions are interesting. Right now, the extended name is the only way to specify them, but somehow it feels like we ought to have something better.... We could treat them as a filter or simply another aspect of the channel (like _scale etc), but that's also a little ugly. Will think about this and see if anyone else has a better suggestion! > Note that .extend_name = "vccint", "vccaux", ... also has the side > effect of creating the sysfs nodes vccint_sampling_frequency, > vccaux_sampling_frequency, ... [due to .info_mask_shared_by_all = > BIT(IIO_CHAN_INFO_SAMP_FREQ)], which is probably not desired. This last one sounds like a bug and definitely isn't the intended result! The extended name should be ignored for shared_by values. > > Best regards, > Thomas Betker > -- 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
