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

Andrew Jeffery andrew at aj.id.au
Wed Jul 12 11:20:12 AEST 2017


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 at aj.id.au>
> > ---
> >   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;
> >   
> > 
> 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20170712/cade83df/attachment-0001.sig>


More information about the openbmc mailing list