Date:	Mon, 20 Feb 2012 19:37:43 +0530
From:	Aneesh V <aneesh@...com>
To:	Greg KH <greg@...ah.com>
CC:	Santosh Shilimkar <santosh.shilimkar@...com>,
	akpm@...ux-foundation.org, linux-omap@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 0/8] Add TI EMIF SDRAM controller driver

On Friday 17 February 2012 11:20 PM, Greg KH wrote:
> On Fri, Feb 17, 2012 at 07:26:29PM +0530, Aneesh V wrote:

[...]

>>> I don't know what any of those TLA words mean, so I really can't suggest
>>
>> This is a driver for TI's memory controller(called EMIF). The
>> driver is needed for adjusting the controller settings on frequency,
>> voltage, and temperature changes. Any suggestion as to where this
>> should go?
>
> For those type of things, don't the iio framework provide what you need
> and what?  Or perhaps the cpufreq layer?
>

I don't think this driver fits into the iio framework. This is not a
sensor or IO kind of device. Neither does it generate events. The
primary responsibility of this driver is to re-configure the SDRAM
controller settings on all transient events affecting it after the
bootloader has set it up at boot-time. These are typically events
generated by other sub-systems in the kernel such as the clock
framework (DDR frequency change) regulator framework (voltage
transitions) etc. Temperature events are generated (by polling the
SDRAM device) and handled within the driver.

Neither do I think it will fit into cpufreq layer because DDR
frequency  can be triggered by clock framework independent of cpufreq.
Moreover handling DDR frequency change is only one of the functions of
the driver.

  >>> where this code should go.  But just from this diffstat, it looks like
>>> you are creating a new user/kernel interface, without documenting it
>>> anywhere, which isn't ok.
>>
>> I think you are referring to the header files added in include/linux/
>> They are not creating new user/kernel interface per se.
>
> Then why are they in include/linux/ ?

My mistake. I will move them to more appropriate places.

>
>> "include/linux/jedec_ddr.h" is the interface to a library that contains
>> data from the DDR specs. "include/linux/emif.h" has definitions for
>> platform data needed by the driver. Maybe these should go to some other
>> sub-directory within include/ or include/linux/ ?
>
> Who needs these files?  If it's only the drivers themselves, then put it
> in the same directory as the driver.  If it's platform data, then put
> it, and only it, in the include/linux/platform_data/ directory.

The above work has two components:
1. The driver
2. A small library with data from the SDRAM specs that the driver uses.

Alan Cox has suggested to move the library part to lib/ so the
corresponding header file jedec_ddr.h has to be in some common place.
Can this be in include/misc or do you have a better place to suggest?

The other one emif.h can be split into two parts, one for platform data
that can go under include/linux/platform_data/ and the rest in the same
directory as the driver, as you suggested.

>
>> I shall add documentation for the driver in the next revision.
>
> That would be good.  Please also cc: me on the next revision if you wish
> me to take the patches (hint, get_maintainer.pl should have told you
> this...)

Sure. Thanks.

- Aneesh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists