Re: MPC8349ea Random Device Generator driver

[Timur Tabi <timur@freescale.com>]

Philippe Lachenal wrote:
> Hello !
> 
> I've made a driver for the MPC8349ea Random Device Generator, and I 
> therefore submit it to your impartial judgment.. ;)
> thanks a lot !

First, please don't post your patch as an attachment.  I recommend you use git-send-email.

> diff --git a/arch/powerpc/platforms/83xx/mpc834x_itx.c b/arch/powerpc/platforms/83xx/mpc834x_itx.c
> old mode 100644
> new mode 100755
> index 40a0194..b05980b
> --- a/arch/powerpc/platforms/83xx/mpc834x_itx.c
> +++ b/arch/powerpc/platforms/83xx/mpc834x_itx.c
> @@ -48,6 +48,24 @@ unsigned long isa_mem_base = 0;
>   * Setup the architecture
>   *
>   */
> +
> +static struct of_device_id mpc834x_itx[] = {
> +	{ .type = "soc", },
> +	{ .type ="crypto", },

You have a lot of minor spacing problems, like this one.  There should be a blank space 
before '"crypto"'.


> +
> +#include <asm/of_platform.h>
> +#include <asm/io.h>
> +
> +#define TALITOS_RNGSR		0x028	/* RNG status register */
> +#define TALITOS_RNGSR_HI	0x02c	/* RNG status register */
> +#define TALITOS_RNGSR_HI_RD	0x1	/* RNG Reset done */
> +#define TALITOS_RNGSR_HI_OFL	0xff0000/* number of dwords in RNG output FIFO*/
> +#define TALITOS_RNGDSR		0x010	/* RNG data size register */
> +#define TALITOS_RNGDSR_HI	0x014	/* RNG data size register */
> +#define TALITOS_RNG_FIFO	0x800	/* RNG FIFO - pool of random numbers */
> +#define TALITOS_RNGISR		0x030	/* RNG Interrupt status register */
> +#define TALITOS_RNGISR_HI	0x034	/* RNG Interrupt status register */
> +#define TALITOS_RNGRCR		0x018	/* RNG Reset control register */
> +#define TALITOS_RNGRCR_HI	0x01c	/* RNG Reset control register */

Please create a structure instead of using macros like this.  Example:

struct sec_rng {
	__be64	rngmr;
	u8 res1[8];
	__be64 rngdsr;
	__be64 rngrcr;
...
};

and then ...	

> +static int talitos_hwrng_data_present(struct hwrng *rng)
> +{
> +	void __iomem *rng_regs = (void __iomem *)rng->priv;

	struct sec_rng __iomem *rng = (struct sec_rng __iomem *) rng->priv;

> +
> +
> +	/* check for things like FIFO underflow */
> +	
> +	u32 v;
> +
> +	v = in_be32(rng_regs + TALITOS_RNGISR_HI);

	u64 v;
	v = rng->rngisr;

or something like that.  Try to use the built-in support for 64-bit data types when possible.

I get this confused easily, but I don't think you should be using the in/out_be macros 
either.  Just do regular reads and writes.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale