Re: [PATCH 2/2] serial: sc16is7xx: setup reset pin if it is defined in device tree

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

 



On Mon,  3 Jun 2024 20:37:10 +0800
Hui Wang <hui.wang@xxxxxxxxxxxxx> 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 reset pin. This change has no
> impact if there is no reset_gpios defined in the device tree.
> 
> Signed-off-by: Hui Wang <hui.wang@xxxxxxxxxxxxx>
> ---
>  drivers/tty/serial/sc16is7xx.c     | 22 ++++++++++++++++++++++
>  drivers/tty/serial/sc16is7xx.h     |  2 ++
>  drivers/tty/serial/sc16is7xx_i2c.c |  2 ++
>  drivers/tty/serial/sc16is7xx_spi.c |  2 ++
>  4 files changed, 28 insertions(+)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index bf0065d1c8e9..53bfb603b03c 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -19,6 +19,7 @@
>  #include <linux/kthread.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
> +#include <linux/of_gpio.h>
>  #include <linux/property.h>
>  #include <linux/regmap.h>
>  #include <linux/sched.h>
> @@ -1467,6 +1468,27 @@ static const struct serial_rs485 sc16is7xx_rs485_supported = {
>  	.delay_rts_after_send = 1,	/* Not supported but keep returning -EINVAL */
>  };
>  
> +void sc16is7xx_setup_reset_pin(struct device *dev)

Potentially rename to sc16is7xx_reset() based on my comments below
about software reset.

> +{
> +	struct device_node *np = dev->of_node;
> +	int reset_gpio, err;
> +
> +	reset_gpio = of_get_named_gpio(np, "reset-gpios", 0);

Maybe use devm_gpiod_get_optional() to simplify and avoid OF-specific
code.

> +	if (!gpio_is_valid(reset_gpio))
> +		return;
> +
> +	err = devm_gpio_request_one(dev, reset_gpio, GPIOF_OUT_INIT_LOW,
> +				    "sc16is7xx-reset");
> +	if (err) {
> +		dev_err(dev, "failed to request sc16is7xx-reset-gpios: %d\n", err);
> +		return;

When this error happens, you return no error to the calling function,
why? If you specify a reset GPIO in your device tree, and you cannot use
it, seems like an error worth reporting.

> +	}
> +
> +	/* Deassert the reset pin */

Do you respect the manufacturer's minimum reset pulse width? The
datasheet states that its 3 us, so maybe add a delay before deassertion.

> +	gpio_set_value_cansleep(reset_gpio, 1);
> +}
> +EXPORT_SYMBOL_GPL(sc16is7xx_setup_reset_pin);
> +
>  int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>  		    struct regmap *regmaps[], int irq)
>  {
> diff --git a/drivers/tty/serial/sc16is7xx.h b/drivers/tty/serial/sc16is7xx.h
> index afb784eaee45..f4ae114cc41a 100644
> --- a/drivers/tty/serial/sc16is7xx.h
> +++ b/drivers/tty/serial/sc16is7xx.h
> @@ -33,6 +33,8 @@ const char *sc16is7xx_regmap_name(u8 port_id);
>  
>  unsigned int sc16is7xx_regmap_port_mask(unsigned int port_id);
>  
> +void sc16is7xx_setup_reset_pin(struct device *dev);
> +
>  int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>  		    struct regmap *regmaps[], int irq);
>  
> diff --git a/drivers/tty/serial/sc16is7xx_i2c.c b/drivers/tty/serial/sc16is7xx_i2c.c
> index 3ed47c306d85..9833c3b935c2 100644
> --- a/drivers/tty/serial/sc16is7xx_i2c.c
> +++ b/drivers/tty/serial/sc16is7xx_i2c.c
> @@ -21,6 +21,8 @@ static int sc16is7xx_i2c_probe(struct i2c_client *i2c)
>  	if (!devtype)
>  		return dev_err_probe(&i2c->dev, -ENODEV, "Failed to match device\n");
>  
> +	sc16is7xx_setup_reset_pin(&i2c->dev);

Move this call inside sc16is7xx_probe() function, since it is common to
both i2c and spi interfaces. Also, you will see in sc16is7xx_probe()
that we already issue a software reset. If you
specify a hardware reset pin, then you shouldn't issue the software
reset.

> +
>  	memcpy(&regcfg, &sc16is7xx_regcfg, sizeof(struct regmap_config));
>  
>  	for (i = 0; i < devtype->nr_uart; i++) {
> diff --git a/drivers/tty/serial/sc16is7xx_spi.c b/drivers/tty/serial/sc16is7xx_spi.c
> index 73df36f8a7fd..ce38561faaf0 100644
> --- a/drivers/tty/serial/sc16is7xx_spi.c
> +++ b/drivers/tty/serial/sc16is7xx_spi.c
> @@ -38,6 +38,8 @@ static int sc16is7xx_spi_probe(struct spi_device *spi)
>  	if (!devtype)
>  		return dev_err_probe(&spi->dev, -ENODEV, "Failed to match device\n");
>  
> +	sc16is7xx_setup_reset_pin(&spi->dev);
> +
>  	memcpy(&regcfg, &sc16is7xx_regcfg, sizeof(struct regmap_config));
>  
>  	for (i = 0; i < devtype->nr_uart; i++) {
> -- 
> 2.34.1
> 
> 
> 


-- 
Hugo Villeneuve




[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