Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753378AbdHJUbD (ORCPT ); Thu, 10 Aug 2017 16:31:03 -0400 Received: from bh-25.webhostbox.net ([208.91.199.152]:59175 "EHLO bh-25.webhostbox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752816AbdHJUbC (ORCPT ); Thu, 10 Aug 2017 16:31:02 -0400 Date: Thu, 10 Aug 2017 13:30:59 -0700 From: Guenter Roeck To: Eddie James Cc: jdelvare@suse.com, linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org, joel@jms.id.au, jk@ozlabs.org, andrew@aj.id.au, cbostic@linux.vnet.ibm.com, "Edward A. James" Subject: Re: [PATCH 4/4] drivers/hwmon/pmbus: Add debugfs for status registers Message-ID: <20170810203059.GB13007@roeck-us.net> References: <1502317157-12648-1-git-send-email-eajames@linux.vnet.ibm.com> <1502317157-12648-5-git-send-email-eajames@linux.vnet.ibm.com> <20170810011524.GB6828@roeck-us.net> <318473de-fa13-2bf7-4f08-95db28c57d49@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) X-Authenticated_sender: guenter@roeck-us.net X-OutGoing-Spam-Status: No, score=-1.0 X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - bh-25.webhostbox.net X-AntiAbuse: Original Domain - vger.kernel.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - roeck-us.net X-Get-Message-Sender-Via: bh-25.webhostbox.net: authenticated_id: guenter@roeck-us.net X-Authenticated-Sender: bh-25.webhostbox.net: guenter@roeck-us.net X-Source: X-Source-Args: X-Source-Dir: Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 15520 Lines: 394 On Thu, Aug 10, 2017 at 03:23:11PM -0500, Eddie James wrote: > > > On 08/10/2017 03:15 PM, Eddie James wrote: > > > > > >On 08/09/2017 08:15 PM, Guenter Roeck wrote: > >>On Wed, Aug 09, 2017 at 05:19:17PM -0500, Eddie James wrote: > >>>From: "Edward A. James" > >>> > >>>Export all the available status registers through debugfs, if the > >>>client > >>>driver wants them. > >>> > >>>Signed-off-by: Edward A. James > >>>--- > >>> drivers/hwmon/pmbus/pmbus.c | 24 +++++- > >>> drivers/hwmon/pmbus/pmbus.h | 11 +++ > >>> drivers/hwmon/pmbus/pmbus_core.c | 175 > >>>+++++++++++++++++++++++++++++++++++++++ > >>> 3 files changed, 209 insertions(+), 1 deletion(-) > >>> > >>>diff --git a/drivers/hwmon/pmbus/pmbus.c b/drivers/hwmon/pmbus/pmbus.c > >>>index 7718e58..fc27417 100644 > >>>--- a/drivers/hwmon/pmbus/pmbus.c > >>>+++ b/drivers/hwmon/pmbus/pmbus.c > >>That won't work: SENSORS_PMBUS is independent of PMBUS and does not have > >>to be enabled even if PMBUS is enabled. This will have to be in > >>pmbus_core.c. > >> > >>I would suggest to do the same as done with other drivers. Move > >>pmbus_debugfs_dir into pmbus_core.c and create the directory on probe > >>if it has not already been created. > > > >Hmm, I'm having trouble with this. Don't I have to call > >debugfs_recursive_remove() on the pmbus_debugfs_dir when the pmbus driver > >is unloaded? That can't be done from pmbus_core.c, as that is just device > >probe/remove. I agree it makes sense to only create it if a device is > >going to be using it (and is enabled in the kernel) but I think I need to > >declare it here so I can remove it on unload. > > Sorry was a bit confused about which file was compiled with what CONFIG. I > should create pmbus_debugfs_dir in pmbus_core.c, agreed. But I should still > do the remove in pmbus.c I think. > nack. pmbus.c is logically completely different to pmbus_core.c, and doesn't even have to be enabled. Guenter > > > >> > >>>@@ -18,6 +18,7 @@ > >>> * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. > >>> */ > >>> +#include > >>> #include > >>> #include > >>> #include > >>>@@ -230,7 +231,28 @@ static int pmbus_probe(struct i2c_client *client, > >>> .id_table = pmbus_id, > >>> }; > >>> -module_i2c_driver(pmbus_driver); > >>>+struct dentry *pmbus_debugfs_dir; > >>>+EXPORT_SYMBOL_GPL(pmbus_debugfs_dir); > >>>+ > >>>+static int __init pmbus_init(void) > >>>+{ > >>>+ pmbus_debugfs_dir = debugfs_create_dir(pmbus_driver.driver.name, > >>>NULL); > >>>+ if (IS_ERR(pmbus_debugfs_dir)) > >>>+ /* Don't fail out if we don't have debugfs support. */ > >>>+ pmbus_debugfs_dir = NULL; > >>>+ > >>>+ return i2c_add_driver(&pmbus_driver); > >>>+} > >>>+ > >>>+static void __exit pmbus_exit(void) > >>>+{ > >>>+ debugfs_remove_recursive(pmbus_debugfs_dir); > >>>+ > >>>+ i2c_del_driver(&pmbus_driver); > >>>+} > >>>+ > >>>+module_init(pmbus_init); > >>>+module_exit(pmbus_exit); > >>> MODULE_AUTHOR("Guenter Roeck"); > >>> MODULE_DESCRIPTION("Generic PMBus driver"); > >>>diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h > >>>index bfcb13b..c772b83 100644 > >>>--- a/drivers/hwmon/pmbus/pmbus.h > >>>+++ b/drivers/hwmon/pmbus/pmbus.h > >>>@@ -25,6 +25,8 @@ > >>> #include > >>> #include > >>> +struct dentry; > >>>+ > >>> /* > >>> * Registers > >>> */ > >>>@@ -383,6 +385,12 @@ struct pmbus_driver_info { > >>> /* Regulator functionality, if supported by this chip driver. */ > >>> int num_regulators; > >>> const struct regulator_desc *reg_desc; > >>>+ > >>>+ /* > >>>+ * Controls whether or not to create debugfs entries for this > >>>driver's > >>>+ * devices. > >>>+ */ > >>>+ bool debugfs; > >>> }; > >>> /* Regulator ops */ > >>>@@ -401,6 +409,9 @@ struct pmbus_driver_info { > >>> .owner = THIS_MODULE, \ > >>> } > >>> +/* Handle for debugfs directory */ > >>>+extern struct dentry *pmbus_debugfs_dir; > >>>+ > >>> /* Function declarations */ > >>> void pmbus_clear_cache(struct i2c_client *client); > >>>diff --git a/drivers/hwmon/pmbus/pmbus_core.c > >>>b/drivers/hwmon/pmbus/pmbus_core.c > >>>index 1a51f8f..afbd364 100644 > >>>--- a/drivers/hwmon/pmbus/pmbus_core.c > >>>+++ b/drivers/hwmon/pmbus/pmbus_core.c > >>>@@ -19,6 +19,7 @@ > >>> * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. > >>> */ > >>> +#include > >>> #include > >>> #include > >>> #include > >>>@@ -49,6 +50,9 @@ > >>> #define PB_STATUS_FAN34_BASE (PB_STATUS_FAN_BASE + PMBUS_PAGES) > >>> #define PB_STATUS_TEMP_BASE (PB_STATUS_FAN34_BASE + PMBUS_PAGES) > >>> #define PB_STATUS_INPUT_BASE (PB_STATUS_TEMP_BASE + PMBUS_PAGES) > >>>+#define PB_STATUS_CML_BASE (PB_STATUS_INPUT_BASE + PMBUS_PAGES) > >>>+#define PB_STATUS_OTHER_BASE (PB_STATUS_CML_BASE + PMBUS_PAGES) > >>>+#define PB_STATUS_MFR_BASE (PB_STATUS_OTHER_BASE + PMBUS_PAGES) > >>We are not actively using those status registers, are we ? > >>I am a bit concerned that we'll continuously read those status registers > >>but don't do anything with it most of the time. Ultimately I'd prefer > >>to get rid of all caching, not more, since it gets quite expensive > >>on chips with many pages. > >> > >>Maybe just display uncached status register values in debugfs ? > > > >Sounds good. > > > >> > >>> #define PB_STATUS_VMON_BASE (PB_STATUS_INPUT_BASE + 1) > >>> #define PB_NUM_STATUS_REG (PB_STATUS_VMON_BASE + 1) > >>>@@ -101,6 +105,7 @@ struct pmbus_data { > >>> int num_attributes; > >>> struct attribute_group group; > >>> const struct attribute_group *groups[2]; > >>>+ struct dentry *debugfs; /* debugfs device directory */ > >>> struct pmbus_sensor *sensors; > >>> @@ -119,6 +124,11 @@ struct pmbus_data { > >>> u8 currpage; > >>> }; > >>> +struct pmbus_debugfs_entry { > >>>+ struct device *dev; > >>>+ int index; > >>>+}; > >>>+ > >>> void pmbus_clear_cache(struct i2c_client *client) > >>> { > >>> struct pmbus_data *data = i2c_get_clientdata(client); > >>>@@ -422,6 +432,19 @@ static struct pmbus_data > >>>*pmbus_update_device(struct device *dev) > >>> = _pmbus_read_byte_data(client, i, > >>> s->reg); > >>> } > >>>+ > >>>+ if (!data->debugfs) > >>>+ continue; > >>>+ > >>>+ data->status[PB_STATUS_CML_BASE + i] > >>>+ = _pmbus_read_byte_data(client, i, > >>>+ PMBUS_STATUS_CML); > >>>+ data->status[PB_STATUS_OTHER_BASE + i] > >>>+ = _pmbus_read_byte_data(client, i, > >>>+ PMBUS_STATUS_OTHER); > >>>+ data->status[PB_STATUS_MFR_BASE + i] > >>>+ = _pmbus_read_byte_data(client, i, > >>>+ PMBUS_STATUS_MFR_SPECIFIC); > >>> } > >>> if (info->func[0] & PMBUS_HAVE_STATUS_INPUT) > >>>@@ -1891,6 +1914,151 @@ static int pmbus_regulator_register(struct > >>>pmbus_data *data) > >>> } > >>> #endif > >>> +static int pmbus_debugfs_get(void *data, u64 *val) > >>>+{ > >>>+ struct pmbus_debugfs_entry *entry = data; > >>>+ struct pmbus_data *pdata = pmbus_update_device(entry->dev); > >>>+ > >>>+ *val = pdata->status[entry->index]; > >>>+ > >>>+ return 0; > >>>+} > >>>+ > >>>+DEFINE_DEBUGFS_ATTRIBUTE(pmbus_debugfs_ops8, pmbus_debugfs_get, NULL, > >>>+ "0x%02llx\n"); > >>>+DEFINE_DEBUGFS_ATTRIBUTE(pmbus_debugfs_ops16, pmbus_debugfs_get, NULL, > >>>+ "0x%04llx\n"); > >>>+ > >>>+static int pmbus_init_debugfs(struct i2c_client *client, > >>>+ struct pmbus_data *data) > >>>+{ > >>>+ int i, idx = 0; > >>>+ struct dentry *dbg; > >>>+ char name[PMBUS_NAME_SIZE]; > >>>+ struct pmbus_debugfs_entry *entries; > >>>+ > >>>+ /* > >>>+ * Either user didn't request debugfs or debugfs is not enabled in > >>>+ * kernel. Exit but don't throw an error in these cases. > >>>+ */ > >>Here might be the place to initialize pmbus_debugfs_dir > >>if it is not yet initialized. > >> > >>>+ if (!data->info->debugfs || !pmbus_debugfs_dir) > >>>+ return 0; > >>>+ > >>>+ /* > >>>+ * Create the debugfs directory for this device. Use the hwmon > >>>device > >>>+ * name to avoid conflicts (hwmon numbers are globally unique). > >>>+ */ > >>>+ data->debugfs = debugfs_create_dir(dev_name(data->hwmon_dev), > >>>+ pmbus_debugfs_dir); > >>>+ if (IS_ERR_OR_NULL(data->debugfs)) { > >>>+ data->debugfs = NULL; > >>>+ return -ENODEV; > >>>+ } > >>>+ > >>>+ /* Allocate the max possible entries we need. */ > >>>+ entries = devm_kzalloc(data->dev, > >>>+ sizeof(*entries) * (data->info->pages * 10), > >>>+ GFP_KERNEL); > >>>+ if (!entries) > >>>+ return -ENOMEM; > >>>+ > >>>+ for (i = 0; i < data->info->pages; ++i) { > >>>+ /* check accessibility of status register if it's not page 0 > >>>*/ > >>>+ if (!i || pmbus_check_status_register(client, i)) { > >>>+ entries[idx].dev = data->hwmon_dev; > >>>+ entries[idx].index = PB_STATUS_BASE + i; > >>>+ snprintf(name, PMBUS_NAME_SIZE, "status%d", i); > >>>+ dbg = debugfs_create_file(name, 0444, data->debugfs, > >>>+ &entries[idx++], > >>>+ &pmbus_debugfs_ops16); > >>What is the point of the dbg variable ? > >> > >>>+ } > >>>+ > >>>+ if (data->info->func[i] & PMBUS_HAVE_STATUS_VOUT) { > >>>+ entries[idx].dev = data->hwmon_dev; > >>>+ entries[idx].index = PB_STATUS_VOUT_BASE + i; > >>>+ snprintf(name, PMBUS_NAME_SIZE, "status%d_vout", i); > >>>+ dbg = debugfs_create_file(name, 0444, data->debugfs, > >>>+ &entries[idx++], > >>>+ &pmbus_debugfs_ops8); > >>>+ } > >>>+ > >>>+ if (data->info->func[i] & PMBUS_HAVE_STATUS_IOUT) { > >>>+ entries[idx].dev = data->hwmon_dev; > >>>+ entries[idx].index = PB_STATUS_IOUT_BASE + i; > >>>+ snprintf(name, PMBUS_NAME_SIZE, "status%d_iout", i); > >>>+ dbg = debugfs_create_file(name, 0444, data->debugfs, > >>>+ &entries[idx++], > >>>+ &pmbus_debugfs_ops8); > >>>+ } > >>>+ > >>>+ if (data->info->func[i] & PMBUS_HAVE_STATUS_INPUT) { > >>>+ entries[idx].dev = data->hwmon_dev; > >>>+ entries[idx].index = PB_STATUS_INPUT_BASE + i; > >>>+ snprintf(name, PMBUS_NAME_SIZE, "status%d_input", i); > >>>+ dbg = debugfs_create_file(name, 0444, data->debugfs, > >>>+ &entries[idx++], > >>>+ &pmbus_debugfs_ops8); > >>>+ } > >>>+ > >>>+ if (data->info->func[i] & PMBUS_HAVE_STATUS_TEMP) { > >>>+ entries[idx].dev = data->hwmon_dev; > >>>+ entries[idx].index = PB_STATUS_TEMP_BASE + i; > >>>+ snprintf(name, PMBUS_NAME_SIZE, "status%d_temp", i); > >>>+ dbg = debugfs_create_file(name, 0444, data->debugfs, > >>>+ &entries[idx++], > >>>+ &pmbus_debugfs_ops8); > >>>+ } > >>>+ > >>>+ if (pmbus_check_byte_register(client, i, PMBUS_STATUS_CML)) { > >>>+ entries[idx].dev = data->hwmon_dev; > >>>+ entries[idx].index = PB_STATUS_CML_BASE + i; > >>>+ snprintf(name, PMBUS_NAME_SIZE, "status%d_cml", i); > >>>+ dbg = debugfs_create_file(name, 0444, data->debugfs, > >>>+ &entries[idx++], > >>>+ &pmbus_debugfs_ops8); > >>>+ } > >>>+ > >>>+ if (pmbus_check_byte_register(client, i, PMBUS_STATUS_OTHER)) > >>>{ > >>>+ entries[idx].dev = data->hwmon_dev; > >>>+ entries[idx].index = PB_STATUS_OTHER_BASE + i; > >>>+ snprintf(name, PMBUS_NAME_SIZE, "status%d_other", i); > >>>+ dbg = debugfs_create_file(name, 0444, data->debugfs, > >>>+ &entries[idx++], > >>>+ &pmbus_debugfs_ops8); > >>>+ } > >>>+ > >>>+ if (pmbus_check_byte_register(client, i, > >>>+ PMBUS_STATUS_MFR_SPECIFIC)) { > >>>+ entries[idx].dev = data->hwmon_dev; > >>>+ entries[idx].index = PB_STATUS_MFR_BASE + i; > >>>+ snprintf(name, PMBUS_NAME_SIZE, "status%d_mfr", i); > >>>+ dbg = debugfs_create_file(name, 0444, data->debugfs, > >>>+ &entries[idx++], > >>>+ &pmbus_debugfs_ops8); > >>>+ } > >>>+ > >>>+ if (data->info->func[i] & PMBUS_HAVE_STATUS_FAN12) { > >>>+ entries[idx].dev = data->hwmon_dev; > >>>+ entries[idx].index = PB_STATUS_FAN_BASE + i; > >>>+ snprintf(name, PMBUS_NAME_SIZE, "status%d_fan12", i); > >>>+ dbg = debugfs_create_file(name, 0444, data->debugfs, > >>>+ &entries[idx++], > >>>+ &pmbus_debugfs_ops8); > >>>+ } > >>>+ > >>>+ if (data->info->func[i] & PMBUS_HAVE_STATUS_FAN34) { > >>>+ entries[idx].dev = data->hwmon_dev; > >>>+ entries[idx].index = PB_STATUS_FAN34_BASE + i; > >>>+ snprintf(name, PMBUS_NAME_SIZE, "status%d_fan34", i); > >>>+ dbg = debugfs_create_file(name, 0444, data->debugfs, > >>>+ &entries[idx++], > >>>+ &pmbus_debugfs_ops8); > >>>+ } > >>>+ } > >>>+ > >>>+ return 0; > >>>+} > >>>+ > >>Typically debugfs code is conditional. What happens if DEBUGFS > >>is not enabled ? See below. > >> > >>> int pmbus_do_probe(struct i2c_client *client, const struct > >>>i2c_device_id *id, > >>> struct pmbus_driver_info *info) > >>> { > >>>@@ -1950,6 +2118,10 @@ int pmbus_do_probe(struct i2c_client *client, > >>>const struct i2c_device_id *id, > >>> if (ret) > >>> goto out_unregister; > >>> + ret = pmbus_init_debugfs(client, data); > >>>+ if (ret) > >>>+ dev_warn(dev, "Failed to register debugfs\n"); > >>I think this warning will be generated if debugfs is disabled. > >>We should not do that. Consider putting the debugfs code into > >>#fidef and have a dummy pmbus_init_debugfs() returning 0 if > >>it is disabled. > > > >Yes, good idea. > > > >> > >>>+ > >>> return 0; > >>> out_unregister: > >>>@@ -1963,6 +2135,9 @@ int pmbus_do_probe(struct i2c_client *client, > >>>const struct i2c_device_id *id, > >>> int pmbus_do_remove(struct i2c_client *client) > >>> { > >>> struct pmbus_data *data = i2c_get_clientdata(client); > >>>+ > >>>+ debugfs_remove_recursive(data->debugfs); > >>>+ > >>> hwmon_device_unregister(data->hwmon_dev); > >>> kfree(data->group.attrs); > >>> return 0; > >>>-- > >>>1.8.3.1 > >>> > >>>-- > >>>To unsubscribe from this list: send the line "unsubscribe linux-hwmon" > >>>in > >>>the body of a message to majordomo@vger.kernel.org > >>>More majordomo info at http://vger.kernel.org/majordomo-info.html > > >