Re: [PATCH] Exynos : Add support for Exynos random number generator

[Stephen Boyd <sboyd@codeaurora.org>]

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.