[PATCH 1/2] include: linux: iio: add IIO_DEVICE_ATTR_{RO,WO,RW} macros

[PATCH 1/2] include: linux: iio: add IIO_DEVICE_ATTR_{RO,WO,RW} macros

[Brian Masney]

Add three new macros: IIO_DEVICE_ATTR_RO, IIO_DEVICE_ATTR_WO and
IIO_DEVICE_ATTR_RW to reduce the amount of boiler plate code that
is needed for creating new attributes. This mimics the *_RO, *_WO,
and *_RW macros that are found in include/linux/device.h and
include/linux/sysfs.h.

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
 include/linux/iio/sysfs.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/linux/iio/sysfs.h b/include/linux/iio/sysfs.h
index 9cd8f74..f7c6431 100644
--- a/include/linux/iio/sysfs.h
+++ b/include/linux/iio/sysfs.h
@@ -59,6 +59,19 @@ struct iio_const_attr {
 	struct iio_dev_attr iio_dev_attr_##_name		\
 	= IIO_ATTR(_name, _mode, _show, _store, _addr)
 
+#define IIO_DEVICE_ATTR_RO(_name, _addr)			\
+	struct iio_dev_attr iio_dev_attr_##_name		\
+	= IIO_ATTR(_name, S_IRUGO, _name##_show, NULL, _addr)
+
+#define IIO_DEVICE_ATTR_WO(_name, _addr)			\
+	struct iio_dev_attr iio_dev_attr_##_name		\
+	= IIO_ATTR(_name, S_IWUSR, NULL, _name##_store, _addr)
+
+#define IIO_DEVICE_ATTR_RW(_name, _addr)			            \
+	struct iio_dev_attr iio_dev_attr_##_name		            \
+	= IIO_ATTR(_name, (S_IWUSR | S_IRUGO), _name##_show, _name##_store, \
+		   _addr)
+
 #define IIO_DEVICE_ATTR_NAMED(_vname, _name, _mode, _show, _store, _addr) \
 	struct iio_dev_attr iio_dev_attr_##_vname			\
 	= IIO_ATTR(_name, _mode, _show, _store, _addr)
-- 
2.7.4

[PATCH 2/2] staging: iio: isl29018: use IIO_DEVICE_ATTR_{RO,RW} macros

[Brian Masney]

Use the IIO_DEVICE_ATTR_RO and IIO_DEVICE_ATTR_RW macros to
create the device attributes.

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
 drivers/staging/iio/light/isl29018.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/drivers/staging/iio/light/isl29018.c b/drivers/staging/iio/light/isl29018.c
index 76d9f74..055981a 100644
--- a/drivers/staging/iio/light/isl29018.c
+++ b/drivers/staging/iio/light/isl29018.c
@@ -280,8 +280,9 @@ static int isl29018_read_proximity_ir(struct isl29018_chip *chip, int scheme,
 	return 0;
 }
 
-static ssize_t show_scale_available(struct device *dev,
-				    struct device_attribute *attr, char *buf)
+static ssize_t in_illuminance_scale_available_show
+			(struct device *dev, struct device_attribute *attr,
+			 char *buf)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct isl29018_chip *chip = iio_priv(indio_dev);
@@ -297,8 +298,9 @@ static ssize_t show_scale_available(struct device *dev,
 	return len;
 }
 
-static ssize_t show_int_time_available(struct device *dev,
-				       struct device_attribute *attr, char *buf)
+static ssize_t in_illuminance_integration_time_available_show
+			(struct device *dev, struct device_attribute *attr,
+			 char *buf)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct isl29018_chip *chip = iio_priv(indio_dev);
@@ -314,9 +316,9 @@ static ssize_t show_int_time_available(struct device *dev,
 }
 
 /* proximity scheme */
-static ssize_t show_prox_infrared_suppression(struct device *dev,
-					      struct device_attribute *attr,
-					      char *buf)
+static ssize_t proximity_on_chip_ambient_infrared_suppression_show
+			(struct device *dev, struct device_attribute *attr,
+			 char *buf)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct isl29018_chip *chip = iio_priv(indio_dev);
@@ -328,9 +330,9 @@ static ssize_t show_prox_infrared_suppression(struct device *dev,
 	return sprintf(buf, "%d\n", chip->prox_scheme);
 }
 
-static ssize_t store_prox_infrared_suppression(struct device *dev,
-					       struct device_attribute *attr,
-					       const char *buf, size_t count)
+static ssize_t proximity_on_chip_ambient_infrared_suppression_store
+			(struct device *dev, struct device_attribute *attr,
+			 const char *buf, size_t count)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct isl29018_chip *chip = iio_priv(indio_dev);
@@ -490,14 +492,9 @@ static const struct iio_chan_spec isl29023_channels[] = {
 	ISL29018_IR_CHANNEL,
 };
 
-static IIO_DEVICE_ATTR(in_illuminance_integration_time_available, S_IRUGO,
-		       show_int_time_available, NULL, 0);
-static IIO_DEVICE_ATTR(in_illuminance_scale_available, S_IRUGO,
-		      show_scale_available, NULL, 0);
-static IIO_DEVICE_ATTR(proximity_on_chip_ambient_infrared_suppression,
-					S_IRUGO | S_IWUSR,
-					show_prox_infrared_suppression,
-					store_prox_infrared_suppression, 0);
+static IIO_DEVICE_ATTR_RO(in_illuminance_integration_time_available, 0);
+static IIO_DEVICE_ATTR_RO(in_illuminance_scale_available, 0);
+static IIO_DEVICE_ATTR_RW(proximity_on_chip_ambient_infrared_suppression, 0);
 
 #define ISL29018_DEV_ATTR(name) (&iio_dev_attr_##name.dev_attr.attr)
 
-- 
2.7.4

Re: [PATCH 1/2] include: linux: iio: add IIO_DEVICE_ATTR_{RO, WO, RW} macros

[Greg KH]

On Sun, Sep 25, 2016 at 01:29:58PM -0400, Brian Masney wrote:
> Add three new macros: IIO_DEVICE_ATTR_RO, IIO_DEVICE_ATTR_WO and
> IIO_DEVICE_ATTR_RW to reduce the amount of boiler plate code that
> is needed for creating new attributes. This mimics the *_RO, *_WO,
> and *_RW macros that are found in include/linux/device.h and
> include/linux/sysfs.h.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
> ---
>  include/linux/iio/sysfs.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/include/linux/iio/sysfs.h b/include/linux/iio/sysfs.h
> index 9cd8f74..f7c6431 100644
> --- a/include/linux/iio/sysfs.h
> +++ b/include/linux/iio/sysfs.h
> @@ -59,6 +59,19 @@ struct iio_const_attr {
>  	struct iio_dev_attr iio_dev_attr_##_name		\
>  	= IIO_ATTR(_name, _mode, _show, _store, _addr)
>  
> +#define IIO_DEVICE_ATTR_RO(_name, _addr)			\
> +	struct iio_dev_attr iio_dev_attr_##_name		\
> +	= IIO_ATTR(_name, S_IRUGO, _name##_show, NULL, _addr)
> +
> +#define IIO_DEVICE_ATTR_WO(_name, _addr)			\
> +	struct iio_dev_attr iio_dev_attr_##_name		\
> +	= IIO_ATTR(_name, S_IWUSR, NULL, _name##_store, _addr)
> +
> +#define IIO_DEVICE_ATTR_RW(_name, _addr)			            \
> +	struct iio_dev_attr iio_dev_attr_##_name		            \
> +	= IIO_ATTR(_name, (S_IWUSR | S_IRUGO), _name##_show, _name##_store, \
> +		   _addr)
> +
>  #define IIO_DEVICE_ATTR_NAMED(_vname, _name, _mode, _show, _store, _addr) \
>  	struct iio_dev_attr iio_dev_attr_##_vname			\
>  	= IIO_ATTR(_name, _mode, _show, _store, _addr)

Why not make IIO_ATTR_RO, and IIO_ATTR_RW and use DEVICE_ATTR_RO and
DEVICE_ATTR_RW in it, making it a bit more obvious that you got the
settings correct?

Ideally you wouldn't have to manually set a S_* value at all here...

thanks,

greg k-h

[PATCH v2 1/2] include: linux: iio: add IIO_ATTR_{RO,WO,RW} and IIO_DEVICE_ATTR_{RO,WO,RW} macros

[Brian Masney]

Add new macros: IIO_ATTR_RO, IIO_ATTR_WO, IIO_ATTR_RW,
IIO_DEVICE_ATTR_RO, IIO_DEVICE_ATTR_WO and IIO_DEVICE_ATTR_RW to reduce
the amount of boiler plate code that is needed for creating new
attributes. This mimics the *_RO, *_WO, and *_RW macros that are found
in include/linux/device.h and include/linux/sysfs.h.

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
 include/linux/iio/sysfs.h | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/include/linux/iio/sysfs.h b/include/linux/iio/sysfs.h
index 9cd8f74..ce9426c 100644
--- a/include/linux/iio/sysfs.h
+++ b/include/linux/iio/sysfs.h
@@ -55,10 +55,34 @@ struct iio_const_attr {
 	{ .dev_attr = __ATTR(_name, _mode, _show, _store),	\
 	  .address = _addr }
 
+#define IIO_ATTR_RO(_name, _addr)       \
+	{ .dev_attr = __ATTR_RO(_name), \
+	  .address = _addr }
+
+#define IIO_ATTR_WO(_name, _addr)       \
+	{ .dev_attr = __ATTR_WO(_name), \
+	  .address = _addr }
+
+#define IIO_ATTR_RW(_name, _addr)       \
+	{ .dev_attr = __ATTR_RW(_name), \
+	  .address = _addr }
+
 #define IIO_DEVICE_ATTR(_name, _mode, _show, _store, _addr)	\
 	struct iio_dev_attr iio_dev_attr_##_name		\
 	= IIO_ATTR(_name, _mode, _show, _store, _addr)
 
+#define IIO_DEVICE_ATTR_RO(_name, _addr)                       \
+	struct iio_dev_attr iio_dev_attr_##_name                \
+	= IIO_ATTR_RO(_name, _addr)
+
+#define IIO_DEVICE_ATTR_WO(_name, _addr)                       \
+	struct iio_dev_attr iio_dev_attr_##_name                \
+	= IIO_ATTR_WO(_name, _addr)
+
+#define IIO_DEVICE_ATTR_RW(_name, _addr)                                   \
+	struct iio_dev_attr iio_dev_attr_##_name                            \
+	= IIO_ATTR_RW(_name, _addr)
+
 #define IIO_DEVICE_ATTR_NAMED(_vname, _name, _mode, _show, _store, _addr) \
 	struct iio_dev_attr iio_dev_attr_##_vname			\
 	= IIO_ATTR(_name, _mode, _show, _store, _addr)
-- 
2.7.4

Re: [PATCH v2 1/2] include: linux: iio: add IIO_ATTR_{RO, WO, RW} and IIO_DEVICE_ATTR_{RO, WO, RW} macros

[Greg KH]

On Sun, Sep 25, 2016 at 03:27:54PM -0400, Brian Masney wrote:
> Add new macros: IIO_ATTR_RO, IIO_ATTR_WO, IIO_ATTR_RW,
> IIO_DEVICE_ATTR_RO, IIO_DEVICE_ATTR_WO and IIO_DEVICE_ATTR_RW to reduce
> the amount of boiler plate code that is needed for creating new
> attributes. This mimics the *_RO, *_WO, and *_RW macros that are found
> in include/linux/device.h and include/linux/sysfs.h.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
> ---
>  include/linux/iio/sysfs.h | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)


Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

[PATCH v3 1/5] include: linux: iio: add IIO_ATTR_{RO, WO, RW} and IIO_DEVICE_ATTR_{RO, WO, RW} macros

[Brian Masney]

Add new macros: IIO_ATTR_RO, IIO_ATTR_WO, IIO_ATTR_RW,
IIO_DEVICE_ATTR_RO, IIO_DEVICE_ATTR_WO and IIO_DEVICE_ATTR_RW to reduce
the amount of boiler plate code that is needed for creating new
attributes. This mimics the *_RO, *_WO, and *_RW macros that are found
in include/linux/device.h and include/linux/sysfs.h.

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
This is the same version of this patch that I sent out on Sunday. I am
resending the patch set since my the second patch in that series would
not apply cleanly to iio.git/testing.

 include/linux/iio/sysfs.h | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/include/linux/iio/sysfs.h b/include/linux/iio/sysfs.h
index 9cd8f74..ce9426c 100644
--- a/include/linux/iio/sysfs.h
+++ b/include/linux/iio/sysfs.h
@@ -55,10 +55,34 @@ struct iio_const_attr {
 	{ .dev_attr = __ATTR(_name, _mode, _show, _store),	\
 	  .address = _addr }
 
+#define IIO_ATTR_RO(_name, _addr)       \
+	{ .dev_attr = __ATTR_RO(_name), \
+	  .address = _addr }
+
+#define IIO_ATTR_WO(_name, _addr)       \
+	{ .dev_attr = __ATTR_WO(_name), \
+	  .address = _addr }
+
+#define IIO_ATTR_RW(_name, _addr)       \
+	{ .dev_attr = __ATTR_RW(_name), \
+	  .address = _addr }
+
 #define IIO_DEVICE_ATTR(_name, _mode, _show, _store, _addr)	\
 	struct iio_dev_attr iio_dev_attr_##_name		\
 	= IIO_ATTR(_name, _mode, _show, _store, _addr)
 
+#define IIO_DEVICE_ATTR_RO(_name, _addr)                       \
+	struct iio_dev_attr iio_dev_attr_##_name                \
+	= IIO_ATTR_RO(_name, _addr)
+
+#define IIO_DEVICE_ATTR_WO(_name, _addr)                       \
+	struct iio_dev_attr iio_dev_attr_##_name                \
+	= IIO_ATTR_WO(_name, _addr)
+
+#define IIO_DEVICE_ATTR_RW(_name, _addr)                                   \
+	struct iio_dev_attr iio_dev_attr_##_name                            \
+	= IIO_ATTR_RW(_name, _addr)
+
 #define IIO_DEVICE_ATTR_NAMED(_vname, _name, _mode, _show, _store, _addr) \
 	struct iio_dev_attr iio_dev_attr_##_vname			\
 	= IIO_ATTR(_name, _mode, _show, _store, _addr)
-- 
2.7.4

[PATCH v3 2/5] staging: iio: isl29018: use IIO_DEVICE_ATTR_{RO, RW} macros

[Brian Masney]

Use the IIO_DEVICE_ATTR_RO and IIO_DEVICE_ATTR_RW macros to
create the device attributes.

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
This version of the patch now applies cleanly to the iio.git/testing
branch.

 drivers/staging/iio/light/isl29018.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/drivers/staging/iio/light/isl29018.c b/drivers/staging/iio/light/isl29018.c
index 30dc1da..19f282c 100644
--- a/drivers/staging/iio/light/isl29018.c
+++ b/drivers/staging/iio/light/isl29018.c
@@ -268,8 +268,9 @@ static int isl29018_read_proximity_ir(struct isl29018_chip *chip, int scheme,
 	return 0;
 }
 
-static ssize_t isl29018_show_scale_available(struct device *dev,
-				    struct device_attribute *attr, char *buf)
+static ssize_t in_illuminance_scale_available_show
+			(struct device *dev, struct device_attribute *attr,
+			 char *buf)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct isl29018_chip *chip = iio_priv(indio_dev);
@@ -286,8 +287,9 @@ static ssize_t isl29018_show_scale_available(struct device *dev,
 	return len;
 }
 
-static ssize_t isl29018_show_int_time_available(struct device *dev,
-				       struct device_attribute *attr, char *buf)
+static ssize_t in_illuminance_integration_time_available_show
+			(struct device *dev, struct device_attribute *attr,
+			 char *buf)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct isl29018_chip *chip = iio_priv(indio_dev);
@@ -303,9 +305,9 @@ static ssize_t isl29018_show_int_time_available(struct device *dev,
 	return len;
 }
 
-static ssize_t isl29018_show_prox_infrared_suppression(struct device *dev,
-					      struct device_attribute *attr,
-					      char *buf)
+static ssize_t proximity_on_chip_ambient_infrared_suppression_show
+			(struct device *dev, struct device_attribute *attr,
+			 char *buf)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct isl29018_chip *chip = iio_priv(indio_dev);
@@ -317,9 +319,9 @@ static ssize_t isl29018_show_prox_infrared_suppression(struct device *dev,
 	return sprintf(buf, "%d\n", chip->prox_scheme);
 }
 
-static ssize_t isl29018_store_prox_infrared_suppression(struct device *dev,
-					       struct device_attribute *attr,
-					       const char *buf, size_t count)
+static ssize_t proximity_on_chip_ambient_infrared_suppression_store
+			(struct device *dev, struct device_attribute *attr,
+			 const char *buf, size_t count)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct isl29018_chip *chip = iio_priv(indio_dev);
@@ -471,14 +473,9 @@ static const struct iio_chan_spec isl29023_channels[] = {
 	ISL29018_IR_CHANNEL,
 };
 
-static IIO_DEVICE_ATTR(in_illuminance_integration_time_available, S_IRUGO,
-		       isl29018_show_int_time_available, NULL, 0);
-static IIO_DEVICE_ATTR(in_illuminance_scale_available, S_IRUGO,
-		      isl29018_show_scale_available, NULL, 0);
-static IIO_DEVICE_ATTR(proximity_on_chip_ambient_infrared_suppression,
-					S_IRUGO | S_IWUSR,
-					isl29018_show_prox_infrared_suppression,
-					isl29018_store_prox_infrared_suppression, 0);
+static IIO_DEVICE_ATTR_RO(in_illuminance_integration_time_available, 0);
+static IIO_DEVICE_ATTR_RO(in_illuminance_scale_available, 0);
+static IIO_DEVICE_ATTR_RW(proximity_on_chip_ambient_infrared_suppression, 0);
 
 #define ISL29018_DEV_ATTR(name) (&iio_dev_attr_##name.dev_attr.attr)
 
-- 
2.7.4

Re: [PATCH v3 2/5] staging: iio: isl29018: use IIO_DEVICE_ATTR_{RO, RW} macros

[Jonathan Cameron]

On 27/09/16 01:20, Brian Masney wrote:
> Use the IIO_DEVICE_ATTR_RO and IIO_DEVICE_ATTR_RW macros to
> create the device attributes.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
Applied to the togreg branch of iio.git - initially pushed out
as testing for the autobuilders to play with it.

Thanks,

Jonathan
> ---
> This version of the patch now applies cleanly to the iio.git/testing
> branch.
> 
>  drivers/staging/iio/light/isl29018.c | 33 +++++++++++++++------------------
>  1 file changed, 15 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/isl29018.c b/drivers/staging/iio/light/isl29018.c
> index 30dc1da..19f282c 100644
> --- a/drivers/staging/iio/light/isl29018.c
> +++ b/drivers/staging/iio/light/isl29018.c
> @@ -268,8 +268,9 @@ static int isl29018_read_proximity_ir(struct isl29018_chip *chip, int scheme,
>  	return 0;
>  }
>  
> -static ssize_t isl29018_show_scale_available(struct device *dev,
> -				    struct device_attribute *attr, char *buf)
> +static ssize_t in_illuminance_scale_available_show
> +			(struct device *dev, struct device_attribute *attr,
> +			 char *buf)
>  {
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>  	struct isl29018_chip *chip = iio_priv(indio_dev);
> @@ -286,8 +287,9 @@ static ssize_t isl29018_show_scale_available(struct device *dev,
>  	return len;
>  }
>  
> -static ssize_t isl29018_show_int_time_available(struct device *dev,
> -				       struct device_attribute *attr, char *buf)
> +static ssize_t in_illuminance_integration_time_available_show
> +			(struct device *dev, struct device_attribute *attr,
> +			 char *buf)
>  {
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>  	struct isl29018_chip *chip = iio_priv(indio_dev);
> @@ -303,9 +305,9 @@ static ssize_t isl29018_show_int_time_available(struct device *dev,
>  	return len;
>  }
>  
> -static ssize_t isl29018_show_prox_infrared_suppression(struct device *dev,
> -					      struct device_attribute *attr,
> -					      char *buf)
> +static ssize_t proximity_on_chip_ambient_infrared_suppression_show
> +			(struct device *dev, struct device_attribute *attr,
> +			 char *buf)
>  {
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>  	struct isl29018_chip *chip = iio_priv(indio_dev);
> @@ -317,9 +319,9 @@ static ssize_t isl29018_show_prox_infrared_suppression(struct device *dev,
>  	return sprintf(buf, "%d\n", chip->prox_scheme);
>  }
>  
> -static ssize_t isl29018_store_prox_infrared_suppression(struct device *dev,
> -					       struct device_attribute *attr,
> -					       const char *buf, size_t count)
> +static ssize_t proximity_on_chip_ambient_infrared_suppression_store
> +			(struct device *dev, struct device_attribute *attr,
> +			 const char *buf, size_t count)
>  {
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>  	struct isl29018_chip *chip = iio_priv(indio_dev);
> @@ -471,14 +473,9 @@ static const struct iio_chan_spec isl29023_channels[] = {
>  	ISL29018_IR_CHANNEL,
>  };
>  
> -static IIO_DEVICE_ATTR(in_illuminance_integration_time_available, S_IRUGO,
> -		       isl29018_show_int_time_available, NULL, 0);
> -static IIO_DEVICE_ATTR(in_illuminance_scale_available, S_IRUGO,
> -		      isl29018_show_scale_available, NULL, 0);
> -static IIO_DEVICE_ATTR(proximity_on_chip_ambient_infrared_suppression,
> -					S_IRUGO | S_IWUSR,
> -					isl29018_show_prox_infrared_suppression,
> -					isl29018_store_prox_infrared_suppression, 0);
> +static IIO_DEVICE_ATTR_RO(in_illuminance_integration_time_available, 0);
> +static IIO_DEVICE_ATTR_RO(in_illuminance_scale_available, 0);
> +static IIO_DEVICE_ATTR_RW(proximity_on_chip_ambient_infrared_suppression, 0);
>  
>  #define ISL29018_DEV_ATTR(name) (&iio_dev_attr_##name.dev_attr.attr)
>  
> 

[PATCH v3 3/5] staging: iio: isl29018: fixed race condition in in_illuminance_scale_available_show()

[Brian Masney]

in_illuminance_scale_available_show() references the
isl29018_chip->int_time variable in three places inside a for loop.
The value of the int_time variable can be updated by the
isl29018_set_integration_time() function, which is called by the
isl29018_write_raw() function. isl29018_write_raw() locks a
mutex specific to this driver when the integration time variable is
updated.

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
 drivers/staging/iio/light/isl29018.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/iio/light/isl29018.c b/drivers/staging/iio/light/isl29018.c
index 19f282c..990c6e5 100644
--- a/drivers/staging/iio/light/isl29018.c
+++ b/drivers/staging/iio/light/isl29018.c
@@ -277,10 +277,12 @@ static ssize_t in_illuminance_scale_available_show
 	unsigned int i;
 	int len = 0;
 
+	mutex_lock(&chip->lock);
 	for (i = 0; i < ARRAY_SIZE(isl29018_scales[chip->int_time]); ++i)
 		len += sprintf(buf + len, "%d.%06d ",
 			       isl29018_scales[chip->int_time][i].scale,
 			       isl29018_scales[chip->int_time][i].uscale);
+	mutex_unlock(&chip->lock);
 
 	buf[len - 1] = '\n';
 
-- 
2.7.4

Re: [PATCH v3 3/5] staging: iio: isl29018: fixed race condition in in_illuminance_scale_available_show()

[Jonathan Cameron]

On 27/09/16 01:20, Brian Masney wrote:
> in_illuminance_scale_available_show() references the
> isl29018_chip->int_time variable in three places inside a for loop.
> The value of the int_time variable can be updated by the
> isl29018_set_integration_time() function, which is called by the
> isl29018_write_raw() function. isl29018_write_raw() locks a
> mutex specific to this driver when the integration time variable is
> updated.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
This one I can get more enthusiastic about.  Nice catch.
I thought about sending this for stable, but as the effect
is minor probably not worth it.

Applied to the togreg branch of iio.git and pushed out as
testing for the autobuilders to play with it.

Thanks,

Jonathan
> ---
>  drivers/staging/iio/light/isl29018.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/staging/iio/light/isl29018.c b/drivers/staging/iio/light/isl29018.c
> index 19f282c..990c6e5 100644
> --- a/drivers/staging/iio/light/isl29018.c
> +++ b/drivers/staging/iio/light/isl29018.c
> @@ -277,10 +277,12 @@ static ssize_t in_illuminance_scale_available_show
>  	unsigned int i;
>  	int len = 0;
>  
> +	mutex_lock(&chip->lock);
>  	for (i = 0; i < ARRAY_SIZE(isl29018_scales[chip->int_time]); ++i)
>  		len += sprintf(buf + len, "%d.%06d ",
>  			       isl29018_scales[chip->int_time][i].scale,
>  			       isl29018_scales[chip->int_time][i].uscale);
> +	mutex_unlock(&chip->lock);
>  
>  	buf[len - 1] = '\n';
>  
> 

[PATCH v3 5/5] staging: iio: isl29018: check if the chip is in a suspended state

[Brian Masney]

Add a check to isl29018_write_raw() to ensure that the chip is not in a
suspended state. This makes the code consistent with what is present
in isl29018_read_raw().

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
 drivers/staging/iio/light/isl29018.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/iio/light/isl29018.c b/drivers/staging/iio/light/isl29018.c
index 3a4d79d..51226bd 100644
--- a/drivers/staging/iio/light/isl29018.c
+++ b/drivers/staging/iio/light/isl29018.c
@@ -355,6 +355,10 @@ static int isl29018_write_raw(struct iio_dev *indio_dev,
 	int ret = -EINVAL;
 
 	mutex_lock(&chip->lock);
+	if (chip->suspended) {
+		ret = -EBUSY;
+		goto write_done;
+	}
 	switch (mask) {
 	case IIO_CHAN_INFO_CALIBSCALE:
 		if (chan->type == IIO_LIGHT) {
@@ -374,8 +378,9 @@ static int isl29018_write_raw(struct iio_dev *indio_dev,
 	default:
 		break;
 	}
-	mutex_unlock(&chip->lock);
 
+write_done:
+	mutex_unlock(&chip->lock);
 	return ret;
 }
 
-- 
2.7.4

Re: [PATCH v3 5/5] staging: iio: isl29018: check if the chip is in a suspended state

[Jonathan Cameron]

On 27/09/16 01:20, Brian Masney wrote:
> Add a check to isl29018_write_raw() to ensure that the chip is not in a
> suspended state. This makes the code consistent with what is present
> in isl29018_read_raw().
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
Applied to the togreg branch of iio.git.

Out of curiosity, do you actually have one of these?

At a quick glance, the only remaining bit keeping this driver
in staging is the lack of docs on the infrared_supression
attribute.  If you want to add something on that and a patch
moving it out of staging that would be great.

However, note that the graduation patch is usually the one
that gets the driver thoroughly reviewed by several people so
more stuff may come out of the woodwork.

Thanks,

Jonathan

> ---
>  drivers/staging/iio/light/isl29018.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/light/isl29018.c b/drivers/staging/iio/light/isl29018.c
> index 3a4d79d..51226bd 100644
> --- a/drivers/staging/iio/light/isl29018.c
> +++ b/drivers/staging/iio/light/isl29018.c
> @@ -355,6 +355,10 @@ static int isl29018_write_raw(struct iio_dev *indio_dev,
>  	int ret = -EINVAL;
>  
>  	mutex_lock(&chip->lock);
> +	if (chip->suspended) {
> +		ret = -EBUSY;
> +		goto write_done;
> +	}
>  	switch (mask) {
>  	case IIO_CHAN_INFO_CALIBSCALE:
>  		if (chan->type == IIO_LIGHT) {
> @@ -374,8 +378,9 @@ static int isl29018_write_raw(struct iio_dev *indio_dev,
>  	default:
>  		break;
>  	}
> -	mutex_unlock(&chip->lock);
>  
> +write_done:
> +	mutex_unlock(&chip->lock);
>  	return ret;
>  }
>  
> 

Re: [PATCH v3 5/5] staging: iio: isl29018: check if the chip is in a suspended state

[Brian Masney]

On Sat, Oct 01, 2016 at 02:59:49PM +0100, Jonathan Cameron wrote:
> On 27/09/16 01:20, Brian Masney wrote:
> > Add a check to isl29018_write_raw() to ensure that the chip is not in a
> > suspended state. This makes the code consistent with what is present
> > in isl29018_read_raw().
> > 
> > Signed-off-by: Brian Masney <masneyb@onstation.org>
> Applied to the togreg branch of iio.git.
> 
> Out of curiosity, do you actually have one of these?
> 
> At a quick glance, the only remaining bit keeping this driver
> in staging is the lack of docs on the infrared_supression
> attribute.  If you want to add something on that and a patch
> moving it out of staging that would be great.
> 
> However, note that the graduation patch is usually the one
> that gets the driver thoroughly reviewed by several people so
> more stuff may come out of the woodwork.

I do not have a device with this hardware. I picked a random driver in
the IIO subsystem to cleanup with the goal of getting it closer towards
graduation from staging. I'm planning to move on to another driver in
IIO once I can't get any further without having the hardware on
hand.

I see two other issues that need to be addressed with that driver:

- in_illuminance_scale_available_show() and
  in_illuminance_integration_time_available_show() each return four
  different values in their sysfs attribute. My understanding is that
  only a single value should be in the sysfs attribute. If that is the
  case, then should the attributes be something like:

  in_illuminance_scale_available_16
  in_illuminance_scale_available_12
  in_illuminance_scale_available_8
  in_illuminance_scale_available_4

  in_illuminance_integration_time_available_16
  ...

  I got the numbers from the isl29018_int_time enum. If this is
  acceptable, then I'll submit some patches with these changes.

- checkpatch - need to add device tree documentation

I'll also investigate adding the infrared supression documentation
although I'm not sure how far I'll get on that without having the
hardware available.

Brian

Re: [PATCH v3 5/5] staging: iio: isl29018: check if the chip is in a suspended state

[Jonathan Cameron]

On 01/10/16 16:30, Brian Masney wrote:
> On Sat, Oct 01, 2016 at 02:59:49PM +0100, Jonathan Cameron wrote:
>> On 27/09/16 01:20, Brian Masney wrote:
>>> Add a check to isl29018_write_raw() to ensure that the chip is not in a
>>> suspended state. This makes the code consistent with what is present
>>> in isl29018_read_raw().
>>>
>>> Signed-off-by: Brian Masney <masneyb@onstation.org>
>> Applied to the togreg branch of iio.git.
>>
>> Out of curiosity, do you actually have one of these?
>>
>> At a quick glance, the only remaining bit keeping this driver
>> in staging is the lack of docs on the infrared_supression
>> attribute.  If you want to add something on that and a patch
>> moving it out of staging that would be great.
>>
>> However, note that the graduation patch is usually the one
>> that gets the driver thoroughly reviewed by several people so
>> more stuff may come out of the woodwork.
> 
> I do not have a device with this hardware. I picked a random driver in
> the IIO subsystem to cleanup with the goal of getting it closer towards
> graduation from staging. I'm planning to move on to another driver in
> IIO once I can't get any further without having the hardware on
> hand.
> 
> I see two other issues that need to be addressed with that driver:
> 
> - in_illuminance_scale_available_show() and
>   in_illuminance_integration_time_available_show() each return four
>   different values in their sysfs attribute. My understanding is that
>   only a single value should be in the sysfs attribute.
It's considered fine when they are providing the range of values
another sysfs attribute can take.  There are two common ways of
doing that

1) What we do in IIO, just have a separate sysfs attribute to say
what values it can take.
2) Have a list with markings in it.
e.g.
 1 2 4 8 [31] 1003
where the 31 was last written to the attribute.

With hindsight I rather prefer the second, but went with the
first long  ago.  Anyhow, upshot is that these are fine as is.

The argument is they represent one thing.  The range of values
that another attribute can take.  Actually the one value
also extends to things like rotation matrices where they
only have meaning as a set (also quaternions)

> If that is the
>   case, then should the attributes be something like:
> 
>   in_illuminance_scale_available_16
>   in_illuminance_scale_available_12
>   in_illuminance_scale_available_8
>   in_illuminance_scale_available_4
> 
>   in_illuminance_integration_time_available_16
>   ...
That would be hideous!
> 
>   I got the numbers from the isl29018_int_time enum. If this is
>   acceptable, then I'll submit some patches with these changes.
> 
> - checkpatch - need to add device tree documentation
cool. I'd forgotten that.

> 
> I'll also investigate adding the infrared supression documentation
> although I'm not sure how far I'll get on that without having the
> hardware available.
If the docs that are out there are any good then should be doable
without.

Sounds good and we might have this moved very soon by the sound
of it.

Jonathan

> 
> Brian
> 

Re: [PATCH v3 1/5] include: linux: iio: add IIO_ATTR_{RO, WO, RW} and IIO_DEVICE_ATTR_{RO, WO, RW} macros

[Jonathan Cameron]

On 27/09/16 01:20, Brian Masney wrote:
> Add new macros: IIO_ATTR_RO, IIO_ATTR_WO, IIO_ATTR_RW,
> IIO_DEVICE_ATTR_RO, IIO_DEVICE_ATTR_WO and IIO_DEVICE_ATTR_RW to reduce
> the amount of boiler plate code that is needed for creating new
> attributes. This mimics the *_RO, *_WO, and *_RW macros that are found
> in include/linux/device.h and include/linux/sysfs.h.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
Would have been good if you'd picked up Greg's Ack and added it here.

Anyhow, can't say I can summon much enthusiasm for this patch.
To my mind the careful matching of names it requires is all a bit
backwards.

Ah well, there is plenty of precedence and in the long run the intent
is to get rid of pretty much all the custom attrs in drivers (as they
can't easily be used by in kernel consumers).

I'll take it but I don't particularly want to see lots of patches
manically converting drivers over to this.  It's absolutely fine
if (like you are!) they come as part of a series working on a driver
more generally.

Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to play with it.

Thanks,

Jonathan
> ---
> This is the same version of this patch that I sent out on Sunday. I am
> resending the patch set since my the second patch in that series would
> not apply cleanly to iio.git/testing.
> 
>  include/linux/iio/sysfs.h | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/include/linux/iio/sysfs.h b/include/linux/iio/sysfs.h
> index 9cd8f74..ce9426c 100644
> --- a/include/linux/iio/sysfs.h
> +++ b/include/linux/iio/sysfs.h
> @@ -55,10 +55,34 @@ struct iio_const_attr {
>  	{ .dev_attr = __ATTR(_name, _mode, _show, _store),	\
>  	  .address = _addr }
>  
> +#define IIO_ATTR_RO(_name, _addr)       \
> +	{ .dev_attr = __ATTR_RO(_name), \
> +	  .address = _addr }
> +
> +#define IIO_ATTR_WO(_name, _addr)       \
> +	{ .dev_attr = __ATTR_WO(_name), \
> +	  .address = _addr }
> +
> +#define IIO_ATTR_RW(_name, _addr)       \
> +	{ .dev_attr = __ATTR_RW(_name), \
> +	  .address = _addr }
> +
>  #define IIO_DEVICE_ATTR(_name, _mode, _show, _store, _addr)	\
>  	struct iio_dev_attr iio_dev_attr_##_name		\
>  	= IIO_ATTR(_name, _mode, _show, _store, _addr)
>  
> +#define IIO_DEVICE_ATTR_RO(_name, _addr)                       \
> +	struct iio_dev_attr iio_dev_attr_##_name                \
> +	= IIO_ATTR_RO(_name, _addr)
> +
> +#define IIO_DEVICE_ATTR_WO(_name, _addr)                       \
> +	struct iio_dev_attr iio_dev_attr_##_name                \
> +	= IIO_ATTR_WO(_name, _addr)
> +
> +#define IIO_DEVICE_ATTR_RW(_name, _addr)                                   \
> +	struct iio_dev_attr iio_dev_attr_##_name                            \
> +	= IIO_ATTR_RW(_name, _addr)
> +
>  #define IIO_DEVICE_ATTR_NAMED(_vname, _name, _mode, _show, _store, _addr) \
>  	struct iio_dev_attr iio_dev_attr_##_vname			\
>  	= IIO_ATTR(_name, _mode, _show, _store, _addr)
> 

Re: [PATCH v3 4/5] staging: iio: isl29018: change isl29018_read_raw() to only have one exit point

[Jonathan Cameron]

On 27/09/16 01:20, Brian Masney wrote:
> When the chip is in a suspended state, isl29018_read_raw() will return
> -EBUSY. Change the function so that it only has a single exit point.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
Applied.

Thanks,

Jonathan
> ---
>  drivers/staging/iio/light/isl29018.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/isl29018.c b/drivers/staging/iio/light/isl29018.c
> index 990c6e5..3a4d79d 100644
> --- a/drivers/staging/iio/light/isl29018.c
> +++ b/drivers/staging/iio/light/isl29018.c
> @@ -390,8 +390,8 @@ static int isl29018_read_raw(struct iio_dev *indio_dev,
>  
>  	mutex_lock(&chip->lock);
>  	if (chip->suspended) {
> -		mutex_unlock(&chip->lock);
> -		return -EBUSY;
> +		ret = -EBUSY;
> +		goto read_done;
>  	}
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> @@ -438,6 +438,8 @@ static int isl29018_read_raw(struct iio_dev *indio_dev,
>  	default:
>  		break;
>  	}
> +
> +read_done:
>  	mutex_unlock(&chip->lock);
>  	return ret;
>  }
> 

[PATCH v2 2/2] staging: iio: isl29018: use IIO_DEVICE_ATTR_{RO,RW} macros

[Brian Masney]

Use the IIO_DEVICE_ATTR_RO and IIO_DEVICE_ATTR_RW macros to
create the device attributes.

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
 drivers/staging/iio/light/isl29018.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/drivers/staging/iio/light/isl29018.c b/drivers/staging/iio/light/isl29018.c
index 76d9f74..055981a 100644
--- a/drivers/staging/iio/light/isl29018.c
+++ b/drivers/staging/iio/light/isl29018.c
@@ -280,8 +280,9 @@ static int isl29018_read_proximity_ir(struct isl29018_chip *chip, int scheme,
 	return 0;
 }
 
-static ssize_t show_scale_available(struct device *dev,
-				    struct device_attribute *attr, char *buf)
+static ssize_t in_illuminance_scale_available_show
+			(struct device *dev, struct device_attribute *attr,
+			 char *buf)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct isl29018_chip *chip = iio_priv(indio_dev);
@@ -297,8 +298,9 @@ static ssize_t show_scale_available(struct device *dev,
 	return len;
 }
 
-static ssize_t show_int_time_available(struct device *dev,
-				       struct device_attribute *attr, char *buf)
+static ssize_t in_illuminance_integration_time_available_show
+			(struct device *dev, struct device_attribute *attr,
+			 char *buf)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct isl29018_chip *chip = iio_priv(indio_dev);
@@ -314,9 +316,9 @@ static ssize_t show_int_time_available(struct device *dev,
 }
 
 /* proximity scheme */
-static ssize_t show_prox_infrared_suppression(struct device *dev,
-					      struct device_attribute *attr,
-					      char *buf)
+static ssize_t proximity_on_chip_ambient_infrared_suppression_show
+			(struct device *dev, struct device_attribute *attr,
+			 char *buf)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct isl29018_chip *chip = iio_priv(indio_dev);
@@ -328,9 +330,9 @@ static ssize_t show_prox_infrared_suppression(struct device *dev,
 	return sprintf(buf, "%d\n", chip->prox_scheme);
 }
 
-static ssize_t store_prox_infrared_suppression(struct device *dev,
-					       struct device_attribute *attr,
-					       const char *buf, size_t count)
+static ssize_t proximity_on_chip_ambient_infrared_suppression_store
+			(struct device *dev, struct device_attribute *attr,
+			 const char *buf, size_t count)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct isl29018_chip *chip = iio_priv(indio_dev);
@@ -490,14 +492,9 @@ static const struct iio_chan_spec isl29023_channels[] = {
 	ISL29018_IR_CHANNEL,
 };
 
-static IIO_DEVICE_ATTR(in_illuminance_integration_time_available, S_IRUGO,
-		       show_int_time_available, NULL, 0);
-static IIO_DEVICE_ATTR(in_illuminance_scale_available, S_IRUGO,
-		      show_scale_available, NULL, 0);
-static IIO_DEVICE_ATTR(proximity_on_chip_ambient_infrared_suppression,
-					S_IRUGO | S_IWUSR,
-					show_prox_infrared_suppression,
-					store_prox_infrared_suppression, 0);
+static IIO_DEVICE_ATTR_RO(in_illuminance_integration_time_available, 0);
+static IIO_DEVICE_ATTR_RO(in_illuminance_scale_available, 0);
+static IIO_DEVICE_ATTR_RW(proximity_on_chip_ambient_infrared_suppression, 0);
 
 #define ISL29018_DEV_ATTR(name) (&iio_dev_attr_##name.dev_attr.attr)
 
-- 
2.7.4