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

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

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é

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

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

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
>
>

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

Fix the IRQ check to treat the negative values as No IRQ.

Signed-off-by: Andre Werner <andre.werner@systec-electronic.com>
---
 drivers/tty/serial/sc16is7xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 7b51cdc274fd..560f45ed19ae 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1561,7 +1561,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");
-- 
2.48.0

This is threaded weirdly.

On 16. 01. 25, 10:32, Andre Werner wrote:
> Fix the IRQ check to treat the negative values as No IRQ.

Care to describe on what HW that can happen?

> Signed-off-by: Andre Werner <andre.werner@systec-electronic.com>
> ---
>   drivers/tty/serial/sc16is7xx.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index 7b51cdc274fd..560f45ed19ae 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -1561,7 +1561,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");


-- 
js
suse labs

On Thu, 16 Jan 2025, Jiri Slaby wrote:

> This is threaded weirdly.

Sorry for that.

>
> On 16. 01. 25, 10:32, Andre Werner wrote:
> > Fix the IRQ check to treat the negative values as No IRQ.
>
> Care to describe on what HW that can happen?

I have no example on that. In the previous thread it was mentioned that
it is not absolutely sure that the API is not called with negative values. Thus,
negative values shall also be treated as no IRQ.

>
> > Signed-off-by: Andre Werner <andre.werner@systec-electronic.com>
> > ---
> >   drivers/tty/serial/sc16is7xx.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> > index 7b51cdc274fd..560f45ed19ae 100644
> > --- a/drivers/tty/serial/sc16is7xx.c
> > +++ b/drivers/tty/serial/sc16is7xx.c
> > @@ -1561,7 +1561,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");
>
>
> --
> js
> suse labs
>
>

Regards,

André

On 16. 01. 25, 10:52, Andre Werner wrote:
> On Thu, 16 Jan 2025, Jiri Slaby wrote:
> 
>> This is threaded weirdly.
> 
> Sorry for that.
> 
>>
>> On 16. 01. 25, 10:32, Andre Werner wrote:
>>> Fix the IRQ check to treat the negative values as No IRQ.
>>
>> Care to describe on what HW that can happen?
> 
> I have no example on that. In the previous thread it was mentioned that
> it is not absolutely sure that the API is not called with negative values. Thus,
> negative values shall also be treated as no IRQ.

SPI:
         if (spi->irq < 0)
                 spi->irq = 0;

I2C:
                 if (irq < 0)
                         irq = 0;

So unlikely :).

>>> Signed-off-by: Andre Werner <andre.werner@systec-electronic.com>
>>> ---
>>>    drivers/tty/serial/sc16is7xx.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
>>> index 7b51cdc274fd..560f45ed19ae 100644
>>> --- a/drivers/tty/serial/sc16is7xx.c
>>> +++ b/drivers/tty/serial/sc16is7xx.c
>>> @@ -1561,7 +1561,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");
>>
>>
>> --
>> js
>> suse labs
>>
>>
> 
> Regards,
> 
> André


-- 
js
suse labs

On Thu, Jan 16, 2025 at 11:01:30AM +0100, Jiri Slaby wrote:
> On 16. 01. 25, 10:52, Andre Werner wrote:
> > On Thu, 16 Jan 2025, Jiri Slaby wrote:
> > > On 16. 01. 25, 10:32, Andre Werner wrote:
> > > > Fix the IRQ check to treat the negative values as No IRQ.
> > > 
> > > Care to describe on what HW that can happen?
> > 
> > I have no example on that. In the previous thread it was mentioned that
> > it is not absolutely sure that the API is not called with negative values. Thus,
> > negative values shall also be treated as no IRQ.
> 
> SPI:
>         if (spi->irq < 0)
>                 spi->irq = 0;

In all cases of how SPI target device is instantiated? I haven't checked myself
all those (three or four? or even five?) ways.

> I2C:
>                 if (irq < 0)
>                         irq = 0;

> So unlikely :).

Yes, but none of them _guarantees_ that (looking at the documentation and
the field type in the respective data structures).

Until there no guarantees, I would consider this patch as robustness increase.

-- 
With Best Regards,
Andy Shevchenko

On Thu, Jan 16, 2025 at 10:42:14AM +0100, Jiri Slaby wrote:
> This is threaded weirdly.

Yeah, new patch (version) — new email thread.

> On 16. 01. 25, 10:32, Andre Werner wrote:
> > Fix the IRQ check to treat the negative values as No IRQ.
> 
> Care to describe on what HW that can happen?

But how does this relevant? The whole idea is that neither I²C nor SPI
frameworks do not guarantee the IRQ field never be negative. This is
the fix to the previously submitted patch.

...

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


-- 
With Best Regards,
Andy Shevchenko

On 16. 01. 25, 10:53, Andy Shevchenko wrote:
> On Thu, Jan 16, 2025 at 10:42:14AM +0100, Jiri Slaby wrote:
>> This is threaded weirdly.
> 
> Yeah, new patch (version) — new email thread.
> 
>> On 16. 01. 25, 10:32, Andre Werner wrote:
>>> Fix the IRQ check to treat the negative values as No IRQ.
>>
>> Care to describe on what HW that can happen?
> 
> But how does this relevant? The whole idea is that neither I²C nor SPI
> frameworks do not guarantee the IRQ field never be negative. This is
> the fix to the previously submitted patch.

They do AFAICS. Could you be more specific?

-- 
js
suse labs

On Thu, Jan 16, 2025 at 12:02 PM Jiri Slaby <jirislaby@kernel.org> wrote:
> On 16. 01. 25, 10:53, Andy Shevchenko wrote:
> > On Thu, Jan 16, 2025 at 10:42:14AM +0100, Jiri Slaby wrote:
> >> This is threaded weirdly.
> >
> > Yeah, new patch (version) — new email thread.
> >
> >> On 16. 01. 25, 10:32, Andre Werner wrote:
> >>> Fix the IRQ check to treat the negative values as No IRQ.
> >>
> >> Care to describe on what HW that can happen?
> >
> > But how does this relevant? The whole idea is that neither I²C nor SPI
> > frameworks do not guarantee the IRQ field never be negative. This is
> > the fix to the previously submitted patch.
>
> They do AFAICS. Could you be more specific?

Hmm... I have looked into the SPI core and this behaviour seems quite
new. Documentation still says the same as before where it's not
guaranteed.


-- 
With Best Regards,
Andy Shevchenko

On Thu, Jan 16, 2025 at 11:02:23AM +0100, Jiri Slaby wrote:
> On 16. 01. 25, 10:53, Andy Shevchenko wrote:
> > On Thu, Jan 16, 2025 at 10:42:14AM +0100, Jiri Slaby wrote:
> > > This is threaded weirdly.
> > 
> > Yeah, new patch (version) — new email thread.
> > 
> > > On 16. 01. 25, 10:32, Andre Werner wrote:
> > > > Fix the IRQ check to treat the negative values as No IRQ.
> > > 
> > > Care to describe on what HW that can happen?
> > 
> > But how does this relevant? The whole idea is that neither I²C nor SPI
> > frameworks do not guarantee the IRQ field never be negative. This is
> > the fix to the previously submitted patch.
> 
> They do AFAICS.

They don't AFAICT.

> Could you be more specific?

The documentation of the fields and their types suggests my way of perception.

-- 
With Best Regards,
Andy Shevchenko