[PATCH 0/5] iio: use permission specific variants of DEVICE_ATTR

[PATCH 0/5] iio: use permission specific variants of DEVICE_ATTR

[Aishwarya Pant]

Series of clean-up patches which replace all DEVICE_ATTR macros with the
permission specific variants DEVICE_ATTR_{RO/WO/RW}.

Patches were created using coccinelle. The following script is for conversion to
the DEVICE_ATTR_RO macro and the other conversions are similar. The script may
require some manual intervention when multiple attributes share the show or
store function.

@r@
identifier attr, show_fn;
declarer name DEVICE_ATTR;
@@

DEVICE_ATTR(attr, \(S_IRUGO\|0444\), show_fn, NULL);

@script: python p@
attr_show;
attr << r.attr;
@@

// standardise the show fn name to {attr}_show
coccinelle.attr_show = attr + "_show"

@@
identifier r.attr, r.show_fn;
declarer name DEVICE_ATTR_RO;
@@

// change the attr declaration
- DEVICE_ATTR(attr, \(S_IRUGO\|0444\), show_fn, NULL);
+ DEVICE_ATTR_RO(attr);

@rr@
identifier r.show_fn, p.attr_show;
@@

// rename the show function
- show_fn
+ attr_show
        (...) {
        ...
  }

@depends on rr@
identifier r.show_fn, p.attr_show;
@@

// rename fn usages
- show_fun
+ attr_show

Aishwarya Pant (5):
  iio: buffer: use permission specific variants of DEVICE_ATTR
  iio: core: use permission specific variants of DEVICE_ATTR
  iio: trigger: use permission specific variants of DEVICE_ATTR
  iio: hrtimer: use permission specific variants of DEVICE_ATTR
  iio: trigger: sysfs: use permisssion specific variants of DEVICE_ATTR

 drivers/iio/industrialio-buffer.c      | 25 +++++++++++--------------
 drivers/iio/industrialio-core.c        | 11 +++++------
 drivers/iio/industrialio-trigger.c     | 18 ++++++++----------
 drivers/iio/trigger/iio-trig-hrtimer.c |  8 +++-----
 drivers/iio/trigger/iio-trig-sysfs.c   |  4 ++--
 5 files changed, 29 insertions(+), 37 deletions(-)

-- 
2.15.1

[PATCH 1/5] iio: buffer: use permission specific variants of DEVICE_ATTR

[Aishwarya Pant]

This is a clean-up patch which replaces DEVICE_ATTR macro with the file
permission specific DEVICE_ATTR_{RO/WO/RW} macros for compaction and
readability. Done using coccinelle.

Signed-off-by: Aishwarya Pant <aishpant@gmail.com>
---
 drivers/iio/industrialio-buffer.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index d2b465140a6b..ca565fbcff90 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -497,7 +497,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
 	return ret;
 }
 
