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

[Guenter Roeck <linux@roeck-us.net>]

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)

#define<space>DEFINE<tab>VALUE, please, and the () around 1000
is unnecessary.

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 ?

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.

> +			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.

> +			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);
>