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

[Marten Lindahl <martenli@axis.com>]

On Fri, Jun 10, 2022 at 04:27:55PM +0200, Guenter Roeck wrote:
> On 6/10/22 04:47, Mårten Lindahl wrote:
> > The LTC2977 regulator does not set the regulator_desc .n_voltages value
> > which is needed in order to let the regulator core list the regulator
> > voltage range.
> > 
> > This patch defines a regulator_desc with a voltage range, and uses it
> > for defining voltage resolution for regulators LTC2972/LTC2974/LTC2975/
> > LTC2977/LTC2978/LTC2979/LTC2980/LTM2987 based on that they all have a 16
> > bit ADC with the same stepwise 122.07uV resolution. It also scales the
> > resolution to a 1mV resolution which is easier to handle.
> > 
> > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> > ---
> >   drivers/hwmon/pmbus/ltc2978.c | 56 ++++++++++++++++++++++++++++++++---
> >   1 file changed, 52 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/hwmon/pmbus/ltc2978.c b/drivers/hwmon/pmbus/ltc2978.c
> > index 531aa674a928..7d44e64c61c1 100644
> > --- a/drivers/hwmon/pmbus/ltc2978.c
> > +++ b/drivers/hwmon/pmbus/ltc2978.c
> > @@ -562,7 +562,36 @@ 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 PMBUS_LTC2978_REGULATOR(_name, _id)               \
> > +	[_id] = {                                               \
> > +		.name = (_name # _id),                                \
> 
> This needs 'supply_name = "vin"'. See commit 54cc3dbfc10dc ("hwmon:
> (pmbus) Add regulator supply into macro").
> 
> > +		.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,                           \
> > +	}
> 
> Please introduce a new generic macro PMBUS_REGULATOR_STEP()
> with additional parameters n_voltages and uV_step and use it here.
> PMBUS_REGULATOR() can then be redefined as
> 
> #define PMBUS_REGULATOR(n, i)	PMBUS_REGULATOR_STEP(n, i, 0, 0)

Hi Guenter!

Yes, that's a good idea. I will do that, and then there wont be any risk
of missing attributes like this new 'supply_name' as it is only defined at
one place.

Kind regards
Mårten

> 
> Thanks,
> Guenter
> 
> > +
> >   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 +868,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_warn(&client->dev, "num_regulators too large!");
> > +			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_warn(&client->dev, "num_regulators too large!");
> > +			info->num_regulators =
> > +			    ARRAY_SIZE(ltc2978_reg_desc_default);
> > +		}
> > +		break;
> >   	}
> >   #endif
> >   
>