spi: Kconfig: spi-synquacer: Added ARM and ARM64 dependence
diff mbox series

Message ID 20190617060957.16171-1-masahisa.kojima@linaro.org
State New, archived
Headers show
Series
  • spi: Kconfig: spi-synquacer: Added ARM and ARM64 dependence
Related show

Commit Message

Masahisa Kojima June 17, 2019, 6:09 a.m. UTC
kbuild test reported that Alpha and some of the architectures
are missing readsx/writesx functions.
spi-synquacer driver is only targeted for arm and arm64 platforms.
With that, added ARM and ARM64 dependence in Kconfig.
This patch also specifies the default compile type as module.

Fixes: b0823ee35cf9b ("spi: Add spi driver for Socionext SynQuacer platform")
Reported-by: kbuild test robot <lkp@intel.com>
Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
 drivers/spi/Kconfig | 2 ++
 1 file changed, 2 insertions(+)

Comments

Mark Brown June 17, 2019, 10:17 a.m. UTC | #1
On Mon, Jun 17, 2019 at 03:09:57PM +0900, Masahisa Kojima wrote:
> kbuild test reported that Alpha and some of the architectures
> are missing readsx/writesx functions.
> spi-synquacer driver is only targeted for arm and arm64 platforms.
> With that, added ARM and ARM64 dependence in Kconfig.

Are you sure it's those functions (which only appear to be defined on
arc according to a quick grep) and are you sure that there's no other
Kconfig symbol specifically for those being defined which can be used
rather than depending on specific architectures?

> This patch also specifies the default compile type as module.

This should be a separate patch and we don't generally change the
default unless there's a reason to do so.
Ard Biesheuvel June 17, 2019, 10:21 a.m. UTC | #2
On Mon, 17 Jun 2019 at 12:17, Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Jun 17, 2019 at 03:09:57PM +0900, Masahisa Kojima wrote:
> > kbuild test reported that Alpha and some of the architectures
> > are missing readsx/writesx functions.
> > spi-synquacer driver is only targeted for arm and arm64 platforms.
> > With that, added ARM and ARM64 dependence in Kconfig.
>
> Are you sure it's those functions (which only appear to be defined on
> arc according to a quick grep) and are you sure that there's no other
> Kconfig symbol specifically for those being defined which can be used
> rather than depending on specific architectures?
>

I'm not sure I see the point of this. Building this driver for alpha
and parisc has no use in practice, and does not provide any additional
build coverage given that the driver can be enabled on any ARMA/ARM64
build.

> > This patch also specifies the default compile type as module.
>
> This should be a separate patch and we don't generally change the
> default unless there's a reason to do so.'

That was my suggestion - just 'default m' is generally not accepted,
but 'default m' for a driver if you enabled the ARCH_xxxx in question
is reasonable, no?
Mark Brown June 17, 2019, 11:46 a.m. UTC | #3
On Mon, Jun 17, 2019 at 12:21:47PM +0200, Ard Biesheuvel wrote:
> On Mon, 17 Jun 2019 at 12:17, Mark Brown <broonie@kernel.org> wrote:

> > Are you sure it's those functions (which only appear to be defined on
> > arc according to a quick grep) and are you sure that there's no other
> > Kconfig symbol specifically for those being defined which can be used
> > rather than depending on specific architectures?

> I'm not sure I see the point of this. Building this driver for alpha
> and parisc has no use in practice, and does not provide any additional
> build coverage given that the driver can be enabled on any ARMA/ARM64
> build.

It's helpful for compile test coverage, which in turn is useful for
people doing subsystem or kernel wide changes.  A dependency on ARM64 ||
COMPILE_TEST would make sense but wouldn't stop things getting built on
the older archtiectures so you'd still want some dependency for that.

> > > This patch also specifies the default compile type as module.

> > This should be a separate patch and we don't generally change the
> > default unless there's a reason to do so.'

> That was my suggestion - just 'default m' is generally not accepted,
> but 'default m' for a driver if you enabled the ARCH_xxxx in question
> is reasonable, no?

No, there's no special treatment for arch specific drivers here.
Masahisa Kojima June 18, 2019, 8:13 a.m. UTC | #4
Hi Mark,

There was a patch to address same compile error.
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1982194.html

