[PATCH] v4l: rcar-csi2: Don't bail out from probe on no ep

[PATCH] v4l: rcar-csi2: Don't bail out from probe on no ep

[Jacopo Mondi]

When rcar-csi interface is not connected to any endpoint, it fails and
bails out from probe before registering its own video subdevice.
This prevents rcar-vin registered notifier from completing and no
subdevice is ever registered, also for other properly connected csi
interfaces.

Fix this not returning an error when no endpoint is connected to a csi
interface and let the driver complete its probe function and register its
own video subdevice.

---
Niklas,
   please squash this patch in your next rcar-csi2 series (if you like it ;)

As we have discussed this is particularly useful for gmsl setup, where adv748x
is connected to CSI20 and max9286 to CSI40/CSI41. If we disable adv748x from DTS
we need CSI20 probe to complete anyhow otherwise no subdevice gets registered
for the two deserializers.

Please note we cannot disable CSI20 entirely otherwise VIN's graph parsing
breaks.

Thanks
   j

---
 drivers/media/platform/rcar-vin/rcar-csi2.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index 2793efb..90c4062 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -928,8 +928,8 @@ static int rcar_csi2_parse_dt(struct rcar_csi2 *priv)

 	ep = of_graph_get_endpoint_by_regs(priv->dev->of_node, 0, 0);
 	if (!ep) {
-		dev_err(priv->dev, "Not connected to subdevice\n");
-		return -EINVAL;
+		dev_dbg(priv->dev, "Not connected to subdevice\n");
+		return 0;
 	}

 	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep);
--
2.7.4

Re: [PATCH] v4l: rcar-csi2: Don't bail out from probe on no ep

[Hans Verkuil]

Niklas,

Did you look at this? If I should take it, can you Ack it? If you are
going to squash or add it to our of your own patch series, then let me
know and I can remove it from my todo queue.

Regards,

	Hans

On 05/12/17 21:41, Jacopo Mondi wrote:
> When rcar-csi interface is not connected to any endpoint, it fails and
> bails out from probe before registering its own video subdevice.
> This prevents rcar-vin registered notifier from completing and no
> subdevice is ever registered, also for other properly connected csi
> interfaces.
> 
> Fix this not returning an error when no endpoint is connected to a csi
> interface and let the driver complete its probe function and register its
> own video subdevice.
> 
> ---
> Niklas,
>    please squash this patch in your next rcar-csi2 series (if you like it ;)
> 
> As we have discussed this is particularly useful for gmsl setup, where adv748x
> is connected to CSI20 and max9286 to CSI40/CSI41. If we disable adv748x from DTS
> we need CSI20 probe to complete anyhow otherwise no subdevice gets registered
> for the two deserializers.
> 
> Please note we cannot disable CSI20 entirely otherwise VIN's graph parsing
> breaks.
> 
> Thanks
>    j
> 
> ---
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index 2793efb..90c4062 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -928,8 +928,8 @@ static int rcar_csi2_parse_dt(struct rcar_csi2 *priv)
> 
>  	ep = of_graph_get_endpoint_by_regs(priv->dev->of_node, 0, 0);
>  	if (!ep) {
> -		dev_err(priv->dev, "Not connected to subdevice\n");
> -		return -EINVAL;
> +		dev_dbg(priv->dev, "Not connected to subdevice\n");
> +		return 0;
>  	}
> 
>  	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep);
> --
> 2.7.4
> 

Re: [PATCH] v4l: rcar-csi2: Don't bail out from probe on no ep

[Niklas Söderlund]

Hi Hans,

On 2017-12-15 15:06:44 +0100, Hans Verkuil wrote:
> Niklas,
> 
> Did you look at this? If I should take it, can you Ack it? If you are
> going to squash or add it to our of your own patch series, then let me
> know and I can remove it from my todo queue.

I did look at it and talked to Jacobo about it. I think I have a better 
solution to his problem which I hope to try out in the near future. If 
my workaround turns out to not solve his problem I will squash this in 
when I resend the rcar-csi2 patch-set so in any case I think you can 
drop it from your todo queue.

