On 03/06/16 12:26, Laxman Dewangan wrote: > > On Friday 03 June 2016 03:36 PM, Jonathan Cameron wrote: >> On 01/06/16 13:34, Laxman Dewangan wrote: >>> Add support for INA3221 SW driver via IIO ADC interface. The device is >>> register as iio-device and provides interface for voltage/current and power >>> monitor. Also provide interface for setting oneshot/continuous mode and >>> critical/warning threshold for the shunt voltage drop. >>> >>> Signed-off-by: Laxman Dewangan <ldewangan@xxxxxxxxxx> >> Hi Laxman, >> >> As ever with any driver lying on the border of IIO and hwmon, please include >> a short justification of why you need an IIO driver and also cc the >> hwmon list + maintainers. (cc'd on this reply). >> >> I simply won't take a driver where the hwmon maintainers aren't happy. >> As it stands I'm not seeing obvious reasons in the code for why this >> should be an IIO device. > > I thought that all ADC or monitors are going to be part of IIO device > framework. I saw the ina2xx which is same (single channel) which was > my reference point. That had a rather specific use case IIRC - they needed the buffered support to get the data fast enough. > >> Funily enough I know this datasheet a little as was evaluating >> it for use on some boards at the day job a week or so ago. >> >> Various comments inline. Major points are: >> * Don't use 'fake' channels to control events. If the events infrastructure >> doesn't handle your events, then fix that rather than working around it. >> * There is a lot of ABI in here concerned with oneshot vs continuous. >> This seems to me to be more than it should be. We wouldn't expect to >> see stuff changing as a result of switching between these modes other >> than wrt to when the data shows up. So I'd expect to not see this >> directly exposed at all - but rather sit in oneshot unless either: >> 1) Buffered mode is running (not currently supported) >> 2) Alerts are on - which I think requires it to be in continuous mode. >> >> Other question to my mind is whether we should be reporting vshunt or >> (using device tree to pass resistance) current. > > This is bus and shunt voltage device for power monitoring. In our > platforms, we use this device for bus current and so power monitor. > > We have two usecases, one is one shot, read when it needs it. And > other continuous when we have multiple core running then continuous > mode to get the power consumption by rail. That's fine, but continuous should be using the buffered interfaces really as that's there explicitly to support groups of channels captured using a sequencer. Then the abi ends up much more standard which is nice. Also allows for high speed ish continuous monitoring which is what the was I think the point of the single channel driver. > > Yaah, alert is used only on continuous mode and mainly used for > throttling when rail power goes beyond some limit. Of interesting in Linux, or routed directly to hardware? > -- > 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 -- 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
