[PATCH v2 3/3] hwmon: max31785: Provide config of fault pin behavior

[Joel Stanley]

On Fri, Jun 9, 2017 at 1:44 PM, Andrew Jeffery <andrew at aj.id.au> wrote:
> From: Christopher Bostic <cbostic at linux.vnet.ibm.com>
>
> Provide interface to set behavior when FAULT pin is asserted.
> When enabled force fan to 100% PWM duty cycle.
>
> Signed-off-by: Christopher Bostic <cbostic at linux.vnet.ibm.com>
> [Andrew Jeffery: Make it apply to reworked driver]
> Signed-off-by: Andrew Jeffery <andrew at aj.id.au>
> ---
>  drivers/hwmon/max31785.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 84 insertions(+)
>
> diff --git a/drivers/hwmon/max31785.c b/drivers/hwmon/max31785.c
> index ecece346be28..b4a350477ee5 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
> +
>  /* Fan Command constants */
>  #define MAX31785_FAN_COMMAND_PWM_RATIO         40
>
> @@ -755,6 +759,82 @@ static const struct hwmon_chip_info max31785_chip_info_0x3040 = {
>         .info = max31785_info_0x3040,
>  };
>
> +static ssize_t max31785_fault_resp_store(struct device *dev,
> +                               struct device_attribute *attr, const char *buf,
> +                               size_t count)
> +{
> +       struct i2c_client *client = to_i2c_client(dev);
> +       struct max31785 *data = dev_get_drvdata(dev);
> +       int rv;
> +
> +       mutex_lock(&data->lock);
> +
> +       rv = max31785_set_page(client, 0);
> +       if (rv < 0)
> +               goto done;
> +
> +       rv = i2c_smbus_read_byte_data(client, MAX31785_REG_MFR_FAULT_RESP);
> +       if (rv < 0)
> +               goto done;
> +
> +       if (!strncmp(buf, "on", strlen("on"))) {

This is interesting. What do you gain from the strlen?

> +               if (!(rv & MAX31785_FAULT_RESP_PIN_MONITOR)) {
> +                       rv |= MAX31785_FAULT_RESP_PIN_MONITOR;

Is this something we're toggling on and off at runtime?

I doubt it is. This should be enabled at probe time. If we want to
make it an option then use a device tree property, but I don't think
it even needs to be optional.

Chris, can you confirm that this does not need to be dynamic?

Cheers,

Joel


> +                       rv = i2c_smbus_write_byte_data(client,
> +                                       MAX31785_REG_MFR_FAULT_RESP, rv);
> +                       if (rv < 0)
> +                               goto done;
> +               }
> +       } else if (!strncmp(buf, "off", strlen("off"))) {
> +               if (rv & MAX31785_FAULT_RESP_PIN_MONITOR) {
> +                       rv &= ~MAX31785_FAULT_RESP_PIN_MONITOR;
> +                       rv = i2c_smbus_write_byte_data(client,
> +                                       MAX31785_REG_MFR_FAULT_RESP, rv);
> +                       if (rv < 0)
> +                               goto done;
> +               }
> +       } else {
> +               dev_warn(dev, "Unknown fault response type: [%s]\n", buf);
> +               rv = -EINVAL;
> +       }
> +
> +done:
> +       mutex_unlock(&data->lock);
> +
> +       return rv;
> +}
> +
> +static ssize_t max31785_fault_resp_show(struct device *dev,
> +                              struct device_attribute *attr, char *buf)
> +{
> +       struct max31785 *data = max31785_update_device(dev);
> +       struct i2c_client *client = to_i2c_client(dev);
> +       int rv;
> +
> +       mutex_lock(&data->lock);
> +
> +       rv = max31785_set_page(client, 0);
> +       if (rv < 0)
> +               goto done;
> +
> +       rv = i2c_smbus_read_byte_data(client, MAX31785_REG_MFR_FAULT_RESP);
> +       if (rv < 0)
> +               goto done;
> +
> +       if (rv & MAX31785_FAULT_RESP_PIN_MONITOR)
> +               rv = sprintf(buf, "on\n");
> +       else
> +               rv = sprintf(buf, "off\n");
> +
> +done:
> +       mutex_unlock(&data->lock);
> +
> +       return rv;
> +}
> +
> +static DEVICE_ATTR(fault_resp, 0644, max31785_fault_resp_show,
> +               max31785_fault_resp_store);
> +
>
>  static int max31785_get_capabilities(struct max31785 *data)
>  {
> @@ -804,6 +884,10 @@ static int max31785_probe(struct i2c_client *client,
>         else
>                 chip = &max31785_chip_info_0x3030;
>
> +       rc = device_create_file(dev, &dev_attr_fault_resp);
> +       if (rc)
> +               return rc;
> +
>         hwmon_dev = devm_hwmon_device_register_with_info(dev,
>                         client->name, data, chip, NULL);
>
> --
> 2.11.0
>