Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   andy.shevchenko@gmail.com
Date:   Sat, 3 Jun 2023 21:35:13 +0300
To:     Nikita Shubin <nikita.shubin@maquefel.me>
Cc:     Alexander Sverdlin <alexander.sverdlin@gmail.com>,
        Arnd Bergmann <arnd@arndb.de>,
        Linus Walleij <linus.walleij@linaro.org>,
        Joel Stanley <joel@jms.id.au>,
        Conor Dooley <conor.dooley@microchip.com>,
        Heiko Stuebner <heiko@sntech.de>,
        Yinbo Zhu <zhuyinbo@loongson.cn>,
        Hitomi Hasegawa <hasegawa-hitomi@fujitsu.com>,
        Jonathan =?iso-8859-1?Q?Neusch=E4fer?= <j.neuschaefer@gmx.net>,
        Walker Chen <walker.chen@starfivetech.com>,
        Paul Menzel <pmenzel@molgen.mpg.de>,
        Emil Renner Berthing <kernel@esmil.dk>,
        Alexander Gordeev <agordeev@linux.ibm.com>,
        Vasily Gorbik <gor@linux.ibm.com>,
        Michael Peters <mpeters@embeddedts.com>,
        Kris Bahnsen <kris@embeddedts.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 03/43] soc: Add SoC driver for Cirrus ep93xx
Message-ID: <ZHuH4U7yGbTvS5km@surfacebook>
References: <20230424123522.18302-1-nikita.shubin@maquefel.me>
 <20230601053546.9574-4-nikita.shubin@maquefel.me>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230601053546.9574-4-nikita.shubin@maquefel.me>
Precedence: bulk

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@maquefel.me>
> + *
> + * Based on a rewrite of arch/arm/mach-ep93xx/core.c
> + * Copyright (C) 2006 Lennert Buytenhek <buytenh@wantstofly.org>
> + * Copyright (C) 2007 Herbert Valerio Riedel <hvr@gnu.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