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

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

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