Re: [Linux-kernel-mentees] modprobing 'vidtv' doesn't really do anything

[Mauro Carvalho Chehab <mchehab@kernel.org>]

Em Sun, 17 May 2020 01:29:35 +0200
Mauro Carvalho Chehab <mchehab@kernel.org> escreveu:

> Em Sat, 16 May 2020 19:09:35 -0300
> "Daniel W. S. Almeida" <dwlsalmeida@gmail.com> escreveu:
> 
> > Hi Mauro!
> > 
> > I am trying to iron out the bugs in the virtual DVB driver I have been
> > working on for the past few months.
> > 
> > modprobing vidtv_bridge apparently works, i.e. 'vidtv_bridge' gets
> > listed in the output of 'lsmod' and there's a message on dmesg warning
> > against loading out of tree modules.
> 
> The warning is probably due to that:
> 
> WARNING: modpost: missing MODULE_LICENSE() in drivers/media/test-drivers/vidtv//vidtv_demod.o
> WARNING: modpost: missing MODULE_LICENSE() in drivers/media/test-drivers/vidtv//vidtv_bridge.o
> 
> You forgot to add MODULE_LICENSE() macro somewhere.
> 
> With is weird, as those are there:
> 
> 	drivers/media/test-drivers/vidtv/vidtv_bridge.c:MODULE_AUTHOR("Daniel W. S. Almeida");
> 	drivers/media/test-drivers/vidtv/vidtv_bridge.c:MODULE_LICENSE("GPL");
> 	drivers/media/test-drivers/vidtv/vidtv_bridge.c:MODULE_DEVICE_TABLE(i2c, vidtv_bridge_id_table);
> 
> (yet, a good practice nowadays is to place all those three at the end of
> a file - not at the beginning)
> 
> This is weird:
> 
> 	drivers/media/test-drivers/vidtv/vidtv_bridge.c:module_i2c_driver(vidtv_bridge_driver);
> 	drivers/media/test-drivers/vidtv/vidtv_demod.c:module_i2c_driver(vidtv_demod_i2c_driver);
> 	drivers/media/test-drivers/vidtv/vidtv_tuner.c:module_i2c_driver(vidtv_tuner_i2c_driver);
> 
> With the above, none of the three files will be initialized, as they
> would wait for some other driver to bind them on some I2C bus.
> 
> See, the way the Kernel works is that each bus has some sort
> of code that initializes the driver. For buses like PCI and USB,
> this happens when an USB ID or PCI ID matches their device tables.
> For normal[1] I2C devices, the driver which creates an I2C adapter
> should either manually bind new I2C devices or ask the I2C core
> to scan.
> 
> [1] ACPI devices using I2C buses use a different logic.
> 
> So, basically, the module_i2c_driver() tells the driver code
> that those devices will be available when some other driver
> would need to bind them. This is the right thing to do for the
> tuner and demod, but the bridge driver should do something else.
> 
> I would be expecting at the bridge driver something like:
> 
> 	static struct platform_driver vidtv_bridge_driver = {
> 		.probe		= vidtv_bridge_probe,
> 		.remove		= vidtv_bridge_remove,
> 		.driver		= {
> 			.name	= "vidtv-bridge",
> 		},
> 	};
> 	module_platform_driver(vidtv_bridge_driver);
> 
> Note: the other media virtual drivers don't use the 
> "module_platform_driver" macro. 
> 
> While I don't test this for a while, but looking at the code
> differences, they also declare a "platform_driver". So, they use 
> a more verbose syntax (see at the end). I suspect that this is
> needed for those devices to be probed unconditionally[2].
> 
> [1] a real platform driver is probed via DT or some other
> mechanism, like ACPI.
> 
> Thanks,
> Mauro
> 
> -
> 
> This is what vim2m.c does, for example:
> 
> 
> static struct platform_driver vim2m_pdrv = {
> 	.probe		= vim2m_probe,
> 	.remove		= vim2m_remove,
> 	.driver		= {
> 		.name	= MEM2MEM_NAME,
> 	},
> };
> 
> static void __exit vim2m_exit(void)
> {
> 	platform_driver_unregister(&vim2m_pdrv);
> 	platform_device_unregister(&vim2m_pdev);
> }
> 
> static int __init vim2m_init(void)
> {
> 	int ret;
> 
> 	ret = platform_device_register(&vim2m_pdev);
> 	if (ret)
> 		return ret;
> 
> 	ret = platform_driver_register(&vim2m_pdrv);
> 	if (ret)
> 		platform_device_unregister(&vim2m_pdev);
> 
> 	return ret;
> }
> 
> module_init(vim2m_init);
> module_exit(vim2m_exit);

There's also a problem at the Makefile. The way it was defined, the
vidtv-bridge driver won't be built.

The enclosed patch should fix the issues. The bridge driver doesn't 
build, though.

Thanks,
Mauro

---

diff --git a/drivers/media/test-drivers/vidtv/Makefile b/drivers/media/test-drivers/vidtv/Makefile
index a1d29001fffe..a56ad8bce0cb 100644
--- a/drivers/media/test-drivers/vidtv/Makefile
+++ b/drivers/media/test-drivers/vidtv/Makefile
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 
 vidtv_demod-objs := vidtv_common.o
-vidtv_bridge-objs := vidtv_common.o vidtv_ts.o vidtv_psi.o vidtv_pes.o \
-		     vidtv_s302m.o vidtv_channel.o vidtv_mux.o
+vidtv-objs := vidtv_common.o vidtv_ts.o vidtv_psi.o vidtv_pes.o \
+	      vidtv_s302m.o vidtv_channel.o vidtv_mux.o vidtv_bridge.o
 
 obj-$(CONFIG_DVB_VIDTV)	+= vidtv_tuner.o vidtv_demod.o vidtv_bridge.o
diff --git a/drivers/media/test-drivers/vidtv/vidtv_bridge.c b/drivers/media/test-drivers/vidtv/vidtv_bridge.c
index c9876372fdeb..762abf45dda0 100644
--- a/drivers/media/test-drivers/vidtv/vidtv_bridge.c
+++ b/drivers/media/test-drivers/vidtv/vidtv_bridge.c
@@ -20,9 +20,6 @@
 #define TUNER_DEFAULT_ADDR 0x68
 #define DEMOD_DEFAULT_ADDR 0x60
 
-MODULE_AUTHOR("Daniel W. S. Almeida");
-MODULE_LICENSE("GPL");
-
 static unsigned int drop_tslock_prob_on_low_snr;
 module_param(drop_tslock_prob_on_low_snr, uint, 0644);
 MODULE_PARM_DESC(drop_tslock_prob_on_low_snr,
@@ -422,21 +419,44 @@ static int vidtv_bridge_i2c_remove(struct i2c_client *client)
 	return 0;
 }
 
-static const struct i2c_device_id vidtv_bridge_id_table[] = {
-	{"vidtv_bridge", 0},
-	{}
+static struct platform_device vidtv_bridge_dev = {
+	.name		= "vidtv_bridge",
+	.dev.release	= vidtv_bridge_dev_release,
 };
 
-MODULE_DEVICE_TABLE(i2c, vidtv_bridge_id_table);
-
-static struct i2c_driver vidtv_bridge_driver = {
+static struct platform_driver vidtv_bridge_driver = {
 	.driver = {
 		.name                = "vidtv_bridge",
 		.suppress_bind_attrs = true,
 	},
 	.probe    = vidtv_bridge_i2c_probe,
 	.remove   = vidtv_bridge_i2c_remove,
-	.id_table = vidtv_bridge_id_table,
 };
 
-module_i2c_driver(vidtv_bridge_driver);
+static void __exit vidtv_bridge_exit(void)
+{
+	platform_driver_unregister(&vidtv_bridge_dev);
+	platform_device_unregister(&vidtv_bridge_driver);
+}
+
+static int __init vidtv_bridge_init(void)
+{
+	int ret;
+
+	ret = platform_device_register(&vidtv_bridge_dev);
+	if (ret)
+		return ret;
+
+	ret = platform_driver_register(&vidtv_bridge_driver);
+	if (ret)
+		platform_device_unregister(&vidtv_bridge_pdev);
+
+	return ret;
+}
+
+module_init(vidtv_bridge_init);
+module_exit(vidtv_bridge_exit);
+
+MODULE_DESCRIPTION("Virtual Digital TV Test Driver");
+MODULE_AUTHOR("Daniel W. S. Almeida");
+MODULE_LICENSE("GPL");
diff --git a/drivers/media/test-drivers/vidtv/vidtv_demod.c b/drivers/media/test-drivers/vidtv/vidtv_demod.c
index 15436e565a7b..01cd4a93b0ec 100644
--- a/drivers/media/test-drivers/vidtv/vidtv_demod.c
+++ b/drivers/media/test-drivers/vidtv/vidtv_demod.c
@@ -21,10 +21,6 @@
 #include "vidtv_demod.h"
 #include "vidtv_config.h"
 
-MODULE_DESCRIPTION("Virtual DVB Demodulator Driver");
-MODULE_AUTHOR("Daniel W. S. Almeida");
-MODULE_LICENSE("GPL");
-
 struct vidtv_demod_cnr_to_qual_s vidtv_demod_c_cnr_2_qual[] = {
 	/* from libdvbv5 source code, in milli db */
 	{ QAM_256, FEC_NONE,  34000, 38000},
@@ -492,3 +488,7 @@ static struct i2c_driver vidtv_demod_i2c_driver = {
 };
 
 module_i2c_driver(vidtv_demod_i2c_driver);
+
+MODULE_DESCRIPTION("Virtual DVB Demodulator Driver");
+MODULE_AUTHOR("Daniel W. S. Almeida");
+MODULE_LICENSE("GPL");

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees