diff mbox series

[v6,2/4] media: v4l: Support obtaining link frequency via get_mbus_config

Message ID 20240516122539.30787-3-sakari.ailus@linux.intel.com
State Superseded
Headers show
Series Use V4L2 mbus config for conveying MEI CSI link frequency | expand

Commit Message

Sakari Ailus May 16, 2024, 12:25 p.m. UTC
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(-)

Comments

Jacopo Mondi May 17, 2024, 10:31 a.m. UTC | #1
On Thu, May 16, 2024 at 03:25:37PM GMT, 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>
> ---
>  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 01650aed7c30..ff859a340c5d 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -506,13 +506,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;

Should we do like what we did with endpoint matching ? (let alone the
fact we then backtracked on that.. :)

WARN to encourage tx drivers to implement get_mbus_config and
advertise the link freq through it ?

>
> -	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;

I will retaliate that link_freq has different meaning for serial and
parallel busses, and it would feel more natural having something like

mipi_csi2.link_freq
parallel.pixel_clock

or do you think it's an overkill ?

>  	union {
>  		struct v4l2_mbus_config_parallel parallel;
>  		struct v4l2_mbus_config_mipi_csi1 mipi_csi1;
> --
> 2.39.2
>
Jacopo Mondi May 28, 2024, 7:16 a.m. UTC | #2
Hi Sakari

On Tue, May 28, 2024 at 06:09:12AM GMT, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Fri, May 17, 2024 at 12:31:43PM +0200, Jacopo Mondi wrote:
> > On Thu, May 16, 2024 at 03:25:37PM GMT, 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>
> > > ---
> > >  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 01650aed7c30..ff859a340c5d 100644
> > > --- a/drivers/media/v4l2-core/v4l2-common.c
> > > +++ b/drivers/media/v4l2-core/v4l2-common.c
> > > @@ -506,13 +506,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;
> >
> > Should we do like what we did with endpoint matching ? (let alone the
> > fact we then backtracked on that.. :)
>
> Hmm. What exactly are you suggesting?
>

See below

> >
> > WARN to encourage tx drivers to implement get_mbus_config and
> > advertise the link freq through it ?
>
> Why? If the value is conveyed by the control, there's no reason to copy it
> here, is it?
>

My understanding is that using get_mbus_config() is preferred, but
yes, if the control is in place the same value should be propagated
through both path, which is probably a biy silly, yeah.

Ok, ignore the suggestion then

> >
> > >
> > > -	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;
> >
> > I will retaliate that link_freq has different meaning for serial and
> > parallel busses, and it would feel more natural having something like
> >
> > mipi_csi2.link_freq
> > parallel.pixel_clock
> >
> > or do you think it's an overkill ?
>
> How is the meaning different? The value reflects the frequency on both
> buses.

The meaning is slightly different. For a parallel bus the "link_freq"
is actually the pixel clock (and thus "link_freq" is an ill-defined concept
for parallel busses ?). For serial busses there actually is a "link
frequency" which corresponds to the clock lane frequency.

In general, however we define it, the link_freq is a bus property and
feels better placed inside the per-bus structures to me.

Up to you

>
> >
> > >  	union {
> > >  		struct v4l2_mbus_config_parallel parallel;
> > >  		struct v4l2_mbus_config_mipi_csi1 mipi_csi1;
>
> --
> Kind regards,
>
> Sakari Ailus
diff mbox series

Patch

diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index 01650aed7c30..ff859a340c5d 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -506,13 +506,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;