From patchwork Fri Oct 12 14:06:55 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Viresh Kumar X-Patchwork-Id: 1586881 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) by patchwork1.kernel.org (Postfix) with ESMTP id 48B823FCFC for ; Fri, 12 Oct 2012 14:08:56 +0000 (UTC) Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1TMftF-0001Du-Rz; Fri, 12 Oct 2012 14:07:01 +0000 Received: from mail-ob0-f177.google.com ([209.85.214.177]) by merlin.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1TMftB-0001CG-Ce for linux-arm-kernel@lists.infradead.org; Fri, 12 Oct 2012 14:06:59 +0000 Received: by mail-ob0-f177.google.com with SMTP id wd20so3006861obb.36 for ; Fri, 12 Oct 2012 07:06:55 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:x-gm-message-state; bh=7JVKGC3nFyNX2E0Ayzm1K7cbx2Pl6PlwY/7rAGCdXrI=; b=kzU4gmz2OmAE3o4yt7uskzQEOw5EFt8pKIJeDh2E9Sl0qtkfu98Fx4SZHddQ5TDVFn 3qOT9nsgiyXziV4cjbrA8lbyQ/HzG82TcmN8HGxizZTr+BtXvJNaMjZszUVDW1dHWcJu PKwaVpq0MzHvuvG9v4oIJ3zsr1AjM+6FcrFiGGXiEqGESiKr9igvMtpGSIjTWbnvI5Ya hOlieRoKiSegaqR01tk4AVxWa+zV6bgRuwXPrultCTMaRDyjWs7+KumdK5pyWOE9LhLc tPBfnBkwkGI+eBcW+5l78Y+EemKifG67+hHOVzABKdXZ85YXCJEf4tWOYke9GtrOEsmI rTdw== MIME-Version: 1.0 Received: by 10.182.38.65 with SMTP id e1mr3586543obk.3.1350050815558; Fri, 12 Oct 2012 07:06:55 -0700 (PDT) Received: by 10.76.98.102 with HTTP; Fri, 12 Oct 2012 07:06:55 -0700 (PDT) In-Reply-To: <1350048519.10584.158.camel@smile> References: <142ef9170a2c69657d8a05ac127a9970d7b04965.1350020375.git.viresh.kumar@linaro.org> <91e41b4cc972d298f714cbd6f400569a9710304c.1350020375.git.viresh.kumar@linaro.org> <1350048519.10584.158.camel@smile> Date: Fri, 12 Oct 2012 19:36:55 +0530 Message-ID: Subject: Re: [PATCH 2/3] dmaengine: dw_dmac: Enhance device tree support From: Viresh Kumar To: "Shevchenko, Andriy" X-Gm-Message-State: ALoCoQnXynCFPeWEx0Ze+6BEpZlP9VKCnpASjOSJ3+K+V9T8dwTdY/DAcEmGbY9ph/4riN8Yslua X-Spam-Note: CRM114 invocation failed X-Spam-Score: -2.6 (--) X-Spam-Report: SpamAssassin version 3.3.2 on merlin.infradead.org summary: Content analysis details: (-2.6 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low trust [209.85.214.177 listed in list.dnswl.org] -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] Cc: "Koul, Vinod" , devicetree-discuss , "spear-devel@list.st.com" , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-arm-kernel-bounces@lists.infradead.org Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org On 12 October 2012 18:58, Shevchenko, Andriy wrote: > On Fri, 2012-10-12 at 11:14 +0530, Viresh Kumar wrote: >> + if (!of_property_read_u32(np, "chan_allocation_order", >> + &val)) > Fits one line? yes. >> + pdata->chan_allocation_order = (unsigned char)val; > do we really need explicit casting here? Obviously not, bigger to smaller is automatically casted. My coding standard is going down :( Same for all casting comments. >> + if (!of_property_read_u32(np, "nr_masters", &val)) { >> + if (val > 4) > I thought once that we might introduce constant like #define > DW_MAX_AHB_MASTERS 4. It seems a bit useless because hw is designed for > that number anyway, but maybe you have another opinion. Not everyone have four masters in there SoC configurations. So its better to export correct values. >> + if (!of_property_read_u32_array(np, "data_width", arr, >> + pdata->nr_masters)) >> + for (count = 0; count < pdata->nr_masters; count++) >> + pdata->data_width[count] = arr[count]; > Ah, it would be nice to have of_property_read_u8_array and so on... :) Maybe yes. I don't want to do it with this patchset, as that will take more time to go through maintainers. >> + /* calculate number of slaves */ >> + for_each_child_of_node(sn, cn) >> + count++; > > of.h: static inline int of_get_child_count(const struct device_node *np) Hehe.. Will use that. This proves there is no efficient way of finding child nodes, than this. >> + for_each_child_of_node(sn, cn) { >> + of_property_read_string(cn, "bus_id", &sd[count].bus_id); >> + of_property_read_u32(cn, "cfg_hi", &sd[count].cfg_hi); >> + of_property_read_u32(cn, "cfg_lo", &sd[count].cfg_lo); >> + if (!of_property_read_u32(cn, "src_master", &val)) >> + sd[count].src_master = (u8)val; >> + >> + if (!of_property_read_u32(cn, "dst_master", &val)) >> + sd[count].dst_master = (u8)val; >> + >> + sd[count].dma_dev = &pdev->dev; >> + count++; >> + } > > what about to define sd as sd[0]; in the platform_data structure and > then use smth like following > > struct ... *sd = pdata->sd; > for_each... { > sd->... = ; > sd++; > } > > it will probably require to split this part in a separate function and > calculate count of slave_info children before pdata memory allocation. Liked the idea partially. Check my new implementation in fixup for all these comments: ---------------------------8<--------------------------- From: Viresh Kumar Date: Fri, 12 Oct 2012 19:35:44 +0530 Subject: [PATCH] fixup! dmaengine: dw_dmac: Enhance device tree support --- drivers/dma/dw_dmac.c | 50 ++++++++++++++++++++++--------------------------- include/linux/dw_dmac.h | 2 +- 2 files changed, 23 insertions(+), 29 deletions(-) diff --git a/drivers/dma/dw_dmac.c b/drivers/dma/dw_dmac.c index c24859e..d72c26f 100644 --- a/drivers/dma/dw_dmac.c +++ b/drivers/dma/dw_dmac.c @@ -1179,7 +1179,7 @@ static void dwc_free_chan_resources(struct dma_chan *chan) dev_vdbg(chan2dev(chan), "%s: done\n", __func__); } -bool dw_generic_filter(struct dma_chan *chan, void *param) +bool dw_dma_generic_filter(struct dma_chan *chan, void *param) { struct dw_dma *dw = to_dw_dma(chan->device); static struct dw_dma *last_dw; @@ -1224,7 +1224,7 @@ bool dw_generic_filter(struct dma_chan *chan, void *param) last_bus_id = param; return false; } -EXPORT_SYMBOL(dw_generic_filter); +EXPORT_SYMBOL(dw_dma_generic_filter); /* --------------------- Cyclic DMA API extensions -------------------- */ @@ -1516,7 +1516,7 @@ __devinit dw_dma_parse_dt(struct platform_device *pdev) struct device_node *sn, *cn, *np = pdev->dev.of_node; struct dw_dma_platform_data *pdata; struct dw_dma_slave *sd; - u32 count, val, arr[4]; + u32 val, arr[4]; if (!np) { dev_err(&pdev->dev, "Missing DT data\n"); @@ -1533,63 +1533,57 @@ __devinit dw_dma_parse_dt(struct platform_device *pdev) if (of_property_read_bool(np, "is_private")) pdata->is_private = true; - if (!of_property_read_u32(np, "chan_allocation_order", - &val)) + if (!of_property_read_u32(np, "chan_allocation_order", &val)) pdata->chan_allocation_order = (unsigned char)val; if (!of_property_read_u32(np, "chan_priority", &val)) - pdata->chan_priority = (unsigned char)val; + pdata->chan_priority = val; if (!of_property_read_u32(np, "block_size", &val)) - pdata->block_size = (unsigned short)val; + pdata->block_size = val; if (!of_property_read_u32(np, "nr_masters", &val)) { if (val > 4) return NULL; - pdata->nr_masters = (unsigned char)val; + pdata->nr_masters = val; } if (!of_property_read_u32_array(np, "data_width", arr, pdata->nr_masters)) - for (count = 0; count < pdata->nr_masters; count++) - pdata->data_width[count] = arr[count]; + for (val = 0; val < pdata->nr_masters; val++) + pdata->data_width[val] = arr[val]; /* parse slave data */ sn = of_find_node_by_name(np, "slave_info"); if (!sn) return pdata; - count = 0; /* calculate number of slaves */ - for_each_child_of_node(sn, cn) - count++; - - if (!count) + val = of_get_child_count(sn); + if (!val) return NULL; - sd = devm_kzalloc(&pdev->dev, sizeof(*sd) * count, GFP_KERNEL); + sd = devm_kzalloc(&pdev->dev, sizeof(*sd) * val, GFP_KERNEL); if (!sd) return NULL; - count = 0; + pdata->sd = sd; + pdata->sd_count = val; + for_each_child_of_node(sn, cn) { - of_property_read_string(cn, "bus_id", &sd[count].bus_id); - of_property_read_u32(cn, "cfg_hi", &sd[count].cfg_hi); - of_property_read_u32(cn, "cfg_lo", &sd[count].cfg_lo); + sd->dma_dev = &pdev->dev; + of_property_read_string(cn, "bus_id", &sd->bus_id); + of_property_read_u32(cn, "cfg_hi", &sd->cfg_hi); + of_property_read_u32(cn, "cfg_lo", &sd->cfg_lo); if (!of_property_read_u32(cn, "src_master", &val)) - sd[count].src_master = (u8)val; + sd->src_master = val; if (!of_property_read_u32(cn, "dst_master", &val)) - sd[count].dst_master = (u8)val; - - sd[count].dma_dev = &pdev->dev; - count++; + sd->dst_master = val; + sd++; } - pdata->sd = sd; - pdata->sd_count = count; - return pdata; } #else diff --git a/include/linux/dw_dmac.h b/include/linux/dw_dmac.h index 4d1c8c3..41766de 100644 --- a/include/linux/dw_dmac.h +++ b/include/linux/dw_dmac.h @@ -114,6 +114,6 @@ void dw_dma_cyclic_stop(struct dma_chan *chan); dma_addr_t dw_dma_get_src_addr(struct dma_chan *chan); dma_addr_t dw_dma_get_dst_addr(struct dma_chan *chan); -bool dw_generic_filter(struct dma_chan *chan, void *param); +bool dw_dma_generic_filter(struct dma_chan *chan, void *param); #endif /* DW_DMAC_H */