Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1539583 > unrolled thread

[PATCH v6 0/3] Add platform clock for BayTrail platforms

Back to article view | Back to linux.kernel

Contents

  [PATCH v6 0/3] Add platform clock for BayTrail platforms Irina Tirdea <irina.tirdea@intel.com> - 2016-12-09 19:10 +0100
    [PATCH v6 2/3] arch/x86/platform/atom: Move pmc_atom to drivers/platform/x86 Irina Tirdea <irina.tirdea@intel.com> - 2016-12-09 19:10 +0100
      Re: [PATCH v6 2/3] arch/x86/platform/atom: Move pmc_atom to drivers/platform/x86 Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-12-13 00:50 +0100
        Re: [PATCH v6 2/3] arch/x86/platform/atom: Move pmc_atom to  drivers/platform/x86 Darren Hart <dvhart@infradead.org> - 2016-12-16 19:30 +0100
          Re: [PATCH v6 2/3] arch/x86/platform/atom: Move pmc_atom to drivers/platform/x86 Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-12-16 19:40 +0100
    [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks Irina Tirdea <irina.tirdea@intel.com> - 2016-12-09 19:10 +0100
      Re: [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-12-13 00:40 +0100
        Re: [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> - 2016-12-13 01:20 +0100
          Re: [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-12-13 01:30 +0100
            Re: [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks Darren Hart <dvhart@infradead.org> - 2016-12-16 19:40 +0100
              Re: [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-12-16 19:50 +0100
                Re: [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks Darren Hart <dvhart@infradead.org> - 2016-12-16 20:30 +0100
                  Re: [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-12-16 23:30 +0100
                    Re: [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks Darren Hart <dvhart@infradead.org> - 2016-12-17 00:00 +0100
              Re: [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks Mark Brown <broonie@kernel.org> - 2016-12-19 12:10 +0100
          Re: [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-12-13 02:20 +0100
        Re: [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks Stephen Boyd <sboyd@codeaurora.org> - 2016-12-14 00:30 +0100
          Re: [alsa-devel] [PATCH v6 1/3] clk: x86: Add Atom PMC platform  clocks Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> - 2016-12-16 06:20 +0100
            Re: [alsa-devel] [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-12-16 09:50 +0100
              Re: [alsa-devel] [PATCH v6 1/3] clk: x86: Add Atom PMC platform  clocks Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> - 2016-12-16 16:00 +0100
            Re: [alsa-devel] [PATCH v6 1/3] clk: x86: Add Atom PMC platform  clocks Stephen Boyd <sboyd@codeaurora.org> - 2016-12-17 02:40 +0100
              Re: [alsa-devel] [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-12-17 15:00 +0100
                Re: [alsa-devel] [PATCH v6 1/3] clk: x86: Add Atom PMC platform  clocks Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> - 2016-12-19 17:20 +0100
                  Re: [alsa-devel] [PATCH v6 1/3] clk: x86: Add Atom PMC platform  clocks Stephen Boyd <sboyd@codeaurora.org> - 2016-12-22 00:10 +0100
                    Re: [alsa-devel] [PATCH v6 1/3] clk: x86: Add Atom PMC platform  clocks Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> - 2016-12-22 02:10 +0100
                      Re: [alsa-devel] [PATCH v6 1/3] clk: x86: Add Atom PMC platform  clocks Stephen Boyd <sboyd@codeaurora.org> - 2016-12-22 19:30 +0100
                      Re: [alsa-devel] [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-12-22 19:50 +0100
    [PATCH v6 3/3] platform/x86: Enable Atom PMC platform clocks Irina Tirdea <irina.tirdea@intel.com> - 2016-12-09 19:10 +0100
      Re: [PATCH v6 3/3] platform/x86: Enable Atom PMC platform clocks Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-12-13 01:10 +0100

Page 1 of 2  [1] 2  Next page →

#1539583 — [PATCH v6 0/3] Add platform clock for BayTrail platforms

These patches specifically enable the audio MCLK required by Baytrail CR
devices. It is the remaining part of a bigger set of patches
(already merged in Mark's tree) that enable sound for Baytrail CR devices
(especially Asus T100TAF) [1]. They include the clock driver and enabling
the clock in the pmc_atom code (along with moving of the non-architectural
pmc_atom driver code into drivers/platform/x86).

[1] http://mailman.alsa-project.org/pipermail/alsa-devel/2016-August/111704.html

Changes from v5:
 - fix build error reported by kbuild test robot
 - split the clk driver code from x86 platform changes

Changes from v4:
 - move the pmc_atom driver from arch/x86/platform/atom to
drivers/platform/x86

Changes from v3:
 - replace devm_kzalloc with devm_kcalloc
 - add x86 architecture maintainers

Changes from v2:
 - move clk platform data structures to a separate include file
 - store clk_hw pointer for the fixed rate clocks

Changes from v1:
 - register the clk device as child of pmc device
 - pass iomem pointer from pmc driver to clk driver to avoid using
pmc_atom_read()/write() and use readl/writel API instead
 - use devm_clk_hw_register/clkdev_hw_create instead of
clk_register/clkdev_create

Irina Tirdea (3):
  clk: x86: Add Atom PMC platform clocks
  arch/x86/platform/atom: Move pmc_atom to drivers/platform/x86
  platform/x86: Enable Atom PMC platform clocks

 arch/x86/Kconfig                                   |   4 -
 arch/x86/platform/atom/Makefile                    |   1 -
 drivers/acpi/acpi_lpss.c                           |   2 +-
 drivers/clk/x86/Makefile                           |   3 +
 drivers/clk/x86/clk-byt-plt.c                      | 380 +++++++++++++++++++++
 drivers/platform/x86/Kconfig                       |   5 +
 drivers/platform/x86/Makefile                      |   1 +
 .../atom => drivers/platform/x86}/pmc_atom.c       |  81 ++++-
 include/linux/platform_data/x86/clk-byt-plt.h      |  31 ++
 .../linux/platform_data/x86}/pmc_atom.h            |   3 +
 10 files changed, 500 insertions(+), 11 deletions(-)
 create mode 100644 drivers/clk/x86/clk-byt-plt.c
 rename {arch/x86/platform/atom => drivers/platform/x86}/pmc_atom.c (88%)
 create mode 100644 include/linux/platform_data/x86/clk-byt-plt.h
 rename {arch/x86/include/asm => include/linux/platform_data/x86}/pmc_atom.h (98%)

-- 
1.9.1

[toc] | [next] | [standalone]

#1539587 — [PATCH v6 2/3] arch/x86/platform/atom: Move pmc_atom to drivers/platform/x86

The pmc_atom driver does not contain any architecture specific
code. It only enables the SOC Power Management Controller Driver
for BayTrail and CherryTrail platforms.

Move the pmc_atom driver from arch/x86/platform/atom to
drivers/platform/x86.

Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
---
 arch/x86/Kconfig                                                     | 4 ----
 arch/x86/platform/atom/Makefile                                      | 1 -
 drivers/acpi/acpi_lpss.c                                             | 2 +-
 drivers/platform/x86/Kconfig                                         | 4 ++++
 drivers/platform/x86/Makefile                                        | 1 +
 {arch/x86/platform/atom => drivers/platform/x86}/pmc_atom.c          | 3 +--
 {arch/x86/include/asm => include/linux/platform_data/x86}/pmc_atom.h | 0
 7 files changed, 7 insertions(+), 8 deletions(-)
 rename {arch/x86/platform/atom => drivers/platform/x86}/pmc_atom.c (99%)
 rename {arch/x86/include/asm => include/linux/platform_data/x86}/pmc_atom.h (100%)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index bada636..5a009f0 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2753,10 +2753,6 @@ config X86_DMA_REMAP
 	bool
 	depends on STA2X11
 
-config PMC_ATOM
-	def_bool y
-        depends on PCI
-
 source "net/Kconfig"
 
 source "drivers/Kconfig"
diff --git a/arch/x86/platform/atom/Makefile b/arch/x86/platform/atom/Makefile
index 40983f5..57be88f 100644
--- a/arch/x86/platform/atom/Makefile
+++ b/arch/x86/platform/atom/Makefile
@@ -1,2 +1 @@
-obj-$(CONFIG_PMC_ATOM)		+= pmc_atom.o
 obj-$(CONFIG_PUNIT_ATOM_DEBUG)	+= punit_atom_debug.o
diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index 373657f..3e4c566 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -18,6 +18,7 @@
 #include <linux/mutex.h>
 #include <linux/platform_device.h>
 #include <linux/platform_data/clk-lpss.h>
+#include <linux/platform_data/x86/pmc_atom.h>
 #include <linux/pm_domain.h>
 #include <linux/pm_runtime.h>
 #include <linux/delay.h>
@@ -31,7 +32,6 @@
 #include <asm/cpu_device_id.h>
 #include <asm/intel-family.h>
 #include <asm/iosf_mbi.h>
-#include <asm/pmc_atom.h>
 
 #define LPSS_ADDR(desc) ((unsigned long)&desc)
 
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index b8a21d7..21dce1e 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1028,3 +1028,7 @@ config INTEL_TELEMETRY
 	  directly via debugfs files. Various tools may use
 	  this interface for SoC state monitoring.
 endif # X86_PLATFORM_DEVICES
+
+config PMC_ATOM
+	def_bool y
+        depends on PCI
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 2efa86d..8568d74 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -71,3 +71,4 @@ obj-$(CONFIG_INTEL_TELEMETRY)	+= intel_telemetry_core.o \
 				   intel_telemetry_pltdrv.o \
 				   intel_telemetry_debugfs.o
 obj-$(CONFIG_INTEL_PMC_CORE)    += intel_pmc_core.o
+obj-$(CONFIG_PMC_ATOM)		+= pmc_atom.o
diff --git a/arch/x86/platform/atom/pmc_atom.c b/drivers/platform/x86/pmc_atom.c
similarity index 99%
rename from arch/x86/platform/atom/pmc_atom.c
rename to drivers/platform/x86/pmc_atom.c
index 964ff4f..b53fbc1 100644
--- a/arch/x86/platform/atom/pmc_atom.c
+++ b/drivers/platform/x86/pmc_atom.c
@@ -21,8 +21,7 @@
 #include <linux/debugfs.h>
 #include <linux/seq_file.h>
 #include <linux/io.h>
-
-#include <asm/pmc_atom.h>
+#include <linux/platform_data/x86/pmc_atom.h>
 
 struct pmc_bit_map {
 	const char *name;
diff --git a/arch/x86/include/asm/pmc_atom.h b/include/linux/platform_data/x86/pmc_atom.h
similarity index 100%
rename from arch/x86/include/asm/pmc_atom.h
rename to include/linux/platform_data/x86/pmc_atom.h
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1540742 — Re: [PATCH v6 2/3] arch/x86/platform/atom: Move pmc_atom to drivers/platform/x86

I have been told I have to send my comments here instead of our
internal ML. I didn't fast enough to comment that during v5. So do it
right now.

On Fri, Dec 9, 2016 at 8:01 PM, Irina Tirdea <irina.tirdea@intel.com> wrote:
> The pmc_atom driver does not contain any architecture specific
> code. It only enables the SOC Power Management Controller Driver

SOC -> SoC
Driver -> driver

> for BayTrail and CherryTrail platforms.
>
> Move the pmc_atom driver from arch/x86/platform/atom to
> drivers/platform/x86.
>
> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> ---
>  arch/x86/Kconfig                                                     | 4 ----
>  arch/x86/platform/atom/Makefile                                      | 1 -
>  drivers/acpi/acpi_lpss.c                                             | 2 +-
>  drivers/platform/x86/Kconfig                                         | 4 ++++
>  drivers/platform/x86/Makefile                                        | 1 +
>  {arch/x86/platform/atom => drivers/platform/x86}/pmc_atom.c          | 3 +--

>  {arch/x86/include/asm => include/linux/platform_data/x86}/pmc_atom.h | 0

No, it's not a *platform data*.

Other that that looks good to me.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1543660 — Re: [PATCH v6 2/3] arch/x86/platform/atom: Move pmc_atom to drivers/platform/x86

On Tue, Dec 13, 2016 at 01:43:31AM +0200, Andy Shevchenko wrote:
> I have been told I have to send my comments here instead of our
> internal ML. I didn't fast enough to comment that during v5. So do it
> right now.
> 
> On Fri, Dec 9, 2016 at 8:01 PM, Irina Tirdea <irina.tirdea@intel.com> wrote:
> > The pmc_atom driver does not contain any architecture specific
> > code. It only enables the SOC Power Management Controller Driver
> 
> SOC -> SoC
> Driver -> driver
> 
> > for BayTrail and CherryTrail platforms.
> >
> > Move the pmc_atom driver from arch/x86/platform/atom to
> > drivers/platform/x86.
> >
> > Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> > ---
> >  arch/x86/Kconfig                                                     | 4 ----
> >  arch/x86/platform/atom/Makefile                                      | 1 -
> >  drivers/acpi/acpi_lpss.c                                             | 2 +-
> >  drivers/platform/x86/Kconfig                                         | 4 ++++
> >  drivers/platform/x86/Makefile                                        | 1 +
> >  {arch/x86/platform/atom => drivers/platform/x86}/pmc_atom.c          | 3 +--
> 
> >  {arch/x86/include/asm => include/linux/platform_data/x86}/pmc_atom.h | 0
> 
> No, it's not a *platform data*.
> 
> Other that that looks good to me.

Where would you recommend instead? It needs a place to serve both acpi_lpss.c
and pmc_atom.c. The include/linux/platform_data/x86 location doesn't seem too
strange as it supports a "platform" driver in the sense that pmc_atom is a
platform driver.

-- 
Darren Hart
Intel Open Source Technology Center

[toc] | [prev] | [next] | [standalone]

#1543672 — Re: [PATCH v6 2/3] arch/x86/platform/atom: Move pmc_atom to drivers/platform/x86

On Fri, Dec 16, 2016 at 8:20 PM, Darren Hart <dvhart@infradead.org> wrote:
> On Tue, Dec 13, 2016 at 01:43:31AM +0200, Andy Shevchenko wrote:
>> I have been told I have to send my comments here instead of our
>> internal ML. I didn't fast enough to comment that during v5. So do it
>> right now.

>> >  {arch/x86/include/asm => include/linux/platform_data/x86}/pmc_atom.h | 0
>>
>> No, it's not a *platform data*.
>>
>> Other that that looks good to me.
>
> Where would you recommend instead? It needs a place to serve both acpi_lpss.c
> and pmc_atom.c. The include/linux/platform_data/x86 location doesn't seem too
> strange as it supports a "platform" driver in the sense that pmc_atom is a
> platform driver.

include/linux/x86 ?

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1539588 — [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks

The BayTrail and CherryTrail platforms provide platform clocks
through their Power Management Controller (PMC).

The SoC supports up to 6 clocks (PMC_PLT_CLK[5:0]) with a
frequency of either 19.2 MHz (PLL) or 25 MHz (XTAL) for BayTrail
and a frequency of 19.2 MHz (XTAL) for CherryTrail. These clocks
are available for general system use, where appropriate, and each
have Control & Frequency register fields associated with them.

Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/clk/x86/Makefile                      |   3 +
 drivers/clk/x86/clk-byt-plt.c                 | 380 ++++++++++++++++++++++++++
 include/linux/platform_data/x86/clk-byt-plt.h |  31 +++
 3 files changed, 414 insertions(+)
 create mode 100644 drivers/clk/x86/clk-byt-plt.c
 create mode 100644 include/linux/platform_data/x86/clk-byt-plt.h

diff --git a/drivers/clk/x86/Makefile b/drivers/clk/x86/Makefile
index 0478138..bf7c132 100644
--- 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)
+obj-$(CONFIG_PMC_ATOM)		+= clk-byt-plt.o
+endif
diff --git a/drivers/clk/x86/clk-byt-plt.c b/drivers/clk/x86/clk-byt-plt.c
new file mode 100644
index 0000000..2303e0d
--- /dev/null
+++ b/drivers/clk/x86/clk-byt-plt.c
@@ -0,0 +1,380 @@
+/*
+ * Intel Atom platform clocks driver for BayTrail and CherryTrail SoC.
+ *
+ * Copyright (C) 2016, Intel Corporation
+ * Author: Irina Tirdea <irina.tirdea@intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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 <linux/clk-provider.h>
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/clkdev.h>
+#include <linux/platform_data/x86/clk-byt-plt.h>
+
+#define PLT_CLK_NAME_BASE	"pmc_plt_clk_"
+#define PLT_CLK_DRIVER_NAME	"clk-byt-plt"
+
+#define PMC_CLK_CTL_SIZE	4
+#define PMC_CLK_NUM		6
+#define PMC_MASK_CLK_CTL	GENMASK(1, 0)
+#define PMC_MASK_CLK_FREQ	BIT(2)
+#define PMC_CLK_CTL_GATED_ON_D3	0x0
+#define PMC_CLK_CTL_FORCE_ON	0x1
+#define PMC_CLK_CTL_FORCE_OFF	0x2
+#define PMC_CLK_CTL_RESERVED	0x3
+#define PMC_CLK_FREQ_XTAL	0x0	/* 25 MHz */
+#define PMC_CLK_FREQ_PLL	0x4	/* 19.2 MHz */
+
+struct clk_plt_fixed {
+	struct clk_hw *clk;
+	struct clk_lookup *lookup;
+};
+
+struct clk_plt {
+	struct clk_hw hw;
+	void __iomem *reg;
+	struct clk_lookup *lookup;
+	spinlock_t lock;
+};
+
+#define to_clk_plt(_hw) container_of(_hw, struct clk_plt, hw)
+
+struct clk_plt_data {
+	struct clk_plt_fixed **parents;
+	u8 nparents;
+	struct clk_plt *clks[PMC_CLK_NUM];
+};
+
+static inline int plt_reg_to_parent(int reg)
+{
+	switch (reg & PMC_MASK_CLK_FREQ) {
+	case PMC_CLK_FREQ_XTAL:
+		return 0;	/* index 0 in parents[] */
+	case PMC_CLK_FREQ_PLL:
+		return 1;	/* index 1 in parents[] */
+	}
+
+	return 0;
+}
+
+static inline int plt_parent_to_reg(int index)
+{
+	switch (index) {
+	case 0:	/* index 0 in parents[] */
+		return PMC_CLK_FREQ_XTAL;
+	case 1:	/* index 0 in parents[] */
+		return PMC_CLK_FREQ_PLL;
+	}
+
+	return PMC_CLK_FREQ_XTAL;
+}
+
+static inline int plt_reg_to_enabled(int reg)
+{
+	switch (reg & PMC_MASK_CLK_CTL) {
+	case PMC_CLK_CTL_GATED_ON_D3:
+	case PMC_CLK_CTL_FORCE_ON:
+		return 1;	/* enabled */
+	case PMC_CLK_CTL_FORCE_OFF:
+	case PMC_CLK_CTL_RESERVED:
+	default:
+		return 0;	/* disabled */
+	}
+}
+
+static void plt_clk_reg_update(struct clk_plt *clk, u32 mask, u32 val)
+{
+	u32 orig, tmp;
+	unsigned long flags = 0;
+
+	spin_lock_irqsave(&clk->lock, flags);
+
+	orig = readl(clk->reg);
+
+	tmp = orig & ~mask;
+	tmp |= val & mask;
+
+	if (tmp != orig)
+		writel(tmp, clk->reg);
+
+	spin_unlock_irqrestore(&clk->lock, flags);
+}
+
+static int plt_clk_set_parent(struct clk_hw *hw, u8 index)
+{
+	struct clk_plt *clk = to_clk_plt(hw);
+
+	plt_clk_reg_update(clk, PMC_MASK_CLK_FREQ, plt_parent_to_reg(index));
+
+	return 0;
+}
+
+static u8 plt_clk_get_parent(struct clk_hw *hw)
+{
+	struct clk_plt *clk = to_clk_plt(hw);
+	u32 value;
+
+	value = readl(clk->reg);
+
+	return plt_reg_to_parent(value);
+}
+
+static int plt_clk_enable(struct clk_hw *hw)
+{
+	struct clk_plt *clk = to_clk_plt(hw);
+
+	plt_clk_reg_update(clk, PMC_MASK_CLK_CTL, PMC_CLK_CTL_FORCE_ON);
+
+	return 0;
+}
+
+static void plt_clk_disable(struct clk_hw *hw)
+{
+	struct clk_plt *clk = to_clk_plt(hw);
+
+	plt_clk_reg_update(clk, PMC_MASK_CLK_CTL, PMC_CLK_CTL_FORCE_OFF);
+}
+
+static int plt_clk_is_enabled(struct clk_hw *hw)
+{
+	struct clk_plt *clk = to_clk_plt(hw);
+	u32 value;
+
+	value = readl(clk->reg);
+
+	return plt_reg_to_enabled(value);
+}
+
+static const struct clk_ops plt_clk_ops = {
+	.enable = plt_clk_enable,
+	.disable = plt_clk_disable,
+	.is_enabled = plt_clk_is_enabled,
+	.get_parent = plt_clk_get_parent,
+	.set_parent = plt_clk_set_parent,
+	.determine_rate = __clk_mux_determine_rate,
+};
+
+static struct clk_plt *plt_clk_register(struct platform_device *pdev, int id,
+					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);
+	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);
+
+	return pclk;
+
+err_free_init:
+	kfree(init.name);
+	return ERR_PTR(ret);
+}
+
+static void plt_clk_unregister(struct clk_plt *pclk)
+{
+	clkdev_drop(pclk->lookup);
+}
+
+static struct clk_plt_fixed *plt_clk_register_fixed_rate(struct platform_device *pdev,
+						 const char *name,
+						 const char *parent_name,
+						 unsigned long fixed_rate)
+{
+	struct clk_plt_fixed *pclk;
+	int ret = 0;
+
+	pclk = devm_kzalloc(&pdev->dev, sizeof(*pclk), GFP_KERNEL);
+	if (!pclk)
+		return ERR_PTR(-ENOMEM);
+
+	pclk->clk = clk_hw_register_fixed_rate(&pdev->dev, name, parent_name,
+					       0, fixed_rate);
+	if (IS_ERR(pclk->clk))
+		return ERR_CAST(pclk->clk);
+
+	pclk->lookup = clkdev_hw_create(pclk->clk, name, NULL);
+	if (!pclk->lookup) {
+		ret = -ENOMEM;
+		goto err_clk_unregister;
+	}
+
+	return pclk;
+
+err_clk_unregister:
+	clk_hw_unregister_fixed_rate(pclk->clk);
+	return ERR_PTR(ret);
+}
+
+static void plt_clk_unregister_fixed_rate(struct clk_plt_fixed *pclk)
+{
+	clkdev_drop(pclk->lookup);
+	clk_hw_unregister_fixed_rate(pclk->clk);
+}
+
+static const char **plt_clk_register_parents(struct platform_device *pdev,
+					     struct clk_plt_data *data,
+					     const struct pmc_clk *clks)
+{
+	const char **parent_names;
+	int i, err;
+
+	data->nparents = 0;
+	while (clks[data->nparents].name)
+		data->nparents++;
+
+	data->parents = devm_kcalloc(&pdev->dev, data->nparents,
+				     sizeof(*data->parents), GFP_KERNEL);
+	if (!data->parents) {
+		err = -ENOMEM;
+		goto err_out;
+	}
+
+	parent_names = kcalloc(data->nparents, sizeof(*parent_names),
+			       GFP_KERNEL);
+	if (!parent_names) {
+		err = -ENOMEM;
+		goto err_out;
+	}
+
+	for (i = 0; i < data->nparents; i++) {
+		data->parents[i] =
+			plt_clk_register_fixed_rate(pdev, clks[i].name,
+						    clks[i].parent_name,
+						    clks[i].freq);
+		if (IS_ERR(data->parents[i])) {
+			err = PTR_ERR(data->parents[i]);
+			goto err_unreg;
+		}
+		parent_names[i] = kstrdup_const(clks[i].name, GFP_KERNEL);
+	}
+
+	return parent_names;
+
+err_unreg:
+	for (i--; i >= 0; i--) {
+		plt_clk_unregister_fixed_rate(data->parents[i]);
+		kfree_const(parent_names[i]);
+	}
+	kfree(parent_names);
+err_out:
+	data->nparents = 0;
+	return ERR_PTR(err);
+}
+
+static void plt_clk_unregister_parents(struct clk_plt_data *data)
+{
+	int i;
+
+	for (i = 0; i < data->nparents; i++)
+		plt_clk_unregister_fixed_rate(data->parents[i]);
+}
+
+static int plt_clk_probe(struct platform_device *pdev)
+{
+	struct clk_plt_data *data;
+	int i, err;
+	const char **parent_names;
+	const struct pmc_clk_data *pmc_data;
+
+	pmc_data = dev_get_platdata(&pdev->dev);
+	if (!pmc_data || !pmc_data->clks)
+		return -EINVAL;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	parent_names = plt_clk_register_parents(pdev, data, pmc_data->clks);
+	if (IS_ERR(parent_names))
+		return PTR_ERR(parent_names);
+
+	for (i = 0; i < PMC_CLK_NUM; i++) {
+		data->clks[i] = plt_clk_register(pdev, i, pmc_data->base,
+						 parent_names, data->nparents);
+		if (IS_ERR(data->clks[i])) {
+			err = PTR_ERR(data->clks[i]);
+			goto err_unreg_clk_plt;
+		}
+	}
+
+	for (i = 0; i < data->nparents; i++)
+		kfree_const(parent_names[i]);
+	kfree(parent_names);
+
+	dev_set_drvdata(&pdev->dev, data);
+	return 0;
+
+err_unreg_clk_plt:
+	for (i--; i >= 0; i--)
+		plt_clk_unregister(data->clks[i]);
+	plt_clk_unregister_parents(data);
+	for (i = 0; i < data->nparents; i++)
+		kfree_const(parent_names[i]);
+	kfree(parent_names);
+	return err;
+}
+
+static int plt_clk_remove(struct platform_device *pdev)
+{
+	struct clk_plt_data *data;
+	int i;
+
+	data = dev_get_drvdata(&pdev->dev);
+	if (!data)
+		return 0;
+
+	for (i = 0; i < PMC_CLK_NUM; i++)
+		plt_clk_unregister(data->clks[i]);
+	plt_clk_unregister_parents(data);
+	return 0;
+}
+
+static struct platform_driver plt_clk_driver = {
+	.driver = {
+		.name = PLT_CLK_DRIVER_NAME,
+	},
+	.probe = plt_clk_probe,
+	.remove = plt_clk_remove,
+};
+module_platform_driver(plt_clk_driver);
+
+MODULE_DESCRIPTION("Intel Atom platform clocks driver");
+MODULE_AUTHOR("Irina Tirdea <irina.tirdea@intel.com>");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/platform_data/x86/clk-byt-plt.h b/include/linux/platform_data/x86/clk-byt-plt.h
new file mode 100644
index 0000000..e6bca9c
--- /dev/null
+++ b/include/linux/platform_data/x86/clk-byt-plt.h
@@ -0,0 +1,31 @@
+/*
+ * Intel Atom platform clocks for BayTrail and CherryTrail SoC.
+ *
+ * Copyright (C) 2016, Intel Corporation
+ * Author: Irina Tirdea <irina.tirdea@intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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.
+ */
+
+#ifndef __CLK_BYT_PLT_H
+#define __CLK_BYT_PLT_H
+
+struct pmc_clk {
+	const char *name;
+	unsigned long freq;
+	const char *parent_name;
+};
+
+struct pmc_clk_data {
+	void __iomem *base;
+	const struct pmc_clk *clks;
+};
+
+#endif /* __CLK_BYT_PLT_H */
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1540741 — Re: [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks

On Fri, Dec 9, 2016 at 8:01 PM, Irina Tirdea <irina.tirdea@intel.com> wrote:

Thanks for an update I will comment all the patches.
Here we start.

> The BayTrail and CherryTrail platforms provide platform clocks
> through their Power Management Controller (PMC).
>
> The SoC supports up to 6 clocks (PMC_PLT_CLK[5:0]) with a
> frequency of either 19.2 MHz (PLL) or 25 MHz (XTAL) for BayTrail
> and a frequency of 19.2 MHz (XTAL) for CherryTrail. These clocks
> are available for general system use, where appropriate, and each
> have Control & Frequency register fields associated with them.
>
> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Who is the actual author? SoB I guess should be either the author, or
1st, 2nd, ..., last one who is submitter.

> --- 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.

> +obj-$(CONFIG_PMC_ATOM)         += clk-byt-plt.o

I'm pretty sure X86_INTEL_LPSS almost replicates what you need (it
also includes Haswell support, but it doesn't matter here).

Can we unify them or is it a bad idea?

Otherwise I would propose to rename module to be something like
clk-x86-pmc.o which follows above pattern: LPSS as provider, PMC as
provider and so on.

Maybe
clk-x86-pmc-objs              := clk-pmc-atom.o
...

By the way lpt is a not good chosen abbreviation for Lynxpoint. I even
had a patch to get rid of this file completely.

> +endif
> diff --git a/drivers/clk/x86/clk-byt-plt.c b/drivers/clk/x86/clk-byt-plt.c
> new file mode 100644
> index 0000000..2303e0d
> --- /dev/null
> +++ b/drivers/clk/x86/clk-byt-plt.c

> @@ -0,0 +1,380 @@
> +/*
> + * Intel Atom platform clocks driver for BayTrail and CherryTrail SoC.

SoCs.

> + *
> + * Copyright (C) 2016, Intel Corporation
> + * Author: Irina Tirdea <irina.tirdea@intel.com>

Be consistent with SoB lines above.

> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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 <linux/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/clkdev.h>

> +#include <linux/platform_data/x86/clk-byt-plt.h>

Is it indeed platform data? I would not create platform_data/x86
without strong argument.
Perhaps include/linux/clk/x86_pmc.h? (Yes, I know about clk-lpss.h
which is old enough and was basically first try of clk stuff on x86)

> +
> +#define PLT_CLK_NAME_BASE      "pmc_plt_clk_"

> +#define PLT_CLK_DRIVER_NAME    "clk-byt-plt"

By default you may use build name of the module, but if you want to
stick with something choose the name I mentioned above like
clk-pmc-atom.

>
> +#define PMC_CLK_CTL_SIZE       4
> +#define PMC_CLK_NUM            6
> +#define PMC_MASK_CLK_CTL       GENMASK(1, 0)
> +#define PMC_MASK_CLK_FREQ      BIT(2)
> +#define PMC_CLK_CTL_GATED_ON_D3        0x0
> +#define PMC_CLK_CTL_FORCE_ON   0x1
> +#define PMC_CLK_CTL_FORCE_OFF  0x2
> +#define PMC_CLK_CTL_RESERVED   0x3

> +#define PMC_CLK_FREQ_XTAL      0x0     /* 25 MHz */
> +#define PMC_CLK_FREQ_PLL       0x4     /* 19.2 MHz */

Looks like (0 << 2) and (1 << 2). I would put that way to show that
it's bitwise value.

> +
> +struct clk_plt_fixed {

Yeah, rename names accordingly.

> +       struct clk_hw *clk;
> +       struct clk_lookup *lookup;
> +};
> +
> +struct clk_plt {
> +       struct clk_hw hw;
> +       void __iomem *reg;
> +       struct clk_lookup *lookup;

> +       spinlock_t lock;

Would be nice to have a comment what is/are protected by it.

> +};
> +
> +#define to_clk_plt(_hw) container_of(_hw, struct clk_plt, hw)
> +
> +struct clk_plt_data {
> +       struct clk_plt_fixed **parents;
> +       u8 nparents;
> +       struct clk_plt *clks[PMC_CLK_NUM];
> +};
> +
> +static inline int plt_reg_to_parent(int reg)
> +{
> +       switch (reg & PMC_MASK_CLK_FREQ) {

+ default: (see below) ?

> +       case PMC_CLK_FREQ_XTAL:
> +               return 0;       /* index 0 in parents[] */
> +       case PMC_CLK_FREQ_PLL:
> +               return 1;       /* index 1 in parents[] */
> +       }
> +

> +       return 0;

default: ?

> +}
> +
> +static inline int plt_parent_to_reg(int index)
> +{
> +       switch (index) {
> +       case 0: /* index 0 in parents[] */
> +               return PMC_CLK_FREQ_XTAL;
> +       case 1: /* index 0 in parents[] */
> +               return PMC_CLK_FREQ_PLL;
> +       }
> +
> +       return PMC_CLK_FREQ_XTAL;

Ditto.

> +}
> +
> +static inline int plt_reg_to_enabled(int reg)
> +{
> +       switch (reg & PMC_MASK_CLK_CTL) {
> +       case PMC_CLK_CTL_GATED_ON_D3:
> +       case PMC_CLK_CTL_FORCE_ON:
> +               return 1;       /* enabled */
> +       case PMC_CLK_CTL_FORCE_OFF:
> +       case PMC_CLK_CTL_RESERVED:
> +       default:
> +               return 0;       /* disabled */
> +       }
> +}
> +
> +static void plt_clk_reg_update(struct clk_plt *clk, u32 mask, u32 val)
> +{
> +       u32 orig, tmp;
> +       unsigned long flags = 0;

Redundant assignment.

> +
> +       spin_lock_irqsave(&clk->lock, flags);
> +
> +       orig = readl(clk->reg);
> +
> +       tmp = orig & ~mask;
> +       tmp |= val & mask;
> +

> +       if (tmp != orig)

Hmm...Is here any benefit? Do we do this 1000s times per ...s? OTOH
readability a bit better w/o it.

> +               writel(tmp, clk->reg);
> +
> +       spin_unlock_irqrestore(&clk->lock, flags);
> +}
> +
> +static int plt_clk_set_parent(struct clk_hw *hw, u8 index)
> +{
> +       struct clk_plt *clk = to_clk_plt(hw);
> +
> +       plt_clk_reg_update(clk, PMC_MASK_CLK_FREQ, plt_parent_to_reg(index));
> +
> +       return 0;
> +}
> +
> +static u8 plt_clk_get_parent(struct clk_hw *hw)
> +{
> +       struct clk_plt *clk = to_clk_plt(hw);
> +       u32 value;
> +
> +       value = readl(clk->reg);
> +
> +       return plt_reg_to_parent(value);
> +}
> +
> +static int plt_clk_enable(struct clk_hw *hw)
> +{
> +       struct clk_plt *clk = to_clk_plt(hw);
> +
> +       plt_clk_reg_update(clk, PMC_MASK_CLK_CTL, PMC_CLK_CTL_FORCE_ON);
> +
> +       return 0;
> +}
> +
> +static void plt_clk_disable(struct clk_hw *hw)
> +{
> +       struct clk_plt *clk = to_clk_plt(hw);
> +
> +       plt_clk_reg_update(clk, PMC_MASK_CLK_CTL, PMC_CLK_CTL_FORCE_OFF);
> +}
> +
> +static int plt_clk_is_enabled(struct clk_hw *hw)
> +{
> +       struct clk_plt *clk = to_clk_plt(hw);
> +       u32 value;
> +
> +       value = readl(clk->reg);
> +
> +       return plt_reg_to_enabled(value);
> +}
> +
> +static const struct clk_ops plt_clk_ops = {
> +       .enable = plt_clk_enable,
> +       .disable = plt_clk_disable,
> +       .is_enabled = plt_clk_is_enabled,
> +       .get_parent = plt_clk_get_parent,
> +       .set_parent = plt_clk_set_parent,
> +       .determine_rate = __clk_mux_determine_rate,
> +};
> +
> +static struct clk_plt *plt_clk_register(struct platform_device *pdev, int id,

I don't see how pdev is involved, perhaps just struct device *dev here.

> +                                       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()

> +       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();

> +
> +       return pclk;

> +
> +err_free_init:
> +       kfree(init.name);
> +       return ERR_PTR(ret);

Might be redundant, see above.

> +}
> +
> +static void plt_clk_unregister(struct clk_plt *pclk)
> +{
> +       clkdev_drop(pclk->lookup);
> +}
> +
> +static struct clk_plt_fixed *plt_clk_register_fixed_rate(struct platform_device *pdev,
> +                                                const char *name,
> +                                                const char *parent_name,
> +                                                unsigned long fixed_rate)
> +{
> +       struct clk_plt_fixed *pclk;
> +       int ret = 0;

Useless assignment.

> +
> +       pclk = devm_kzalloc(&pdev->dev, sizeof(*pclk), GFP_KERNEL);
> +       if (!pclk)
> +               return ERR_PTR(-ENOMEM);
> +
> +       pclk->clk = clk_hw_register_fixed_rate(&pdev->dev, name, parent_name,
> +                                              0, fixed_rate);
> +       if (IS_ERR(pclk->clk))
> +               return ERR_CAST(pclk->clk);
> +
> +       pclk->lookup = clkdev_hw_create(pclk->clk, name, NULL);
> +       if (!pclk->lookup) {
> +               ret = -ENOMEM;
> +               goto err_clk_unregister;
> +       }
> +
> +       return pclk;
> +
> +err_clk_unregister:
> +       clk_hw_unregister_fixed_rate(pclk->clk);
> +       return ERR_PTR(ret);
> +}
> +
> +static void plt_clk_unregister_fixed_rate(struct clk_plt_fixed *pclk)
> +{
> +       clkdev_drop(pclk->lookup);
> +       clk_hw_unregister_fixed_rate(pclk->clk);
> +}
> +
> +static const char **plt_clk_register_parents(struct platform_device *pdev,
> +                                            struct clk_plt_data *data,
> +                                            const struct pmc_clk *clks)
> +{
> +       const char **parent_names;
> +       int i, err;
> +
> +       data->nparents = 0;
> +       while (clks[data->nparents].name)
> +               data->nparents++;
> +
> +       data->parents = devm_kcalloc(&pdev->dev, data->nparents,
> +                                    sizeof(*data->parents), GFP_KERNEL);
> +       if (!data->parents) {
> +               err = -ENOMEM;
> +               goto err_out;
> +       }
> +
> +       parent_names = kcalloc(data->nparents, sizeof(*parent_names),
> +                              GFP_KERNEL);
> +       if (!parent_names) {
> +               err = -ENOMEM;
> +               goto err_out;
> +       }
> +
> +       for (i = 0; i < data->nparents; i++) {
> +               data->parents[i] =
> +                       plt_clk_register_fixed_rate(pdev, clks[i].name,
> +                                                   clks[i].parent_name,
> +                                                   clks[i].freq);
> +               if (IS_ERR(data->parents[i])) {
> +                       err = PTR_ERR(data->parents[i]);
> +                       goto err_unreg;
> +               }
> +               parent_names[i] = kstrdup_const(clks[i].name, GFP_KERNEL);
> +       }
> +
> +       return parent_names;
> +
> +err_unreg:
> +       for (i--; i >= 0; i--) {

while (i--) {
}

> +               plt_clk_unregister_fixed_rate(data->parents[i]);
> +               kfree_const(parent_names[i]);
> +       }
> +       kfree(parent_names);

> +err_out:
> +       data->nparents = 0;

You will not need this if you define local variable for nparents and
assign data->nparents at last in the function.

> +       return ERR_PTR(err);
> +}
> +
> +static void plt_clk_unregister_parents(struct clk_plt_data *data)
> +{
> +       int i;
> +

> +       for (i = 0; i < data->nparents; i++)
> +               plt_clk_unregister_fixed_rate(data->parents[i]);



> +}
> +
> +static int plt_clk_probe(struct platform_device *pdev)
> +{
> +       struct clk_plt_data *data;
> +       int i, err;
> +       const char **parent_names;
> +       const struct pmc_clk_data *pmc_data;

Reversed order, please. Usually: assignments at very beginning, longer
first, short later, error code variable last, flags for spin lock --
depends.

> +
> +       pmc_data = dev_get_platdata(&pdev->dev);

> +       if (!pmc_data || !pmc_data->clks)
> +               return -EINVAL;
> +
> +       data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       parent_names = plt_clk_register_parents(pdev, data, pmc_data->clks);
> +       if (IS_ERR(parent_names))
> +               return PTR_ERR(parent_names);
> +
> +       for (i = 0; i < PMC_CLK_NUM; i++) {
> +               data->clks[i] = plt_clk_register(pdev, i, pmc_data->base,
> +                                                parent_names, data->nparents);
> +               if (IS_ERR(data->clks[i])) {
> +                       err = PTR_ERR(data->clks[i]);
> +                       goto err_unreg_clk_plt;
> +               }
> +       }
> +

> +       for (i = 0; i < data->nparents; i++)
> +               kfree_const(parent_names[i]);
> +       kfree(parent_names);

(1)

> +
> +       dev_set_drvdata(&pdev->dev, data);
> +       return 0;
> +
> +err_unreg_clk_plt:

> +       for (i--; i >= 0; i--)
> +               plt_clk_unregister(data->clks[i]);
> +       plt_clk_unregister_parents(data);

(3)


> +       for (i = 0; i < data->nparents; i++)
> +               kfree_const(parent_names[i]);
> +       kfree(parent_names);

(2)

(1) and (2) -> helper function?

> +       return err;
> +}
> +
> +static int plt_clk_remove(struct platform_device *pdev)
> +{
> +       struct clk_plt_data *data;
> +       int i;
> +
> +       data = dev_get_drvdata(&pdev->dev);
> +       if (!data)
> +               return 0;
> +
> +       for (i = 0; i < PMC_CLK_NUM; i++)
> +               plt_clk_unregister(data->clks[i]);
> +       plt_clk_unregister_parents(data);

(4)

(3) and (4) -> helper function ?

> +       return 0;
> +}
> +
> +static struct platform_driver plt_clk_driver = {
> +       .driver = {
> +               .name = PLT_CLK_DRIVER_NAME,

Better to put such inplace here. You know why? Instead of one git grep
one has to run two in order to find actual driver name.

> +       },
> +       .probe = plt_clk_probe,
> +       .remove = plt_clk_remove,
> +};
> +module_platform_driver(plt_clk_driver);
> +
> +MODULE_DESCRIPTION("Intel Atom platform clocks driver");

> +MODULE_AUTHOR("Irina Tirdea <irina.tirdea@intel.com>");

Be consistent with SoB lines

> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/platform_data/x86/clk-byt-plt.h b/include/linux/platform_data/x86/clk-byt-plt.h
> new file mode 100644
> index 0000000..e6bca9c
> --- /dev/null
> +++ b/include/linux/platform_data/x86/clk-byt-plt.h
> @@ -0,0 +1,31 @@
> +/*
> + * Intel Atom platform clocks for BayTrail and CherryTrail SoC.
> + *
> + * Copyright (C) 2016, Intel Corporation
> + * Author: Irina Tirdea <irina.tirdea@intel.com>

Ditto.
Of course in all cases exceptions are possible (if another author has
done partial stuff)

> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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.
> + */
> +
> +#ifndef __CLK_BYT_PLT_H
> +#define __CLK_BYT_PLT_H
> +
> +struct pmc_clk {
> +       const char *name;
> +       unsigned long freq;
> +       const char *parent_name;
> +};
> +
> +struct pmc_clk_data {
> +       void __iomem *base;
> +       const struct pmc_clk *clks;
> +};

Those are definitely do not look like a *platform data* at all.

> +
> +#endif /* __CLK_BYT_PLT_H */


-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1540757 — Re: [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks

> Thanks for an update I will comment all the patches.
> Here we start.

Thanks Andy for the review. Two quick comments before going further in 
the details later.

>
>> The BayTrail and CherryTrail platforms provide platform clocks
>> through their Power Management Controller (PMC).
>>
>> The SoC supports up to 6 clocks (PMC_PLT_CLK[5:0]) with a
>> frequency of either 19.2 MHz (PLL) or 25 MHz (XTAL) for BayTrail
>> and a frequency of 19.2 MHz (XTAL) for CherryTrail. These clocks
>> are available for general system use, where appropriate, and each
>> have Control & Frequency register fields associated with them.
>>
>> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>
> Who is the actual author? SoB I guess should be either the author, or
> 1st, 2nd, ..., last one who is submitter.

I ported the initial code from Android legacy stuff and Irina ported the 
functionality to the clk framework. It seems appropriate to have both 
signed-offs?

[snip]
>
>> +#include <linux/platform_data/x86/clk-byt-plt.h>

This was a suggestion of Darren Hart in agreement with Thomas Gleixner.
see 
http://mailman.alsa-project.org/pipermail/alsa-devel/2016-October/113936.html

Darren, did we get your proposal right?

>
> Is it indeed platform data? I would not create platform_data/x86
> without strong argument.
> Perhaps include/linux/clk/x86_pmc.h? (Yes, I know about clk-lpss.h
> which is old enough and was basically first try of clk stuff on x86)

[toc] | [prev] | [next] | [standalone]

#1540761 — Re: [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks

On Tue, Dec 13, 2016 at 2:15 AM, Pierre-Louis Bossart
<pierre-louis.bossart@linux.intel.com> wrote:
>
>> Thanks for an update I will comment all the patches.
>> Here we start.
>
>
> Thanks Andy for the review. Two quick comments before going further in the
> details later.
>
>>
>>> The BayTrail and CherryTrail platforms provide platform clocks
>>> through their Power Management Controller (PMC).
>>>
>>> The SoC supports up to 6 clocks (PMC_PLT_CLK[5:0]) with a
>>> frequency of either 19.2 MHz (PLL) or 25 MHz (XTAL) for BayTrail
>>> and a frequency of 19.2 MHz (XTAL) for CherryTrail. These clocks
>>> are available for general system use, where appropriate, and each
>>> have Control & Frequency register fields associated with them.
>>>
>>> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
>>> Signed-off-by: Pierre-Louis Bossart
>>> <pierre-louis.bossart@linux.intel.com>
>>
>>
>> Who is the actual author? SoB I guess should be either the author, or
>> 1st, 2nd, ..., last one who is submitter.
>
>
> I ported the initial code from Android legacy stuff and Irina ported the
> functionality to the clk framework. It seems appropriate to have both
> signed-offs?

Yes, but as I mentioned:
1) submitter goes last;
2) SoB lines and Author(s) should reflect actual state of the sources.
If patch has 2 SoBs I'm expecting see different names of Authors in
the source code. *Or* in some cases it's possible to explain in the
commit message why you have former SoB and for what the credit that
person(s) get.

>>> +#include <linux/platform_data/x86/clk-byt-plt.h>
>
>
> This was a suggestion of Darren Hart in agreement with Thomas Gleixner.
> see
> http://mailman.alsa-project.org/pipermail/alsa-devel/2016-October/113936.html

Hmm... Thanks for pointing to this I didn't aware about such details.

But... I still insist that is not a platform data at all in both cases.

For clock I would suggest include/linux/clk/ with x86_ prefix.
For the rest I have no strong opinion except trying to avoid
platform_data wording in the path as much as possible.

As an example I could recall DMA engine subsystem where we have

include/linux/platform_data/dma-*.h

and

include/linux/dma/*.h

So, this sounds more to me as

include/linux/x86/pmc_atom.h

> Darren, did we get your proposal right?

>>
>> Is it indeed platform data? I would not create platform_data/x86
>> without strong argument.
>> Perhaps include/linux/clk/x86_pmc.h? (Yes, I know about clk-lpss.h
>> which is old enough and was basically first try of clk stuff on x86)

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1543676 — Re: [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks

On Tue, Dec 13, 2016 at 02:26:21AM +0200, Andy Shevchenko wrote:
> On Tue, Dec 13, 2016 at 2:15 AM, Pierre-Louis Bossart
> <pierre-louis.bossart@linux.intel.com> wrote:
> >
> >> Thanks for an update I will comment all the patches.
> >> Here we start.
> >
> >
> > Thanks Andy for the review. Two quick comments before going further in the
> > details later.
> >
> >>
> >>> The BayTrail and CherryTrail platforms provide platform clocks
> >>> through their Power Management Controller (PMC).
> >>>
> >>> The SoC supports up to 6 clocks (PMC_PLT_CLK[5:0]) with a
> >>> frequency of either 19.2 MHz (PLL) or 25 MHz (XTAL) for BayTrail
> >>> and a frequency of 19.2 MHz (XTAL) for CherryTrail. These clocks
> >>> are available for general system use, where appropriate, and each
> >>> have Control & Frequency register fields associated with them.
> >>>
> >>> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> >>> Signed-off-by: Pierre-Louis Bossart
> >>> <pierre-louis.bossart@linux.intel.com>
> >>
> >>
> >> Who is the actual author? SoB I guess should be either the author, or
> >> 1st, 2nd, ..., last one who is submitter.
> >
> >
> > I ported the initial code from Android legacy stuff and Irina ported the
> > functionality to the clk framework. It seems appropriate to have both
> > signed-offs?
> 
> Yes, but as I mentioned:
> 1) submitter goes last;
> 2) SoB lines and Author(s) should reflect actual state of the sources.
> If patch has 2 SoBs I'm expecting see different names of Authors in
> the source code. *Or* in some cases it's possible to explain in the
> commit message why you have former SoB and for what the credit that
> person(s) get.
> 
> >>> +#include <linux/platform_data/x86/clk-byt-plt.h>
> >
> >
> > This was a suggestion of Darren Hart in agreement with Thomas Gleixner.
> > see
> > http://mailman.alsa-project.org/pipermail/alsa-devel/2016-October/113936.html
> 
> Hmm... Thanks for pointing to this I didn't aware about such details.
> 
> But... I still insist that is not a platform data at all in both cases.
> 
> For clock I would suggest include/linux/clk/ with x86_ prefix.
> For the rest I have no strong opinion except trying to avoid
> platform_data wording in the path as much as possible.
> 
> As an example I could recall DMA engine subsystem where we have
> 
> include/linux/platform_data/dma-*.h
> 
> and
> 
> include/linux/dma/*.h
> 
> So, this sounds more to me as
> 
> include/linux/x86/pmc_atom.h

There should really be some Documentation about how to choose an include
directory :-)

My understanding is include/linux should be more generic, rather than platform
specific headers. So while platform_data may refer specifically to the platform
bus drivers, it's the closest thing we have to include/platform, which would be
ideal. I would prefer to stick with include/platform_data because:

1) Semantically, it's the closest thing there is
2) include/linux should be for more generic headers related to the OS or
   subsystems
3) It doesn't make sense to create a separate include/platform directory for a
   single header.
4) We don't want to rename platform_data to platform now and change all the
   drivers, but it could be changed later.

Thomas, do you disagree with any of the above?

> 
> > Darren, did we get your proposal right?
> 

Yes, your submission matches the intent from Thomas and I as I understand it.

> >>
> >> Is it indeed platform data? I would not create platform_data/x86
> >> without strong argument.
> >> Perhaps include/linux/clk/x86_pmc.h? (Yes, I know about clk-lpss.h
> >> which is old enough and was basically first try of clk stuff on x86)
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 

-- 
Darren Hart
Intel Open Source Technology Center

[toc] | [prev] | [next] | [standalone]

#1543689 — Re: [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks

On Fri, Dec 16, 2016 at 8:36 PM, Darren Hart <dvhart@infradead.org> wrote:
> On Tue, Dec 13, 2016 at 02:26:21AM +0200, Andy Shevchenko wrote:
>> On Tue, Dec 13, 2016 at 2:15 AM, Pierre-Louis Bossart
>> <pierre-louis.bossart@linux.intel.com> wrote:

>> For clock I would suggest include/linux/clk/ with x86_ prefix.
>> For the rest I have no strong opinion except trying to avoid
>> platform_data wording in the path as much as possible.
>>
>> As an example I could recall DMA engine subsystem where we have
>>
>> include/linux/platform_data/dma-*.h
>>
>> and
>>
>> include/linux/dma/*.h
>>
>> So, this sounds more to me as
>>
>> include/linux/x86/pmc_atom.h
>
> There should really be some Documentation about how to choose an include
> directory :-)

So true!

> My understanding is include/linux should be more generic, rather than platform
> specific headers. So while platform_data may refer specifically to the platform
> bus drivers, it's the closest thing we have to include/platform, which would be
> ideal. I would prefer to stick with include/platform_data because:
>
> 1) Semantically, it's the closest thing there is
> 2) include/linux should be for more generic headers related to the OS or
>    subsystems
> 3) It doesn't make sense to create a separate include/platform directory for a
>    single header.
> 4) We don't want to rename platform_data to platform now and change all the
>    drivers, but it could be changed later.

My understanding that part like P-Unit, PMIC, PMC, SCU, whatever we
have inside SoC is platform from hardware prospective, but from
software (driver) it doesn't use platform data since it's quite SoC
specific (like CPU model to differentiate). That's why something in
the middle between arch/x86/include/asm and
include/linux/platform_data.

I assume I would be not good in naming schemes, though platform_data
for file which doesn't contain platform data for platform device
sounds a bit confusing to me. Like someone already noticed
include/platform_data is already messy. This might just add another
level of it.

So, what is exactly confuses me is mixing data for *platform devices*
(as represented via *platform driver* -- struct platform_driver)  and
for SoC devices (no struct platform_driver per se).
Maybe I misunderstood something...

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1543706 — Re: [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks

On Fri, Dec 16, 2016 at 08:49:13PM +0200, Andy Shevchenko wrote:
> On Fri, Dec 16, 2016 at 8:36 PM, Darren Hart <dvhart@infradead.org> wrote:
> > On Tue, Dec 13, 2016 at 02:26:21AM +0200, Andy Shevchenko wrote:
> >> On Tue, Dec 13, 2016 at 2:15 AM, Pierre-Louis Bossart
> >> <pierre-louis.bossart@linux.intel.com> wrote:
> 
> >> For clock I would suggest include/linux/clk/ with x86_ prefix.
> >> For the rest I have no strong opinion except trying to avoid
> >> platform_data wording in the path as much as possible.
> >>
> >> As an example I could recall DMA engine subsystem where we have
> >>
> >> include/linux/platform_data/dma-*.h
> >>
> >> and
> >>
> >> include/linux/dma/*.h
> >>
> >> So, this sounds more to me as
> >>
> >> include/linux/x86/pmc_atom.h
> >
> > There should really be some Documentation about how to choose an include
> > directory :-)
> 
> So true!
> 
> > My understanding is include/linux should be more generic, rather than platform
> > specific headers. So while platform_data may refer specifically to the platform
> > bus drivers, it's the closest thing we have to include/platform, which would be
> > ideal. I would prefer to stick with include/platform_data because:
> >
> > 1) Semantically, it's the closest thing there is
> > 2) include/linux should be for more generic headers related to the OS or
> >    subsystems

Scratch #2 from the arguments since it's include/linux/platform_data that we're
talking about here.

> > 3) It doesn't make sense to create a separate include/platform directory for a
> >    single header.
> > 4) We don't want to rename platform_data to platform now and change all the
> >    drivers, but it could be changed later.
> 
> My understanding that part like P-Unit, PMIC, PMC, SCU, whatever we
> have inside SoC is platform from hardware prospective, but from
> software (driver) it doesn't use platform data since it's quite SoC
> specific (like CPU model to differentiate). That's why something in
> the middle between arch/x86/include/asm and
> include/linux/platform_data.
> 
> I assume I would be not good in naming schemes, though platform_data
> for file which doesn't contain platform data for platform device
> sounds a bit confusing to me. Like someone already noticed
> include/platform_data is already messy. This might just add another
> level of it.
> 
> So, what is exactly confuses me is mixing data for *platform devices*
> (as represented via *platform driver* -- struct platform_driver)  and
> for SoC devices (no struct platform_driver per se).
> Maybe I misunderstood something...

You're understanding is correct. We're just applying different values to the
respective merits of each argument.

The options are:

a) include/linux/x86
b) include/linux/platform_data/x86

In my opinion, a) looks like architecture and would be difficult to distinguish
from arch/x86/include. b) on the other hand clearly notes that it is for
platform specific information. If it was platform instead of platform_data, that
would be even better, but that could be a later change. But I think the
confusion over x86 arch in a) is worse than the more subtle (in my opinion)
distinction between "platform" and "platform_data".

I would want x86 maintainer approval before adding a), while b) I'm happy to add
ourselves - and we already have agreement from tglx on that.

To move forward, let's go with b). The new x86 directory clearly separates out
content which will make it trivial to move later if the need arises.

-- 
Darren Hart
Intel Open Source Technology Center

[toc] | [prev] | [next] | [standalone]

#1543805 — Re: [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks

On Fri, Dec 16, 2016 at 9:19 PM, Darren Hart <dvhart@infradead.org> wrote:
> On Fri, Dec 16, 2016 at 08:49:13PM +0200, Andy Shevchenko wrote:
>> On Fri, Dec 16, 2016 at 8:36 PM, Darren Hart <dvhart@infradead.org> wrote:
>> > On Tue, Dec 13, 2016 at 02:26:21AM +0200, Andy Shevchenko wrote:
>> >> On Tue, Dec 13, 2016 at 2:15 AM, Pierre-Louis Bossart

>> > There should really be some Documentation about how to choose an include
>> > directory :-)
>>
>> So true!

(1)

> The options are:
>
> a) include/linux/x86
> b) include/linux/platform_data/x86

Correct.

> In my opinion, a) looks like architecture and would be difficult to distinguish
> from arch/x86/include. b) on the other hand clearly notes that it is for
> platform specific information. If it was platform instead of platform_data, that
> would be even better, but that could be a later change. But I think the
> confusion over x86 arch in a) is worse than the more subtle (in my opinion)
> distinction between "platform" and "platform_data".
>
> I would want x86 maintainer approval before adding a), while b) I'm happy to add
> ourselves - and we already have agreement from tglx on that.
>
> To move forward, let's go with b).

Let me say I'm not fully satisfied, though for sake of moving forward
I agree with these arguments.

> The new x86 directory clearly separates out
> content which will make it trivial to move later if the need arises.

See (1). I would really appreciate if some agreement and documentation
will be developed.
In that case one of us would really have one serious argument to one
of the sides.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1543820 — Re: [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks

On Sat, Dec 17, 2016 at 12:29:41AM +0200, Andy Shevchenko wrote:
> On Fri, Dec 16, 2016 at 9:19 PM, Darren Hart <dvhart@infradead.org> wrote:
> > On Fri, Dec 16, 2016 at 08:49:13PM +0200, Andy Shevchenko wrote:
> >> On Fri, Dec 16, 2016 at 8:36 PM, Darren Hart <dvhart@infradead.org> wrote:
> >> > On Tue, Dec 13, 2016 at 02:26:21AM +0200, Andy Shevchenko wrote:
> >> >> On Tue, Dec 13, 2016 at 2:15 AM, Pierre-Louis Bossart
> 
> >> > There should really be some Documentation about how to choose an include
> >> > directory :-)
> >>
> >> So true!
> 
> (1)
> 
> > The options are:
> >
> > a) include/linux/x86
> > b) include/linux/platform_data/x86
> 
> Correct.
> 
> > In my opinion, a) looks like architecture and would be difficult to distinguish
> > from arch/x86/include. b) on the other hand clearly notes that it is for
> > platform specific information. If it was platform instead of platform_data, that
> > would be even better, but that could be a later change. But I think the
> > confusion over x86 arch in a) is worse than the more subtle (in my opinion)
> > distinction between "platform" and "platform_data".
> >
> > I would want x86 maintainer approval before adding a), while b) I'm happy to add
> > ourselves - and we already have agreement from tglx on that.
> >
> > To move forward, let's go with b).
> 
> Let me say I'm not fully satisfied, though for sake of moving forward
> I agree with these arguments.
> 
> > The new x86 directory clearly separates out
> > content which will make it trivial to move later if the need arises.
> 
> See (1). I would really appreciate if some agreement and documentation
> will be developed.
> In that case one of us would really have one serious argument to one
> of the sides.

Agreed. I always prefer to make decisions based on Documented precedent whenever
possible.

-- 
Darren Hart
Intel Open Source Technology Center

[toc] | [prev] | [next] | [standalone]

#1544464 — Re: [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks

[Multipart message — attachments visible in raw view] — view raw

On Fri, Dec 16, 2016 at 10:36:07AM -0800, Darren Hart wrote:

> My understanding is include/linux should be more generic, rather than platform
> specific headers. So while platform_data may refer specifically to the platform
> bus drivers, it's the closest thing we have to include/platform, which would be
> ideal. I would prefer to stick with include/platform_data because:

It's not specific to the platform bus, it's for use with the platform_data
pointer embedded in struct device that all buses can have - it's
extensively used for things like I2C and SPI for example.  But really it
doesn't matter *that* much.

[toc] | [prev] | [next] | [standalone]

#1540783 — Re: [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks

On Tue, Dec 13, 2016 at 2:15 AM, Pierre-Louis Bossart
<pierre-louis.bossart@linux.intel.com> wrote:
>
>>> +#include <linux/platform_data/x86/clk-byt-plt.h>
>
>
> This was a suggestion of Darren Hart in agreement with Thomas Gleixner.
> see
> http://mailman.alsa-project.org/pipermail/alsa-devel/2016-October/113936.html
>
> Darren, did we get your proposal right?
>
>> Is it indeed platform data? I would not create platform_data/x86
>> without strong argument.
>> Perhaps include/linux/clk/x86_pmc.h? (Yes, I know about clk-lpss.h
>> which is old enough and was basically first try of clk stuff on x86)

Looking more into the patch I got another question. Do we really need
a platform driver for that?

That's what I think motivated me for the header location. And that's
why I asked about use of the driver/clock provider in the latter
patch.

If the answer is yes, then I doubt which location is preferable,
otherwise include/clk/x86_*.h looks appropriate.

Sorry if I wasn't clear in the first place.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1541558 — Re: [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks

On 12/13, Andy Shevchenko wrote:
> On Fri, Dec 9, 2016 at 8:01 PM, Irina Tirdea <irina.tirdea@intel.com> 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

[toc] | [prev] | [next] | [standalone]

#1543204 — Re: [alsa-devel] [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks

Hi Stephen,

can you elaborate on the last comment?

thanks,

-Pierre


On 12/13/2016 05:25 PM, Stephen Boyd wrote:
>
>>> +                                       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.
I am not sure I understand this last comment.
init.name is not a constant, it's made of the "pmc_plt_clk_" string 
concatenated with an id which directly maps to which hardware clock is 
registered. Clients use devm_clk_get() with a "pmc_plt_clk_<n>" argument.
>

[toc] | [prev] | [next] | [standalone]

#1543253 — Re: [alsa-devel] [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks

On Fri, Dec 16, 2016 at 7:15 AM, Pierre-Louis Bossart
<pierre-louis.bossart@linux.intel.com> wrote:
> Hi Stephen,
>
> can you elaborate on the last comment?

Please don't do top posting.

>>> devm_kasprintf()
>>
>> Please no.

That's why I used modal verb "might" instead of "would".

>> 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.
>
> I am not sure I understand this last comment.
> init.name is not a constant, it's made of the "pmc_plt_clk_" string
> concatenated with an id which directly maps to which hardware clock is
> registered. Clients use devm_clk_get() with a "pmc_plt_clk_<n>" argument.

Giving more thoughts about design and use of this I would propose to
do the following.

1. Create under clock framework something like clk-pmc-atom clock
driver (see, for example, clk-fractional-divider, though this one
should indeed go under x86 folder).
2. In real provider, i.e. pmc_atom, create the necessary clock tree
with *names*.

Scheme with ID is fragile, imagine another version of PMC where
ordering would be mixed up? It's not hypothetical since we used to
have this already in pmc_atom for some registers and bits.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1543503 — Re: [alsa-devel] [PATCH v6 1/3] clk: x86: Add Atom PMC platform clocks

On 12/16/16 2:46 AM, Andy Shevchenko wrote:
> On Fri, Dec 16, 2016 at 7:15 AM, Pierre-Louis Bossart
> <pierre-louis.bossart@linux.intel.com> wrote:
>> Hi Stephen,
>>
>> can you elaborate on the last comment?
>
> Please don't do top posting.
>
>>>> devm_kasprintf()
>>>
>>> Please no.
>
> That's why I used modal verb "might" instead of "would".
>
>>> 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.
>>
>> I am not sure I understand this last comment.
>> init.name is not a constant, it's made of the "pmc_plt_clk_" string
>> concatenated with an id which directly maps to which hardware clock is
>> registered. Clients use devm_clk_get() with a "pmc_plt_clk_<n>" argument.
>
> Giving more thoughts about design and use of this I would propose to
> do the following.
>
> 1. Create under clock framework something like clk-pmc-atom clock
> driver (see, for example, clk-fractional-divider, though this one
> should indeed go under x86 folder).

apart from the name the current code already does this with code in 
drivers/clk/x86

> 2. In real provider, i.e. pmc_atom, create the necessary clock tree
> with *names*.
>
> Scheme with ID is fragile, imagine another version of PMC where
> ordering would be mixed up? It's not hypothetical since we used to
> have this already in pmc_atom for some registers and bits.

I don't want to deal with hypothetical stuff happening to legacy 
hardware. If there is a problem at some point, it's no big deal to add a 
platform-dependent lookup table and change the registers being accessed.

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web