* 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ messages in thread
end of thread, other threads:[~2021-02-08 16:28 UTC | newest]
Thread overview: 28+ 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
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).