Message-ID: <a34778a6-088e-d4df-c063-db394baa3153@roeck-us.net>
Date:   Fri, 10 Nov 2017 00:24:21 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Andrew Jeffery <andrew@...id.au>
Cc:     linux-hwmon@...r.kernel.org, robh+dt@...nel.org,
        mark.rutland@....com, jdelvare@...e.com, corbet@....net,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-doc@...r.kernel.org, joel@....id.au, openbmc@...ts.ozlabs.org
Subject: Re: [v4,3/6] pmbus: core: Add fan control support

On 11/09/2017 07:03 PM, Andrew Jeffery wrote:
> On Sun, 2017-11-05 at 06:39 -0800, Guenter Roeck wrote:
>> On Fri, Nov 03, 2017 at 03:53:03PM +1100, Andrew Jeffery wrote:
>>> Expose fanX_target, pwmX and pwmX_enable hwmon sysfs attributes.
>>>
>>> Fans in a PMBus device are driven by the configuration of two registers:
>>> FAN_CONFIG_x_y and FAN_COMMAND_x: FAN_CONFIG_x_y dictates how the fan
>>> and the tacho operate (if installed), while FAN_COMMAND_x sets the
>>> desired fan rate. The unit of FAN_COMMAND_x is dependent on the
>>> operational fan mode, RPM or PWM percent duty, as determined by the
>>> corresponding FAN_CONFIG_x_y.
>>>
>>> The mapping of fanX_target, pwmX and pwmX_enable onto FAN_CONFIG_x_y and
>>> FAN_COMMAND_x is implemented with the addition of virtual registers and
>>> generic implementations in the core:
>>>
>>> 1. PMBUS_VIRT_FAN_TARGET_x
>>> 2. PMBUS_VIRT_PWM_x
>>> 3. PMBUS_VIRT_PWM_ENABLE_x
>>>
>>> The virtual registers facilitate the necessary side-effects of each
>>> access. Examples of the read case, assuming m = 1, b = 0, R = 0:
>>>
>>>               Read     |              With              || Gives
>>>           -------------------------------------------------------
>>>             Attribute  | FAN_CONFIG_x_y | FAN_COMMAND_y || Value
>>>           ----------------------------------------------++-------
>>>            fanX_target | ~PB_FAN_z_RPM  | 0x0001        || 1
>>>            pwm1        | ~PB_FAN_z_RPM  | 0x0064        || 255
>>>            pwmX_enable | ~PB_FAN_z_RPM  | 0x0001        || 1
>>>            fanX_target |  PB_FAN_z_RPM  | 0x0001        || 1
>>>            pwm1        |  PB_FAN_z_RPM  | 0x0064        || 0
>>>            pwmX_enable |  PB_FAN_z_RPM  | 0x0001        || 1
>>>
>>> And the write case:
>>>
>>>               Write    | With  ||               Sets
>>>           -------------+-------++----------------+---------------
>>>             Attribute  | Value || FAN_CONFIG_x_y | FAN_COMMAND_x
>>>           -------------+-------++----------------+---------------
>>>            fanX_target | 1     ||  PB_FAN_z_RPM  | 0x0001
>>>            pwmX        | 255   || ~PB_FAN_z_RPM  | 0x0064
>>>            pwmX_enable | 1     || ~PB_FAN_z_RPM  | 0x0064
>>>
>>> Also, the DIRECT mode scaling of some controllers is different between
>>> RPM and PWM percent duty control modes, so PSC_PWM is introduced to
>>> capture the necessary coefficients.
>>>
>>> Signed-off-by: Andrew Jeffery <andrew@...id.au>
>>> ---
>>>   drivers/hwmon/pmbus/pmbus.h      |  29 +++++
>>>   drivers/hwmon/pmbus/pmbus_core.c | 224 ++++++++++++++++++++++++++++++++++++---
>>>   2 files changed, 238 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
>>> index 4efa2bd4f6d8..cdf3e288e626 100644
>>> --- a/drivers/hwmon/pmbus/pmbus.h
>>> +++ b/drivers/hwmon/pmbus/pmbus.h
>>> @@ -190,6 +190,28 @@ enum pmbus_regs {
>>>   	PMBUS_VIRT_VMON_UV_FAULT_LIMIT,
>>>   	PMBUS_VIRT_VMON_OV_FAULT_LIMIT,
>>>   	PMBUS_VIRT_STATUS_VMON,
>>> +
>>> +	/*
>>> +	 * RPM and PWM Fan control
>>> +	 *
>>> +	 * Drivers wanting to expose PWM control must define the behaviour of
>>> +	 * PMBUS_VIRT_PWM_ENABLE_[1-4] in the {read,write}_word_data callback.
>>> +	 *
>>> +	 * pmbus core provides default implementations for
>>> +	 * PMBUS_VIRT_FAN_TARGET_[1-4] and PMBUS_VIRT_PWM_[1-4].
>>> +	 */
>>> +	PMBUS_VIRT_FAN_TARGET_1,
>>> +	PMBUS_VIRT_FAN_TARGET_2,
>>> +	PMBUS_VIRT_FAN_TARGET_3,
>>> +	PMBUS_VIRT_FAN_TARGET_4,
>>> +	PMBUS_VIRT_PWM_1,
>>> +	PMBUS_VIRT_PWM_2,
>>> +	PMBUS_VIRT_PWM_3,
>>> +	PMBUS_VIRT_PWM_4,
>>> +	PMBUS_VIRT_PWM_ENABLE_1,
>>> +	PMBUS_VIRT_PWM_ENABLE_2,
>>> +	PMBUS_VIRT_PWM_ENABLE_3,
>>> +	PMBUS_VIRT_PWM_ENABLE_4,
>>>   };
>>>   
>>>   /*
>>> @@ -223,6 +245,8 @@ enum pmbus_regs {
>>>   #define PB_FAN_1_RPM			BIT(6)
>>>   #define PB_FAN_1_INSTALLED		BIT(7)
>>>   
>>> +enum pmbus_fan_mode { percent = 0, rpm };
>>> +
>>>   /*
>>>    * STATUS_BYTE, STATUS_WORD (lower)
>>>    */
>>> @@ -313,6 +337,7 @@ enum pmbus_sensor_classes {
>>>   	PSC_POWER,
>>>   	PSC_TEMPERATURE,
>>>   	PSC_FAN,
>>> +	PSC_PWM,
>>>   	PSC_NUM_CLASSES		/* Number of power sensor classes */
>>>   };
>>>   
>>> @@ -339,6 +364,8 @@ enum pmbus_sensor_classes {
>>>   #define PMBUS_HAVE_STATUS_FAN34	BIT(17)
>>>   #define PMBUS_HAVE_VMON		BIT(18)
>>>   #define PMBUS_HAVE_STATUS_VMON	BIT(19)
>>> +#define PMBUS_HAVE_PWM12	BIT(20)
>>> +#define PMBUS_HAVE_PWM34	BIT(21)
>>>   
>>>   enum pmbus_data_format { linear = 0, direct, vid };
>>>   enum vrm_version { vr11 = 0, vr12, vr13 };
>>> @@ -413,6 +440,8 @@ int pmbus_write_byte_data(struct i2c_client *client, int page, u8 reg,
>>>   			  u8 value);
>>>   int pmbus_update_byte_data(struct i2c_client *client, int page, u8 reg,
>>>   			   u8 mask, u8 value);
>>> +int pmbus_update_fan(struct i2c_client *client, int page, int id,
>>> +		     u8 config, u8 mask, u16 command);
>>>   void pmbus_clear_faults(struct i2c_client *client);
>>>   bool pmbus_check_byte_register(struct i2c_client *client, int page, int reg);
>>>   bool pmbus_check_word_register(struct i2c_client *client, int page, int reg);
>>> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
>>> index 302f0aef59de..55838b69e99a 100644
>>> --- a/drivers/hwmon/pmbus/pmbus_core.c
>>> +++ b/drivers/hwmon/pmbus/pmbus_core.c
>>> @@ -64,6 +64,7 @@ struct pmbus_sensor {
>>>   	u16 reg;		/* register */
>>>   	enum pmbus_sensor_classes class;	/* sensor class */
>>>   	bool update;		/* runtime sensor update needed */
>>> +	bool convert;		/* Whether or not to apply linear/vid/direct */
>>>   	int data;		/* Sensor data.
>>>   				   Negative if there was a read error */
>>>   };
>>> @@ -128,6 +129,27 @@ struct pmbus_debugfs_entry {
>>>   	u8 reg;
>>>   };
>>>   
>>> +static const int pmbus_fan_rpm_mask[] = {
>>> +	PB_FAN_1_RPM,
>>> +	PB_FAN_2_RPM,
>>> +	PB_FAN_1_RPM,
>>> +	PB_FAN_2_RPM,
>>> +};
>>> +
>>> +static const int pmbus_fan_config_registers[] = {
>>> +	PMBUS_FAN_CONFIG_12,
>>> +	PMBUS_FAN_CONFIG_12,
>>> +	PMBUS_FAN_CONFIG_34,
>>> +	PMBUS_FAN_CONFIG_34
>>> +};
>>> +
>>> +static const int pmbus_fan_command_registers[] = {
>>> +	PMBUS_FAN_COMMAND_1,
>>> +	PMBUS_FAN_COMMAND_2,
>>> +	PMBUS_FAN_COMMAND_3,
>>> +	PMBUS_FAN_COMMAND_4,
>>> +};
>>> +
>>>   void pmbus_clear_cache(struct i2c_client *client)
>>>   {
>>>   	struct pmbus_data *data = i2c_get_clientdata(client);
>>> @@ -198,6 +220,31 @@ int pmbus_write_word_data(struct i2c_client *client, u8 page, u8 reg, u16 word)
>>>   }
>>>   EXPORT_SYMBOL_GPL(pmbus_write_word_data);
>>>   
>>> +int pmbus_update_fan(struct i2c_client *client, int page, int id,
>>> +			       u8 config, u8 mask, u16 command)
>>
>> Please make sure continuation lines are aligned to '(' where possible.
> 
> Yep, sorry about that.
> 
>>
>>> +{
>>> +	int from, rv;
>>> +	u8 to;
>>> +
>>> +	from = pmbus_read_byte_data(client, page,
>>> +				    pmbus_fan_config_registers[id]);
>>> +	if (from < 0)
>>> +		return from;
>>> +
>>> +	to = (from & ~mask) | (config & mask);
>>> +
>>> +	if (to != from) {
>>> +		rv = pmbus_write_byte_data(client, page,
>>> +					   pmbus_fan_config_registers[id], to);
>>> +		if (rv < 0)
>>> +			return rv;
>>> +	}
>>> +
>>> +	return pmbus_write_word_data(client, page,
>>> +				     pmbus_fan_command_registers[id], command);
>>> +}
>>> +EXPORT_SYMBOL_GPL(pmbus_update_fan);
>>> +
>>>   /*
>>>    * _pmbus_write_word_data() is similar to pmbus_write_word_data(), but checks if
>>>    * a device specific mapping function exists and calls it if necessary.
>>> @@ -214,8 +261,40 @@ static int _pmbus_write_word_data(struct i2c_client *client, int page, int reg,
>>>   		if (status != -ENODATA)
>>>   			return status;
>>>   	}
>>> -	if (reg >= PMBUS_VIRT_BASE)
>>> -		return -ENXIO;
>>> +	if (reg >= PMBUS_VIRT_BASE) {
>>> +		int id, bit;
>>> +
>>> +		switch (reg) {
>>> +		case PMBUS_VIRT_FAN_TARGET_1:
>>> +		case PMBUS_VIRT_FAN_TARGET_2:
>>> +		case PMBUS_VIRT_FAN_TARGET_3:
>>> +		case PMBUS_VIRT_FAN_TARGET_4:
>>
>> Maybe
>> 		case PMBUS_VIRT_FAN_TARGET_1 ... PMBUS_VIRT_FAN_TARGET_4:
> 
> Sure. I'll fix all instances of this.
> 
>>
>>> +			id = reg - PMBUS_VIRT_FAN_TARGET_1;
>>> +			bit = pmbus_fan_rpm_mask[id];
>>> +			status = pmbus_update_fan(client, page, id, bit, bit,
>>> +						  word);
>>> +			break;
>>> +		case PMBUS_VIRT_PWM_1:
>>> +		case PMBUS_VIRT_PWM_2:
>>> +		case PMBUS_VIRT_PWM_3:
>>> +		case PMBUS_VIRT_PWM_4:
>>> +		{
>>> +			u32 command = word;
>>
>> Please move command up and drop the {.
> 
> My default is to scope variables to where they are needed, but will
> fix.
> 
I am fine with that and do it as well as long as it is in loops or if statements.
In case statements it becomes awkward. Of course, that is all POV.

>>
>> Why u32 ? The value should be bound to 0..255 (if it isn't and a larger
>> value is accepted we have overflow issues).
> 
> It gets scaled below, though we still only need u16 if the max value is
> 255. I'll fix it.
> 
Good point. u32 is better than u16 - it results in less code on many architectures.

>>
>>> +
>>> +			id = reg - PMBUS_VIRT_PWM_1;
>>> +			bit = pmbus_fan_rpm_mask[id];
>>> +			command *= 100;
>>> +			command /= 255;
>>> +			status = pmbus_update_fan(client, page, id, 0, bit,
>>> +						  command);
>>> +			break;
>>> +		}
>>> +		default:
>>> +			status = -ENXIO;
>>> +			break;
>>> +		}
>>> +		return status;
>>
>> Please move this code to a separate function.
> 
> Will do.
> 
>>
>>> +	}
>>>   	return pmbus_write_word_data(client, page, reg, word);
>>>   }
>>>   
>>> @@ -231,6 +310,9 @@ int pmbus_read_word_data(struct i2c_client *client, u8 page, u8 reg)
>>>   }
>>>   EXPORT_SYMBOL_GPL(pmbus_read_word_data);
>>>   
>>> +static int pmbus_get_fan_command(struct i2c_client *client, int page, int id,
>>> +				 enum pmbus_fan_mode mode);
>>> +
>>>   /*
>>>    * _pmbus_read_word_data() is similar to pmbus_read_word_data(), but checks if
>>>    * a device specific mapping function exists and calls it if necessary.
>>> @@ -246,8 +328,42 @@ static int _pmbus_read_word_data(struct i2c_client *client, int page, int reg)
>>>   		if (status != -ENODATA)
>>>   			return status;
>>>   	}
>>> -	if (reg >= PMBUS_VIRT_BASE)
>>> -		return -ENXIO;
>>> +	if (reg >= PMBUS_VIRT_BASE) {
>>> +		int id;
>>> +
>>> +		switch (reg) {
>>> +		case PMBUS_VIRT_FAN_TARGET_1:
>>> +		case PMBUS_VIRT_FAN_TARGET_2:
>>> +		case PMBUS_VIRT_FAN_TARGET_3:
>>> +		case PMBUS_VIRT_FAN_TARGET_4:
>>
>> Since there is an implied assumption that those are sequential,
>> how about the following ?
>>
>> 		case PMBUS_VIRT_FAN_TARGET_1 ... PMBUS_VIRT_FAN_TARGET_4:
> 
> Will fix all instances of this, as noted above.
> 
>>
>>> +			id = reg - PMBUS_VIRT_FAN_TARGET_1;
>>
>> This warrants a comment in the definition of PMBUS_VIRT_FAN_TARGET_X
>> and PMBUS_VIRT_PWM_X stating that the definitions must be in sequence.
> 
> Right, will add a comment to that effect.
> 
>>
>>> +			status = pmbus_get_fan_command(client, page, id, rpm);
>>> +			break;
>>> +		case PMBUS_VIRT_PWM_1:
>>> +		case PMBUS_VIRT_PWM_2:
>>> +		case PMBUS_VIRT_PWM_3:
>>> +		case PMBUS_VIRT_PWM_4:
>>
>> Same as above.
> 
> Ack.
> 
>>
>>> +		{
>>> +			int rv;
>>
>> Please move the declaration up and drop the {.
> 
> Ack.
> 
>>
>>> +
>>> +			id = reg - PMBUS_VIRT_PWM_1;
>>> +			rv = pmbus_get_fan_command(client, page, id, percent);
>>> +			if (rv < 0)
>>> +				return rv;
>>> +
>>
>> Is this guaranteed to be <= 100 ?
> 
> PMBus spec rev 1.2 part II says in 14.10 and 14.11:
> 
> The second part of the configuration tells the device whether the fan
> speed commands are in RPM or PWM duty cycle (in percent).
> 

Hmm, yes, sure, but can you trust the chip to follow the specification ?

> However, the MAX31785 accepts the ranges 0-0x2710, so allows fractions
> of a percent.
> 
Answering my own question ... apparently not.

> Not sure what my thoughts were here, it's probably best just to drop
> this default PWM implementation as well.
> 

I agree.

>>
>>> +			rv *= 255;
>>> +			rv /= 100;
>>> +
>>> +			status = rv;
>>> +			break;
>>> +		}
>>> +		default:
>>> +			status = -ENXIO;
>>> +			break;
>>> +		}
>>
>> Please move this code to a separate function.
> 
> Will do, though given I plan to gut the PWM implementation is it still
> so disruptive?
> 
Let's see how it looks like. If it does more than just call another function
it should be a separate function.

>>
>>> +
>>> +		return status;
>>> +	}
>>>   	return pmbus_read_word_data(client, page, reg);
>>>   }
>>>   
>>> @@ -314,6 +430,28 @@ static int _pmbus_read_byte_data(struct i2c_client *client, int page, int reg)
>>>   	return pmbus_read_byte_data(client, page, reg);
>>>   }
>>>   
>>> +static int pmbus_get_fan_command(struct i2c_client *client, int page, int id,
>>> +				 enum pmbus_fan_mode mode)
>>
>> Maybe better pmbus_get_fan_get_speed_rpm to clarify that we are not
>> really interested in the command register value but but in speed or rpm ?
> 
> Sounds reasonable on the surface. I'll look into it.
> 
>>
>>> +{
>>> +	int config;
>>> +
>>> +	config = _pmbus_read_byte_data(client, page,
>>> +				       pmbus_fan_config_registers[id]);
>>> +	if (config < 0)
>>> +		return config;
>>> +
>>> +	/*
>>> +	 * We can't meaningfully translate between PWM and RPM, so if the
>>> +	 * attribute mode (fan[1-*]_target is RPM, pwm[1-*] and pwm[1-*]_enable
>>> +	 * are PWM) doesn't match the hardware mode, then report 0 instead.
>>> +	 */
>>> +	if ((mode == rpm) != (!!(config & pmbus_fan_rpm_mask[id])))
>>> +		return 0;
>>
>> Please drop the unnecessary ().
> 
> Sure.
> 
>>
>> I am not too happy about this - the user has no means to specify pwm or
>> fan target speed _before_ changing the mode. It would be better to report
>> (and accept) cached values in that situation, and update the actual value
>> as the mode is changed.
> 
> I'm with you on the caching idea for switching modes, but do we want to
> report values that don't obviously reflect the fan rate to userspace?

0 doesn't reflect the rate either. Bad choices either way.

> In previous revisions I was returning an error here but you pointed out
> this would break sensors(1) and suggested I switch to 0 in the conflict
> case.
> 
>>
>>> +
>>> +	return _pmbus_read_word_data(client, page,
>>> +				     pmbus_fan_command_registers[id]);
>>> +}
>>> +
>>>   static void pmbus_clear_fault_page(struct i2c_client *client, int page)
>>>   {
>>>   	_pmbus_write_byte(client, page, PMBUS_CLEAR_FAULTS);
>>> @@ -515,7 +653,7 @@ static long pmbus_reg2data_direct(struct pmbus_data *data,
>>>   	/* X = 1/m * (Y * 10^-R - b) */
>>>   	R = -R;
>>>   	/* scale result to milli-units for everything but fans */
>>> -	if (sensor->class != PSC_FAN) {
>>> +	if (!(sensor->class == PSC_FAN || sensor->class == PSC_PWM)) {
>>
>> Please use positive logic
>>
>> 	if (sensor->class != PSC_FAN && sensor->class != PSC_PWM)
>>
>> It is (at least for me) much easier to read.
> 
> Okay.
> 
>>
>>>   		R += 3;
>>>   		b *= 1000;
>>>   	}
>>> @@ -569,6 +707,9 @@ static long pmbus_reg2data(struct pmbus_data *data, struct pmbus_sensor *sensor)
>>>   {
>>>   	long val;
>>>   
>>> +	if (!sensor->convert)
>>> +		return sensor->data;
>>> +
>>>   	switch (data->info->format[sensor->class]) {
>>>   	case direct:
>>>   		val = pmbus_reg2data_direct(data, sensor);
>>> @@ -672,7 +813,7 @@ static u16 pmbus_data2reg_direct(struct pmbus_data *data,
>>>   	}
>>>   
>>>   	/* Calculate Y = (m * X + b) * 10^R */
>>> -	if (sensor->class != PSC_FAN) {
>>> +	if (!(sensor->class == PSC_FAN || sensor->class == PSC_PWM)) {
>>
>> Same as above.
>>
> 
> Ack.
> 
>>>   		R -= 3;		/* Adjust R and b for data in milli-units */
>>>   		b *= 1000;
>>>   	}
>>> @@ -703,6 +844,9 @@ static u16 pmbus_data2reg(struct pmbus_data *data,
>>>   {
>>>   	u16 regval;
>>>   
>>> +	if (!sensor->convert)
>>> +		return val;
>>> +
>>>   	switch (data->info->format[sensor->class]) {
>>>   	case direct:
>>>   		regval = pmbus_data2reg_direct(data, sensor, val);
>>> @@ -925,12 +1069,18 @@ static struct pmbus_sensor *pmbus_add_sensor(struct pmbus_data *data,
>>>   		return NULL;
>>>   	a = &sensor->attribute;
>>>   
>>> -	snprintf(sensor->name, sizeof(sensor->name), "%s%d_%s",
>>> -		 name, seq, type);
>>> +	if (type)
>>> +		snprintf(sensor->name, sizeof(sensor->name), "%s%d_%s",
>>> +			 name, seq, type);
>>> +	else
>>> +		snprintf(sensor->name, sizeof(sensor->name), "%s%d",
>>> +			 name, seq);
>>> +
>>>   	sensor->page = page;
>>>   	sensor->reg = reg;
>>>   	sensor->class = class;
>>>   	sensor->update = update;
>>> +	sensor->convert = true;
>>>   	pmbus_dev_attr_init(a, sensor->name,
>>>   			    readonly ? S_IRUGO : S_IRUGO | S_IWUSR,
>>>   			    pmbus_show_sensor, pmbus_set_sensor);
>>> @@ -1592,13 +1742,6 @@ static const int pmbus_fan_registers[] = {
>>>   	PMBUS_READ_FAN_SPEED_4
>>>   };
>>>   
>>> -static const int pmbus_fan_config_registers[] = {
>>> -	PMBUS_FAN_CONFIG_12,
>>> -	PMBUS_FAN_CONFIG_12,
>>> -	PMBUS_FAN_CONFIG_34,
>>> -	PMBUS_FAN_CONFIG_34
>>> -};
>>> -
>>>   static const int pmbus_fan_status_registers[] = {
>>>   	PMBUS_STATUS_FAN_12,
>>>   	PMBUS_STATUS_FAN_12,
>>> @@ -1621,6 +1764,48 @@ 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)
>>> +{
>>> +	struct pmbus_sensor *sensor;
>>> +	int rv;
>>> +
>>> +	rv = _pmbus_read_word_data(client, page,
>>> +				   pmbus_fan_command_registers[id]);
>>> +	if (rv < 0)
>>> +		return rv;
>>> +
>>> +	sensor = pmbus_add_sensor(data, "fan", "target", index, page,
>>> +				  PMBUS_VIRT_FAN_TARGET_1 + id, PSC_FAN,
>>> +				  true, false);
>>> +
>>> +	if (!sensor)
>>> +		return -ENOMEM;
>>> +
>>> +	if (!((data->info->func[page] & PMBUS_HAVE_PWM12) ||
>>> +			(data->info->func[page] & PMBUS_HAVE_PWM34)))
>>
>> why not just the following ?
>>
>> 	if (!(data->info->func[page] & (PMBUS_HAVE_PWM12 | PMBUS_HAVE_PWM34)))
> 
> Sure, that's much neater.
> 
>>
>> Also, doesn't this add attributes for 1,2 even if only 3,4 are
>> supported, and 3,4 even if only 1,2 are supported ?
> 
> It shouldn't, but that's due to the way I've called
> pmbus_add_fan_ctrl() in pmbus_add_fan_attributes().
> 
> The call is guarded with:
> 
>      /* Fan control */
>      if (pmbus_check_word_register(client, page,
>      		    pmbus_fan_command_registers[f])) {
>      	    ret = pmbus_add_fan_ctrl(client, data, index,
>      	    	    	    	     page, f, regval);
>      	    if (ret < 0)
>      	    	    return ret;
>      }
> 
> So 'f', or the formal parameter 'id', is only ever provided as
> appropriate, and together the two tests should ensure that only the
> appropriate attributes are created.
> 
> If the fan is marked as "not installed" then the addition of the
> attribute is skipped. This code was already in place to handle the
> fanX_input attribute.
> 
> So to the caller test, regardless of RPM or PWM mode, we need the
> FAN_COMMAND register to configure the values. As it stands the change
> makes fan control available for any PMBus device which has the
> FAN_COMMANDxy register and whose driver configures a page with
> PMBUS_HAVE_FANxy, exposing the fanX_target attribute. The test in
> question above deals only with PMBUS_HAVE_PWMxy which was introduced in
> this change.
> 
> There are some constraints: PMBUS_HAVE_FANxy is required if
> PMBUS_HAVE_PWMxy is to be specified for a given page. This should
> probably be documented in a comment alongside the #defines.
> 
>>
>>> +		return 0;
>>> +
>>> +	sensor = pmbus_add_sensor(data, "pwm", NULL, index, page,
>>> +				  PMBUS_VIRT_PWM_1 + id, PSC_PWM,
>>> +				  true, false);
>>> +
>>> +	if (!sensor)
>>> +		return -ENOMEM;
>>> +
>>> +	sensor = pmbus_add_sensor(data, "pwm", "enable", index, page,
>>> +				  PMBUS_VIRT_PWM_ENABLE_1 + id, PSC_PWM,
>>> +				  true, false);
>>> +
>>> +	if (!sensor)
>>> +		return -ENOMEM;
>>> +
>>> +	sensor->convert = false;
>>
>> convert should be a new parameter to pmbus_add_sensor().
> 
> Will fix.
> 
> Cheers for the detailed feedback.
> 
> Andrew
> 
>>
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>   static int pmbus_add_fan_attributes(struct i2c_client *client,
>>>   				    struct pmbus_data *data)
>>>   {
>>> @@ -1658,6 +1843,15 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
>>>   					     PSC_FAN, true, true) == NULL)
>>>   				return -ENOMEM;
>>>   
>>> +			/* Fan control */
>>> +			if (pmbus_check_word_register(client, page,
>>> +					pmbus_fan_command_registers[f])) {
>>> +				ret = pmbus_add_fan_ctrl(client, data, index,
>>> +							 page, f, regval);
>>> +				if (ret < 0)
>>> +					return ret;
>>> +			}
>>> +
>>>   			/*
>>>   			 * Each fan status register covers multiple fans,
>>>   			 * so we have to do some magic.

Powered by blists - more mailing lists