From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752118AbaLCM7O (ORCPT ); Wed, 3 Dec 2014 07:59:14 -0500 Received: from h1.radempa.de ([176.9.142.194]:59374 "EHLO mail.cosmopool.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750855AbaLCM7N (ORCPT ); Wed, 3 Dec 2014 07:59:13 -0500 From: Harald Geyer To: Richard Weinberger cc: jic23@kernel.org, knaack.h@gmx.de, lars@metafoo.de, pmeerw@pmeerw.net, sanjeev_sharma@mentor.com, linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 3/4] iio: dht11: Logging updates In-reply-to: <1417563176-31972-4-git-send-email-richard@nod.at> References: <1417563176-31972-1-git-send-email-richard@nod.at> <1417563176-31972-4-git-send-email-richard@nod.at> Comments: In-reply-to Richard Weinberger message dated "Wed, 03 Dec 2014 00:32:55 +0100." MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-ID: <4685.1417611532.1@stardust.g4.wien.funkfeuer.at> Date: Wed, 03 Dec 2014 13:58:52 +0100 Message-Id: Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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(). > This change set also adds new messages to diagnose issues. Comment below. > Signed-off-by: Richard Weinberger > --- > 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. > 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. > dht11->timestamp = iio_get_time_ns(); > if (hum_int < 20) { /* DHT22 */ > @@ -193,7 +197,7 @@ static int dht11_read_raw(struct iio_dev *iio_dev, > free_irq(dht11->irq, iio_dev); > > if (ret == 0 && dht11->num_edges < DHT11_EDGES_PER_READ - 1) { > - dev_err(&iio_dev->dev, > + dev_err(dht11->dev, Ack. Thanks, Harald > "Only %d signal edges detected\n", > dht11->num_edges); > ret = -ETIMEDOUT; > -- > 1.8.4.5 >