2021-03-19 16:43:04

by Jacopo Mondi

[permalink] [raw]
Subject: [PATCH v3 00/19] media: gmsl: Reliability improvement

Hello,
series following:
https://patchwork.linuxtv.org/project/linux-media/list/?series=4650
https://patchwork.linuxtv.org/project/linux-media/list/?series=4861

Compared to the following iteration it seems RDACM21 is finally fixed \o/

I'll sum-up the issue here from the last email exchange:
-------------------------------------------------------------------------------
Basically, as patch "media: i2c: rdacm21: Fix OV10640 powerdown" of
this series describes, the OV10640 power-up was broken before you
spotted the usage of the wrong gpio pad and it was working because of
an internal pull-up on the SPWDN line, which was erroneously left
floating. Once that was fixed, the OV10640 was always identified
correctly, leaving us with this puzzling "ov490 boot timeout error"
that manifested with more or less the same frequency of the ov10640
identification issue.

In the current implementation we power up the OV490 and wait for its
firmware to boot -before- powering up the ov10640 sensor. Most
probably (or looking at the results I get noaw, most certainly) the
OV490 firmware checks for the sensor to be available and probably
tries to program it. So we're back to the issue we originally had when
the sensor was powered because of the pull up resistor, failing to
boot in case the sensor didn't startup correctly which happened in the
20% of the cases.

If I do power up the OV10640 -before- the OV490 all the firmware boot
errors are now gone. I need to tune a bit the timeouts as after the
OV490 boot the OV10640 requires some time before being accessible.
Once I nail down the right timeouts I'll send v3. So far I got 0
errors on 50 boot attempts, finally \o/
-------------------------------------------------------------------------------

The rest of the series is almost identical a few grammar fixes apart.
Thanks
j

Jacopo Mondi (19):
media: i2c: max9286: Adjust parameters indent
media: i2c: max9286: Rename reverse_channel_mv
media: i2c: max9286: Cache channel amplitude
media: i2c: max9286: Define high channel amplitude
media: v4l2-subdev: De-deprecate init() subdev op
media: gmsl: Reimplement initialization sequence
media: i2c: max9286: Rework comments in .bound()
media: i2c: max9271: Check max9271_write() return
media: i2c: max9271: Introduce wake_up() function
media: i2c: rdamc21: Fix warning on u8 cast
media: i2c: rdacm21: Add dealy after OV490 reset
media: i2c: rdacm21: Fix OV10640 powerup
media: i2c: rdacm21: Power up OV10640 before OV490
media: i2c: rdacm20: Enable noise immunity
media: i2c: rdacm20: Embed 'serializer' field
media: i2c: rdacm20: Replace goto with a loop
media: i2c: rdacm20: Report camera module name
media: i2c: rdacm20: Check return values
media: i2c: rdacm20: Re-work ov10635 reset

drivers/media/i2c/max9271.c | 37 ++++++--
drivers/media/i2c/max9271.h | 9 ++
drivers/media/i2c/max9286.c | 61 ++++++++-----
drivers/media/i2c/rdacm20.c | 174 +++++++++++++++++++++---------------
drivers/media/i2c/rdacm21.c | 117 +++++++++++++++---------
include/media/v4l2-subdev.h | 15 +++-
6 files changed, 268 insertions(+), 145 deletions(-)

--
2.30.0


2021-03-19 16:43:13

by Jacopo Mondi

[permalink] [raw]
Subject: [PATCH v3 01/19] media: i2c: max9286: Adjust parameters indent

The parameters to max9286_i2c_mux_configure() fits on the previous
line. Adjust it.

Cosmetic change only.

Reviewed-by: Kieran Bingham <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
Signed-off-by: Jacopo Mondi <[email protected]>
---
drivers/media/i2c/max9286.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 6fd4d59fcc72..1d9951215868 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -287,9 +287,8 @@ static int max9286_i2c_mux_select(struct i2c_mux_core *muxc, u32 chan)

priv->mux_channel = chan;

- max9286_i2c_mux_configure(priv,
- MAX9286_FWDCCEN(chan) |
- MAX9286_REVCCEN(chan));
+ max9286_i2c_mux_configure(priv, MAX9286_FWDCCEN(chan) |
+ MAX9286_REVCCEN(chan));

return 0;
}
--
2.30.0

2021-03-19 16:43:27

by Jacopo Mondi

[permalink] [raw]
Subject: [PATCH v3 03/19] media: i2c: max9286: Cache channel amplitude

Cache the current channel amplitude in a driver variable
to skip updating it if the newly requested value is the same
as the currently configured one.

Reviewed-by: Kieran Bingham <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
Signed-off-by: Jacopo Mondi <[email protected]>
---
drivers/media/i2c/max9286.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 82ec05e96cb7..ed1cdefe7c30 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -165,6 +165,7 @@ struct max9286_priv {

/* The initial reverse control channel amplitude. */
u32 init_rev_chan_mv;
+ u32 rev_chan_mv;

struct v4l2_ctrl_handler ctrls;
struct v4l2_ctrl *pixelrate;
@@ -341,8 +342,15 @@ static void max9286_configure_i2c(struct max9286_priv *priv, bool localack)
static void max9286_reverse_channel_setup(struct max9286_priv *priv,
unsigned int chan_amplitude)
{
+ u8 chan_config;
+
+ if (priv->rev_chan_mv == chan_amplitude)
+ return;
+
+ priv->rev_chan_mv = chan_amplitude;
+
/* Reverse channel transmission time: default to 1. */
- u8 chan_config = MAX9286_REV_TRF(1);
+ chan_config = MAX9286_REV_TRF(1);

/*
* Reverse channel setup.
@@ -564,8 +572,7 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
* - Disable auto-ack as communication on the control channel are now
* stable.
*/
- if (priv->init_rev_chan_mv < 170)
- max9286_reverse_channel_setup(priv, 170);
+ max9286_reverse_channel_setup(priv, 170);
max9286_check_config_link(priv, priv->source_mask);

/*
--
2.30.0

2021-03-19 16:43:37

by Jacopo Mondi

[permalink] [raw]
Subject: [PATCH v3 04/19] media: i2c: max9286: Define high channel amplitude

Provide a macro to define the reverse channel amplitude to
be used to compensate the remote serializer noise immunity.

While at it, update a comment.

Reviewed-by: Kieran Bingham <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
Signed-off-by: Jacopo Mondi <[email protected]>
---
drivers/media/i2c/max9286.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index ed1cdefe7c30..9124d5fa6ea3 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -113,6 +113,7 @@
#define MAX9286_REV_TRF(n) ((n) << 4)
#define MAX9286_REV_AMP(n) ((((n) - 30) / 10) << 1) /* in mV */
#define MAX9286_REV_AMP_X BIT(0)
+#define MAX9286_REV_AMP_HIGH 170
/* Register 0x3f */
#define MAX9286_EN_REV_CFG BIT(6)
#define MAX9286_REV_FLEN(n) ((n) - 20)
@@ -567,12 +568,12 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
* channels:
*
* - Increase the reverse channel amplitude to compensate for the
- * remote ends high threshold, if not done already
+ * 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, 170);
+ max9286_reverse_channel_setup(priv, MAX9286_REV_AMP_HIGH);
max9286_check_config_link(priv, priv->source_mask);

/*
--
2.30.0

2021-03-19 16:43:37

by Jacopo Mondi

[permalink] [raw]
Subject: [PATCH v3 07/19] media: i2c: max9286: Rework comments in .bound()

Rephrase a comment in .bound() callback to make it clear we register
a subdev notifier and remove a redundant comment about disabling i2c
auto-ack.

No functional changes intended.

Reviewed-by: Kieran Bingham <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
Signed-off-by: Jacopo Mondi <[email protected]>
---
drivers/media/i2c/max9286.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index b6347639901e..16b2cb9b44a2 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -556,9 +556,9 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
subdev->name, src_pad, index);

/*
- * We can only register v4l2_async_notifiers, which do not provide a
- * means to register a complete callback. bound_sources allows us to
- * identify when all remote serializers have completed their probe.
+ * As we register a subdev notifiers we won't get a .complete() callback
+ * here, so we have to use bound_sources to identify when all remote
+ * serializers have probed.
*/
if (priv->bound_sources != priv->source_mask)
return 0;
@@ -581,16 +581,12 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
/*
* All enabled sources have probed and enabled their reverse control
* channels:
+ * - The reverse channel amplitude stays high
* - Verify all configuration links are properly detected
- * - Disable auto-ack as communication on the control channel are now
- * stable.
+ * - Disable auto-ack as communications on the control channel are now
+ * stable
*/
max9286_check_config_link(priv, priv->source_mask);
-
- /*
- * Re-configure I2C with local acknowledge disabled after cameras have
- * probed.
- */
max9286_configure_i2c(priv, false);

return max9286_set_pixelrate(priv);
--
2.30.0

2021-03-19 16:43:37

by Jacopo Mondi

[permalink] [raw]
Subject: [PATCH v3 06/19] media: gmsl: Reimplement initialization sequence

The current probe() procedure of the RDACM20 and RDACM20 GMSL cameras is
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 <[email protected]>
Reviewed-by: Kieran Bingham <[email protected]>
Signed-off-by: Jacopo Mondi <[email protected]>
---
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.
*
--
2.30.0

2021-03-19 16:43:37

by Jacopo Mondi

[permalink] [raw]
Subject: [PATCH v3 05/19] media: v4l2-subdev: De-deprecate init() subdev op

The init() subdev core operation is deemed to be deprecated for new
subdevice drivers. However it could prove useful for complex
architectures to defer operation that require access to the
communication bus if said bus is not available (or fully configured)
at the time when the subdevice probe() function is run.

As an example, the GMSL architecture requires the GMSL configuration
link to be configured on the host side after the remote subdevice
has completed its probe function. After the configuration on the host
side has been performed, the subdevice registers can be accessed through
the communication bus.

In particular:

HOST REMOTE

probe()
|
---------------------> |
probe() {
bus config()
}
|<--------------------|
v4l2 async bound {
bus config()
call subdev init()
|-------------------->|
init() {
access register on the bus()
}
|<-------------------
}

In the GMSL use case the bus configuration requires the enablement of the
noise immunity threshold on the remote side which ensures reliability
of communications in electrically noisy environments. After the subdevice
has enabled the threshold at the end of its probe() sequence the host
side shall compensate it with an higher signal amplitude. Once this
sequence has completed the bus can be accessed with noise protection
enabled and all the operations that require a considerable number of
transactions on the bus (such as the image sensor configuration
sequence) are run in the subdevice init() operation implementation.

Signed-off-by: Jacopo Mondi <[email protected]>
---
include/media/v4l2-subdev.h | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index d0e9a5bdb08b..3068d9940669 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -148,9 +148,18 @@ struct v4l2_subdev_io_pin_config {
* each pin being configured. This function could be called at times
* other than just subdevice initialization.
*
- * @init: initialize the sensor registers to some sort of reasonable default
- * values. Do not use for new drivers and should be removed in existing
- * drivers.
+ * @init: initialize the subdevice registers to some sort of reasonable default
+ * values. Do not use for new drivers (and should be removed in existing
+ * ones) for regular architectures where the image sensor is connected to
+ * the host receiver. For more complex architectures where the subdevice
+ * initialization should be deferred to the completion of the probe
+ * sequence of some intermediate component, or the communication bus
+ * requires configurations on the host side that depend on the completion
+ * of the probe sequence of the remote subdevices, the usage of this
+ * operation could be considered to allow the devices along the pipeline to
+ * probe and register in the media graph and to defer any operation that
+ * require actual access to the communication bus to their init() function
+ * implementation.
*
* @load_fw: load firmware.
*
--
2.30.0

2021-03-19 16:43:40

by Jacopo Mondi

[permalink] [raw]
Subject: [PATCH v3 11/19] media: i2c: rdacm21: Add dealy after OV490 reset

Add a delay after the OV490 chip is put in reset state. The reset
signal shall be held for at least 250 useconds.

Signed-off-by: Jacopo Mondi <[email protected]>
---
drivers/media/i2c/rdacm21.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
index babd14b21252..875bec9f7904 100644
--- a/drivers/media/i2c/rdacm21.c
+++ b/drivers/media/i2c/rdacm21.c
@@ -448,7 +448,10 @@ static int rdacm21_init(struct v4l2_subdev *sd, unsigned int val)
if (ret)
return ret;

- /* Enable GPIO1 and hold OV490 in reset during max9271 configuration. */
+ /*
+ * Enable GPIO1 and hold OV490 in reset during max9271 configuration.
+ * The reset signal has to be asserted for at least 250 useconds.
+ */
ret = max9271_enable_gpios(&dev->serializer, MAX9271_GPIO1OUT);
if (ret)
return ret;
@@ -456,6 +459,7 @@ static int rdacm21_init(struct v4l2_subdev *sd, unsigned int val)
ret = max9271_clear_gpios(&dev->serializer, MAX9271_GPIO1OUT);
if (ret)
return ret;
+ usleep_range(250, 500);

ret = max9271_configure_gmsl_link(&dev->serializer);
if (ret)
--
2.30.0

2021-03-19 16:43:41

by Jacopo Mondi

[permalink] [raw]
Subject: [PATCH v3 10/19] media: i2c: rdamc21: Fix warning on u8 cast

Sparse reports a warning on a cast to u8 of a 16 bits constant.

drivers/media/i2c/rdacm21.c:348:62: warning: cast truncates bits
from constant value (300a becomes a)

Even if the behaviour is intended, silence the sparse warning replacing
the cast with a bitwise & operation.

Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
Reported-by: Hans Verkuil <[email protected]>
Reviewed-by: Kieran Bingham <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
Signed-off-by: Jacopo Mondi <[email protected]>
---
drivers/media/i2c/rdacm21.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
index 1706a59bfd58..babd14b21252 100644
--- a/drivers/media/i2c/rdacm21.c
+++ b/drivers/media/i2c/rdacm21.c
@@ -330,7 +330,7 @@ static int ov10640_initialize(struct rdacm21_device *dev)
/* Read OV10640 ID to test communications. */
ov490_write_reg(dev, OV490_SCCB_SLAVE0_DIR, OV490_SCCB_SLAVE_READ);
ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_HIGH, OV10640_CHIP_ID >> 8);
- ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_LOW, (u8)OV10640_CHIP_ID);
+ ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_LOW, OV10640_CHIP_ID & 0xff);

