From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>,
niklas.soderlund+renesas@ragnatech.se, geert@linux-m68k.org,
Mauro Carvalho Chehab <mchehab@kernel.org>,
linux-media@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 13/16] media: i2c: rdacm2x: Implement .init() subdev op
Date: Mon, 22 Feb 2021 03:52:41 +0200 [thread overview]
Message-ID: <YDMOaZUi8Q1vWq9j@pendragon.ideasonboard.com> (raw)
In-Reply-To: <ce1673db-9ea7-f28c-a22a-0d129c0bff98@ideasonboard.com>
Hi Jacopo,
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
On Thu, Feb 18, 2021 at 04:13:14PM +0000, Kieran Bingham wrote:
> On 16/02/2021 17:41, Jacopo Mondi wrote:
> > The current probe() procedure of the RDACM20 and RDACM20 performs
>
> and RDACM21?
>
> > initialization of the serializer image sensors and increases the noise
>
> Of the serializer 'and' image sensors ?
> or perhaps just
>
> s/serializer/serializer,/ ?
>
> > immunity threshold as last operation, which is then compensated by the
>
> as a last operation
> or
> as a final operation
>
>
> > remote deserializer by increasing the reverse channel signal amplitude
> > once all remotes have bound.
> >
> > The probe routine is then run without noise immunity activated which
Maybe s/then/thus/ ? I initially interpreted this as meaning the probe
routine is run after the operations listed above.
> > in noisy environment conditions makes the probe sequence less reliable as
> > the chips configuration requires a relevant amount of i2c transactions.
>
> s/relevant/relatively high/ ?
>
> > 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
> > - Move the chips initialization to the .init() core subdev operation to
> > be invoked by the deserializer after the camera has probed and it has
> > increased the reverse channel amplitude
>
> Is this the op you said was deprecated?
>
>
> Functionally in this code, it seems fine, but as mentioned on the next
> patch, I suspect it might need squashing to make sure it stays functional...
>
> I'm not fully sure of the implications of this patch, but your tests
> have reportede that this series is helping a lot with reliability so I
> don't want to block it.
>
> The code changes themselves look ok, with the following thougts:
>
> - If this op/methodology is deprecated it might be harder to get
> acceptance?
I think we need to propose de-deprecating the operation, which should
include a clear explanation of the valid use cases.
> - now we have _init and _initialise - should that be made more
> distinct?
>
> - Seeing the duplication of the MAX9271_DEFAULT_ADDR / ping again
> really makes me want to see that wrapped in the max9271.c ;-)
>
> - Likely needs squashed with relevant changes in max9286?
I share the bisectability concerns.
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> But even with those thoughts, I don't think this is necessarily wrong so:
>
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> > drivers/media/i2c/rdacm20.c | 65 ++++++++++++++++++++++---------------
> > drivers/media/i2c/rdacm21.c | 65 ++++++++++++++++++++++---------------
> > 2 files changed, 78 insertions(+), 52 deletions(-)
> >
> > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> > index 39e4b4241870..0632ef98eea7 100644
> > --- a/drivers/media/i2c/rdacm20.c
> > +++ b/drivers/media/i2c/rdacm20.c
> > @@ -437,36 +437,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 i;
> > 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(5000, 8000);
> > -
> > - /* 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.
> > @@ -537,6 +513,43 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
> >
> > dev_info(dev->dev, "Identified RDACM20 camera module\n");
> >
> > + 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);
> > + usleep_range(5000, 8000);
> > +
> > + /* 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.
> > *
> > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> > index c420a6b96879..80b6f16f87a8 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;
> > @@ -448,20 +433,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(5000, 8000);
> > -
> > - /* 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 |
> > @@ -508,6 +484,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(5000, 8000);
> > +
> > + /* Enable reverse channel and disable the serial link. */
> > + ret = max9271_set_serial_link(&dev->serializer, false);
> > + if (ret)
> > + return ret;
> > +
> > /*
> > * Set reverse channel high threshold to increase noise immunity.
> > *
--
Regards,
Laurent Pinchart
next prev parent reply other threads:[~2021-02-22 1:54 UTC|newest]
Thread overview: 65+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-16 17:41 [PATCH 00/16] media: i2c: GMSL reliability improvements Jacopo Mondi
2021-02-16 17:41 ` [PATCH 01/16] media: i2c: rdacm20: Enable noise immunity Jacopo Mondi
2021-02-17 12:55 ` Kieran Bingham
2021-02-22 0:49 ` Laurent Pinchart
2021-02-22 14:59 ` Jacopo Mondi
2021-02-24 20:35 ` Laurent Pinchart
2021-02-16 17:41 ` [PATCH 02/16] media: i2c: rdacm20: Embedded 'serializer' field Jacopo Mondi
2021-02-17 12:56 ` Kieran Bingham
2021-02-22 0:58 ` Laurent Pinchart
2021-02-16 17:41 ` [PATCH 03/16] media: i2c: rdacm20: Replace goto with a loop Jacopo Mondi
2021-02-17 13:01 ` Kieran Bingham
2021-02-22 1:05 ` Laurent Pinchart
2021-02-22 15:06 ` Jacopo Mondi
2021-02-24 20:27 ` Laurent Pinchart
2021-02-25 8:56 ` Kieran Bingham
2021-02-16 17:41 ` [PATCH 04/16] media: i2c: rdacm20: Report camera module name Jacopo Mondi
2021-02-17 13:02 ` Kieran Bingham
2021-02-22 1:06 ` Laurent Pinchart
2021-02-16 17:41 ` [PATCH 05/16] media: i2c: rdacm20: Check return values Jacopo Mondi
2021-02-17 13:08 ` Kieran Bingham
2021-02-22 1:09 ` Laurent Pinchart
2021-02-22 15:08 ` Jacopo Mondi
2021-02-16 17:41 ` [PATCH 06/16] media: i2c: rdacm20: Re-work ov10635 reset Jacopo Mondi
2021-02-17 13:22 ` Kieran Bingham
2021-02-22 1:15 ` Laurent Pinchart
2021-02-16 17:41 ` [PATCH 07/16] media: i2c: rdacm2x: Fix wake up delay Jacopo Mondi
2021-02-17 8:02 ` Geert Uytterhoeven
2021-02-17 13:33 ` Kieran Bingham
2021-02-22 1:18 ` Laurent Pinchart
2021-02-22 15:11 ` Jacopo Mondi
2021-02-24 20:29 ` Laurent Pinchart
2021-02-16 17:41 ` [PATCH 08/16] media: i2c: max9286: Adjust parameters indent Jacopo Mondi
2021-02-17 13:36 ` Kieran Bingham
2021-02-22 1:20 ` Laurent Pinchart
2021-02-16 17:41 ` [PATCH 09/16] media: i2c: rdacm21: Re-work OV10640 initialization Jacopo Mondi
2021-02-17 8:06 ` Geert Uytterhoeven
2021-02-17 13:55 ` Kieran Bingham
2021-02-22 1:27 ` Laurent Pinchart
2021-02-22 15:19 ` Jacopo Mondi
2021-02-24 20:30 ` Laurent Pinchart
2021-02-16 17:41 ` [PATCH 10/16] media: i2c: max9286: Rename reverse_channel_mv Jacopo Mondi
2021-02-17 8:07 ` Geert Uytterhoeven
2021-02-18 15:06 ` Kieran Bingham
2021-02-22 1:29 ` Laurent Pinchart
2021-02-16 17:41 ` [PATCH 11/16] media: i2c: max9286: Cache channel amplitude Jacopo Mondi
2021-02-17 8:08 ` Geert Uytterhoeven
2021-02-18 15:39 ` Kieran Bingham
2021-02-22 1:33 ` Laurent Pinchart
2021-02-16 17:41 ` [PATCH 12/16] media: i2c: max9286: Define high " Jacopo Mondi
2021-02-18 15:39 ` Kieran Bingham
2021-02-22 1:35 ` Laurent Pinchart
2021-02-16 17:41 ` [PATCH 13/16] media: i2c: rdacm2x: Implement .init() subdev op Jacopo Mondi
2021-02-17 8:17 ` Geert Uytterhoeven
2021-02-18 16:13 ` Kieran Bingham
2021-02-22 1:52 ` Laurent Pinchart [this message]
2021-02-16 17:41 ` [PATCH 14/16] media: i2c: max9286: Initialize remotes when bound Jacopo Mondi
2021-02-17 8:19 ` Geert Uytterhoeven
2021-02-18 16:00 ` Kieran Bingham
2021-02-22 2:03 ` Laurent Pinchart
2021-02-16 17:41 ` [PATCH 15/16] media: i2c: max9286: Rework comments in .bound() Jacopo Mondi
2021-02-18 16:03 ` Kieran Bingham
2021-02-16 17:41 ` [PATCH 16/16] media: i2c: gmsl: Use 339Kbps I2C bit-rate Jacopo Mondi
2021-02-17 8:19 ` Geert Uytterhoeven
2021-02-18 16:07 ` Kieran Bingham
2021-02-22 2:06 ` Laurent Pinchart
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YDMOaZUi8Q1vWq9j@pendragon.ideasonboard.com \
--to=laurent.pinchart@ideasonboard.com \
--cc=geert@linux-m68k.org \
--cc=jacopo+renesas@jmondi.org \
--cc=kieran.bingham+renesas@ideasonboard.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=niklas.soderlund+renesas@ragnatech.se \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).