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

[Joel Stanley]

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.


> +
>  /* 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)?

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.

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
>