lkml.org 
[lkml]   [2023]   [Jun]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [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


    \
     
     \ /
      Last update: 2023-06-03 20:35    [W:4.242 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site