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

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

[Hui Wang]

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

Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
Signed-off-by: Hui Wang <hui.wang@canonical.com>
---
In the v2:
 - include the gpio.h
 - run the 'make dt_binding_check' and 'make dtbs_check'

 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 v2 2/2] serial: sc16is7xx: hard reset the chip if reset-gpios is defined in dt

[Hui Wang]

Certain designs connect a gpio to the reset pin, and the reset pin
needs to be setup correctly before accessing the chip.

Here adding a function to handle the chip reset. If the reset-gpios is
defined in the dt, do the hard reset through this gpio, othwerwise do
the soft reset as before.

Reviewed-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
Signed-off-by: Hui Wang <hui.wang@canonical.com>
---
In the v2:
 - move the soft reset and hard reset into one fucntion
 - move the reset function to sc16is7xx.c and call it in _probe()
 - add udelay(5) before deasserting the gpio reset pin

 drivers/tty/serial/sc16is7xx.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index bf0065d1c8e9..119abfb4607c 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,25 @@ static const struct serial_rs485 sc16is7xx_rs485_supported = {
 	.delay_rts_after_send = 1,	/* Not supported but keep returning -EINVAL */
 };
 
+static int sc16is7xx_reset(struct device *dev, struct regmap *regmaps[])
+{
+	struct gpio_desc *reset_gpiod;
+
+	reset_gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
+	if (!reset_gpiod)
+		/* soft reset device, purging any pending irq / data */
+		regmap_write(regmaps[0], SC16IS7XX_IOCONTROL_REG,
+			     SC16IS7XX_IOCONTROL_SRESET_BIT);
+	else if (!IS_ERR(reset_gpiod)) {
+		/* delay 5 us (at least 3 us) and deassert the gpio to exit the hard reset */
+		udelay(5);
+		gpiod_set_value_cansleep(reset_gpiod, 0);
+	} else
+		return PTR_ERR(reset_gpiod);
+
+	return 0;
+}
+
 int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
 		    struct regmap *regmaps[], int irq)
 {
@@ -1527,6 +1547,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);
+	if (ret)
+		goto out_clk;
+
 	kthread_init_worker(&s->kworker);
 	s->kworker_task = kthread_run(kthread_worker_fn, &s->kworker,
 				      "sc16is7xx");
@@ -1536,10 +1560,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 v2 1/2] dt-bindings: serial: sc16is7xx: add reset-gpios

[Krzysztof Kozlowski]

On 04/06/2024 15:27, Hui Wang wrote:
> In some designs, the chip reset pin is connected to a gpio, this
> gpio needs to be set correctly before probing the driver, so adding
> a reset-gpios in the device tree.
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Where did these happen?!?!?

NAK

Please talk with your colleagues in Canonical to explain how this works.
Then read submitting patches document.

Best regards,
Krzysztof

Re: [PATCH v2 2/2] serial: sc16is7xx: hard reset the chip if reset-gpios is defined in dt

[Krzysztof Kozlowski]

On 04/06/2024 15:27, Hui Wang wrote:
> Certain designs connect a gpio to the reset pin, and the reset pin
> needs to be setup correctly before accessing the chip.
> 
> Here adding a function to handle the chip reset. If the reset-gpios is
> defined in the dt, do the hard reset through this gpio, othwerwise do
> the soft reset as before.
> 
> Reviewed-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>

No, stop making up tags!

Best regards,
Krzysztof

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

[Andy Shevchenko]

On Tue, Jun 04, 2024 at 09:27:25PM +0800, Hui Wang wrote:
> In some designs, the chip reset pin is connected to a gpio, this

GPIO

> gpio needs to be set correctly before probing the driver, so adding

GPIO

> a reset-gpios in the device tree.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] serial: sc16is7xx: hard reset the chip if reset-gpios is defined in dt

[Andy Shevchenko]

On Tue, Jun 04, 2024 at 09:27:26PM +0800, Hui Wang wrote:
> Certain designs connect a gpio to the reset pin, and the reset pin

GPIO

> needs to be setup correctly before accessing the chip.
> 
> Here adding a function to handle the chip reset. If the reset-gpios is
> defined in the dt, do the hard reset through this gpio, othwerwise do

DT

> the soft reset as before.

...

