[RFC] hwmon: (pmbus) Some questions about PMBUS_STATUS

[RFC] hwmon: (pmbus) Some questions about PMBUS_STATUS

[ainux.wang]

From: "Ainux.Wang" <ainux.wang@gmail.com>

There are some questions about PMBUS_STATUS in core.

Signed-off-by: Ainux.Wang <ainux.wang@gmail.com>
---
 drivers/hwmon/pmbus/pmbus_core.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index bbd745178147..e16c85997148 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -2200,6 +2200,19 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
 	 * Some PMBus chips don't support PMBUS_STATUS_WORD, so try
 	 * to use PMBUS_STATUS_BYTE instead if that is the case.
 	 * Bail out if both registers are not supported.
+	 *
+	 * Question 1:
+	 *  Why bail out if both registers are not supported?
+	 *  MP2949A both registers are not supported.
+	 *
+	 * Question 2:
+	 *  Use i2c_smbus_read_word_data() or i2c_smbus_read_byte_data
+	 *  to read, the MP2949A will return undetermined value, although
+	 *  we already known this chip do not support both registers.
+	 *  What should we do?
+	 *  Can we use pmbus_read_status_byte() or pmbus_read_status_word()?
+	 *  and in MP2949A driver's .read_byte_data and .read_word_data to
+	 *  filter out both registers?
 	 */
 	data->read_status = pmbus_read_status_word;
 	ret = i2c_smbus_read_word_data(client, PMBUS_STATUS_WORD);
-- 
2.18.1

Re: [RFC] hwmon: (pmbus) Some questions about PMBUS_STATUS

[Guenter Roeck]

On Thu, Jun 24, 2021 at 10:23:27AM +0800, ainux.wang@gmail.com wrote:
> From: "Ainux.Wang" <ainux.wang@gmail.com>
> 
> There are some questions about PMBUS_STATUS in core.
> 
I am curious - why do you think such questions would be appropriate as
comment in the code ?

> Signed-off-by: Ainux.Wang <ainux.wang@gmail.com>
> ---
>  drivers/hwmon/pmbus/pmbus_core.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index bbd745178147..e16c85997148 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -2200,6 +2200,19 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
>  	 * Some PMBus chips don't support PMBUS_STATUS_WORD, so try
>  	 * to use PMBUS_STATUS_BYTE instead if that is the case.
>  	 * Bail out if both registers are not supported.
> +	 *
> +	 * Question 1:
> +	 *  Why bail out if both registers are not supported?
> +	 *  MP2949A both registers are not supported.
> +	 *

The status registers are essential for chip operation.

_Normally_ the chip should return an error when trying to access an
unsupported command/register, and it should set an error bit in the
status register. Obviusly the second part won't work here.

> +	 * Question 2:
> +	 *  Use i2c_smbus_read_word_data() or i2c_smbus_read_byte_data
> +	 *  to read, the MP2949A will return undetermined value, although
> +	 *  we already known this chip do not support both registers.
> +	 *  What should we do?

PMBus drivers should never call those functions directly but use the functions
provided by the PMBUs core.

> +	 *  Can we use pmbus_read_status_byte() or pmbus_read_status_word()?
> +	 *  and in MP2949A driver's .read_byte_data and .read_word_data to
> +	 *  filter out both registers?

You would use those functions, but not to filter out both commands but to
simulate one of them and filter out the other.

In general, it is acceptable to simulate or filter out a command if a chip
doesn't support it but does not return an error when accessing it. If you
may recall, I asked you several times why you wanted to filter out the
PMBUS_VOUT_MODE command. You always answered with "the chip does not
support it". Again, that is not a reason to filter out a command. However,
if a chip does not support a command but does not return an error when
accessing it either, it is perfectly valid (and even necessary) to filter
out or simulate such unsupported commands. This is, however, only
acceptable if a chip does not return an error when trying to access the
unsupported registers. Such situations need to be documented in the code,
which should include a comment such as "This chip does not support the
following command(s) and returns random data when reading from them/it".

>  	 */
>  	data->read_status = pmbus_read_status_word;
>  	ret = i2c_smbus_read_word_data(client, PMBUS_STATUS_WORD);
> -- 
> 2.18.1
>