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.
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.
>
> 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.
>
> static bool pmbus_sensor_is_paged(const struct pmbus_driver_info *info,
> const struct pmbus_sensor_attr *attr)
> {
> int p;
>
> if (attr->paged)
> return true;
>
> for (p = 1; p < info->pages; p++) {
> if (info->func[p] & attr->func)
> return true;
> }
> return false;
> }
>
> ...
>
> static int pmbus_add_sensor_attrs(...)
> {
> ...
>
> bool paged = pmbus_sensor_is_paged(info, attrs);
>
> pages = paged ? info->pages : 1;
> ...
> ret = pmbus_add_sensor_attrs_one(client, data, info, name, index, page, attrs, paged)
> ^^^
> new parameter
> }
>
> Guenter
--
Robert Hancock
Senior Software Developer
SED Systems, a division of Calian Ltd.
Email: hancock@xxxxxxxxxxxxx
