[PATCH v3] hwmon: (pmbus/mp2975) Fix driver initialization for MP2975 device

[PATCH v3] hwmon: (pmbus/mp2975) Fix driver initialization for MP2975 device

[Konstantin Aladyshev]

The commit 1feb31e810b0 ("hwmon: (pmbus/mp2975) Simplify VOUT code")
has introduced a bug that makes it impossible to initialize MP2975
device:
"""
mp2975 5-0020: Failed to identify chip capabilities
i2c i2c-5: new_device: Instantiated device mp2975 at 0x20
i2c i2c-5: delete_device: Deleting device mp2975 at 0x20
"""
Since the 'read_byte_data' function was removed from the
'pmbus_driver_info ' structure the driver no longer reports correctly
that VOUT mode is direct. Therefore 'pmbus_identify_common' fails
with error, making it impossible to initialize the device.

Restore 'read_byte_data' function to fix the issue.

Tested:
- before: it is not possible to initialize MP2975 device with the
'mp2975' driver,
- after: 'mp2975' correctly initializes MP2975 device and all sensor
data is correct.

Fixes: 1feb31e810b0 ("hwmon: (pmbus/mp2975) Simplify VOUT code")

Signed-off-by: Konstantin Aladyshev <aladyshev22@gmail.com>
---
Changes in v3:
 - Drop accidentally added file from patch

Changes in v2:
 - Fix indentation issues

 drivers/hwmon/pmbus/mp2975.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/hwmon/pmbus/mp2975.c b/drivers/hwmon/pmbus/mp2975.c