/* Trigger SCCB slave transaction and give it some time to complete. */
ov490_write_reg(dev, OV490_HOST_CMD, OV490_HOST_CMD_TRIGGER);
--
2.30.0

2021-03-19 16:43:41

by Jacopo Mondi

[permalink] [raw]
Subject: [PATCH v3 08/19] media: i2c: max9271: Check max9271_write() return

Check the return value of the max9271_write() function in the
max9271 library driver.

While at it, modify an existing condition to be made identical
to other checks.

Reviewed-by: Kieran Bingham <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
Signed-off-by: Jacopo Mondi <[email protected]>
---
drivers/media/i2c/max9271.c | 30 +++++++++++++++++++++++-------
1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/drivers/media/i2c/max9271.c b/drivers/media/i2c/max9271.c
index c495582dcff6..2c7dc7fb9846 100644
--- a/drivers/media/i2c/max9271.c
+++ b/drivers/media/i2c/max9271.c
@@ -106,7 +106,10 @@ int max9271_set_serial_link(struct max9271_device *dev, bool enable)
* Short delays here appear to show bit-errors in the writes following.
* Therefore a conservative delay seems best here.
*/
- max9271_write(dev, 0x04, val);
+ ret = max9271_write(dev, 0x04, val);
+ if (ret < 0)
+ return ret;
+
usleep_range(5000, 8000);

return 0;
@@ -118,7 +121,7 @@ int max9271_configure_i2c(struct max9271_device *dev, u8 i2c_config)
int ret;

ret = max9271_write(dev, 0x0d, i2c_config);
- if (ret)
+ if (ret < 0)
return ret;

/* The delay required after an I2C bus configuration change is not
@@ -143,7 +146,10 @@ int max9271_set_high_threshold(struct max9271_device *dev, bool enable)
* Enable or disable reverse channel high threshold to increase
* immunity to power supply noise.
*/
- max9271_write(dev, 0x08, enable ? ret | BIT(0) : ret & ~BIT(0));
+ ret = max9271_write(dev, 0x08, enable ? ret | BIT(0) : ret & ~BIT(0));
+ if (ret < 0)
+ return ret;
+
usleep_range(2000, 2500);

return 0;
@@ -152,6 +158,8 @@ EXPORT_SYMBOL_GPL(max9271_set_high_threshold);

int max9271_configure_gmsl_link(struct max9271_device *dev)
{
+ int ret;
+
/*
* Configure the GMSL link:
*
@@ -162,16 +170,24 @@ int max9271_configure_gmsl_link(struct max9271_device *dev)
*
* TODO: Make the GMSL link configuration parametric.
*/
- max9271_write(dev, 0x07, MAX9271_DBL | MAX9271_HVEN |
- MAX9271_EDC_1BIT_PARITY);
+ ret = max9271_write(dev, 0x07, MAX9271_DBL | MAX9271_HVEN |
+ MAX9271_EDC_1BIT_PARITY);
+ if (ret < 0)
+ return ret;
+
usleep_range(5000, 8000);

/*
* Adjust spread spectrum to +4% and auto-detect pixel clock
* and serial link rate.
*/
- max9271_write(dev, 0x02, MAX9271_SPREAD_SPECT_4 | MAX9271_R02_RES |
- MAX9271_PCLK_AUTODETECT | MAX9271_SERIAL_AUTODETECT);
+ ret = max9271_write(dev, 0x02,
+ MAX9271_SPREAD_SPECT_4 | MAX9271_R02_RES |
+ MAX9271_PCLK_AUTODETECT |
+ MAX9271_SERIAL_AUTODETECT);
+ if (ret < 0)
+ return ret;
+
usleep_range(5000, 8000);

return 0;
--
2.30.0

2021-03-19 16:43:41

by Jacopo Mondi

[permalink] [raw]
Subject: [PATCH v3 09/19] media: i2c: max9271: Introduce wake_up() function

The MAX9271 chip manual prescribes a delay of 5 milliseconds
after the chip exits from low power state.

Add a new function to the max9271 library driver that wakes up the chip
with a dummy i2c transaction and implements the correct delay of 5
milliseconds after the chip exits from low power state.

Use the newly introduced function in the rdacm20 and rdacm21 camera
drivers. The former was not respecting the required delay while the
latter was waiting for a too-short timeout.

Do not handle the initial i2c address configuration in the library
driver function as the camera module drivers control address
reprogramming.

Reviewed-by: Kieran Bingham <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
Signed-off-by: Jacopo Mondi <[email protected]>
---
drivers/media/i2c/max9271.c | 7 +++++++
drivers/media/i2c/max9271.h | 9 +++++++++
drivers/media/i2c/rdacm20.c | 2 +-
drivers/media/i2c/rdacm21.c | 3 +--
4 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/max9271.c b/drivers/media/i2c/max9271.c
index 2c7dc7fb9846..f7bfe7266763 100644
--- a/drivers/media/i2c/max9271.c
+++ b/drivers/media/i2c/max9271.c
@@ -80,6 +80,13 @@ static int max9271_pclk_detect(struct max9271_device *dev)
return -EIO;
}

+void max9271_wake_up(struct max9271_device *dev)
+{
+ i2c_smbus_read_byte(dev->client);
+ usleep_range(5000, 8000);
+}
+EXPORT_SYMBOL_GPL(max9271_wake_up);
+
int max9271_set_serial_link(struct max9271_device *dev, bool enable)
{
int ret;
diff --git a/drivers/media/i2c/max9271.h b/drivers/media/i2c/max9271.h
index d78fb21441e9..dc5e4e70ba6f 100644
--- a/drivers/media/i2c/max9271.h
+++ b/drivers/media/i2c/max9271.h
@@ -85,6 +85,15 @@ struct max9271_device {
struct i2c_client *client;
};

+/**
+ * max9271_wake_up() - Wake up the serializer by issuing an i2c transaction
+ * @dev: The max9271 device
+ *
+ * This function shall be called before any other interaction with the
+ * serializer.
+ */
+void max9271_wake_up(struct max9271_device *dev);
+
/**
* max9271_set_serial_link() - Enable/disable serial link
* @dev: The max9271 device
diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index 3cab6af7eba6..2881e752efbe 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -547,7 +547,7 @@ static int rdacm20_initialize(struct rdacm20_device *dev)

/* Verify communication with the MAX9271: ping to wakeup. */
dev->serializer->client->addr = MAX9271_DEFAULT_ADDR;
- i2c_smbus_read_byte(dev->serializer->client);
+ max9271_wake_up(dev->serializer);

/* Serial link disabled during config as it needs a valid pixel clock. */
ret = max9271_set_serial_link(dev->serializer, false);
diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
index 318a3b40d9e6..1706a59bfd58 100644
--- a/drivers/media/i2c/rdacm21.c
+++ b/drivers/media/i2c/rdacm21.c
@@ -511,8 +511,7 @@ static int rdacm21_initialize(struct rdacm21_device *dev)

/* 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);
+ max9271_wake_up(&dev->serializer);

/* Enable reverse channel and disable the serial link. */
ret = max9271_set_serial_link(&dev->serializer, false);
--
2.30.0

2021-03-19 16:43:42

by Jacopo Mondi

[permalink] [raw]
Subject: [PATCH v3 12/19] media: i2c: rdacm21: Fix OV10640 powerup

The OV10640 image sensor powerdown signal is controlled by the first
line of the OV490 GPIO pad #1, but the pad #0 identifier
OV490_GPIO_OUTPUT_VALUE0 was erroneously used. As a result the image
sensor powerdown signal was never asserted but was left floating and
kept high by an internal pull-up resistor, causing sporadic failures
during the image sensor startup phase.

Fix this by using the correct GPIO pad identifier and wait the mandatory
1.5 millisecond delay after the powerup lane is asserted. The reset
delay is not characterized in the chip manual if not as "255 XVCLK +
initialization". Wait for at least 3 milliseconds to guarantee the SCCB
bus is available.

While at it also fix the reset sequence, as the reset line was released
before the powerdown one, and the line was not cycled.

This commit fixes a sporadic start-up error triggered by a failure to
read the OV10640 chip ID:
rdacm21 8-0054: OV10640 ID mismatch: (0x01)

Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
Reviewed-by: Kieran Bingham <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
Signed-off-by: Jacopo Mondi <[email protected]>
---
drivers/media/i2c/rdacm21.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
index 875bec9f7904..3f38c465b348 100644
--- a/drivers/media/i2c/rdacm21.c
+++ b/drivers/media/i2c/rdacm21.c
@@ -318,13 +318,19 @@ static int ov10640_initialize(struct rdacm21_device *dev)
{
u8 val;

- /* Power-up OV10640 by setting RESETB and PWDNB pins high. */
+ /* Enable GPIO0#0 (reset) and GPIO1#0 (pwdn) as output lines. */
ov490_write_reg(dev, OV490_GPIO_SEL0, OV490_GPIO0);
ov490_write_reg(dev, OV490_GPIO_SEL1, OV490_SPWDN0);
ov490_write_reg(dev, OV490_GPIO_DIRECTION0, OV490_GPIO0);
ov490_write_reg(dev, OV490_GPIO_DIRECTION1, OV490_SPWDN0);
+
+ /* Power up OV10640 and then reset it. */
+ ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE1, OV490_SPWDN0);
+ usleep_range(1500, 3000);
+
+ ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, 0x00);
+ usleep_range(1500, 3000);
ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0);
- ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_SPWDN0);
usleep_range(3000, 5000);

