On Fri, 01 Aug 2014, Thor Thayer wrote: > On 08/01/2014 03:13 AM, Lee Jones wrote: > >On Thu, 31 Jul 2014, Thor Thayer wrote: > >>On 07/31/2014 03:26 AM, Lee Jones wrote: > >>>On Wed, 30 Jul 2014, tthayer@xxxxxxxxxxxxxxxxxxxxx wrote: > >>> > >>>>From: Thor Thayer <tthayer@xxxxxxxxxxxxxxxxxxxxx> > >>>> > >>>>Add a simple MFD for the Altera SDRAM Controller. > >>>> > >>>>Signed-off-by: Alan Tull <atull@xxxxxxxxxxxxxxxxxxxxx> > >>>>Signed-off-by: Thor Thayer <tthayer@xxxxxxxxxxxxxxxxxxxxx> > >>>>--- > >>>>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 [...] > >>>>+ 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? > >>>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. [...] > >>>>+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? [...] > >>>>+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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html