Re: [PATCH 01/13] media: v4l2-fwnode: Let parse_endpoint callback decide if no remote is error
Hi Philipp,
On Tuesday, 27 February 2018 11:13:04 EET Philipp Zabel wrote:
> On Fri, 2018-02-23 at 14:47 +0200, Sakari Ailus wrote:
> > On Fri, Feb 23, 2018 at 12:16:17PM +0100, Philipp Zabel wrote:
> >> On Fri, 2018-02-23 at 12:05 +0200, Laurent Pinchart wrote:
> >>> On Friday, 23 February 2018 11:56:52 EET Philipp Zabel wrote:
> On Fri, 2018-02-23 at 11:29 +0200, Laurent Pinchart wrote:
> > On Thursday, 22 February 2018 03:39:37 EET Steve Longerbeam wrote:
> >> For some subdevices, a fwnode endpoint that has no connection to
> >> a remote endpoint may not be an error. Let the parse_endpoint
> >> callback
> >
> > make that decision in v4l2_async_notifier_fwnode_parse_endpoint().
> > If
> >
> >> the callback indicates that is not an error, skip adding the asd
> >> to the notifier and return 0.
> >>
> >> For the current users of v4l2_async_notifier_parse_fwnode_endpoints()
> >> (omap3isp, rcar-vin, intel-ipu3), return -EINVAL in the callback
> >> for unavailable remote fwnodes to maintain the previous behavior.
> >
> > I'm not sure this should be a per-driver decision.
> >
> > Generally speaking, if an endpoint node has no remote-endpoint
> > property, the endpoint node is not needed. I've always considered such
> > an endpoint node as invalid. The OF graphs DT bindings are however not
> > clear on this subject.
>
> Documentation/devicetree/bindings/graph.txt says:
> Each endpoint should contain a 'remote-endpoint' phandle property
> that points to the corresponding endpoint in the port of the
> remote device.
>
> ("should", not "must").
> >>>
> >>> The DT bindings documentation has historically used "should" to mean
> >>> "must" in many places :-( That was a big mistake.
> >>
> >> Maybe I could have worded that better? The intention was to let "should"
> >> be read as a strong suggestion, like "it is recommended", but not as a
> >> requirement.
> >
> > Is there a reason for have an endpoint without a remote-endpoint property?
>
> It allows to slightly reduce boilerplate in board device trees at the
> cost of empty endpoint nodes included from the dtsi in board DTs that
> don't use them.
>
> > The problem with should (in general when it is used when the intention is
> > "shall") is that it lets the developer to write broken DT source that is
> > still conforming to the spec.
> >
> > I don't have a strong preference to change should to shall in this
> > particular case now but I would have used "shall" to begin with.
>
> I used "should" on purpose, but I'd be fine with giving up on it when
> all current users of this loophole are transitioned away from it:
>
> git grep -A1 "endpoint {" arch/ | grep -B1 "};"
>
> I'm very much against enforcing a required remote-endpoint property in
> core DT parsing code, though.
Just to make it clear, I'm fine with making the property either optional or
mandatory, but I would like the rule to be global, not per-bindings. When the
OF graphs bindings were developed we reasoned that there was no use for empty
endpoints and that they should thus be forbidden. Now, if we have good use
cases for empty endpoints, I don't object them.
Regardless of what we decide I agree that we need to support existing device
trees and must thus not reject empty endpoints as invalid. We could, however,
if we decide to forbid empty endpoints, print a warning to encourage DT
authors to fix the problem.
> Later, the remote-node property explicitly lists the remote-endpoint
> property as optional.
> >>>
> >>> I've seen that too, and that's why I mentioned that the documentation
> >>> isn't clear on the subject.
> >>
> >> Do you have a suggestion how to improve the documentation? I thought
> >> listing the remote-endpoint property under a header called "Optional
> >> endpoint properties" was pretty clear.
> >>
> >>> This could also be achieved by adding the endpoints in the board DT
> >>> files. See for instance the hdmi@fead node in
> >>> arch/arm64/boot/dts/renesas/ r8a7795.dtsi and how it gets extended in
> >>> arch/arm64/boot/dts/renesas/r8a7795- salvator-x.dts. On the other
> >>> hand, I also have empty endpoints in the display@feb0 node of
> >>> arch/arm64/boot/dts/renesas/r8a7795.dtsi.
> >>
> >> Right, that would be possible.
> >>
> >>> I think we should first decide what we want to do going forward
> >>> (allowing for empty endpoints or not), clarify the documentation, and
> >>> then update the code. In any case I don't think it should be a
> >>> per-device decision.
> >>
> >> There are device trees in the wild that have those empty endpoints, so I
> >> don't think retroactively declaring the remote-endpoint property
> >> required is a good idea.
> >
> > You could IMO, but the kernel (and existing drivers) would still need to
> > work with DT binaries without those bits. And leave comments in the code
> > why it's
Re: [PATCH 01/13] media: v4l2-fwnode: Let parse_endpoint callback decide if no remote is error
Hi Sakari,
On Fri, 2018-02-23 at 14:47 +0200, Sakari Ailus wrote:
> Hi Philipp,
>
> On Fri, Feb 23, 2018 at 12:16:17PM +0100, Philipp Zabel wrote:
> > Hi Laurent,
> >
> > On Fri, 2018-02-23 at 12:05 +0200, Laurent Pinchart wrote:
> > > Hi Philipp,
> > >
> > > On Friday, 23 February 2018 11:56:52 EET Philipp Zabel wrote:
> > > > On Fri, 2018-02-23 at 11:29 +0200, Laurent Pinchart wrote:
> > > > > On Thursday, 22 February 2018 03:39:37 EET Steve Longerbeam wrote:
> > > > > > For some subdevices, a fwnode endpoint that has no connection to a
> > > > > > remote endpoint may not be an error. Let the parse_endpoint callback
> > > > >
> > > > > make that decision in v4l2_async_notifier_fwnode_parse_endpoint(). If
> > > > > > the callback indicates that is not an error, skip adding the asd to
> > > > > > the
> > > > > > notifier and return 0.
> > > > > >
> > > > > > For the current users of
> > > > > > v4l2_async_notifier_parse_fwnode_endpoints()
> > > > > > (omap3isp, rcar-vin, intel-ipu3), return -EINVAL in the callback for
> > > > > > unavailable remote fwnodes to maintain the previous behavior.
> > > > >
> > > > > I'm not sure this should be a per-driver decision.
> > > > >
> > > > > Generally speaking, if an endpoint node has no remote-endpoint
> > > > > property,
> > > > > the endpoint node is not needed. I've always considered such an
> > > > > endpoint
> > > > > node as invalid. The OF graphs DT bindings are however not clear on
> > > > > this
> > > > > subject.
> > > >
> > > > Documentation/devicetree/bindings/graph.txt says:
> > > >
> > > > Each endpoint should contain a 'remote-endpoint' phandle property
> > > > that points to the corresponding endpoint in the port of the remote
> > > > device.
> > > >
> > > > ("should", not "must").
> > >
> > > The DT bindings documentation has historically used "should" to mean
> > > "must" in
> > > many places :-( That was a big mistake.
> >
> > Maybe I could have worded that better? The intention was to let "should"
> > be read as a strong suggestion, like "it is recommended", but not as a
> > requirement.
>
> Is there a reason for have an endpoint without a remote-endpoint property?
It allows to slightly reduce boilerplate in board device trees at the
cost of empty endpoint nodes included from the dtsi in board DTs that
don't use them.
> The problem with should (in general when it is used when the intention is
> "shall") is that it lets the developer to write broken DT source that is
> still conforming to the spec.
>
> I don't have a strong preference to change should to shall in this
> particular case now but I would have used "shall" to begin with.
I used "should" on purpose, but I'd be fine with giving up on it when
all current users of this loophole are transitioned away from it:
git grep -A1 "endpoint {" arch/ | grep -B1 "};"
I'm very much against enforcing a required remote-endpoint property in
core DT parsing code, though.
> > > > Later, the remote-node property explicitly lists the remote-endpoint
> > > > property as optional.
> > >
> > > I've seen that too, and that's why I mentioned that the documentation
> > > isn't
> > > clear on the subject.
> >
> > Do you have a suggestion how to improve the documentation? I thought
> > listing the remote-endpoint property under a header called "Optional
> > endpoint properties" was pretty clear.
> >
> > > This could also be achieved by adding the endpoints in the board DT
> > > files. See
> > > for instance the hdmi@fead node in arch/arm64/boot/dts/renesas/
> > > r8a7795.dtsi and how it gets extended in
> > > arch/arm64/boot/dts/renesas/r8a7795-
> > > salvator-x.dts. On the other hand, I also have empty endpoints in the
> > > display@feb0 node of arch/arm64/boot/dts/renesas/r8a7795.dtsi.
> >
> > Right, that would be possible.
> >
> > > I think we should first decide what we want to do going forward (allowing
> > > for
> > > empty endpoints or not), clarify the documentation, and then update the
> > > code.
> > > In any case I don't think it should be a per-device decision.
> >
> > There are device trees in the wild that have those empty endpoints, so I
> > don't think retroactively declaring the remote-endpoint property
> > required is a good idea.
>
> You could IMO, but the kernel (and existing drivers) would still need to
> work with DT binaries without those bits. And leave comments in the code
> why it's there.
>
> >
> > Is there any driver that currently benefits from throwing an error on
> > empty endpoints in any way? I'd prefer to just let the core ignore empty
> > endpoints for all drivers.
>
> Not necessarily, but it's overhead in parsing the DT as well as in the
> DT source and in the DT binary.
True. I suppose whether or not that is enough reason to change the
wording in the existing of-graph bindings is something to be decided on
the devicetree list (in Cc).
regards
Philipp
Re: [PATCH 01/13] media: v4l2-fwnode: Let parse_endpoint callback decide if no remote is error
Hi all,
On 02/23/2018 03:16 AM, Philipp Zabel wrote:
Hi Laurent,
On Fri, 2018-02-23 at 12:05 +0200, Laurent Pinchart wrote:
Hi Philipp,
On Friday, 23 February 2018 11:56:52 EET Philipp Zabel wrote:
On Fri, 2018-02-23 at 11:29 +0200, Laurent Pinchart wrote:
On Thursday, 22 February 2018 03:39:37 EET Steve Longerbeam wrote:
For some subdevices, a fwnode endpoint that has no connection to a
remote endpoint may not be an error. Let the parse_endpoint callback
make that decision in v4l2_async_notifier_fwnode_parse_endpoint(). If
the callback indicates that is not an error, skip adding the asd to the
notifier and return 0.
For the current users of v4l2_async_notifier_parse_fwnode_endpoints()
(omap3isp, rcar-vin, intel-ipu3), return -EINVAL in the callback for
unavailable remote fwnodes to maintain the previous behavior.
I'm not sure this should be a per-driver decision.
Generally speaking, if an endpoint node has no remote-endpoint property,
the endpoint node is not needed. I've always considered such an endpoint
node as invalid. The OF graphs DT bindings are however not clear on this
subject.
Documentation/devicetree/bindings/graph.txt says:
Each endpoint should contain a 'remote-endpoint' phandle property
that points to the corresponding endpoint in the port of the remote
device.
("should", not "must").
The DT bindings documentation has historically used "should" to mean "must" in
many places :-( That was a big mistake.
Maybe I could have worded that better? The intention was to let "should"
be read as a strong suggestion, like "it is recommended", but not as a
requirement.
Later, the remote-node property explicitly lists the remote-endpoint
property as optional.
I've seen that too, and that's why I mentioned that the documentation isn't
clear on the subject.
Do you have a suggestion how to improve the documentation? I thought
listing the remote-endpoint property under a header called "Optional
endpoint properties" was pretty clear.
This could also be achieved by adding the endpoints in the board DT files. See
for instance the hdmi@fead node in arch/arm64/boot/dts/renesas/
r8a7795.dtsi and how it gets extended in arch/arm64/boot/dts/renesas/r8a7795-
salvator-x.dts. On the other hand, I also have empty endpoints in the
display@feb0 node of arch/arm64/boot/dts/renesas/r8a7795.dtsi.
Right, that would be possible.
Yes, I think this is doable in the specific case of the video mux device.
For example on the i.MX6 SabreAuto reference boards, only the parallel
bus mux input is connected to a device (to adv7180). The MIPI CSI-2 mux
input is left unconnected.
So probably the right approach is to not define any endpoint nodes under the
unused mux input ports.
The video-mux driver will need to be modified to enumerate ports, instead of
endpoints, to determine its number of mux inputs.
I will start looking into this change for v2.
The trick would be to do this while still remaining compatible with old
DTB's in the wild with unconnected endpoints. Unfortunately that might
not be possible, unless we were to let v4l2-core ignore empty endpoints.
I think we should first decide what we want to do going forward (allowing for
empty endpoints or not), clarify the documentation, and then update the code.
In any case I don't think it should be a per-device decision.
There are device trees in the wild that have those empty endpoints, so I
don't think retroactively declaring the remote-endpoint property
required is a good idea.
Is there any driver that currently benefits from throwing an error on
empty endpoints in any way? I'd prefer to just let the core ignore empty
endpoints for all drivers.
I tend to agree that, given the fuzziness of the DT binding docs regarding
unconnected endpoints, v4l2-core should ignore them.
In v2 I can modify this patch to ignore empty endpoints without error and
continue with endpoint parsing rather than pass the empty endpoint to the
caller's parse_endpoint callback.
Or this patch can be dropped altogether. It won't be needed for i.MX6 after
making the change described above, but again if this patch is dropped it
will
break b/w compatibility with old i.MX6 DTB's.
Steve
Re: [PATCH 01/13] media: v4l2-fwnode: Let parse_endpoint callback decide if no remote is error
Hi Philipp,
On Fri, Feb 23, 2018 at 12:16:17PM +0100, Philipp Zabel wrote:
> Hi Laurent,
>
> On Fri, 2018-02-23 at 12:05 +0200, Laurent Pinchart wrote:
> > Hi Philipp,
> >
> > On Friday, 23 February 2018 11:56:52 EET Philipp Zabel wrote:
> > > On Fri, 2018-02-23 at 11:29 +0200, Laurent Pinchart wrote:
> > > > On Thursday, 22 February 2018 03:39:37 EET Steve Longerbeam wrote:
> > > > > For some subdevices, a fwnode endpoint that has no connection to a
> > > > > remote endpoint may not be an error. Let the parse_endpoint callback
> > > >
> > > > make that decision in v4l2_async_notifier_fwnode_parse_endpoint(). If
> > > > > the callback indicates that is not an error, skip adding the asd to
> > > > > the
> > > > > notifier and return 0.
> > > > >
> > > > > For the current users of v4l2_async_notifier_parse_fwnode_endpoints()
> > > > > (omap3isp, rcar-vin, intel-ipu3), return -EINVAL in the callback for
> > > > > unavailable remote fwnodes to maintain the previous behavior.
> > > >
> > > > I'm not sure this should be a per-driver decision.
> > > >
> > > > Generally speaking, if an endpoint node has no remote-endpoint property,
> > > > the endpoint node is not needed. I've always considered such an endpoint
> > > > node as invalid. The OF graphs DT bindings are however not clear on this
> > > > subject.
> > >
> > > Documentation/devicetree/bindings/graph.txt says:
> > >
> > > Each endpoint should contain a 'remote-endpoint' phandle property
> > > that points to the corresponding endpoint in the port of the remote
> > > device.
> > >
> > > ("should", not "must").
> >
> > The DT bindings documentation has historically used "should" to mean "must"
> > in
> > many places :-( That was a big mistake.
>
> Maybe I could have worded that better? The intention was to let "should"
> be read as a strong suggestion, like "it is recommended", but not as a
> requirement.
Is there a reason for have an endpoint without a remote-endpoint property?
The problem with should (in general when it is used when the intention is
"shall") is that it lets the developer to write broken DT source that is
still conforming to the spec.
I don't have a strong preference to change should to shall in this
particular case now but I would have used "shall" to begin with.
>
> > > Later, the remote-node property explicitly lists the remote-endpoint
> > > property as optional.
> >
> > I've seen that too, and that's why I mentioned that the documentation isn't
> > clear on the subject.
>
> Do you have a suggestion how to improve the documentation? I thought
> listing the remote-endpoint property under a header called "Optional
> endpoint properties" was pretty clear.
>
> > This could also be achieved by adding the endpoints in the board DT files.
> > See
> > for instance the hdmi@fead node in arch/arm64/boot/dts/renesas/
> > r8a7795.dtsi and how it gets extended in
> > arch/arm64/boot/dts/renesas/r8a7795-
> > salvator-x.dts. On the other hand, I also have empty endpoints in the
> > display@feb0 node of arch/arm64/boot/dts/renesas/r8a7795.dtsi.
>
> Right, that would be possible.
>
> > I think we should first decide what we want to do going forward (allowing
> > for
> > empty endpoints or not), clarify the documentation, and then update the
> > code.
> > In any case I don't think it should be a per-device decision.
>
> There are device trees in the wild that have those empty endpoints, so I
> don't think retroactively declaring the remote-endpoint property
> required is a good idea.
You could IMO, but the kernel (and existing drivers) would still need to
work with DT binaries without those bits. And leave comments in the code
why it's there.
>
> Is there any driver that currently benefits from throwing an error on
> empty endpoints in any way? I'd prefer to just let the core ignore empty
> endpoints for all drivers.
Not necessarily, but it's overhead in parsing the DT as well as in the
DT source and in the DT binary.
--
Regards,
Sakari Ailus
sakari.ai...@linux.intel.com
Re: [PATCH 01/13] media: v4l2-fwnode: Let parse_endpoint callback decide if no remote is error
Hi Laurent,
On Fri, 2018-02-23 at 12:05 +0200, Laurent Pinchart wrote:
> Hi Philipp,
>
> On Friday, 23 February 2018 11:56:52 EET Philipp Zabel wrote:
> > On Fri, 2018-02-23 at 11:29 +0200, Laurent Pinchart wrote:
> > > On Thursday, 22 February 2018 03:39:37 EET Steve Longerbeam wrote:
> > > > For some subdevices, a fwnode endpoint that has no connection to a
> > > > remote endpoint may not be an error. Let the parse_endpoint callback
> > >
> > > make that decision in v4l2_async_notifier_fwnode_parse_endpoint(). If
> > > > the callback indicates that is not an error, skip adding the asd to the
> > > > notifier and return 0.
> > > >
> > > > For the current users of v4l2_async_notifier_parse_fwnode_endpoints()
> > > > (omap3isp, rcar-vin, intel-ipu3), return -EINVAL in the callback for
> > > > unavailable remote fwnodes to maintain the previous behavior.
> > >
> > > I'm not sure this should be a per-driver decision.
> > >
> > > Generally speaking, if an endpoint node has no remote-endpoint property,
> > > the endpoint node is not needed. I've always considered such an endpoint
> > > node as invalid. The OF graphs DT bindings are however not clear on this
> > > subject.
> >
> > Documentation/devicetree/bindings/graph.txt says:
> >
> > Each endpoint should contain a 'remote-endpoint' phandle property
> > that points to the corresponding endpoint in the port of the remote
> > device.
> >
> > ("should", not "must").
>
> The DT bindings documentation has historically used "should" to mean "must"
> in
> many places :-( That was a big mistake.
Maybe I could have worded that better? The intention was to let "should"
be read as a strong suggestion, like "it is recommended", but not as a
requirement.
> > Later, the remote-node property explicitly lists the remote-endpoint
> > property as optional.
>
> I've seen that too, and that's why I mentioned that the documentation isn't
> clear on the subject.
Do you have a suggestion how to improve the documentation? I thought
listing the remote-endpoint property under a header called "Optional
endpoint properties" was pretty clear.
> This could also be achieved by adding the endpoints in the board DT files.
> See
> for instance the hdmi@fead node in arch/arm64/boot/dts/renesas/
> r8a7795.dtsi and how it gets extended in arch/arm64/boot/dts/renesas/r8a7795-
> salvator-x.dts. On the other hand, I also have empty endpoints in the
> display@feb0 node of arch/arm64/boot/dts/renesas/r8a7795.dtsi.
Right, that would be possible.
> I think we should first decide what we want to do going forward (allowing for
> empty endpoints or not), clarify the documentation, and then update the code.
> In any case I don't think it should be a per-device decision.
There are device trees in the wild that have those empty endpoints, so I
don't think retroactively declaring the remote-endpoint property
required is a good idea.
Is there any driver that currently benefits from throwing an error on
empty endpoints in any way? I'd prefer to just let the core ignore empty
endpoints for all drivers.
regards
Philipp
Re: [PATCH 01/13] media: v4l2-fwnode: Let parse_endpoint callback decide if no remote is error
Hi guys,
On Fri, Feb 23, 2018 at 12:05:38PM +0200, Laurent Pinchart wrote:
> Hi Philipp,
>
> On Friday, 23 February 2018 11:56:52 EET Philipp Zabel wrote:
> > On Fri, 2018-02-23 at 11:29 +0200, Laurent Pinchart wrote:
> > > On Thursday, 22 February 2018 03:39:37 EET Steve Longerbeam wrote:
> > >> For some subdevices, a fwnode endpoint that has no connection to a
> > >> remote endpoint may not be an error. Let the parse_endpoint callback
> > > make that decision in v4l2_async_notifier_fwnode_parse_endpoint(). If
> > >> the callback indicates that is not an error, skip adding the asd to the
> > >> notifier and return 0.
> > >>
> > >> For the current users of v4l2_async_notifier_parse_fwnode_endpoints()
> > >> (omap3isp, rcar-vin, intel-ipu3), return -EINVAL in the callback for
> > >> unavailable remote fwnodes to maintain the previous behavior.
> > >
> > > I'm not sure this should be a per-driver decision.
> > >
> > > Generally speaking, if an endpoint node has no remote-endpoint property,
> > > the endpoint node is not needed. I've always considered such an endpoint
> > > node as invalid. The OF graphs DT bindings are however not clear on this
> > > subject.
> >
> > Documentation/devicetree/bindings/graph.txt says:
> >
> > Each endpoint should contain a 'remote-endpoint' phandle property
> > that points to the corresponding endpoint in the port of the remote
> > device.
> >
> > ("should", not "must").
>
> The DT bindings documentation has historically used "should" to mean "must"
> in
> many places :-( That was a big mistake.
"Shall", not "must".
Indeed, there's hardly use for an endpoint without the remote-endpoint
property. My understanding is that such nodes might be best ignored, that's
been at least the practice in a lot of DT parsing code. There are arguments
both ways I guess.
What comes to this patch is that I'd rather not burden individual drivers
with such checks.
>
> > Later, the remote-node property explicitly lists the remote-endpoint
> > property as optional.
>
> I've seen that too, and that's why I mentioned that the documentation isn't
> clear on the subject.
>
> > > I have either failed to notice when they got merged, or they slowly
> > > evolved over time to contain contradictory information. In any case, I
> > > think we should decide on whether such a situation is valid or not from
> > > an OF graph point of view, and then always reject or always accept and
> > > ignore those endpoints.
> >
> > We are currently using this on i.MX6 to provide empty labeled endpoints
> > in the dtsi files for board DT writers to link to, both for the display
> > output and video capture ports.
> > See for example the endpoints with the labels ipu1_di0_disp0 and
> > ipu1_csi0_mux_from_parallel_sensor in arch/arm/boot/dts/imx6q.dtsi.
>
> This could also be achieved by adding the endpoints in the board DT files.
> See
> for instance the hdmi@fead node in arch/arm64/boot/dts/renesas/
> r8a7795.dtsi and how it gets extended in arch/arm64/boot/dts/renesas/r8a7795-
> salvator-x.dts. On the other hand, I also have empty endpoints in the
> display@feb0 node of arch/arm64/boot/dts/renesas/r8a7795.dtsi.
>
> I think we should first decide what we want to do going forward (allowing for
> empty endpoints or not), clarify the documentation, and then update the code.
> In any case I don't think it should be a per-device decision.
I don't think they should be allowed in the documentation. The
implementation could still simply ignore them.
Cc the devicetree list.
--
Regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi
Re: [PATCH 01/13] media: v4l2-fwnode: Let parse_endpoint callback decide if no remote is error
Hi Philipp,
On Friday, 23 February 2018 11:56:52 EET Philipp Zabel wrote:
> On Fri, 2018-02-23 at 11:29 +0200, Laurent Pinchart wrote:
> > On Thursday, 22 February 2018 03:39:37 EET Steve Longerbeam wrote:
> >> For some subdevices, a fwnode endpoint that has no connection to a
> >> remote endpoint may not be an error. Let the parse_endpoint callback
> > make that decision in v4l2_async_notifier_fwnode_parse_endpoint(). If
> >> the callback indicates that is not an error, skip adding the asd to the
> >> notifier and return 0.
> >>
> >> For the current users of v4l2_async_notifier_parse_fwnode_endpoints()
> >> (omap3isp, rcar-vin, intel-ipu3), return -EINVAL in the callback for
> >> unavailable remote fwnodes to maintain the previous behavior.
> >
> > I'm not sure this should be a per-driver decision.
> >
> > Generally speaking, if an endpoint node has no remote-endpoint property,
> > the endpoint node is not needed. I've always considered such an endpoint
> > node as invalid. The OF graphs DT bindings are however not clear on this
> > subject.
>
> Documentation/devicetree/bindings/graph.txt says:
>
> Each endpoint should contain a 'remote-endpoint' phandle property
> that points to the corresponding endpoint in the port of the remote
> device.
>
> ("should", not "must").
The DT bindings documentation has historically used "should" to mean "must" in
many places :-( That was a big mistake.
> Later, the remote-node property explicitly lists the remote-endpoint
> property as optional.
I've seen that too, and that's why I mentioned that the documentation isn't
clear on the subject.
> > I have either failed to notice when they got merged, or they slowly
> > evolved over time to contain contradictory information. In any case, I
> > think we should decide on whether such a situation is valid or not from
> > an OF graph point of view, and then always reject or always accept and
> > ignore those endpoints.
>
> We are currently using this on i.MX6 to provide empty labeled endpoints
> in the dtsi files for board DT writers to link to, both for the display
> output and video capture ports.
> See for example the endpoints with the labels ipu1_di0_disp0 and
> ipu1_csi0_mux_from_parallel_sensor in arch/arm/boot/dts/imx6q.dtsi.
This could also be achieved by adding the endpoints in the board DT files. See
for instance the hdmi@fead node in arch/arm64/boot/dts/renesas/
r8a7795.dtsi and how it gets extended in arch/arm64/boot/dts/renesas/r8a7795-
salvator-x.dts. On the other hand, I also have empty endpoints in the
display@feb0 node of arch/arm64/boot/dts/renesas/r8a7795.dtsi.
I think we should first decide what we want to do going forward (allowing for
empty endpoints or not), clarify the documentation, and then update the code.
In any case I don't think it should be a per-device decision.
--
Regards,
Laurent Pinchart
Re: [PATCH 01/13] media: v4l2-fwnode: Let parse_endpoint callback decide if no remote is error
Hi Laurent,
On Fri, 2018-02-23 at 11:29 +0200, Laurent Pinchart wrote:
> Hi Steve,
>
> Thank you for the patch.
>
> On Thursday, 22 February 2018 03:39:37 EET Steve Longerbeam wrote:
> > For some subdevices, a fwnode endpoint that has no connection to a remote
> > endpoint may not be an error. Let the parse_endpoint callback make that
> > decision in v4l2_async_notifier_fwnode_parse_endpoint(). If the callback
> > indicates that is not an error, skip adding the asd to the notifier and
> > return 0.
> >
> > For the current users of v4l2_async_notifier_parse_fwnode_endpoints()
> > (omap3isp, rcar-vin, intel-ipu3), return -EINVAL in the callback for
> > unavailable remote fwnodes to maintain the previous behavior.
>
> I'm not sure this should be a per-driver decision.
>
> Generally speaking, if an endpoint node has no remote-endpoint property, the
> endpoint node is not needed. I've always considered such an endpoint node as
> invalid. The OF graphs DT bindings are however not clear on this subject.
Documentation/devicetree/bindings/graph.txt says:
Each endpoint should contain a 'remote-endpoint' phandle property
that points to the corresponding endpoint in the port of the remote
device.
("should", not "must"). Later, the remote-node property explicitly lists
the remote-endpoint property as optional.
> I have either failed to notice when they got merged, or they slowly evolved
> over
> time to contain contradictory information. In any case, I think we should
> decide on whether such a situation is valid or not from an OF graph point of
> view, and then always reject or always accept and ignore those endpoints.
We are currently using this on i.MX6 to provide empty labeled endpoints
in the dtsi files for board DT writers to link to, both for the display
output and video capture ports.
See for example the endpoints with the labels ipu1_di0_disp0 and
ipu1_csi0_mux_from_parallel_sensor in arch/arm/boot/dts/imx6q.dtsi.
regards
Philipp
Re: [PATCH 01/13] media: v4l2-fwnode: Let parse_endpoint callback decide if no remote is error
Hi Steve,
Thank you for the patch.
On Thursday, 22 February 2018 03:39:37 EET Steve Longerbeam wrote:
> For some subdevices, a fwnode endpoint that has no connection to a remote
> endpoint may not be an error. Let the parse_endpoint callback make that
> decision in v4l2_async_notifier_fwnode_parse_endpoint(). If the callback
> indicates that is not an error, skip adding the asd to the notifier and
> return 0.
>
> For the current users of v4l2_async_notifier_parse_fwnode_endpoints()
> (omap3isp, rcar-vin, intel-ipu3), return -EINVAL in the callback for
> unavailable remote fwnodes to maintain the previous behavior.
I'm not sure this should be a per-driver decision.
Generally speaking, if an endpoint node has no remote-endpoint property, the
endpoint node is not needed. I've always considered such an endpoint node as
invalid. The OF graphs DT bindings are however not clear on this subject. I
have either failed to notice when they got merged, or they slowly evolved over
time to contain contradictory information. In any case, I think we should
decide on whether such a situation is valid or not from an OF graph point of
view, and then always reject or always accept and ignore those endpoints.
> Signed-off-by: Steve Longerbeam
> ---
> drivers/media/pci/intel/ipu3/ipu3-cio2.c| 3 +++
> drivers/media/platform/omap3isp/isp.c | 3 +++
> drivers/media/platform/rcar-vin/rcar-core.c | 3 +++
> drivers/media/v4l2-core/v4l2-fwnode.c | 4 ++--
> 4 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> b/drivers/media/pci/intel/ipu3/ipu3-cio2.c index 6cb..2323151 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> @@ -1477,6 +1477,9 @@ static int cio2_fwnode_parse(struct device *dev,
> struct sensor_async_subdev *s_asd =
> container_of(asd, struct sensor_async_subdev, asd);
>
> + if (!fwnode_device_is_available(asd->match.fwnode))
> + return -EINVAL;
> +
> if (vep->bus_type != V4L2_MBUS_CSI2) {
> dev_err(dev, "Only CSI2 bus type is currently supported\n");
> return -EINVAL;
> diff --git a/drivers/media/platform/omap3isp/isp.c
> b/drivers/media/platform/omap3isp/isp.c index 8eb000e..4a302f2 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2025,6 +2025,9 @@ static int isp_fwnode_parse(struct device *dev,
> dev_dbg(dev, "parsing endpoint %pOF, interface %u\n",
> to_of_node(vep->base.local_fwnode), vep->base.port);
>
> + if (!fwnode_device_is_available(asd->match.fwnode))
> + return -EINVAL;
> +
> switch (vep->base.port) {
> case ISP_OF_PHY_PARALLEL:
> buscfg->interface = ISP_INTERFACE_PARALLEL;
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> b/drivers/media/platform/rcar-vin/rcar-core.c index f1fc797..51bb8f1 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -149,6 +149,9 @@ static int rvin_digital_parse_v4l2(struct device *dev,
> struct rvin_graph_entity *rvge =
> container_of(asd, struct rvin_graph_entity, asd);
>
> + if (!fwnode_device_is_available(asd->match.fwnode))
> + return -EINVAL;
> +
> if (vep->base.port || vep->base.id)
> return -ENOTCONN;
>
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c
> b/drivers/media/v4l2-core/v4l2-fwnode.c index d630640..446646b 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -361,7 +361,7 @@ static int v4l2_async_notifier_fwnode_parse_endpoint(
> asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
> asd->match.fwnode =
> fwnode_graph_get_remote_port_parent(endpoint);
> - if (!asd->match.fwnode) {
> + if (!asd->match.fwnode && !parse_endpoint) {
> dev_warn(dev, "bad remote port parent\n");
> ret = -EINVAL;
> goto out_err;
> @@ -384,7 +384,7 @@ static int v4l2_async_notifier_fwnode_parse_endpoint(
>"driver could not parse port@%u/endpoint@%u (%d)\n",
>vep->base.port, vep->base.id, ret);
> v4l2_fwnode_endpoint_free(vep);
> - if (ret < 0)
> + if (ret < 0 || !asd->match.fwnode)
> goto out_err;
>
> notifier->subdevs[notifier->num_subdevs] = asd;
--
Regards,
Laurent Pinchart
