Re: [PATCH 4/4] hwmon: (pmbus/tps40422) add support for output voltage margining

[Guenter Roeck]

On Tue, Apr 16, 2019 at 11:36:19AM -0700, Ruslan Babayev wrote:
> TPS40422 has MFR_SPECIFIC registers STEP_VREF_MARGIN_HIGH and
> STEP_VREF_MARGIN_LOW, which are signed 16-bit in units of 2mV. This
> value is an offset from the nominal reference voltage of 600mV.
> 
> For instance, the default value of STEP_VREF_MARGIN_LOW is -30
> (decimal), which corresponds to a default margin low voltage of
> 600 - 30 * 2 = 540mV.
> 
> Introduce tps40422_{read/write}_word_data() callbacks to map the
> MFR_SPECIFIC registers to standard PMBUS_VOUT_MARGIN_HIGH and
> PMBUS_VOUT_MARGIN_LOW. For the latter, pmbus_core stores sensor data
> in linear16 format as specified in VOUT_MODE.  Note, that on TPS40422
> the exponent is fixed at (-9). The callbacks will perform the
> necessary conversions.
> 

As mentioned with the other patches, setting actual voltages using the
hwmon subsystem is not appropriate. I can understand the need or desire
to be able to set margin voltages, but the hardware monitoring subsystem
is about monitoring. I would suggest to look at regulator subsystem,
though I don't know if that supports setting margin voltages.
If it doesn't, it might be appropriate to add support there.

The actual support can still be implemented in the pmbus drivers, but not
using hwmon attributes.

Thanks,
Guenter

> Cc: xe-linux-external@xxxxxxxxx
> Signed-off-by: Ruslan Babayev <ruslan@xxxxxxxxxxx>
> ---
>  drivers/hwmon/pmbus/tps40422.c | 54 ++++++++++++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
> 
> diff --git a/drivers/hwmon/pmbus/tps40422.c b/drivers/hwmon/pmbus/tps40422.c
> index 32803825d47e..129d1b0e6b43 100644
> --- a/drivers/hwmon/pmbus/tps40422.c
> +++ b/drivers/hwmon/pmbus/tps40422.c
> @@ -21,16 +21,70 @@
>  #include <linux/i2c.h>
>  #include "pmbus.h"
>  
> +#define TPS40422_STEP_VREF_MARGIN_HIGH	0xd5
> +#define TPS40422_STEP_VREF_MARGIN_LOW	0xd6
> +#define TPS40422_VREF			600
> +
> +static int tps40422_read_word_data(struct i2c_client *client, int page, int reg)
> +{
> +	int ret;
> +
> +	switch (reg) {
> +	case PMBUS_VOUT_MARGIN_HIGH:
> +		reg = TPS40422_STEP_VREF_MARGIN_HIGH;
> +		break;
> +	case PMBUS_VOUT_MARGIN_LOW:
> +		reg = TPS40422_STEP_VREF_MARGIN_LOW;
> +		break;
> +	default:
> +		return -ENODATA;
> +	}
> +
> +	ret = pmbus_read_word_data(client, page, reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = (TPS40422_VREF + ((s16)ret) * 2) << 9;
> +	ret = DIV_ROUND_CLOSEST(ret, 1000);
> +
> +	return ret;
> +}
> +
> +static int tps40422_write_word_data(struct i2c_client *client, int page,
> +				    int reg, u16 word)
> +{
> +	u16 val;
> +
> +	switch (reg) {
> +	case PMBUS_VOUT_MARGIN_HIGH:
> +		reg = TPS40422_STEP_VREF_MARGIN_HIGH;
> +		break;
> +	case PMBUS_VOUT_MARGIN_LOW:
> +		reg = TPS40422_STEP_VREF_MARGIN_LOW;
> +		break;
> +	default:
> +		return -ENODATA;
> +	}
> +
> +	val = ((((long)word * 1000) >> 9) - TPS40422_VREF + 1) / 2;
> +	return pmbus_write_word_data(client, page, reg, val);
> +}
> +
> +
>  static struct pmbus_driver_info tps40422_info = {
>  	.pages = 2,
> +	.read_word_data = tps40422_read_word_data,
> +	.write_word_data = tps40422_write_word_data,
>  	.format[PSC_VOLTAGE_IN] = linear,
>  	.format[PSC_VOLTAGE_OUT] = linear,
>  	.format[PSC_TEMPERATURE] = linear,
>  	.func[0] = PMBUS_HAVE_VOUT | PMBUS_HAVE_TEMP2
>  		| PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_TEMP
> +		| PMBUS_HAVE_VOUT_MARGIN_HIGH | PMBUS_HAVE_VOUT_MARGIN_LOW
>  		| PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT,
>  	.func[1] = PMBUS_HAVE_VOUT | PMBUS_HAVE_TEMP2
>  		| PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_TEMP
> +		| PMBUS_HAVE_VOUT_MARGIN_HIGH | PMBUS_HAVE_VOUT_MARGIN_LOW
>  		| PMBUS_HAVE_IOUT | PMBUS_HAVE_STATUS_IOUT,
>  };
>  
> -- 
> 2.17.1
>