[PATCH 0/4] serial: 8250_dw: Rework ->serial_out() & LCR retry logic

[PATCH 0/4] serial: 8250_dw: Rework ->serial_out() & LCR retry logic

[Ilpo Järvinen]

This series reworks DW UART's write logic such that the write in
->serial_out() is reused for LCR retries which allows removing those
ugly duplicated writes in dw8250_check_lcr() (renamed to
dw8250_verify_write() by this series).

I've used label+goto since the retry is really an exceptional thing. If
somebody insists, I can convert those to do {} while (); but I feel it
will give wrong impression that there's a "loop" there.

Ilpo Järvinen (4):
  serial: 8250_dw: Use dw8250_serial_out() in dw8250_serial_out38x()
  serial: 8250_dw: Rename offset to reg_offset
  serial: 8250_dw: Move 16550 compatible & LCR checks to
    dw8250_verify_write()
  serial: 8250_dw: Rework ->serial_out() LCR write retry logic

 drivers/tty/serial/8250/8250_dw.c | 90 ++++++++++++++++---------------
 1 file changed, 47 insertions(+), 43 deletions(-)

-- 
2.30.2

[PATCH 1/4] serial: 8250_dw: Use dw8250_serial_out() in dw8250_serial_out38x()

[Ilpo Järvinen]

Place dw8250_serial_out() before dw8250_serial_out38x() so that it can
be called from dw8250_serial_out38x() to do the actual write.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/8250/8250_dw.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 167a691c7b19..41bf063396e4 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -143,29 +143,23 @@ static void dw8250_tx_wait_empty(struct uart_port *p)
 	}
 }
 
-static void dw8250_serial_out38x(struct uart_port *p, int offset, int value)
+static void dw8250_serial_out(struct uart_port *p, int offset, int value)
 {
 	struct dw8250_data *d = to_dw8250_data(p->private_data);
 
-	/* Allow the TX to drain before we reconfigure */
-	if (offset == UART_LCR)
-		dw8250_tx_wait_empty(p);
-
 	writeb(value, p->membase + (offset << p->regshift));
 
 	if (offset == UART_LCR && !d->uart_16550_compatible)
 		dw8250_check_lcr(p, value);
 }
 
-
-static void dw8250_serial_out(struct uart_port *p, int offset, int value)
+static void dw8250_serial_out38x(struct uart_port *p, int offset, int value)
 {
-	struct dw8250_data *d = to_dw8250_data(p->private_data);
-
-	writeb(value, p->membase + (offset << p->regshift));
+	/* Allow the TX to drain before we reconfigure */
+	if (offset == UART_LCR)
+		dw8250_tx_wait_empty(p);
 
-	if (offset == UART_LCR && !d->uart_16550_compatible)
-		dw8250_check_lcr(p, value);
+	dw8250_serial_out(p, offset, value);
 }
 
 static unsigned int dw8250_serial_in(struct uart_port *p, int offset)
-- 
2.30.2

[PATCH 2/4] serial: 8250_dw: Rename offset to reg_offset

[Ilpo Järvinen]