/* Read OV10640 ID to test communications. */
--
2.30.0

2021-03-19 16:44:10

by Jacopo Mondi

[permalink] [raw]
Subject: [PATCH v3 14/19] media: i2c: rdacm20: Enable noise immunity

Enable the noise immunity threshold at the end of the rdacm20
initialization routine.

The rdacm20 camera module has been so far tested with a startup
delay that allowed the embedded MCU to program the serializer. If
the initialization routine is run before the MCU programs the
serializer and the image sensor and their addresses gets changed
by the rdacm20 driver it is required to manually enable the noise
immunity threshold to make the communication on the control channel
more reliable.

Reviewed-by: Kieran Bingham <[email protected]>
Signed-off-by: Jacopo Mondi <[email protected]>
---
drivers/media/i2c/rdacm20.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index 2881e752efbe..554fd3b3ec3d 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -518,7 +518,19 @@ static int rdacm20_init(struct v4l2_subdev *sd, unsigned int val)

dev_info(dev->dev, "Identified MAX9271 + OV10635 device\n");

- return 0;
+ /*
+ * Set reverse channel high threshold to increase noise immunity.
+ *
+ * This should be compensated by increasing the reverse channel
+ * amplitude on the remote deserializer side.
+ *
+ * TODO Inspect the embedded MCU programming sequence to make sure
+ * there are no conflicts with the configuration applied here.
+ *
+ * TODO Clarify the embedded MCU startup delay to avoid write
+ * collisions on the I2C bus.
+ */
+ return max9271_set_high_threshold(&dev->serializer, true);
}

static const struct v4l2_subdev_core_ops rdacm20_core_ops = {
--
2.30.0

2021-03-19 16:44:12

by Jacopo Mondi

[permalink] [raw]
Subject: [PATCH v3 15/19] media: i2c: rdacm20: Embed 'serializer' field

There's no reason to allocate dynamically the 'serializer' field in
the driver structure.

Embed the field and adjust all its users in the driver.

Reviewed-by: Kieran Bingham <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
Signed-off-by: Jacopo Mondi <[email protected]>
---
drivers/media/i2c/rdacm20.c | 40 ++++++++++++++++---------------------
1 file changed, 17 insertions(+), 23 deletions(-)

diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index 554fd3b3ec3d..7bdcfafa6c10 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -312,7 +312,7 @@ static const struct ov10635_reg {

struct rdacm20_device {
struct device *dev;
- struct max9271_device *serializer;
+ struct max9271_device serializer;
struct i2c_client *sensor;
struct v4l2_subdev sd;
struct media_pad pad;
@@ -399,7 +399,7 @@ 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);
+ return max9271_set_serial_link(&dev->serializer, enable);
}

static int rdacm20_enum_mbus_code(struct v4l2_subdev *sd,
@@ -445,35 +445,35 @@ static int rdacm20_init(struct v4l2_subdev *sd, unsigned int val)
* Ensure that we have a good link configuration before attempting to
* identify the device.
*/
- max9271_configure_i2c(dev->serializer, MAX9271_I2CSLVSH_469NS_234NS |
- MAX9271_I2CSLVTO_1024US |
- MAX9271_I2CMSTBT_105KBPS);
+ max9271_configure_i2c(&dev->serializer, MAX9271_I2CSLVSH_469NS_234NS |
+ MAX9271_I2CSLVTO_1024US |
+ MAX9271_I2CMSTBT_105KBPS);

- max9271_configure_gmsl_link(dev->serializer);
+ max9271_configure_gmsl_link(&dev->serializer);

- ret = max9271_verify_id(dev->serializer);
+ ret = max9271_verify_id(&dev->serializer);
if (ret < 0)
return ret;

- ret = max9271_set_address(dev->serializer, dev->addrs[0]);
+ ret = max9271_set_address(&dev->serializer, dev->addrs[0]);
if (ret < 0)
return ret;
- dev->serializer->client->addr = dev->addrs[0];
+ dev->serializer.client->addr = dev->addrs[0];

/*
* Reset the sensor by cycling the OV10635 reset signal connected to the
* MAX9271 GPIO1 and verify communication with the OV10635.
*/
- ret = max9271_enable_gpios(dev->serializer, MAX9271_GPIO1OUT);
+ ret = max9271_enable_gpios(&dev->serializer, MAX9271_GPIO1OUT);
if (ret)
return ret;

- ret = max9271_clear_gpios(dev->serializer, MAX9271_GPIO1OUT);
+ ret = max9271_clear_gpios(&dev->serializer, MAX9271_GPIO1OUT);
if (ret)
return ret;
usleep_range(10000, 15000);

- ret = max9271_set_gpios(dev->serializer, MAX9271_GPIO1OUT);
+ ret = max9271_set_gpios(&dev->serializer, MAX9271_GPIO1OUT);
if (ret)
return ret;
usleep_range(10000, 15000);
@@ -558,11 +558,11 @@ 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;
- max9271_wake_up(dev->serializer);
+ dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
+ max9271_wake_up(&dev->serializer);

/* Serial link disabled during config as it needs a valid pixel clock. */
- ret = max9271_set_serial_link(dev->serializer, false);
+ ret = max9271_set_serial_link(&dev->serializer, false);
if (ret)
return ret;

@@ -572,7 +572,7 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
* This should be compensated by increasing the reverse channel
* amplitude on the remote deserializer side.
*/
- return max9271_set_high_threshold(dev->serializer, true);
+ return max9271_set_high_threshold(&dev->serializer, true);
}

static int rdacm20_probe(struct i2c_client *client)
@@ -585,13 +585,7 @@ static int rdacm20_probe(struct i2c_client *client)
if (!dev)
return -ENOMEM;
dev->dev = &client->dev;
-
- dev->serializer = devm_kzalloc(&client->dev, sizeof(*dev->serializer),
- GFP_KERNEL);
- if (!dev->serializer)
- return -ENOMEM;
-
- dev->serializer->client = client;
+ dev->serializer.client = client;

ret = of_property_read_u32_array(client->dev.of_node, "reg",
dev->addrs, 2);
--
2.30.0

2021-03-19 16:44:46

by Jacopo Mondi

[permalink] [raw]
Subject: [PATCH v3 17/19] media: i2c: rdacm20: Report camera module name

When the device is identified the driver currently reports the
names of the chips embedded in the camera module.

Report the name of the camera module itself instead.
Cosmetic change only.

Reviewed-by: Kieran Bingham <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
Signed-off-by: Jacopo Mondi <[email protected]>
---
drivers/media/i2c/rdacm20.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index 760705dd2918..d3a2520c4113 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -513,7 +513,7 @@ static int rdacm20_init(struct v4l2_subdev *sd, unsigned int val)
if (ret)
return ret;

- dev_info(dev->dev, "Identified MAX9271 + OV10635 device\n");
+ dev_info(dev->dev, "Identified RDACM20 camera module\n");

/*
* Set reverse channel high threshold to increase noise immunity.
--
2.30.0

2021-03-19 16:44:49

by Jacopo Mondi

[permalink] [raw]
Subject: [PATCH v3 19/19] media: i2c: rdacm20: Re-work ov10635 reset

The OV10635 image sensor embedded in the camera module is currently
reset after the MAX9271 initialization with two long delays that were
most probably not correctly characterized.

Re-work the image sensor reset procedure by holding the chip in reset
during the MAX9271 configuration, removing the long sleep delays and
only wait after the chip exits from reset for 350-500 microseconds
interval, which is larger than the minimum (2048 * (1 / XVCLK)) timeout
characterized in the chip manual.

Reviewed-by: Kieran Bingham <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
Signed-off-by: Jacopo Mondi <[email protected]>
---
drivers/media/i2c/rdacm20.c | 29 +++++++++++++++++------------
1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index 7ed3866b5335..7ba2d0249da8 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -454,6 +454,19 @@ static int rdacm20_init(struct v4l2_subdev *sd, unsigned int val)
if (ret)
return ret;

+ /*
+ * Hold OV10635 in reset during max9271 configuration. The reset signal
+ * has to be asserted for at least 200 microseconds.
+ */
+ ret = max9271_enable_gpios(&dev->serializer, MAX9271_GPIO1OUT);
+ if (ret)
+ return ret;
+
+ ret = max9271_clear_gpios(&dev->serializer, MAX9271_GPIO1OUT);
+ if (ret)
+ return ret;
+ usleep_range(200, 500);
+
ret = max9271_configure_gmsl_link(&dev->serializer);
if (ret)
return ret;
@@ -468,22 +481,14 @@ static int rdacm20_init(struct v4l2_subdev *sd, unsigned int val)
dev->serializer.client->addr = dev->addrs[0];

/*
- * Reset the sensor by cycling the OV10635 reset signal connected to the
- * MAX9271 GPIO1 and verify communication with the OV10635.
+ * Release ov10635 from reset and initialize it. The image sensor
+ * requires at least 2048 XVCLK cycles (85 micro-seconds at 24MHz)
+ * before being available. Stay safe and wait up to 500 micro-seconds.
*/
- ret = max9271_enable_gpios(&dev->serializer, MAX9271_GPIO1OUT);
- if (ret)
- return ret;
-
- ret = max9271_clear_gpios(&dev->serializer, MAX9271_GPIO1OUT);
- if (ret)
- return ret;
- usleep_range(10000, 15000);
-
ret = max9271_set_gpios(&dev->serializer, MAX9271_GPIO1OUT);
if (ret)
return ret;
- usleep_range(10000, 15000);
+ usleep_range(100, 500);

for (i = 0; i < OV10635_PID_TIMEOUT; ++i) {
ret = ov10635_read16(dev, OV10635_PID);
--
2.30.0

2021-03-19 16:44:49

by Jacopo Mondi

[permalink] [raw]
Subject: [PATCH v3 02/19] media: i2c: max9286: Rename reverse_channel_mv

Rename the reverse_channel_mv variable to init_rev_chan_mv as
the next patch will cache the reverse channel amplitude in
a new driver variable.

Reviewed-by: Kieran Bingham <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
Signed-off-by: Jacopo Mondi <[email protected]>
---
drivers/media/i2c/max9286.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 1d9951215868..82ec05e96cb7 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -163,7 +163,8 @@ struct max9286_priv {
unsigned int mux_channel;
bool mux_open;

- u32 reverse_channel_mv;
+ /* The initial reverse control channel amplitude. */
+ u32 init_rev_chan_mv;

struct v4l2_ctrl_handler ctrls;
struct v4l2_ctrl *pixelrate;
@@ -563,7 +564,7 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
* - Disable auto-ack as communication on the control channel are now
* stable.
*/
- if (priv->reverse_channel_mv < 170)
+ if (priv->init_rev_chan_mv < 170)
max9286_reverse_channel_setup(priv, 170);
max9286_check_config_link(priv, priv->source_mask);

@@ -971,7 +972,7 @@ static int max9286_setup(struct max9286_priv *priv)
* only. This should be disabled after the mux is initialised.
*/
max9286_configure_i2c(priv, true);
- max9286_reverse_channel_setup(priv, priv->reverse_channel_mv);
+ max9286_reverse_channel_setup(priv, priv->init_rev_chan_mv);

/*
* Enable GMSL links, mask unused ones and autodetect link
@@ -1236,9 +1237,9 @@ static int max9286_parse_dt(struct max9286_priv *priv)
if (of_property_read_u32(dev->of_node,
"maxim,reverse-channel-microvolt",
&reverse_channel_microvolt))
- priv->reverse_channel_mv = 170;
+ priv->init_rev_chan_mv = 170;
else
- priv->reverse_channel_mv = reverse_channel_microvolt / 1000U;
+ priv->init_rev_chan_mv = reverse_channel_microvolt / 1000U;

priv->route_mask = priv->source_mask;

--
2.30.0

2021-03-19 16:45:43

by Jacopo Mondi

[permalink] [raw]
Subject: [PATCH v3 13/19] media: i2c: rdacm21: Power up OV10640 before OV490

The current RDACM21 initialization routine powers up the
OV10640 image sensor after the OV490 ISP. The ISP is programmed with
a firmware loaded from an embedded EEPROM that (most probably) tries
to interact and program also the image sensor connected to the ISP.

As described in commit ccb26c5742f5 ("media: i2c: rdacm21: Fix OV10640
powerup") the image sensor powerdown signal is kept high by an internal
pull up resistor and occasionally fails to startup correctly if the
powerdown line is not asserted explicitly. Failures in the OV10640
startup causes the OV490 firmware to fail to boot correctly resulting in
the camera module initialization to fail consequentially.

Fix this by powering up the OV10640 image sensor before testing the
OV490 firmware boot completion, by splitting the ov10640_initialize()
function in an ov10640_power_up() one and an ov10640_check_id() one.

Also make sure the OV10640 identification procedure gives enough time to
the image sensor to resume after the programming phase performed by the
OV490 firmware by repeating the ID read procedure,

This commit fixes a sporadic start-up error triggered by a failure to
detect the OV490 firmware boot completion:
rdacm21 8-0054: Timeout waiting for firmware boot

Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
Signed-off-by: Jacopo Mondi <[email protected]>
---
drivers/media/i2c/rdacm21.c | 46 ++++++++++++++++++++++++++-----------
1 file changed, 32 insertions(+), 14 deletions(-)

diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
index 3f38c465b348..3763eb690d74 100644
--- a/drivers/media/i2c/rdacm21.c
+++ b/drivers/media/i2c/rdacm21.c
@@ -69,6 +69,7 @@
#define OV490_ISP_VSIZE_LOW 0x80820062
#define OV490_ISP_VSIZE_HIGH 0x80820063

+#define OV10640_PID_TIMEOUT 20
#define OV10640_ID_HIGH 0xa6
#define OV10640_CHIP_ID 0x300a
#define OV10640_PIXEL_RATE 55000000
@@ -314,10 +315,8 @@ static int rdacm21_get_fmt(struct v4l2_subdev *sd,
return 0;
}

-static int ov10640_initialize(struct rdacm21_device *dev)
+static void ov10640_power_up(struct rdacm21_device *dev)
{
- u8 val;
-
/* Enable GPIO0#0 (reset) and GPIO1#0 (pwdn) as output lines. */
ov490_write_reg(dev, OV490_GPIO_SEL0, OV490_GPIO0);
ov490_write_reg(dev, OV490_GPIO_SEL1, OV490_SPWDN0);
@@ -332,18 +331,35 @@ static int ov10640_initialize(struct rdacm21_device *dev)
usleep_range(1500, 3000);
ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0);
usleep_range(3000, 5000);
+}

- /* Read OV10640 ID to test communications. */
- ov490_write_reg(dev, OV490_SCCB_SLAVE0_DIR, OV490_SCCB_SLAVE_READ);
- ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_HIGH, OV10640_CHIP_ID >> 8);
- ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_LOW, OV10640_CHIP_ID & 0xff);
-
- /* Trigger SCCB slave transaction and give it some time to complete. */
- ov490_write_reg(dev, OV490_HOST_CMD, OV490_HOST_CMD_TRIGGER);
- usleep_range(1000, 1500);
+static int ov10640_check_id(struct rdacm21_device *dev)
+{
+ unsigned int i;
+ u8 val;

- ov490_read_reg(dev, OV490_SCCB_SLAVE0_DIR, &val);
- if (val != OV10640_ID_HIGH) {
+ /* Read OV10640 ID to test communications. */
+ for (i = 0; i < OV10640_PID_TIMEOUT; ++i) {
+ ov490_write_reg(dev, OV490_SCCB_SLAVE0_DIR,
+ OV490_SCCB_SLAVE_READ);
+ ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_HIGH,
+ OV10640_CHIP_ID >> 8);
+ ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_LOW,
+ OV10640_CHIP_ID & 0xff);
+
+ /*
+ * Trigger SCCB slave transaction and give it some time
+ * to complete.
+ */
+ ov490_write_reg(dev, OV490_HOST_CMD, OV490_HOST_CMD_TRIGGER);
+ usleep_range(1000, 1500);
+
+ ov490_read_reg(dev, OV490_SCCB_SLAVE0_DIR, &val);
+ if (val == OV10640_ID_HIGH)
+ break;
+ usleep_range(1000, 1500);
+ }
+ if (i == OV10640_PID_TIMEOUT) {
dev_err(dev->dev, "OV10640 ID mismatch: (0x%02x)\n", val);
return -ENODEV;
}
@@ -359,6 +375,8 @@ static int ov490_initialize(struct rdacm21_device *dev)
unsigned int i;
int ret;

+ ov10640_power_up(dev);
+
/*
* Read OV490 Id to test communications. Give it up to 40msec to
* exit from reset.
@@ -396,7 +414,7 @@ static int ov490_initialize(struct rdacm21_device *dev)
return -ENODEV;
}

- ret = ov10640_initialize(dev);
+ ret = ov10640_check_id(dev);
if (ret)
return ret;

--
2.30.0

2021-03-19 16:46:06

by Jacopo Mondi

[permalink] [raw]
Subject: [PATCH v3 16/19] media: i2c: rdacm20: Replace goto with a loop

During the camera module initialization the image sensor PID is read to
verify it can correctly be identified. The current implementation is
rather confused and uses a loop implemented with a label and a goto.

Replace it with a more compact for() loop.

No functional changes intended.

Reviewed-by: Kieran Bingham <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
Signed-off-by: Jacopo Mondi <[email protected]>
---
drivers/media/i2c/rdacm20.c | 29 +++++++++++++----------------
1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index 7bdcfafa6c10..760705dd2918 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -59,6 +59,8 @@
*/
#define OV10635_PIXEL_RATE (44000000)

+#define OV10635_PID_TIMEOUT 3
+
static const struct ov10635_reg {
u16 reg;
u8 val;
@@ -438,7 +440,7 @@ static int rdacm20_get_fmt(struct v4l2_subdev *sd,
static int rdacm20_init(struct v4l2_subdev *sd, unsigned int val)
{
struct rdacm20_device *dev = sd_to_rdacm20(sd);
- unsigned int retry = 3;
+ unsigned int i;
int ret;

/*
@@ -478,23 +480,18 @@ static int rdacm20_init(struct v4l2_subdev *sd, unsigned int val)
return ret;
usleep_range(10000, 15000);

-again:
- ret = ov10635_read16(dev, OV10635_PID);
- if (ret < 0) {
- if (retry--)
- goto again;
+ for (i = 0; i < OV10635_PID_TIMEOUT; ++i) {
+ ret = ov10635_read16(dev, OV10635_PID);
+ if (ret == OV10635_VERSION)
+ break;
+ else if (ret >= 0)
+ /* Sometimes we get a successful read but a wrong ID. */
+ dev_dbg(dev->dev, "OV10635 ID mismatch (%d)\n", ret);

- dev_err(dev->dev, "OV10635 ID read failed (%d)\n",
- ret);
- return -ENXIO;
+ usleep_range(1000, 2000);
}
-
- if (ret != OV10635_VERSION) {
- if (retry--)
- goto again;
-
- dev_err(dev->dev, "OV10635 ID mismatch (0x%04x)\n",
- ret);
+ if (i == OV10635_PID_TIMEOUT) {
+ dev_err(dev->dev, "OV10635 ID read failed (%d)\n", ret);
return -ENXIO;
}

--
2.30.0

2021-03-19 16:46:48

by Jacopo Mondi

[permalink] [raw]
Subject: [PATCH v3 18/19] media: i2c: rdacm20: Check return values

The camera module initialization routine does not check the return
value of a few functions. Fix that.

Reviewed-by: Kieran Bingham <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
Signed-off-by: Jacopo Mondi <[email protected]>
---
drivers/media/i2c/rdacm20.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
index d3a2520c4113..7ed3866b5335 100644
--- a/drivers/media/i2c/rdacm20.c
+++ b/drivers/media/i2c/rdacm20.c
@@ -447,11 +447,16 @@ static int rdacm20_init(struct v4l2_subdev *sd, unsigned int val)
* Ensure that we have a good link configuration before attempting to
* identify the device.
*/
- max9271_configure_i2c(&dev->serializer, MAX9271_I2CSLVSH_469NS_234NS |
- MAX9271_I2CSLVTO_1024US |
- MAX9271_I2CMSTBT_105KBPS);
+ ret = max9271_configure_i2c(&dev->serializer,
+ MAX9271_I2CSLVSH_469NS_234NS |
+ MAX9271_I2CSLVTO_1024US |
+ MAX9271_I2CMSTBT_105KBPS);
+ if (ret)
+ return ret;

- max9271_configure_gmsl_link(&dev->serializer);
+ ret = max9271_configure_gmsl_link(&dev->serializer);
+ if (ret)
+ return ret;

ret = max9271_verify_id(&dev->serializer);
if (ret < 0)
--
2.30.0

2021-03-19 16:51:37

by Kieran Bingham

[permalink] [raw]
Subject: Re: [PATCH v3 11/19] media: i2c: rdacm21: Add dealy after OV490 reset

Hi Jacopo,

s/dealy/delay/ in $SUBJECT

On 19/03/2021 16:41, Jacopo Mondi wrote:
> Add a delay after the OV490 chip is put in reset state. The reset
> signal shall be held for at least 250 useconds.
>
> Signed-off-by: Jacopo Mondi <[email protected]>
> ---
> drivers/media/i2c/rdacm21.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> index babd14b21252..875bec9f7904 100644
> --- a/drivers/media/i2c/rdacm21.c
> +++ b/drivers/media/i2c/rdacm21.c
> @@ -448,7 +448,10 @@ static int rdacm21_init(struct v4l2_subdev *sd, unsigned int val)
> if (ret)
> return ret;
>
> - /* Enable GPIO1 and hold OV490 in reset during max9271 configuration. */
> + /*
> + * Enable GPIO1 and hold OV490 in reset during max9271 configuration.
> + * The reset signal has to be asserted for at least 250 useconds.
> + */
> ret = max9271_enable_gpios(&dev->serializer, MAX9271_GPIO1OUT);
> if (ret)
> return ret;
> @@ -456,6 +459,7 @@ static int rdacm21_init(struct v4l2_subdev *sd, unsigned int val)
> ret = max9271_clear_gpios(&dev->serializer, MAX9271_GPIO1OUT);
> if (ret)
> return ret;
> + usleep_range(250, 500);

Aha, for a second there I thought the comment meant it had to be
asserted for 250 uS before clearing it again. But it's 250 uS before
using the OV490.

Perhaps possible to update the comment a little, but nothing that matters.

Reviewed-by: Kieran Bingham <[email protected]>


> ret = max9271_configure_gmsl_link(&dev->serializer);
> if (ret)
>

2021-03-20 15:44:40

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH v3 05/19] media: v4l2-subdev: De-deprecate init() subdev op

Hi Jacopo,

Thank you for the patch.

CC'ing Sakari on v3 to get feedback.

On Fri, Mar 19, 2021 at 05:41:34PM +0100, Jacopo Mondi wrote:
> The init() subdev core operation is deemed to be deprecated for new
> subdevice drivers. However it could prove useful for complex
> architectures to defer operation that require access to the
> communication bus if said bus is not available (or fully configured)
> at the time when the subdevice probe() function is run.
>
> As an example, the GMSL architecture requires the GMSL configuration
> link to be configured on the host side after the remote subdevice
> has completed its probe function. After the configuration on the host
> side has been performed, the subdevice registers can be accessed through
> the communication bus.
>
> In particular:
>
> HOST REMOTE
>
> probe()
> |
> ---------------------> |
> probe() {
> bus config()
> }
> |<--------------------|
> v4l2 async bound {
> bus config()
> call subdev init()
> |-------------------->|
> init() {
> access register on the bus()
> }
> |<-------------------
> }
>
> In the GMSL use case the bus configuration requires the enablement of the
> noise immunity threshold on the remote side which ensures reliability
> of communications in electrically noisy environments. After the subdevice
> has enabled the threshold at the end of its probe() sequence the host
> side shall compensate it with an higher signal amplitude. Once this
> sequence has completed the bus can be accessed with noise protection
> enabled and all the operations that require a considerable number of
> transactions on the bus (such as the image sensor configuration
> sequence) are run in the subdevice init() operation implementation.
>
> Signed-off-by: Jacopo Mondi <[email protected]>
> ---
> include/media/v4l2-subdev.h | 15 ++++++++++++---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index d0e9a5bdb08b..3068d9940669 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -148,9 +148,18 @@ struct v4l2_subdev_io_pin_config {
> * each pin being configured. This function could be called at times
> * other than just subdevice initialization.
> *
> - * @init: initialize the sensor registers to some sort of reasonable default
> - * values. Do not use for new drivers and should be removed in existing
> - * drivers.
> + * @init: initialize the subdevice registers to some sort of reasonable default
> + * values. Do not use for new drivers (and should be removed in existing
> + * ones) for regular architectures where the image sensor is connected to
> + * the host receiver. For more complex architectures where the subdevice
> + * initialization should be deferred to the completion of the probe
> + * sequence of some intermediate component, or the communication bus
> + * requires configurations on the host side that depend on the completion
> + * of the probe sequence of the remote subdevices, the usage of this
> + * operation could be considered to allow the devices along the pipeline to
> + * probe and register in the media graph and to defer any operation that
> + * require actual access to the communication bus to their init() function
> + * implementation.
> *
> * @load_fw: load firmware.
> *

--
Regards,

Laurent Pinchart

2021-03-20 15:50:34

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH v3 06/19] media: gmsl: Reimplement initialization sequence

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 <[email protected]>
> Reviewed-by: Kieran Bingham <[email protected]>
> Signed-off-by: Jacopo Mondi <[email protected]>
> ---
> 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

2021-03-20 15:54:27

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH v3 11/19] media: i2c: rdacm21: Add dealy after OV490 reset

Hi Jacopo,

Thank you for the patch.

On Fri, Mar 19, 2021 at 04:49:44PM +0000, Kieran Bingham wrote:
> Hi Jacopo,
>
> s/dealy/delay/ in $SUBJECT
>
> On 19/03/2021 16:41, Jacopo Mondi wrote:
> > Add a delay after the OV490 chip is put in reset state. The reset
> > signal shall be held for at least 250 useconds.
> >
> > Signed-off-by: Jacopo Mondi <[email protected]>
> > ---
> > drivers/media/i2c/rdacm21.c | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> > index babd14b21252..875bec9f7904 100644
> > --- a/drivers/media/i2c/rdacm21.c
> > +++ b/drivers/media/i2c/rdacm21.c
> > @@ -448,7 +448,10 @@ static int rdacm21_init(struct v4l2_subdev *sd, unsigned int val)
> > if (ret)
> > return ret;
> >
> > - /* Enable GPIO1 and hold OV490 in reset during max9271 configuration. */
> > + /*
> > + * Enable GPIO1 and hold OV490 in reset during max9271 configuration.
> > + * The reset signal has to be asserted for at least 250 useconds.
> > + */
> > ret = max9271_enable_gpios(&dev->serializer, MAX9271_GPIO1OUT);
> > if (ret)
> > return ret;
> > @@ -456,6 +459,7 @@ static int rdacm21_init(struct v4l2_subdev *sd, unsigned int val)
> > ret = max9271_clear_gpios(&dev->serializer, MAX9271_GPIO1OUT);
> > if (ret)
> > return ret;
> > + usleep_range(250, 500);
>
> Aha, for a second there I thought the comment meant it had to be
> asserted for 250 uS before clearing it again. But it's 250 uS before
> using the OV490.
>
> Perhaps possible to update the comment a little, but nothing that matters.

The commit message and comment should match the code, especially given
that I'm not sure here which of the two is actually incorrect. I suspect
the sleep is actually in the wrong location.

> Reviewed-by: Kieran Bingham <[email protected]>
>
>
> > ret = max9271_configure_gmsl_link(&dev->serializer);
> > if (ret)
> >

--
Regards,

Laurent Pinchart

2021-03-20 16:18:01

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH v3 13/19] media: i2c: rdacm21: Power up OV10640 before OV490

