Re: [PATCH 2/2] media: Add imx334 camera sensor driver

['Sakari Ailus' <sakari.ailus@linux.intel.com>]

Hi Martina,

On Mon, Nov 30, 2020 at 10:21:12AM -0000, martinax.krasteva@linux.intel.com wrote:
> Hi Sakari, Jacopo,
> 
> Thank you for the review
> 
> > -----Original Message-----
> > From: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Sent: Monday, November 23, 2020 2:02 PM
> > To: Jacopo Mondi <jacopo@jmondi.org>
> > Cc: Martina Krasteva <martinax.krasteva@linux.intel.com>; linux-
> > media@vger.kernel.org; mchehab@kernel.org; robh+dt@kernel.org;
> > devicetree@vger.kernel.org; daniele.alessandrelli@linux.intel.com;
> > gjorgjix.rosikopulos@linux.intel.com
> > Subject: Re: [PATCH 2/2] media: Add imx334 camera sensor driver
> > 
> > Hi Jacopo,
> > 
> > On Mon, Nov 23, 2020 at 12:10:29PM +0100, Jacopo Mondi wrote:
> > ...
> > > > +#include <media/v4l2-fwnode.h>
> > >
> > > You only use v4l2_async_register_subdev_sensor_common() from fwnde.h
> > > If you think you can replace it with v4l2_async_register_subdev() (see
> > > below comment) this should be changed in v4l2-async.h
> > 
> > Either is fine in principle. I'd use
> > v4l2_async_register_subdev_sensor_common() for sensors though, as it
> allows
> > connecting lens and flash sub-devices.
> > 
> > Regarding DT bindings --- I wonder if there's a way to say these are
> relevant for
> > all sensors. That'd be another discussion though.
> > 
> 
> Should I add lens and flash in DT binding doc, so it is clear that
> connecting such sub-devs is supported?
> I thought the binding doc should include only the bare minimum for a certain
> driver to be used, but it does make sense adding this info.

I wouldn't add them to bindings here as they're not related to this device
but to other devices.

I wonder what Rob thinks.

...

> > > > +static const struct media_entity_operations imx334_subdev_entity_ops
> = {
> > > > +	.link_validate = v4l2_subdev_link_validate, };
> > >
> > > Is link_validate called on sensor subdev ? My understanding is that
> > > they're called on the sink entity, but I might be mistaken.
> > 
> > Correct.
> > 
> 
> This is what I read in the v4l2-subdev.rst:
> " If the subdev driver intends to process video and integrate with the media
> framework, it must implement format related functionality using
> :c:type:`v4l2_subdev_pad_ops` instead of :c:type:`v4l2_subdev_video_ops`.
> 
> In that case, the subdev driver may set the link_validate field to provide
> its own link validation function. <<The link validation function is called
> for
> every link in the pipeline where both of the ends of the links are V4L2
> sub-devices.>> The driver is still responsible for validating the
> correctness
> of the format configuration between sub-devices and video nodes."
> 
> I find it a bit misleading, however I checked the source code, so I will
> remove it in the next version.
> 
> Something that is not clear to me is, do I have to explicitly set
> link_validate for the sink pad's entity to trigger validation. According to
> the doc
> I don't need to, but I cannot find the place in the source code where the
> default func is called even if the op is not set, neither setting default
> ops in case they weren't set.
>  
> "If link_validate op is not set, the default function
> :c:func:`v4l2_subdev_link_validate_default` is used instead. This function
> ensures that width, height and the media bus pixel code are equal on both
> source
> and sink of the link. Subdev drivers are also free to use this function to
> perform the checks mentioned above in addition to their own checks."

When a link is validated, it's the link_validate callback of the sink pad
that will be used for the purpose. This is currently not documented here
but should be added.

-- 
Kind regards,

Sakari Ailus