Get 'offset' variable out of the way of parameter named 'offset',
rename it to 'reg_offset'. This is very short lived change as
reg_offset is going to be soon removed.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/8250/8250_dw.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 41bf063396e4..f18975b4d2c7 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -89,7 +89,7 @@ static void dw8250_force_idle(struct uart_port *p)
 
 static void dw8250_check_lcr(struct uart_port *p, int value)
 {
-	void __iomem *offset = p->membase + (UART_LCR << p->regshift);
+	void __iomem *reg_offset = p->membase + (UART_LCR << p->regshift);
 	int tries = 1000;
 
 	/* Make sure LCR write wasn't ignored */
@@ -103,15 +103,15 @@ static void dw8250_check_lcr(struct uart_port *p, int value)
 
 #ifdef CONFIG_64BIT
 		if (p->type == PORT_OCTEON)
-			__raw_writeq(value & 0xff, offset);
+			__raw_writeq(value & 0xff, reg_offset);
 		else
 #endif
 		if (p->iotype == UPIO_MEM32)
-			writel(value, offset);
+			writel(value, reg_offset);
 		else if (p->iotype == UPIO_MEM32BE)
-			iowrite32be(value, offset);
+			iowrite32be(value, reg_offset);
 		else
-			writeb(value, offset);
+			writeb(value, reg_offset);
 	}
 	/*
 	 * FIXME: this deadlocks if port->lock is already held
-- 
2.30.2

[PATCH 3/4] serial: 8250_dw: Move 16550 compatible & LCR checks to dw8250_verify_write()

[Ilpo Järvinen]

Rename dw8250_check_lcr() -> dw8250_verify_write() and add comment.
Move LCR and 16550_compatible checks there. As offset is now passed and
dw8250_verify_write() ensures it's UART_LCR, offset can use used
instead of explicit UART_LCR.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/8250/8250_dw.c | 35 +++++++++++++++----------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index f18975b4d2c7..fc367d44f86d 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -87,14 +87,24 @@ static void dw8250_force_idle(struct uart_port *p)
 	(void)p->serial_in(p, UART_RX);
 }
 
-static void dw8250_check_lcr(struct uart_port *p, int value)
+/*
+ * DW UART can be configured to indicate BUSY in USR (with
+ * UART_16550_COMPATIBLE=NO or version prior to introducing that option).
+ * If BUSY is set while writing to LCR register, the write is ignored and
+ * needs to be retries.
+ */
+static void dw8250_verify_write(struct uart_port *p, int offset, int value)
 {
-	void __iomem *reg_offset = p->membase + (UART_LCR << p->regshift);
+	void __iomem *reg_offset = p->membase + (offset << p->regshift);
+	struct dw8250_data *d = to_dw8250_data(p->private_data);
 	int tries = 1000;
 
+	if ((offset != UART_LCR) || !d->uart_16550_compatible)
+		return;
+
 	/* Make sure LCR write wasn't ignored */
 	while (tries--) {
-		unsigned int lcr = p->serial_in(p, UART_LCR);
+		unsigned int lcr = p->serial_in(p, offset);
 
 		if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
 			return;
@@ -145,12 +155,9 @@ static void dw8250_tx_wait_empty(struct uart_port *p)
 
 static void dw8250_serial_out(struct uart_port *p, int offset, int value)
 {
-	struct dw8250_data *d = to_dw8250_data(p->private_data);
-
 	writeb(value, p->membase + (offset << p->regshift));
 
-	if (offset == UART_LCR && !d->uart_16550_compatible)
-		dw8250_check_lcr(p, value);
+	dw8250_verify_write(p, offset, value);
 }
 
 static void dw8250_serial_out38x(struct uart_port *p, int offset, int value)
@@ -181,26 +188,20 @@ static unsigned int dw8250_serial_inq(struct uart_port *p, int offset)
 
 static void dw8250_serial_outq(struct uart_port *p, int offset, int value)
 {
-	struct dw8250_data *d = to_dw8250_data(p->private_data);
-
 	value &= 0xff;
 	__raw_writeq(value, p->membase + (offset << p->regshift));
 	/* Read back to ensure register write ordering. */
 	__raw_readq(p->membase + (UART_LCR << p->regshift));
 
-	if (offset == UART_LCR && !d->uart_16550_compatible)
-		dw8250_check_lcr(p, value);
+	dw8250_verify_write(p, offset, value);
 }
 #endif /* CONFIG_64BIT */
 
 static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
 {
-	struct dw8250_data *d = to_dw8250_data(p->private_data);
-
 	writel(value, p->membase + (offset << p->regshift));
 
-	if (offset == UART_LCR && !d->uart_16550_compatible)
-		dw8250_check_lcr(p, value);
+	dw8250_verify_write(p, offset, value);
 }
 
 static unsigned int dw8250_serial_in32(struct uart_port *p, int offset)
@@ -212,12 +213,10 @@ static unsigned int dw8250_serial_in32(struct uart_port *p, int offset)
 
 static void dw8250_serial_out32be(struct uart_port *p, int offset, int value)
 {
-	struct dw8250_data *d = to_dw8250_data(p->private_data);
 
 	iowrite32be(value, p->membase + (offset << p->regshift));
 
-	if (offset == UART_LCR && !d->uart_16550_compatible)
-		dw8250_check_lcr(p, value);
+	dw8250_verify_write(p, offset, value);
 }
 
 static unsigned int dw8250_serial_in32be(struct uart_port *p, int offset)
-- 
2.30.2

[PATCH 4/4] serial: 8250_dw: Rework ->serial_out() LCR write retry logic

[Ilpo Järvinen]

Currently dw8250_verify_write() (was dw8250_check_lcr()) nullifies the
benefit from differentiated ->serial_out() by having big if tree to
select correct write type.

Rework the logic such that the LCR write can be retried within the
relevant ->serial_out() handler:
  1. Move retries counter on the caller level and pass as pointer to
     dw8250_verify_write()
  2. Make dw8250_verify_write() return bool
  3. Retry the write on caller level (if needed)

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/8250/8250_dw.c | 59 ++++++++++++++++++-------------
 1 file changed, 35 insertions(+), 24 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index fc367d44f86d..f6846363341b 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -92,41 +92,36 @@ static void dw8250_force_idle(struct uart_port *p)
  * UART_16550_COMPATIBLE=NO or version prior to introducing that option).
  * If BUSY is set while writing to LCR register, the write is ignored and
  * needs to be retries.
+ *
+ * Returns: false if the caller should retry the write.
  */
-static void dw8250_verify_write(struct uart_port *p, int offset, int value)
+static bool dw8250_verify_write(struct uart_port *p, int offset, int value,
+				    unsigned int *retries)
 {
-	void __iomem *reg_offset = p->membase + (offset << p->regshift);
 	struct dw8250_data *d = to_dw8250_data(p->private_data);
-	int tries = 1000;
+	unsigned int lcr;
 
 	if ((offset != UART_LCR) || !d->uart_16550_compatible)
-		return;
+		return true;
 
 	/* Make sure LCR write wasn't ignored */
-	while (tries--) {
-		unsigned int lcr = p->serial_in(p, offset);
+	lcr = p->serial_in(p, offset);
 
-		if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
-			return;
+	if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
+		return true;
 
-		dw8250_force_idle(p);
+	dw8250_force_idle(p);
 
-#ifdef CONFIG_64BIT
-		if (p->type == PORT_OCTEON)
-			__raw_writeq(value & 0xff, reg_offset);
-		else
-#endif
-		if (p->iotype == UPIO_MEM32)
-			writel(value, reg_offset);
-		else if (p->iotype == UPIO_MEM32BE)
-			iowrite32be(value, reg_offset);
-		else
-			writeb(value, reg_offset);
+	if (*retries) {
+		*retries -= 1;
+		return false;
 	}
+
 	/*
 	 * FIXME: this deadlocks if port->lock is already held
 	 * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
 	 */
+	return true;
 }
 
 /* Returns once the transmitter is empty or we run out of retries */
@@ -155,9 +150,13 @@ static void dw8250_tx_wait_empty(struct uart_port *p)
 
 static void dw8250_serial_out(struct uart_port *p, int offset, int value)
 {
+	unsigned int retries = 1000;
+
+retry:
 	writeb(value, p->membase + (offset << p->regshift));
 
-	dw8250_verify_write(p, offset, value);
+	if (!dw8250_verify_write(p, offset, value, &retries))
+		goto retry;
 }
 
 static void dw8250_serial_out38x(struct uart_port *p, int offset, int value)
@@ -188,20 +187,29 @@ static unsigned int dw8250_serial_inq(struct uart_port *p, int offset)
 
 static void dw8250_serial_outq(struct uart_port *p, int offset, int value)
 {
+	unsigned int retries = 1000;
+
 	value &= 0xff;
+
+retry:
 	__raw_writeq(value, p->membase + (offset << p->regshift));
 	/* Read back to ensure register write ordering. */
 	__raw_readq(p->membase + (UART_LCR << p->regshift));
 
-	dw8250_verify_write(p, offset, value);
+	if (!dw8250_verify_write(p, offset, value, &retries))
+		goto retry;
 }
 #endif /* CONFIG_64BIT */
 
 static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
 {
+	unsigned int retries = 1000;
+
+retry:
 	writel(value, p->membase + (offset << p->regshift));
 
-	dw8250_verify_write(p, offset, value);
+	if (!dw8250_verify_write(p, offset, value, &retries))
+		goto retry;
 }
 
 static unsigned int dw8250_serial_in32(struct uart_port *p, int offset)
@@ -213,10 +221,13 @@ static unsigned int dw8250_serial_in32(struct uart_port *p, int offset)
 
 static void dw8250_serial_out32be(struct uart_port *p, int offset, int value)
 {
+	unsigned int retries = 1000;
 
+retry:
 	iowrite32be(value, p->membase + (offset << p->regshift));
 
-	dw8250_verify_write(p, offset, value);
+	if (!dw8250_verify_write(p, offset, value, &retries))
+		goto retry;
 }
 
 static unsigned int dw8250_serial_in32be(struct uart_port *p, int offset)
-- 
2.30.2

Re: [PATCH 3/4] serial: 8250_dw: Move 16550 compatible & LCR checks to dw8250_verify_write()

[Andy Shevchenko]

On Tue, Jun 28, 2022 at 3:43 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> Rename dw8250_check_lcr() -> dw8250_verify_write() and add comment.
> Move LCR and 16550_compatible checks there. As offset is now passed and
> dw8250_verify_write() ensures it's UART_LCR, offset can use used
> instead of explicit UART_LCR.

...

> +/*
> + * DW UART can be configured to indicate BUSY in USR (with
> + * UART_16550_COMPATIBLE=NO or version prior to introducing that option).
> + * If BUSY is set while writing to LCR register, the write is ignored and
> + * needs to be retries.

retried

> + */


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/4] serial: 8250_dw: Rework ->serial_out() LCR write retry logic

[Andy Shevchenko]

On Tue, Jun 28, 2022 at 3:43 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> Currently dw8250_verify_write() (was dw8250_check_lcr()) nullifies the
> benefit from differentiated ->serial_out() by having big if tree to
> select correct write type.
>
> Rework the logic such that the LCR write can be retried within the
> relevant ->serial_out() handler:
>   1. Move retries counter on the caller level and pass as pointer to
>      dw8250_verify_write()
>   2. Make dw8250_verify_write() return bool
>   3. Retry the write on caller level (if needed)

I'm wondering if it's possible to utilize one of iopoll.h macro here
instead of copying retries and that not-so-obvious IO poll write.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/4] serial: 8250_dw: Rename offset to reg_offset

