iio: introduce iio_{claim|release}_direct_mode()
Subsystems (4)
cover iio: introduce iio_{claim|release}_direct_mode()
This patchset introduces two helper functions to simplify driver code requiring the device to be locked in direct mode during execution of a code path. The staging driver ad7192 is updated to demonstrate usage.
This could be applied to approximately 18 known cases where the driver is holding the lock in direct mode. Unknown cases might be those that should, but don't, hold the lock.
Alternate implementation: Generalize to support a claim on any mode. Do iio_claim_mode(device,mode) where if the device is in *mode*, it is guaranteed to stay that way until release is called. I considered and rejected this option because a) not sure other modes would ever need to be locked, and b) the semantic improvement is less when it is generalized.
This patchset was inspired by a discussion on linux-iio:
http://www.spinics.net/lists/linux-iio/msg18540.html
Alison Schofield (2):
iio: core: implement iio_{claim|release}_direct_mode()
staging: iio: adc7192: use iio_{claim|release}_direct_mode()
drivers/iio/industrialio-core.c | 39 +++++++++++++++++++++++++++++++++++++++ drivers/staging/iio/adc/ad7192.c | 24 +++++++++--------------- include/linux/iio/iio.h | 2 ++ 3 files changed, 50 insertions(+), 15 deletions(-)
Signature · 1 line
2.1.4
RFC PATCH 1/2 iio: core: implement iio_{claim|release}_direct_mode()
It is often the case that the driver wants to be sure a device stays in direct mode while it is executing a task or series of tasks. To accomplish this today, the driver performs this sequence: 1) take the device state lock, 2)verify it is not in a buffered mode, 3) execute some tasks, and 4) release that lock.
This patch introduces a pair of helper functions that simplify these steps and make it more semantically expressive.
iio_claim_direct_mode()
If the device is not in any buffered mode it is guaranteed to stay that way until iio_release_direct_mode() is called.
iio_release_direct_mode()
Release the claim. Device is no longer guaranteed to stay in direct mode.
--- drivers/iio/industrialio-core.c | 39 +++++++++++++++++++++++++++++++++++++++ include/linux/iio/iio.h | 2 ++ 2 files changed, 41 insertions(+)
drivers/iio/industrialio-core.c +39 -0
@@ -25,6 +25,7 @@#include <linux/slab.h> #include <linux/anon_inodes.h> #include <linux/debugfs.h> +#include <linux/mutex.h> #include <linux/iio/iio.h> #include "iio_core.h" #include "iio_core_trigger.h"
@@ -1375,6 +1376,44 @@ void devm_iio_device_unregister(struct device *dev, struct iio_dev *indio_dev) }EXPORT_SYMBOL_GPL (devm_iio_device_unregister); +/** +* iio_claim_direct_mode - Keep device in direct mode +* @indio_dev: the iio_dev associated with the device +* +* If the device is in direct mode it is guaranteed to +* stay that way until iio_release_direct_mode() is called. +* +* Use with iio_release_direct_mode() +* +* Returns: 0 on success, -EINVAL on failure +*/ +int iio_claim_direct_mode (struct iio_dev *indio_dev) +{ +mutex_lock (&indio_dev->mlock); + +if (iio_buffer_enabled (indio_dev)) { +mutex_unlock (&indio_dev->mlock); +return -EINVAL ; + } +return 0 ; +} +EXPORT_SYMBOL_GPL (iio_claim_direct_mode); + +/** +* iio_release_direct_mode - releases claim on direct mode +* @indio_dev: the iio_dev associated with the device +* +* Release the claim. Device is no longer guaranteed to stay +* in direct mode. +* +* Use with iio_claim_direct_mode() +*/ +void iio_release_direct_mode (struct iio_dev *indio_dev) +{ +mutex_unlock (&indio_dev->mlock); +} +EXPORT_SYMBOL_GPL (iio_release_direct_mode); +subsys_initcall (iio_init);module_exit (iio_exit);
include/linux/iio/iio.h +2 -0
@@ -527,6 +527,8 @@ void iio_device_unregister(struct iio_dev *indio_dev);int devm_iio_device_register (struct device *dev,struct iio_dev *indio_dev);void devm_iio_device_unregister (struct device *dev,struct iio_dev *indio_dev);int iio_push_event (struct iio_dev *indio_dev,u64 ev_code,s64 timestamp); +int iio_claim_direct_mode (struct iio_dev *indio_dev); +void iio_release_direct_mode (struct iio_dev *indio_dev);extern struct bus_type iio_bus_type;
Signature · 1 line
2.1.4
Re: [RFC PATCH 1/2] iio: core: implement iio_{claim|release}_direct_mode()
Quoted 13 lines · Alison Schofield
It is often the case that the driver wants to be sure a device stays in direct mode while it is executing a task or series of tasks. To accomplish this today, the driver performs this sequence: 1) take the device state lock, 2)verify it is not in a buffered mode, 3) execute some tasks, and 4) release that lock.
This patch introduces a pair of helper functions that simplify these steps and make it more semantically expressive.
iio_claim_direct_mode()
If the device is not in any buffered mode it is guaranteed to stay that way untiliio_release_direct_mode()is called.
Quoted 5 lines
iio_release_direct_mode()
Release the claim. Device is no longer guaranteed to stay in direct mode.Signed-off-by Alison Schofield <[email protected]>
Looks basically good.
Quoted 23 lines
--- drivers/iio/industrialio-core.c | 39 +++++++++++++++++++++++++++++++++++++++ include/linux/iio/iio.h | 2 ++ 2 files changed, 41 insertions(+)drivers/iio/industrialio-core.c +3
-0@@ -25,6 +25,7 @@#include <linux/slab.h> #include <linux/anon_inodes.h> #include <linux/debugfs.h> +#include <linux/mutex.h> #include <linux/iio/iio.h> #include "iio_core.h" #include "iio_core_trigger.h" @@ -1375,6 +1376,44 @@ void devm_iio_device_unregister(struct device *dev, struct iio_dev *indio_dev) }EXPORT_SYMBOL_GPL (devm_iio_device_unregister); +/** +* iio_claim_direct_mode - Keep device in direct mode
drivers/iio/industrialio-core.c void devm_iio_device_unregister(struct device *dev, struct iio_dev *indio_dev) · line 1390
Quoted 10 lines
drivers/iio/industrialio-core.c +10
-0+* @indio_dev: the iio_dev associated with the device +* +* If the device is in direct mode it is guaranteed to +* stay that way until iio_release_direct_mode() is called. +* +* Use with iio_release_direct_mode() +* +* Returns: 0 on success, -EINVAL on failure +*/ +int iio_claim_direct_mode (struct iio_dev *indio_dev)
To be consistent with the reset of the API I'd use the iio_device_... prefix here, same for the release function.
drivers/iio/industrialio-core.c void devm_iio_device_unregister(struct device *dev, struct iio_dev *indio_dev) · line 1396
Quoted 6 lines
drivers/iio/industrialio-core.c +6
-0+{ +mutex_lock (&indio_dev->mlock); + +if (iio_buffer_enabled (indio_dev)) { +mutex_unlock (&indio_dev->mlock); +return -EINVAL ;
-EINVAL doesn't make much sense here, -EBUSY is better.
Quoted 5 lines
drivers/iio/industrialio-core.c +4
-0+ } +return 0 ; +} +EXPORT_SYMBOL_GPL (iio_claim_direct_mode);
Jonathan Cameron
maintainerRe: [RFC PATCH 1/2] iio: core: implement iio_{claim|release}_direct_mode()
Quoted 14 lines · Lars-Peter Clausen
Quoted 13 lines · Alison Schofield
It is often the case that the driver wants to be sure a device stays in direct mode while it is executing a task or series of tasks. To accomplish this today, the driver performs this sequence: 1) take the device state lock, 2)verify it is not in a buffered mode, 3) execute some tasks, and 4) release that lock.
This patch introduces a pair of helper functions that simplify these steps and make it more semantically expressive.
iio_claim_direct_mode()
If the device is not in any buffered mode it is guaranteed to stay that way untiliio_release_direct_mode()is called.
Quoted 7 lines
Quoted 5 lines
iio_release_direct_mode()
Release the claim. Device is no longer guaranteed to stay in direct mode.Signed-off-by Alison Schofield <[email protected]>Looks basically good.
Agreed - nothing to add from me to what Lars has covered here. Nice to 'hide' the accesses to mlock as well as will cut out the desire to 'abuse it'. Amusingly we only just 'fixed' the docs to to say this element of iio_dev was usable by drivers. Once we have these new functions in use throughout the tree, we will want to flip that back again to internal only.
Jonathan
Quoted 52 lines
Quoted 24 lines
--- drivers/iio/industrialio-core.c | 39 +++++++++++++++++++++++++++++++++++++++ include/linux/iio/iio.h | 2 ++ 2 files changed, 41 insertions(+)drivers/iio/industrialio-core.c +3
-0@@ -25,6 +25,7 @@#include <linux/slab.h> #include <linux/anon_inodes.h> #include <linux/debugfs.h> +#include <linux/mutex.h> #include <linux/iio/iio.h> #include "iio_core.h" #include "iio_core_trigger.h" @@ -1375,6 +1376,44 @@ void devm_iio_device_unregister(struct device *dev, struct iio_dev *indio_dev) }EXPORT_SYMBOL_GPL (devm_iio_device_unregister); +/** +* iio_claim_direct_mode - Keep device in direct mode drivers/iio/industrialio-core.c void devm_iio_device_unregister(struct device *dev, struct iio_dev *indio_dev) · line 1390
Quoted 10 lines
drivers/iio/industrialio-core.c +10
-0+* @indio_dev: the iio_dev associated with the device +* +* If the device is in direct mode it is guaranteed to +* stay that way until iio_release_direct_mode() is called. +* +* Use with iio_release_direct_mode() +* +* Returns: 0 on success, -EINVAL on failure +*/ +int iio_claim_direct_mode (struct iio_dev *indio_dev)To be consistent with the reset of the API I'd use the
iio_device_... prefix here, same for the release function.drivers/iio/industrialio-core.c void devm_iio_device_unregister(struct device *dev, struct iio_dev *indio_dev) · line 1396
Quoted 6 lines
drivers/iio/industrialio-core.c +6
-0+{ +mutex_lock (&indio_dev->mlock); + +if (iio_buffer_enabled (indio_dev)) { +mutex_unlock (&indio_dev->mlock); +return -EINVAL ;
-EINVALdoesn't make much sense here,-EBUSYis better.Quoted 5 lines
drivers/iio/industrialio-core.c +4
-0+ } +return 0 ; +} +EXPORT_SYMBOL_GPL (iio_claim_direct_mode);
Re: [RFC PATCH 1/2] iio: core: implement iio_{claim|release}_direct_mode()
Quoted 22 lines · Jonathan Cameron
Quoted 14 lines · Lars-Peter Clausen
Quoted 13 lines · Alison Schofield
It is often the case that the driver wants to be sure a device stays in direct mode while it is executing a task or series of tasks. To accomplish this today, the driver performs this sequence: 1) take the device state lock, 2)verify it is not in a buffered mode, 3) execute some tasks, and 4) release that lock.
This patch introduces a pair of helper functions that simplify these steps and make it more semantically expressive.
iio_claim_direct_mode()
If the device is not in any buffered mode it is guaranteed to stay that way untiliio_release_direct_mode()is called.Quoted 7 lines
Quoted 5 lines
iio_release_direct_mode()
Release the claim. Device is no longer guaranteed to stay in direct mode.Signed-off-by Alison Schofield <[email protected]>Looks basically good.
Quoted 9 lines
Agreed - nothing to add from me to what Lars has covered here. Nice to 'hide' the accesses to mlock as well as will cut out the desire to 'abuse it'. Amusingly we only just 'fixed' the docs to to say this element of
iio_devwas usable by drivers. Once we have these new functions in use throughout the tree, we will want to flip that back again to internal only.Jonathan
Thanks for the review (& Lars too)
Thinking about your note about flipping the mlock field back to INTERNAL (from DRIVER), this change, even when it's applied to all relevant instances, doesn't get us all the way there.
While these claim/release functions will remove direct access to mlock where a driver wants to hold direct mode, the drivers are grabbing mlock for other reasons also. (too many reasons/instances for me to quickly understand or summarize)
I'm willing to look at it further and comment if that's helpful.
alisons
Jonathan Cameron
maintainerRe: [RFC PATCH 1/2] iio: core: implement iio_{claim|release}_direct_mode()
Quoted 38 lines · Alison Schofield
Quoted 22 lines · Jonathan Cameron
Quoted 14 lines · Lars-Peter Clausen
Quoted 13 lines · Alison Schofield
It is often the case that the driver wants to be sure a device stays in direct mode while it is executing a task or series of tasks. To accomplish this today, the driver performs this sequence: 1) take the device state lock, 2)verify it is not in a buffered mode, 3) execute some tasks, and 4) release that lock.
This patch introduces a pair of helper functions that simplify these steps and make it more semantically expressive.
iio_claim_direct_mode()
If the device is not in any buffered mode it is guaranteed to stay that way untiliio_release_direct_mode()is called.Quoted 7 lines
Quoted 5 lines
iio_release_direct_mode()
Release the claim. Device is no longer guaranteed to stay in direct mode.Signed-off-by Alison Schofield <[email protected]>Looks basically good.
Quoted 9 lines
Agreed - nothing to add from me to what Lars has covered here. Nice to 'hide' the accesses to mlock as well as will cut out the desire to 'abuse it'. Amusingly we only just 'fixed' the docs to to say this element of
iio_devwas usable by drivers. Once we have these new functions in use throughout the tree, we will want to flip that back again to internal only.Jonathan
Thanks for the review (& Lars too)
Thinking about your note about flipping the mlock field back to INTERNAL (from DRIVER), this change, even when it's applied to all relevant instances, doesn't get us all the way there.
Quoted 6 lines
While these claim/release functions will remove direct access to mlock where a driver wants to hold direct mode, the drivers are grabbing mlock for other reasons also. (too many reasons/instances for me to quickly understand or summarize)
I'm willing to look at it further and comment if that's helpful.
It would certainly be interesting to evaluate this. I suspect that most are either in some obscure way connected to the mode or are 'misusing' the lock for more general purposes where a driver specific lock would make more sense.
Jonathan
Quoted 12 lines
alisons
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
RFC PATCH 2/2 staging: iio: adc7192: use iio_{claim|release}_direct_mode()
In reply to Alison SchofieldReplace the code that guarantees the device stays in direct mode with iio_{claim|release}_direct_mode() which does same.
--- drivers/staging/iio/adc/ad7192.c | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-)
drivers/staging/iio/adc/ad7192.c +9 -15
@@ -349,11 +349,9 @@ static ssize_t ad7192_write_frequency(struct device *dev,if (lval ==0 )return -EINVAL ; -mutex_lock (&indio_dev->mlock); -if (iio_buffer_enabled (indio_dev)) { -mutex_unlock (&indio_dev->mlock); + ret =iio_claim_direct_mode (indio_dev); +if (ret)return -EBUSY ; - } div = st->mclk / (lval * st->f_order *1024 );if (div <1 || div >1023 ) {
@@ -366,7 +364,7 @@ static ssize_t ad7192_write_frequency(struct device *dev,ad_sd_write_reg (&st->sd,AD7192_REG_MODE ,3 , st->mode); out: -mutex_unlock (&indio_dev->mlock); +iio_release_direct_mode (indio_dev);return ret ? ret : len; }
@@ -434,11 +432,9 @@ static ssize_t ad7192_set(struct device *dev,if (ret <0 )return ret; -mutex_lock (&indio_dev->mlock); -if (iio_buffer_enabled (indio_dev)) { -mutex_unlock (&indio_dev->mlock); + ret =iio_claim_direct_mode (indio_dev); +if (ret)return -EBUSY ; - }switch ((u32 )this_attr->address) {case AD7192_REG_GPOCON :
@@ -461,7 +457,7 @@ static ssize_t ad7192_set(struct device *dev, ret = -EINVAL ; } -mutex_unlock (&indio_dev->mlock); +iio_release_direct_mode (indio_dev);return ret ? ret : len; }
@@ -555,11 +551,9 @@ static int ad7192_write_raw(struct iio_dev *indio_dev,int ret, i;unsigned int tmp; -mutex_lock (&indio_dev->mlock); -if (iio_buffer_enabled (indio_dev)) { -mutex_unlock (&indio_dev->mlock); + ret =iio_claim_direct_mode (indio_dev); +if (ret)return -EBUSY ; - }switch (mask) {case IIO_CHAN_INFO_SCALE :
@@ -582,7 +576,7 @@ static int ad7192_write_raw(struct iio_dev *indio_dev, ret = -EINVAL ; } -mutex_unlock (&indio_dev->mlock); +iio_release_direct_mode (indio_dev);return ret; }
Signature · 1 line
2.1.4
v2 cover iio: introduce iio_device_{claim|release}_direct_mode()
In reply to Alison SchofieldThis patchset introduces two helper functions to simplify driver code requiring the device to be locked in direct mode during execution of a code path. The staging driver ad7192 is updated to demonstrate usage.
This could be applied to approximately 18 known cases where the driver is holding the lock in direct mode. Unknown cases might be those that should, but don't, hold the lock.
Alternate implementation: Generalize to support a claim on any mode. Do iio_claim_mode(device,mode) where if the device is in *mode*, it is guaranteed to stay that way until release is called. I considered and rejected this option because a) not sure other modes would ever need to be locked, and b) the semantic improvement is less when it is generalized.
This patchset was inspired by a discussion on linux-iio:
http://www.spinics.net/lists/linux-iio/msg18540.html
Changes in v2:
o use iio_device prefix for new functions
o replace EINVAL with EBUSY on failure to claim direct mode
o update commit msg & changelog to reflect new prefix
Alison Schofield (2):
iio: core: implement iio_device_{claim|release}_direct_mode()
staging: iio: ad7192: use iio_device_{claim|release}_direct_mode()
drivers/iio/industrialio-core.c | 39 +++++++++++++++++++++++++++++++++++++++ drivers/staging/iio/adc/ad7192.c | 24 +++++++++--------------- include/linux/iio/iio.h | 2 ++ 3 files changed, 50 insertions(+), 15 deletions(-)
Signature · 1 line
2.1.4
RFC PATCH v2 1/2 iio: core: implement iio_device_{claim|release}_direct_mode()
It is often the case that the driver wants to be sure a device stays in direct mode while it is executing a task or series of tasks. To accomplish this today, the driver performs this sequence: 1) take the device state lock, 2) verify it is not in a buffered mode, 3) execute some tasks, and 4) release that lock.
This patch introduces a pair of helper functions that simplify these steps and make it more semantically expressive.
iio_device_claim_direct_mode()
If the device is not in any buffered mode it is guaranteed to stay that way until iio_release_direct_mode() is called.
iio_device_release_direct_mode()
Release the claim. Device is no longer guaranteed to stay in direct mode.
--- drivers/iio/industrialio-core.c | 39 +++++++++++++++++++++++++++++++++++++++ include/linux/iio/iio.h | 2 ++ 2 files changed, 41 insertions(+)
drivers/iio/industrialio-core.c +39 -0
@@ -25,6 +25,7 @@#include <linux/slab.h> #include <linux/anon_inodes.h> #include <linux/debugfs.h> +#include <linux/mutex.h> #include <linux/iio/iio.h> #include "iio_core.h" #include "iio_core_trigger.h"
@@ -1375,6 +1376,44 @@ void devm_iio_device_unregister(struct device *dev, struct iio_dev *indio_dev) }EXPORT_SYMBOL_GPL (devm_iio_device_unregister); +/** +* iio_device_claim_direct_mode - Keep device in direct mode +* @indio_dev: the iio_dev associated with the device +* +* If the device is in direct mode it is guaranteed to stay +* that way until iio_device_release_direct_mode() is called. +* +* Use with iio_device_release_direct_mode() +* +* Returns: 0 on success, -EBUSY on failure +*/ +int iio_device_claim_direct_mode (struct iio_dev *indio_dev) +{ +mutex_lock (&indio_dev->mlock); + +if (iio_buffer_enabled (indio_dev)) { +mutex_unlock (&indio_dev->mlock); +return -EBUSY ; + } +return 0 ; +} +EXPORT_SYMBOL_GPL (iio_device_claim_direct_mode); + +/** +* iio_device_release_direct_mode - releases claim on direct mode +* @indio_dev: the iio_dev associated with the device +* +* Release the claim. Device is no longer guaranteed to stay +* in direct mode. +* +* Use with iio_device_claim_direct_mode() +*/ +void iio_device_release_direct_mode (struct iio_dev *indio_dev) +{ +mutex_unlock (&indio_dev->mlock); +} +EXPORT_SYMBOL_GPL (iio_device_release_direct_mode); +subsys_initcall (iio_init);module_exit (iio_exit);
include/linux/iio/iio.h +2 -0
@@ -527,6 +527,8 @@ void iio_device_unregister(struct iio_dev *indio_dev);int devm_iio_device_register (struct device *dev,struct iio_dev *indio_dev);void devm_iio_device_unregister (struct device *dev,struct iio_dev *indio_dev);int iio_push_event (struct iio_dev *indio_dev,u64 ev_code,s64 timestamp); +int iio_device_claim_direct_mode (struct iio_dev *indio_dev); +void iio_device_release_direct_mode (struct iio_dev *indio_dev);extern struct bus_type iio_bus_type;
Signature · 1 line
2.1.4
Jonathan Cameron
maintainerRe: [RFC PATCH v2 1/2] iio: core: implement iio_device_{claim|release}_direct_mode()
Quoted 13 lines · Alison Schofield
It is often the case that the driver wants to be sure a device stays in direct mode while it is executing a task or series of tasks. To accomplish this today, the driver performs this sequence: 1) take the device state lock, 2) verify it is not in a buffered mode, 3) execute some tasks, and 4) release that lock.
This patch introduces a pair of helper functions that simplify these steps and make it more semantically expressive.
iio_device_claim_direct_mode()
If the device is not in any buffered mode it is guaranteed to stay that way untiliio_release_direct_mode()is called.
Quoted 5 lines
iio_device_release_direct_mode()
Release the claim. Device is no longer guaranteed to stay in direct mode.Signed-off-by Alison Schofield <[email protected]>
I like change. Nice and clean and should help reduce 'misuse' of this lock by making it more focused in the long run.
Applied to the togreg branch of iio.git - initially pushed out as testing for the autobuilders to play with it.
Quoted 76 lines
--- drivers/iio/industrialio-core.c | 39 +++++++++++++++++++++++++++++++++++++++ include/linux/iio/iio.h | 2 ++ 2 files changed, 41 insertions(+)drivers/iio/industrialio-core.c +39
-0@@ -25,6 +25,7 @@#include <linux/slab.h> #include <linux/anon_inodes.h> #include <linux/debugfs.h> +#include <linux/mutex.h> #include <linux/iio/iio.h> #include "iio_core.h" #include "iio_core_trigger.h" @@ -1375,6 +1376,44 @@ void devm_iio_device_unregister(struct device *dev, struct iio_dev *indio_dev) }EXPORT_SYMBOL_GPL (devm_iio_device_unregister); +/** +* iio_device_claim_direct_mode - Keep device in direct mode +* @indio_dev: the iio_dev associated with the device +* +* If the device is in direct mode it is guaranteed to stay +* that way until iio_device_release_direct_mode() is called. +* +* Use with iio_device_release_direct_mode() +* +* Returns: 0 on success, -EBUSY on failure +*/ +int iio_device_claim_direct_mode (struct iio_dev *indio_dev) +{ +mutex_lock (&indio_dev->mlock); + +if (iio_buffer_enabled (indio_dev)) { +mutex_unlock (&indio_dev->mlock); +return -EBUSY ; + } +return 0 ; +} +EXPORT_SYMBOL_GPL (iio_device_claim_direct_mode); + +/** +* iio_device_release_direct_mode - releases claim on direct mode +* @indio_dev: the iio_dev associated with the device +* +* Release the claim. Device is no longer guaranteed to stay +* in direct mode. +* +* Use with iio_device_claim_direct_mode() +*/ +void iio_device_release_direct_mode (struct iio_dev *indio_dev) +{ +mutex_unlock (&indio_dev->mlock); +} +EXPORT_SYMBOL_GPL (iio_device_release_direct_mode); +subsys_initcall (iio_init);module_exit (iio_exit);include/linux/iio/iio.h +2
-0@@ -527,6 +527,8 @@ void iio_device_unregister(struct iio_dev *indio_dev);int devm_iio_device_register (struct device *dev,struct iio_dev *indio_dev);void devm_iio_device_unregister (struct device *dev,struct iio_dev *indio_dev);int iio_push_event (struct iio_dev *indio_dev,u64 ev_code,s64 timestamp); +int iio_device_claim_direct_mode (struct iio_dev *indio_dev); +void iio_device_release_direct_mode (struct iio_dev *indio_dev);extern struct bus_type iio_bus_type;
RFC PATCH v2 2/2 staging: iio: ad7192: use iio_device_{claim|release}_direct_mode()
In reply to Alison SchofieldReplace the code that guarantees the device stays in direct mode with iio_device_{claim|release}_direct_mode() which does same.
--- drivers/staging/iio/adc/ad7192.c | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-)
drivers/staging/iio/adc/ad7192.c +9 -15
@@ -349,11 +349,9 @@ static ssize_t ad7192_write_frequency(struct device *dev,if (lval ==0 )return -EINVAL ; -mutex_lock (&indio_dev->mlock); -if (iio_buffer_enabled (indio_dev)) { -mutex_unlock (&indio_dev->mlock); + ret =iio_device_claim_direct_mode (indio_dev); +if (ret)return -EBUSY ; - } div = st->mclk / (lval * st->f_order *1024 );if (div <1 || div >1023 ) {
@@ -366,7 +364,7 @@ static ssize_t ad7192_write_frequency(struct device *dev,ad_sd_write_reg (&st->sd,AD7192_REG_MODE ,3 , st->mode); out: -mutex_unlock (&indio_dev->mlock); +iio_device_release_direct_mode (indio_dev);return ret ? ret : len; }
@@ -434,11 +432,9 @@ static ssize_t ad7192_set(struct device *dev,if (ret <0 )return ret; -mutex_lock (&indio_dev->mlock); -if (iio_buffer_enabled (indio_dev)) { -mutex_unlock (&indio_dev->mlock); + ret =iio_device_claim_direct_mode (indio_dev); +if (ret)return -EBUSY ; - }switch ((u32 )this_attr->address) {case AD7192_REG_GPOCON :
@@ -461,7 +457,7 @@ static ssize_t ad7192_set(struct device *dev, ret = -EINVAL ; } -mutex_unlock (&indio_dev->mlock); +iio_device_release_direct_mode (indio_dev);return ret ? ret : len; }
@@ -555,11 +551,9 @@ static int ad7192_write_raw(struct iio_dev *indio_dev,int ret, i;unsigned int tmp; -mutex_lock (&indio_dev->mlock); -if (iio_buffer_enabled (indio_dev)) { -mutex_unlock (&indio_dev->mlock); + ret =iio_device_claim_direct_mode (indio_dev); +if (ret)return -EBUSY ; - }switch (mask) {case IIO_CHAN_INFO_SCALE :
@@ -582,7 +576,7 @@ static int ad7192_write_raw(struct iio_dev *indio_dev, ret = -EINVAL ; } -mutex_unlock (&indio_dev->mlock); +iio_device_release_direct_mode (indio_dev);return ret; }
Signature · 1 line
2.1.4
Jonathan Cameron
maintainerRe: [RFC PATCH v2 2/2] staging: iio: ad7192: use iio_device_{claim|release}_direct_mode()
Quoted 4 lines · Alison Schofield
Replace the code that guarantees the device stays in direct mode with iio_device_{claim|release}_direct_mode() which does same.
Signed-off-by Alison Schofield <[email protected]>
A small improvement inline - don't eat the errors within the driver.
Jonathan
Quoted 9 lines
--- drivers/staging/iio/adc/ad7192.c | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-)drivers/staging/iio/adc/ad7192.c +0
-0
drivers/staging/iio/adc/ad7192.c static ssize_t ad7192_write_frequency(struct device *dev, · line 354
Quoted 9 lines
drivers/staging/iio/adc/ad7192.c +2
-3@@ -349,11 +349,9 @@ static ssize_t ad7192_write_frequency(struct device *dev,if (lval ==0 )return -EINVAL ; -mutex_lock (&indio_dev->mlock); -if (iio_buffer_enabled (indio_dev)) { -mutex_unlock (&indio_dev->mlock); + ret =iio_device_claim_direct_mode (indio_dev); +if (ret)return -EBUSY ;
return ret; In theory we might be returning some other error code so best not to 'eat' the result.
Quoted 14 lines
drivers/staging/iio/adc/ad7192.c +1
-2- } div = st->mclk / (lval * st->f_order *1024 );if (div <1 || div >1023 ) {@@ -366,7 +364,7 @@ static ssize_t ad7192_write_frequency(struct device *dev,ad_sd_write_reg (&st->sd,AD7192_REG_MODE ,3 , st->mode); out: -mutex_unlock (&indio_dev->mlock); +iio_device_release_direct_mode (indio_dev);return ret ? ret : len; }
drivers/staging/iio/adc/ad7192.c static ssize_t ad7192_set(struct device *dev, · line 437
Quoted 9 lines
drivers/staging/iio/adc/ad7192.c +2
-3@@ -434,11 +432,9 @@ static ssize_t ad7192_set(struct device *dev,if (ret <0 )return ret; -mutex_lock (&indio_dev->mlock); -if (iio_buffer_enabled (indio_dev)) { -mutex_unlock (&indio_dev->mlock); + ret =iio_device_claim_direct_mode (indio_dev); +if (ret)return -EBUSY ;
Same here.
Quoted 14 lines
drivers/staging/iio/adc/ad7192.c +1
-2- }switch ((u32 )this_attr->address) {case AD7192_REG_GPOCON :@@ -461,7 +457,7 @@ static ssize_t ad7192_set(struct device *dev, ret = -EINVAL ; } -mutex_unlock (&indio_dev->mlock); +iio_device_release_direct_mode (indio_dev);return ret ? ret : len; }
drivers/staging/iio/adc/ad7192.c static int ad7192_write_raw(struct iio_dev *indio_dev, · line 556
Quoted 9 lines
drivers/staging/iio/adc/ad7192.c +2
-3@@ -555,11 +551,9 @@ static int ad7192_write_raw(struct iio_dev *indio_dev,int ret, i;unsigned int tmp; -mutex_lock (&indio_dev->mlock); -if (iio_buffer_enabled (indio_dev)) { -mutex_unlock (&indio_dev->mlock); + ret =iio_device_claim_direct_mode (indio_dev); +if (ret)return -EBUSY ;
And here.
Quoted 14 lines
drivers/staging/iio/adc/ad7192.c +1
-2- }switch (mask) {case IIO_CHAN_INFO_SCALE :@@ -582,7 +576,7 @@ static int ad7192_write_raw(struct iio_dev *indio_dev, ret = -EINVAL ; } -mutex_unlock (&indio_dev->mlock); +iio_device_release_direct_mode (indio_dev);return ret; }
Jonathan Cameron
maintainerRe: [RFC PATCH v2 2/2] staging: iio: ad7192: use iio_device_{claim|release}_direct_mode()
Quoted 7 lines · Jonathan Cameron
Quoted 4 lines · Alison Schofield
Replace the code that guarantees the device stays in direct mode with iio_device_{claim|release}_direct_mode() which does same.
Signed-off-by Alison Schofield <[email protected]>A small improvement inline - don't eat the errors within the driver.
As it is trivial - I've fixed it up as suggested and applied. Seemed silly to go round again for such a small change.
I'm hoping you have the opportunity to take this tree wide.
Thanks,
Jonathan
Quoted 90 lines
Jonathan
Quoted 9 lines
--- drivers/staging/iio/adc/ad7192.c | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-)drivers/staging/iio/adc/ad7192.c +0
-0drivers/staging/iio/adc/ad7192.c static ssize_t ad7192_write_frequency(struct device *dev, · line 354
Quoted 9 lines
drivers/staging/iio/adc/ad7192.c +2
-3@@ -349,11 +349,9 @@ static ssize_t ad7192_write_frequency(struct device *dev,if (lval ==0 )return -EINVAL ; -mutex_lock (&indio_dev->mlock); -if (iio_buffer_enabled (indio_dev)) { -mutex_unlock (&indio_dev->mlock); + ret =iio_device_claim_direct_mode (indio_dev); +if (ret)return -EBUSY ;return ret; In theory we might be returning some other error code so best not to 'eat' the result.
Quoted 14 lines
drivers/staging/iio/adc/ad7192.c +1
-2- } div = st->mclk / (lval * st->f_order *1024 );if (div <1 || div >1023 ) {@@ -366,7 +364,7 @@ static ssize_t ad7192_write_frequency(struct device *dev,ad_sd_write_reg (&st->sd,AD7192_REG_MODE ,3 , st->mode); out: -mutex_unlock (&indio_dev->mlock); +iio_device_release_direct_mode (indio_dev);return ret ? ret : len; }drivers/staging/iio/adc/ad7192.c static ssize_t ad7192_set(struct device *dev, · line 437
Quoted 9 lines
drivers/staging/iio/adc/ad7192.c +2
-3@@ -434,11 +432,9 @@ static ssize_t ad7192_set(struct device *dev,if (ret <0 )return ret; -mutex_lock (&indio_dev->mlock); -if (iio_buffer_enabled (indio_dev)) { -mutex_unlock (&indio_dev->mlock); + ret =iio_device_claim_direct_mode (indio_dev); +if (ret)return -EBUSY ;Same here.
Quoted 14 lines
drivers/staging/iio/adc/ad7192.c +1
-2- }switch ((u32 )this_attr->address) {case AD7192_REG_GPOCON :@@ -461,7 +457,7 @@ static ssize_t ad7192_set(struct device *dev, ret = -EINVAL ; } -mutex_unlock (&indio_dev->mlock); +iio_device_release_direct_mode (indio_dev);return ret ? ret : len; }drivers/staging/iio/adc/ad7192.c static int ad7192_write_raw(struct iio_dev *indio_dev, · line 556
Quoted 9 lines
drivers/staging/iio/adc/ad7192.c +2
-3@@ -555,11 +551,9 @@ static int ad7192_write_raw(struct iio_dev *indio_dev,int ret, i;unsigned int tmp; -mutex_lock (&indio_dev->mlock); -if (iio_buffer_enabled (indio_dev)) { -mutex_unlock (&indio_dev->mlock); + ret =iio_device_claim_direct_mode (indio_dev); +if (ret)return -EBUSY ;And here.
Quoted 6 lines
drivers/staging/iio/adc/ad7192.c +0
-1- }switch (mask) {case IIO_CHAN_INFO_SCALE :drivers/staging/iio/adc/ad7192.c static int ad7192_write_raw(struct iio_dev *indio_dev, · line 582
Quoted 8 lines
drivers/staging/iio/adc/ad7192.c +1
-1@@ -582,7 +576,7 @@ static int ad7192_write_raw(struct iio_dev *indio_dev, ret = -EINVAL ; } -mutex_unlock (&indio_dev->mlock); +iio_device_release_direct_mode (indio_dev);return ret; }--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Jonathan Cameron
maintainerRe: [RFC PATCH v2 0/2] iio: introduce iio_device_{claim|release}_direct_mode()
In reply to Alison SchofieldQuoted 8 lines · Alison Schofield
This patchset introduces two helper functions to simplify driver code requiring the device to be locked in direct mode during execution of a code path. The staging driver ad7192 is updated to demonstrate usage.
This could be applied to approximately 18 known cases where the driver is holding the lock in direct mode. Unknown cases might be those that should, but don't, hold the lock.
Quoted 6 lines
Alternate implementation: Generalize to support a claim on any mode. Do
iio_claim_mode(device,mode)where if the device is in *mode*, it is guaranteed to stay that way until release is called. I considered and rejected this option because a) not sure other modes would ever need to be locked, and b) the semantic improvement is less when it is generalized.
I agree with your chosen approach.
One general process comment. After the positive emails you received for v1, dropping the Request For Comment would have made sense marking the series as in your opinion ready to be applied.
Jonathan
Quoted 18 lines
This patchset was inspired by a discussion on linux-iio:
http://www.spinics.net/lists/linux-iio/msg18540.htmlChanges in v2:
o useiio_deviceprefix for new functions
o replace EINVAL with EBUSY on failure to claim direct mode
o update commit msg & changelog to reflect new prefixAlison Schofield (2):
iio: core: implement iio_device_{claim|release}_direct_mode()
staging: iio: ad7192: use iio_device_{claim|release}_direct_mode()drivers/iio/industrialio-core.c | 39 +++++++++++++++++++++++++++++++++++++++ drivers/staging/iio/adc/ad7192.c | 24 +++++++++--------------- include/linux/iio/iio.h | 2 ++ 3 files changed, 50 insertions(+), 15 deletions(-)