Hi Jacopo,

Thank you for the patch.

On Fri, Mar 19, 2021 at 05:41:42PM +0100, Jacopo Mondi wrote:
> The current RDACM21 initialization routine powers up the
> OV10640 image sensor after the OV490 ISP. The ISP is programmed with
> a firmware loaded from an embedded EEPROM that (most probably) tries

It's actually a serial flash, not an EEPROM.

> to interact and program also the image sensor connected to the ISP.
>
> As described in commit ccb26c5742f5 ("media: i2c: rdacm21: Fix OV10640

That commit ID won't be valid anymore once the patches get merged,
unless they are pulled instead of cherry-picked, which is currently not
done in the Linux media subsystem. I'd drop it an only mention the
commit's subject.

> powerup") the image sensor powerdown signal is kept high by an internal
> pull up resistor and occasionally fails to startup correctly if the
> powerdown line is not asserted explicitly. Failures in the OV10640
> startup causes the OV490 firmware to fail to boot correctly resulting in
> the camera module initialization to fail consequentially.
>
> Fix this by powering up the OV10640 image sensor before testing the
> OV490 firmware boot completion, by splitting the ov10640_initialize()
> function in an ov10640_power_up() one and an ov10640_check_id() one.
>
> Also make sure the OV10640 identification procedure gives enough time to
> the image sensor to resume after the programming phase performed by the
> OV490 firmware by repeating the ID read procedure,

s/,/./

>
> This commit fixes a sporadic start-up error triggered by a failure to
> detect the OV490 firmware boot completion:
> rdacm21 8-0054: Timeout waiting for firmware boot
>
> Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
> Signed-off-by: Jacopo Mondi <[email protected]>
> ---
> drivers/media/i2c/rdacm21.c | 46 ++++++++++++++++++++++++++-----------
> 1 file changed, 32 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> index 3f38c465b348..3763eb690d74 100644
> --- a/drivers/media/i2c/rdacm21.c
> +++ b/drivers/media/i2c/rdacm21.c
> @@ -69,6 +69,7 @@
> #define OV490_ISP_VSIZE_LOW 0x80820062
> #define OV490_ISP_VSIZE_HIGH 0x80820063
>
> +#define OV10640_PID_TIMEOUT 20
> #define OV10640_ID_HIGH 0xa6
> #define OV10640_CHIP_ID 0x300a
> #define OV10640_PIXEL_RATE 55000000
> @@ -314,10 +315,8 @@ static int rdacm21_get_fmt(struct v4l2_subdev *sd,
> return 0;
> }
>
> -static int ov10640_initialize(struct rdacm21_device *dev)
> +static void ov10640_power_up(struct rdacm21_device *dev)
> {
> - u8 val;
> -
> /* Enable GPIO0#0 (reset) and GPIO1#0 (pwdn) as output lines. */
> ov490_write_reg(dev, OV490_GPIO_SEL0, OV490_GPIO0);
> ov490_write_reg(dev, OV490_GPIO_SEL1, OV490_SPWDN0);
> @@ -332,18 +331,35 @@ static int ov10640_initialize(struct rdacm21_device *dev)
> usleep_range(1500, 3000);
> ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0);
> usleep_range(3000, 5000);
> +}
>
> - /* Read OV10640 ID to test communications. */
> - ov490_write_reg(dev, OV490_SCCB_SLAVE0_DIR, OV490_SCCB_SLAVE_READ);
> - ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_HIGH, OV10640_CHIP_ID >> 8);
> - ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_LOW, OV10640_CHIP_ID & 0xff);
> -
> - /* Trigger SCCB slave transaction and give it some time to complete. */
> - ov490_write_reg(dev, OV490_HOST_CMD, OV490_HOST_CMD_TRIGGER);
> - usleep_range(1000, 1500);
> +static int ov10640_check_id(struct rdacm21_device *dev)
> +{
> + unsigned int i;
> + u8 val;
>
> - ov490_read_reg(dev, OV490_SCCB_SLAVE0_DIR, &val);
> - if (val != OV10640_ID_HIGH) {
> + /* Read OV10640 ID to test communications. */
> + for (i = 0; i < OV10640_PID_TIMEOUT; ++i) {

OV10640_PID_TIMEOUT is used in this function only, I would have made it
local.

> + ov490_write_reg(dev, OV490_SCCB_SLAVE0_DIR,
> + OV490_SCCB_SLAVE_READ);
> + ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_HIGH,
> + OV10640_CHIP_ID >> 8);
> + ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_LOW,
> + OV10640_CHIP_ID & 0xff);
> +
> + /*
> + * Trigger SCCB slave transaction and give it some time
> + * to complete.
> + */
> + ov490_write_reg(dev, OV490_HOST_CMD, OV490_HOST_CMD_TRIGGER);
> + usleep_range(1000, 1500);

It would make sense to create an ov490_sensor_read() function to
encapsulate all this.

It would also be nicer to poll an OV490 register instead of sleeping
blindly. That likely requires the OV490 application note (referenced in
the datasheet), which we don't have. It may be useful to try and get
hold of that. I still feel there may be something a bit fishy, but way
less than before.

> +
> + ov490_read_reg(dev, OV490_SCCB_SLAVE0_DIR, &val);
> + if (val == OV10640_ID_HIGH)
> + break;
> + usleep_range(1000, 1500);
> + }

Blank line ?

> + if (i == OV10640_PID_TIMEOUT) {
> dev_err(dev->dev, "OV10640 ID mismatch: (0x%02x)\n", val);
> return -ENODEV;
> }
> @@ -359,6 +375,8 @@ static int ov490_initialize(struct rdacm21_device *dev)
> unsigned int i;
> int ret;
>
> + ov10640_power_up(dev);

On top of this series, error handling of OV490 writes would make sense.

