On Wed, Jun 05, 2019 at 01:22:21PM -0600, Robert Hancock wrote: > On 2019-06-05 12:27 p.m., Guenter Roeck wrote: > >>> To reduce risk due to potentially mis-detecting support on other chips, > >>> it may be better to add a separate backend driver for this chip. This > >>> would also enable support for the MFR_VOUT_PEAK, MFR_IOUT_PEAK, and > >>> MFR_TEMPERATURE_PEAK registers which is otherwise unavailable. > >> > >> To clarify, you're saying instead of having the auto-detection for those > >> in the generic pmbus driver, create a separate irps5401 driver that > >> would explicitly add in those parameters? > >> > > Yes. > > OK, I can do that. We initially had a separate driver file for this, but > ended up changing it to update the generic detection instead. But the > separate driver file is likely safer. > > > > >> This particular patch to pmbus_core.c would still be required in order > >> for the paged parameters to be displayed properly - it shouldn't break > >> anything on chips that don't detect these parameters on multiple pages. > >> > > > > It should still work, though, except that there would be duplicate labels. > > I don't think it actually does work, at least not in the sensors utility > - I haven't diagnosed where it was going wrong, but either not all of > the attributes are successfully added at the kernel level, or libsensors > filters out the "duplicate" entries and you only see one of them in the > output. > Possibly it is a problem with libsensors. I don't have any of those chips (or evaluation boards), so I never noticed. > Actually, it appears there are already drivers in tree that have > multiple pages with VIN, PIN and/or IIN values, such as ir35521, which > would already have this problem. > Yes. > > > > On the downside, with this change, the "vin" etc. label would be "vin1" > > for all chips, not just this one. That may break compatibility if there > > are users out there looking for specific labels. We may need a better > > and more flexible solution to address this problem. For example, the core > > could not only look for ".paged", but check if the respective attributes are > > present in more than one page, and if so override the "paged" flag. > > Something like the following (untested). > > I think that may be the best solution - though as I mentioned, there are > already some drivers whose behavior would be changed by this in that > they would end up with vin1 and vin2 instead of vin, etc. In those > cases, however, there might be no real alternative - it's not even clear > that the current behavior the user would get in that case would even be > consistent in terms of which parameter would actually be shown, > depending on how that is handled in libsensors. > For those drivers I would consider this to be a bug fix. An alternative would be to go with "vin", "vin2", and so on, but I don't really like that. Guenter
