From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753380AbbBYSnp (ORCPT ); Wed, 25 Feb 2015 13:43:45 -0500 Received: from mail-ig0-f178.google.com ([209.85.213.178]:44883 "EHLO mail-ig0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753006AbbBYSnn (ORCPT ); Wed, 25 Feb 2015 13:43:43 -0500 Date: Wed, 25 Feb 2015 10:43:38 -0800 From: Dmitry Torokhov To: Scott Branden Cc: bcm-kernel-feedback-list@broadcom.com, Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Matt Mackall , Herbert Xu , Grant Likely , Ray Jui , Jonathan Richardson , Anatol Pomazao , devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 2/2] hwrng: iproc-rng200 - Add Broadcom IPROC RNG driver Message-ID: <20150225184338.GA3215@dtor-ws> References: <1424888184-22180-1-git-send-email-sbranden@broadcom.com> <1424888184-22180-3-git-send-email-sbranden@broadcom.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1424888184-22180-3-git-send-email-sbranden@broadcom.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Scott, On Wed, Feb 25, 2015 at 10:16:24AM -0800, Scott Branden wrote: > This adds a driver for random number generator present on Broadcom > IPROC devices. > > Reviewed-by: Ray Jui > Signed-off-by: Scott Branden > --- > drivers/char/hw_random/Kconfig | 13 ++ > drivers/char/hw_random/Makefile | 1 + > drivers/char/hw_random/iproc-rng200.c | 239 ++++++++++++++++++++++++++++++++++ > 3 files changed, 253 insertions(+) > create mode 100644 drivers/char/hw_random/iproc-rng200.c > > diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig > index de57b38..f48cf11 100644 > --- a/drivers/char/hw_random/Kconfig > +++ b/drivers/char/hw_random/Kconfig > @@ -101,6 +101,19 @@ config HW_RANDOM_BCM2835 > > If unsure, say Y. > > +config HW_RANDOM_IPROC_RNG200 > + tristate "Broadcom iProc RNG200 support" > + depends on ARCH_BCM_IPROC > + default HW_RANDOM > + ---help--- > + This driver provides kernel-side support for the RNG200 > + hardware found on the Broadcom iProc SoCs. > + > + To compile this driver as a module, choose M here: the > + module will be called iproc-rng200 > + > + If unsure, say Y. > + > config HW_RANDOM_GEODE > tristate "AMD Geode HW Random Number Generator support" > depends on X86_32 && PCI > diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile > index 0b4cd57..055bb01 100644 > --- a/drivers/char/hw_random/Makefile > +++ b/drivers/char/hw_random/Makefile > @@ -28,5 +28,6 @@ obj-$(CONFIG_HW_RANDOM_POWERNV) += powernv-rng.o > obj-$(CONFIG_HW_RANDOM_EXYNOS) += exynos-rng.o > obj-$(CONFIG_HW_RANDOM_TPM) += tpm-rng.o > obj-$(CONFIG_HW_RANDOM_BCM2835) += bcm2835-rng.o > +obj-$(CONFIG_HW_RANDOM_IPROC_RNG200) += iproc-rng200.o > obj-$(CONFIG_HW_RANDOM_MSM) += msm-rng.o > obj-$(CONFIG_HW_RANDOM_XGENE) += xgene-rng.o > diff --git a/drivers/char/hw_random/iproc-rng200.c b/drivers/char/hw_random/iproc-rng200.c > new file mode 100644 > index 0000000..4643aa9 > --- /dev/null > +++ b/drivers/char/hw_random/iproc-rng200.c > @@ -0,0 +1,239 @@ > +/* > +* Copyright (C) 2014 Broadcom Corporation > +* > +* This program is free software; you can redistribute it and/or > +* modify it under the terms of the GNU General Public License as > +* published by the Free Software Foundation version 2. > +* > +* This program is distributed "as is" WITHOUT ANY WARRANTY of any > +* kind, whether express or implied; without even the implied warranty > +* of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +* GNU General Public License for more details. > +*/ > +/* > + * DESCRIPTION: The Broadcom iProc RNG200 Driver > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > + > +/* Registers */ > +#define RNG_CTRL_OFFSET 0x00 > +#define RNG_CTRL_RNG_RBGEN_MASK 0x00001FFF > +#define RNG_CTRL_RNG_RBGEN_ENABLE 0x00000001 > +#define RNG_CTRL_RNG_RBGEN_DISABLE 0x00000000 > + > +#define RNG_SOFT_RESET_OFFSET 0x04 > +#define RNG_SOFT_RESET_RNG_SOFT_RESET_MASK 0x00000001 > +#define RNG_SOFT_RESET_RNG_SOFT_RESET_ACTIVE 0x00000001 > +#define RNG_SOFT_RESET_RNG_SOFT_RESET_CLEAR 0x00000000 > + > +#define RBG_SOFT_RESET_OFFSET 0x08 > +#define RBG_SOFT_RESET_RNG_SOFT_RESET_MASK 0x00000001 > +#define RBG_SOFT_RESET_RNG_SOFT_RESET_ACTIVE 0x00000001 > +#define RBG_SOFT_RESET_RNG_SOFT_RESET_CLEAR 0x00000000 > + > +#define RNG_INT_STATUS_OFFSET 0x18 > +#define RNG_INT_STATUS_MASTER_FAIL_LOCKOUT_IRQ_MASK 0x80000000 > +#define RNG_INT_STATUS_STARTUP_TRANSITIONS_MET_IRQ_MASK 0x00020000 > +#define RNG_INT_STATUS_NIST_FAIL_IRQ_MASK 0x00000020 > +#define RNG_INT_STATUS_TOTAL_BITS_COUNT_IRQ_MASK 0x00000001 > + > +#define RNG_FIFO_DATA_OFFSET 0x20 > + > +#define RNG_FIFO_COUNT_OFFSET 0x24 > +#define RNG_FIFO_COUNT_RNG_FIFO_COUNT_MASK 0x000000FF > + > +static void iproc_rng200_restart(void __iomem *rng_base) > +{ > + uint32_t val; > + > + /* Disable RBG */ > + val = ioread32(rng_base + RNG_CTRL_OFFSET); > + val &= ~RNG_CTRL_RNG_RBGEN_MASK; > + val |= RNG_CTRL_RNG_RBGEN_DISABLE; > + iowrite32(val, rng_base + RNG_CTRL_OFFSET); > + > + /* Clear all interrupt status */ > + iowrite32(0xFFFFFFFFUL, rng_base + RNG_INT_STATUS_OFFSET); > + > + /* Reset RNG and RBG */ > + val = ioread32(rng_base + RBG_SOFT_RESET_OFFSET); > + val &= ~RBG_SOFT_RESET_RNG_SOFT_RESET_MASK; > + val |= RBG_SOFT_RESET_RNG_SOFT_RESET_ACTIVE; > + iowrite32(val, rng_base + RBG_SOFT_RESET_OFFSET); > + > + val = ioread32(rng_base + RNG_SOFT_RESET_OFFSET); > + val &= ~RNG_SOFT_RESET_RNG_SOFT_RESET_MASK; > + val |= RNG_SOFT_RESET_RNG_SOFT_RESET_ACTIVE; > + iowrite32(val, rng_base + RNG_SOFT_RESET_OFFSET); > + > + val = ioread32(rng_base + RNG_SOFT_RESET_OFFSET); > + val &= ~RNG_SOFT_RESET_RNG_SOFT_RESET_MASK; > + val |= RNG_SOFT_RESET_RNG_SOFT_RESET_CLEAR; > + iowrite32(val, rng_base + RNG_SOFT_RESET_OFFSET); > + > + val = ioread32(rng_base + RBG_SOFT_RESET_OFFSET); > + val &= ~RBG_SOFT_RESET_RNG_SOFT_RESET_MASK; > + val |= RBG_SOFT_RESET_RNG_SOFT_RESET_CLEAR; > + iowrite32(val, rng_base + RBG_SOFT_RESET_OFFSET); > + > + /* Enable RBG */ > + val = ioread32(rng_base + RNG_CTRL_OFFSET); > + val &= ~RNG_CTRL_RNG_RBGEN_MASK; > + val |= RNG_CTRL_RNG_RBGEN_ENABLE; > + iowrite32(val, rng_base + RNG_CTRL_OFFSET); > +} > + > +static int iproc_rng200_read(struct hwrng *rng, void *buf, size_t max, > + bool wait) > +{ > + uint32_t status = 0; > + uint32_t num_remaining = max; > + > + #define MAX_RESETS_PER_READ 1 > + uint32_t num_resets = 0; > + > + #define MAX_IDLE_TIME (1 * HZ) > + unsigned long idle_endtime = jiffies + MAX_IDLE_TIME; > + > + /* Retrieve HW RNG registers base address. */ > + void __iomem *rng_base = (void __iomem *)rng->priv; > + > + while ((num_remaining > 0) && time_before(jiffies, idle_endtime)) { > + > + /* Is RNG sane? If not, reset it. */ > + status = ioread32(rng_base + RNG_INT_STATUS_OFFSET); > + if ((status & (RNG_INT_STATUS_MASTER_FAIL_LOCKOUT_IRQ_MASK | > + RNG_INT_STATUS_NIST_FAIL_IRQ_MASK)) != 0) { > + > + if (num_resets >= MAX_RESETS_PER_READ) > + return max - num_remaining; > + > + iproc_rng200_restart(rng_base); > + num_resets++; > + } > + > + /* Are there any random numbers available? */ > + if ((ioread32(rng_base + RNG_FIFO_COUNT_OFFSET) & > + RNG_FIFO_COUNT_RNG_FIFO_COUNT_MASK) > 0) { > + > + if (num_remaining >= sizeof(uint32_t)) { > + /* Buffer has room to store entire word */ > + *(uint32_t *)buf = ioread32(rng_base + > + RNG_FIFO_DATA_OFFSET); > + buf += sizeof(uint32_t); > + num_remaining -= sizeof(uint32_t); > + } else { > + /* Buffer can only store partial word */ > + uint32_t rnd_number = ioread32(rng_base + > + RNG_FIFO_DATA_OFFSET); > + memcpy(buf, &rnd_number, num_remaining); > + buf += num_remaining; > + num_remaining = 0; > + } > + > + /* Reset the IDLE timeout */ > + idle_endtime = jiffies + MAX_IDLE_TIME; > + } else { > + if (!wait) > + /* Cannot wait, return immediately */ > + return max - num_remaining; > + > + /* Can wait, give others chance to run */ > + cpu_relax(); > + } > + } > + > + return max - num_remaining; > +} > + > +static struct hwrng iproc_rng200_ops = { > + .name = "iproc-rng200", > + .read = iproc_rng200_read, > +}; I would prefer if we allocated a driver-private structure that wraps hwrng instead of using statically allocated singleton as I wonder how it will behave if I bind/unbind the device several times. > + > +static int iproc_rng200_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + void __iomem *rng_base = 0; > + uint32_t val = 0; > + int err = 0; There is no need to initialize all variables as it suppressed "used but uninitialized" warnings from the compiler. Imagine someone adding a new init operation and when they handle failure they forget to adjust error code so it stays 0: for upper layers we'd signal success even though our device/driver are half-bound. > + > + /* Map peripheral */ > + struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + > + if (!res) { > + dev_err(dev, "failed to get rng resources\n"); > + return -ENODEV; If we have platform device but it is missing parts of necessary data it should be -EINVAL. > + } > + > + rng_base = devm_ioremap_resource(dev, res); > + if (!rng_base) { devm_ioremap_resource() returns ERR_PTR-encoded result, you need to check with IS_ERR(). > + dev_err(dev, "failed to remap rng regs\n"); > + return -ENODEV; return PTR_ERR(rng_base); > + } There is no clock for RNG? > + > + iproc_rng200_ops.priv = (unsigned long)rng_base; > + > + /* Setup RNG. */ > + val = ioread32(rng_base + RNG_CTRL_OFFSET); > + val &= ~RNG_CTRL_RNG_RBGEN_MASK; > + val |= RNG_CTRL_RNG_RBGEN_ENABLE; > + iowrite32(val, rng_base + RNG_CTRL_OFFSET); That should be in [new] iproc_rng200_init(). > + > + /* Register driver */ > + err = hwrng_register(&iproc_rng200_ops); > + if (err) { > + dev_err(dev, "hwrng registration failed\n"); > + return err; > + } > + dev_info(dev, "hwrng registered\n"); > + > + return 0; > +} > + > +static int iproc_rng200_remove(struct platform_device *pdev) > +{ > + uint32_t val = 0; > + void __iomem *rng_base = (void __iomem *)iproc_rng200_ops.priv; > + > + /* Unregister driver */ > + hwrng_unregister(&iproc_rng200_ops); > + > + /* Disable RNG hardware */ > + val = ioread32(rng_base + RNG_CTRL_OFFSET); > + val &= ~RNG_CTRL_RNG_RBGEN_MASK; > + val |= RNG_CTRL_RNG_RBGEN_DISABLE; > + iowrite32(val, rng_base + RNG_CTRL_OFFSET); That should be in [new] iproc_rng200_cleanup(). > + > + return 0; > +} > + #ifdef CONFIG_OF maybe? > +static const struct of_device_id iproc_rng200_of_match[] = { > + { .compatible = "brcm,iproc-rng200", }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, iproc_rng200_of_match); > + > +static struct platform_driver iproc_rng200_driver = { > + .driver = { > + .name = "iproc-rng200", > + .of_match_table = iproc_rng200_of_match, > + }, > + .probe = iproc_rng200_probe, > + .remove = iproc_rng200_remove, > +}; > +module_platform_driver(iproc_rng200_driver); > + > +MODULE_AUTHOR("Broadcom"); > +MODULE_DESCRIPTION("iProc RNG200 Random Number Generator driver"); > +MODULE_LICENSE("GPL v2"); > -- > 2.3.0 > Thanks. -- Dmitry From mboxrd@z Thu Jan 1 00:00:00 1970 From: dtor@google.com (Dmitry Torokhov) Date: Wed, 25 Feb 2015 10:43:38 -0800 Subject: [PATCH v2 2/2] hwrng: iproc-rng200 - Add Broadcom IPROC RNG driver In-Reply-To: <1424888184-22180-3-git-send-email-sbranden@broadcom.com> References: <1424888184-22180-1-git-send-email-sbranden@broadcom.com> <1424888184-22180-3-git-send-email-sbranden@broadcom.com> Message-ID: <20150225184338.GA3215@dtor-ws> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Scott, On Wed, Feb 25, 2015 at 10:16:24AM -0800, Scott Branden wrote: > This adds a driver for random number generator present on Broadcom > IPROC devices. > > Reviewed-by: Ray Jui > Signed-off-by: Scott Branden > --- > drivers/char/hw_random/Kconfig | 13 ++ > drivers/char/hw_random/Makefile | 1 + > drivers/char/hw_random/iproc-rng200.c | 239 ++++++++++++++++++++++++++++++++++ > 3 files changed, 253 insertions(+) > create mode 100644 drivers/char/hw_random/iproc-rng200.c > > diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig > index de57b38..f48cf11 100644 > --- a/drivers/char/hw_random/Kconfig > +++ b/drivers/char/hw_random/Kconfig > @@ -101,6 +101,19 @@ config HW_RANDOM_BCM2835 > > If unsure, say Y. > > +config HW_RANDOM_IPROC_RNG200 > + tristate "Broadcom iProc RNG200 support" > + depends on ARCH_BCM_IPROC > + default HW_RANDOM > + ---help--- > + This driver provides kernel-side support for the RNG200 > + hardware found on the Broadcom iProc SoCs. > + > + To compile this driver as a module, choose M here: the > + module will be called iproc-rng200 > + > + If unsure, say Y. > + > config HW_RANDOM_GEODE > tristate "AMD Geode HW Random Number Generator support" > depends on X86_32 && PCI > diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile > index 0b4cd57..055bb01 100644 > --- a/drivers/char/hw_random/Makefile > +++ b/drivers/char/hw_random/Makefile > @@ -28,5 +28,6 @@ obj-$(CONFIG_HW_RANDOM_POWERNV) += powernv-rng.o > obj-$(CONFIG_HW_RANDOM_EXYNOS) += exynos-rng.o > obj-$(CONFIG_HW_RANDOM_TPM) += tpm-rng.o > obj-$(CONFIG_HW_RANDOM_BCM2835) += bcm2835-rng.o > +obj-$(CONFIG_HW_RANDOM_IPROC_RNG200) += iproc-rng200.o > obj-$(CONFIG_HW_RANDOM_MSM) += msm-rng.o > obj-$(CONFIG_HW_RANDOM_XGENE) += xgene-rng.o > diff --git a/drivers/char/hw_random/iproc-rng200.c b/drivers/char/hw_random/iproc-rng200.c > new file mode 100644 > index 0000000..4643aa9 > --- /dev/null > +++ b/drivers/char/hw_random/iproc-rng200.c > @@ -0,0 +1,239 @@ > +/* > +* Copyright (C) 2014 Broadcom Corporation > +* > +* This program is free software; you can redistribute it and/or > +* modify it under the terms of the GNU General Public License as > +* published by the Free Software Foundation version 2. > +* > +* This program is distributed "as is" WITHOUT ANY WARRANTY of any > +* kind, whether express or implied; without even the implied warranty > +* of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +* GNU General Public License for more details. > +*/ > +/* > + * DESCRIPTION: The Broadcom iProc RNG200 Driver > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > + > +/* Registers */ > +#define RNG_CTRL_OFFSET 0x00 > +#define RNG_CTRL_RNG_RBGEN_MASK 0x00001FFF > +#define RNG_CTRL_RNG_RBGEN_ENABLE 0x00000001 > +#define RNG_CTRL_RNG_RBGEN_DISABLE 0x00000000 > + > +#define RNG_SOFT_RESET_OFFSET 0x04 > +#define RNG_SOFT_RESET_RNG_SOFT_RESET_MASK 0x00000001 > +#define RNG_SOFT_RESET_RNG_SOFT_RESET_ACTIVE 0x00000001 > +#define RNG_SOFT_RESET_RNG_SOFT_RESET_CLEAR 0x00000000 > + > +#define RBG_SOFT_RESET_OFFSET 0x08 > +#define RBG_SOFT_RESET_RNG_SOFT_RESET_MASK 0x00000001 > +#define RBG_SOFT_RESET_RNG_SOFT_RESET_ACTIVE 0x00000001 > +#define RBG_SOFT_RESET_RNG_SOFT_RESET_CLEAR 0x00000000 > + > +#define RNG_INT_STATUS_OFFSET 0x18 > +#define RNG_INT_STATUS_MASTER_FAIL_LOCKOUT_IRQ_MASK 0x80000000 > +#define RNG_INT_STATUS_STARTUP_TRANSITIONS_MET_IRQ_MASK 0x00020000 > +#define RNG_INT_STATUS_NIST_FAIL_IRQ_MASK 0x00000020 > +#define RNG_INT_STATUS_TOTAL_BITS_COUNT_IRQ_MASK 0x00000001 > + > +#define RNG_FIFO_DATA_OFFSET 0x20 > + > +#define RNG_FIFO_COUNT_OFFSET 0x24 > +#define RNG_FIFO_COUNT_RNG_FIFO_COUNT_MASK 0x000000FF > + > +static void iproc_rng200_restart(void __iomem *rng_base) > +{ > + uint32_t val; > + > + /* Disable RBG */ > + val = ioread32(rng_base + RNG_CTRL_OFFSET); > + val &= ~RNG_CTRL_RNG_RBGEN_MASK; > + val |= RNG_CTRL_RNG_RBGEN_DISABLE; > + iowrite32(val, rng_base + RNG_CTRL_OFFSET); > + > + /* Clear all interrupt status */ > + iowrite32(0xFFFFFFFFUL, rng_base + RNG_INT_STATUS_OFFSET); > + > + /* Reset RNG and RBG */ > + val = ioread32(rng_base + RBG_SOFT_RESET_OFFSET); > + val &= ~RBG_SOFT_RESET_RNG_SOFT_RESET_MASK; > + val |= RBG_SOFT_RESET_RNG_SOFT_RESET_ACTIVE; > + iowrite32(val, rng_base + RBG_SOFT_RESET_OFFSET); > + > + val = ioread32(rng_base + RNG_SOFT_RESET_OFFSET); > + val &= ~RNG_SOFT_RESET_RNG_SOFT_RESET_MASK; > + val |= RNG_SOFT_RESET_RNG_SOFT_RESET_ACTIVE; > + iowrite32(val, rng_base + RNG_SOFT_RESET_OFFSET); > + > + val = ioread32(rng_base + RNG_SOFT_RESET_OFFSET); > + val &= ~RNG_SOFT_RESET_RNG_SOFT_RESET_MASK; > + val |= RNG_SOFT_RESET_RNG_SOFT_RESET_CLEAR; > + iowrite32(val, rng_base + RNG_SOFT_RESET_OFFSET); > + > + val = ioread32(rng_base + RBG_SOFT_RESET_OFFSET); > + val &= ~RBG_SOFT_RESET_RNG_SOFT_RESET_MASK; > + val |= RBG_SOFT_RESET_RNG_SOFT_RESET_CLEAR; > + iowrite32(val, rng_base + RBG_SOFT_RESET_OFFSET); > + > + /* Enable RBG */ > + val = ioread32(rng_base + RNG_CTRL_OFFSET); > + val &= ~RNG_CTRL_RNG_RBGEN_MASK; > + val |= RNG_CTRL_RNG_RBGEN_ENABLE; > + iowrite32(val, rng_base + RNG_CTRL_OFFSET); > +} > + > +static int iproc_rng200_read(struct hwrng *rng, void *buf, size_t max, > + bool wait) > +{ > + uint32_t status = 0; > + uint32_t num_remaining = max; > + > + #define MAX_RESETS_PER_READ 1 > + uint32_t num_resets = 0; > + > + #define MAX_IDLE_TIME (1 * HZ) > + unsigned long idle_endtime = jiffies + MAX_IDLE_TIME; > + > + /* Retrieve HW RNG registers base address. */ > + void __iomem *rng_base = (void __iomem *)rng->priv; > + > + while ((num_remaining > 0) && time_before(jiffies, idle_endtime)) { > + > + /* Is RNG sane? If not, reset it. */ > + status = ioread32(rng_base + RNG_INT_STATUS_OFFSET); > + if ((status & (RNG_INT_STATUS_MASTER_FAIL_LOCKOUT_IRQ_MASK | > + RNG_INT_STATUS_NIST_FAIL_IRQ_MASK)) != 0) { > + > + if (num_resets >= MAX_RESETS_PER_READ) > + return max - num_remaining; > + > + iproc_rng200_restart(rng_base); > + num_resets++; > + } > + > + /* Are there any random numbers available? */ > + if ((ioread32(rng_base + RNG_FIFO_COUNT_OFFSET) & > + RNG_FIFO_COUNT_RNG_FIFO_COUNT_MASK) > 0) { > + > + if (num_remaining >= sizeof(uint32_t)) { > + /* Buffer has room to store entire word */ > + *(uint32_t *)buf = ioread32(rng_base + > + RNG_FIFO_DATA_OFFSET); > + buf += sizeof(uint32_t); > + num_remaining -= sizeof(uint32_t); > + } else { > + /* Buffer can only store partial word */ > + uint32_t rnd_number = ioread32(rng_base + > + RNG_FIFO_DATA_OFFSET); > + memcpy(buf, &rnd_number, num_remaining); > + buf += num_remaining; > + num_remaining = 0; > + } > + > + /* Reset the IDLE timeout */ > + idle_endtime = jiffies + MAX_IDLE_TIME; > + } else { > + if (!wait) > + /* Cannot wait, return immediately */ > + return max - num_remaining; > + > + /* Can wait, give others chance to run */ > + cpu_relax(); > + } > + } > + > + return max - num_remaining; > +} > + > +static struct hwrng iproc_rng200_ops = { > + .name = "iproc-rng200", > + .read = iproc_rng200_read, > +}; I would prefer if we allocated a driver-private structure that wraps hwrng instead of using statically allocated singleton as I wonder how it will behave if I bind/unbind the device several times. > + > +static int iproc_rng200_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + void __iomem *rng_base = 0; > + uint32_t val = 0; > + int err = 0; There is no need to initialize all variables as it suppressed "used but uninitialized" warnings from the compiler. Imagine someone adding a new init operation and when they handle failure they forget to adjust error code so it stays 0: for upper layers we'd signal success even though our device/driver are half-bound. > + > + /* Map peripheral */ > + struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + > + if (!res) { > + dev_err(dev, "failed to get rng resources\n"); > + return -ENODEV; If we have platform device but it is missing parts of necessary data it should be -EINVAL. > + } > + > + rng_base = devm_ioremap_resource(dev, res); > + if (!rng_base) { devm_ioremap_resource() returns ERR_PTR-encoded result, you need to check with IS_ERR(). > + dev_err(dev, "failed to remap rng regs\n"); > + return -ENODEV; return PTR_ERR(rng_base); > + } There is no clock for RNG? > + > + iproc_rng200_ops.priv = (unsigned long)rng_base; > + > + /* Setup RNG. */ > + val = ioread32(rng_base + RNG_CTRL_OFFSET); > + val &= ~RNG_CTRL_RNG_RBGEN_MASK; > + val |= RNG_CTRL_RNG_RBGEN_ENABLE; > + iowrite32(val, rng_base + RNG_CTRL_OFFSET); That should be in [new] iproc_rng200_init(). > + > + /* Register driver */ > + err = hwrng_register(&iproc_rng200_ops); > + if (err) { > + dev_err(dev, "hwrng registration failed\n"); > + return err; > + } > + dev_info(dev, "hwrng registered\n"); > + > + return 0; > +} > + > +static int iproc_rng200_remove(struct platform_device *pdev) > +{ > + uint32_t val = 0; > + void __iomem *rng_base = (void __iomem *)iproc_rng200_ops.priv; > + > + /* Unregister driver */ > + hwrng_unregister(&iproc_rng200_ops); > + > + /* Disable RNG hardware */ > + val = ioread32(rng_base + RNG_CTRL_OFFSET); > + val &= ~RNG_CTRL_RNG_RBGEN_MASK; > + val |= RNG_CTRL_RNG_RBGEN_DISABLE; > + iowrite32(val, rng_base + RNG_CTRL_OFFSET); That should be in [new] iproc_rng200_cleanup(). > + > + return 0; > +} > + #ifdef CONFIG_OF maybe? > +static const struct of_device_id iproc_rng200_of_match[] = { > + { .compatible = "brcm,iproc-rng200", }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, iproc_rng200_of_match); > + > +static struct platform_driver iproc_rng200_driver = { > + .driver = { > + .name = "iproc-rng200", > + .of_match_table = iproc_rng200_of_match, > + }, > + .probe = iproc_rng200_probe, > + .remove = iproc_rng200_remove, > +}; > +module_platform_driver(iproc_rng200_driver); > + > +MODULE_AUTHOR("Broadcom"); > +MODULE_DESCRIPTION("iProc RNG200 Random Number Generator driver"); > +MODULE_LICENSE("GPL v2"); > -- > 2.3.0 > Thanks. -- Dmitry