diff mbox

[media,kbuild] Add and use IS_REACHABLE macro

Message ID 20150219121107.GA19684@sepie.suse.cz (mailing list archive)
State New, archived
Headers show

Commit Message

Michal Marek Feb. 19, 2015, 12:11 p.m. UTC 
On 2015-02-18 18:12, Arnd Bergmann wrote:
> In the media drivers, the v4l2 core knows about all submodules
> and calls into them from a common function. However this cannot
> work if the modules that get called are loadable and the
> core is built-in. In that case we get
> 
> drivers/built-in.o: In function `set_type':
> drivers/media/v4l2-core/tuner-core.c:301: undefined reference to `tea5767_attach'
> drivers/media/v4l2-core/tuner-core.c:307: undefined reference to `tea5761_attach'
> drivers/media/v4l2-core/tuner-core.c:349: undefined reference to `tda9887_attach'
> drivers/media/v4l2-core/tuner-core.c:405: undefined reference to `xc4000_attach'
> [...]
> Ideally Kconfig would be used to avoid the case of a broken dependency,
> or the code restructured in a way to turn around the dependency, but either
> way would require much larger changes here.

What can be done without extending kbuild is to accept
CONFIG_VIDEO_TUNER=y and CONFIG_MEDIA_TUNER_FOO=m, but build both into
the kernel, e.g.


Actually, I have hard time coming up with a kconfig syntactic sugar to
express such dependency. If I understand it correctly, the valid
configurations in this case are

MEDIA_TUNER_TEA5767	n	m	y
VIDEO_TUNER	n	x	x	x
		m	x	x	x
		y	x		x

I.e. only VIDEO_TUNER=y and MEDIA_TUNER_TEA5767=m is incorrect, isn't
it?

Thanks,
Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Arnd Bergmann Feb. 19, 2015, 2:53 p.m. UTC | #1 
On Thursday 19 February 2015 13:11:07 Michal Marek wrote:
> On 2015-02-18 18:12, Arnd Bergmann wrote:
> > In the media drivers, the v4l2 core knows about all submodules
> > and calls into them from a common function. However this cannot
> > work if the modules that get called are loadable and the
> > core is built-in. In that case we get
> > 
> > drivers/built-in.o: In function `set_type':
> > drivers/media/v4l2-core/tuner-core.c:301: undefined reference to `tea5767_attach'
> > drivers/media/v4l2-core/tuner-core.c:307: undefined reference to `tea5761_attach'
> > drivers/media/v4l2-core/tuner-core.c:349: undefined reference to `tda9887_attach'
> > drivers/media/v4l2-core/tuner-core.c:405: undefined reference to `xc4000_attach'
> > [...]
> > Ideally Kconfig would be used to avoid the case of a broken dependency,
> > or the code restructured in a way to turn around the dependency, but either
> > way would require much larger changes here.
> 
> What can be done without extending kbuild is to accept
> CONFIG_VIDEO_TUNER=y and CONFIG_MEDIA_TUNER_FOO=m, but build both into
> the kernel, e.g.

Right, but

> diff --git a/drivers/media/tuners/Kconfig b/drivers/media/tuners/Kconfig
> index 42e5a01..d2c7e89 100644
> --- a/drivers/media/tuners/Kconfig
> +++ b/drivers/media/tuners/Kconfig
> @@ -71,6 +71,11 @@ config MEDIA_TUNER_TEA5767
>         help
>           Say Y here to include support for the Philips TEA5767 radio tuner.
>  
> +config MEDIA_TUNER_TEA5767_BUILD
> +       tristate
> +       default VIDEO_TUNER || MEDIA_TUNER_TEA5767
> +       depends on MEDIA_TUNER_TEA5767!=n
> +
>  config MEDIA_TUNER_MSI001
>         tristate "Mirics MSi001"
>         depends on MEDIA_SUPPORT && SPI && VIDEO_V4L2

We'd then have to do the same for each tuner driver that we have in the
kernel or that gets added later. My patch was intended to just restore
the previous behavior that was accidentally changed as part of a misguided
cleanup.

> Actually, I have hard time coming up with a kconfig syntactic sugar to
> express such dependency. If I understand it correctly, the valid
> configurations in this case are
> 
> MEDIA_TUNER_TEA5767     n       m       y
> VIDEO_TUNER     n       x       x       x
>                 m       x       x       x
>                 y       x               x
> 
> I.e. only VIDEO_TUNER=y and MEDIA_TUNER_TEA5767=m is incorrect, isn't
> it?

