On Wed, Jun 05, 2019 at 11:14:46AM -0600, Robert Hancock wrote:
> On 2019-06-05 10:48 a.m., Guenter Roeck wrote:
> > On Wed, Jun 05, 2019 at 10:17:12AM -0600, Robert Hancock wrote:
> >> Previously the VIN, IIN and PIN parameters were marked as non-paged,
> >> however on the IRPS5401 these parameters are present on multiple pages.
> >> Add the paged flag for these parameters so they can be detected properly
> >> on such chips.
> >>
> >
> > Have you tested the impact of this change on other chips where the
> > registers are non-paged ?
>
> No, I don't think I have any devices available that have another pmbus chip.
>
> > 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.
> 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.
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).
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
> >
> > Thanks,
> > Guenter
> >
> >> Signed-off-by: Robert Hancock <hancock@xxxxxxxxxxxxx>
> >> ---
> >> drivers/hwmon/pmbus/pmbus_core.c | 3 +++
> >> 1 file changed, 3 insertions(+)
> >>
> >> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> >> index ef7ee90..6e3aaf1 100644
> >> --- a/drivers/hwmon/pmbus/pmbus_core.c
> >> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> >> @@ -1395,6 +1395,7 @@ static int pmbus_add_sensor_attrs(struct i2c_client *client,
> >> .reg = PMBUS_READ_VIN,
> >> .class = PSC_VOLTAGE_IN,
> >> .label = "vin",
> >> + .paged = true,
> >> .func = PMBUS_HAVE_VIN,
> >> .sfunc = PMBUS_HAVE_STATUS_INPUT,
> >> .sbase = PB_STATUS_INPUT_BASE,
> >> @@ -1499,6 +1500,7 @@ static int pmbus_add_sensor_attrs(struct i2c_client *client,
> >> .reg = PMBUS_READ_IIN,
> >> .class = PSC_CURRENT_IN,
> >> .label = "iin",
> >> + .paged = true,
> >> .func = PMBUS_HAVE_IIN,
> >> .sfunc = PMBUS_HAVE_STATUS_INPUT,
> >> .sbase = PB_STATUS_INPUT_BASE,
> >> @@ -1584,6 +1586,7 @@ static int pmbus_add_sensor_attrs(struct i2c_client *client,
> >> .reg = PMBUS_READ_PIN,
> >> .class = PSC_POWER,
> >> .label = "pin",
> >> + .paged = true,
> >> .func = PMBUS_HAVE_PIN,
> >> .sfunc = PMBUS_HAVE_STATUS_INPUT,
> >> .sbase = PB_STATUS_INPUT_BASE,
> >> --
> >> 1.8.3.1
> >>
>
> --
> Robert Hancock
> Senior Software Developer
> SED Systems, a division of Calian Ltd.
> Email: hancock@xxxxxxxxxxxxx
