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

[Joel Stanley]

On Fri, Jun 9, 2017 at 10:08 PM, Brad Bishop
<bradleyb at fuzziesquirrel.com> wrote:
>
>> 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?

As with the watchdog, this is a property of the wiring of the system -
a hardware property - so I think it should be device tree.

I applied the other two patches in the series, but not this one.

Cheers,

Joel

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