| From | andy.shevchenko@gmail ... | Date | Sat, 3 Jun 2023 21:35:13 +0300 | 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@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
|