Messages in this thread | | | Date | Mon, 20 Feb 2012 19:37:43 +0530 | From | Aneesh V <> | 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
| |