Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752827AbcLMXZb (ORCPT ); Tue, 13 Dec 2016 18:25:31 -0500 Received: from smtp.codeaurora.org ([198.145.29.96]:50810 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751567AbcLMXZ1 (ORCPT ); Tue, 13 Dec 2016 18:25:27 -0500 DMARC-Filter: OpenDMARC Filter v1.3.1 smtp.codeaurora.org E8294606CC Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=pass smtp.mailfrom=sboyd@codeaurora.org Date: Tue, 13 Dec 2016 15:25:24 -0800 From: Stephen Boyd To: Andy Shevchenko Cc: Irina Tirdea , linux-clk@vger.kernel.org, "x86@kernel.org" , platform-driver-x86@vger.kernel.org, Darren Hart , Thomas Gleixner , Michael Turquette , Ingo Molnar , "H. Peter Anvin" , ALSA Development Mailing List , Mark Brown , Takashi Iwai , Pierre-Louis Bossart , "Rafael J. Wysocki" , Len Brown , "linux-acpi@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Pierre-Louis Bossart Subject: Re: [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks Message-ID: <20161213232524.GQ5423@codeaurora.org> References: <1481306510-7471-1-git-send-email-irina.tirdea@intel.com> <1481306510-7471-2-git-send-email-irina.tirdea@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2101 Lines: 67 On 12/13, Andy Shevchenko wrote: > On Fri, Dec 9, 2016 at 8:01 PM, Irina Tirdea wrote: > > > --- a/drivers/clk/x86/Makefile > > +++ b/drivers/clk/x86/Makefile > > @@ -1,2 +1,5 @@ > > clk-x86-lpss-objs := clk-lpt.o > > obj-$(CONFIG_X86_INTEL_LPSS) += clk-x86-lpss.o > > > +ifeq ($(CONFIG_COMMON_CLK), y) > > Hmm... I think (I didn't check) we don't go here otherwise. We should move this statement to drivers/clk/Makefile around the x86 line. > > + void __iomem *base, > > + const char **parent_names, > > + int num_parents) > > +{ > > + struct clk_plt *pclk; > > + struct clk_init_data init; > > + int ret; > > + > > + pclk = devm_kzalloc(&pdev->dev, sizeof(*pclk), GFP_KERNEL); > > + if (!pclk) > > + return ERR_PTR(-ENOMEM); > > + > > + init.name = kasprintf(GFP_KERNEL, "%s%d", PLT_CLK_NAME_BASE, id); > > devm_kasprintf() Please no. > > > + init.ops = &plt_clk_ops; > > + init.flags = 0; > > + init.parent_names = parent_names; > > + init.num_parents = num_parents; > > + > > + pclk->hw.init = &init; > > + pclk->reg = base + id * PMC_CLK_CTL_SIZE; > > + spin_lock_init(&pclk->lock); > > + > > + ret = devm_clk_hw_register(&pdev->dev, &pclk->hw); > > + if (ret) > > + goto err_free_init; > > + > > + pclk->lookup = clkdev_hw_create(&pclk->hw, init.name, NULL); > > + if (!pclk->lookup) { > > + ret = -ENOMEM; > > + goto err_free_init; > > + } > > + > > > + kfree(init.name); > > devm_kfree(); It's all local to this function, devm isn't helping anything. Having one kfree() would be good though. And using init.name for the clkdev lookup is probably wrong and should be replaced with something more generic along with an associated device name. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project