> +static int sc16is7xx_reset(struct device *dev, struct regmap *regmaps[])
> +{
> +	struct gpio_desc *reset_gpiod;

> +	reset_gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> +	if (!reset_gpiod)
> +		/* soft reset device, purging any pending irq / data */
> +		regmap_write(regmaps[0], SC16IS7XX_IOCONTROL_REG,
> +			     SC16IS7XX_IOCONTROL_SRESET_BIT);
> +	else if (!IS_ERR(reset_gpiod)) {
> +		/* delay 5 us (at least 3 us) and deassert the gpio to exit the hard reset */
> +		udelay(5);
> +		gpiod_set_value_cansleep(reset_gpiod, 0);
> +	} else
> +		return PTR_ERR(reset_gpiod);

You can do better here.

	reset_gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
	if (IS_ERR(reset_gpiod))
		return PTR_ERR(reset_gpiod);

	if (reset_gpiod) {
		/* delay 5 us (at least 3 us) and deassert the GPIO to exit the hard reset */
		fsleep(5);
		gpiod_set_value_cansleep(reset_gpiod, 0);
	} else {
		/* soft reset device, purging any pending IRQ / data */
		regmap_write(regmaps[0], SC16IS7XX_IOCONTROL_REG,
			     SC16IS7XX_IOCONTROL_SRESET_BIT);
	}

> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] serial: sc16is7xx: hard reset the chip if reset-gpios is defined in dt

[Hugo Villeneuve]

On Tue,  4 Jun 2024 21:27:26 +0800
Hui Wang <hui.wang@canonical.com> wrote:

Hi Hui,

> Certain designs connect a gpio to the reset pin, and the reset pin
> needs to be setup correctly before accessing the chip.
> 
> Here adding a function to handle the chip reset. If the reset-gpios is
> defined in the dt, do the hard reset through this gpio, othwerwise do
> the soft reset as before.
> 
> Reviewed-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>

I never gave you permission to add this tag, remove it. Make sure you
fully understand the meaning of tags by reading patches submission
guidelines.


> Signed-off-by: Hui Wang <hui.wang@canonical.com>
> ---
> In the v2:
>  - move the soft reset and hard reset into one fucntion
>  - move the reset function to sc16is7xx.c and call it in _probe()
>  - add udelay(5) before deasserting the gpio reset pin
> 
>  drivers/tty/serial/sc16is7xx.c | 28 ++++++++++++++++++++++++----
>  1 file changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index bf0065d1c8e9..119abfb4607c 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,25 @@ static const struct serial_rs485 sc16is7xx_rs485_supported = {
>  	.delay_rts_after_send = 1,	/* Not supported but keep returning -EINVAL */
>  };
>  

Add function description from original comment "Reset device,
purging any pending irq / data", since the comment applies to both
hardware and software reset,

> +static int sc16is7xx_reset(struct device *dev, struct regmap *regmaps[])

Simply pass "struct regmap *regmap" as the second argument. See
sc16is7xx_setup_mctrl_ports() for example.

> +{
> +	struct gpio_desc *reset_gpiod;

reset_gpiod -> reset_gpio

> +
> +	reset_gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> +	if (!reset_gpiod)

Follow Andy's suggestion here.

> +		/* soft reset device, purging any pending irq / data */

"Software reset".

> +		regmap_write(regmaps[0], SC16IS7XX_IOCONTROL_REG,
> +			     SC16IS7XX_IOCONTROL_SRESET_BIT);
> +	else if (!IS_ERR(reset_gpiod)) {
> +		/* delay 5 us (at least 3 us) and deassert the gpio to exit the hard reset */

You can omit the "delay 5 us" since it is obvious from the code. Maybe
add that "The minimum reset pulse width is 3 us" as stated in the
datasheet.

As a general note for your comments: capitalize the first letter,
ex: "Deassert GPIO" and not "deassert GPIO".


> +		udelay(5);
> +		gpiod_set_value_cansleep(reset_gpiod, 0);

Move the comment "deassert the gpio to exit the hard reset" here. You
could also simplify it as "Deassert GPIO.".


> +	} else
> +		return PTR_ERR(reset_gpiod);

return dev_err_probe(dev, PTR_ERR(reset_gpiod), "Failed to get reset
GPIO\n");

> +
> +	return 0;
> +}
> +
>  int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>  		    struct regmap *regmaps[], int irq)
>  {
> @@ -1527,6 +1547,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);
> +	if (ret)
> +		goto out_clk;
> +
>  	kthread_init_worker(&s->kworker);
>  	s->kworker_task = kthread_run(kthread_worker_fn, &s->kworker,
>  				      "sc16is7xx");
> @@ -1536,10 +1560,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 v2 2/2] serial: sc16is7xx: hard reset the chip if reset-gpios is defined in dt

[Hugo Villeneuve]

On Tue,  4 Jun 2024 21:27:26 +0800
Hui Wang <hui.wang@canonical.com> wrote:

Hi Hui,

> Certain designs connect a gpio to the reset pin, and the reset pin

"Some boards..."

