[PATCH 0/1] hwmon: (pmbus) Try to match MFR_MODEL to pmbus device id

[PATCH 0/1] hwmon: (pmbus) Try to match MFR_MODEL to pmbus device id

[Erik Rosen]

The UCD3138 chip from TI used in a number of Flex pmbus converters
(BMR310, BMR458, BMR480, BMR490 and BMR491) has a bug in the PMBus
interface hardware that causes it to not respond properly after
trying to read a register that does not exist
(see the 'UCD3138 Integrated Power Controller Family Errata').
This will mess up the pmbus driver's auto-detection process.

To remedy this situation the current version of the driver
supports a flag named PMBUS_READ_STATUS_AFTER_FAILED_CHECK.
If this flag is set the driver will read the STATUS register
after each failed register check with the assumption that this
will reset the hardware interface to a known state.

This strategy significantly improves the situation but does
not completely resolve the issue. In rare cases this seems
to be insufficient to reset the interface and the auto-detection
still fails.

In addition to this, to use this workaround one must explicitly
instruct the driver to expect for instance a BMR490 module.
Otherwise the driver will default to the generic behaviour.

We have a situation where the client wants to use the generic
pmbus driver to monitor the temperature of the converter,
but does not know a priori exactly what converter model is
actually installed; only that it supports PMBus.

To solve this problem, it is possible to use the MFR_MODEL
command to try and match the model name to the device id name
and predefine the functions supported by this specific converter.
In this way one can avoid the auto-detection process
altogether for the problematic models. If there is no match,
the driver reverts to auto-detection.

Erik Rosen (1):
  Try to match MFR_MODEL to pmbus device id

 drivers/hwmon/pmbus/pmbus.c | 57 +++++++++++++++++++++++++++++++------
 1 file changed, 49 insertions(+), 8 deletions(-)


base-commit: df0cc57e057f18e44dac8e6c18aba47ab53202f9
-- 
2.20.1

[PATCH 1/1] hwmon: (pmbus) Try to match MFR_MODEL to pmbus device id

[Erik Rosen]

Add a new device id to  read the MFR_MODEL command
to try and match the model name to the device id name and
predefine the functions supported by this specific converter.
In this way one can avoid the auto-detection process
altogether for the problematic models.
If there is no match, the driver reverts to auto-detection.

Signed-off-by: Erik Rosen <erik.rosen@metormote.com>
---
 drivers/hwmon/pmbus/pmbus.c | 57 +++++++++++++++++++++++++++++++------
 1 file changed, 49 insertions(+), 8 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus.c b/drivers/hwmon/pmbus/pmbus.c
index d0d386990af5..278b2a927ce0 100644
--- a/drivers/hwmon/pmbus/pmbus.c
+++ b/drivers/hwmon/pmbus/pmbus.c
@@ -18,9 +18,11 @@
 struct pmbus_device_info {
 	int pages;
 	u32 flags;
+	u32 func[PMBUS_PAGES];
 };
 
 static const struct i2c_device_id pmbus_id[];
+static const struct pmbus_device_info pmbus_info_zero;
 
 /*
  * Find sensor groups and status registers on each page.
@@ -156,13 +158,18 @@ static int pmbus_identify(struct i2c_client *client,
 	}
 
 	/* Try to find sensor groups  */
