Messages in this thread | | | Date | Mon, 17 Apr 2023 11:31:46 +0530 | Subject | Re: [PATCH V1 2/3] drivers: misc: dcc: Add driver support for Data Capture and Compare unit(DCC) | From | Souradeep Chowdhury <> |
| |
On 4/15/2023 11:09 AM, Greg Kroah-Hartman wrote: > On Fri, Apr 14, 2023 at 07:29:12PM +0530, Souradeep Chowdhury wrote: >> The DCC is a DMA Engine designed to capture and store data >> during system crash or software triggers. The DCC operates >> based on user inputs via the debugfs interface. The user gives >> addresses as inputs and these addresses are stored in the >> dcc sram. In case of a system crash or a manual software >> trigger by the user through the debugfs interface, >> the dcc captures and stores the values at these addresses. >> This patch contains the driver which has all the methods >> pertaining to the debugfs interface, auxiliary functions to >> support all the four fundamental operations of dcc namely >> read, write, read/modify/write and loop. The probe method >> here instantiates all the resources necessary for dcc to >> operate mainly the dedicated dcc sram where it stores the >> values. The DCC driver can be used for debugging purposes >> without going for a reboot since it can perform software >> triggers as well based on user inputs. > > You have 72 columns, why not use them all please? > >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Copyright (c) 2015-2021, The Linux Foundation. All rights reserved. >> + * Copyright (c) 2022, Qualcomm Innovation Center, Inc. All rights reserved. > > It is now 2023 :)
Ack
> > > > >> + */ >> + >> +#include <linux/bitfield.h> >> +#include <linux/bitops.h> >> +#include <linux/debugfs.h> >> +#include <linux/delay.h> >> +#include <linux/fs.h> >> +#include <linux/io.h> >> +#include <linux/iopoll.h> >> +#include <linux/miscdevice.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/of_device.h> >> +#include <linux/platform_device.h> >> +#include <linux/slab.h> >> +#include <linux/uaccess.h> >> + >> +#define STATUS_READY_TIMEOUT 5000 /* microseconds */ >> + >> +#define DCC_SRAM_NODE "dcc_sram" > > You only use this once, why is a #define needed?
This is as per the comment on version 1 of the patch series on DCC
https://lore.kernel.org/linux-arm-kernel/YElUCaBUOx7hEuIh@builder.lan/
"kzalloc + strlcpy can be replaced by kstrdup(), but that said...all this does seems to be to copy a const string to the heap and lugging it around. Use a define instead."
> >> +static void dcc_create_debug_dir(struct dcc_drvdata *drvdata) >> +{ >> + int i; >> + char list_num[10]; >> + struct dentry *list; >> + struct device *dev = drvdata->dev; >> + >> + drvdata->dbg_dir = debugfs_create_dir(dev_name(dev), NULL); > > You are creating a directory at the root of debugfs with just your > device name? While that will work, that feels very odd. Please use a > subdirectory.
This is as per the comment on v17 of the patch series on DCC
https://lore.kernel.org/linux-arm-kernel/6693993c-bd81-c974-a903-52a62bfec606@ieee.org/
"You never remove this dcc_dbg directory. Why not?
And since you don't, dcc_dbg could just be a local variable here rather than being a global. But it seems to me this is the root directory you want to remove when you're done."
> >> + if (IS_ERR(drvdata->dbg_dir)) { >> + pr_err("can't create debugfs dir\n"); > > There is no need to ever check the return value of a debugfs call. > > Nor do you really ever even need to save off the dentry here, just look > it up when you need to remove it.
Ack
> >> + return; >> + } >> + >> + for (i = 0; i <= drvdata->nr_link_list; i++) { >> + sprintf(list_num, "%d", i); >> + list = debugfs_create_dir(list_num, drvdata->dbg_dir); >> + debugfs_create_file("enable", 0600, list, drvdata, &enable_fops); >> + debugfs_create_file("config", 0600, list, drvdata, &config_fops); >> + } >> + >> + debugfs_create_file("trigger", 0200, drvdata->dbg_dir, drvdata, &trigger_fops); >> + debugfs_create_file("ready", 0400, drvdata->dbg_dir, drvdata, &ready_fops); >> + debugfs_create_file("config_reset", 0200, drvdata->dbg_dir, >> + drvdata, &config_reset_fops); > > This really looks like you are using debugfs to control the device, not > just for debugging information. How are you going to be able to use the > device in a system that has debugfs disabled?
As per upstream discussions it was decided that debugfs will be a suitable interface for DCC. More on this in the following link:-
https://lore.kernel.org/linux-arm-kernel/20221228172825.r32vpphbdulaldvv@builder.lan/
> > thanks, > > greg k-h
| |