Re: [PATCH v1 03/43] soc: Add SoC driver for Cirrus ep93xx
From: andy . shevchenko
Date: Sat Jun 03 2023 - 14:35:25 EST
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@xxxxxxxxxxx>
> + *
> + * Based on a rewrite of arch/arm/mach-ep93xx/core.c
> + * Copyright (C) 2006 Lennert Buytenhek <buytenh@xxxxxxxxxxxxxx>
> + * Copyright (C) 2007 Herbert Valerio Riedel <hvr@xxxxxxx>
> + *
> + * 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