26 Feb
2020
26 Feb
'20
9:18 p.m.
On 2/26/20 8:32 PM, Sughosh Ganu wrote:
On Wed, 26 Feb 2020 at 23:34, Heinrich Schuchardt <xypron.glpk@gmx.de mailto:xypron.glpk@gmx.de> wrote:
On 2/26/20 11:51 AM, Sughosh Ganu wrote: > > On Tue, 25 Feb 2020 at 03:56, Heinrich Schuchardt <xypron.glpk@gmx.de <mailto:xypron.glpk@gmx.de> > <mailto:xypron.glpk@gmx.de <mailto:xypron.glpk@gmx.de>>> wrote: > > Add support for the hardware random number generator of Amlogic SOCs. > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de <mailto:xypron.glpk@gmx.de> > <mailto:xypron.glpk@gmx.de <mailto:xypron.glpk@gmx.de>>> > --- > drivers/rng/Kconfig | 8 +++ > drivers/rng/Makefile | 1 + > drivers/rng/meson-rng.c | 120 ++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 129 insertions(+) > create mode 100644 drivers/rng/meson-rng.c > > diff --git a/drivers/rng/Kconfig b/drivers/rng/Kconfig > index c1aa43b823..edb6152bb9 100644 > --- a/drivers/rng/Kconfig > +++ b/drivers/rng/Kconfig > @@ -8,6 +8,14 @@ config DM_RNG > > if DM_RNG > > +config RNG_MESON > + bool "Amlogic Meson Random Number Generator support" > + depends on ARCH_MESON > + default y > + help > + Enable support for hardware random number generator > + of Amlogic Meson SoCs. > + > config RNG_SANDBOX > bool "Sandbox random number generator" > depends on SANDBOX > diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile > index 3517005541..6a8a66779b 100644 > --- a/drivers/rng/Makefile > +++ b/drivers/rng/Makefile > @@ -4,5 +4,6 @@ > # > > obj-$(CONFIG_DM_RNG) += rng-uclass.o > +obj-$(CONFIG_RNG_MESON) += meson-rng.o > obj-$(CONFIG_RNG_SANDBOX) += sandbox_rng.o > obj-$(CONFIG_RNG_STM32MP1) += stm32mp1_rng.o > diff --git a/drivers/rng/meson-rng.c b/drivers/rng/meson-rng.c > new file mode 100644 > index 0000000000..4b81a62353 > --- /dev/null > +++ b/drivers/rng/meson-rng.c > @@ -0,0 +1,120 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright 2020, Heinrich Schuchardt <xypron.glpk@gmx.de <mailto:xypron.glpk@gmx.de> > <mailto:xypron.glpk@gmx.de <mailto:xypron.glpk@gmx.de>>> > + * > + * Driver for Amlogic hardware random number generator > + */ > + > +#include <common.h> > +#include <clk.h> > +#include <dm.h> > +#include <rng.h> > +#include <asm/io.h> > + > +struct meson_rng_platdata { > + fdt_addr_t base; > + struct clk clk; > +}; > + > +/** > + * meson_rng_read() - fill buffer with random bytes > + * > + * @buffer: buffer to receive data > + * @size: size of buffer > + * > + * Return: 0 > + */ > +static int meson_rng_read(struct udevice *dev, void *data, size_t len) > +{ > + struct meson_rng_platdata *pdata = dev_get_platdata(dev); > + char *buffer = (char *)data; > + > + while (len) { > + u32 rand = readl(pdata->base); > + size_t step; > > > I believe declaration of variables in the middle of the function is > frowned upon. Can be moved to the start of the function. rand is defined at the top of the smallest possible code block delineated by braces. C compilers before C99 required you to put definitions at the top of functions. This led to the unsafe coding practice to reuse the same variable for different purposes in the same function. Currently we are using -std=gnu11. U-Boot uses cppcheck in Gitlab CI. Cppcheck gives you a warning if you could reduce the scope of a variable. Neither the U-Boot coding style guide https://www.denx.de/wiki/U-Boot/CodingStyle nor the Linux kernel coding style guide https://www.kernel.org/doc/html/v4.10/process/coding-style.html recommend such unsafe practice.Yes, I had checked that this is not mentioned in the coding style guides. But i have seen Wolfgang mention this on multiple occasions as part of his review comments earlier, one example being [1].
https://lists.denx.de/pipermail/u-boot/2020-January/397651.html has a variable definition which is not at the start of code block delineated by {} but in the middle of the code block. I agree with Wolfgang that this is not advisable.
Best regards
Heinrich
If you look at other parts of the U-Boot code, you will find that in lots of places variables are defined in the smallest possible code block (e.g. fs/fat/fat.c, fs/ext4/ext4_common.c). Due to our legacy we still carry a lot of code lines not following this rule. But anyway if you still want it in a different way, I will resubmit.I don't have a strong opinion on this. I was simply stating what I thought to be a coding style in u-boot based on some earlier review comments that I have seen.
-sughosh
[1] - https://lists.denx.de/pipermail/u-boot/2020-January/397651.html