LinuxTV Patchwork [v1.1,4/4] ti-vpe: Parse local endpoint for properties, not the remote one

login
register
mail settings
Submitter Sakari Ailus
Date March 5, 2019, 2:02 p.m.
Message ID <20190305140224.25889-1-sakari.ailus@linux.intel.com>
Download mbox | patch
Permalink /patch/54850/
State Accepted
Headers show

Comments

Sakari Ailus - March 5, 2019, 2:02 p.m.
ti-vpe driver parsed the remote endpoints for properties but ignored the
local ones. Fix this by parsing the local endpoint properties instead.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
since v1:

- Remove of_node_put(remote_ep) as well, the only remaining reference to it.

 drivers/media/platform/ti-vpe/cal.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)
Benoit Parrot - March 5, 2019, 2:34 p.m.
Sakari,

Thank you for the patch.

Sakari Ailus <sakari.ailus@linux.intel.com> wrote on Tue [2019-Mar-05 16:02:24 +0200]:
> ti-vpe driver parsed the remote endpoints for properties but ignored the
> local ones. Fix this by parsing the local endpoint properties instead.

I am not sure I understand the logic here.  For CSI2 sensor as far as I
understand the lane mapping (clock and data) is driven from the sensor
side. The bridge driver (in this case CAL) needs to setup the receiver side
based on what the sensor (aka remote endpoint) can provide.

I failed to see how this fixes things here.

Are you suggesting that sensor relevant properties be set (and effectively
duplicated) on the bridge/receiver side?

Some sensor can and do handle multiple data lanes configuration so the
sensor driver also needs to use those properties at probe time, duplicating
the lane data is just asking for a mismatch to happen, no?

Benoit

> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> since v1:
> 
> - Remove of_node_put(remote_ep) as well, the only remaining reference to it.
> 
>  drivers/media/platform/ti-vpe/cal.c | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
> index fc3c212b96e1..8d075683e448 100644
> --- a/drivers/media/platform/ti-vpe/cal.c
> +++ b/drivers/media/platform/ti-vpe/cal.c
> @@ -1643,8 +1643,7 @@ of_get_next_endpoint(const struct device_node *parent,
>  static int of_cal_create_instance(struct cal_ctx *ctx, int inst)
>  {
>  	struct platform_device *pdev = ctx->dev->pdev;
> -	struct device_node *ep_node, *port, *remote_ep,
> -			*sensor_node, *parent;
> +	struct device_node *ep_node, *port, *sensor_node, *parent;
>  	struct v4l2_fwnode_endpoint *endpoint;
>  	struct v4l2_async_subdev *asd;
>  	u32 regval = 0;
> @@ -1657,7 +1656,6 @@ static int of_cal_create_instance(struct cal_ctx *ctx, int inst)
>  
>  	ep_node = NULL;
>  	port = NULL;
> -	remote_ep = NULL;
>  	sensor_node = NULL;
>  	ret = -EINVAL;
>  
> @@ -1703,12 +1701,7 @@ static int of_cal_create_instance(struct cal_ctx *ctx, int inst)
>  	asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
>  	asd->match.fwnode = of_fwnode_handle(sensor_node);
>  
> -	remote_ep = of_graph_get_remote_endpoint(ep_node);
> -	if (!remote_ep) {
> -		ctx_dbg(3, ctx, "can't get remote-endpoint\n");
> -		goto cleanup_exit;
> -	}
> -	v4l2_fwnode_endpoint_parse(of_fwnode_handle(remote_ep), endpoint);
> +	v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep_node), endpoint);
>  
>  	if (endpoint->bus_type != V4L2_MBUS_CSI2_DPHY) {
>  		ctx_err(ctx, "Port:%d sub-device %pOFn is not a CSI2 device\n",
> @@ -1759,7 +1752,6 @@ static int of_cal_create_instance(struct cal_ctx *ctx, int inst)
>  	sensor_node = NULL;
>  
>  cleanup_exit:
> -	of_node_put(remote_ep);
>  	of_node_put(sensor_node);
>  	of_node_put(ep_node);
>  	of_node_put(port);
> -- 
> 2.11.0
>
Sakari Ailus - March 5, 2019, 4:32 p.m.
Hi Benoit,

On Tue, Mar 05, 2019 at 08:34:09AM -0600, Benoit Parrot wrote:
> Sakari,
> 
> Thank you for the patch.
> 
> Sakari Ailus <sakari.ailus@linux.intel.com> wrote on Tue [2019-Mar-05 16:02:24 +0200]:
> > ti-vpe driver parsed the remote endpoints for properties but ignored the
> > local ones. Fix this by parsing the local endpoint properties instead.
> 
> I am not sure I understand the logic here.  For CSI2 sensor as far as I
> understand the lane mapping (clock and data) is driven from the sensor
> side. The bridge driver (in this case CAL) needs to setup the receiver side
> based on what the sensor (aka remote endpoint) can provide.
> 
> I failed to see how this fixes things here.
> 
> Are you suggesting that sensor relevant properties be set (and effectively
> duplicated) on the bridge/receiver side?

Yes. The endpoint configuration in general is local to the device and
should not be accessed from other device drivers.

The lane mapping, for instance, is specific to a given device --- and may
differ even between for two connected endpoints. It's used to reorder the
PHY lanes (if the device supports that). Same goes for the clock lane.

See e.g. arch/arm/boot/dts/omap3-n9.dts .

> 
> Some sensor can and do handle multiple data lanes configuration so the
> sensor driver also needs to use those properties at probe time, duplicating
> the lane data is just asking for a mismatch to happen, no?

It's a different configuration on the sensor side. We currently have no
checks in place to verify that the two would match. I haven't heard of this
would have really been a problem though.

The frame descriptors should be used for runtime configuration. Niklas and
more recently Jacopo have been working on that.
Benoit Parrot - March 5, 2019, 5:38 p.m.
Sakari Ailus <sakari.ailus@linux.intel.com> wrote on Tue [2019-Mar-05 18:32:40 +0200]:
> Hi Benoit,
> 
> On Tue, Mar 05, 2019 at 08:34:09AM -0600, Benoit Parrot wrote:
> > Sakari,
> > 
> > Thank you for the patch.
> > 
> > Sakari Ailus <sakari.ailus@linux.intel.com> wrote on Tue [2019-Mar-05 16:02:24 +0200]:
> > > ti-vpe driver parsed the remote endpoints for properties but ignored the
> > > local ones. Fix this by parsing the local endpoint properties instead.
> > 
> > I am not sure I understand the logic here.  For CSI2 sensor as far as I
> > understand the lane mapping (clock and data) is driven from the sensor
> > side. The bridge driver (in this case CAL) needs to setup the receiver side
> > based on what the sensor (aka remote endpoint) can provide.
> > 
> > I failed to see how this fixes things here.
> > 
> > Are you suggesting that sensor relevant properties be set (and effectively
> > duplicated) on the bridge/receiver side?
> 
> Yes. The endpoint configuration in general is local to the device and
> should not be accessed from other device drivers.
> 
> The lane mapping, for instance, is specific to a given device --- and may
> differ even between for two connected endpoints. It's used to reorder the
> PHY lanes (if the device supports that). Same goes for the clock lane.

I did not see omap3isp having lane reorder capability, but I guess it would
be possible for instance, that a sensor uses clock lane 0 and data lane 1
& 2 but the way it is wired on the board makes it that the receiver would see
sensor lane 0 on device lane 2 and so on... Not sure why you would wire it
up that way but who knows...

> 
> See e.g. arch/arm/boot/dts/omap3-n9.dts .
> 
> > 
> > Some sensor can and do handle multiple data lanes configuration so the
> > sensor driver also needs to use those properties at probe time, duplicating
> > the lane data is just asking for a mismatch to happen, no?
> 
> It's a different configuration on the sensor side. We currently have no
> checks in place to verify that the two would match. I haven't heard of this
> would have really been a problem though.

I had just never thought about this cases, to me a single source of
information is better than 2. But anyhow I guess I'll have to update all of
my relevant dts files in the near future.

Benoit

> 
> The frame descriptors should be used for runtime configuration. Niklas and
> more recently Jacopo have been working on that.
> 
> -- 
> Kind regards,
> 
> Sakari Ailus
> sakari.ailus@linux.intel.com
Sakari Ailus - March 5, 2019, 8:48 p.m.
On Tue, Mar 05, 2019 at 11:38:42AM -0600, Benoit Parrot wrote:
> Sakari Ailus <sakari.ailus@linux.intel.com> wrote on Tue [2019-Mar-05 18:32:40 +0200]:
> > Hi Benoit,
> > 
> > On Tue, Mar 05, 2019 at 08:34:09AM -0600, Benoit Parrot wrote:
> > > Sakari,
> > > 
> > > Thank you for the patch.
> > > 
> > > Sakari Ailus <sakari.ailus@linux.intel.com> wrote on Tue [2019-Mar-05 16:02:24 +0200]:
> > > > ti-vpe driver parsed the remote endpoints for properties but ignored the
> > > > local ones. Fix this by parsing the local endpoint properties instead.
> > > 
> > > I am not sure I understand the logic here.  For CSI2 sensor as far as I
> > > understand the lane mapping (clock and data) is driven from the sensor
> > > side. The bridge driver (in this case CAL) needs to setup the receiver side
> > > based on what the sensor (aka remote endpoint) can provide.
> > > 
> > > I failed to see how this fixes things here.
> > > 
> > > Are you suggesting that sensor relevant properties be set (and effectively
> > > duplicated) on the bridge/receiver side?
> > 
> > Yes. The endpoint configuration in general is local to the device and
> > should not be accessed from other device drivers.
> > 
> > The lane mapping, for instance, is specific to a given device --- and may
> > differ even between for two connected endpoints. It's used to reorder the
> > PHY lanes (if the device supports that). Same goes for the clock lane.
> 
> I did not see omap3isp having lane reorder capability, but I guess it would
> be possible for instance, that a sensor uses clock lane 0 and data lane 1
> & 2 but the way it is wired on the board makes it that the receiver would see
> sensor lane 0 on device lane 2 and so on... Not sure why you would wire it
> up that way but who knows...

I presume the feature is there to ease PCB design.

> 
> > 
> > See e.g. arch/arm/boot/dts/omap3-n9.dts .

	     ^

There it is.

> > 
> > > 
> > > Some sensor can and do handle multiple data lanes configuration so the
> > > sensor driver also needs to use those properties at probe time, duplicating
> > > the lane data is just asking for a mismatch to happen, no?
> > 
> > It's a different configuration on the sensor side. We currently have no
> > checks in place to verify that the two would match. I haven't heard of this
> > would have really been a problem though.
> 
> I had just never thought about this cases, to me a single source of
> information is better than 2. But anyhow I guess I'll have to update all of
> my relevant dts files in the near future.

Do you have in-kernel dts files using this? I presume the driver should
then figure out whether the local endpoint has a configuration and if it
doesn't, then look it up from the remote one. Otherwise old dts binaries
will break. :-(
Benoit Parrot - March 7, 2019, 3:34 p.m.
Sakari Ailus <sakari.ailus@linux.intel.com> wrote on Tue [2019-Mar-05 22:48:44 +0200]:
> On Tue, Mar 05, 2019 at 11:38:42AM -0600, Benoit Parrot wrote:
> > Sakari Ailus <sakari.ailus@linux.intel.com> wrote on Tue [2019-Mar-05 18:32:40 +0200]:
> > > Hi Benoit,
> > > 
> > > On Tue, Mar 05, 2019 at 08:34:09AM -0600, Benoit Parrot wrote:
> > > > Sakari,
> > > > 
> > > > Thank you for the patch.
> > > > 
> > > > Sakari Ailus <sakari.ailus@linux.intel.com> wrote on Tue [2019-Mar-05 16:02:24 +0200]:
> > > > > ti-vpe driver parsed the remote endpoints for properties but ignored the
> > > > > local ones. Fix this by parsing the local endpoint properties instead.
> > > > 
> > > > I am not sure I understand the logic here.  For CSI2 sensor as far as I
> > > > understand the lane mapping (clock and data) is driven from the sensor
> > > > side. The bridge driver (in this case CAL) needs to setup the receiver side
> > > > based on what the sensor (aka remote endpoint) can provide.
> > > > 
> > > > I failed to see how this fixes things here.
> > > > 
> > > > Are you suggesting that sensor relevant properties be set (and effectively
> > > > duplicated) on the bridge/receiver side?
> > > 
> > > Yes. The endpoint configuration in general is local to the device and
> > > should not be accessed from other device drivers.
> > > 
> > > The lane mapping, for instance, is specific to a given device --- and may
> > > differ even between for two connected endpoints. It's used to reorder the
> > > PHY lanes (if the device supports that). Same goes for the clock lane.
> > 
> > I did not see omap3isp having lane reorder capability, but I guess it would
> > be possible for instance, that a sensor uses clock lane 0 and data lane 1
> > & 2 but the way it is wired on the board makes it that the receiver would see
> > sensor lane 0 on device lane 2 and so on... Not sure why you would wire it
> > up that way but who knows...
> 
> I presume the feature is there to ease PCB design.
> 
> > 
> > > 
> > > See e.g. arch/arm/boot/dts/omap3-n9.dts .
> 
> 	     ^
> 
> There it is.

Yes I saw that the sensor describes its clock-lanes as 0 and data-lanes as
1 & 2. And that the OMAP3ISP receiver describes its clock-lanes as 2 and
data-lanes as 1 & 3.

But when I looked at the driver code itself it just uses those lane config
without doing anything else, so to me that just point to the way it's wired up
on the board, nothing more. (although I have not looked into any schematics
so I am just guessing here)

> 
> > > 
> > > > 
> > > > Some sensor can and do handle multiple data lanes configuration so the
> > > > sensor driver also needs to use those properties at probe time, duplicating
> > > > the lane data is just asking for a mismatch to happen, no?
> > > 
> > > It's a different configuration on the sensor side. We currently have no
> > > checks in place to verify that the two would match. I haven't heard of this
> > > would have really been a problem though.
> > 
> > I had just never thought about this cases, to me a single source of
> > information is better than 2. But anyhow I guess I'll have to update all of
> > my relevant dts files in the near future.
> 
> Do you have in-kernel dts files using this? I presume the driver should
> then figure out whether the local endpoint has a configuration and if it
> doesn't, then look it up from the remote one. Otherwise old dts binaries
> will break. :-(

No I do not currently have any dts in mainline using this feature as of
yet. It is used in several DT file in our own kernel tree so I'll have to
update those for sure. But between our major releases we do not guarantee
DTBs backward compatibility, so depending on when I merge this we may not
need to add backward compat code.

I guess you are free to augment the patch to add backward support since this
patch is changing the current DT parsing behavior for this driver.

I do have a backlog of patches for this driver I need to up-stream.
If you prefer you can drop this patch from the series then I can include a
version of it with my set. Up to you.

Benoit

> 
> -- 
> Regards,
> 
> Sakari Ailus
> sakari.ailus@linux.intel.com
Sakari Ailus - March 7, 2019, 3:55 p.m.
On Thu, Mar 07, 2019 at 09:34:12AM -0600, Benoit Parrot wrote:
> Sakari Ailus <sakari.ailus@linux.intel.com> wrote on Tue [2019-Mar-05 22:48:44 +0200]:
> > On Tue, Mar 05, 2019 at 11:38:42AM -0600, Benoit Parrot wrote:
> > > Sakari Ailus <sakari.ailus@linux.intel.com> wrote on Tue [2019-Mar-05 18:32:40 +0200]:
> > > > Hi Benoit,
> > > > 
> > > > On Tue, Mar 05, 2019 at 08:34:09AM -0600, Benoit Parrot wrote:
> > > > > Sakari,
> > > > > 
> > > > > Thank you for the patch.
> > > > > 
> > > > > Sakari Ailus <sakari.ailus@linux.intel.com> wrote on Tue [2019-Mar-05 16:02:24 +0200]:
> > > > > > ti-vpe driver parsed the remote endpoints for properties but ignored the
> > > > > > local ones. Fix this by parsing the local endpoint properties instead.
> > > > > 
> > > > > I am not sure I understand the logic here.  For CSI2 sensor as far as I
> > > > > understand the lane mapping (clock and data) is driven from the sensor
> > > > > side. The bridge driver (in this case CAL) needs to setup the receiver side
> > > > > based on what the sensor (aka remote endpoint) can provide.
> > > > > 
> > > > > I failed to see how this fixes things here.
> > > > > 
> > > > > Are you suggesting that sensor relevant properties be set (and effectively
> > > > > duplicated) on the bridge/receiver side?
> > > > 
> > > > Yes. The endpoint configuration in general is local to the device and
> > > > should not be accessed from other device drivers.
> > > > 
> > > > The lane mapping, for instance, is specific to a given device --- and may
> > > > differ even between for two connected endpoints. It's used to reorder the
> > > > PHY lanes (if the device supports that). Same goes for the clock lane.
> > > 
> > > I did not see omap3isp having lane reorder capability, but I guess it would
> > > be possible for instance, that a sensor uses clock lane 0 and data lane 1
> > > & 2 but the way it is wired on the board makes it that the receiver would see
> > > sensor lane 0 on device lane 2 and so on... Not sure why you would wire it
> > > up that way but who knows...
> > 
> > I presume the feature is there to ease PCB design.
> > 
> > > 
> > > > 
> > > > See e.g. arch/arm/boot/dts/omap3-n9.dts .
> > 
> > 	     ^
> > 
> > There it is.
> 
> Yes I saw that the sensor describes its clock-lanes as 0 and data-lanes as
> 1 & 2. And that the OMAP3ISP receiver describes its clock-lanes as 2 and
> data-lanes as 1 & 3.
> 
> But when I looked at the driver code itself it just uses those lane config
> without doing anything else, so to me that just point to the way it's wired up
> on the board, nothing more. (although I have not looked into any schematics
> so I am just guessing here)

It is wired on the board... and that's the point here indeed. The register
configured based on this is ISPCSI2_PHY_CFG (driver name, I don't know what
the datasheet uses).

> 
> > 
> > > > 
> > > > > 
> > > > > Some sensor can and do handle multiple data lanes configuration so the
> > > > > sensor driver also needs to use those properties at probe time, duplicating
> > > > > the lane data is just asking for a mismatch to happen, no?
> > > > 
> > > > It's a different configuration on the sensor side. We currently have no
> > > > checks in place to verify that the two would match. I haven't heard of this
> > > > would have really been a problem though.
> > > 
> > > I had just never thought about this cases, to me a single source of
> > > information is better than 2. But anyhow I guess I'll have to update all of
> > > my relevant dts files in the near future.
> > 
> > Do you have in-kernel dts files using this? I presume the driver should
> > then figure out whether the local endpoint has a configuration and if it
> > doesn't, then look it up from the remote one. Otherwise old dts binaries
> > will break. :-(
> 
> No I do not currently have any dts in mainline using this feature as of
> yet. It is used in several DT file in our own kernel tree so I'll have to
> update those for sure. But between our major releases we do not guarantee
> DTBs backward compatibility, so depending on when I merge this we may not
> need to add backward compat code.
> 
> I guess you are free to augment the patch to add backward support since this
> patch is changing the current DT parsing behavior for this driver.
> 
> I do have a backlog of patches for this driver I need to up-stream.
> If you prefer you can drop this patch from the series then I can include a
> version of it with my set. Up to you.

I have an upcoming patch that changes the code again. :-) So I'll keep it
in my set.

Btw. I checked Documentation/devicetree/bindings/media/ti-cal.txt and it
seems a bit incomplete. There is no endpoint configuration on CAL side
(which is understandable now), but there's also the "slave-mode" property
which looks just inapplicable for this type or hardware.

Also "reg = <0>" can be omitted on ar0330 side. Is this btw. an Aptina
image sensor or some lesser known TI chip with somewhat similar properties?
:-) The binding file should describe relevant properties for the device as
well as the valid values for them. (Excluding graph etc. documentation
which already exists elsewhere.)

Would you like to address these? :-)

Thanks.

Patch

diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c
index fc3c212b96e1..8d075683e448 100644
--- a/drivers/media/platform/ti-vpe/cal.c
+++ b/drivers/media/platform/ti-vpe/cal.c
@@ -1643,8 +1643,7 @@  of_get_next_endpoint(const struct device_node *parent,
 static int of_cal_create_instance(struct cal_ctx *ctx, int inst)
 {
 	struct platform_device *pdev = ctx->dev->pdev;
-	struct device_node *ep_node, *port, *remote_ep,
-			*sensor_node, *parent;
+	struct device_node *ep_node, *port, *sensor_node, *parent;
 	struct v4l2_fwnode_endpoint *endpoint;
 	struct v4l2_async_subdev *asd;
 	u32 regval = 0;
@@ -1657,7 +1656,6 @@  static int of_cal_create_instance(struct cal_ctx *ctx, int inst)
 
 	ep_node = NULL;
 	port = NULL;
-	remote_ep = NULL;
 	sensor_node = NULL;
 	ret = -EINVAL;
 
@@ -1703,12 +1701,7 @@  static int of_cal_create_instance(struct cal_ctx *ctx, int inst)
 	asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
 	asd->match.fwnode = of_fwnode_handle(sensor_node);
 
-	remote_ep = of_graph_get_remote_endpoint(ep_node);
-	if (!remote_ep) {
-		ctx_dbg(3, ctx, "can't get remote-endpoint\n");
-		goto cleanup_exit;
-	}
-	v4l2_fwnode_endpoint_parse(of_fwnode_handle(remote_ep), endpoint);
+	v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep_node), endpoint);
 
 	if (endpoint->bus_type != V4L2_MBUS_CSI2_DPHY) {
 		ctx_err(ctx, "Port:%d sub-device %pOFn is not a CSI2 device\n",
@@ -1759,7 +1752,6 @@  static int of_cal_create_instance(struct cal_ctx *ctx, int inst)
 	sensor_node = NULL;
 
 cleanup_exit:
-	of_node_put(remote_ep);
 	of_node_put(sensor_node);
 	of_node_put(ep_node);
 	of_node_put(port);

Privacy Policy