Message-ID: <4FE2913B.1040102@codeaurora.org>
Date: Wed, 20 Jun 2012 20:12:59 -0700
From: Stephen Boyd <sboyd@codeaurora.org>
User-Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:12.0) Gecko/20120428 Thunderbird/12.0.1
MIME-Version: 1.0
To: jonghwa3.lee@samsung.com
CC: linux-kernel@vger.kernel.org, Matt Mackall <mpm@selenic.com>,
        Herbert Xu <herbert@gondor.hengli.com.au>,
        Nicolas Ferre <nicolas.ferre@atmel.com>,
        Julia Lawall <Julia.Lawall@lip6.fr>, Jamie Iles <jamie@jamieiles.com>,
        Kyungmin Park <kyungmin.park@samsung.com>
Subject: Re: [PATCH] Exynos : Add support for Exynos random number generator
References: <1340180526-24542-1-git-send-email-jonghwa3.lee@samsung.com> <4FE26F27.4010106@codeaurora.org> <4FE2897C.6050002@samsung.com>
In-Reply-To: <4FE2897C.6050002@samsung.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2371
Lines: 64

On 06/20/12 19:39, jonghwa3.lee@samsung.com wrote:
>
>>> +static int exynos_init(struct hwrng *rng)
>>> +{
>>> +	struct exynos_rng *exynos_rng = container_of(rng,
>>> +						struct exynos_rng, rng);
>>> +	int i;
>>> +	int ret = 0;
>>> +	u32 PRND_SEED[5];
>>> +
>>> +	pm_runtime_put_noidle(exynos_rng->dev);
>>> +	pm_runtime_get_sync(exynos_rng->dev);
>> This looks very odd. Why are you calling pm_runtime_put_noidle()?
>>
>
> When this callback function is called, the status of power state is
> 'suspended' and use_count is 1. To perform pm_runtime_get_sync()
> correctly, it requires to have 'suspended' status and use_count is 0.
> Thus i force to decrease use_count only with using
> pm_runtime_put_noidle. I know it looks odd, but i couldn't find better
> way. Otherwise it can use clk_enable() directly, but i think that it
> isn't good neither.

Is the device suspended initially at probe? If so can you set the state
of the device to suspended?

>
>>> +{
>>> +	int ret;
>>> +	struct exynos_rng *exynos_rng;
>>> +	struct resource *res;
>>> +
>>> +	exynos_rng = devm_kzalloc(&pdev->dev, sizeof(struct exynos_rng),
>>> +					GFP_KERNEL);
>>> +	if (!exynos_rng)
>>> +		return -ENOMEM;
>>> +
>>> +	exynos_rng->dev = &pdev->dev;
>>> +	exynos_rng->rng.name = "exynos";
>>> +	exynos_rng->rng.init =	exynos_init;
>>> +	exynos_rng->rng.read = exynos_read;
>>> +	exynos_rng->clk = clk_get(NULL, "secss");
>> Can you please pass &pdev->dev to clk_get()?
>>
>
> But, this clock is not only used in PRNG. Should i put it in?

Why would that matter? I'm not familiar with the samsung clock
implementation but I would assume that something like clkdev is being
used. Ideally you would have one lookup entry per struct device that
uses this clock and then tie each lookup entry to the same clock. This
allows you to support multiple instances of the same device in a generic
way (e.g. two rng devices that use different clocks but from the driver
perspective they have the same name 'secss').

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/