| Message ID | 20241210075906.609490-3-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Use V4L2 mbus config for conveying MEI CSI link frequency | expand |
Hi Sakari On Tue, Dec 10, 2024 at 09:59:03AM +0200, Sakari Ailus wrote: > Add link_freq field to struct v4l2_mbus_config in order to pass the link > frequency to the receiving sub-device. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > --- > drivers/media/v4l2-core/v4l2-common.c | 11 ++++++++--- > include/media/v4l2-mediabus.h | 2 ++ > 2 files changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c > index 9fe74c7e064f..88fbd5608f00 100644 > --- a/drivers/media/v4l2-core/v4l2-common.c > +++ b/drivers/media/v4l2-core/v4l2-common.c > @@ -508,13 +508,18 @@ EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_ctrl); > s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul, > unsigned int div) > { > + 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); > #endif /* CONFIG_MEDIA_CONTROLLER */ > diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h > index 5bce6e423e94..cc5f776dc662 100644 > --- a/include/media/v4l2-mediabus.h > +++ b/include/media/v4l2-mediabus.h > @@ -148,6 +148,7 @@ enum v4l2_mbus_type { > /** > * struct v4l2_mbus_config - media bus configuration > * @type: interface type > + * @link_freq: The link frequency. See also V4L2_CID_LINK_FREQ control. > * @bus: bus configuration data structure > * @bus.parallel: embedded &struct v4l2_mbus_config_parallel. > * Used if the bus is parallel or BT.656. > @@ -162,6 +163,7 @@ enum v4l2_mbus_type { > */ > struct v4l2_mbus_config { > enum v4l2_mbus_type type; > + u64 link_freq; > union { > struct v4l2_mbus_config_parallel parallel; > struct v4l2_mbus_config_mipi_csi1 mipi_csi1; > -- > 2.39.5 > >
Hi Sakari, Thank you for the patch. On Tue, Dec 10, 2024 at 09:59:03AM +0200, Sakari Ailus wrote: > Add link_freq field to struct v4l2_mbus_config in order to pass the link > frequency to the receiving sub-device. The documentation of v4l2_get_link_freq() should be expanded to explain the new mode of operation. It should document which of the supported methods are recommended for new drivers. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/v4l2-core/v4l2-common.c | 11 ++++++++--- > include/media/v4l2-mediabus.h | 2 ++ > 2 files changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c > index 9fe74c7e064f..88fbd5608f00 100644 > --- a/drivers/media/v4l2-core/v4l2-common.c > +++ b/drivers/media/v4l2-core/v4l2-common.c > @@ -508,13 +508,18 @@ EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_ctrl); > s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul, > unsigned int div) > { > + struct v4l2_mbus_config mbus_config = {}; Isn't mbus_config fully populated by the .get_mbus_config() operation ? That should be documented in the .get_mbus_config() operation documentation. > 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); if (mbus_config.link_freq) return mbus_config.link_freq; return __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div); I wonder if we should also add a message in case link_freq is 0, to get drivers to convert to reporting the link frequency through .get_mbus_config() if they already implement the operation. > } > EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_pad); > #endif /* CONFIG_MEDIA_CONTROLLER */ > diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h > index 5bce6e423e94..cc5f776dc662 100644 > --- a/include/media/v4l2-mediabus.h > +++ b/include/media/v4l2-mediabus.h > @@ -148,6 +148,7 @@ enum v4l2_mbus_type { > /** > * struct v4l2_mbus_config - media bus configuration > * @type: interface type > + * @link_freq: The link frequency. See also V4L2_CID_LINK_FREQ control. Not a candidate for this series, but I'd like to simplify drivers by implementing the LINK_FREQ control automatically. > * @bus: bus configuration data structure > * @bus.parallel: embedded &struct v4l2_mbus_config_parallel. > * Used if the bus is parallel or BT.656. > @@ -162,6 +163,7 @@ enum v4l2_mbus_type { > */ > struct v4l2_mbus_config { > enum v4l2_mbus_type type; > + u64 link_freq; > union { > struct v4l2_mbus_config_parallel parallel; > struct v4l2_mbus_config_mipi_csi1 mipi_csi1;
Hi Laurent, On Sun, Dec 15, 2024 at 06:59:35PM +0200, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Tue, Dec 10, 2024 at 09:59:03AM +0200, Sakari Ailus wrote: > > Add link_freq field to struct v4l2_mbus_config in order to pass the link > > frequency to the receiving sub-device. > > The documentation of v4l2_get_link_freq() should be expanded to explain > the new mode of operation. It should document which of the supported > methods are recommended for new drivers. > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > drivers/media/v4l2-core/v4l2-common.c | 11 ++++++++--- > > include/media/v4l2-mediabus.h | 2 ++ > > 2 files changed, 10 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c > > index 9fe74c7e064f..88fbd5608f00 100644 > > --- a/drivers/media/v4l2-core/v4l2-common.c > > +++ b/drivers/media/v4l2-core/v4l2-common.c > > @@ -508,13 +508,18 @@ EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_ctrl); > > s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul, > > unsigned int div) > > { > > + struct v4l2_mbus_config mbus_config = {}; > > Isn't mbus_config fully populated by the .get_mbus_config() operation ? > That should be documented in the .get_mbus_config() operation > documentation. It's a good practice to zero the memory before drivers get to work on it. I presume drivers will set the fields that are relevant for them and ignore the rest. I can add a note on get_mbus_config() the drivers should set all struct fields to known values. > > > 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); > > if (mbus_config.link_freq) > return mbus_config.link_freq; > > return __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div); Whether this would be cleaner is debatable at least. I can switch if you insist though. > > I wonder if we should also add a message in case link_freq is 0, to get > drivers to convert to reporting the link frequency through > .get_mbus_config() if they already implement the operation. I'm not sure this will be useful: in most cases the LINK_FREQ control is used and for a reason: it's user-configurable. Drivers should only populate the field if the link frequency is determined by the driver. For the receiver drivers it does not matter: they use v4l2_get_link_freq(). > > > } > > EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_pad); > > #endif /* CONFIG_MEDIA_CONTROLLER */ > > diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h > > index 5bce6e423e94..cc5f776dc662 100644 > > --- a/include/media/v4l2-mediabus.h > > +++ b/include/media/v4l2-mediabus.h > > @@ -148,6 +148,7 @@ enum v4l2_mbus_type { > > /** > > * struct v4l2_mbus_config - media bus configuration > > * @type: interface type > > + * @link_freq: The link frequency. See also V4L2_CID_LINK_FREQ control. > > Not a candidate for this series, but I'd like to simplify drivers by > implementing the LINK_FREQ control automatically. > > > * @bus: bus configuration data structure > > * @bus.parallel: embedded &struct v4l2_mbus_config_parallel. > > * Used if the bus is parallel or BT.656. > > @@ -162,6 +163,7 @@ enum v4l2_mbus_type { > > */ > > struct v4l2_mbus_config { > > enum v4l2_mbus_type type; > > + u64 link_freq; > > union { > > struct v4l2_mbus_config_parallel parallel; > > struct v4l2_mbus_config_mipi_csi1 mipi_csi1; >
On Mon, Dec 16, 2024 at 08:46:43AM +0000, Sakari Ailus wrote: > On Sun, Dec 15, 2024 at 06:59:35PM +0200, Laurent Pinchart wrote: > > On Tue, Dec 10, 2024 at 09:59:03AM +0200, Sakari Ailus wrote: > > > Add link_freq field to struct v4l2_mbus_config in order to pass the link > > > frequency to the receiving sub-device. > > > > The documentation of v4l2_get_link_freq() should be expanded to explain > > the new mode of operation. It should document which of the supported > > methods are recommended for new drivers. > > > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > --- > > > drivers/media/v4l2-core/v4l2-common.c | 11 ++++++++--- > > > include/media/v4l2-mediabus.h | 2 ++ > > > 2 files changed, 10 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c > > > index 9fe74c7e064f..88fbd5608f00 100644 > > > --- a/drivers/media/v4l2-core/v4l2-common.c > > > +++ b/drivers/media/v4l2-core/v4l2-common.c > > > @@ -508,13 +508,18 @@ EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_ctrl); > > > s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul, > > > unsigned int div) > > > { > > > + struct v4l2_mbus_config mbus_config = {}; > > > > Isn't mbus_config fully populated by the .get_mbus_config() operation ? > > That should be documented in the .get_mbus_config() operation > > documentation. > > It's a good practice to zero the memory before drivers get to work on it. I > presume drivers will set the fields that are relevant for them and ignore > the rest. > > I can add a note on get_mbus_config() the drivers should set all struct > fields to known values. Thanks. > > > 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); > > > > if (mbus_config.link_freq) > > return mbus_config.link_freq; > > > > return __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div); > > Whether this would be cleaner is debatable at least. I can switch if you > insist though. I think it's nicer. You could even write if (mbus_config.link_freq) return mbus_config.link_freq; /* * Fall back to using the link frequency control if the media bus config * doesn't provide a link frequency. */ return __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div); > > I wonder if we should also add a message in case link_freq is 0, to get > > drivers to convert to reporting the link frequency through > > .get_mbus_config() if they already implement the operation. > > I'm not sure this will be useful: in most cases the LINK_FREQ control is > used and for a reason: it's user-configurable. Drivers should only populate > the field if the link frequency is determined by the driver. For the > receiver drivers it does not matter: they use v4l2_get_link_freq(). I think this depends on whether or not driver that have a configurable link frequency should report the current value through .get_mbus_config(). I think that drivers that implement .get_mbus_config() should, for consistency. > > > } > > > EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_pad); > > > #endif /* CONFIG_MEDIA_CONTROLLER */ > > > diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h > > > index 5bce6e423e94..cc5f776dc662 100644 > > > --- a/include/media/v4l2-mediabus.h > > > +++ b/include/media/v4l2-mediabus.h > > > @@ -148,6 +148,7 @@ enum v4l2_mbus_type { > > > /** > > > * struct v4l2_mbus_config - media bus configuration > > > * @type: interface type > > > + * @link_freq: The link frequency. See also V4L2_CID_LINK_FREQ control. > > > > Not a candidate for this series, but I'd like to simplify drivers by > > implementing the LINK_FREQ control automatically. > > > > > * @bus: bus configuration data structure > > > * @bus.parallel: embedded &struct v4l2_mbus_config_parallel. > > > * Used if the bus is parallel or BT.656. > > > @@ -162,6 +163,7 @@ enum v4l2_mbus_type { > > > */ > > > struct v4l2_mbus_config { > > > enum v4l2_mbus_type type; > > > + u64 link_freq; > > > union { > > > struct v4l2_mbus_config_parallel parallel; > > > struct v4l2_mbus_config_mipi_csi1 mipi_csi1;
Hi Laurent, On Mon, Dec 16, 2024 at 01:16:45PM +0200, Laurent Pinchart wrote: > On Mon, Dec 16, 2024 at 08:46:43AM +0000, Sakari Ailus wrote: > > On Sun, Dec 15, 2024 at 06:59:35PM +0200, Laurent Pinchart wrote: > > > On Tue, Dec 10, 2024 at 09:59:03AM +0200, Sakari Ailus wrote: > > > > Add link_freq field to struct v4l2_mbus_config in order to pass the link > > > > frequency to the receiving sub-device. > > > > > > The documentation of v4l2_get_link_freq() should be expanded to explain > > > the new mode of operation. It should document which of the supported > > > methods are recommended for new drivers. > > > > > > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > > --- > > > > drivers/media/v4l2-core/v4l2-common.c | 11 ++++++++--- > > > > include/media/v4l2-mediabus.h | 2 ++ > > > > 2 files changed, 10 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c > > > > index 9fe74c7e064f..88fbd5608f00 100644 > > > > --- a/drivers/media/v4l2-core/v4l2-common.c > > > > +++ b/drivers/media/v4l2-core/v4l2-common.c > > > > @@ -508,13 +508,18 @@ EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_ctrl); > > > > s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul, > > > > unsigned int div) > > > > { > > > > + struct v4l2_mbus_config mbus_config = {}; > > > > > > Isn't mbus_config fully populated by the .get_mbus_config() operation ? > > > That should be documented in the .get_mbus_config() operation > > > documentation. > > > > It's a good practice to zero the memory before drivers get to work on it. I > > presume drivers will set the fields that are relevant for them and ignore > > the rest. > > > > I can add a note on get_mbus_config() the drivers should set all struct > > fields to known values. > > Thanks. > > > > > 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); > > > > > > if (mbus_config.link_freq) > > > return mbus_config.link_freq; > > > > > > return __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div); > > > > Whether this would be cleaner is debatable at least. I can switch if you > > insist though. > > I think it's nicer. You could even write > > if (mbus_config.link_freq) > return mbus_config.link_freq; > > /* > * Fall back to using the link frequency control if the media bus config > * doesn't provide a link frequency. > */ > return __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div); I can use this. > > > > I wonder if we should also add a message in case link_freq is 0, to get > > > drivers to convert to reporting the link frequency through > > > .get_mbus_config() if they already implement the operation. > > > > I'm not sure this will be useful: in most cases the LINK_FREQ control is > > used and for a reason: it's user-configurable. Drivers should only populate > > the field if the link frequency is determined by the driver. For the > > receiver drivers it does not matter: they use v4l2_get_link_freq(). > > I think this depends on whether or not driver that have a configurable > link frequency should report the current value through > .get_mbus_config(). I think that drivers that implement > .get_mbus_config() should, for consistency. We should have a helper that obtains information using get_mbus_config() as well as the fwnode endpoint. I've proposed that a few times over the years. I'm hoping for someone who needs dynamic configuration e.g. for lane numbers to implement it. :-) I wouldn't try to add more burden for drivers on this. Beyond that, it's common that if you have multiple implementations of something, one of them (the unused one) eventually breaks. What we really need is to obtain the information from the sub-device API, using the method the driver uses to report it.
On Mon, Dec 16, 2024 at 12:15:31PM +0000, Sakari Ailus wrote: > On Mon, Dec 16, 2024 at 01:16:45PM +0200, Laurent Pinchart wrote: > > On Mon, Dec 16, 2024 at 08:46:43AM +0000, Sakari Ailus wrote: > > > On Sun, Dec 15, 2024 at 06:59:35PM +0200, Laurent Pinchart wrote: > > > > On Tue, Dec 10, 2024 at 09:59:03AM +0200, Sakari Ailus wrote: > > > > > Add link_freq field to struct v4l2_mbus_config in order to pass the link > > > > > frequency to the receiving sub-device. > > > > > > > > The documentation of v4l2_get_link_freq() should be expanded to explain > > > > the new mode of operation. It should document which of the supported > > > > methods are recommended for new drivers. > > > > > > > > > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > > > --- > > > > > drivers/media/v4l2-core/v4l2-common.c | 11 ++++++++--- > > > > > include/media/v4l2-mediabus.h | 2 ++ > > > > > 2 files changed, 10 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c > > > > > index 9fe74c7e064f..88fbd5608f00 100644 > > > > > --- a/drivers/media/v4l2-core/v4l2-common.c > > > > > +++ b/drivers/media/v4l2-core/v4l2-common.c > > > > > @@ -508,13 +508,18 @@ EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_ctrl); > > > > > s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul, > > > > > unsigned int div) > > > > > { > > > > > + struct v4l2_mbus_config mbus_config = {}; > > > > > > > > Isn't mbus_config fully populated by the .get_mbus_config() operation ? > > > > That should be documented in the .get_mbus_config() operation > > > > documentation. > > > > > > It's a good practice to zero the memory before drivers get to work on it. I > > > presume drivers will set the fields that are relevant for them and ignore > > > the rest. > > > > > > I can add a note on get_mbus_config() the drivers should set all struct > > > fields to known values. > > > > Thanks. > > > > > > > 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); > > > > > > > > if (mbus_config.link_freq) > > > > return mbus_config.link_freq; > > > > > > > > return __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div); > > > > > > Whether this would be cleaner is debatable at least. I can switch if you > > > insist though. > > > > I think it's nicer. You could even write > > > > if (mbus_config.link_freq) > > return mbus_config.link_freq; > > > > /* > > * Fall back to using the link frequency control if the media bus config > > * doesn't provide a link frequency. > > */ > > return __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div); > > I can use this. > > > > > > > I wonder if we should also add a message in case link_freq is 0, to get > > > > drivers to convert to reporting the link frequency through > > > > .get_mbus_config() if they already implement the operation. > > > > > > I'm not sure this will be useful: in most cases the LINK_FREQ control is > > > used and for a reason: it's user-configurable. Drivers should only populate > > > the field if the link frequency is determined by the driver. For the > > > receiver drivers it does not matter: they use v4l2_get_link_freq(). > > > > I think this depends on whether or not driver that have a configurable > > link frequency should report the current value through > > .get_mbus_config(). I think that drivers that implement > > .get_mbus_config() should, for consistency. > > We should have a helper that obtains information using get_mbus_config() as > well as the fwnode endpoint. I've proposed that a few times over the years. > I'm hoping for someone who needs dynamic configuration e.g. for lane > numbers to implement it. :-) I'm not sure to follow you here, I don't really see how it's related, or exactly what that helper would do. > I wouldn't try to add more burden for drivers on this. Beyond that, it's > common that if you have multiple implementations of something, one of them > (the unused one) eventually breaks. What we really need is to obtain the > information from the sub-device API, using the method the driver uses to > report it. We should have a single way to report a given piece of information, that would be the simplest for drivers and the least error prone. I think drivers should implement the LINK_FREQ control for userspace, and the .get_mbus_config() operation for kernel space.
diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c index 9fe74c7e064f..88fbd5608f00 100644 --- a/drivers/media/v4l2-core/v4l2-common.c +++ b/drivers/media/v4l2-core/v4l2-common.c @@ -508,13 +508,18 @@ EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_ctrl); s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul, unsigned int div) { + 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); #endif /* CONFIG_MEDIA_CONTROLLER */ diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h index 5bce6e423e94..cc5f776dc662 100644 --- a/include/media/v4l2-mediabus.h +++ b/include/media/v4l2-mediabus.h @@ -148,6 +148,7 @@ enum v4l2_mbus_type { /** * struct v4l2_mbus_config - media bus configuration * @type: interface type + * @link_freq: The link frequency. See also V4L2_CID_LINK_FREQ control. * @bus: bus configuration data structure * @bus.parallel: embedded &struct v4l2_mbus_config_parallel. * Used if the bus is parallel or BT.656. @@ -162,6 +163,7 @@ enum v4l2_mbus_type { */ struct v4l2_mbus_config { enum v4l2_mbus_type type; + u64 link_freq; union { struct v4l2_mbus_config_parallel parallel; struct v4l2_mbus_config_mipi_csi1 mipi_csi1;
Add link_freq field to struct v4l2_mbus_config in order to pass the link frequency to the receiving sub-device. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/v4l2-core/v4l2-common.c | 11 ++++++++--- include/media/v4l2-mediabus.h | 2 ++ 2 files changed, 10 insertions(+), 3 deletions(-)