Re: [PATCH 2/4] hwmon: (pmbus/core) Generalise pmbus get status

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

On Mon, Jan 23, 2023 at 07:40:19AM +0100, Naresh Solanki wrote:
> Add function pmbus get status that can be used to get both pmbus
> specific status & regulator status
> 
> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> ---
>  drivers/hwmon/pmbus/pmbus_core.c | 148 +++++++++++++++++--------------
>  1 file changed, 82 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 1b70cf3be313..12b662b91306 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -2735,64 +2735,16 @@ static const struct pmbus_status_category __maybe_unused pmbus_status_flag_map[]
>  	},
>  };
>  
> -#if IS_ENABLED(CONFIG_REGULATOR)
> -static int pmbus_regulator_is_enabled(struct regulator_dev *rdev)
> -{
> -	struct device *dev = rdev_get_dev(rdev);
> -	struct i2c_client *client = to_i2c_client(dev->parent);
> -	struct pmbus_data *data = i2c_get_clientdata(client);
> -	u8 page = rdev_get_id(rdev);
> -	int ret;
>  
> -	mutex_lock(&data->update_lock);
> -	ret = _pmbus_read_byte_data(client, page, PMBUS_OPERATION);
> -	mutex_unlock(&data->update_lock);
> -
> -	if (ret < 0)
> -		return ret;
> -
> -	return !!(ret & PB_OPERATION_CONTROL_ON);
> -}
> -
> -static int _pmbus_regulator_on_off(struct regulator_dev *rdev, bool enable)
> +static int pmbus_get_flags(struct pmbus_data *data, u8 page, unsigned int *error)

Moving this code outside the conditional without usage there will result
in compile errors due to an unused function if compiled with REGULATOR
disabled. While this will (hopefully) change in one of the later patches,
it is still unacceptable because it may result in bisect failures.

