Re: [PATCH 7/8] cpufreq: st: Provide runtime initialised driver for ST's platforms

On Tue, 23 Jun 2015, Viresh Kumar wrote:

On 23-06-15, 08:16, Lee Jones wrote:

quoted

Thanks for your timely review Viresh.

Your welcome Lee :)

quoted

On Tue, 23 Jun 2015, Viresh Kumar wrote:

quoted

On 22-06-15, 16:43, Lee Jones wrote:

quoted

+config ARM_ST_CPUFREQ
+	bool "ST CPUFreq support"

Isn't using ST just too generic? There are multiple SoCs ST has been
involved with, I have worked on a completely different series.
Probably a more relative string is required here, like stih407 ?

This is ST's only CPUFreq implementation and is pretty board
agnostic.  This particular driver only currently supports the STiH407
family, but internally it supports some others too.  I'll have a chat
and see if we can make it more specific somehow.

So, SPEAr is also from ST. And it already have a driver for itself.

Sure.  I will use STI as suggested by Maxime.

quoted

quoted

quoted

+	if (!ddata->dvfs_tab_count) {
+		dev_err(&pdev->dev, "No suitable AVS table found\n");

Why is this an error? I thought in this case you will go ahead with
the normal OPP-table.

I've written it so it's an error within this function, as it makes the
function fail, but is downgraded by the caller to a warning and
gracefully bypassed to still allow frequency scaling.

Not that, I was asking about the print. I thought we will still try to
find OPP from the CPU node and a warning or a error might not be the
right choice. You can surely add a debug print. Currently you are
doing a dev_err() here, followed by a dev_warn() I think..

Okay, but the reasoning is the same.  I consider the function to have
failed, but the over-all failure culminates in just a warning that
voltage scaling has indeed failed, but we can still go on with
frequency scaling.

Unless his is a big blocker for you, I would like to keep these
semantics.

quoted

quoted

So you have added new OPPs here, but cpufreq-dt will try to add old
OPPs. You must be getting lots of warnings ?

Yes, we recieve the 'duplicate OPPs detected' warning, but there is
nothing we can do about that.

:)

OPP-v2 will get that solved too..

I'll take another look at them to see if there is anything we can
use.

quoted

quoted

quoted

+	if (ddata->substrate < 0)
+		goto set_default;

Maybe:

	if (ddata->substrate >= 0)
                return;

0 is a valid substrate value.

I had >= in the comparison. Wasn't that right?

Oh, you reversed the condition, I see now.

And I was just suggesting that a single return can be used instead of

So technically you are correct, but it makes the code slightly more
confusing IMHO.  Yes, it's one more line of code, but it's worth it to
add clarity.
 

if (xyz)
        goto set_default;
return;

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog