[PATCH 0/3] Add pmbus support for Infineon IRPS5401

[PATCH 0/3] Add pmbus support for Infineon IRPS5401

[Robert Hancock]

This patch set adds support for the Infineon IRPS5401 PMIC to pmbus.
This chip has 5 outputs and includes separate VIN, IIN and PIN parameters
on multiple pages, which is something that the pmbus core did not
previously support.

Robert Hancock (3):
  hwmon: (pmbus) Add paged support for VIN, IIN, PIN parameters
  hwmon: (pmbus) Add paged VIN, IIN, PIN, temp detection support
  hwmon: (pmbus) add support for Infineon IRPS5401

 drivers/hwmon/pmbus/pmbus.c      | 51 +++++++++++++++++++++++-----------------
 drivers/hwmon/pmbus/pmbus_core.c |  3 +++
 2 files changed, 33 insertions(+), 21 deletions(-)

-- 
1.8.3.1

[PATCH 1/3] hwmon: (pmbus) Add paged support for VIN, IIN, PIN parameters

[Robert Hancock]

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(+)

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

[PATCH 2/3] hwmon: (pmbus) Add paged VIN, IIN, PIN, temp detection support

[Robert Hancock]

Add support for detecting multi-paged VIN, IIN, PIN and temperature
parameters to the generic detection code in pmbus_find_sensor_groups.
Paged VIN, IIN and PIN parameters were just added to the pmbus core,
and temperature parameters could already be paged but were not
auto-detected as such.

Signed-off-by: Robert Hancock <hancock@sedsystems.ca>
---
 drivers/hwmon/pmbus/pmbus.c | 46 ++++++++++++++++++++++++---------------------
 1 file changed, 25 insertions(+), 21 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus.c b/drivers/hwmon/pmbus/pmbus.c
