* [RFC] hwmon: (pmbus) Some questions about PMBUS_STATUS
@ 2021-06-24 2:23 ainux.wang
2021-06-24 12:32 ` Guenter Roeck
0 siblings, 1 reply; 2+ messages in thread
From: ainux.wang @ 2021-06-24 2:23 UTC (permalink / raw
To: jdelvare, linux, corbet, ainux.wang
Cc: linux-hwmon, linux-doc, sterlingteng, chenhuacai, chenhuacai
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
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [RFC] hwmon: (pmbus) Some questions about PMBUS_STATUS
2021-06-24 2:23 [RFC] hwmon: (pmbus) Some questions about PMBUS_STATUS ainux.wang
@ 2021-06-24 12:32 ` Guenter Roeck
0 siblings, 0 replies; 2+ messages in thread
From: Guenter Roeck @ 2021-06-24 12:32 UTC (permalink / raw
To: ainux.wang
Cc: jdelvare, corbet, linux-hwmon, linux-doc, sterlingteng,
chenhuacai, chenhuacai
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
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-06-24 12:32 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-06-24 2:23 [RFC] hwmon: (pmbus) Some questions about PMBUS_STATUS ainux.wang
2021-06-24 12:32 ` Guenter Roeck
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.