lkml.org 
[lkml]   [2014]   [Aug]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCHv9 1/3] mfd: altera: Add Altera SDRAM Controller
    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@opensource.altera.com wrote:
    > >>>
    > >>>>From: Thor Thayer <tthayer@opensource.altera.com>
    > >>>>
    > >>>>Add a simple MFD for the Altera SDRAM Controller.
    > >>>>
    > >>>>Signed-off-by: Alan Tull <atull@opensource.altera.com>
    > >>>>Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
    > >>>>---
    > >>>>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 linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2014-08-04 11:21    [W:2.416 / U:0.096 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site