[Andy Shevchenko]

On Tue, Jun 28, 2022 at 04:42:32PM +0300, Ilpo Järvinen wrote:
> Get 'offset' variable out of the way of parameter named 'offset',
> rename it to 'reg_offset'. This is very short lived change as
> reg_offset is going to be soon removed.

I'm not sure why this change then even needed...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/4] serial: 8250_dw: Rename offset to reg_offset

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 833 bytes --]

On Wed, 29 Jun 2022, Andy Shevchenko wrote:

> On Tue, Jun 28, 2022 at 04:42:32PM +0300, Ilpo Järvinen wrote:
> > Get 'offset' variable out of the way of parameter named 'offset',
> > rename it to 'reg_offset'. This is very short lived change as
> > reg_offset is going to be soon removed.
> 
> I'm not sure why this change then even needed...

I could either:
    1) create one large patch doing many thing (2+3 or 2+3+4)
 or
    2) add the 'offset' parameter with some other name first and rename it 
       to its final name after local var 'offset' is eliminated by patch 4
 or
    3) rename local var 'offset' first out of the way so that I can add 
       'offset' parameter in patch 3 (=this patch)

If I just drop patch 2 and only do 3, it won't build because 'offset' 
variable appears twice (as arg and local var).

-- 
 i.

Re: [PATCH 4/4] serial: 8250_dw: Rework ->serial_out() LCR write retry logic

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 980 bytes --]