The reason I'm a bit reluctant to ack this straight away is that I think 
it's kind of good that rcar-csi2 fails to probe if it finds no endpoints 
to work with, there is little use for the driver instance then. The 
problem Jacobo is trying to fix is related to how the rcar-vin Gen3 
group parses DT and I made a small mistake there which was discovered by 
Jacobo. And since the original fault is in the rcar-vin driver I think 
the issue should be fixed in that driver.

> 
> Regards,
> 
> 	Hans
> 
> On 05/12/17 21:41, Jacopo Mondi wrote:
> > When rcar-csi interface is not connected to any endpoint, it fails and
> > bails out from probe before registering its own video subdevice.
> > This prevents rcar-vin registered notifier from completing and no
> > subdevice is ever registered, also for other properly connected csi
> > interfaces.
> > 
> > Fix this not returning an error when no endpoint is connected to a csi
> > interface and let the driver complete its probe function and register its
> > own video subdevice.
> > 
> > ---
> > Niklas,
> >    please squash this patch in your next rcar-csi2 series (if you like it ;)
> > 
> > As we have discussed this is particularly useful for gmsl setup, where adv748x
> > is connected to CSI20 and max9286 to CSI40/CSI41. If we disable adv748x from DTS
> > we need CSI20 probe to complete anyhow otherwise no subdevice gets registered
> > for the two deserializers.
> > 
> > Please note we cannot disable CSI20 entirely otherwise VIN's graph parsing
> > breaks.
> > 
> > Thanks
> >    j
> > 
> > ---
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > index 2793efb..90c4062 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -928,8 +928,8 @@ static int rcar_csi2_parse_dt(struct rcar_csi2 *priv)
> > 
> >  	ep = of_graph_get_endpoint_by_regs(priv->dev->of_node, 0, 0);
> >  	if (!ep) {
> > -		dev_err(priv->dev, "Not connected to subdevice\n");
> > -		return -EINVAL;
> > +		dev_dbg(priv->dev, "Not connected to subdevice\n");
> > +		return 0;
> >  	}
> > 
> >  	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep);
> > --
> > 2.7.4
> > 
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH] v4l: rcar-csi2: Don't bail out from probe on no ep

[Niklas Söderlund]

Hi Hans,

On 2017-12-15 15:06:44 +0100, Hans Verkuil wrote:
> Niklas,
> 
> Did you look at this? If I should take it, can you Ack it? If you are
> going to squash or add it to our of your own patch series, then let me
> know and I can remove it from my todo queue.

I did look at it and talked to Jacobo about it. I think I have a better 
solution to his problem which I hope to try out in the near future. If 
my workaround turns out to not solve his problem I will squash this in 
when I resend the rcar-csi2 patch-set so in any case I think you can 
drop it from your todo queue.

The reason I'm a bit reluctant to ack this straight away is that I think 
it's kind of good that rcar-csi2 fails to probe if it finds no endpoints 
to work with, there is little use for the driver instance then. The 
problem Jacobo is trying to fix is related to how the rcar-vin Gen3 
group parses DT and I made a small mistake there which was discovered by 
Jacobo. And since the original fault is in the rcar-vin driver I think 
the issue should be fixed in that driver.

