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

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: <88d1dbd92e922ad002367d8dac67d0eb.sboyd@kernel.org>
Date: Wed, 10 Jul 2024 15:49:38 -0700
From: Stephen Boyd <sboyd@...nel.org>
To: Jerome Brunet <jbrunet@...libre.com>, Neil Armstrong <neil.armstrong@...aro.org>, Philipp Zabel <p.zabel@...gutronix.de>
Cc: Jerome Brunet <jbrunet@...libre.com>, Jan Dakinevich <jan.dakinevich@...utedevices.com>, linux-kernel@...r.kernel.org, linux-amlogic@...ts.infradead.org, linux-clk@...r.kernel.org
Subject: Re: [PATCH 7/8] reset: amlogic: add auxiliary reset driver support

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@...libre.com>");
> +MODULE_AUTHOR("Jerome Brunet <jbrunet@...libre.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?