From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752362AbcDZQSz (ORCPT ); Tue, 26 Apr 2016 12:18:55 -0400 Received: from mga14.intel.com ([192.55.52.115]:28623 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751317AbcDZQSx (ORCPT ); Tue, 26 Apr 2016 12:18:53 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,537,1455004800"; d="scan'208";a="966927037" Date: Tue, 26 Apr 2016 21:54:41 +0530 From: Vinod Koul To: Sinan Kaya Cc: dmaengine@vger.kernel.org, timur@codeaurora.org, devicetree@vger.kernel.org, cov@codeaurora.org, jcm@redhat.com, shankerd@codeaurora.org, vikrams@codeaurora.org, marc.zyngier@arm.com, mark.rutland@arm.com, eric.auger@linaro.org, agross@codeaurora.org, arnd@arndb.de, linux-arm-msm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Dan Williams , Andy Shevchenko , linux-kernel@vger.kernel.org Subject: Re: [PATCH V17 1/3] dmaengine: qcom_hidma: implement lower level hardware interface Message-ID: <20160426162441.GJ2274@localhost> References: <1460384473-5775-1-git-send-email-okaya@codeaurora.org> <1460384473-5775-2-git-send-email-okaya@codeaurora.org> <20160426032805.GA2274@localhost> <571F8397.5000803@codeaurora.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <571F8397.5000803@codeaurora.org> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Apr 26, 2016 at 11:04:55AM -0400, Sinan Kaya wrote: > On 4/25/2016 11:28 PM, Vinod Koul wrote: > >> + > >> + /* reset the channel for recovery */ > >> + if (hidma_ll_setup(lldev)) { > > > > should this be done in ISR? > > I created a new tasklet called rst_task and posted the code there. sounds better > > /* > + * Abort all transactions and perform a reset. > + */ > +static void hidma_ll_abort(unsigned long arg) > +{ > + u8 err_code = HIDMA_EVRE_STATUS_ERROR; > + u8 err_info = 0xFF; > + > + dev_err(lldev->dev, "error 0x%x, resetting...\n", > + cause); right justify this and others as well please > >> +int hidma_ll_resume(struct hidma_lldev *lldev) > >> +{ > >> + return hidma_ll_enable(lldev); > >> +} > > > > why do we need this empty function, use hidma_ll_enable. > > hidma_ll_enable is a common function that gets called from multiple places. > hidma_ll_resume and hidma_ll_pause is used by the OS interface for pausing > and resuming the DMA channel. is there a reason why we can't have the code in resume and that being called internally as well as externally? > >> +/* > >> + * Note that even though we stop this channel > >> + * if there is a pending transaction in flight > >> + * it will complete and follow the callback. > >> + * This request will prevent further requests > >> + * to be made. > > > > Why the odd formating? > > aligned to 75 characters. This seems to be 50 chars! -- ~Vinod