[{"id":1764441,"web_url":"http://patchwork.ozlabs.org/comment/1764441/","msgid":"<1504744069.5042.6.camel@aj.id.au>","list_archive_url":null,"date":"2017-09-07T00:27:50","subject":"Re: [PATCH linux dev-4.10 v7 1/2] hwmon: (ucd9000) Add gpio chip\n\tinterface","submitter":{"id":68332,"url":"http://patchwork.ozlabs.org/api/people/68332/","name":"Andrew Jeffery","email":"andrew@aj.id.au"},"content":"On Wed, 2017-09-06 at 16:00 -0500, Christopher Bostic wrote:\n> Add a struct gpio_chip and define some methods so that this device's\n> I/O can be accessed via /sys/class/gpio.\n> \n> Present requirements only call for retrieving current state of pin\n> values and their direction.  No requirement at this time to switch\n> modes between output and input within the device driver.\n> \n> > Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>\n> ---\n> v7 - Reorder this patch from #2 in series to #1\n> v6 - Break out sysfs clear logged faults into separate patch\n>    - Fix logic bug that was bitwise inverting instead of !\n>    - Add further comments about assumptions related to pin mux.\n> v5 - Clean up remaining branch return statements to !! non\n>      branching method.\n>    - Clean up white space issues.\n>    - Add return codes to error messages.\n>    - Add comments describing assumptions of ucd90160 type.\n>    - Define gpio direction set methods.\n>    - Add unique id for each ucd9000 in system for gpio chip.\n> v4 - Change status check from branch to a !! non branching method\n>    - Remove usage comments on libgpiod for the struct gpio_chip\n>      methods.\n>    - Clean up some text formatting.\n> v3 - Correct bug in gpio_chip get method.  Wasn't retrieving\n>      gpio config information correctly.\n>    - Remove old debugfs flag from previous pmbus core changes.\n>    - Remove all sysfs files for mfr_status command.\n>    - Add comments on direct i2c_smbus calls to clarify that no page\n>      set is required.\n> v2 - Remove clear_faults file - redundant since all other sysfs\n>      core accesses result in an automatic clear fault.\n>    - Removed local status_word and status_vout register dumps and\n>      use the new pmbus core status facilities instead.\n>    - Rename gpi<x>_fault to gpi<x>_alarm to better match core naming\n>      conventions.\n>    - Add full register dump for mfr_status.\n>    - Add gpio chip to device structure and use gpio interfaces.\n> ---\n>  drivers/hwmon/pmbus/ucd9000.c | 206 ++++++++++++++++++++++++++++++++++++++++++\n>  1 file changed, 206 insertions(+)\n> \n> diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c\n> index 3e3aa95..9a82e88 100644\n> --- a/drivers/hwmon/pmbus/ucd9000.c\n> +++ b/drivers/hwmon/pmbus/ucd9000.c\n> @@ -26,6 +26,7 @@\n>  #include <linux/slab.h>\n>  #include <linux/i2c.h>\n>  #include <linux/i2c/pmbus.h>\n> +#include <linux/gpio.h>\n>  #include \"pmbus.h\"\n>  \n>  enum chips { ucd9000, ucd90120, ucd90124, ucd90160, ucd9090, ucd90910 };\n> @@ -34,8 +35,18 @@\n>  #define UCD9000_NUM_PAGES\t\t0xd6\n>  #define UCD9000_FAN_CONFIG_INDEX\t0xe7\n>  #define UCD9000_FAN_CONFIG\t\t0xe8\n> +#define UCD9000_GPIO_SELECT\t\t0xfa\n> +#define UCD9000_GPIO_CONFIG\t\t0xfb\n>  #define UCD9000_DEVICE_ID\t\t0xfd\n>  \n> +/* GPIO CONFIG bits */\n> +#define UCD9000_GPIO_CONFIG_ENABLE\tBIT(0)\n> +#define UCD9000_GPIO_CONFIG_OUT_ENABLE\tBIT(1)\n> +#define UCD9000_GPIO_CONFIG_OUT_VALUE\tBIT(2)\n> +#define UCD9000_GPIO_CONFIG_STATUS\tBIT(3)\n> +#define UCD9000_GPIO_INPUT\t\t0\n> +#define UCD9000_GPIO_OUTPUT\t\t1\n> +\n>  #define UCD9000_MON_TYPE(x)\t(((x) >> 5) & 0x07)\n>  #define UCD9000_MON_PAGE(x)\t((x) & 0x0f)\n>  \n> @@ -46,9 +57,13 @@\n>  \n>  #define UCD9000_NUM_FAN\t\t4\n>  \n> +#define UCD9000_GPIO_NAME_LEN\t16\n> +#define UCD90160_NUM_GPIOS\t26\n> +\n>  struct ucd9000_data {\n>  \tu8 fan_data[UCD9000_NUM_FAN][I2C_SMBUS_BLOCK_MAX];\n>  \tstruct pmbus_driver_info info;\n> +\tstruct gpio_chip gpio;\n>  };\n>  #define to_ucd9000_data(_info) container_of(_info, struct ucd9000_data, info)\n>  \n> @@ -119,6 +134,166 @@ static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg)\n>  };\n>  MODULE_DEVICE_TABLE(i2c, ucd9000_id);\n>  \n> +static int ucd9000_gpio_read_config(struct i2c_client *client,\n> +\t\t\t\tunsigned int offset)\n> +{\n> +\tint ret;\n> +\n> +\t/* No page set required */\n> +\tret = i2c_smbus_write_byte_data(client, UCD9000_GPIO_SELECT, offset);\n> +\tif (ret < 0) {\n> +\t\tdev_err(&client->dev, \"Failed to select GPIO %d: %d\\n\", offset,\n> +\t\t\tret);\n> +\n> +\t\treturn ret;\n> +\t}\n> +\n> +\treturn i2c_smbus_read_byte_data(client, UCD9000_GPIO_CONFIG);\n> +}\n> +\n> +static int ucd9000_gpio_get(struct gpio_chip *gc, unsigned int offset)\n> +{\n> +\tstruct i2c_client *client  = gpiochip_get_data(gc);\n> +\tint ret;\n> +\n> +\tret = ucd9000_gpio_read_config(client, offset);\n> +\tif (ret < 0) {\n> +\t\tdev_err(&client->dev, \"failed to read GPIO %d config: %d\\n\",\n> +\t\t\toffset, ret);\n> +\n> +\t\treturn ret;\n> +\t}\n> +\n> +\treturn !!(ret & UCD9000_GPIO_CONFIG_STATUS);\n> +}\n> +\n> +static void ucd9000_gpio_set(struct gpio_chip *gc, unsigned int offset,\n> +\t\t\tint value)\n> +{\n> +\tstruct i2c_client *client = gpiochip_get_data(gc);\n> +\tint ret;\n> +\n> +\tret = ucd9000_gpio_read_config(client, offset);\n> +\tif (ret < 0) {\n> +\t\tdev_err(&client->dev, \"failed to read GPIO %d config: %d\\n\",\n> +\t\t\toffset, ret);\n> +\n> +\t\treturn;\n> +\t}\n> +\n> +\tif (value) {\n> +\t\tif (ret & UCD9000_GPIO_CONFIG_STATUS)\n> +\t\t\treturn;\n> +\n> +\t\tret |= UCD9000_GPIO_CONFIG_STATUS;\n> +\t} else {\n> +\t\tif (!(ret & UCD9000_GPIO_CONFIG_STATUS))\n> +\t\t\treturn;\n> +\n> +\t\tret &= ~UCD9000_GPIO_CONFIG_STATUS;\n> +\t}\n> +\n> +\tret |= UCD9000_GPIO_CONFIG_ENABLE;\n> +\n> +\t/* Page set not required */\n> +\tret = i2c_smbus_write_byte_data(client, UCD9000_GPIO_CONFIG, ret);\n> +\tif (ret < 0) {\n> +\t\tdev_err(&client->dev, \"Failed to write GPIO %d config: %d\\n\",\n> +\t\t\toffset, ret);\n> +\n> +\t\treturn;\n> +\t}\n> +\n> +\tret &= ~UCD9000_GPIO_CONFIG_ENABLE;\n> +\n> +\tret = i2c_smbus_write_byte_data(client, UCD9000_GPIO_CONFIG, ret);\n> +\tif (ret < 0)\n> +\t\tdev_err(&client->dev, \"Failed to write GPIO %d config: %d\\n\",\n> +\t\t\toffset, ret);\n> +}\n> +\n> +static int ucd9000_gpio_get_direction(struct gpio_chip *gc,\n> +\t\t\t\t\tunsigned int offset)\n> +{\n> +\tstruct i2c_client *client = gpiochip_get_data(gc);\n> +\tint ret;\n> +\n> +\tret = ucd9000_gpio_read_config(client, offset);\n> +\tif (ret < 0) {\n> +\t\tdev_err(&client->dev, \"failed to read GPIO %d config: %d\\n\",\n> +\t\t\toffset, ret);\n> +\n> +\t\treturn ret;\n> +\t}\n> +\n> +\treturn !(!!(ret & UCD9000_GPIO_CONFIG_OUT_ENABLE));\n\nWhy are we triple negating? One is enough. This will work, but it's unnecessary\nand excessive.\n\n> +}\n> +\n> +static int ucd9000_gpio_set_direction(struct gpio_chip *gc, unsigned int offset,\n> +\t\t\t\t\tbool direction_out, int requested_out)\n> +{\n> +\tstruct i2c_client *client = gpiochip_get_data(gc);\n> +\tint ret, config, out_val;\n> +\n> +\n> +\tret = ucd9000_gpio_read_config(client, offset);\n> +\tif (ret < 0) {\n> +\t\tdev_err(&client->dev, \"failed to read GPIO %d config: %d\\n\",\n> +\t\t\toffset, ret);\n> +\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tif (direction_out) {\n> +\t\tout_val = requested_out ? UCD9000_GPIO_CONFIG_OUT_VALUE : 0;\n> +\n> +\t\tif (ret & UCD9000_GPIO_CONFIG_OUT_ENABLE) {\n> +\t\t\tif ((ret & UCD9000_GPIO_CONFIG_OUT_VALUE) == out_val)\n> +\t\t\t\treturn 0;\n> +\t\t} else\n> +\t\t\tret |= UCD9000_GPIO_CONFIG_OUT_ENABLE;\n> +\n> +\t\tif (out_val)\n> +\t\t\tret |= UCD9000_GPIO_CONFIG_OUT_VALUE;\n> +\t\telse\n> +\t\t\tret &= ~UCD9000_GPIO_CONFIG_OUT_VALUE;\n> +\n> +\t} else {\n> +\t\tif (!(ret & UCD9000_GPIO_CONFIG_OUT_ENABLE))\n> +\t\t\treturn 0;\n> +\n> +\t\tret &= ~UCD9000_GPIO_CONFIG_OUT_ENABLE;\n> +\t}\n> +\n> +\tret |= UCD9000_GPIO_CONFIG_ENABLE;\n> +\tconfig = ret;\n> +\n> +\t/* Page set not required */\n> +\tret = i2c_smbus_write_byte_data(client, UCD9000_GPIO_CONFIG, config);\n> +\tif (ret < 0) {\n> +\t\tdev_err(&client->dev, \"Failed to write GPIO %d config: %d\\n\",\n> +\t\t\toffset, ret);\n> +\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tconfig &= ~UCD9000_GPIO_CONFIG_ENABLE;\n> +\n> +\treturn i2c_smbus_write_byte_data(client, UCD9000_GPIO_CONFIG, config);\n> +}\n> +\n> +static int ucd9000_gpio_direction_input(struct gpio_chip *gc,\n> +\t\t\t\t\tunsigned int offset)\n> +{\n> +\treturn ucd9000_gpio_set_direction(gc, offset, UCD9000_GPIO_INPUT, 0);\n> +}\n> +\n> +static int ucd9000_gpio_direction_output(struct gpio_chip *gc,\n> +\t\t\t\t\tunsigned int offset, int val)\n> +{\n> +\treturn ucd9000_gpio_set_direction(gc, offset, UCD9000_GPIO_OUTPUT, val);\n> +}\n> +\n>  static int ucd9000_probe(struct i2c_client *client,\n>  \t\t\t const struct i2c_device_id *id)\n>  {\n> @@ -227,6 +402,37 @@ static int ucd9000_probe(struct i2c_client *client,\n>  \t\t  | PMBUS_HAVE_FAN34 | PMBUS_HAVE_STATUS_FAN34;\n>  \t}\n>  \n> +\t/*\n> +\t * Note:\n> +\t *\n> +\t * Pinmux support has not been added to the new gpio_chip.\n> +\t * This support should be added when possible given the mux\n> +\t * behavior of these IO devices.\n> +\t */\n> +\tdata->gpio.label = (const char *)&client->name;\n> +\tdata->gpio.get_direction = ucd9000_gpio_get_direction;\n> +\tdata->gpio.direction_input = ucd9000_gpio_direction_input;\n> +\tdata->gpio.direction_output = ucd9000_gpio_direction_output;\n> +\tdata->gpio.get = ucd9000_gpio_get;\n> +\tdata->gpio.set = ucd9000_gpio_set;\n> +\tdata->gpio.can_sleep = 1;\n> +\tdata->gpio.base = -1;\n> +\n> +\t/*\n> +\t * TODO: set ngpio for ucd9000 devs that aren't 90160 type\n> +\t */\n> +\tif (mid->driver_data == ucd90160)\n> +\t\tdata->gpio.ngpio = UCD90160_NUM_GPIOS;\n> +\tdata->gpio.parent = &client->dev;\n> +\tdata->gpio.owner = THIS_MODULE;\n> +\n> +\tret = devm_gpiochip_add_data(&client->dev, &data->gpio, client);\n> +\tif (ret) {\n> +\t\tdata->gpio.parent = NULL;\n> +\t\tdev_warn(&client->dev, \"Could not add gpiochip: %d\\n\", ret);\n> +\t\treturn ret;\n> +\t}\n> +\n>  \treturn pmbus_do_probe(client, mid, info);\n>  }\n>  \n\nIn the interest of expediting this:\n\nReviewed-by: Andrew Jeffery <andrew@aj.id.au>","headers":{"Return-Path":"<openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org>","X-Original-To":["incoming@patchwork.ozlabs.org","openbmc@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","openbmc@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xnhJ50LJ1z9s8J\n\tfor <incoming@patchwork.ozlabs.org>;\n\tThu,  7 Sep 2017 10:33:09 +1000 (AEST)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3xnhJ46DSQzDrWF\n\tfor <incoming@patchwork.ozlabs.org>;\n\tThu,  7 Sep 2017 10:33:08 +1000 (AEST)","from out1-smtp.messagingengine.com (out1-smtp.messagingengine.com\n\t[66.111.4.25])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3xnhHr2mQ8zDrVy\n\tfor <openbmc@lists.ozlabs.org>; Thu,  7 Sep 2017 10:32:54 +1000 (AEST)","from compute4.internal (compute4.nyi.internal [10.202.2.44])\n\tby mailout.nyi.internal (Postfix) with ESMTP id B86AD21A7F;\n\tWed,  6 Sep 2017 20:32:48 -0400 (EDT)","from frontend2 ([10.202.2.161])\n\tby compute4.internal (MEProxy); Wed, 06 Sep 2017 20:32:48 -0400","from keelia (bh02i525f01.au.ibm.com [202.81.18.30])\n\tby mail.messagingengine.com (Postfix) with ESMTPA id E801C240A4;\n\tWed,  6 Sep 2017 20:32:46 -0400 (EDT)"],"Authentication-Results":["ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=aj.id.au header.i=@aj.id.au header.b=\"izN+9/1s\";\n\tdkim=pass (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com header.b=\"XA4KGqDb\"; \n\tdkim-atps=neutral","lists.ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=aj.id.au header.i=@aj.id.au header.b=\"izN+9/1s\";\n\tdkim=pass (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com header.b=\"XA4KGqDb\"; \n\tdkim-atps=neutral","ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=aj.id.au\n\t(client-ip=66.111.4.25; helo=out1-smtp.messagingengine.com;\n\tenvelope-from=andrew@aj.id.au; receiver=<UNKNOWN>)","lists.ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=aj.id.au header.i=@aj.id.au header.b=\"izN+9/1s\";\n\tdkim=pass (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com\n\theader.b=\"XA4KGqDb\"; dkim-atps=neutral"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/relaxed; d=aj.id.au; h=cc\n\t:content-type:date:from:in-reply-to:message-id:mime-version\n\t:references:subject:to:x-me-sender:x-me-sender:x-sasl-enc\n\t:x-sasl-enc; s=fm1; bh=kXaDQszdhqE+3WrADbF4rvt7lt1J+ofocF3JLkVRW\n\t0U=; b=izN+9/1sgtQXKXNS9i7NURKfBIoU17a+u5oOzdwEX5AauNSz/qFvue2Xz\n\tLc3N1J6ueQgp/CErBtQnXGUsPn8vPko4OwCgwJUBJtW1xWRvc3iOhKFtTUYozt8T\n\tYuO6+ctyJMPWPI5R3HSLdtJoCmZ/O70z3gCEAejpzWuJV/AlydZWi1my9AdBu8As\n\tCAj+4YIICs0vFUSvovYMp0+3RJyT5v2SH6p/OZxE5pxd12mk63ipYwAPX6Wpme+I\n\tsDApg1+2Fa5HXminY2Nv4NmvaYKPDbUHm79kAqyWb0n6zwxFMmfcOpLw8tNlZxep\n\tp87dXO8pGZFyfBzngQxjj6cYkInGg==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=\n\tmessagingengine.com; h=cc:content-type:date:from:in-reply-to\n\t:message-id:mime-version:references:subject:to:x-me-sender\n\t:x-me-sender:x-sasl-enc:x-sasl-enc; s=fm1; bh=kXaDQszdhqE+3WrADb\n\tF4rvt7lt1J+ofocF3JLkVRW0U=; b=XA4KGqDbW0WJSQTq8yF+QwRKuQcy8GWJIL\n\tzlcrfL3I1Uf8cU0PufGapp/NfaKij0t3tZgPjhHHz1eowoIB9Z7aiTDl5xSZM9S8\n\tnFRIYxxV6hjsxBawsNIKNA2HNOA+6Ygz5GKTyR00JjGLMqmDmfhL9Ovy7SvROlgM\n\tKrgX9l/MseYwead22CAwnKPK/7XG+JF74bX4gJX393KgMhfD/8oxDXf/jy03zI3T\n\ts1ndPVYCfoAvyzvxZgXi6P4HNh+NB+1fuss+QNykNfM3WzqzBo+BiSt5IKSBDl/m\n\t4lTAuX9U0NWbUIDz4sy8Y/VASLPXFFQpemhovG/0iwzhDzO1VVnA=="],"X-ME-Sender":"<xms:sJOwWRLgA5UPsSLIvzr5l1TnvfX6HcCrDWhav-hsiRAzTC0lsnGdPQ>","X-Sasl-enc":"/ZFMKq/cxft8JBrHsff1QE8SA1/ugzC3WAu6MGk5X5YR 1504744367","Message-ID":"<1504744069.5042.6.camel@aj.id.au>","Subject":"Re: [PATCH linux dev-4.10 v7 1/2] hwmon: (ucd9000) Add gpio chip\n\tinterface","From":"Andrew Jeffery <andrew@aj.id.au>","To":"Christopher Bostic <cbostic@linux.vnet.ibm.com>, joel@jms.id.au","In-Reply-To":"<20170906210041.22533-2-cbostic@linux.vnet.ibm.com>","References":"<20170906210041.22533-1-cbostic@linux.vnet.ibm.com>\n\t<20170906210041.22533-2-cbostic@linux.vnet.ibm.com>","Content-Type":"multipart/signed; micalg=\"pgp-sha512\";\n\tprotocol=\"application/pgp-signature\";\n\tboundary=\"=-O2JYco2d97AXNkh6ISXh\"","Date":"Thu, 07 Sep 2017 10:27:50 +1000","Mime-Version":"1.0","X-Mailer":"Evolution 3.22.6-1ubuntu1 ","X-BeenThere":"openbmc@lists.ozlabs.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"Development list for OpenBMC <openbmc.lists.ozlabs.org>","List-Unsubscribe":"<https://lists.ozlabs.org/options/openbmc>,\n\t<mailto:openbmc-request@lists.ozlabs.org?subject=unsubscribe>","List-Archive":"<http://lists.ozlabs.org/pipermail/openbmc/>","List-Post":"<mailto:openbmc@lists.ozlabs.org>","List-Help":"<mailto:openbmc-request@lists.ozlabs.org?subject=help>","List-Subscribe":"<https://lists.ozlabs.org/listinfo/openbmc>,\n\t<mailto:openbmc-request@lists.ozlabs.org?subject=subscribe>","Cc":"openbmc@lists.ozlabs.org","Errors-To":"openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org","Sender":"\"openbmc\"\n\t<openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org>"}},{"id":1764854,"web_url":"http://patchwork.ozlabs.org/comment/1764854/","msgid":"<a5d1415c-95c4-05f3-a12e-2f1b7083ac6c@linux.vnet.ibm.com>","list_archive_url":null,"date":"2017-09-07T17:42:36","subject":"Re: [PATCH linux dev-4.10 v7 1/2] hwmon: (ucd9000) Add gpio chip\n\tinterface","submitter":{"id":70879,"url":"http://patchwork.ozlabs.org/api/people/70879/","name":"Christopher Bostic","email":"cbostic@linux.vnet.ibm.com"},"content":"On 9/6/17 7:27 PM, Andrew Jeffery wrote:\n> On Wed, 2017-09-06 at 16:00 -0500, Christopher Bostic wrote:\n>> Add a struct gpio_chip and define some methods so that this device's\n>> I/O can be accessed via /sys/class/gpio.\n>>   \n>> Present requirements only call for retrieving current state of pin\n>> values and their direction.  No requirement at this time to switch\n>> modes between output and input within the device driver.\n>>   \n>>> Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>\n>> ---\n>> v7 - Reorder this patch from #2 in series to #1\n>> v6 - Break out sysfs clear logged faults into separate patch\n>>     - Fix logic bug that was bitwise inverting instead of !\n>>     - Add further comments about assumptions related to pin mux.\n>> v5 - Clean up remaining branch return statements to !! non\n>>       branching method.\n>>     - Clean up white space issues.\n>>     - Add return codes to error messages.\n>>     - Add comments describing assumptions of ucd90160 type.\n>>     - Define gpio direction set methods.\n>>     - Add unique id for each ucd9000 in system for gpio chip.\n>> v4 - Change status check from branch to a !! non branching method\n>>     - Remove usage comments on libgpiod for the struct gpio_chip\n>>       methods.\n>>     - Clean up some text formatting.\n>> v3 - Correct bug in gpio_chip get method.  Wasn't retrieving\n>>       gpio config information correctly.\n>>     - Remove old debugfs flag from previous pmbus core changes.\n>>     - Remove all sysfs files for mfr_status command.\n>>     - Add comments on direct i2c_smbus calls to clarify that no page\n>>       set is required.\n>> v2 - Remove clear_faults file - redundant since all other sysfs\n>>       core accesses result in an automatic clear fault.\n>>     - Removed local status_word and status_vout register dumps and\n>>       use the new pmbus core status facilities instead.\n>>     - Rename gpi<x>_fault to gpi<x>_alarm to better match core naming\n>>       conventions.\n>>     - Add full register dump for mfr_status.\n>>     - Add gpio chip to device structure and use gpio interfaces.\n>> ---\n>>   drivers/hwmon/pmbus/ucd9000.c | 206 ++++++++++++++++++++++++++++++++++++++++++\n>>   1 file changed, 206 insertions(+)\n>>   \n>> diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c\n>> index 3e3aa95..9a82e88 100644\n>> --- a/drivers/hwmon/pmbus/ucd9000.c\n>> +++ b/drivers/hwmon/pmbus/ucd9000.c\n>> @@ -26,6 +26,7 @@\n>>   #include <linux/slab.h>\n>>   #include <linux/i2c.h>\n>>   #include <linux/i2c/pmbus.h>\n>> +#include <linux/gpio.h>\n>>   #include \"pmbus.h\"\n>>   \n>>   enum chips { ucd9000, ucd90120, ucd90124, ucd90160, ucd9090, ucd90910 };\n>> @@ -34,8 +35,18 @@\n>>   #define UCD9000_NUM_PAGES\t\t0xd6\n>>   #define UCD9000_FAN_CONFIG_INDEX\t0xe7\n>>   #define UCD9000_FAN_CONFIG\t\t0xe8\n>> +#define UCD9000_GPIO_SELECT\t\t0xfa\n>> +#define UCD9000_GPIO_CONFIG\t\t0xfb\n>>   #define UCD9000_DEVICE_ID\t\t0xfd\n>>   \n>> +/* GPIO CONFIG bits */\n>> +#define UCD9000_GPIO_CONFIG_ENABLE\tBIT(0)\n>> +#define UCD9000_GPIO_CONFIG_OUT_ENABLE\tBIT(1)\n>> +#define UCD9000_GPIO_CONFIG_OUT_VALUE\tBIT(2)\n>> +#define UCD9000_GPIO_CONFIG_STATUS\tBIT(3)\n>> +#define UCD9000_GPIO_INPUT\t\t0\n>> +#define UCD9000_GPIO_OUTPUT\t\t1\n>> +\n>>   #define UCD9000_MON_TYPE(x)\t(((x) >> 5) & 0x07)\n>>   #define UCD9000_MON_PAGE(x)\t((x) & 0x0f)\n>>   \n>> @@ -46,9 +57,13 @@\n>>   \n>>   #define UCD9000_NUM_FAN\t\t4\n>>   \n>> +#define UCD9000_GPIO_NAME_LEN\t16\n>> +#define UCD90160_NUM_GPIOS\t26\n>> +\n>>   struct ucd9000_data {\n>>   \tu8 fan_data[UCD9000_NUM_FAN][I2C_SMBUS_BLOCK_MAX];\n>>   \tstruct pmbus_driver_info info;\n>> +\tstruct gpio_chip gpio;\n>>   };\n>>   #define to_ucd9000_data(_info) container_of(_info, struct ucd9000_data, info)\n>>   \n>> @@ -119,6 +134,166 @@ static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg)\n>>   };\n>>   MODULE_DEVICE_TABLE(i2c, ucd9000_id);\n>>   \n>> +static int ucd9000_gpio_read_config(struct i2c_client *client,\n>> +\t\t\t\tunsigned int offset)\n>> +{\n>> +\tint ret;\n>> +\n>> +\t/* No page set required */\n>> +\tret = i2c_smbus_write_byte_data(client, UCD9000_GPIO_SELECT, offset);\n>> +\tif (ret < 0) {\n>> +\t\tdev_err(&client->dev, \"Failed to select GPIO %d: %d\\n\", offset,\n>> +\t\t\tret);\n>> +\n>> +\t\treturn ret;\n>> +\t}\n>> +\n>> +\treturn i2c_smbus_read_byte_data(client, UCD9000_GPIO_CONFIG);\n>> +}\n>> +\n>> +static int ucd9000_gpio_get(struct gpio_chip *gc, unsigned int offset)\n>> +{\n>> +\tstruct i2c_client *client  = gpiochip_get_data(gc);\n>> +\tint ret;\n>> +\n>> +\tret = ucd9000_gpio_read_config(client, offset);\n>> +\tif (ret < 0) {\n>> +\t\tdev_err(&client->dev, \"failed to read GPIO %d config: %d\\n\",\n>> +\t\t\toffset, ret);\n>> +\n>> +\t\treturn ret;\n>> +\t}\n>> +\n>> +\treturn !!(ret & UCD9000_GPIO_CONFIG_STATUS);\n>> +}\n>> +\n>> +static void ucd9000_gpio_set(struct gpio_chip *gc, unsigned int offset,\n>> +\t\t\tint value)\n>> +{\n>> +\tstruct i2c_client *client = gpiochip_get_data(gc);\n>> +\tint ret;\n>> +\n>> +\tret = ucd9000_gpio_read_config(client, offset);\n>> +\tif (ret < 0) {\n>> +\t\tdev_err(&client->dev, \"failed to read GPIO %d config: %d\\n\",\n>> +\t\t\toffset, ret);\n>> +\n>> +\t\treturn;\n>> +\t}\n>> +\n>> +\tif (value) {\n>> +\t\tif (ret & UCD9000_GPIO_CONFIG_STATUS)\n>> +\t\t\treturn;\n>> +\n>> +\t\tret |= UCD9000_GPIO_CONFIG_STATUS;\n>> +\t} else {\n>> +\t\tif (!(ret & UCD9000_GPIO_CONFIG_STATUS))\n>> +\t\t\treturn;\n>> +\n>> +\t\tret &= ~UCD9000_GPIO_CONFIG_STATUS;\n>> +\t}\n>> +\n>> +\tret |= UCD9000_GPIO_CONFIG_ENABLE;\n>> +\n>> +\t/* Page set not required */\n>> +\tret = i2c_smbus_write_byte_data(client, UCD9000_GPIO_CONFIG, ret);\n>> +\tif (ret < 0) {\n>> +\t\tdev_err(&client->dev, \"Failed to write GPIO %d config: %d\\n\",\n>> +\t\t\toffset, ret);\n>> +\n>> +\t\treturn;\n>> +\t}\n>> +\n>> +\tret &= ~UCD9000_GPIO_CONFIG_ENABLE;\n>> +\n>> +\tret = i2c_smbus_write_byte_data(client, UCD9000_GPIO_CONFIG, ret);\n>> +\tif (ret < 0)\n>> +\t\tdev_err(&client->dev, \"Failed to write GPIO %d config: %d\\n\",\n>> +\t\t\toffset, ret);\n>> +}\n>> +\n>> +static int ucd9000_gpio_get_direction(struct gpio_chip *gc,\n>> +\t\t\t\t\tunsigned int offset)\n>> +{\n>> +\tstruct i2c_client *client = gpiochip_get_data(gc);\n>> +\tint ret;\n>> +\n>> +\tret = ucd9000_gpio_read_config(client, offset);\n>> +\tif (ret < 0) {\n>> +\t\tdev_err(&client->dev, \"failed to read GPIO %d config: %d\\n\",\n>> +\t\t\toffset, ret);\n>> +\n>> +\t\treturn ret;\n>> +\t}\n>> +\n>> +\treturn !(!!(ret & UCD9000_GPIO_CONFIG_OUT_ENABLE));\n> Why are we triple negating? One is enough. This will work, but it's unnecessary\n> and excessive.\n>\n\nWill remove the unnecessary negating since debugfs updates in patch 0002 \nof this series needs updating anyway.\n\nThanks\nChris\n\n>> +}\n>> +\n>> +static int ucd9000_gpio_set_direction(struct gpio_chip *gc, unsigned int offset,\n>> +\t\t\t\t\tbool direction_out, int requested_out)\n>> +{\n>> +\tstruct i2c_client *client = gpiochip_get_data(gc);\n>> +\tint ret, config, out_val;\n>> +\n>> +\n>> +\tret = ucd9000_gpio_read_config(client, offset);\n>> +\tif (ret < 0) {\n>> +\t\tdev_err(&client->dev, \"failed to read GPIO %d config: %d\\n\",\n>> +\t\t\toffset, ret);\n>> +\n>> +\t\treturn ret;\n>> +\t}\n>> +\n>> +\tif (direction_out) {\n>> +\t\tout_val = requested_out ? UCD9000_GPIO_CONFIG_OUT_VALUE : 0;\n>> +\n>> +\t\tif (ret & UCD9000_GPIO_CONFIG_OUT_ENABLE) {\n>> +\t\t\tif ((ret & UCD9000_GPIO_CONFIG_OUT_VALUE) == out_val)\n>> +\t\t\t\treturn 0;\n>> +\t\t} else\n>> +\t\t\tret |= UCD9000_GPIO_CONFIG_OUT_ENABLE;\n>> +\n>> +\t\tif (out_val)\n>> +\t\t\tret |= UCD9000_GPIO_CONFIG_OUT_VALUE;\n>> +\t\telse\n>> +\t\t\tret &= ~UCD9000_GPIO_CONFIG_OUT_VALUE;\n>> +\n>> +\t} else {\n>> +\t\tif (!(ret & UCD9000_GPIO_CONFIG_OUT_ENABLE))\n>> +\t\t\treturn 0;\n>> +\n>> +\t\tret &= ~UCD9000_GPIO_CONFIG_OUT_ENABLE;\n>> +\t}\n>> +\n>> +\tret |= UCD9000_GPIO_CONFIG_ENABLE;\n>> +\tconfig = ret;\n>> +\n>> +\t/* Page set not required */\n>> +\tret = i2c_smbus_write_byte_data(client, UCD9000_GPIO_CONFIG, config);\n>> +\tif (ret < 0) {\n>> +\t\tdev_err(&client->dev, \"Failed to write GPIO %d config: %d\\n\",\n>> +\t\t\toffset, ret);\n>> +\n>> +\t\treturn ret;\n>> +\t}\n>> +\n>> +\tconfig &= ~UCD9000_GPIO_CONFIG_ENABLE;\n>> +\n>> +\treturn i2c_smbus_write_byte_data(client, UCD9000_GPIO_CONFIG, config);\n>> +}\n>> +\n>> +static int ucd9000_gpio_direction_input(struct gpio_chip *gc,\n>> +\t\t\t\t\tunsigned int offset)\n>> +{\n>> +\treturn ucd9000_gpio_set_direction(gc, offset, UCD9000_GPIO_INPUT, 0);\n>> +}\n>> +\n>> +static int ucd9000_gpio_direction_output(struct gpio_chip *gc,\n>> +\t\t\t\t\tunsigned int offset, int val)\n>> +{\n>> +\treturn ucd9000_gpio_set_direction(gc, offset, UCD9000_GPIO_OUTPUT, val);\n>> +}\n>> +\n>>   static int ucd9000_probe(struct i2c_client *client,\n>>   \t\t\t const struct i2c_device_id *id)\n>>   {\n>> @@ -227,6 +402,37 @@ static int ucd9000_probe(struct i2c_client *client,\n>>   \t\t  | PMBUS_HAVE_FAN34 | PMBUS_HAVE_STATUS_FAN34;\n>>   \t}\n>>   \n>> +\t/*\n>> +\t * Note:\n>> +\t *\n>> +\t * Pinmux support has not been added to the new gpio_chip.\n>> +\t * This support should be added when possible given the mux\n>> +\t * behavior of these IO devices.\n>> +\t */\n>> +\tdata->gpio.label = (const char *)&client->name;\n>> +\tdata->gpio.get_direction = ucd9000_gpio_get_direction;\n>> +\tdata->gpio.direction_input = ucd9000_gpio_direction_input;\n>> +\tdata->gpio.direction_output = ucd9000_gpio_direction_output;\n>> +\tdata->gpio.get = ucd9000_gpio_get;\n>> +\tdata->gpio.set = ucd9000_gpio_set;\n>> +\tdata->gpio.can_sleep = 1;\n>> +\tdata->gpio.base = -1;\n>> +\n>> +\t/*\n>> +\t * TODO: set ngpio for ucd9000 devs that aren't 90160 type\n>> +\t */\n>> +\tif (mid->driver_data == ucd90160)\n>> +\t\tdata->gpio.ngpio = UCD90160_NUM_GPIOS;\n>> +\tdata->gpio.parent = &client->dev;\n>> +\tdata->gpio.owner = THIS_MODULE;\n>> +\n>> +\tret = devm_gpiochip_add_data(&client->dev, &data->gpio, client);\n>> +\tif (ret) {\n>> +\t\tdata->gpio.parent = NULL;\n>> +\t\tdev_warn(&client->dev, \"Could not add gpiochip: %d\\n\", ret);\n>> +\t\treturn ret;\n>> +\t}\n>> +\n>>   \treturn pmbus_do_probe(client, mid, info);\n>>   }\n>>   \n> In the interest of expediting this:\n>\n> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>","headers":{"Return-Path":"<openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org>","X-Original-To":["incoming@patchwork.ozlabs.org","openbmc@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","openbmc@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xp78N1CSyz9s83\n\tfor <incoming@patchwork.ozlabs.org>;\n\tFri,  8 Sep 2017 03:43:00 +1000 (AEST)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3xp78M6M7SzDrY5\n\tfor <incoming@patchwork.ozlabs.org>;\n\tFri,  8 Sep 2017 03:42:59 +1000 (AEST)","from mx0a-001b2d01.pphosted.com (mx0a-001b2d01.pphosted.com\n\t[148.163.156.1])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3xp7882vGMzDrWV\n\tfor <openbmc@lists.ozlabs.org>; Fri,  8 Sep 2017 03:42:48 +1000 (AEST)","from pps.filterd (m0098409.ppops.net [127.0.0.1])\n\tby mx0a-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id\n\tv87HgSHx097412\n\tfor <openbmc@lists.ozlabs.org>; Thu, 7 Sep 2017 13:42:46 -0400","from e31.co.us.ibm.com (e31.co.us.ibm.com [32.97.110.149])\n\tby mx0a-001b2d01.pphosted.com with ESMTP id 2cu5f11bw0-1\n\t(version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT)\n\tfor <openbmc@lists.ozlabs.org>; Thu, 07 Sep 2017 13:42:45 -0400","from localhost\n\tby e31.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use\n\tOnly! Violators will be prosecuted\n\tfor <openbmc@lists.ozlabs.org> from <cbostic@linux.vnet.ibm.com>;\n\tThu, 7 Sep 2017 11:42:45 -0600","from b03cxnp08028.gho.boulder.ibm.com (9.17.130.20)\n\tby e31.co.us.ibm.com (192.168.1.131) with IBM ESMTP SMTP Gateway:\n\tAuthorized Use Only! Violators will be prosecuted; \n\tThu, 7 Sep 2017 11:42:42 -0600","from b03ledav006.gho.boulder.ibm.com\n\t(b03ledav006.gho.boulder.ibm.com [9.17.130.237])\n\tby b03cxnp08028.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with\n\tESMTP id v87Hgfvm31326306; Thu, 7 Sep 2017 10:42:41 -0700","from b03ledav006.gho.boulder.ibm.com (unknown [127.0.0.1])\n\tby IMSVA (Postfix) with ESMTP id 1C684C6047;\n\tThu,  7 Sep 2017 11:42:41 -0600 (MDT)","from christophersmbp.austin.ibm.com (unknown [9.83.1.167])\n\tby b03ledav006.gho.boulder.ibm.com (Postfix) with ESMTP id F400CC6043;\n\tThu,  7 Sep 2017 11:42:38 -0600 (MDT)"],"Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=linux.vnet.ibm.com\n\t(client-ip=148.163.156.1; helo=mx0a-001b2d01.pphosted.com;\n\tenvelope-from=cbostic@linux.vnet.ibm.com; receiver=<UNKNOWN>)","Subject":"Re: [PATCH linux dev-4.10 v7 1/2] hwmon: (ucd9000) Add gpio chip\n\tinterface","To":"Andrew Jeffery <andrew@aj.id.au>, joel@jms.id.au","References":"<20170906210041.22533-1-cbostic@linux.vnet.ibm.com>\n\t<20170906210041.22533-2-cbostic@linux.vnet.ibm.com>\n\t<1504744069.5042.6.camel@aj.id.au>","From":"Christopher Bostic <cbostic@linux.vnet.ibm.com>","Date":"Thu, 7 Sep 2017 12:42:36 -0500","User-Agent":"Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:45.0)\n\tGecko/20100101 Thunderbird/45.7.0","MIME-Version":"1.0","In-Reply-To":"<1504744069.5042.6.camel@aj.id.au>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","X-TM-AS-GCONF":"00","x-cbid":"17090717-8235-0000-0000-00000C3C0B93","X-IBM-SpamModules-Scores":"","X-IBM-SpamModules-Versions":"BY=3.00007684; HX=3.00000241; KW=3.00000007;\n\tPH=3.00000004; SC=3.00000226; SDB=6.00913651; UDB=6.00458579;\n\tIPR=6.00693909; \n\tBA=6.00005576; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009;\n\tZB=6.00000000; \n\tZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00017054;\n\tXFM=3.00000015; UTC=2017-09-07 17:42:44","X-IBM-AV-DETECTION":"SAVI=unused REMOTE=unused XFE=unused","x-cbparentid":"17090717-8236-0000-0000-00003D8CD3D6","Message-Id":"<a5d1415c-95c4-05f3-a12e-2f1b7083ac6c@linux.vnet.ibm.com>","X-Proofpoint-Virus-Version":"vendor=fsecure engine=2.50.10432:, ,\n\tdefinitions=2017-09-07_10:, , signatures=0","X-Proofpoint-Spam-Details":"rule=outbound_notspam policy=outbound score=0\n\tspamscore=0 suspectscore=0\n\tmalwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam\n\tadjust=0 reason=mlx scancount=1 engine=8.0.1-1707230000\n\tdefinitions=main-1709070261","X-BeenThere":"openbmc@lists.ozlabs.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"Development list for OpenBMC <openbmc.lists.ozlabs.org>","List-Unsubscribe":"<https://lists.ozlabs.org/options/openbmc>,\n\t<mailto:openbmc-request@lists.ozlabs.org?subject=unsubscribe>","List-Archive":"<http://lists.ozlabs.org/pipermail/openbmc/>","List-Post":"<mailto:openbmc@lists.ozlabs.org>","List-Help":"<mailto:openbmc-request@lists.ozlabs.org?subject=help>","List-Subscribe":"<https://lists.ozlabs.org/listinfo/openbmc>,\n\t<mailto:openbmc-request@lists.ozlabs.org?subject=subscribe>","Cc":"openbmc@lists.ozlabs.org","Errors-To":"openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org","Sender":"\"openbmc\"\n\t<openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org>"}}]