linux-kernel - Re: [PATCH 4/4] 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]
Message-ID: <11055a03e605f9134f91af3e3f3a6c58.sboyd@kernel.org>
Date: Thu, 11 Apr 2024 02:26:31 -0700
From: Stephen Boyd <sboyd@...nel.org>
To: Alexander Sverdlin <alexander.sverdlin@...il.com>, Conor Dooley <conor+dt@...nel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Michael Turquette <mturquette@...libre.com>, Nikita Shubin via B4 Relay <devnull+nikita.shubin.maquefel.me@...nel.org>, Rob Herring <robh@...nel.org>, nikita.shubin@...uefel.me
Cc: linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org, devicetree@...r.kernel.org, Nikita Shubin <nikita.shubin@...uefel.me>, Arnd Bergmann <arnd@...db.de>, Linus Walleij <linus.walleij@...aro.org>
Subject: Re: [PATCH 4/4] soc: Add SoC driver for Cirrus ep93xx

Quoting Nikita Shubin via B4 Relay (2024-04-08 01:09:56)
> diff --git a/drivers/soc/cirrus/soc-ep93xx.c b/drivers/soc/cirrus/soc-ep93xx.c
> new file mode 100644
> index 000000000000..044f17f9ba55
> --- /dev/null
> +++ b/drivers/soc/cirrus/soc-ep93xx.c
> @@ -0,0 +1,240 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * SoC driver for Cirrus EP93xx chips.
> + * 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.
> + */
> +
> +#include <linux/bits.h>
> +#include <linux/cleanup.h>
> +#include <linux/init.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of.h>
> +#include <linux/of_fdt.h>

Are these of includes used?

> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/sys_soc.h>
> +
> +#include <linux/soc/cirrus/ep93xx.h>
> +
> +#define EP93XX_SYSCON_DEVCFG           0x80
> +
> +#define EP93XX_SWLOCK_MAGICK           0xaa
> +#define EP93XX_SYSCON_SWLOCK           0xc0
> +#define EP93XX_SYSCON_SYSCFG           0x9c
> +#define EP93XX_SYSCON_SYSCFG_REV_MASK  GENMASK(31, 28)
> +#define EP93XX_SYSCON_SYSCFG_REV_SHIFT 28
> +
[...]
> +
> +static void ep93xx_unregister_adev(void *_adev)
> +{
> +       struct auxiliary_device *adev = _adev;
> +
> +       auxiliary_device_delete(adev);
> +       auxiliary_device_uninit(adev);
> +}

It really seems like auxiliary bus code should expose a
simple_unregister_adev() function that does this two line function once
instead of every driver repeating it.

> +
> +static void ep93xx_adev_release(struct device *dev)
> +{
> +       struct auxiliary_device *adev = to_auxiliary_dev(dev);
> +       struct ep93xx_regmap_adev *rdev = to_ep93xx_regmap_adev(adev);
> +
> +       kfree(rdev);
> +}
> +
> +static struct auxiliary_device *ep93xx_adev_alloc(struct device *parent, const char *name,

__init?

> +                                                 struct ep93xx_map_info *info)
> +{
> +       struct ep93xx_regmap_adev *rdev __free(kfree) = NULL;
> +       struct auxiliary_device *adev;
> +       int ret;
> +
> +       rdev = kzalloc(sizeof(*rdev), GFP_KERNEL);
> +       if (!rdev)
> +               return ERR_PTR(-ENOMEM);
> +
> +       rdev->map = info->map;
> +       rdev->base = info->base;
> +       rdev->lock = &info->lock;
> +       rdev->write = ep93xx_regmap_write;
> +       rdev->update_bits = ep93xx_regmap_update_bits;
> +
> +       adev = &rdev->adev;
> +       adev->name = name;
> +       adev->dev.parent = parent;
> +       adev->dev.release = ep93xx_adev_release;
> +
> +       ret = auxiliary_device_init(adev);
> +       if (ret)
> +               return ERR_PTR(ret);
> +
> +       return &no_free_ptr(rdev)->adev;
> +}
> +
> +static int ep93xx_controller_register(struct device *parent, const char *name,

__init?

> +                                     struct ep93xx_map_info *info)
> +{
> +       struct auxiliary_device *adev;
> +       int ret;
> +
> +       adev = ep93xx_adev_alloc(parent, name, info);
> +       if (IS_ERR(adev))
> +               return PTR_ERR(adev);
> +
> +       ret = auxiliary_device_add(adev);
> +       if (ret) {
> +               auxiliary_device_uninit(adev);
> +               return ret;
> +       }
> +
> +       return devm_add_action_or_reset(parent, ep93xx_unregister_adev, adev);
> +}
> +
[...]
> +
> +static const char __init *ep93xx_get_soc_rev(struct regmap *map)
> +{
> +       switch (ep93xx_soc_revision(map)) {
> +       case EP93XX_CHIP_REV_D0:
> +               return "D0";
> +       case EP93XX_CHIP_REV_D1:
> +               return "D1";
> +       case EP93XX_CHIP_REV_E0:
> +               return "E0";
> +       case EP93XX_CHIP_REV_E1:
> +               return "E1";
> +       case EP93XX_CHIP_REV_E2:
> +               return "E2";
> +       default:
> +               return "unknown";
> +       }
> +}
> +
> +const char *pinctrl_names[] = {

static? Can also be __initconst? or moved into probe scope and placed on
the stack?

> +       "pinctrl-ep9301",       /* EP93XX_9301_SOC */
> +       "pinctrl-ep9307",       /* EP93XX_9307_SOC */
> +       "pinctrl-ep9312",       /* EP93XX_9312_SOC */
> +};