> 
> Regards,
> 
> 	Hans
> 
> On 05/12/17 21:41, Jacopo Mondi wrote:
> > When rcar-csi interface is not connected to any endpoint, it fails and
> > bails out from probe before registering its own video subdevice.
> > This prevents rcar-vin registered notifier from completing and no
> > subdevice is ever registered, also for other properly connected csi
> > interfaces.
> > 
> > Fix this not returning an error when no endpoint is connected to a csi
> > interface and let the driver complete its probe function and register its
> > own video subdevice.
> > 
> > ---
> > Niklas,
> >    please squash this patch in your next rcar-csi2 series (if you like it ;)
> > 
> > As we have discussed this is particularly useful for gmsl setup, where adv748x
> > is connected to CSI20 and max9286 to CSI40/CSI41. If we disable adv748x from DTS
> > we need CSI20 probe to complete anyhow otherwise no subdevice gets registered
> > for the two deserializers.
> > 
> > Please note we cannot disable CSI20 entirely otherwise VIN's graph parsing
> > breaks.
> > 
> > Thanks
> >    j
> > 
> > ---
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > index 2793efb..90c4062 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -928,8 +928,8 @@ static int rcar_csi2_parse_dt(struct rcar_csi2 *priv)
> > 
> >  	ep = of_graph_get_endpoint_by_regs(priv->dev->of_node, 0, 0);
> >  	if (!ep) {
> > -		dev_err(priv->dev, "Not connected to subdevice\n");
> > -		return -EINVAL;
> > +		dev_dbg(priv->dev, "Not connected to subdevice\n");
> > +		return 0;
> >  	}
> > 
> >  	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep);
> > --
> > 2.7.4
> > 
> 

-- 
Regards,
Niklas S�derlund

Re: [PATCH] v4l: rcar-csi2: Don't bail out from probe on no ep

[Laurent Pinchart]

Hi Niklas,

On Friday, 15 December 2017 16:23:10 EET Niklas Söderlund wrote:
> On 2017-12-15 15:06:44 +0100, Hans Verkuil wrote:
> > Niklas,
> > 
> > Did you look at this? If I should take it, can you Ack it? If you are
> > going to squash or add it to our of your own patch series, then let me
> > know and I can remove it from my todo queue.
> 
> I did look at it and talked to Jacobo about it. I think I have a better
> solution to his problem which I hope to try out in the near future. If
> my workaround turns out to not solve his problem I will squash this in
> when I resend the rcar-csi2 patch-set so in any case I think you can
> drop it from your todo queue.
> 
> The reason I'm a bit reluctant to ack this straight away is that I think
> it's kind of good that rcar-csi2 fails to probe if it finds no endpoints
> to work with, there is little use for the driver instance then. The
> problem Jacobo is trying to fix is related to how the rcar-vin Gen3
> group parses DT and I made a small mistake there which was discovered by
> Jacobo. And since the original fault is in the rcar-vin driver I think
> the issue should be fixed in that driver.

How do you plan to handle the case where only one CSI-2 receiver has a 
connected input ?

> > On 05/12/17 21:41, Jacopo Mondi wrote:
> >> When rcar-csi interface is not connected to any endpoint, it fails and
> >> bails out from probe before registering its own video subdevice.
> >> This prevents rcar-vin registered notifier from completing and no
> >> subdevice is ever registered, also for other properly connected csi
> >> interfaces.
> >> 
> >> Fix this not returning an error when no endpoint is connected to a csi
> >> interface and let the driver complete its probe function and register
> >> its own video subdevice.
> >> 
> >> ---
> >> Niklas,
> >> 
> >>    please squash this patch in your next rcar-csi2 series (if you like
> >>    it ;)
> >> 
> >> As we have discussed this is particularly useful for gmsl setup, where
> >> adv748x is connected to CSI20 and max9286 to CSI40/CSI41. If we disable
> >> adv748x from DTS we need CSI20 probe to complete anyhow otherwise no
> >> subdevice gets registered for the two deserializers.
> >> 
> >> Please note we cannot disable CSI20 entirely otherwise VIN's graph
> >> parsing breaks.
> >> 
> >> Thanks
> >> 
> >>    j
> >> 
> >> ---
> >> 
> >>  drivers/media/platform/rcar-vin/rcar-csi2.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c
> >> b/drivers/media/platform/rcar-vin/rcar-csi2.c index 2793efb..90c4062
> >> 100644
> >> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> >> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> >> @@ -928,8 +928,8 @@ static int rcar_csi2_parse_dt(struct rcar_csi2
> >> *priv)
> >> 
> >>  	ep = of_graph_get_endpoint_by_regs(priv->dev->of_node, 0, 0);
> >>  	if (!ep) {
> >> -		dev_err(priv->dev, "Not connected to subdevice\n");
> >> -		return -EINVAL;
> >> +		dev_dbg(priv->dev, "Not connected to subdevice\n");
> >> +		return 0;
> >>  	}
> >>  	
> >>  	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep);

