From: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Jacopo Mondi <jacopo@jmondi.org>,
Sakari Ailus <sakari.ailus@linux.intel.com>,
Linux Media Mailing List <linux-media@vger.kernel.org>,
Jacopo Mondi <jacopo+renesas@jmondi.org>,
Stephen Rothwell <sfr@canb.auug.org.au>,
linux-next@vger.kernel.org
Subject: Re: [PATCH] media: i2c: fix max9271 build dependencies
Date: Mon, 8 Feb 2021 15:23:59 +0100 [thread overview]
Message-ID: <20210208152359.17831275@coco.lan> (raw)
In-Reply-To: <YCE/OkSfIbxUB4XQ@pendragon.ideasonboard.com>
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
next prev parent reply other threads:[~2021-02-08 14:37 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-08 0:32 linux-next: build warning after merge of the v4l-dvb tree Stephen Rothwell
2021-02-08 6:30 ` Mauro Carvalho Chehab
2021-02-08 8:33 ` Geert Uytterhoeven
2021-02-08 8:49 ` Mauro Carvalho Chehab
2021-02-08 8:52 ` Geert Uytterhoeven
2021-02-08 9:14 ` Mauro Carvalho Chehab
2021-02-08 6:53 ` [PATCH] media: i2c: fix max9271 build dependencies Mauro Carvalho Chehab
2021-02-08 7:27 ` Sakari Ailus
2021-02-08 8:36 ` Jacopo Mondi
2021-02-08 8:41 ` Sakari Ailus
2021-02-08 8:59 ` Jacopo Mondi
2021-02-08 9:03 ` Sakari Ailus
2021-02-08 9:08 ` Mauro Carvalho Chehab
2021-02-08 9:24 ` Sakari Ailus
2021-02-08 10:07 ` Mauro Carvalho Chehab
2021-02-08 11:32 ` Laurent Pinchart
2021-02-08 11:41 ` Jacopo Mondi
2021-02-08 13:11 ` Mauro Carvalho Chehab
2021-02-08 13:31 ` Mauro Carvalho Chehab
2021-02-08 13:40 ` Laurent Pinchart
2021-02-08 14:23 ` Mauro Carvalho Chehab [this message]
2021-02-08 14:31 ` Laurent Pinchart
2021-02-08 15:01 ` Mauro Carvalho Chehab
2021-02-08 13:55 ` Jacopo Mondi
2021-02-08 14:14 ` Mauro Carvalho Chehab
2021-02-08 14:49 ` Jacopo Mondi
2021-02-08 15:42 ` Mauro Carvalho Chehab
2021-02-08 16:26 ` Laurent Pinchart
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210208152359.17831275@coco.lan \
--to=mchehab+huawei@kernel.org \
--cc=jacopo+renesas@jmondi.org \
--cc=jacopo@jmondi.org \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-media@vger.kernel.org \
--cc=linux-next@vger.kernel.org \
--cc=sakari.ailus@linux.intel.com \
--cc=sfr@canb.auug.org.au \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).