On Fri, 14 Jun 2024 18:29:52 +0800
Hui Wang <hui.wang@xxxxxxxxxxxxx> wrote:
Hi Hui,
> Some boards connect a GPIO to the reset pin, and the reset pin needs
> to be set up 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, otherwise do software
> reset as before.
>
> Signed-off-by: Hui Wang <hui.wang@xxxxxxxxxxxxx>
> ---
> in the V5:
> - change setup to set up in the commit header
> - change othwerwise to otherwise in the commit header
> - change usleep_range(5, 10) to fsleep(5)
>
> 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..eefa40006c71 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.
> + */
I would replace all the above comments with:
/* Assert reset GPIO if defined and valid. */
The correct polarity is already defined by the device
tree reset-gpios entry, and can be high or low depending on the design
(ex: there can be an inverter between the CPU and the chip reset input,
etc).
> + 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. */
> + fsleep(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)
> {
> @@ -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
>
I could not test the validity of the 3us delay since I do not have an
oscilloscope, but testing with a 10s delay instead and a
multimeter showed that it works ok. You can add my Tested-by tag:
Tested-by: Hugo Villeneuve <hvilleneuve@xxxxxxxxxxxx>
And if you modify the comment as I suggested above, then you can add my
R-b tag:
Reviewed-by: Hugo Villeneuve <hvilleneuve@xxxxxxxxxxxx>
--
Hugo Villeneuve
