From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932555AbdBHPME (ORCPT ); Wed, 8 Feb 2017 10:12:04 -0500 Received: from mail-wr0-f178.google.com ([209.85.128.178]:33240 "EHLO mail-wr0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754181AbdBHPL5 (ORCPT ); Wed, 8 Feb 2017 10:11:57 -0500 Subject: Re: [PATCH v6 4/9] media: venus: adding core part and helper functions To: Bjorn Andersson References: <1486473024-21705-1-git-send-email-stanimir.varbanov@linaro.org> <1486473024-21705-5-git-send-email-stanimir.varbanov@linaro.org> <20170207233202.GI27837@minitux> Cc: Mauro Carvalho Chehab , Hans Verkuil , Andy Gross , Stephen Boyd , Srinivas Kandagatla , linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org From: Stanimir Varbanov Message-ID: Date: Wed, 8 Feb 2017 17:01:54 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: <20170207233202.GI27837@minitux> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Bjorn, thanks for the comments! On 02/08/2017 01:32 AM, Bjorn Andersson wrote: > On Tue 07 Feb 05:10 PST 2017, Stanimir Varbanov wrote: > >> * firmware loader >> > > I like the way this turns out, just some style comments below. > > [..] >> diff --git a/drivers/media/platform/qcom/venus/firmware.c b/drivers/media/platform/qcom/venus/firmware.c >> new file mode 100644 >> index 000000000000..4057696abaf5 >> --- /dev/null >> +++ b/drivers/media/platform/qcom/venus/firmware.c >> @@ -0,0 +1,151 @@ >> +/* >> + * Copyright (C) 2017 Linaro Ltd. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 and >> + * only version 2 as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#define VENUS_FIRMWARE_NAME "venus.mdt" >> +#define VENUS_PAS_ID 9 >> +#define VENUS_FW_MEM_SIZE SZ_8M >> + >> +struct firmware_mem { >> + struct device dev; >> + void *mem_va; >> + phys_addr_t mem_phys; >> + size_t mem_size; >> +}; >> + >> +static struct firmware_mem fw; > > Rather than operating on a global variable I think you should either > return your firmware_mem pointer or the device pointer to the caller of > venus_boot() and have the core pass that back into venus_shutdown(). I will take your comments and will pass struct device *fw_dev as an argument of venus_boot. Also I will move memory allocation in venus_boot and by that way I don't need to keep memory attributes from above structure. -- regards, Stan