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

Stephen Boyd
2024-07-10T22:49:41Z

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?