Re: [PATCH] media: imx: Use get_mbus_config instead of parsing upstream DT endpoints

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Linux Driver Development]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux