Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934619AbbEOMFp (ORCPT ); Fri, 15 May 2015 08:05:45 -0400 Received: from arroyo.ext.ti.com ([192.94.94.40]:37760 "EHLO arroyo.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934182AbbEOMFn (ORCPT ); Fri, 15 May 2015 08:05:43 -0400 Message-ID: <5555E0EE.1010204@ti.com> Date: Fri, 15 May 2015 17:35:02 +0530 From: Vignesh R User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Andrew Morton CC: Evgeniy Polyakov , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Jonathan Corbet , NeilBrown , Tony Lindgren , Greg Kroah-Hartman , Fabian Frederick , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "linux-doc@vger.kernel.org" Subject: Re: [PATCH RESEND] w1: masters: omap_hdq: Add support for 1-wire mode References: <1430728735-22747-1-git-send-email-vigneshr@ti.com> <20150513134958.e21e538c3c8fe00d3fcd8854@linux-foundation.org> In-Reply-To: <20150513134958.e21e538c3c8fe00d3fcd8854@linux-foundation.org> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3541 Lines: 119 On Thursday 14 May 2015 02:19 AM, Andrew Morton wrote: > On Mon, 4 May 2015 14:08:55 +0530 Vignesh R wrote: > >> This patches makes following changes to omap_hdq driver >> - Enable 1-wire mode. >> - Implement w1_triplet callback to facilitate search rom >> procedure and auto detection of 1-wire slaves. >> - Proper enabling and disabling of interrupt. >> - Cleanups (formatting and return value checks). >> >> HDQ mode remains unchanged. >> >> ... >> >> +/* >> + * W1 triplet callback function - used for searching ROM addresses. >> + * Registered only when controller is in 1-wire mode. >> + */ >> +static u8 omap_w1_triplet(void *_hdq, u8 bdir) >> +{ >> + u8 ret, id_bit, comp_bit; >> + struct hdq_data *hdq_data = _hdq; >> + u8 ctrl = OMAP_HDQ_CTRL_STATUS_SINGLE | OMAP_HDQ_CTRL_STATUS_GO | >> + OMAP_HDQ_CTRL_STATUS_INTERRUPTMASK; >> + u8 mask = ctrl | OMAP_HDQ_CTRL_STATUS_DIR; >> + >> + omap_hdq_get(_hdq); >> + >> + ret = mutex_lock_interruptible(&hdq_data->hdq_mutex); >> + if (ret < 0) { >> + ret = -EINTR; >> + goto rtn; > > Has this code path been tested? What happens when it is taken? Driver > initialization fails? If so, why is this desirable behaviour? The w1 core code that calls omap_w1_triplet (w1_triplet from w1_io.c) doesn't care about error codes returned by omap_w1_triplet. Hence, I will add a debug print and return 0x3 indicating to the w1 core to start the search again. > >> + } >> + >> + hdq_data->hdq_irqstatus = 0; >> + /* read id_bit */ >> + hdq_reg_merge(_hdq, OMAP_HDQ_CTRL_STATUS, >> + ctrl | OMAP_HDQ_CTRL_STATUS_DIR, mask); >> + wait_event_timeout(hdq_wait_queue, >> + (hdq_data->hdq_irqstatus >> + & OMAP_HDQ_INT_STATUS_RXCOMPLETE), >> + OMAP_HDQ_TIMEOUT); > > It seems bad to ignore a timeout. Shouldn't the code clean up and > report error when this occurs? > > >> + id_bit = (hdq_reg_in(_hdq, OMAP_HDQ_RX_DATA) & 0x01); >> + >> + hdq_data->hdq_irqstatus = 0; >> + /* read comp_bit */ >> + hdq_reg_merge(_hdq, OMAP_HDQ_CTRL_STATUS, >> + ctrl | OMAP_HDQ_CTRL_STATUS_DIR, mask); >> + wait_event_timeout(hdq_wait_queue, >> + (hdq_data->hdq_irqstatus >> + & OMAP_HDQ_INT_STATUS_RXCOMPLETE), >> + OMAP_HDQ_TIMEOUT); > > Here too. > >> + comp_bit = (hdq_reg_in(_hdq, OMAP_HDQ_RX_DATA) & 0x01); >> + >> + if (id_bit && comp_bit) { >> + ret = 0x03; /* error */ >> + goto rtn; >> + } >> + if (!id_bit && !comp_bit) { >> + /* Both bits are valid, take the direction given */ >> + ret = bdir ? 0x04 : 0; >> + } else { >> + /* Only one bit is valid, take that direction */ >> + bdir = id_bit; >> + ret = id_bit ? 0x05 : 0x02; >> + } >> + >> + /* write bdir bit */ >> + hdq_reg_out(_hdq, OMAP_HDQ_TX_DATA, bdir); >> + hdq_reg_merge(_hdq, OMAP_HDQ_CTRL_STATUS, ctrl, mask); >> + wait_event_timeout(hdq_wait_queue, >> + (hdq_data->hdq_irqstatus >> + & OMAP_HDQ_INT_STATUS_TXCOMPLETE), >> + OMAP_HDQ_TIMEOUT); > > And here. I will add a dev_dbg() and return 0x3 indicating no devices responded in all the above cases. > >> + hdq_reg_merge(_hdq, OMAP_HDQ_CTRL_STATUS, 0, >> + OMAP_HDQ_CTRL_STATUS_SINGLE); >> + >> +rtn: >> + mutex_unlock(&hdq_data->hdq_mutex); >> + omap_hdq_put(_hdq); >> + return ret; >> +} >> >> ... >> Thanks & Regards Vignesh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/