From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751842AbdLAQjI (ORCPT ); Fri, 1 Dec 2017 11:39:08 -0500 Received: from smtp.codeaurora.org ([198.145.29.96]:46060 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751486AbdLAQjG (ORCPT ); Fri, 1 Dec 2017 11:39:06 -0500 DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org E0FC260364 Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=none smtp.mailfrom=sboyd@codeaurora.org Date: Fri, 1 Dec 2017 08:39:03 -0800 From: Stephen Boyd To: Yixun Lan Cc: Neil Armstrong , Jerome Brunet , Kevin Hilman , Rob Herring , Mark Rutland , Michael Turquette , Carlo Caione , Qiufang Dai , linux-amlogic@lists.infradead.org, devicetree@vger.kernel.org, linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v3 2/3] clk: meson-axg: add clock controller drivers Message-ID: <20171201163903.GF19419@codeaurora.org> References: <20171128125330.363-1-yixun.lan@amlogic.com> <20171128125330.363-3-yixun.lan@amlogic.com> <20171129193433.GA19419@codeaurora.org> 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 On 11/30, Yixun Lan wrote: > Hi Stephen > > On 11/30/17 03:34, Stephen Boyd wrote: > > On 11/28, Yixun Lan wrote: > >> diff --git a/drivers/clk/meson/axg.c b/drivers/clk/meson/axg.c > >> new file mode 100644 > >> index 000000000000..51c5b4062715 > >> --- /dev/null > >> +++ b/drivers/clk/meson/axg.c > >> @@ -0,0 +1,948 @@ > >> +/* > >> + * AmLogic Meson-AXG Clock Controller Driver > >> + * > >> + * Copyright (c) 2016 Baylibre SAS. > >> + * Author: Michael Turquette > >> + * > >> + * Copyright (c) 2017 Amlogic, inc. > >> + * Author: Qiufang Dai > >> + * > >> + * SPDX-License-Identifier: GPL-2.0+ > >> + */ > >> + > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> + > >> +#include "clkc.h" > >> +#include "axg.h" > >> + > >> +static DEFINE_SPINLOCK(clk_lock); > > > > meson_axg_clk_lock? > > > em... I'd leave it unchanged > > because the spinlock will be used at macro MESON_GATE() [1] which > defined at drivers/clk/meson/clkc.h, and it assume using the generic > name 'clk_lock', change name will break the code.. > > and besides it's already defined as static, so I see no problem here The problem is lockdep debugging and ctags/grep on the source code. clk_lock is very generic when it should be more specific so we can find this lock later on from a lockdep report with a simple search of the code. Maybe make another patch to rename it to meson_clk_lock so the macro doesn't have to change too much, and we get it slightly more unique. Looks like v4l2 also has a clk_lock. > > >> +}; > >> + > >> +static int axg_clkc_probe(struct platform_device *pdev) > >> +{ > >> + const struct clkc_data *clkc_data; > >> + void __iomem *clk_base; > >> + int ret, clkid, i; > >> + struct device *dev = &pdev->dev; > >> + > >> + clkc_data = of_device_get_match_data(&pdev->dev); > >> + if (!clkc_data) > >> + return -EINVAL; > >> + > >> + /* Generic clocks and PLLs */ > >> + clk_base = of_iomap(dev->of_node, 0); > > > > Use platform device APIs for ioremapping? > > > I assume you are referring to 'platform_get_resource + > devm_ioremap_resource' ? > > the idea sounds good to me. Yes. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Boyd Subject: Re: [PATCH v3 2/3] clk: meson-axg: add clock controller drivers Date: Fri, 1 Dec 2017 08:39:03 -0800 Message-ID: <20171201163903.GF19419@codeaurora.org> References: <20171128125330.363-1-yixun.lan@amlogic.com> <20171128125330.363-3-yixun.lan@amlogic.com> <20171129193433.GA19419@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Yixun Lan Cc: Neil Armstrong , Jerome Brunet , Kevin Hilman , Rob Herring , Mark Rutland , Michael Turquette , Carlo Caione , Qiufang Dai , linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-clk-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: devicetree@vger.kernel.org On 11/30, Yixun Lan wrote: > Hi Stephen > > On 11/30/17 03:34, Stephen Boyd wrote: > > On 11/28, Yixun Lan wrote: > >> diff --git a/drivers/clk/meson/axg.c b/drivers/clk/meson/axg.c > >> new file mode 100644 > >> index 000000000000..51c5b4062715 > >> --- /dev/null > >> +++ b/drivers/clk/meson/axg.c > >> @@ -0,0 +1,948 @@ > >> +/* > >> + * AmLogic Meson-AXG Clock Controller Driver > >> + * > >> + * Copyright (c) 2016 Baylibre SAS. > >> + * Author: Michael Turquette > >> + * > >> + * Copyright (c) 2017 Amlogic, inc. > >> + * Author: Qiufang Dai > >> + * > >> + * SPDX-License-Identifier: GPL-2.0+ > >> + */ > >> + > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> + > >> +#include "clkc.h" > >> +#include "axg.h" > >> + > >> +static DEFINE_SPINLOCK(clk_lock); > > > > meson_axg_clk_lock? > > > em... I'd leave it unchanged > > because the spinlock will be used at macro MESON_GATE() [1] which > defined at drivers/clk/meson/clkc.h, and it assume using the generic > name 'clk_lock', change name will break the code.. > > and besides it's already defined as static, so I see no problem here The problem is lockdep debugging and ctags/grep on the source code. clk_lock is very generic when it should be more specific so we can find this lock later on from a lockdep report with a simple search of the code. Maybe make another patch to rename it to meson_clk_lock so the macro doesn't have to change too much, and we get it slightly more unique. Looks like v4l2 also has a clk_lock. > > >> +}; > >> + > >> +static int axg_clkc_probe(struct platform_device *pdev) > >> +{ > >> + const struct clkc_data *clkc_data; > >> + void __iomem *clk_base; > >> + int ret, clkid, i; > >> + struct device *dev = &pdev->dev; > >> + > >> + clkc_data = of_device_get_match_data(&pdev->dev); > >> + if (!clkc_data) > >> + return -EINVAL; > >> + > >> + /* Generic clocks and PLLs */ > >> + clk_base = of_iomap(dev->of_node, 0); > > > > Use platform device APIs for ioremapping? > > > I assume you are referring to 'platform_get_resource + > devm_ioremap_resource' ? > > the idea sounds good to me. Yes. -- 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 devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 From: sboyd@codeaurora.org (Stephen Boyd) Date: Fri, 1 Dec 2017 08:39:03 -0800 Subject: [PATCH v3 2/3] clk: meson-axg: add clock controller drivers In-Reply-To: References: <20171128125330.363-1-yixun.lan@amlogic.com> <20171128125330.363-3-yixun.lan@amlogic.com> <20171129193433.GA19419@codeaurora.org> Message-ID: <20171201163903.GF19419@codeaurora.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 11/30, Yixun Lan wrote: > Hi Stephen > > On 11/30/17 03:34, Stephen Boyd wrote: > > On 11/28, Yixun Lan wrote: > >> diff --git a/drivers/clk/meson/axg.c b/drivers/clk/meson/axg.c > >> new file mode 100644 > >> index 000000000000..51c5b4062715 > >> --- /dev/null > >> +++ b/drivers/clk/meson/axg.c > >> @@ -0,0 +1,948 @@ > >> +/* > >> + * AmLogic Meson-AXG Clock Controller Driver > >> + * > >> + * Copyright (c) 2016 Baylibre SAS. > >> + * Author: Michael Turquette > >> + * > >> + * Copyright (c) 2017 Amlogic, inc. > >> + * Author: Qiufang Dai > >> + * > >> + * SPDX-License-Identifier: GPL-2.0+ > >> + */ > >> + > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> + > >> +#include "clkc.h" > >> +#include "axg.h" > >> + > >> +static DEFINE_SPINLOCK(clk_lock); > > > > meson_axg_clk_lock? > > > em... I'd leave it unchanged > > because the spinlock will be used at macro MESON_GATE() [1] which > defined at drivers/clk/meson/clkc.h, and it assume using the generic > name 'clk_lock', change name will break the code.. > > and besides it's already defined as static, so I see no problem here The problem is lockdep debugging and ctags/grep on the source code. clk_lock is very generic when it should be more specific so we can find this lock later on from a lockdep report with a simple search of the code. Maybe make another patch to rename it to meson_clk_lock so the macro doesn't have to change too much, and we get it slightly more unique. Looks like v4l2 also has a clk_lock. > > >> +}; > >> + > >> +static int axg_clkc_probe(struct platform_device *pdev) > >> +{ > >> + const struct clkc_data *clkc_data; > >> + void __iomem *clk_base; > >> + int ret, clkid, i; > >> + struct device *dev = &pdev->dev; > >> + > >> + clkc_data = of_device_get_match_data(&pdev->dev); > >> + if (!clkc_data) > >> + return -EINVAL; > >> + > >> + /* Generic clocks and PLLs */ > >> + clk_base = of_iomap(dev->of_node, 0); > > > > Use platform device APIs for ioremapping? > > > I assume you are referring to 'platform_get_resource + > devm_ioremap_resource' ? > > the idea sounds good to me. Yes. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project From mboxrd@z Thu Jan 1 00:00:00 1970 From: sboyd@codeaurora.org (Stephen Boyd) Date: Fri, 1 Dec 2017 08:39:03 -0800 Subject: [PATCH v3 2/3] clk: meson-axg: add clock controller drivers In-Reply-To: References: <20171128125330.363-1-yixun.lan@amlogic.com> <20171128125330.363-3-yixun.lan@amlogic.com> <20171129193433.GA19419@codeaurora.org> Message-ID: <20171201163903.GF19419@codeaurora.org> To: linus-amlogic@lists.infradead.org List-Id: linus-amlogic.lists.infradead.org On 11/30, Yixun Lan wrote: > Hi Stephen > > On 11/30/17 03:34, Stephen Boyd wrote: > > On 11/28, Yixun Lan wrote: > >> diff --git a/drivers/clk/meson/axg.c b/drivers/clk/meson/axg.c > >> new file mode 100644 > >> index 000000000000..51c5b4062715 > >> --- /dev/null > >> +++ b/drivers/clk/meson/axg.c > >> @@ -0,0 +1,948 @@ > >> +/* > >> + * AmLogic Meson-AXG Clock Controller Driver > >> + * > >> + * Copyright (c) 2016 Baylibre SAS. > >> + * Author: Michael Turquette > >> + * > >> + * Copyright (c) 2017 Amlogic, inc. > >> + * Author: Qiufang Dai > >> + * > >> + * SPDX-License-Identifier: GPL-2.0+ > >> + */ > >> + > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> +#include > >> + > >> +#include "clkc.h" > >> +#include "axg.h" > >> + > >> +static DEFINE_SPINLOCK(clk_lock); > > > > meson_axg_clk_lock? > > > em... I'd leave it unchanged > > because the spinlock will be used at macro MESON_GATE() [1] which > defined at drivers/clk/meson/clkc.h, and it assume using the generic > name 'clk_lock', change name will break the code.. > > and besides it's already defined as static, so I see no problem here The problem is lockdep debugging and ctags/grep on the source code. clk_lock is very generic when it should be more specific so we can find this lock later on from a lockdep report with a simple search of the code. Maybe make another patch to rename it to meson_clk_lock so the macro doesn't have to change too much, and we get it slightly more unique. Looks like v4l2 also has a clk_lock. > > >> +}; > >> + > >> +static int axg_clkc_probe(struct platform_device *pdev) > >> +{ > >> + const struct clkc_data *clkc_data; > >> + void __iomem *clk_base; > >> + int ret, clkid, i; > >> + struct device *dev = &pdev->dev; > >> + > >> + clkc_data = of_device_get_match_data(&pdev->dev); > >> + if (!clkc_data) > >> + return -EINVAL; > >> + > >> + /* Generic clocks and PLLs */ > >> + clk_base = of_iomap(dev->of_node, 0); > > > > Use platform device APIs for ioremapping? > > > I assume you are referring to 'platform_get_resource + > devm_ioremap_resource' ? > > the idea sounds good to me. Yes. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project