[{"id":1759855,"web_url":"http://patchwork.ozlabs.org/comment/1759855/","msgid":"<1504065563.5933.18.camel@aj.id.au>","list_archive_url":null,"date":"2017-08-30T03:59:23","subject":"Re: [PATCH linux dev-4.10 v5 2/2] hwmon: (ucd9000) Add debugfs for\n\tmfr_status info","submitter":{"id":68332,"url":"http://patchwork.ozlabs.org/api/people/68332/","name":"Andrew Jeffery","email":"andrew@aj.id.au"},"content":"On Tue, 2017-08-29 at 15:08 -0500, Christopher Bostic wrote:\n> Add debugfs to list mfr_status register and its gpiN_fault fields.\n> \n> > Signed-off-by: Christopher Bostic \n> ---\n> v5 - Add unique debugfs dir ID for each ucd9000 sysfs device.\n>    - Change branching return to a !! method.\n>    - Clean up line breaks, other white space.\n>    - Add comments on assumptions made regarding ucd90160 versus other\n>      types.\n> v2 - Remove mfr_status directory in debugfs and place the files\n>      up in the parent ucd9000 directory.\n> ---\n>  drivers/hwmon/pmbus/ucd9000.c | 154 ++++++++++++++++++++++++++++++++++++++++++\n>  1 file changed, 154 insertions(+)\n> \n> diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c\n> index cfd7703..edb026f 100644\n> --- a/drivers/hwmon/pmbus/ucd9000.c\n> +++ b/drivers/hwmon/pmbus/ucd9000.c\n> @@ -19,6 +19,7 @@\n>   * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.\n>   */\n>  \n> +#include \n>  #include \n>  #include \n>  #include \n> @@ -29,6 +30,7 @@\n>  #include \n>  #include \n>  #include \n> +#include \n>  #include \"pmbus.h\"\n>  \n>  enum chips { ucd9000, ucd90120, ucd90124, ucd90160, ucd9090, ucd90910 };\n> @@ -38,6 +40,7 @@\n> >  #define UCD9000_FAN_CONFIG_INDEX\t0xe7\n> >  #define UCD9000_FAN_CONFIG\t\t0xe8\n> >  #define UCD9000_LOGGED_FAULTS\t\t0xea\n> > +#define UCD9000_MFR_STATUS\t\t0xf3\n> >  #define UCD9000_GPIO_SELECT\t\t0xfa\n> >  #define UCD9000_GPIO_CONFIG\t\t0xfb\n> >  #define UCD9000_DEVICE_ID\t\t0xfd\n> @@ -59,17 +62,28 @@\n> >  #define UCD9000_MON_VOLTAGE_HW\t4\n>  \n> >  #define UCD9000_NUM_FAN\t\t4\n> > +#define UCD9000_NAME_SIZE\t24\n>  \n> >  #define UCD9000_GPIO_NAME_LEN\t16\n> >  #define UCD90160_NUM_GPIOS\t26\n> > +#define UCD90160_GPI_COUNT\t8\n> > +#define UCD90160_GPI_FAULT_BASE\t16\n> +\n> +static DEFINE_IDA(ucd9000_ida);\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> > +\tstruct dentry *debugfs;\n>  };\n>  #define to_ucd9000_data(_info) container_of(_info, struct ucd9000_data, info)\n>  \n> +struct ucd9000_debugfs_entry {\n> > +\tstruct i2c_client *client;\n> > +\tu8 index;\n> +};\n> +\n>  static int ucd9000_get_fan_config(struct i2c_client *client, int fan)\n>  {\n> >  \tint fan_config = 0;\n> @@ -297,6 +311,138 @@ static int ucd9000_gpio_direction_output(struct gpio_chip *gc,\n> >  \treturn ucd9000_gpio_set_direction(gc, offset, UCD9000_GPIO_OUTPUT, val);\n>  }\n>  \n> +static struct dentry *ucd9000_debugfs_dir;\n> +\n> +#if IS_ENABLED(CONFIG_DEBUG_FS)\n> +static int ucd9000_get_mfr_status(struct i2c_client *client, u32 *buffer)\n> +{\n> > +\tint ret;\n> +\n> > +\tret = pmbus_set_page(client, 0);\n> > +\tif (ret < 0) {\n> > +\t\tdev_err(&client->dev, \"pmbus_set_page failed. rc:%d\\n\", ret);\n> +\n> > +\t\treturn ret;\n> > +\t}\n> +\n> > +\t/*\n> > +\t * Warning:\n> > +\t *\n> > +\t * Though not currently supported this will cause stack corruption for\n> > +\t * ucd90240!  Command reference, page 81:\n> > +\t *\n> > +\t *    With the ucd90120 and ucd90124 devices, this command [MFR_STATUS]\n> > +\t *    is 2 bytes long (bits 0-15).  With the ucd90240 this command is 5\n> > +\t *    bytes long.  With all other devices, it is 4 bytes long.\n> +\t */\n\nHopefully it won't be missed :)\n\n> +\treturn i2c_smbus_read_block_data(client, UCD9000_MFR_STATUS,\n> > +\t\t\t\t\t(u8 *)buffer);\n> +}\n> +\n> +static int ucd9000_debugfs_get_mfr_status_bit(void *data, u64 *val)\n> +{\n> > +\tstruct ucd9000_debugfs_entry *entry = data;\n> > +\tstruct i2c_client *client = entry->client;\n> > +\tint nr = entry->index;\n> > +\tu32 buffer;\n> > +\tint ret;\n> +\n> > +\tret = ucd9000_get_mfr_status(client, &buffer);\n> > +\tif (ret < 0) {\n> > +\t\tdev_err(&client->dev, \"Failed to read mfr status. rc:%d\\n\",\n> > +\t\t\tret);\n> +\n> > +\t\treturn ret;\n> > +\t}\n> +\n> > +\t*val = !!(ret & BIT(nr));\n> +\n> > +\treturn 0;\n> +}\n> +DEFINE_DEBUGFS_ATTRIBUTE(ucd9000_debugfs_ops_mfr_status_bit,\n> > +\t\tucd9000_debugfs_get_mfr_status_bit, NULL, \"%1lld\\n\");\n> +\n> +static int ucd9000_debugfs_get_mfr_status_word(void *data, u64 *val)\n> +{\n> > +\tstruct ucd9000_debugfs_entry *entry = data;\n> > +\tstruct i2c_client *client = entry->client;\n> > +\tu32 buffer;\n> > +\tint ret;\n> +\n> > +\tret = ucd9000_get_mfr_status(client, &buffer);\n> > +\tif (ret < 0) {\n> > +\t\tdev_err(&client->dev, \"Failed to read mfr status. rc:%d\\n\",\n> > +\t\t\tret);\n> +\n> > +\t\treturn ret;\n> > +\t}\n> +\n> > +\t*val = ret;\n> +\n> > +\treturn 0;\n> +}\n> +DEFINE_DEBUGFS_ATTRIBUTE(ucd9000_debugfs_ops_mfr_status_word,\n> > +\t\tucd9000_debugfs_get_mfr_status_word, NULL, \"%08llx\\n\");\n> +\n> +static int ucd9000_init_debugfs(struct i2c_client *client,\n> > +\t\t\t\tstruct ucd9000_data *data)\n> +{\n> > +\tstruct ucd9000_debugfs_entry *entries;\n> > +\tchar name[UCD9000_NAME_SIZE];\n> > +\tint i;\n> +\n> > +\ti = ida_simple_get(&ucd9000_ida, 0, INT_MAX, GFP_KERNEL);\n> > +\tscnprintf(name, UCD9000_NAME_SIZE, \"ucd9000.%d\", i);\n\nGiven it's debugfs it doesn't matter for the moment, but I do wonder whether we\nshould use the actual device name rather than the driver name here. Some thing\nto fix later.\n\n> > +\tucd9000_debugfs_dir = debugfs_create_dir(name, NULL);\n> > +\tif (IS_ERR(ucd9000_debugfs_dir)) {\n> > +\t\tdev_warn(&client->dev, \"Failed to create debugfs dir: %p\\n\",\n> > +\t\t\tucd9000_debugfs_dir);\n> > +\t\tucd9000_debugfs_dir = NULL;\n> +\n> > +\t\treturn 0;\n> > +\t}\n> +\n> > +\t/*\n> > +\t * Warning:\n> > +\t *\n> > +\t * Makes assumption we're on a ucd90160 type! entries will be different\n> > +\t * sizes for other types.\n> > +\t */\n> > +\tentries = devm_kzalloc(&client->dev, sizeof(*entries) *\n> > +\t\t\t\t(UCD90160_GPI_COUNT + 1) * 10, GFP_KERNEL);\n> > +\tif (!entries)\n> > +\t\treturn -ENOMEM;\n> +\n> > +\t/*\n> > +\t * Warning:\n> > +\t *\n> > +\t * This makes the assumption we're probing a ucd90160 type and how the\n> > +\t * GPI information is organized.  Needs to account for all other\n> > +\t * ucd9000 varieties.\n> > +\t */\n> > +\tfor (i = 0; i < UCD90160_GPI_COUNT; i++) {\n> > +\t\tentries[i].client = client;\n> > +\t\tentries[i].index = UCD90160_GPI_FAULT_BASE + i;\n> > +\t\tscnprintf(name, UCD9000_NAME_SIZE, \"gpi%d_alarm\", i+1);\n> > +\t\tdebugfs_create_file(name, 0444, ucd9000_debugfs_dir,\n> > +\t\t\t\t&entries[i],\n> > +\t\t\t\t&ucd9000_debugfs_ops_mfr_status_bit);\n> > +\t}\n> > +\tentries[i].client = client;\n> > +\tscnprintf(name, UCD9000_NAME_SIZE, \"mfr_status\");\n> > +\tdebugfs_create_file(name, 0444, ucd9000_debugfs_dir, &entries[i],\n> > +\t\t\t&ucd9000_debugfs_ops_mfr_status_word);\n> +\n> > +\treturn 0;\n> +}\n> +#else\n> +static int ucd9000_init_debugfs(struct i2c_client *client,\n> > +\t\t\t\tstruct ucd9000_data *data)\n> +{\n> > +\treturn 0;\n> +}\n> +#endif /* IS_ENABLED(CONFIG_DEBUG_FS) */\n> +\n>  static ssize_t ucd9000_clear_logged_faults(struct device *dev,\n> >  \t\t\t\tstruct device_attribute *attr, const char *buf,\n> >  \t\t\t\tsize_t count)\n> @@ -467,12 +613,20 @@ static int ucd9000_probe(struct i2c_client *client,\n> >  \t\treturn ret;\n> >  \t}\n>  \n> > +\tif (mid->driver_data == ucd90160) {\n> > +\t\tret = ucd9000_init_debugfs(client, data);\n> > +\t\tif (ret < 0)\n> > +\t\t\tdev_warn(&client->dev,\n> > +\t\t\t\t\"Failed to register debugfs: %d\\n\", ret);\n> > +\t}\n> +\n> >  \treturn  pmbus_do_probe(client, mid, info);\n>  }\n>  \n>  static int ucd9000_remove(struct i2c_client *client)\n>  {\n> >  \tsysfs_remove_group(&client->dev.kobj, &ucd9000_attr_group);\n> > +\tdebugfs_remove_recursive(ucd9000_debugfs_dir);\n> >  \treturn pmbus_do_remove(client);\n>  }\n>  \n\nLooks good.\n\nThanks,\n\nAndrew","headers":{"Return-Path":"","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 [103.22.144.68])\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 3xhsG452pfz9s81\n\tfor ;\n\tWed, 30 Aug 2017 13:59:40 +1000 (AEST)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3xhsG43tsSzDqJ9\n\tfor ;\n\tWed, 30 Aug 2017 13:59:40 +1000 (AEST)","from out2-smtp.messagingengine.com (out2-smtp.messagingengine.com\n\t[66.111.4.26])\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 3xhsFx3ZHwzDqGG\n\tfor ; Wed, 30 Aug 2017 13:59:33 +1000 (AEST)","from compute4.internal (compute4.nyi.internal [10.202.2.44])\n\tby mailout.nyi.internal (Postfix) with ESMTP id E90F920F86;\n\tTue, 29 Aug 2017 23:59:30 -0400 (EDT)","from frontend2 ([10.202.2.161])\n\tby compute4.internal (MEProxy); Tue, 29 Aug 2017 23:59:30 -0400","from keelia (unknown [203.0.153.9])\n\tby mail.messagingengine.com (Postfix) with ESMTPA id C25A524871;\n\tTue, 29 Aug 2017 23:59:28 -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=\"YlRHZvBy\";\n\tdkim=pass (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com header.b=\"BhWRHDjs\"; \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=\"YlRHZvBy\";\n\tdkim=pass (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com header.b=\"BhWRHDjs\"; \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=\"YlRHZvBy\";\n\tdkim=pass (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com\n\theader.b=\"BhWRHDjs\"; 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=VeSjbmTES/ok49dDiDL6hUGE03BgAockMcC3Kayay\n\t5I=; b=YlRHZvBySWHwOKnozU4tsnZueW1oZZ96Inh0zmuzamBVLS1stKVqWVOiA\n\trov4gOQKk8PrIewE7FI205lD9cSRHlgx2DWlfROya0nJZZUlEmBNTVnc04NgHk6V\n\tF3Xk4JVCnqkvh+X4zTSlGCF/HNJzSeWnLi2Qnu0fIDcMhWhGE4Nx8mFFncIe97R4\n\tzS4KgHJpvAYKU3iy4wsQC5udnj7bSsfErfU9WvTw+f2rcz5rK3zD2tgfdKeBIFNz\n\ttKUEvpjZl8tHOleMkivqSrxO2EpQIW+N0BaUuKYs4zIJmJNpoTlRgPXqek0mHlT9\n\tAbmXAox0KZdquRhEKC14ElDkUR4Qg==","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=VeSjbmTES/ok49dDiD\n\tL6hUGE03BgAockMcC3Kayay5I=; b=BhWRHDjsxcOHymig3ucUFbhY4nlEH2CMAb\n\t8F0x1gMHbMJJI6ugFrbHhpPatMdEC0f7thF0VbdHbfRXjSgIoO7hW+NfK/M2Ktaa\n\tZOoqu9OyLAOdhnhgNgr/e+T1+EZVnzVMjLc5vFv43xzzPCiVpnyfzS+k6d1PoxIf\n\tyaBRFJ/sIYDExfaL87/umta4O2gBYhDsHeZoDzXh/r49Lwm0nuZslxhIQ9b6H41r\n\tVfpOAip7h0OTK2+K2jtMg8Mxt1a4ra2fp14pe1Uc6RB8LLWMie3heBydQq/MgVmp\n\twiBmySNHSNNyI9Tfi9LH3RR8yJ1veQKuzoBMbGRppMCRApfxdM3Q=="],"X-ME-Sender":"","X-Sasl-enc":"a7iXHyepl58ABLQHUhavskROVQUag+gPoI0F6qRlWCMy 1504065570","Message-ID":"<1504065563.5933.18.camel@aj.id.au>","Subject":"Re: [PATCH linux dev-4.10 v5 2/2] hwmon: (ucd9000) Add debugfs for\n\tmfr_status info","From":"Andrew Jeffery ","To":"Christopher Bostic , joel@jms.id.au","Date":"Wed, 30 Aug 2017 13:29:23 +0930","In-Reply-To":"<20170829200813.66010-3-cbostic@linux.vnet.ibm.com>","References":"<20170829200813.66010-1-cbostic@linux.vnet.ibm.com>\n\t<20170829200813.66010-3-cbostic@linux.vnet.ibm.com>","Content-Type":"multipart/signed; micalg=\"pgp-sha512\";\n\tprotocol=\"application/pgp-signature\";\n\tboundary=\"=-etJ+tVv0G1lRQG4U5+FE\"","X-Mailer":"Evolution 3.22.6-1ubuntu1 ","Mime-Version":"1.0","X-BeenThere":"openbmc@lists.ozlabs.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"Development list for OpenBMC ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"openbmc@lists.ozlabs.org","Errors-To":"openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org","Sender":"\"openbmc\"\n\t"}}]