Re: [PATCH 4/4] soc: Add SoC driver for Cirrus ep93xx

[Stephen Boyd]

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@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.
> + */
> +
> +#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 */
> +};