Re: [PATCH 3/3] reset: eswin: Add eic7700 HSP reset driver

[Philipp Zabel]

On Fr, 2026-04-03 at 17:36 +0800, dongxuyang@xxxxxxxxxxxxxxxxxx wrote:
> From: Xuyang Dong <dongxuyang@xxxxxxxxxxxxxxxxxx>
> 
> Add auxiliary driver to support ESWIN EIC7700 high-speed peripherals
> system. The reset controller is created using the auxiliary device
> framework and set up in the clock driver.
> 
> Signed-off-by: Xuyang Dong <dongxuyang@xxxxxxxxxxxxxxxxxx>
> ---
>  drivers/reset/Kconfig             |  13 +++
>  drivers/reset/Makefile            |   1 +
>  drivers/reset/reset-eic7700-hsp.c | 151 ++++++++++++++++++++++++++++++
>  3 files changed, 165 insertions(+)
>  create mode 100644 drivers/reset/reset-eic7700-hsp.c
> 
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 7ce151f6a7e4..50bb0cd069ba 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -83,6 +83,19 @@ config RESET_EIC7700
>  	  The driver supports eic7700 series chips and provides functionality for
>  	  asserting and deasserting resets on the chip.
>  
> +config RESET_EIC7700_HSP
> +	tristate "EIC7700 HSP Reset controller"
> +	depends on ARCH_ESWIN || COMPILE_TEST
> +	depends on COMMON_CLK_EIC7700_HSP

Why?

Please make this buildable under COMPILE_TEST without enabling the
clock driver.

[...]
> diff --git a/drivers/reset/reset-eic7700-hsp.c b/drivers/reset/reset-eic7700-hsp.c
> new file mode 100644
> index 000000000000..fe9822078bcc
> --- /dev/null
> +++ b/drivers/reset/reset-eic7700-hsp.c
> @@ -0,0 +1,151 @@
[...]
> +static int eic7700_hsp_reset_assert(struct reset_controller_dev *rcdev,
> +				    unsigned long id)
> +{
> +	struct eic7700_hsp_reset_data *data = to_eic7700_hsp_reset(rcdev);
> +	int ret;
> +
> +	if (eic7700_hsp_reset[id].active_low)
> +		ret = regmap_clear_bits(data->regmap, eic7700_hsp_reset[id].reg,
> +					eic7700_hsp_reset[id].bit);
> +	else
> +		ret = regmap_set_bits(data->regmap, eic7700_hsp_reset[id].reg,
> +				      eic7700_hsp_reset[id].bit);

This is essentially regmap_assign_bits() open-coded.

> +
> +	return ret;
> +}
> +
> +static int eic7700_hsp_reset_deassert(struct reset_controller_dev *rcdev,
> +				      unsigned long id)
> +{
> +	struct eic7700_hsp_reset_data *data = to_eic7700_hsp_reset(rcdev);
> +	int ret;
> +
> +	if (eic7700_hsp_reset[id].active_low)
> +		ret = regmap_set_bits(data->regmap, eic7700_hsp_reset[id].reg,
> +				      eic7700_hsp_reset[id].bit);
> +	else
> +		ret = regmap_clear_bits(data->regmap, eic7700_hsp_reset[id].reg,
> +					eic7700_hsp_reset[id].bit);

Same as above.

> +
> +	return ret;
> +}
> +
> +static int eic7700_hsp_reset_reset(struct reset_controller_dev *rcdev,
> +				   unsigned long id)
> +{
> +	int ret;
> +
> +	ret = eic7700_hsp_reset_assert(rcdev, id);
> +	if (ret)
> +		return ret;
> +
> +	usleep_range(10, 15);
> +
> +	return eic7700_hsp_reset_deassert(rcdev, id);
> +}

Does any of the consumer drivers (SATA, USB) actually use
reset_control_reset()? If not, don't implement this.

> +
> +static const struct reset_control_ops eic7700_hsp_reset_ops = {
> +	.reset = eic7700_hsp_reset_reset,
> +	.assert = eic7700_hsp_reset_assert,
> +	.deassert = eic7700_hsp_reset_deassert,
> +};
> +
> +static int eic7700_hsp_reset_probe(struct auxiliary_device *adev,
> +				   const struct auxiliary_device_id *id)
> +{
> +	struct eic7700_hsp_reset_data *data;
> +	struct device *dev = &adev->dev;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->regmap = devm_regmap_init_mmio
> +			(dev, (__force void __iomem *)adev->dev.platform_data,

Consider letting the parent clk driver create the regmap und using
dev_get_regmap().

> +			&eic7700_hsp_regmap_config);
> +	if (IS_ERR(data->regmap))
> +		return dev_err_probe(dev, PTR_ERR(data->regmap),
> +				     "failed to get regmap!\n");
> +
> +	data->rcdev.owner = THIS_MODULE;
> +	data->rcdev.ops = &eic7700_hsp_reset_ops;
> +	data->rcdev.of_node = dev->parent->of_node;
> +	data->rcdev.of_reset_n_cells = 1;

No need to set of_reset_n_cells, this value is ignored if of_xlate
isn't also set.

regards
Philipp