[PATCH v7 1/2] dt-bindings: serial: sc16is7xx: Add description for polling mode

[PATCH v7 1/2] dt-bindings: serial: sc16is7xx: Add description for polling mode

[Andre Werner]

Polling mode is enabled if the "interrupts" property is missing.
Thus, this commit deletes "interrupts" entry from "required" section
and adds a description for the fallback to polling mode at the
"interrupts" entry.

Signed-off-by: Andre Werner <andre.werner@systec-electronic.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
V3:
- No changes on source.
- Add Acked-By to commit message.
V4:
- No changes
V5:
- No changes
V6:
- No changes
V7:
- No changes
---
 Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml
index 88871480018e..ab39b95dae40 100644
--- a/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml
+++ b/Documentation/devicetree/bindings/serial/nxp,sc16is7xx.yaml
@@ -23,6 +23,8 @@ properties:
     maxItems: 1
 
   interrupts:
+    description:
+      When missing, device driver uses polling instead.
     maxItems: 1
 
   clocks:
@@ -76,7 +78,6 @@ properties:
 required:
   - compatible
   - reg
-  - interrupts
 
 allOf:
   - $ref: /schemas/spi/spi-peripheral-props.yaml#
-- 
2.48.0

[PATCH v7 2/2] serial: sc16is7xx: Add polling mode if no IRQ pin is available

[Andre Werner]

Fall back to polling mode if no interrupt is configured because there
is no possibility to connect the interrupt pin.

If no interrupt pin is available the driver uses a delayed worker to
poll the state of interrupt status registers (IIR).

Signed-off-by: Andre Werner <andre.werner@systec-electronic.com>
---
V2:
- Change warning for polling mode to debug log entry
- Correct typo: Resuse -> Reuse
- Format define with missing tabs for SC16IS7XX_POLL_PERIOD
- Format struct declaration sc16is7xx_one_config with missing tabs for polling and shutdown
- Adapt dtbinding with new polling feature
V3:
- Use suffix with units and drop a comment SC16IS7XX_POLL_PERIOD_MS. Sorry for that miss.
- Make Kernel lowercase.
V4:
- Reword commit messages for better understanding.
- Remove 'shutdown' property for canceling delayed worker.
- Rename worker function: sc16is7xx_transmission_poll -> sc16is7xx_poll_proc
- Unify argument for worker functions: kthread_work *work -> kthread_work *ws
V5:
- Replace of_property check with IRQ number check to set polling
  property. This will add support for usage without device tree
  definitions. Thanks for that advice.
- Add blank line es requested.
V6:
- Use polling mode for IRQ numbers <= 0 which encounter no valid IRQ
  were found/defined.
V7:
- Try to improve and unify comments as requested.
- Fix typo in commit message: pull -> poll
---
 drivers/tty/serial/sc16is7xx.c | 39 ++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index a3093e09309f..348ddc3103cd 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -314,6 +314,7 @@
 #define SC16IS7XX_FIFO_SIZE		(64)
 #define SC16IS7XX_GPIOS_PER_BANK	4
 
+#define SC16IS7XX_POLL_PERIOD_MS	10
 #define SC16IS7XX_RECONF_MD		BIT(0)
 #define SC16IS7XX_RECONF_IER		BIT(1)
 #define SC16IS7XX_RECONF_RS485		BIT(2)
@@ -348,6 +349,8 @@ struct sc16is7xx_port {
 	u8				mctrl_mask;
 	struct kthread_worker		kworker;
 	struct task_struct		*kworker_task;
+	struct kthread_delayed_work	poll_work;
+	bool				polling;
 	struct sc16is7xx_one		p[];
 };
 