> needs to be setup correctly before accessing the chip.
> 
> Here adding a function to handle the chip reset. If the reset-gpios is

"Add a function..."

> defined in the dt, do the hard reset through this gpio, othwerwise do
> the soft reset as before.
> 
> Reviewed-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> Signed-off-by: Hui Wang <hui.wang@canonical.com>
> ---
> In the v2:
>  - move the soft reset and hard reset into one fucntion
>  - move the reset function to sc16is7xx.c and call it in _probe()
>  - add udelay(5) before deasserting the gpio reset pin
> 
>  drivers/tty/serial/sc16is7xx.c | 28 ++++++++++++++++++++++++----
>  1 file changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index bf0065d1c8e9..119abfb4607c 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,25 @@ static const struct serial_rs485 sc16is7xx_rs485_supported = {
>  	.delay_rts_after_send = 1,	/* Not supported but keep returning -EINVAL */
>  };
>  
> +static int sc16is7xx_reset(struct device *dev, struct regmap *regmaps[])
> +{
> +	struct gpio_desc *reset_gpiod;
> +
> +	reset_gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> +	if (!reset_gpiod)
> +		/* soft reset device, purging any pending irq / data */
> +		regmap_write(regmaps[0], SC16IS7XX_IOCONTROL_REG,
> +			     SC16IS7XX_IOCONTROL_SRESET_BIT);
> +	else if (!IS_ERR(reset_gpiod)) {
> +		/* delay 5 us (at least 3 us) and deassert the gpio to exit the hard reset */
> +		udelay(5);
> +		gpiod_set_value_cansleep(reset_gpiod, 0);
> +	} else
> +		return PTR_ERR(reset_gpiod);
> +
> +	return 0;
> +}
> +
>  int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>  		    struct regmap *regmaps[], int irq)
>  {
> @@ -1527,6 +1547,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);
> +	if (ret)
> +		goto out_clk;
> +
>  	kthread_init_worker(&s->kworker);
>  	s->kworker_task = kthread_run(kthread_worker_fn, &s->kworker,
>  				      "sc16is7xx");
> @@ -1536,10 +1560,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 v2 1/2] dt-bindings: serial: sc16is7xx: add reset-gpios

[Hugo Villeneuve]

On Tue,  4 Jun 2024 21:27:25 +0800
Hui Wang <hui.wang@canonical.com> wrote:

> In some designs, the chip reset pin is connected to a gpio, this

"and this ..."

> gpio needs to be set correctly before probing the driver, so adding

"so add ..."

> a reset-gpios in the device tree.
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
> Signed-off-by: Hui Wang <hui.wang@canonical.com>
> ---
> In the v2:
>  - include the gpio.h
>  - run the 'make dt_binding_check' and 'make dtbs_check'
> 
>  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
> 
> 
> 


-- 
Hugo Villeneuve

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

[Hui Wang]

On 6/4/24 21:29, Krzysztof Kozlowski wrote:
> On 04/06/2024 15:27, Hui Wang wrote:
>> In some designs, the chip reset pin is connected to a gpio, this
>> gpio needs to be set correctly before probing the driver, so adding
>> a reset-gpios in the device tree.
>>
>> Reviewed-by: Rob Herring <robh@kernel.org>
>> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
> Where did these happen?!?!?
>
> NAK
>
> Please talk with your colleagues in Canonical to explain how this works.
> Then read submitting patches document.

Sorry, my bad, I will read submitting-pathces.rst carefully.

Thanks,

Hui.

>
> Best regards,
> Krzysztof
>

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

[Hui Wang]

On 6/4/24 21:37, Andy Shevchenko wrote:
> On Tue, Jun 04, 2024 at 09:27:25PM +0800, Hui Wang wrote:
>> In some designs, the chip reset pin is connected to a gpio, this
> GPIO
>
>> gpio needs to be set correctly before probing the driver, so adding
> GPIO

Got them, and will fix them in the v3.

Thanks.

>> a reset-gpios in the device tree.
>

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

[Hui Wang]

On 6/4/24 22:35, Hugo Villeneuve wrote:
> On Tue,  4 Jun 2024 21:27:25 +0800
> Hui Wang <hui.wang@canonical.com> wrote:
>
>> In some designs, the chip reset pin is connected to a gpio, this
> "and this ..."
>
>> gpio needs to be set correctly before probing the driver, so adding
> "so add ..."
>
>
OK, will fix them in the v3.

Thanks.

Re: [PATCH v2 2/2] serial: sc16is7xx: hard reset the chip if reset-gpios is defined in dt

[Hui Wang]

