linux-kernel - Re: [PATCH 1/3] reset: Add support for the Amlogic Meson GXBB Reset Controller

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 20 May 2016 11:04:48 +0200
From:	Carlo Caione <carlo@...one.org>
To:	Neil Armstrong <narmstrong@...libre.com>
Cc:	linux-kernel@...r.kernel.org, p.zabel@...gutronix.de,
	linux-arm-kernel@...ts.infradead.org,
	linux-amlogic@...ts.infradead.org, jerry.cao@...ogic.com,
	xing.xu@...ogic.com, victor.wan@...ogic.com
Subject: Re: [PATCH 1/3] reset: Add support for the Amlogic Meson GXBB Reset
 Controller

On 20/05/16 10:27, Neil Armstrong wrote:
> This patch adds the platform driver for the Amlogic Meson GXBB Reset
> Controller.
> 
> Signed-off-by: Neil Armstrong <narmstrong@...libre.com>
> ---
>  drivers/reset/Kconfig            |   6 ++
>  drivers/reset/Makefile           |   1 +
>  drivers/reset/reset-meson-gxbb.c | 129 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 136 insertions(+)
>  create mode 100644 drivers/reset/reset-meson-gxbb.c

Do we really need to be that specific (-gxbb)? This driver looks generic
and simple enough to be used for several Amlogic families. You are
already differentiating between them with the include file defining the
reset indexes for the SoC.

> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index df37212..4ac5c4d 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -12,5 +12,11 @@ menuconfig RESET_CONTROLLER
>  
>  	  If unsure, say no.
>  
> +config MESON_GXBB_RESET
> +	tristate "Amlogic Meson GXBB Reset Driver"
> +	depends on (ARCH_MESON && RESET_CONTROLLER)
> +	help
> +	  Build the Amlogic Meson GxBB reset driver.
> +
>  source "drivers/reset/sti/Kconfig"
>  source "drivers/reset/hisilicon/Kconfig"
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index a1fc8ed..5ff83a1 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -3,6 +3,7 @@ obj-$(CONFIG_ARCH_LPC18XX) += reset-lpc18xx.o
>  obj-$(CONFIG_ARCH_SOCFPGA) += reset-socfpga.o
>  obj-$(CONFIG_ARCH_BERLIN) += reset-berlin.o
>  obj-$(CONFIG_MACH_PISTACHIO) += reset-pistachio.o
> +obj-$(CONFIG_MESON_GXBB_RESET) += reset-meson-gxbb.o

obj-$(CONFIG_ARCH_MESON) ?

[...]
> +static const struct reset_control_ops meson_gxbb_reset_ops = {
> +	.reset		= meson_gxbb_reset_reset,
> +};

nit: unfortunate name :)

Any reason to not have also .assert / .deassert?

[...]
> +static struct platform_driver meson_gxbb_reset_driver = {
> +	.probe	= meson_gxbb_reset_probe,
> +	.remove	= meson_gxbb_reset_remove,
> +	.driver = {
> +		.name		= "meson_gxbb_reset",
> +		.of_match_table	= meson_gxbb_reset_dt_ids,
> +	},
> +};
> +
> +module_platform_driver(meson_gxbb_reset_driver);

No MODULE_AUTHOR, MODULE_LICENSE, etc... ?

-- 
Carlo Caione