linux-kernel - Re: [PATCH v1 03/43] soc: Add SoC driver for Cirrus ep93xx

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:   Sat, 3 Jun 2023 21:35:13 +0300
From:   andy.shevchenko@...il.com
To:     Nikita Shubin <nikita.shubin@...uefel.me>
Cc:     Alexander Sverdlin <alexander.sverdlin@...il.com>,
        Arnd Bergmann <arnd@...db.de>,
        Linus Walleij <linus.walleij@...aro.org>,
        Joel Stanley <joel@....id.au>,
        Conor Dooley <conor.dooley@...rochip.com>,
        Heiko Stuebner <heiko@...ech.de>,
        Yinbo Zhu <zhuyinbo@...ngson.cn>,
        Hitomi Hasegawa <hasegawa-hitomi@...itsu.com>,
        Jonathan Neuschäfer <j.neuschaefer@....net>,
        Walker Chen <walker.chen@...rfivetech.com>,
        Paul Menzel <pmenzel@...gen.mpg.de>,
        Emil Renner Berthing <kernel@...il.dk>,
        Alexander Gordeev <agordeev@...ux.ibm.com>,
        Vasily Gorbik <gor@...ux.ibm.com>,
        Michael Peters <mpeters@...eddedts.com>,
        Kris Bahnsen <kris@...eddedts.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 03/43] soc: Add SoC driver for Cirrus ep93xx

Thu, Jun 01, 2023 at 08:33:54AM +0300, Nikita Shubin kirjoitti:
> This adds an SoC driver for the ep93xx. Currently there
> is only one thing not fitting into any other framework,
> and that is the swlock setting.
> 
> It's used for clock settings and restart.

...

>  source "drivers/soc/ux500/Kconfig"
>  source "drivers/soc/versatile/Kconfig"
>  source "drivers/soc/xilinx/Kconfig"
> +source "drivers/soc/cirrus/Kconfig"

Why not ordered?

...

>  obj-$(CONFIG_ARCH_U8500)	+= ux500/
>  obj-$(CONFIG_PLAT_VERSATILE)	+= versatile/
>  obj-y				+= xilinx/
> +obj-$(CONFIG_EP93XX_SOC)	+= cirrus/

Ditto.

...

> +/*
> + * Soc driver for Cirrus EP93xx chips.

SoC

> + * Copyright (C) 2022 Nikita Shubin <nikita.shubin@...uefel.me>
> + *
> + * Based on a rewrite of arch/arm/mach-ep93xx/core.c
> + * Copyright (C) 2006 Lennert Buytenhek <buytenh@...tstofly.org>
> + * Copyright (C) 2007 Herbert Valerio Riedel <hvr@....org>
> + *
> + * Thanks go to Michael Burian and Ray Lehtiniemi for their key
> + * role in the ep93xx linux community

Linux
community.

> + */

...

> +#include <linux/soc/cirrus/ep93xx.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>
> +#include <linux/of.h>

Can this be ordered?

...

> +#define EP93XX_SYSCON_SYSCFG_REV_MASK	(0xf0000000)

GENMASK() ?
(will need bits.h)

> +#define EP93XX_SYSCON_SYSCFG_REV_SHIFT	(28)

Here and above, do you need parentheses?

...

> +static struct regmap *map;

Global?!

...

> +EXPORT_SYMBOL_GPL(ep93xx_syscon_swlocked_write);

Can it (and other exported symbols) be exported with a namespace?

...

> +/**
> + * ep93xx_chip_revision() - returns the EP93xx chip revision

> + *

Redundant (?) blank line, but...

kernel doc validation will complain here a lot. Either drop kernel doc style or
fill it correctly.

> + */

...

> +static int __init ep93xx_soc_init(void)
> +{
> +	/* Multiplatform guard, only proceed on ep93xx */
> +	if (!of_machine_is_compatible("cirrus,ep9301"))
> +		return 0;
> +
> +	map = syscon_regmap_lookup_by_compatible("cirrus,ep9301-syscon");
> +	if (IS_ERR(map))
> +		return PTR_ERR(map);
> +
> +	pr_info("EP93xx SoC revision %s\n", ep93xx_get_soc_rev());
> +
> +	return 0;
> +}

> +

Unneeded blank line.

> +core_initcall(ep93xx_soc_init);
> +

Trailing blank line.

-- 
With Best Regards,
Andy Shevchenko