Re: [PATCH] v4l: async: make v4l2 coexists with devicetree nodes in a dt overlay

[Javi Merino]

On Wed, Nov 23, 2016 at 11:25:39AM -0300, Javier Martinez Canillas wrote:
> Hello Javi,
> 
> On 11/23/2016 07:09 AM, Javi Merino wrote:
> > In asd's configured with V4L2_ASYNC_MATCH_OF, if the v4l2 subdev is in
> > a devicetree overlay, its of_node pointer will be different each time
> > the overlay is applied.  We are not interested in matching the
> > pointer, what we want to match is that the path is the one we are
> > expecting.  Change to use of_node_cmp() so that we continue matching
> > after the overlay has been removed and reapplied.
> >
> 
> I'm still not that familiar with DT overlays (and I guess others aren't
> either) so I think that including an example of a base tree and overlay
> DTS where this is an issue, could make things more clear in the commit.
> 
> IIUC, it should be something like this?
> 
> -- base tree --
> 
> &i2c1 {
> 	camera: camera@10 {
> 		reg = <0x10>;
> 		port {
> 			cam_ep: endpoint {
> 				...
> 			};
> 		};
> 	};
> };
> 
> &media_bridge {
> 	...
> 	ports {
> 		port@0 {
> 			reg = <0>;
> 			ep: endpoint {
> 				remote-endpoint = <&cam_ep>;
> 			};
> 		};
> 	};
> };
> 
> -- overlay --
> 
> /plugin/;
> / {
> 	...
> 	fragment@0 {
> 		target = <&camera>;
> 		__overlay__ {
> 			compatible = "foo,bar";
> 			...
> 			port {
> 				cam_ep: endpoint {
> 					...
> 				};
> 			};
> 		};
> 	}
> }

Yes, that's right.  What I have is that the whole camera can be
plugged or unplugged, so the overlay adds/removes the camera node:

/ {
	fragment@0 {
		target-path = "/i2c0";
		__overlay__ {
			my_cam {
				compatible = "foo,bar";
				port {
					camera0: endpoint {
						remote-endpoint = <&vin2a>;
						...
					};
				};
			};
		};
	};

I will add it to the commit message.

> > Cc: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>
> > Cc: Javier Martinez Canillas <javier@xxxxxxxxxxxxxxx>
> > Cc: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > Signed-off-by: Javi Merino <javi.merino@xxxxxxxxxx>
> > ---
> > Hi,
> > 
> > I feel it is a bit of a hack, but I couldn't think of anything better.
> > I'm ccing devicetree@ and Pantelis because there may be a simpler
> > solution.
> >
> 
> I also couldn't think a better way to do this, since IIUC the node's name is
> the only thing that doesn't change, and is available at the time the bridge
> driver calls v4l2_async_notifier_register() when parsing the base tree.
> 
> >  drivers/media/v4l2-core/v4l2-async.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > index 5bada20..d33a17c 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -42,7 +42,8 @@ static bool match_devname(struct v4l2_subdev *sd,
> >  
> >  static bool match_of(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
> >  {
> > -	return sd->of_node == asd->match.of.node;
> > +	return !of_node_cmp(of_node_full_name(sd->of_node),
> > +			    of_node_full_name(asd->match.of.node));
> >  }
> >  
> >  static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
> > 
> 
> Reviewed-by: Javier Martinez Canillas <javier@xxxxxxxxxxxxxxx>
> 
> Best regards,
> -- 
> Javier Martinez Canillas
> Open Source Group
> Samsung Research America