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

Hi Krzysztof,

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

quoted

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

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

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

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

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

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)

I like the third option... although I would be also happy to see
COMPILE_TEST (probably with IOMEM). You need to check which
dependencies are still necessary for COMPILE_TEST.

Best regards,
Krzysztof