@@ -861,6 +864,20 @@ static irqreturn_t sc16is7xx_irq(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static void sc16is7xx_poll_proc(struct kthread_work *ws)
+{
+	struct sc16is7xx_port *s = container_of(ws, struct sc16is7xx_port, poll_work.work);
+
+	/*
+	 * Reuse standard IRQ handler. Interrupt ID is unused in this
+	 * context and set to zero.
+	 */
+	sc16is7xx_irq(0, s);
+
+	kthread_queue_delayed_work(&s->kworker, &s->poll_work,
+				   msecs_to_jiffies(SC16IS7XX_POLL_PERIOD_MS));
+}
+
 static void sc16is7xx_tx_proc(struct kthread_work *ws)
 {
 	struct uart_port *port = &(to_sc16is7xx_one(ws, tx_work)->port);
@@ -1149,6 +1166,7 @@ static int sc16is7xx_config_rs485(struct uart_port *port, struct ktermios *termi
 static int sc16is7xx_startup(struct uart_port *port)
 {
 	struct sc16is7xx_one *one = to_sc16is7xx_one(port, port);
+	struct sc16is7xx_port *s = dev_get_drvdata(port->dev);
 	unsigned int val;
 	unsigned long flags;
 
@@ -1211,6 +1229,10 @@ static int sc16is7xx_startup(struct uart_port *port)
 	sc16is7xx_enable_ms(port);
 	uart_port_unlock_irqrestore(port, flags);
 
+	if (s->polling)
+		kthread_queue_delayed_work(&s->kworker, &s->poll_work,
+					   msecs_to_jiffies(SC16IS7XX_POLL_PERIOD_MS));
+
 	return 0;
 }
 
@@ -1232,6 +1254,9 @@ static void sc16is7xx_shutdown(struct uart_port *port)
 
 	sc16is7xx_power(port, 0);
 
+	if (s->polling)
+		kthread_cancel_delayed_work_sync(&s->poll_work);
+
 	kthread_flush_worker(&s->kworker);
 }
 
@@ -1538,6 +1563,11 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
 	/* Always ask for fixed clock rate from a property. */
 	device_property_read_u32(dev, "clock-frequency", &uartclk);
 
+	s->polling = (irq <= 0);
+	if (s->polling)
+		dev_dbg(dev,
+			"No interrupt pin definition, falling back to polling mode\n");
+
 	s->clk = devm_clk_get_optional(dev, NULL);
 	if (IS_ERR(s->clk))
 		return PTR_ERR(s->clk);
@@ -1665,6 +1695,12 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
 		goto out_ports;
 #endif
 
+	if (s->polling) {
+		/* Initialize a kthread work struct that is dedicated to polling */
+		kthread_init_delayed_work(&s->poll_work, sc16is7xx_poll_proc);
+		return 0;
+	}
+
 	/*
 	 * Setup interrupt. We first try to acquire the IRQ line as level IRQ.
 	 * If that succeeds, we can allow sharing the interrupt as well.
@@ -1724,6 +1760,9 @@ void sc16is7xx_remove(struct device *dev)
 		sc16is7xx_power(&s->p[i].port, 0);
 	}
 
+	if (s->polling)
+		kthread_cancel_delayed_work_sync(&s->poll_work);
+
 	kthread_flush_worker(&s->kworker);
 	kthread_stop(s->kworker_task);
 
-- 
2.48.0

Re: [PATCH v7 2/2] serial: sc16is7xx: Add polling mode if no IRQ pin is available

[Andy Shevchenko]

On Tue, Jan 14, 2025 at 08:04:49AM +0100, Andre Werner wrote:
> Fall back to polling mode if no interrupt is configured because there
> is no possibility to connect the interrupt pin.
> 
> If no interrupt pin is available the driver uses a delayed worker to
> poll the state of interrupt status registers (IIR).

...

> V6:
> - Use polling mode for IRQ numbers <= 0 which encounter no valid IRQ
>   were found/defined.
> V7:
> - Try to improve and unify comments as requested.
> - Fix typo in commit message: pull -> poll

Please, rebase your series on top of the recent Linux Next.

-- 
With Best Regards,
Andy Shevchenko

Re: [External Email] Re: [PATCH v7 2/2] serial: sc16is7xx: Add polling mode if no IRQ pin is available

[Andre Werner]

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


Dear Andy,

On Tue, 14 Jan 2025, Andy Shevchenko wrote:

> On Tue, Jan 14, 2025 at 08:04:49AM +0100, Andre Werner wrote:
> > Fall back to polling mode if no interrupt is configured because there
> > is no possibility to connect the interrupt pin.
> >
> > If no interrupt pin is available the driver uses a delayed worker to
> > poll the state of interrupt status registers (IIR).
>
> ...
>
> > V6:
> > - Use polling mode for IRQ numbers <= 0 which encounter no valid IRQ
> >   were found/defined.
> > V7:
> > - Try to improve and unify comments as requested.
> > - Fix typo in commit message: pull -> poll
>
> Please, rebase your series on top of the recent Linux Next.

I performed the rebase and get a single new commit to fix the merge
conflicts. How do you expect the submission? Is it a new commit or
should I mark it as a fixup? Shall it have new version number v8 or
is it a full new commit series?

>
> --
> With Best Regards,
> Andy Shevchenko
>
>
>

Thanks and best regards,

André

Re: [External Email] Re: [PATCH v7 2/2] serial: sc16is7xx: Add polling mode if no IRQ pin is available

[Andy Shevchenko]

On Wed, Jan 15, 2025 at 7:23 AM Andre Werner
<andre.werner@systec-electronic.com> wrote:
> On Tue, 14 Jan 2025, Andy Shevchenko wrote:
> > On Tue, Jan 14, 2025 at 08:04:49AM +0100, Andre Werner wrote:

...

> > > V6:
> > > - Use polling mode for IRQ numbers <= 0 which encounter no valid IRQ
> > >   were found/defined.
> > > V7:
> > > - Try to improve and unify comments as requested.
> > > - Fix typo in commit message: pull -> poll
> >
> > Please, rebase your series on top of the recent Linux Next.
>
> I performed the rebase and get a single new commit to fix the merge
> conflicts. How do you expect the submission? Is it a new commit or
> should I mark it as a fixup? Shall it have new version number v8 or
> is it a full new commit series?

When you rebase you should not get _merge_ conflicts.
What is expected is that the first (DT binding) commit will be gone
from your local branch, followed by the shrunken / conflicting second
one.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v7] serial: sc16is7xx: Add polling mode if no IRQ pin is available

[Andre Werner]

Fall back to polling mode if no interrupt is configured because there
is no possibility to connect the interrupt pin.

If no interrupt pin is available the driver uses a delayed worker to
poll the state of interrupt status registers (IIR).

Signed-off-by: Andre Werner <andre.werner@systec-electronic.com>
Link: https://lore.kernel.org/r/20250110073104.1029633-2-andre.werner@systec-electronic.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
V2:
- Change warning for polling mode to debug log entry
- Correct typo: Resuse -> Reuse
- Format define with missing tabs for SC16IS7XX_POLL_PERIOD
- Format struct declaration sc16is7xx_one_config with missing tabs for polling and shutdown
- Adapt dtbinding with new polling feature
V3:
- Use suffix with units and drop a comment SC16IS7XX_POLL_PERIOD_MS. Sorry for that miss.
- Make Kernel lowercase.
V4:
- Reword commit messages for better understanding.
- Remove 'shutdown' property for canceling delayed worker.
- Rename worker function: sc16is7xx_transmission_poll -> sc16is7xx_poll_proc
- Unify argument for worker functions: kthread_work *work -> kthread_work *ws
V5:
- Replace of_property check with IRQ number check to set polling
  property. This will add support for usage without device tree
  definitions. Thanks for that advice.
- Add blank line es requested.
V6:
- Use polling mode for IRQ numbers <= 0 which encounter no valid IRQ
  were found/defined.
V7:
- Try to improve and unify comments as requested.
- Fix typo in commit message: pull -> poll
---
 drivers/tty/serial/sc16is7xx.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 7b51cdc274fd..348ddc3103cd 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -868,10 +868,12 @@ static void sc16is7xx_poll_proc(struct kthread_work *ws)
 {
 	struct sc16is7xx_port *s = container_of(ws, struct sc16is7xx_port, poll_work.work);
 
-	/* Reuse standard IRQ handler. Interrupt ID is unused in this context. */
+	/*
+	 * Reuse standard IRQ handler. Interrupt ID is unused in this
+	 * context and set to zero.
+	 */
 	sc16is7xx_irq(0, s);
 
-	/* Setup delay based on SC16IS7XX_POLL_PERIOD_MS */
 	kthread_queue_delayed_work(&s->kworker, &s->poll_work,
 				   msecs_to_jiffies(SC16IS7XX_POLL_PERIOD_MS));
 }
@@ -1561,7 +1563,7 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
 	/* Always ask for fixed clock rate from a property. */
 	device_property_read_u32(dev, "clock-frequency", &uartclk);
 
-	s->polling = !!irq;
+	s->polling = (irq <= 0);
 	if (s->polling)
 		dev_dbg(dev,
 			"No interrupt pin definition, falling back to polling mode\n");
@@ -1694,7 +1696,7 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
 #endif
 
 	if (s->polling) {
-		/* Initialize kernel thread for polling */
+		/* Initialize a kthread work struct that is dedicated to polling */
 		kthread_init_delayed_work(&s->poll_work, sc16is7xx_poll_proc);
 		return 0;
 	}
-- 
2.48.0

Re: [PATCH v7] serial: sc16is7xx: Add polling mode if no IRQ pin is available

[Andre Werner]

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

Dear Andy,

I hope this is what you expected and is what I have understood so far.
I performed the rebase to
git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
branch. This is a commit that has been created due to the rebase. The
commit message title is the same as from the previous commit already in
linux-next and the commit message body differs, because there was a typo
in the message and the description was still including the "interrupts"
property from dt to use to select polling mode that is not directly used
anymore in the latest version of the patch.

Sorry if I still did something wrong, but this is what I thought I had
to do.

Best regards,
André


On Thu, 16 Jan 2025, Andre Werner wrote:

> Fall back to polling mode if no interrupt is configured because there
> is no possibility to connect the interrupt pin.
>
> If no interrupt pin is available the driver uses a delayed worker to
> poll the state of interrupt status registers (IIR).
>
> Signed-off-by: Andre Werner <andre.werner@systec-electronic.com>
> Link: https://lore.kernel.org/r/20250110073104.1029633-2-andre.werner@systec-electronic.com
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> V2:
> - Change warning for polling mode to debug log entry
> - Correct typo: Resuse -> Reuse
> - Format define with missing tabs for SC16IS7XX_POLL_PERIOD
> - Format struct declaration sc16is7xx_one_config with missing tabs for polling and shutdown
> - Adapt dtbinding with new polling feature
> V3:
> - Use suffix with units and drop a comment SC16IS7XX_POLL_PERIOD_MS. Sorry for that miss.
> - Make Kernel lowercase.
> V4:
> - Reword commit messages for better understanding.
> - Remove 'shutdown' property for canceling delayed worker.
> - Rename worker function: sc16is7xx_transmission_poll -> sc16is7xx_poll_proc
> - Unify argument for worker functions: kthread_work *work -> kthread_work *ws
> V5:
> - Replace of_property check with IRQ number check to set polling
>   property. This will add support for usage without device tree
>   definitions. Thanks for that advice.
> - Add blank line es requested.
> V6:
> - Use polling mode for IRQ numbers <= 0 which encounter no valid IRQ
>   were found/defined.
> V7:
> - Try to improve and unify comments as requested.
> - Fix typo in commit message: pull -> poll
> ---
>  drivers/tty/serial/sc16is7xx.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index 7b51cdc274fd..348ddc3103cd 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -868,10 +868,12 @@ static void sc16is7xx_poll_proc(struct kthread_work *ws)
>  {
>  	struct sc16is7xx_port *s = container_of(ws, struct sc16is7xx_port, poll_work.work);
>
> -	/* Reuse standard IRQ handler. Interrupt ID is unused in this context. */
> +	/*
> +	 * Reuse standard IRQ handler. Interrupt ID is unused in this
> +	 * context and set to zero.
> +	 */
>  	sc16is7xx_irq(0, s);
>
> -	/* Setup delay based on SC16IS7XX_POLL_PERIOD_MS */
>  	kthread_queue_delayed_work(&s->kworker, &s->poll_work,
>  				   msecs_to_jiffies(SC16IS7XX_POLL_PERIOD_MS));
>  }
> @@ -1561,7 +1563,7 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>  	/* Always ask for fixed clock rate from a property. */
>  	device_property_read_u32(dev, "clock-frequency", &uartclk);
>
> -	s->polling = !!irq;
> +	s->polling = (irq <= 0);
>  	if (s->polling)
>  		dev_dbg(dev,
>  			"No interrupt pin definition, falling back to polling mode\n");
> @@ -1694,7 +1696,7 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>  #endif
>
>  	if (s->polling) {
> -		/* Initialize kernel thread for polling */
> +		/* Initialize a kthread work struct that is dedicated to polling */
>  		kthread_init_delayed_work(&s->poll_work, sc16is7xx_poll_proc);
>  		return 0;
>  	}
> --
> 2.48.0
>
>

Re: [PATCH v7] serial: sc16is7xx: Add polling mode if no IRQ pin is available

[Andy Shevchenko]

On Thu, Jan 16, 2025 at 09:42:27AM +0100, Andre Werner wrote:
> Dear Andy,
> 
> I hope this is what you expected and is what I have understood so far.

Nope. Please, find my replies there.

> I performed the rebase to
> git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
> branch. This is a commit that has been created due to the rebase. The
> commit message title is the same as from the previous commit already in
> linux-next and the commit message body differs, because there was a typo
> in the message and the description was still including the "interrupts"
> property from dt to use to select polling mode that is not directly used
> anymore in the latest version of the patch.
> 
> Sorry if I still did something wrong, but this is what I thought I had
> to do.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7] serial: sc16is7xx: Add polling mode if no IRQ pin is available

