[PATCH] media: i2c: fix max9271 build dependencies

[PATCH] media: i2c: fix max9271 build dependencies

[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

Re: [PATCH] media: i2c: fix max9271 build dependencies

[Sakari Ailus]

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

Re: [PATCH] media: i2c: fix max9271 build dependencies

[Jacopo Mondi]

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

Re: [PATCH] media: i2c: fix max9271 build dependencies

[Sakari Ailus]

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

Re: [PATCH] media: i2c: fix max9271 build dependencies

[Jacopo Mondi]

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

Re: [PATCH] media: i2c: fix max9271 build dependencies

[Sakari Ailus]

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

Re: [PATCH] media: i2c: fix max9271 build dependencies

[Mauro Carvalho Chehab]

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

Re: [PATCH] media: i2c: fix max9271 build dependencies

[Sakari Ailus]

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

Re: [PATCH] media: i2c: fix max9271 build dependencies

[Mauro Carvalho Chehab]

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

Re: [PATCH] media: i2c: fix max9271 build dependencies

[Laurent Pinchart]

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

Re: [PATCH] media: i2c: fix max9271 build dependencies

[Jacopo Mondi]

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

Re: [PATCH] media: i2c: fix max9271 build dependencies

[Mauro Carvalho Chehab]

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

Re: [PATCH] media: i2c: fix max9271 build dependencies

[Mauro Carvalho Chehab]

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

Re: [PATCH] media: i2c: fix max9271 build dependencies

[Laurent Pinchart]

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

Re: [PATCH] media: i2c: fix max9271 build dependencies

[Mauro Carvalho Chehab]

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

Re: [PATCH] media: i2c: fix max9271 build dependencies

[Laurent Pinchart]

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

Re: [PATCH] media: i2c: fix max9271 build dependencies

[Mauro Carvalho Chehab]

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

Re: [PATCH] media: i2c: fix max9271 build dependencies

[Jacopo Mondi]

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

Re: [PATCH] media: i2c: fix max9271 build dependencies

[Mauro Carvalho Chehab]

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

Re: [PATCH] media: i2c: fix max9271 build dependencies

[Jacopo Mondi]

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

Re: [PATCH] media: i2c: fix max9271 build dependencies

[Mauro Carvalho Chehab]

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

Re: [PATCH] media: i2c: fix max9271 build dependencies

[Laurent Pinchart]

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