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

[Hugo Villeneuve <hugo@hugovil.com>]

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