From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754550AbbKBVbi (ORCPT ); Mon, 2 Nov 2015 16:31:38 -0500 Received: from mout.kundenserver.de ([212.227.126.130]:60193 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752845AbbKBVbf (ORCPT ); Mon, 2 Nov 2015 16:31:35 -0500 From: Arnd Bergmann To: Sinan Kaya Cc: dmaengine@vger.kernel.org, timur@codeaurora.org, cov@codeaurora.org, jcm@redhat.com, Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Vinod Koul , Dan Williams , devicetree@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/2] dma: add Qualcomm Technologies HIDMA management driver Date: Mon, 02 Nov 2015 22:30:58 +0100 Message-ID: <9016417.z9Hz6MbmvQ@wuerfel> User-Agent: KMail/4.11.5 (Linux/3.16.0-10-generic; KDE/4.11.5; x86_64; ; ) In-Reply-To: <56346502.5070600@codeaurora.org> References: <1446174501-8870-1-git-send-email-okaya@codeaurora.org> <4548985.gMZJhM3R1p@wuerfel> <56346502.5070600@codeaurora.org> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Provags-ID: V03:K0:wZIhTRGMJk2o6VVqvUNY326VD70ireEeMi4QIIrK4gpOBr93zcp A4NzDtIiMfxtyk/vbWAE6IEMe/BdBMMmqLSHSkAiKzyr6FW4YbOCNEiT/0tWtMpe7nzepOi Fro8WTqggahZd1oMUvr0rIiLn5n1vCs9C34E9C5Zh+05g05h93POIyUIlxFwL47M2sdeTB/ UyqG9leYz9At1X2mzOX6g== X-UI-Out-Filterresults: notjunk:1;V01:K0:fPsLqjVJlrY=:sd9E6tqkus8DB/q47q2PtA 5VSViLsvUPjR92OmqNJT7g8NFCIslvbfW0S0CucXh32vPCpG9Uz9U8U0dX7/JrlQbS2l5d9Zw Vcn26HVHHyLsIClTtkYg/x0TqOOjam4+o/l9Ve4kEsncKySk+F0bkrsUe+gDbfmAzvAOK1IsI ZwOzgt1tw40gZjOTaef4doKUi4LSGijsTOPG7Z09FoPE2NkwigAgT4B4wyxVNzRq5tjmuJQXY 5p0zR/a/2wAic1xEwp8A71Ro4XmUvvrDznkpPmvrdAXDH+dCFw8A0efuDPRRx+fo5tFko6Xzu 0btzUexKfEbJkDvHbqzlliUYjvtmyPXHTcYD/9HKt/EeV1c+RIpMqgpPDL0DyUl56nVkinjdP D9KbVNsmXcFQnA3p5jTJ/ZtE//B22ambv2SjUPkioLkm+8+cgaLSTis9VhlU/SI39Fhk6o8Hv 1jNEjrV0bKeSLTNN2uxGo1f1vMIFuDxjnLMPs11YRDUpwghjIqUAYNsdawnkJxgskoM0RjnnU tHUmZSGKvcFpSUtwO9H63LvjKR+2bNBNh3pzpoRMGo6CmThsxZj5Vkh39iN0nktep/TOZApLE tx+ptnhVx6XpmEWgHElzmo0GvmpW+7CYxOtMhbSSQxqoysK3CHpJ3M1oM32ggYt3t0Br3aZWJ EpJe8C8LXYNRcWxN15fXMIdYFb7IK96o46ttcbZA0V7OSNyfnUfrpZppjVtH9RpfLHItpoSSA PYZPtx6Hrrh4Gx1N Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Saturday 31 October 2015 02:51:46 Sinan Kaya wrote: > On 10/30/2015 5:34 AM, Arnd Bergmann wrote: > > On Thursday 29 October 2015 23:08:12 Sinan Kaya wrote: > >> diff --git a/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt b/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt > >> + > >> +static int qcom_hidma_mgmt_err_open(struct inode *inode, struct file *file) > >> +{ > >> + return single_open(file, qcom_hidma_mgmt_err, inode->i_private); > >> +} > >> + > >> +static const struct file_operations qcom_hidma_mgmt_err_fops = { > >> + .open = qcom_hidma_mgmt_err_open, > >> + .read = seq_read, > >> + .llseek = seq_lseek, > >> + .release = single_release, > >> +}; > >> + > >> +static ssize_t qcom_hidma_mgmt_mhiderr_clr(struct file *file, > >> + const char __user *user_buf, size_t count, loff_t *ppos) > >> +{ > >> + struct qcom_hidma_mgmt_dev *mgmtdev = file->f_inode->i_private; > >> + > >> + HIDMA_RUNTIME_GET(mgmtdev); > >> + writel(1, mgmtdev->dev_virtaddr + MHID_BUS_ERR_CLR_OFFSET); > >> + HIDMA_RUNTIME_SET(mgmtdev); > >> + return count; > >> +} > >> + > >> +static const struct file_operations qcom_hidma_mgmt_mhiderr_clrfops = { > >> + .write = qcom_hidma_mgmt_mhiderr_clr, > >> +}; > > > > Is this really just a debugging interface? If anyone would do this > > for normal operation, it needs to be a proper API. > > > This will be used by the system admin to monitor/reset the execution > state of the DMA channels. This will be the management interface. > Debugfs is probably not the right choice. I originally had sysfs but > than had some doubts. I'm open to suggestions. User interface design is unfortunately always hard, and I don't have an obvious answer for you. Using debugfs by definition means that you don't expect users to rely on ABI stability, so they should not write any automated scripts against the contents of the files. With sysfs, the opposite is true: you need to maintain compatibility for as long as anyone might rely on the current interface, and it needs to be reviewed properly and documented in Documentation/ABI/. Other options are to use ioctl(), netlink or your own virtual file system, but each of them has the same ABI requirements as sysfs. Regardless of what you pick, you also need to consider how other drivers would use the same interface: If someone else has hardware that does the same thing, we want to be able to use the same tools to access it, so you should avoid having any hardware specific data in it and keep it as generic and extensible as possible. In this particular case, that probably means you should implement the user interfaces in the dmaengine core driver, and let the specific DMA driver provide callback function pointers along with the normal ones to fill that data. > >> + dev_info(&pdev->dev, > >> + "HI-DMA engine management driver registration complete\n"); > >> + platform_set_drvdata(pdev, mgmtdev); > >> + HIDMA_RUNTIME_SET(mgmtdev); > >> + return 0; > >> +out: > >> + pm_runtime_disable(&pdev->dev); > >> + pm_runtime_put_sync_suspend(&pdev->dev); > >> + return rc; > >> +} > > > > The rest of the probe function does not register any user interface aside from > > the debugging stuff. Can you explain in the changelog how you expect the > > driver to be used in a real system? Is there another driver coming? > > I expect this driver to grow in functionality over time. Right now, it > does the global init for the DMA. After that all channels execute on > their own without depending on each other. Global init has to be done > first before attempting to do any channel initialization. > > There is also implied startup ordering requirements. I was doing this by > using channel driver with the late binding to guarantee that. > > As soon as I use module_platform_driver, the ordering gets reversed for > some reason. For the ordering requirements, it's probably best to export a symbol with the entry point and let the normal driver call into that. Using separate initcall levels is not something you should do in a normal device driver like this. What is the relation between the device nodes for the two kinds of devices? Does it make sense to model the other one as a child device of this one? That way you would trivially do the ordering by not marking this one as 'compatible="simple-bus"' and triggering the registration of the child from the parent probe function. Arnd