Hi Jacopo,
On Fr, 2022-08-19 at 10:03 +0200, Jacopo Mondi wrote:
> Hi Philipp
>
> On Thu, Aug 18, 2022 at 02:42:53PM +0200, Philipp Zabel wrote:
[...]
> > -static int csi_get_upstream_endpoint(struct csi_priv *priv,
> > - struct v4l2_fwnode_endpoint *ep)
> > +static int csi_get_upstream_mbus_config(struct csi_priv *priv,
> > + struct v4l2_mbus_config *mbus_cfg)
> > {
> > - struct fwnode_handle *endpoint;
> > - struct v4l2_subdev *sd;
> > - struct media_pad *pad;
> > + struct v4l2_subdev *sd, *remote_sd;
> > + struct media_pad *remote_pad;
> > + int ret;
> >
> > if (!IS_ENABLED(CONFIG_OF))
> > return -ENXIO;
>
> Is this still necessary ?
I think not. I will drop it.
> > @@ -206,19 +204,21 @@ static int csi_get_upstream_endpoint(struct csi_priv *priv,
> > }
> >
> > /* get source pad of entity directly upstream from sd */
> > - pad = imx_media_pipeline_pad(&sd->entity, 0, 0, true);
> > - if (!pad)
> > - return -ENODEV;
> > -
> > - endpoint = imx_media_get_pad_fwnode(pad);
> > - if (IS_ERR(endpoint))
> > - return PTR_ERR(endpoint);
> > + remote_pad = media_entity_remote_pad_unique(&sd->entity,
> > + MEDIA_PAD_FL_SOURCE);
> > + if (IS_ERR(remote_pad))
> > + return PTR_ERR(remote_pad);
> >
> > - v4l2_fwnode_endpoint_parse(endpoint, ep);
> > + remote_sd = media_entity_to_v4l2_subdev(remote_pad->entity);
> >
> > - fwnode_handle_put(endpoint);
> > + ret = v4l2_subdev_call(remote_sd, pad, get_mbus_config,
> > + remote_pad->index, mbus_cfg);
> > + if (ret == -ENOIOCTLCMD)
> > + v4l2_err(&priv->sd,
> > + "upstream entity %s does not implement get_mbus_config()\n",
> > + remote_pad->entity->name);
>
> This makes mandatory for the remote to implement .get_mbus_config().
> I think it is fine, even more this is staging, and I don't think you
> can use any meaningful default in case the op is not implemented...
Yes. Many of the relevant drivers already implement this (adv7180,
tc358743, tvp5150), but it's missing from ov5640 and some of the other
ovti drivers.
> > @@ -1254,13 +1246,13 @@ static int csi_enum_mbus_code(struct v4l2_subdev *sd,
> > break;
> > case CSI_SRC_PAD_DIRECT:
> > case CSI_SRC_PAD_IDMAC:
> > - ret = csi_get_upstream_endpoint(priv, &upstream_ep);
> > + ret = csi_get_upstream_mbus_config(priv, &mbus_cfg);
> > if (ret) {
> > v4l2_err(&priv->sd, "failed to find upstream endpoint\n");
>
> Should the error message be updated ?
Indeed, I updated one and forgot about the rest. I'll update them all.
[...]
>
>
> All minors, with the error messages fixed
> Reviewed-by: Jacopo Mondi <jacopo@xxxxxxxxxx>
Thank you for the review!
regards
Philipp
