| Message ID | 20240219-realtek-reset-v4-0-858b82a29503@gmail.com |
|---|---|
| Headers | show |
| Series | net: dsa: realtek: support reset controller and update docs | expand |
On Mon, Feb 19, 2024 at 08:44:42PM -0300, Luiz Angelo Daros de Luca wrote: > +void rtl83xx_reset_assert(struct realtek_priv *priv) > +{ > + int ret; > + > + ret = reset_control_assert(priv->reset_ctl); > + if (!ret) > + return; If priv->reset_ctl is NULL - i.e. if no DT property is specified - then this will always return early and the GPIO will not be asserted. > + > + dev_warn(priv->dev, > + "Failed to assert the switch reset control: %pe\n", > + ERR_PTR(ret)); You only log an error if the reset controller assert fails, but not if the GPIO assert fails. Why the unequal treatment? I suggest keeping it simple: void rtl83xx_reset_assert(struct realtek_priv *priv) { int ret; ret = reset_control_assert(priv->reset_ctl); if (ret) dev_warn(priv->dev, "failed to assert reset control: %d\n", ret); ret = gpiod_set_value(priv->reset, false); if (ret) dev_warn(priv->dev, "failed to assert reset GPIO: %d\n", ret); } or even drop the warnings altogether. > + > + gpiod_set_value(priv->reset, true); > +} > + > +void rtl83xx_reset_deassert(struct realtek_priv *priv) > +{ > + int ret; > + > + ret = reset_control_deassert(priv->reset_ctl); > + if (!ret) > + return; > + > + dev_warn(priv->dev, > + "Failed to deassert the switch reset control: %pe\n", > + ERR_PTR(ret)); > + > + gpiod_set_value(priv->reset, false); > +} Same comments apply to this function. Just deassert both. > + > MODULE_AUTHOR("Luiz Angelo Daros de Luca <luizluca@gmail.com>"); > MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>"); > MODULE_DESCRIPTION("Realtek DSA switches common module"); > diff --git a/drivers/net/dsa/realtek/rtl83xx.h b/drivers/net/dsa/realtek/rtl83xx.h > index 0ddff33df6b0..c8a0ff8fd75e 100644 > --- a/drivers/net/dsa/realtek/rtl83xx.h > +++ b/drivers/net/dsa/realtek/rtl83xx.h > @@ -18,5 +18,7 @@ int rtl83xx_register_switch(struct realtek_priv *priv); > void rtl83xx_unregister_switch(struct realtek_priv *priv); > void rtl83xx_shutdown(struct realtek_priv *priv); > void rtl83xx_remove(struct realtek_priv *priv); > +void rtl83xx_reset_assert(struct realtek_priv *priv); > +void rtl83xx_reset_deassert(struct realtek_priv *priv); > > #endif /* _RTL83XX_H */ > > -- > 2.43.0 >
Hi Alvin, > On Mon, Feb 19, 2024 at 08:44:42PM -0300, Luiz Angelo Daros de Luca wrote: > > +void rtl83xx_reset_assert(struct realtek_priv *priv) > > +{ > > + int ret; > > + > > + ret = reset_control_assert(priv->reset_ctl); > > + if (!ret) > > + return; > > If priv->reset_ctl is NULL - i.e. if no DT property is specified - then > this will always return early and the GPIO will not be asserted. I made a mistake. I should be if (ret) { dev_warn... } not returning on error (as you suggested below). I was sure I was doing just that... I was surprised to see it as it is. I'll recheck my branch with all the integrated changes. It passed my tests as when reset is missed, it normally does not matter. Thanks for the catch. > > > + > > + dev_warn(priv->dev, > > + "Failed to assert the switch reset control: %pe\n", > > + ERR_PTR(ret)); > > You only log an error if the reset controller assert fails, but not if > the GPIO assert fails. Why the unequal treatment? Because it does not return a value. There is no way to tell if it failed. > > I suggest keeping it simple: > > void rtl83xx_reset_assert(struct realtek_priv *priv) > { > int ret; > > ret = reset_control_assert(priv->reset_ctl); > if (ret) > dev_warn(priv->dev, "failed to assert reset control: %d\n", ret); > > ret = gpiod_set_value(priv->reset, false); > if (ret) > dev_warn(priv->dev, "failed to assert reset GPIO: %d\n", ret); > } > > or even drop the warnings altogether. > > > + > > + gpiod_set_value(priv->reset, true); > > +} > > + > > +void rtl83xx_reset_deassert(struct realtek_priv *priv) > > +{ > > + int ret; > > + > > + ret = reset_control_deassert(priv->reset_ctl); > > + if (!ret) > > + return; > > + > > + dev_warn(priv->dev, > > + "Failed to deassert the switch reset control: %pe\n", > > + ERR_PTR(ret)); > > + > > + gpiod_set_value(priv->reset, false); > > +} > > Same comments apply to this function. Just deassert both. > > > + > > MODULE_AUTHOR("Luiz Angelo Daros de Luca <luizluca@gmail.com>"); > > MODULE_AUTHOR("Linus Walleij <linus.walleij@linaro.org>"); > > MODULE_DESCRIPTION("Realtek DSA switches common module"); > > diff --git a/drivers/net/dsa/realtek/rtl83xx.h b/drivers/net/dsa/realtek/rtl83xx.h > > index 0ddff33df6b0..c8a0ff8fd75e 100644 > > --- a/drivers/net/dsa/realtek/rtl83xx.h > > +++ b/drivers/net/dsa/realtek/rtl83xx.h > > @@ -18,5 +18,7 @@ int rtl83xx_register_switch(struct realtek_priv *priv); > > void rtl83xx_unregister_switch(struct realtek_priv *priv); > > void rtl83xx_shutdown(struct realtek_priv *priv); > > void rtl83xx_remove(struct realtek_priv *priv); > > +void rtl83xx_reset_assert(struct realtek_priv *priv); > > +void rtl83xx_reset_deassert(struct realtek_priv *priv); > > > > #endif /* _RTL83XX_H */ > > > > -- > > 2.43.0 > > Regards, Luiz
On Tue, Feb 20, 2024 at 09:22:44AM -0300, Luiz Angelo Daros de Luca wrote: > Hi Alvin, > > > On Mon, Feb 19, 2024 at 08:44:42PM -0300, Luiz Angelo Daros de Luca wrote: > > > +void rtl83xx_reset_assert(struct realtek_priv *priv) > > > +{ > > > + int ret; > > > + > > > + ret = reset_control_assert(priv->reset_ctl); > > > + if (!ret) > > > + return; > > > > If priv->reset_ctl is NULL - i.e. if no DT property is specified - then > > this will always return early and the GPIO will not be asserted. > > I made a mistake. I should be > > if (ret) { > dev_warn... > } > > not returning on error (as you suggested below). > > I was sure I was doing just that... I was surprised to see it as it > is. I'll recheck my branch with all the integrated changes. It passed > my tests as when reset is missed, it normally does not matter. Thanks > for the catch. > > > > > > + > > > + dev_warn(priv->dev, > > > + "Failed to assert the switch reset control: %pe\n", > > > + ERR_PTR(ret)); > > > > You only log an error if the reset controller assert fails, but not if > > the GPIO assert fails. Why the unequal treatment? > > Because it does not return a value. There is no way to tell if it failed. Ah ok, nevermind that part then. BTW, please use gpiod_set_value_cansleep(). With that I think this is good.
On Tue, Feb 20, 2024 at 01:30:33PM +0000, Alvin Šipraga wrote: > On Tue, Feb 20, 2024 at 09:22:44AM -0300, Luiz Angelo Daros de Luca wrote: > > Hi Alvin, > > > > > On Mon, Feb 19, 2024 at 08:44:42PM -0300, Luiz Angelo Daros de Luca wrote: > > > > +void rtl83xx_reset_assert(struct realtek_priv *priv) > > > > +{ > > > > + int ret; > > > > + > > > > + ret = reset_control_assert(priv->reset_ctl); > > > > + if (!ret) > > > > + return; > > > > > > If priv->reset_ctl is NULL - i.e. if no DT property is specified - then > > > this will always return early and the GPIO will not be asserted. > > > > I made a mistake. I should be > > > > if (ret) { > > dev_warn... > > } > > > > not returning on error (as you suggested below). > > > > I was sure I was doing just that... I was surprised to see it as it > > is. I'll recheck my branch with all the integrated changes. It passed > > my tests as when reset is missed, it normally does not matter. Thanks > > for the catch. > > > > > > > > > + > > > > + dev_warn(priv->dev, > > > > + "Failed to assert the switch reset control: %pe\n", > > > > + ERR_PTR(ret)); > > > > > > You only log an error if the reset controller assert fails, but not if > > > the GPIO assert fails. Why the unequal treatment? > > > > Because it does not return a value. There is no way to tell if it failed. > > Ah ok, nevermind that part then. > > BTW, please use gpiod_set_value_cansleep(). With that I think this is good. OK, actually the original code wasn't doing that, so not crucial for this change. It can be done in a follow-up.
The driver previously supported reset pins using GPIO, but it lacked support for reset controllers. Although a reset method is generally not required, the driver fails to detect the switch if the reset was kept asserted by a previous driver. This series adds support to reset a Realtek switch using a reset controller. It also updates the binding documentation to remove the requirement of a reset method and to add the new reset controller property. It was tested on a TL-WR1043ND v1 router (rtl8366rb via SMI). Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> --- Changes in v4: - do not test for priv->reset,priv->reset_ctl - updated commit message - Link to v3: https://lore.kernel.org/r/20240213-realtek-reset-v3-0-37837e574713@gmail.com Changes in v3: - Rebased on the Realtek DSA driver refactoring (08f627164126) - Dropped the reset controller example in bindings - Used %pe in error printing - Linked to v2: https://lore.kernel.org/r/20231027190910.27044-1-luizluca@gmail.com/ Changes in v2: - Introduced a dedicated commit for removing the reset-gpios requirement - Placed binding patches before code changes - Removed the 'reset-names' property - Moved the example from the commit message to realtek.yaml - Split the reset function into _assert/_deassert variants - Modified reset functions to return a warning instead of a value - Utilized devm_reset_control_get_optional to prevent failure when the reset control is missing - Used 'true' and 'false' for boolean values - Removed the CONFIG_RESET_CONTROLLER check as stub methods are sufficient when undefined - Linked to v1: https://lore.kernel.org/r/20231024205805.19314-1-luizluca@gmail.com/ --- Luiz Angelo Daros de Luca (3): dt-bindings: net: dsa: realtek: reset-gpios is not required dt-bindings: net: dsa: realtek: add reset controller net: dsa: realtek: support reset controller .../devicetree/bindings/net/dsa/realtek.yaml | 4 +- drivers/net/dsa/realtek/realtek.h | 2 + drivers/net/dsa/realtek/rtl83xx.c | 46 +++++++++++++++++++--- drivers/net/dsa/realtek/rtl83xx.h | 2 + 4 files changed, 48 insertions(+), 6 deletions(-) --- base-commit: d1d77120bc2867b3e449e07ee656a26b2fb03d1e change-id: 20240212-realtek-reset-88a0bf25bb22 Best regards,