On 5/23/21 10:52 AM, Erik Rosen wrote:
On Sat, May 22, 2021 at 3:41 PM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:On 5/22/21 3:55 AM, Erik Rosen wrote:Add support for reading and decoding direct format coefficients to the PMBus core driver. If the new flag PMBUS_USE_COEFFICIENTS_CMD is set, the driver will use the COEFFICIENTS register together with the information in the pmbus_sensor_attr structs to initialize relevant coefficients for the direct mode format. Signed-off-by: Erik Rosen <erik.rosen@xxxxxxxxxxxxx> --- drivers/hwmon/pmbus/pmbus_core.c | 93 ++++++++++++++++++++++++++++++++ include/linux/pmbus.h | 8 +++ 2 files changed, 101 insertions(+) diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c index 460cbfd716e4..03c169bf5633 100644 --- a/drivers/hwmon/pmbus/pmbus_core.c +++ b/drivers/hwmon/pmbus/pmbus_core.c @@ -2177,6 +2177,38 @@ static int pmbus_find_attributes(struct i2c_client *client, return ret; } +static int pmbus_init_coefficients(struct i2c_client *client, + struct pmbus_data *data, int page,This seems wrong. Coefficients are not maintained per page but per class, and (re-)reading them for each supported page doesn't really add value or even make sense.+ enum pmbus_sensor_classes sensor_class, + const struct pmbus_sensor_attr *attrs, + int nattrs) +{ + int i, status; + + for (i = 0; i < nattrs; i++) { + if (attrs->class == sensor_class && + (attrs->func & data->info->func[page])) { + status = pmbus_read_coefficients(client, + (struct pmbus_driver_info *)data->info, + sensor_class, + attrs->reg); + if (status < 0) { + dev_err(&client->dev, + "Failed to read coefficients for register: %x\n", + attrs->reg); + return status; + } + return 0; + } + attrs++; + } + + dev_err(&client->dev, "No coefficients found for register: %x\n", + attrs->reg); +attrs points beyond the array size here, so attrs->reg does not point to a valid array element. The problem would also not be the register this happens to point to, but the class (ie the chip does not support a sensor of the requested class). Not sure if this should trigger a message or error in the first place. It won't matter since the chip will never need those coefficients. If anything, this would be a misconfiguration (the driver should not set direct format for this sensor class), and the return value should be -EINVAL. Either case, I wonder if this can be handled with less complex code, ie without having to check data->info->func[] for all pages. How about just walking through attrs and try all class matches until one is found that works (ie not return on error but keep trying) ?Ok, I'll send a new version based on your comments. I'm not entirely comfortable with just silently ignoring any failure to retrieve the coefficients for a sensor class. I mean it could be due to any reason; a bus error for instance. I'll return a -EINVAL for now if you don't disagree.
Ok. After all, it does suggest a misconfiguration. Thanks, Guenter
/Erik+ return -ENODEV; +} + /* * Identify chip parameters. * This function is called for all chips. @@ -2185,6 +2217,7 @@ static int pmbus_identify_common(struct i2c_client *client, struct pmbus_data *data, int page) { int vout_mode = -1; + int ret; if (pmbus_check_byte_register(client, page, PMBUS_VOUT_MODE)) vout_mode = _pmbus_read_byte_data(client, page, @@ -2214,6 +2247,66 @@ static int pmbus_identify_common(struct i2c_client *client, } } + if (data->flags & PMBUS_USE_COEFFICIENTS_CMD) {I think there should be a separate function to handle that, to be called only once, not once per page.+ if (!i2c_check_functionality(client->adapter, + I2C_FUNC_SMBUS_BLOCK_PROC_CALL)) + return -ENODEV; + + if (data->info->format[PSC_VOLTAGE_IN] == direct) { + ret = pmbus_init_coefficients(client, data, page, + PSC_VOLTAGE_IN, + voltage_attributes, + ARRAY_SIZE(voltage_attributes)); + if (ret) + return ret; + }It might be useful to have a little structure with {class, attribute list pointer, attribute list size} and walk through that in a loop instead of repeating essentially the same code multiple times.+ + if (data->info->format[PSC_VOLTAGE_OUT] == direct) { + ret = pmbus_init_coefficients(client, data, page, + PSC_VOLTAGE_OUT, + voltage_attributes, + ARRAY_SIZE(voltage_attributes)); + if (ret) + return ret; + } + + if (data->info->format[PSC_CURRENT_IN] == direct) { + ret = pmbus_init_coefficients(client, data, page, + PSC_CURRENT_IN, + current_attributes, + ARRAY_SIZE(current_attributes)); + if (ret) + return ret; + } + + if (data->info->format[PSC_CURRENT_OUT] == direct) { + ret = pmbus_init_coefficients(client, data, page, + PSC_CURRENT_OUT, + current_attributes, + ARRAY_SIZE(current_attributes)); + if (ret) + return ret; + } + + if (data->info->format[PSC_POWER] == direct) { + ret = pmbus_init_coefficients(client, data, page, + PSC_POWER, + power_attributes, + ARRAY_SIZE(power_attributes)); + if (ret) + return ret; + } + + if (data->info->format[PSC_TEMPERATURE] == direct) { + ret = pmbus_init_coefficients(client, data, page, + PSC_TEMPERATURE, + temp_attributes, + ARRAY_SIZE(temp_attributes)); + if (ret) + return ret; + } + } + pmbus_clear_fault_page(client, page); return 0; } diff --git a/include/linux/pmbus.h b/include/linux/pmbus.h index f720470b1bab..7fdc282dab5a 100644 --- a/include/linux/pmbus.h +++ b/include/linux/pmbus.h @@ -52,6 +52,14 @@ */ #define PMBUS_NO_WRITE_PROTECT BIT(4) +/* + * PMBUS_USE_COEFFICIENTS_CMD + * + * When this flag is set the PMBus core driver will use the COEFFICIENTS + * register to initialize the coefficients for the direct mode format. + */ +#define PMBUS_USE_COEFFICIENTS_CMD BIT(5) + struct pmbus_platform_data { u32 flags; /* Device specific flags */
