[PATCH linux dev-4.10 v3 2/5] drivers/hwmon/pmbus: ibmps: Add non-hwmon attributes
Andrew Jeffery
andrew at aj.id.au
Mon Aug 7 16:03:30 AEST 2017
On Fri, 2017-08-04 at 11:17 -0500, Matt Spinler wrote:
>
> 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.
I appreciate that.
> 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...
Well, because:
1. There are existing ABIs for exposing a lot (but not all) of these
properties, and hwmon/pmbus devices should have drivers that conform to
these ABIs
2. Using the existing ABIs allows existing applications (e.g.
`sensors`) to display interesting information without knowledge of the
hardware (PMBus spec is pretty loose in terms of what you can do, what
you support, and how you go about it)
The problem I have with the proposal is it emulates what we can
equivalently scrape with `i2cget -f` by documenting a new kernel ABI
that has no additional semantics. This isn't going to fly with
upstream, and isn't terribly useful for us given the single page this
device supports.
The spanner in the works is the UCD driver where we will want to do the
same thing, but where the device has multiple pages. The `i2cget -f`
approach falls down there, and does seem it might require something
like what's proposed here if time is a factor. Given that there's the
question of consistency in the approach (i.e. providing the sysfs
interface for the UCD device but requiring the use of `i2cget -f` here
seems broken).
I'm a bit torn really.
Has anyone looked at how much effort it would really be to split out
the bits?
>
>
> >
> > >
> > > > 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.
Right; it seems the kernel's pmbus subsystem has mainly been concerned
with monitoring and not controlling the associated device. Clearing
faults would be a departure from that, as are my core changes to
support the MAX31785. I think there's a good chance that something like
what's proposed is acceptable, just no-one had implemented it.
Andrew
>
> >
> > >
> > > 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);
> > > >
> > > > }
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20170807/fc7e2558/attachment-0001.sig>
More information about the openbmc
mailing list