-	pmbus_find_sensor_groups(client, info);
+	if (info->func[0] == 0)
+		pmbus_find_sensor_groups(client, info);
+
 abort:
 	return ret;
 }
 
 static int pmbus_probe(struct i2c_client *client)
 {
+	int ret, i;
+	u8 mfr_model[I2C_SMBUS_BLOCK_MAX + 1];
+	const struct i2c_device_id *device_id = NULL;
 	struct pmbus_driver_info *info;
 	struct pmbus_platform_data *pdata = NULL;
 	struct device *dev = &client->dev;
@@ -173,6 +180,30 @@ static int pmbus_probe(struct i2c_client *client)
 		return -ENOMEM;
 
 	device_info = (struct pmbus_device_info *)i2c_match_id(pmbus_id, client)->driver_data;
+	if (!device_info) {
+		if (!i2c_check_functionality(client->adapter,
+					     I2C_FUNC_SMBUS_BLOCK_DATA))
+			return -ENODEV;
+
+		ret = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, mfr_model);
+		if (ret < 0)
+			return ret;
+
+		for (device_id = pmbus_id; device_id->name[0]; device_id++) {
+			if (device_id->driver_data &&
+			    !strncasecmp(device_id->name, mfr_model, strlen(device_id->name)))
+				break;
+		}
+
+		if (device_id->name[0])
+			device_info = (struct pmbus_device_info *)device_id->driver_data;
+		else
+			device_info = (struct pmbus_device_info *)&pmbus_info_zero;
+
+		dev_info(dev, "Use pmbus device id: %s\n",
+			 device_id->name[0] ? device_id->name : "pmbus");
+	}
+
 	if (device_info->flags) {
 		pdata = devm_kzalloc(dev, sizeof(struct pmbus_platform_data),
 				     GFP_KERNEL);
@@ -183,6 +214,8 @@ static int pmbus_probe(struct i2c_client *client)
 	}
 
 	info->pages = device_info->pages;
+	for (i = 0; i < info->pages; i++)
+		info->func[i] = device_info->func[i];
 	info->identify = pmbus_identify;
 	dev->platform_data = pdata;
 
@@ -204,9 +237,16 @@ static const struct pmbus_device_info pmbus_info_one_skip = {
 	.flags = PMBUS_SKIP_STATUS_CHECK
 };
 
