Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753272AbdHJUX0 (ORCPT ); Thu, 10 Aug 2017 16:23:26 -0400 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:33594 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752840AbdHJUXY (ORCPT ); Thu, 10 Aug 2017 16:23:24 -0400 Subject: Re: [PATCH 4/4] drivers/hwmon/pmbus: Add debugfs for status registers From: Eddie James 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> <318473de-fa13-2bf7-4f08-95db28c57d49@linux.vnet.ibm.com> Date: Thu, 10 Aug 2017 15:23:11 -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: <318473de-fa13-2bf7-4f08-95db28c57d49@linux.vnet.ibm.com> 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-0056-0000-0000-000003B4F2F3 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.00900441; UDB=6.00450800; IPR=6.00680642; 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:23:20 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17081020-0057-0000-0000-000007EB204C Message-Id: 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-1708100320 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 14950 Lines: 386 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. > >> >>> @@ -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 >