On Sun, Jan 27, 2013 at 11:07:22AM +0100, Andrew Lunn wrote:
> +static struct priv
> +{
> + struct clk *cpu_clk;
> + struct clk *ddr_clk;
> + struct clk *powersave_clk;
> + struct device *dev;
> + void __iomem *base;
> +} priv;
I guess you probably think that the compiler will do something special
with this, like reference these all through a base address and offset,
but you'd be wrong there. This is no different from declaring each
as an individual static variable.
> +static unsigned int kirkwood_cpufreq_get_cpu_frequency(unsigned int cpu)
> +{
> + if (__clk_is_enabled(priv.powersave_clk))
This looks to me to be a layering violation.
> +static int kirkwood_cpufreq_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = of_find_compatible_node(
> + NULL, NULL, "marvell,kirkwood-core-clock");
What if np is NULL?
> +
> + struct of_phandle_args clkspec;
> + struct resource *res;
> + int err;
> +
> + priv.dev = &pdev->dev;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(&pdev->dev, "Cannot get memory resource\n");
> + return -ENODEV;
> + }
> + priv.base = devm_request_and_ioremap(&pdev->dev, res);
> + if (!priv.base) {
> + dev_err(&pdev->dev, "Cannot ioremap\n");
> + return -ENOMEM;
A number of people have been pointing out that the right return value for
this is -EADDRNOTAVAIL as documented against devm_request_and_ioremap().
> + }
> +
> + clkspec.np = np;
> + clkspec.args_count = 1;
> + clkspec.args[0] = 1;
> +
> + priv.cpu_clk = of_clk_get_from_provider(&clkspec);
Oh, yet another way to get clocks...
--
To unsubscribe from this list: send the line "unsubscribe cpufreq" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
