Hi Laurent, Hans, Sakari
On Mon, Apr 20, 2020 at 04:48:46AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Apr 15, 2020 at 12:49:59PM +0200, Jacopo Mondi wrote:
> > Deprecate 'g_mbus_config' video operation in favor of the newly
> > introduced 'get_mbus_config' pad operation.
> >
> > Suggested-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>
> Not necessarily a blocker for this series, but it would be nice to
> convert the handful of users of this API (you can leave soc-camera
> untouched as it's on its way out of the kernel).
I'm trying to, but I'm having an hard time fixing the new operations
semantic, specifically on set_mbus_config, as if I have to deprecate
the existing users, a replacement for s_mbus_config is required.
In particular, the old API seems to work as follows:
1) Report all supported config options through g_mbus_config
2) Apply the requested ones (assumed to be in the set of configurable
ones) though s_mbus_config.
There is no mention of how to treat unsupported configuration
parameters, no mention of how handle configuration errors, if
s_mbus_config should behave like set_fmt (update the received
configuration to reflect the current status and don't fail if the
configuration cannot be applied).
The new operation has a different semantic (which still has to be
defined) particularly:
1) get_mbus_config() reports the -current- media bus configuration, not the
list of supported options that could be tweaked.
2) set_mbus_config() has the following behavior specified:
- Do not fail if received config is unsupported, update it to
reflect the current configuration
- Apply changes to the supported configuration options, ignore the
non supported one, but do not fail.
I'm still trying to understand if a new set_mbus_config() is actually
welcome or not, but apart from this, I think porting existing users to
the new API, even if it could be made to compile successfully will
change quite some behaviour, and honestly I'm not sure we want to go
that way, or keep both the existing operations and only deprecate the
existing one in the documentation.
For reference ov6650.c is the only mainline user of s_mbus_config I
can see as well pxa_camera is the only platform driver using
g_mbus_config. Porting both to the new API would require testing I
cannot perform probably, or would you be ok with just compile testing
it ?
Thanks
j
>
> > ---
> > include/media/v4l2-subdev.h | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index d1a5e9d1ea63..9bf14c41626d 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -466,7 +466,9 @@ struct v4l2_mbus_pad_config {
> > *
> > * @query_dv_timings: callback for VIDIOC_QUERY_DV_TIMINGS() ioctl handler code.
> > *
> > - * @g_mbus_config: get supported mediabus configurations
> > + * @g_mbus_config: get supported mediabus configurations. This operation is
> > + * deprecated in favour of the get_mbus_config() pad operation
> > + * and should not be used by new software.
> > *
> > * @s_mbus_config: set a certain mediabus configuration. This operation is added
> > * for compatibility with soc-camera drivers and should not be used by new
>
> --
> Regards,
>
> Laurent Pinchart
