Re: [PATCH 3/4] iio: dht11: Logging updates

[Richard Weinberger <richard@nod.at>]

Am 03.12.2014 um 13:58 schrieb Harald Geyer:
> Richard Weinberger writes:
>> Currently the driver uses pr_* and dev_* functions.
>> Change all logging functions to dev_* style to be consistent
>> and have the correct device prefix in all messages.
> 
> Yes, actually this was on purpose:
> The dev_ messages are really about something wrong with the device.
> Ie if something goes wrong with one device but could perfectly work
> with some other device.
> The pr_ messages OTOH are about something wrong with clock resolution,
> etc that would affect any DHT11 sensor on the system. Ideally we would
> notice these things during probe() and just return with an error there.
> Right now we aren't as clever as that, so we just log an error message
> about the driver, when actually we are reading the device.
> 
> That said, I don't have strong feelings about this. If you want to
> change this, I won't object. However if you really want to fix this,
> then the proper thing would be to check for this conditions in
> probe().

Currently we get log messages of style:
"iio iio:deviceX: foo bar"
and:
"dht11 <name in DT>: foo bar"

I really favorite "dht11 <name in DT".
In my device tree every senor has a sane name and log messages look like:
"dht11 toiletten sensor: invalid checksum"


>> This change set also adds new messages to diagnose issues.
> 
> Comment below.
> 
>> Signed-off-by: Richard Weinberger <richard@nod.at>
>> ---
>>  drivers/iio/humidity/dht11.c | 16 ++++++++++------
>>  1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
>> index 0023699..fbcd7cb 100644
>> --- a/drivers/iio/humidity/dht11.c
>> +++ b/drivers/iio/humidity/dht11.c
>> @@ -96,20 +96,22 @@ static int dht11_decode(struct dht11 *dht11, int offset)
>>  			timeres = t;
>>  	}
>>  	if (2*timeres > DHT11_DATA_BIT_HIGH) {
>> -		pr_err("dht11: timeresolution %d too bad for decoding\n",
>> +		dev_err(dht11->dev, "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");
>> +		dev_err(dht11->dev, "decoding ambiguous\n");
>>  
>>  	/* scale down with timeres and check validity */
>>  	for (i = 0; i < DHT11_BITS_PER_READ; ++i) {
>>  		t = dht11->edges[offset + 2*i + 2].ts -
>>  			dht11->edges[offset + 2*i + 1].ts;
>> -		if (!dht11->edges[offset + 2*i + 1].value)
>> -			return -EIO;  /* lost synchronisation */
>> +		if (!dht11->edges[offset + 2*i + 1].value) {
>> +			dev_err(dht11->dev, "lost synchronisation\n");
>> +			return -EIO;
>> +		}
> 
> Are you sure this warrants a log message? I don't think this provides
> much information. The userspace application should just try reading
> the sensor again.
> 
> We could do someting smart and try to recover from such errors, but
> ultimately userspace will need to deal with failed sensor communication
> anyway, so I don't see the point.

The sensors are rather flaky and if they go nuts the user/developer maybe wants
to know why.
>From a plain EIO she has no chance to find out. He'll have to add printk()s by hand
to find out.
With a log message one can start digging into the issue.

>>  		timing[i] = t / timeres;
>>  	}
>>  
>> @@ -119,8 +121,10 @@ static int dht11_decode(struct dht11 *dht11, int offset)
>>  	temp_dec = dht11_decode_byte(&timing[24], threshold);
>>  	checksum = dht11_decode_byte(&timing[32], threshold);
>>  
>> -	if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum)
>> +	if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum) {
>> +		dev_err(dht11->dev, "invalid checksum\n");
>>  		return -EIO;
>> +	}
> 
> Same thing here.

Same here.
I had some wiring issues with my sensors and wanted to know from where the EIO comes.

Thanks,
//richard