[PATCH linux dev-4.10 v3 2/5] drivers/hwmon/pmbus: ibmps: Add non-hwmon attributes

[Matt Spinler]

On 8/4/2017 10:16 AM, Eddie James wrote:
>
>
> On 08/03/2017 07:33 PM, Andrew Jeffery wrote:
>> On Wed, 2017-08-02 at 15:25 -0500, Eddie James wrote:
>>>> From: "Edward A. James" <eajames at us.ibm.com>
>>> Add sysfs entries to dump out PS registers
>> Why are we working against what the pmbus subsystem already gives us?
>> Why not augment support in pmbus core to give us the information we
>> need in a manner acceptable to upstream?
>
> Yea, that would be great. I feel like we're a little short on time for 
> that, as we need this functionality basically now. But we can give it 
> a shot.
>
>>
>> Flipping to the other end of the spectrum: The device only has one
>> page. We can just use `i2cget -f -y ...` without risk of corruption.
>
> Yea, I just get a lot of requests to have it all in the device driver. 
> I guess we need to just push back and ask people to use i2c tools. But 
> we will have to solve this for the UCD driver.
In general, IBM likes to capture a lot of registers on failures so we 
can actually figure out what went wrong after the fact, as we have 
customers who get mad when we can't give them root cause.  It's a bummer 
that is so hard to do here.  And if we had to use the i2cdev stuff for 
that, then why even use a device driver for any of this...


>
>>
>>>   and clear faults.
>> Can you describe the motivation for this? I feel like it would be worth
>> having this as a separate patch, and to the core rather than in an
>> arbitrary driver.
>
> Users need to be able to clear faults, that was just on my list of 
> requirements :) They could probably use i2cset though.

Yea, userspace needs this.  I'm kinda surprised there wasn't already a 
way to expose it.

>
>>
>> Cheers,
>>
>> Andrew
>>
>>>> Signed-off-by: Edward A. James <eajames at us.ibm.com>
>>> ---
>>>   drivers/hwmon/pmbus/ibmps.c | 78 
>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 78 insertions(+)
>>>
>>> diff --git a/drivers/hwmon/pmbus/ibmps.c b/drivers/hwmon/pmbus/ibmps.c
>>> index 1928dd9..f55d2bf 100644
>>> --- a/drivers/hwmon/pmbus/ibmps.c
>>> +++ b/drivers/hwmon/pmbus/ibmps.c
>>> @@ -9,8 +9,10 @@
>>>     #include <linux/device.h>
>>>   #include <linux/i2c.h>
>>> +#include <linux/hwmon-sysfs.h>
>>>   #include <linux/jiffies.h>
>>>   #include <linux/module.h>
>>> +#include <linux/sysfs.h>
>>>     #include "pmbus.h"
>>>   @@ -122,14 +124,90 @@ static int ibmps_read_word_data(struct 
>>> i2c_client *client, int page, int reg)
>>>>       .read_word_data = ibmps_read_word_data,
>>>   };
>>>   +static ssize_t ibmps_clear_faults(struct device *dev,
>>>> +                  struct device_attribute *dev_attr,
>>>> +                  const char *buf, size_t count)
>>> +{
>>>> +    struct i2c_client *client = to_i2c_client(dev);
>>> +
>>>> +    pmbus_clear_faults(client);
>>> +
>>>> +    return count;
>>> +}
>>> +static DEVICE_ATTR(clear_faults, 0200, NULL, ibmps_clear_faults);
>>> +
>>> +static ssize_t ibmps_show_status_word(struct device *dev,
>>>> +                      struct device_attribute *dev_attr,
>>>> +                      char *buf)
>>> +{
>>>> +    int rc;
>>>> +    struct i2c_client *client = to_i2c_client(dev);
>>> +
>>>> +    rc = pmbus_read_word_data(client, 0, PMBUS_STATUS_WORD);
>>>> +    if (rc < 0)
>>>> +        return rc;
>>> +
>>>> +    return snprintf(buf, PAGE_SIZE - 1, "%04x\n", rc);
>>> +}
>>> +static DEVICE_ATTR(status_word, 0444, ibmps_show_status_word, NULL);
>>> +
>>> +static ssize_t ibmps_show_status_byte(struct device *dev,
>>>> +                      struct device_attribute *dev_attr,
>>>> +                      char *buf)
>>> +{
>>>> +    int rc;
>>>> +    struct i2c_client *client = to_i2c_client(dev);
>>>> +    struct sensor_device_attribute *sattr = 
>>>> to_sensor_dev_attr(dev_attr);
>>> +
>>>> +    rc = pmbus_read_byte_data(client, 0, PMBUS_STATUS_VOUT + 
>>>> sattr->index);
>>>> +    if (rc < 0)
>>>> +        return rc;
>>> +
>>>> +    return snprintf(buf, PAGE_SIZE - 1, "%02x\n", rc);
>>> +}
>>> +
>>> +static SENSOR_DEVICE_ATTR(status_vout, 0444, 
>>> ibmps_show_status_byte, NULL, 0);
>>> +static SENSOR_DEVICE_ATTR(status_iout, 0444, 
>>> ibmps_show_status_byte, NULL, 1);
>>> +static SENSOR_DEVICE_ATTR(status_input, 0444, 
>>> ibmps_show_status_byte, NULL, 2);
>>> +static SENSOR_DEVICE_ATTR(status_temp, 0444, 
>>> ibmps_show_status_byte, NULL, 3);
>>> +static SENSOR_DEVICE_ATTR(status_cml, 0444, ibmps_show_status_byte, 
>>> NULL, 4);
>>> +static SENSOR_DEVICE_ATTR(status_other, 0444, 
>>> ibmps_show_status_byte, NULL, 5);
>>> +static SENSOR_DEVICE_ATTR(status_mfr, 0444, ibmps_show_status_byte, 
>>> NULL, 6);
>>> +static SENSOR_DEVICE_ATTR(status_fan, 0444, ibmps_show_status_byte, 
>>> NULL, 7);
>>> +
>>> +static struct attribute *ibmps_attributes[] = {
>>>> +    &dev_attr_clear_faults.attr,
>>>> +    &dev_attr_status_word.attr,
>>>> +    &sensor_dev_attr_status_vout.dev_attr.attr,
>>>> +    &sensor_dev_attr_status_iout.dev_attr.attr,
>>>> +    &sensor_dev_attr_status_input.dev_attr.attr,
>>>> +    &sensor_dev_attr_status_temp.dev_attr.attr,
>>>> +    &sensor_dev_attr_status_cml.dev_attr.attr,
>>>> +    &sensor_dev_attr_status_other.dev_attr.attr,
>>>> +    &sensor_dev_attr_status_mfr.dev_attr.attr,
>>>> +    &sensor_dev_attr_status_fan.dev_attr.attr,
>>>> +    NULL
>>> +};
>>> +
>>> +static const struct attribute_group ibmps_attribute_group = {
>>>> +    .attrs = ibmps_attributes,
>>> +};
>>> +
>>>   static int ibmps_probe(struct i2c_client *client,
>>>>                  const struct i2c_device_id *id)
>>>   {
>>>> +    int rc = sysfs_create_group(&client->dev.kobj,
>>>> +                    &ibmps_attribute_group);
>>>> +    if (rc)
>>>> +        return rc;
>>> +
>>>>       return pmbus_do_probe(client, id, &ibmps_info);
>>>   }
>>>     static int ibmps_remove(struct i2c_client *client)
>>>   {
>>>> + sysfs_remove_group(&client->dev.kobj, &ibmps_attribute_group);
>>> +
>>>>       return pmbus_do_remove(client);
>>>   }
>