| Message ID | 20210817072703.1167-5-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | media: i2c: Add MAX9271 subdevice driver | expand |
Hi Jacopo, Thank you for the patch. On Tue, Aug 17, 2021 at 09:27:02AM +0200, Jacopo Mondi wrote: > The max9286 driver needs to fetch the remote serializer PIXEL_RATE > control value in order to compute its own one, as the sum of the s/its own one/its own/ (or its own pixel rate) > values reported by the connected subdevices. > > Currently the control is verified to be present at notifier's bound > time, which requires the serializer driver to register the control at > probe time. As the serializer driver might need to register the control > later, by adding the control handler of its connected sensor, post-pone > the max9286 check for the control availability at start stream time. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > drivers/media/i2c/max9286.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > index 1b92d18a1f94..98fc90339a9e 100644 > --- a/drivers/media/i2c/max9286.c > +++ b/drivers/media/i2c/max9286.c > @@ -595,7 +595,7 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier, > max9286_check_config_link(priv, priv->source_mask); > max9286_configure_i2c(priv, false); > > - return max9286_set_pixelrate(priv); > + return 0; > } > > static void max9286_notify_unbind(struct v4l2_async_notifier *notifier, > @@ -674,6 +674,10 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable) > int ret; > > if (enable) { > + ret = max9286_set_pixelrate(priv); > + if (ret) > + return ret; That's very likely not going to work. The CSI-2 receiver connected to the max9286 will need to know the pixel rate as part of its initialization sequence, before calling .s_stream(1) on the max9286. > + > /* > * The frame sync between cameras is transmitted across the > * reverse channel as GPIO. We must open all channels while
Hi Laurent, On Mon, Aug 23, 2021 at 05:17:05AM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Tue, Aug 17, 2021 at 09:27:02AM +0200, Jacopo Mondi wrote: > > The max9286 driver needs to fetch the remote serializer PIXEL_RATE > > control value in order to compute its own one, as the sum of the > > s/its own one/its own/ (or its own pixel rate) > > > values reported by the connected subdevices. > > > > Currently the control is verified to be present at notifier's bound > > time, which requires the serializer driver to register the control at > > probe time. As the serializer driver might need to register the control > > later, by adding the control handler of its connected sensor, post-pone > > the max9286 check for the control availability at start stream time. > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > --- > > drivers/media/i2c/max9286.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > > index 1b92d18a1f94..98fc90339a9e 100644 > > --- a/drivers/media/i2c/max9286.c > > +++ b/drivers/media/i2c/max9286.c > > @@ -595,7 +595,7 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier, > > max9286_check_config_link(priv, priv->source_mask); > > max9286_configure_i2c(priv, false); > > > > - return max9286_set_pixelrate(priv); > > + return 0; > > } > > > > static void max9286_notify_unbind(struct v4l2_async_notifier *notifier, > > @@ -674,6 +674,10 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable) > > int ret; > > > > if (enable) { > > + ret = max9286_set_pixelrate(priv); > > + if (ret) > > + return ret; > > That's very likely not going to work. The CSI-2 receiver connected to > the max9286 will need to know the pixel rate as part of its > initialization sequence, before calling .s_stream(1) on the max9286. > How so ? The R-Car CSI-2 receiver feteches the pixel rate at s_stream time, I thought it was customary to do so. What is it needed for before streamon time ? > > + > > /* > > * The frame sync between cameras is transmitted across the > > * reverse channel as GPIO. We must open all channels while > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Mon, Aug 23, 2021 at 09:20:08AM +0200, Jacopo Mondi wrote: > On Mon, Aug 23, 2021 at 05:17:05AM +0300, Laurent Pinchart wrote: > > On Tue, Aug 17, 2021 at 09:27:02AM +0200, Jacopo Mondi wrote: > > > The max9286 driver needs to fetch the remote serializer PIXEL_RATE > > > control value in order to compute its own one, as the sum of the > > > > s/its own one/its own/ (or its own pixel rate) > > > > > values reported by the connected subdevices. > > > > > > Currently the control is verified to be present at notifier's bound > > > time, which requires the serializer driver to register the control at > > > probe time. As the serializer driver might need to register the control > > > later, by adding the control handler of its connected sensor, post-pone > > > the max9286 check for the control availability at start stream time. > > > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > > --- > > > drivers/media/i2c/max9286.c | 6 +++++- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > > > index 1b92d18a1f94..98fc90339a9e 100644 > > > --- a/drivers/media/i2c/max9286.c > > > +++ b/drivers/media/i2c/max9286.c > > > @@ -595,7 +595,7 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier, > > > max9286_check_config_link(priv, priv->source_mask); > > > max9286_configure_i2c(priv, false); > > > > > > - return max9286_set_pixelrate(priv); > > > + return 0; > > > } > > > > > > static void max9286_notify_unbind(struct v4l2_async_notifier *notifier, > > > @@ -674,6 +674,10 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable) > > > int ret; > > > > > > if (enable) { > > > + ret = max9286_set_pixelrate(priv); > > > + if (ret) > > > + return ret; > > > > That's very likely not going to work. The CSI-2 receiver connected to > > the max9286 will need to know the pixel rate as part of its > > initialization sequence, before calling .s_stream(1) on the max9286. > > How so ? The R-Car CSI-2 receiver feteches the pixel rate at s_stream > time, I thought it was customary to do so. What is it needed for before > streamon time ? At stream on time is usually fine, but the CSI-2 receiver usually needs to be configured before starting the source, not after. > > > + > > > /* > > > * The frame sync between cameras is transmitted across the > > > * reverse channel as GPIO. We must open all channels while
diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c index 1b92d18a1f94..98fc90339a9e 100644 --- a/drivers/media/i2c/max9286.c +++ b/drivers/media/i2c/max9286.c @@ -595,7 +595,7 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier, max9286_check_config_link(priv, priv->source_mask); max9286_configure_i2c(priv, false); - return max9286_set_pixelrate(priv); + return 0; } static void max9286_notify_unbind(struct v4l2_async_notifier *notifier, @@ -674,6 +674,10 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable) int ret; if (enable) { + ret = max9286_set_pixelrate(priv); + if (ret) + return ret; + /* * The frame sync between cameras is transmitted across the * reverse channel as GPIO. We must open all channels while
The max9286 driver needs to fetch the remote serializer PIXEL_RATE control value in order to compute its own one, as the sum of the values reported by the connected subdevices. Currently the control is verified to be present at notifier's bound time, which requires the serializer driver to register the control at probe time. As the serializer driver might need to register the control later, by adding the control handler of its connected sensor, post-pone the max9286 check for the control availability at start stream time. Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- drivers/media/i2c/max9286.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)