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

[Christopher Bostic]

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.

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
>>