Re: [PATCHv2 2/2] iio: dht11: Use new function ktime_get_resolution_ns()

[Hartmut Knaack <knaack.h@gmx.de>]

harald@ccbib.org schrieb am 30.06.2015 um 21:37:
> On Mon, 29 Jun 2015 21:37:22 +0200, Hartmut Knaack <knaack.h@gmx.de>
> wrote:
>> Harald Geyer schrieb am 29.06.2015 um 20:59:
>>> Hi Jonathan!
>>>
>>> Jonathan Cameron writes:
> [...]
>>>> Looks good to me.  Will wait for comments on the first patch though
>>>> before taking this... clearly that one will need a few acks if I take
>>>> it through IIO!
>>>
>>> The first patch has been merged via tip tree for 4.2 (is already
>>> available
>>> in staging-next tree). Are you going to pick up this patch or do I need
>>> to resend?
>>>
>>> Thanks,
>>> Harald
>>>
>>
>> Hi Harald,
>> there are a few small style issues, which checkpatch.pl --strict should
>> show you. 
> 
> Thanks for pointing out --strict - didn't know about that yet.
> 
> I get the following output:
> 
> CHECK: spaces preferred around that '*' (ctx:VxV)
> #96: FILE: drivers/iio/humidity/dht11.c:167:
> +               if (DHT11_DATA_BIT_HIGH < 2*timeres) {
>                                            ^
> 
> With this one I disagree. Omitting the spaces makes the code easier to
> read, because it reflects the relative binding power of operators involed.
> This style (omitting spaces around * if there is a +, <, etc in the same
> experession) is used throughout the driver. I'd change this only if there
> is consensus, that this really is the preferred way.

I would prefer to make use of parenthesis to reflect a precedence instead
of violating coding style guidelines.
Honestly, it didn't come to my mind that you left out those spaces on
purpose.

> 
> CHECK: Alignment should match open parenthesis
> #98: FILE: drivers/iio/humidity/dht11.c:169:
> +                       dev_err(dht11->dev, "timeresolution %dns too
> low\n",
> +                                               timeres);
> 
> Ok, I'll post a patch that updates the driver to this style and update the
> above patch acordingly.
> 
> Thanks,
> Harald
> 
> 
>> Thanks,
>> Hartmut
>>
>>>> Jonathan
>>>>> ---
>>>>> changes since V1:
>>>>> call ktime_get_xxx() functions directly instead of using the wrappers
>>>>> of the iio subsystem.
>>>>>
>>>>>  drivers/iio/humidity/dht11.c |   42
>>>>>  ++++++++++++++++++++++--------------------
>>>>>  1 files changed, 22 insertions(+), 20 deletions(-)
>>>>>
>>>>> diff --git a/drivers/iio/humidity/dht11.c
>>>>> b/drivers/iio/humidity/dht11.c
>>>>> index 7d79a1a..4cb25dc 100644
>>>>> --- a/drivers/iio/humidity/dht11.c
>>>>> +++ b/drivers/iio/humidity/dht11.c
>>>>> @@ -33,6 +33,7 @@
>>>>>  #include <linux/delay.h>
>>>>>  #include <linux/gpio.h>
>>>>>  #include <linux/of_gpio.h>
>>>>> +#include <linux/timekeeping.h>
>>>>>  
>>>>>  #include <linux/iio/iio.h>
>>>>>  
>>>>> @@ -87,23 +88,11 @@ static unsigned char dht11_decode_byte(int
>>>>> *timing, int threshold)
>>>>>  	return ret;
>>>>>  }
>>>>>  
>>>>> -static int dht11_decode(struct dht11 *dht11, int offset)
>>>>> +static int dht11_decode(struct dht11 *dht11, int offset, int
> timeres)
>>>>>  {
>>>>> -	int i, t, timing[DHT11_BITS_PER_READ], threshold,
>>>>> -		timeres = DHT11_SENSOR_RESPONSE;
>>>>> +	int i, t, timing[DHT11_BITS_PER_READ], threshold;
>>>>>  	unsigned char temp_int, temp_dec, hum_int, hum_dec, checksum;
>>>>>  
>>>>> -	/* Calculate timestamp resolution */
>>>>> -	for (i = 1; i < dht11->num_edges; ++i) {
>>>>> -		t = dht11->edges[i].ts - dht11->edges[i-1].ts;
>>>>> -		if (t > 0 && t < timeres)
>>>>> -			timeres = t;
>>>>> -	}
>>>>> -	if (2*timeres > DHT11_DATA_BIT_HIGH) {
>>>>> -		pr_err("dht11: timeresolution %d too bad for decoding\n",
>>>>> -			timeres);
>>>>> -		return -EIO;
>>>>> -	}
>>>>>  	threshold = DHT11_DATA_BIT_HIGH / timeres;
>>>>>  	if (DHT11_DATA_BIT_LOW/timeres + 1 >= threshold)
>>>>>  		pr_err("dht11: WARNING: decoding ambiguous\n");
>>>>> @@ -126,7 +115,7 @@ static int dht11_decode(struct dht11 *dht11, int
>>>>> offset)
>>>>>  	if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum)
>>>>>  		return -EIO;
>>>>>  
>>>>> -	dht11->timestamp = iio_get_time_ns();
>>>>> +	dht11->timestamp = ktime_get_real_ns();
>>>>>  	if (hum_int < 20) {  /* DHT22 */
>>>>>  		dht11->temperature = (((temp_int & 0x7f) << 8) + temp_dec) *
>>>>>  					((temp_int & 0x80) ? -100 : 100);
>>>>> @@ -154,7 +143,7 @@ static irqreturn_t dht11_handle_irq(int irq, void
>>>>> *data)
>>>>>  
>>>>>  	/* TODO: Consider making the handler safe for IRQ sharing */
>>>>>  	if (dht11->num_edges < DHT11_EDGES_PER_READ && dht11->num_edges >=
>>>>>  	0) {
>>>>> -		dht11->edges[dht11->num_edges].ts = iio_get_time_ns();
>>>>> +		dht11->edges[dht11->num_edges].ts = ktime_get_real_ns();
>>>>>  		dht11->edges[dht11->num_edges++].value =
>>>>>  						gpio_get_value(dht11->gpio);
>>>>>  
>>>>> @@ -170,10 +159,22 @@ static int dht11_read_raw(struct iio_dev
>>>>> *iio_dev,
>>>>>  			int *val, int *val2, long m)
>>>>>  {
>>>>>  	struct dht11 *dht11 = iio_priv(iio_dev);
>>>>> -	int ret;
>>>>> +	int ret, timeres;
>>>>>  
>>>>>  	mutex_lock(&dht11->lock);
>>>>> -	if (dht11->timestamp + DHT11_DATA_VALID_TIME < iio_get_time_ns()) {
>>>>> +	if (dht11->timestamp + DHT11_DATA_VALID_TIME < ktime_get_real_ns())
> {
>>>>> +		timeres = ktime_get_resolution_ns();
>>>>> +		if (DHT11_DATA_BIT_HIGH < 2*timeres) {
>>>>> +			dev_err(dht11->dev, "timeresolution %dns too low\n",
>>>>> +						timeres);
>>>>> +			/* In theory a better clock could become available
>>>>> +			 * at some point ... and there is no error code
>>>>> +			 * that really fits better.
>>>>> +			 */
>>>>> +			ret = -EAGAIN;
>>>>> +			goto err;
>>>>> +		}
>>>>> +
>>>>>  		reinit_completion(&dht11->completion);
>>>>>  
>>>>>  		dht11->num_edges = 0;
>>>>> @@ -208,7 +209,8 @@ static int dht11_read_raw(struct iio_dev
> *iio_dev,
>>>>>  		ret = dht11_decode(dht11,
>>>>>  				dht11->num_edges == DHT11_EDGES_PER_READ ?
>>>>>  					DHT11_EDGES_PREAMBLE :
>>>>> -					DHT11_EDGES_PREAMBLE - 2);
>>>>> +					DHT11_EDGES_PREAMBLE - 2,
>>>>> +				timeres);
>>>>>  		if (ret)
>>>>>  			goto err;
>>>>>  	}
>>>>> @@ -274,7 +276,7 @@ static int dht11_probe(struct platform_device
>>>>> *pdev)
>>>>>  		return -EINVAL;
>>>>>  	}
>>>>>  
>>>>> -	dht11->timestamp = iio_get_time_ns() - DHT11_DATA_VALID_TIME - 1;
>>>>> +	dht11->timestamp = ktime_get_real_ns() - DHT11_DATA_VALID_TIME - 1;
>>>>>  	dht11->num_edges = -1;
>>>>>  
>>>>>  	platform_set_drvdata(pdev, iio);
>>>>>
>>>>
>>> --
>>> 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
>>>
> --
> 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
>