On Mon, 2023-12-11 at 07:36 -0800, Guenter Roeck wrote: > On Tue, Dec 05, 2023 at 04:22:56PM +0100, Nuno Sa via B4 Relay wrote: > > From: Nuno Sa <nuno.sa@xxxxxxxxxx> > > > > The LTC4282 hot swap controller allows a board to be safely inserted and > > removed from a live backplane. Using one or more external N-channel pass > > transistors, board supply voltage and inrush current are ramped up at an > > adjustable rate. An I2C interface and onboard ADC allows for monitoring > > of board current, voltage, power, energy and fault status. > > > > Signed-off-by: Nuno Sa <nuno.sa@xxxxxxxxxx> > > --- > > + > > > +power1_good Power considered good > > I really don't like this attribute. Like the ones below it is non-standard > and invisible for standard applications. On top of that, I think it isn't > really related to "power" but to the output voltage. What does it actually > report that isn't included in the FET faults ? > This is detected with the FB pin and a voltage divider (from the output voltage). Basically depending on the level of that pin, the chip indicate power good or power bad. I was also very reluctant with this attribute (I mention it in the v1 cover). This might not even indicate any misbehave. We also support reporting this using the gpio1 pin (if we set it that way). So, I guess I can just drop this one and add support for it if we ever have a real usecase where I can actually justify having it :). We already have the power_bad fault log in debugfs so I'm not sure if adding this one there adds much value. > > > +fet_short_fault FET short alarm > > +fet_bad_fault FET bad alarm > > Those attributes have little value since they are not standard attributes > and won't be seen by standard applications. On top of that, it is not clear > (not documented) what the attribute actually reports. I assume it is > associated with the output voltage, i.e., in0, but that is just an > assumption. > fet_short - This is one is detected if the ADC measures a current sense voltage > 0.25mv while the fet gate is off. fet_bad - Is set by monitoring the voltage at the gate and the drain to source voltage. These ones might indicate real issues with the HW so I thought they could be important... > What do you think about introducing a standard inX_fault attribute ? > It would not be as specific as short/bad, but I think it would be more > useful and we could add it to the ABI. > It would be better than nothing. And we do have fault logs for both these failures so userspace could also use that to know exactly what was the issue. If that's ok with you, I would then report this in inX_fault? Did you had in mind putting this in in0 (vsource) or adding a new channel? In my first draft I had another voltage channel (label: VFET) to report the fet_bad condition. I was using the inX_crit or inX_lcrit but it felt bad so I removed it... > > +fault_logs_reset Clears all the Logged Faults > What exactly does that do that is user visible ? Well, this one is because in some configurations the chip won't enable the output load until you reset/clear the fault log keeping it from enabling the output. This is the comment I have in the code: "Fault log failures. These faults might be important in systems where auto-retry is not enabled since they will cause the part to latch off until they are cleared. Typically that happens when the system admin is close enough so he can check what happened and manually clear the faults. Moreover, manually clearing the faults might only matter when ON_FAULT_MASK in the CONTROL register is set (which is the default) as in that case, a turn off signal from the ON pin won't clear them." In v1 I was allowing to clear fauls log individually and you recommended to have an attribute to clear them all at once as that would simplify things. I just kept it in here because this might be important for the chip to work as expected again so having it in debugfs might be weird. Thanks! - Nuno Sá
