Re: [PATCH v6 2/2] hwmon: (pmbus) Try use read_status() to read status register

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

On 7/2/21 12:31 AM, ainux.wang@gmail.com wrote:
> From: "Ainux.Wang" <ainux.wang@gmail.com>
> 
> There is a case(like MP2949A) that the chip do not support STATUS_WORD
> and STATUS_BYTE command, but return some random data when reading.
> 
> So we should call read_status() instead of i2c_smbus_read_word_data()
> and i2c_smbus_read_byte_data(), and the chip driver should implement a
> read_word_data() function and a read_byte_data() function to return
> -ENXIO.

No, the chip driver needs to simulate at least one of the commands. That makes most
of the changes below unnecessary. We should limit complexity to where it is needed,
meaning the chip driver.

> 
> Signed-off-by: Ainux.Wang <ainux.wang@gmail.com>
> ---
>   drivers/hwmon/pmbus/pmbus_core.c | 20 +++++++++++++++-----
>   1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 776ee2237be2..d3273a20e76e 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -503,6 +503,9 @@ static int pmbus_check_status_cml(struct i2c_client *client)
>   	struct pmbus_data *data = i2c_get_clientdata(client);
>   	int status, status2;
>   
> +	if (!data->read_status)
> +		return -EINVAL;
> +

Unnecessary.

>   	status = data->read_status(client, -1);
>   	if (status < 0 || (status & PB_STATUS_CML)) {
>   		status2 = _pmbus_read_byte_data(client, -1, PMBUS_STATUS_CML);
> @@ -534,6 +537,9 @@ static bool pmbus_check_status_register(struct i2c_client *client, int page)
>   	int status;
>   	struct pmbus_data *data = i2c_get_clientdata(client);
>   
> +	if (!data->read_status)
> +		return false;
> +

Unnecessary.

>   	status = data->read_status(client, page);
>   	if (status >= 0 && !(data->flags & PMBUS_SKIP_STATUS_CHECK) &&
>   	    (status & PB_STATUS_CML)) {
> @@ -573,6 +579,8 @@ static int pmbus_get_status(struct i2c_client *client, int page, int reg)
>   
>   	switch (reg) {
>   	case PMBUS_STATUS_WORD:
> +		if (!data->read_status)
> +			return -EINVAL;

Unnecessary.

>   		status = data->read_status(client, page);
>   		break;
>   	default:
> @@ -2306,16 +2314,15 @@ 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.
>   	 */
>   	data->read_status = pmbus_read_status_word;
> -	ret = i2c_smbus_read_word_data(client, PMBUS_STATUS_WORD);
> +	ret = data->read_status(client, -1);

This ...

>   	if (ret < 0 || ret == 0xffff) {
>   		data->read_status = pmbus_read_status_byte;
> -		ret = i2c_smbus_read_byte_data(client, PMBUS_STATUS_BYTE);
> +		ret = data->read_status(client, -1);

and this are the only two changes needed.

>   		if (ret < 0 || ret == 0xff) {
> -			dev_err(dev, "PMBus status register not found\n");
> -			return -ENODEV;
> +			/* Both registers are not supported. */
> +			data->read_status = NULL;

Unnecessary.

>   		}
>   	} else {
>   		data->has_status_word = true;
> @@ -2484,6 +2491,9 @@ static int pmbus_debugfs_get_status(void *data, u64 *val)
>   	struct pmbus_debugfs_entry *entry = data;
>   	struct pmbus_data *pdata = i2c_get_clientdata(entry->client);
>   
> +	if (!pdata->read_status)
> +		return -EINVAL;
> +
Unnecessary.

>   	rc = pdata->read_status(entry->client, entry->page);
>   	if (rc < 0)
>   		return rc;
>