-- 
Regards,

Laurent Pinchart

Re: [PATCH] v4l: rcar-csi2: Don't bail out from probe on no ep

[Niklas Söderlund]

Hi Laurent,

On 2017-12-20 20:06:13 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> On Friday, 15 December 2017 16:23:10 EET Niklas Söderlund wrote:
> > On 2017-12-15 15:06:44 +0100, Hans Verkuil wrote:
> > > Niklas,
> > > 
> > > Did you look at this? If I should take it, can you Ack it? If you are
> > > going to squash or add it to our of your own patch series, then let me
> > > know and I can remove it from my todo queue.
> > 
> > I did look at it and talked to Jacobo about it. I think I have a better
> > solution to his problem which I hope to try out in the near future. If
> > my workaround turns out to not solve his problem I will squash this in
> > when I resend the rcar-csi2 patch-set so in any case I think you can
> > drop it from your todo queue.
> > 
> > The reason I'm a bit reluctant to ack this straight away is that I think
> > it's kind of good that rcar-csi2 fails to probe if it finds no endpoints
> > to work with, there is little use for the driver instance then. The
> > problem Jacobo is trying to fix is related to how the rcar-vin Gen3
> > group parses DT and I made a small mistake there which was discovered by
> > Jacobo. And since the original fault is in the rcar-vin driver I think
> > the issue should be fixed in that driver.
> 
> How do you plan to handle the case where only one CSI-2 receiver has a 
> connected input ?

I'm sorry, I don't understand this question I think. If there only is 
one CSI-2 receiver with connected endpoints, should that not be the only 
hardware the driver handles? What would be the reason to enable a CSI-2 
device in DT and not connect endpoints to it?

> 
> > > On 05/12/17 21:41, Jacopo Mondi wrote:
> > >> When rcar-csi interface is not connected to any endpoint, it fails and
> > >> bails out from probe before registering its own video subdevice.
> > >> This prevents rcar-vin registered notifier from completing and no
> > >> subdevice is ever registered, also for other properly connected csi
> > >> interfaces.
> > >> 
> > >> Fix this not returning an error when no endpoint is connected to a csi
> > >> interface and let the driver complete its probe function and register
> > >> its own video subdevice.
> > >> 
> > >> ---
> > >> Niklas,
> > >> 
> > >>    please squash this patch in your next rcar-csi2 series (if you like
> > >>    it ;)
> > >> 
> > >> As we have discussed this is particularly useful for gmsl setup, where
> > >> adv748x is connected to CSI20 and max9286 to CSI40/CSI41. If we disable
> > >> adv748x from DTS we need CSI20 probe to complete anyhow otherwise no
> > >> subdevice gets registered for the two deserializers.
> > >> 
> > >> Please note we cannot disable CSI20 entirely otherwise VIN's graph
> > >> parsing breaks.
> > >> 
> > >> Thanks
> > >> 
> > >>    j
> > >> 
> > >> ---
> > >> 
> > >>  drivers/media/platform/rcar-vin/rcar-csi2.c | 4 ++--
> > >>  1 file changed, 2 insertions(+), 2 deletions(-)
> > >> 
> > >> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > >> b/drivers/media/platform/rcar-vin/rcar-csi2.c index 2793efb..90c4062
> > >> 100644
> > >> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > >> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > >> @@ -928,8 +928,8 @@ static int rcar_csi2_parse_dt(struct rcar_csi2
> > >> *priv)
> > >> 
> > >>  	ep = of_graph_get_endpoint_by_regs(priv->dev->of_node, 0, 0);
> > >>  	if (!ep) {
> > >> -		dev_err(priv->dev, "Not connected to subdevice\n");
> > >> -		return -EINVAL;
> > >> +		dev_dbg(priv->dev, "Not connected to subdevice\n");
> > >> +		return 0;
> > >>  	}
> > >>  	
> > >>  	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep);
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH] v4l: rcar-csi2: Don't bail out from probe on no ep

