Hi Jacopo,
On Tue, Apr 30, 2024 at 09:19:32AM +0200, Jacopo Mondi wrote:
> Hi Sakari
>
> On Mon, Apr 29, 2024 at 10:08:50PM +0300, Sakari Ailus wrote:
> > Add link_freq field to struct v4l2_mbus_config in order to pass the link
> > frequency to the reciving sub-device.
>
> 'receiving'
>
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > ---
> > drivers/media/v4l2-core/v4l2-common.c | 13 +++++++++----
> > include/media/v4l2-mediabus.h | 2 ++
> > 2 files changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> > index 7f69b5a025fa..09b26ce612e9 100644
> > --- a/drivers/media/v4l2-core/v4l2-common.c
> > +++ b/drivers/media/v4l2-core/v4l2-common.c
> > @@ -503,15 +503,20 @@ s64 __v4l2_get_link_freq_ctrl(struct v4l2_ctrl_handler *handler,
> > EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_ctrl);
> >
> > s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul,
> > - unsigned int div)
> > + unsigned int div)
>
> Goes in the previous patch that introduces the function ?
Yes.
>
> > {
> > + struct v4l2_mbus_config mbus_config = {};
> > struct v4l2_subdev *sd;
> > + int ret;
> >
> > sd = media_entity_to_v4l2_subdev(pad->entity);
> > - if (!sd)
> > - return -ENODEV;
> > + ret = v4l2_subdev_call(sd, pad, get_mbus_config, pad->index,
> > + &mbus_config);
> > + if (ret < 0 && ret != -ENOIOCTLCMD)
> > + return ret;
> >
> > - return __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div);
> > + return mbus_config.link_freq ?:
> > + __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div);
> > }
> > EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_pad);
> >
> > diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
> > index 5bce6e423e94..2f39b52bb4d4 100644
> > --- a/include/media/v4l2-mediabus.h
> > +++ b/include/media/v4l2-mediabus.h
> > @@ -159,6 +159,7 @@ enum v4l2_mbus_type {
> > * @bus.mipi_csi2: embedded &struct v4l2_mbus_config_mipi_csi2.
> > * Used if the bus is MIPI Alliance's Camera Serial
> > * Interface version 2 (MIPI CSI2).
> > + * @link_freq: The link frequency. See also V4L2_CID_LINK_FREQ control.
> > */
> > struct v4l2_mbus_config {
> > enum v4l2_mbus_type type;
> > @@ -167,6 +168,7 @@ struct v4l2_mbus_config {
> > struct v4l2_mbus_config_mipi_csi1 mipi_csi1;
> > struct v4l2_mbus_config_mipi_csi2 mipi_csi2;
> > } bus;
> > + u64 link_freq;
>
> Does this apply to all the supported busses ?
Just like the LINK_FREQ control, it does, albeit the current user (IVSC
driver) uses CSI-2 only.
I'm fine moving it after "type" though, that may be a better location for
it.
>
> I count:
>
> struct v4l2_mbus_config_parallel parallel;
> struct v4l2_mbus_config_mipi_csi1 mipi_csi1;
> struct v4l2_mbus_config_mipi_csi2 mipi_csi2;
>
> not sure about csi1, but I would say "yes" ?
>
> However it would feel more natural to fetch the 'link_freq' from the
> bus-specific structure like in 'bus.mipi_csi2.link_freq' (and 'bus' is
> an union, so we're not consuming any additional space if we move it to
> the per-bus structure).
>
>
> > };
> >
> > /**
--
Kind regards,
Sakari Ailus
