Hi Philipp,
Thanks for the review.
On Tue, 2021-11-30 at 10:46 +0100, Philipp Zabel wrote:
> 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://urldefense.com/v3/__https://www.kernel.org/doc/html/latest/driver-api/reset.htm*triggering__;Iw!!CTRNKA9wMg0ARbw!zETfuA55cH_3uiTFEknFoWXKkApsSKFbemms9qLwluMdylp-FVK3jn5E9Ld1ZCwv$
>
>
> 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
These reset lines doesn't share with other hardware units. I will use
devm_reset_control_array_get_optional_exclusive instead and and reqest
reset control in the probe function.
Regards,
Nancy
