Re: [PATCH RFC v5 11/12] reset: zte: Add a zx297520v3 reset driver

[Philipp Zabel]

On So, 2026-06-28 at 22:59 +0300, Stefan Dösinger wrote:
> This drives the MFD child devices created by the zx297520v3-crm driver
> as well as the aux device created by the zx297520v3-lspclk driver.
> 
> Signed-off-by: Stefan Dösinger <stefandoesinger@xxxxxxxxx>
> 
> ---
> 
> v5:
> Make top and matrix MFD children instead of aux devices

Why? What is the difference between top/matrix and lsp here?

Couldn't you just keep all three as aux devices and remove the
platform_device boilerplate half of the driver?


[...]
> diff --git a/drivers/reset/reset-zte-zx297520v3.c b/drivers/reset/reset-zte-zx297520v3.c
> new file mode 100644
> index 000000000000..8ef434904230
> --- /dev/null
> +++ b/drivers/reset/reset-zte-zx297520v3.c
> @@ -0,0 +1,274 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2026 Stefan Dösinger
> + */
> +#include <dt-bindings/reset/zte,zx297520v3-reset.h>
> +#include <linux/reset-controller.h>
> +#include <linux/platform_device.h>
> +#include <linux/auxiliary_bus.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>
> +#include <linux/iopoll.h>

Looks like this is not needed anymore.

