[PATCH linux dev-4.10 v3 1/2] hwmon: (ucd9000) Add gpio chip interface and clear logged faults
Andrew Jeffery
andrew at aj.id.au
Mon Aug 21 11:57:26 AEST 2017
Hi Chris,
On Fri, 2017-08-18 at 15:38 -0500, Christopher Bostic wrote:
>
> On 8/18/17 12:44 AM, Andrew Jeffery wrote:
> > Hi Chris,
> >
> > On Thu, 2017-08-17 at 20:35 -0500, Christopher Bostic wrote:
> > > Add a struct gpio_chip and define some methods so that this device's
> > > I/O can be accessed via /sys/class/gpio.
> > >
> > > Add capability to clear logged faults via sysfs.
> > >
> > > > Signed-off-by: Christopher Bostic <cbostic at linux.vnet.ibm.com>
> > >
> > > ---
> > > v3 - Correct bug in gpio_chip get method. Wasn't retrieving
> > > gpio config information correctly.
> > > - Remove old debugfs flag from previous pmbus core changes.
> > > - Remove all sysfs files for mfr_status command.
> > > - Add comments on direct i2c_smbus calls to clarify that no page
> > > set is required.
> > > v2 - Remove clear_faults file - redundant since all other sysfs
> > > core accesses result in an automatic clear fault.
> > > - Removed local status_word and status_vout register dumps and
> > > use the new pmbus core status facilities instead.
> > > - Rename gpi<x>_fault to gpi<x>_alarm to better match core naming
> > > conventions.
> > > - Add full register dump for mfr_status.
> > > - Add gpio chip to device structure and use gpio interfaces.
> > > ---
> > > drivers/hwmon/pmbus/ucd9000.c | 136 +++++++++++++++++++++++++++++++++++++++++-
> > > 1 file changed, 134 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c
> > > index 3e3aa95..af44c1e 100644
> > > --- a/drivers/hwmon/pmbus/ucd9000.c
> > > +++ b/drivers/hwmon/pmbus/ucd9000.c
> > > @@ -26,6 +26,9 @@
> > > #include <linux/slab.h>
> > > #include <linux/i2c.h>
> > > #include <linux/i2c/pmbus.h>
> > > +#include <linux/sysfs.h>
> > > +#include <linux/hwmon-sysfs.h>
> > > +#include <linux/gpio.h>
> > > #include "pmbus.h"
> > >
> > > enum chips { ucd9000, ucd90120, ucd90124, ucd90160, ucd9090, ucd90910 };
> > > @@ -34,8 +37,16 @@
> > > > > > > > #define UCD9000_NUM_PAGES 0xd6
> > > > > > > > #define UCD9000_FAN_CONFIG_INDEX 0xe7
> > > > > > > > #define UCD9000_FAN_CONFIG 0xe8
> > > > > > > > +#define UCD9000_LOGGED_FAULTS 0xea
> > > > > > > > +#define UCD9000_GPIO_SELECT 0xfa
> > > > > > > > +#define UCD9000_GPIO_CONFIG 0xfb
> > > > #define UCD9000_DEVICE_ID 0xfd
> > >
> > >
> > > +/* GPIO CONFIG bits */
> > > > > > > > +#define UCD9000_GPIO_CONFIG_ENABLE BIT(0)
> > > > > > > > +#define UCD9000_GPIO_CONFIG_OUT_ENABLE BIT(1)
> > > > +#define UCD9000_GPIO_CONFIG_STATUS BIT(3)
> > >
> > > +
> > > > > > > > #define UCD9000_MON_TYPE(x) (((x) >> 5) & 0x07)
> > > > #define UCD9000_MON_PAGE(x) ((x) & 0x0f)
> > >
> > >
> > > @@ -46,9 +57,12 @@
> > >
> > > > #define UCD9000_NUM_FAN 4
> > >
> > >
> > > > +#define UCD90160_NUM_GPIOS 26
> >
> > This is mostly true, though there are only 22 physical GPIOs, plus 4 GPIs.
> > We're additionally constrained on the UCD90160 by "Table 3. GPIO Pin
> > Configuration Options" in "16-Rail Power Supply Sequencer and Monitor with ACPI
> > Support datasheet (Rev. C)" (ucd90160.pdf, page 22) to 8/26 as GPIs and 16/22
> > as GPOs. It would probably pay to think about how these constraints would
> > impact the implementation.
>
> Hi Andrew,
>
> Will go through and review the data for the various ucd9000 devs. I
> understand the need to cover all these device types but want to also be
> able to deliver internally what is needed which is just the gpi faults.
> Maybe we can discuss further on Sunday's call.
Yeah, understood, it was a thinking-out-loud comment. However I would hate to
get too deep on an implementation only to find that it's not appropriate for
the device's behaviour.
>
> > > +
> > > struct ucd9000_data {
> > > > > > > > u8 fan_data[UCD9000_NUM_FAN][I2C_SMBUS_BLOCK_MAX];
> > > > > > > > struct pmbus_driver_info info;
> > > > + struct gpio_chip gpio;
> > >
> > > };
> > > #define to_ucd9000_data(_info) container_of(_info, struct ucd9000_data, info)
> > >
> > > @@ -119,6 +133,92 @@ static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg)
> > > };
> > > MODULE_DEVICE_TABLE(i2c, ucd9000_id);
> > >
> > > +static int ucd9000_gpio_read_config(struct i2c_client *client,
> > > > + unsigned int offset)
> > >
> > > +{
> > > > + int ret;
> > >
> > > +
> > > > > > > > + /* No page set required */
> > > > > > > > + ret = i2c_smbus_write_byte_data(client, UCD9000_GPIO_SELECT, offset);
> > > > > > > > + if (ret < 0) {
> > > > + dev_err(&client->dev,
> >
> > Why is there a linebreak here?
>
> No good reason, will correct.
>
> >
> > > > + "Failed to read pin ID %d\n", offset);
> >
> > The error message is confusing - it relates to the semantics of
> > ucd9000_gpio_read_config() and not the operation on which the error occurred.
> >
> > I feel strongly that we should also report the error code as well: It provides
> > additional context on what went wrong.
> >
> > As a consequence I would suggest something like:
> >
> > > > dev_err(&client->dev, "Failed to select GPIO %d: %d\n", offset,
> > ret);
>
> OK, will go with your suggestion.
>
> > > > > > > > + return ret;
> > > > + }
> > >
> > > +
> > > > + return i2c_smbus_read_byte_data(client, UCD9000_GPIO_CONFIG);
> > >
> > > +}
> > > +
> > > +static int ucd9000_gpio_get(struct gpio_chip *gc, unsigned int offset)
> > > +{
> > > > > > > > + struct i2c_client *client = gpiochip_get_data(gc);
> > > > + int ret;
> > >
> > > +
> > > > > > > > + ret = ucd9000_gpio_read_config(client, offset);
> > > > > > > > + if (ret < 0) {
> > > > > > > > + dev_err(&client->dev,
> > > > + "failed to read GPIO config\n");
> >
> > Again, we don't need a linebreak above, but we probably should have one between
> > the dev_err() and the return statement.
>
> Will take care of that.
>
>
> > > > > > > > + return ret;
> > > > + }
> > >
> > > +
> > > > + return ret & UCD9000_GPIO_CONFIG_STATUS ? 1 : 0;
> >
> > We can avoid the branch with:
> >
> > return !!(ret & UCD9000_GPIO_CONFIG_STATUS);
>
> Will update.
>
> > Interestingly it's not clear from the datasheet whether the status bit contains
> > a relevant value when the pin is in output mode, as there's a separate bit (bit
> > 2) to set the value. Have you tested the behaviour?
>
> I've not tested this in output mode. Up to this point there isn't any
> means to set direction so, at least for how we're using it currently
> these are all set to inputs.
> Will see how this behaves in output mode.
Do it as you have time - as you point out there's not support for output
currently so it's not the end of the world. I just wanted to note the behaviour
so we didn't forget about it.
> > > +}
> > > +
> > > +static int ucd9000_gpio_get_direction(struct gpio_chip *gc,
> > > > + unsigned int offset)
> > >
> > > +{
> > > > > > > > + struct i2c_client *client = gpiochip_get_data(gc);
> > > > + int ret;
> > >
> > > +
> > > > > > > > + ret = ucd9000_gpio_read_config(client, offset);
> > > > > > > > + if (ret < 0) {
> > > > > > > > + dev_err(&client->dev,
> > > > > > > > + "failed to read GPIO config\n");
> > > > > > > > + return ret;
> > > > + }
> > >
> > > +
> > > > + return ret & UCD9000_GPIO_CONFIG_OUT_ENABLE ? 0 : 1;
> > >
> > > +}
> > > +
> > > +/*
> > > + * Defined so that libgpiod tool can access pin states. No plan to actually
> > > + * change pin direction within this driver.
> >
> > That's not true - the driver is general purpose. Lets not limit it too much to
> > *how you intend to use it* vs the actual hardware capability.
>
> Understand. Will implement direction switch for next revision in any case.
Great.
>
> > > + */
> > > +static int ucd9000_gpio_direction_input(struct gpio_chip *gc,
> > > > + unsigned int offset)
> > >
> > > +{
> > > > + return 0;
> >
> > I don't think we should do this for several reasons:
> >
> > 1) It's not that hard to implement the functionality
> > 2) It's not true that all GPIOs are inputs with factory defaults
> > 3) There will be uncertainty if custom defaults are programmed
> >
> > Again from ucd90160.pdf, on page 34:
> >
> > 7.5 Programming
> > 7.5.1 Device Configuration and Programming
> > From the factory, the device contains the sequencing and monitoring
> > firmware. It is also configured so that all GPOs are high-impedance
> > (except for FPWM/GPIO pins 17-24, which are driven low), with no
> > sequencing or fault-response operation. See Configuration Programming
> > of UCD Devices, available from the Documentation & Help Center that can
> > be selected from the Fusion GUI Help menu, for full UCD90160
> > configuration details.
> >
> > The FPWM pins are driven low, implying they are configured for output mode.
> > It's not obvious to me from this whether the other GPIOs are input or output
> > out-of-the-box as the chip supports active and open-drain drive types. It does
> > say "GPOs" in the paragraph above, but I haven't found documentation on which
> > pins are in what state from factory, and I don't know how we're configuring it
> > for production.
> >
> > Regardless, the driver shouldn't make assumptions about how the device is
> > pre-programmed - without further information there's already ambiguity as to
> > whether the default settings are in play or whether the system integrator has
> > configured the chip. We need information from the platform about how the device
> > is/should be managed.
> >
> > Further, the note in section 10.43 (FBh) GPIO_CONFIG (MFR_SPECIFIC_43) says:
> >
> > Configuring a pin that is also being used by another function (enable,
> > fan control, and so forth) may likely result in unexpected and unwanted
> > behavior.
>
> All good points. Will implement direction switching for next submit.
Thanks.
>
> > This suggests my favourite topic: pinctrl.
> >
> > Twenty-two of the pins on the UCD90160 are GPIO-capable. Of these 22 pins, 16
> > of them have alternate functions. See "Table 3. GPIO Pin Configuration Options"
> > on page 22 of ucd90160.pdf. But a quick rundown:
> >
> > 1. 4 PWM pins (GPIOs 1-4, pins 31, 32, 42, 41)
> > 2. 8 FPWM (Fast PWM) pins (GPIOs 5-12, pins 17-24)
> > 3. 4 pins shared with JTAG (GPIOs 19-22, pins 36-39)
> >
> > Now, in terms of devicetree we'd be breaking things in the future by specifying
> > a GPIO controller that relies on a pinmux in hardware without also specifying
> > the pin controller in the devicetree. This necessitates at least *some* pinctrl
> > capability be baked into the driver as well, so we can point the gpiochip to
> > the relevant pin controller (the same device, but the bindings are general) and
> > map the pin spaces in the devicetree (the gpio and pin spaces are different).
>
> I'd like to avoid implementing pinctrl for the time being given the
> limited function we need internally. I do see the advantages as you
> point out so maybe we can phase this in?
I think it should be possible to do a bare-bones pinctrl implementation that
describes no pinmux functions but allows allocation of GPIOs. That way the
devicetree won't have to change in a breaking way. See how the GPIO controller
defers to the pinctrl node to allocate pins in the Aspeed DTSIs:
https://github.com/openbmc/linux/blob/dev-4.10/Documentation/devicetree/bindings/gpio/gpio.txt#L230
https://github.com/openbmc/linux/blob/dev-4.10/arch/arm/boot/dts/aspeed-g5.dtsi#L271
https://github.com/openbmc/linux/blob/dev-4.10/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c#L2439
https://github.com/openbmc/linux/blob/dev-4.10/drivers/pinctrl/aspeed/pinctrl-aspeed.c#L453
The implementation should be no-where near as complex as the Aspeed pinmux, and
hopefully we only need the GPIO portions of it.
Cheers,
Andrew
>
> > > +}
> > > +
> > > +static ssize_t ucd9000_clear_logged_faults(struct device *dev,
> > > > > > > > + struct device_attribute *attr, const char *buf,
> > > > + size_t count)
> > >
> > > +{
> > > > > > > > + struct i2c_client *client = to_i2c_client(dev);
> > > > + int ret;
> > >
> > > +
> > > > > > > > + /* No page set required */
> > > > > > > > + ret = i2c_smbus_write_byte_data(client, UCD9000_LOGGED_FAULTS, 0);
> > > > > > > > + if (ret) {
> > > > > > > > + dev_err(&client->dev, "Failed to clear logged faults\n");
"> > > > > > > > + return ret;
> > > > + }
> > >
> > > +
> > > > + return count;
> > >
> > > +}
> > > +
> > > +static DEVICE_ATTR(clear_logged_faults, 0200, NULL,
> > > > + ucd9000_clear_logged_faults);
> > >
> > > +
> > > +static struct attribute *ucd9000_attributes[] = {
> > > > > > > > + &dev_attr_clear_logged_faults.attr,
> > > > + NULL
> > >
> > > +};
> > > +
> > > +static const struct attribute_group ucd9000_attr_group = {
> > > > + .attrs = ucd9000_attributes,
> > >
> > > +};
> > > +
> >
> > Joel's made a fair comment here regarding upstream. Please address that.
>
> Next patch set will be sent upstream.
>
> > > static int ucd9000_probe(struct i2c_client *client,
> > > > const struct i2c_device_id *id)
> > >
> > > {
> > > @@ -227,7 +327,39 @@ static int ucd9000_probe(struct i2c_client *client,
> > > > > > > > | PMBUS_HAVE_FAN34 | PMBUS_HAVE_STATUS_FAN34;
> > > > }
> > >
> > >
> > > > > > > > - return pmbus_do_probe(client, mid, info);
> > > > > > > > + data->gpio.label = "gpio-ucd9000";
> > > > > > > > + data->gpio.get_direction = ucd9000_gpio_get_direction;
> > > > > > > > + data->gpio.direction_input = ucd9000_gpio_direction_input;
> > > > + data->gpio.get = ucd9000_gpio_get;
> >
> > I feel like it wouldn't be much effort to make this more complete and specify
> > the direction_output() and set() callbacks as well.
>
> Right, will do that.
>
> >
> > > > > > > > + data->gpio.can_sleep = 1;
> > > > > > > > + data->gpio.base = -1;
> > > > > > > > + if (mid->driver_data == ucd90160)
> > > > + data->gpio.ngpio = UCD90160_NUM_GPIOS;
> >
> > This might need some rework in the future as the other ucd9000 variants want
> > GPIO support.
> >
> > In fact, what's the behaviour of the other drivers now given we're
> > unconditionally registering a gpiochip?
>
> The other drivers would have a gpio_chip that is available but if they
> don't use it I'm not thinking this would break them in any way. Would
> it be better to only register a gpiochip if its a ucd90160 in particular?
>
> > > > > > > > + data->gpio.parent = &client->dev;
> > > > + data->gpio.owner = THIS_MODULE;
> >
> > At some point we should think about pinconf as well (push-pull vs open-drain).
>
> Ok, will make a note of that.
>
>
> > > +
> > > > > > > > + ret = devm_gpiochip_add_data(&client->dev, &data->gpio, client);
> > > > > > > > + if (ret) {
> > > > > > > > + data->gpio.parent = NULL;
> > > > > > > > + dev_warn(&client->dev, "Count not add gpiochip\n");
> > > > > > > > + return ret;
> > > > + }
> > >
> > > +
> > > > > > > > + dev_info(&client->dev, "gpios %i...%i\n", data->gpio.base,
> > > > + data->gpio.base + data->gpio.ngpio - 1);
> >
> > Let's drop this.
>
> Will remove.
>
> Thanks,
> Chris
>
>
> >
> > Cheers,
> >
> > Andrew
> >
> > > +
> > > > > > > > + ret = sysfs_create_group(&client->dev.kobj,
> > > > > > > > + &ucd9000_attr_group);
> > > > > > > > + if (ret < 0)
> > > > + return ret;
> > >
> > > +
> > > > + return pmbus_do_probe(client, mid, info);
> > >
> > > +}
> > > +
> > > +static int ucd9000_remove(struct i2c_client *client)
> > > +{
> > > > > > > > + sysfs_remove_group(&client->dev.kobj, &ucd9000_attr_group);
> > > > + return pmbus_do_remove(client);
> > >
> > > }
> > >
> > > /* This is the driver that will be inserted */
> > > @@ -236,7 +368,7 @@ static int ucd9000_probe(struct i2c_client *client,
> > > > > > > > .name = "ucd9000",
> > > > > > > > },
> > > > > > > > .probe = ucd9000_probe,
> > > > > > > > - .remove = pmbus_do_remove,
> > > > > > > > + .remove = ucd9000_remove,
> > > > .id_table = ucd9000_id,
> > >
> > > };
> > >
>
>
-------------- 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/20170821/c8acf657/attachment-0001.sig>
More information about the openbmc
mailing list