On Wed, Dec 21, 2011 at 09:20:46AM +0800, Richard Zhao wrote:

Hi Mark,

On Tue, Dec 20, 2011 at 11:48:45PM +0000, Mark Brown wrote:

quoted

On Wed, Dec 21, 2011 at 07:27:03AM +0800, Richard Zhao wrote:

quoted

On Tue, Dec 20, 2011 at 02:59:04PM +0000, Mark Brown wrote:

quoted

quoted

My comments on the previous version of the patch still apply:

quoted

quoted

 - The voltage ranges being set need to be specified as ranges.

quoted

cpu normally need strict voltages. and only proved operating opints
are allowed to set in dts. If the voltage changes slightly especially
for high frequency, it's easy to cause unstable.

Clearly there will be limits which get more and more restrictive as the
frequencies get higher but there will always be at least some play in
the numbers as one must at a minimum specify tolerance ranges, and at
lower frequencies the ranges specified will typically get compartively
large.

hmm, reasonable. I'll add it in dts. Thanks.

quoted

Note also that not all hardware specifies things in terms of a fixed set
of operating points, sometimes only the minimum voltage specification is
varied with frequency or sometimes you see maximum and minimum stepping
independently.

cpus that don't use freq table is out of scope of this driver.

quoted

Further note that if all hardware really does have as tight a set of
requirements as you suggest then the regulator support in the driver
needs to be non-optional otherwise a board without software regulator
control might drop the frequency without also dropping the voltage.

It's ok to only adjuct freq without changes voltage. You can find many
arm soc cpufreq drivers don't change voltage.
If dts specify voltage but don't have such regulator. I'll assume it
always runs on the initial volatage (highest for most cases).

quoted

quoted

quoted

 - Frequencies that can't be supported due to limitations of the
   available supplies shouldn't be exposed to users.

quoted

As I said, only proved operation points are allowed.

This statement appears to be unrelated to the comment you're replying
to.

I meant the exact voltage can always successfull set. Anyway, I'll add
regulator_set_voltage return value checking.

quoted

quoted

quoted

You also need to define how the core supplies get looked up.

quoted

It's pure software. platform uses this driver have to define "cpu" consumer.

You still need to define this in the binding.

You mean regulator DT binding? already in ? I'll check it.

Mark, cpu node is not a struct device, sys_device instead. I can not find
regulator via device/dt node. Can I still use the string to get regulator
after converting to DT?

And regulator binding is not on cpufreq git tree.

Thanks
Richard

quoted

quoted

quoted

quoted

+	pr_info("Generic CPU frequency driver\n");

quoted

quoted

This seems noisy...

quoted

Why? Do you think only errors and warnings can print out?

Yes.

Maybe I can remove it. But I'd probably add freq table dump. It's more important.
Agree?

I checked pr_fmt. Thanks very much. I'll use below and remove __func_.
#define pr_fmt(fmt)             KBUILD_MODNAME ": " fmt


Thanks
Richard

quoted

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel