[PATCH v3 1/2] dt-bindings: serial: sc16is7xx: add reset-gpios

[PATCH v3 1/2] dt-bindings: serial: sc16is7xx: add reset-gpios

[Hui Wang]

In some designs, the chip reset pin is connected to a GPIO, and this
GPIO needs to be set correctly before probing the driver, so add a
reset-gpios in the device tree.

Signed-off-by: Hui Wang <hui.wang@canonical.com>
---
In the v3:
 - drop the Reviewed-by
 - change gpio to GPIO
 - change "this GPIO" to "and this GPIO"
 - change "so adding" to "so add"

 Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml
index 5dec15b7e7c3..88871480018e 100644
--- a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml
+++ b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml
@@ -28,6 +28,9 @@ properties:
   clocks:
     maxItems: 1
 
+  reset-gpios:
+    maxItems: 1
+
   clock-frequency:
     description:
       When there is no clock provider visible to the platform, this
@@ -91,6 +94,7 @@ unevaluatedProperties: false
 examples:
   - |
     #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/gpio/gpio.h>
     i2c {
         #address-cells = <1>;
         #size-cells = <0>;
@@ -120,6 +124,7 @@ examples:
             compatible = "nxp,sc16is752";
             reg = <0x54>;
             clocks = <&clk20m>;
+            reset-gpios = <&gpio5 13 GPIO_ACTIVE_LOW>;
             interrupt-parent = <&gpio3>;
             interrupts = <7 IRQ_TYPE_EDGE_FALLING>;
             nxp,modem-control-line-ports = <0 1>; /* Ports 0 and 1 as modem control lines */
-- 
2.34.1

[PATCH v3 2/2] serial: sc16is7xx: hardware reset chip if reset-gpios is defined in DT

[Hui Wang]

Some boards connect a GPIO to the reset pin, and the reset pin needs
to be setup correctly before accessing the chip.

Add a function to handle the chip reset. If the reset-gpios is defined
in the DT, do hardware reset through this GPIO, othwerwise do software
reset as before.

Signed-off-by: Hui Wang <hui.wang@canonical.com>
---
In the v3:
 - drop Reviewed-by
 - adjust the sequence of if, else if and else
 - replace PTR_ERR(reset_gpiod) with dev_err_probe
 - change GPIOD_OUT_LOW to GPIOD_OUT_HIGH, this will assert the reset pin after requesting the GPIO
 - change the 2nd parameter struct regmap *regmap[] to struct regmap *regmap
 - address all spelling and description issues in the v2
 
 drivers/tty/serial/sc16is7xx.c | 33 +++++++++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index bf0065d1c8e9..8c7e0fe76049 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -14,6 +14,7 @@
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/export.h>
+#include <linux/gpio/consumer.h>
 #include <linux/gpio/driver.h>
 #include <linux/idr.h>
 #include <linux/kthread.h>
@@ -1467,6 +1468,30 @@ static const struct serial_rs485 sc16is7xx_rs485_supported = {
 	.delay_rts_after_send = 1,	/* Not supported but keep returning -EINVAL */
 };
 
+/* Reset device, purging any pending irq / data */
+static int sc16is7xx_reset(struct device *dev, struct regmap *regmap)
+{
+	struct gpio_desc *reset_gpio;
+
+	/* The reset GPIO is ACTIVE_LOW, flag GPIOD_OUT_HIGH asserts the reset GPIO */
+	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(reset_gpio))
+		return dev_err_probe(dev, PTR_ERR(reset_gpio), "Failed to get reset GPIO\n");
+
+	if (reset_gpio) {
+		/* The minimum reset pulse width is 3 us. */
+		udelay(5);
+		gpiod_set_value_cansleep(reset_gpio, 0);
+		/* Deassert GPIO */
+	} else {
+		/* Software reset */
+		regmap_write(regmap, SC16IS7XX_IOCONTROL_REG,
+			     SC16IS7XX_IOCONTROL_SRESET_BIT);
+	}
+
+	return 0;
+}
+
 int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
 		    struct regmap *regmaps[], int irq)
 {
@@ -1527,6 +1552,10 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
 	s->devtype = devtype;
 	dev_set_drvdata(dev, s);
 
+	ret = sc16is7xx_reset(dev, regmaps[0]);
+	if (ret)
+		goto out_clk;
+
 	kthread_init_worker(&s->kworker);
 	s->kworker_task = kthread_run(kthread_worker_fn, &s->kworker,
 				      "sc16is7xx");
@@ -1536,10 +1565,6 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
 	}
 	sched_set_fifo(s->kworker_task);
 
-	/* reset device, purging any pending irq / data */
-	regmap_write(regmaps[0], SC16IS7XX_IOCONTROL_REG,
-		     SC16IS7XX_IOCONTROL_SRESET_BIT);
-
 	/* Mark each port line and status as uninitialised. */
 	for (i = 0; i < devtype->nr_uart; ++i) {
 		s->p[i].port.line = SC16IS7XX_MAX_DEVS;
-- 
2.34.1

Re: [PATCH v3 2/2] serial: sc16is7xx: hardware reset chip if reset-gpios is defined in DT

[Hugo Villeneuve]

On Wed, 12 Jun 2024 21:14:54 +0800
Hui Wang <hui.wang@canonical.com> wrote:

Hi Hui,

> Some boards connect a GPIO to the reset pin, and the reset pin needs
> to be setup correctly before accessing the chip.
> 
> Add a function to handle the chip reset. If the reset-gpios is defined
> in the DT, do hardware reset through this GPIO, othwerwise do software
> reset as before.
> 
> Signed-off-by: Hui Wang <hui.wang@canonical.com>
> ---
> In the v3:
>  - drop Reviewed-by
>  - adjust the sequence of if, else if and else
>  - replace PTR_ERR(reset_gpiod) with dev_err_probe
>  - change GPIOD_OUT_LOW to GPIOD_OUT_HIGH, this will assert the reset pin after requesting the GPIO
>  - change the 2nd parameter struct regmap *regmap[] to struct regmap *regmap
>  - address all spelling and description issues in the v2
>  
>  drivers/tty/serial/sc16is7xx.c | 33 +++++++++++++++++++++++++++++----
>  1 file changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index bf0065d1c8e9..8c7e0fe76049 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -14,6 +14,7 @@
>  #include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/export.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/gpio/driver.h>
>  #include <linux/idr.h>
>  #include <linux/kthread.h>
> @@ -1467,6 +1468,30 @@ static const struct serial_rs485 sc16is7xx_rs485_supported = {
>  	.delay_rts_after_send = 1,	/* Not supported but keep returning -EINVAL */
>  };
>  
> +/* Reset device, purging any pending irq / data */
> +static int sc16is7xx_reset(struct device *dev, struct regmap *regmap)
> +{
> +	struct gpio_desc *reset_gpio;
> +
> +	/* The reset GPIO is ACTIVE_LOW, flag GPIOD_OUT_HIGH asserts the reset GPIO */
> +	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(reset_gpio))
> +		return dev_err_probe(dev, PTR_ERR(reset_gpio), "Failed to get reset GPIO\n");
> +
> +	if (reset_gpio) {
> +		/* The minimum reset pulse width is 3 us. */
> +		udelay(5);
> +		gpiod_set_value_cansleep(reset_gpio, 0);
> +		/* Deassert GPIO */

Move this comment after its corresponding statement:
    gpiod_set_value_cansleep(reset_gpio, 0); /* Deassert GPIO */


> +	} else {
> +		/* Software reset */
> +		regmap_write(regmap, SC16IS7XX_IOCONTROL_REG,
> +			     SC16IS7XX_IOCONTROL_SRESET_BIT);
> +	}
> +
> +	return 0;
> +}
> +
>  int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>  		    struct regmap *regmaps[], int irq)
>  {
> @@ -1527,6 +1552,10 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>  	s->devtype = devtype;
>  	dev_set_drvdata(dev, s);
>  
> +	ret = sc16is7xx_reset(dev, regmaps[0]);
> +	if (ret)
> +		goto out_clk;
> +

You could move this block where the original software reset was
located, unless you have a good reason to move it here? This will make
the review (diff) easier...

>  	kthread_init_worker(&s->kworker);
>  	s->kworker_task = kthread_run(kthread_worker_fn, &s->kworker,
>  				      "sc16is7xx");
> @@ -1536,10 +1565,6 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>  	}
>  	sched_set_fifo(s->kworker_task);
>  
> -	/* reset device, purging any pending irq / data */
> -	regmap_write(regmaps[0], SC16IS7XX_IOCONTROL_REG,
> -		     SC16IS7XX_IOCONTROL_SRESET_BIT);
> -
>  	/* Mark each port line and status as uninitialised. */
>  	for (i = 0; i < devtype->nr_uart; ++i) {
>  		s->p[i].port.line = SC16IS7XX_MAX_DEVS;
> -- 
> 2.34.1


-- 
Hugo Villeneuve

Re: [PATCH v3 2/2] serial: sc16is7xx: hardware reset chip if reset-gpios is defined in DT

[Lech Perczak]

Hi,
Some comments inline.

W dniu 12.06.2024 o 15:14, Hui Wang pisze:
> Some boards connect a GPIO to the reset pin, and the reset pin needs
> to be setup correctly before accessing the chip.
>
> Add a function to handle the chip reset. If the reset-gpios is defined
> in the DT, do hardware reset through this GPIO, othwerwise do software
> reset as before.
>
> Signed-off-by: Hui Wang <hui.wang@canonical.com>
> ---
> In the v3:
>  - drop Reviewed-by
>  - adjust the sequence of if, else if and else
>  - replace PTR_ERR(reset_gpiod) with dev_err_probe
>  - change GPIOD_OUT_LOW to GPIOD_OUT_HIGH, this will assert the reset pin after requesting the GPIO
>  - change the 2nd parameter struct regmap *regmap[] to struct regmap *regmap
>  - address all spelling and description issues in the v2
>
>  drivers/tty/serial/sc16is7xx.c | 33 +++++++++++++++++++++++++++++----
>  1 file changed, 29 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index bf0065d1c8e9..8c7e0fe76049 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -14,6 +14,7 @@
>  #include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/export.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/gpio/driver.h>
>  #include <linux/idr.h>
>  #include <linux/kthread.h>
> @@ -1467,6 +1468,30 @@ static const struct serial_rs485 sc16is7xx_rs485_supported = {
>         .delay_rts_after_send = 1,      /* Not supported but keep returning -EINVAL */
>  };
>
> +/* Reset device, purging any pending irq / data */
> +static int sc16is7xx_reset(struct device *dev, struct regmap *regmap)
> +{
> +       struct gpio_desc *reset_gpio;
> +
> +       /* The reset GPIO is ACTIVE_LOW, flag GPIOD_OUT_HIGH asserts the reset GPIO */
s/reset GPIO is ACTIVE_LOW /reset input is active low/ - because we're referring to the chip input here, and this is reflected in DT bindings.

Also, it isn't clear from the comment that GPIO is asserted immediately.


> +       reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +       if (IS_ERR(reset_gpio))
> +               return dev_err_probe(dev, PTR_ERR(reset_gpio), "Failed to get reset GPIO\n");
> +
> +       if (reset_gpio) {
> +               /* The minimum reset pulse width is 3 us. */
> +               udelay(5);

Prefer usleep_range() over that, since maximum reset time isn't all that critical.


> +               gpiod_set_value_cansleep(reset_gpio, 0);
> +               /* Deassert GPIO */

This comment should go one line above or be removed entirely.


> +       } else {
> +               /* Software reset */
> +               regmap_write(regmap, SC16IS7XX_IOCONTROL_REG,
> +                            SC16IS7XX_IOCONTROL_SRESET_BIT);
> +       }
> +
> +       return 0;
> +}
> +
>  int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>                     struct regmap *regmaps[], int irq)
>  {
> @@ -1527,6 +1552,10 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>         s->devtype = devtype;
>         dev_set_drvdata(dev, s);
>
> +       ret = sc16is7xx_reset(dev, regmaps[0]);
> +       if (ret)
> +               goto out_clk;
> +
>         kthread_init_worker(&s->kworker);
>         s->kworker_task = kthread_run(kthread_worker_fn, &s->kworker,
>                                       "sc16is7xx");
> @@ -1536,10 +1565,6 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>         }
>         sched_set_fifo(s->kworker_task);
>
> -       /* reset device, purging any pending irq / data */
> -       regmap_write(regmaps[0], SC16IS7XX_IOCONTROL_REG,
> -                    SC16IS7XX_IOCONTROL_SRESET_BIT);
> -
>         /* Mark each port line and status as uninitialised. */
>         for (i = 0; i < devtype->nr_uart; ++i) {
>                 s->p[i].port.line = SC16IS7XX_MAX_DEVS;
> --
> 2.34.1

-- 
Pozdrawiam/With kind regards,
Lech Perczak

Sr. Software Engineer
Camlin Technologies Poland Limited Sp. z o.o.
Strzegomska 54,
53-611 Wroclaw
Tel:     (+48) 71 75 000 16
Email:   lech.perczak@camlingroup.com
Website: http://www.camlingroup.com

Re: [PATCH v3 2/2] serial: sc16is7xx: hardware reset chip if reset-gpios is defined in DT

[Hui Wang]

On 6/12/24 21:50, Hugo Villeneuve wrote:
> On Wed, 12 Jun 2024 21:14:54 +0800
> Hui Wang <hui.wang@canonical.com> wrote:
>
> Hi Hui,
>
>> Some boards connect a GPIO to the reset pin, and the reset pin needs
>> to be setup correctly before accessing the chip.
>>
>> Add a function to handle the chip reset. If the reset-gpios is defined
>> in the DT, do hardware reset through this GPIO, othwerwise do software
>> reset as before.
...
>> +		/* The minimum reset pulse width is 3 us. */
>> +		udelay(5);
>> +		gpiod_set_value_cansleep(reset_gpio, 0);
>> +		/* Deassert GPIO */
> Move this comment after its corresponding statement:
>      gpiod_set_value_cansleep(reset_gpio, 0); /* Deassert GPIO */
>
Got it.
>> +	} else {
>> +		/* Software reset */
>> +		regmap_write(regmap, SC16IS7XX_IOCONTROL_REG,
>> +			     SC16IS7XX_IOCONTROL_SRESET_BIT);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>>   		    struct regmap *regmaps[], int irq)
>>   {
>> @@ -1527,6 +1552,10 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>>   	s->devtype = devtype;
>>   	dev_set_drvdata(dev, s);
>>   
>> +	ret = sc16is7xx_reset(dev, regmaps[0]);
>> +	if (ret)
>> +		goto out_clk;
>> +
> You could move this block where the original software reset was
> located, unless you have a good reason to move it here? This will make
> the review (diff) easier...

If I move it to the place where the original software reset was located, 
I can't use "goto out_clk" anymore, it not only needs to release clk but 
also needs to stop the kthread, so I have to add a new tag before the 
kthread_stop(s->kworker_task) and goto that tag.

Thanks.

>>   	kthread_init_worker(&s->kworker);
>>   	s->kworker_task = kthread_run(kthread_worker_fn, &s->kworker,
>>   				      "sc16is7xx");
>> @@ -1536,10 +1565,6 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>>   	}
>>   	sched_set_fifo(s->kworker_task);
>>   
>> -	/* reset device, purging any pending irq / data */
>> -	regmap_write(regmaps[0], SC16IS7XX_IOCONTROL_REG,
>> -		     SC16IS7XX_IOCONTROL_SRESET_BIT);
>> -
>>   	/* Mark each port line and status as uninitialised. */
>>   	for (i = 0; i < devtype->nr_uart; ++i) {
>>   		s->p[i].port.line = SC16IS7XX_MAX_DEVS;
>> -- 
>> 2.34.1
>

Re: [PATCH v3 2/2] serial: sc16is7xx: hardware reset chip if reset-gpios is defined in DT

[Hui Wang]

On 6/12/24 21:56, Lech Perczak wrote:
> Hi,
> Some comments inline.
>
> W dniu 12.06.2024 o 15:14, Hui Wang pisze:
>> Some boards connect a GPIO to the reset pin, and the reset pin needs
>> to be setup correctly before accessing the chip.
>>
>> Add a function to handle the chip reset. If the reset-gpios is defined
>> in the DT, do hardware reset through this GPIO, othwerwise do software
>> reset as before.
>>
>> Signed-off-by: Hui Wang <hui.wang@canonical.com>
>> ---
>> In the v3:
>>   - drop Reviewed-by
>>   - adjust the sequence of if, else if and else
>>   - replace PTR_ERR(reset_gpiod) with dev_err_probe
>>   - change GPIOD_OUT_LOW to GPIOD_OUT_HIGH, this will assert the reset pin after requesting the GPIO
>>   - change the 2nd parameter struct regmap *regmap[] to struct regmap *regmap
>>   - address all spelling and description issues in the v2
>>
>>   drivers/tty/serial/sc16is7xx.c | 33 +++++++++++++++++++++++++++++----
>>   1 file changed, 29 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
>> index bf0065d1c8e9..8c7e0fe76049 100644
>> --- a/drivers/tty/serial/sc16is7xx.c
>> +++ b/drivers/tty/serial/sc16is7xx.c
>> @@ -14,6 +14,7 @@
>>   #include <linux/delay.h>
>>   #include <linux/device.h>
>>   #include <linux/export.h>
>> +#include <linux/gpio/consumer.h>
>>   #include <linux/gpio/driver.h>
>>   #include <linux/idr.h>
>>   #include <linux/kthread.h>
>> @@ -1467,6 +1468,30 @@ static const struct serial_rs485 sc16is7xx_rs485_supported = {
>>          .delay_rts_after_send = 1,      /* Not supported but keep returning -EINVAL */
>>   };
>>
>> +/* Reset device, purging any pending irq / data */
>> +static int sc16is7xx_reset(struct device *dev, struct regmap *regmap)
>> +{
>> +       struct gpio_desc *reset_gpio;
>> +
>> +       /* The reset GPIO is ACTIVE_LOW, flag GPIOD_OUT_HIGH asserts the reset GPIO */
> s/reset GPIO is ACTIVE_LOW /reset input is active low/ - because we're referring to the chip input here, and this is reflected in DT bindings.
>
> Also, it isn't clear from the comment that GPIO is asserted immediately.
Got it.
>
>
>> +       reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
>> +       if (IS_ERR(reset_gpio))
>> +               return dev_err_probe(dev, PTR_ERR(reset_gpio), "Failed to get reset GPIO\n");
>> +
>> +       if (reset_gpio) {
>> +               /* The minimum reset pulse width is 3 us. */
>> +               udelay(5);
> Prefer usleep_range() over that, since maximum reset time isn't all that critical.
Got it.
>
>
>> +               gpiod_set_value_cansleep(reset_gpio, 0);
>> +               /* Deassert GPIO */
> This comment should go one line above or be removed entirely.

Got it.

Thanks.

>
>
>> +       } else {
>> +               /* Software reset */
>> +               regmap_write(regmap, SC16IS7XX_IOCONTROL_REG,
>> +                            SC16IS7XX_IOCONTROL_SRESET_BIT);
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>>   int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>>                      struct regmap *regmaps[], int irq)
>>   {
>> @@ -1527,6 +1552,10 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>>          s->devtype = devtype;
>>          dev_set_drvdata(dev, s);
>>
>> +       ret = sc16is7xx_reset(dev, regmaps[0]);
>> +       if (ret)
>> +               goto out_clk;
>> +
>>          kthread_init_worker(&s->kworker);
>>          s->kworker_task = kthread_run(kthread_worker_fn, &s->kworker,
>>                                        "sc16is7xx");
>> @@ -1536,10 +1565,6 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>>          }
>>          sched_set_fifo(s->kworker_task);
>>
>> -       /* reset device, purging any pending irq / data */
>> -       regmap_write(regmaps[0], SC16IS7XX_IOCONTROL_REG,
>> -                    SC16IS7XX_IOCONTROL_SRESET_BIT);
>> -
>>          /* Mark each port line and status as uninitialised. */
>>          for (i = 0; i < devtype->nr_uart; ++i) {
>>                  s->p[i].port.line = SC16IS7XX_MAX_DEVS;
>> --
>> 2.34.1

Re: [PATCH v3 1/2] dt-bindings: serial: sc16is7xx: add reset-gpios

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 522 bytes --]

On Wed, Jun 12, 2024 at 09:14:53PM +0800, Hui Wang wrote:
> In some designs, the chip reset pin is connected to a GPIO, and this
> GPIO needs to be set correctly before probing the driver, so add a
> reset-gpios in the device tree.
> 
> Signed-off-by: Hui Wang <hui.wang@canonical.com>
> ---
> In the v3:
>  - drop the Reviewed-by
>  - change gpio to GPIO
>  - change "this GPIO" to "and this GPIO"
>  - change "so adding" to "so add"

There's no need to drop an R-b for grammar changes in a commit message.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 1/2] dt-bindings: serial: sc16is7xx: add reset-gpios

[Hugo Villeneuve]

On Wed, 12 Jun 2024 17:37:30 +0100
Conor Dooley <conor@kernel.org> wrote:

> On Wed, Jun 12, 2024 at 09:14:53PM +0800, Hui Wang wrote:
> > In some designs, the chip reset pin is connected to a GPIO, and this
> > GPIO needs to be set correctly before probing the driver, so add a
> > reset-gpios in the device tree.
> > 
> > Signed-off-by: Hui Wang <hui.wang@canonical.com>
> > ---
> > In the v3:
> >  - drop the Reviewed-by
> >  - change gpio to GPIO
> >  - change "this GPIO" to "and this GPIO"
> >  - change "so adding" to "so add"
> 
> There's no need to drop an R-b for grammar changes in a commit message.

Hi Conor,
The R-b tags were never given in the first place, that is why they are
removed:

https://lore.kernel.org/all/6b1b0635-304c-48d7-a941-fae30962083a@canonical.com/

-- 
Hugo Villeneuve

Re: [PATCH v3 1/2] dt-bindings: serial: sc16is7xx: add reset-gpios

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1063 bytes --]

On Wed, Jun 12, 2024 at 12:49:43PM -0400, Hugo Villeneuve wrote:
> On Wed, 12 Jun 2024 17:37:30 +0100
> Conor Dooley <conor@kernel.org> wrote:
> 
> > On Wed, Jun 12, 2024 at 09:14:53PM +0800, Hui Wang wrote:
> > > In some designs, the chip reset pin is connected to a GPIO, and this
> > > GPIO needs to be set correctly before probing the driver, so add a
> > > reset-gpios in the device tree.
> > > 
> > > Signed-off-by: Hui Wang <hui.wang@canonical.com>
> > > ---
> > > In the v3:
> > >  - drop the Reviewed-by
> > >  - change gpio to GPIO
> > >  - change "this GPIO" to "and this GPIO"
> > >  - change "so adding" to "so add"
> > 
> > There's no need to drop an R-b for grammar changes in a commit message.
> 
> Hi Conor,
> The R-b tags were never given in the first place, that is why they are
> removed:
> 
> https://lore.kernel.org/all/6b1b0635-304c-48d7-a941-fae30962083a@canonical.com/

lmao, yea that's justified (and probably should've been mentioned).

