On Thu, Apr 28, 2022 at 06:49:21PM +0200, Guenter Roeck wrote:
> On 4/28/22 07:40, 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..fe7dbb496e3b 100644
> > --- a/drivers/hwmon/pmbus/pmbus_core.c
> > +++ b/drivers/hwmon/pmbus/pmbus_core.c
> > @@ -1531,6 +1531,11 @@ static const struct pmbus_sensor_attr voltage_attributes[] = {
> > .gbit = PB_STATUS_VOUT_OV,
> > .limit = vout_limit_attrs,
> > .nlimit = ARRAY_SIZE(vout_limit_attrs),
> > + }, {
> > + .reg = PMBUS_VOUT_COMMAND,
> > + .class = PSC_VOLTAGE_OUT,
> > + .paged = true,
> > + .func = PMBUS_HAVE_VOUT,
> > }
>
> Ok, you lost me here. This adds an inX_input attribute. Why ? This is completely
> unrelated to the intended scope of this patch. It also doesn't report a measured
> voltage, but a configuration value. If anything, it would have to be a separate
> patch, and you'd have to argue hard why it makes sense to report it as measured
> voltage.
I see. The reason for adding this is as simple as I now understand it is wrong.
Please remember, my first version of the set/get_voltage functions where hardcoded
with L16 input/output. Then in order to use the already existing convertion functions
pmbus_data2reg and pmbus_reg2data I added this only for the need of a sensor object,
as those functions are tailored for a sensor object.
So now I have to ask you for advice. Should I use the existing convertion
functions, or do you suggest new variants of them? If reusing them, I guess I have
two options:
1: Modify them to take class, page, and data outside of a sensor object as input.
2: Use them as they are, but create a local 'dummy' sensor object with class, page,
and data to use when calling the convertion functions.
I hope I made it more clear for you now how I was thinking. There is
absolutely no intention of having sensor inX_input attributes for
reading the setpoint values. This was just an unwanted sideeffect, and I
will glady remove it again.
>
> > };
> >
> > @@ -2563,11 +2568,69 @@ 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 *sensor;
> > + u8 page = rdev_get_id(rdev);
> > + int ret;
> > +
> > + sensor = pmbus_find_sensor(data, page, PMBUS_READ_VOUT);
> > + if (IS_ERR(sensor))
> > + return -ENODATA;
> > +
> > + mutex_lock(&data->update_lock);
> > + pmbus_update_sensor_data(client, sensor);
> > + if (sensor->data < 0)
> > + ret = sensor->data;
> > + else
> > + ret = (int)pmbus_reg2data(data, sensor) * 1000; /* unit is uV */
> > + mutex_unlock(&data->update_lock);
> > +
>
> Same question. Why ?
Same reason as above. Only to get the sensor object for pmbus_reg2data.
>
> > + return ret;
> > +}
> > +
> > +static int pmbus_regulator_set_voltage(struct regulator_dev *rdev, int min_uV,
> > + int max_uV, unsigned int *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 *sensor;
> > + u8 page = rdev_get_id(rdev);
> > + s64 tmp = DIV_ROUND_CLOSEST_ULL(min_uV, 1000); /* convert to mV */
> > + u16 val;
> > + int ret;
> > + *selector = 0;
> > +
> > + sensor = pmbus_find_sensor(data, page, PMBUS_VOUT_COMMAND);
> > + if (IS_ERR(sensor))
> > + return -ENODATA;
> > +
> > + ret = _pmbus_read_word_data(client, page, 0xff, PMBUS_VOUT_MARGIN_LOW);
> > + if (ret < 0)
> > + return ret;
> > +
> That actually makes me wonder: What about VOUT_MARGIN_HIGH ?
Ok, I will add a check for VOUT_MARGIN_HIGH also.
> Also, there are optional MFR_VOUT_MIN and MFR_VOUT_MAX registers.
> Would it possibly make sense to determine the valid range once
> during probe and then compare against it ?
Maybe this could be a good thing, so that we don't need to read both
margins every time. But I guess that would need a new kind of page list
with margins added to the pmbus_driver_info struct?
I would prefer to make that change in a separate patch if it's ok with
you?
Kind regards
Mårten
>
> Thanks,
> Guenter
>
> > + val = pmbus_data2reg(data, sensor, tmp);
> > +
> > + /* Do not fall shorter than low margin */
> > + if (ret > val)
> > + val = ret;
> > +
> > + ret = _pmbus_write_word_data(client, page, PMBUS_VOUT_COMMAND, val);
> > +
> > + return ret;
> > +}
> > +
> > const struct regulator_ops pmbus_regulator_ops = {
> > .enable = pmbus_regulator_enable,
> > .disable = pmbus_regulator_disable,
> > .is_enabled = pmbus_regulator_is_enabled,
> > .get_error_flags = pmbus_regulator_get_error_flags,
> > + .get_voltage = pmbus_regulator_get_voltage,
> > + .set_voltage = pmbus_regulator_set_voltage,
> > };
> > EXPORT_SYMBOL_NS_GPL(pmbus_regulator_ops, PMBUS);
> >
>
