| Message ID | 20220610114732.2370242-3-marten.lindahl@axis.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hwmon: (pmbus/ltc2978) Set voltage resolution | expand |
On 6/10/22 04:47, Mårten Lindahl wrote: > When checking if a regulator supports a voltage range, the regulator > needs to have a list_voltage callback set to the regulator_ops or else > -EINVAL will be returned. This support does not exist for the pmbus > regulators, so this patch adds pmbus_regulator_list_voltage to the > pmbus_regulator_ops. > > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com> > --- > drivers/hwmon/pmbus/pmbus_core.c | 50 ++++++++++++++++++++++++++++++++ > 1 file changed, 50 insertions(+) > > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c > index 478dda49a45f..24ba4b2b03d4 100644 > --- a/drivers/hwmon/pmbus/pmbus_core.c > +++ b/drivers/hwmon/pmbus/pmbus_core.c > @@ -2711,6 +2711,55 @@ static int pmbus_regulator_set_voltage(struct regulator_dev *rdev, int min_uv, > return _pmbus_write_word_data(client, s.page, PMBUS_VOUT_COMMAND, (u16)val); > } > > +static int pmbus_regulator_list_voltage(struct regulator_dev *rdev, > + 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 s = { > + .page = rdev_get_id(rdev), > + .class = PSC_VOLTAGE_OUT, > + .convert = true, > + .data = -1, > + }; > + int val = DIV_ROUND_CLOSEST(rdev->desc->min_uV + > + (rdev->desc->uV_step * selector), > + 1000); /* convert to mV */ > + > + if (!data->vout_low[s.page]) { > + if (pmbus_check_word_register(client, s.page, PMBUS_MFR_VOUT_MIN)) > + s.data = _pmbus_read_word_data(client, s.page, 0xff, > + PMBUS_MFR_VOUT_MIN); > + if (s.data < 0) { > + s.data = _pmbus_read_word_data(client, s.page, 0xff, > + PMBUS_VOUT_MARGIN_LOW); > + if (s.data < 0) > + return s.data; > + } > + data->vout_low[s.page] = pmbus_reg2data(data, &s); > + } > + > + if (!data->vout_high[s.page]) { > + s.data = -1; > + if (pmbus_check_word_register(client, s.page, PMBUS_MFR_VOUT_MAX)) > + s.data = _pmbus_read_word_data(client, s.page, 0xff, > + PMBUS_MFR_VOUT_MAX); > + if (s.data < 0) { > + s.data = _pmbus_read_word_data(client, s.page, 0xff, > + PMBUS_VOUT_MARGIN_HIGH); > + if (s.data < 0) > + return s.data; > + } > + data->vout_high[s.page] = pmbus_reg2data(data, &s); > + } > + The code above is similar to the same code in the first patch. Please move it into a function (in the first patch). > + if (val >= data->vout_low[s.page] && val <= data->vout_high[s.page]) > + return val * 1000; /* unit is uV */ > + > + return 0; Other drivers return -EINVAL here. Should this be returned as well if rdev->desc->min_uV or rdev->desc->uV_step is 0, if selector is out of range, or if data->vout_low[s.page] / data->vout_high[s.page] is 0 ? Thanks, Guenter > +} > + > const struct regulator_ops pmbus_regulator_ops = { > .enable = pmbus_regulator_enable, > .disable = pmbus_regulator_disable, > @@ -2718,6 +2767,7 @@ const struct regulator_ops pmbus_regulator_ops = { > .get_error_flags = pmbus_regulator_get_error_flags, > .get_voltage = pmbus_regulator_get_voltage, > .set_voltage = pmbus_regulator_set_voltage, > + .list_voltage = pmbus_regulator_list_voltage, > }; > EXPORT_SYMBOL_NS_GPL(pmbus_regulator_ops, PMBUS); >
On Fri, Jun 10, 2022 at 04:16:46PM +0200, Guenter Roeck wrote: > On 6/10/22 04:47, Mårten Lindahl wrote: > > When checking if a regulator supports a voltage range, the regulator > > needs to have a list_voltage callback set to the regulator_ops or else > > -EINVAL will be returned. This support does not exist for the pmbus > > regulators, so this patch adds pmbus_regulator_list_voltage to the > > pmbus_regulator_ops. > > > > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com> > > --- > > drivers/hwmon/pmbus/pmbus_core.c | 50 ++++++++++++++++++++++++++++++++ > > 1 file changed, 50 insertions(+) > > > > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c > > index 478dda49a45f..24ba4b2b03d4 100644 > > --- a/drivers/hwmon/pmbus/pmbus_core.c > > +++ b/drivers/hwmon/pmbus/pmbus_core.c > > @@ -2711,6 +2711,55 @@ static int pmbus_regulator_set_voltage(struct regulator_dev *rdev, int min_uv, > > return _pmbus_write_word_data(client, s.page, PMBUS_VOUT_COMMAND, (u16)val); > > } > > > > +static int pmbus_regulator_list_voltage(struct regulator_dev *rdev, > > + 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 s = { > > + .page = rdev_get_id(rdev), > > + .class = PSC_VOLTAGE_OUT, > > + .convert = true, > > + .data = -1, > > + }; > > + int val = DIV_ROUND_CLOSEST(rdev->desc->min_uV + > > + (rdev->desc->uV_step * selector), > > + 1000); /* convert to mV */ > > + > > + if (!data->vout_low[s.page]) { > > + if (pmbus_check_word_register(client, s.page, PMBUS_MFR_VOUT_MIN)) > > + s.data = _pmbus_read_word_data(client, s.page, 0xff, > > + PMBUS_MFR_VOUT_MIN); > > + if (s.data < 0) { > > + s.data = _pmbus_read_word_data(client, s.page, 0xff, > > + PMBUS_VOUT_MARGIN_LOW); > > + if (s.data < 0) > > + return s.data; > > + } > > + data->vout_low[s.page] = pmbus_reg2data(data, &s); > > + } > > + > > + if (!data->vout_high[s.page]) { > > + s.data = -1; > > + if (pmbus_check_word_register(client, s.page, PMBUS_MFR_VOUT_MAX)) > > + s.data = _pmbus_read_word_data(client, s.page, 0xff, > > + PMBUS_MFR_VOUT_MAX); > > + if (s.data < 0) { > > + s.data = _pmbus_read_word_data(client, s.page, 0xff, > > + PMBUS_VOUT_MARGIN_HIGH); > > + if (s.data < 0) > > + return s.data; > > + } > > + data->vout_high[s.page] = pmbus_reg2data(data, &s); > > + } > > + Hi Guenter! > > The code above is similar to the same code in the first patch. Please > move it into a function (in the first patch). Ok, I will do that. I think it will be one function for low_margin and one for high_margin. > > > + if (val >= data->vout_low[s.page] && val <= data->vout_high[s.page]) > > + return val * 1000; /* unit is uV */ > > + > > + return 0; > > Other drivers return -EINVAL here. Should this be returned as well > if rdev->desc->min_uV or rdev->desc->uV_step is 0, if selector > is out of range, or if data->vout_low[s.page] / data->vout_high[s.page] > is 0 ? I will add some more checks. I will try to follow what the description of list_voltages says in include/linux/regulator/driver.h, but it's a bit free for interpretation, thinking of what counts as unusable voltages :) * @list_voltage: Return one of the supported voltages, in microvolts; zero * if the selector indicates a voltage that is unusable on this system; * or negative errno. Selectors range from zero to one less than * regulator_desc.n_voltages. Voltages may be reported in any order. I guess, as long as a voltage is inside of the low/high margins it is a valid voltage (even though high and/or low is 0). Kind regards Mårten > > Thanks, > Guenter > > > +} > > + > > const struct regulator_ops pmbus_regulator_ops = { > > .enable = pmbus_regulator_enable, > > .disable = pmbus_regulator_disable, > > @@ -2718,6 +2767,7 @@ const struct regulator_ops pmbus_regulator_ops = { > > .get_error_flags = pmbus_regulator_get_error_flags, > > .get_voltage = pmbus_regulator_get_voltage, > > .set_voltage = pmbus_regulator_set_voltage, > > + .list_voltage = pmbus_regulator_list_voltage, > > }; > > EXPORT_SYMBOL_NS_GPL(pmbus_regulator_ops, PMBUS); > > >
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c index 478dda49a45f..24ba4b2b03d4 100644 --- a/drivers/hwmon/pmbus/pmbus_core.c +++ b/drivers/hwmon/pmbus/pmbus_core.c @@ -2711,6 +2711,55 @@ static int pmbus_regulator_set_voltage(struct regulator_dev *rdev, int min_uv, return _pmbus_write_word_data(client, s.page, PMBUS_VOUT_COMMAND, (u16)val); } +static int pmbus_regulator_list_voltage(struct regulator_dev *rdev, + 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 s = { + .page = rdev_get_id(rdev), + .class = PSC_VOLTAGE_OUT, + .convert = true, + .data = -1, + }; + int val = DIV_ROUND_CLOSEST(rdev->desc->min_uV + + (rdev->desc->uV_step * selector), + 1000); /* convert to mV */ + + if (!data->vout_low[s.page]) { + if (pmbus_check_word_register(client, s.page, PMBUS_MFR_VOUT_MIN)) + s.data = _pmbus_read_word_data(client, s.page, 0xff, + PMBUS_MFR_VOUT_MIN); + if (s.data < 0) { + s.data = _pmbus_read_word_data(client, s.page, 0xff, + PMBUS_VOUT_MARGIN_LOW); + if (s.data < 0) + return s.data; + } + data->vout_low[s.page] = pmbus_reg2data(data, &s); + } + + if (!data->vout_high[s.page]) { + s.data = -1; + if (pmbus_check_word_register(client, s.page, PMBUS_MFR_VOUT_MAX)) + s.data = _pmbus_read_word_data(client, s.page, 0xff, + PMBUS_MFR_VOUT_MAX); + if (s.data < 0) { + s.data = _pmbus_read_word_data(client, s.page, 0xff, + PMBUS_VOUT_MARGIN_HIGH); + if (s.data < 0) + return s.data; + } + data->vout_high[s.page] = pmbus_reg2data(data, &s); + } + + if (val >= data->vout_low[s.page] && val <= data->vout_high[s.page]) + return val * 1000; /* unit is uV */ + + return 0; +} + const struct regulator_ops pmbus_regulator_ops = { .enable = pmbus_regulator_enable, .disable = pmbus_regulator_disable, @@ -2718,6 +2767,7 @@ const struct regulator_ops pmbus_regulator_ops = { .get_error_flags = pmbus_regulator_get_error_flags, .get_voltage = pmbus_regulator_get_voltage, .set_voltage = pmbus_regulator_set_voltage, + .list_voltage = pmbus_regulator_list_voltage, }; EXPORT_SYMBOL_NS_GPL(pmbus_regulator_ops, PMBUS);
When checking if a regulator supports a voltage range, the regulator needs to have a list_voltage callback set to the regulator_ops or else -EINVAL will be returned. This support does not exist for the pmbus regulators, so this patch adds pmbus_regulator_list_voltage to the pmbus_regulator_ops. Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com> --- drivers/hwmon/pmbus/pmbus_core.c | 50 ++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+)