* [PATCH] iio: light: tcs3472: use iio helper function to guarantee direct mode
@ 2016-06-07 5:10 Alison Schofield
2016-06-07 5:23 ` Peter Meerwald-Stadler
0 siblings, 1 reply; 4+ messages in thread
From: Alison Schofield @ 2016-06-07 5:10 UTC (permalink / raw)
To: jic23; +Cc: pmeerw, knaack.h, lars, linux-iio, linux-kernel
Replace the code that guarantees the device stays in direct mode
with iio_device_claim_direct_mode() which does same. This allows
removal of an unused lock in the device private global data.
Signed-off-by: Alison Schofield <amsfield22@gmail.com>
Cc: Daniel Baluta <daniel.baluta@gmail.com>
---
drivers/iio/light/tcs3472.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
index 1b530bf..b29312f 100644
--- a/drivers/iio/light/tcs3472.c
+++ b/drivers/iio/light/tcs3472.c
@@ -52,7 +52,6 @@
struct tcs3472_data {
struct i2c_client *client;
- struct mutex lock;
u8 enable;
u8 control;
u8 atime;
@@ -117,17 +116,16 @@ static int tcs3472_read_raw(struct iio_dev *indio_dev,
switch (mask) {
case IIO_CHAN_INFO_RAW:
- if (iio_buffer_enabled(indio_dev))
- return -EBUSY;
-
- mutex_lock(&data->lock);
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret)
+ return ret;
ret = tcs3472_req_data(data);
if (ret < 0) {
- mutex_unlock(&data->lock);
+ iio_device_release_direct_mode(indio_dev);
return ret;
}
ret = i2c_smbus_read_word_data(data->client, chan->address);
- mutex_unlock(&data->lock);
+ iio_device_release_direct_mode(indio_dev);
if (ret < 0)
return ret;
*val = ret;
@@ -263,7 +261,6 @@ static int tcs3472_probe(struct i2c_client *client,
data = iio_priv(indio_dev);
i2c_set_clientdata(client, indio_dev);
data->client = client;
- mutex_init(&data->lock);
indio_dev->dev.parent = &client->dev;
indio_dev->info = &tcs3472_info;
--
2.1.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] iio: light: tcs3472: use iio helper function to guarantee direct mode
2016-06-07 5:10 [PATCH] iio: light: tcs3472: use iio helper function to guarantee direct mode Alison Schofield
@ 2016-06-07 5:23 ` Peter Meerwald-Stadler
2016-06-11 16:03 ` Jonathan Cameron
0 siblings, 1 reply; 4+ messages in thread
From: Peter Meerwald-Stadler @ 2016-06-07 5:23 UTC (permalink / raw)
To: Alison Schofield; +Cc: jic23, knaack.h, lars, linux-iio, linux-kernel
On Mon, 6 Jun 2016, Alison Schofield wrote:
> Replace the code that guarantees the device stays in direct mode
> with iio_device_claim_direct_mode() which does same. This allows
> removal of an unused lock in the device private global data.
Acked-by: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
> Signed-off-by: Alison Schofield <amsfield22@gmail.com>
> Cc: Daniel Baluta <daniel.baluta@gmail.com>
> ---
> drivers/iio/light/tcs3472.c | 13 +++++--------
> 1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
> index 1b530bf..b29312f 100644
> --- a/drivers/iio/light/tcs3472.c
> +++ b/drivers/iio/light/tcs3472.c
> @@ -52,7 +52,6 @@
>
> struct tcs3472_data {
> struct i2c_client *client;
> - struct mutex lock;
> u8 enable;
> u8 control;
> u8 atime;
> @@ -117,17 +116,16 @@ static int tcs3472_read_raw(struct iio_dev *indio_dev,
>
> switch (mask) {
> case IIO_CHAN_INFO_RAW:
> - if (iio_buffer_enabled(indio_dev))
> - return -EBUSY;
> -
> - mutex_lock(&data->lock);
> + ret = iio_device_claim_direct_mode(indio_dev);
> + if (ret)
> + return ret;
> ret = tcs3472_req_data(data);
> if (ret < 0) {
> - mutex_unlock(&data->lock);
> + iio_device_release_direct_mode(indio_dev);
> return ret;
> }
> ret = i2c_smbus_read_word_data(data->client, chan->address);
> - mutex_unlock(&data->lock);
> + iio_device_release_direct_mode(indio_dev);
> if (ret < 0)
> return ret;
> *val = ret;
> @@ -263,7 +261,6 @@ static int tcs3472_probe(struct i2c_client *client,
> data = iio_priv(indio_dev);
> i2c_set_clientdata(client, indio_dev);
> data->client = client;
> - mutex_init(&data->lock);
>
> indio_dev->dev.parent = &client->dev;
> indio_dev->info = &tcs3472_info;
>
--
Peter Meerwald-Stadler
+43-664-2444418 (mobile)
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] iio: light: tcs3472: use iio helper function to guarantee direct mode
2016-06-07 5:23 ` Peter Meerwald-Stadler
@ 2016-06-11 16:03 ` Jonathan Cameron
2016-06-11 16:13 ` Jonathan Cameron
0 siblings, 1 reply; 4+ messages in thread
From: Jonathan Cameron @ 2016-06-11 16:03 UTC (permalink / raw)
To: Peter Meerwald-Stadler, Alison Schofield
Cc: knaack.h, lars, linux-iio, linux-kernel
On 07/06/16 06:23, Peter Meerwald-Stadler wrote:
> On Mon, 6 Jun 2016, Alison Schofield wrote:
>
>> Replace the code that guarantees the device stays in direct mode
>> with iio_device_claim_direct_mode() which does same. This allows
>> removal of an unused lock in the device private global data.
>
> Acked-by: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
>
>> Signed-off-by: Alison Schofield <amsfield22@gmail.com>
>> Cc: Daniel Baluta <daniel.baluta@gmail.com>
There is an ever so slightly difference in how these all work after
the change. They will return -EBUSY rather than holding then returning
a valid value under the circumstances of two reads coming through
sysfs at the same time. This is a pretty obscure case so
I think we are OK with this.
Actually pending responses to this, I'm going to back out the other
two similar patches. Just goes to show, that if you show someone
something 3 times they might finally notice the issue!
>> ---
>> drivers/iio/light/tcs3472.c | 13 +++++--------
>> 1 file changed, 5 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
>> index 1b530bf..b29312f 100644
>> --- a/drivers/iio/light/tcs3472.c
>> +++ b/drivers/iio/light/tcs3472.c
>> @@ -52,7 +52,6 @@
>>
>> struct tcs3472_data {
>> struct i2c_client *client;
>> - struct mutex lock;
>> u8 enable;
>> u8 control;
>> u8 atime;
>> @@ -117,17 +116,16 @@ static int tcs3472_read_raw(struct iio_dev *indio_dev,
>>
>> switch (mask) {
>> case IIO_CHAN_INFO_RAW:
>> - if (iio_buffer_enabled(indio_dev))
>> - return -EBUSY;
>> -
>> - mutex_lock(&data->lock);
>> + ret = iio_device_claim_direct_mode(indio_dev);
>> + if (ret)
>> + return ret;
>> ret = tcs3472_req_data(data);
>> if (ret < 0) {
>> - mutex_unlock(&data->lock);
>> + iio_device_release_direct_mode(indio_dev);
>> return ret;
>> }
>> ret = i2c_smbus_read_word_data(data->client, chan->address);
>> - mutex_unlock(&data->lock);
>> + iio_device_release_direct_mode(indio_dev);
>> if (ret < 0)
>> return ret;
>> *val = ret;
>> @@ -263,7 +261,6 @@ static int tcs3472_probe(struct i2c_client *client,
>> data = iio_priv(indio_dev);
>> i2c_set_clientdata(client, indio_dev);
>> data->client = client;
>> - mutex_init(&data->lock);
>>
>> indio_dev->dev.parent = &client->dev;
>> indio_dev->info = &tcs3472_info;
>>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] iio: light: tcs3472: use iio helper function to guarantee direct mode
2016-06-11 16:03 ` Jonathan Cameron
@ 2016-06-11 16:13 ` Jonathan Cameron
0 siblings, 0 replies; 4+ messages in thread
From: Jonathan Cameron @ 2016-06-11 16:13 UTC (permalink / raw)
To: Peter Meerwald-Stadler, Alison Schofield
Cc: knaack.h, lars, linux-iio, linux-kernel
On 11/06/16 17:03, Jonathan Cameron wrote:
> On 07/06/16 06:23, Peter Meerwald-Stadler wrote:
>> On Mon, 6 Jun 2016, Alison Schofield wrote:
>>
>>> Replace the code that guarantees the device stays in direct mode
>>> with iio_device_claim_direct_mode() which does same. This allows
>>> removal of an unused lock in the device private global data.
>>
>> Acked-by: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
>>
>>> Signed-off-by: Alison Schofield <amsfield22@gmail.com>
>>> Cc: Daniel Baluta <daniel.baluta@gmail.com>
> There is an ever so slightly difference in how these all work after
> the change. They will return -EBUSY rather than holding then returning
> a valid value under the circumstances of two reads coming through
> sysfs at the same time. This is a pretty obscure case so
> I think we are OK with this.
>
> Actually pending responses to this, I'm going to back out the other
> two similar patches. Just goes to show, that if you show someone
> something 3 times they might finally notice the issue!
Looking at this more carefully the lack of locking around the buffer
in use check was clearly broken and your new code fixes both that
and allows the lock removal.
I was overthinking this...
Applied
>
>
>>> ---
>>> drivers/iio/light/tcs3472.c | 13 +++++--------
>>> 1 file changed, 5 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
>>> index 1b530bf..b29312f 100644
>>> --- a/drivers/iio/light/tcs3472.c
>>> +++ b/drivers/iio/light/tcs3472.c
>>> @@ -52,7 +52,6 @@
>>>
>>> struct tcs3472_data {
>>> struct i2c_client *client;
>>> - struct mutex lock;
>>> u8 enable;
>>> u8 control;
>>> u8 atime;
>>> @@ -117,17 +116,16 @@ static int tcs3472_read_raw(struct iio_dev *indio_dev,
>>>
>>> switch (mask) {
>>> case IIO_CHAN_INFO_RAW:
>>> - if (iio_buffer_enabled(indio_dev))
>>> - return -EBUSY;
>>> -
>>> - mutex_lock(&data->lock);
>>> + ret = iio_device_claim_direct_mode(indio_dev);
>>> + if (ret)
>>> + return ret;
>>> ret = tcs3472_req_data(data);
>>> if (ret < 0) {
>>> - mutex_unlock(&data->lock);
>>> + iio_device_release_direct_mode(indio_dev);
>>> return ret;
>>> }
>>> ret = i2c_smbus_read_word_data(data->client, chan->address);
>>> - mutex_unlock(&data->lock);
>>> + iio_device_release_direct_mode(indio_dev);
>>> if (ret < 0)
>>> return ret;
>>> *val = ret;
>>> @@ -263,7 +261,6 @@ static int tcs3472_probe(struct i2c_client *client,
>>> data = iio_priv(indio_dev);
>>> i2c_set_clientdata(client, indio_dev);
>>> data->client = client;
>>> - mutex_init(&data->lock);
>>>
>>> indio_dev->dev.parent = &client->dev;
>>> indio_dev->info = &tcs3472_info;
>>>
>>
>
> --
> 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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-06-11 16:13 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-07 5:10 [PATCH] iio: light: tcs3472: use iio helper function to guarantee direct mode Alison Schofield
2016-06-07 5:23 ` Peter Meerwald-Stadler
2016-06-11 16:03 ` Jonathan Cameron
2016-06-11 16:13 ` Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).