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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxxxxxxx>
---
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




[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux PPP]     [Linux FS]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Linmodem]     [Device Mapper]     [Linux Kernel for ARM]

  Powered by Linux