Patchwork ARM: mxc-rnga: fix data_present API

login
register
mail settings
Submitter Benoît Thébaudeau
Date June 12, 2012, 9:07 p.m.
Message ID <1795608889.2533817.1339535267600.JavaMail.root@advansee.com>
Download mbox | patch
Permalink /patch/164499/
State New
Headers show

Comments

Benoît Thébaudeau - June 12, 2012, 9:07 p.m.
Cc: Matt Mackall <mpm@selenic.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Alan Carvalho de Assis <acassis@gmail.com>
Cc: <linux-arm-kernel@lists.infradead.org>
Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
---
 .../drivers/char/hw_random/mxc-rnga.c              |   22 +++++++++++++-------
 1 file changed, 14 insertions(+), 8 deletions(-)
Fabio Estevam - June 12, 2012, 9:12 p.m.
On Tue, Jun 12, 2012 at 6:07 PM, Benoît Thébaudeau
<benoit.thebaudeau@advansee.com> wrote:
> +
> +       for (i = 0; i < 20; i++) {

Where does this magic 20 comes from?
Benoît Thébaudeau - June 12, 2012, 9:25 p.m.
On Tue, Jun 12, 2012 at 11:12 PM, Fabio Estevam <festevam@gmail.com> wrote:
> On Tue, Jun 12, 2012 at 6:07 PM, Benoît Thébaudeau
> <benoit.thebaudeau@advansee.com> wrote:
> > +
> > +       for (i = 0; i < 20; i++) {
> 
> Where does this magic 20 comes from?

From the other hw_random drivers. Most use this value, and it works fine on i.MX
too.

Regards,
Benoît
Uwe Kleine-König - June 13, 2012, 6:36 a.m.
Hello,

your changelog is very sparse. Maybe point out the commit that changed
the API without fixing its users?

On Tue, Jun 12, 2012 at 11:07:47PM +0200, Benoît Thébaudeau wrote:
> Cc: Matt Mackall <mpm@selenic.com>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Alan Carvalho de Assis <acassis@gmail.com>
> Cc: <linux-arm-kernel@lists.infradead.org>
> Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
> ---
>  .../drivers/char/hw_random/mxc-rnga.c              |   22 +++++++++++++-------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git linux-next-HEAD-c90e5d2.orig/drivers/char/hw_random/mxc-rnga.c linux-next-HEAD-c90e5d2/drivers/char/hw_random/mxc-rnga.c
> index 187c6be..2924253 100644
> --- linux-next-HEAD-c90e5d2.orig/drivers/char/hw_random/mxc-rnga.c
> +++ linux-next-HEAD-c90e5d2/drivers/char/hw_random/mxc-rnga.c
> @@ -24,6 +24,7 @@
>  #include <linux/ioport.h>
>  #include <linux/platform_device.h>
>  #include <linux/hw_random.h>
> +#include <linux/delay.h>
>  #include <linux/io.h>
>  
>  /* RNGA Registers */
> @@ -60,16 +61,21 @@
>  
>  static struct platform_device *rng_dev;
>  
> -static int mxc_rnga_data_present(struct hwrng *rng)
> +static int mxc_rnga_data_present(struct hwrng *rng, int wait)
>  {
> -	int level;
>  	void __iomem *rng_base = (void __iomem *)rng->priv;
> -
> -	/* how many random numbers is in FIFO? [0-16] */
> -	level = ((__raw_readl(rng_base + RNGA_STATUS) &
> -			RNGA_STATUS_LEVEL_MASK) >> 8);
> -
> -	return level > 0 ? 1 : 0;
> +	int level, data, i;
level is only used in the for loop, so it should be declared there, too.

> +
> +	for (i = 0; i < 20; i++) {
> +		/* how many random numbers is in FIFO? [0-16] */
As you touch this comment can you fix the grammar, too? (i.e. s/is/are/)

> +		level = ((__raw_readl(rng_base + RNGA_STATUS) &
> +				RNGA_STATUS_LEVEL_MASK) >> 8);
> +		data = level > 0 ? 1 : 0;
This statement is equivalent to:

	data = level > 0;

so IMHO there is no need for the data variable.

> +		if (data || !wait)
> +			break;
> +		udelay(10);
Apart from the magic 20 that Fabio already pointed out, these 10 us are
magic, too. What is the requirement when wait evaluates to true? Are you
allowed to sleep? If so, maybe better do that?

> +	}
> +	return data;
>  }
>  
>  static int mxc_rnga_data_read(struct hwrng *rng, u32 * data)

Best regards
Uwe

Patch

diff --git linux-next-HEAD-c90e5d2.orig/drivers/char/hw_random/mxc-rnga.c linux-next-HEAD-c90e5d2/drivers/char/hw_random/mxc-rnga.c
index 187c6be..2924253 100644
--- linux-next-HEAD-c90e5d2.orig/drivers/char/hw_random/mxc-rnga.c
+++ linux-next-HEAD-c90e5d2/drivers/char/hw_random/mxc-rnga.c
@@ -24,6 +24,7 @@ 
 #include <linux/ioport.h>
 #include <linux/platform_device.h>
 #include <linux/hw_random.h>
+#include <linux/delay.h>
 #include <linux/io.h>
 
 /* RNGA Registers */
@@ -60,16 +61,21 @@ 
 
 static struct platform_device *rng_dev;
 
-static int mxc_rnga_data_present(struct hwrng *rng)
+static int mxc_rnga_data_present(struct hwrng *rng, int wait)
 {
-	int level;
 	void __iomem *rng_base = (void __iomem *)rng->priv;
-
-	/* how many random numbers is in FIFO? [0-16] */
-	level = ((__raw_readl(rng_base + RNGA_STATUS) &
-			RNGA_STATUS_LEVEL_MASK) >> 8);
-
-	return level > 0 ? 1 : 0;
+	int level, data, i;
+
+	for (i = 0; i < 20; i++) {
+		/* how many random numbers is in FIFO? [0-16] */
+		level = ((__raw_readl(rng_base + RNGA_STATUS) &
+				RNGA_STATUS_LEVEL_MASK) >> 8);
+		data = level > 0 ? 1 : 0;
+		if (data || !wait)
+			break;
+		udelay(10);
+	}
+	return data;
 }
 
 static int mxc_rnga_data_read(struct hwrng *rng, u32 * data)