-static const struct pmbus_device_info pmbus_info_one_status = {
+static const struct pmbus_device_info pmbus_info_bmr458 = {
 	.pages = 1,
-	.flags = PMBUS_READ_STATUS_AFTER_FAILED_CHECK
+	.flags = PMBUS_READ_STATUS_AFTER_FAILED_CHECK,
+	.func = {
+			PMBUS_HAVE_VIN | PMBUS_HAVE_STATUS_INPUT
+		      | PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT
+		      | PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT
+		      | PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2
+		      | PMBUS_HAVE_STATUS_TEMP
+		}
 };
 
 /*
@@ -214,15 +254,15 @@ static const struct pmbus_device_info pmbus_info_one_status = {
  */
 static const struct i2c_device_id pmbus_id[] = {
 	{"adp4000", (kernel_ulong_t)&pmbus_info_one},
-	{"bmr310", (kernel_ulong_t)&pmbus_info_one_status},
+	{"bmr310", (kernel_ulong_t)&pmbus_info_bmr458},
 	{"bmr453", (kernel_ulong_t)&pmbus_info_one},
 	{"bmr454", (kernel_ulong_t)&pmbus_info_one},
 	{"bmr456", (kernel_ulong_t)&pmbus_info_one},
 	{"bmr457", (kernel_ulong_t)&pmbus_info_one},
-	{"bmr458", (kernel_ulong_t)&pmbus_info_one_status},
-	{"bmr480", (kernel_ulong_t)&pmbus_info_one_status},
-	{"bmr490", (kernel_ulong_t)&pmbus_info_one_status},
-	{"bmr491", (kernel_ulong_t)&pmbus_info_one_status},
+	{"bmr458", (kernel_ulong_t)&pmbus_info_bmr458},
+	{"bmr480", (kernel_ulong_t)&pmbus_info_bmr458},
+	{"bmr490", (kernel_ulong_t)&pmbus_info_bmr458},
+	{"bmr491", (kernel_ulong_t)&pmbus_info_bmr458},
 	{"bmr492", (kernel_ulong_t)&pmbus_info_one},
 	{"dps460", (kernel_ulong_t)&pmbus_info_one_skip},
 	{"dps650ab", (kernel_ulong_t)&pmbus_info_one_skip},
@@ -235,6 +275,7 @@ static const struct i2c_device_id pmbus_id[] = {
 	{"pdt006", (kernel_ulong_t)&pmbus_info_one},
 	{"pdt012", (kernel_ulong_t)&pmbus_info_one},
 	{"pmbus", (kernel_ulong_t)&pmbus_info_zero},
+	{"pmbus_match_model", (kernel_ulong_t)0},
 	{"sgd009", (kernel_ulong_t)&pmbus_info_one_skip},
 	{"tps40400", (kernel_ulong_t)&pmbus_info_one},
 	{"tps544b20", (kernel_ulong_t)&pmbus_info_one},
-- 
2.20.1

Re: [PATCH 1/1] hwmon: (pmbus) Try to match MFR_MODEL to pmbus device id

[Guenter Roeck]

On 2/16/22 03:55, Erik Rosen wrote:
> Add a new device id to  read the MFR_MODEL command
> to try and match the model name to the device id name and
> predefine the functions supported by this specific converter.
> In this way one can avoid the auto-detection process
> altogether for the problematic models.
> If there is no match, the driver reverts to auto-detection.
> 
> Signed-off-by: Erik Rosen <erik.rosen@metormote.com>

That looks really messy, and it does more than it claims to do.
Really, if the bcm chips need special treatment they should not
use the generic driver but use their own front-end driver like
other chips. The required flags can then be provided in that driver.
Also "pmbus_match_model" is _really_ not acceptable. Those names
are expected to match chips, not some arbitrary string.

If we have a separate driver, and assuming the customer using
different modules uses various BMR chips, a separate driver could
check PMBUS_MFR_MODEL and use it to match the exact chip (if that is
even needed since all the BMR chips seem to have the same
configuration data). Maybe PMBUS_READ_STATUS_AFTER_FAILED_CHECK
could then be dropped entirely.

Guenter

> ---
>   drivers/hwmon/pmbus/pmbus.c | 57 +++++++++++++++++++++++++++++++------
>   1 file changed, 49 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus.c b/drivers/hwmon/pmbus/pmbus.c
> index d0d386990af5..278b2a927ce0 100644
> --- a/drivers/hwmon/pmbus/pmbus.c
> +++ b/drivers/hwmon/pmbus/pmbus.c
> @@ -18,9 +18,11 @@
>   struct pmbus_device_info {
>   	int pages;
>   	u32 flags;
> +	u32 func[PMBUS_PAGES];
>   };
>   
>   static const struct i2c_device_id pmbus_id[];
> +static const struct pmbus_device_info pmbus_info_zero;
>   
>   /*
>    * Find sensor groups and status registers on each page.
> @@ -156,13 +158,18 @@ static int pmbus_identify(struct i2c_client *client,
>   	}
>   
>   	/* Try to find sensor groups  */
> -	pmbus_find_sensor_groups(client, info);
> +	if (info->func[0] == 0)
> +		pmbus_find_sensor_groups(client, info);
> +
>   abort:
>   	return ret;
>   }
>   
>   static int pmbus_probe(struct i2c_client *client)
>   {
> +	int ret, i;
> +	u8 mfr_model[I2C_SMBUS_BLOCK_MAX + 1];
> +	const struct i2c_device_id *device_id = NULL;
>   	struct pmbus_driver_info *info;
>   	struct pmbus_platform_data *pdata = NULL;
>   	struct device *dev = &client->dev;
> @@ -173,6 +180,30 @@ static int pmbus_probe(struct i2c_client *client)
>   		return -ENOMEM;
>   
>   	device_info = (struct pmbus_device_info *)i2c_match_id(pmbus_id, client)->driver_data;
> +	if (!device_info) {
> +		if (!i2c_check_functionality(client->adapter,
> +					     I2C_FUNC_SMBUS_BLOCK_DATA))
> +			return -ENODEV;
> +
> +		ret = i2c_smbus_read_block_data(client, PMBUS_MFR_MODEL, mfr_model);
> +		if (ret < 0)
> +			return ret;
> +
> +		for (device_id = pmbus_id; device_id->name[0]; device_id++) {
> +			if (device_id->driver_data &&
> +			    !strncasecmp(device_id->name, mfr_model, strlen(device_id->name)))
> +				break;
> +		}
> +
> +		if (device_id->name[0])
> +			device_info = (struct pmbus_device_info *)device_id->driver_data;
> +		else
> +			device_info = (struct pmbus_device_info *)&pmbus_info_zero;
> +
> +		dev_info(dev, "Use pmbus device id: %s\n",
> +			 device_id->name[0] ? device_id->name : "pmbus");
> +	}
> +
>   	if (device_info->flags) {
>   		pdata = devm_kzalloc(dev, sizeof(struct pmbus_platform_data),
>   				     GFP_KERNEL);
> @@ -183,6 +214,8 @@ static int pmbus_probe(struct i2c_client *client)
>   	}
>   
>   	info->pages = device_info->pages;
> +	for (i = 0; i < info->pages; i++)
> +		info->func[i] = device_info->func[i];
>   	info->identify = pmbus_identify;
>   	dev->platform_data = pdata;
>   
> @@ -204,9 +237,16 @@ static const struct pmbus_device_info pmbus_info_one_skip = {
>   	.flags = PMBUS_SKIP_STATUS_CHECK
>   };
>   
> -static const struct pmbus_device_info pmbus_info_one_status = {
> +static const struct pmbus_device_info pmbus_info_bmr458 = {
>   	.pages = 1,
> -	.flags = PMBUS_READ_STATUS_AFTER_FAILED_CHECK
> +	.flags = PMBUS_READ_STATUS_AFTER_FAILED_CHECK,
> +	.func = {
> +			PMBUS_HAVE_VIN | PMBUS_HAVE_STATUS_INPUT
> +		      | PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT
> +		      | PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT
> +		      | PMBUS_HAVE_TEMP | PMBUS_HAVE_TEMP2
> +		      | PMBUS_HAVE_STATUS_TEMP
> +		}
>   };
>   
>   /*
> @@ -214,15 +254,15 @@ static const struct pmbus_device_info pmbus_info_one_status = {
>    */
>   static const struct i2c_device_id pmbus_id[] = {
>   	{"adp4000", (kernel_ulong_t)&pmbus_info_one},
> -	{"bmr310", (kernel_ulong_t)&pmbus_info_one_status},
> +	{"bmr310", (kernel_ulong_t)&pmbus_info_bmr458},
>   	{"bmr453", (kernel_ulong_t)&pmbus_info_one},
>   	{"bmr454", (kernel_ulong_t)&pmbus_info_one},
>   	{"bmr456", (kernel_ulong_t)&pmbus_info_one},
>   	{"bmr457", (kernel_ulong_t)&pmbus_info_one},
> -	{"bmr458", (kernel_ulong_t)&pmbus_info_one_status},
> -	{"bmr480", (kernel_ulong_t)&pmbus_info_one_status},
> -	{"bmr490", (kernel_ulong_t)&pmbus_info_one_status},
> -	{"bmr491", (kernel_ulong_t)&pmbus_info_one_status},
> +	{"bmr458", (kernel_ulong_t)&pmbus_info_bmr458},
> +	{"bmr480", (kernel_ulong_t)&pmbus_info_bmr458},
> +	{"bmr490", (kernel_ulong_t)&pmbus_info_bmr458},
> +	{"bmr491", (kernel_ulong_t)&pmbus_info_bmr458},
>   	{"bmr492", (kernel_ulong_t)&pmbus_info_one},
>   	{"dps460", (kernel_ulong_t)&pmbus_info_one_skip},
>   	{"dps650ab", (kernel_ulong_t)&pmbus_info_one_skip},
> @@ -235,6 +275,7 @@ static const struct i2c_device_id pmbus_id[] = {
>   	{"pdt006", (kernel_ulong_t)&pmbus_info_one},
>   	{"pdt012", (kernel_ulong_t)&pmbus_info_one},
>   	{"pmbus", (kernel_ulong_t)&pmbus_info_zero},
> +	{"pmbus_match_model", (kernel_ulong_t)0},
>   	{"sgd009", (kernel_ulong_t)&pmbus_info_one_skip},
>   	{"tps40400", (kernel_ulong_t)&pmbus_info_one},
>   	{"tps544b20", (kernel_ulong_t)&pmbus_info_one},