On 6/4/24 21:42, Andy Shevchenko wrote:
> On Tue, Jun 04, 2024 at 09:27:26PM +0800, Hui Wang wrote:
>> Certain designs connect a gpio to the reset pin, and the reset pin
> GPIO
Got it.
>> needs to be setup correctly before accessing the chip.
>>
>> Here adding a function to handle the chip reset. If the reset-gpios is
>> defined in the dt, do the hard reset through this gpio, othwerwise do
> DT
Got it.
>
>> the soft reset as before.
> ...
>
>> +static int sc16is7xx_reset(struct device *dev, struct regmap *regmaps[])
>> +{
>> +	struct gpio_desc *reset_gpiod;
>> +	reset_gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
>> +	if (!reset_gpiod)
>> +		/* soft reset device, purging any pending irq / data */
>> +		regmap_write(regmaps[0], SC16IS7XX_IOCONTROL_REG,
>> +			     SC16IS7XX_IOCONTROL_SRESET_BIT);
>> +	else if (!IS_ERR(reset_gpiod)) {
>> +		/* delay 5 us (at least 3 us) and deassert the gpio to exit the hard reset */
>> +		udelay(5);
>> +		gpiod_set_value_cansleep(reset_gpiod, 0);
>> +	} else
>> +		return PTR_ERR(reset_gpiod);
> You can do better here.
>
> 	reset_gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> 	if (IS_ERR(reset_gpiod))
> 		return PTR_ERR(reset_gpiod);
>
> 	if (reset_gpiod) {
> 		/* delay 5 us (at least 3 us) and deassert the GPIO to exit the hard reset */
> 		fsleep(5);
> 		gpiod_set_value_cansleep(reset_gpiod, 0);
> 	} else {
> 		/* soft reset device, purging any pending IRQ / data */
> 		regmap_write(regmaps[0], SC16IS7XX_IOCONTROL_REG,
> 			     SC16IS7XX_IOCONTROL_SRESET_BIT);
> 	}

OK, got it, will fix all comment in the v3.

Thanks.

>> +	return 0;
>> +}

Re: [PATCH v2 2/2] serial: sc16is7xx: hard reset the chip if reset-gpios is defined in dt

[Hui Wang]

On 6/4/24 22:23, Hugo Villeneuve wrote:
> On Tue,  4 Jun 2024 21:27:26 +0800
> Hui Wang <hui.wang@canonical.com> wrote:
>
> Hi Hui,
>
>> Certain designs connect a gpio to the reset pin, and the reset pin
>> needs to be setup correctly before accessing the chip.
>>
>> Here adding a function to handle the chip reset. If the reset-gpios is
>> defined in the dt, do the hard reset through this gpio, othwerwise do
>> the soft reset as before.
>>
>> Reviewed-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> I never gave you permission to add this tag, remove it. Make sure you
> fully understand the meaning of tags by reading patches submission
> guidelines.

I misunderstood the meaning of "reviewed-by", will remove it and study 
the guidelines.

>
>> Signed-off-by: Hui Wang <hui.wang@canonical.com>
>> ---
>> In the v2:
>>   - move the soft reset and hard reset into one fucntion
>>   - move the reset function to sc16is7xx.c and call it in _probe()
>>   - add udelay(5) before deasserting the gpio reset pin
>>
>>   drivers/tty/serial/sc16is7xx.c | 28 ++++++++++++++++++++++++----
>>   1 file changed, 24 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
>> index bf0065d1c8e9..119abfb4607c 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,25 @@ static const struct serial_rs485 sc16is7xx_rs485_supported = {
>>   	.delay_rts_after_send = 1,	/* Not supported but keep returning -EINVAL */
>>   };
>>   
> Add function description from original comment "Reset device,
> purging any pending irq / data", since the comment applies to both
> hardware and software reset,
Got it.
>> +static int sc16is7xx_reset(struct device *dev, struct regmap *regmaps[])
> Simply pass "struct regmap *regmap" as the second argument. See
> sc16is7xx_setup_mctrl_ports() for example.
Got it.
>
>> +{
>> +	struct gpio_desc *reset_gpiod;
> reset_gpiod -> reset_gpio
Got it.
>> +
>> +	reset_gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
>> +	if (!reset_gpiod)
> Follow Andy's suggestion here.
OK.
>
>> +		/* soft reset device, purging any pending irq / data */
> "Software reset".
Got it.
>> +		regmap_write(regmaps[0], SC16IS7XX_IOCONTROL_REG,
>> +			     SC16IS7XX_IOCONTROL_SRESET_BIT);
>> +	else if (!IS_ERR(reset_gpiod)) {
>> +		/* delay 5 us (at least 3 us) and deassert the gpio to exit the hard reset */
> You can omit the "delay 5 us" since it is obvious from the code. Maybe
> add that "The minimum reset pulse width is 3 us" as stated in the
> datasheet.
>
> As a general note for your comments: capitalize the first letter,
> ex: "Deassert GPIO" and not "deassert GPIO".
OK.
>
>> +		udelay(5);
>> +		gpiod_set_value_cansleep(reset_gpiod, 0);
> Move the comment "deassert the gpio to exit the hard reset" here. You
> could also simplify it as "Deassert GPIO.".
>
OK.
>> +	} else
>> +		return PTR_ERR(reset_gpiod);
> return dev_err_probe(dev, PTR_ERR(reset_gpiod), "Failed to get reset
> GPIO\n");

Got it, will address all comment in the v3.

Thanks.

>
>> +
>> +	return 0;
>> +}
>> +
>>   int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>>   		    struct regmap *regmaps[], int irq)
>>   {
>> @@ -1527,6 +1547,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);
>> +	if (ret)
>> +		goto out_clk;
>> +
>>   	kthread_init_worker(&s->kworker);
>>   	s->kworker_task = kthread_run(kthread_worker_fn, &s->kworker,
>>   				      "sc16is7xx");
>> @@ -1536,10 +1560,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 v2 2/2] serial: sc16is7xx: hard reset the chip if reset-gpios is defined in dt