[Andy Shevchenko]

On Thu, Jan 16, 2025 at 09:34:47AM +0100, Andre Werner wrote:
> Fall back to polling mode if no interrupt is configured because there
> is no possibility to connect the interrupt pin.
> 
> If no interrupt pin is available the driver uses a delayed worker to
> poll the state of interrupt status registers (IIR).

The commit message should be changed to reflect the code.
I.e. "Fix the IRQ check to treat the negative values as No IRQ."

> Signed-off-by: Andre Werner <andre.werner@systec-electronic.com>

> Link: https://lore.kernel.org/r/20250110073104.1029633-2-andre.werner@systec-electronic.com

This should not be here.

> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

It's not signed by Greg.

> V7:
> - Try to improve and unify comments as requested.
> - Fix typo in commit message: pull -> poll

Version should be 1 and Subject has to be changed accordingly.

...

> -	s->polling = !!irq;
> +	s->polling = (irq <= 0);

This is the only line has to be changed in the patch.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7] serial: sc16is7xx: Add polling mode if no IRQ pin is available

[Greg KH]

On Thu, Jan 16, 2025 at 09:34:47AM +0100, Andre Werner wrote:
> Fall back to polling mode if no interrupt is configured because there
> is no possibility to connect the interrupt pin.
> 
> If no interrupt pin is available the driver uses a delayed worker to
> poll the state of interrupt status registers (IIR).
> 
> Signed-off-by: Andre Werner <andre.werner@systec-electronic.com>
> Link: https://lore.kernel.org/r/20250110073104.1029633-2-andre.werner@systec-electronic.com
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

