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

[PATCH v4 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>
---
No change between v3 and v4.

 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 v4 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 v4:
 - Move sc16is7xx_reset(dev, regmaps[0]) to the where software reset was located
 - Move /* Deassert GPIO */ to the end of corresponding statement
 - Change the comment before reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH)
 - Change udelay(5) to usleep_range(5, 10)
 
 drivers/tty/serial/sc16is7xx.c | 34 +++++++++++++++++++++++++++++++---
 1 file changed, 31 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index bf0065d1c8e9..a67c4eda0c03 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,32 @@ 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 input is active low, and flag GPIOD_OUT_HIGH ensures the
+	 * GPIO is low once devm_gpiod_get_optional returns a valid gpio_desc.
+	 */
+	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. */
+		usleep_range(5, 10);
+		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)
 {
@@ -1536,9 +1563,9 @@ 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);
+	ret = sc16is7xx_reset(dev, regmaps[0]);
+	if (ret)
+		goto out_kthread;
 
 	/* Mark each port line and status as uninitialised. */
 	for (i = 0; i < devtype->nr_uart; ++i) {
@@ -1663,6 +1690,7 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
 			uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port);
 	}
 
+out_kthread:
 	kthread_stop(s->kworker_task);
 
 out_clk:
-- 
2.34.1

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

[Krzysztof Kozlowski]

On 13/06/2024 10:25, 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>

<form letter>
This is a friendly reminder during the review process.

It looks like you received a tag and forgot to add it.

If you do not know the process, here is a short explanation:
Please add Acked-by/Reviewed-by/Tested-by tags when posting new
versions, under or above your Signed-off-by tag. Tag is "received", when
provided in a message replied to you on the mailing list. Tools like b4
can help here. However, there's no need to repost patches *only* to add
the tags. The upstream maintainer will do that for tags received on the
version they apply.

https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577

If a tag was not added on purpose, please state why and what changed.
</form letter>

Best regards,
Krzysztof

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

[Hui Wang]

On 6/13/24 17:08, Krzysztof Kozlowski wrote:
> On 13/06/2024 10:25, 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>
> <form letter>
> This is a friendly reminder during the review process.
>
> It looks like you received a tag and forgot to add it.
>
> If you do not know the process, here is a short explanation:
> Please add Acked-by/Reviewed-by/Tested-by tags when posting new
> versions, under or above your Signed-off-by tag. Tag is "received", when
> provided in a message replied to you on the mailing list. Tools like b4
> can help here. However, there's no need to repost patches *only* to add
> the tags. The upstream maintainer will do that for tags received on the
> version they apply.
>
> https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577
>
> If a tag was not added on purpose, please state why and what changed.
> </form letter>

Got it. Will add that tag if I need to change sth in the v4 and need to 
send the v5. :-) I thought the Ack is for Hugo's explanation rather than 
for my 1st patch, I mis-understood it. And I plan to study b4 in the 
near future.

Thanks,

Hui.

> Best regards,
> Krzysztof
>

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

[Krzysztof Kozlowski]

On 13/06/2024 12:22, Hui Wang wrote:
> 
> On 6/13/24 17:08, Krzysztof Kozlowski wrote:
>> On 13/06/2024 10:25, 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>
>> <form letter>
>> This is a friendly reminder during the review process.
>>
>> It looks like you received a tag and forgot to add it.
>>
>> If you do not know the process, here is a short explanation:
>> Please add Acked-by/Reviewed-by/Tested-by tags when posting new
>> versions, under or above your Signed-off-by tag. Tag is "received", when
>> provided in a message replied to you on the mailing list. Tools like b4
>> can help here. However, there's no need to repost patches *only* to add
>> the tags. The upstream maintainer will do that for tags received on the
>> version they apply.
>>
>> https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577
>>
>> If a tag was not added on purpose, please state why and what changed.
>> </form letter>
> 
> Got it. Will add that tag if I need to change sth in the v4 and need to 
> send the v5. :-) I thought the Ack is for Hugo's explanation rather than 
> for my 1st patch, I mis-understood it. And I plan to study b4 in the 
> near future.
> 

Please start using b4. There would be no such ambiguity from your side,
because maintainers know what they do and tool handles it correctly.


Best regards,
Krzysztof

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

[Andy Shevchenko]

On Thu, Jun 13, 2024 at 10:26 AM Hui Wang <hui.wang@canonical.com> wrote:

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

set up

> 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.

...

> +               /* The minimum reset pulse width is 3 us. */
> +               usleep_range(5, 10);

Simply use fsleep() and it will take care of the sane API to be called.

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

-- 
With Best Regards,
Andy Shevchenko

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

[Hui Wang]

On 6/14/24 04:47, Andy Shevchenko wrote:
> On Thu, Jun 13, 2024 at 10:26 AM Hui Wang <hui.wang@canonical.com> wrote:
>
>> Some boards connect a GPIO to the reset pin, and the reset pin needs
>> to be setup correctly before accessing the chip.
> set up
Got it.
>> 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
Got it.
>> reset as before.
> ...
>
>> +               /* The minimum reset pulse width is 3 us. */
>> +               usleep_range(5, 10);
> Simply use fsleep() and it will take care of the sane API to be called.
OK, Thanks.
>
>> +               gpiod_set_value_cansleep(reset_gpio, 0); /* Deassert GPIO */