[Niklas Söderlund]

Hi Laurent,

On 2017-12-20 20:06:13 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> On Friday, 15 December 2017 16:23:10 EET Niklas S�derlund wrote:
> > On 2017-12-15 15:06:44 +0100, Hans Verkuil wrote:
> > > Niklas,
> > > 
> > > Did you look at this? If I should take it, can you Ack it? If you are
> > > going to squash or add it to our of your own patch series, then let me
> > > know and I can remove it from my todo queue.
> > 
> > I did look at it and talked to Jacobo about it. I think I have a better
> > solution to his problem which I hope to try out in the near future. If
> > my workaround turns out to not solve his problem I will squash this in
> > when I resend the rcar-csi2 patch-set so in any case I think you can
> > drop it from your todo queue.
> > 
> > The reason I'm a bit reluctant to ack this straight away is that I think
> > it's kind of good that rcar-csi2 fails to probe if it finds no endpoints
> > to work with, there is little use for the driver instance then. The
> > problem Jacobo is trying to fix is related to how the rcar-vin Gen3
> > group parses DT and I made a small mistake there which was discovered by
> > Jacobo. And since the original fault is in the rcar-vin driver I think
> > the issue should be fixed in that driver.
> 
> How do you plan to handle the case where only one CSI-2 receiver has a 
> connected input ?

I'm sorry, I don't understand this question I think. If there only is 
one CSI-2 receiver with connected endpoints, should that not be the only 
hardware the driver handles? What would be the reason to enable a CSI-2 
device in DT and not connect endpoints to it?

> 
> > > On 05/12/17 21:41, Jacopo Mondi wrote:
> > >> When rcar-csi interface is not connected to any endpoint, it fails and
> > >> bails out from probe before registering its own video subdevice.
> > >> This prevents rcar-vin registered notifier from completing and no
> > >> subdevice is ever registered, also for other properly connected csi
> > >> interfaces.
> > >> 
> > >> Fix this not returning an error when no endpoint is connected to a csi
> > >> interface and let the driver complete its probe function and register
> > >> its own video subdevice.
> > >> 
> > >> ---
> > >> Niklas,
> > >> 
> > >>    please squash this patch in your next rcar-csi2 series (if you like
> > >>    it ;)
> > >> 
> > >> As we have discussed this is particularly useful for gmsl setup, where
> > >> adv748x is connected to CSI20 and max9286 to CSI40/CSI41. If we disable
> > >> adv748x from DTS we need CSI20 probe to complete anyhow otherwise no
> > >> subdevice gets registered for the two deserializers.
> > >> 
> > >> Please note we cannot disable CSI20 entirely otherwise VIN's graph
> > >> parsing breaks.
> > >> 
> > >> Thanks
> > >> 
> > >>    j
> > >> 
> > >> ---
> > >> 
> > >>  drivers/media/platform/rcar-vin/rcar-csi2.c | 4 ++--
> > >>  1 file changed, 2 insertions(+), 2 deletions(-)
> > >> 
> > >> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > >> b/drivers/media/platform/rcar-vin/rcar-csi2.c index 2793efb..90c4062
> > >> 100644
> > >> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > >> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > >> @@ -928,8 +928,8 @@ static int rcar_csi2_parse_dt(struct rcar_csi2
> > >> *priv)
> > >> 
> > >>  	ep = of_graph_get_endpoint_by_regs(priv->dev->of_node, 0, 0);
> > >>  	if (!ep) {
> > >> -		dev_err(priv->dev, "Not connected to subdevice\n");
> > >> -		return -EINVAL;
> > >> +		dev_dbg(priv->dev, "Not connected to subdevice\n");
> > >> +		return 0;
> > >>  	}
> > >>  	
> > >>  	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep);
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Regards,
Niklas S�derlund