| Message ID | 20240614102952.679806-2-hui.wang@canonical.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
On Fri, 14 Jun 2024 18:29:52 +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 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@canonical.com> > --- > 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@dimonoff.com> And if you modify the comment as I suggested above, then you can add my R-b tag: Reviewed-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
On Mon, 17 Jun 2024 18:49:30 +0200 Lech Perczak <lech.perczak@camlingroup.com> wrote: > Hello, > > W dniu 17.06.2024 o 18:03, Hugo Villeneuve pisze: > > On Fri, 14 Jun 2024 18:29:52 +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 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@canonical.com> > >> --- > >> 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). > Seconded - this way it's clear for the reader. > >> + 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@dimonoff.com> > My hardware doesn't connect this line to the CPU's GPIOs, so I couldn't test this properly - but you can at least have my R-b tag. Hi Lech, just to mention that on my hardware the SC16 reset line is also not connected to the CPU, so I only tested the GPIO assert/deassert logic. Hugo. > > > > And if you modify the comment as I suggested above, then you can add my > > R-b tag: > > > > Reviewed-by: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > > > > > -- > > Hugo Villeneuve > > > > ___ > > This email originated from outside of Camlin Group. Do not click on links or open attachments unless you are confident they are secure. If in doubt, please raise a security incident via the following portal: Camlin Helpdesk – Report an Information Security Incident/Non-Conformance <https://camlin-helpdesk.atlassian.net/servicedesk/customer/portal/2/group/34/create/62> > > -- > Pozdrawiam/With kind regards, > Lech Perczak > > Sr. Software Engineer > Camlin Technologies Poland Limited Sp. z o.o. > Strzegomska 54, > 53-611 Wroclaw > Tel: (+48) 71 75 000 16 > Email: lech.perczak@camlingroup.com > Website: http://www.camlingroup.com
On Mon, Jun 17, 2024 at 6:49 PM Lech Perczak <lech.perczak@camlingroup.com> wrote: > W dniu 17.06.2024 o 18:03, Hugo Villeneuve pisze: > On Fri, 14 Jun 2024 18:29:52 +0800 > Hui Wang <hui.wang@canonical.com> wrote: ... > My hardware doesn't connect this line to the CPU's GPIOs, so I couldn't test this properly - but you can at least have my R-b tag. Lech, you need to provide a formal tag as it's described in Submitting Patches.
W dniu 17.06.2024 o 22:43, Andy Shevchenko pisze: > On Mon, Jun 17, 2024 at 6:49 PM Lech Perczak > <lech.perczak@camlingroup.com> wrote: >> W dniu 17.06.2024 o 18:03, Hugo Villeneuve pisze: >> On Fri, 14 Jun 2024 18:29:52 +0800 >> Hui Wang <hui.wang@canonical.com> wrote: > ... > >> My hardware doesn't connect this line to the CPU's GPIOs, so I couldn't test this properly - but you can at least have my R-b tag. > Lech, you need to provide a formal tag as it's described in Submitting Patches. I already did for v4, but to make it clear for v5 too: Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com> > > > -- > With Best Regards, > Andy Shevchenko
On 6/18/24 00:03, Hugo Villeneuve wrote: > On Fri, 14 Jun 2024 18:29:52 +0800 > Hui Wang <hui.wang@canonical.com> wrote: > > Hi Hui, > >> Some boards connect a GPIO to the reset pin, and the reset pin needs ... >> + 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). > Agree with that, I will change it in the v6. >> + 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"); >> + ... >> +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@dimonoff.com> > > And if you modify the comment as I suggested above, then you can add my > R-b tag: > > Reviewed-by: Hugo Villeneuve <hvilleneuve@dimonoff.com> OK. thanks. >
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. + */ + 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:
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@canonical.com> --- 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(-)