Re: [PATCH v5 4/4] hwmon: (pmbus) Add get_voltage/set_voltage ops

[Marten Lindahl <martenli@axis.com>]

On Mon, May 02, 2022 at 06:48:25PM +0200, Guenter Roeck wrote:
> On 5/2/22 04:13, Mårten Lindahl wrote:
> > The pmbus core does not have operations for getting or setting voltage.
> > Add functions get/set voltage for the dynamic regulator framework.
> > 
> > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> > ---
> >   drivers/hwmon/pmbus/pmbus_core.c | 63 ++++++++++++++++++++++++++++++++
> >   1 file changed, 63 insertions(+)
> > 
> > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> > index bd143ca0c320..455d06ba5fdf 100644
> > --- a/drivers/hwmon/pmbus/pmbus_core.c
> > +++ b/drivers/hwmon/pmbus/pmbus_core.c
> > @@ -2563,11 +2563,74 @@ static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned
> >   	return 0;
> >   }
> >   
> > +static int pmbus_regulator_get_voltage(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);
> > +	struct pmbus_sensor s = {
> > +		.page = rdev_get_id(rdev),
> > +		.class = PSC_VOLTAGE_OUT,
> > +		.convert = true,
> > +	};
> > +
> > +	s.data = _pmbus_read_word_data(client, s.page, 0xff, PMBUS_READ_VOUT);
> > +	if (s.data < 0)
> > +		return s.data;
> > +
> > +	return (int)pmbus_reg2data(data, &s) * 1000; /* unit is uV */
> > +}
> > +
> > +static int pmbus_regulator_set_voltage(struct regulator_dev *rdev, int min_uV,
> > +					 int max_uV, unsigned int *selector)
> 
> Just noticed: Please don't use camelCase.

Ok, I will change it. But for this I blame include/linux/regulator/driver.h:
struct regulator_ops {
	...
	/* get/set regulator voltage */
	int (*set_voltage) (struct regulator_dev *, int min_uV, int max_uV,
			    unsigned *selector);

> 
> > +{
> > +	struct device *dev = rdev_get_dev(rdev);
> > +	struct i2c_client *client = to_i2c_client(dev->parent);
> > +	struct pmbus_data *data = i2c_get_clientdata(client);
> > +	struct pmbus_sensor s = {
> > +		.page = rdev_get_id(rdev),
> > +		.class = PSC_VOLTAGE_OUT,
> > +		.convert = true,
> > +	};
> > +	s64 tmp = DIV_ROUND_CLOSEST_ULL(min_uV, 1000); /* convert to mV */
> 
> min_uV is already an int, so converting it to s64 will never be
> necessary.

True. Will change.

> 
> > +	int low = -1, high = -1;
> > +	u16 val;
> > +	*selector = 0;
> > +
> > +	if (pmbus_check_word_register(client, s.page, PMBUS_MFR_VOUT_MIN))
> > +		low = _pmbus_read_word_data(client, s.page, 0xff, PMBUS_MFR_VOUT_MIN);
> > +	if (low < 0) {
> > +		low = _pmbus_read_word_data(client, s.page, 0xff, PMBUS_VOUT_MARGIN_LOW);
> > +		if (low < 0)
> > +			return low;
> > +	}
> > +
> > +	if (pmbus_check_word_register(client, s.page, PMBUS_MFR_VOUT_MAX))
> > +		high = _pmbus_read_word_data(client, s.page, 0xff, PMBUS_MFR_VOUT_MAX);
> > +	if (high < 0) {
> > +		high = _pmbus_read_word_data(client, s.page, 0xff, PMBUS_VOUT_MARGIN_HIGH);
> > +		if (high < 0)
> > +			return high;
> > +	}
> > +
> > +	val = pmbus_data2reg(data, &s, tmp);
> > +
> > +	/* Make sure we are within margins */
> > +	if (low > val)
> > +		val = low;
> > +	if (high < val)
> > +		val = high;
> > +
> 
> The above assumes that register values are directly comparable.
> Unfortunately that isn't really the case. It happens to work
> for ULINEAR16 and DIRECT mode, but chips could also support
> IEEE-754 (maybe in the future) or VID mode.
> 
> You need to read the limits from the registers, convert to voltages,
> compare and adjust the voltage, and as final step convert the adjusted
> voltage to a register value.

Thanks. That is a good observation.

Kind regards
Mårten

> 
> Thanks,
> Guenter