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

Commit Message

Aishwarya Pant Jan. 4, 2018, 2:37 p.m. UTC

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 <[e-mail]>
---
 drivers/iio/industrialio-buffer.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

Comments

Jonathan Cameron Jan. 6, 2018, 12:35 p.m. UTC | #1

On Thu, 4 Jan 2018 20:07:14 +0530
Aishwarya Pant <[e-mail]> 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 <[e-mail]>
> ---
>  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,

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Lars-Peter Clausen Jan. 6, 2018, 12:50 p.m. UTC | #2

On 01/06/2018 01:35 PM, Jonathan Cameron wrote:
> On Thu, 4 Jan 2018 20:07:14 +0530
> Aishwarya Pant <[e-mail]> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Julia Lawall Jan. 6, 2018, 1:18 p.m. UTC | #3

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 <[e-mail]> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jonathan Cameron Jan. 6, 2018, 2:17 p.m. UTC | #4

On Sat, 6 Jan 2018 14:18:58 +0100 (CET)
Julia Lawall <[e-mail]> 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 <[e-mail]> 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 [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Joe Perches Jan. 6, 2018, 3:28 p.m. UTC | #5

(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 <[e-mail]> 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 <[e-mail]>
> > ---
> >  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,
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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,