This whole commit is already in my tree, why is it submitted again?

confused,

greg k-h

Re: [External Email] Re: [PATCH v7] serial: sc16is7xx: Add polling mode if no IRQ pin is available

[Andre Werner]

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

Dear Greg,

> On Thu, Jan 16, 2025 at 09:34:47AM +0100, Andre Werner wrote:
> > Fall back to polling mode if no interrupt is configured because there
> > is no possibility to connect the interrupt pin.
> >
> > If no interrupt pin is available the driver uses a delayed worker to
> > poll the state of interrupt status registers (IIR).
> >
> > Signed-off-by: Andre Werner <andre.werner@systec-electronic.com>
> > Link: https://lore.kernel.org/r/20250110073104.1029633-2-andre.werner@systec-electronic.com
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> This whole commit is already in my tree, why is it submitted again?

This is my fault. I misunderstood a comment from a reviewer.
There is a patch that i still discussed in this thread:

https://lkml.org/lkml/2025/1/16/398

Also the thread has a weird structure. Sorry!

>
> confused,
>
> greg k-h
>

Regards,
André

Re: [External Email] Re: [PATCH v7] serial: sc16is7xx: Add polling mode if no IRQ pin is available

[Greg KH]

On Fri, Jan 17, 2025 at 01:37:54PM +0100, Andre Werner wrote:
> Dear Greg,
> 
> > On Thu, Jan 16, 2025 at 09:34:47AM +0100, Andre Werner wrote:
> > > Fall back to polling mode if no interrupt is configured because there
> > > is no possibility to connect the interrupt pin.
> > >
> > > If no interrupt pin is available the driver uses a delayed worker to
> > > poll the state of interrupt status registers (IIR).
> > >
> > > Signed-off-by: Andre Werner <andre.werner@systec-electronic.com>
> > > Link: https://lore.kernel.org/r/20250110073104.1029633-2-andre.werner@systec-electronic.com
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >
> > This whole commit is already in my tree, why is it submitted again?
> 
> This is my fault. I misunderstood a comment from a reviewer.
> There is a patch that i still discussed in this thread:
> 
> https://lkml.org/lkml/2025/1/16/398
> 
> Also the thread has a weird structure. Sorry!

Yes, please send a new version so that it can be considered to be
applied properly, I can't dig it out of that thread easily, could you if
you had to do it? :)

thanks,

greg k-h