| Message ID | 20210624022327.6192-1-ainux.wang@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [RFC] hwmon: (pmbus) Some questions about PMBUS_STATUS | expand |
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 >
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);