From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Jacopo Mondi <jacopo+renesas@jmondi.org>
Cc: kieran.bingham+renesas@ideasonboard.com,
niklas.soderlund+renesas@ragnatech.se, geert@linux-m68k.org,
Mauro Carvalho Chehab <mchehab@kernel.org>,
linux-media@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 06/19] media: gmsl: Reimplement initialization sequence
Date: Sat, 20 Mar 2021 17:46:07 +0200 [thread overview]
Message-ID: <YFYYvwChCsF1rPxv@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20210319164148.199192-7-jacopo+renesas@jmondi.org>
Hi Jacopo,
Thank you for the patch.
On Fri, Mar 19, 2021 at 05:41:35PM +0100, Jacopo Mondi wrote:
> The current probe() procedure of the RDACM20 and RDACM20 GMSL cameras is
s/RDACM20 and RDACM20/RDACM20 and RDACM21/
> performed with the embedded MAX9271 serializer's noise immunity
> threshold disabled. Once the camera has been initialized by probing the
> embedded chips, the threshold is enabled and then compensated on the
> deserializer's side by increasing the reverse channel signal amplitude
> once all cameras have bound.
>
> The probe routine is thus run without noise immunity activated which
> in noisy environment conditions makes the probe sequence less reliable as
> the chips configuration requires a relatively high amount of i2c
> transactions.
>
> Break chip initialization in two:
> - At probe time only configure the serializer's reverse channel with
> noise immunity activated, to reduce the number of transactions
> performed without noise immunity protection enabled
> - Move the chips initialization to the .init() core subdev operation
> called by the deserializer after all camera have probed and
> have increased their noise immunity threshold
>
> The initialization routine looks like the following:
>
> MAX9286 RDACM20/21
>
> probe()
> |
> ---------------------> |
> probe() {
> enable_threshold()
> }
> |<--------------------|
> v4l2 async bound {
> compensate_amplitude()
> call subdev init()
> |-------------------->|
> init() {
> access camera registers()
> }
> |<-------------------
> }
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
> drivers/media/i2c/max9286.c | 19 +++++++---
> drivers/media/i2c/rdacm20.c | 69 +++++++++++++++++++++++--------------
> drivers/media/i2c/rdacm21.c | 65 ++++++++++++++++++++--------------
> 3 files changed, 98 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 9124d5fa6ea3..b6347639901e 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -563,17 +563,28 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
> if (priv->bound_sources != priv->source_mask)
> return 0;
>
> + /*
> + * Initialize all the remote camera. Increase the channel amplitude
> + * to compensate for the remote noise immunity threshold.
> + */
> + max9286_reverse_channel_setup(priv, MAX9286_REV_AMP_HIGH);
> + for_each_source(priv, source) {
> + ret = v4l2_subdev_call(source->sd, core, init, 0);
> + if (ret) {
> + dev_err(&priv->client->dev,
> + "Failed to initialize camera device %u\n",
> + index);
> + return ret;
> + }
> + }
> +
> /*
> * All enabled sources have probed and enabled their reverse control
> * channels:
> - *
> - * - Increase the reverse channel amplitude to compensate for the
> - * remote ends high threshold
> * - Verify all configuration links are properly detected
> * - Disable auto-ack as communication on the control channel are now
> * stable.
> */
> - max9286_reverse_channel_setup(priv, MAX9286_REV_AMP_HIGH);
> max9286_check_config_link(priv, priv->source_mask);
>
> /*
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index 90eb73f0e6e9..3cab6af7eba6 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -435,35 +435,12 @@ static int rdacm20_get_fmt(struct v4l2_subdev *sd,
> return 0;
> }
>
> -static const struct v4l2_subdev_video_ops rdacm20_video_ops = {
> - .s_stream = rdacm20_s_stream,
> -};
> -
> -static const struct v4l2_subdev_pad_ops rdacm20_subdev_pad_ops = {
> - .enum_mbus_code = rdacm20_enum_mbus_code,
> - .get_fmt = rdacm20_get_fmt,
> - .set_fmt = rdacm20_get_fmt,
> -};
> -
> -static const struct v4l2_subdev_ops rdacm20_subdev_ops = {
> - .video = &rdacm20_video_ops,
> - .pad = &rdacm20_subdev_pad_ops,
> -};
> -
> -static int rdacm20_initialize(struct rdacm20_device *dev)
> +static int rdacm20_init(struct v4l2_subdev *sd, unsigned int val)
> {
> + struct rdacm20_device *dev = sd_to_rdacm20(sd);
> unsigned int retry = 3;
> int ret;
>
> - /* Verify communication with the MAX9271: ping to wakeup. */
> - dev->serializer->client->addr = MAX9271_DEFAULT_ADDR;
> - i2c_smbus_read_byte(dev->serializer->client);
> -
> - /* Serial link disabled during config as it needs a valid pixel clock. */
> - ret = max9271_set_serial_link(dev->serializer, false);
> - if (ret)
> - return ret;
> -
> /*
> * Ensure that we have a good link configuration before attempting to
> * identify the device.
> @@ -544,6 +521,48 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
> return 0;
> }
>
> +static const struct v4l2_subdev_core_ops rdacm20_core_ops = {
> + .init = rdacm20_init,
> +};
> +
> +static const struct v4l2_subdev_video_ops rdacm20_video_ops = {
> + .s_stream = rdacm20_s_stream,
> +};
> +
> +static const struct v4l2_subdev_pad_ops rdacm20_subdev_pad_ops = {
> + .enum_mbus_code = rdacm20_enum_mbus_code,
> + .get_fmt = rdacm20_get_fmt,
> + .set_fmt = rdacm20_get_fmt,
> +};
> +
> +static const struct v4l2_subdev_ops rdacm20_subdev_ops = {
> + .core = &rdacm20_core_ops,
> + .video = &rdacm20_video_ops,
> + .pad = &rdacm20_subdev_pad_ops,
> +};
> +
> +static int rdacm20_initialize(struct rdacm20_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);
> +
> + /* Serial link disabled during config as it needs a valid pixel clock. */
> + ret = max9271_set_serial_link(dev->serializer, false);
> + if (ret)
> + return ret;
> +
> + /*
> + * Set reverse channel high threshold to increase noise immunity.
> + *
> + * This should be compensated by increasing the reverse channel
> + * amplitude on the remote deserializer side.
> + */
> + return max9271_set_high_threshold(dev->serializer, true);
> +}
> +
> static int rdacm20_probe(struct i2c_client *client)
> {
> struct rdacm20_device *dev;
> diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> index dcc21515e5a4..318a3b40d9e6 100644
> --- a/drivers/media/i2c/rdacm21.c
> +++ b/drivers/media/i2c/rdacm21.c
> @@ -314,21 +314,6 @@ static int rdacm21_get_fmt(struct v4l2_subdev *sd,
> return 0;
> }
>
> -static const struct v4l2_subdev_video_ops rdacm21_video_ops = {
> - .s_stream = rdacm21_s_stream,
> -};
> -
> -static const struct v4l2_subdev_pad_ops rdacm21_subdev_pad_ops = {
> - .enum_mbus_code = rdacm21_enum_mbus_code,
> - .get_fmt = rdacm21_get_fmt,
> - .set_fmt = rdacm21_get_fmt,
> -};
> -
> -static const struct v4l2_subdev_ops rdacm21_subdev_ops = {
> - .video = &rdacm21_video_ops,
> - .pad = &rdacm21_subdev_pad_ops,
> -};
> -
> static int ov10640_initialize(struct rdacm21_device *dev)
> {
> u8 val;
> @@ -446,20 +431,11 @@ static int ov490_initialize(struct rdacm21_device *dev)
> return 0;
> }
>
> -static int rdacm21_initialize(struct rdacm21_device *dev)
> +static int rdacm21_init(struct v4l2_subdev *sd, unsigned int val)
> {
> + struct rdacm21_device *dev = sd_to_rdacm21(sd);
> 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 |
> @@ -506,6 +482,43 @@ static int rdacm21_initialize(struct rdacm21_device *dev)
> if (ret)
> return ret;
>
> + return 0;
> +}
> +
> +static const struct v4l2_subdev_core_ops rdacm21_core_ops = {
> + .init = rdacm21_init,
> +};
> +
> +static const struct v4l2_subdev_video_ops rdacm21_video_ops = {
> + .s_stream = rdacm21_s_stream,
> +};
> +
> +static const struct v4l2_subdev_pad_ops rdacm21_subdev_pad_ops = {
> + .enum_mbus_code = rdacm21_enum_mbus_code,
> + .get_fmt = rdacm21_get_fmt,
> + .set_fmt = rdacm21_get_fmt,
> +};
> +
> +static const struct v4l2_subdev_ops rdacm21_subdev_ops = {
> + .core = &rdacm21_core_ops,
> + .video = &rdacm21_video_ops,
> + .pad = &rdacm21_subdev_pad_ops,
> +};
> +
> +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;
> +
> /*
> * Set reverse channel high threshold to increase noise immunity.
> *
--
Regards,
Laurent Pinchart
next prev parent reply other threads:[~2021-03-20 15:47 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-19 16:41 [PATCH v3 00/19] media: gmsl: Reliability improvement Jacopo Mondi
2021-03-19 16:41 ` [PATCH v3 01/19] media: i2c: max9286: Adjust parameters indent Jacopo Mondi
2021-03-19 16:41 ` [PATCH v3 02/19] media: i2c: max9286: Rename reverse_channel_mv Jacopo Mondi
2021-03-19 16:41 ` [PATCH v3 03/19] media: i2c: max9286: Cache channel amplitude Jacopo Mondi
2021-03-19 16:41 ` [PATCH v3 04/19] media: i2c: max9286: Define high " Jacopo Mondi
2021-03-19 16:41 ` [PATCH v3 05/19] media: v4l2-subdev: De-deprecate init() subdev op Jacopo Mondi
2021-03-20 15:42 ` Laurent Pinchart
2021-03-21 20:52 ` Sakari Ailus
2021-03-22 12:51 ` Jacopo Mondi
2021-03-26 11:37 ` Jacopo Mondi
2021-03-19 16:41 ` [PATCH v3 06/19] media: gmsl: Reimplement initialization sequence Jacopo Mondi
2021-03-20 15:46 ` Laurent Pinchart [this message]
2021-03-19 16:41 ` [PATCH v3 07/19] media: i2c: max9286: Rework comments in .bound() Jacopo Mondi
2021-03-19 16:41 ` [PATCH v3 08/19] media: i2c: max9271: Check max9271_write() return Jacopo Mondi
2021-03-19 16:41 ` [PATCH v3 09/19] media: i2c: max9271: Introduce wake_up() function Jacopo Mondi
2021-03-19 16:41 ` [PATCH v3 10/19] media: i2c: rdamc21: Fix warning on u8 cast Jacopo Mondi
2021-03-19 16:41 ` [PATCH v3 11/19] media: i2c: rdacm21: Add dealy after OV490 reset Jacopo Mondi
2021-03-19 16:49 ` Kieran Bingham
2021-03-20 15:52 ` Laurent Pinchart
2021-03-26 11:22 ` Jacopo Mondi
2021-03-19 16:41 ` [PATCH v3 12/19] media: i2c: rdacm21: Fix OV10640 powerup Jacopo Mondi
2021-03-19 16:41 ` [PATCH v3 13/19] media: i2c: rdacm21: Power up OV10640 before OV490 Jacopo Mondi
2021-03-20 16:14 ` Laurent Pinchart
2021-03-26 11:07 ` Jacopo Mondi
2021-03-19 16:41 ` [PATCH v3 14/19] media: i2c: rdacm20: Enable noise immunity Jacopo Mondi
2021-03-20 16:16 ` Laurent Pinchart
2021-03-19 16:41 ` [PATCH v3 15/19] media: i2c: rdacm20: Embed 'serializer' field Jacopo Mondi
2021-03-19 16:41 ` [PATCH v3 16/19] media: i2c: rdacm20: Replace goto with a loop Jacopo Mondi
2021-03-20 16:17 ` Laurent Pinchart
2021-03-19 16:41 ` [PATCH v3 17/19] media: i2c: rdacm20: Report camera module name Jacopo Mondi
2021-03-19 16:41 ` [PATCH v3 18/19] media: i2c: rdacm20: Check return values Jacopo Mondi
2021-03-19 16:41 ` [PATCH v3 19/19] media: i2c: rdacm20: Re-work ov10635 reset Jacopo Mondi
2021-03-20 16:24 ` Laurent Pinchart
2021-03-26 11:11 ` Jacopo Mondi
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=YFYYvwChCsF1rPxv@pendragon.ideasonboard.com \
--to=laurent.pinchart@ideasonboard.com \
--cc=geert@linux-m68k.org \
--cc=jacopo+renesas@jmondi.org \
--cc=kieran.bingham+renesas@ideasonboard.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=niklas.soderlund+renesas@ragnatech.se \
/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).