[PATCH linux dev-4.10 v3 1/2] hwmon: (ucd9000) Add gpio chip interface and clear logged faults
Matt Spinler
mspinler at linux.vnet.ibm.com
Sat Aug 19 00:35:27 AEST 2017
Hi Andrew,
At this point, we just need a working driver so we can deliver the
function. This is all I need:
* read status_word
* read mfr_status (though it isn't that useful because the pmbus core
code that Chris had to start using sends a clear faults every second and
clears out the edge triggered bits I'm interested in)
* read status_vout from each page
* read the realtime GPI status using regs 0xFA and 0xFB for the 6 inputs
that are wired on this system
Chris's current code does all that, as did several of his previous
versions, so I think we need the path of least resistance and just go
with one of them.
On 8/18/2017 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.
>
>> +
>> 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?
>
>>> + "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);
>
>>> + 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.
>
>>> + return ret;
>>> + }
>> +
>>> + return ret & UCD9000_GPIO_CONFIG_STATUS ? 1 : 0;
> We can avoid the branch with:
>
> return !!(ret & UCD9000_GPIO_CONFIG_STATUS);
>
> 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?
>
>> +}
>> +
>> +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.
>
>> + */
>> +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.
>
> 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).
>
>> +}
>> +
>> +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.
>
>> 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.
>
>>> + 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?
>
>>> + 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).
>
>> +
>>> + 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.
>
> 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,
>> };
>>
More information about the openbmc
mailing list