Re: [RFC PATCH 3/4] pmbus: Allow dynamic fan coefficient values

[Andrew Jeffery]

On Tue, 2017-07-11 at 06:31 -0700, Guenter Roeck wrote:
> On 07/10/2017 06:56 AM, Andrew Jeffery wrote:
> > Some PMBus chips, such as the MAX31785, use different coefficients for
> > FAN_COMMAND_[1-4] depending on whether the fan is in PWM (percent duty)
> > or RPM mode. Add a callback to allow the driver to provide the
> > applicable coefficients to avoid imposing on devices that don't have
> > this requirement.
> > 
> 
> Why not just introduce another class, such as PSC_PWM ?

I considered this up front however I wasn't sure where the PMBus sensor
classes were modelled from. The PMBus spec generally doesn't discuss
PMW beyond the concept of fans, and given PSC_FAN already existed I had
a vague feeling that introducing PSC_PWM might not be the way to go. It
also means that PSC_FAN is implicitly RPM in some circumstances, or
both RPM and PWM in others, and wasn't previously contrasted against
PWM as PWM-specific configuration wasn't an option.

However if it's reasonable it should lead to a more straight forward
patch. I'll rework and resend if it falls out nicely.

Thanks,

Andrew

> 
> > > > Signed-off-by: Andrew Jeffery <andrew@xxxxxxxx>
> > ---
> > Â drivers/hwmon/pmbus/pmbus.hÂÂÂÂÂÂ|ÂÂ18 +++++--
> > Â drivers/hwmon/pmbus/pmbus_core.c | 112 ++++++++++++++++++++++++++++++++-------
> > Â 2 files changed, 108 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> > index 927eabc1b273..338ecc8b25a4 100644
> > --- a/drivers/hwmon/pmbus/pmbus.h
> > +++ b/drivers/hwmon/pmbus/pmbus.h
> > @@ -345,6 +345,12 @@ enum pmbus_sensor_classes {
> > Â enum pmbus_data_format { linear = 0, direct, vid };
> > Â enum vrm_version { vr11 = 0, vr12 };
> > ÂÂ
> > +struct pmbus_coeffs {
> > > > +	int m; /* mantissa for direct data format */
> > > > +	int b; /* offset */
> > > > +	int R; /* exponent */
> > +};
> > +
> > Â struct pmbus_driver_info {
> > > > > > ÂÂ	int pages;		/* Total number of pages */
> > > > ÂÂ	enum pmbus_data_format format[PSC_NUM_CLASSES];
> > @@ -353,9 +359,7 @@ struct pmbus_driver_info {
> > > > ÂÂ	Â* Support one set of coefficients for each sensor type
> > > > ÂÂ	Â* Used for chips providing data in direct mode.
> > > > ÂÂ	Â*/
> > > > > > -	int m[PSC_NUM_CLASSES];	/* mantissa for direct data format */
> > > > > > -	int b[PSC_NUM_CLASSES];	/* offset */
> > > > > > -	int R[PSC_NUM_CLASSES];	/* exponent */
> > > > +	struct pmbus_coeffs coeffs[PSC_NUM_CLASSES];
> > ÂÂ
> > > > > > ÂÂ	u32 func[PMBUS_PAGES];	/* Functionality, per page */
> > > > ÂÂ	/*
> > @@ -382,6 +386,14 @@ struct pmbus_driver_info {
> > > > ÂÂ	int (*identify)(struct i2c_client *client,
> > > > ÂÂ			struct pmbus_driver_info *info);
> > ÂÂ
> > > > +	/*
> > > > +	Â* If a fan's coefficents change over time (e.g. between RPM and PWM
> > > > +	Â* mode), then the driver can provide a function for retrieving the
> > > > +	Â* currently applicable coefficients.
> > > > +	Â*/
> > > > +	const struct pmbus_coeffs *(*get_fan_coeffs)(
> > > > +			const struct pmbus_driver_info *info, int index,
> > > > +			enum pmbus_fan_mode mode, int command);
> > > > ÂÂ	/* Allow the driver to interpret the fan command value */
> > > > ÂÂ	int (*get_pwm_mode)(int id, u8 fan_config, u16 fan_command);
> > > > ÂÂ	int (*set_pwm_mode)(int id, long mode, u8 *fan_config,
> > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> > index 3b0a55bbbd2c..4ff6a1fd5cce 100644
> > --- a/drivers/hwmon/pmbus/pmbus_core.c
> > +++ b/drivers/hwmon/pmbus/pmbus_core.c
> > @@ -58,10 +58,11 @@
> > Â struct pmbus_sensor {
> > > > ÂÂ	struct pmbus_sensor *next;
> > > > > > ÂÂ	char name[PMBUS_NAME_SIZE];	/* sysfs sensor name */
> > > > -	struct device_attribute attribute;
> > > > +	struct sensor_device_attribute attribute;
> > > > > > ÂÂ	u8 page;		/* page number */
> > > > > > ÂÂ	u16 reg;		/* register */
> > > > > > ÂÂ	enum pmbus_sensor_classes class;	/* sensor class */
> > > > +	const struct pmbus_coeffs *coeffs;
> > > > > > ÂÂ	bool update;		/* runtime sensor update needed */
> > > > > > ÂÂ	int data;		/* Sensor data.
> > > > ÂÂ				ÂÂÂNegative if there was a read error */
> > @@ -89,6 +90,7 @@ struct pmbus_fan_ctrl {
> > > > ÂÂ	u8 id;
> > > > ÂÂ	u8 config;
> > > > ÂÂ	u16 command;
> > > > +	const struct pmbus_coeffs *coeffs;
> > Â };
> > Â #define to_pmbus_fan_ctrl_attr(_attr) \
> > > > ÂÂ	container_of(_attr, struct pmbus_fan_ctrl_attr, attribute)
> > @@ -511,9 +513,15 @@ static long pmbus_reg2data_direct(struct pmbus_data *data,
> > > > ÂÂ	long val = (s16) sensor->data;
> > > > ÂÂ	long m, b, R;
> > ÂÂ
> > > > -	m = data->info->m[sensor->class];
> > > > -	b = data->info->b[sensor->class];
> > > > -	R = data->info->R[sensor->class];
> > > > +	if (sensor->coeffs) {
> > > > +		m = sensor->coeffs->m;
> > > > +		b = sensor->coeffs->b;
> > > > +		R = sensor->coeffs->R;
> > > > +	} else {
> > > > +		m = data->info->coeffs[sensor->class].m;
> > > > +		b = data->info->coeffs[sensor->class].b;
> > > > +		R = data->info->coeffs[sensor->class].R;
> > > > +	}
> > ÂÂ
> > > > ÂÂ	if (m == 0)
> > > > ÂÂ		return 0;
> > @@ -663,9 +671,15 @@ static u16 pmbus_data2reg_direct(struct pmbus_data *data,
> > Â {
> > > > ÂÂ	long m, b, R;
> > ÂÂ
> > > > -	m = data->info->m[sensor->class];
> > > > -	b = data->info->b[sensor->class];
> > > > -	R = data->info->R[sensor->class];
> > > > +	if (sensor->coeffs) {
> > > > +		m = sensor->coeffs->m;
> > > > +		b = sensor->coeffs->b;
> > > > +		R = sensor->coeffs->R;
> > > > +	} else {
> > > > +		m = data->info->coeffs[sensor->class].m;
> > > > +		b = data->info->coeffs[sensor->class].b;
> > > > +		R = data->info->coeffs[sensor->class].R;
> > > > +	}
> > ÂÂ
> > > > ÂÂ	/* Power is in uW. Adjust R and b. */
> > > > ÂÂ	if (sensor->class == PSC_POWER) {
> > @@ -796,7 +810,9 @@ static ssize_t pmbus_show_sensor(struct device *dev,
> > > > ÂÂ				Âstruct device_attribute *devattr, char *buf)
> > Â {
> > > > ÂÂ	struct pmbus_data *data = pmbus_update_device(dev);
> > > > -	struct pmbus_sensor *sensor = to_pmbus_sensor(devattr);
> > > > +	struct pmbus_sensor *sensor;
> > +
> > > > +	sensor = to_pmbus_sensor(to_sensor_dev_attr(devattr));
> > ÂÂ
> > > > ÂÂ	if (sensor->data < 0)
> > > > ÂÂ		return sensor->data;
> > @@ -810,12 +826,14 @@ static ssize_t pmbus_set_sensor(struct device *dev,
> > Â {
> > > > ÂÂ	struct i2c_client *client = to_i2c_client(dev->parent);
> > > > ÂÂ	struct pmbus_data *data = i2c_get_clientdata(client);
> > > > -	struct pmbus_sensor *sensor = to_pmbus_sensor(devattr);
> > > > +	struct pmbus_sensor *sensor;
> > > > ÂÂ	ssize_t rv = count;
> > > > ÂÂ	long val = 0;
> > > > ÂÂ	int ret;
> > > > ÂÂ	u16 regval;
> > ÂÂ
> > > > +	sensor = to_pmbus_sensor(to_sensor_dev_attr(devattr));
> > +
> > > > ÂÂ	if (kstrtol(buf, 10, &val) < 0)
> > > > ÂÂ		return -EINVAL;
> > ÂÂ
> > @@ -856,6 +874,7 @@ static ssize_t pmbus_show_fan_command(struct device *dev,
> > > > ÂÂ	}
> > ÂÂ
> > > > ÂÂ	sensor.class = PSC_FAN;
> > > > +	sensor.coeffs = fan->coeffs;
> > > > ÂÂ	if (mode == percent)
> > > > ÂÂ		sensor.data = fan->command * 255 / 100;
> > > > ÂÂ	else
> > @@ -882,6 +901,29 @@ static ssize_t pmbus_show_pwm(struct device *dev,
> > > > ÂÂ				ÂÂÂÂÂÂbuf);
> > Â }
> > ÂÂ
> > +static int pmbus_update_fan_coeffs(struct pmbus_data *data,
> > > > +				ÂÂÂstruct pmbus_fan_ctrl *fan,
> > > > +				ÂÂÂenum pmbus_fan_mode mode)
> > +{
> > > > +	const struct pmbus_driver_info *info = data->info;
> > > > +	struct pmbus_sensor *curr = data->sensors;
> > +
> > > > +	fan->coeffs = info->get_fan_coeffs(info, fan->index, mode,
> > > > +					ÂÂÂPMBUS_FAN_COMMAND_1 + fan->id);
> > +
> > > > +	while (curr) {
> > > > +		if (curr->class == PSC_FAN &&
> > > > +				curr->attribute.index == fan->index) {
> > > > +			curr->coeffs = info->get_fan_coeffs(info, fan->index,
> > > > +							ÂÂÂÂmode, curr->reg);
> > > > +		}
> > +
> > > > +		curr = curr->next;
> > > > +	}
> > +
> > > > +	return 0;
> > +}
> > +
> > Â static ssize_t pmbus_set_fan_command(struct device *dev,
> > > > ÂÂ				ÂÂÂÂÂenum pmbus_fan_mode mode,
> > > > ÂÂ				ÂÂÂÂÂstruct pmbus_fan_ctrl *fan,
> > @@ -889,6 +931,7 @@ static ssize_t pmbus_set_fan_command(struct device *dev,
> > Â {
> > > > ÂÂ	struct i2c_client *client = to_i2c_client(dev->parent);
> > > > ÂÂ	struct pmbus_data *data = i2c_get_clientdata(client);
> > > > +	const struct pmbus_driver_info *info = data->info;
> > > > ÂÂ	int config_addr, command_addr;
> > > > ÂÂ	struct pmbus_sensor sensor;
> > > > ÂÂ	ssize_t rv;
> > @@ -899,7 +942,13 @@ static ssize_t pmbus_set_fan_command(struct device *dev,
> > ÂÂ
> > > > ÂÂ	mutex_lock(&data->update_lock);
> > ÂÂ
> > > > +	if (info->format[PSC_FAN] == direct && info->get_fan_coeffs) {
> > > > +		pmbus_update_fan_coeffs(data, fan, mode);
> > > > +		sensor.coeffs = fan->coeffs;
> > > > +	}
> > +
> > > > ÂÂ	sensor.class = PSC_FAN;
> > > > +	sensor.attribute.index = fan->index;
> > ÂÂ
> > > > ÂÂ	val = pmbus_data2reg(data, &sensor, val);
> > ÂÂ
> > @@ -968,6 +1017,8 @@ static ssize_t pmbus_show_pwm_enable(struct device *dev,
> > > > ÂÂ		struct pmbus_sensor sensor = {
> > > > ÂÂ			.class = PSC_FAN,
> > > > ÂÂ			.data = fan->command,
> > > > +			.attribute.index = fan->index,
> > > > +			.coeffs = fan->coeffs,
> > > > ÂÂ		};
> > > > ÂÂ		long command;
> > ÂÂ
> > @@ -992,6 +1043,7 @@ static ssize_t pmbus_set_pwm_enable(struct device *dev,
> > > > ÂÂ	struct pmbus_fan_ctrl *fan = pwm_enable_to_pmbus_fan_ctrl(da);
> > > > ÂÂ	struct i2c_client *client = to_i2c_client(dev->parent);
> > > > ÂÂ	struct pmbus_data *data = i2c_get_clientdata(client);
> > > > +	const struct pmbus_driver_info *info = data->info;
> > > > ÂÂ	int config_addr, command_addr;
> > > > ÂÂ	struct pmbus_sensor sensor;
> > > > ÂÂ	ssize_t rv = count;
> > @@ -1003,15 +1055,21 @@ static ssize_t pmbus_set_pwm_enable(struct device *dev,
> > > > ÂÂ	mutex_lock(&data->update_lock);
> > ÂÂ
> > > > ÂÂ	sensor.class = PSC_FAN;
> > > > +	sensor.attribute.index = fan->index;
> > +
> > > > +	if (info->format[PSC_FAN] == direct && info->get_fan_coeffs) {
> > > > +		pmbus_update_fan_coeffs(data, fan, percent);
> > > > +		sensor.coeffs = fan->coeffs;
> > > > +	}
> > ÂÂ
> > > > ÂÂ	config_addr = (fan->id < 2) ? PMBUS_FAN_CONFIG_12 : PMBUS_FAN_CONFIG_34;
> > > > ÂÂ	command_addr = config_addr + 1 + (fan->id & 1);
> > ÂÂ
> > > > -	if (data->info->set_pwm_mode) {
> > > > +	if (info->set_pwm_mode) {
> > > > ÂÂ		u8 config = PB_FAN_CONFIG_PUT(fan->id, fan->config);
> > > > ÂÂ		u16 command = fan->command;
> > ÂÂ
> > > > -		rv = data->info->set_pwm_mode(fan->id, mode, &config, &command);
> > > > +		rv = info->set_pwm_mode(fan->id, mode, &config, &command);
> > > > ÂÂ		if (rv < 0)
> > > > ÂÂ			goto done;
> > ÂÂ
> > @@ -1138,7 +1196,7 @@ static struct pmbus_sensor *pmbus_add_sensor(struct pmbus_data *data,
> > > > ÂÂ	sensor = devm_kzalloc(data->dev, sizeof(*sensor), GFP_KERNEL);
> > > > ÂÂ	if (!sensor)
> > > > ÂÂ		return NULL;
> > > > -	a = &sensor->attribute;
> > > > +	a = &sensor->attribute.dev_attr;
> > ÂÂ
> > > > ÂÂ	snprintf(sensor->name, sizeof(sensor->name), "%s%d_%s",
> > > > ÂÂ		Âname, seq, type);
> > @@ -1146,9 +1204,9 @@ static struct pmbus_sensor *pmbus_add_sensor(struct pmbus_data *data,
> > > > ÂÂ	sensor->reg = reg;
> > > > ÂÂ	sensor->class = class;
> > > > ÂÂ	sensor->update = update;
> > > > -	pmbus_dev_attr_init(a, sensor->name,
> > > > +	pmbus_attr_init(&sensor->attribute, sensor->name,
> > > > ÂÂ			ÂÂÂÂreadonly ? S_IRUGO : S_IRUGO | S_IWUSR,
> > > > -			ÂÂÂÂpmbus_show_sensor, pmbus_set_sensor);
> > > > +			ÂÂÂÂpmbus_show_sensor, pmbus_set_sensor, seq);
> > ÂÂ
> > > > ÂÂ	if (pmbus_add_attribute(data, &a->attr))
> > > > ÂÂ		return NULL;
> > @@ -1886,7 +1944,7 @@ static const u32 pmbus_fan_status_flags[] = {
> > Â /* Fans */
> > Â static int pmbus_add_fan_ctrl(struct i2c_client *client,
> > > > ÂÂ		struct pmbus_data *data, int index, int page, int id,
> > > > -		u8 config)
> > > > +		u8 config, const struct pmbus_coeffs *coeffs)
> > Â {
> > > > ÂÂ	struct pmbus_fan_ctrl *fan;
> > > > ÂÂ	int rv;
> > @@ -1898,6 +1956,7 @@ static int pmbus_add_fan_ctrl(struct i2c_client *client,
> > > > ÂÂ	fan->index = index;
> > > > ÂÂ	fan->page = page;
> > > > ÂÂ	fan->id = id;
> > > > +	fan->coeffs = coeffs;
> > > > ÂÂ	fan->config = config;
> > ÂÂ
> > > > ÂÂ	rv = _pmbus_read_word_data(client, page,
> > @@ -1921,6 +1980,8 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
> > > > ÂÂ				ÂÂÂÂstruct pmbus_data *data)
> > Â {
> > > > ÂÂ	const struct pmbus_driver_info *info = data->info;
> > > > +	const struct pmbus_coeffs *coeffs = NULL;
> > > > +	enum pmbus_fan_mode mode;
> > > > ÂÂ	int index = 1;
> > > > ÂÂ	int page;
> > > > ÂÂ	int ret;
> > @@ -1929,6 +1990,7 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
> > > > ÂÂ		int f;
> > ÂÂ
> > > > ÂÂ		for (f = 0; f < ARRAY_SIZE(pmbus_fan_registers); f++) {
> > > > +			struct pmbus_sensor *sensor;
> > > > ÂÂ			int regval;
> > ÂÂ
> > > > ÂÂ			if (!(info->func[page] & pmbus_fan_flags[f]))
> > @@ -1949,13 +2011,25 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
> > > > ÂÂ			ÂÂÂÂ(!(regval & (PB_FAN_1_INSTALLED >> ((f & 1) * 4)))))
> > > > ÂÂ				continue;
> > ÂÂ
> > > > -			if (pmbus_add_sensor(data, "fan", "input", index,
> > > > -					ÂÂÂÂÂpage, pmbus_fan_registers[f],
> > > > -					ÂÂÂÂÂPSC_FAN, true, true) == NULL)
> > > > +			sensor = pmbus_add_sensor(data, "fan", "input", index,
> > > > +						ÂÂpage, pmbus_fan_registers[f],
> > > > +						ÂÂPSC_FAN, true, true);
> > > > +			if (!sensor)
> > > > ÂÂ				return -ENOMEM;
> > ÂÂ
> > > > +			/* Add coeffs here as we have access to the fan mode */
> > > > +			if (info->format[PSC_FAN] == direct &&
> > > > +					info->get_fan_coeffs) {
> > > > +				const u16 mask = PB_FAN_1_RPM >> ((f & 1) * 4);
> > +
> > > > +				mode = (regval & mask) ? rpm : percent;
> > > > +				coeffs = info->get_fan_coeffs(info, index, mode,
> > > > +							pmbus_fan_registers[f]);
> > > > +				sensor->coeffs = coeffs;
> > > > +			}
> > +
> > > > ÂÂ			ret = pmbus_add_fan_ctrl(client, data, index, page, f,
> > > > -						Âregval);
> > > > +						Âregval, coeffs);
> > > > ÂÂ			if (ret < 0)
> > > > ÂÂ				return ret;
> > ÂÂ
> > 
> 
>

Attachment: signature.asc
Description: This is a digitally signed message part