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

[Brad Bishop]

> On Jun 9, 2017, at 1:27 AM, Joel Stanley <joel at jms.id.au> wrote:
> 
> 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.

I had asked jk if this should be DT or sysfs.  jk: can you comment?

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

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