diff mbox series

[v4,05/41] counter: add HAS_IOPORT dependencies

Message ID 20230516110038.2413224-6-schnelle@linux.ibm.com
State New
Headers
Series treewide: Remove I/O port accessors for HAS_IOPORT=n |

Commit Message

Niklas Schnelle May 16, 2023, 11 a.m. UTC 
  In a future patch HAS_IOPORT=n will result in inb()/outb() and friends
not being declared. We thus need to add HAS_IOPORT as dependency for
those drivers using them.

Co-developed-by: Arnd Bergmann <arnd@kernel.org>
Signed-off-by: Arnd Bergmann <arnd@kernel.org>
Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
Note: The HAS_IOPORT Kconfig option was added in v6.4-rc1 so
      per-subsystem patches may be applied independently

 drivers/counter/Kconfig | 1 +
 1 file changed, 1 insertion(+)

Comments

William Breathitt Gray May 19, 2023, 1:26 a.m. UTC | #1 
On Tue, May 16, 2023 at 01:00:01PM +0200, Niklas Schnelle wrote:
> In a future patch HAS_IOPORT=n will result in inb()/outb() and friends
> not being declared. We thus need to add HAS_IOPORT as dependency for
> those drivers using them.
> 
> Co-developed-by: Arnd Bergmann <arnd@kernel.org>
> Signed-off-by: Arnd Bergmann <arnd@kernel.org>
> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>

Hi Niklas,

The change itself is fine, but please update the description to reflect
that this is adding a depends on HAS_IOPORT_MAP rather than HAS_IOPORT,
along with the reason why it's needed (i.e. devm_ioport_map() is used).

Thanks,

William Breathitt Gray

> ---
> Note: The HAS_IOPORT Kconfig option was added in v6.4-rc1 so
>       per-subsystem patches may be applied independently
> 
>  drivers/counter/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
> index 4228be917038..e65a2bf178b8 100644
> --- a/drivers/counter/Kconfig
> +++ b/drivers/counter/Kconfig
> @@ -15,6 +15,7 @@ if COUNTER
>  config 104_QUAD_8
>  	tristate "ACCES 104-QUAD-8 driver"
>  	depends on (PC104 && X86) || COMPILE_TEST
> +	depends on HAS_IOPORT_MAP
>  	select ISA_BUS_API
>  	help
>  	  Say yes here to build support for the ACCES 104-QUAD-8 quadrature
> -- 
> 2.39.2
>
Niklas Schnelle May 19, 2023, 1:17 p.m. UTC | #2 
On Thu, 2023-05-18 at 21:26 -0400, William Breathitt Gray wrote:
> On Tue, May 16, 2023 at 01:00:01PM +0200, Niklas Schnelle wrote:
> > In a future patch HAS_IOPORT=n will result in inb()/outb() and friends
> > not being declared. We thus need to add HAS_IOPORT as dependency for
> > those drivers using them.
> > 
> > Co-developed-by: Arnd Bergmann <arnd@kernel.org>
> > Signed-off-by: Arnd Bergmann <arnd@kernel.org>
> > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> 
> Hi Niklas,
> 
> The change itself is fine, but please update the description to reflect
> that this is adding a depends on HAS_IOPORT_MAP rather than HAS_IOPORT,
> along with the reason why it's needed (i.e. devm_ioport_map() is used).
> 
> Thanks,
> 
> William Breathitt Gray
> 
> 

Right, this clearly needs adjustment. I went with the following commit
message for v5:

"counter: add HAS_IOPORT_MAP dependency

The 104_QUAD_8 counter driver uses devm_ioport_map() without depending
on HAS_IOPORT_MAP. This causes compilation to fail on platforms such as
s390 which do not support I/O port mapping. Add the missing
HAS_IOPORT_MAP dependency to fix this."

Thanks,
Niklas
Niklas Schnelle May 19, 2023, 1:38 p.m. UTC | #3 
On Fri, 2023-05-19 at 15:17 +0200, Niklas Schnelle wrote:
> On Thu, 2023-05-18 at 21:26 -0400, William Breathitt Gray wrote:
> > On Tue, May 16, 2023 at 01:00:01PM +0200, Niklas Schnelle wrote:
> > > In a future patch HAS_IOPORT=n will result in inb()/outb() and friends
> > > not being declared. We thus need to add HAS_IOPORT as dependency for
> > > those drivers using them.
> > > 
> > > Co-developed-by: Arnd Bergmann <arnd@kernel.org>
> > > Signed-off-by: Arnd Bergmann <arnd@kernel.org>
> > > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> > 
> > Hi Niklas,
> > 
> > The change itself is fine, but please update the description to reflect
> > that this is adding a depends on HAS_IOPORT_MAP rather than HAS_IOPORT,
> > along with the reason why it's needed (i.e. devm_ioport_map() is used).
> > 
> > Thanks,
> > 
> > William Breathitt Gray
> > 
> > 
> 
> Right, this clearly needs adjustment. I went with the following commit
> message for v5:
> 
> "counter: add HAS_IOPORT_MAP dependency
> 
> The 104_QUAD_8 counter driver uses devm_ioport_map() without depending
> on HAS_IOPORT_MAP. This causes compilation to fail on platforms such as
> s390 which do not support I/O port mapping. Add the missing
> HAS_IOPORT_MAP dependency to fix this."
> 

Just noticed this isn't entirely correct. As devm_ioport_map() has an
empty stub for HAS_IOPORT_MAP=n this doesn't lead to a compile error it
just doesn't work. Will reword to "This causes the driver to not be
useable on platforms ..."
Niklas Schnelle May 19, 2023, 1:39 p.m. UTC | #4 
On Fri, 2023-05-19 at 15:38 +0200, Niklas Schnelle wrote:
> On Fri, 2023-05-19 at 15:17 +0200, Niklas Schnelle wrote:
> > On Thu, 2023-05-18 at 21:26 -0400, William Breathitt Gray wrote:
> > > On Tue, May 16, 2023 at 01:00:01PM +0200, Niklas Schnelle wrote:
> > > > In a future patch HAS_IOPORT=n will result in inb()/outb() and friends
> > > > not being declared. We thus need to add HAS_IOPORT as dependency for
> > > > those drivers using them.
> > > > 
> > > > Co-developed-by: Arnd Bergmann <arnd@kernel.org>
> > > > Signed-off-by: Arnd Bergmann <arnd@kernel.org>
> > > > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> > > 
> > > Hi Niklas,
> > > 
> > > The change itself is fine, but please update the description to reflect
> > > that this is adding a depends on HAS_IOPORT_MAP rather than HAS_IOPORT,
> > > along with the reason why it's needed (i.e. devm_ioport_map() is used).
> > > 
> > > Thanks,
> > > 
> > > William Breathitt Gray
> > > 
> > > 
> > 
> > Right, this clearly needs adjustment. I went with the following commit
> > message for v5:
> > 
> > "counter: add HAS_IOPORT_MAP dependency
> > 
> > The 104_QUAD_8 counter driver uses devm_ioport_map() without depending
> > on HAS_IOPORT_MAP. This causes compilation to fail on platforms such as
> > s390 which do not support I/O port mapping. Add the missing
> > HAS_IOPORT_MAP dependency to fix this."
> > 
> 
> Just noticed this isn't entirely correct. As devm_ioport_map() has an
> empty stub for HAS_IOPORT_MAP=n this doesn't lead to a compile error it
> just doesn't work. Will reword to "This causes the driver to not be
> useable on platforms ..."

s/useable/usable/
William Breathitt Gray May 19, 2023, 2:21 p.m. UTC | #5 
On Fri, May 19, 2023 at 03:39:57PM +0200, Niklas Schnelle wrote:
> On Fri, 2023-05-19 at 15:38 +0200, Niklas Schnelle wrote:
> > On Fri, 2023-05-19 at 15:17 +0200, Niklas Schnelle wrote:
> > > On Thu, 2023-05-18 at 21:26 -0400, William Breathitt Gray wrote:
> > > > On Tue, May 16, 2023 at 01:00:01PM +0200, Niklas Schnelle wrote:
> > > > > In a future patch HAS_IOPORT=n will result in inb()/outb() and friends
> > > > > not being declared. We thus need to add HAS_IOPORT as dependency for
> > > > > those drivers using them.
> > > > > 
> > > > > Co-developed-by: Arnd Bergmann <arnd@kernel.org>
> > > > > Signed-off-by: Arnd Bergmann <arnd@kernel.org>
> > > > > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> > > > 
> > > > Hi Niklas,
> > > > 
> > > > The change itself is fine, but please update the description to reflect
> > > > that this is adding a depends on HAS_IOPORT_MAP rather than HAS_IOPORT,
> > > > along with the reason why it's needed (i.e. devm_ioport_map() is used).
> > > > 
> > > > Thanks,
> > > > 
> > > > William Breathitt Gray
> > > > 
> > > > 
> > > 
> > > Right, this clearly needs adjustment. I went with the following commit
> > > message for v5:
> > > 
> > > "counter: add HAS_IOPORT_MAP dependency
> > > 
> > > The 104_QUAD_8 counter driver uses devm_ioport_map() without depending
> > > on HAS_IOPORT_MAP. This causes compilation to fail on platforms such as
> > > s390 which do not support I/O port mapping. Add the missing
> > > HAS_IOPORT_MAP dependency to fix this."
> > > 
> > 
> > Just noticed this isn't entirely correct. As devm_ioport_map() has an
> > empty stub for HAS_IOPORT_MAP=n this doesn't lead to a compile error it
> > just doesn't work. Will reword to "This causes the driver to not be
> > useable on platforms ..."
> 
> s/useable/usable/

104_QUAD_8 has an explicit dependency on PC104 and X86, so I don't think
it would ever be used outside of x86 platforms. Does it still make sense
to have the HAS_IOPORT_MAP dependency in this case?

William Breathitt Gray
Niklas Schnelle May 22, 2023, 10:42 a.m. UTC | #6 
On Fri, 2023-05-19 at 10:21 -0400, William Breathitt Gray wrote:
> On Fri, May 19, 2023 at 03:39:57PM +0200, Niklas Schnelle wrote:
> > On Fri, 2023-05-19 at 15:38 +0200, Niklas Schnelle wrote:
> > > On Fri, 2023-05-19 at 15:17 +0200, Niklas Schnelle wrote:
> > > > On Thu, 2023-05-18 at 21:26 -0400, William Breathitt Gray wrote:
> > > > > On Tue, May 16, 2023 at 01:00:01PM +0200, Niklas Schnelle wrote:
> > > > > > In a future patch HAS_IOPORT=n will result in inb()/outb() and friends
> > > > > > not being declared. We thus need to add HAS_IOPORT as dependency for
> > > > > > those drivers using them.
> > > > > > 
> > > > > > Co-developed-by: Arnd Bergmann <arnd@kernel.org>
> > > > > > Signed-off-by: Arnd Bergmann <arnd@kernel.org>
> > > > > > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> > > > > 
> > > > > Hi Niklas,
> > > > > 
> > > > > The change itself is fine, but please update the description to reflect
> > > > > that this is adding a depends on HAS_IOPORT_MAP rather than HAS_IOPORT,
> > > > > along with the reason why it's needed (i.e. devm_ioport_map() is used).
> > > > > 
> > > > > Thanks,
> > > > > 
> > > > > William Breathitt Gray
> > > > > 
> > > > > 
> > > > 
> > > > Right, this clearly needs adjustment. I went with the following commit
> > > > message for v5:
> > > > 
> > > > "counter: add HAS_IOPORT_MAP dependency
> > > > 
> > > > The 104_QUAD_8 counter driver uses devm_ioport_map() without depending
> > > > on HAS_IOPORT_MAP. This causes compilation to fail on platforms such as
> > > > s390 which do not support I/O port mapping. Add the missing
> > > > HAS_IOPORT_MAP dependency to fix this."
> > > > 
> > > 
> > > Just noticed this isn't entirely correct. As devm_ioport_map() has an
> > > empty stub for HAS_IOPORT_MAP=n this doesn't lead to a compile error it
> > > just doesn't work. Will reword to "This causes the driver to not be
> > > useable on platforms ..."
> > 
> > s/useable/usable/
> 
> 104_QUAD_8 has an explicit dependency on PC104 and X86, so I don't think
> it would ever be used outside of x86 platforms. Does it still make sense
> to have the HAS_IOPORT_MAP dependency in this case?
> 
> William Breathitt Gray

Well, yes and no, you're right that it doesn't really cause compile
issues despite the "|| COMPILE_TEST" albeit the code could never work.
Still, I'd add the dependency. At the very least it serves as
documentation and maybe in the future someone will want to remove those
empty stubs for HAS_IOPORT_MAP=n.

