Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753260AbbDBW37 (ORCPT ); Thu, 2 Apr 2015 18:29:59 -0400 Received: from mail-yh0-f53.google.com ([209.85.213.53]:35543 "EHLO mail-yh0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753228AbbDBW3y (ORCPT ); Thu, 2 Apr 2015 18:29:54 -0400 MIME-Version: 1.0 In-Reply-To: <3b757ab0549f700dafb87abb2cda3ac5.squirrel@www.codeaurora.org> References: <1427922770-13721-1-git-send-email-jilaiw@codeaurora.org> <3b757ab0549f700dafb87abb2cda3ac5.squirrel@www.codeaurora.org> Date: Thu, 2 Apr 2015 23:29:53 +0100 Message-ID: Subject: Re: [PATCH 2/3] drm:msm: Initial Add Writeback Support From: Emil Velikov To: Jilai Wang Cc: ML dri-devel , linux-arm-msm , "Linux-Kernel@Vger. Kernel. Org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2395 Lines: 68 On 2 April 2015 at 19:07, wrote: > Thanks Emil. Please check the comments embedded and for the rest I will > update the code. > >> Hi Jilai, >> >> Just a few questions, not really a review as I'm not that familiar >> with the code. >> >> >>> +++ b/drivers/gpu/drm/msm/Kconfig >>> @@ -27,6 +27,16 @@ config DRM_MSM_FBDEV >>> support. Note that this support also provide the linux console >>> support on top of the MSM modesetting driver. >>> >>> +config DRM_MSM_WB >>> + bool "Enable writeback support for MSM modesetting driver" >>> + depends on DRM_MSM >>> + depends on VIDEO_V4L2 >>> + select VIDEOBUF2_CORE >>> + default y >>> + help >>> + Choose this option if you have a need to support writeback >>> + connector. >>> + >> Is it worth mentioning which devices/SoCs have such connector ? > If the devices have WB connector, it will be added in the device tree. > I was thinking more about listing a one or two SoCs so that one gets the general idea. Although it might end up more confusing than helpful as more devices become available. Suggestion withdrawn. >>> +static struct msm_wb *msm_wb_init(struct platform_device *pdev) >>> +{ >>> + struct msm_wb *wb = NULL; >>> + >>> + wb = devm_kzalloc(&pdev->dev, sizeof(*wb), GFP_KERNEL); >>> + if (!wb) >>> + return ERR_PTR(-ENOMEM); >>> + >>> + wb->pdev = pdev; >>> + wb->priv_data = devm_kzalloc(&pdev->dev, sizeof(*wb->priv_data), >>> + GFP_KERNEL); >>> + if (!wb->priv_data) >>> + return ERR_PTR(-ENOMEM); >>> + >>> + if (msm_wb_v4l2_init(wb)) { >>> + pr_err("%s: wb v4l2 init failed\n", __func__); >>> + return ERR_PTR(-ENODEV); >>> + } >>> + >> Seems like we're leaking wb and/or wb->priv_data. Add a label and >> consolidate error handling in there ? > > Since the devm_kzalloc function is used here, the system should take care > freeing the memory. You're spot on. Somehow I did not notice that we're using the devm_ version of kzalloc. /me runs in shame :-) Cheers, Emil -- 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/