> +/* Most devices on the zx297520v3 SoC have one reset bit per clock line. As a rule of thumb, the
> + * lower bit disconnects the device from the bus, similarly to turning off PCLK - registers read 0
> + * or hang indefinitely. Unlike PCLK, this reset may have a lingering effect after deasserting.
> + * E.g. timers will be disabled, but retain their counter value.
> + *
> + * The other bit resets the actual device registers.
> + *
> + * For some devices, e.g. GMAC, the reset bits behave in the same way: They disconnect the device
> + * and registers will have their default state after deasserting. For devices that have both reset
> + * bits, both need to be deasserted for the device to function.
> + */
> +struct zte_reset_reg {
> +	u32 mask;
> +	u16 reg;
> +};
> +
> +struct zte_reset_info {
> +	const struct zte_reset_reg *resets;
> +	unsigned int num;
> +};
> +
> +struct zte_reset {
> +	struct reset_controller_dev rcdev;
> +	struct regmap *map;
> +	const struct zte_reset_reg *resets;
> +};
> +
> +static inline struct zte_reset *to_zte_reset(struct reset_controller_dev *rcdev)
> +{
> +	return container_of(rcdev, struct zte_reset, rcdev);
> +}
> +
> +static int zx29_rst_assert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct zte_reset *rst = to_zte_reset(rcdev);
> +
> +	return regmap_clear_bits(rst->map, rst->resets[id].reg, rst->resets[id].mask);
> +}
> +
> +static int zx29_rst_deassert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct zte_reset *rst = to_zte_reset(rcdev);
> +
> +	return regmap_set_bits(rst->map, rst->resets[id].reg, rst->resets[id].mask);
> +}
> +
> +static int zx29_rst_status(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct zte_reset *rst = to_zte_reset(rcdev);
> +	int res;
> +
> +	res = regmap_test_bits(rst->map, rst->resets[id].reg, rst->resets[id].mask);

The correct thing to do here would be to only check the reset bit.

This happens to work anyway because we always set reset and isolation
bits together, but maybe this warrants a comment.

I assume the registers just read back the value that was set.

> +	if (res < 0)
> +		return res;
> +
> +	return !res;
> +}
> +
[...]
> +static int reset_zx297520v3_common_probe(struct device *dev,
> +					 struct device_node *of_node,
> +					 const struct zte_reset_info *drv_info)
> +{
> +	struct zte_reset *rst;
> +
> +	rst = devm_kzalloc(dev, sizeof(*rst), GFP_KERNEL);
> +	if (!rst)
> +		return -ENOMEM;
> +
> +	rst->resets = drv_info->resets;
> +	rst->rcdev.owner = THIS_MODULE;
> +	rst->rcdev.nr_resets = drv_info->num;
> +	rst->rcdev.ops = &zx29_rst_ops;
> +	rst->rcdev.of_node = of_node;
> +	rst->rcdev.dev = dev;
> +
> +	rst->map = device_node_to_regmap(of_node);
> +	if (IS_ERR(rst->map))
> +		return dev_err_probe(dev, PTR_ERR(rst->map), "Cannot get parent syscon regmap\n");
> +
> +	return devm_reset_controller_register(dev, &rst->rcdev);
> +

Unnecessary blank line.

> +}
> +
> +static int reset_zx297520v3_aux_probe(struct auxiliary_device *adev,
> +				      const struct auxiliary_device_id *id)
> +{
> +	return reset_zx297520v3_common_probe(&adev->dev, adev->dev.of_node,
> +					     (const struct zte_reset_info *)id->driver_data);
> +}
> +
> +static int reset_zx297520v3_top_probe(struct platform_device *pdev)
> +{
> +	return reset_zx297520v3_common_probe(&pdev->dev, pdev->dev.parent->of_node,
> +					     &zx297520v3_top_info);
> +}
> +
> +static struct platform_driver reset_zx297520v3_top = {
> +	.probe = reset_zx297520v3_top_probe,
> +	.driver = {
> +		.name = "zx297520v3-toprst",
> +	},
> +};
> +
> +static int reset_zx297520v3_matrix_probe(struct platform_device *pdev)
> +{
> +	return reset_zx297520v3_common_probe(&pdev->dev, pdev->dev.parent->of_node,
> +					     &zx297520v3_matrix_info);
> +}
> +
> +static struct platform_driver reset_zx297520v3_matrix = {
> +	.probe = reset_zx297520v3_matrix_probe,
> +	.driver = {
> +		.name = "zx297520v3-matrixrst",
> +	},
> +};
> +
> +static const struct auxiliary_device_id reset_zx297520v3_ids[] = {
> +	{
> +		.name = "clk_zte.zx297520v3_lsprst",
> +		.driver_data = (kernel_ulong_t)&zx297520v3_lsp_info,
> +	},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(auxiliary, reset_zx297520v3_ids);
> +
> +static struct auxiliary_driver reset_zx297520v3_auxdrv = {
> +	.name = "zx297520v3_lsp_reset",
> +	.id_table = reset_zx297520v3_ids,
> +	.probe = reset_zx297520v3_aux_probe,
> +};
> +
> +static struct platform_driver * const reset_zx297520v3_mfddrv[] = {
> +	&reset_zx297520v3_top,
> +	&reset_zx297520v3_matrix,
> +};
> +
> +static int __init reset_zx297520v3_init(void)
> +{
> +	int res;
> +
> +	res = auxiliary_driver_register(&reset_zx297520v3_auxdrv);
> +	if (res)
> +		return res;
> +
> +	res = platform_register_drivers(reset_zx297520v3_mfddrv,
> +					ARRAY_SIZE(reset_zx297520v3_mfddrv));
> +	if (res)
> +		auxiliary_driver_unregister(&reset_zx297520v3_auxdrv);
> +
> +	return res;
> +}
> +
> +static void __exit reset_zx297520v3_exit(void)
> +{
> +	platform_unregister_drivers(reset_zx297520v3_mfddrv,
> +				    ARRAY_SIZE(reset_zx297520v3_mfddrv));
> +	auxiliary_driver_unregister(&reset_zx297520v3_auxdrv);
> +}
> +
> +module_init(reset_zx297520v3_init);
> +module_exit(reset_zx297520v3_exit);

That's too much boilerplate given I don't understand the benefit of
using platform_device for top/matrix resets yet.

regards
Philipp