Yes, I think that is correct. We have similar problems in other areas
of the kernel. In theory, we could enforce the VIDEO_TUNER driver to
be modular here by adding lots of dependencies to it:

config VIDEO_TUNER
	tristate
	depends on MEDIA_TUNER_TEA5761 || !MEDIA_TUNER_TEA5761
	depends on MEDIA_TUNER_TEA5767 || !MEDIA_TUNER_TEA5767
	depends on MEDIA_TUNER_MSI001  || !MEDIA_TUNER_MSI001

but that would also soon get out of hand, and we probably need another
indirection here too, for each symbol that selects VIDEO_TUNER.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michal Marek Feb. 19, 2015, 3:06 p.m. UTC | #2 
Dne 19.2.2015 v 15:53 Arnd Bergmann napsal(a):
> On Thursday 19 February 2015 13:11:07 Michal Marek wrote:
>> On 2015-02-18 18:12, Arnd Bergmann wrote:
>>> In the media drivers, the v4l2 core knows about all submodules
>>> and calls into them from a common function. However this cannot
>>> work if the modules that get called are loadable and the
>>> core is built-in. In that case we get
>>>
>>> drivers/built-in.o: In function `set_type':
>>> drivers/media/v4l2-core/tuner-core.c:301: undefined reference to `tea5767_attach'
>>> drivers/media/v4l2-core/tuner-core.c:307: undefined reference to `tea5761_attach'
>>> drivers/media/v4l2-core/tuner-core.c:349: undefined reference to `tda9887_attach'
>>> drivers/media/v4l2-core/tuner-core.c:405: undefined reference to `xc4000_attach'
>>> [...]
>>> Ideally Kconfig would be used to avoid the case of a broken dependency,
>>> or the code restructured in a way to turn around the dependency, but either
>>> way would require much larger changes here.
>>
>> What can be done without extending kbuild is to accept
>> CONFIG_VIDEO_TUNER=y and CONFIG_MEDIA_TUNER_FOO=m, but build both into
>> the kernel, e.g.
> 
> Right, but
> 
>> diff --git a/drivers/media/tuners/Kconfig b/drivers/media/tuners/Kconfig
>> index 42e5a01..d2c7e89 100644
>> --- a/drivers/media/tuners/Kconfig
>> +++ b/drivers/media/tuners/Kconfig
>> @@ -71,6 +71,11 @@ config MEDIA_TUNER_TEA5767
>>         help
>>           Say Y here to include support for the Philips TEA5767 radio tuner.
>>  
>> +config MEDIA_TUNER_TEA5767_BUILD
>> +       tristate
>> +       default VIDEO_TUNER || MEDIA_TUNER_TEA5767
>> +       depends on MEDIA_TUNER_TEA5767!=n
>> +
>>  config MEDIA_TUNER_MSI001
>>         tristate "Mirics MSi001"
>>         depends on MEDIA_SUPPORT && SPI && VIDEO_V4L2
> 
> We'd then have to do the same for each tuner driver that we have in the
> kernel or that gets added later.

Yes :-(.


>> Actually, I have hard time coming up with a kconfig syntactic sugar to
>> express such dependency. If I understand it correctly, the valid
>> configurations in this case are
>>
>> MEDIA_TUNER_TEA5767     n       m       y
>> VIDEO_TUNER     n       x       x       x
>>                 m       x       x       x
>>                 y       x               x
>>
>> I.e. only VIDEO_TUNER=y and MEDIA_TUNER_TEA5767=m is incorrect, isn't
>> it?
> 
> Yes, I think that is correct.

OK. In this case, a kconfig extension would be really too specific: "if
set, set to at least X but only if X is set as well."


> We have similar problems in other areas
> of the kernel. In theory, we could enforce the VIDEO_TUNER driver to
> be modular here by adding lots of dependencies to it:
> 
> config VIDEO_TUNER
> 	tristate
> 	depends on MEDIA_TUNER_TEA5761 || !MEDIA_TUNER_TEA5761
> 	depends on MEDIA_TUNER_TEA5767 || !MEDIA_TUNER_TEA5767
> 	depends on MEDIA_TUNER_MSI001  || !MEDIA_TUNER_MSI001

Nah, that's even uglier. I suggest to merge your IS_REACHABLE patch.

Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Feb. 20, 2015, 9:29 a.m. UTC | #3 
On Thursday 19 February 2015 16:06:18 Michal Marek wrote:
> > We have similar problems in other areas
> > of the kernel. In theory, we could enforce the VIDEO_TUNER driver to
> > be modular here by adding lots of dependencies to it:
> > 
> > config VIDEO_TUNER
> >       tristate
> >       depends on MEDIA_TUNER_TEA5761 || !MEDIA_TUNER_TEA5761
> >       depends on MEDIA_TUNER_TEA5767 || !MEDIA_TUNER_TEA5767
> >       depends on MEDIA_TUNER_MSI001  || !MEDIA_TUNER_MSI001
> 
> Nah, that's even uglier. I suggest to merge your IS_REACHABLE patch.
> 

Ok, can I take this as an ack from your side to merge the
include/linux/kconfig.h part of the patch through the linux-media
tree?

I thought about splitting up the patch into two, but that would
just make merging it harder because we'd still have the dependency.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mauro Carvalho Chehab Feb. 20, 2015, 9:37 a.m. UTC | #4 
Em Fri, 20 Feb 2015 10:29:42 +0100
Arnd Bergmann <arnd@arndb.de> escreveu:

> On Thursday 19 February 2015 16:06:18 Michal Marek wrote:
> > > We have similar problems in other areas
> > > of the kernel. In theory, we could enforce the VIDEO_TUNER driver to
> > > be modular here by adding lots of dependencies to it:
> > > 
> > > config VIDEO_TUNER
> > >       tristate
> > >       depends on MEDIA_TUNER_TEA5761 || !MEDIA_TUNER_TEA5761
> > >       depends on MEDIA_TUNER_TEA5767 || !MEDIA_TUNER_TEA5767
> > >       depends on MEDIA_TUNER_MSI001  || !MEDIA_TUNER_MSI001
> > 
> > Nah, that's even uglier. I suggest to merge your IS_REACHABLE patch.
> > 
> 
> Ok, can I take this as an ack from your side to merge the
> include/linux/kconfig.h part of the patch through the linux-media
> tree?
> 
> I thought about splitting up the patch into two, but that would
> just make merging it harder because we'd still have the dependency.

It is likely better if I merge at the linux-media tree. This way,
we can avoid conflicts with those headers, if this is ok for
Marek.

Regards,
Mauro

> 
> 	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michal Marek Feb. 20, 2015, 10:24 a.m. UTC | #5 
On 2015-02-20 10:29, Arnd Bergmann wrote:
> On Thursday 19 February 2015 16:06:18 Michal Marek wrote:
>>> We have similar problems in other areas
>>> of the kernel. In theory, we could enforce the VIDEO_TUNER driver to
>>> be modular here by adding lots of dependencies to it:
>>>
>>> config VIDEO_TUNER
>>>       tristate
>>>       depends on MEDIA_TUNER_TEA5761 || !MEDIA_TUNER_TEA5761
>>>       depends on MEDIA_TUNER_TEA5767 || !MEDIA_TUNER_TEA5767
>>>       depends on MEDIA_TUNER_MSI001  || !MEDIA_TUNER_MSI001
>>
>> Nah, that's even uglier. I suggest to merge your IS_REACHABLE patch.
>>
> 
> Ok, can I take this as an ack from your side to merge the
> include/linux/kconfig.h part of the patch through the linux-media
> tree?

Yes. If you want

Acked-by: Michal Marek <mmarek@suse.cz> [kconfig]


> I thought about splitting up the patch into two, but that would
> just make merging it harder because we'd still have the dependency.

Agreed, no need to pedantically split patches just for the sake of it.

Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/media/tuners/Kconfig b/drivers/media/tuners/Kconfig
index 42e5a01..d2c7e89 100644
--- a/drivers/media/tuners/Kconfig
+++ b/drivers/media/tuners/Kconfig
@@ -71,6 +71,11 @@  config MEDIA_TUNER_TEA5767
 	help
 	  Say Y here to include support for the Philips TEA5767 radio tuner.
 
+config MEDIA_TUNER_TEA5767_BUILD
+	tristate
+	default VIDEO_TUNER || MEDIA_TUNER_TEA5767
+	depends on MEDIA_TUNER_TEA5767!=n
+
 config MEDIA_TUNER_MSI001
 	tristate "Mirics MSi001"
 	depends on MEDIA_SUPPORT && SPI && VIDEO_V4L2