Does it preferable solution to use iowrite32_rep() series?

On Mon, 17 Jun 2019 at 20:46, Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Jun 17, 2019 at 12:21:47PM +0200, Ard Biesheuvel wrote:
> > On Mon, 17 Jun 2019 at 12:17, Mark Brown <broonie@kernel.org> wrote:
>
> > > Are you sure it's those functions (which only appear to be defined on
> > > arc according to a quick grep) and are you sure that there's no other
> > > Kconfig symbol specifically for those being defined which can be used
> > > rather than depending on specific architectures?
>
> > I'm not sure I see the point of this. Building this driver for alpha
> > and parisc has no use in practice, and does not provide any additional
> > build coverage given that the driver can be enabled on any ARMA/ARM64
> > build.
>
> It's helpful for compile test coverage, which in turn is useful for
> people doing subsystem or kernel wide changes.  A dependency on ARM64 ||
> COMPILE_TEST would make sense but wouldn't stop things getting built on
> the older archtiectures so you'd still want some dependency for that.
>
> > > > This patch also specifies the default compile type as module.
>
> > > This should be a separate patch and we don't generally change the
> > > default unless there's a reason to do so.'
>
> > That was my suggestion - just 'default m' is generally not accepted,
> > but 'default m' for a driver if you enabled the ARCH_xxxx in question
> > is reasonable, no?
>
> No, there's no special treatment for arch specific drivers here.
Mark Brown June 18, 2019, 10:57 a.m. UTC | #5
On Tue, Jun 18, 2019 at 05:13:57PM +0900, Masahisa Kojima wrote:

Please don't top post, reply in line with needed context.  This allows
readers to readily follow the flow of conversation and understand what
you are talking about and also helps ensure that everything in the
discussion is being addressed.

> There was a patch to address same compile error.
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1982194.html
> 
> Does it preferable solution to use iowrite32_rep() series?

If there's a patch that removes the need to have a hard dependency on
the particular architectures that'd definitely be better.  Separately a
patch adding a depends on the architectures with || COMPILE_TEST would
be OK though and potentially useful like Ard says.
Masahisa Kojima June 18, 2019, 11:20 a.m. UTC | #6
On Tue, 18 Jun 2019 at 19:57, Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Jun 18, 2019 at 05:13:57PM +0900, Masahisa Kojima wrote:
>
> Please don't top post, reply in line with needed context.  This allows
> readers to readily follow the flow of conversation and understand what
> you are talking about and also helps ensure that everything in the
> discussion is being addressed.

I'm sorry, I understood.

> > There was a patch to address same compile error.
> > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1982194.html
> >
> > Does it preferable solution to use iowrite32_rep() series?
>
> If there's a patch that removes the need to have a hard dependency on
> the particular architectures that'd definitely be better.  Separately a
> patch adding a depends on the architectures with || COMPILE_TEST would
> be OK though and potentially useful like Ard says.

I will try to use iowrite32_rep() series.
Mark Brown June 18, 2019, 11:30 a.m. UTC | #7
On Tue, Jun 18, 2019 at 08:20:39PM +0900, Masahisa Kojima wrote:
> On Tue, 18 Jun 2019 at 19:57, Mark Brown <broonie@kernel.org> wrote:

> > On Tue, Jun 18, 2019 at 05:13:57PM +0900, Masahisa Kojima wrote:

> > Please don't top post, reply in line with needed context.  This allows
> > readers to readily follow the flow of conversation and understand what
> > you are talking about and also helps ensure that everything in the
> > discussion is being addressed.

> I'm sorry, I understood.

Rather than writing your reply to a message at the top of the message as
you did it's better to add your reply after some of the quoted message
(as you've done here) since that makes it easier for people to read your
message and follow what the reply is about.

Patch
diff mbox series

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index ae9c81214298..667d341e8e95 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -736,6 +736,8 @@  config SPI_SUN6I
 config SPI_SYNQUACER
 	tristate "Socionext's SynQuacer HighSpeed SPI controller"
 	depends on ARCH_SYNQUACER || COMPILE_TEST
+	depends on ARM || ARM64
+	default m if ARCH_SYNQUACER
 	help
 	  SPI driver for Socionext's High speed SPI controller which provides
 	  various operating modes for interfacing to serial peripheral devices