Hi Krzysztof,

On 6/14/19 2:58 PM, Krzysztof Kozlowski wrote:

On Fri, 14 Jun 2019 at 11:53, Lukasz Luba [off-list ref] wrote:

quoted

This patch adds driver for Exynos5422 Dynamic Memory Controller.
The driver provides support for dynamic frequency and voltage scaling for
DMC and DRAM. It supports changing timings of DRAM running with different
frequency. There is also an algorithm to calculate timigns based on
memory description provided in DT.
The patch also contains needed MAINTAINERS file update.

Signed-off-by: Lukasz Luba <redacted>
---
  MAINTAINERS                             |    8 +
  drivers/memory/samsung/Kconfig          |   17 +
  drivers/memory/samsung/Makefile         |    1 +
  drivers/memory/samsung/exynos5422-dmc.c | 1262 +++++++++++++++++++++++
  4 files changed, 1288 insertions(+)
  create mode 100644 drivers/memory/samsung/exynos5422-dmc.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 57f496cff999..6ffccfd95351 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS

@@ -3470,6 +3470,14 @@ S:       Maintained
  F:     drivers/devfreq/exynos-bus.c
  F:     Documentation/devicetree/bindings/devfreq/exynos-bus.txt

+DMC FREQUENCY DRIVER FOR SAMSUNG EXYNOS5422

Eh, more comments from my side.
"For the hard of thinking, this list is meant to remain in alphabetical order."

OK

quoted

+M:     Lukasz Luba [off-list ref]
+L:     linux-pm@vger.kernel.org
+L:     linux-samsung-soc@vger.kernel.org
+S:     Maintained
+F:     drivers/memory/samsung/exynos5422-dmc.c
+F:     Documentation/devicetree/bindings/memory-controllers/exynos5422-dmc.txt
+
  BUSLOGIC SCSI DRIVER
  M:     Khalid Aziz [off-list ref]
  L:     linux-scsi@vger.kernel.org

diff --git a/drivers/memory/samsung/Kconfig b/drivers/memory/samsung/Kconfig
index 79ce7ea58903..c93baa029654 100644
--- a/drivers/memory/samsung/Kconfig
+++ b/drivers/memory/samsung/Kconfig

@@ -5,6 +5,23 @@ config SAMSUNG_MC
           Support for the Memory Controller (MC) devices found on
           Samsung Exynos SoCs.

+config ARM_EXYNOS5422_DMC

Why you added prefix of ARM to CONFIG think none of other Exynos
Kconfig options follow such convention (except devfreq).

In the previous versions the driver was in drivers/devfreq/,
that's why they have this prefix.

quoted

+       tristate "ARM EXYNOS5422 Dynamic Memory Controller driver"
+       depends on ARCH_EXYNOS
+       select DDR

In general select should be used only for non-visible symbols and DDR
is visible. Use depends.

OK

quoted

+       select PM_DEVFREQ

This definitely cannot be select. You do not select entire subsystem
because some similar driver was chosen.

So I will use depends int this case

quoted

+       select DEVFREQ_GOV_SIMPLE_ONDEMAND
+       select DEVFREQ_GOV_USERSPACE

I think only simple_ondemand is needed. Is userspace governor
necessary for working? Or actually maybe both could be skipped?

Actually we can skip both governors from here.

quoted

+       select PM_DEVFREQ_EVENT

Again, depends.

OK

There are these 4 options which the driver depends on:
         depends on ARCH_EXYNOS
         depends on DDR
         depends on PM_DEVFREQ
         depends on PM_DEVFREQ_EVENT

Should I merged them into one, two lines? like below:
a)
depends on (ARCH_EXYNOS && DDR && PM_DEVFREQ && PM_DEVFREQ_EVENT)
b) grouped into two sets
depends on (ARCH_EXYNOS && DDR)
depends on (PM_DEVFREQ && PM_DEVFREQ_EVENT)
c) grouped by pm_devfreq only
depends on ARCH_EXYNOS
depends on DDR
depends on (PM_DEVFREQ && PM_DEVFREQ_EVENT)

quoted

+       select PM_OPP

This option can be used here IIUC

quoted

+       help
+         This adds driver for Exynos5422 DMC (Dynamic Memory Controller).
+         The driver provides support for Dynamic Voltage and Frequency Scaling in
+         DMC and DRAM. It also supports changing timings of DRAM running with
+         different frequency. The timings are calculated based on DT memory
+         information.
+
+
  if SAMSUNG_MC

Why this is outside of SAMSUNG_MC?

Good question, I will move it below this 'if' statement.

Regards,
Lukasz

Best regards,
Krzysztof