On Fri, 01 Aug 2014, Thor Thayer wrote:

On 08/01/2014 03:13 AM, Lee Jones wrote:

quoted

On Thu, 31 Jul 2014, Thor Thayer wrote:

quoted

On 07/31/2014 03:26 AM, Lee Jones wrote:

quoted

On Wed, 30 Jul 2014, tthayer@opensource.altera.com wrote:

quoted

From: Thor Thayer <redacted>

Add a simple MFD for the Altera SDRAM Controller.

Signed-off-by: Alan Tull <redacted>
Signed-off-by: Thor Thayer <redacted>
---
v1-8: The MFD implementation was not included in the original series.

v9: New MFD implementation.
---
 MAINTAINERS                    |    5 ++
 drivers/mfd/Kconfig            |    7 ++
 drivers/mfd/Makefile           |    1 +
 drivers/mfd/altera-sdr.c       |  162 ++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/altera-sdr.h |  102 +++++++++++++++++++++++++
 5 files changed, 277 insertions(+)
 create mode 100644 drivers/mfd/altera-sdr.c
 create mode 100644 include/linux/mfd/altera-sdr.h

[...]

quoted

quoted

quoted

quoted

+	return readl(sdr->reg_base + reg_offset);
+}
+EXPORT_SYMBOL_GPL(altera_sdr_readl);
+
+void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value)
+{
+	writel(value, sdr->reg_base + reg_offset);
+}
+EXPORT_SYMBOL_GPL(altera_sdr_writel);

Why are you abstracting these?

You still didn't answer this?

quoted

quoted

quoted

Might be better to use Regmap even.

regmap seems unnecessarily complex for what we're doing which is why
this method was chosen.

Future drivers will access different sets of registers in the
device. These drivers won't share bitfields in the same register so
the MFD seemed like the best solution. Originally we implemented
this using syscon but that seems to be frowned upon so we changed to
using a MFD.

Why was the use of syscon frowned upon?  Can you link me to the
thread?  Writing directly to the registers sounds to me a lot worse
than using infrastructure which was designed for these kinds of
accesses.

If you do choose to fiddle with the registers in this manner, is there
any reason why you're calling back into here, rather than using
readl() and writel() directly?

We'd prefer to use syscon and that is what we started with. If you'd
like to be our advocate, I will return to that because it was pretty
clean. My primary concern is to get it upstreamed and if it is MFD
then I'll make the changes.

Here are the threads.
http://marc.info/?l=linux-kernel&m=140128791902800&w=2
and
http://article.gmane.org/gmane.linux.kernel/1679601

Syscon looks the most appropriate to me.

[...]

quoted

quoted

quoted

quoted

+EXPORT_SYMBOL_GPL(altera_sdr_mem_size);

Should this really be done in here?  Isn't this an SDRAM function?

This register is part of the SDRAM controller and size information
may be required by the other drivers that share this memory
area/need SDRAM information.

Then export a function from the SDRAM driver, not from here.

We don't have an SDRAM driver.  Although I could put this in the
EDAC driver it would be lost to anyone else wanting this
functionality so this seemed to be the logical place.

Why can't you export it from the EDAC driver?

[...]

quoted

quoted

quoted

quoted

+static int __init altera_sdr_init(void)
+{
+	return platform_driver_register(&altera_sdr_driver);
+}
+postcore_initcall(altera_sdr_init);

Why was this chosen?

We want this to happen pretty early.

If you _need_ this is happen early, core_initcall() is more commonly
used, but _why_ do you need it to happen this early?

The syscon driver used this designation. After talking with Alan,
this could be changed to a core_initcall(). However, it could also
be a subsys_initcall which seems to be more common in the MFD
drivers.

That doesn't answer my question still. 

What is the reason, requirement, need for this driver to be probed so
early during the boot process?

[...]

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog