Re: [PATCH v1] hwmon: (pmbus/ltc2978) Set voltage resolution

[Marten Lindahl <martenli@axis.com>]

On Thu, Jun 02, 2022 at 04:40:25PM +0200, Mårten Lindahl wrote:
> On Wed, Jun 01, 2022 at 09:12:56PM +0200, Guenter Roeck wrote:
> > On Mon, May 30, 2022 at 04:34:46PM +0200, Mårten Lindahl wrote:
> > > When checking if a regulator supports a voltage range, the regulator
> > > needs to have support for listing the range or else -EINVAL will be
> > > returned.
> > > 
> > > This support does not exist for the LTC2977 regulator, so this patch
> > > adds support for list voltage to the pmbus regulators by adding
> > > regulator_list_voltage_linear to the pmbus_regulator_ops. It also
> > > defines the voltage resolution for regulators ltc2972/LTC2974/LTC2975/
> > > LTC2977/LTC2978/LTC2979/LTC2980/LTM2987 based on that they all have the
> > > same stepwise 122.07uV resolution.
> > > 
> > > Since 122.07uV resolution is very small the resolution is set to a 1mV
> > > resolution to be easier to handle.
> > > 
> > > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> > > ---
> > >  drivers/hwmon/pmbus/ltc2978.c    | 57 +++++++++++++++++++++++++++++---
> > >  drivers/hwmon/pmbus/pmbus_core.c |  1 +
> > >  2 files changed, 54 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/hwmon/pmbus/ltc2978.c b/drivers/hwmon/pmbus/ltc2978.c
> > > index 531aa674a928..cfb568c6c155 100644
> > > --- a/drivers/hwmon/pmbus/ltc2978.c
> > > +++ b/drivers/hwmon/pmbus/ltc2978.c
> > > @@ -562,7 +562,37 @@ static const struct i2c_device_id ltc2978_id[] = {
> > >  MODULE_DEVICE_TABLE(i2c, ltc2978_id);
> > >  
> > >  #if IS_ENABLED(CONFIG_SENSORS_LTC2978_REGULATOR)
> > > +#define LTC2978_ADC_RES 0xFFFF
> > > +#define LTC2978_N_ADC 122
> > > +#define LTC2978_MAX_UV (LTC2978_ADC_RES * LTC2978_N_ADC)
> > > +#define LTC2978_UV_STEP (1000)
> >
> 
> Hi Guenter!
> 
> > #define<space>DEFINE<tab>VALUE, please, and the () around 1000
> > is unnecessary.
> 
> Ok, will fix.
> 
> > 
> > Also, is the range really correct ? The valid / acceptable
> > voltages are in the range detected in pmbus_regulator_set_voltage(),
> > based on PMBUS_MFR_VOUT_MIN/PMBUS_VOUT_MARGIN_LOW and
> > PMBUS_MFR_VOUT_MAX/PMBUS_VOUT_MARGIN_HIGH, and that will likely differ
> > from the fixed number of voltages provided here.
> > 
> > That makes me wonder if it would make more sense to move this
> > functionality into the PMBus core code. Any thoughts on that ?
> 
> Yes, I did think about that, but to support regulator_count_voltages
> the regulator_desc needs to have .n_voltages set and since the
> regulator_desc is const I have to set .n_voltages to the full range
> of the 16 bit adc resolution. And then I scale it to 1 mV units by the
> defines above.
> 
> My understanding of the regulator core list_voltage implementation is
> that it tests if a requested range fits within this range, using the
> min_uV and max_uV limits (which can be specified in DT) to validate it.
> 
> Maybe a new pmbus_regulator_list_voltage function could read the voltage
> upper and lower limits and then compare the requested range to that one?
> 

Hi Guenter!

I implemented a new pmbus_regulator_list_voltage function that compares
a requested voltage to the lower/upper limits, and it seems to work fine.

There is a performance problem with reading the boundaries
(lots of smbus transmissions) for each call of a list_voltage iteration
from the regulator core, so I made a patch to cache the boundaries after
the first read.

I will send two patches shortly.

Kind regards
Mårten

> But the regulator_desc .n_voltages still needs to be preset to
> something. What should it be set to? Should I perhaps set it to the full
> ADC resolution (0xFFFF) without scaling it?

Answer to myself: I think it is correct to set .n_voltages to the full
range of the adc resolution, and then scale it.

> 
> Please note, I added the PMBUS_LTC2978_REGULATOR macro with included
> voltages and steps only for the chips listed in the commit msg, based on
> that they all have the same ADC resolution and ADC unit size (122 uV).
> I would prefer to not add voltages and steps to the generic
> PMBUS_REGULATOR macro in pmbus.h, as I don't have knowledge about the
> other chips.
> 
> > 
> > Thanks,
> > Guenter
> > 
> > > +
> > > +#define PMBUS_LTC2978_REGULATOR(_name, _id)               \
> > > +	[_id] = {                                               \
> > > +		.name = (_name # _id),                                \
> > > +		.supply_name = "vin",                                 \
> > > +		.id = (_id),                                          \
> > > +		.of_match = of_match_ptr(_name # _id),                \
> > > +		.regulators_node = of_match_ptr("regulators"),        \
> > > +		.ops = &pmbus_regulator_ops,                          \
> > > +		.type = REGULATOR_VOLTAGE,                            \
> > > +		.owner = THIS_MODULE,                                 \
> > > +		.n_voltages = (LTC2978_MAX_UV / LTC2978_UV_STEP) + 1, \
> > > +		.uV_step = LTC2978_UV_STEP,                           \
> > > +	}
> > > +
> > >  static const struct regulator_desc ltc2978_reg_desc[] = {
> > > +	PMBUS_LTC2978_REGULATOR("vout", 0),
> > > +	PMBUS_LTC2978_REGULATOR("vout", 1),
> > > +	PMBUS_LTC2978_REGULATOR("vout", 2),
> > > +	PMBUS_LTC2978_REGULATOR("vout", 3),
> > > +	PMBUS_LTC2978_REGULATOR("vout", 4),
> > > +	PMBUS_LTC2978_REGULATOR("vout", 5),
> > > +	PMBUS_LTC2978_REGULATOR("vout", 6),
> > > +	PMBUS_LTC2978_REGULATOR("vout", 7),
> > > +};
> > > +
> > > +static const struct regulator_desc ltc2978_reg_desc_default[] = {
> > >  	PMBUS_REGULATOR("vout", 0),
> > >  	PMBUS_REGULATOR("vout", 1),
> > >  	PMBUS_REGULATOR("vout", 2),
> > > @@ -839,10 +869,29 @@ static int ltc2978_probe(struct i2c_client *client)
> > >  
> > >  #if IS_ENABLED(CONFIG_SENSORS_LTC2978_REGULATOR)
> > >  	info->num_regulators = info->pages;
> > > -	info->reg_desc = ltc2978_reg_desc;
> > > -	if (info->num_regulators > ARRAY_SIZE(ltc2978_reg_desc)) {
> > > -		dev_err(&client->dev, "num_regulators too large!");
> > > -		info->num_regulators = ARRAY_SIZE(ltc2978_reg_desc);
> > > +	switch (data->id) {
> > > +	case ltc2972:
> > > +	case ltc2974:
> > > +	case ltc2975:
> > > +	case ltc2977:
> > > +	case ltc2978:
> > > +	case ltc2979:
> > > +	case ltc2980:
> > > +	case ltm2987:
> > > +		info->reg_desc = ltc2978_reg_desc;
> > > +		if (info->num_regulators > ARRAY_SIZE(ltc2978_reg_desc)) {
> > > +			dev_err(&client->dev, "num_regulators too large!");
> > 
> > Let's make this a dev_warn(); it does not result in an error abort,
> > after all.
> 
> Ok, will fix.
> 
> > 
> > > +			info->num_regulators = ARRAY_SIZE(ltc2978_reg_desc);
> > > +		}
> > > +		break;
> > > +	default:
> > > +		info->reg_desc = ltc2978_reg_desc_default;
> > > +		if (info->num_regulators > ARRAY_SIZE(ltc2978_reg_desc_default)) {
> > > +			dev_err(&client->dev, "num_regulators too large!");
> > 
> > Same here.
> 
> Ok, will fix.
> 
> Thanks!
> 
> Kind regards
> Mårten
> 
> > 
> > > +			info->num_regulators =
> > > +			    ARRAY_SIZE(ltc2978_reg_desc_default);
> > > +		}
> > > +		break;
> > >  	}
> > >  #endif
> > >  
> > > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> > > index f2cf0439da37..7d642b57c8b2 100644
> > > --- a/drivers/hwmon/pmbus/pmbus_core.c
> > > +++ b/drivers/hwmon/pmbus/pmbus_core.c
> > > @@ -2634,6 +2634,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 = regulator_list_voltage_linear,
> > >  };
> > >  EXPORT_SYMBOL_NS_GPL(pmbus_regulator_ops, PMBUS);
> > >