Re: [PATCH 3/3] hwmon: pmbus: Fix -EIO seen on pli1209

From: Guenter Roeck
Date: Sat Aug 19 2023 - 08:22:05 EST


On Thu, Aug 17, 2023 at 11:25:26AM +0200, Naresh Solanki wrote:
> From: Patrick Rudolph <patrick.rudolph@xxxxxxxxxxxxx>
>
> After doing performance optimizations the pli1209 driver failed to
> probe with a probabilty of 2%. It wasn't able to read the PMBUS_OPERATION
> register due to an -EIO error.
>
> An investigation showed that the PLI1209 takes 230 usec to execute the
> CLEAR_FAULTS command. During that time it's busy and NACKs all requests
> on the SMBUS interface. It also NACKs reads on PMBUS_STATUS_BYTE
> making it impossible to poll the BUSY flag.
>
> Add a custom write_data function to just wait for not BUSY unconditionally
> after sending a CLEAR_FAULTS command.
>
> TEST: Verified using an I2C bus analyser that no more NACKs are seen after
> sending a CLEAR_FAULTS command.
>
> Signed-off-by: Patrick Rudolph <patrick.rudolph@xxxxxxxxxxxxx>
> Signed-off-by: Naresh Solanki <Naresh.Solanki@xxxxxxxxxxxxx>

Applied.

Thanks,
Guenter

> ---
> drivers/hwmon/pmbus/pli1209bc.c | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/drivers/hwmon/pmbus/pli1209bc.c b/drivers/hwmon/pmbus/pli1209bc.c
> index 7d8bd3167b21..c95433790b11 100644
> --- a/drivers/hwmon/pmbus/pli1209bc.c
> +++ b/drivers/hwmon/pmbus/pli1209bc.c
> @@ -5,6 +5,7 @@
> * Copyright (c) 2022 9elements GmbH
> */
>
> +#include <linux/delay.h>
> #include <linux/i2c.h>
> #include <linux/module.h>
> #include <linux/pmbus.h>
> @@ -53,6 +54,30 @@ static int pli1209bc_read_word_data(struct i2c_client *client, int page,
> }
> }
>
> +static int pli1209bc_write_byte(struct i2c_client *client, int page, u8 reg)
> +{
> + int ret;
> +
> + switch (reg) {
> + case PMBUS_CLEAR_FAULTS:
> + ret = pmbus_write_byte(client, page, reg);
> + /*
> + * PLI1209 takes 230 usec to execute the CLEAR_FAULTS command.
> + * During that time it's busy and NACKs all requests on the
> + * SMBUS interface. It also NACKs reads on PMBUS_STATUS_BYTE
> + * making it impossible to poll the BUSY flag.
> + *
> + * Just wait for not BUSY unconditionally.
> + */
> + usleep_range(250, 300);
> + break;
> + default:
> + ret = -ENODATA;
> + break;
> + }
> + return ret;
> +}
> +
> #if IS_ENABLED(CONFIG_SENSORS_PLI1209BC_REGULATOR)
> static const struct regulator_desc pli1209bc_reg_desc = {
> .name = "vout2",
> @@ -102,6 +127,7 @@ static struct pmbus_driver_info pli1209bc_info = {
> | PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP
> | PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_STATUS_INPUT,
> .read_word_data = pli1209bc_read_word_data,
> + .write_byte = pli1209bc_write_byte,
> #if IS_ENABLED(CONFIG_SENSORS_PLI1209BC_REGULATOR)
> .num_regulators = 1,
> .reg_desc = &pli1209bc_reg_desc,