On Tue, 28 Jun 2022, Andy Shevchenko wrote:

> On Tue, Jun 28, 2022 at 3:43 PM Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> >
> > Currently dw8250_verify_write() (was dw8250_check_lcr()) nullifies the
> > benefit from differentiated ->serial_out() by having big if tree to
> > select correct write type.
> >
> > Rework the logic such that the LCR write can be retried within the
> > relevant ->serial_out() handler:
> >   1. Move retries counter on the caller level and pass as pointer to
> >      dw8250_verify_write()
> >   2. Make dw8250_verify_write() return bool
> >   3. Retry the write on caller level (if needed)
> 
> I'm wondering if it's possible to utilize one of iopoll.h macro here
> instead of copying retries and that not-so-obvious IO poll write.

Eh, are you suggesting I should do write as a side-effect inside one of 
the iopoll.h macros? Because those available seem to only read?

Or should I create another macro there which writes too?


-- 
 i.

Re: [PATCH 4/4] serial: 8250_dw: Rework ->serial_out() LCR write retry logic

[Andy Shevchenko]

On Wed, Jun 29, 2022 at 10:47 AM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
> On Tue, 28 Jun 2022, Andy Shevchenko wrote:
> > On Tue, Jun 28, 2022 at 3:43 PM Ilpo Järvinen
> > <ilpo.jarvinen@linux.intel.com> wrote:
> > >
> > > Currently dw8250_verify_write() (was dw8250_check_lcr()) nullifies the
> > > benefit from differentiated ->serial_out() by having big if tree to
> > > select correct write type.
> > >
> > > Rework the logic such that the LCR write can be retried within the
> > > relevant ->serial_out() handler:
> > >   1. Move retries counter on the caller level and pass as pointer to
> > >      dw8250_verify_write()
> > >   2. Make dw8250_verify_write() return bool
> > >   3. Retry the write on caller level (if needed)
> >
> > I'm wondering if it's possible to utilize one of iopoll.h macro here
> > instead of copying retries and that not-so-obvious IO poll write.
>
> Eh, are you suggesting I should do write as a side-effect inside one of
> the iopoll.h macros? Because those available seem to only read?
>
> Or should I create another macro there which writes too?

