Re: [PATCH v2 3/3] hwmon: pmbus: mpq8785: force direct mode for VID VOUT on MPQ8785/MPQ8786

[Guenter Roeck <linux@roeck-us.net>]

On 2/8/26 22:28, Carl Lee wrote:
> On Thu, Feb 05, 2026 at 08:58:37AM -0800, Guenter Roeck wrote:
>> On Thu, Feb 05, 2026 at 06:01:39PM +0800, Carl Lee via B4 Relay wrote:
>>> From: Carl Lee <carl.lee@amd.com>
>>>
>>> According to MPQ8785/MPQ8786 datasheet, VID mode configuration is
>>> the same as direct mode configuration. Therefore, when VOUT is
>>> reported in VID mode, it must be forced to use direct format.
>>>
>>
>> Why "must" ? Yes, the LSB is the same, at least for MPQ8785,
>> but that doesn't mean that the mode _must_ be overwritten. Maybe
>> I am missing it, but as far as I can see the datasheet doesn't
>> say that the VID mode configuration is the same as direct mode
>> configuration. It says that the _LSB_ is the same for both modes.
>>
>> I _think_ the problem may be that the output voltages are not really
>> reported as VID values but as raw voltages, but the datasheet is a bit
>> vague in that regard. It talks about LSB values but doesn't exactly
>> say how voltages are reported, and for READ_VIN it is most definitely
>> wrong ("This bit is in VID mode with 25mv/LSB" doesn't make any sense).
>>
>> Thanks,
>> Guenter
> 
> Thanks for your feedback. I see your point about “must.”
> The datasheet only says the LSB is the same for VID and direct modes;
> it doesn’t state that VID mode configuration is identical to direct mode.
> 
> Based on current hardware testing where the chip reports VOUT Mode as VID,
> Observations on actual hardware:
> 
> 1.Without forcing the mode: driver fails to initialize.
> dmesg | grep -i mpq8785
> mpq8785 58-0046: Failed to identify chip capabilities
> 

Wrong conclusion. That message means that the chip reports to be in VID mode,
while the configuration data disagrees.

That has nothing to do with how voltages are actually reported by the chip.

As it turns out, mpq8785_identify() already translates VID mode to direct
mode. Here is the real problem: The identify function knows that VID mode
is handled wrongly by the chip, and configures the driver for direct mode.
pmbus_identify_common(), however, does not take that into account and bails
out if the mode read from the chip does not match the configured mode.

That is what needs to be fixed, and until I find a cleaner solution the
patch is indeed acceptable. However, the above needs to be explained in
a comment and in the patch description.

Thanks,
Guenter

> 2.Forcing direct mode: voltage readings are consistent and as expected.
> cat /sys/bus/i2c/devices/58-0046/hwmon/hwmon2/in2_input
> 3293
> 
> This suggests that the issue is related to how the chip reports voltages in VID mode,
> rather than a datasheet requirement to overwrite the mode.
> I’ll revise the patch and update the wording accordingly.
> 
> Thanks,
> Carl
> 
>   
>>> Signed-off-by: Carl Lee <carl.lee@amd.com>
>>> ---
>>>   drivers/hwmon/pmbus/mpq8785.c | 20 ++++++++++++++++++++
>>>   1 file changed, 20 insertions(+)
>>>
>>> diff --git a/drivers/hwmon/pmbus/mpq8785.c b/drivers/hwmon/pmbus/mpq8785.c
>>> index f35534836cb8..d6624af076c3 100644
>>> --- a/drivers/hwmon/pmbus/mpq8785.c
>>> +++ b/drivers/hwmon/pmbus/mpq8785.c
>>> @@ -48,6 +48,25 @@ static int mpq8785_identify(struct i2c_client *client,
>>>   	return 0;
>>>   };
>>>   
>>> +static int mpq8785_read_byte_data(struct i2c_client *client, int page, int reg)
>>> +{
>>> +	int ret;
>>> +
>>> +	switch (reg) {
>>> +	case PMBUS_VOUT_MODE:
>>> +		ret = pmbus_read_byte_data(client, page, reg);
>>> +		if (ret < 0)
>>> +			return ret;
>>> +
>>> +		if ((ret >> 5) == 1)
>>> +			return PB_VOUT_MODE_DIRECT;
>>> +	default:
>>> +		return -ENODATA;
>>> +	}
>>> +
>>> +	return ret;
>>> +}
>>> +
>>>   static int mpm82504_read_word_data(struct i2c_client *client, int page,
>>>   				   int phase, int reg)
>>>   {
>>> @@ -133,6 +152,7 @@ static int mpq8785_probe(struct i2c_client *client)
>>>   	case mpq8785:
>>>   	case mpq8786:
>>>   		info->identify = mpq8785_identify;
>>> +		info->read_byte_data = mpq8785_read_byte_data;
>>>   		break;
>>>   	default:
>>>   		return -ENODEV;
>>>
>>> -- 
>>> 2.34.1
>>>
>>>