-static ssize_t iio_buffer_read_length(struct device *dev,
+static ssize_t length_show(struct device *dev,
 				      struct device_attribute *attr,
 				      char *buf)
 {
@@ -507,7 +507,7 @@ static ssize_t iio_buffer_read_length(struct device *dev,
 	return sprintf(buf, "%d\n", buffer->length);
 }
 
-static ssize_t iio_buffer_write_length(struct device *dev,
+static ssize_t length_store(struct device *dev,
 				       struct device_attribute *attr,
 				       const char *buf, size_t len)
 {
@@ -540,7 +540,7 @@ static ssize_t iio_buffer_write_length(struct device *dev,
 	return ret ? ret : len;
 }
 
-static ssize_t iio_buffer_show_enable(struct device *dev,
+static ssize_t enable_show(struct device *dev,
 				      struct device_attribute *attr,
 				      char *buf)
 {
@@ -1117,7 +1117,7 @@ void iio_disable_all_buffers(struct iio_dev *indio_dev)
 	iio_buffer_deactivate_all(indio_dev);
 }
 
-static ssize_t iio_buffer_store_enable(struct device *dev,
+static ssize_t enable_store(struct device *dev,
 				       struct device_attribute *attr,
 				       const char *buf,
 				       size_t len)
@@ -1153,7 +1153,7 @@ static ssize_t iio_buffer_store_enable(struct device *dev,
 
 static const char * const iio_scan_elements_group_name = "scan_elements";
 
-static ssize_t iio_buffer_show_watermark(struct device *dev,
+static ssize_t watermark_show(struct device *dev,
 					 struct device_attribute *attr,
 					 char *buf)
 {
@@ -1163,7 +1163,7 @@ static ssize_t iio_buffer_show_watermark(struct device *dev,
 	return sprintf(buf, "%u\n", buffer->watermark);
 }
 
-static ssize_t iio_buffer_store_watermark(struct device *dev,
+static ssize_t watermark_store(struct device *dev,
 					  struct device_attribute *attr,
 					  const char *buf,
 					  size_t len)
@@ -1198,16 +1198,13 @@ static ssize_t iio_buffer_store_watermark(struct device *dev,
 	return ret ? ret : len;
 }
 
-static DEVICE_ATTR(length, S_IRUGO | S_IWUSR, iio_buffer_read_length,
-		   iio_buffer_write_length);
+static DEVICE_ATTR_RW(length);
 static struct device_attribute dev_attr_length_ro = __ATTR(length,
-	S_IRUGO, iio_buffer_read_length, NULL);
-static DEVICE_ATTR(enable, S_IRUGO | S_IWUSR,
-		   iio_buffer_show_enable, iio_buffer_store_enable);
-static DEVICE_ATTR(watermark, S_IRUGO | S_IWUSR,
-		   iio_buffer_show_watermark, iio_buffer_store_watermark);
+	S_IRUGO, length_show, NULL);
+static DEVICE_ATTR_RW(enable);
+static DEVICE_ATTR_RW(watermark);
 static struct device_attribute dev_attr_watermark_ro = __ATTR(watermark,
-	S_IRUGO, iio_buffer_show_watermark, NULL);
+	S_IRUGO, watermark_show, NULL);
 
 static struct attribute *iio_buffer_attrs[] = {
 	&dev_attr_length.attr,
-- 
2.15.1

[PATCH 2/5] iio: core: use permission specific variants of DEVICE_ATTR

[Aishwarya Pant]

This is a clean-up patch which replaces DEVICE_ATTR macro with the file
permission specific DEVICE_ATTR_{RO/WO/RW} macros for compaction and
readability. Done using coccinelle.

Signed-off-by: Aishwarya Pant <aishpant@gmail.com>
---
 drivers/iio/industrialio-core.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 19bdf3d2962a..d2ac544f192d 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1253,7 +1253,7 @@ void iio_free_chan_devattr_list(struct list_head *attr_list)
 	}
 }
 
-static ssize_t iio_show_dev_name(struct device *dev,
+static ssize_t name_show(struct device *dev,
 				 struct device_attribute *attr,
 				 char *buf)
 {
@@ -1261,9 +1261,9 @@ static ssize_t iio_show_dev_name(struct device *dev,
 	return snprintf(buf, PAGE_SIZE, "%s\n", indio_dev->name);
 }
 
-static DEVICE_ATTR(name, S_IRUGO, iio_show_dev_name, NULL);
+static DEVICE_ATTR_RO(name);
 
-static ssize_t iio_show_timestamp_clock(struct device *dev,
+static ssize_t current_timestamp_clock_show(struct device *dev,
 					struct device_attribute *attr,
 					char *buf)
 {
@@ -1309,7 +1309,7 @@ static ssize_t iio_show_timestamp_clock(struct device *dev,
 	return sz;
 }
 
-static ssize_t iio_store_timestamp_clock(struct device *dev,
+static ssize_t current_timestamp_clock_store(struct device *dev,
 					 struct device_attribute *attr,
 					 const char *buf, size_t len)
 {
@@ -1340,8 +1340,7 @@ static ssize_t iio_store_timestamp_clock(struct device *dev,
 	return len;
 }
 
-static DEVICE_ATTR(current_timestamp_clock, S_IRUGO | S_IWUSR,
-		   iio_show_timestamp_clock, iio_store_timestamp_clock);
+static DEVICE_ATTR_RW(current_timestamp_clock);
 
 static int iio_device_register_sysfs(struct iio_dev *indio_dev)
 {
-- 
2.15.1

[PATCH 3/5] iio: trigger: use permission specific variants of DEVICE_ATTR

[Aishwarya Pant]

This is a clean-up patch which replaces DEVICE_ATTR macro with the file
permission specific DEVICE_ATTR_{RO/WO/RW} macros for compaction and
readability. Done using coccinelle.

Signed-off-by: Aishwarya Pant <aishpant@gmail.com>
---
 drivers/iio/industrialio-trigger.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
index ce66699c7fcc..d24e49a4bdcc 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -39,7 +39,7 @@ static LIST_HEAD(iio_trigger_list);
 static DEFINE_MUTEX(iio_trigger_list_lock);
 
 /**
- * iio_trigger_read_name() - retrieve useful identifying name
+ * name_show() - retrieve useful identifying name
  * @dev:	device associated with the iio_trigger
  * @attr:	pointer to the device_attribute structure that is
  *		being processed
@@ -48,7 +48,7 @@ static DEFINE_MUTEX(iio_trigger_list_lock);
  * Return: a negative number on failure or the number of written
  *	   characters on success.
  */
-static ssize_t iio_trigger_read_name(struct device *dev,
+static ssize_t name_show(struct device *dev,
 				     struct device_attribute *attr,
 				     char *buf)
 {
@@ -56,7 +56,7 @@ static ssize_t iio_trigger_read_name(struct device *dev,
 	return sprintf(buf, "%s\n", trig->name);
 }
 
-static DEVICE_ATTR(name, S_IRUGO, iio_trigger_read_name, NULL);
+static DEVICE_ATTR_RO(name);
 
 static struct attribute *iio_trig_dev_attrs[] = {
 	&dev_attr_name.attr,
@@ -358,7 +358,7 @@ void iio_dealloc_pollfunc(struct iio_poll_func *pf)
 EXPORT_SYMBOL_GPL(iio_dealloc_pollfunc);
 
 /**
- * iio_trigger_read_current() - trigger consumer sysfs query current trigger
+ * current_trigger_show() - trigger consumer sysfs query current trigger
  * @dev:	device associated with an industrial I/O device
  * @attr:	pointer to the device_attribute structure that
  *		is being processed
@@ -370,7 +370,7 @@ EXPORT_SYMBOL_GPL(iio_dealloc_pollfunc);
  * Return: a negative number on failure, the number of characters written
  *	   on success or 0 if no trigger is available
  */
-static ssize_t iio_trigger_read_current(struct device *dev,
+static ssize_t current_trigger_show(struct device *dev,
 					struct device_attribute *attr,
 					char *buf)
 {
@@ -382,7 +382,7 @@ static ssize_t iio_trigger_read_current(struct device *dev,
 }
 
 /**
- * iio_trigger_write_current() - trigger consumer sysfs set current trigger
+ * current_trigger_store() - trigger consumer sysfs set current trigger
  * @dev:	device associated with an industrial I/O device
  * @attr:	device attribute that is being processed
  * @buf:	string buffer that holds the name of the trigger
@@ -395,7 +395,7 @@ static ssize_t iio_trigger_read_current(struct device *dev,
  * Return: negative error code on failure or length of the buffer
  *	   on success
  */
-static ssize_t iio_trigger_write_current(struct device *dev,
+static ssize_t current_trigger_store(struct device *dev,
 					 struct device_attribute *attr,
 					 const char *buf,
 					 size_t len)
@@ -456,9 +456,7 @@ static ssize_t iio_trigger_write_current(struct device *dev,
 	return ret;
 }
 
-static DEVICE_ATTR(current_trigger, S_IRUGO | S_IWUSR,
-		   iio_trigger_read_current,
-		   iio_trigger_write_current);
+static DEVICE_ATTR_RW(current_trigger);
 
 static struct attribute *iio_trigger_consumer_attrs[] = {
 	&dev_attr_current_trigger.attr,
-- 
2.15.1

[PATCH 4/5] iio: hrtimer: use permission specific variants of DEVICE_ATTR

[Aishwarya Pant]

This is a clean-up patch which replaces DEVICE_ATTR macro with the file
permission specific DEVICE_ATTR_{RO/WO/RW} macros for compaction and
readability. Done using coccinelle.

Signed-off-by: Aishwarya Pant <aishpant@gmail.com>
---
 drivers/iio/trigger/iio-trig-hrtimer.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/trigger/iio-trig-hrtimer.c b/drivers/iio/trigger/iio-trig-hrtimer.c
index 7accd0187ba1..57e46669a5fb 100644
--- a/drivers/iio/trigger/iio-trig-hrtimer.c
+++ b/drivers/iio/trigger/iio-trig-hrtimer.c
@@ -35,7 +35,7 @@ static const struct config_item_type iio_hrtimer_type = {
 };
 
 static
-ssize_t iio_hrtimer_show_sampling_frequency(struct device *dev,
+ssize_t sampling_frequency_show(struct device *dev,
 					    struct device_attribute *attr,
 					    char *buf)
 {
@@ -46,7 +46,7 @@ ssize_t iio_hrtimer_show_sampling_frequency(struct device *dev,
 }
 
 static
-ssize_t iio_hrtimer_store_sampling_frequency(struct device *dev,
+ssize_t sampling_frequency_store(struct device *dev,
 					     struct device_attribute *attr,
 					     const char *buf, size_t len)
 {
@@ -68,9 +68,7 @@ ssize_t iio_hrtimer_store_sampling_frequency(struct device *dev,
 	return len;
 }
 
-static DEVICE_ATTR(sampling_frequency, S_IRUGO | S_IWUSR,
-		   iio_hrtimer_show_sampling_frequency,
-		   iio_hrtimer_store_sampling_frequency);
+static DEVICE_ATTR_RW(sampling_frequency);
 
 static struct attribute *iio_hrtimer_attrs[] = {
 	&dev_attr_sampling_frequency.attr,
-- 
2.15.1

[PATCH 5/5] iio: trigger: sysfs: use permisssion specific variants of DEVICE_ATTR

[Aishwarya Pant]

This is a clean-up patch which replaces DEVICE_ATTR macro with the file
permission specific DEVICE_ATTR_{RO/WO/RW} macros for compaction and
readability. Done using coccinelle.

Signed-off-by: Aishwarya Pant <aishpant@gmail.com>
---
 drivers/iio/trigger/iio-trig-sysfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/trigger/iio-trig-sysfs.c b/drivers/iio/trigger/iio-trig-sysfs.c
index 3f0dc9a1a514..281ed6602f35 100644
--- a/drivers/iio/trigger/iio-trig-sysfs.c
+++ b/drivers/iio/trigger/iio-trig-sysfs.c
@@ -99,7 +99,7 @@ static void iio_sysfs_trigger_work(struct irq_work *work)
 	iio_trigger_poll(trig->trig);
 }
 
-static ssize_t iio_sysfs_trigger_poll(struct device *dev,
+static ssize_t trigger_now_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t count)
 {
 	struct iio_trigger *trig = to_iio_trigger(dev);
@@ -110,7 +110,7 @@ static ssize_t iio_sysfs_trigger_poll(struct device *dev,
 	return count;
 }
 
-static DEVICE_ATTR(trigger_now, S_IWUSR, NULL, iio_sysfs_trigger_poll);
+static DEVICE_ATTR_WO(trigger_now);
 
 static struct attribute *iio_sysfs_trigger_attrs[] = {
 	&dev_attr_trigger_now.attr,
-- 
2.15.1

Re: [PATCH 0/5] iio: use permission specific variants of DEVICE_ATTR

[Joe Perches]

On Thu, 2018-01-04 at 20:06 +0530, Aishwarya Pant wrote:
> Series of clean-up patches which replace all DEVICE_ATTR macros with the
> permission specific variants DEVICE_ATTR_{RO/WO/RW}.
> 
> Patches were created using coccinelle. The following script is for conversion to
> the DEVICE_ATTR_RO macro and the other conversions are similar. The script may
> require some manual intervention when multiple attributes share the show or
> store function.

I do think the perl scripts are a bit better here
as they can verify the show/store functions are
static, used once, the new functions names are
unused and so can be renamed without conflict.

Maybe the python bits can be extended to copy
these perl checks and avoid the possible manual
inspection and verification requirements.

https://lkml.org/lkml/2017/12/22/844

Re: [PATCH 1/5] iio: buffer: use permission specific variants of DEVICE_ATTR

[Jonathan Cameron]

On Thu, 4 Jan 2018 20:07:14 +0530
Aishwarya Pant <aishpant@gmail.com> wrote:

> This is a clean-up patch which replaces DEVICE_ATTR macro with the file
> permission specific DEVICE_ATTR_{RO/WO/RW} macros for compaction and
> readability. Done using coccinelle.

Hmm. I'll be honest, I personally really dislike these macros.
They absolutely don't help with readability because they obscure
the connection between the attributes being created and their callbacks.
Short is not the same as more readable.

Dropping off the prefixes from function names suddenly makes them
appear a lot more generic than they are.  The only advantage is is
standardizes their naming slightly - but that could be done whilst
maintaining the more readable version that makes everything obvious.

I'll think on whether the compactness gains are sufficient to justify their
use.

> 
> Signed-off-by: Aishwarya Pant <aishpant@gmail.com>
> ---
>  drivers/iio/industrialio-buffer.c | 25 +++++++++++--------------
>  1 file changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index d2b465140a6b..ca565fbcff90 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -497,7 +497,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
>  	return ret;
>  }
>  
> -static ssize_t iio_buffer_read_length(struct device *dev,
> +static ssize_t length_show(struct device *dev,
>  				      struct device_attribute *attr,
>  				      char *buf)
>  {
> @@ -507,7 +507,7 @@ static ssize_t iio_buffer_read_length(struct device *dev,
>  	return sprintf(buf, "%d\n", buffer->length);
>  }
>  
> -static ssize_t iio_buffer_write_length(struct device *dev,
> +static ssize_t length_store(struct device *dev,
>  				       struct device_attribute *attr,
>  				       const char *buf, size_t len)
>  {
> @@ -540,7 +540,7 @@ static ssize_t iio_buffer_write_length(struct device *dev,
>  	return ret ? ret : len;
>  }
>  
> -static ssize_t iio_buffer_show_enable(struct device *dev,
> +static ssize_t enable_show(struct device *dev,
>  				      struct device_attribute *attr,
>  				      char *buf)
>  {
> @@ -1117,7 +1117,7 @@ void iio_disable_all_buffers(struct iio_dev *indio_dev)
>  	iio_buffer_deactivate_all(indio_dev);
>  }
>  
> -static ssize_t iio_buffer_store_enable(struct device *dev,
> +static ssize_t enable_store(struct device *dev,
>  				       struct device_attribute *attr,
>  				       const char *buf,
>  				       size_t len)
> @@ -1153,7 +1153,7 @@ static ssize_t iio_buffer_store_enable(struct device *dev,
>  
>  static const char * const iio_scan_elements_group_name = "scan_elements";
>  
> -static ssize_t iio_buffer_show_watermark(struct device *dev,
> +static ssize_t watermark_show(struct device *dev,
>  					 struct device_attribute *attr,
>  					 char *buf)
>  {
> @@ -1163,7 +1163,7 @@ static ssize_t iio_buffer_show_watermark(struct device *dev,
>  	return sprintf(buf, "%u\n", buffer->watermark);
>  }
>  
> -static ssize_t iio_buffer_store_watermark(struct device *dev,
> +static ssize_t watermark_store(struct device *dev,
>  					  struct device_attribute *attr,
>  					  const char *buf,
>  					  size_t len)
> @@ -1198,16 +1198,13 @@ static ssize_t iio_buffer_store_watermark(struct device *dev,
>  	return ret ? ret : len;
>  }
>  
> -static DEVICE_ATTR(length, S_IRUGO | S_IWUSR, iio_buffer_read_length,
> -		   iio_buffer_write_length);
> +static DEVICE_ATTR_RW(length);
>  static struct device_attribute dev_attr_length_ro = __ATTR(length,
> -	S_IRUGO, iio_buffer_read_length, NULL);
> -static DEVICE_ATTR(enable, S_IRUGO | S_IWUSR,
> -		   iio_buffer_show_enable, iio_buffer_store_enable);
> -static DEVICE_ATTR(watermark, S_IRUGO | S_IWUSR,
> -		   iio_buffer_show_watermark, iio_buffer_store_watermark);
> +	S_IRUGO, length_show, NULL);
> +static DEVICE_ATTR_RW(enable);
> +static DEVICE_ATTR_RW(watermark);
>  static struct device_attribute dev_attr_watermark_ro = __ATTR(watermark,
> -	S_IRUGO, iio_buffer_show_watermark, NULL);
> +	S_IRUGO, watermark_show, NULL);
>  
>  static struct attribute *iio_buffer_attrs[] = {
>  	&dev_attr_length.attr,

Re: [PATCH 1/5] iio: buffer: use permission specific variants of DEVICE_ATTR

[Lars-Peter Clausen]

On 01/06/2018 01:35 PM, Jonathan Cameron wrote:
> On Thu, 4 Jan 2018 20:07:14 +0530
> Aishwarya Pant <aishpant@gmail.com> wrote:
> 
>> This is a clean-up patch which replaces DEVICE_ATTR macro with the file
>> permission specific DEVICE_ATTR_{RO/WO/RW} macros for compaction and
>> readability. Done using coccinelle.
> 
> Hmm. I'll be honest, I personally really dislike these macros.
> They absolutely don't help with readability because they obscure
> the connection between the attributes being created and their callbacks.
> Short is not the same as more readable.

FWIW fully agreed.

Re: [PATCH 1/5] iio: buffer: use permission specific variants of DEVICE_ATTR

[Julia Lawall]

On Sat, 6 Jan 2018, Lars-Peter Clausen wrote:

> On 01/06/2018 01:35 PM, Jonathan Cameron wrote:
> > On Thu, 4 Jan 2018 20:07:14 +0530
> > Aishwarya Pant <aishpant@gmail.com> wrote:
> >
> >> This is a clean-up patch which replaces DEVICE_ATTR macro with the file
> >> permission specific DEVICE_ATTR_{RO/WO/RW} macros for compaction and
> >> readability. Done using coccinelle.
> >
> > Hmm. I'll be honest, I personally really dislike these macros.
> > They absolutely don't help with readability because they obscure
> > the connection between the attributes being created and their callbacks.
> > Short is not the same as more readable.
>
> FWIW fully agreed.

Could there be variants that keep the function names as arguments, but
keep the benefit of the simpler permission specification and ensuring that
the right functions are provided for the given permissions?

julia

Re: [PATCH 1/5] iio: buffer: use permission specific variants of DEVICE_ATTR

[Jonathan Cameron]

On Sat, 6 Jan 2018 14:18:58 +0100 (CET)
Julia Lawall <julia.lawall@lip6.fr> wrote:

> On Sat, 6 Jan 2018, Lars-Peter Clausen wrote:
> 
> > On 01/06/2018 01:35 PM, Jonathan Cameron wrote:  
> > > On Thu, 4 Jan 2018 20:07:14 +0530
> > > Aishwarya Pant <aishpant@gmail.com> wrote:
> > >  
> > >> This is a clean-up patch which replaces DEVICE_ATTR macro with the file
> > >> permission specific DEVICE_ATTR_{RO/WO/RW} macros for compaction and
> > >> readability. Done using coccinelle.  
> > >
> > > Hmm. I'll be honest, I personally really dislike these macros.
> > > They absolutely don't help with readability because they obscure
> > > the connection between the attributes being created and their callbacks.
> > > Short is not the same as more readable.  
> >
> > FWIW fully agreed.  
> 
> Could there be variants that keep the function names as arguments, but
> keep the benefit of the simpler permission specification and ensuring that
> the right functions are provided for the given permissions?

Yes, that would bring the benefits without the readability cost.
No one needs to know that RW means some octal number after all.

Jonathan
> 
> julia
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] iio: buffer: use permission specific variants of DEVICE_ATTR

[Joe Perches]

(Adding Greg KH who is I think the biggest proponent of these macros)

On Sat, 2018-01-06 at 12:35 +0000, Jonathan Cameron wrote:
> On Thu, 4 Jan 2018 20:07:14 +0530
> Aishwarya Pant <aishpant@gmail.com> wrote
> 
> > This is a clean-up patch which replaces DEVICE_ATTR macro with the file
> > permission specific DEVICE_ATTR_{RO/WO/RW} macros for compaction and
> > readability. Done using coccinelle.
> 
> Hmm. I'll be honest, I personally really dislike these macros.
> They absolutely don't help with readability because they obscure
> the connection between the attributes being created and their callbacks.
> Short is not the same as more readable.
> 
> Dropping off the prefixes from function names suddenly makes them
> appear a lot more generic than they are.  The only advantage is is
> standardizes their naming slightly - but that could be done whilst
> maintaining the more readable version that makes everything obvious.
> 
> I'll think on whether the compactness gains are sufficient to justify their
> use.
> 
> > 
> > Signed-off-by: Aishwarya Pant <aishpant@gmail.com>
> > ---
> >  drivers/iio/industrialio-buffer.c | 25 +++++++++++--------------
> >  1 file changed, 11 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> > index d2b465140a6b..ca565fbcff90 100644
> > --- a/drivers/iio/industrialio-buffer.c
> > +++ b/drivers/iio/industrialio-buffer.c
> > @@ -497,7 +497,7 @@ static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
> >  	return ret;
> >  }
> >  
> > -static ssize_t iio_buffer_read_length(struct device *dev,
> > +static ssize_t length_show(struct device *dev,
> >  				      struct device_attribute *attr,
> >  				      char *buf)
> >  {
> > @@ -507,7 +507,7 @@ static ssize_t iio_buffer_read_length(struct device *dev,
> >  	return sprintf(buf, "%d\n", buffer->length);
> >  }
> >  
> > -static ssize_t iio_buffer_write_length(struct device *dev,
> > +static ssize_t length_store(struct device *dev,
> >  				       struct device_attribute *attr,
> >  				       const char *buf, size_t len)
> >  {
> > @@ -540,7 +540,7 @@ static ssize_t iio_buffer_write_length(struct device *dev,
> >  	return ret ? ret : len;
> >  }
> >  
> > -static ssize_t iio_buffer_show_enable(struct device *dev,
> > +static ssize_t enable_show(struct device *dev,
> >  				      struct device_attribute *attr,
> >  				      char *buf)
> >  {
> > @@ -1117,7 +1117,7 @@ void iio_disable_all_buffers(struct iio_dev *indio_dev)
> >  	iio_buffer_deactivate_all(indio_dev);
> >  }
> >  
> > -static ssize_t iio_buffer_store_enable(struct device *dev,
> > +static ssize_t enable_store(struct device *dev,
> >  				       struct device_attribute *attr,
> >  				       const char *buf,
> >  				       size_t len)
> > @@ -1153,7 +1153,7 @@ static ssize_t iio_buffer_store_enable(struct device *dev,
> >  
> >  static const char * const iio_scan_elements_group_name = "scan_elements";
> >  
> > -static ssize_t iio_buffer_show_watermark(struct device *dev,
> > +static ssize_t watermark_show(struct device *dev,
> >  					 struct device_attribute *attr,
> >  					 char *buf)
> >  {
> > @@ -1163,7 +1163,7 @@ static ssize_t iio_buffer_show_watermark(struct device *dev,
> >  	return sprintf(buf, "%u\n", buffer->watermark);
> >  }
> >  
> > -static ssize_t iio_buffer_store_watermark(struct device *dev,
> > +static ssize_t watermark_store(struct device *dev,
> >  					  struct device_attribute *attr,
> >  					  const char *buf,
> >  					  size_t len)
> > @@ -1198,16 +1198,13 @@ static ssize_t iio_buffer_store_watermark(struct device *dev,
> >  	return ret ? ret : len;
> >  }
> >  
> > -static DEVICE_ATTR(length, S_IRUGO | S_IWUSR, iio_buffer_read_length,
> > -		   iio_buffer_write_length);
> > +static DEVICE_ATTR_RW(length);
> >  static struct device_attribute dev_attr_length_ro = __ATTR(length,
> > -	S_IRUGO, iio_buffer_read_length, NULL);
> > -static DEVICE_ATTR(enable, S_IRUGO | S_IWUSR,
> > -		   iio_buffer_show_enable, iio_buffer_store_enable);
> > -static DEVICE_ATTR(watermark, S_IRUGO | S_IWUSR,
> > -		   iio_buffer_show_watermark, iio_buffer_store_watermark);
> > +	S_IRUGO, length_show, NULL);
> > +static DEVICE_ATTR_RW(enable);
> > +static DEVICE_ATTR_RW(watermark);
> >  static struct device_attribute dev_attr_watermark_ro = __ATTR(watermark,
> > -	S_IRUGO, iio_buffer_show_watermark, NULL);
> > +	S_IRUGO, watermark_show, NULL);
> >  
> >  static struct attribute *iio_buffer_attrs[] = {
> >  	&dev_attr_length.attr,
> 
>