It seems to me that it would be a macro on top of iopoll's one which
will take an op read and op write arguments depending on the case.
Note, for that special case you would need a custom write op instead
of simple __raw_writeq().

Try and if it looks better, convert, otherwise it would be nice to
hear why it won't fly in your opinion.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/4] serial: 8250_dw: Rename offset to reg_offset

[Andy Shevchenko]

On Wed, Jun 29, 2022 at 9:52 AM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Wed, 29 Jun 2022, Andy Shevchenko wrote:
>
> > On Tue, Jun 28, 2022 at 04:42:32PM +0300, Ilpo Järvinen wrote:
> > > Get 'offset' variable out of the way of parameter named 'offset',
> > > rename it to 'reg_offset'. This is very short lived change as

a very

> > > reg_offset is going to be soon removed.
> >
> > I'm not sure why this change then even needed...
>
> I could either:
>     1) create one large patch doing many thing (2+3 or 2+3+4)
>  or
>     2) add the 'offset' parameter with some other name first and rename it
>        to its final name after local var 'offset' is eliminated by patch 4
>  or
>     3) rename local var 'offset' first out of the way so that I can add
>        'offset' parameter in patch 3 (=this patch)
>
> If I just drop patch 2 and only do 3, it won't build because 'offset'
> variable appears twice (as arg and local var).

Now I got it, thanks. See one remark above.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/4] serial: 8250_dw: Rework ->serial_out() LCR write retry logic

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1785 bytes --]

On Wed, 29 Jun 2022, Andy Shevchenko wrote:

> On Wed, Jun 29, 2022 at 10:47 AM Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> > On Tue, 28 Jun 2022, Andy Shevchenko wrote:
> > > On Tue, Jun 28, 2022 at 3:43 PM Ilpo Järvinen
> > > <ilpo.jarvinen@linux.intel.com> wrote:
> > > >
> > > > Currently dw8250_verify_write() (was dw8250_check_lcr()) nullifies the
> > > > benefit from differentiated ->serial_out() by having big if tree to
> > > > select correct write type.
> > > >
> > > > Rework the logic such that the LCR write can be retried within the
> > > > relevant ->serial_out() handler:
> > > >   1. Move retries counter on the caller level and pass as pointer to
> > > >      dw8250_verify_write()
> > > >   2. Make dw8250_verify_write() return bool
> > > >   3. Retry the write on caller level (if needed)
> > >
> > > I'm wondering if it's possible to utilize one of iopoll.h macro here
> > > instead of copying retries and that not-so-obvious IO poll write.
> >
> > Eh, are you suggesting I should do write as a side-effect inside one of
> > the iopoll.h macros? Because those available seem to only read?
> >
> > Or should I create another macro there which writes too?
> 
> It seems to me that it would be a macro on top of iopoll's one which
> will take an op read and op write arguments depending on the case.

The thing is those iopoll macros don't return until the timeout is 
exhausted so I don't think I can reuse them easily for this task ("on top 
of iopoll's one")? That is, w/o some major side-effect hack (which is 
IMHO a no-go).

-- 
 i.

> Note, for that special case you would need a custom write op instead
> of simple __raw_writeq().
>
> Try and if it looks better, convert, otherwise it would be nice to
> hear why it won't fly in your opinion.

Re: [PATCH 4/4] serial: 8250_dw: Rework ->serial_out() LCR write retry logic

[Andy Shevchenko]

On Wed, Jun 29, 2022 at 11:40 AM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
> On Wed, 29 Jun 2022, Andy Shevchenko wrote:
> > On Wed, Jun 29, 2022 at 10:47 AM Ilpo Järvinen
> > <ilpo.jarvinen@linux.intel.com> wrote:
> > > On Tue, 28 Jun 2022, Andy Shevchenko wrote:

...

> > > Eh, are you suggesting I should do write as a side-effect inside one of
> > > the iopoll.h macros? Because those available seem to only read?
> > >
> > > Or should I create another macro there which writes too?
> >
> > It seems to me that it would be a macro on top of iopoll's one which
> > will take an op read and op write arguments depending on the case.
>
> The thing is those iopoll macros don't return until the timeout is
> exhausted

It returns when the condition is true (in your case verify_lcr is OK).

> so I don't think I can reuse them easily for this task ("on top
> of iopoll's one")? That is, w/o some major side-effect hack (which is
> IMHO a no-go).

Basically what we need is a write-read type of polling.

With your current approach I don't like that retries assignment is
duplicated several times and decrement happens in the callee. What I'm
trying to suggest is to research alternatives.

-- 
With Best Regards,
Andy Shevchenko