Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755145AbbL3P2X (ORCPT ); Wed, 30 Dec 2015 10:28:23 -0500 Received: from smtp.codeaurora.org ([198.145.29.96]:39991 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755114AbbL3P2S (ORCPT ); Wed, 30 Dec 2015 10:28:18 -0500 Subject: Re: [PATCH V10 7/7] dma: qcom_hidma: add support for object hierarchy To: Andy Shevchenko References: <1450371684-23415-1-git-send-email-okaya@codeaurora.org> <1450371684-23415-8-git-send-email-okaya@codeaurora.org> Cc: dmaengine , Mark Rutland , Timur Tabi , devicetree , cov@codeaurora.org, Vinod Koul , jcm@redhat.com, Andy Gross , Arnd Bergmann , linux-arm-msm@vger.kernel.org, linux-arm Mailing List , "linux-kernel@vger.kernel.org" From: Sinan Kaya Message-ID: <5683F81E.5050406@codeaurora.org> Date: Wed, 30 Dec 2015 10:28:30 -0500 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6620 Lines: 197 On 12/19/2015 1:37 PM, Andy Shevchenko wrote: > On Thu, Dec 17, 2015 at 7:01 PM, Sinan Kaya wrote: >> In order to create a relationship model between the channels and the >> management object, we are adding support for object hierarchy to the >> drivers. This patch simplifies the userspace application development. >> We will not have to traverse different firmware paths based on device >> tree or ACPI baed kernels. >> >> No matter what flavor of kernel is used, objects will be represented as >> platform devices. >> >> The new layout is as follows: >> >> hidmam_10: hidma-mgmt@0x5A000000 { >> compatible = "qcom,hidma-mgmt-1.0"; >> ... >> >> hidma_10: hidma@0x5a010000 { >> compatible = "qcom,hidma-1.0"; >> ... >> } >> } >> >> The hidma_mgmt_init detects each instance of the hidma-mgmt-1.0 objects >> in device tree and calls into the channel driver to create platform devices >> for each child of the management object. >> >> Signed-off-by: Sinan Kaya > > >> +What: /sys/devices/platform/hidma-*/chid >> + /sys/devices/platform/QCOM8061:*/chid >> +Date: Dec 2015 >> +KernelVersion: 4.4 >> +Contact: "Sinan Kaya " >> +Description: >> + Contains the ID of the channel within the HIDMA instance. >> + It is used to associate a given HIDMA channel with the >> + priority and weight calls in the management interface. > >> -module_platform_driver(hidma_mgmt_driver); >> +#ifdef CONFIG_OF >> +static int object_counter; >> + >> +static int __init hidma_mgmt_of_populate_channels(struct device_node *np) >> +{ >> + struct platform_device *pdev_parent = of_find_device_by_node(np); >> + struct platform_device_info pdevinfo; >> + struct of_phandle_args out_irq; >> + struct device_node *child; >> + struct resource *res; >> + const __be32 *cell; > > Always BE ? Yes, as Timur indicated device tree is big endian all the time. > >> + int ret = 0, size, i, num; >> + u64 addr, addr_size; > > resource_size_t These values are read from the device tree blob using of_read_number function. of_read_number returns a u64. > >> + >> + for_each_available_child_of_node(np, child) { >> + int iter = 0; >> + >> + cell = of_get_property(child, "reg", &size); >> + if (!cell) { >> + ret = -EINVAL; >> + goto out; >> + } >> + >> + size /= (sizeof(*cell)); > > Redundant parens. I think it might be good to use common sense for > operator precedence, here is a radical example of ignoring it. And > this is not the first case. Removed. Note that I copied most of this function from another driver. > >> + num = size / >> + (of_n_addr_cells(child) + of_n_size_cells(child)) + 1; >> + >> + /* allocate a resource array */ >> + res = kcalloc(num, sizeof(*res), GFP_KERNEL); >> + if (!res) { >> + ret = -ENOMEM; >> + goto out; >> + } >> + >> + /* read each reg value */ >> + i = 0; >> + while (i < size) { >> + addr = of_read_number(&cell[i], >> + of_n_addr_cells(child)); >> + i += of_n_addr_cells(child); >> + >> + addr_size = of_read_number(&cell[i], >> + of_n_size_cells(child)); >> + i += of_n_size_cells(child); >> + >> + res[iter].start = addr; >> + res[iter].end = res[iter].start + addr_size - 1; >> + res[iter].flags = IORESOURCE_MEM; > > res->start = … > … > res++; ok > >> + iter++; >> + } >> + >> + ret = of_irq_parse_one(child, 0, &out_irq); >> + if (ret) >> + goto out; >> + >> + res[iter].start = irq_create_of_mapping(&out_irq); >> + res[iter].name = "hidma event irq"; >> + res[iter].flags = IORESOURCE_IRQ; > > Ditto. ok > >> + >> + pdevinfo.fwnode = &child->fwnode; >> + pdevinfo.parent = pdev_parent ? &pdev_parent->dev : NULL; >> + pdevinfo.name = child->name; >> + pdevinfo.id = object_counter++; >> + pdevinfo.res = res; >> + pdevinfo.num_res = num; >> + pdevinfo.data = NULL; >> + pdevinfo.size_data = 0; >> + pdevinfo.dma_mask = DMA_BIT_MASK(64); >> + platform_device_register_full(&pdevinfo); >> + >> + kfree(res); >> + res = NULL; >> + } >> +out: > >> + kfree(res); > > >> + >> + return ret; >> +} >> +#endif >> + >> +static int __init hidma_mgmt_init(void) >> +{ >> +#ifdef CONFIG_OF >> + struct device_node *child; >> + >> + for (child = of_find_matching_node(NULL, hidma_mgmt_match); child; >> + child = of_find_matching_node(child, hidma_mgmt_match)) { >> + /* device tree based firmware here */ >> + hidma_mgmt_of_populate_channels(child); >> + of_node_put(child); >> + } > > And why this is can't be done in probe of parent node driver? The populate function creates platform objects using platform_device_register_full function above. The hidma device driver later probes the object during object enumeration phase. I tried moving platform_device_register_full into the probe context. The objects got generated but hidma device driver didn't probe the object. I suppose we are required to create the objects before the probing starts. I copied this pattern from another driver. > >> +#endif >> + platform_driver_register(&hidma_mgmt_driver); >> + >> + return 0; >> +} >> +module_init(hidma_mgmt_init); > -- Sinan Kaya Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- 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/