* [PATCH 0/1] hwmon: (pmbus) Try to match MFR_MODEL to pmbus device id @ 2022-02-16 11:55 Erik Rosen 2022-02-16 11:55 ` [PATCH 1/1] " Erik Rosen 0 siblings, 1 reply; 3+ messages in thread From: Erik Rosen @ 2022-02-16 11:55 UTC (permalink / raw) To: Jean Delvare, Guenter Roeck, linux-hwmon, linux-kernel, Chu Lin, Jason Ling Cc: 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH 1/1] hwmon: (pmbus) Try to match MFR_MODEL to pmbus device id 2022-02-16 11:55 [PATCH 0/1] hwmon: (pmbus) Try to match MFR_MODEL to pmbus device id Erik Rosen @ 2022-02-16 11:55 ` Erik Rosen 2022-02-16 23:17 ` Guenter Roeck 0 siblings, 1 reply; 3+ messages in thread From: Erik Rosen @ 2022-02-16 11:55 UTC (permalink / raw) To: Jean Delvare, Guenter Roeck, linux-hwmon, linux-kernel, Chu Lin, Jason Ling Cc: 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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 1/1] hwmon: (pmbus) Try to match MFR_MODEL to pmbus device id 2022-02-16 11:55 ` [PATCH 1/1] " Erik Rosen @ 2022-02-16 23:17 ` Guenter Roeck 0 siblings, 0 replies; 3+ messages in thread From: Guenter Roeck @ 2022-02-16 23:17 UTC (permalink / raw) To: Erik Rosen, Jean Delvare, linux-hwmon, linux-kernel, Chu Lin, Jason Ling 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}, ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-02-16 23:17 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-02-16 11:55 [PATCH 0/1] hwmon: (pmbus) Try to match MFR_MODEL to pmbus device id Erik Rosen 2022-02-16 11:55 ` [PATCH 1/1] " Erik Rosen 2022-02-16 23:17 ` Guenter Roeck
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).