Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965298AbbEMUuF (ORCPT ); Wed, 13 May 2015 16:50:05 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:55519 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965113AbbEMUuA (ORCPT ); Wed, 13 May 2015 16:50:00 -0400 Date: Wed, 13 May 2015 13:49:58 -0700 From: Andrew Morton To: Vignesh R Cc: Evgeniy Polyakov , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Jonathan Corbet , NeilBrown , Tony Lindgren , Greg Kroah-Hartman , Fabian Frederick , , , Subject: Re: [PATCH RESEND] w1: masters: omap_hdq: Add support for 1-wire mode Message-Id: <20150513134958.e21e538c3c8fe00d3fcd8854@linux-foundation.org> In-Reply-To: <1430728735-22747-1-git-send-email-vigneshr@ti.com> References: <1430728735-22747-1-git-send-email-vigneshr@ti.com> X-Mailer: Sylpheed 3.4.1 (GTK+ 2.24.23; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3017 Lines: 104 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? > + } > + > + 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. > + 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; > +} > > ... > -- 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/