| Message ID | 20240604132726.1272475-2-hui.wang@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] dt-bindings: serial: sc16is7xx: add reset-gpios | expand |
On Tue, 4 Jun 2024 21:27:26 +0800 Hui Wang <hui.wang@canonical.com> 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 chip reset. If the reset-gpios is > defined in the dt, do the hard reset through this gpio, othwerwise do > the soft reset as before. > > Reviewed-by: Hugo Villeneuve <hvilleneuve@dimonoff.com> I never gave you permission to add this tag, remove it. Make sure you fully understand the meaning of tags by reading patches submission guidelines. > Signed-off-by: Hui Wang <hui.wang@canonical.com> > --- > In the v2: > - move the soft reset and hard reset into one fucntion > - move the reset function to sc16is7xx.c and call it in _probe() > - add udelay(5) before deasserting the gpio reset pin > > drivers/tty/serial/sc16is7xx.c | 28 ++++++++++++++++++++++++---- > 1 file changed, 24 insertions(+), 4 deletions(-) > > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c > index bf0065d1c8e9..119abfb4607c 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,25 @@ static const struct serial_rs485 sc16is7xx_rs485_supported = { > .delay_rts_after_send = 1, /* Not supported but keep returning -EINVAL */ > }; > Add function description from original comment "Reset device, purging any pending irq / data", since the comment applies to both hardware and software reset, > +static int sc16is7xx_reset(struct device *dev, struct regmap *regmaps[]) Simply pass "struct regmap *regmap" as the second argument. See sc16is7xx_setup_mctrl_ports() for example. > +{ > + struct gpio_desc *reset_gpiod; reset_gpiod -> reset_gpio > + > + reset_gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); > + if (!reset_gpiod) Follow Andy's suggestion here. > + /* soft reset device, purging any pending irq / data */ "Software reset". > + regmap_write(regmaps[0], SC16IS7XX_IOCONTROL_REG, > + SC16IS7XX_IOCONTROL_SRESET_BIT); > + else if (!IS_ERR(reset_gpiod)) { > + /* delay 5 us (at least 3 us) and deassert the gpio to exit the hard reset */ You can omit the "delay 5 us" since it is obvious from the code. Maybe add that "The minimum reset pulse width is 3 us" as stated in the datasheet. As a general note for your comments: capitalize the first letter, ex: "Deassert GPIO" and not "deassert GPIO". > + udelay(5); > + gpiod_set_value_cansleep(reset_gpiod, 0); Move the comment "deassert the gpio to exit the hard reset" here. You could also simplify it as "Deassert GPIO.". > + } else > + return PTR_ERR(reset_gpiod); return dev_err_probe(dev, PTR_ERR(reset_gpiod), "Failed to get reset GPIO\n"); > + > + return 0; > +} > + > int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype, > struct regmap *regmaps[], int irq) > { > @@ -1527,6 +1547,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); > + if (ret) > + goto out_clk; > + > kthread_init_worker(&s->kworker); > s->kworker_task = kthread_run(kthread_worker_fn, &s->kworker, > "sc16is7xx"); > @@ -1536,10 +1560,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 > > >
On 6/4/24 21:42, Andy Shevchenko wrote: > On Tue, Jun 04, 2024 at 09:27:26PM +0800, Hui Wang wrote: >> Certain designs connect a gpio to the reset pin, and the reset pin > GPIO Got it. >> needs to be setup correctly before accessing the chip. >> >> Here adding a function to handle the chip reset. If the reset-gpios is >> defined in the dt, do the hard reset through this gpio, othwerwise do > DT Got it. > >> the soft reset as before. > ... > >> +static int sc16is7xx_reset(struct device *dev, struct regmap *regmaps[]) >> +{ >> + struct gpio_desc *reset_gpiod; >> + reset_gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); >> + if (!reset_gpiod) >> + /* soft reset device, purging any pending irq / data */ >> + regmap_write(regmaps[0], SC16IS7XX_IOCONTROL_REG, >> + SC16IS7XX_IOCONTROL_SRESET_BIT); >> + else if (!IS_ERR(reset_gpiod)) { >> + /* delay 5 us (at least 3 us) and deassert the gpio to exit the hard reset */ >> + udelay(5); >> + gpiod_set_value_cansleep(reset_gpiod, 0); >> + } else >> + return PTR_ERR(reset_gpiod); > You can do better here. > > reset_gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); > if (IS_ERR(reset_gpiod)) > return PTR_ERR(reset_gpiod); > > if (reset_gpiod) { > /* delay 5 us (at least 3 us) and deassert the GPIO to exit the hard reset */ > fsleep(5); > gpiod_set_value_cansleep(reset_gpiod, 0); > } else { > /* soft reset device, purging any pending IRQ / data */ > regmap_write(regmaps[0], SC16IS7XX_IOCONTROL_REG, > SC16IS7XX_IOCONTROL_SRESET_BIT); > } OK, got it, will fix all comment in the v3. Thanks. >> + return 0; >> +}
On 6/4/24 22:23, Hugo Villeneuve wrote: > On Tue, 4 Jun 2024 21:27:26 +0800 > Hui Wang <hui.wang@canonical.com> 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 chip reset. If the reset-gpios is >> defined in the dt, do the hard reset through this gpio, othwerwise do >> the soft reset as before. >> >> Reviewed-by: Hugo Villeneuve <hvilleneuve@dimonoff.com> > I never gave you permission to add this tag, remove it. Make sure you > fully understand the meaning of tags by reading patches submission > guidelines. I misunderstood the meaning of "reviewed-by", will remove it and study the guidelines. > >> Signed-off-by: Hui Wang <hui.wang@canonical.com> >> --- >> In the v2: >> - move the soft reset and hard reset into one fucntion >> - move the reset function to sc16is7xx.c and call it in _probe() >> - add udelay(5) before deasserting the gpio reset pin >> >> drivers/tty/serial/sc16is7xx.c | 28 ++++++++++++++++++++++++---- >> 1 file changed, 24 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c >> index bf0065d1c8e9..119abfb4607c 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,25 @@ static const struct serial_rs485 sc16is7xx_rs485_supported = { >> .delay_rts_after_send = 1, /* Not supported but keep returning -EINVAL */ >> }; >> > Add function description from original comment "Reset device, > purging any pending irq / data", since the comment applies to both > hardware and software reset, Got it. >> +static int sc16is7xx_reset(struct device *dev, struct regmap *regmaps[]) > Simply pass "struct regmap *regmap" as the second argument. See > sc16is7xx_setup_mctrl_ports() for example. Got it. > >> +{ >> + struct gpio_desc *reset_gpiod; > reset_gpiod -> reset_gpio Got it. >> + >> + reset_gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); >> + if (!reset_gpiod) > Follow Andy's suggestion here. OK. > >> + /* soft reset device, purging any pending irq / data */ > "Software reset". Got it. >> + regmap_write(regmaps[0], SC16IS7XX_IOCONTROL_REG, >> + SC16IS7XX_IOCONTROL_SRESET_BIT); >> + else if (!IS_ERR(reset_gpiod)) { >> + /* delay 5 us (at least 3 us) and deassert the gpio to exit the hard reset */ > You can omit the "delay 5 us" since it is obvious from the code. Maybe > add that "The minimum reset pulse width is 3 us" as stated in the > datasheet. > > As a general note for your comments: capitalize the first letter, > ex: "Deassert GPIO" and not "deassert GPIO". OK. > >> + udelay(5); >> + gpiod_set_value_cansleep(reset_gpiod, 0); > Move the comment "deassert the gpio to exit the hard reset" here. You > could also simplify it as "Deassert GPIO.". > OK. >> + } else >> + return PTR_ERR(reset_gpiod); > return dev_err_probe(dev, PTR_ERR(reset_gpiod), "Failed to get reset > GPIO\n"); Got it, will address all comment in the v3. Thanks. > >> + >> + return 0; >> +} >> + >> int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype, >> struct regmap *regmaps[], int irq) >> { >> @@ -1527,6 +1547,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); >> + if (ret) >> + goto out_clk; >> + >> kthread_init_worker(&s->kworker); >> s->kworker_task = kthread_run(kthread_worker_fn, &s->kworker, >> "sc16is7xx"); >> @@ -1536,10 +1560,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 >> >> >> >
On 6/4/24 22:34, Hugo Villeneuve wrote: > On Tue, 4 Jun 2024 21:27:26 +0800 > Hui Wang <hui.wang@canonical.com> wrote: > > Hi Hui, > >> Certain designs connect a gpio to the reset pin, and the reset pin > "Some boards..." > >> needs to be setup correctly before accessing the chip. >> >> Here adding a function to handle the chip reset. If the reset-gpios is > "Add a function..." > > Got them, will fix them in the v3. Thanks.
> From: Hugo Villeneuve <hugo@hugovil.com> > Sent: Tuesday, 4 June 2024 16:23 <...> > Add function description from original comment "Reset device, > purging any pending irq / data", since the comment applies to both > hardware and software reset, No it does not (at this moment). See below. > > +static int sc16is7xx_reset(struct device *dev, struct regmap *regmaps[]) > > Simply pass "struct regmap *regmap" as the second argument. See > sc16is7xx_setup_mctrl_ports() for example. > > > +{ > > + struct gpio_desc *reset_gpiod; > > reset_gpiod -> reset_gpio > > > + else if (!IS_ERR(reset_gpiod)) { > > + /* delay 5 us (at least 3 us) and deassert the gpio to exit the hard > reset */ > > You can omit the "delay 5 us" since it is obvious from the code. Maybe > add that "The minimum reset pulse width is 3 us" as stated in the > datasheet. > > As a general note for your comments: capitalize the first letter, > ex: "Deassert GPIO" and not "deassert GPIO". > > > > + udelay(5); > > + gpiod_set_value_cansleep(reset_gpiod, 0); > > Move the comment "deassert the gpio to exit the hard reset" here. You > could also simplify it as "Deassert GPIO.". The problem here is that this only deasserts the reset if it were asserted before. Nothing happens if it already was deasserted. This makes the delay also pretty useless. To make this a proper reset pulse for the device you must first assert the reset, then wait >3us, and finally deassert the reset. Maarten Brock
On 6/5/24 18:30, Maarten Brock wrote: >> From: Hugo Villeneuve <hugo@hugovil.com> >> Sent: Tuesday, 4 June 2024 16:23 > <...> > >> Add function description from original comment "Reset device, >> purging any pending irq / data", since the comment applies to both >> hardware and software reset, > No it does not (at this moment). See below. > >>> +static int sc16is7xx_reset(struct device *dev, struct regmap *regmaps[]) >> Simply pass "struct regmap *regmap" as the second argument. See >> sc16is7xx_setup_mctrl_ports() for example. >> >>> +{ >>> + struct gpio_desc *reset_gpiod; >> reset_gpiod -> reset_gpio >> >>> + else if (!IS_ERR(reset_gpiod)) { >>> + /* delay 5 us (at least 3 us) and deassert the gpio to exit the hard >> reset */ >> >> You can omit the "delay 5 us" since it is obvious from the code. Maybe >> add that "The minimum reset pulse width is 3 us" as stated in the >> datasheet. >> >> As a general note for your comments: capitalize the first letter, >> ex: "Deassert GPIO" and not "deassert GPIO". >> >> >>> + udelay(5); >>> + gpiod_set_value_cansleep(reset_gpiod, 0); >> Move the comment "deassert the gpio to exit the hard reset" here. You >> could also simplify it as "Deassert GPIO.". > The problem here is that this only deasserts the reset if it were asserted before. > Nothing happens if it already was deasserted. This makes the delay also pretty > useless. > > To make this a proper reset pulse for the device you must first assert the reset, > then wait >3us, and finally deassert the reset. > > Maarten Brock Hi Maarten, My understanding is when calling devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW) and returning a valid (gpio_desc *), the flag GPIOD_OUT_LOW guarantees the GPIO is set to output and low (assert the reset pin). Thanks, Hui. >
> On 6/5/24 18:30, Maarten Brock wrote: > >> From: Hugo Villeneuve <hugo@hugovil.com> > >> Sent: Tuesday, 4 June 2024 16:23 > > <...> > > > >> Add function description from original comment "Reset device, > >> purging any pending irq / data", since the comment applies to both > >> hardware and software reset, > > No it does not (at this moment). See below. > > > >>> +static int sc16is7xx_reset(struct device *dev, struct regmap *regmaps[]) > >> Simply pass "struct regmap *regmap" as the second argument. See > >> sc16is7xx_setup_mctrl_ports() for example. > >> > >>> +{ > >>> + struct gpio_desc *reset_gpiod; > >> reset_gpiod -> reset_gpio > >> > >>> + else if (!IS_ERR(reset_gpiod)) { > >>> + /* delay 5 us (at least 3 us) and deassert the gpio to exit the hard > >> reset */ > >> > >> You can omit the "delay 5 us" since it is obvious from the code. Maybe > >> add that "The minimum reset pulse width is 3 us" as stated in the > >> datasheet. > >> > >> As a general note for your comments: capitalize the first letter, > >> ex: "Deassert GPIO" and not "deassert GPIO". > >> > >> > >>> + udelay(5); > >>> + gpiod_set_value_cansleep(reset_gpiod, 0); > >> Move the comment "deassert the gpio to exit the hard reset" here. You > >> could also simplify it as "Deassert GPIO.". > > The problem here is that this only deasserts the reset if it were asserted before. > > Nothing happens if it already was deasserted. This makes the delay also pretty > > useless. > > > > To make this a proper reset pulse for the device you must first assert the reset, > > then wait >3us, and finally deassert the reset. > > > > Maarten Brock > Hi Maarten, > > My understanding is when calling devm_gpiod_get_optional(dev, "reset", > GPIOD_OUT_LOW) and returning a valid (gpio_desc *), the flag > GPIOD_OUT_LOW guarantees the GPIO is set to output and low (assert the > reset pin). Ah, right. Sorry, I missed that. So GPIOD_OUT_LOW disregards the inversion from GPIO_ACTIVE_LOW. And gpiod_set_value_cansleep(reset_gpiod, 0) uses the inversion to make the pin high. Looks fine to me now. > Thanks, > > Hui. Maarten
On Wed, Jun 5, 2024 at 1:55 PM Hui Wang <hui.wang@canonical.com> wrote: > On 6/5/24 18:30, Maarten Brock wrote: > >> From: Hugo Villeneuve <hugo@hugovil.com> > >> Sent: Tuesday, 4 June 2024 16:23 <...> > >> Add function description from original comment "Reset device, > >> purging any pending irq / data", since the comment applies to both > >> hardware and software reset, > > No it does not (at this moment). See below. ... > > The problem here is that this only deasserts the reset if it were asserted before. > > Nothing happens if it already was deasserted. This makes the delay also pretty > > useless. > > > > To make this a proper reset pulse for the device you must first assert the reset, > > then wait >3us, and finally deassert the reset. > My understanding is when calling devm_gpiod_get_optional(dev, "reset", > GPIOD_OUT_LOW) and returning a valid (gpio_desc *), the flag > GPIOD_OUT_LOW guarantees the GPIO is set to output and low (assert the > reset pin). No, this is logical, not physical state. Maarten is correct. How did you test this?
On Wed, Jun 5, 2024 at 2:19 PM Maarten Brock <Maarten.Brock@sttls.nl> wrote: > > On 6/5/24 18:30, Maarten Brock wrote: > > >> From: Hugo Villeneuve <hugo@hugovil.com> > > >> Sent: Tuesday, 4 June 2024 16:23 <...> > > >> Add function description from original comment "Reset device, > > >> purging any pending irq / data", since the comment applies to both > > >> hardware and software reset, > > > No it does not (at this moment). See below. ... > > > The problem here is that this only deasserts the reset if it were asserted before. > > > Nothing happens if it already was deasserted. This makes the delay also pretty > > > useless. > > > > > > To make this a proper reset pulse for the device you must first assert the reset, > > > then wait >3us, and finally deassert the reset. > > > > > > Maarten Brock > > Hi Maarten, > > > > My understanding is when calling devm_gpiod_get_optional(dev, "reset", > > GPIOD_OUT_LOW) and returning a valid (gpio_desc *), the flag > > GPIOD_OUT_LOW guarantees the GPIO is set to output and low (assert the > > reset pin). > > Ah, right. Sorry, I missed that. > So GPIOD_OUT_LOW disregards the inversion from GPIO_ACTIVE_LOW. > And gpiod_set_value_cansleep(reset_gpiod, 0) uses the inversion to make the pin high. > Looks fine to me now. No, you used the correct terms "assert"/"deassert", the parameter there is logical, means "deassert". It was differently in the legacy GPIO APIs, which are almost gone.
On 05/06/2024 13:19, Maarten Brock wrote: >>> To make this a proper reset pulse for the device you must first assert the reset, >>> then wait >3us, and finally deassert the reset. >>> >>> Maarten Brock >> Hi Maarten, >> >> My understanding is when calling devm_gpiod_get_optional(dev, "reset", >> GPIOD_OUT_LOW) and returning a valid (gpio_desc *), the flag >> GPIOD_OUT_LOW guarantees the GPIO is set to output and low (assert the >> reset pin). > > Ah, right. Sorry, I missed that. > So GPIOD_OUT_LOW disregards the inversion from GPIO_ACTIVE_LOW. It doesn't. > And gpiod_set_value_cansleep(reset_gpiod, 0) uses the inversion to make the pin high. > Looks fine to me now. They both respect pin polarity. Best regards, Krzysztof
On 6/5/24 19:24, Krzysztof Kozlowski wrote: > On 05/06/2024 13:19, Maarten Brock wrote: >>>> To make this a proper reset pulse for the device you must first assert the reset, >>>> then wait >3us, and finally deassert the reset. >>>> >>>> Maarten Brock >>> Hi Maarten, >>> >>> My understanding is when calling devm_gpiod_get_optional(dev, "reset", >>> GPIOD_OUT_LOW) and returning a valid (gpio_desc *), the flag >>> GPIOD_OUT_LOW guarantees the GPIO is set to output and low (assert the >>> reset pin). >> Ah, right. Sorry, I missed that. >> So GPIOD_OUT_LOW disregards the inversion from GPIO_ACTIVE_LOW. > It doesn't. > >> And gpiod_set_value_cansleep(reset_gpiod, 0) uses the inversion to make the pin high. >> Looks fine to me now. > They both respect pin polarity. Will correct it. Thanks. > > Best regards, > Krzysztof >
On Wed, 5 Jun 2024 20:55:21 +0800 Hui Wang <hui.wang@canonical.com> wrote: > > On 6/5/24 19:19, Andy Shevchenko wrote: > > On Wed, Jun 5, 2024 at 1:55 PM Hui Wang<hui.wang@canonical.com> wrote: > >> On 6/5/24 18:30, Maarten Brock wrote: > >>>> From: Hugo Villeneuve<hugo@hugovil.com> > >>>> Sent: Tuesday, 4 June 2024 16:23 > > <...> > > > >>>> Add function description from original comment "Reset device, > >>>> purging any pending irq / data", since the comment applies to both > >>>> hardware and software reset, > >>> No it does not (at this moment). See below. > > ... > > > >>> The problem here is that this only deasserts the reset if it were asserted before. > >>> Nothing happens if it already was deasserted. This makes the delay also pretty > >>> useless. > >>> > >>> To make this a proper reset pulse for the device you must first assert the reset, > >>> then wait >3us, and finally deassert the reset. > >> My understanding is when calling devm_gpiod_get_optional(dev, "reset", > >> GPIOD_OUT_LOW) and returning a valid (gpio_desc *), the flag > >> GPIOD_OUT_LOW guarantees the GPIO is set to output and low (assert the > >> reset pin). > > No, this is logical, not physical state. Maarten is correct. How did > > you test this? > > Yes, Maarten, Krzysztof and you are correct. Thanks for pointing out > this error. > > If I call "reset_gpio = devm_gpiod_get_optional(dev, "reset", > GPIOD_OUT_HIGH);" I will get the result as below: > > # cat /sys/kernel/debug/gpio > > gpiochip4: GPIOs 128-159, parent: platform/30240000.gpio, 30240000.gpio: > gpio-141 ( |reset ) out lo ACTIVE LOW > > If I call "reset_gpio = devm_gpiod_get_optional(dev, "reset", > GPIOD_OUT_LOW);" I will get the result as below: > > # cat /sys/kernel/debug/gpio > > gpiochip4: GPIOs 128-159, parent: platform/30240000.gpio, 30240000.gpio: > gpio-141 ( |reset ) out hi ACTIVE LOW > > I tested the reset pin by dumping chip registers, if the reset pin is > asserted (out lo), I will get the result like this: > > # cat /sys/kernel/debug/regmap/spi0.0-port0/registers 1: 10 2: ff 3: 00 > 4: ec 5: ff 6: ff 7: ff 8: ff 9: ff a: ff b: ff c: ff d: ff e: ff f: 06 Hi Hui, the best way to test a reset pin is with a voltmeter, if you can. It is way too easy to get confused with reset pins values/polarities, etc. By the way, if the reset pin is asserted, you cannot communicate with the device, therefore dumping registers cannot work for debug purpose. Hugo. > > If the reset pin is deasserted (out hi), I will get: > > # cat /sys/kernel/debug/regmap/spi0.0-port0/registers 1: 10 2: 01 3: 00 > 4: 00 5: 60 6: 00 7: 2e 8: 40 9: 00 a: 00 b: ff c: 00 d: 00 e: 00 f: 06 > > My original code set the reset GPIO to high (the reset pin is > deasserted) continuously, so I didn't notice this hidden error. > > Thanks, > > Hui. > > >
On 6/5/24 23:32, Hugo Villeneuve wrote: > On Wed, 5 Jun 2024 20:55:21 +0800 > Hui Wang <hui.wang@canonical.com> wrote: > >> On 6/5/24 19:19, Andy Shevchenko wrote: >>> On Wed, Jun 5, 2024 at 1:55 PM Hui Wang<hui.wang@canonical.com> wrote: >>>> On 6/5/24 18:30, Maarten Brock wrote: ... >> # cat /sys/kernel/debug/regmap/spi0.0-port0/registers 1: 10 2: ff 3: 00 >> 4: ec 5: ff 6: ff 7: ff 8: ff 9: ff a: ff b: ff c: ff d: ff e: ff f: 06 > Hi Hui, > the best way to test a reset pin is with a voltmeter, if you can. It is > way too easy to get confused with reset pins values/polarities, etc. Yes. got it. > > By the way, if the reset pin is asserted, you cannot communicate with > the device, therefore dumping registers cannot work for debug purpose. Got it. I just use it to check the reset pin status. If the returned register values look reasonable (not many 0xff), It means the reset GPIO is de-asserted. Thanks. > Hugo. > > >> >
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c index bf0065d1c8e9..119abfb4607c 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,25 @@ static const struct serial_rs485 sc16is7xx_rs485_supported = { .delay_rts_after_send = 1, /* Not supported but keep returning -EINVAL */ }; +static int sc16is7xx_reset(struct device *dev, struct regmap *regmaps[]) +{ + struct gpio_desc *reset_gpiod; + + reset_gpiod = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); + if (!reset_gpiod) + /* soft reset device, purging any pending irq / data */ + regmap_write(regmaps[0], SC16IS7XX_IOCONTROL_REG, + SC16IS7XX_IOCONTROL_SRESET_BIT); + else if (!IS_ERR(reset_gpiod)) { + /* delay 5 us (at least 3 us) and deassert the gpio to exit the hard reset */ + udelay(5); + gpiod_set_value_cansleep(reset_gpiod, 0); + } else + return PTR_ERR(reset_gpiod); + + return 0; +} + int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype, struct regmap *regmaps[], int irq) { @@ -1527,6 +1547,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); + if (ret) + goto out_clk; + kthread_init_worker(&s->kworker); s->kworker_task = kthread_run(kthread_worker_fn, &s->kworker, "sc16is7xx"); @@ -1536,10 +1560,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;