| Message ID | 20200303234256.8928-7-slongerbeam@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: imx: Create media links in bound notifiers | expand |
Hi Steve, Thank you for the patch. On Tue, Mar 03, 2020 at 03:42:45PM -0800, Steve Longerbeam wrote: > Implement get_fwnode_pad operation. If the endpoint is owned by the MIPI > CSI-2 receiver, return the endpoint's port number. The MIPI CSI-2 receiver > maps port numbers and pad indexes 1:1. > > Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com> > --- > drivers/staging/media/imx/imx6-mipi-csi2.c | 28 ++++++++++++++++++++++ > 1 file changed, 28 insertions(+) > > diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c > index fdd763587e6c..8500207e5ea9 100644 > --- a/drivers/staging/media/imx/imx6-mipi-csi2.c > +++ b/drivers/staging/media/imx/imx6-mipi-csi2.c > @@ -507,9 +507,37 @@ static int csi2_registered(struct v4l2_subdev *sd) > 640, 480, 0, V4L2_FIELD_NONE, NULL); > } > > +static int csi2_get_fwnode_pad(struct media_entity *entity, > + struct fwnode_endpoint *endpoint) > +{ > + struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity); > + struct csi2_dev *csi2 = sd_to_dev(sd); > + struct fwnode_handle *csi2_ep; > + > + /* > + * If the endpoint is one of ours, return the endpoint's port > + * number. This device maps port numbers and pad indexes 1:1. > + */ > + fwnode_graph_for_each_endpoint(dev_fwnode(csi2->dev), csi2_ep) { > + if (endpoint->local_fwnode == csi2_ep) { > + struct fwnode_endpoint fwep; > + int ret; > + > + ret = fwnode_graph_parse_endpoint(csi2_ep, &fwep); > + > + fwnode_handle_put(csi2_ep); > + > + return ret ? ret : fwep.port; > + } > + } > + > + return -ENXIO; > +} As the 1:1 mapping is the common case, would it make sense to modify media_entity_get_fwnode_pad() accordingly when .get_fwnode_pad is not set ? The current behaviour is to return the first pad that matches the requested direction, which could be preserved as a second-level fallback if the 1:1 mapping doesn't give the right direction (but I'm not sure there's a use case for that, the 1:1 mapping seems to be all we need if there's no specific .get_fwnode_pad implementation). > + > static const struct media_entity_operations csi2_entity_ops = { > .link_setup = csi2_link_setup, > .link_validate = v4l2_subdev_link_validate, > + .get_fwnode_pad = csi2_get_fwnode_pad, > }; > > static const struct v4l2_subdev_video_ops csi2_video_ops = {
Hi Laurent, On Wed, Apr 15, 2020 at 02:07:29AM +0300, Laurent Pinchart wrote: > Hi Steve, > > Thank you for the patch. > > On Tue, Mar 03, 2020 at 03:42:45PM -0800, Steve Longerbeam wrote: > > Implement get_fwnode_pad operation. If the endpoint is owned by the MIPI > > CSI-2 receiver, return the endpoint's port number. The MIPI CSI-2 receiver > > maps port numbers and pad indexes 1:1. > > > > Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com> > > --- > > drivers/staging/media/imx/imx6-mipi-csi2.c | 28 ++++++++++++++++++++++ > > 1 file changed, 28 insertions(+) > > > > diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c > > index fdd763587e6c..8500207e5ea9 100644 > > --- a/drivers/staging/media/imx/imx6-mipi-csi2.c > > +++ b/drivers/staging/media/imx/imx6-mipi-csi2.c > > @@ -507,9 +507,37 @@ static int csi2_registered(struct v4l2_subdev *sd) > > 640, 480, 0, V4L2_FIELD_NONE, NULL); > > } > > > > +static int csi2_get_fwnode_pad(struct media_entity *entity, > > + struct fwnode_endpoint *endpoint) > > +{ > > + struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity); > > + struct csi2_dev *csi2 = sd_to_dev(sd); > > + struct fwnode_handle *csi2_ep; > > + > > + /* > > + * If the endpoint is one of ours, return the endpoint's port > > + * number. This device maps port numbers and pad indexes 1:1. > > + */ > > + fwnode_graph_for_each_endpoint(dev_fwnode(csi2->dev), csi2_ep) { > > + if (endpoint->local_fwnode == csi2_ep) { > > + struct fwnode_endpoint fwep; > > + int ret; > > + > > + ret = fwnode_graph_parse_endpoint(csi2_ep, &fwep); > > + > > + fwnode_handle_put(csi2_ep); > > + > > + return ret ? ret : fwep.port; > > + } > > + } > > + > > + return -ENXIO; > > +} > > As the 1:1 mapping is the common case, would it make sense to modify > media_entity_get_fwnode_pad() accordingly when .get_fwnode_pad is not > set ? The current behaviour is to return the first pad that matches the I also think this would make sense. > requested direction, which could be preserved as a second-level fallback > if the 1:1 mapping doesn't give the right direction (but I'm not sure > there's a use case for that, the 1:1 mapping seems to be all we need if > there's no specific .get_fwnode_pad implementation). I believe at least the smiapp driver breaks if you do that, so the current behaviour should be retained (secondary to the 1:1 mapping).
Hi Sakari, On Wed, Apr 15, 2020 at 02:20:36AM +0300, Sakari Ailus wrote: > On Wed, Apr 15, 2020 at 02:07:29AM +0300, Laurent Pinchart wrote: > > On Tue, Mar 03, 2020 at 03:42:45PM -0800, Steve Longerbeam wrote: > > > Implement get_fwnode_pad operation. If the endpoint is owned by the MIPI > > > CSI-2 receiver, return the endpoint's port number. The MIPI CSI-2 receiver > > > maps port numbers and pad indexes 1:1. > > > > > > Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com> > > > --- > > > drivers/staging/media/imx/imx6-mipi-csi2.c | 28 ++++++++++++++++++++++ > > > 1 file changed, 28 insertions(+) > > > > > > diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c > > > index fdd763587e6c..8500207e5ea9 100644 > > > --- a/drivers/staging/media/imx/imx6-mipi-csi2.c > > > +++ b/drivers/staging/media/imx/imx6-mipi-csi2.c > > > @@ -507,9 +507,37 @@ static int csi2_registered(struct v4l2_subdev *sd) > > > 640, 480, 0, V4L2_FIELD_NONE, NULL); > > > } > > > > > > +static int csi2_get_fwnode_pad(struct media_entity *entity, > > > + struct fwnode_endpoint *endpoint) > > > +{ > > > + struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity); > > > + struct csi2_dev *csi2 = sd_to_dev(sd); > > > + struct fwnode_handle *csi2_ep; > > > + > > > + /* > > > + * If the endpoint is one of ours, return the endpoint's port > > > + * number. This device maps port numbers and pad indexes 1:1. > > > + */ > > > + fwnode_graph_for_each_endpoint(dev_fwnode(csi2->dev), csi2_ep) { > > > + if (endpoint->local_fwnode == csi2_ep) { > > > + struct fwnode_endpoint fwep; > > > + int ret; > > > + > > > + ret = fwnode_graph_parse_endpoint(csi2_ep, &fwep); > > > + > > > + fwnode_handle_put(csi2_ep); > > > + > > > + return ret ? ret : fwep.port; > > > + } > > > + } > > > + > > > + return -ENXIO; > > > +} > > > > As the 1:1 mapping is the common case, would it make sense to modify > > media_entity_get_fwnode_pad() accordingly when .get_fwnode_pad is not > > set ? The current behaviour is to return the first pad that matches the > > I also think this would make sense. > > > requested direction, which could be preserved as a second-level fallback > > if the 1:1 mapping doesn't give the right direction (but I'm not sure > > there's a use case for that, the 1:1 mapping seems to be all we need if > > there's no specific .get_fwnode_pad implementation). > > I believe at least the smiapp driver breaks if you do that, so the current > behaviour should be retained (secondary to the 1:1 mapping). Shouldn't the smiapp driver get a .get_fwnode_pad() implementation then ? It could be argued that the smiapp use case is also quite common, and would deserve handling in the core, as a second fallback. It makes me wonder if we should really try to have a core fallback in the first place then, we could instead create helpers for the common cases that would be used by drivers as their .get_fwnode_pad() implementation, forcing each driver to think about which version matches its needs best.
Hi Laurent, On 4/14/20 4:07 PM, Laurent Pinchart wrote: > Hi Steve, > > Thank you for the patch. > > On Tue, Mar 03, 2020 at 03:42:45PM -0800, Steve Longerbeam wrote: >> Implement get_fwnode_pad operation. If the endpoint is owned by the MIPI >> CSI-2 receiver, return the endpoint's port number. The MIPI CSI-2 receiver >> maps port numbers and pad indexes 1:1. >> >> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com> >> --- >> drivers/staging/media/imx/imx6-mipi-csi2.c | 28 ++++++++++++++++++++++ >> 1 file changed, 28 insertions(+) >> >> diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c >> index fdd763587e6c..8500207e5ea9 100644 >> --- a/drivers/staging/media/imx/imx6-mipi-csi2.c >> +++ b/drivers/staging/media/imx/imx6-mipi-csi2.c >> @@ -507,9 +507,37 @@ static int csi2_registered(struct v4l2_subdev *sd) >> 640, 480, 0, V4L2_FIELD_NONE, NULL); >> } >> >> +static int csi2_get_fwnode_pad(struct media_entity *entity, >> + struct fwnode_endpoint *endpoint) >> +{ >> + struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity); >> + struct csi2_dev *csi2 = sd_to_dev(sd); >> + struct fwnode_handle *csi2_ep; >> + >> + /* >> + * If the endpoint is one of ours, return the endpoint's port >> + * number. This device maps port numbers and pad indexes 1:1. >> + */ >> + fwnode_graph_for_each_endpoint(dev_fwnode(csi2->dev), csi2_ep) { >> + if (endpoint->local_fwnode == csi2_ep) { >> + struct fwnode_endpoint fwep; >> + int ret; >> + >> + ret = fwnode_graph_parse_endpoint(csi2_ep, &fwep); >> + >> + fwnode_handle_put(csi2_ep); >> + >> + return ret ? ret : fwep.port; >> + } >> + } >> + >> + return -ENXIO; >> +} > As the 1:1 mapping is the common case, would it make sense to modify > media_entity_get_fwnode_pad() accordingly when .get_fwnode_pad is not > set ? Yes! In fact that was a previous revision of this patchset (v2). But I dropped that idea in v3 and v4 because it didn't seem to be getting any traction. I guess I'll try again. I'll bring back [1] in v5. Steve [1] https://patchwork.linuxtv.org/patch/60312/ > The current behaviour is to return the first pad that matches the > requested direction, which could be preserved as a second-level fallback > if the 1:1 mapping doesn't give the right direction (but I'm not sure > there's a use case for that, the 1:1 mapping seems to be all we need if > there's no specific .get_fwnode_pad implementation). > >> + >> static const struct media_entity_operations csi2_entity_ops = { >> .link_setup = csi2_link_setup, >> .link_validate = v4l2_subdev_link_validate, >> + .get_fwnode_pad = csi2_get_fwnode_pad, >> }; >> >> static const struct v4l2_subdev_video_ops csi2_video_ops = {
Hi Sakari, Laurent, On 4/14/20 4:20 PM, Sakari Ailus wrote: > Hi Laurent, > > On Wed, Apr 15, 2020 at 02:07:29AM +0300, Laurent Pinchart wrote: >> Hi Steve, >> >> Thank you for the patch. >> >> On Tue, Mar 03, 2020 at 03:42:45PM -0800, Steve Longerbeam wrote: >>> Implement get_fwnode_pad operation. If the endpoint is owned by the MIPI >>> CSI-2 receiver, return the endpoint's port number. The MIPI CSI-2 receiver >>> maps port numbers and pad indexes 1:1. >>> >>> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com> >>> --- >>> drivers/staging/media/imx/imx6-mipi-csi2.c | 28 ++++++++++++++++++++++ >>> 1 file changed, 28 insertions(+) >>> >>> diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c >>> index fdd763587e6c..8500207e5ea9 100644 >>> --- a/drivers/staging/media/imx/imx6-mipi-csi2.c >>> +++ b/drivers/staging/media/imx/imx6-mipi-csi2.c >>> @@ -507,9 +507,37 @@ static int csi2_registered(struct v4l2_subdev *sd) >>> 640, 480, 0, V4L2_FIELD_NONE, NULL); >>> } >>> >>> +static int csi2_get_fwnode_pad(struct media_entity *entity, >>> + struct fwnode_endpoint *endpoint) >>> +{ >>> + struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity); >>> + struct csi2_dev *csi2 = sd_to_dev(sd); >>> + struct fwnode_handle *csi2_ep; >>> + >>> + /* >>> + * If the endpoint is one of ours, return the endpoint's port >>> + * number. This device maps port numbers and pad indexes 1:1. >>> + */ >>> + fwnode_graph_for_each_endpoint(dev_fwnode(csi2->dev), csi2_ep) { >>> + if (endpoint->local_fwnode == csi2_ep) { >>> + struct fwnode_endpoint fwep; >>> + int ret; >>> + >>> + ret = fwnode_graph_parse_endpoint(csi2_ep, &fwep); >>> + >>> + fwnode_handle_put(csi2_ep); >>> + >>> + return ret ? ret : fwep.port; >>> + } >>> + } >>> + >>> + return -ENXIO; >>> +} >> As the 1:1 mapping is the common case, would it make sense to modify >> media_entity_get_fwnode_pad() accordingly when .get_fwnode_pad is not >> set ? The current behaviour is to return the first pad that matches the > I also think this would make sense. What do you think about https://patchwork.linuxtv.org/patch/60312/ ? I'm planning to resurrect it for v5. Steve >> requested direction, which could be preserved as a second-level fallback >> if the 1:1 mapping doesn't give the right direction (but I'm not sure >> there's a use case for that, the 1:1 mapping seems to be all we need if >> there's no specific .get_fwnode_pad implementation). > I believe at least the smiapp driver breaks if you do that, so the current > behaviour should be retained (secondary to the 1:1 mapping). >
Hi Laurent, On Wed, Apr 15, 2020 at 02:27:03AM +0300, Laurent Pinchart wrote: > Hi Sakari, > > On Wed, Apr 15, 2020 at 02:20:36AM +0300, Sakari Ailus wrote: > > On Wed, Apr 15, 2020 at 02:07:29AM +0300, Laurent Pinchart wrote: > > > On Tue, Mar 03, 2020 at 03:42:45PM -0800, Steve Longerbeam wrote: > > > > Implement get_fwnode_pad operation. If the endpoint is owned by the MIPI > > > > CSI-2 receiver, return the endpoint's port number. The MIPI CSI-2 receiver > > > > maps port numbers and pad indexes 1:1. > > > > > > > > Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com> > > > > --- > > > > drivers/staging/media/imx/imx6-mipi-csi2.c | 28 ++++++++++++++++++++++ > > > > 1 file changed, 28 insertions(+) > > > > > > > > diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c > > > > index fdd763587e6c..8500207e5ea9 100644 > > > > --- a/drivers/staging/media/imx/imx6-mipi-csi2.c > > > > +++ b/drivers/staging/media/imx/imx6-mipi-csi2.c > > > > @@ -507,9 +507,37 @@ static int csi2_registered(struct v4l2_subdev *sd) > > > > 640, 480, 0, V4L2_FIELD_NONE, NULL); > > > > } > > > > > > > > +static int csi2_get_fwnode_pad(struct media_entity *entity, > > > > + struct fwnode_endpoint *endpoint) > > > > +{ > > > > + struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity); > > > > + struct csi2_dev *csi2 = sd_to_dev(sd); > > > > + struct fwnode_handle *csi2_ep; > > > > + > > > > + /* > > > > + * If the endpoint is one of ours, return the endpoint's port > > > > + * number. This device maps port numbers and pad indexes 1:1. > > > > + */ > > > > + fwnode_graph_for_each_endpoint(dev_fwnode(csi2->dev), csi2_ep) { > > > > + if (endpoint->local_fwnode == csi2_ep) { > > > > + struct fwnode_endpoint fwep; > > > > + int ret; > > > > + > > > > + ret = fwnode_graph_parse_endpoint(csi2_ep, &fwep); > > > > + > > > > + fwnode_handle_put(csi2_ep); > > > > + > > > > + return ret ? ret : fwep.port; > > > > + } > > > > + } > > > > + > > > > + return -ENXIO; > > > > +} > > > > > > As the 1:1 mapping is the common case, would it make sense to modify > > > media_entity_get_fwnode_pad() accordingly when .get_fwnode_pad is not > > > set ? The current behaviour is to return the first pad that matches the > > > > I also think this would make sense. > > > > > requested direction, which could be preserved as a second-level fallback > > > if the 1:1 mapping doesn't give the right direction (but I'm not sure > > > there's a use case for that, the 1:1 mapping seems to be all we need if > > > there's no specific .get_fwnode_pad implementation). > > > > I believe at least the smiapp driver breaks if you do that, so the current > > behaviour should be retained (secondary to the 1:1 mapping). > > Shouldn't the smiapp driver get a .get_fwnode_pad() implementation then > ? It could be argued that the smiapp use case is also quite common, and > would deserve handling in the core, as a second fallback. It makes me > wonder if we should really try to have a core fallback in the first > place then, we could instead create helpers for the common cases that > would be used by drivers as their .get_fwnode_pad() implementation, > forcing each driver to think about which version matches its needs best. I guess this could be a little cleaner with explicit helpers set by drives. I'm not against that, but at the same time I don't think this is an area the cleaning of which would significantly help something.
Hi Steve, On Tue, Apr 14, 2020 at 04:50:55PM -0700, Steve Longerbeam wrote: > On 4/14/20 4:20 PM, Sakari Ailus wrote: > > On Wed, Apr 15, 2020 at 02:07:29AM +0300, Laurent Pinchart wrote: > >> On Tue, Mar 03, 2020 at 03:42:45PM -0800, Steve Longerbeam wrote: > >>> Implement get_fwnode_pad operation. If the endpoint is owned by the MIPI > >>> CSI-2 receiver, return the endpoint's port number. The MIPI CSI-2 receiver > >>> maps port numbers and pad indexes 1:1. > >>> > >>> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com> > >>> --- > >>> drivers/staging/media/imx/imx6-mipi-csi2.c | 28 ++++++++++++++++++++++ > >>> 1 file changed, 28 insertions(+) > >>> > >>> diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c > >>> index fdd763587e6c..8500207e5ea9 100644 > >>> --- a/drivers/staging/media/imx/imx6-mipi-csi2.c > >>> +++ b/drivers/staging/media/imx/imx6-mipi-csi2.c > >>> @@ -507,9 +507,37 @@ static int csi2_registered(struct v4l2_subdev *sd) > >>> 640, 480, 0, V4L2_FIELD_NONE, NULL); > >>> } > >>> > >>> +static int csi2_get_fwnode_pad(struct media_entity *entity, > >>> + struct fwnode_endpoint *endpoint) > >>> +{ > >>> + struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity); > >>> + struct csi2_dev *csi2 = sd_to_dev(sd); > >>> + struct fwnode_handle *csi2_ep; > >>> + > >>> + /* > >>> + * If the endpoint is one of ours, return the endpoint's port > >>> + * number. This device maps port numbers and pad indexes 1:1. > >>> + */ > >>> + fwnode_graph_for_each_endpoint(dev_fwnode(csi2->dev), csi2_ep) { > >>> + if (endpoint->local_fwnode == csi2_ep) { > >>> + struct fwnode_endpoint fwep; > >>> + int ret; > >>> + > >>> + ret = fwnode_graph_parse_endpoint(csi2_ep, &fwep); > >>> + > >>> + fwnode_handle_put(csi2_ep); > >>> + > >>> + return ret ? ret : fwep.port; > >>> + } > >>> + } > >>> + > >>> + return -ENXIO; > >>> +} > >> > >> As the 1:1 mapping is the common case, would it make sense to modify > >> media_entity_get_fwnode_pad() accordingly when .get_fwnode_pad is not > >> set ? The current behaviour is to return the first pad that matches the > > > > I also think this would make sense. > > What do you think about https://patchwork.linuxtv.org/patch/60312/ ? I'm > planning to resurrect it for v5. The approach looks good to me. > >> requested direction, which could be preserved as a second-level fallback > >> if the 1:1 mapping doesn't give the right direction (but I'm not sure > >> there's a use case for that, the 1:1 mapping seems to be all we need if > >> there's no specific .get_fwnode_pad implementation). > > > > I believe at least the smiapp driver breaks if you do that, so the current > > behaviour should be retained (secondary to the 1:1 mapping).
diff --git a/drivers/staging/media/imx/imx6-mipi-csi2.c b/drivers/staging/media/imx/imx6-mipi-csi2.c index fdd763587e6c..8500207e5ea9 100644 --- a/drivers/staging/media/imx/imx6-mipi-csi2.c +++ b/drivers/staging/media/imx/imx6-mipi-csi2.c @@ -507,9 +507,37 @@ static int csi2_registered(struct v4l2_subdev *sd) 640, 480, 0, V4L2_FIELD_NONE, NULL); } +static int csi2_get_fwnode_pad(struct media_entity *entity, + struct fwnode_endpoint *endpoint) +{ + struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity); + struct csi2_dev *csi2 = sd_to_dev(sd); + struct fwnode_handle *csi2_ep; + + /* + * If the endpoint is one of ours, return the endpoint's port + * number. This device maps port numbers and pad indexes 1:1. + */ + fwnode_graph_for_each_endpoint(dev_fwnode(csi2->dev), csi2_ep) { + if (endpoint->local_fwnode == csi2_ep) { + struct fwnode_endpoint fwep; + int ret; + + ret = fwnode_graph_parse_endpoint(csi2_ep, &fwep); + + fwnode_handle_put(csi2_ep); + + return ret ? ret : fwep.port; + } + } + + return -ENXIO; +} + static const struct media_entity_operations csi2_entity_ops = { .link_setup = csi2_link_setup, .link_validate = v4l2_subdev_link_validate, + .get_fwnode_pad = csi2_get_fwnode_pad, }; static const struct v4l2_subdev_video_ops csi2_video_ops = {
Implement get_fwnode_pad operation. If the endpoint is owned by the MIPI CSI-2 receiver, return the endpoint's port number. The MIPI CSI-2 receiver maps port numbers and pad indexes 1:1. Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com> --- drivers/staging/media/imx/imx6-mipi-csi2.c | 28 ++++++++++++++++++++++ 1 file changed, 28 insertions(+)