Thanks
Niklas
William Breathitt Gray May 22, 2023, 11:15 a.m. UTC | #7 
On Mon, May 22, 2023 at 12:42:15PM +0200, Niklas Schnelle wrote:
> On Fri, 2023-05-19 at 10:21 -0400, William Breathitt Gray wrote:
> > On Fri, May 19, 2023 at 03:39:57PM +0200, Niklas Schnelle wrote:
> > > On Fri, 2023-05-19 at 15:38 +0200, Niklas Schnelle wrote:
> > > > On Fri, 2023-05-19 at 15:17 +0200, Niklas Schnelle wrote:
> > > > > On Thu, 2023-05-18 at 21:26 -0400, William Breathitt Gray wrote:
> > > > > > On Tue, May 16, 2023 at 01:00:01PM +0200, Niklas Schnelle wrote:
> > > > > > > In a future patch HAS_IOPORT=n will result in inb()/outb() and friends
> > > > > > > not being declared. We thus need to add HAS_IOPORT as dependency for
> > > > > > > those drivers using them.
> > > > > > > 
> > > > > > > Co-developed-by: Arnd Bergmann <arnd@kernel.org>
> > > > > > > Signed-off-by: Arnd Bergmann <arnd@kernel.org>
> > > > > > > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> > > > > > 
> > > > > > Hi Niklas,
> > > > > > 
> > > > > > The change itself is fine, but please update the description to reflect
> > > > > > that this is adding a depends on HAS_IOPORT_MAP rather than HAS_IOPORT,
> > > > > > along with the reason why it's needed (i.e. devm_ioport_map() is used).
> > > > > > 
> > > > > > Thanks,
> > > > > > 
> > > > > > William Breathitt Gray
> > > > > > 
> > > > > > 
> > > > > 
> > > > > Right, this clearly needs adjustment. I went with the following commit
> > > > > message for v5:
> > > > > 
> > > > > "counter: add HAS_IOPORT_MAP dependency
> > > > > 
> > > > > The 104_QUAD_8 counter driver uses devm_ioport_map() without depending
> > > > > on HAS_IOPORT_MAP. This causes compilation to fail on platforms such as
> > > > > s390 which do not support I/O port mapping. Add the missing
> > > > > HAS_IOPORT_MAP dependency to fix this."
> > > > > 
> > > > 
> > > > Just noticed this isn't entirely correct. As devm_ioport_map() has an
> > > > empty stub for HAS_IOPORT_MAP=n this doesn't lead to a compile error it
> > > > just doesn't work. Will reword to "This causes the driver to not be
> > > > useable on platforms ..."
> > > 
> > > s/useable/usable/
> > 
> > 104_QUAD_8 has an explicit dependency on PC104 and X86, so I don't think
> > it would ever be used outside of x86 platforms. Does it still make sense
> > to have the HAS_IOPORT_MAP dependency in this case?
> > 
> > William Breathitt Gray
> 
> Well, yes and no, you're right that it doesn't really cause compile
> issues despite the "|| COMPILE_TEST" albeit the code could never work.
> Still, I'd add the dependency. At the very least it serves as
> documentation and maybe in the future someone will want to remove those
> empty stubs for HAS_IOPORT_MAP=n.
> 
> Thanks
> Niklas

Sure, that reasoning makes sense to me too, so let's go with the
explicit depends afterall.

By the way, I noticed two other modules that call devm_ioport_map() but
seem to be missing the HAS_IOPORT_MAP depends lines: the
drivers/iio/addac/stx104.c and drivers/iio/dac/cio-dac.c drivers. Do
these need respective patches as well?

As an aside, I haven't been following the previous patchsets closely so
forgive me if this has already been discussed in another thread: why
doesn't X86 automatically select HAS_IOPORT? Are there x86 platforms
that do not support ioport?

William Breathitt Gray
diff mbox series

Patch

diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
index 4228be917038..e65a2bf178b8 100644
--- a/drivers/counter/Kconfig
+++ b/drivers/counter/Kconfig
@@ -15,6 +15,7 @@  if COUNTER
 config 104_QUAD_8
 	tristate "ACCES 104-QUAD-8 driver"
 	depends on (PC104 && X86) || COMPILE_TEST
+	depends on HAS_IOPORT_MAP
 	select ISA_BUS_API
 	help
 	  Say yes here to build support for the ACCES 104-QUAD-8 quadrature