Re: [PATCH] iio: light: tcs3472: use iio helper function to guarantee direct mode

[Jonathan Cameron]

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@xxxxxxxxxx>
>  
>> Signed-off-by: Alison Schofield <amsfield22@xxxxxxxxx>
>> Cc: Daniel Baluta <daniel.baluta@xxxxxxxxx>
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;
>>
>