Re: [PATCH] media: staging: max96712: Add support for 3-lane C-PHY

["Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>]

Hej Sakari,

On 2023-03-31 17:08:53 +0300, Sakari Ailus wrote:
> Hejssan, Niklas!
> 
> On Fri, Mar 31, 2023 at 03:24:04PM +0200, Niklas Söderlund wrote:
> > Hej Sakari,
> > 
> > Tack för din feedback.
> > 
> > On 2023-03-31 15:21:34 +0300, Sakari Ailus wrote:
> > 
> > ...
> > 
> > > > @@ -332,8 +341,9 @@ static int max96712_parse_dt(struct 
> > > > max96712_priv *priv)
> > > >  {
> > > >  	struct fwnode_handle *ep;
> > > >  	struct v4l2_fwnode_endpoint v4l2_ep = {
> > > > -		.bus_type = V4L2_MBUS_CSI2_DPHY
> > > > +		.bus_type = V4L2_MBUS_UNKNOWN,
> > > 
> > > The bindings don't require setting bus-type. Please change the bindings as
> > > well. And assume D-PHY in the driver if bus-type isn't set.
> > 
> > Thanks for spotting this, I will send out an update to update the 
> > binding to require setting bus-type.
> > 
> > About updating the driver to assume D-PHY if bus-type is not set. I 
> > wonder if we can avoid that and keep the driver change as is? The only 
> > in-tree user of this binding is in r8a779a0-falcon-csi-dsi.dtsi, and I 
> > intend to send out a patch to set the bus-type for that together with 
> > the updated bindings.
> > 
> > I have tested this driver change on the Falcon board and if bus-type is 
> > not explicitly set in the DTS it is reported as D-PHY and everything 
> > works as expected. So if I
> > 
> > - Send out a patch to update the bindings to require bus-type being set.
> > - Send out patch to update the only in-tree use of the driver to set 
> >   bus-type.
> > 
> > Can't we avoid issues in the future by not assuming no bus-type is D-PHY 
> > in the driver while still being backward compatible with the old DTS?
> 
> If you want to keep supporting D-PHY as a default, you should instead try
> parsing with V4L2_MBUS_CSI2_DPHY as bus_type. Then CPHY if that fails.
> 
> Although if bus_type is mandatory, then this patch is fine.

Thanks. I prefers to make bus-type a mandatory property, it feels like 
being explicit in the bindings is the best way going forward. I have 
posted the patches to make this change and to update the only user.

There is only one user in-tree, and making the change now would still be 
backward compatible with it. So better bite the bullet now before the 
binding spreads.

Trevlig helg!

> 
> -- 
> Trevliga hälsningar,
> 
> Sakari Ailus

-- 
Kind Regards,
Niklas Söderlund