* linux-next: build warning after merge of the v4l-dvb tree
@ 2021-02-08 0:32 Stephen Rothwell
2021-02-08 6:30 ` Mauro Carvalho Chehab
2021-02-08 6:53 ` [PATCH] media: i2c: fix max9271 build dependencies Mauro Carvalho Chehab
0 siblings, 2 replies; 55+ messages in thread
From: Stephen Rothwell @ 2021-02-08 0:32 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Jacopo Mondi, Sakari Ailus, Linux Kernel Mailing List,
Linux Next Mailing List
[-- Attachment #1: Type: text/plain, Size: 2251 bytes --]
Hi all,
After merging the v4l-dvb tree, today's linux-next build (x86_64
allmodconfig) produced this warning:
WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_serial_link' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_configure_i2c' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_high_threshold' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_configure_gmsl_link' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_clear_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_enable_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_disable_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_verify_id' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_address' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_deserializer_address' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_translation' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
Introduced by commit
a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: linux-next: build warning after merge of the v4l-dvb tree
2021-02-08 0:32 linux-next: build warning after merge of the v4l-dvb tree Stephen Rothwell
@ 2021-02-08 6:30 ` Mauro Carvalho Chehab
2021-02-08 8:33 ` Geert Uytterhoeven
2021-02-08 6:53 ` [PATCH] media: i2c: fix max9271 build dependencies Mauro Carvalho Chehab
1 sibling, 1 reply; 55+ messages in thread
From: Mauro Carvalho Chehab @ 2021-02-08 6:30 UTC (permalink / raw)
To: Stephen Rothwell
Cc: Jacopo Mondi, Sakari Ailus, Linux Kernel Mailing List,
Linux Next Mailing List
Em Mon, 8 Feb 2021 11:32:08 +1100
Stephen Rothwell <sfr@canb.auug.org.au> escreveu:
> Hi all,
>
> After merging the v4l-dvb tree, today's linux-next build (x86_64
> allmodconfig) produced this warning:
>
> WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_serial_link' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_configure_i2c' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_high_threshold' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_configure_gmsl_link' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_clear_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_enable_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_disable_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_verify_id' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_address' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_deserializer_address' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_translation' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
>
> Introduced by commit
>
> a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
>
It sounds to be due to a Makefile mess:
drivers/media/i2c/Makefile:rdacm20-camera_module-objs := rdacm20.o max9271.o
drivers/media/i2c/Makefile:rdacm21-camera_module-objs := rdacm21.o max9271.o
Neither drivers should be including max9271.o as their objects, but, instead,
be addressing max9271 dependency via Kconfig.
Thanks,
Mauro
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH] media: i2c: fix max9271 build dependencies
2021-02-08 0:32 linux-next: build warning after merge of the v4l-dvb tree Stephen Rothwell
2021-02-08 6:30 ` Mauro Carvalho Chehab
@ 2021-02-08 6:53 ` Mauro Carvalho Chehab
2021-02-08 7:27 ` Sakari Ailus
1 sibling, 1 reply; 55+ messages in thread
From: Mauro Carvalho Chehab @ 2021-02-08 6:53 UTC (permalink / raw)
To: Sakari Ailus, Linux Media Mailing List
Cc: Laurent Pinchart, Jacopo Mondi, Stephen Rothwell, linux-next,
Mauro Carvalho Chehab
As described on its c file, the Maxim MAX9271 GMSL serializer isn't a
self-contained driver, as MAX9271 is usually embedded in camera modules
with at least one image sensor and optional additional components,
such as uController units or ISPs/DSPs.
After chanseset a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module"),
there are now two drivers currently needing it: rdacm20 and rdacm21.
Building with allmodconfig is now causing those warnings:
WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_serial_link' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_configure_i2c' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_high_threshold' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_configure_gmsl_link' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_clear_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_enable_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_disable_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_verify_id' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_address' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_deserializer_address' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_translation' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
Address the issue by adding a Kconfig item for it, that it is
seleced if either one of the modules that need max9271 is used.
Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
drivers/media/i2c/Kconfig | 10 ++++++++++
drivers/media/i2c/Makefile | 8 ++++----
2 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 2d3dc0d82f9e..a6802195d583 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -712,6 +712,16 @@ config VIDEO_ST_MIPID02
module will be called st-mipid02.
endmenu
+#
+# Camera ancillary chips
+#
+
+# MAX9271 is usually embedded in camera modules
+config VIDEO_MAX9271_SERIALIZER
+ tristate
+ default y
+ depends on VIDEO_RDACM20 || VIDEO_RDACM21
+
#
# V4L2 I2C drivers that are related with Camera support
#
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 6bd22d63e1a7..63bb16e51876 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -125,10 +125,10 @@ obj-$(CONFIG_VIDEO_IMX319) += imx319.o
obj-$(CONFIG_VIDEO_IMX334) += imx334.o
obj-$(CONFIG_VIDEO_IMX355) += imx355.o
obj-$(CONFIG_VIDEO_MAX9286) += max9286.o
-rdacm20-camera_module-objs := rdacm20.o max9271.o
-obj-$(CONFIG_VIDEO_RDACM20) += rdacm20-camera_module.o
-rdacm21-camera_module-objs := rdacm21.o max9271.o
-obj-$(CONFIG_VIDEO_RDACM21) += rdacm21-camera_module.o
+obj-$(CONFIG_VIDEO_RDACM20) += rdacm20.o
+obj-$(CONFIG_VIDEO_RDACM21) += rdacm21.o
obj-$(CONFIG_VIDEO_ST_MIPID02) += st-mipid02.o
+obj-$(CONFIG_VIDEO_MAX9271_SERIALIZER) += max9271.o
+
obj-$(CONFIG_SDR_MAX2175) += max2175.o
--
2.29.2
^ permalink raw reply related [flat|nested] 55+ messages in thread
* Re: [PATCH] media: i2c: fix max9271 build dependencies
2021-02-08 6:53 ` [PATCH] media: i2c: fix max9271 build dependencies Mauro Carvalho Chehab
@ 2021-02-08 7:27 ` Sakari Ailus
2021-02-08 8:36 ` Jacopo Mondi
0 siblings, 1 reply; 55+ messages in thread
From: Sakari Ailus @ 2021-02-08 7:27 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Linux Media Mailing List, Laurent Pinchart, Jacopo Mondi,
Stephen Rothwell, linux-next
Hi Mauro,
Thanks for the patch.
On Mon, Feb 08, 2021 at 07:53:15AM +0100, Mauro Carvalho Chehab wrote:
> As described on its c file, the Maxim MAX9271 GMSL serializer isn't a
> self-contained driver, as MAX9271 is usually embedded in camera modules
> with at least one image sensor and optional additional components,
> such as uController units or ISPs/DSPs.
>
> After chanseset a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module"),
> there are now two drivers currently needing it: rdacm20 and rdacm21.
>
> Building with allmodconfig is now causing those warnings:
>
> WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_serial_link' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_configure_i2c' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_high_threshold' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_configure_gmsl_link' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_clear_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_enable_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_disable_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_verify_id' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_address' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_deserializer_address' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_translation' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
>
> Address the issue by adding a Kconfig item for it, that it is
> seleced if either one of the modules that need max9271 is used.
>
> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
> drivers/media/i2c/Kconfig | 10 ++++++++++
> drivers/media/i2c/Makefile | 8 ++++----
> 2 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 2d3dc0d82f9e..a6802195d583 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -712,6 +712,16 @@ config VIDEO_ST_MIPID02
> module will be called st-mipid02.
> endmenu
>
> +#
> +# Camera ancillary chips
> +#
> +
> +# MAX9271 is usually embedded in camera modules
> +config VIDEO_MAX9271_SERIALIZER
> + tristate
> + default y
> + depends on VIDEO_RDACM20 || VIDEO_RDACM21
> +
I'd instead make the RDACM drivers depend on this one instead. The RDACM20
driver directly depends on the symbols in the MAX9271 driver.
> #
> # V4L2 I2C drivers that are related with Camera support
> #
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index 6bd22d63e1a7..63bb16e51876 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -125,10 +125,10 @@ obj-$(CONFIG_VIDEO_IMX319) += imx319.o
> obj-$(CONFIG_VIDEO_IMX334) += imx334.o
> obj-$(CONFIG_VIDEO_IMX355) += imx355.o
> obj-$(CONFIG_VIDEO_MAX9286) += max9286.o
> -rdacm20-camera_module-objs := rdacm20.o max9271.o
> -obj-$(CONFIG_VIDEO_RDACM20) += rdacm20-camera_module.o
> -rdacm21-camera_module-objs := rdacm21.o max9271.o
> -obj-$(CONFIG_VIDEO_RDACM21) += rdacm21-camera_module.o
> +obj-$(CONFIG_VIDEO_RDACM20) += rdacm20.o
> +obj-$(CONFIG_VIDEO_RDACM21) += rdacm21.o
> obj-$(CONFIG_VIDEO_ST_MIPID02) += st-mipid02.o
>
> +obj-$(CONFIG_VIDEO_MAX9271_SERIALIZER) += max9271.o
> +
> obj-$(CONFIG_SDR_MAX2175) += max2175.o
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: linux-next: build warning after merge of the v4l-dvb tree
2021-02-08 6:30 ` Mauro Carvalho Chehab
@ 2021-02-08 8:33 ` Geert Uytterhoeven
2021-02-08 8:49 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 55+ messages in thread
From: Geert Uytterhoeven @ 2021-02-08 8:33 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Stephen Rothwell, Jacopo Mondi, Sakari Ailus,
Linux Kernel Mailing List, Linux Next Mailing List
On Mon, Feb 8, 2021 at 7:35 AM Mauro Carvalho Chehab <mchehab@kernel.org> wrote:
> Em Mon, 8 Feb 2021 11:32:08 +1100
> Stephen Rothwell <sfr@canb.auug.org.au> escreveu:
>
> > After merging the v4l-dvb tree, today's linux-next build (x86_64
> > allmodconfig) produced this warning:
> >
> > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_serial_link' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_configure_i2c' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_high_threshold' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_configure_gmsl_link' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_clear_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_enable_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_disable_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_verify_id' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_address' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_deserializer_address' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_translation' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> >
> > Introduced by commit
> >
> > a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
> >
>
> It sounds to be due to a Makefile mess:
>
> drivers/media/i2c/Makefile:rdacm20-camera_module-objs := rdacm20.o max9271.o
> drivers/media/i2c/Makefile:rdacm21-camera_module-objs := rdacm21.o max9271.o
>
> Neither drivers should be including max9271.o as their objects, but, instead,
> be addressing max9271 dependency via Kconfig.
Wouldn't
obj-$(CONFIG_VIDEO_RDACM20) += rdacm20.o max9271.o
obj-$(CONFIG_VIDEO_RDACM21) += rdacm21.o max9271.o
work, too?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] media: i2c: fix max9271 build dependencies
2021-02-08 7:27 ` Sakari Ailus
@ 2021-02-08 8:36 ` Jacopo Mondi
2021-02-08 8:41 ` Sakari Ailus
0 siblings, 1 reply; 55+ messages in thread
From: Jacopo Mondi @ 2021-02-08 8:36 UTC (permalink / raw)
To: Sakari Ailus
Cc: Mauro Carvalho Chehab, Linux Media Mailing List,
Laurent Pinchart, Jacopo Mondi, Stephen Rothwell, linux-next
Hello everyone,
On Mon, Feb 08, 2021 at 09:27:01AM +0200, Sakari Ailus wrote:
> Hi Mauro,
>
> Thanks for the patch.
Sorry, that's cleary an oversight from my side.
Thanks for tackling it.
>
> On Mon, Feb 08, 2021 at 07:53:15AM +0100, Mauro Carvalho Chehab wrote:
> > As described on its c file, the Maxim MAX9271 GMSL serializer isn't a
> > self-contained driver, as MAX9271 is usually embedded in camera modules
> > with at least one image sensor and optional additional components,
> > such as uController units or ISPs/DSPs.
> >
> > After chanseset a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module"),
> > there are now two drivers currently needing it: rdacm20 and rdacm21.
> >
> > Building with allmodconfig is now causing those warnings:
> >
> > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_serial_link' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_configure_i2c' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_high_threshold' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_configure_gmsl_link' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_clear_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_enable_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_disable_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_verify_id' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_address' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_deserializer_address' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_translation' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> >
> > Address the issue by adding a Kconfig item for it, that it is
> > seleced if either one of the modules that need max9271 is used.
> >
> > Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> > Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > ---
> > drivers/media/i2c/Kconfig | 10 ++++++++++
> > drivers/media/i2c/Makefile | 8 ++++----
> > 2 files changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > index 2d3dc0d82f9e..a6802195d583 100644
> > --- a/drivers/media/i2c/Kconfig
> > +++ b/drivers/media/i2c/Kconfig
> > @@ -712,6 +712,16 @@ config VIDEO_ST_MIPID02
> > module will be called st-mipid02.
> > endmenu
> >
> > +#
> > +# Camera ancillary chips
> > +#
> > +
> > +# MAX9271 is usually embedded in camera modules
> > +config VIDEO_MAX9271_SERIALIZER
> > + tristate
> > + default y
> > + depends on VIDEO_RDACM20 || VIDEO_RDACM21
> > +
>
> I'd instead make the RDACM drivers depend on this one instead. The RDACM20
> driver directly depends on the symbols in the MAX9271 driver.
>
OTOH I it makes sense to have MAX9271 depend on the camera modules, as
selecting the serializer alone is not that useful.
Could the two camera modules symbols instead select the MAX9271 one ?
> > #
> > # V4L2 I2C drivers that are related with Camera support
> > #
> > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> > index 6bd22d63e1a7..63bb16e51876 100644
> > --- a/drivers/media/i2c/Makefile
> > +++ b/drivers/media/i2c/Makefile
> > @@ -125,10 +125,10 @@ obj-$(CONFIG_VIDEO_IMX319) += imx319.o
> > obj-$(CONFIG_VIDEO_IMX334) += imx334.o
> > obj-$(CONFIG_VIDEO_IMX355) += imx355.o
> > obj-$(CONFIG_VIDEO_MAX9286) += max9286.o
> > -rdacm20-camera_module-objs := rdacm20.o max9271.o
> > -obj-$(CONFIG_VIDEO_RDACM20) += rdacm20-camera_module.o
> > -rdacm21-camera_module-objs := rdacm21.o max9271.o
> > -obj-$(CONFIG_VIDEO_RDACM21) += rdacm21-camera_module.o
> > +obj-$(CONFIG_VIDEO_RDACM20) += rdacm20.o
> > +obj-$(CONFIG_VIDEO_RDACM21) += rdacm21.o
> > obj-$(CONFIG_VIDEO_ST_MIPID02) += st-mipid02.o
> >
> > +obj-$(CONFIG_VIDEO_MAX9271_SERIALIZER) += max9271.o
> > +
> > obj-$(CONFIG_SDR_MAX2175) += max2175.o
>
> --
> Kind regards,
>
> Sakari Ailus
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] media: i2c: fix max9271 build dependencies
2021-02-08 8:36 ` Jacopo Mondi
@ 2021-02-08 8:41 ` Sakari Ailus
2021-02-08 8:59 ` Jacopo Mondi
2021-02-08 9:08 ` Mauro Carvalho Chehab
0 siblings, 2 replies; 55+ messages in thread
From: Sakari Ailus @ 2021-02-08 8:41 UTC (permalink / raw)
To: Jacopo Mondi
Cc: Mauro Carvalho Chehab, Linux Media Mailing List,
Laurent Pinchart, Jacopo Mondi, Stephen Rothwell, linux-next
Hi Jacopo,
On Mon, Feb 08, 2021 at 09:36:16AM +0100, Jacopo Mondi wrote:
> Hello everyone,
>
> On Mon, Feb 08, 2021 at 09:27:01AM +0200, Sakari Ailus wrote:
> > Hi Mauro,
> >
> > Thanks for the patch.
>
> Sorry, that's cleary an oversight from my side.
> Thanks for tackling it.
>
> >
> > On Mon, Feb 08, 2021 at 07:53:15AM +0100, Mauro Carvalho Chehab wrote:
> > > As described on its c file, the Maxim MAX9271 GMSL serializer isn't a
> > > self-contained driver, as MAX9271 is usually embedded in camera modules
> > > with at least one image sensor and optional additional components,
> > > such as uController units or ISPs/DSPs.
> > >
> > > After chanseset a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module"),
> > > there are now two drivers currently needing it: rdacm20 and rdacm21.
> > >
> > > Building with allmodconfig is now causing those warnings:
> > >
> > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_serial_link' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_configure_i2c' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_high_threshold' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_configure_gmsl_link' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_clear_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_enable_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_disable_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_verify_id' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_address' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_deserializer_address' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_translation' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > >
> > > Address the issue by adding a Kconfig item for it, that it is
> > > seleced if either one of the modules that need max9271 is used.
> > >
> > > Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> > > Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > ---
> > > drivers/media/i2c/Kconfig | 10 ++++++++++
> > > drivers/media/i2c/Makefile | 8 ++++----
> > > 2 files changed, 14 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > > index 2d3dc0d82f9e..a6802195d583 100644
> > > --- a/drivers/media/i2c/Kconfig
> > > +++ b/drivers/media/i2c/Kconfig
> > > @@ -712,6 +712,16 @@ config VIDEO_ST_MIPID02
> > > module will be called st-mipid02.
> > > endmenu
> > >
> > > +#
> > > +# Camera ancillary chips
> > > +#
> > > +
> > > +# MAX9271 is usually embedded in camera modules
> > > +config VIDEO_MAX9271_SERIALIZER
> > > + tristate
> > > + default y
> > > + depends on VIDEO_RDACM20 || VIDEO_RDACM21
> > > +
> >
> > I'd instead make the RDACM drivers depend on this one instead. The RDACM20
> > driver directly depends on the symbols in the MAX9271 driver.
> >
>
> OTOH I it makes sense to have MAX9271 depend on the camera modules, as
> selecting the serializer alone is not that useful.
>
> Could the two camera modules symbols instead select the MAX9271 one ?
MAX9271 could be used elsewhere than in RDACM* devices.
Also, as select does not handle dependencies, all drivers that need MAX9271
would have to include the dependencies of MAX9271.
>
> > > #
> > > # V4L2 I2C drivers that are related with Camera support
> > > #
> > > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> > > index 6bd22d63e1a7..63bb16e51876 100644
> > > --- a/drivers/media/i2c/Makefile
> > > +++ b/drivers/media/i2c/Makefile
> > > @@ -125,10 +125,10 @@ obj-$(CONFIG_VIDEO_IMX319) += imx319.o
> > > obj-$(CONFIG_VIDEO_IMX334) += imx334.o
> > > obj-$(CONFIG_VIDEO_IMX355) += imx355.o
> > > obj-$(CONFIG_VIDEO_MAX9286) += max9286.o
> > > -rdacm20-camera_module-objs := rdacm20.o max9271.o
> > > -obj-$(CONFIG_VIDEO_RDACM20) += rdacm20-camera_module.o
> > > -rdacm21-camera_module-objs := rdacm21.o max9271.o
> > > -obj-$(CONFIG_VIDEO_RDACM21) += rdacm21-camera_module.o
> > > +obj-$(CONFIG_VIDEO_RDACM20) += rdacm20.o
> > > +obj-$(CONFIG_VIDEO_RDACM21) += rdacm21.o
> > > obj-$(CONFIG_VIDEO_ST_MIPID02) += st-mipid02.o
> > >
> > > +obj-$(CONFIG_VIDEO_MAX9271_SERIALIZER) += max9271.o
> > > +
> > > obj-$(CONFIG_SDR_MAX2175) += max2175.o
> >
> > --
> > Kind regards,
> >
> > Sakari Ailus
--
Sakari Ailus
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: linux-next: build warning after merge of the v4l-dvb tree
2021-02-08 8:33 ` Geert Uytterhoeven
@ 2021-02-08 8:49 ` Mauro Carvalho Chehab
2021-02-08 8:52 ` Geert Uytterhoeven
0 siblings, 1 reply; 55+ messages in thread
From: Mauro Carvalho Chehab @ 2021-02-08 8:49 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Stephen Rothwell, Jacopo Mondi, Sakari Ailus,
Linux Kernel Mailing List, Linux Next Mailing List
Em Mon, 8 Feb 2021 09:33:14 +0100
Geert Uytterhoeven <geert@linux-m68k.org> escreveu:
> On Mon, Feb 8, 2021 at 7:35 AM Mauro Carvalho Chehab <mchehab@kernel.org> wrote:
> > Em Mon, 8 Feb 2021 11:32:08 +1100
> > Stephen Rothwell <sfr@canb.auug.org.au> escreveu:
> >
> > > After merging the v4l-dvb tree, today's linux-next build (x86_64
> > > allmodconfig) produced this warning:
> > >
> > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_serial_link' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_configure_i2c' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_high_threshold' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_configure_gmsl_link' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_clear_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_enable_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_disable_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_verify_id' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_address' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_deserializer_address' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_translation' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > >
> > > Introduced by commit
> > >
> > > a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
> > >
> >
> > It sounds to be due to a Makefile mess:
> >
> > drivers/media/i2c/Makefile:rdacm20-camera_module-objs := rdacm20.o max9271.o
> > drivers/media/i2c/Makefile:rdacm21-camera_module-objs := rdacm21.o max9271.o
> >
> > Neither drivers should be including max9271.o as their objects, but, instead,
> > be addressing max9271 dependency via Kconfig.
>
> Wouldn't
>
> obj-$(CONFIG_VIDEO_RDACM20) += rdacm20.o max9271.o
> obj-$(CONFIG_VIDEO_RDACM21) += rdacm21.o max9271.o
>
> work, too?
Not 100% sure, but I guess this would cause problems with allyesconfig.
Thanks,
Mauro
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: linux-next: build warning after merge of the v4l-dvb tree
2021-02-08 8:49 ` Mauro Carvalho Chehab
@ 2021-02-08 8:52 ` Geert Uytterhoeven
2021-02-08 9:14 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 55+ messages in thread
From: Geert Uytterhoeven @ 2021-02-08 8:52 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Stephen Rothwell, Jacopo Mondi, Sakari Ailus,
Linux Kernel Mailing List, Linux Next Mailing List
Hi Mauro,
On Mon, Feb 8, 2021 at 9:49 AM Mauro Carvalho Chehab <mchehab@kernel.org> wrote:
> Em Mon, 8 Feb 2021 09:33:14 +0100
> Geert Uytterhoeven <geert@linux-m68k.org> escreveu:
> > On Mon, Feb 8, 2021 at 7:35 AM Mauro Carvalho Chehab <mchehab@kernel.org> wrote:
> > > Em Mon, 8 Feb 2021 11:32:08 +1100
> > > Stephen Rothwell <sfr@canb.auug.org.au> escreveu:
> > >
> > > > After merging the v4l-dvb tree, today's linux-next build (x86_64
> > > > allmodconfig) produced this warning:
> > > >
> > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_serial_link' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_configure_i2c' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_high_threshold' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_configure_gmsl_link' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_clear_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_enable_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_disable_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_verify_id' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_address' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_deserializer_address' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_translation' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > >
> > > > Introduced by commit
> > > >
> > > > a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
> > > >
> > >
> > > It sounds to be due to a Makefile mess:
> > >
> > > drivers/media/i2c/Makefile:rdacm20-camera_module-objs := rdacm20.o max9271.o
> > > drivers/media/i2c/Makefile:rdacm21-camera_module-objs := rdacm21.o max9271.o
> > >
> > > Neither drivers should be including max9271.o as their objects, but, instead,
> > > be addressing max9271 dependency via Kconfig.
> >
> > Wouldn't
> >
> > obj-$(CONFIG_VIDEO_RDACM20) += rdacm20.o max9271.o
> > obj-$(CONFIG_VIDEO_RDACM21) += rdacm21.o max9271.o
> >
> > work, too?
>
> Not 100% sure, but I guess this would cause problems with allyesconfig.
Duplicates will be filtered out.
An example using this method is drivers/net/ethernet/8390/Makefile.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] media: i2c: fix max9271 build dependencies
2021-02-08 8:41 ` Sakari Ailus
@ 2021-02-08 8:59 ` Jacopo Mondi
2021-02-08 9:03 ` Sakari Ailus
2021-02-08 9:08 ` Mauro Carvalho Chehab
1 sibling, 1 reply; 55+ messages in thread
From: Jacopo Mondi @ 2021-02-08 8:59 UTC (permalink / raw)
To: Sakari Ailus
Cc: Mauro Carvalho Chehab, Linux Media Mailing List,
Laurent Pinchart, Jacopo Mondi, Stephen Rothwell, linux-next
Hi Sakari,
On Mon, Feb 08, 2021 at 10:41:47AM +0200, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Mon, Feb 08, 2021 at 09:36:16AM +0100, Jacopo Mondi wrote:
> > Hello everyone,
> >
> > On Mon, Feb 08, 2021 at 09:27:01AM +0200, Sakari Ailus wrote:
> > > Hi Mauro,
> > >
> > > Thanks for the patch.
> >
> > Sorry, that's cleary an oversight from my side.
> > Thanks for tackling it.
> >
> > >
> > > On Mon, Feb 08, 2021 at 07:53:15AM +0100, Mauro Carvalho Chehab wrote:
> > > > As described on its c file, the Maxim MAX9271 GMSL serializer isn't a
> > > > self-contained driver, as MAX9271 is usually embedded in camera modules
> > > > with at least one image sensor and optional additional components,
> > > > such as uController units or ISPs/DSPs.
> > > >
> > > > After chanseset a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module"),
> > > > there are now two drivers currently needing it: rdacm20 and rdacm21.
> > > >
> > > > Building with allmodconfig is now causing those warnings:
> > > >
> > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_serial_link' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_configure_i2c' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_high_threshold' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_configure_gmsl_link' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_clear_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_enable_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_disable_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_verify_id' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_address' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_deserializer_address' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_translation' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > >
> > > > Address the issue by adding a Kconfig item for it, that it is
> > > > seleced if either one of the modules that need max9271 is used.
> > > >
> > > > Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> > > > Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
> > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > > ---
> > > > drivers/media/i2c/Kconfig | 10 ++++++++++
> > > > drivers/media/i2c/Makefile | 8 ++++----
> > > > 2 files changed, 14 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > > > index 2d3dc0d82f9e..a6802195d583 100644
> > > > --- a/drivers/media/i2c/Kconfig
> > > > +++ b/drivers/media/i2c/Kconfig
> > > > @@ -712,6 +712,16 @@ config VIDEO_ST_MIPID02
> > > > module will be called st-mipid02.
> > > > endmenu
> > > >
> > > > +#
> > > > +# Camera ancillary chips
> > > > +#
> > > > +
> > > > +# MAX9271 is usually embedded in camera modules
> > > > +config VIDEO_MAX9271_SERIALIZER
> > > > + tristate
> > > > + default y
> > > > + depends on VIDEO_RDACM20 || VIDEO_RDACM21
> > > > +
> > >
> > > I'd instead make the RDACM drivers depend on this one instead. The RDACM20
> > > driver directly depends on the symbols in the MAX9271 driver.
> > >
> >
> > OTOH I it makes sense to have MAX9271 depend on the camera modules, as
> > selecting the serializer alone is not that useful.
> >
> > Could the two camera modules symbols instead select the MAX9271 one ?
>
> MAX9271 could be used elsewhere than in RDACM* devices.
Shouldn't other camera modules that include such serializer select its
symbol then ?
>
> Also, as select does not handle dependencies, all drivers that need MAX9271
> would have to include the dependencies of MAX9271.
>
Wouldn't select just select the MAX9271 symbol, which will list its own
dependencies ?
> >
> > > > #
> > > > # V4L2 I2C drivers that are related with Camera support
> > > > #
> > > > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> > > > index 6bd22d63e1a7..63bb16e51876 100644
> > > > --- a/drivers/media/i2c/Makefile
> > > > +++ b/drivers/media/i2c/Makefile
> > > > @@ -125,10 +125,10 @@ obj-$(CONFIG_VIDEO_IMX319) += imx319.o
> > > > obj-$(CONFIG_VIDEO_IMX334) += imx334.o
> > > > obj-$(CONFIG_VIDEO_IMX355) += imx355.o
> > > > obj-$(CONFIG_VIDEO_MAX9286) += max9286.o
> > > > -rdacm20-camera_module-objs := rdacm20.o max9271.o
> > > > -obj-$(CONFIG_VIDEO_RDACM20) += rdacm20-camera_module.o
> > > > -rdacm21-camera_module-objs := rdacm21.o max9271.o
> > > > -obj-$(CONFIG_VIDEO_RDACM21) += rdacm21-camera_module.o
> > > > +obj-$(CONFIG_VIDEO_RDACM20) += rdacm20.o
> > > > +obj-$(CONFIG_VIDEO_RDACM21) += rdacm21.o
> > > > obj-$(CONFIG_VIDEO_ST_MIPID02) += st-mipid02.o
> > > >
> > > > +obj-$(CONFIG_VIDEO_MAX9271_SERIALIZER) += max9271.o
> > > > +
> > > > obj-$(CONFIG_SDR_MAX2175) += max2175.o
> > >
> > > --
> > > Kind regards,
> > >
> > > Sakari Ailus
>
> --
> Sakari Ailus
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] media: i2c: fix max9271 build dependencies
2021-02-08 8:59 ` Jacopo Mondi
@ 2021-02-08 9:03 ` Sakari Ailus
0 siblings, 0 replies; 55+ messages in thread
From: Sakari Ailus @ 2021-02-08 9:03 UTC (permalink / raw)
To: Jacopo Mondi
Cc: Mauro Carvalho Chehab, Linux Media Mailing List,
Laurent Pinchart, Jacopo Mondi, Stephen Rothwell, linux-next
On Mon, Feb 08, 2021 at 09:59:07AM +0100, Jacopo Mondi wrote:
> Hi Sakari,
>
> On Mon, Feb 08, 2021 at 10:41:47AM +0200, Sakari Ailus wrote:
> > Hi Jacopo,
> >
> > On Mon, Feb 08, 2021 at 09:36:16AM +0100, Jacopo Mondi wrote:
> > > Hello everyone,
> > >
> > > On Mon, Feb 08, 2021 at 09:27:01AM +0200, Sakari Ailus wrote:
> > > > Hi Mauro,
> > > >
> > > > Thanks for the patch.
> > >
> > > Sorry, that's cleary an oversight from my side.
> > > Thanks for tackling it.
> > >
> > > >
> > > > On Mon, Feb 08, 2021 at 07:53:15AM +0100, Mauro Carvalho Chehab wrote:
> > > > > As described on its c file, the Maxim MAX9271 GMSL serializer isn't a
> > > > > self-contained driver, as MAX9271 is usually embedded in camera modules
> > > > > with at least one image sensor and optional additional components,
> > > > > such as uController units or ISPs/DSPs.
> > > > >
> > > > > After chanseset a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module"),
> > > > > there are now two drivers currently needing it: rdacm20 and rdacm21.
> > > > >
> > > > > Building with allmodconfig is now causing those warnings:
> > > > >
> > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_serial_link' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_configure_i2c' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_high_threshold' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_configure_gmsl_link' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_clear_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_enable_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_disable_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_verify_id' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_address' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_deserializer_address' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_translation' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > >
> > > > > Address the issue by adding a Kconfig item for it, that it is
> > > > > seleced if either one of the modules that need max9271 is used.
> > > > >
> > > > > Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> > > > > Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
> > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > > > ---
> > > > > drivers/media/i2c/Kconfig | 10 ++++++++++
> > > > > drivers/media/i2c/Makefile | 8 ++++----
> > > > > 2 files changed, 14 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > > > > index 2d3dc0d82f9e..a6802195d583 100644
> > > > > --- a/drivers/media/i2c/Kconfig
> > > > > +++ b/drivers/media/i2c/Kconfig
> > > > > @@ -712,6 +712,16 @@ config VIDEO_ST_MIPID02
> > > > > module will be called st-mipid02.
> > > > > endmenu
> > > > >
> > > > > +#
> > > > > +# Camera ancillary chips
> > > > > +#
> > > > > +
> > > > > +# MAX9271 is usually embedded in camera modules
> > > > > +config VIDEO_MAX9271_SERIALIZER
> > > > > + tristate
> > > > > + default y
> > > > > + depends on VIDEO_RDACM20 || VIDEO_RDACM21
> > > > > +
> > > >
> > > > I'd instead make the RDACM drivers depend on this one instead. The RDACM20
> > > > driver directly depends on the symbols in the MAX9271 driver.
> > > >
> > >
> > > OTOH I it makes sense to have MAX9271 depend on the camera modules, as
> > > selecting the serializer alone is not that useful.
> > >
> > > Could the two camera modules symbols instead select the MAX9271 one ?
> >
> > MAX9271 could be used elsewhere than in RDACM* devices.
>
> Shouldn't other camera modules that include such serializer select its
> symbol then ?
>
> >
> > Also, as select does not handle dependencies, all drivers that need MAX9271
> > would have to include the dependencies of MAX9271.
> >
>
> Wouldn't select just select the MAX9271 symbol, which will list its own
> dependencies ?
You can have anything there but it'll be ignored.
Documentation/kbuild/kconfig-language.rst:
select should be used with care. select will force
a symbol to a value without visiting the dependencies.
>
>
> > >
> > > > > #
> > > > > # V4L2 I2C drivers that are related with Camera support
> > > > > #
> > > > > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> > > > > index 6bd22d63e1a7..63bb16e51876 100644
> > > > > --- a/drivers/media/i2c/Makefile
> > > > > +++ b/drivers/media/i2c/Makefile
> > > > > @@ -125,10 +125,10 @@ obj-$(CONFIG_VIDEO_IMX319) += imx319.o
> > > > > obj-$(CONFIG_VIDEO_IMX334) += imx334.o
> > > > > obj-$(CONFIG_VIDEO_IMX355) += imx355.o
> > > > > obj-$(CONFIG_VIDEO_MAX9286) += max9286.o
> > > > > -rdacm20-camera_module-objs := rdacm20.o max9271.o
> > > > > -obj-$(CONFIG_VIDEO_RDACM20) += rdacm20-camera_module.o
> > > > > -rdacm21-camera_module-objs := rdacm21.o max9271.o
> > > > > -obj-$(CONFIG_VIDEO_RDACM21) += rdacm21-camera_module.o
> > > > > +obj-$(CONFIG_VIDEO_RDACM20) += rdacm20.o
> > > > > +obj-$(CONFIG_VIDEO_RDACM21) += rdacm21.o
> > > > > obj-$(CONFIG_VIDEO_ST_MIPID02) += st-mipid02.o
> > > > >
> > > > > +obj-$(CONFIG_VIDEO_MAX9271_SERIALIZER) += max9271.o
> > > > > +
> > > > > obj-$(CONFIG_SDR_MAX2175) += max2175.o
> > > >
> > > > --
> > > > Kind regards,
> > > >
> > > > Sakari Ailus
> >
> > --
> > Sakari Ailus
--
Sakari Ailus
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] media: i2c: fix max9271 build dependencies
2021-02-08 8:41 ` Sakari Ailus
2021-02-08 8:59 ` Jacopo Mondi
@ 2021-02-08 9:08 ` Mauro Carvalho Chehab
2021-02-08 9:24 ` Sakari Ailus
1 sibling, 1 reply; 55+ messages in thread
From: Mauro Carvalho Chehab @ 2021-02-08 9:08 UTC (permalink / raw)
To: Sakari Ailus
Cc: Jacopo Mondi, Linux Media Mailing List, Laurent Pinchart,
Jacopo Mondi, Stephen Rothwell, linux-next
Em Mon, 8 Feb 2021 10:41:47 +0200
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> Hi Jacopo,
>
> On Mon, Feb 08, 2021 at 09:36:16AM +0100, Jacopo Mondi wrote:
> > Hello everyone,
> >
> > On Mon, Feb 08, 2021 at 09:27:01AM +0200, Sakari Ailus wrote:
> > > Hi Mauro,
> > >
> > > Thanks for the patch.
> >
> > Sorry, that's cleary an oversight from my side.
> > Thanks for tackling it.
> >
> > >
> > > On Mon, Feb 08, 2021 at 07:53:15AM +0100, Mauro Carvalho Chehab wrote:
> > > > As described on its c file, the Maxim MAX9271 GMSL serializer isn't a
> > > > self-contained driver, as MAX9271 is usually embedded in camera modules
> > > > with at least one image sensor and optional additional components,
> > > > such as uController units or ISPs/DSPs.
> > > >
> > > > After chanseset a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module"),
> > > > there are now two drivers currently needing it: rdacm20 and rdacm21.
> > > >
> > > > Building with allmodconfig is now causing those warnings:
> > > >
> > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_serial_link' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_configure_i2c' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_high_threshold' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_configure_gmsl_link' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_clear_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_enable_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_disable_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_verify_id' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_address' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_deserializer_address' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_translation' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > >
> > > > Address the issue by adding a Kconfig item for it, that it is
> > > > seleced if either one of the modules that need max9271 is used.
> > > >
> > > > Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> > > > Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
> > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > > ---
> > > > drivers/media/i2c/Kconfig | 10 ++++++++++
> > > > drivers/media/i2c/Makefile | 8 ++++----
> > > > 2 files changed, 14 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > > > index 2d3dc0d82f9e..a6802195d583 100644
> > > > --- a/drivers/media/i2c/Kconfig
> > > > +++ b/drivers/media/i2c/Kconfig
> > > > @@ -712,6 +712,16 @@ config VIDEO_ST_MIPID02
> > > > module will be called st-mipid02.
> > > > endmenu
> > > >
> > > > +#
> > > > +# Camera ancillary chips
> > > > +#
> > > > +
> > > > +# MAX9271 is usually embedded in camera modules
> > > > +config VIDEO_MAX9271_SERIALIZER
> > > > + tristate
> > > > + default y
> > > > + depends on VIDEO_RDACM20 || VIDEO_RDACM21
> > > > +
> > >
> > > I'd instead make the RDACM drivers depend on this one instead. The RDACM20
> > > driver directly depends on the symbols in the MAX9271 driver.
While such solution is technically OK, it will make harder for the ones
wanting to use the RDACM drivers, because those two drivers will be
ridden at the config menu, until MAX9271 got selected.
With the above solution, this driver will be auto-selected if either
RDACM20 or RDACM21 is needed.
Btw, this is exactly the same strategy we use for tuner I2C modules:
the user doesn't need to know that his board uses tuner "foo" or "bar".
All it needs is to know that it will require, for instance, the em28xx
driver.
> > >
> >
> > OTOH I it makes sense to have MAX9271 depend on the camera modules, as
> > selecting the serializer alone is not that useful.
> >
> > Could the two camera modules symbols instead select the MAX9271 one ?
>
> MAX9271 could be used elsewhere than in RDACM* devices.
Sure, but then it is just a matter of adding the other driver there:
depends on VIDEO_RDACM20 || VIDEO_RDACM21 || VIDEO_FOO
To be frank, I doubt that we'll end having dozens of boards with this.
If we end having lots of drivers, we can work on a different
solution.
>
> Also, as select does not handle dependencies, all drivers that need MAX9271
> would have to include the dependencies of MAX9271.
Yes. "Depends on" handles any dependencies that are required for building
MAX9271. In this specific case, the dependencies are the same, so select
could equally work.
Thanks,
Mauro
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: linux-next: build warning after merge of the v4l-dvb tree
2021-02-08 8:52 ` Geert Uytterhoeven
@ 2021-02-08 9:14 ` Mauro Carvalho Chehab
0 siblings, 0 replies; 55+ messages in thread
From: Mauro Carvalho Chehab @ 2021-02-08 9:14 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Stephen Rothwell, Jacopo Mondi, Sakari Ailus,
Linux Kernel Mailing List, Linux Next Mailing List
Em Mon, 8 Feb 2021 09:52:22 +0100
Geert Uytterhoeven <geert@linux-m68k.org> escreveu:
> Hi Mauro,
>
> On Mon, Feb 8, 2021 at 9:49 AM Mauro Carvalho Chehab <mchehab@kernel.org> wrote:
> > Em Mon, 8 Feb 2021 09:33:14 +0100
> > Geert Uytterhoeven <geert@linux-m68k.org> escreveu:
> > > On Mon, Feb 8, 2021 at 7:35 AM Mauro Carvalho Chehab <mchehab@kernel.org> wrote:
> > > > Em Mon, 8 Feb 2021 11:32:08 +1100
> > > > Stephen Rothwell <sfr@canb.auug.org.au> escreveu:
> > > >
> > > > > After merging the v4l-dvb tree, today's linux-next build (x86_64
> > > > > allmodconfig) produced this warning:
> > > > >
> > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_serial_link' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_configure_i2c' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_high_threshold' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_configure_gmsl_link' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_clear_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_enable_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_disable_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_verify_id' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_address' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_deserializer_address' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_translation' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > >
> > > > > Introduced by commit
> > > > >
> > > > > a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
> > > > >
> > > >
> > > > It sounds to be due to a Makefile mess:
> > > >
> > > > drivers/media/i2c/Makefile:rdacm20-camera_module-objs := rdacm20.o max9271.o
> > > > drivers/media/i2c/Makefile:rdacm21-camera_module-objs := rdacm21.o max9271.o
> > > >
> > > > Neither drivers should be including max9271.o as their objects, but, instead,
> > > > be addressing max9271 dependency via Kconfig.
> > >
> > > Wouldn't
> > >
> > > obj-$(CONFIG_VIDEO_RDACM20) += rdacm20.o max9271.o
> > > obj-$(CONFIG_VIDEO_RDACM21) += rdacm21.o max9271.o
> > >
> > > work, too?
> >
> > Not 100% sure, but I guess this would cause problems with allyesconfig.
>
> Duplicates will be filtered out.
> An example using this method is drivers/net/ethernet/8390/Makefile.
Good to know!
From my side, I prefer that support for different chips would have
their own Kconfig vars. It seems to document better what's
happening there.
Thanks,
Mauro
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] media: i2c: fix max9271 build dependencies
2021-02-08 9:08 ` Mauro Carvalho Chehab
@ 2021-02-08 9:24 ` Sakari Ailus
2021-02-08 10:07 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 55+ messages in thread
From: Sakari Ailus @ 2021-02-08 9:24 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Jacopo Mondi, Linux Media Mailing List, Laurent Pinchart,
Jacopo Mondi, Stephen Rothwell, linux-next
On Mon, Feb 08, 2021 at 10:08:22AM +0100, Mauro Carvalho Chehab wrote:
> Em Mon, 8 Feb 2021 10:41:47 +0200
> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
>
> > Hi Jacopo,
> >
> > On Mon, Feb 08, 2021 at 09:36:16AM +0100, Jacopo Mondi wrote:
> > > Hello everyone,
> > >
> > > On Mon, Feb 08, 2021 at 09:27:01AM +0200, Sakari Ailus wrote:
> > > > Hi Mauro,
> > > >
> > > > Thanks for the patch.
> > >
> > > Sorry, that's cleary an oversight from my side.
> > > Thanks for tackling it.
> > >
> > > >
> > > > On Mon, Feb 08, 2021 at 07:53:15AM +0100, Mauro Carvalho Chehab wrote:
> > > > > As described on its c file, the Maxim MAX9271 GMSL serializer isn't a
> > > > > self-contained driver, as MAX9271 is usually embedded in camera modules
> > > > > with at least one image sensor and optional additional components,
> > > > > such as uController units or ISPs/DSPs.
> > > > >
> > > > > After chanseset a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module"),
> > > > > there are now two drivers currently needing it: rdacm20 and rdacm21.
> > > > >
> > > > > Building with allmodconfig is now causing those warnings:
> > > > >
> > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_serial_link' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_configure_i2c' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_high_threshold' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_configure_gmsl_link' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_clear_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_enable_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_disable_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_verify_id' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_address' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_deserializer_address' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_translation' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > >
> > > > > Address the issue by adding a Kconfig item for it, that it is
> > > > > seleced if either one of the modules that need max9271 is used.
> > > > >
> > > > > Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> > > > > Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
> > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > > > ---
> > > > > drivers/media/i2c/Kconfig | 10 ++++++++++
> > > > > drivers/media/i2c/Makefile | 8 ++++----
> > > > > 2 files changed, 14 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > > > > index 2d3dc0d82f9e..a6802195d583 100644
> > > > > --- a/drivers/media/i2c/Kconfig
> > > > > +++ b/drivers/media/i2c/Kconfig
> > > > > @@ -712,6 +712,16 @@ config VIDEO_ST_MIPID02
> > > > > module will be called st-mipid02.
> > > > > endmenu
> > > > >
> > > > > +#
> > > > > +# Camera ancillary chips
> > > > > +#
> > > > > +
> > > > > +# MAX9271 is usually embedded in camera modules
> > > > > +config VIDEO_MAX9271_SERIALIZER
> > > > > + tristate
> > > > > + default y
> > > > > + depends on VIDEO_RDACM20 || VIDEO_RDACM21
> > > > > +
> > > >
> > > > I'd instead make the RDACM drivers depend on this one instead. The RDACM20
> > > > driver directly depends on the symbols in the MAX9271 driver.
>
> While such solution is technically OK, it will make harder for the ones
> wanting to use the RDACM drivers, because those two drivers will be
> ridden at the config menu, until MAX9271 got selected.
>
> With the above solution, this driver will be auto-selected if either
> RDACM20 or RDACM21 is needed.
>
> Btw, this is exactly the same strategy we use for tuner I2C modules:
> the user doesn't need to know that his board uses tuner "foo" or "bar".
> All it needs is to know that it will require, for instance, the em28xx
> driver.
>
> > > >
> > >
> > > OTOH I it makes sense to have MAX9271 depend on the camera modules, as
> > > selecting the serializer alone is not that useful.
> > >
> > > Could the two camera modules symbols instead select the MAX9271 one ?
> >
> > MAX9271 could be used elsewhere than in RDACM* devices.
>
> Sure, but then it is just a matter of adding the other driver there:
>
> depends on VIDEO_RDACM20 || VIDEO_RDACM21 || VIDEO_FOO
>
> To be frank, I doubt that we'll end having dozens of boards with this.
> If we end having lots of drivers, we can work on a different
> solution.
Also note that there will be combinations of something compiled as a module
and another driver linked directly to the kernel.
So each of the RDACM drivers would likely also need:
depends on VIDEO_MAX9271_SERIALIZER || !VIDEO_MAX9271_SERIALIZER
>
> >
> > Also, as select does not handle dependencies, all drivers that need MAX9271
> > would have to include the dependencies of MAX9271.
>
> Yes. "Depends on" handles any dependencies that are required for building
> MAX9271. In this specific case, the dependencies are the same, so select
> could equally work.
Sure.
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] media: i2c: fix max9271 build dependencies
2021-02-08 9:24 ` Sakari Ailus
@ 2021-02-08 10:07 ` Mauro Carvalho Chehab
2021-02-08 11:32 ` Laurent Pinchart
0 siblings, 1 reply; 55+ messages in thread
From: Mauro Carvalho Chehab @ 2021-02-08 10:07 UTC (permalink / raw)
To: Sakari Ailus
Cc: Jacopo Mondi, Linux Media Mailing List, Laurent Pinchart,
Jacopo Mondi, Stephen Rothwell, linux-next
Em Mon, 8 Feb 2021 11:24:25 +0200
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> On Mon, Feb 08, 2021 at 10:08:22AM +0100, Mauro Carvalho Chehab wrote:
> > Em Mon, 8 Feb 2021 10:41:47 +0200
> > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> >
> > > Hi Jacopo,
> > >
> > > On Mon, Feb 08, 2021 at 09:36:16AM +0100, Jacopo Mondi wrote:
> > > > Hello everyone,
> > > >
> > > > On Mon, Feb 08, 2021 at 09:27:01AM +0200, Sakari Ailus wrote:
> > > > > Hi Mauro,
> > > > >
> > > > > Thanks for the patch.
> > > >
> > > > Sorry, that's cleary an oversight from my side.
> > > > Thanks for tackling it.
> > > >
> > > > >
> > > > > On Mon, Feb 08, 2021 at 07:53:15AM +0100, Mauro Carvalho Chehab wrote:
> > > > > > As described on its c file, the Maxim MAX9271 GMSL serializer isn't a
> > > > > > self-contained driver, as MAX9271 is usually embedded in camera modules
> > > > > > with at least one image sensor and optional additional components,
> > > > > > such as uController units or ISPs/DSPs.
> > > > > >
> > > > > > After chanseset a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module"),
> > > > > > there are now two drivers currently needing it: rdacm20 and rdacm21.
> > > > > >
> > > > > > Building with allmodconfig is now causing those warnings:
> > > > > >
> > > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_serial_link' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_configure_i2c' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_high_threshold' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_configure_gmsl_link' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_clear_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_enable_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_disable_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_verify_id' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_address' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_deserializer_address' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_translation' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > >
> > > > > > Address the issue by adding a Kconfig item for it, that it is
> > > > > > seleced if either one of the modules that need max9271 is used.
> > > > > >
> > > > > > Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> > > > > > Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
> > > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > > > > ---
> > > > > > drivers/media/i2c/Kconfig | 10 ++++++++++
> > > > > > drivers/media/i2c/Makefile | 8 ++++----
> > > > > > 2 files changed, 14 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > > > > > index 2d3dc0d82f9e..a6802195d583 100644
> > > > > > --- a/drivers/media/i2c/Kconfig
> > > > > > +++ b/drivers/media/i2c/Kconfig
> > > > > > @@ -712,6 +712,16 @@ config VIDEO_ST_MIPID02
> > > > > > module will be called st-mipid02.
> > > > > > endmenu
> > > > > >
> > > > > > +#
> > > > > > +# Camera ancillary chips
> > > > > > +#
> > > > > > +
> > > > > > +# MAX9271 is usually embedded in camera modules
> > > > > > +config VIDEO_MAX9271_SERIALIZER
> > > > > > + tristate
> > > > > > + default y
> > > > > > + depends on VIDEO_RDACM20 || VIDEO_RDACM21
> > > > > > +
> > > > >
> > > > > I'd instead make the RDACM drivers depend on this one instead. The RDACM20
> > > > > driver directly depends on the symbols in the MAX9271 driver.
> >
> > While such solution is technically OK, it will make harder for the ones
> > wanting to use the RDACM drivers, because those two drivers will be
> > ridden at the config menu, until MAX9271 got selected.
> >
> > With the above solution, this driver will be auto-selected if either
> > RDACM20 or RDACM21 is needed.
> >
> > Btw, this is exactly the same strategy we use for tuner I2C modules:
> > the user doesn't need to know that his board uses tuner "foo" or "bar".
> > All it needs is to know that it will require, for instance, the em28xx
> > driver.
> >
> > > > >
> > > >
> > > > OTOH I it makes sense to have MAX9271 depend on the camera modules, as
> > > > selecting the serializer alone is not that useful.
> > > >
> > > > Could the two camera modules symbols instead select the MAX9271 one ?
> > >
> > > MAX9271 could be used elsewhere than in RDACM* devices.
> >
> > Sure, but then it is just a matter of adding the other driver there:
> >
> > depends on VIDEO_RDACM20 || VIDEO_RDACM21 || VIDEO_FOO
> >
> > To be frank, I doubt that we'll end having dozens of boards with this.
> > If we end having lots of drivers, we can work on a different
> > solution.
>
> Also note that there will be combinations of something compiled as a module
> and another driver linked directly to the kernel.
>
> So each of the RDACM drivers would likely also need:
>
> depends on VIDEO_MAX9271_SERIALIZER || !VIDEO_MAX9271_SERIALIZER
No. I fail to see where this would work, as MAX9271 is a mandatory
dependency of those drivers. See:
static int rdacm20_s_stream(struct v4l2_subdev *sd, int enable)
{
struct rdacm20_device *dev = sd_to_rdacm20(sd);
return max9271_set_serial_link(dev->serializer, enable);
}
Without compiling max9271, the device won't stream ;-)
-
Yet, assuming that this could be an optional feature, if you use this:
config VIDEO_MAX9271_SERIALIZER
tristate
default y
depends on VIDEO_RDACM20 || VIDEO_RDACM21
You should *not* never do:
depends on VIDEO_MAX9271_SERIALIZER || !VIDEO_MAX9271_SERIALIZER
for VIDEO_RDACM20 or VIDEO_RDACM21, as it would cause a dependency
loop.
-
If you do, instead:
if VIDEO_V4L2 && I2C
config VIDEO_MAX9271_SERIALIZER
tristate
config VIDEO_RDACM20
select VIDEO_MAX9271_SERIALIZER
...
config VIDEO_RDACM21
select VIDEO_MAX9271_SERIALIZER
...
endif
Then you also won't need:
depends on VIDEO_MAX9271_SERIALIZER || !VIDEO_MAX9271_SERIALIZER
As select should do the right thing in this case, ensuring that MAX9271
will be builtin either if RDACM20 or RDACM21 is builtin.
-
This should also work:
config VIDEO_MAX9271_SERIALIZER
tristate
...
if VIDEO_MAX9271_SERIALIZER
config VIDEO_RDACM20
tristate
...
config VIDEO_RDACM21
tristate
...
endif
without needing a "depend on FOO || !FOO" logic, but, as I said before, this
case will force the user that want to build the RDACM20/RDACM21 drivers to
be aware that a MAX9271 driver is required.
-
You would only need a "depend on FOO || !FOO" if the driver FOO is
optional and if you try do something like:
config FOO
tristate
...
config VIDEO_RDACM20
tristate
depends on FOO
...
config VIDEO_RDACM21
tristate
depends on FOO
...
as, on such case, it would allow the vars to be:
FOO="m"
VIDEO_RDACM20="y"
VIDEO_RDACM21="m"
which would cause build errors.
Thanks,
Mauro
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] media: i2c: fix max9271 build dependencies
2021-02-08 10:07 ` Mauro Carvalho Chehab
@ 2021-02-08 11:32 ` Laurent Pinchart
2021-02-08 11:41 ` Jacopo Mondi
0 siblings, 1 reply; 55+ messages in thread
From: Laurent Pinchart @ 2021-02-08 11:32 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Sakari Ailus, Jacopo Mondi, Linux Media Mailing List,
Jacopo Mondi, Stephen Rothwell, linux-next
Hi Mauro,
On Mon, Feb 08, 2021 at 11:07:23AM +0100, Mauro Carvalho Chehab wrote:
> Em Mon, 8 Feb 2021 11:24:25 +0200 Sakari Ailus escreveu:
> > On Mon, Feb 08, 2021 at 10:08:22AM +0100, Mauro Carvalho Chehab wrote:
> > > Em Mon, 8 Feb 2021 10:41:47 +0200 Sakari Ailus escreveu:
> > > > On Mon, Feb 08, 2021 at 09:36:16AM +0100, Jacopo Mondi wrote:
> > > > > On Mon, Feb 08, 2021 at 09:27:01AM +0200, Sakari Ailus wrote:
> > > > > > Hi Mauro,
> > > > > >
> > > > > > Thanks for the patch.
> > > > >
> > > > > Sorry, that's cleary an oversight from my side.
> > > > > Thanks for tackling it.
> > > > >
> > > > > > On Mon, Feb 08, 2021 at 07:53:15AM +0100, Mauro Carvalho Chehab wrote:
> > > > > > > As described on its c file, the Maxim MAX9271 GMSL serializer isn't a
> > > > > > > self-contained driver, as MAX9271 is usually embedded in camera modules
> > > > > > > with at least one image sensor and optional additional components,
> > > > > > > such as uController units or ISPs/DSPs.
> > > > > > >
> > > > > > > After chanseset a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module"),
> > > > > > > there are now two drivers currently needing it: rdacm20 and rdacm21.
> > > > > > >
> > > > > > > Building with allmodconfig is now causing those warnings:
> > > > > > >
> > > > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_serial_link' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_configure_i2c' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_high_threshold' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_configure_gmsl_link' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_clear_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_enable_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_disable_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_verify_id' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_address' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_deserializer_address' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_translation' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > > >
> > > > > > > Address the issue by adding a Kconfig item for it, that it is
> > > > > > > seleced if either one of the modules that need max9271 is used.
> > > > > > >
> > > > > > > Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> > > > > > > Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
> > > > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > > > > > ---
> > > > > > > drivers/media/i2c/Kconfig | 10 ++++++++++
> > > > > > > drivers/media/i2c/Makefile | 8 ++++----
> > > > > > > 2 files changed, 14 insertions(+), 4 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > > > > > > index 2d3dc0d82f9e..a6802195d583 100644
> > > > > > > --- a/drivers/media/i2c/Kconfig
> > > > > > > +++ b/drivers/media/i2c/Kconfig
> > > > > > > @@ -712,6 +712,16 @@ config VIDEO_ST_MIPID02
> > > > > > > module will be called st-mipid02.
> > > > > > > endmenu
> > > > > > >
> > > > > > > +#
> > > > > > > +# Camera ancillary chips
> > > > > > > +#
> > > > > > > +
> > > > > > > +# MAX9271 is usually embedded in camera modules
> > > > > > > +config VIDEO_MAX9271_SERIALIZER
> > > > > > > + tristate
> > > > > > > + default y
> > > > > > > + depends on VIDEO_RDACM20 || VIDEO_RDACM21
> > > > > > > +
> > > > > >
> > > > > > I'd instead make the RDACM drivers depend on this one instead. The RDACM20
> > > > > > driver directly depends on the symbols in the MAX9271 driver.
> > >
> > > While such solution is technically OK, it will make harder for the ones
> > > wanting to use the RDACM drivers, because those two drivers will be
> > > ridden at the config menu, until MAX9271 got selected.
> > >
> > > With the above solution, this driver will be auto-selected if either
> > > RDACM20 or RDACM21 is needed.
> > >
> > > Btw, this is exactly the same strategy we use for tuner I2C modules:
> > > the user doesn't need to know that his board uses tuner "foo" or "bar".
> > > All it needs is to know that it will require, for instance, the em28xx
> > > driver.
> > >
> > > > > >
> > > > >
> > > > > OTOH I it makes sense to have MAX9271 depend on the camera modules, as
> > > > > selecting the serializer alone is not that useful.
> > > > >
> > > > > Could the two camera modules symbols instead select the MAX9271 one ?
> > > >
> > > > MAX9271 could be used elsewhere than in RDACM* devices.
> > >
> > > Sure, but then it is just a matter of adding the other driver there:
> > >
> > > depends on VIDEO_RDACM20 || VIDEO_RDACM21 || VIDEO_FOO
> > >
> > > To be frank, I doubt that we'll end having dozens of boards with this.
> > > If we end having lots of drivers, we can work on a different
> > > solution.
> >
> > Also note that there will be combinations of something compiled as a module
> > and another driver linked directly to the kernel.
> >
> > So each of the RDACM drivers would likely also need:
> >
> > depends on VIDEO_MAX9271_SERIALIZER || !VIDEO_MAX9271_SERIALIZER
>
> No. I fail to see where this would work, as MAX9271 is a mandatory
> dependency of those drivers. See:
>
> static int rdacm20_s_stream(struct v4l2_subdev *sd, int enable)
> {
> struct rdacm20_device *dev = sd_to_rdacm20(sd);
>
> return max9271_set_serial_link(dev->serializer, enable);
> }
>
>
> Without compiling max9271, the device won't stream ;-)
>
> -
>
> Yet, assuming that this could be an optional feature, if you use this:
>
> config VIDEO_MAX9271_SERIALIZER
> tristate
> default y
> depends on VIDEO_RDACM20 || VIDEO_RDACM21
>
> You should *not* never do:
>
> depends on VIDEO_MAX9271_SERIALIZER || !VIDEO_MAX9271_SERIALIZER
>
> for VIDEO_RDACM20 or VIDEO_RDACM21, as it would cause a dependency
> loop.
>
> -
>
> If you do, instead:
>
> if VIDEO_V4L2 && I2C
> config VIDEO_MAX9271_SERIALIZER
> tristate
>
> config VIDEO_RDACM20
> select VIDEO_MAX9271_SERIALIZER
> ...
>
> config VIDEO_RDACM21
> select VIDEO_MAX9271_SERIALIZER
> ...
> endif
>
> Then you also won't need:
> depends on VIDEO_MAX9271_SERIALIZER || !VIDEO_MAX9271_SERIALIZER
>
> As select should do the right thing in this case, ensuring that MAX9271
> will be builtin either if RDACM20 or RDACM21 is builtin.
I also vote for usage of "select".
> -
>
> This should also work:
>
> config VIDEO_MAX9271_SERIALIZER
> tristate
> ...
>
> if VIDEO_MAX9271_SERIALIZER
>
> config VIDEO_RDACM20
> tristate
> ...
>
> config VIDEO_RDACM21
> tristate
> ...
>
> endif
>
> without needing a "depend on FOO || !FOO" logic, but, as I said before, this
> case will force the user that want to build the RDACM20/RDACM21 drivers to
> be aware that a MAX9271 driver is required.
>
> -
>
> You would only need a "depend on FOO || !FOO" if the driver FOO is
> optional and if you try do something like:
>
> config FOO
> tristate
> ...
>
> config VIDEO_RDACM20
> tristate
> depends on FOO
> ...
>
> config VIDEO_RDACM21
> tristate
> depends on FOO
> ...
>
> as, on such case, it would allow the vars to be:
>
> FOO="m"
> VIDEO_RDACM20="y"
> VIDEO_RDACM21="m"
>
> which would cause build errors.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] media: i2c: fix max9271 build dependencies
2021-02-08 11:32 ` Laurent Pinchart
@ 2021-02-08 11:41 ` Jacopo Mondi
2021-02-08 13:11 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 55+ messages in thread
From: Jacopo Mondi @ 2021-02-08 11:41 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mauro Carvalho Chehab, Sakari Ailus, Linux Media Mailing List,
Jacopo Mondi, Stephen Rothwell, linux-next
On Mon, Feb 08, 2021 at 01:32:38PM +0200, Laurent Pinchart wrote:
> Hi Mauro,
>
> On Mon, Feb 08, 2021 at 11:07:23AM +0100, Mauro Carvalho Chehab wrote:
> > Em Mon, 8 Feb 2021 11:24:25 +0200 Sakari Ailus escreveu:
> > > On Mon, Feb 08, 2021 at 10:08:22AM +0100, Mauro Carvalho Chehab wrote:
> > > > Em Mon, 8 Feb 2021 10:41:47 +0200 Sakari Ailus escreveu:
> > > > > On Mon, Feb 08, 2021 at 09:36:16AM +0100, Jacopo Mondi wrote:
> > > > > > On Mon, Feb 08, 2021 at 09:27:01AM +0200, Sakari Ailus wrote:
> > > > > > > Hi Mauro,
> > > > > > >
> > > > > > > Thanks for the patch.
> > > > > >
> > > > > > Sorry, that's cleary an oversight from my side.
> > > > > > Thanks for tackling it.
> > > > > >
> > > > > > > On Mon, Feb 08, 2021 at 07:53:15AM +0100, Mauro Carvalho Chehab wrote:
> > > > > > > > As described on its c file, the Maxim MAX9271 GMSL serializer isn't a
> > > > > > > > self-contained driver, as MAX9271 is usually embedded in camera modules
> > > > > > > > with at least one image sensor and optional additional components,
> > > > > > > > such as uController units or ISPs/DSPs.
> > > > > > > >
> > > > > > > > After chanseset a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module"),
> > > > > > > > there are now two drivers currently needing it: rdacm20 and rdacm21.
> > > > > > > >
> > > > > > > > Building with allmodconfig is now causing those warnings:
> > > > > > > >
> > > > > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_serial_link' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_configure_i2c' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_high_threshold' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_configure_gmsl_link' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_clear_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_enable_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_disable_gpios' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_verify_id' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_address' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_deserializer_address' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > > > > WARNING: modpost: drivers/media/i2c/rdacm21-camera_module: 'max9271_set_translation' exported twice. Previous export was in drivers/media/i2c/rdacm20-camera_module.ko
> > > > > > > >
> > > > > > > > Address the issue by adding a Kconfig item for it, that it is
> > > > > > > > seleced if either one of the modules that need max9271 is used.
> > > > > > > >
> > > > > > > > Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
> > > > > > > > Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
> > > > > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > > > > > > ---
> > > > > > > > drivers/media/i2c/Kconfig | 10 ++++++++++
> > > > > > > > drivers/media/i2c/Makefile | 8 ++++----
> > > > > > > > 2 files changed, 14 insertions(+), 4 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > > > > > > > index 2d3dc0d82f9e..a6802195d583 100644
> > > > > > > > --- a/drivers/media/i2c/Kconfig
> > > > > > > > +++ b/drivers/media/i2c/Kconfig
> > > > > > > > @@ -712,6 +712,16 @@ config VIDEO_ST_MIPID02
> > > > > > > > module will be called st-mipid02.
> > > > > > > > endmenu
> > > > > > > >
> > > > > > > > +#
> > > > > > > > +# Camera ancillary chips
> > > > > > > > +#
> > > > > > > > +
> > > > > > > > +# MAX9271 is usually embedded in camera modules
> > > > > > > > +config VIDEO_MAX9271_SERIALIZER
> > > > > > > > + tristate
> > > > > > > > + default y
> > > > > > > > + depends on VIDEO_RDACM20 || VIDEO_RDACM21
> > > > > > > > +
> > > > > > >
> > > > > > > I'd instead make the RDACM drivers depend on this one instead. The RDACM20
> > > > > > > driver directly depends on the symbols in the MAX9271 driver.
> > > >
> > > > While such solution is technically OK, it will make harder for the ones
> > > > wanting to use the RDACM drivers, because those two drivers will be
> > > > ridden at the config menu, until MAX9271 got selected.
> > > >
> > > > With the above solution, this driver will be auto-selected if either
> > > > RDACM20 or RDACM21 is needed.
> > > >
> > > > Btw, this is exactly the same strategy we use for tuner I2C modules:
> > > > the user doesn't need to know that his board uses tuner "foo" or "bar".
> > > > All it needs is to know that it will require, for instance, the em28xx
> > > > driver.
> > > >
> > > > > > >
> > > > > >
> > > > > > OTOH I it makes sense to have MAX9271 depend on the camera modules, as
> > > > > > selecting the serializer alone is not that useful.
> > > > > >
> > > > > > Could the two camera modules symbols instead select the MAX9271 one ?
> > > > >
> > > > > MAX9271 could be used elsewhere than in RDACM* devices.
> > > >
> > > > Sure, but then it is just a matter of adding the other driver there:
> > > >
> > > > depends on VIDEO_RDACM20 || VIDEO_RDACM21 || VIDEO_FOO
> > > >
> > > > To be frank, I doubt that we'll end having dozens of boards with this.
> > > > If we end having lots of drivers, we can work on a different
> > > > solution.
> > >
> > > Also note that there will be combinations of something compiled as a module
> > > and another driver linked directly to the kernel.
> > >
> > > So each of the RDACM drivers would likely also need:
> > >
> > > depends on VIDEO_MAX9271_SERIALIZER || !VIDEO_MAX9271_SERIALIZER
> >
> > No. I fail to see where this would work, as MAX9271 is a mandatory
> > dependency of those drivers. See:
> >
> > static int rdacm20_s_stream(struct v4l2_subdev *sd, int enable)
> > {
> > struct rdacm20_device *dev = sd_to_rdacm20(sd);
> >
> > return max9271_set_serial_link(dev->serializer, enable);
> > }
> >
> >
> > Without compiling max9271, the device won't stream ;-)
> >
> > -
> >
> > Yet, assuming that this could be an optional feature, if you use this:
> >
> > config VIDEO_MAX9271_SERIALIZER
> > tristate
> > default y
> > depends on VIDEO_RDACM20 || VIDEO_RDACM21
> >
> > You should *not* never do:
> >
> > depends on VIDEO_MAX9271_SERIALIZER || !VIDEO_MAX9271_SERIALIZER
> >
> > for VIDEO_RDACM20 or VIDEO_RDACM21, as it would cause a dependency
> > loop.
> >
> > -
> >
> > If you do, instead:
> >
> > if VIDEO_V4L2 && I2C
> > config VIDEO_MAX9271_SERIALIZER
> > tristate
> >
> > config VIDEO_RDACM20
> > select VIDEO_MAX9271_SERIALIZER
> > ...
> >
> > config VIDEO_RDACM21
> > select VIDEO_MAX9271_SERIALIZER
> > ...
> > endif
> >
> > Then you also won't need:
> > depends on VIDEO_MAX9271_SERIALIZER || !VIDEO_MAX9271_SERIALIZER
> >
> > As select should do the right thing in this case, ensuring that MAX9271
> > will be builtin either if RDACM20 or RDACM21 is builtin.
>
> I also vote for usage of "select".
>
I would prefer that too, I was concerned about possible un-met
dependencies, as Sakari pointed out, but the current situation is no
better, as the only Kconfig symbols where those can be listed are the
camera modules one.
> > -
> >
> > This should also work:
> >
> > config VIDEO_MAX9271_SERIALIZER
> > tristate
> > ...
> >
> > if VIDEO_MAX9271_SERIALIZER
> >
> > config VIDEO_RDACM20
> > tristate
> > ...
> >
> > config VIDEO_RDACM21
> > tristate
> > ...
> >
> > endif
> >
> > without needing a "depend on FOO || !FOO" logic, but, as I said before, this
> > case will force the user that want to build the RDACM20/RDACM21 drivers to
> > be aware that a MAX9271 driver is required.
> >
> > -
> >
> > You would only need a "depend on FOO || !FOO" if the driver FOO is
> > optional and if you try do something like:
> >
> > config FOO
> > tristate
> > ...
> >
> > config VIDEO_RDACM20
> > tristate
> > depends on FOO
> > ...
> >
> > config VIDEO_RDACM21
> > tristate
> > depends on FOO
> > ...
> >
> > as, on such case, it would allow the vars to be:
> >
> > FOO="m"
> > VIDEO_RDACM20="y"
> > VIDEO_RDACM21="m"
> >
> > which would cause build errors.
>
> --
> Regards,
>
> Laurent Pinchart
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] media: i2c: fix max9271 build dependencies
2021-02-08 11:41 ` Jacopo Mondi
@ 2021-02-08 13:11 ` Mauro Carvalho Chehab
2021-02-08 13:31 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 55+ messages in thread
From: Mauro Carvalho Chehab @ 2021-02-08 13:11 UTC (permalink / raw)
To: Jacopo Mondi
Cc: Laurent Pinchart, Sakari Ailus, Linux Media Mailing List,
Jacopo Mondi, Stephen Rothwell, linux-next
Em Mon, 8 Feb 2021 12:41:42 +0100
Jacopo Mondi <jacopo@jmondi.org> escreveu:
> > > If you do, instead:
> > >
> > > if VIDEO_V4L2 && I2C
> > > config VIDEO_MAX9271_SERIALIZER
> > > tristate
> > >
> > > config VIDEO_RDACM20
> > > select VIDEO_MAX9271_SERIALIZER
> > > ...
> > >
> > > config VIDEO_RDACM21
> > > select VIDEO_MAX9271_SERIALIZER
> > > ...
> > > endif
> > >
> > > Then you also won't need:
> > > depends on VIDEO_MAX9271_SERIALIZER || !VIDEO_MAX9271_SERIALIZER
> > >
> > > As select should do the right thing in this case, ensuring that MAX9271
> > > will be builtin either if RDACM20 or RDACM21 is builtin.
> >
> > I also vote for usage of "select".
> >
>
> I would prefer that too, I was concerned about possible un-met
> dependencies, as Sakari pointed out, but the current situation is no
> better, as the only Kconfig symbols where those can be listed are the
> camera modules one.
Works for me. I'll make a patch for it.
Thanks,
Mauro
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] media: i2c: fix max9271 build dependencies
2021-02-08 13:11 ` Mauro Carvalho Chehab
@ 2021-02-08 13:31 ` Mauro Carvalho Chehab
2021-02-08 13:40 ` Laurent Pinchart
2021-02-08 13:55 ` Jacopo Mondi
0 siblings, 2 replies; 55+ messages in thread
From: Mauro Carvalho Chehab @ 2021-02-08 13:31 UTC (permalink / raw)
To: Jacopo Mondi
Cc: Laurent Pinchart, Sakari Ailus, Linux Media Mailing List,
Jacopo Mondi, Stephen Rothwell, linux-next
Em Mon, 8 Feb 2021 14:11:02 +0100
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:
> Em Mon, 8 Feb 2021 12:41:42 +0100
> Jacopo Mondi <jacopo@jmondi.org> escreveu:
>
> > > > If you do, instead:
> > > >
> > > > if VIDEO_V4L2 && I2C
> > > > config VIDEO_MAX9271_SERIALIZER
> > > > tristate
> > > >
> > > > config VIDEO_RDACM20
> > > > select VIDEO_MAX9271_SERIALIZER
> > > > ...
> > > >
> > > > config VIDEO_RDACM21
> > > > select VIDEO_MAX9271_SERIALIZER
> > > > ...
> > > > endif
> > > >
> > > > Then you also won't need:
> > > > depends on VIDEO_MAX9271_SERIALIZER || !VIDEO_MAX9271_SERIALIZER
> > > >
> > > > As select should do the right thing in this case, ensuring that MAX9271
> > > > will be builtin either if RDACM20 or RDACM21 is builtin.
> > >
> > > I also vote for usage of "select".
> > >
> >
> > I would prefer that too, I was concerned about possible un-met
> > dependencies, as Sakari pointed out, but the current situation is no
> > better, as the only Kconfig symbols where those can be listed are the
> > camera modules one.
>
> Works for me. I'll make a patch for it.
Hmm... after taking a deeper look at the rcma20 drivers, and on its
Kconfig help text:
config VIDEO_RDACM20
tristate "IMI RDACM20 camera support"
select V4L2_FWNODE
select VIDEO_V4L2_SUBDEV_API
select MEDIA_CONTROLLER
help
This driver supports the IMI RDACM20 GMSL camera, used in
ADAS systems.
This camera should be used in conjunction with a GMSL
deserialiser such as the MAX9286.
I'm starting to suspect that there's something very wrong here...
The help text mentions the MAX9286 driver, which is a complete
driver, and not MAX9271, which seems to implement a set of PHY functions
needed by those drivers, and which lacks a proper I2C binding code on it.
The I2C binding code is, instead, inside RDACM20 and RDACM21:
static int rdacm21_initialize(struct rdacm21_device *dev)
{
int ret;
/* Verify communication with the MAX9271: ping to wakeup. */
dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
i2c_smbus_read_byte(dev->serializer.client);
usleep_range(3000, 5000);
/* Enable reverse channel and disable the serial link. */
ret = max9271_set_serial_link(&dev->serializer, false);
if (ret)
return ret;
/* Configure I2C bus at 105Kbps speed and configure GMSL. */
ret = max9271_configure_i2c(&dev->serializer,
MAX9271_I2CSLVSH_469NS_234NS |
MAX9271_I2CSLVTO_1024US |
MAX9271_I2CMSTBT_105KBPS);
/* Several other max9271-specific init code */
ret = ov490_initialize(dev);
if (ret)
return ret;
And, at max9271 "driver", there's just a bunch of exported functions.
This is all wrong.
I'm seriously considering to move all those 3 drivers to staging,
while they're not fixed to use a proper I2C binding mechanism.
Thanks,
Mauro
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] media: i2c: fix max9271 build dependencies
2021-02-08 13:31 ` Mauro Carvalho Chehab
@ 2021-02-08 13:40 ` Laurent Pinchart
2021-02-08 14:23 ` Mauro Carvalho Chehab
2021-02-08 13:55 ` Jacopo Mondi
1 sibling, 1 reply; 55+ messages in thread
From: Laurent Pinchart @ 2021-02-08 13:40 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Jacopo Mondi, Sakari Ailus, Linux Media Mailing List,
Jacopo Mondi, Stephen Rothwell, linux-next
Hi Mauro,
On Mon, Feb 08, 2021 at 02:31:50PM +0100, Mauro Carvalho Chehab wrote:
> Em Mon, 8 Feb 2021 14:11:02 +0100 Mauro Carvalho Chehab escreveu:
> > Em Mon, 8 Feb 2021 12:41:42 +0100 Jacopo Mondi escreveu:
> >
> > > > > If you do, instead:
> > > > >
> > > > > if VIDEO_V4L2 && I2C
> > > > > config VIDEO_MAX9271_SERIALIZER
> > > > > tristate
> > > > >
> > > > > config VIDEO_RDACM20
> > > > > select VIDEO_MAX9271_SERIALIZER
> > > > > ...
> > > > >
> > > > > config VIDEO_RDACM21
> > > > > select VIDEO_MAX9271_SERIALIZER
> > > > > ...
> > > > > endif
> > > > >
> > > > > Then you also won't need:
> > > > > depends on VIDEO_MAX9271_SERIALIZER || !VIDEO_MAX9271_SERIALIZER
> > > > >
> > > > > As select should do the right thing in this case, ensuring that MAX9271
> > > > > will be builtin either if RDACM20 or RDACM21 is builtin.
> > > >
> > > > I also vote for usage of "select".
> > > >
> > >
> > > I would prefer that too, I was concerned about possible un-met
> > > dependencies, as Sakari pointed out, but the current situation is no
> > > better, as the only Kconfig symbols where those can be listed are the
> > > camera modules one.
> >
> > Works for me. I'll make a patch for it.
>
> Hmm... after taking a deeper look at the rcma20 drivers, and on its
> Kconfig help text:
>
> config VIDEO_RDACM20
> tristate "IMI RDACM20 camera support"
> select V4L2_FWNODE
> select VIDEO_V4L2_SUBDEV_API
> select MEDIA_CONTROLLER
> help
> This driver supports the IMI RDACM20 GMSL camera, used in
> ADAS systems.
>
> This camera should be used in conjunction with a GMSL
> deserialiser such as the MAX9286.
>
> I'm starting to suspect that there's something very wrong here...
>
> The help text mentions the MAX9286 driver, which is a complete
> driver, and not MAX9271, which seems to implement a set of PHY functions
> needed by those drivers, and which lacks a proper I2C binding code on it.
>
> The I2C binding code is, instead, inside RDACM20 and RDACM21:
>
> static int rdacm21_initialize(struct rdacm21_device *dev)
> {
> int ret;
>
> /* Verify communication with the MAX9271: ping to wakeup. */
> dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
> i2c_smbus_read_byte(dev->serializer.client);
> usleep_range(3000, 5000);
>
> /* Enable reverse channel and disable the serial link. */
> ret = max9271_set_serial_link(&dev->serializer, false);
> if (ret)
> return ret;
>
> /* Configure I2C bus at 105Kbps speed and configure GMSL. */
> ret = max9271_configure_i2c(&dev->serializer,
> MAX9271_I2CSLVSH_469NS_234NS |
> MAX9271_I2CSLVTO_1024US |
> MAX9271_I2CMSTBT_105KBPS);
>
> /* Several other max9271-specific init code */
>
> ret = ov490_initialize(dev);
> if (ret)
> return ret;
>
> And, at max9271 "driver", there's just a bunch of exported functions.
>
> This is all wrong.
>
> I'm seriously considering to move all those 3 drivers to staging,
> while they're not fixed to use a proper I2C binding mechanism.
They can't. The RDACM20 and RDACM21 are GMSL cameras, that are
internally made of a GMSL serializer (MAX9271 in both cases) and a
camera sensor (OV10625 for the RDACM20, OV10640 + OV490 ISP for the
RDAMC21). The sensor and serializer are tightly couple, so much so that
in the RDACM20, there's a microcontroller that configures both when
power is applied. In the RDACM21, the OV490 firmware has a similar role.
Due to the tight coupling and the presense of a device-specific
microcontroller, the cameras need to be handled as a whole, we can't
have one driver for the sensor and one driver for the serializer that
would work in isolation and be controlled separately from userspace. The
MAX9271, however, still needs to be configured from the host, and we've
thus moved common code to a common file instead of duplicating it.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] media: i2c: fix max9271 build dependencies
2021-02-08 13:31 ` Mauro Carvalho Chehab
2021-02-08 13:40 ` Laurent Pinchart
@ 2021-02-08 13:55 ` Jacopo Mondi
2021-02-08 14:14 ` Mauro Carvalho Chehab
1 sibling, 1 reply; 55+ messages in thread
From: Jacopo Mondi @ 2021-02-08 13:55 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Laurent Pinchart, Sakari Ailus, Linux Media Mailing List,
Jacopo Mondi, Stephen Rothwell, linux-next
Hi Mauro,
On Mon, Feb 08, 2021 at 02:31:50PM +0100, Mauro Carvalho Chehab wrote:
> Em Mon, 8 Feb 2021 14:11:02 +0100
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:
>
> > Em Mon, 8 Feb 2021 12:41:42 +0100
> > Jacopo Mondi <jacopo@jmondi.org> escreveu:
> >
> > > > > If you do, instead:
> > > > >
> > > > > if VIDEO_V4L2 && I2C
> > > > > config VIDEO_MAX9271_SERIALIZER
> > > > > tristate
> > > > >
> > > > > config VIDEO_RDACM20
> > > > > select VIDEO_MAX9271_SERIALIZER
> > > > > ...
> > > > >
> > > > > config VIDEO_RDACM21
> > > > > select VIDEO_MAX9271_SERIALIZER
> > > > > ...
> > > > > endif
> > > > >
> > > > > Then you also won't need:
> > > > > depends on VIDEO_MAX9271_SERIALIZER || !VIDEO_MAX9271_SERIALIZER
> > > > >
> > > > > As select should do the right thing in this case, ensuring that MAX9271
> > > > > will be builtin either if RDACM20 or RDACM21 is builtin.
> > > >
> > > > I also vote for usage of "select".
> > > >
> > >
> > > I would prefer that too, I was concerned about possible un-met
> > > dependencies, as Sakari pointed out, but the current situation is no
> > > better, as the only Kconfig symbols where those can be listed are the
> > > camera modules one.
> >
> > Works for me. I'll make a patch for it.
>
> Hmm... after taking a deeper look at the rcma20 drivers, and on its
> Kconfig help text:
>
> config VIDEO_RDACM20
> tristate "IMI RDACM20 camera support"
> select V4L2_FWNODE
> select VIDEO_V4L2_SUBDEV_API
> select MEDIA_CONTROLLER
> help
> This driver supports the IMI RDACM20 GMSL camera, used in
> ADAS systems.
>
> This camera should be used in conjunction with a GMSL
> deserialiser such as the MAX9286.
>
> I'm starting to suspect that there's something very wrong here...
>
> The help text mentions the MAX9286 driver, which is a complete
> driver, and not MAX9271, which seems to implement a set of PHY functions
> needed by those drivers, and which lacks a proper I2C binding code on it.
What is it puzzling you here ? The fact max9286 is mentioned ?
Maybe it is not clear but the max9286 and max9271 are, respectively,
the deserializer and serializers chips that form a GMSL link.
Camera modules usually embed an image sensor (plus a variety of
ISP/uControllers for internal image processing) whose output is
directed to an embedded GMSL serializer (the max9271), which captures
the image output and serializes it on the GMSL link.
On the other side of the link a GMSLa deserializer (the max9286) is
required, to receive and interpret the GMSL signal and convert it back
to an image stream then transmitted though a MIPI CSI-2 interface to
the SoC.
Maybe the last statement is redundant and should not be placed in the
camera module Kconfig description, as system integrators are of course
aware that a deserializer is required on the other side of the link ?
>
> The I2C binding code is, instead, inside RDACM20 and RDACM21:
>
> static int rdacm21_initialize(struct rdacm21_device *dev)
> {
> int ret;
>
> /* Verify communication with the MAX9271: ping to wakeup. */
> dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
> i2c_smbus_read_byte(dev->serializer.client);
> usleep_range(3000, 5000);
>
> /* Enable reverse channel and disable the serial link. */
> ret = max9271_set_serial_link(&dev->serializer, false);
> if (ret)
> return ret;
>
> /* Configure I2C bus at 105Kbps speed and configure GMSL. */
> ret = max9271_configure_i2c(&dev->serializer,
> MAX9271_I2CSLVSH_469NS_234NS |
> MAX9271_I2CSLVTO_1024US |
> MAX9271_I2CMSTBT_105KBPS);
>
> /* Several other max9271-specific init code */
>
> ret = ov490_initialize(dev);
> if (ret)
> return ret;
>
> And, at max9271 "driver", there's just a bunch of exported functions.
>
max9271 is a library module that provides functions for other drivers to use.
The MAX9271 chip alone has no actual use, as it is usually embedded in
a camera module with an image sensor (and other chips).
The binding documents of the MAX9286 and RDACM2x chips provides an
overview of the system architecture.
The max9286 bounces all the I2C messages it receives and which are
not directed to its address to the other side of the GMSL link, and
the configurations bits you have pasted here performs the intial setup
of the I2C-over-GMSL interface of the serializer.
What is the 'i2c binding code' that is bothering you ?
> This is all wrong.
>
> I'm seriously considering to move all those 3 drivers to staging,
> while they're not fixed to use a proper I2C binding mechanism.
>
> Thanks,
> Mauro
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] media: i2c: fix max9271 build dependencies
2021-02-08 13:55 ` Jacopo Mondi
@ 2021-02-08 14:14 ` Mauro Carvalho Chehab
2021-02-08 14:49 ` Jacopo Mondi
0 siblings, 1 reply; 55+ messages in thread
From: Mauro Carvalho Chehab @ 2021-02-08 14:14 UTC (permalink / raw)
To: Jacopo Mondi
Cc: Laurent Pinchart, Sakari Ailus, Linux Media Mailing List,
Jacopo Mondi, Stephen Rothwell, linux-next
Em Mon, 8 Feb 2021 14:55:14 +0100
Jacopo Mondi <jacopo@jmondi.org> escreveu:
> Hi Mauro,
>
> On Mon, Feb 08, 2021 at 02:31:50PM +0100, Mauro Carvalho Chehab wrote:
> > Em Mon, 8 Feb 2021 14:11:02 +0100
> > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:
> >
> > > Em Mon, 8 Feb 2021 12:41:42 +0100
> > > Jacopo Mondi <jacopo@jmondi.org> escreveu:
> > >
> > > > > > If you do, instead:
> > > > > >
> > > > > > if VIDEO_V4L2 && I2C
> > > > > > config VIDEO_MAX9271_SERIALIZER
> > > > > > tristate
> > > > > >
> > > > > > config VIDEO_RDACM20
> > > > > > select VIDEO_MAX9271_SERIALIZER
> > > > > > ...
> > > > > >
> > > > > > config VIDEO_RDACM21
> > > > > > select VIDEO_MAX9271_SERIALIZER
> > > > > > ...
> > > > > > endif
> > > > > >
> > > > > > Then you also won't need:
> > > > > > depends on VIDEO_MAX9271_SERIALIZER || !VIDEO_MAX9271_SERIALIZER
> > > > > >
> > > > > > As select should do the right thing in this case, ensuring that MAX9271
> > > > > > will be builtin either if RDACM20 or RDACM21 is builtin.
> > > > >
> > > > > I also vote for usage of "select".
> > > > >
> > > >
> > > > I would prefer that too, I was concerned about possible un-met
> > > > dependencies, as Sakari pointed out, but the current situation is no
> > > > better, as the only Kconfig symbols where those can be listed are the
> > > > camera modules one.
> > >
> > > Works for me. I'll make a patch for it.
> >
> > Hmm... after taking a deeper look at the rcma20 drivers, and on its
> > Kconfig help text:
> >
> > config VIDEO_RDACM20
> > tristate "IMI RDACM20 camera support"
> > select V4L2_FWNODE
> > select VIDEO_V4L2_SUBDEV_API
> > select MEDIA_CONTROLLER
> > help
> > This driver supports the IMI RDACM20 GMSL camera, used in
> > ADAS systems.
> >
> > This camera should be used in conjunction with a GMSL
> > deserialiser such as the MAX9286.
> >
> > I'm starting to suspect that there's something very wrong here...
> >
> > The help text mentions the MAX9286 driver, which is a complete
> > driver, and not MAX9271, which seems to implement a set of PHY functions
> > needed by those drivers, and which lacks a proper I2C binding code on it.
>
> What is it puzzling you here ? The fact max9286 is mentioned ?
> Maybe it is not clear but the max9286 and max9271 are, respectively,
> the deserializer and serializers chips that form a GMSL link.
>
> Camera modules usually embed an image sensor (plus a variety of
> ISP/uControllers for internal image processing) whose output is
> directed to an embedded GMSL serializer (the max9271), which captures
> the image output and serializes it on the GMSL link.
>
> On the other side of the link a GMSLa deserializer (the max9286) is
> required, to receive and interpret the GMSL signal and convert it back
> to an image stream then transmitted though a MIPI CSI-2 interface to
> the SoC.
>
> Maybe the last statement is redundant and should not be placed in the
> camera module Kconfig description, as system integrators are of course
> aware that a deserializer is required on the other side of the link ?
>
> >
> > The I2C binding code is, instead, inside RDACM20 and RDACM21:
> >
> > static int rdacm21_initialize(struct rdacm21_device *dev)
> > {
> > int ret;
> >
> > /* Verify communication with the MAX9271: ping to wakeup. */
> > dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
> > i2c_smbus_read_byte(dev->serializer.client);
> > usleep_range(3000, 5000);
> >
> > /* Enable reverse channel and disable the serial link. */
> > ret = max9271_set_serial_link(&dev->serializer, false);
> > if (ret)
> > return ret;
> >
> > /* Configure I2C bus at 105Kbps speed and configure GMSL. */
> > ret = max9271_configure_i2c(&dev->serializer,
> > MAX9271_I2CSLVSH_469NS_234NS |
> > MAX9271_I2CSLVTO_1024US |
> > MAX9271_I2CMSTBT_105KBPS);
> >
> > /* Several other max9271-specific init code */
> >
> > ret = ov490_initialize(dev);
> > if (ret)
> > return ret;
> >
> > And, at max9271 "driver", there's just a bunch of exported functions.
> >
>
> max9271 is a library module that provides functions for other drivers to use.
> The MAX9271 chip alone has no actual use, as it is usually embedded in
> a camera module with an image sensor (and other chips).
I'm not discussing what the driver does. The point the max9271
should be turned into a real driver. I fail to see any reason why
it is code is currently turned into a bad hack, where all max9271
specific initialization is outside its driver (and duplicated on
two separate drivers).
Btw the max9286 driver does that:
static struct i2c_driver max9286_i2c_driver = {
.driver = {
.name = "max9286",
.of_match_table = of_match_ptr(max9286_dt_ids),
},
.probe_new = max9286_probe,
.remove = max9286_remove,
};
module_i2c_driver(max9286_i2c_driver);
In other words, it has its own .probe_new/.remove methods.
The max9271 has its probing method inside rdacm21_initialize()
and rdacm20_initialize().
You should, instead, move the max9271 probe/init code into
a max9271_probe function, and use module_i2c_driver().
Then, use i2c_new_client_device[1] at the camera drivers, checking if
the driver was properly loaded, returning an error if not.
[1] or one of the other alternative ways to probe/bind an i2c device,
like using the v4l2 helper function v4l2_i2c_new_subdev().
Thanks,
Mauro
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] media: i2c: fix max9271 build dependencies
2021-02-08 13:40 ` Laurent Pinchart
@ 2021-02-08 14:23 ` Mauro Carvalho Chehab
2021-02-08 14:31 ` Laurent Pinchart
0 siblings, 1 reply; 55+ messages in thread
From: Mauro Carvalho Chehab @ 2021-02-08 14:23 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Jacopo Mondi, Sakari Ailus, Linux Media Mailing List,
Jacopo Mondi, Stephen Rothwell, linux-next
Em Mon, 8 Feb 2021 15:40:10 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:
> Hi Mauro,
>
> On Mon, Feb 08, 2021 at 02:31:50PM +0100, Mauro Carvalho Chehab wrote:
> > Em Mon, 8 Feb 2021 14:11:02 +0100 Mauro Carvalho Chehab escreveu:
> > > Em Mon, 8 Feb 2021 12:41:42 +0100 Jacopo Mondi escreveu:
> > >
> > > > > > If you do, instead:
> > > > > >
> > > > > > if VIDEO_V4L2 && I2C
> > > > > > config VIDEO_MAX9271_SERIALIZER
> > > > > > tristate
> > > > > >
> > > > > > config VIDEO_RDACM20
> > > > > > select VIDEO_MAX9271_SERIALIZER
> > > > > > ...
> > > > > >
> > > > > > config VIDEO_RDACM21
> > > > > > select VIDEO_MAX9271_SERIALIZER
> > > > > > ...
> > > > > > endif
> > > > > >
> > > > > > Then you also won't need:
> > > > > > depends on VIDEO_MAX9271_SERIALIZER || !VIDEO_MAX9271_SERIALIZER
> > > > > >
> > > > > > As select should do the right thing in this case, ensuring that MAX9271
> > > > > > will be builtin either if RDACM20 or RDACM21 is builtin.
> > > > >
> > > > > I also vote for usage of "select".
> > > > >
> > > >
> > > > I would prefer that too, I was concerned about possible un-met
> > > > dependencies, as Sakari pointed out, but the current situation is no
> > > > better, as the only Kconfig symbols where those can be listed are the
> > > > camera modules one.
> > >
> > > Works for me. I'll make a patch for it.
> >
> > Hmm... after taking a deeper look at the rcma20 drivers, and on its
> > Kconfig help text:
> >
> > config VIDEO_RDACM20
> > tristate "IMI RDACM20 camera support"
> > select V4L2_FWNODE
> > select VIDEO_V4L2_SUBDEV_API
> > select MEDIA_CONTROLLER
> > help
> > This driver supports the IMI RDACM20 GMSL camera, used in
> > ADAS systems.
> >
> > This camera should be used in conjunction with a GMSL
> > deserialiser such as the MAX9286.
> >
> > I'm starting to suspect that there's something very wrong here...
> >
> > The help text mentions the MAX9286 driver, which is a complete
> > driver, and not MAX9271, which seems to implement a set of PHY functions
> > needed by those drivers, and which lacks a proper I2C binding code on it.
> >
> > The I2C binding code is, instead, inside RDACM20 and RDACM21:
> >
> > static int rdacm21_initialize(struct rdacm21_device *dev)
> > {
> > int ret;
> >
> > /* Verify communication with the MAX9271: ping to wakeup. */
> > dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
> > i2c_smbus_read_byte(dev->serializer.client);
> > usleep_range(3000, 5000);
> >
> > /* Enable reverse channel and disable the serial link. */
> > ret = max9271_set_serial_link(&dev->serializer, false);
> > if (ret)
> > return ret;
> >
> > /* Configure I2C bus at 105Kbps speed and configure GMSL. */
> > ret = max9271_configure_i2c(&dev->serializer,
> > MAX9271_I2CSLVSH_469NS_234NS |
> > MAX9271_I2CSLVTO_1024US |
> > MAX9271_I2CMSTBT_105KBPS);
> >
> > /* Several other max9271-specific init code */
> >
> > ret = ov490_initialize(dev);
> > if (ret)
> > return ret;
> >
> > And, at max9271 "driver", there's just a bunch of exported functions.
> >
> > This is all wrong.
> >
> > I'm seriously considering to move all those 3 drivers to staging,
> > while they're not fixed to use a proper I2C binding mechanism.
>
> They can't. The RDACM20 and RDACM21 are GMSL cameras, that are
> internally made of a GMSL serializer (MAX9271 in both cases) and a
> camera sensor (OV10625 for the RDACM20, OV10640 + OV490 ISP for the
> RDAMC21). The sensor and serializer are tightly couple, so much so that
> in the RDACM20, there's a microcontroller that configures both when
> power is applied. In the RDACM21, the OV490 firmware has a similar role.
> Due to the tight coupling and the presense of a device-specific
> microcontroller, the cameras need to be handled as a whole, we can't
> have one driver for the sensor and one driver for the serializer that
> would work in isolation and be controlled separately from userspace. The
> MAX9271, however, still needs to be configured from the host, and we've
> thus moved common code to a common file instead of duplicating it.
I'm not saying that max9271 should expose a media-controller (or any
other interface) to the userspace. It is perfectly fine to have the
RDACM20 and RDACM21 drivers fully controlling it. There are *lots of*
other media drivers that are implemented using multiple separate I2C
chips. Several of them have internally micro-controller(s) that may
control some I2C devices.
There are even some DVB designs where there is a microcontroller
(usually cypress) that it is connected to different I2C chips.
For instance, 23 of them use a Cypress microcontroller:
$ git grep -li cypress drivers/media/|grep .c$|wc -l
23
The problem here is that the max9271 probing code is at the wrong
place. It belongs to max9271 driver, and should not be outside
it.
Thanks,
Mauro
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] media: i2c: fix max9271 build dependencies
2021-02-08 14:23 ` Mauro Carvalho Chehab
@ 2021-02-08 14:31 ` Laurent Pinchart
2021-02-08 15:01 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 55+ messages in thread
From: Laurent Pinchart @ 2021-02-08 14:31 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Jacopo Mondi, Sakari Ailus, Linux Media Mailing List,
Jacopo Mondi, Stephen Rothwell, linux-next
Hi Mauro,
On Mon, Feb 08, 2021 at 03:23:59PM +0100, Mauro Carvalho Chehab wrote:
> Em Mon, 8 Feb 2021 15:40:10 +0200 Laurent Pinchart escreveu:
> > On Mon, Feb 08, 2021 at 02:31:50PM +0100, Mauro Carvalho Chehab wrote:
> > > Em Mon, 8 Feb 2021 14:11:02 +0100 Mauro Carvalho Chehab escreveu:
> > > > Em Mon, 8 Feb 2021 12:41:42 +0100 Jacopo Mondi escreveu:
> > > >
> > > > > > > If you do, instead:
> > > > > > >
> > > > > > > if VIDEO_V4L2 && I2C
> > > > > > > config VIDEO_MAX9271_SERIALIZER
> > > > > > > tristate
> > > > > > >
> > > > > > > config VIDEO_RDACM20
> > > > > > > select VIDEO_MAX9271_SERIALIZER
> > > > > > > ...
> > > > > > >
> > > > > > > config VIDEO_RDACM21
> > > > > > > select VIDEO_MAX9271_SERIALIZER
> > > > > > > ...
> > > > > > > endif
> > > > > > >
> > > > > > > Then you also won't need:
> > > > > > > depends on VIDEO_MAX9271_SERIALIZER || !VIDEO_MAX9271_SERIALIZER
> > > > > > >
> > > > > > > As select should do the right thing in this case, ensuring that MAX9271
> > > > > > > will be builtin either if RDACM20 or RDACM21 is builtin.
> > > > > >
> > > > > > I also vote for usage of "select".
> > > > > >
> > > > >
> > > > > I would prefer that too, I was concerned about possible un-met
> > > > > dependencies, as Sakari pointed out, but the current situation is no
> > > > > better, as the only Kconfig symbols where those can be listed are the
> > > > > camera modules one.
> > > >
> > > > Works for me. I'll make a patch for it.
> > >
> > > Hmm... after taking a deeper look at the rcma20 drivers, and on its
> > > Kconfig help text:
> > >
> > > config VIDEO_RDACM20
> > > tristate "IMI RDACM20 camera support"
> > > select V4L2_FWNODE
> > > select VIDEO_V4L2_SUBDEV_API
> > > select MEDIA_CONTROLLER
> > > help
> > > This driver supports the IMI RDACM20 GMSL camera, used in
> > > ADAS systems.
> > >
> > > This camera should be used in conjunction with a GMSL
> > > deserialiser such as the MAX9286.
> > >
> > > I'm starting to suspect that there's something very wrong here...
> > >
> > > The help text mentions the MAX9286 driver, which is a complete
> > > driver, and not MAX9271, which seems to implement a set of PHY functions
> > > needed by those drivers, and which lacks a proper I2C binding code on it.
> > >
> > > The I2C binding code is, instead, inside RDACM20 and RDACM21:
> > >
> > > static int rdacm21_initialize(struct rdacm21_device *dev)
> > > {
> > > int ret;
> > >
> > > /* Verify communication with the MAX9271: ping to wakeup. */
> > > dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
> > > i2c_smbus_read_byte(dev->serializer.client);
> > > usleep_range(3000, 5000);
> > >
> > > /* Enable reverse channel and disable the serial link. */
> > > ret = max9271_set_serial_link(&dev->serializer, false);
> > > if (ret)
> > > return ret;
> > >
> > > /* Configure I2C bus at 105Kbps speed and configure GMSL. */
> > > ret = max9271_configure_i2c(&dev->serializer,
> > > MAX9271_I2CSLVSH_469NS_234NS |
> > > MAX9271_I2CSLVTO_1024US |
> > > MAX9271_I2CMSTBT_105KBPS);
> > >
> > > /* Several other max9271-specific init code */
> > >
> > > ret = ov490_initialize(dev);
> > > if (ret)
> > > return ret;
> > >
> > > And, at max9271 "driver", there's just a bunch of exported functions.
> > >
> > > This is all wrong.
> > >
> > > I'm seriously considering to move all those 3 drivers to staging,
> > > while they're not fixed to use a proper I2C binding mechanism.
> >
> > They can't. The RDACM20 and RDACM21 are GMSL cameras, that are
> > internally made of a GMSL serializer (MAX9271 in both cases) and a
> > camera sensor (OV10625 for the RDACM20, OV10640 + OV490 ISP for the
> > RDAMC21). The sensor and serializer are tightly couple, so much so that
> > in the RDACM20, there's a microcontroller that configures both when
> > power is applied. In the RDACM21, the OV490 firmware has a similar role.
> > Due to the tight coupling and the presense of a device-specific
> > microcontroller, the cameras need to be handled as a whole, we can't
> > have one driver for the sensor and one driver for the serializer that
> > would work in isolation and be controlled separately from userspace. The
> > MAX9271, however, still needs to be configured from the host, and we've
> > thus moved common code to a common file instead of duplicating it.
>
> I'm not saying that max9271 should expose a media-controller (or any
> other interface) to the userspace. It is perfectly fine to have the
> RDACM20 and RDACM21 drivers fully controlling it. There are *lots of*
> other media drivers that are implemented using multiple separate I2C
> chips. Several of them have internally micro-controller(s) that may
> control some I2C devices.
>
> There are even some DVB designs where there is a microcontroller
> (usually cypress) that it is connected to different I2C chips.
>
> For instance, 23 of them use a Cypress microcontroller:
>
> $ git grep -li cypress drivers/media/|grep .c$|wc -l
> 23
>
> The problem here is that the max9271 probing code is at the wrong
> place. It belongs to max9271 driver, and should not be outside
> it.
Could you then submit a patch with a fix, as the right solution has
clearly escaped 4 engineers working on this issue for 2 years ? :-)
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] media: i2c: fix max9271 build dependencies
2021-02-08 14:14 ` Mauro Carvalho Chehab
@ 2021-02-08 14:49 ` Jacopo Mondi
2021-02-08 15:42 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 55+ messages in thread
From: Jacopo Mondi @ 2021-02-08 14:49 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Laurent Pinchart, Sakari Ailus, Linux Media Mailing List,
Jacopo Mondi, Stephen Rothwell, linux-next
Hi Mauro,
On Mon, Feb 08, 2021 at 03:14:46PM +0100, Mauro Carvalho Chehab wrote:
> Em Mon, 8 Feb 2021 14:55:14 +0100
> Jacopo Mondi <jacopo@jmondi.org> escreveu:
>
> > Hi Mauro,
> >
> > On Mon, Feb 08, 2021 at 02:31:50PM +0100, Mauro Carvalho Chehab wrote:
> > > Em Mon, 8 Feb 2021 14:11:02 +0100
> > > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:
> > >
> > > > Em Mon, 8 Feb 2021 12:41:42 +0100
> > > > Jacopo Mondi <jacopo@jmondi.org> escreveu:
> > > >
> > > > > > > If you do, instead:
> > > > > > >
> > > > > > > if VIDEO_V4L2 && I2C
> > > > > > > config VIDEO_MAX9271_SERIALIZER
> > > > > > > tristate
> > > > > > >
> > > > > > > config VIDEO_RDACM20
> > > > > > > select VIDEO_MAX9271_SERIALIZER
> > > > > > > ...
> > > > > > >
> > > > > > > config VIDEO_RDACM21
> > > > > > > select VIDEO_MAX9271_SERIALIZER
> > > > > > > ...
> > > > > > > endif
> > > > > > >
> > > > > > > Then you also won't need:
> > > > > > > depends on VIDEO_MAX9271_SERIALIZER || !VIDEO_MAX9271_SERIALIZER
> > > > > > >
> > > > > > > As select should do the right thing in this case, ensuring that MAX9271
> > > > > > > will be builtin either if RDACM20 or RDACM21 is builtin.
> > > > > >
> > > > > > I also vote for usage of "select".
> > > > > >
> > > > >
> > > > > I would prefer that too, I was concerned about possible un-met
> > > > > dependencies, as Sakari pointed out, but the current situation is no
> > > > > better, as the only Kconfig symbols where those can be listed are the
> > > > > camera modules one.
> > > >
> > > > Works for me. I'll make a patch for it.
> > >
> > > Hmm... after taking a deeper look at the rcma20 drivers, and on its
> > > Kconfig help text:
> > >
> > > config VIDEO_RDACM20
> > > tristate "IMI RDACM20 camera support"
> > > select V4L2_FWNODE
> > > select VIDEO_V4L2_SUBDEV_API
> > > select MEDIA_CONTROLLER
> > > help
> > > This driver supports the IMI RDACM20 GMSL camera, used in
> > > ADAS systems.
> > >
> > > This camera should be used in conjunction with a GMSL
> > > deserialiser such as the MAX9286.
> > >
> > > I'm starting to suspect that there's something very wrong here...
> > >
> > > The help text mentions the MAX9286 driver, which is a complete
> > > driver, and not MAX9271, which seems to implement a set of PHY functions
> > > needed by those drivers, and which lacks a proper I2C binding code on it.
> >
> > What is it puzzling you here ? The fact max9286 is mentioned ?
> > Maybe it is not clear but the max9286 and max9271 are, respectively,
> > the deserializer and serializers chips that form a GMSL link.
> >
> > Camera modules usually embed an image sensor (plus a variety of
> > ISP/uControllers for internal image processing) whose output is
> > directed to an embedded GMSL serializer (the max9271), which captures
> > the image output and serializes it on the GMSL link.
> >
> > On the other side of the link a GMSLa deserializer (the max9286) is
> > required, to receive and interpret the GMSL signal and convert it back
> > to an image stream then transmitted though a MIPI CSI-2 interface to
> > the SoC.
> >
> > Maybe the last statement is redundant and should not be placed in the
> > camera module Kconfig description, as system integrators are of course
> > aware that a deserializer is required on the other side of the link ?
> >
> > >
> > > The I2C binding code is, instead, inside RDACM20 and RDACM21:
> > >
> > > static int rdacm21_initialize(struct rdacm21_device *dev)
> > > {
> > > int ret;
> > >
> > > /* Verify communication with the MAX9271: ping to wakeup. */
> > > dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
> > > i2c_smbus_read_byte(dev->serializer.client);
> > > usleep_range(3000, 5000);
> > >
> > > /* Enable reverse channel and disable the serial link. */
> > > ret = max9271_set_serial_link(&dev->serializer, false);
> > > if (ret)
> > > return ret;
> > >
> > > /* Configure I2C bus at 105Kbps speed and configure GMSL. */
> > > ret = max9271_configure_i2c(&dev->serializer,
> > > MAX9271_I2CSLVSH_469NS_234NS |
> > > MAX9271_I2CSLVTO_1024US |
> > > MAX9271_I2CMSTBT_105KBPS);
> > >
> > > /* Several other max9271-specific init code */
> > >
> > > ret = ov490_initialize(dev);
> > > if (ret)
> > > return ret;
> > >
> > > And, at max9271 "driver", there's just a bunch of exported functions.
> > >
> >
> > max9271 is a library module that provides functions for other drivers to use.
> > The MAX9271 chip alone has no actual use, as it is usually embedded in
> > a camera module with an image sensor (and other chips).
>
> I'm not discussing what the driver does. The point the max9271
> should be turned into a real driver. I fail to see any reason why
> it is code is currently turned into a bad hack, where all max9271
> specific initialization is outside its driver (and duplicated on
> two separate drivers).
No, that's not true. The -camera module- initialization uses functions
exported by the max9271 module, to configure it depending on how the
camera module is assembled. iirc the GPIO handling for the 20 and the
21 are different in example. There might be modules that require a
different configuration of the serializer video input port and such
depending on how they're wired internally. We don't want to describe,
in example, the internal parallel video port between the serializer
and the ISP and the one between the ISP and the image sensor.
The camera module has a compatible string and that's what you want
specify in your DTS, and the camera module driver once probed
initializes the embedded serializer and the image sensor and the other
chips.
I understand the disconnection and the next question: what if we have
a standalone driver for the image sensor then ? Are you going to
duplicate the code inside the camera module driver ? The thing is that
these ADAS camera modules are designed to be more or less
plug-and-play objects, which are configured to stream images with a
fixed format and with internal circuitry that configures them at power
up. There's no need for a full sensor driver as well as the max9271
code alone serves no purposes.
>
> Btw the max9286 driver does that:
>
> static struct i2c_driver max9286_i2c_driver = {
> .driver = {
> .name = "max9286",
> .of_match_table = of_match_ptr(max9286_dt_ids),
> },
> .probe_new = max9286_probe,
> .remove = max9286_remove,
> };
>
> module_i2c_driver(max9286_i2c_driver);
>
> In other words, it has its own .probe_new/.remove methods.
The max9286 is a self-contained module. It has a GMSL and a CSI-2
interface. It's a bridge that connects the SoC to a camera module that
embeds a compatible serializer. It's dual its a 'GMSL camera module'
not a 'GMSL serializer' alone.
>
> The max9271 has its probing method inside rdacm21_initialize()
> and rdacm20_initialize().
>
> You should, instead, move the max9271 probe/init code into
> a max9271_probe function, and use module_i2c_driver().
I don't get what we would gain.
Conceptually to me this is like asking to separate handling of the
CSI-2 TX configuration from the imager part configuration in a sensor
driver.
The objection might be "yes but the sensor has a single i2c address".
For GMSL that's not true as we have multiple addresses 'on the other
side' of the GMSL link, but they're handled by address translation by
the serializer itself, it is conceptually more similar to a device that
register a range of i2c addresses than at three different devices.
>
> Then, use i2c_new_client_device[1] at the camera drivers, checking if
> the driver was properly loaded, returning an error if not.
>
That's how, through the instantiation of an i2c-mux on the
deserializer side we instantiate the camera module devices.
Getting to the serializer, then instantiate yet another i2c client for
the embedded ISP and embedded image sensor doesn't bring any benefit
as their configuration and run-time handling would be limited to a few
operations, you can see how thin are the camera module drivers on that
part.
Thanks
j
> [1] or one of the other alternative ways to probe/bind an i2c device,
> like using the v4l2 helper function v4l2_i2c_new_subdev().
>
> Thanks,
> Mauro
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] media: i2c: fix max9271 build dependencies
2021-02-08 14:31 ` Laurent Pinchart
@ 2021-02-08 15:01 ` Mauro Carvalho Chehab
0 siblings, 0 replies; 55+ messages in thread
From: Mauro Carvalho Chehab @ 2021-02-08 15:01 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Jacopo Mondi, Sakari Ailus, Linux Media Mailing List,
Jacopo Mondi, Stephen Rothwell, linux-next
Em Mon, 8 Feb 2021 16:31:16 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:
> Hi Mauro,
>
> On Mon, Feb 08, 2021 at 03:23:59PM +0100, Mauro Carvalho Chehab wrote:
> > Em Mon, 8 Feb 2021 15:40:10 +0200 Laurent Pinchart escreveu:
> > > On Mon, Feb 08, 2021 at 02:31:50PM +0100, Mauro Carvalho Chehab wrote:
> > > > Em Mon, 8 Feb 2021 14:11:02 +0100 Mauro Carvalho Chehab escreveu:
> > > > > Em Mon, 8 Feb 2021 12:41:42 +0100 Jacopo Mondi escreveu:
> > > > >
> > > > > > > > If you do, instead:
> > > > > > > >
> > > > > > > > if VIDEO_V4L2 && I2C
> > > > > > > > config VIDEO_MAX9271_SERIALIZER
> > > > > > > > tristate
> > > > > > > >
> > > > > > > > config VIDEO_RDACM20
> > > > > > > > select VIDEO_MAX9271_SERIALIZER
> > > > > > > > ...
> > > > > > > >
> > > > > > > > config VIDEO_RDACM21
> > > > > > > > select VIDEO_MAX9271_SERIALIZER
> > > > > > > > ...
> > > > > > > > endif
> > > > > > > >
> > > > > > > > Then you also won't need:
> > > > > > > > depends on VIDEO_MAX9271_SERIALIZER || !VIDEO_MAX9271_SERIALIZER
> > > > > > > >
> > > > > > > > As select should do the right thing in this case, ensuring that MAX9271
> > > > > > > > will be builtin either if RDACM20 or RDACM21 is builtin.
> > > > > > >
> > > > > > > I also vote for usage of "select".
> > > > > > >
> > > > > >
> > > > > > I would prefer that too, I was concerned about possible un-met
> > > > > > dependencies, as Sakari pointed out, but the current situation is no
> > > > > > better, as the only Kconfig symbols where those can be listed are the
> > > > > > camera modules one.
> > > > >
> > > > > Works for me. I'll make a patch for it.
> > > >
> > > > Hmm... after taking a deeper look at the rcma20 drivers, and on its
> > > > Kconfig help text:
> > > >
> > > > config VIDEO_RDACM20
> > > > tristate "IMI RDACM20 camera support"
> > > > select V4L2_FWNODE
> > > > select VIDEO_V4L2_SUBDEV_API
> > > > select MEDIA_CONTROLLER
> > > > help
> > > > This driver supports the IMI RDACM20 GMSL camera, used in
> > > > ADAS systems.
> > > >
> > > > This camera should be used in conjunction with a GMSL
> > > > deserialiser such as the MAX9286.
> > > >
> > > > I'm starting to suspect that there's something very wrong here...
> > > >
> > > > The help text mentions the MAX9286 driver, which is a complete
> > > > driver, and not MAX9271, which seems to implement a set of PHY functions
> > > > needed by those drivers, and which lacks a proper I2C binding code on it.
> > > >
> > > > The I2C binding code is, instead, inside RDACM20 and RDACM21:
> > > >
> > > > static int rdacm21_initialize(struct rdacm21_device *dev)
> > > > {
> > > > int ret;
> > > >
> > > > /* Verify communication with the MAX9271: ping to wakeup. */
> > > > dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
> > > > i2c_smbus_read_byte(dev->serializer.client);
> > > > usleep_range(3000, 5000);
> > > >
> > > > /* Enable reverse channel and disable the serial link. */
> > > > ret = max9271_set_serial_link(&dev->serializer, false);
> > > > if (ret)
> > > > return ret;
> > > >
> > > > /* Configure I2C bus at 105Kbps speed and configure GMSL. */
> > > > ret = max9271_configure_i2c(&dev->serializer,
> > > > MAX9271_I2CSLVSH_469NS_234NS |
> > > > MAX9271_I2CSLVTO_1024US |
> > > > MAX9271_I2CMSTBT_105KBPS);
> > > >
> > > > /* Several other max9271-specific init code */
> > > >
> > > > ret = ov490_initialize(dev);
> > > > if (ret)
> > > > return ret;
> > > >
> > > > And, at max9271 "driver", there's just a bunch of exported functions.
> > > >
> > > > This is all wrong.
> > > >
> > > > I'm seriously considering to move all those 3 drivers to staging,
> > > > while they're not fixed to use a proper I2C binding mechanism.
> > >
> > > They can't. The RDACM20 and RDACM21 are GMSL cameras, that are
> > > internally made of a GMSL serializer (MAX9271 in both cases) and a
> > > camera sensor (OV10625 for the RDACM20, OV10640 + OV490 ISP for the
> > > RDAMC21). The sensor and serializer are tightly couple, so much so that
> > > in the RDACM20, there's a microcontroller that configures both when
> > > power is applied. In the RDACM21, the OV490 firmware has a similar role.
> > > Due to the tight coupling and the presense of a device-specific
> > > microcontroller, the cameras need to be handled as a whole, we can't
> > > have one driver for the sensor and one driver for the serializer that
> > > would work in isolation and be controlled separately from userspace. The
> > > MAX9271, however, still needs to be configured from the host, and we've
> > > thus moved common code to a common file instead of duplicating it.
> >
> > I'm not saying that max9271 should expose a media-controller (or any
> > other interface) to the userspace. It is perfectly fine to have the
> > RDACM20 and RDACM21 drivers fully controlling it. There are *lots of*
> > other media drivers that are implemented using multiple separate I2C
> > chips. Several of them have internally micro-controller(s) that may
> > control some I2C devices.
> >
> > There are even some DVB designs where there is a microcontroller
> > (usually cypress) that it is connected to different I2C chips.
> >
> > For instance, 23 of them use a Cypress microcontroller:
> >
> > $ git grep -li cypress drivers/media/|grep .c$|wc -l
> > 23
> >
> > The problem here is that the max9271 probing code is at the wrong
> > place. It belongs to max9271 driver, and should not be outside
> > it.
>
> Could you then submit a patch with a fix, as the right solution has
> clearly escaped 4 engineers working on this issue for 2 years ? :-)
As far as I noticed, max9271 code is called only two times at the
driver:
1. at probe time:
static int rdacm21_initialize(struct rdacm21_device *dev)
{
int ret;
/* Verify communication with the MAX9271: ping to wakeup. */
dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
i2c_smbus_read_byte(dev->serializer.client);
usleep_range(3000, 5000);
...
}
static int rdacm21_probe(struct i2c_client *client)
...
ret = rdacm21_initialize(dev);
if (ret < 0)
goto error;
...
}
2. At stream on time:
static int rdacm21_s_stream(struct v4l2_subdev *sd, int enable)
{
struct rdacm21_device *dev = sd_to_rdacm21(sd);
return max9271_set_serial_link(&dev->serializer, enable);
}
As I said before, there are *lots* of other drivers doing about the same.
Just to get a random example inside the V4L drivers, the em28xx relies on
an external chip, controlled via I2C, that needs to be enabled before
the stream on (like tvp5051 or saa711x).
Inside its probing code, it does I2C binding in order to initialize the
I2C drivers.
It also wakes up the I2C devices during stream on, in order to wake up
tvp5051 (and other subdevs):
int em28xx_start_analog_streaming(struct vb2_queue *vq, unsigned int count)
{
...
/*
* Needed, since GPIO might have disabled power of
* some i2c device
*/
em28xx_wake_i2c(dev);
It is not an exact match to what rdacm21 does, but it seems close enough.
Thanks,
Mauro
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] media: i2c: fix max9271 build dependencies
2021-02-08 14:49 ` Jacopo Mondi
@ 2021-02-08 15:42 ` Mauro Carvalho Chehab
2021-02-08 16:26 ` Laurent Pinchart
0 siblings, 1 reply; 55+ messages in thread
From: Mauro Carvalho Chehab @ 2021-02-08 15:42 UTC (permalink / raw)
To: Jacopo Mondi
Cc: Laurent Pinchart, Sakari Ailus, Linux Media Mailing List,
Jacopo Mondi, Stephen Rothwell, linux-next
Em Mon, 8 Feb 2021 15:49:57 +0100
Jacopo Mondi <jacopo@jmondi.org> escreveu:
> Hi Mauro,
>
> On Mon, Feb 08, 2021 at 03:14:46PM +0100, Mauro Carvalho Chehab wrote:
> > Em Mon, 8 Feb 2021 14:55:14 +0100
> > Jacopo Mondi <jacopo@jmondi.org> escreveu:
> >
> > > Hi Mauro,
> > >
> > > On Mon, Feb 08, 2021 at 02:31:50PM +0100, Mauro Carvalho Chehab wrote:
> > > > Em Mon, 8 Feb 2021 14:11:02 +0100
> > > > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:
> > > >
> > > > > Em Mon, 8 Feb 2021 12:41:42 +0100
> > > > > Jacopo Mondi <jacopo@jmondi.org> escreveu:
> > > > >
> > > > > > > > If you do, instead:
> > > > > > > >
> > > > > > > > if VIDEO_V4L2 && I2C
> > > > > > > > config VIDEO_MAX9271_SERIALIZER
> > > > > > > > tristate
> > > > > > > >
> > > > > > > > config VIDEO_RDACM20
> > > > > > > > select VIDEO_MAX9271_SERIALIZER
> > > > > > > > ...
> > > > > > > >
> > > > > > > > config VIDEO_RDACM21
> > > > > > > > select VIDEO_MAX9271_SERIALIZER
> > > > > > > > ...
> > > > > > > > endif
> > > > > > > >
> > > > > > > > Then you also won't need:
> > > > > > > > depends on VIDEO_MAX9271_SERIALIZER || !VIDEO_MAX9271_SERIALIZER
> > > > > > > >
> > > > > > > > As select should do the right thing in this case, ensuring that MAX9271
> > > > > > > > will be builtin either if RDACM20 or RDACM21 is builtin.
> > > > > > >
> > > > > > > I also vote for usage of "select".
> > > > > > >
> > > > > >
> > > > > > I would prefer that too, I was concerned about possible un-met
> > > > > > dependencies, as Sakari pointed out, but the current situation is no
> > > > > > better, as the only Kconfig symbols where those can be listed are the
> > > > > > camera modules one.
> > > > >
> > > > > Works for me. I'll make a patch for it.
> > > >
> > > > Hmm... after taking a deeper look at the rcma20 drivers, and on its
> > > > Kconfig help text:
> > > >
> > > > config VIDEO_RDACM20
> > > > tristate "IMI RDACM20 camera support"
> > > > select V4L2_FWNODE
> > > > select VIDEO_V4L2_SUBDEV_API
> > > > select MEDIA_CONTROLLER
> > > > help
> > > > This driver supports the IMI RDACM20 GMSL camera, used in
> > > > ADAS systems.
> > > >
> > > > This camera should be used in conjunction with a GMSL
> > > > deserialiser such as the MAX9286.
> > > >
> > > > I'm starting to suspect that there's something very wrong here...
> > > >
> > > > The help text mentions the MAX9286 driver, which is a complete
> > > > driver, and not MAX9271, which seems to implement a set of PHY functions
> > > > needed by those drivers, and which lacks a proper I2C binding code on it.
> > >
> > > What is it puzzling you here ? The fact max9286 is mentioned ?
> > > Maybe it is not clear but the max9286 and max9271 are, respectively,
> > > the deserializer and serializers chips that form a GMSL link.
> > >
> > > Camera modules usually embed an image sensor (plus a variety of
> > > ISP/uControllers for internal image processing) whose output is
> > > directed to an embedded GMSL serializer (the max9271), which captures
> > > the image output and serializes it on the GMSL link.
> > >
> > > On the other side of the link a GMSLa deserializer (the max9286) is
> > > required, to receive and interpret the GMSL signal and convert it back
> > > to an image stream then transmitted though a MIPI CSI-2 interface to
> > > the SoC.
> > >
> > > Maybe the last statement is redundant and should not be placed in the
> > > camera module Kconfig description, as system integrators are of course
> > > aware that a deserializer is required on the other side of the link ?
> > >
> > > >
> > > > The I2C binding code is, instead, inside RDACM20 and RDACM21:
> > > >
> > > > static int rdacm21_initialize(struct rdacm21_device *dev)
> > > > {
> > > > int ret;
> > > >
> > > > /* Verify communication with the MAX9271: ping to wakeup. */
> > > > dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
> > > > i2c_smbus_read_byte(dev->serializer.client);
> > > > usleep_range(3000, 5000);
> > > >
> > > > /* Enable reverse channel and disable the serial link. */
> > > > ret = max9271_set_serial_link(&dev->serializer, false);
> > > > if (ret)
> > > > return ret;
> > > >
> > > > /* Configure I2C bus at 105Kbps speed and configure GMSL. */
> > > > ret = max9271_configure_i2c(&dev->serializer,
> > > > MAX9271_I2CSLVSH_469NS_234NS |
> > > > MAX9271_I2CSLVTO_1024US |
> > > > MAX9271_I2CMSTBT_105KBPS);
> > > >
> > > > /* Several other max9271-specific init code */
> > > >
> > > > ret = ov490_initialize(dev);
> > > > if (ret)
> > > > return ret;
> > > >
> > > > And, at max9271 "driver", there's just a bunch of exported functions.
> > > >
> > >
> > > max9271 is a library module that provides functions for other drivers to use.
> > > The MAX9271 chip alone has no actual use, as it is usually embedded in
> > > a camera module with an image sensor (and other chips).
> >
> > I'm not discussing what the driver does. The point the max9271
> > should be turned into a real driver. I fail to see any reason why
> > it is code is currently turned into a bad hack, where all max9271
> > specific initialization is outside its driver (and duplicated on
> > two separate drivers).
>
> No, that's not true. The -camera module- initialization uses functions
> exported by the max9271 module, to configure it depending on how the
> camera module is assembled. iirc the GPIO handling for the 20 and the
> 21 are different in example. There might be modules that require a
> different configuration of the serializer video input port and such
> depending on how they're wired internally. We don't want to describe,
> in example, the internal parallel video port between the serializer
> and the ISP and the one between the ISP and the image sensor.
No problem with that. Just pass whatever init is needed via platform
data.
> The camera module has a compatible string and that's what you want
> specify in your DTS, and the camera module driver once probed
> initializes the embedded serializer and the image sensor and the other
> chips.
Again, no problem. It can be easily solved. E either:
1) have a compatible string for max9271, and let the normal OF code
to load it. That could require the usage of the V4L2 async API,
in order to wait for the module to be probed at the rdacm20/21 code;
2) use v4l2_i2c_new_subdev(), which will call request_module(), in
order to load it.
> I understand the disconnection and the next question: what if we have
> a standalone driver for the image sensor then ? Are you going to
> duplicate the code inside the camera module driver ? The thing is that
> these ADAS camera modules are designed to be more or less
> plug-and-play objects, which are configured to stream images with a
> fixed format and with internal circuitry that configures them at power
> up. There's no need for a full sensor driver as well as the max9271
> code alone serves no purposes.
A "full" driver will basically have the code you already wrote.
All it would need extra would be a static struct i2c_driver.
The rest would be to basically move the code to their rightful place,
and to provide a struct to be filled with the things that will
be different between different drivers (e. g. the platform_data).
See, for instance:
include/media/i2c/ir-kbd-i2c.h
(driver is at drivers/media/i2c/ir-kbd-i2c.c)
This is for a wide range of different I2C-based micro-controller commonly
found on some designs. Each vendor had their own micro-controller, with
different implementations, but they all do the same. So, a single driver
works among very different phy implementations.
The struct data there is particularly tricky, as it even includes a
callback function (used only on a handful devices that are unusually
weird). You probably won't need anything so complex ;-)
Another less-tricky device device (but with a bigger platform data
struct, as it supports an entire family of chips, from saa7110 to
saa7118, plus some clones) is:
include/media/i2c/saa7115.h
(driver is at drivers/media/i2c/saa7115.c)
> >
> > Btw the max9286 driver does that:
> >
> > static struct i2c_driver max9286_i2c_driver = {
> > .driver = {
> > .name = "max9286",
> > .of_match_table = of_match_ptr(max9286_dt_ids),
> > },
> > .probe_new = max9286_probe,
> > .remove = max9286_remove,
> > };
> >
> > module_i2c_driver(max9286_i2c_driver);
> >
> > In other words, it has its own .probe_new/.remove methods.
>
> The max9286 is a self-contained module. It has a GMSL and a CSI-2
> interface. It's a bridge that connects the SoC to a camera module that
> embeds a compatible serializer. It's dual its a 'GMSL camera module'
> not a 'GMSL serializer' alone.
Sorry but I miss the point here. It doesn't matter if it has a CSI-2
interface or not. It is a device that can be controlled via I2C.
It should use the proper Linux Kernel way to be probed.
>
> >
> > The max9271 has its probing method inside rdacm21_initialize()
> > and rdacm20_initialize().
> >
> > You should, instead, move the max9271 probe/init code into
> > a max9271_probe function, and use module_i2c_driver().
>
> I don't get what we would gain.
>
> Conceptually to me this is like asking to separate handling of the
> CSI-2 TX configuration from the imager part configuration in a sensor
> driver.
>
> The objection might be "yes but the sensor has a single i2c address".
>
> For GMSL that's not true as we have multiple addresses 'on the other
> side' of the GMSL link, but they're handled by address translation by
> the serializer itself, it is conceptually more similar to a device that
> register a range of i2c addresses than at three different devices.
>
> >
> > Then, use i2c_new_client_device[1] at the camera drivers, checking if
> > the driver was properly loaded, returning an error if not.
> >
>
> That's how, through the instantiation of an i2c-mux on the
> deserializer side we instantiate the camera module devices.
>
> Getting to the serializer, then instantiate yet another i2c client for
> the embedded ISP and embedded image sensor doesn't bring any benefit
> as their configuration and run-time handling would be limited to a few
> operations, you can see how thin are the camera module drivers on that
> part.
We actually have things very similar to a GMSL serializer at the Linux
Kernel, well supported as separate Kernel drivers. The only difference
is that, instead of being GMSL, they're I2S and are used for audio
channels and not for the video ones. I2S is also a bus (although the
devices we support on media are single-connected).
But yeah, in the future, perhaps we need something similar to i2c-mux
in order to better support it.
For now, I'm happy if max9271 gets converted into a real driver,
using the already-existing I2C binding logic.
Thanks,
Mauro
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH] media: i2c: fix max9271 build dependencies
2021-02-08 15:42 ` Mauro Carvalho Chehab
@ 2021-02-08 16:26 ` Laurent Pinchart
0 siblings, 0 replies; 55+ messages in thread
From: Laurent Pinchart @ 2021-02-08 16:26 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Jacopo Mondi, Sakari Ailus, Linux Media Mailing List,
Jacopo Mondi, Stephen Rothwell, linux-next
Hi Mauro,
On Mon, Feb 08, 2021 at 04:42:53PM +0100, Mauro Carvalho Chehab wrote:
> Em Mon, 8 Feb 2021 15:49:57 +0100 Jacopo Mondi escreveu:
> > On Mon, Feb 08, 2021 at 03:14:46PM +0100, Mauro Carvalho Chehab wrote:
> > > Em Mon, 8 Feb 2021 14:55:14 +0100 Jacopo Mondi escreveu:
> > > > On Mon, Feb 08, 2021 at 02:31:50PM +0100, Mauro Carvalho Chehab wrote:
> > > > > Em Mon, 8 Feb 2021 14:11:02 +0100 Mauro Carvalho Chehab escreveu:
> > > > > > Em Mon, 8 Feb 2021 12:41:42 +0100 Jacopo Mondi escreveu:
> > > > > >
> > > > > > > > > If you do, instead:
> > > > > > > > >
> > > > > > > > > if VIDEO_V4L2 && I2C
> > > > > > > > > config VIDEO_MAX9271_SERIALIZER
> > > > > > > > > tristate
> > > > > > > > >
> > > > > > > > > config VIDEO_RDACM20
> > > > > > > > > select VIDEO_MAX9271_SERIALIZER
> > > > > > > > > ...
> > > > > > > > >
> > > > > > > > > config VIDEO_RDACM21
> > > > > > > > > select VIDEO_MAX9271_SERIALIZER
> > > > > > > > > ...
> > > > > > > > > endif
> > > > > > > > >
> > > > > > > > > Then you also won't need:
> > > > > > > > > depends on VIDEO_MAX9271_SERIALIZER || !VIDEO_MAX9271_SERIALIZER
> > > > > > > > >
> > > > > > > > > As select should do the right thing in this case, ensuring that MAX9271
> > > > > > > > > will be builtin either if RDACM20 or RDACM21 is builtin.
> > > > > > > >
> > > > > > > > I also vote for usage of "select".
> > > > > > > >
> > > > > > >
> > > > > > > I would prefer that too, I was concerned about possible un-met
> > > > > > > dependencies, as Sakari pointed out, but the current situation is no
> > > > > > > better, as the only Kconfig symbols where those can be listed are the
> > > > > > > camera modules one.
> > > > > >
> > > > > > Works for me. I'll make a patch for it.
> > > > >
> > > > > Hmm... after taking a deeper look at the rcma20 drivers, and on its
> > > > > Kconfig help text:
> > > > >
> > > > > config VIDEO_RDACM20
> > > > > tristate "IMI RDACM20 camera support"
> > > > > select V4L2_FWNODE
> > > > > select VIDEO_V4L2_SUBDEV_API
> > > > > select MEDIA_CONTROLLER
> > > > > help
> > > > > This driver supports the IMI RDACM20 GMSL camera, used in
> > > > > ADAS systems.
> > > > >
> > > > > This camera should be used in conjunction with a GMSL
> > > > > deserialiser such as the MAX9286.
> > > > >
> > > > > I'm starting to suspect that there's something very wrong here...
> > > > >
> > > > > The help text mentions the MAX9286 driver, which is a complete
> > > > > driver, and not MAX9271, which seems to implement a set of PHY functions
> > > > > needed by those drivers, and which lacks a proper I2C binding code on it.
> > > >
> > > > What is it puzzling you here ? The fact max9286 is mentioned ?
> > > > Maybe it is not clear but the max9286 and max9271 are, respectively,
> > > > the deserializer and serializers chips that form a GMSL link.
> > > >
> > > > Camera modules usually embed an image sensor (plus a variety of
> > > > ISP/uControllers for internal image processing) whose output is
> > > > directed to an embedded GMSL serializer (the max9271), which captures
> > > > the image output and serializes it on the GMSL link.
> > > >
> > > > On the other side of the link a GMSLa deserializer (the max9286) is
> > > > required, to receive and interpret the GMSL signal and convert it back
> > > > to an image stream then transmitted though a MIPI CSI-2 interface to
> > > > the SoC.
> > > >
> > > > Maybe the last statement is redundant and should not be placed in the
> > > > camera module Kconfig description, as system integrators are of course
> > > > aware that a deserializer is required on the other side of the link ?
> > > >
> > > > >
> > > > > The I2C binding code is, instead, inside RDACM20 and RDACM21:
> > > > >
> > > > > static int rdacm21_initialize(struct rdacm21_device *dev)
> > > > > {
> > > > > int ret;
> > > > >
> > > > > /* Verify communication with the MAX9271: ping to wakeup. */
> > > > > dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
> > > > > i2c_smbus_read_byte(dev->serializer.client);
> > > > > usleep_range(3000, 5000);
> > > > >
> > > > > /* Enable reverse channel and disable the serial link. */
> > > > > ret = max9271_set_serial_link(&dev->serializer, false);
> > > > > if (ret)
> > > > > return ret;
> > > > >
> > > > > /* Configure I2C bus at 105Kbps speed and configure GMSL. */
> > > > > ret = max9271_configure_i2c(&dev->serializer,
> > > > > MAX9271_I2CSLVSH_469NS_234NS |
> > > > > MAX9271_I2CSLVTO_1024US |
> > > > > MAX9271_I2CMSTBT_105KBPS);
> > > > >
> > > > > /* Several other max9271-specific init code */
> > > > >
> > > > > ret = ov490_initialize(dev);
> > > > > if (ret)
> > > > > return ret;
> > > > >
> > > > > And, at max9271 "driver", there's just a bunch of exported functions.
> > > > >
> > > >
> > > > max9271 is a library module that provides functions for other drivers to use.
> > > > The MAX9271 chip alone has no actual use, as it is usually embedded in
> > > > a camera module with an image sensor (and other chips).
> > >
> > > I'm not discussing what the driver does. The point the max9271
> > > should be turned into a real driver. I fail to see any reason why
> > > it is code is currently turned into a bad hack, where all max9271
> > > specific initialization is outside its driver (and duplicated on
> > > two separate drivers).
> >
> > No, that's not true. The -camera module- initialization uses functions
> > exported by the max9271 module, to configure it depending on how the
> > camera module is assembled. iirc the GPIO handling for the 20 and the
> > 21 are different in example. There might be modules that require a
> > different configuration of the serializer video input port and such
> > depending on how they're wired internally. We don't want to describe,
> > in example, the internal parallel video port between the serializer
> > and the ISP and the one between the ISP and the image sensor.
>
> No problem with that. Just pass whatever init is needed via platform
> data.
No, please, no platform data. That should be avoided as much as possible
on DT-based systems.
> > The camera module has a compatible string and that's what you want
> > specify in your DTS, and the camera module driver once probed
> > initializes the embedded serializer and the image sensor and the other
> > chips.
>
> Again, no problem. It can be easily solved. E either:
>
> 1) have a compatible string for max9271, and let the normal OF code
> to load it. That could require the usage of the V4L2 async API,
> in order to wait for the module to be probed at the rdacm20/21 code;
> 2) use v4l2_i2c_new_subdev(), which will call request_module(), in
> order to load it.
But it's completely pointless, it won't bring any benefit compared to
the current implementation. I'm sure we can make the code needlessly
complicated, but there's no good reason to do so.
> > I understand the disconnection and the next question: what if we have
> > a standalone driver for the image sensor then ? Are you going to
> > duplicate the code inside the camera module driver ? The thing is that
> > these ADAS camera modules are designed to be more or less
> > plug-and-play objects, which are configured to stream images with a
> > fixed format and with internal circuitry that configures them at power
> > up. There's no need for a full sensor driver as well as the max9271
> > code alone serves no purposes.
>
> A "full" driver will basically have the code you already wrote.
>
> All it would need extra would be a static struct i2c_driver.
> The rest would be to basically move the code to their rightful place,
> and to provide a struct to be filled with the things that will
> be different between different drivers (e. g. the platform_data).
>
> See, for instance:
>
> include/media/i2c/ir-kbd-i2c.h
> (driver is at drivers/media/i2c/ir-kbd-i2c.c)
>
> This is for a wide range of different I2C-based micro-controller commonly
> found on some designs. Each vendor had their own micro-controller, with
> different implementations, but they all do the same. So, a single driver
> works among very different phy implementations.
>
> The struct data there is particularly tricky, as it even includes a
> callback function (used only on a handful devices that are unusually
> weird). You probably won't need anything so complex ;-)
>
> Another less-tricky device device (but with a bigger platform data
> struct, as it supports an entire family of chips, from saa7110 to
> saa7118, plus some clones) is:
>
> include/media/i2c/saa7115.h
> (driver is at drivers/media/i2c/saa7115.c)
>
> > >
> > > Btw the max9286 driver does that:
> > >
> > > static struct i2c_driver max9286_i2c_driver = {
> > > .driver = {
> > > .name = "max9286",
> > > .of_match_table = of_match_ptr(max9286_dt_ids),
> > > },
> > > .probe_new = max9286_probe,
> > > .remove = max9286_remove,
> > > };
> > >
> > > module_i2c_driver(max9286_i2c_driver);
> > >
> > > In other words, it has its own .probe_new/.remove methods.
> >
> > The max9286 is a self-contained module. It has a GMSL and a CSI-2
> > interface. It's a bridge that connects the SoC to a camera module that
> > embeds a compatible serializer. It's dual its a 'GMSL camera module'
> > not a 'GMSL serializer' alone.
>
> Sorry but I miss the point here. It doesn't matter if it has a CSI-2
> interface or not. It is a device that can be controlled via I2C.
> It should use the proper Linux Kernel way to be probed.
We're not probing a MAX9271. We're probing an RDACM21.
> > > The max9271 has its probing method inside rdacm21_initialize()
> > > and rdacm20_initialize().
> > >
> > > You should, instead, move the max9271 probe/init code into
> > > a max9271_probe function, and use module_i2c_driver().
> >
> > I don't get what we would gain.
> >
> > Conceptually to me this is like asking to separate handling of the
> > CSI-2 TX configuration from the imager part configuration in a sensor
> > driver.
> >
> > The objection might be "yes but the sensor has a single i2c address".
> >
> > For GMSL that's not true as we have multiple addresses 'on the other
> > side' of the GMSL link, but they're handled by address translation by
> > the serializer itself, it is conceptually more similar to a device that
> > register a range of i2c addresses than at three different devices.
> >
> > > Then, use i2c_new_client_device[1] at the camera drivers, checking if
> > > the driver was properly loaded, returning an error if not.
> >
> > That's how, through the instantiation of an i2c-mux on the
> > deserializer side we instantiate the camera module devices.
> >
> > Getting to the serializer, then instantiate yet another i2c client for
> > the embedded ISP and embedded image sensor doesn't bring any benefit
> > as their configuration and run-time handling would be limited to a few
> > operations, you can see how thin are the camera module drivers on that
> > part.
>
> We actually have things very similar to a GMSL serializer at the Linux
> Kernel, well supported as separate Kernel drivers. The only difference
> is that, instead of being GMSL, they're I2S and are used for audio
> channels and not for the video ones. I2S is also a bus (although the
> devices we support on media are single-connected).
>
> But yeah, in the future, perhaps we need something similar to i2c-mux
> in order to better support it.
>
> For now, I'm happy if max9271 gets converted into a real driver,
> using the already-existing I2C binding logic.
I'm not. The max9271 code should be turned into a module, but not a
driver. That's just not needed.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: linux-next: build warning after merge of the v4l-dvb tree
2021-05-10 23:46 linux-next: build warning after merge of the v4l-dvb tree Stephen Rothwell
@ 2021-05-21 0:48 ` Stephen Rothwell
0 siblings, 0 replies; 55+ messages in thread
From: Stephen Rothwell @ 2021-05-21 0:48 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: Linux Kernel Mailing List, Linux Next Mailing List
[-- Attachment #1: Type: text/plain, Size: 622 bytes --]
Hi all,
On Tue, 11 May 2021 09:46:49 +1000 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> After merging the v4l-dvb tree, today's linux-next build (arm
> multi_v7_defconfig) produced this warning:
>
> drivers/media/platform/exynos4-is/media-dev.c: In function 'cam_clk_prepare':
> drivers/media/platform/exynos4-is/media-dev.c:1287:6: warning: unused variable 'ret' [-Wunused-variable]
> 1287 | int ret;
> | ^~~
>
> Introduced by commit
>
> 59f96244af94 ("media: exynos4-is: fix pm_runtime_get_sync() usage count")
I am still seeing this warning.
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 55+ messages in thread
* linux-next: build warning after merge of the v4l-dvb tree
@ 2021-05-10 23:46 Stephen Rothwell
2021-05-21 0:48 ` Stephen Rothwell
0 siblings, 1 reply; 55+ messages in thread
From: Stephen Rothwell @ 2021-05-10 23:46 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: Linux Kernel Mailing List, Linux Next Mailing List
[-- Attachment #1: Type: text/plain, Size: 479 bytes --]
Hi all,
After merging the v4l-dvb tree, today's linux-next build (arm
multi_v7_defconfig) produced this warning:
drivers/media/platform/exynos4-is/media-dev.c: In function 'cam_clk_prepare':
drivers/media/platform/exynos4-is/media-dev.c:1287:6: warning: unused variable 'ret' [-Wunused-variable]
1287 | int ret;
| ^~~
Introduced by commit
59f96244af94 ("media: exynos4-is: fix pm_runtime_get_sync() usage count")
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 55+ messages in thread
* linux-next: build warning after merge of the v4l-dvb tree
@ 2021-03-23 5:56 Stephen Rothwell
0 siblings, 0 replies; 55+ messages in thread
From: Stephen Rothwell @ 2021-03-23 5:56 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: Linux Kernel Mailing List, Linux Next Mailing List
[-- Attachment #1: Type: text/plain, Size: 450 bytes --]
Hi all,
After merging the v4l-dvb tree, today's linux-next build (htmldocs)
produced this warning:
Documentation/driver-api/media/camera-sensor.rst:147: WARNING: Error in "c:function" directive:
1 argument(s) required, 0 supplied.
.. c:function::
int pm_runtime_get_if_in_use(struct device *dev);
Introduced by commit
c0e3bcb25390 ("media: camera-sensor.rst: fix a doc build warning")
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: linux-next: build warning after merge of the v4l-dvb tree
2021-02-15 10:20 ` Mauro Carvalho Chehab
@ 2021-02-15 10:42 ` Mauro Carvalho Chehab
0 siblings, 0 replies; 55+ messages in thread
From: Mauro Carvalho Chehab @ 2021-02-15 10:42 UTC (permalink / raw)
To: Ezequiel Garcia
Cc: Sakari Ailus, Stephen Rothwell, Linux Kernel Mailing List,
Linux Next Mailing List
Em Mon, 15 Feb 2021 11:20:24 +0100
Mauro Carvalho Chehab <mchehab@kernel.org> escreveu:
> Em Mon, 08 Feb 2021 15:53:00 -0300
> Ezequiel Garcia <ezequiel@collabora.com> escreveu:
>
> > On Mon, 2021-02-08 at 18:40 +0100, Mauro Carvalho Chehab wrote:
> > > Em Mon, 08 Feb 2021 13:57:56 -0300
> > > Ezequiel Garcia <ezequiel@collabora.com> escreveu:
> > >
> > > > On Mon, 2021-02-08 at 18:46 +0200, Sakari Ailus wrote:
> > > > > Hi Ezequiel,
> > > > >
> > > > > Thanks for addressing this.
> > > > >
> > > > > On Mon, Feb 08, 2021 at 01:42:21PM -0300, Ezequiel Garcia wrote:
> > > > > > Hi Stephen,
> > > > > >
> > > > > > On Mon, 2021-02-08 at 23:37 +1100, Stephen Rothwell wrote:
> > > > > > > Hi all,
> > > > > > >
> > > > > > > After merging the v4l-dvb tree, today's linux-next build (htmldocs)
> > > > > > > produced this warning:
> > > > > > >
> > > > > > > include/media/v4l2-async.h:178: warning: expecting prototype for v4l2_async_notifier_add_fwnode_subdev(). Prototype was for
> > > > > > > __v4l2_async_notifier_add_fwnode_subdev() instead
> > > > > > > include/media/v4l2-async.h:207: warning: expecting prototype for v4l2_async_notifier_add_fwnode_remote_subdev(). Prototype was for
> > > > > > > __v4l2_async_notifier_add_fwnode_remote_subdev() instead
> > > > > > > include/media/v4l2-async.h:230: warning: expecting prototype for v4l2_async_notifier_add_i2c_subdev(). Prototype was for
> > > > > > > __v4l2_async_notifier_add_i2c_subdev() instead
> > > > > > >
> > > > > > > Maybe introduced by commit
> > > > > > >
> > > > > > > c1cc23625062 ("media: v4l2-async: Discourage use of v4l2_async_notifier_add_subdev")
> > > > > > >
> > > > > >
> > > > > > Thanks for spotting this. Should be fixed by:
> > > > > >
> > > > > > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> > > > > > index 6f22daa6f067..3785445282fc 100644
> > > > > > --- a/include/media/v4l2-async.h
> > > > > > +++ b/include/media/v4l2-async.h
> > > > > > @@ -157,7 +157,7 @@ int __v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier,
> > > > > > struct v4l2_async_subdev *asd);
> > > > > >
> > > > > > /**
> > > > > > - * v4l2_async_notifier_add_fwnode_subdev - Allocate and add a fwnode async
> > > > > > + * __v4l2_async_notifier_add_fwnode_subdev - Allocate and add a fwnode async
> > > > >
> > > > > The problem with the approach is that this no longer documents the API that
> > > > > drivers are intended to use, but the intermediate one.
> > >
> > > Yep. the better would be to keep documenting what will be used.
> > >
> >
> > Is there a way to silence/ignore the warning for a specific function(s)?
>
> No. The warning is there for a very good reason: if you do something like:
>
>
> /**
> * delete - delete some file
> *
> * This function deletes something.
> */
> void insert() {}
> /**
> * insert - creates a new file
> *
> * This function creates a file and inserts something.
> */
> void delete() {}
>
> the output of it will be:
>
> $ ./scripts/kernel-doc -sphinx-version 3.0.0 silly.c
> .. c:function:: void insert ()
>
> delete some file
>
> **Parameters**
>
> **Description**
>
>
> This function deletes something.
>
>
> .. c:function:: void delete ()
>
> creates a new file
>
> **Parameters**
>
> **Description**
>
>
> This function creates a file and inserts something.
>
> Which is completely wrong. If someone tries to write a code using the
> above, the result will be just the opposite than what it was documented.
>
> Btw, I noticed several places where things like that happened, because
> some code were added between a Kernel-doc markup and some function.
Btw, in the specific case of this change, something like the above
quick hack would fix it.
PS: I didn't think much when I wrote the __type description.
Also, keeping the symbols that should be documented as __foo
doesn't seem the right thing to me :-)
---
[PATCH] v4l2-async.h: Fix kerneldoc markups
Document the functions that should be used by the kAPI clients,
instead of the internal functions.
Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
index 6f22daa6f067..91dbeaa4e6ee 100644
--- a/include/media/v4l2-async.h
+++ b/include/media/v4l2-async.h
@@ -156,42 +156,38 @@ void v4l2_async_notifier_init(struct v4l2_async_notifier *notifier);
int __v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier,
struct v4l2_async_subdev *asd);
+struct v4l2_async_subdev *
+__v4l2_async_notifier_add_fwnode_subdev(struct v4l2_async_notifier *notifier,
+ struct fwnode_handle *fwnode,
+ unsigned int asd_struct_size);
/**
* v4l2_async_notifier_add_fwnode_subdev - Allocate and add a fwnode async
* subdev to the notifier's master asd_list.
*
- * @notifier: pointer to &struct v4l2_async_notifier
- * @fwnode: fwnode handle of the sub-device to be matched
- * @asd_struct_size: size of the driver's async sub-device struct, including
- * sizeof(struct v4l2_async_subdev). The &struct
- * v4l2_async_subdev shall be the first member of
- * the driver's async sub-device struct, i.e. both
- * begin at the same memory address.
+ * @__notifier: pointer to &struct v4l2_async_notifier
+ * @__fwnode: fwnode handle of the sub-device to be matched
+ * @__type: type of the struct that contains a struct v4l2_async_subdev.
*
* Allocate a fwnode-matched asd of size asd_struct_size, and add it to the
* notifiers @asd_list. The function also gets a reference of the fwnode which
* is released later at notifier cleanup time.
*/
-struct v4l2_async_subdev *
-__v4l2_async_notifier_add_fwnode_subdev(struct v4l2_async_notifier *notifier,
- struct fwnode_handle *fwnode,
- unsigned int asd_struct_size);
#define v4l2_async_notifier_add_fwnode_subdev(__notifier, __fwnode, __type) \
((__type *)__v4l2_async_notifier_add_fwnode_subdev(__notifier, __fwnode, \
sizeof(__type)))
+struct v4l2_async_subdev *
+__v4l2_async_notifier_add_fwnode_remote_subdev(struct v4l2_async_notifier *notif,
+ struct fwnode_handle *endpoint,
+ unsigned int asd_struct_size);
/**
* v4l2_async_notifier_add_fwnode_remote_subdev - Allocate and add a fwnode
* remote async subdev to the
* notifier's master asd_list.
*
- * @notif: pointer to &struct v4l2_async_notifier
- * @endpoint: local endpoint pointing to the remote sub-device to be matched
- * @asd_struct_size: size of the driver's async sub-device struct, including
- * sizeof(struct v4l2_async_subdev). The &struct
- * v4l2_async_subdev shall be the first member of
- * the driver's async sub-device struct, i.e. both
- * begin at the same memory address.
+ * @__notifier: pointer to &struct v4l2_async_notifier
+ * @__ep: local endpoint pointing to the remote sub-device to be matched
+ * @__type: type of the struct that contains a struct v4l2_async_subdev.
*
* Gets the remote endpoint of a given local endpoint, set it up for fwnode
* matching and adds the async sub-device to the notifier's @asd_list. The
@@ -201,33 +197,25 @@ __v4l2_async_notifier_add_fwnode_subdev(struct v4l2_async_notifier *notifier,
* This is just like @v4l2_async_notifier_add_fwnode_subdev, but with the
* exception that the fwnode refers to a local endpoint, not the remote one.
*/
-struct v4l2_async_subdev *
-__v4l2_async_notifier_add_fwnode_remote_subdev(struct v4l2_async_notifier *notif,
- struct fwnode_handle *endpoint,
- unsigned int asd_struct_size);
#define v4l2_async_notifier_add_fwnode_remote_subdev(__notifier, __ep, __type) \
((__type *)__v4l2_async_notifier_add_fwnode_remote_subdev(__notifier, __ep, \
sizeof(__type)))
+struct v4l2_async_subdev *
+__v4l2_async_notifier_add_i2c_subdev(struct v4l2_async_notifier *notifier,
+ int adapter_id, unsigned short address,
+ unsigned int asd_struct_size);
/**
* v4l2_async_notifier_add_i2c_subdev - Allocate and add an i2c async
* subdev to the notifier's master asd_list.
*
- * @notifier: pointer to &struct v4l2_async_notifier
- * @adapter_id: I2C adapter ID to be matched
- * @address: I2C address of sub-device to be matched
- * @asd_struct_size: size of the driver's async sub-device struct, including
- * sizeof(struct v4l2_async_subdev). The &struct
- * v4l2_async_subdev shall be the first member of
- * the driver's async sub-device struct, i.e. both
- * begin at the same memory address.
+ * @__notifier: pointer to &struct v4l2_async_notifier
+ * @__adap: I2C adapter ID to be matched
+ * @__addr: I2C address of sub-device to be matched
+ * @__type: type of the struct that contains a struct v4l2_async_subdev.
*
* Same as above but for I2C matched sub-devices.
*/
-struct v4l2_async_subdev *
-__v4l2_async_notifier_add_i2c_subdev(struct v4l2_async_notifier *notifier,
- int adapter_id, unsigned short address,
- unsigned int asd_struct_size);
#define v4l2_async_notifier_add_i2c_subdev(__notifier, __adap, __addr, __type) \
((__type *)__v4l2_async_notifier_add_i2c_subdev(__notifier, __adap, __addr, \
sizeof(__type)))
Thanks,
Mauro
^ permalink raw reply related [flat|nested] 55+ messages in thread
* Re: linux-next: build warning after merge of the v4l-dvb tree
2021-02-08 18:53 ` Ezequiel Garcia
@ 2021-02-15 10:20 ` Mauro Carvalho Chehab
2021-02-15 10:42 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 55+ messages in thread
From: Mauro Carvalho Chehab @ 2021-02-15 10:20 UTC (permalink / raw)
To: Ezequiel Garcia
Cc: Sakari Ailus, Stephen Rothwell, Linux Kernel Mailing List,
Linux Next Mailing List
Em Mon, 08 Feb 2021 15:53:00 -0300
Ezequiel Garcia <ezequiel@collabora.com> escreveu:
> On Mon, 2021-02-08 at 18:40 +0100, Mauro Carvalho Chehab wrote:
> > Em Mon, 08 Feb 2021 13:57:56 -0300
> > Ezequiel Garcia <ezequiel@collabora.com> escreveu:
> >
> > > On Mon, 2021-02-08 at 18:46 +0200, Sakari Ailus wrote:
> > > > Hi Ezequiel,
> > > >
> > > > Thanks for addressing this.
> > > >
> > > > On Mon, Feb 08, 2021 at 01:42:21PM -0300, Ezequiel Garcia wrote:
> > > > > Hi Stephen,
> > > > >
> > > > > On Mon, 2021-02-08 at 23:37 +1100, Stephen Rothwell wrote:
> > > > > > Hi all,
> > > > > >
> > > > > > After merging the v4l-dvb tree, today's linux-next build (htmldocs)
> > > > > > produced this warning:
> > > > > >
> > > > > > include/media/v4l2-async.h:178: warning: expecting prototype for v4l2_async_notifier_add_fwnode_subdev(). Prototype was for
> > > > > > __v4l2_async_notifier_add_fwnode_subdev() instead
> > > > > > include/media/v4l2-async.h:207: warning: expecting prototype for v4l2_async_notifier_add_fwnode_remote_subdev(). Prototype was for
> > > > > > __v4l2_async_notifier_add_fwnode_remote_subdev() instead
> > > > > > include/media/v4l2-async.h:230: warning: expecting prototype for v4l2_async_notifier_add_i2c_subdev(). Prototype was for
> > > > > > __v4l2_async_notifier_add_i2c_subdev() instead
> > > > > >
> > > > > > Maybe introduced by commit
> > > > > >
> > > > > > c1cc23625062 ("media: v4l2-async: Discourage use of v4l2_async_notifier_add_subdev")
> > > > > >
> > > > >
> > > > > Thanks for spotting this. Should be fixed by:
> > > > >
> > > > > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> > > > > index 6f22daa6f067..3785445282fc 100644
> > > > > --- a/include/media/v4l2-async.h
> > > > > +++ b/include/media/v4l2-async.h
> > > > > @@ -157,7 +157,7 @@ int __v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier,
> > > > > struct v4l2_async_subdev *asd);
> > > > >
> > > > > /**
> > > > > - * v4l2_async_notifier_add_fwnode_subdev - Allocate and add a fwnode async
> > > > > + * __v4l2_async_notifier_add_fwnode_subdev - Allocate and add a fwnode async
> > > >
> > > > The problem with the approach is that this no longer documents the API that
> > > > drivers are intended to use, but the intermediate one.
> >
> > Yep. the better would be to keep documenting what will be used.
> >
>
> Is there a way to silence/ignore the warning for a specific function(s)?
No. The warning is there for a very good reason: if you do something like:
/**
* delete - delete some file
*
* This function deletes something.
*/
void insert() {}
/**
* insert - creates a new file
*
* This function creates a file and inserts something.
*/
void delete() {}
the output of it will be:
$ ./scripts/kernel-doc -sphinx-version 3.0.0 silly.c
.. c:function:: void insert ()
delete some file
**Parameters**
**Description**
This function deletes something.
.. c:function:: void delete ()
creates a new file
**Parameters**
**Description**
This function creates a file and inserts something.
Which is completely wrong. If someone tries to write a code using the
above, the result will be just the opposite than what it was documented.
Btw, I noticed several places where things like that happened, because
some code were added between a Kernel-doc markup and some function.
Thanks,
Mauro
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: linux-next: build warning after merge of the v4l-dvb tree
2021-02-08 12:37 Stephen Rothwell
2021-02-08 16:42 ` Ezequiel Garcia
@ 2021-02-14 22:44 ` Stephen Rothwell
1 sibling, 0 replies; 55+ messages in thread
From: Stephen Rothwell @ 2021-02-14 22:44 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Ezequiel Garcia, Sakari Ailus, Linux Kernel Mailing List,
Linux Next Mailing List
[-- Attachment #1: Type: text/plain, Size: 991 bytes --]
Hi all,
On Mon, 8 Feb 2021 23:37:16 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> After merging the v4l-dvb tree, today's linux-next build (htmldocs)
> produced this warning:
>
> include/media/v4l2-async.h:178: warning: expecting prototype for v4l2_async_notifier_add_fwnode_subdev(). Prototype was for __v4l2_async_notifier_add_fwnode_subdev() instead
> include/media/v4l2-async.h:207: warning: expecting prototype for v4l2_async_notifier_add_fwnode_remote_subdev(). Prototype was for __v4l2_async_notifier_add_fwnode_remote_subdev() instead
> include/media/v4l2-async.h:230: warning: expecting prototype for v4l2_async_notifier_add_i2c_subdev(). Prototype was for __v4l2_async_notifier_add_i2c_subdev() instead
>
> Maybe introduced by commit
>
> c1cc23625062 ("media: v4l2-async: Discourage use of v4l2_async_notifier_add_subdev")
With the merge window about to open, this is a reminder that these
warnings still exist.
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: linux-next: build warning after merge of the v4l-dvb tree
2021-02-08 17:40 ` Mauro Carvalho Chehab
@ 2021-02-08 18:53 ` Ezequiel Garcia
2021-02-15 10:20 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 55+ messages in thread
From: Ezequiel Garcia @ 2021-02-08 18:53 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Sakari Ailus, Stephen Rothwell, Linux Kernel Mailing List,
Linux Next Mailing List
On Mon, 2021-02-08 at 18:40 +0100, Mauro Carvalho Chehab wrote:
> Em Mon, 08 Feb 2021 13:57:56 -0300
> Ezequiel Garcia <ezequiel@collabora.com> escreveu:
>
> > On Mon, 2021-02-08 at 18:46 +0200, Sakari Ailus wrote:
> > > Hi Ezequiel,
> > >
> > > Thanks for addressing this.
> > >
> > > On Mon, Feb 08, 2021 at 01:42:21PM -0300, Ezequiel Garcia wrote:
> > > > Hi Stephen,
> > > >
> > > > On Mon, 2021-02-08 at 23:37 +1100, Stephen Rothwell wrote:
> > > > > Hi all,
> > > > >
> > > > > After merging the v4l-dvb tree, today's linux-next build (htmldocs)
> > > > > produced this warning:
> > > > >
> > > > > include/media/v4l2-async.h:178: warning: expecting prototype for v4l2_async_notifier_add_fwnode_subdev(). Prototype was for
> > > > > __v4l2_async_notifier_add_fwnode_subdev() instead
> > > > > include/media/v4l2-async.h:207: warning: expecting prototype for v4l2_async_notifier_add_fwnode_remote_subdev(). Prototype was for
> > > > > __v4l2_async_notifier_add_fwnode_remote_subdev() instead
> > > > > include/media/v4l2-async.h:230: warning: expecting prototype for v4l2_async_notifier_add_i2c_subdev(). Prototype was for
> > > > > __v4l2_async_notifier_add_i2c_subdev() instead
> > > > >
> > > > > Maybe introduced by commit
> > > > >
> > > > > c1cc23625062 ("media: v4l2-async: Discourage use of v4l2_async_notifier_add_subdev")
> > > > >
> > > >
> > > > Thanks for spotting this. Should be fixed by:
> > > >
> > > > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> > > > index 6f22daa6f067..3785445282fc 100644
> > > > --- a/include/media/v4l2-async.h
> > > > +++ b/include/media/v4l2-async.h
> > > > @@ -157,7 +157,7 @@ int __v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier,
> > > > struct v4l2_async_subdev *asd);
> > > >
> > > > /**
> > > > - * v4l2_async_notifier_add_fwnode_subdev - Allocate and add a fwnode async
> > > > + * __v4l2_async_notifier_add_fwnode_subdev - Allocate and add a fwnode async
> > >
> > > The problem with the approach is that this no longer documents the API that
> > > drivers are intended to use, but the intermediate one.
>
> Yep. the better would be to keep documenting what will be used.
>
Is there a way to silence/ignore the warning for a specific function(s)?
Ezequiel
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: linux-next: build warning after merge of the v4l-dvb tree
2021-02-08 16:57 ` Ezequiel Garcia
@ 2021-02-08 17:40 ` Mauro Carvalho Chehab
2021-02-08 18:53 ` Ezequiel Garcia
0 siblings, 1 reply; 55+ messages in thread
From: Mauro Carvalho Chehab @ 2021-02-08 17:40 UTC (permalink / raw)
To: Ezequiel Garcia
Cc: Sakari Ailus, Stephen Rothwell, Linux Kernel Mailing List,
Linux Next Mailing List
Em Mon, 08 Feb 2021 13:57:56 -0300
Ezequiel Garcia <ezequiel@collabora.com> escreveu:
> On Mon, 2021-02-08 at 18:46 +0200, Sakari Ailus wrote:
> > Hi Ezequiel,
> >
> > Thanks for addressing this.
> >
> > On Mon, Feb 08, 2021 at 01:42:21PM -0300, Ezequiel Garcia wrote:
> > > Hi Stephen,
> > >
> > > On Mon, 2021-02-08 at 23:37 +1100, Stephen Rothwell wrote:
> > > > Hi all,
> > > >
> > > > After merging the v4l-dvb tree, today's linux-next build (htmldocs)
> > > > produced this warning:
> > > >
> > > > include/media/v4l2-async.h:178: warning: expecting prototype for v4l2_async_notifier_add_fwnode_subdev(). Prototype was for
> > > > __v4l2_async_notifier_add_fwnode_subdev() instead
> > > > include/media/v4l2-async.h:207: warning: expecting prototype for v4l2_async_notifier_add_fwnode_remote_subdev(). Prototype was for
> > > > __v4l2_async_notifier_add_fwnode_remote_subdev() instead
> > > > include/media/v4l2-async.h:230: warning: expecting prototype for v4l2_async_notifier_add_i2c_subdev(). Prototype was for
> > > > __v4l2_async_notifier_add_i2c_subdev() instead
> > > >
> > > > Maybe introduced by commit
> > > >
> > > > c1cc23625062 ("media: v4l2-async: Discourage use of v4l2_async_notifier_add_subdev")
> > > >
> > >
> > > Thanks for spotting this. Should be fixed by:
> > >
> > > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> > > index 6f22daa6f067..3785445282fc 100644
> > > --- a/include/media/v4l2-async.h
> > > +++ b/include/media/v4l2-async.h
> > > @@ -157,7 +157,7 @@ int __v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier,
> > > struct v4l2_async_subdev *asd);
> > >
> > > /**
> > > - * v4l2_async_notifier_add_fwnode_subdev - Allocate and add a fwnode async
> > > + * __v4l2_async_notifier_add_fwnode_subdev - Allocate and add a fwnode async
> >
> > The problem with the approach is that this no longer documents the API that
> > drivers are intended to use, but the intermediate one.
Yep. the better would be to keep documenting what will be used.
> > I guess fixing
> > this properly could require changes to kerneldoc so I have no objections to
> > the approach.
It is not a simple kernel-doc change.
The problem is that Kernel-doc expects:
/**
* foo - something
*/
void foo(...)
As it parses the file lines sequentially, using the parameters at
foo(...) to double-check if everything is ok.
In order for it to parse things like:
/**
* foo - something
*/
... (some other functions in the middle)
void foo(...)
Would require kernel-doc to first parse all the file, storing markups
on a separate struct, and then, on a second step, produce an output.
Even if modified to do that, there's a question if the result would
be what it is expected.
A separate thing would be to do things like:
/**
* foo - something
*/
void __foo(...)
The problem here is that usually the arguments for __foo() are
different than the ones for foo(). See for example the macros that
have a __foo() functions with an owner argument, that are solved
on a macro called foo().
Thanks,
Mauro
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: linux-next: build warning after merge of the v4l-dvb tree
2021-02-08 16:46 ` Sakari Ailus
@ 2021-02-08 16:57 ` Ezequiel Garcia
2021-02-08 17:40 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 55+ messages in thread
From: Ezequiel Garcia @ 2021-02-08 16:57 UTC (permalink / raw)
To: Sakari Ailus
Cc: Stephen Rothwell, Mauro Carvalho Chehab,
Linux Kernel Mailing List, Linux Next Mailing List
On Mon, 2021-02-08 at 18:46 +0200, Sakari Ailus wrote:
> Hi Ezequiel,
>
> Thanks for addressing this.
>
> On Mon, Feb 08, 2021 at 01:42:21PM -0300, Ezequiel Garcia wrote:
> > Hi Stephen,
> >
> > On Mon, 2021-02-08 at 23:37 +1100, Stephen Rothwell wrote:
> > > Hi all,
> > >
> > > After merging the v4l-dvb tree, today's linux-next build (htmldocs)
> > > produced this warning:
> > >
> > > include/media/v4l2-async.h:178: warning: expecting prototype for v4l2_async_notifier_add_fwnode_subdev(). Prototype was for
> > > __v4l2_async_notifier_add_fwnode_subdev() instead
> > > include/media/v4l2-async.h:207: warning: expecting prototype for v4l2_async_notifier_add_fwnode_remote_subdev(). Prototype was for
> > > __v4l2_async_notifier_add_fwnode_remote_subdev() instead
> > > include/media/v4l2-async.h:230: warning: expecting prototype for v4l2_async_notifier_add_i2c_subdev(). Prototype was for
> > > __v4l2_async_notifier_add_i2c_subdev() instead
> > >
> > > Maybe introduced by commit
> > >
> > > c1cc23625062 ("media: v4l2-async: Discourage use of v4l2_async_notifier_add_subdev")
> > >
> >
> > Thanks for spotting this. Should be fixed by:
> >
> > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> > index 6f22daa6f067..3785445282fc 100644
> > --- a/include/media/v4l2-async.h
> > +++ b/include/media/v4l2-async.h
> > @@ -157,7 +157,7 @@ int __v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier,
> > struct v4l2_async_subdev *asd);
> >
> > /**
> > - * v4l2_async_notifier_add_fwnode_subdev - Allocate and add a fwnode async
> > + * __v4l2_async_notifier_add_fwnode_subdev - Allocate and add a fwnode async
>
> The problem with the approach is that this no longer documents the API that
> drivers are intended to use, but the intermediate one. I guess fixing
> this properly could require changes to kerneldoc so I have no objections to
> the approach.
>
Right, but do we have any other solution here?
> > * subdev to the notifier's master asd_list.
> > *
> > * @notifier: pointer to &struct v4l2_async_notifier
> > @@ -181,7 +181,7 @@ __v4l2_async_notifier_add_fwnode_subdev(struct v4l2_async_notifier *notifier,
> > sizeof(__type)))
> >
> > /**
> > - * v4l2_async_notifier_add_fwnode_remote_subdev - Allocate and add a fwnode
> > + * __v4l2_async_notifier_add_fwnode_remote_subdev - Allocate and add a fwnode
> > * remote async subdev to the
> > * notifier's master asd_list.
> > *
> > @@ -210,7 +210,7 @@ __v4l2_async_notifier_add_fwnode_remote_subdev(struct v4l2_async_notifier *notif
> > sizeof(__type)))
> >
> > /**
> > - * v4l2_async_notifier_add_i2c_subdev - Allocate and add an i2c async
> > + * __v4l2_async_notifier_add_i2c_subdev - Allocate and add an i2c async
> > * subdev to the notifier's master asd_list.
> > *
> > * @notifier: pointer to &struct v4l2_async_notifier
> > @@ -228,7 +228,7 @@ struct v4l2_async_subdev *
> > __v4l2_async_notifier_add_i2c_subdev(struct v4l2_async_notifier *notifier,
> > int adapter_id, unsigned short address,
> > unsigned int asd_struct_size);
> > -#define v4l2_async_notifier_add_i2c_subdev(__notifier, __adap, __addr, __type) \
> > +#define v4l2_async_notifier_i2c(__notifier, __adap, __addr, __type) \
>
> I guess this change was not intentional?
>
Indeed :)
Thanks,
Ezequiel
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: linux-next: build warning after merge of the v4l-dvb tree
2021-02-08 16:42 ` Ezequiel Garcia
@ 2021-02-08 16:46 ` Sakari Ailus
2021-02-08 16:57 ` Ezequiel Garcia
0 siblings, 1 reply; 55+ messages in thread
From: Sakari Ailus @ 2021-02-08 16:46 UTC (permalink / raw)
To: Ezequiel Garcia
Cc: Stephen Rothwell, Mauro Carvalho Chehab,
Linux Kernel Mailing List, Linux Next Mailing List
Hi Ezequiel,
Thanks for addressing this.
On Mon, Feb 08, 2021 at 01:42:21PM -0300, Ezequiel Garcia wrote:
> Hi Stephen,
>
> On Mon, 2021-02-08 at 23:37 +1100, Stephen Rothwell wrote:
> > Hi all,
> >
> > After merging the v4l-dvb tree, today's linux-next build (htmldocs)
> > produced this warning:
> >
> > include/media/v4l2-async.h:178: warning: expecting prototype for v4l2_async_notifier_add_fwnode_subdev(). Prototype was for
> > __v4l2_async_notifier_add_fwnode_subdev() instead
> > include/media/v4l2-async.h:207: warning: expecting prototype for v4l2_async_notifier_add_fwnode_remote_subdev(). Prototype was for
> > __v4l2_async_notifier_add_fwnode_remote_subdev() instead
> > include/media/v4l2-async.h:230: warning: expecting prototype for v4l2_async_notifier_add_i2c_subdev(). Prototype was for
> > __v4l2_async_notifier_add_i2c_subdev() instead
> >
> > Maybe introduced by commit
> >
> > c1cc23625062 ("media: v4l2-async: Discourage use of v4l2_async_notifier_add_subdev")
> >
>
> Thanks for spotting this. Should be fixed by:
>
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index 6f22daa6f067..3785445282fc 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -157,7 +157,7 @@ int __v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier,
> struct v4l2_async_subdev *asd);
>
> /**
> - * v4l2_async_notifier_add_fwnode_subdev - Allocate and add a fwnode async
> + * __v4l2_async_notifier_add_fwnode_subdev - Allocate and add a fwnode async
The problem with the approach is that this no longer documents the API that
drivers are intended to use, but the intermediate one. I guess fixing
this properly could require changes to kerneldoc so I have no objections to
the approach.
> * subdev to the notifier's master asd_list.
> *
> * @notifier: pointer to &struct v4l2_async_notifier
> @@ -181,7 +181,7 @@ __v4l2_async_notifier_add_fwnode_subdev(struct v4l2_async_notifier *notifier,
> sizeof(__type)))
>
> /**
> - * v4l2_async_notifier_add_fwnode_remote_subdev - Allocate and add a fwnode
> + * __v4l2_async_notifier_add_fwnode_remote_subdev - Allocate and add a fwnode
> * remote async subdev to the
> * notifier's master asd_list.
> *
> @@ -210,7 +210,7 @@ __v4l2_async_notifier_add_fwnode_remote_subdev(struct v4l2_async_notifier *notif
> sizeof(__type)))
>
> /**
> - * v4l2_async_notifier_add_i2c_subdev - Allocate and add an i2c async
> + * __v4l2_async_notifier_add_i2c_subdev - Allocate and add an i2c async
> * subdev to the notifier's master asd_list.
> *
> * @notifier: pointer to &struct v4l2_async_notifier
> @@ -228,7 +228,7 @@ struct v4l2_async_subdev *
> __v4l2_async_notifier_add_i2c_subdev(struct v4l2_async_notifier *notifier,
> int adapter_id, unsigned short address,
> unsigned int asd_struct_size);
> -#define v4l2_async_notifier_add_i2c_subdev(__notifier, __adap, __addr, __type) \
> +#define v4l2_async_notifier_i2c(__notifier, __adap, __addr, __type) \
I guess this change was not intentional?
> ((__type *)__v4l2_async_notifier_add_i2c_subdev(__notifier, __adap, __addr, \
> sizeof(__type)))
>
>
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: linux-next: build warning after merge of the v4l-dvb tree
2021-02-08 12:37 Stephen Rothwell
@ 2021-02-08 16:42 ` Ezequiel Garcia
2021-02-08 16:46 ` Sakari Ailus
2021-02-14 22:44 ` Stephen Rothwell
1 sibling, 1 reply; 55+ messages in thread
From: Ezequiel Garcia @ 2021-02-08 16:42 UTC (permalink / raw)
To: Stephen Rothwell, Mauro Carvalho Chehab
Cc: Sakari Ailus, Linux Kernel Mailing List, Linux Next Mailing List
Hi Stephen,
On Mon, 2021-02-08 at 23:37 +1100, Stephen Rothwell wrote:
> Hi all,
>
> After merging the v4l-dvb tree, today's linux-next build (htmldocs)
> produced this warning:
>
> include/media/v4l2-async.h:178: warning: expecting prototype for v4l2_async_notifier_add_fwnode_subdev(). Prototype was for
> __v4l2_async_notifier_add_fwnode_subdev() instead
> include/media/v4l2-async.h:207: warning: expecting prototype for v4l2_async_notifier_add_fwnode_remote_subdev(). Prototype was for
> __v4l2_async_notifier_add_fwnode_remote_subdev() instead
> include/media/v4l2-async.h:230: warning: expecting prototype for v4l2_async_notifier_add_i2c_subdev(). Prototype was for
> __v4l2_async_notifier_add_i2c_subdev() instead
>
> Maybe introduced by commit
>
> c1cc23625062 ("media: v4l2-async: Discourage use of v4l2_async_notifier_add_subdev")
>
Thanks for spotting this. Should be fixed by:
diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
index 6f22daa6f067..3785445282fc 100644
--- a/include/media/v4l2-async.h
+++ b/include/media/v4l2-async.h
@@ -157,7 +157,7 @@ int __v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier,
struct v4l2_async_subdev *asd);
/**
- * v4l2_async_notifier_add_fwnode_subdev - Allocate and add a fwnode async
+ * __v4l2_async_notifier_add_fwnode_subdev - Allocate and add a fwnode async
* subdev to the notifier's master asd_list.
*
* @notifier: pointer to &struct v4l2_async_notifier
@@ -181,7 +181,7 @@ __v4l2_async_notifier_add_fwnode_subdev(struct v4l2_async_notifier *notifier,
sizeof(__type)))
/**
- * v4l2_async_notifier_add_fwnode_remote_subdev - Allocate and add a fwnode
+ * __v4l2_async_notifier_add_fwnode_remote_subdev - Allocate and add a fwnode
* remote async subdev to the
* notifier's master asd_list.
*
@@ -210,7 +210,7 @@ __v4l2_async_notifier_add_fwnode_remote_subdev(struct v4l2_async_notifier *notif
sizeof(__type)))
/**
- * v4l2_async_notifier_add_i2c_subdev - Allocate and add an i2c async
+ * __v4l2_async_notifier_add_i2c_subdev - Allocate and add an i2c async
* subdev to the notifier's master asd_list.
*
* @notifier: pointer to &struct v4l2_async_notifier
@@ -228,7 +228,7 @@ struct v4l2_async_subdev *
__v4l2_async_notifier_add_i2c_subdev(struct v4l2_async_notifier *notifier,
int adapter_id, unsigned short address,
unsigned int asd_struct_size);
-#define v4l2_async_notifier_add_i2c_subdev(__notifier, __adap, __addr, __type) \
+#define v4l2_async_notifier_i2c(__notifier, __adap, __addr, __type) \
((__type *)__v4l2_async_notifier_add_i2c_subdev(__notifier, __adap, __addr, \
sizeof(__type)))
I'll send a fix for Mauro.
Best regards,
Ezequiel
^ permalink raw reply related [flat|nested] 55+ messages in thread
* linux-next: build warning after merge of the v4l-dvb tree
@ 2021-02-08 12:37 Stephen Rothwell
2021-02-08 16:42 ` Ezequiel Garcia
2021-02-14 22:44 ` Stephen Rothwell
0 siblings, 2 replies; 55+ messages in thread
From: Stephen Rothwell @ 2021-02-08 12:37 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Ezequiel Garcia, Sakari Ailus, Linux Kernel Mailing List,
Linux Next Mailing List
[-- Attachment #1: Type: text/plain, Size: 793 bytes --]
Hi all,
After merging the v4l-dvb tree, today's linux-next build (htmldocs)
produced this warning:
include/media/v4l2-async.h:178: warning: expecting prototype for v4l2_async_notifier_add_fwnode_subdev(). Prototype was for __v4l2_async_notifier_add_fwnode_subdev() instead
include/media/v4l2-async.h:207: warning: expecting prototype for v4l2_async_notifier_add_fwnode_remote_subdev(). Prototype was for __v4l2_async_notifier_add_fwnode_remote_subdev() instead
include/media/v4l2-async.h:230: warning: expecting prototype for v4l2_async_notifier_add_i2c_subdev(). Prototype was for __v4l2_async_notifier_add_i2c_subdev() instead
Maybe introduced by commit
c1cc23625062 ("media: v4l2-async: Discourage use of v4l2_async_notifier_add_subdev")
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: linux-next: build warning after merge of the v4l-dvb tree
2021-01-13 4:10 Stephen Rothwell
@ 2021-01-20 6:14 ` Stephen Rothwell
0 siblings, 0 replies; 55+ messages in thread
From: Stephen Rothwell @ 2021-01-20 6:14 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Sakari Ailus, Linux Kernel Mailing List, Linux Next Mailing List
[-- Attachment #1: Type: text/plain, Size: 544 bytes --]
Hi all,
On Wed, 13 Jan 2021 15:10:27 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> After merging the v4l-dvb tree, today's linux-next build (htmldocs)
> produced this warning:
>
> Documentation/driver-api/media/v4l2-subdev.rst:125: WARNING: Inline interpreted text or phrase reference start-string without end-string.
>
> Introduced by commit
>
> 25c8d9a7689e ("media: Documentation: v4l: Document that link_validate op is valid for sink only")
I am still getting this warning.
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 55+ messages in thread
* linux-next: build warning after merge of the v4l-dvb tree
@ 2021-01-13 4:10 Stephen Rothwell
2021-01-20 6:14 ` Stephen Rothwell
0 siblings, 1 reply; 55+ messages in thread
From: Stephen Rothwell @ 2021-01-13 4:10 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Sakari Ailus, Linux Kernel Mailing List, Linux Next Mailing List
[-- Attachment #1: Type: text/plain, Size: 406 bytes --]
Hi all,
After merging the v4l-dvb tree, today's linux-next build (htmldocs)
produced this warning:
Documentation/driver-api/media/v4l2-subdev.rst:125: WARNING: Inline interpreted text or phrase reference start-string without end-string.
Introduced by commit
25c8d9a7689e ("media: Documentation: v4l: Document that link_validate op is valid for sink only")
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: linux-next: build warning after merge of the v4l-dvb tree
2020-11-26 6:54 ` Stephen Rothwell
@ 2020-11-27 10:11 ` Sean Young
0 siblings, 0 replies; 55+ messages in thread
From: Sean Young @ 2020-11-27 10:11 UTC (permalink / raw)
To: Stephen Rothwell
Cc: Mauro Carvalho Chehab, Dan Carpenter, Linux Kernel Mailing List,
Linux Next Mailing List
On Thu, Nov 26, 2020 at 05:54:52PM +1100, Stephen Rothwell wrote:
> Hi all,
>
> On Wed, 18 Nov 2020 16:29:34 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> >
> > After merging the v4l-dvb tree, today's linux-next build (htmldocs)
> > produced this warning:
> >
> > Documentation/output/lirc.h.rst:6: WARNING: undefined label: rc-proto-max (if the link has no caption the label must precede a section header)
> >
> > Introduced by commit
> >
> > 72e637fec558 ("media: rc: validate that "rc_proto" is reasonable")
>
> I am still getting this - despite commit
>
> cea357bc2571 ("media: lirc: ensure RC_PROTO_MAX has documentation")
>
> and today I got a second copy of the warning ...
Yes, this my bad. Thanks for pointing this out -- again.
In the mean time, this should be resolved by commit 711561a41d1f ("media:
lirc: fix lirc.h documentation generation").
Thanks,
Sean
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: linux-next: build warning after merge of the v4l-dvb tree
2020-11-18 5:29 Stephen Rothwell
@ 2020-11-26 6:54 ` Stephen Rothwell
2020-11-27 10:11 ` Sean Young
0 siblings, 1 reply; 55+ messages in thread
From: Stephen Rothwell @ 2020-11-26 6:54 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Dan Carpenter, Sean Young, Linux Kernel Mailing List,
Linux Next Mailing List
[-- Attachment #1: Type: text/plain, Size: 650 bytes --]
Hi all,
On Wed, 18 Nov 2020 16:29:34 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> After merging the v4l-dvb tree, today's linux-next build (htmldocs)
> produced this warning:
>
> Documentation/output/lirc.h.rst:6: WARNING: undefined label: rc-proto-max (if the link has no caption the label must precede a section header)
>
> Introduced by commit
>
> 72e637fec558 ("media: rc: validate that "rc_proto" is reasonable")
I am still getting this - despite commit
cea357bc2571 ("media: lirc: ensure RC_PROTO_MAX has documentation")
and today I got a second copy of the warning ...
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 55+ messages in thread
* linux-next: build warning after merge of the v4l-dvb tree
@ 2020-11-18 5:29 Stephen Rothwell
2020-11-26 6:54 ` Stephen Rothwell
0 siblings, 1 reply; 55+ messages in thread
From: Stephen Rothwell @ 2020-11-18 5:29 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Dan Carpenter, Sean Young, Linux Kernel Mailing List,
Linux Next Mailing List
[-- Attachment #1: Type: text/plain, Size: 380 bytes --]
Hi all,
After merging the v4l-dvb tree, today's linux-next build (htmldocs)
produced this warning:
Documentation/output/lirc.h.rst:6: WARNING: undefined label: rc-proto-max (if the link has no caption the label must precede a section header)
Introduced by commit
72e637fec558 ("media: rc: validate that "rc_proto" is reasonable")
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 55+ messages in thread
* linux-next: build warning after merge of the v4l-dvb tree
@ 2020-05-21 2:07 Stephen Rothwell
0 siblings, 0 replies; 55+ messages in thread
From: Stephen Rothwell @ 2020-05-21 2:07 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: Linux Next Mailing List, Linux Kernel Mailing List
[-- Attachment #1: Type: text/plain, Size: 504 bytes --]
Hi all,
After merging the v4l-dvb tree, today's linux-next build (x86_84
allmodconfig) produced this warning:
drivers/staging/media/atomisp/pci/atomisp_v4l2.c:764:12: warning: 'atomisp_mrfld_power' defined but not used [-Wunused-function]
764 | static int atomisp_mrfld_power(struct atomisp_device *isp, bool enable)
| ^~~~~~~~~~~~~~~~~~~
Introduced by commit
95d1f398c4dc ("media: atomisp: keep the ISP powered on when setting it")
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: linux-next: build warning after merge of the v4l-dvb tree
2020-04-17 5:13 ` Ezequiel Garcia
@ 2020-04-17 7:01 ` Mauro Carvalho Chehab
0 siblings, 0 replies; 55+ messages in thread
From: Mauro Carvalho Chehab @ 2020-04-17 7:01 UTC (permalink / raw)
To: Ezequiel Garcia
Cc: Stephen Rothwell, Linux Next Mailing List,
Linux Kernel Mailing List, linux-media, Hans Verkuil
Em Fri, 17 Apr 2020 02:13:47 -0300
Ezequiel Garcia <ezequiel@collabora.com> escreveu:
> Hi Stephen,
>
> On Fri, 2020-04-17 at 10:22 +1000, Stephen Rothwell wrote:
> > Hi all,
> >
> > After merging the v4l-dvb tree, today's linux-next build (arm
> > multi_v7_defconfig) produced this warning:
> >
> > WARNING: unmet direct dependencies detected for MEDIA_CONTROLLER_REQUEST_API
> > Depends on [n]: MEDIA_SUPPORT [=m] && MEDIA_CONTROLLER [=y] && STAGING_MEDIA [=n]
> > Selected by [m]:
> > - VIDEO_VIVID [=m] && MEDIA_SUPPORT [=m] && MEDIA_TEST_SUPPORT [=y] && V4L_TEST_DRIVERS [=y] && VIDEO_DEV [=m] && VIDEO_V4L2 [=m] && !SPARC32 &&
> > !SPARC64 && FB [=y] && HAS_DMA [=y]
> >
>
> Ugh, my bad. MEDIA_CONTROLLER_REQUEST_API can't
> depend on staging, after this recently merged commit:
>
> "media: Kconfig: Don't expose the Request API option"
>
> So, we should fix that with:
>
> diff --git a/drivers/media/mc/Kconfig b/drivers/media/mc/Kconfig
> index 7c9628f37196..4815b9dde9af 100644
> --- a/drivers/media/mc/Kconfig
> +++ b/drivers/media/mc/Kconfig
> @@ -14,7 +14,7 @@ config MEDIA_CONTROLLER_DVB
>
> config MEDIA_CONTROLLER_REQUEST_API
> bool
> - depends on MEDIA_CONTROLLER && STAGING_MEDIA
> + depends on MEDIA_CONTROLLER
> help
> DO NOT ENABLE THIS OPTION UNLESS YOU KNOW WHAT YOU'RE DOING.
>
> Mauro what do you think?
Dropped the dependency and applied on media.
I also addressed the "select PCI" issue, with causes troubles on
s390 random configs (due to HAS_PCI=n on s390).
Thanks,
Mauro
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: linux-next: build warning after merge of the v4l-dvb tree
2020-04-17 0:22 Stephen Rothwell
@ 2020-04-17 5:13 ` Ezequiel Garcia
2020-04-17 7:01 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 55+ messages in thread
From: Ezequiel Garcia @ 2020-04-17 5:13 UTC (permalink / raw)
To: Stephen Rothwell, Mauro Carvalho Chehab
Cc: Linux Next Mailing List, Linux Kernel Mailing List, linux-media,
Hans Verkuil
Hi Stephen,
On Fri, 2020-04-17 at 10:22 +1000, Stephen Rothwell wrote:
> Hi all,
>
> After merging the v4l-dvb tree, today's linux-next build (arm
> multi_v7_defconfig) produced this warning:
>
> WARNING: unmet direct dependencies detected for MEDIA_CONTROLLER_REQUEST_API
> Depends on [n]: MEDIA_SUPPORT [=m] && MEDIA_CONTROLLER [=y] && STAGING_MEDIA [=n]
> Selected by [m]:
> - VIDEO_VIVID [=m] && MEDIA_SUPPORT [=m] && MEDIA_TEST_SUPPORT [=y] && V4L_TEST_DRIVERS [=y] && VIDEO_DEV [=m] && VIDEO_V4L2 [=m] && !SPARC32 &&
> !SPARC64 && FB [=y] && HAS_DMA [=y]
>
Ugh, my bad. MEDIA_CONTROLLER_REQUEST_API can't
depend on staging, after this recently merged commit:
"media: Kconfig: Don't expose the Request API option"
So, we should fix that with:
diff --git a/drivers/media/mc/Kconfig b/drivers/media/mc/Kconfig
index 7c9628f37196..4815b9dde9af 100644
--- a/drivers/media/mc/Kconfig
+++ b/drivers/media/mc/Kconfig
@@ -14,7 +14,7 @@ config MEDIA_CONTROLLER_DVB
config MEDIA_CONTROLLER_REQUEST_API
bool
- depends on MEDIA_CONTROLLER && STAGING_MEDIA
+ depends on MEDIA_CONTROLLER
help
DO NOT ENABLE THIS OPTION UNLESS YOU KNOW WHAT YOU'RE DOING.
Mauro what do you think?
^ permalink raw reply related [flat|nested] 55+ messages in thread
* linux-next: build warning after merge of the v4l-dvb tree
@ 2020-04-17 0:22 Stephen Rothwell
2020-04-17 5:13 ` Ezequiel Garcia
0 siblings, 1 reply; 55+ messages in thread
From: Stephen Rothwell @ 2020-04-17 0:22 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Linux Next Mailing List, Linux Kernel Mailing List, Ezequiel Garcia
[-- Attachment #1: Type: text/plain, Size: 521 bytes --]
Hi all,
After merging the v4l-dvb tree, today's linux-next build (arm
multi_v7_defconfig) produced this warning:
WARNING: unmet direct dependencies detected for MEDIA_CONTROLLER_REQUEST_API
Depends on [n]: MEDIA_SUPPORT [=m] && MEDIA_CONTROLLER [=y] && STAGING_MEDIA [=n]
Selected by [m]:
- VIDEO_VIVID [=m] && MEDIA_SUPPORT [=m] && MEDIA_TEST_SUPPORT [=y] && V4L_TEST_DRIVERS [=y] && VIDEO_DEV [=m] && VIDEO_V4L2 [=m] && !SPARC32 && !SPARC64 && FB [=y] && HAS_DMA [=y]
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 55+ messages in thread
* linux-next: build warning after merge of the v4l-dvb tree
@ 2020-04-15 1:29 Stephen Rothwell
0 siblings, 0 replies; 55+ messages in thread
From: Stephen Rothwell @ 2020-04-15 1:29 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: Linux Next Mailing List, Linux Kernel Mailing List
[-- Attachment #1: Type: text/plain, Size: 1037 bytes --]
Hi all,
After merging the v4l-dvb tree, today's linux-next build (arm
multi_v7_defconfig) produced this warning:
WARNING: unmet direct dependencies detected for CEC_CORE
Depends on [m]: MEDIA_SUPPORT [=m]
Selected by [y]:
- DRM_TEGRA [=y] && HAS_IOMEM [=y] && (ARCH_TEGRA [=y] || ARM [=y] && COMPILE_TEST [=n]) && COMMON_CLK [=y] && DRM [=y] && OF [=y] && CEC_NOTIFIER [=y]
Selected by [m]:
- VIDEO_SAMSUNG_S5P_CEC [=m] && MEDIA_SUPPORT [=m] && MEDIA_PLATFORM_SUPPORT [=y] && CEC_PLATFORM_DRIVERS [=y] && (ARCH_EXYNOS [=y] || COMPILE_TEST [=n])
- DRM_EXYNOS_HDMI [=y] && HAS_IOMEM [=y] && DRM_EXYNOS [=m] && (DRM_EXYNOS_MIXER [=y] || DRM_EXYNOS5433_DECON [=n]) && CEC_NOTIFIER [=y]
- DRM_I2C_ADV7511_CEC [=y] && HAS_IOMEM [=y] && DRM [=y] && DRM_BRIDGE [=y] && DRM_I2C_ADV7511 [=m]
- DRM_DW_HDMI [=m] && HAS_IOMEM [=y] && DRM [=y] && DRM_BRIDGE [=y] && CEC_NOTIFIER [=y]
There have been so many Kconfig changes today that I cannot figure out
what caused this, sorry.
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 55+ messages in thread
* linux-next: build warning after merge of the v4l-dvb tree
@ 2019-01-31 23:25 Stephen Rothwell
0 siblings, 0 replies; 55+ messages in thread
From: Stephen Rothwell @ 2019-01-31 23:25 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Linux Next Mailing List, Linux Kernel Mailing List, Pawel Osciak,
Paul Kocialkowski, Hans Verkuil
[-- Attachment #1: Type: text/plain, Size: 482 bytes --]
Hi all,
After merging the v4l-dvb tree, today's linux-next build (arm
multi_v7_defconfig) produced this warning:
drivers/media/common/videobuf2/videobuf2-core.c: In function '__vb2_dqbuf':
drivers/media/common/videobuf2/videobuf2-core.c:1772:15: warning: unused variable 'i' [-Wunused-variable]
unsigned int i;
^
Introduced by commit
2cc1802f62e5 ("media: vb2: Keep dma-buf buffers mapped until they are freed")
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 55+ messages in thread
* linux-next: build warning after merge of the v4l-dvb tree
@ 2011-03-17 0:29 Stephen Rothwell
0 siblings, 0 replies; 55+ messages in thread
From: Stephen Rothwell @ 2011-03-17 0:29 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: linux-next, linux-kernel, Manjunatha Halli
[-- Attachment #1: Type: text/plain, Size: 538 bytes --]
Hi Mauro,
After merging the v4l-dvb tree, today's linux-next build (x86_64
allmodconfig) produced this warning:
drivers/media/radio/wl128x/fmdrv_common.c: In function 'recv_tasklet':
drivers/media/radio/wl128x/fmdrv_common.c:274: warning: format '%d' expects type 'int', but argument 4 has type 'long unsigned int'
Introduced by commit 38b9da379627 ("[media] drivers:media:radio: wl128x:
FM Driver Common sources").
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 55+ messages in thread
* linux-next: build warning after merge of the v4l-dvb tree
@ 2010-12-31 0:59 Stephen Rothwell
0 siblings, 0 replies; 55+ messages in thread
From: Stephen Rothwell @ 2010-12-31 0:59 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: linux-next, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 606 bytes --]
Hi Mauro,
After merging the v4l-dvb tree, today's linux-next build (x86_64
allmodconfig) produced this warning:
drivers/media/video/ivtv/ivtv-i2c.c: In function 'get_key_adaptec':
drivers/media/video/ivtv/ivtv-i2c.c:170: warning: cast from pointer to integer of different size
drivers/media/video/ivtv/ivtv-i2c.c:171: warning: cast from pointer to integer of different size
Introduced by commit e1e2c57565635310209566a31a300e593f74cc22 ("[media]
ivtv: Add Adaptec Remote Controller").
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: linux-next: build warning after merge of the v4l-dvb tree
2010-10-08 0:49 Stephen Rothwell
@ 2010-10-08 3:13 ` Mauro Carvalho Chehab
0 siblings, 0 replies; 55+ messages in thread
From: Mauro Carvalho Chehab @ 2010-10-08 3:13 UTC (permalink / raw)
To: Stephen Rothwell; +Cc: linux-next, linux-kernel
Em 07-10-2010 21:49, Stephen Rothwell escreveu:
> Hi Mauro,
>
> After merging the kvm tree, today's linux-next build (x86_64 allmodconfig)
> produced this warning:
>
> drivers/media/video/videobuf-dma-sg.c: In function 'videobuf_pages_to_sg':
> drivers/media/video/videobuf-dma-sg.c:119: warning: comparison of distinct pointer types lacks a cast
> drivers/media/video/videobuf-dma-sg.c:120: warning: comparison of distinct pointer types lacks a cast
>
> Commit ecc736735ecf922d7f31d34417f7c42f8ec9eb67 ("V4L/DVB:
> videobuf-dma-sg: Fix a warning due to the usage of min(PAGE_SIZE, arg)")
> tried to fix it (presumably on a 32 bit build), but is not correct for 64
> bits .
>
Hi Stephen,
Thanks. I have already a fix for it, using min_t(size_t, PAGE_SIZE, arg).
I'll add it probably tomorrow for the linux-next tree.
Thanks,
Mauro
^ permalink raw reply [flat|nested] 55+ messages in thread
* linux-next: build warning after merge of the v4l-dvb tree
@ 2010-10-08 0:49 Stephen Rothwell
2010-10-08 3:13 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 55+ messages in thread
From: Stephen Rothwell @ 2010-10-08 0:49 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: linux-next, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 725 bytes --]
Hi Mauro,
After merging the kvm tree, today's linux-next build (x86_64 allmodconfig)
produced this warning:
drivers/media/video/videobuf-dma-sg.c: In function 'videobuf_pages_to_sg':
drivers/media/video/videobuf-dma-sg.c:119: warning: comparison of distinct pointer types lacks a cast
drivers/media/video/videobuf-dma-sg.c:120: warning: comparison of distinct pointer types lacks a cast
Commit ecc736735ecf922d7f31d34417f7c42f8ec9eb67 ("V4L/DVB:
videobuf-dma-sg: Fix a warning due to the usage of min(PAGE_SIZE, arg)")
tried to fix it (presumably on a 32 bit build), but is not correct for 64
bits .
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 55+ messages in thread
end of thread, other threads:[~2021-05-21 0:48 UTC | newest]
Thread overview: 55+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-08 0:32 linux-next: build warning after merge of the v4l-dvb tree Stephen Rothwell
2021-02-08 6:30 ` Mauro Carvalho Chehab
2021-02-08 8:33 ` Geert Uytterhoeven
2021-02-08 8:49 ` Mauro Carvalho Chehab
2021-02-08 8:52 ` Geert Uytterhoeven
2021-02-08 9:14 ` Mauro Carvalho Chehab
2021-02-08 6:53 ` [PATCH] media: i2c: fix max9271 build dependencies Mauro Carvalho Chehab
2021-02-08 7:27 ` Sakari Ailus
2021-02-08 8:36 ` Jacopo Mondi
2021-02-08 8:41 ` Sakari Ailus
2021-02-08 8:59 ` Jacopo Mondi
2021-02-08 9:03 ` Sakari Ailus
2021-02-08 9:08 ` Mauro Carvalho Chehab
2021-02-08 9:24 ` Sakari Ailus
2021-02-08 10:07 ` Mauro Carvalho Chehab
2021-02-08 11:32 ` Laurent Pinchart
2021-02-08 11:41 ` Jacopo Mondi
2021-02-08 13:11 ` Mauro Carvalho Chehab
2021-02-08 13:31 ` Mauro Carvalho Chehab
2021-02-08 13:40 ` Laurent Pinchart
2021-02-08 14:23 ` Mauro Carvalho Chehab
2021-02-08 14:31 ` Laurent Pinchart
2021-02-08 15:01 ` Mauro Carvalho Chehab
2021-02-08 13:55 ` Jacopo Mondi
2021-02-08 14:14 ` Mauro Carvalho Chehab
2021-02-08 14:49 ` Jacopo Mondi
2021-02-08 15:42 ` Mauro Carvalho Chehab
2021-02-08 16:26 ` Laurent Pinchart
-- strict thread matches above, loose matches on Subject: below --
2021-05-10 23:46 linux-next: build warning after merge of the v4l-dvb tree Stephen Rothwell
2021-05-21 0:48 ` Stephen Rothwell
2021-03-23 5:56 Stephen Rothwell
2021-02-08 12:37 Stephen Rothwell
2021-02-08 16:42 ` Ezequiel Garcia
2021-02-08 16:46 ` Sakari Ailus
2021-02-08 16:57 ` Ezequiel Garcia
2021-02-08 17:40 ` Mauro Carvalho Chehab
2021-02-08 18:53 ` Ezequiel Garcia
2021-02-15 10:20 ` Mauro Carvalho Chehab
2021-02-15 10:42 ` Mauro Carvalho Chehab
2021-02-14 22:44 ` Stephen Rothwell
2021-01-13 4:10 Stephen Rothwell
2021-01-20 6:14 ` Stephen Rothwell
2020-11-18 5:29 Stephen Rothwell
2020-11-26 6:54 ` Stephen Rothwell
2020-11-27 10:11 ` Sean Young
2020-05-21 2:07 Stephen Rothwell
2020-04-17 0:22 Stephen Rothwell
2020-04-17 5:13 ` Ezequiel Garcia
2020-04-17 7:01 ` Mauro Carvalho Chehab
2020-04-15 1:29 Stephen Rothwell
2019-01-31 23:25 Stephen Rothwell
2011-03-17 0:29 Stephen Rothwell
2010-12-31 0:59 Stephen Rothwell
2010-10-08 0:49 Stephen Rothwell
2010-10-08 3:13 ` Mauro Carvalho Chehab
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).