[PATCH] spi: atmel-quadspi: Print the controller version and used irq

[PATCH] spi: atmel-quadspi: Print the controller version and used irq

[Mihai Sain]

Add support to print the controller version and used irq
similar to other at91 drivers (spi, twi, usart).

Signed-off-by: Mihai Sain <mihai.sain@microchip.com>
---
 drivers/spi/atmel-quadspi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
index 95cdfc28361e..757f07132585 100644
--- a/drivers/spi/atmel-quadspi.c
+++ b/drivers/spi/atmel-quadspi.c
@@ -687,6 +687,8 @@ static int atmel_qspi_probe(struct platform_device *pdev)
 	pm_runtime_mark_last_busy(&pdev->dev);
 	pm_runtime_put_autosuspend(&pdev->dev);
 
+	dev_info(&pdev->dev, "AT91 QSPI Controller version %#x (irq %d)\n",
+		 atmel_qspi_read(aq, QSPI_VERSION), irq);
 	return 0;
 
 disable_qspick:

base-commit: 87d6aab2389e5ce0197d8257d5f8ee965a67c4cd
prerequisite-patch-id: 5e1313094386b146c9180d72c15bae49aaffbfa8
-- 
2.47.0

Re: [PATCH] spi: atmel-quadspi: Print the controller version and used irq

[Tudor Ambarus]

Hi, Mihai,

On 10/8/24 9:32 AM, Mihai Sain wrote:
> Add support to print the controller version and used irq
> similar to other at91 drivers (spi, twi, usart).
> 
> Signed-off-by: Mihai Sain <mihai.sain@microchip.com>
> ---
>  drivers/spi/atmel-quadspi.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
> index 95cdfc28361e..757f07132585 100644
> --- a/drivers/spi/atmel-quadspi.c
> +++ b/drivers/spi/atmel-quadspi.c
> @@ -687,6 +687,8 @@ static int atmel_qspi_probe(struct platform_device *pdev)
>  	pm_runtime_mark_last_busy(&pdev->dev);
>  	pm_runtime_put_autosuspend(&pdev->dev);
>  
> +	dev_info(&pdev->dev, "AT91 QSPI Controller version %#x (irq %d)\n",
> +		 atmel_qspi_read(aq, QSPI_VERSION), irq);

This pollutes the console. Better to add a dev_dbg if you care.
And irq number doesn't bring too much value as you can see it in dt,
isn't it?

Cheers,
ta

Re: [PATCH] spi: atmel-quadspi: Print the controller version and used irq

[Mark Brown]

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

On Tue, Oct 08, 2024 at 10:34:39AM +0100, Tudor Ambarus wrote:
> On 10/8/24 9:32 AM, Mihai Sain wrote:

> > Add support to print the controller version and used irq
> > similar to other at91 drivers (spi, twi, usart).

> > +	dev_info(&pdev->dev, "AT91 QSPI Controller version %#x (irq %d)\n",
> > +		 atmel_qspi_read(aq, QSPI_VERSION), irq);

> This pollutes the console. Better to add a dev_dbg if you care.
> And irq number doesn't bring too much value as you can see it in dt,
> isn't it?

The objective of bringing the various AT91 drivers into consistency does
seem useful so if this isn't OK for this driver we should probably
update the other drivers as well.  Ensuring that people can get at the
IP version does feel useful, I guess it could also be a sysfs thing?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] spi: atmel-quadspi: Print the controller version and used irq

[Tudor Ambarus]

On 10/8/24 11:29 AM, Mark Brown wrote:
> On Tue, Oct 08, 2024 at 10:34:39AM +0100, Tudor Ambarus wrote:
>> On 10/8/24 9:32 AM, Mihai Sain wrote:
> 
>>> Add support to print the controller version and used irq
>>> similar to other at91 drivers (spi, twi, usart).
> 
>>> +	dev_info(&pdev->dev, "AT91 QSPI Controller version %#x (irq %d)\n",
>>> +		 atmel_qspi_read(aq, QSPI_VERSION), irq);
> 
>> This pollutes the console. Better to add a dev_dbg if you care.
>> And irq number doesn't bring too much value as you can see it in dt,
>> isn't it?
> 
> The objective of bringing the various AT91 drivers into consistency does
> seem useful so if this isn't OK for this driver we should probably

right, consistency is good.

> update the other drivers as well.  Ensuring that people can get at the

Can be a goal. My point was that printing too much info in the boot log
may hide other more important information. Printing IP versions, irqs,
dma channels acquired (or worse, that a driver probed successful) shall
be part of debug prints if someone cares.

> IP version does feel useful, I guess it could also be a sysfs thing?

I'd also consider debugfs for IP version, it has less restrictions.

Mihai, do as you find best, it's just a suggestion.

Re: [PATCH] spi: atmel-quadspi: Print the controller version and used irq

[Alexander Dahl]

Hello Mihai,

just saw I made a similar patch for myself lately, so regarding the
discussion of the need of such a patch, I would opt for it.  However,
see below …

Am Tue, Oct 08, 2024 at 11:32:26AM +0300 schrieb Mihai Sain:
> Add support to print the controller version and used irq
> similar to other at91 drivers (spi, twi, usart).
> 
> Signed-off-by: Mihai Sain <mihai.sain@microchip.com>
> ---
>  drivers/spi/atmel-quadspi.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
> index 95cdfc28361e..757f07132585 100644
> --- a/drivers/spi/atmel-quadspi.c
> +++ b/drivers/spi/atmel-quadspi.c
> @@ -687,6 +687,8 @@ static int atmel_qspi_probe(struct platform_device *pdev)
>  	pm_runtime_mark_last_busy(&pdev->dev);
>  	pm_runtime_put_autosuspend(&pdev->dev);
>  
> +	dev_info(&pdev->dev, "AT91 QSPI Controller version %#x (irq %d)\n",
> +		 atmel_qspi_read(aq, QSPI_VERSION), irq);
>  	return 0;

I think this should go above pm_runtime functions, because it does i/o
on a register.  This is how my diff looks like:

diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
index 3d95b887235e6..7405b66e14b30 100644
--- a/drivers/spi/atmel-quadspi.c
+++ b/drivers/spi/atmel-quadspi.c
@@ -1356,6 +1356,7 @@ static int atmel_qspi_probe(struct platform_device *pdev)
         struct atmel_qspi *aq;
         struct resource *res;
         int irq, err = 0;
+        u32 version;
 
         ctrl = devm_spi_alloc_host(&pdev->dev, sizeof(*aq));
         if (!ctrl)
@@ -1470,6 +1471,12 @@ static int atmel_qspi_probe(struct platform_device *pdev)
                 pm_runtime_dont_use_autosuspend(&pdev->dev);
                 goto dma_release;
         }
+
+        version = atmel_qspi_read(aq, QSPI_VERSION) & 0x00000fff;
+        dev_info(&pdev->dev,
+                 "Atmel QSPI Controller version 0x%x at %pR (irq %d)\n",
+                 version, pdev->resource, irq);
+
         pm_runtime_mark_last_busy(&pdev->dev);
         pm_runtime_put_autosuspend(&pdev->dev);

Greets
Alex

RE: [PATCH] spi: atmel-quadspi: Print the controller version and used irq

[Mihai.Sain]

> Hello Mihai,

Hi Alexander,

> 
> just saw I made a similar patch for myself lately, so regarding the discussion
> of the need of such a patch, I would opt for it.  However, see below …
> 
> Am Tue, Oct 08, 2024 at 11:32:26AM +0300 schrieb Mihai Sain:
> > Add support to print the controller version and used irq similar to
> > other at91 drivers (spi, twi, usart).
> >
> > Signed-off-by: Mihai Sain <mihai.sain@microchip.com>
> > ---
> >  drivers/spi/atmel-quadspi.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
> > index 95cdfc28361e..757f07132585 100644
> > --- a/drivers/spi/atmel-quadspi.c
> > +++ b/drivers/spi/atmel-quadspi.c
> > @@ -687,6 +687,8 @@ static int atmel_qspi_probe(struct platform_device *pdev)
> >       pm_runtime_mark_last_busy(&pdev->dev);
> >       pm_runtime_put_autosuspend(&pdev->dev);
> >
> > +     dev_info(&pdev->dev, "AT91 QSPI Controller version %#x (irq %d)\n",
> > +              atmel_qspi_read(aq, QSPI_VERSION), irq);
> >       return 0;
> 
> I think this should go above pm_runtime functions, because it does i/o on a
> register.  This is how my diff looks like:

Yes, you're right 😊
I also think that this print should be done using dev_dbg().

> 
> diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c index
> 3d95b887235e6..7405b66e14b30 100644
> --- a/drivers/spi/atmel-quadspi.c
> +++ b/drivers/spi/atmel-quadspi.c
> @@ -1356,6 +1356,7 @@ static int atmel_qspi_probe(struct platform_device *pdev)
>          struct atmel_qspi *aq;
>          struct resource *res;
>          int irq, err = 0;
> +        u32 version;
> 
>          ctrl = devm_spi_alloc_host(&pdev->dev, sizeof(*aq));
>          if (!ctrl)
> @@ -1470,6 +1471,12 @@ static int atmel_qspi_probe(struct platform_device
> *pdev)
>                  pm_runtime_dont_use_autosuspend(&pdev->dev);
>                  goto dma_release;
>          }
> +
> +        version = atmel_qspi_read(aq, QSPI_VERSION) & 0x00000fff;
> +        dev_info(&pdev->dev,
> +                 "Atmel QSPI Controller version 0x%x at %pR (irq %d)\n",
> +                 version, pdev->resource, irq);
> +
>          pm_runtime_mark_last_busy(&pdev->dev);
>          pm_runtime_put_autosuspend(&pdev->dev);
> 
> Greets
> Alex

Regards,
Mihai