| Message ID | 20230725142343.1724130-10-hugo@hugovil.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Tue, Jul 25, 2023 at 10:23:41AM -0400, Hugo Villeneuve wrote: > From: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > Make sure we wait at least 3us before initiating communication with > the device after reset. That says what you do, but not _why_ you do it? Please read the kernel documentation again for how to write a good changelog text. It's usually the hardest part of submitting a patch. > > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com> > Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com> > Tested-by: Lech Perczak <lech.perczak@camlingroup.com> > --- > drivers/tty/serial/sc16is7xx.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c > index 49213be60baf..718e982e1efe 100644 > --- a/drivers/tty/serial/sc16is7xx.c > +++ b/drivers/tty/serial/sc16is7xx.c > @@ -1526,6 +1526,12 @@ static int sc16is7xx_probe(struct device *dev, > regmap_write(s->regmap, SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT, > SC16IS7XX_IOCONTROL_SRESET_BIT); > > + /* > + * After reset, the host must wait at least 3us before initializing a > + * communication with the device. > + */ > + usleep_range(5, 10); 5, 10 is NOT 3us. :(
On Mon, 31 Jul 2023 17:57:50 +0200 Greg KH <gregkh@linuxfoundation.org> wrote: > On Tue, Jul 25, 2023 at 10:23:41AM -0400, Hugo Villeneuve wrote: > > From: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > > > Make sure we wait at least 3us before initiating communication with > > the device after reset. > > That says what you do, but not _why_ you do it? You are right, it is not clear. I will add a note that it is recommended to do so by the manufacturer in the device datasheet. > Please read the kernel documentation again for how to write a good > changelog text. It's usually the hardest part of submitting a patch. Yes. > > Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com> > > Reviewed-by: Lech Perczak <lech.perczak@camlingroup.com> > > Tested-by: Lech Perczak <lech.perczak@camlingroup.com> > > --- > > drivers/tty/serial/sc16is7xx.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c > > index 49213be60baf..718e982e1efe 100644 > > --- a/drivers/tty/serial/sc16is7xx.c > > +++ b/drivers/tty/serial/sc16is7xx.c > > @@ -1526,6 +1526,12 @@ static int sc16is7xx_probe(struct device *dev, > > regmap_write(s->regmap, SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT, > > SC16IS7XX_IOCONTROL_SRESET_BIT); > > > > + /* > > + * After reset, the host must wait at least 3us before initializing a > > + * communication with the device. > > + */ > > + usleep_range(5, 10); > > 5, 10 is NOT 3us. In v3, Andy Shevchenko suggested "I would put (5, 10) instead to relax a bit the scheduler." Should I add a comment to that effect: /* * After reset, the datasheet indicates that the host must wait at least * 3us before initializing a communication with the device. * Use (5, 10) range to relax the scheduler. */ ? Hugo.
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c index 49213be60baf..718e982e1efe 100644 --- a/drivers/tty/serial/sc16is7xx.c +++ b/drivers/tty/serial/sc16is7xx.c @@ -1526,6 +1526,12 @@ static int sc16is7xx_probe(struct device *dev, regmap_write(s->regmap, SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT, SC16IS7XX_IOCONTROL_SRESET_BIT); + /* + * After reset, the host must wait at least 3us before initializing a + * communication with the device. + */ + usleep_range(5, 10); + for (i = 0; i < devtype->nr_uart; ++i) { s->p[i].line = i; /* Initialize port data */