> +
> /*
> * Read OV490 Id to test communications. Give it up to 40msec to
> * exit from reset.
> @@ -396,7 +414,7 @@ static int ov490_initialize(struct rdacm21_device *dev)
> return -ENODEV;
> }
>
> - ret = ov10640_initialize(dev);
> + ret = ov10640_check_id(dev);
> if (ret)
> return ret;
>

--
Regards,

Laurent Pinchart

2021-03-20 16:21:56

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH v3 14/19] media: i2c: rdacm20: Enable noise immunity

Hi Jacopo,

Thank you for the patch.

On Fri, Mar 19, 2021 at 05:41:43PM +0100, Jacopo Mondi wrote:
> Enable the noise immunity threshold at the end of the rdacm20
> initialization routine.
>
> The rdacm20 camera module has been so far tested with a startup
> delay that allowed the embedded MCU to program the serializer. If
> the initialization routine is run before the MCU programs the
> serializer and the image sensor and their addresses gets changed
> by the rdacm20 driver it is required to manually enable the noise
> immunity threshold to make the communication on the control channel
> more reliable.
>
> Reviewed-by: Kieran Bingham <[email protected]>
> Signed-off-by: Jacopo Mondi <[email protected]>

Reviewed-by: Laurent Pinchart <[email protected]>

> ---
> drivers/media/i2c/rdacm20.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index 2881e752efbe..554fd3b3ec3d 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -518,7 +518,19 @@ static int rdacm20_init(struct v4l2_subdev *sd, unsigned int val)
>
> dev_info(dev->dev, "Identified MAX9271 + OV10635 device\n");
>
> - return 0;
> + /*
> + * Set reverse channel high threshold to increase noise immunity.
> + *
> + * This should be compensated by increasing the reverse channel
> + * amplitude on the remote deserializer side.
> + *
> + * TODO Inspect the embedded MCU programming sequence to make sure
> + * there are no conflicts with the configuration applied here.
> + *
> + * TODO Clarify the embedded MCU startup delay to avoid write
> + * collisions on the I2C bus.
> + */
> + return max9271_set_high_threshold(&dev->serializer, true);
> }
>
> static const struct v4l2_subdev_core_ops rdacm20_core_ops = {

--
Regards,

Laurent Pinchart

2021-03-20 16:22:57

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH v3 16/19] media: i2c: rdacm20: Replace goto with a loop

Hi Jacopo,

Thank you for the patch.

