On Wednesday 06 October 2010, Yong Shen wrote:
Hi Arnd,
Really appreciate your valuable comments. Most of them are accepted. I have different option about two comments.
It would be better to make this code a proper device driver, probably a platform_driver unless you have a way to probe the presence of the registers on another bus.
Making it a driver that registers to a bus lets you separate the probing from the implementation, and gives you a structure to add your private variables to.
cpufreq is supposed to be registered using cpufreq_register_driver directly, so no other platform data is needed. You can also find out other cpufreq driver is designed this way, like omap cpufreq driver.
Ok, it is indeed different from what I thought.
My thought was that you have resources that need to be attached to a device which then can get matched to a device_driver.
However, this is not how it works with the generic clock framework. The device that you are controlling is not a random platform device but the actual CPU, which has a node in the device tree already (/sys/devices/system/cpu/*) and that gets controlled by the generic cpufreq layer, while the resources are tied to the struct clk that you are controlling.
So if anything, the clk is what needs to be a platform device, not an artificial cpufreq device. You are only adding another clock to the clock-mx51.c file and you are consistent with the existing clocks in there, so I'm not asking you to change anything here.
I wonder however if we can create a common cpufreq driver that would work for all platforms that just need to call clk_set_rate in the end and can operate from platform specific tables otherwise. About half the cpufreq drivers in arm seem to be a variation of this already, so we might be able to make the clock framework good enough for this.
Don't use __raw_readl but readl/ioread32, which have more well-defined behaviour.
I think __raw_readl is ok here, since in the platform related code, we know the register layout and length, this is more efficient. Also this is extensively used in arch/arm/.
I still disagree, but it's not important. IMHO most of the uses of __raw_readl should be converted to readl or readl_relaxed if you are worried about efficiency.
The main difference between __raw_readl and readl_relaxed is that the endianess is well-defined on readl_relaxed.
Arnd