[PATCH linux dev-4.10 v2 3/3] drivers/hwmon: max31785 Set fan fault response based on dev tree

[Christopher Bostic]

On 6/19/17 4:10 PM, Christopher Bostic wrote:
>
>
> On 6/16/17 1:18 AM, Joel Stanley wrote:
>> On Fri, Jun 16, 2017 at 5:30 AM, Christopher Bostic
>> <cbostic at linux.vnet.ibm.com> wrote:
>>> Check for the optional device tree property 'fault-max-fan'.  If
>>> present, configure hardware for 100% PWM fan duty cycle on fault
>>> condition.
>>>
>>> Signed-off-by: Christopher Bostic <cbostic at linux.vnet.ibm.com>
>>> ---
>>>   drivers/hwmon/max31785.c | 38 ++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 38 insertions(+)
>>>
>>> diff --git a/drivers/hwmon/max31785.c b/drivers/hwmon/max31785.c
>>> index fc03b7c..a67d5e5 100644
>>> --- a/drivers/hwmon/max31785.c
>>> +++ b/drivers/hwmon/max31785.c
>>> @@ -38,6 +38,7 @@
>>>   #define MAX31785_REG_MFR_ID                    0x99
>>>   #define MAX31785_REG_MFR_MODEL                 0x9a
>>>   #define MAX31785_REG_MFR_REVISION              0x9b
>>> +#define MAX31785_REG_MFR_FAULT_RESP            0xd9
>>>   #define MAX31785_REG_MFR_FAN_CONFIG            0xf1
>>>   #define MAX31785_REG_READ_FAN_PWM              0xf3
>>>
>>> @@ -51,6 +52,9 @@
>>>   /* Fan Status register bits */
>>>   #define MAX31785_FAN_STATUS_FAULT_MASK         0x80
>>>
>>> +/* Fault response register bits */
>>> +#define MAX31785_FAULT_RESP_PIN_MONITOR                0x01
>> It's a bit, so use BIT(0).
>>
>> The bit is called "FAULT_PIN_MONITOR". Please use that naming in your 
>> code.
>
> Hi Joel,
>
> OK will make those updates.
>>
>>> +
>>>   /* Fan Command constants */
>>>   #define MAX31785_FAN_COMMAND_PWM_RATIO         40
>>>
>>> @@ -762,6 +766,36 @@ static int max31785_get_capabilities(struct 
>>> max31785 *data)
>>>          return 0;
>>>   }
>>>
>>> +static int max31785_init_fault_resp(struct i2c_client *client)
>>> +{
>>> +       struct device_node *np = client->dev.of_node;
>>> +       int page;
>>> +       int rc;
>>> +
>>> +       if (np && of_get_property(np, "fault-max-fan", NULL)) {
>>> +               /* TODO: number of fans/pages is platform dependent */
>>> +               for (page = 0; page < 4; page++) {
>> The datasheet says "this bit is only valid for pages 0 to 5. Other
>> pages return 0".
>>
>> Is there a reason you only apply it to the first four pages, and not
>> the fifth (index 4) and sixth (index 5)?
>
> Was thinking in terms of Witherspoon which only uses fans 0 - 3. 
> Clearly not a generic
> solution.
>>
>> If you intended to apply this for all 6 supported pages, you could set
>> page 255, and have the condition applied for all of the valid pages.
>
> I'll use this approach.
>

Hi Joel,

Actually, from the spec sheet I have on the max31785 it appears page 255 
doesn't allow access to the fault register, 0xd9.
My spec version is 19-5703; Rev 3; 8/12

Unless I missed something in a later revision I'll apply the fault mode 
for all 6 supported pages.

Thanks,
> Chris
>>
>> If you only set it for the first four pages on purpose then please add
>> a comment to say that. However, I think we should fix the binding so
>> the pages can be selected. I will review the bindings separately.
>>
>> Cheers,
>>
>> Joel
>>
>>
>>> +
>>> +                       /* set max fans on fault */
>>> +                       rc = max31785_set_page(client, page);
>>> +                       if (rc < 0)
>>> +                               return rc;
>>> +
>>> +                       rc = i2c_smbus_read_byte_data(client,
>>> + MAX31785_REG_MFR_FAULT_RESP);
>>> +                       if (rc < 0)
>>> +                               return rc;
>>> +
>>> +                       rc |= MAX31785_FAULT_RESP_PIN_MONITOR;
>>
>>
>>> +                       rc = i2c_smbus_write_byte_data(client,
>>> + MAX31785_REG_MFR_FAULT_RESP, rc);
>>> +               }
>>> +               return rc;
>>> +       }
>>> +
>>> +       return 0;
>>> +}
>>> +
>>>   static int max31785_probe(struct i2c_client *client,
>>>                            const struct i2c_device_id *id)
>>>   {
>>> @@ -783,6 +817,10 @@ static int max31785_probe(struct i2c_client 
>>> *client,
>>>          data->client = client;
>>>          mutex_init(&data->lock);
>>>
>>> +       rc = max31785_init_fault_resp(client);
>>> +       if (rc)
>>> +               return rc;
>>> +
>>>          rc = max31785_init_fans(data);
>>>          if (rc)
>>>                  return rc;
>>> -- 
>>> 1.8.2.2
>>>
>