Re: [PATCH v2 2/4] iio: accel: adxl345_core: Introduce set_mode and data_ready functions

[Jonathan Cameron]

On 02/05/17 12:39, Eva Rachel Retuya wrote:
> On Mon, May 01, 2017 at 01:22:52AM +0100, Jonathan Cameron wrote:
> Hello Jonathan,
> [...]
>>> +static int adxl345_set_mode(struct adxl345_data *data, u8 mode)
>>> +{
>>> +	struct device *dev = regmap_get_device(data->regmap);
>>> +	int ret;
>>> +
>>> +	ret = regmap_write(data->regmap, ADXL345_REG_POWER_CTL, mode);
>>> +	if (ret < 0) {
>>> +		dev_err(dev, "Failed to set power mode, %d\n", ret);
>>> +		return ret;
>> drop the return ret here and just return ret at the end of the function.
>> One of the static checkers will probably moan about this otherwise.
> 
> OK.
> 
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int adxl345_data_ready(struct adxl345_data *data)
>>> +{
>> So this is a polling the dataready bit.  Will ensure we always
>> get fresh data when a read occurs.  Please add a comment to
>> that effect as that's not always how devices work.
> 
> OK.
> 
>>> +	struct device *dev = regmap_get_device(data->regmap);
>>> +	int tries = 5;
>>> +	u32 val;
>>> +	int ret;
>>> +
>>> +	do {
>>> +		/*
>>> +		 * 1/ODR + 1.1ms; 11.1ms at ODR of 0.10 Hz
>>> +		 * Sensor currently operates at default ODR of 100 Hz
>>> +		 */
>>> +		usleep_range(1100, 11100);
>> That's a huge range to allow... I'm not following the argument for why.
>> Or do we have a stray 0?
>>
> 
> Not a stray 0. Range is from 1.1ms to 11.1ms, this represents the
> wake-up time when going to standby/other power saving modes ->
> measurement mode. I'm going to clarify the comment on why it is needed
> on the next revision.
The point about a range sleep is to allow the kernel flexibility in scheduling
so as to avoid waking the processor from lower power states when high precision is
not needed.

If the thing you are talking about needs the maximum time sometimes then you
should set the minimum value to that and add a bit to avoid unnecessary processor
wake ups.
> 
>>> +
>>> +		ret = regmap_read(data->regmap, ADXL345_REG_INT_SOURCE, &val);
>>> +		if (ret < 0)
>>> +			return ret;
>>> +		if ((val & ADXL345_INT_DATA_READY) == ADXL345_INT_DATA_READY)
>>> +			return 0;
>>> +	} while (--tries);
>>> +	dev_err(dev, "Data is not yet ready, try again.\n");
>>> +
>> This is almost certainly a hardware fault. I'd be more brutal with
>> the error and return -EIO.  If you get here your hardware is very unlikely
>> to be working correctly if you try again.
> 
> OK, will change it to -EIO then.
> 
>>> +	return -EAGAIN;
>>> +}
>>> +
>>>  #define ADXL345_CHANNEL(reg, axis) {					\
>>>  	.type = IIO_ACCEL,						\
>>>  	.modified = 1,							\
>>> @@ -72,6 +118,19 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
>>>  
>>>  	switch (mask) {
>>>  	case IIO_CHAN_INFO_RAW:
>>> +		mutex_lock(&data->lock);
>>> +		ret = adxl345_set_mode(data, ADXL345_POWER_CTL_MEASURE);
>>> +		if (ret < 0) {
>>> +			mutex_unlock(&data->lock);
>>> +			return ret;
>>> +		}
>>> +
>>> +		ret = adxl345_data_ready(data);
>>> +		if (ret < 0) {
>>> +			adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
>>> +			mutex_unlock(&data->lock);
>> What is the logic that puts the mutex_unlock here in the error case
>> and before the set_mode in the normal path?  Even if it doesn't
>> matter make them the same as it is less likely to raise questions
>> in the future!
> 
> OK, will make it consistent.
> 
>>> +			return ret;
>>> +		}
>>>  		/*
>>>  		 * Data is stored in adjacent registers:
>>>  		 * ADXL345_REG_DATA(X0/Y0/Z0) contain the least significant byte
>>> @@ -79,10 +138,15 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
>>>  		 */
>>>  		ret = regmap_bulk_read(data->regmap, chan->address, &regval,
>>>  				       sizeof(regval));
>>> -		if (ret < 0)
>>> +		mutex_unlock(&data->lock);
>>> +		if (ret < 0) {
>>> +			adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
>>>  			return ret;
>>> +		}
>>>  
>>>  		*val = sign_extend32(le16_to_cpu(regval), 12);
>>> +		adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
>>> +
>>>  		return IIO_VAL_INT;
>>>  	case IIO_CHAN_INFO_SCALE:
>>>  		*val = 0;
> [...]
>>> @@ -169,8 +224,7 @@ int adxl345_core_remove(struct device *dev)
>>>  
>>>  	iio_device_unregister(indio_dev);
>>>  
>>> -	return regmap_write(data->regmap, ADXL345_REG_POWER_CTL,
>>> -			    ADXL345_POWER_CTL_STANDBY);
>>> +	return adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
>> Under what circumstances would we not already be in the correct state?
>> A brief comment here would be good.
> 
> I'm leaving this unremoved to catch cases where in the sensor fails to
> return to standby mode after a read. Will add the said comment.
> 
> Thanks,
> Eva
> 
>>>  }
>>>  EXPORT_SYMBOL_GPL(adxl345_core_remove);
>>>  
>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>