Re: [PATCH v9 16/22] drm/mediatek: add ETHDR support for MT8195

[Philipp Zabel <p.zabel@pengutronix.de>]

Hi Nancy,

On Tue, 2021-11-30 at 11:35 +0800, Nancy.Lin wrote:
[...]
> +void mtk_ethdr_stop(struct device *dev)
> +{
> +	struct mtk_ethdr *priv = dev_get_drvdata(dev);
> +	struct mtk_ethdr_comp *mixer = &priv->ethdr_comp[ETHDR_MIXER];
> +
> +	writel(0, mixer->regs + MIX_EN);
> +	writel(1, mixer->regs + MIX_RST);
> +	reset_control_reset(devm_reset_control_array_get(dev, true, true));

Are these reset lines really shared with other hardware units, and more
specifically, are there other drivers that also try to control them?

If so, please use devm_reset_control_array_get_optional_shared().

Otherwise use devm_reset_control_array_get_optional_exclusive() instead.

If you really need to share the reset line with other hardware and have
to trigger it more than once, the only use case supported by the reset
framework is to use reset_control_reset() before starting the hardware
for each user and then reset_control_rearm() after stopping it for all
users (see [1]). That would both stop the reset from being triggered
while the hardware is active (before last _rearm) and allow another
reset once after all hardware is stopped.

[1] https://www.kernel.org/doc/html/latest/driver-api/reset.htm#triggering

Also, request the reset control once in the probe function, not every
time ETHDR is stopped. Otherwise you'll end up leaking devres memory

regards
Philipp