Acked-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 2/2] serial: sc16is7xx: hardware reset chip if reset-gpios is defined in DT

[Andy Shevchenko]

On Wed, Jun 12, 2024 at 3:57 PM Lech Perczak
<lech.perczak@camlingroup.com> wrote:
> W dniu 12.06.2024 o 15:14, Hui Wang pisze:
> > Some boards connect a GPIO to the reset pin, and the reset pin needs
> > to be setup correctly before accessing the chip.
> >
> > Add a function to handle the chip reset. If the reset-gpios is defined
> > in the DT, do hardware reset through this GPIO, othwerwise do software

otherwise

> > reset as before.

...


> > +       if (reset_gpio) {
> > +               /* The minimum reset pulse width is 3 us. */
> > +               udelay(5);
>
> Prefer usleep_range() over that, since maximum reset time isn't all that critical.

For this little sleep the usleep_range() won't gain much. OTOH one may
use flseep() and let the sane default to be used.

> > +               gpiod_set_value_cansleep(reset_gpio, 0);
> > +               /* Deassert GPIO */
>
> This comment should go one line above or be removed entirely.
>
> > +       } else {
> > +               /* Software reset */
> > +               regmap_write(regmap, SC16IS7XX_IOCONTROL_REG,
> > +                            SC16IS7XX_IOCONTROL_SRESET_BIT);
> > +       }

-- 
With Best Regards,
Andy Shevchenko