>  {
> -	struct device *dev = rdev_get_dev(rdev);
> -	struct i2c_client *client = to_i2c_client(dev->parent);
> -	struct pmbus_data *data = i2c_get_clientdata(client);
> -	u8 page = rdev_get_id(rdev);
> -	int ret;
> -
> -	mutex_lock(&data->update_lock);
> -	ret = pmbus_update_byte_data(client, page, PMBUS_OPERATION,
> -				     PB_OPERATION_CONTROL_ON,
> -				     enable ? PB_OPERATION_CONTROL_ON : 0);
> -	mutex_unlock(&data->update_lock);
> -
> -	return ret;
> -}
> -
> -static int pmbus_regulator_enable(struct regulator_dev *rdev)
> -{
> -	return _pmbus_regulator_on_off(rdev, 1);
> -}
> -
> -static int pmbus_regulator_disable(struct regulator_dev *rdev)
> -{
> -	return _pmbus_regulator_on_off(rdev, 0);
> -}
> -
> -static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned int *flags)
> -{
> -	int i, status;
>  	const struct pmbus_status_category *cat;
>  	const struct pmbus_status_assoc *bit;
> -	struct device *dev = rdev_get_dev(rdev);
> -	struct i2c_client *client = to_i2c_client(dev->parent);
> -	struct pmbus_data *data = i2c_get_clientdata(client);
> -	u8 page = rdev_get_id(rdev);
> +	struct i2c_client *client = to_i2c_client(data->dev);
>  	int func = data->info->func[page];
> +	int i, status, ret;
>  
> -	*flags = 0;
> +	*error = 0;

You are making personal preference changes here. Maybe that is the reason
why the patch looks that large. Please try to leave existing code alone.
If there is a reason to change a variable name (or other cosmetic changes,
like moving variable declarations around), do it in a separate patch which
only does that, and explain why it is needed and/or makes sense.

Thanks,
Guenter

>  
>  	mutex_lock(&data->update_lock);
>  
> @@ -2803,14 +2755,15 @@ static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned
>  
>  		status = _pmbus_read_byte_data(client, page, cat->reg);
>  		if (status < 0) {
> -			mutex_unlock(&data->update_lock);
> -			return status;
> +			ret = status;
> +			goto unlock;
>  		}
>  
>  		for (bit = cat->bits; bit->pflag; bit++) {
>  			if (status & bit->pflag)
> -				*flags |= bit->rflag;
> +				*error |= bit->rflag;
>  		}
> +
>  	}
>  
>  	/*
> @@ -2823,36 +2776,99 @@ static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned
>  	 * REGULATOR_ERROR_<foo>_WARN.
>  	 */
>  	status = pmbus_get_status(client, page, PMBUS_STATUS_WORD);
> -	mutex_unlock(&data->update_lock);
> -	if (status < 0)
> -		return status;
>  
> -	if (pmbus_regulator_is_enabled(rdev)) {
> +	if (status < 0) {
> +		ret = status;
> +		goto unlock;
> +	}
> +
> +	ret = _pmbus_read_byte_data(client, page, PMBUS_OPERATION);
> +	if (ret < 0)
> +		goto unlock;
> +
> +	if (ret & PB_OPERATION_CONTROL_ON) {
>  		if (status & PB_STATUS_OFF)
> -			*flags |= REGULATOR_ERROR_FAIL;
> +			*error |= REGULATOR_ERROR_FAIL;
>  
>  		if (status & PB_STATUS_POWER_GOOD_N)
> -			*flags |= REGULATOR_ERROR_REGULATION_OUT;
> +			*error |= REGULATOR_ERROR_REGULATION_OUT;
>  	}
>  	/*
>  	 * Unlike most other status bits, PB_STATUS_{IOUT_OC,VOUT_OV} are
>  	 * defined strictly as fault indicators (not warnings).
>  	 */
>  	if (status & PB_STATUS_IOUT_OC)
> -		*flags |= REGULATOR_ERROR_OVER_CURRENT;
> +		*error |= REGULATOR_ERROR_OVER_CURRENT;
>  	if (status & PB_STATUS_VOUT_OV)
> -		*flags |= REGULATOR_ERROR_REGULATION_OUT;
> +		*error |= REGULATOR_ERROR_REGULATION_OUT;
>  
>  	/*
>  	 * If we haven't discovered any thermal faults or warnings via
>  	 * PMBUS_STATUS_TEMPERATURE, map PB_STATUS_TEMPERATURE to a warning as
>  	 * a (conservative) best-effort interpretation.
>  	 */
> -	if (!(*flags & (REGULATOR_ERROR_OVER_TEMP | REGULATOR_ERROR_OVER_TEMP_WARN)) &&
> +	if (!(*error & (REGULATOR_ERROR_OVER_TEMP | REGULATOR_ERROR_OVER_TEMP_WARN)) &&
>  	    (status & PB_STATUS_TEMPERATURE))
> -		*flags |= REGULATOR_ERROR_OVER_TEMP_WARN;
> +		*error |= REGULATOR_ERROR_OVER_TEMP_WARN;
>  
> -	return 0;
> +unlock:
> +	mutex_unlock(&data->update_lock);
> +	return ret;
> +}
> +
> +#if IS_ENABLED(CONFIG_REGULATOR)
> +static int pmbus_regulator_is_enabled(struct regulator_dev *rdev)
> +{
> +	struct device *dev = rdev_get_dev(rdev);
> +	struct i2c_client *client = to_i2c_client(dev->parent);
> +	struct pmbus_data *data = i2c_get_clientdata(client);
> +	u8 page = rdev_get_id(rdev);
> +	int ret;
> +
> +	mutex_lock(&data->update_lock);
> +	ret = _pmbus_read_byte_data(client, page, PMBUS_OPERATION);
> +	mutex_unlock(&data->update_lock);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return !!(ret & PB_OPERATION_CONTROL_ON);
> +}
> +
> +static int _pmbus_regulator_on_off(struct regulator_dev *rdev, bool enable)
> +{
> +	struct device *dev = rdev_get_dev(rdev);
> +	struct i2c_client *client = to_i2c_client(dev->parent);
> +	struct pmbus_data *data = i2c_get_clientdata(client);
> +	u8 page = rdev_get_id(rdev);
> +	int ret;
> +
> +	mutex_lock(&data->update_lock);
> +	ret = pmbus_update_byte_data(client, page, PMBUS_OPERATION,
> +				     PB_OPERATION_CONTROL_ON,
> +				     enable ? PB_OPERATION_CONTROL_ON : 0);
> +	mutex_unlock(&data->update_lock);
> +
> +	return ret;
> +}
> +
> +static int pmbus_regulator_enable(struct regulator_dev *rdev)
> +{
> +	return _pmbus_regulator_on_off(rdev, 1);
> +}
> +
> +static int pmbus_regulator_disable(struct regulator_dev *rdev)
> +{
> +	return _pmbus_regulator_on_off(rdev, 0);
> +}
> +
> +static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned int *flags)
> +{
> +	struct device *dev = rdev_get_dev(rdev);
> +	struct i2c_client *client = to_i2c_client(dev->parent);
> +	struct pmbus_data *data = i2c_get_clientdata(client);
> +
> +	return pmbus_get_flags(data, rdev_get_id(rdev), flags);
>  }
>  
>  static int pmbus_regulator_get_status(struct regulator_dev *rdev)
> -- 
> 2.38.1
>