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@xxxxxxxx>
> > ---
> > 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
