Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753306AbdHJUQH (ORCPT ); Thu, 10 Aug 2017 16:16:07 -0400 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:45396 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753017AbdHJUQF (ORCPT ); Thu, 10 Aug 2017 16:16:05 -0400 Subject: Re: [PATCH 4/4] drivers/hwmon/pmbus: Add debugfs for status registers To: Guenter Roeck 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" 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> From: Eddie James Date: Thu, 10 Aug 2017 15:15:57 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.0 MIME-Version: 1.0 In-Reply-To: <20170810011524.GB6828@roeck-us.net> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US X-TM-AS-GCONF: 00 x-cbid: 17081020-0024-0000-0000-000002BDE8B1 X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00007521; HX=3.00000241; KW=3.00000007; PH=3.00000004; SC=3.00000219; SDB=6.00900439; UDB=6.00450799; IPR=6.00680640; BA=6.00005522; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00016633; XFM=3.00000015; UTC=2017-08-10 20:16:01 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17081020-0025-0000-0000-0000450E223C Message-Id: <318473de-fa13-2bf7-4f08-95db28c57d49@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-08-10_08:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=2 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1706020000 definitions=main-1708100318 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 12935 Lines: 378 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. > >> @@ -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