| Message ID | 1559751434-19879-2-git-send-email-hancock@sedsystems.ca (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add pmbus support for Infineon IRPS5401 | expand |
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 ? 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. Thanks, Guenter > Signed-off-by: Robert Hancock <hancock@sedsystems.ca> > --- > 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 >
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? 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. > > Thanks, > Guenter > >> Signed-off-by: Robert Hancock <hancock@sedsystems.ca> >> --- >> 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 >>
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@sedsystems.ca> > >> --- > >> 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@sedsystems.ca
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
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
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,
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. Signed-off-by: Robert Hancock <hancock@sedsystems.ca> --- drivers/hwmon/pmbus/pmbus_core.c | 3 +++ 1 file changed, 3 insertions(+)