Hi Hans, Thank you for the review. On Wed, Nov 13, 2024 at 09:03:57AM +0100, Hans Verkuil wrote: > On 11/10/2024 09:55, Sakari Ailus wrote: > > Add the V4L2_CID_CONFIG_MODEL control for the configuration model. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> > > --- > > .../userspace-api/media/v4l/ext-ctrls-image-process.rst | 4 ++++ > > .../userspace-api/media/v4l/subdev-config-model.rst | 2 ++ > > drivers/media/v4l2-core/v4l2-ctrls-defs.c | 5 +++++ > > include/uapi/linux/v4l2-controls.h | 3 +++ > > 4 files changed, 14 insertions(+) > > > > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst > > index 27803dca8d3e..928e8e3eed7f 100644 > > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst > > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-image-process.rst > > @@ -55,3 +55,7 @@ Image Process Control IDs > > control value divided by e.g. 0x100, meaning that to get no > > digital gain the control value needs to be 0x100. The no-gain > > configuration is also typically the default. > > + > > +``V4L2_CID_CONFIG_MODEL (bitmask)`` > > + Which configuration models the sub-device supports. Please see > > + :ref:`media_subdev_config_model`. > > First of all the naming is confusing: since this is specific to sub-devices, it > should at least have 'SUBDEV' in the name. I first thought this reported the I don't object in principle, but the reason why I didn't add that in v1 was the names would get quite long. Maybe V4L2_CID_SUBDEV_CFG_MODEL? > model name or something like that, I'm not sure "configuration model" is a very > good name. Feel free to propose a different one. :-) > > Secondly, is this supposed to be valid for all subdevices? Only for sensors? > Would an HDMI-to-CSI bridge qualify? I think it could but we should have a use case for it. In other words, something we can't reasonably express using existing means. In this case it's a number of interfaces and device type specific behaviour (see the 3rd patch). > > Thirdly, only V4L2_CID_CONFIG_MODEL_COMMON_RAW is defined right now. What other > models do you have in mind? What models can co-exist (since this is a bitmask)? We could have different raw camera models if needed. I don't have any planned right now, though. > > Finally, why choose a control for this? Should this perhaps be better done as > a field in media_entity_desc/media_v2_entity? I don't think it's a great fit. This is largely about V4L2 (to some but lesser extent about MC) and we traditionally have avoided MC -> V4L2 dependencies. -- Kind regards, Sakari Ailus
