[PATCH] reset: gpio: Add self-deasserting reset callback

[PATCH] reset: gpio: Add self-deasserting reset callback

[Shenwei Wang]

During the driver probe phase, many drivers leverage convenience
wrapper APIs such as device_reset and device_reset_optional provided
by the reset core driver. However, both of these APIs depend on the
presence of a .reset callback within the reset controller's operations
structure.

Introducing the self-deasserting reset callback enhances flexibility for
users and enables a more simple reset process during device initialization.

Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
---
 drivers/reset/reset-gpio.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/reset/reset-gpio.c b/drivers/reset/reset-gpio.c
index 2290b25b6703..614f9e261a13 100644
--- a/drivers/reset/reset-gpio.c
+++ b/drivers/reset/reset-gpio.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 
+#include <linux/delay.h>
 #include <linux/gpio/consumer.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
@@ -37,6 +38,17 @@ static int reset_gpio_deassert(struct reset_controller_dev *rc,
 	return 0;
 }
 
+static int reset_gpio_reset(struct reset_controller_dev *rc, unsigned long id)
+{
+	struct reset_gpio_priv *priv = rc_to_reset_gpio(rc);
+
+	gpiod_set_value_cansleep(priv->reset, 1);
+	usleep_range(10, 20);
+	gpiod_set_value_cansleep(priv->reset, 0);
+
+	return 0;
+}
+
 static int reset_gpio_status(struct reset_controller_dev *rc, unsigned long id)
 {
 	struct reset_gpio_priv *priv = rc_to_reset_gpio(rc);
@@ -47,6 +59,7 @@ static int reset_gpio_status(struct reset_controller_dev *rc, unsigned long id)
 static const struct reset_control_ops reset_gpio_ops = {
 	.assert = reset_gpio_assert,
 	.deassert = reset_gpio_deassert,
+	.reset = reset_gpio_reset,
 	.status = reset_gpio_status,
 };
 
-- 
2.43.0

Re: [PATCH] reset: gpio: Add self-deasserting reset callback

[Krzysztof Kozlowski]

On 30/01/2025 22:53, Shenwei Wang wrote:
> During the driver probe phase, many drivers leverage convenience
> wrapper APIs such as device_reset and device_reset_optional provided
> by the reset core driver. However, both of these APIs depend on the
> presence of a .reset callback within the reset controller's operations
> structure.
> 
> Introducing the self-deasserting reset callback enhances flexibility for
> users and enables a more simple reset process during device initialization.


The reset callback was not added on purpose and you totally ignored the
reasons here. See below.

> 
> Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
> ---
>  drivers/reset/reset-gpio.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/reset/reset-gpio.c b/drivers/reset/reset-gpio.c
> index 2290b25b6703..614f9e261a13 100644
> --- a/drivers/reset/reset-gpio.c
> +++ b/drivers/reset/reset-gpio.c
> @@ -1,5 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0
>  
> +#include <linux/delay.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
> @@ -37,6 +38,17 @@ static int reset_gpio_deassert(struct reset_controller_dev *rc,
>  	return 0;
>  }
>  
> +static int reset_gpio_reset(struct reset_controller_dev *rc, unsigned long id)
> +{
> +	struct reset_gpio_priv *priv = rc_to_reset_gpio(rc);
> +
> +	gpiod_set_value_cansleep(priv->reset, 1);
> +	usleep_range(10, 20);


No, because this is some arbitrary value which might or might not work.
If this gets accepted, next person will change it to their own need.
Then next person will revert previous change... and so on.


Best regards,
Krzysztof

Re: [PATCH] reset: gpio: Add self-deasserting reset callback

[Shenwei Wang]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Friday, January 31, 2025 1:18 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>; Philipp Zabel
> <p.zabel@pengutronix.de>
> Cc: imx@lists.linux.dev; dl-linux-imx <linux-imx@nxp.com>
> Subject: [EXT] Re: [PATCH] reset: gpio: Add self-deasserting reset callback
> >
> > Introducing the self-deasserting reset callback enhances flexibility
> > for users and enables a more simple reset process during device initialization.
> 
> 
> The reset callback was not added on purpose and you totally ignored the reasons
> here. See below.
> 
> >
> > Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
> > ---
> >  drivers/reset/reset-gpio.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/drivers/reset/reset-gpio.c b/drivers/reset/reset-gpio.c
> > index 2290b25b6703..614f9e261a13 100644
> > --- a/drivers/reset/reset-gpio.c
> > +++ b/drivers/reset/reset-gpio.c
> > @@ -1,5 +1,6 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >
> > +#include <linux/delay.h>
> >  #include <linux/gpio/consumer.h>
> >  #include <linux/mod_devicetable.h>
> >  #include <linux/module.h>
> > @@ -37,6 +38,17 @@ static int reset_gpio_deassert(struct
> reset_controller_dev *rc,
> >       return 0;
> >  }
> >
> > +static int reset_gpio_reset(struct reset_controller_dev *rc, unsigned
> > +long id) {
> > +     struct reset_gpio_priv *priv = rc_to_reset_gpio(rc);
> > +
> > +     gpiod_set_value_cansleep(priv->reset, 1);
> > +     usleep_range(10, 20);
> 
> 
> No, because this is some arbitrary value which might or might not work.
> If this gets accepted, next person will change it to their own need.
> Then next person will revert previous change... and so on.
> 

If the arbitrary value is a concern, would moving it to the DTS be an appropriate solution?
This may give the user more flexible.

Shenwei

> 
> Best regards,
> Krzysztof

Re: [PATCH] reset: gpio: Add self-deasserting reset callback

[Krzysztof Kozlowski]

On 31/01/2025 15:58, Shenwei Wang wrote:
>>> +#include <linux/delay.h>
>>>  #include <linux/gpio/consumer.h>
>>>  #include <linux/mod_devicetable.h>
>>>  #include <linux/module.h>
>>> @@ -37,6 +38,17 @@ static int reset_gpio_deassert(struct
>> reset_controller_dev *rc,
>>>       return 0;
>>>  }
>>>
>>> +static int reset_gpio_reset(struct reset_controller_dev *rc, unsigned
>>> +long id) {
>>> +     struct reset_gpio_priv *priv = rc_to_reset_gpio(rc);
>>> +
>>> +     gpiod_set_value_cansleep(priv->reset, 1);
>>> +     usleep_range(10, 20);
>>
>>
>> No, because this is some arbitrary value which might or might not work.
>> If this gets accepted, next person will change it to their own need.
>> Then next person will revert previous change... and so on.
>>
> 
> If the arbitrary value is a concern, would moving it to the DTS be an appropriate solution?
> This may give the user more flexible.

Sure, as discussed when this series were upstreamed and as discussed
with Sean's patchset. BTW, Sean's mentioned need to revive his work, but
I think it did not materialize.

Best regards,
Krzysztof

Re: [PATCH] reset: gpio: Add self-deasserting reset callback

[Shenwei Wang]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Friday, January 31, 2025 9:10 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>; Philipp Zabel
> <p.zabel@pengutronix.de>
> Cc: imx@lists.linux.dev; dl-linux-imx <linux-imx@nxp.com>
> Subject: [EXT] Re: [PATCH] reset: gpio: Add self-deasserting reset callback
> >>> +     usleep_range(10, 20);
> >>
> >>
> >> No, because this is some arbitrary value which might or might not work.
> >> If this gets accepted, next person will change it to their own need.
> >> Then next person will revert previous change... and so on.
> >>
> >
> > If the arbitrary value is a concern, would moving it to the DTS be an appropriate
> solution?
> > This may give the user more flexible.
> 
> Sure, as discussed when this series were upstreamed and as discussed with
> Sean's patchset. BTW, Sean's mentioned need to revive his work, but I think it did
> not materialize.
> 

Do you know if Sean is still working on this feature? If not, I'm willing to take it over 
and complete the implementation.

Thanks,
Shenwei

> Best regards,
> Krzysztof