[PATCH] Input: edt-ft5x06: Use DEFINE_DEBUGFS_ATTRIBUTE to define debugfs fops

[PATCH] Input: edt-ft5x06: Use DEFINE_DEBUGFS_ATTRIBUTE to define debugfs fops

[Aishwarya Ramakrishnan]

It is more clear to use DEFINE_DEBUGFS_ATTRIBUTE to define debugfs file
operation rather than DEFINE_SIMPLE_ATTRIBUTE.

Signed-off-by: Aishwarya Ramakrishnan <aishwaryarj100@gmail.com>
---
 drivers/input/touchscreen/edt-ft5x06.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index d2587724c52a..7f2070fde2ae 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -676,7 +676,7 @@ static int edt_ft5x06_debugfs_mode_set(void *data, u64 mode)
 	return retval;
 };
 
-DEFINE_SIMPLE_ATTRIBUTE(debugfs_mode_fops, edt_ft5x06_debugfs_mode_get,
+DEFINE_DEBUGFS_ATTRIBUTE(debugfs_mode_fops, edt_ft5x06_debugfs_mode_get,
 			edt_ft5x06_debugfs_mode_set, "%llu\n");
 
 static ssize_t edt_ft5x06_debugfs_raw_data_read(struct file *file,
-- 
2.17.1

Re: [PATCH] Input: edt-ft5x06: Use DEFINE_DEBUGFS_ATTRIBUTE to define debugfs fops

[Greg Kroah-Hartman]

On Tue, May 05, 2020 at 09:03:24PM +0530, Aishwarya Ramakrishnan wrote:
> It is more clear to use DEFINE_DEBUGFS_ATTRIBUTE to define debugfs file
> operation rather than DEFINE_SIMPLE_ATTRIBUTE.

No it is not, why do you think so?

The two defines do different things, that is why we have 2 different
defines.  You can not just replace one with the other without
understanding why one was used and not the other one.

Did you test this change to verify that everything still works
properly?  Why is it needed to be changed at all?

thanks,

greg k-h

[PATCH] Input: edt-ft5x06: Use DEFINE_DEBUGFS_ATTRIBUTE to define debugfs fops

[Aishwarya Ramakrishnan]

From: Aishwarya Ramakrishnan <aishwaryarj100@gmail.com>

On Tue, May 5, 2020 at 11:49 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> On Tue, May 05, 2020 at 09:03:24PM +0530, Aishwarya Ramakrishnan wrote:
>> It is more clear to use DEFINE_DEBUGFS_ATTRIBUTE to define debugfs file
>> operation rather than DEFINE_SIMPLE_ATTRIBUTE.

> No it is not, why do you think so?

This change is suggested by Coccinelle software.
Generated by: scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci

> The two defines do different things, that is why we have 2 different
> defines.  You can not just replace one with the other without
> understanding why one was used and not the other one.

> Did you test this change to verify that everything still works
> properly?  Why is it needed to be changed at all?

DEFINE_SIMPLE_ATTRIBUTE + debugfs_create_file()
imposes some significant overhead as compared to
DEFINE_DEBUGFS_ATTRIBUTE + debugfs_create_file_unsafe().
But I missed to use debugfs_create_file_unsafe() function in the patch.

Re: [PATCH] Input: edt-ft5x06: Use DEFINE_DEBUGFS_ATTRIBUTE to define debugfs fops

[Greg KH]

On Wed, May 06, 2020 at 08:36:22PM +0530, Aishwarya Ramakrishnan wrote:
> From: Aishwarya Ramakrishnan <aishwaryarj100@gmail.com>
> 
> On Tue, May 5, 2020 at 11:49 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > On Tue, May 05, 2020 at 09:03:24PM +0530, Aishwarya Ramakrishnan wrote:
> >> It is more clear to use DEFINE_DEBUGFS_ATTRIBUTE to define debugfs file
> >> operation rather than DEFINE_SIMPLE_ATTRIBUTE.
> 
> > No it is not, why do you think so?
> 
> This change is suggested by Coccinelle software.
> Generated by: scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci

Not a good thing, I do not know why that was added :(

> > The two defines do different things, that is why we have 2 different
> > defines.  You can not just replace one with the other without
> > understanding why one was used and not the other one.
> 
> > Did you test this change to verify that everything still works
> > properly?  Why is it needed to be changed at all?
> 
> DEFINE_SIMPLE_ATTRIBUTE + debugfs_create_file()
> imposes some significant overhead as compared to
> DEFINE_DEBUGFS_ATTRIBUTE + debugfs_create_file_unsafe().

What kind of overhead?  Is it required?

> But I missed to use debugfs_create_file_unsafe() function in the patch.

Yeah, don't use the unsafe stuff unless you know what is happening here
please.

greg k-h