On Fri, Mar 19, 2021 at 05:41:45PM +0100, Jacopo Mondi wrote:
> During the camera module initialization the image sensor PID is read to
> verify it can correctly be identified. The current implementation is
> rather confused and uses a loop implemented with a label and a goto.
>
> Replace it with a more compact for() loop.
>
> No functional changes intended.
>
> Reviewed-by: Kieran Bingham <[email protected]>
> Reviewed-by: Laurent Pinchart <[email protected]>
> Signed-off-by: Jacopo Mondi <[email protected]>
> ---
> drivers/media/i2c/rdacm20.c | 29 +++++++++++++----------------
> 1 file changed, 13 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index 7bdcfafa6c10..760705dd2918 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -59,6 +59,8 @@
> */
> #define OV10635_PIXEL_RATE (44000000)
>
> +#define OV10635_PID_TIMEOUT 3
> +
> static const struct ov10635_reg {
> u16 reg;
> u8 val;
> @@ -438,7 +440,7 @@ static int rdacm20_get_fmt(struct v4l2_subdev *sd,
> static int rdacm20_init(struct v4l2_subdev *sd, unsigned int val)
> {
> struct rdacm20_device *dev = sd_to_rdacm20(sd);
> - unsigned int retry = 3;
> + unsigned int i;
> int ret;
>
> /*
> @@ -478,23 +480,18 @@ static int rdacm20_init(struct v4l2_subdev *sd, unsigned int val)
> return ret;
> usleep_range(10000, 15000);
>
> -again:
> - ret = ov10635_read16(dev, OV10635_PID);
> - if (ret < 0) {
> - if (retry--)
> - goto again;
> + for (i = 0; i < OV10635_PID_TIMEOUT; ++i) {

As commented on a previous patch, this macro is used here only, I would
have made it local.

> + ret = ov10635_read16(dev, OV10635_PID);
> + if (ret == OV10635_VERSION)
> + break;
> + else if (ret >= 0)
> + /* Sometimes we get a successful read but a wrong ID. */
> + dev_dbg(dev->dev, "OV10635 ID mismatch (%d)\n", ret);
>
> - dev_err(dev->dev, "OV10635 ID read failed (%d)\n",
> - ret);
> - return -ENXIO;
> + usleep_range(1000, 2000);
> }
> -
> - if (ret != OV10635_VERSION) {
> - if (retry--)
> - goto again;
> -
> - dev_err(dev->dev, "OV10635 ID mismatch (0x%04x)\n",
> - ret);

Blank line ?

> + if (i == OV10635_PID_TIMEOUT) {
> + dev_err(dev->dev, "OV10635 ID read failed (%d)\n", ret);
> return -ENXIO;
> }
>

--
Regards,

Laurent Pinchart

2021-03-20 16:26:32

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH v3 19/19] media: i2c: rdacm20: Re-work ov10635 reset

Hi Jacopo,

Thank you for the patch.

On Fri, Mar 19, 2021 at 05:41:48PM +0100, Jacopo Mondi wrote:
> The OV10635 image sensor embedded in the camera module is currently
> reset after the MAX9271 initialization with two long delays that were
> most probably not correctly characterized.
>
> Re-work the image sensor reset procedure by holding the chip in reset
> during the MAX9271 configuration, removing the long sleep delays and
> only wait after the chip exits from reset for 350-500 microseconds
> interval, which is larger than the minimum (2048 * (1 / XVCLK)) timeout
> characterized in the chip manual.
>
> Reviewed-by: Kieran Bingham <[email protected]>
> Reviewed-by: Laurent Pinchart <[email protected]>
> Signed-off-by: Jacopo Mondi <[email protected]>
> ---
> drivers/media/i2c/rdacm20.c | 29 +++++++++++++++++------------
> 1 file changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index 7ed3866b5335..7ba2d0249da8 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -454,6 +454,19 @@ static int rdacm20_init(struct v4l2_subdev *sd, unsigned int val)
> if (ret)
> return ret;
>
> + /*
> + * Hold OV10635 in reset during max9271 configuration. The reset signal
> + * has to be asserted for at least 200 microseconds.
> + */
> + ret = max9271_enable_gpios(&dev->serializer, MAX9271_GPIO1OUT);
> + if (ret)
> + return ret;
> +
> + ret = max9271_clear_gpios(&dev->serializer, MAX9271_GPIO1OUT);

enable and clear is very cnonfusing. How about mimicking the GPIO API,
with direction_input(), direction_output() and set_value() functions ?
It would also be nice if the polarity could be handled in a nicer way.
There's no GPIO request API here, but maybe a max9271_gpio_set_flags() ?

> + if (ret)
> + return ret;
> + usleep_range(200, 500);
> +
> ret = max9271_configure_gmsl_link(&dev->serializer);
> if (ret)
> return ret;
> @@ -468,22 +481,14 @@ static int rdacm20_init(struct v4l2_subdev *sd, unsigned int val)
> dev->serializer.client->addr = dev->addrs[0];
>
> /*
> - * Reset the sensor by cycling the OV10635 reset signal connected to the
> - * MAX9271 GPIO1 and verify communication with the OV10635.
> + * Release ov10635 from reset and initialize it. The image sensor
> + * requires at least 2048 XVCLK cycles (85 micro-seconds at 24MHz)
> + * before being available. Stay safe and wait up to 500 micro-seconds.
> */
> - ret = max9271_enable_gpios(&dev->serializer, MAX9271_GPIO1OUT);
> - if (ret)
> - return ret;
> -
> - ret = max9271_clear_gpios(&dev->serializer, MAX9271_GPIO1OUT);
> - if (ret)
> - return ret;
> - usleep_range(10000, 15000);
> -
> ret = max9271_set_gpios(&dev->serializer, MAX9271_GPIO1OUT);
> if (ret)
> return ret;
> - usleep_range(10000, 15000);
> + usleep_range(100, 500);
>
> for (i = 0; i < OV10635_PID_TIMEOUT; ++i) {
> ret = ov10635_read16(dev, OV10635_PID);

--
Regards,

Laurent Pinchart

2021-03-21 22:54:39

by Sakari Ailus

[permalink] [raw]
Subject: Re: [PATCH v3 05/19] media: v4l2-subdev: De-deprecate init() subdev op

Hi Laurent and Jacopo,

Thanks for cc'ing me.

On Sat, Mar 20, 2021 at 05:42:12PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> CC'ing Sakari on v3 to get feedback.
>
> On Fri, Mar 19, 2021 at 05:41:34PM +0100, Jacopo Mondi wrote:
> > The init() subdev core operation is deemed to be deprecated for new
> > subdevice drivers. However it could prove useful for complex
> > architectures to defer operation that require access to the
> > communication bus if said bus is not available (or fully configured)
> > at the time when the subdevice probe() function is run.
> >
> > As an example, the GMSL architecture requires the GMSL configuration
> > link to be configured on the host side after the remote subdevice
> > has completed its probe function. After the configuration on the host
> > side has been performed, the subdevice registers can be accessed through
> > the communication bus.

What does the remote device's probe do that needs to be done before bus
config on the host side?

Alternatively, could the remote init() work be done at the time streaming
is started?

> >
> > In particular:
> >
> > HOST REMOTE
> >
> > probe()
> > |
> > ---------------------> |
> > probe() {
> > bus config()
> > }
> > |<--------------------|
> > v4l2 async bound {
> > bus config()
> > call subdev init()
> > |-------------------->|
> > init() {
> > access register on the bus()
> > }
> > |<-------------------
> > }
> >
> > In the GMSL use case the bus configuration requires the enablement of the
> > noise immunity threshold on the remote side which ensures reliability
> > of communications in electrically noisy environments. After the subdevice
> > has enabled the threshold at the end of its probe() sequence the host
> > side shall compensate it with an higher signal amplitude. Once this
> > sequence has completed the bus can be accessed with noise protection
> > enabled and all the operations that require a considerable number of
> > transactions on the bus (such as the image sensor configuration
> > sequence) are run in the subdevice init() operation implementation.
> >
> > Signed-off-by: Jacopo Mondi <[email protected]>
> > ---
> > include/media/v4l2-subdev.h | 15 ++++++++++++---
> > 1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index d0e9a5bdb08b..3068d9940669 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -148,9 +148,18 @@ struct v4l2_subdev_io_pin_config {
> > * each pin being configured. This function could be called at times
> > * other than just subdevice initialization.
> > *
> > - * @init: initialize the sensor registers to some sort of reasonable default
> > - * values. Do not use for new drivers and should be removed in existing
> > - * drivers.
> > + * @init: initialize the subdevice registers to some sort of reasonable default
> > + * values. Do not use for new drivers (and should be removed in existing
> > + * ones) for regular architectures where the image sensor is connected to
> > + * the host receiver. For more complex architectures where the subdevice
> > + * initialization should be deferred to the completion of the probe
> > + * sequence of some intermediate component, or the communication bus
> > + * requires configurations on the host side that depend on the completion
> > + * of the probe sequence of the remote subdevices, the usage of this
> > + * operation could be considered to allow the devices along the pipeline to
> > + * probe and register in the media graph and to defer any operation that
> > + * require actual access to the communication bus to their init() function
> > + * implementation.
> > *
> > * @load_fw: load firmware.
> > *

--
Kind regards,

Sakari Ailus

2021-03-22 13:06:27

by jacopo mondi

[permalink] [raw]
Subject: Re: [PATCH v3 05/19] media: v4l2-subdev: De-deprecate init() subdev op

Hi Sakari,

On Sun, Mar 21, 2021 at 10:52:56PM +0200, Sakari Ailus wrote:
> Hi Laurent and Jacopo,
>
> Thanks for cc'ing me.
>
> On Sat, Mar 20, 2021 at 05:42:12PM +0200, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > Thank you for the patch.
> >
> > CC'ing Sakari on v3 to get feedback.
> >
> > On Fri, Mar 19, 2021 at 05:41:34PM +0100, Jacopo Mondi wrote:
> > > The init() subdev core operation is deemed to be deprecated for new
> > > subdevice drivers. However it could prove useful for complex
> > > architectures to defer operation that require access to the
> > > communication bus if said bus is not available (or fully configured)
> > > at the time when the subdevice probe() function is run.
> > >
> > > As an example, the GMSL architecture requires the GMSL configuration
> > > link to be configured on the host side after the remote subdevice
> > > has completed its probe function. After the configuration on the host
> > > side has been performed, the subdevice registers can be accessed through
> > > the communication bus.
>
> What does the remote device's probe do that needs to be done before bus
> config on the host side?

A few lines here below:

In the GMSL use case the bus configuration requires the enablement of the
noise immunity threshold on the remote side which ensures reliability
of communications in electrically noisy environments. After the subdevice
has enabled the threshold at the end of its probe() sequence the host
side shall compensate it with an higher signal amplitude. Once this
sequence has completed the bus can be accessed with noise protection
enabled and all the operations that require a considerable number of
transactions on the bus (such as the image sensor configuration
sequence) are run in the subdevice init() operation implementation.

>
> Alternatively, could the remote init() work be done at the time streaming
> is started?

That would require programing the sensor, the embedded ISP at s_stream
time which would take some time.

I'll take this suggestion into account though and run some more tests.

Thanks
j

>
> > >
> > > In particular:
> > >
> > > HOST REMOTE
> > >
> > > probe()
> > > |
> > > ---------------------> |
> > > probe() {
> > > bus config()
> > > }
> > > |<--------------------|
> > > v4l2 async bound {
> > > bus config()
> > > call subdev init()
> > > |-------------------->|
> > > init() {
> > > access register on the bus()
> > > }
> > > |<-------------------
> > > }
> > >
> > > In the GMSL use case the bus configuration requires the enablement of the
> > > noise immunity threshold on the remote side which ensures reliability
> > > of communications in electrically noisy environments. After the subdevice
> > > has enabled the threshold at the end of its probe() sequence the host
> > > side shall compensate it with an higher signal amplitude. Once this
> > > sequence has completed the bus can be accessed with noise protection
> > > enabled and all the operations that require a considerable number of
> > > transactions on the bus (such as the image sensor configuration
> > > sequence) are run in the subdevice init() operation implementation.
> > >
> > > Signed-off-by: Jacopo Mondi <[email protected]>
> > > ---
> > > include/media/v4l2-subdev.h | 15 ++++++++++++---
> > > 1 file changed, 12 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > index d0e9a5bdb08b..3068d9940669 100644
> > > --- a/include/media/v4l2-subdev.h
> > > +++ b/include/media/v4l2-subdev.h
> > > @@ -148,9 +148,18 @@ struct v4l2_subdev_io_pin_config {
> > > * each pin being configured. This function could be called at times
> > > * other than just subdevice initialization.
> > > *
> > > - * @init: initialize the sensor registers to some sort of reasonable default
> > > - * values. Do not use for new drivers and should be removed in existing
> > > - * drivers.
> > > + * @init: initialize the subdevice registers to some sort of reasonable default
> > > + * values. Do not use for new drivers (and should be removed in existing
> > > + * ones) for regular architectures where the image sensor is connected to
> > > + * the host receiver. For more complex architectures where the subdevice
> > > + * initialization should be deferred to the completion of the probe
> > > + * sequence of some intermediate component, or the communication bus
> > > + * requires configurations on the host side that depend on the completion
> > > + * of the probe sequence of the remote subdevices, the usage of this
> > > + * operation could be considered to allow the devices along the pipeline to
> > > + * probe and register in the media graph and to defer any operation that
> > > + * require actual access to the communication bus to their init() function
> > > + * implementation.
> > > *
> > > * @load_fw: load firmware.
> > > *
>
> --
> Kind regards,
>
> Sakari Ailus

2021-03-26 11:08:47

by jacopo mondi

[permalink] [raw]
Subject: Re: [PATCH v3 13/19] media: i2c: rdacm21: Power up OV10640 before OV490

Hi Laurent,

On Sat, Mar 20, 2021 at 06:14:43PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Fri, Mar 19, 2021 at 05:41:42PM +0100, Jacopo Mondi wrote:
> > The current RDACM21 initialization routine powers up the
> > OV10640 image sensor after the OV490 ISP. The ISP is programmed with
> > a firmware loaded from an embedded EEPROM that (most probably) tries
>
> It's actually a serial flash, not an EEPROM.
>
> > to interact and program also the image sensor connected to the ISP.
> >
> > As described in commit ccb26c5742f5 ("media: i2c: rdacm21: Fix OV10640
>
> That commit ID won't be valid anymore once the patches get merged,
> unless they are pulled instead of cherry-picked, which is currently not
> done in the Linux media subsystem. I'd drop it an only mention the
> commit's subject.
>
> > powerup") the image sensor powerdown signal is kept high by an internal
> > pull up resistor and occasionally fails to startup correctly if the
> > powerdown line is not asserted explicitly. Failures in the OV10640
> > startup causes the OV490 firmware to fail to boot correctly resulting in
> > the camera module initialization to fail consequentially.
> >
> > Fix this by powering up the OV10640 image sensor before testing the
> > OV490 firmware boot completion, by splitting the ov10640_initialize()
> > function in an ov10640_power_up() one and an ov10640_check_id() one.
> >
> > Also make sure the OV10640 identification procedure gives enough time to
> > the image sensor to resume after the programming phase performed by the
> > OV490 firmware by repeating the ID read procedure,
>
> s/,/./
>

I'll rework

> >
> > This commit fixes a sporadic start-up error triggered by a failure to
> > detect the OV490 firmware boot completion:
> > rdacm21 8-0054: Timeout waiting for firmware boot
> >
> > Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
> > Signed-off-by: Jacopo Mondi <[email protected]>
> > ---
> > drivers/media/i2c/rdacm21.c | 46 ++++++++++++++++++++++++++-----------
> > 1 file changed, 32 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> > index 3f38c465b348..3763eb690d74 100644
> > --- a/drivers/media/i2c/rdacm21.c
> > +++ b/drivers/media/i2c/rdacm21.c
> > @@ -69,6 +69,7 @@
> > #define OV490_ISP_VSIZE_LOW 0x80820062
> > #define OV490_ISP_VSIZE_HIGH 0x80820063
> >
> > +#define OV10640_PID_TIMEOUT 20
> > #define OV10640_ID_HIGH 0xa6
> > #define OV10640_CHIP_ID 0x300a
> > #define OV10640_PIXEL_RATE 55000000
> > @@ -314,10 +315,8 @@ static int rdacm21_get_fmt(struct v4l2_subdev *sd,
> > return 0;
> > }
> >
> > -static int ov10640_initialize(struct rdacm21_device *dev)
> > +static void ov10640_power_up(struct rdacm21_device *dev)
> > {
> > - u8 val;
> > -
> > /* Enable GPIO0#0 (reset) and GPIO1#0 (pwdn) as output lines. */
> > ov490_write_reg(dev, OV490_GPIO_SEL0, OV490_GPIO0);
> > ov490_write_reg(dev, OV490_GPIO_SEL1, OV490_SPWDN0);
> > @@ -332,18 +331,35 @@ static int ov10640_initialize(struct rdacm21_device *dev)
> > usleep_range(1500, 3000);
> > ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0);
> > usleep_range(3000, 5000);
> > +}
> >
> > - /* Read OV10640 ID to test communications. */
> > - ov490_write_reg(dev, OV490_SCCB_SLAVE0_DIR, OV490_SCCB_SLAVE_READ);
> > - ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_HIGH, OV10640_CHIP_ID >> 8);
> > - ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_LOW, OV10640_CHIP_ID & 0xff);
> > -
> > - /* Trigger SCCB slave transaction and give it some time to complete. */
> > - ov490_write_reg(dev, OV490_HOST_CMD, OV490_HOST_CMD_TRIGGER);
> > - usleep_range(1000, 1500);
> > +static int ov10640_check_id(struct rdacm21_device *dev)
> > +{
> > + unsigned int i;
> > + u8 val;
> >
> > - ov490_read_reg(dev, OV490_SCCB_SLAVE0_DIR, &val);
> > - if (val != OV10640_ID_HIGH) {
> > + /* Read OV10640 ID to test communications. */
> > + for (i = 0; i < OV10640_PID_TIMEOUT; ++i) {
>
> OV10640_PID_TIMEOUT is used in this function only, I would have made it
> local.
>

I don't think a local variable is better. The timeout is a read-only
constant which is better defined as a macro than a variable.

> > + ov490_write_reg(dev, OV490_SCCB_SLAVE0_DIR,
> > + OV490_SCCB_SLAVE_READ);
> > + ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_HIGH,
> > + OV10640_CHIP_ID >> 8);
> > + ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_LOW,
> > + OV10640_CHIP_ID & 0xff);
> > +
> > + /*
> > + * Trigger SCCB slave transaction and give it some time
> > + * to complete.
> > + */
> > + ov490_write_reg(dev, OV490_HOST_CMD, OV490_HOST_CMD_TRIGGER);
> > + usleep_range(1000, 1500);
>
> It would make sense to create an ov490_sensor_read() function to
> encapsulate all this.
>

Maybe on top when we'll have more users

> It would also be nicer to poll an OV490 register instead of sleeping
> blindly. That likely requires the OV490 application note (referenced in
> the datasheet), which we don't have. It may be useful to try and get
> hold of that. I still feel there may be something a bit fishy, but way
> less than before.
>
> > +
> > + ov490_read_reg(dev, OV490_SCCB_SLAVE0_DIR, &val);
> > + if (val == OV10640_ID_HIGH)
> > + break;
> > + usleep_range(1000, 1500);
> > + }
>
> Blank line ?
>

There isn't one intentionally as the two blocks are tightly coupled.

> > + if (i == OV10640_PID_TIMEOUT) {
> > dev_err(dev->dev, "OV10640 ID mismatch: (0x%02x)\n", val);
> > return -ENODEV;
> > }
> > @@ -359,6 +375,8 @@ static int ov490_initialize(struct rdacm21_device *dev)
> > unsigned int i;
> > int ret;
> >
> > + ov10640_power_up(dev);
>
> On top of this series, error handling of OV490 writes would make sense.
>

Maybe on top, yes

Is there anything major I should do to get a tag here ?

Thanks
j


> > +
> > /*
> > * Read OV490 Id to test communications. Give it up to 40msec to
> > * exit from reset.
> > @@ -396,7 +414,7 @@ static int ov490_initialize(struct rdacm21_device *dev)
> > return -ENODEV;
> > }
> >
> > - ret = ov10640_initialize(dev);
> > + ret = ov10640_check_id(dev);
> > if (ret)
> > return ret;
> >
>
> --
> Regards,
>
> Laurent Pinchart

2021-03-26 11:14:46

by jacopo mondi

[permalink] [raw]
Subject: Re: [PATCH v3 19/19] media: i2c: rdacm20: Re-work ov10635 reset

Hi Laurent,

On Sat, Mar 20, 2021 at 06:24:09PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Fri, Mar 19, 2021 at 05:41:48PM +0100, Jacopo Mondi wrote:
> > The OV10635 image sensor embedded in the camera module is currently
> > reset after the MAX9271 initialization with two long delays that were
> > most probably not correctly characterized.
> >
> > Re-work the image sensor reset procedure by holding the chip in reset
> > during the MAX9271 configuration, removing the long sleep delays and
> > only wait after the chip exits from reset for 350-500 microseconds
> > interval, which is larger than the minimum (2048 * (1 / XVCLK)) timeout
> > characterized in the chip manual.
> >
> > Reviewed-by: Kieran Bingham <[email protected]>
> > Reviewed-by: Laurent Pinchart <[email protected]>
> > Signed-off-by: Jacopo Mondi <[email protected]>
> > ---
> > drivers/media/i2c/rdacm20.c | 29 +++++++++++++++++------------
> > 1 file changed, 17 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> > index 7ed3866b5335..7ba2d0249da8 100644
> > --- a/drivers/media/i2c/rdacm20.c
> > +++ b/drivers/media/i2c/rdacm20.c
> > @@ -454,6 +454,19 @@ static int rdacm20_init(struct v4l2_subdev *sd, unsigned int val)
> > if (ret)
> > return ret;
> >
> > + /*
> > + * Hold OV10635 in reset during max9271 configuration. The reset signal
> > + * has to be asserted for at least 200 microseconds.
> > + */
> > + ret = max9271_enable_gpios(&dev->serializer, MAX9271_GPIO1OUT);
> > + if (ret)
> > + return ret;
> > +
> > + ret = max9271_clear_gpios(&dev->serializer, MAX9271_GPIO1OUT);
>
> enable and clear is very cnonfusing. How about mimicking the GPIO API,
> with direction_input(), direction_output() and set_value() functions ?
> It would also be nice if the polarity could be handled in a nicer way.
> There's no GPIO request API here, but maybe a max9271_gpio_set_flags() ?
>

Probably, that's an unrelated rework that could go on top unrelated to
this patch

> > + if (ret)
> > + return ret;
> > + usleep_range(200, 500);
> > +
> > ret = max9271_configure_gmsl_link(&dev->serializer);
> > if (ret)
> > return ret;
> > @@ -468,22 +481,14 @@ static int rdacm20_init(struct v4l2_subdev *sd, unsigned int val)
> > dev->serializer.client->addr = dev->addrs[0];
> >
> > /*
> > - * Reset the sensor by cycling the OV10635 reset signal connected to the
> > - * MAX9271 GPIO1 and verify communication with the OV10635.
> > + * Release ov10635 from reset and initialize it. The image sensor
> > + * requires at least 2048 XVCLK cycles (85 micro-seconds at 24MHz)
> > + * before being available. Stay safe and wait up to 500 micro-seconds.
> > */
> > - ret = max9271_enable_gpios(&dev->serializer, MAX9271_GPIO1OUT);
> > - if (ret)
> > - return ret;
> > -
> > - ret = max9271_clear_gpios(&dev->serializer, MAX9271_GPIO1OUT);
> > - if (ret)
> > - return ret;
> > - usleep_range(10000, 15000);
> > -
> > ret = max9271_set_gpios(&dev->serializer, MAX9271_GPIO1OUT);
> > if (ret)
> > return ret;
> > - usleep_range(10000, 15000);
> > + usleep_range(100, 500);
> >
> > for (i = 0; i < OV10635_PID_TIMEOUT; ++i) {
> > ret = ov10635_read16(dev, OV10635_PID);
>
> --
> Regards,
>
> Laurent Pinchart

2021-03-26 11:23:38

by jacopo mondi

[permalink] [raw]
Subject: Re: [PATCH v3 11/19] media: i2c: rdacm21: Add dealy after OV490 reset

Hi Laurent, Kieran,

On Sat, Mar 20, 2021 at 05:52:18PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Fri, Mar 19, 2021 at 04:49:44PM +0000, Kieran Bingham wrote:
> > Hi Jacopo,
> >
> > s/dealy/delay/ in $SUBJECT
> >
> > On 19/03/2021 16:41, Jacopo Mondi wrote:
> > > Add a delay after the OV490 chip is put in reset state. The reset
> > > signal shall be held for at least 250 useconds.
> > >
> > > Signed-off-by: Jacopo Mondi <[email protected]>
> > > ---
> > > drivers/media/i2c/rdacm21.c | 6 +++++-
> > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> > > index babd14b21252..875bec9f7904 100644
> > > --- a/drivers/media/i2c/rdacm21.c
> > > +++ b/drivers/media/i2c/rdacm21.c
> > > @@ -448,7 +448,10 @@ static int rdacm21_init(struct v4l2_subdev *sd, unsigned int val)
> > > if (ret)
> > > return ret;
> > >
> > > - /* Enable GPIO1 and hold OV490 in reset during max9271 configuration. */
> > > + /*
> > > + * Enable GPIO1 and hold OV490 in reset during max9271 configuration.
> > > + * The reset signal has to be asserted for at least 250 useconds.
> > > + */
> > > ret = max9271_enable_gpios(&dev->serializer, MAX9271_GPIO1OUT);
> > > if (ret)
> > > return ret;
> > > @@ -456,6 +459,7 @@ static int rdacm21_init(struct v4l2_subdev *sd, unsigned int val)
> > > ret = max9271_clear_gpios(&dev->serializer, MAX9271_GPIO1OUT);
> > > if (ret)
> > > return ret;
> > > + usleep_range(250, 500);
> >
> > Aha, for a second there I thought the comment meant it had to be
> > asserted for 250 uS before clearing it again. But it's 250 uS before
> > using the OV490.

No, it's 250usecond before releasing it. After the OV490 is released
from reset there's a 3-5 milliseconds sleep below.

> >
> > Perhaps possible to update the comment a little, but nothing that matters.
>
> The commit message and comment should match the code, especially given

They do if I'm not mistaken.

> that I'm not sure here which of the two is actually incorrect. I suspect
> the sleep is actually in the wrong location.
>
> > Reviewed-by: Kieran Bingham <[email protected]>
> >
> >
> > > ret = max9271_configure_gmsl_link(&dev->serializer);
> > > if (ret)
> > >
>
> --
> Regards,
>
> Laurent Pinchart

2021-03-26 11:38:39

by jacopo mondi

[permalink] [raw]
Subject: Re: [PATCH v3 05/19] media: v4l2-subdev: De-deprecate init() subdev op

Hi Sakari,

On Mon, Mar 22, 2021 at 01:51:44PM +0100, Jacopo Mondi wrote:
> Hi Sakari,
>
> On Sun, Mar 21, 2021 at 10:52:56PM +0200, Sakari Ailus wrote:
> > Hi Laurent and Jacopo,
> >
> > Thanks for cc'ing me.
> >
> > On Sat, Mar 20, 2021 at 05:42:12PM +0200, Laurent Pinchart wrote:
> > > Hi Jacopo,
> > >
> > > Thank you for the patch.
> > >
> > > CC'ing Sakari on v3 to get feedback.
> > >
> > > On Fri, Mar 19, 2021 at 05:41:34PM +0100, Jacopo Mondi wrote:
> > > > The init() subdev core operation is deemed to be deprecated for new
> > > > subdevice drivers. However it could prove useful for complex
> > > > architectures to defer operation that require access to the
> > > > communication bus if said bus is not available (or fully configured)
> > > > at the time when the subdevice probe() function is run.
> > > >
> > > > As an example, the GMSL architecture requires the GMSL configuration
> > > > link to be configured on the host side after the remote subdevice
> > > > has completed its probe function. After the configuration on the host
> > > > side has been performed, the subdevice registers can be accessed through
> > > > the communication bus.
> >
> > What does the remote device's probe do that needs to be done before bus
> > config on the host side?
>
> A few lines here below:
>
> In the GMSL use case the bus configuration requires the enablement of the
> noise immunity threshold on the remote side which ensures reliability
> of communications in electrically noisy environments. After the subdevice
> has enabled the threshold at the end of its probe() sequence the host
> side shall compensate it with an higher signal amplitude. Once this
> sequence has completed the bus can be accessed with noise protection
> enabled and all the operations that require a considerable number of
> transactions on the bus (such as the image sensor configuration
> sequence) are run in the subdevice init() operation implementation.
>
> >
> > Alternatively, could the remote init() work be done at the time streaming
> > is started?
>
> That would require programing the sensor, the embedded ISP at s_stream
> time which would take some time.

I'm afraid but from my testing also the chip identification is more
reliable if run in init(). As identifying chips is something that has
to happen at probe/initialization I fear it is not possible to move it
to s_stream time.

>
> I'll take this suggestion into account though and run some more tests.
>
> Thanks
> j
>
> >
> > > >
> > > > In particular:
> > > >
> > > > HOST REMOTE
> > > >
> > > > probe()
> > > > |
> > > > ---------------------> |
> > > > probe() {
> > > > bus config()
> > > > }
> > > > |<--------------------|
> > > > v4l2 async bound {
> > > > bus config()
> > > > call subdev init()
> > > > |-------------------->|
> > > > init() {
> > > > access register on the bus()
> > > > }
> > > > |<-------------------
> > > > }
> > > >
> > > > In the GMSL use case the bus configuration requires the enablement of the
> > > > noise immunity threshold on the remote side which ensures reliability
> > > > of communications in electrically noisy environments. After the subdevice
> > > > has enabled the threshold at the end of its probe() sequence the host
> > > > side shall compensate it with an higher signal amplitude. Once this
> > > > sequence has completed the bus can be accessed with noise protection
> > > > enabled and all the operations that require a considerable number of
> > > > transactions on the bus (such as the image sensor configuration
> > > > sequence) are run in the subdevice init() operation implementation.
> > > >
> > > > Signed-off-by: Jacopo Mondi <[email protected]>
> > > > ---
> > > > include/media/v4l2-subdev.h | 15 ++++++++++++---
> > > > 1 file changed, 12 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > > index d0e9a5bdb08b..3068d9940669 100644
> > > > --- a/include/media/v4l2-subdev.h
> > > > +++ b/include/media/v4l2-subdev.h
> > > > @@ -148,9 +148,18 @@ struct v4l2_subdev_io_pin_config {
> > > > * each pin being configured. This function could be called at times
> > > > * other than just subdevice initialization.
> > > > *
> > > > - * @init: initialize the sensor registers to some sort of reasonable default
> > > > - * values. Do not use for new drivers and should be removed in existing
> > > > - * drivers.
> > > > + * @init: initialize the subdevice registers to some sort of reasonable default
> > > > + * values. Do not use for new drivers (and should be removed in existing
> > > > + * ones) for regular architectures where the image sensor is connected to
> > > > + * the host receiver. For more complex architectures where the subdevice
> > > > + * initialization should be deferred to the completion of the probe
> > > > + * sequence of some intermediate component, or the communication bus
> > > > + * requires configurations on the host side that depend on the completion
> > > > + * of the probe sequence of the remote subdevices, the usage of this
> > > > + * operation could be considered to allow the devices along the pipeline to
> > > > + * probe and register in the media graph and to defer any operation that
> > > > + * require actual access to the communication bus to their init() function
> > > > + * implementation.
> > > > *
> > > > * @load_fw: load firmware.
> > > > *
> >
> > --
> > Kind regards,
> >
> > Sakari Ailus