Re: [PATCH 7/8] reset: amlogic: add auxiliary reset driver support

[Stephen Boyd <sboyd@kernel.org>]

Quoting Jerome Brunet (2024-07-10 09:25:16)
> diff --git a/drivers/reset/reset-meson.c b/drivers/reset/reset-meson.c
> index e34a10b15593..5cc767d50e8f 100644
> --- a/drivers/reset/reset-meson.c
> +++ b/drivers/reset/reset-meson.c
[...]
> +
> +int devm_meson_rst_aux_register(struct device *dev,
> +                               struct regmap *map,
> +                               const char *adev_name)
> +{
> +       struct meson_reset_adev *raux;
> +       struct auxiliary_device *adev;
> +       int ret;
> +
> +       raux = kzalloc(sizeof(*raux), GFP_KERNEL);
> +       if (!raux)
> +               return -ENOMEM;
> +
> +       ret = ida_alloc(&meson_rst_aux_ida, GFP_KERNEL);

Do we expect more than one device with the same name? I wonder if the
IDA can be skipped.

> +       if (ret < 0)
> +               goto raux_free;
> +
> +       raux->map = map;
> +
> +       adev = &raux->adev;
> +       adev->id = ret;
> +       adev->name = adev_name;
> +       adev->dev.parent = dev;
> +       adev->dev.release = meson_rst_aux_release;
> +       device_set_of_node_from_dev(&adev->dev, dev);
> +
> +       ret = auxiliary_device_init(adev);
> +       if (ret)
> +               goto ida_free;
> +
> +       ret = __auxiliary_device_add(adev, dev->driver->name);
> +       if (ret) {
> +               auxiliary_device_uninit(adev);
> +               return ret;
> +       }
> +
> +       return devm_add_action_or_reset(dev, meson_rst_aux_unregister_adev,
> +                                       adev);
> +
> +ida_free:
> +       ida_free(&meson_rst_aux_ida, adev->id);
> +raux_free:
> +       kfree(raux);
> +       return ret;
> +

Nitpick: Drop extra newline?

> +}
> +EXPORT_SYMBOL_GPL(devm_meson_rst_aux_register);
> +
> +MODULE_DESCRIPTION("Amlogic Meson Reset driver");
>  MODULE_AUTHOR("Neil Armstrong <narmstrong@baylibre.com>");
> +MODULE_AUTHOR("Jerome Brunet <jbrunet@baylibre.com>");
>  MODULE_LICENSE("Dual BSD/GPL");
> diff --git a/include/soc/amlogic/meson-auxiliary-reset.h b/include/soc/amlogic/meson-auxiliary-reset.h
> new file mode 100644
> index 000000000000..8fdb02b18d8c
> --- /dev/null
> +++ b/include/soc/amlogic/meson-auxiliary-reset.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __SOC_AMLOGIC_MESON_AUX_RESET_H
> +#define __SOC_AMLOGIC_MESON_AUX_RESET_H
> +
> +#include <linux/err.h>
> +
> +struct device;
> +struct regmap;
> +
> +#ifdef CONFIG_RESET_MESON
> +int devm_meson_rst_aux_register(struct device *dev,
> +                               struct regmap *map,
> +                               const char *adev_name);
> +#else
> +static inline int devm_meson_rst_aux_register(struct device *dev,
> +                                             struct regmap *map,
> +                                             const char *adev_name)
> +{
> +       return -EOPNOTSUPP;

Shouldn't this be 'return 0' so that the clk driver doesn't have to care
about the config?