On Wed, May 11, 2011 at 08:39:12AM -0400, Jean Delvare wrote: > Hi Guenter, Ira, > > On Tue, 10 May 2011 19:26:47 -0700, Guenter Roeck wrote: > > On Tue, May 10, 2011 at 04:38:44PM -0400, Ira W. Snyder wrote: > > > I've been maintaining my own port of the ina209 driver. Now that the > > > "in*_lcrit" interfaces are added to the sysfs-interface, we only have > > > two disagreements: > > > > > > 1) shunt voltage is reported in uV instead of mV > > > > > > This avoids losing half of the precision supported by the chip. This > > > makes a big difference for my application: I need it. IMHO, the > > > sysfs-interface was short sighted. > > I'm fairly certain I already explained months ago, but... > > The sysfs interface was originally designed in 2005. Back then, the > lowest monitored system voltage was 3.3V, with memory voltage at 1.8V > (DDR2) and CPU core voltage in the 1.0-1.5V. Most monitoring chips were > using ADCs with a resolution of 16 mV. Using 1 mV as the base unit > seemed totally reasonable. As a matter of fact, we now see ADCs with > resolution 12 mV or 8 mV, and our 1 mV unit is still totally fine. > > Your problem is that you are not monitoring a voltage. You are > monitoring a current, so requirements are totally different. And it's > not always possible to anticipate future requirements. Current > measurement was totally out of the scope of hardware monitoring > initially... And I would claim it still is, BTW. Probably the IIO > subsystem would be better suited at the need. > I disagree here. Current measurement is critical to detect over-current conditions. This may indicate a short or some other problem, and require an immediate system shutdown to prevent (further) damage to the HW - especially to the voltage converters. > > > We'll see more chips in the future > > > that are sensitive enough to report in uV. > > > > mV seems to be pretty accurate for voltage measurements. > > The shunt voltage is really just a reflection of the voltage across > > the current shunt resistor and reflects a current, not a voltage. > > > > Sure, chips can sense voltages in the uV range, but that is typically > > not used for voltage but for current measurements. With a 0.001 Ohm > > shunt resistor, 1uV translates to 1mA. Reporting that "voltage" in mA > > makes, in my opinion, more sense than reporting it in uV and expecting > > a common library to perform a translation of a reported voltage into mA. > > > > Having said that, Jean has a support ticket open to implement such a conversion: > > http://www.lm-sensors.org/ticket/2258 > > If he ever gets to it, I suspect we will have to address the uV issue, > > since mV are definitely insufficient for reporting shunt resistor voltages. > > Yes, I remember we discussed this lately, and I didn't have the time to > work on this so far, sorry. > No need to be sorry. I don't even know if it is a good idea - definitely not for cases like here, where it is well known that the measured object reflects a current. > I have nothing against extending the sysfs interface to support uV > voltages. But this must be done in a way which doesn't break current > installations. Arbitrarily reporting in uV instead of the standard mV > isn't an option, neither for the ina209 driver alone, nor for all > drivers. > > One way to do it would be to invent a brand new set of attributes for > uV voltages. For example volt[0-*]_input, etc. This is straightforward > and won't break anything, and would be transparent for applications, > however this would incur a lot of redundancy in the sysfs interface > definition document and in libsensors. If done smartly, this may be > bearable. The user constraints would be: you need the latest version of > libsensors for ina219 support, otherwise the attributes in question > aren't visible at all. > > Another way would be to create a dedicated attribute for hwmon drivers > to advertise whether they are reporting voltages in uV or mV. For > example, file "unit_in" would contain -6 when the chip reports voltages > in uV, -3 if the chip reports voltages in mV (which would be the > default for backwards compatibility.) This would require very little > work in the sysfs-interface document and in libsensors, and would be > transparent to applications. The user constraints would be: you need > the latest version of libsensors for ina219 support, otherwise the > attributes in question are improperly scaled. Whether this is better or > worse than the constraints of the first option can be discussed... It > has the problem that the incorrectness is silent, but the advantage > that it can be corrected in sensors.conf if known (with, again, the > problem that a future update of libsensors will require a fix of > sensors.conf.) > I would prefer something like inX_unit, ie per-sensor attributes. But I don't have a strong opinion about it either. > I don't have a strong opinion. I don't think the INA219 chip is popular > enough that the second option is problematic. But if we go that route, > we should implement the libsensors side immediately so that it starts > propagating to the user base. > > Note, the second option would also have the benefit that we can switch > to nV or V or kV at any time if we need to support strange hardware > monitoring devices. I don't expect it, but I didn't expect that we'd > need uV either, so... > I am still not sure if we need it now. As we both noticed, what is really measured here is current, not voltage. > If we go for the second option, I presume we would do the same for > current and power. Just in case... > > > > I'm sure this is why the power* sysfs-interface is exported in uW > > > (microwatt). Eventually, devices will be sensitive enough to provide > > > data with this very fine sensitivity. > > > > I don't know for sure, but I suspect it is simply because mV * mA = uW. > > Yes, I think that was the reasoning behind it. And also a simple "play > it safe" decision, because the feature was new and we didn't really > know what to expect from devices (contrary to voltages in 2005 as we > had 7 years of experience behind us.) > > But you know, maybe someday someone will need nW and yell at us because > we didn't anticipate it ;) > ... or because measuring uW is really ridiculous and we should have picked mW instead for consistency ;). > > > 2) current calculation is not handled on-chip, it is handled by > > > libsensors > > > > > > This was done to sidestep the "configuration register" setup which is > > > needed for the chip. The value of this register must be computed using > > > the datasheet and your specific sense resistor value in your specific > > > circuit. By computing current in userspace using the sense voltage, we > > > sidestep the issue completely. > > > > The chips provide current as > > Current Register = Shunt Register * Calibration Register / 4096 > > > > so the question for me would be why it would be necessary report the raw value > > of the shunt resistor voltage in the first place and not just stick with in0 > > (in mV) and curr1 (in mA). If you want to report the "raw" value, you could > > report it in mA as suggested above (ie assume a 0.001 Ohm shunt resistor). > > > > Part of the problem here is how to provide configuration data. > > The overall expectation for chip configuration is that it should be handled > > in the BIOS, ie before Linux starts. For applications where this is > > not the case, would be to define platform data to provide the necessary > > information. Another alternative would be to use OF (see the ads1015 driver > > for an example). Using sysfs attributes for this purpose is not a good idea. > > +1 > > > Jean had some reservations about assuming a 0.001 Ohm shunt resistor. See > > http://article.gmane.org/gmane.linux.documentation/2744/ > > Providing platform data to configure the chips would definitely be > > a better option. > > Well well... If it makes everyone's life easier, I would be happy to > (let you) document the 0.001 Ohm rule in D/h/sysfs-interface and > possibly sensors.conf.5. > I would not call it a rule ... maybe a guideline. If there is something better than "assume a 0.001 Ohm shunt resistor", I'd be all for it. It is just the best idea I was able to up with ... I still think that is better than reporting known current measurements as voltages and expecting user space to convert it to currents - especially if it is as obvious as it is here. Thanks, Guenter _______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