[Hui Wang]

On 6/4/24 22:34, Hugo Villeneuve wrote:
> On Tue,  4 Jun 2024 21:27:26 +0800
> Hui Wang <hui.wang@canonical.com> wrote:
>
> Hi Hui,
>
>> Certain designs connect a gpio to the reset pin, and the reset pin
> "Some boards..."
>
>> needs to be setup correctly before accessing the chip.
>>
>> Here adding a function to handle the chip reset. If the reset-gpios is
> "Add a function..."
>
>
Got them, will fix them in the v3.

Thanks.

RE: [PATCH v2 2/2] serial: sc16is7xx: hard reset the chip if reset-gpios is defined in dt

[Maarten Brock]

> From: Hugo Villeneuve <hugo@hugovil.com>
> Sent: Tuesday, 4 June 2024 16:23

<...>

> Add function description from original comment "Reset device,
> purging any pending irq / data", since the comment applies to both
> hardware and software reset,

No it does not (at this moment). See below.

> > +static int sc16is7xx_reset(struct device *dev, struct regmap *regmaps[])
> 
> Simply pass "struct regmap *regmap" as the second argument. See
> sc16is7xx_setup_mctrl_ports() for example.
> 
> > +{
> > +	struct gpio_desc *reset_gpiod;
> 
> reset_gpiod -> reset_gpio
> 
> > +	else if (!IS_ERR(reset_gpiod)) {
> > +		/* delay 5 us (at least 3 us) and deassert the gpio to exit the hard
> reset */
> 
> You can omit the "delay 5 us" since it is obvious from the code. Maybe
> add that "The minimum reset pulse width is 3 us" as stated in the
> datasheet.
> 
> As a general note for your comments: capitalize the first letter,
> ex: "Deassert GPIO" and not "deassert GPIO".
> 
> 
> > +		udelay(5);
> > +		gpiod_set_value_cansleep(reset_gpiod, 0);
> 
> Move the comment "deassert the gpio to exit the hard reset" here. You
> could also simplify it as "Deassert GPIO.".

The problem here is that this only deasserts the reset if it were asserted before.
Nothing happens if it already was deasserted. This makes the delay also pretty
useless.

To make this a proper reset pulse for the device you must first assert the reset,
then wait >3us, and finally deassert the reset.

Maarten Brock

Re: [PATCH v2 2/2] serial: sc16is7xx: hard reset the chip if reset-gpios is defined in dt

[Hui Wang]

On 6/5/24 18:30, Maarten Brock wrote:
>> From: Hugo Villeneuve <hugo@hugovil.com>
>> Sent: Tuesday, 4 June 2024 16:23
> <...>
>
>> Add function description from original comment "Reset device,
>> purging any pending irq / data", since the comment applies to both
>> hardware and software reset,
> No it does not (at this moment). See below.
>
>>> +static int sc16is7xx_reset(struct device *dev, struct regmap *regmaps[])
>> Simply pass "struct regmap *regmap" as the second argument. See
>> sc16is7xx_setup_mctrl_ports() for example.
>>
>>> +{
>>> +	struct gpio_desc *reset_gpiod;
>> reset_gpiod -> reset_gpio
>>
>>> +	else if (!IS_ERR(reset_gpiod)) {
>>> +		/* delay 5 us (at least 3 us) and deassert the gpio to exit the hard
>> reset */
>>
>> You can omit the "delay 5 us" since it is obvious from the code. Maybe
>> add that "The minimum reset pulse width is 3 us" as stated in the
>> datasheet.
>>
>> As a general note for your comments: capitalize the first letter,
>> ex: "Deassert GPIO" and not "deassert GPIO".
>>
>>
>>> +		udelay(5);
>>> +		gpiod_set_value_cansleep(reset_gpiod, 0);
>> Move the comment "deassert the gpio to exit the hard reset" here. You
>> could also simplify it as "Deassert GPIO.".
> The problem here is that this only deasserts the reset if it were asserted before.
> Nothing happens if it already was deasserted. This makes the delay also pretty
> useless.
>
> To make this a proper reset pulse for the device you must first assert the reset,
> then wait >3us, and finally deassert the reset.
>
> Maarten Brock
Hi Maarten,

My understanding is when calling devm_gpiod_get_optional(dev, "reset", 
GPIOD_OUT_LOW) and returning a valid (gpio_desc *), the flag 
GPIOD_OUT_LOW guarantees the GPIO is set to output and low (assert the 
reset pin).

Thanks,

Hui.


>

RE: [PATCH v2 2/2] serial: sc16is7xx: hard reset the chip if reset-gpios is defined in dt

[Maarten Brock]

> On 6/5/24 18:30, Maarten Brock wrote:
> >> From: Hugo Villeneuve <hugo@hugovil.com>
> >> Sent: Tuesday, 4 June 2024 16:23
> > <...>
> >
> >> Add function description from original comment "Reset device,
> >> purging any pending irq / data", since the comment applies to both
> >> hardware and software reset,
> > No it does not (at this moment). See below.
> >
> >>> +static int sc16is7xx_reset(struct device *dev, struct regmap *regmaps[])
> >> Simply pass "struct regmap *regmap" as the second argument. See
> >> sc16is7xx_setup_mctrl_ports() for example.
> >>
> >>> +{
> >>> +	struct gpio_desc *reset_gpiod;
> >> reset_gpiod -> reset_gpio
> >>
> >>> +	else if (!IS_ERR(reset_gpiod)) {
> >>> +		/* delay 5 us (at least 3 us) and deassert the gpio to exit the hard
> >> reset */
> >>
> >> You can omit the "delay 5 us" since it is obvious from the code. Maybe
> >> add that "The minimum reset pulse width is 3 us" as stated in the
> >> datasheet.
> >>
> >> As a general note for your comments: capitalize the first letter,
> >> ex: "Deassert GPIO" and not "deassert GPIO".
> >>
> >>
> >>> +		udelay(5);
> >>> +		gpiod_set_value_cansleep(reset_gpiod, 0);
> >> Move the comment "deassert the gpio to exit the hard reset" here. You
> >> could also simplify it as "Deassert GPIO.".
> > The problem here is that this only deasserts the reset if it were asserted before.
> > Nothing happens if it already was deasserted. This makes the delay also pretty
> > useless.
> >
> > To make this a proper reset pulse for the device you must first assert the reset,
> > then wait >3us, and finally deassert the reset.
> >
> > Maarten Brock
> Hi Maarten,
> 
> My understanding is when calling devm_gpiod_get_optional(dev, "reset",
> GPIOD_OUT_LOW) and returning a valid (gpio_desc *), the flag
> GPIOD_OUT_LOW guarantees the GPIO is set to output and low (assert the
> reset pin).

Ah, right. Sorry, I missed that.
So GPIOD_OUT_LOW disregards the inversion from GPIO_ACTIVE_LOW.
And gpiod_set_value_cansleep(reset_gpiod, 0) uses the inversion to make the pin high.
Looks fine to me now.

> Thanks,
> 
> Hui.

Maarten

Re: [PATCH v2 2/2] serial: sc16is7xx: hard reset the chip if reset-gpios is defined in dt

[Andy Shevchenko]

On Wed, Jun 5, 2024 at 1:55 PM Hui Wang <hui.wang@canonical.com> wrote:
> On 6/5/24 18:30, Maarten Brock wrote:
> >> From: Hugo Villeneuve <hugo@hugovil.com>
> >> Sent: Tuesday, 4 June 2024 16:23

<...>

> >> Add function description from original comment "Reset device,
> >> purging any pending irq / data", since the comment applies to both
> >> hardware and software reset,
> > No it does not (at this moment). See below.

...

> > The problem here is that this only deasserts the reset if it were asserted before.
> > Nothing happens if it already was deasserted. This makes the delay also pretty
> > useless.
> >
> > To make this a proper reset pulse for the device you must first assert the reset,
> > then wait >3us, and finally deassert the reset.

> My understanding is when calling devm_gpiod_get_optional(dev, "reset",
> GPIOD_OUT_LOW) and returning a valid (gpio_desc *), the flag
> GPIOD_OUT_LOW guarantees the GPIO is set to output and low (assert the
> reset pin).

No, this is logical, not physical state. Maarten is correct. How did
you test this?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] serial: sc16is7xx: hard reset the chip if reset-gpios is defined in dt

[Andy Shevchenko]

On Wed, Jun 5, 2024 at 2:19 PM Maarten Brock <Maarten.Brock@sttls.nl> wrote:
> > On 6/5/24 18:30, Maarten Brock wrote:
> > >> From: Hugo Villeneuve <hugo@hugovil.com>
> > >> Sent: Tuesday, 4 June 2024 16:23

<...>

> > >> Add function description from original comment "Reset device,
> > >> purging any pending irq / data", since the comment applies to both
> > >> hardware and software reset,
> > > No it does not (at this moment). See below.

...

> > > The problem here is that this only deasserts the reset if it were asserted before.
> > > Nothing happens if it already was deasserted. This makes the delay also pretty
> > > useless.
> > >
> > > To make this a proper reset pulse for the device you must first assert the reset,
> > > then wait >3us, and finally deassert the reset.
> > >
> > > Maarten Brock
> > Hi Maarten,
> >
> > My understanding is when calling devm_gpiod_get_optional(dev, "reset",
> > GPIOD_OUT_LOW) and returning a valid (gpio_desc *), the flag
> > GPIOD_OUT_LOW guarantees the GPIO is set to output and low (assert the
> > reset pin).
>
> Ah, right. Sorry, I missed that.
> So GPIOD_OUT_LOW disregards the inversion from GPIO_ACTIVE_LOW.
> And gpiod_set_value_cansleep(reset_gpiod, 0) uses the inversion to make the pin high.
> Looks fine to me now.

No, you used the correct terms "assert"/"deassert", the parameter
there is logical, means "deassert". It was differently in the legacy
GPIO APIs, which are almost gone.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] serial: sc16is7xx: hard reset the chip if reset-gpios is defined in dt

[Krzysztof Kozlowski]

On 05/06/2024 13:19, Maarten Brock wrote:
>>> To make this a proper reset pulse for the device you must first assert the reset,
>>> then wait >3us, and finally deassert the reset.
>>>
>>> Maarten Brock
>> Hi Maarten,
>>
>> My understanding is when calling devm_gpiod_get_optional(dev, "reset",
>> GPIOD_OUT_LOW) and returning a valid (gpio_desc *), the flag
>> GPIOD_OUT_LOW guarantees the GPIO is set to output and low (assert the
>> reset pin).
> 
> Ah, right. Sorry, I missed that.
> So GPIOD_OUT_LOW disregards the inversion from GPIO_ACTIVE_LOW.

It doesn't.

> And gpiod_set_value_cansleep(reset_gpiod, 0) uses the inversion to make the pin high.
> Looks fine to me now.

They both respect pin polarity.

Best regards,
Krzysztof

Re: [PATCH v2 2/2] serial: sc16is7xx: hard reset the chip if reset-gpios is defined in dt

[Hui Wang]

On 6/5/24 19:24, Krzysztof Kozlowski wrote:
> On 05/06/2024 13:19, Maarten Brock wrote:
>>>> To make this a proper reset pulse for the device you must first assert the reset,
>>>> then wait >3us, and finally deassert the reset.
>>>>
>>>> Maarten Brock
>>> Hi Maarten,
>>>
>>> My understanding is when calling devm_gpiod_get_optional(dev, "reset",
>>> GPIOD_OUT_LOW) and returning a valid (gpio_desc *), the flag
>>> GPIOD_OUT_LOW guarantees the GPIO is set to output and low (assert the
>>> reset pin).
>> Ah, right. Sorry, I missed that.
>> So GPIOD_OUT_LOW disregards the inversion from GPIO_ACTIVE_LOW.
> It doesn't.
>
>> And gpiod_set_value_cansleep(reset_gpiod, 0) uses the inversion to make the pin high.
>> Looks fine to me now.
> They both respect pin polarity.

Will correct it.

Thanks.

>
> Best regards,
> Krzysztof
>

Re: [PATCH v2 2/2] serial: sc16is7xx: hard reset the chip if reset-gpios is defined in dt

[Hugo Villeneuve]

On Wed, 5 Jun 2024 20:55:21 +0800
Hui Wang <hui.wang@canonical.com> wrote:

> 
> On 6/5/24 19:19, Andy Shevchenko wrote:
> > On Wed, Jun 5, 2024 at 1:55 PM Hui Wang<hui.wang@canonical.com>  wrote:
> >> On 6/5/24 18:30, Maarten Brock wrote:
> >>>> From: Hugo Villeneuve<hugo@hugovil.com>
> >>>> Sent: Tuesday, 4 June 2024 16:23
> > <...>
> >
> >>>> Add function description from original comment "Reset device,
> >>>> purging any pending irq / data", since the comment applies to both
> >>>> hardware and software reset,
> >>> No it does not (at this moment). See below.
> > ...
> >
> >>> The problem here is that this only deasserts the reset if it were asserted before.
> >>> Nothing happens if it already was deasserted. This makes the delay also pretty
> >>> useless.
> >>>
> >>> To make this a proper reset pulse for the device you must first assert the reset,
> >>> then wait >3us, and finally deassert the reset.
> >> My understanding is when calling devm_gpiod_get_optional(dev, "reset",
> >> GPIOD_OUT_LOW) and returning a valid (gpio_desc *), the flag
> >> GPIOD_OUT_LOW guarantees the GPIO is set to output and low (assert the
> >> reset pin).
> > No, this is logical, not physical state. Maarten is correct. How did
> > you test this?
> 
> Yes, Maarten, Krzysztof and you are correct. Thanks for pointing out 
> this error.
> 
> If I call "reset_gpio = devm_gpiod_get_optional(dev, "reset", 
> GPIOD_OUT_HIGH);" I will get the result as below:
> 
> # cat /sys/kernel/debug/gpio
> 
> gpiochip4: GPIOs 128-159, parent: platform/30240000.gpio, 30240000.gpio: 
> gpio-141 ( |reset ) out lo ACTIVE LOW
> 
> If I call "reset_gpio = devm_gpiod_get_optional(dev, "reset", 
> GPIOD_OUT_LOW);" I will get the result as below:
> 
> # cat /sys/kernel/debug/gpio
> 
> gpiochip4: GPIOs 128-159, parent: platform/30240000.gpio, 30240000.gpio: 
> gpio-141 ( |reset ) out hi ACTIVE LOW
> 
> I tested the reset pin by dumping chip registers, if the reset pin is 
> asserted (out lo), I will get the result like this:
> 
> # cat /sys/kernel/debug/regmap/spi0.0-port0/registers 1: 10 2: ff 3: 00 
> 4: ec 5: ff 6: ff 7: ff 8: ff 9: ff a: ff b: ff c: ff d: ff e: ff f: 06

Hi Hui,
the best way to test a reset pin is with a voltmeter, if you can. It is
way too easy to get confused with reset pins values/polarities, etc.

By the way, if the reset pin is asserted, you cannot communicate with
the device, therefore dumping registers cannot work for debug purpose.

Hugo.


> 
> If the reset pin is deasserted (out hi), I will get:
> 
> # cat /sys/kernel/debug/regmap/spi0.0-port0/registers 1: 10 2: 01 3: 00 
> 4: 00 5: 60 6: 00 7: 2e 8: 40 9: 00 a: 00 b: ff c: 00 d: 00 e: 00 f: 06
> 
> My original code set the reset GPIO to high (the reset pin is 
> deasserted) continuously, so I didn't notice this hidden error.
> 
> Thanks,
> 
> Hui.
> 
> >


-- 
Hugo Villeneuve

Re: [PATCH v2 2/2] serial: sc16is7xx: hard reset the chip if reset-gpios is defined in dt

[Hui Wang]

On 6/5/24 23:32, Hugo Villeneuve wrote:
> On Wed, 5 Jun 2024 20:55:21 +0800
> Hui Wang <hui.wang@canonical.com> wrote:
>
>> On 6/5/24 19:19, Andy Shevchenko wrote:
>>> On Wed, Jun 5, 2024 at 1:55 PM Hui Wang<hui.wang@canonical.com>  wrote:
>>>> On 6/5/24 18:30, Maarten Brock wrote:
...
>> # cat /sys/kernel/debug/regmap/spi0.0-port0/registers 1: 10 2: ff 3: 00
>> 4: ec 5: ff 6: ff 7: ff 8: ff 9: ff a: ff b: ff c: ff d: ff e: ff f: 06
> Hi Hui,
> the best way to test a reset pin is with a voltmeter, if you can. It is
> way too easy to get confused with reset pins values/polarities, etc.
Yes. got it.
>
> By the way, if the reset pin is asserted, you cannot communicate with
> the device, therefore dumping registers cannot work for debug purpose.

Got it.  I just use it to check the reset pin status. If the returned 
register values look reasonable (not many 0xff), It means the reset GPIO 
is de-asserted.

Thanks.

> Hugo.
>
>
>>
>