index b9bb469e2d8f..5bbfdacb61a7 100644
--- a/drivers/hwmon/pmbus/mp2975.c
+++ b/drivers/hwmon/pmbus/mp2975.c
@@ -126,6 +126,22 @@ static const struct regulator_desc __maybe_unused mp2975_reg_desc[] = {
 
 #define to_mp2975_data(x)  container_of(x, struct mp2975_data, info)
 
+static int mp2975_read_byte_data(struct i2c_client *client, int page, int reg)
+{
+	switch (reg) {
+	case PMBUS_VOUT_MODE:
+		/*
+		 * Enforce VOUT direct format, since device allows to set the
+		 * different formats for the different rails. Conversion from
+		 * VID to direct provided by driver internally, in case it is
+		 * necessary.
+		 */
+		return PB_VOUT_MODE_DIRECT;
+	default:
+		return -ENODATA;
+	}
+}
+
 static int
 mp2975_read_word_helper(struct i2c_client *client, int page, int phase, u8 reg,
 			u16 mask)
@@ -869,6 +885,7 @@ static struct pmbus_driver_info mp2975_info = {
 		PMBUS_HAVE_IIN | PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT |
 		PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP | PMBUS_HAVE_POUT |
 		PMBUS_HAVE_PIN | PMBUS_HAVE_STATUS_INPUT | PMBUS_PHASE_VIRTUAL,
+	.read_byte_data = mp2975_read_byte_data,
 	.read_word_data = mp2975_read_word_data,
 #if IS_ENABLED(CONFIG_SENSORS_MP2975_REGULATOR)
 	.num_regulators = 1,
-- 
2.34.1

Re: [PATCH v3] hwmon: (pmbus/mp2975) Fix driver initialization for MP2975 device

[Guenter Roeck]

On Fri, Jan 26, 2024 at 11:57:14PM +0300, Konstantin Aladyshev wrote:
> The commit 1feb31e810b0 ("hwmon: (pmbus/mp2975) Simplify VOUT code")
> has introduced a bug that makes it impossible to initialize MP2975
> device:
> """
> mp2975 5-0020: Failed to identify chip capabilities
> i2c i2c-5: new_device: Instantiated device mp2975 at 0x20
> i2c i2c-5: delete_device: Deleting device mp2975 at 0x20
> """
> Since the 'read_byte_data' function was removed from the
> 'pmbus_driver_info ' structure the driver no longer reports correctly
> that VOUT mode is direct. Therefore 'pmbus_identify_common' fails
> with error, making it impossible to initialize the device.
> 
> Restore 'read_byte_data' function to fix the issue.
> 
> Tested:
> - before: it is not possible to initialize MP2975 device with the
> 'mp2975' driver,
> - after: 'mp2975' correctly initializes MP2975 device and all sensor
> data is correct.
> 
> Fixes: 1feb31e810b0 ("hwmon: (pmbus/mp2975) Simplify VOUT code")
> 
> Signed-off-by: Konstantin Aladyshev <aladyshev22@gmail.com>

Applied.

Thanks,
Guenter

Re: [PATCH v3] hwmon: (pmbus/mp2975) Fix driver initialization for MP2975 device

[Patrick Rudolph]

Hi Konstantin,
Thank you for fixing this regression.

The comment is no longer true as the driver doesn't internally convert
from VID to direct,
but rather configures READ_VOUT using MFR_DC_LOOP_CTRL.

The comment thus should read as the following:

Report direct format as configured by MFR_DC_LOOP_CTRL.
Unlike on MP2971/MP2973 the reported VOUT_MODE isn't automatically
internally updated,
but always reads as PB_VOUT_MODE_VID.

Regards,
Patrick
On Fri, Jan 26, 2024 at 9:59 PM Konstantin Aladyshev
<aladyshev22@gmail.com> wrote:
>
> The commit 1feb31e810b0 ("hwmon: (pmbus/mp2975) Simplify VOUT code")
> has introduced a bug that makes it impossible to initialize MP2975
> device:
> """
> mp2975 5-0020: Failed to identify chip capabilities
> i2c i2c-5: new_device: Instantiated device mp2975 at 0x20
> i2c i2c-5: delete_device: Deleting device mp2975 at 0x20
> """
> Since the 'read_byte_data' function was removed from the
> 'pmbus_driver_info ' structure the driver no longer reports correctly
> that VOUT mode is direct. Therefore 'pmbus_identify_common' fails
> with error, making it impossible to initialize the device.
>
> Restore 'read_byte_data' function to fix the issue.
>
> Tested:
> - before: it is not possible to initialize MP2975 device with the
> 'mp2975' driver,
> - after: 'mp2975' correctly initializes MP2975 device and all sensor
> data is correct.
>
> Fixes: 1feb31e810b0 ("hwmon: (pmbus/mp2975) Simplify VOUT code")
>
> Signed-off-by: Konstantin Aladyshev <aladyshev22@gmail.com>
> ---
> Changes in v3:
>  - Drop accidentally added file from patch
>
> Changes in v2:
>  - Fix indentation issues
>
>  drivers/hwmon/pmbus/mp2975.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/drivers/hwmon/pmbus/mp2975.c b/drivers/hwmon/pmbus/mp2975.c
> index b9bb469e2d8f..5bbfdacb61a7 100644
> --- a/drivers/hwmon/pmbus/mp2975.c
> +++ b/drivers/hwmon/pmbus/mp2975.c
> @@ -126,6 +126,22 @@ static const struct regulator_desc __maybe_unused mp2975_reg_desc[] = {
>
>  #define to_mp2975_data(x)  container_of(x, struct mp2975_data, info)
>
> +static int mp2975_read_byte_data(struct i2c_client *client, int page, int reg)
> +{
> +       switch (reg) {
> +       case PMBUS_VOUT_MODE:
> +               /*
> +                * Enforce VOUT direct format, since device allows to set the
> +                * different formats for the different rails. Conversion from
> +                * VID to direct provided by driver internally, in case it is
> +                * necessary.
> +                */
> +               return PB_VOUT_MODE_DIRECT;
> +       default:
> +               return -ENODATA;
> +       }
> +}
> +
>  static int
>  mp2975_read_word_helper(struct i2c_client *client, int page, int phase, u8 reg,
>                         u16 mask)
> @@ -869,6 +885,7 @@ static struct pmbus_driver_info mp2975_info = {
>                 PMBUS_HAVE_IIN | PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT |
>                 PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP | PMBUS_HAVE_POUT |
>                 PMBUS_HAVE_PIN | PMBUS_HAVE_STATUS_INPUT | PMBUS_PHASE_VIRTUAL,
> +       .read_byte_data = mp2975_read_byte_data,
>         .read_word_data = mp2975_read_word_data,
>  #if IS_ENABLED(CONFIG_SENSORS_MP2975_REGULATOR)
>         .num_regulators = 1,
> --
> 2.34.1
>

Re: [PATCH v3] hwmon: (pmbus/mp2975) Fix driver initialization for MP2975 device

[Guenter Roeck]

On 1/27/24 00:56, Patrick Rudolph wrote:
> Hi Konstantin,
> Thank you for fixing this regression.
> 
> The comment is no longer true as the driver doesn't internally convert
> from VID to direct,
> but rather configures READ_VOUT using MFR_DC_LOOP_CTRL.
> 
> The comment thus should read as the following:
> 
> Report direct format as configured by MFR_DC_LOOP_CTRL.
> Unlike on MP2971/MP2973 the reported VOUT_MODE isn't automatically
> internally updated,
> but always reads as PB_VOUT_MODE_VID.
> 
> Regards,
> Patrick

Please don't top-post.

This patch has already been applied. Please send a follow-up to that patch
to update it.

Thanks,
Guenter