Re: [PATCH v2 07/13] reset: add Synaptics SL2610 reset controller support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] reset: sl2610: wrong errno in sl2610_reset_assert() and
  deassert()
--

commit fd9868407ed20a422bf28d62579da17aa23dc9fc
Author: Jisheng Zhang <jszhang@kernel.org>

reset: add Synaptics SL2610 reset controller support

This commit adds a reset controller driver for the SoC and MCU system
control blocks on Synaptics SL2610 SoCs, supporting both pulse and sticky
reset lines.

> diff --git a/drivers/reset/reset-sl2610.c b/drivers/reset/reset-sl2610.c
> new file mode 100644
> index 0000000000000..1ca58d5a3e713
> --- /dev/null
> +++ b/drivers/reset/reset-sl2610.c
[ ... ]
> +static int sl2610_reset_assert(struct reset_controller_dev *rcdev,
> +			       unsigned long id)
> +{
> +	struct sl2610_reset_priv *priv = to_sl2610_reset_priv(rcdev);
> +	const struct sl2610_reset_cfg *cfg = &priv->cfgs[id];
> +
> +	if (!cfg->sticky)
> +		return -EINVAL;
> +
> +	return regmap_clear_bits(priv->regmap, cfg->reg, cfg->bit);
> +}
> +
> +static int sl2610_reset_deassert(struct reset_controller_dev *rcdev,
> +				 unsigned long id)
> +{
> +	struct sl2610_reset_priv *priv = to_sl2610_reset_priv(rcdev);
> +	const struct sl2610_reset_cfg *cfg = &priv->cfgs[id];
> +
> +	if (!cfg->sticky)
> +		return -EINVAL;
> +
> +	return regmap_set_bits(priv->regmap, cfg->reg, cfg->bit);
> +}

[Severity: Medium]
Should sl2610_reset_assert() and sl2610_reset_deassert() return -ENOTSUPP
(or 0 in sl2610_reset_deassert()) instead of -EINVAL when !cfg->sticky?

In drivers/reset/core.c, reset_control_deassert() treats self-deasserting
pulse reset lines as deasserted by default when .deassert() is not
implemented, and documents that .deassert() should return -ENOTSUPP when
deassertion cannot be guaranteed:

drivers/reset/core.c:reset_control_deassert() {
	...
	/*
	 * If the reset controller does not implement .deassert(), we assume
	 * that it handles self-deasserting reset lines via .reset(). In that
	 * case, the reset lines are deasserted by default. If that is not the
	 * case, the reset controller driver should implement .deassert() and
	 * return -ENOTSUPP.
	 */
	if (!rcdev->ops->deassert)
		return 0;
	...
}

Because this controller implements .assert() and .deassert() for its
sticky lines, returning -EINVAL for the 22 pulse reset lines in
sl2610_reset_cfgs[] causes consumer drivers that call
reset_control_deassert() during probe (such as 8250_dw) to fail probe, and
prevents consumers such as stmmac_dvr_probe() from checking for -ENOTSUPP
on reset_control_assert() and falling back to reset_control_reset().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261115102401.3520-1-jszhang@kernel.org?part=7