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

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

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