On 5/29/20 10:42 AM, Patrick Williams wrote:
> Hi Guenter,
> 
> Thanks for the initial look at this.
> 
> One question for you below...
> 
> On Fri, May 29, 2020 at 10:30:16AM -0700, Guenter Roeck wrote:
>> On 5/29/20 5:46 AM, Manikandan Elumalai wrote:
>>> +		/* Enable TEMP1 by default */
>>> +		config |= ADM1278_TEMP1_EN;
>>> +		ret = i2c_smbus_write_byte_data(client,
>>> +					ADM1275_PMON_CONFIG,
>>> +					config);
>>> +		if (ret < 0) {
>>> +		dev_err(&client->dev,
>>> +			"Failed to enable temperature config\n");
>>> +		return -ENODEV;
>>> +		}
>>
>> This can be handled in a single operation, together with ADM1278_VOUT_EN
>> below. There is no need for two separate write operations.
> 
> I don't know if you noticed here but the change ends up enabling
> TEMP1_EN in all cases.  Is this acceptable?  If not, do you have any
> preference on how it is selected for enablement?
> 

I did. We are doing the same for output voltage already, so I am not that
much concerned about it. If it is, we might consider adding _enable
attribute support (see Documentation/hwmon/sysfs-interface.rst) to the
PMBus core (presumably as virtual PMBus commands) and let the user
enable/disable individual attributes as needed.

What _should_ really be done, of course, is that the BIOS/ROMMON
configures the chip as desired. Obviously that is not happening here.

Guenter

>>>  		/* Enable VOUT if not enabled (it is disabled by default) */
>>>  		if (!(config & ADM1278_VOUT_EN)) {
>>> @@ -681,9 +697,6 @@ static int adm1275_probe(struct i2c_client *client,
>>>  			}
>>>  		}
>>>  
>>> -		if (config & ADM1278_TEMP1_EN)
>>> -			info->func[0] |=
>>> -				PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP;
>>>  		if (config & ADM1278_VIN_EN)
>>>  			info->func[0] |= PMBUS_HAVE_VIN;
>>>  		break;
>>>
>>
>