index c0bc43d..970322f 100644
--- a/drivers/hwmon/pmbus/pmbus.c
+++ b/drivers/hwmon/pmbus/pmbus.c
@@ -29,17 +29,8 @@ static void pmbus_find_sensor_groups(struct i2c_client *client,
 	int page;
 
 	/* Sensors detected on page 0 only */
-	if (pmbus_check_word_register(client, 0, PMBUS_READ_VIN))
-		info->func[0] |= PMBUS_HAVE_VIN;
 	if (pmbus_check_word_register(client, 0, PMBUS_READ_VCAP))
 		info->func[0] |= PMBUS_HAVE_VCAP;
-	if (pmbus_check_word_register(client, 0, PMBUS_READ_IIN))
-		info->func[0] |= PMBUS_HAVE_IIN;
-	if (pmbus_check_word_register(client, 0, PMBUS_READ_PIN))
-		info->func[0] |= PMBUS_HAVE_PIN;
-	if (info->func[0]
-	    && pmbus_check_byte_register(client, 0, PMBUS_STATUS_INPUT))
-		info->func[0] |= PMBUS_HAVE_STATUS_INPUT;
 	if (pmbus_check_byte_register(client, 0, PMBUS_FAN_CONFIG_12) &&
 	    pmbus_check_word_register(client, 0, PMBUS_READ_FAN_SPEED_1)) {
 		info->func[0] |= PMBUS_HAVE_FAN12;
@@ -52,20 +43,19 @@ static void pmbus_find_sensor_groups(struct i2c_client *client,
 		if (pmbus_check_byte_register(client, 0, PMBUS_STATUS_FAN_34))
 			info->func[0] |= PMBUS_HAVE_STATUS_FAN34;
 	}
-	if (pmbus_check_word_register(client, 0, PMBUS_READ_TEMPERATURE_1))
-		info->func[0] |= PMBUS_HAVE_TEMP;
-	if (pmbus_check_word_register(client, 0, PMBUS_READ_TEMPERATURE_2))
-		info->func[0] |= PMBUS_HAVE_TEMP2;
-	if (pmbus_check_word_register(client, 0, PMBUS_READ_TEMPERATURE_3))
-		info->func[0] |= PMBUS_HAVE_TEMP3;
-	if (info->func[0] & (PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2
-			     | PMBUS_HAVE_TEMP3)
-	    && pmbus_check_byte_register(client, 0,
-					 PMBUS_STATUS_TEMPERATURE))
-			info->func[0] |= PMBUS_HAVE_STATUS_TEMP;
 
 	/* Sensors detected on all pages */
 	for (page = 0; page < info->pages; page++) {
+		if (pmbus_check_word_register(client, page, PMBUS_READ_VIN))
+			info->func[page] |= PMBUS_HAVE_VIN;
+		if (pmbus_check_word_register(client, page, PMBUS_READ_IIN))
+			info->func[page] |= PMBUS_HAVE_IIN;
+		if (pmbus_check_word_register(client, page, PMBUS_READ_PIN))
+			info->func[page] |= PMBUS_HAVE_PIN;
+		if (info->func[page] & (PMBUS_HAVE_VIN | PMBUS_HAVE_IIN
+				      | PMBUS_HAVE_PIN) &&
+		    pmbus_check_byte_register(client, page, PMBUS_STATUS_INPUT))
+			info->func[page] |= PMBUS_HAVE_STATUS_INPUT;
 		if (pmbus_check_word_register(client, page, PMBUS_READ_VOUT)) {
 			info->func[page] |= PMBUS_HAVE_VOUT;
 			if (pmbus_check_byte_register(client, page,
@@ -74,12 +64,26 @@ static void pmbus_find_sensor_groups(struct i2c_client *client,
 		}
 		if (pmbus_check_word_register(client, page, PMBUS_READ_IOUT)) {
 			info->func[page] |= PMBUS_HAVE_IOUT;
-			if (pmbus_check_byte_register(client, 0,
+			if (pmbus_check_byte_register(client, page,
 						      PMBUS_STATUS_IOUT))
 				info->func[page] |= PMBUS_HAVE_STATUS_IOUT;
 		}
 		if (pmbus_check_word_register(client, page, PMBUS_READ_POUT))
 			info->func[page] |= PMBUS_HAVE_POUT;
+		if (pmbus_check_word_register(client, page,
+					      PMBUS_READ_TEMPERATURE_1))
+			info->func[page] |= PMBUS_HAVE_TEMP;
+		if (pmbus_check_word_register(client, page,
+					      PMBUS_READ_TEMPERATURE_2))
+			info->func[page] |= PMBUS_HAVE_TEMP2;
+		if (pmbus_check_word_register(client, page,
+					      PMBUS_READ_TEMPERATURE_3))
+			info->func[page] |= PMBUS_HAVE_TEMP3;
+		if (info->func[page] & (PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2
+				     | PMBUS_HAVE_TEMP3) &&
+		    pmbus_check_byte_register(client, page,
+					      PMBUS_STATUS_TEMPERATURE))
+			info->func[page] |= PMBUS_HAVE_STATUS_TEMP;
 	}
 }
 
-- 
1.8.3.1

[PATCH 3/3] hwmon: (pmbus) add support for Infineon IRPS5401

[Robert Hancock]

Add detection support for the Infineon IRPS5401 PMIC. This chip has 5
pages corresponding to 4 switching outputs and one linear (LDO) output.

Signed-off-by: Robert Hancock <hancock@sedsystems.ca>
---
 drivers/hwmon/pmbus/pmbus.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/hwmon/pmbus/pmbus.c b/drivers/hwmon/pmbus/pmbus.c
index 970322f..158d61e 100644
--- a/drivers/hwmon/pmbus/pmbus.c
+++ b/drivers/hwmon/pmbus/pmbus.c
@@ -191,6 +191,10 @@ static int pmbus_probe(struct i2c_client *client,
 	return pmbus_do_probe(client, id, info);
 }
 
+static const struct pmbus_device_info pmbus_info_five = {
+	.pages = 5,
+	.flags = 0
+};
 static const struct pmbus_device_info pmbus_info_one = {
 	.pages = 1,
 	.flags = 0
@@ -214,6 +218,7 @@ static int pmbus_probe(struct i2c_client *client,
 	{"dps460", (kernel_ulong_t)&pmbus_info_one_skip},
 	{"dps650ab", (kernel_ulong_t)&pmbus_info_one_skip},
 	{"dps800", (kernel_ulong_t)&pmbus_info_one_skip},
+	{"irps5401", (kernel_ulong_t)&pmbus_info_five},
 	{"mdt040", (kernel_ulong_t)&pmbus_info_one},
 	{"ncp4200", (kernel_ulong_t)&pmbus_info_one},
 	{"ncp4208", (kernel_ulong_t)&pmbus_info_one},
-- 
1.8.3.1

Re: [PATCH 1/3] hwmon: (pmbus) Add paged support for VIN, IIN, PIN parameters

[Guenter Roeck]

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
> 

Re: [PATCH 1/3] hwmon: (pmbus) Add paged support for VIN, IIN, PIN parameters

[Robert Hancock]

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
>>

-- 
Robert Hancock
Senior Software Developer
SED Systems, a division of Calian Ltd.
Email: hancock@sedsystems.ca

Re: [PATCH 1/3] hwmon: (pmbus) Add paged support for VIN, IIN, PIN parameters

[Guenter Roeck]

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

Re: [PATCH 1/3] hwmon: (pmbus) Add paged support for VIN, IIN, PIN parameters

[Robert Hancock]

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@sedsystems.ca

Re: [PATCH 1/3] hwmon: (pmbus) Add paged support for VIN, IIN, PIN parameters

[Guenter Roeck]

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