[PATCH v2 0/3] Drop control handler argument support for v4l2_get_link_freq()

[PATCH v2 0/3] Drop control handler argument support for v4l2_get_link_freq()

[Sakari Ailus]

Hi folks,

This set removes the workaround from v4l2_get_link_freq() that allowed
calling it on a control handler. The Amlogic c3-mipi-csi2 driver is
converted in the process as well.

As a result there's also no definition of v4l2_get_link_freq() without
CONFIG_MEDIA_CONTROLLER but that should be fine now. The Amlogic patch is
untested.

since v2:

- Improve documentation for v4l2_get_link_freq(), aligning it to the
  removal of control handler argument support.

- Add a patch to mention the primary source of the link frequency is the
  get_mbus_config pad operation.

since v1:

- Remove redundant src_pad argument from c3_mipi_csi_start_stream().

Sakari Ailus (3):
  media: amlogic: c3-mipi-csi2: Call v4l2_get_link_freq() on source pad
  media: v4l2-common: Drop the workaround from v4l2_get_link_freq()
  media: v4l2-common: Update v4l2_get_link_freq() documentation

 .../amlogic/c3/mipi-csi2/c3-mipi-csi2.c       |  7 +++---
 drivers/media/v4l2-core/v4l2-common.c         | 17 ++++++-------
 include/media/v4l2-common.h                   | 25 ++++++-------------
 3 files changed, 19 insertions(+), 30 deletions(-)

-- 
2.47.2

[PATCH v3 1/3] media: amlogic: c3-mipi-csi2: Call v4l2_get_link_freq() on source pad

[Sakari Ailus]

Call v4l2_get_link_freq() on the source pad so we can remove the
workaround that allows calling it on a control handler.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Keke Li <keke.li@amlogic.com>
Tested-by: Keke Li <keke.li@amlogic.com>
---
 drivers/media/platform/amlogic/c3/mipi-csi2/c3-mipi-csi2.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/amlogic/c3/mipi-csi2/c3-mipi-csi2.c b/drivers/media/platform/amlogic/c3/mipi-csi2/c3-mipi-csi2.c
index 1011ab3ebac7..b9e4ef3fc308 100644
--- a/drivers/media/platform/amlogic/c3/mipi-csi2/c3-mipi-csi2.c
+++ b/drivers/media/platform/amlogic/c3/mipi-csi2/c3-mipi-csi2.c
@@ -388,13 +388,12 @@ static void c3_mipi_csi_cfg_host(struct c3_csi_device *csi)
 	c3_mipi_csi_write(csi, CSI2_HOST_N_LANES, csi->bus.num_data_lanes - 1);
 }
 
-static int c3_mipi_csi_start_stream(struct c3_csi_device *csi,
-				    struct v4l2_subdev *src_sd)
+static int c3_mipi_csi_start_stream(struct c3_csi_device *csi)
 {
 	s64 link_freq;
 	s64 lane_rate;
 
-	link_freq = v4l2_get_link_freq(src_sd->ctrl_handler, 0, 0);
+	link_freq = v4l2_get_link_freq(csi->src_pad, 0, 0);
 	if (link_freq < 0) {
 		dev_err(csi->dev,
 			"Unable to obtain link frequency: %lld\n", link_freq);
@@ -434,7 +433,7 @@ static int c3_mipi_csi_enable_streams(struct v4l2_subdev *sd,
 
 	pm_runtime_resume_and_get(csi->dev);
 
-	c3_mipi_csi_start_stream(csi, src_sd);
+	c3_mipi_csi_start_stream(csi);
 
 	ret = v4l2_subdev_enable_streams(src_sd, csi->src_pad->index, BIT(0));
 	if (ret) {
-- 
2.47.2

[PATCH v3 2/3] media: v4l2-common: Drop the workaround from v4l2_get_link_freq()

[Sakari Ailus]

Remove the workaround that allowed calling v4l2_get_link_freq() on the
control handler.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/v4l2-core/v4l2-common.c | 17 ++++++++---------
 include/media/v4l2-common.h           | 19 ++++---------------
 2 files changed, 12 insertions(+), 24 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index a5334aa35992..0574f5d685f8 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -512,8 +512,9 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
 }
 EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt);
 
-s64 __v4l2_get_link_freq_ctrl(struct v4l2_ctrl_handler *handler,
-			      unsigned int mul, unsigned int div)
+#ifdef CONFIG_MEDIA_CONTROLLER
+static s64 v4l2_get_link_freq_ctrl(struct v4l2_ctrl_handler *handler,
+				   unsigned int mul, unsigned int div)
 {
 	struct v4l2_ctrl *ctrl;
 	s64 freq;
@@ -548,11 +549,9 @@ s64 __v4l2_get_link_freq_ctrl(struct v4l2_ctrl_handler *handler,
 
 	return freq > 0 ? freq : -EINVAL;
 }
-EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_ctrl);
 
-#ifdef CONFIG_MEDIA_CONTROLLER
-s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul,
-			     unsigned int div)
+s64 v4l2_get_link_freq(struct media_pad *pad, unsigned int mul,
+		       unsigned int div)
 {
 	struct v4l2_mbus_config mbus_config = {};
 	struct v4l2_subdev *sd;
@@ -571,10 +570,10 @@ s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul,
 	 * 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);
+	return v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div);
 }
-EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_pad);
-#endif /* CONFIG_MEDIA_CONTROLLER */
+EXPORT_SYMBOL_GPL(v4l2_get_link_freq);
+#endif
 
 /*
  * Simplify a fraction using a simple continued fraction decomposition. The
diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
index 39dd0c78d70f..ab0ce8e605c3 100644
--- a/include/media/v4l2-common.h
+++ b/include/media/v4l2-common.h
@@ -560,15 +560,14 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, u32 pixelformat,
 /**
  * v4l2_get_link_freq - Get link rate from transmitter
  *
- * @pad: The transmitter's media pad (or control handler for non-MC users or
- *	 compatibility reasons, don't use in new code)
+ * @pad: The transmitter's media pad
  * @mul: The multiplier between pixel rate and link frequency. Bits per pixel on
  *	 D-PHY, samples per clock on parallel. 0 otherwise.
  * @div: The divisor between pixel rate and link frequency. Number of data lanes
  *	 times two on D-PHY, 1 on parallel. 0 otherwise.
  *
  * This function is intended for obtaining the link frequency from the
- * transmitter sub-devices. It returns the link rate, either from the
+ * transmitter sub-device's pad. It returns the link rate, either from the
  * V4L2_CID_LINK_FREQ control implemented by the transmitter, or value
  * calculated based on the V4L2_CID_PIXEL_RATE implemented by the transmitter.
  *
@@ -578,19 +577,9 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, u32 pixelformat,
  * * %-EINVAL: Invalid link frequency value
  */
 #ifdef CONFIG_MEDIA_CONTROLLER
-#define v4l2_get_link_freq(pad, mul, div)				\
-	_Generic(pad,							\
-		 struct media_pad *: __v4l2_get_link_freq_pad,		\
-		 struct v4l2_ctrl_handler *: __v4l2_get_link_freq_ctrl)	\
-	(pad, mul, div)
-s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul,
-			     unsigned int div);
-#else
-#define v4l2_get_link_freq(handler, mul, div)		\
-	__v4l2_get_link_freq_ctrl(handler, mul, div)
+s64 v4l2_get_link_freq(struct media_pad *pad, unsigned int mul,
+		       unsigned int div);
 #endif
-s64 __v4l2_get_link_freq_ctrl(struct v4l2_ctrl_handler *handler,
-			      unsigned int mul, unsigned int div);
 
 void v4l2_simplify_fraction(u32 *numerator, u32 *denominator,
 		unsigned int n_terms, unsigned int threshold);
-- 
2.47.2

Re: [PATCH v3 2/3] media: v4l2-common: Drop the workaround from v4l2_get_link_freq()

[Laurent Pinchart]

On Thu, Aug 21, 2025 at 03:12:06PM +0300, Sakari Ailus wrote:
> Remove the workaround that allowed calling v4l2_get_link_freq() on the
> control handler.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> ---
>  drivers/media/v4l2-core/v4l2-common.c | 17 ++++++++---------
>  include/media/v4l2-common.h           | 19 ++++---------------
>  2 files changed, 12 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> index a5334aa35992..0574f5d685f8 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -512,8 +512,9 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
>  }
>  EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt);
>  
> -s64 __v4l2_get_link_freq_ctrl(struct v4l2_ctrl_handler *handler,
> -			      unsigned int mul, unsigned int div)
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +static s64 v4l2_get_link_freq_ctrl(struct v4l2_ctrl_handler *handler,
> +				   unsigned int mul, unsigned int div)
>  {
>  	struct v4l2_ctrl *ctrl;
>  	s64 freq;
> @@ -548,11 +549,9 @@ s64 __v4l2_get_link_freq_ctrl(struct v4l2_ctrl_handler *handler,
>  
>  	return freq > 0 ? freq : -EINVAL;
>  }
> -EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_ctrl);
>  
> -#ifdef CONFIG_MEDIA_CONTROLLER
> -s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul,
> -			     unsigned int div)
> +s64 v4l2_get_link_freq(struct media_pad *pad, unsigned int mul,
> +		       unsigned int div)
>  {
>  	struct v4l2_mbus_config mbus_config = {};
>  	struct v4l2_subdev *sd;
> @@ -571,10 +570,10 @@ s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul,
>  	 * 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);
> +	return v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div);
>  }
> -EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_pad);
> -#endif /* CONFIG_MEDIA_CONTROLLER */
> +EXPORT_SYMBOL_GPL(v4l2_get_link_freq);
> +#endif
>  
>  /*
>   * Simplify a fraction using a simple continued fraction decomposition. The
> diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> index 39dd0c78d70f..ab0ce8e605c3 100644
> --- a/include/media/v4l2-common.h
> +++ b/include/media/v4l2-common.h
> @@ -560,15 +560,14 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, u32 pixelformat,
>  /**
>   * v4l2_get_link_freq - Get link rate from transmitter
>   *
> - * @pad: The transmitter's media pad (or control handler for non-MC users or
> - *	 compatibility reasons, don't use in new code)
> + * @pad: The transmitter's media pad
>   * @mul: The multiplier between pixel rate and link frequency. Bits per pixel on
>   *	 D-PHY, samples per clock on parallel. 0 otherwise.
>   * @div: The divisor between pixel rate and link frequency. Number of data lanes
>   *	 times two on D-PHY, 1 on parallel. 0 otherwise.
>   *
>   * This function is intended for obtaining the link frequency from the
> - * transmitter sub-devices. It returns the link rate, either from the
> + * transmitter sub-device's pad. It returns the link rate, either from the
>   * V4L2_CID_LINK_FREQ control implemented by the transmitter, or value
>   * calculated based on the V4L2_CID_PIXEL_RATE implemented by the transmitter.
>   *
> @@ -578,19 +577,9 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, u32 pixelformat,
>   * * %-EINVAL: Invalid link frequency value
>   */
>  #ifdef CONFIG_MEDIA_CONTROLLER
> -#define v4l2_get_link_freq(pad, mul, div)				\
> -	_Generic(pad,							\
> -		 struct media_pad *: __v4l2_get_link_freq_pad,		\
> -		 struct v4l2_ctrl_handler *: __v4l2_get_link_freq_ctrl)	\
> -	(pad, mul, div)
> -s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul,
> -			     unsigned int div);
> -#else
> -#define v4l2_get_link_freq(handler, mul, div)		\
> -	__v4l2_get_link_freq_ctrl(handler, mul, div)
> +s64 v4l2_get_link_freq(struct media_pad *pad, unsigned int mul,
> +		       unsigned int div);
>  #endif
> -s64 __v4l2_get_link_freq_ctrl(struct v4l2_ctrl_handler *handler,
> -			      unsigned int mul, unsigned int div);
>  
>  void v4l2_simplify_fraction(u32 *numerator, u32 *denominator,
>  		unsigned int n_terms, unsigned int threshold);

-- 
Regards,

Laurent Pinchart

[PATCH v3 3/3] media: v4l2-common: Update v4l2_get_link_freq() documentation

[Sakari Ailus]

Document that v4l2_get_link_freq() obtains the link frequency primarily
by calling the get_mbus_config sub-device pad operation.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 include/media/v4l2-common.h | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
index ab0ce8e605c3..c387deffbdd0 100644
--- a/include/media/v4l2-common.h
+++ b/include/media/v4l2-common.h
@@ -567,9 +567,11 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, u32 pixelformat,
  *	 times two on D-PHY, 1 on parallel. 0 otherwise.
  *
  * This function is intended for obtaining the link frequency from the
- * transmitter sub-device's pad. It returns the link rate, either from the
- * V4L2_CID_LINK_FREQ control implemented by the transmitter, or value
- * calculated based on the V4L2_CID_PIXEL_RATE implemented by the transmitter.
+ * transmitter sub-device's @pad. It returns the link rate, primarily obtained
+ * through the get_mbus_config sub-device pad operation or secondarily through
+ * either from the V4L2_CID_LINK_FREQ control implemented by the transmitter, or
+ * value calculated based on the V4L2_CID_PIXEL_RATE implemented by the
+ * transmitter.
  *
  * Return:
  * * >0: Link frequency
-- 
2.47.2

Re: [PATCH v3 3/3] media: v4l2-common: Update v4l2_get_link_freq() documentation

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Thu, Aug 21, 2025 at 03:12:07PM +0300, Sakari Ailus wrote:
> Document that v4l2_get_link_freq() obtains the link frequency primarily
> by calling the get_mbus_config sub-device pad operation.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  include/media/v4l2-common.h | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> index ab0ce8e605c3..c387deffbdd0 100644
> --- a/include/media/v4l2-common.h
> +++ b/include/media/v4l2-common.h
> @@ -567,9 +567,11 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, u32 pixelformat,
>   *	 times two on D-PHY, 1 on parallel. 0 otherwise.
>   *
>   * This function is intended for obtaining the link frequency from the
> - * transmitter sub-device's pad. It returns the link rate, either from the
> - * V4L2_CID_LINK_FREQ control implemented by the transmitter, or value
> - * calculated based on the V4L2_CID_PIXEL_RATE implemented by the transmitter.
> + * transmitter sub-device's @pad. It returns the link rate, primarily obtained
> + * through the get_mbus_config sub-device pad operation or secondarily through
> + * either from the V4L2_CID_LINK_FREQ control implemented by the transmitter, or

"through either from" should be either "through either" or "either
from".

> + * value calculated based on the V4L2_CID_PIXEL_RATE implemented by the
> + * transmitter.

I think this can be clarified further:

 * This function obtains and returns the link frequency from the transmitter
 * sub-device's pad. The link frequency is retrieved using the get_mbus_config
 * sub-device pad operation. If this fails, the function falls back to obtaining
 * the frequency either directly from the V4L2_CID_LINK_FREQ control if
 * implemented by the transmitter, or by calculating it from the pixel rate
 * obtained from the V4L2_CID_PIXEL_RATE control.

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

>   *
>   * Return:
>   * * >0: Link frequency

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 3/3] media: v4l2-common: Update v4l2_get_link_freq() documentation

[Sakari Ailus]

Hi Laurent,

On Thu, Aug 21, 2025 at 03:40:56PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Thu, Aug 21, 2025 at 03:12:07PM +0300, Sakari Ailus wrote:
> > Document that v4l2_get_link_freq() obtains the link frequency primarily
> > by calling the get_mbus_config sub-device pad operation.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  include/media/v4l2-common.h | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> > index ab0ce8e605c3..c387deffbdd0 100644
> > --- a/include/media/v4l2-common.h
> > +++ b/include/media/v4l2-common.h
> > @@ -567,9 +567,11 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, u32 pixelformat,
> >   *	 times two on D-PHY, 1 on parallel. 0 otherwise.
> >   *
> >   * This function is intended for obtaining the link frequency from the
> > - * transmitter sub-device's pad. It returns the link rate, either from the
> > - * V4L2_CID_LINK_FREQ control implemented by the transmitter, or value
> > - * calculated based on the V4L2_CID_PIXEL_RATE implemented by the transmitter.
> > + * transmitter sub-device's @pad. It returns the link rate, primarily obtained
> > + * through the get_mbus_config sub-device pad operation or secondarily through
> > + * either from the V4L2_CID_LINK_FREQ control implemented by the transmitter, or
> 
> "through either from" should be either "through either" or "either
> from".
> 
> > + * value calculated based on the V4L2_CID_PIXEL_RATE implemented by the
> > + * transmitter.
> 
> I think this can be clarified further:
> 
>  * This function obtains and returns the link frequency from the transmitter
>  * sub-device's pad. The link frequency is retrieved using the get_mbus_config
>  * sub-device pad operation. If this fails, the function falls back to obtaining
>  * the frequency either directly from the V4L2_CID_LINK_FREQ control if
>  * implemented by the transmitter, or by calculating it from the pixel rate
>  * obtained from the V4L2_CID_PIXEL_RATE control.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Thanks. I'll use the text in v4.

-- 
Sakari Ailus

Re: [PATCH v2 0/3] Drop control handler argument support for v4l2_get_link_freq()

[Laurent Pinchart]

On Thu, Aug 21, 2025 at 03:12:04PM +0300, Sakari Ailus wrote:
> Hi folks,
> 
> This set removes the workaround from v4l2_get_link_freq() that allowed
> calling it on a control handler. The Amlogic c3-mipi-csi2 driver is
> converted in the process as well.
> 
> As a result there's also no definition of v4l2_get_link_freq() without
> CONFIG_MEDIA_CONTROLLER but that should be fine now. The Amlogic patch is
> untested.

Could you please cherry-pick
https://lore.kernel.org/linux-media/20250821000944.27849-2-laurent.pinchart@ideasonboard.com/
and include it in this series ?

> since v2:
> 
> - Improve documentation for v4l2_get_link_freq(), aligning it to the
>   removal of control handler argument support.
> 
> - Add a patch to mention the primary source of the link frequency is the
>   get_mbus_config pad operation.
> 
> since v1:
> 
> - Remove redundant src_pad argument from c3_mipi_csi_start_stream().
> 
> Sakari Ailus (3):
>   media: amlogic: c3-mipi-csi2: Call v4l2_get_link_freq() on source pad
>   media: v4l2-common: Drop the workaround from v4l2_get_link_freq()
>   media: v4l2-common: Update v4l2_get_link_freq() documentation
> 
>  .../amlogic/c3/mipi-csi2/c3-mipi-csi2.c       |  7 +++---
>  drivers/media/v4l2-core/v4l2-common.c         | 17 ++++++-------
>  include/media/v4l2-common.h                   | 25 ++++++-------------
>  3 files changed, 19 insertions(+), 30 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 0/3] Drop control handler argument support for v4l2_get_link_freq()

[Sakari Ailus]

Hi Laurent,

On Thu, Aug 21, 2025 at 03:41:48PM +0300, Laurent Pinchart wrote:
> On Thu, Aug 21, 2025 at 03:12:04PM +0300, Sakari Ailus wrote:
> > Hi folks,
> > 
> > This set removes the workaround from v4l2_get_link_freq() that allowed
> > calling it on a control handler. The Amlogic c3-mipi-csi2 driver is
> > converted in the process as well.
> > 
> > As a result there's also no definition of v4l2_get_link_freq() without
> > CONFIG_MEDIA_CONTROLLER but that should be fine now. The Amlogic patch is
> > untested.
> 
> Could you please cherry-pick
> https://lore.kernel.org/linux-media/20250821000944.27849-2-laurent.pinchart@ideasonboard.com/
> and include it in this series ?

If a pad is const, I'd expect the container sub-device would be, too. I'll
see if anything breaks if media_entity_to_v4l2_subdev() would switch to use
container_of_const().

-- 
Regards,

Sakari Ailus

Re: [PATCH v2 0/3] Drop control handler argument support for v4l2_get_link_freq()

[Laurent Pinchart]

On Thu, Aug 21, 2025 at 03:56:20PM +0300, Sakari Ailus wrote:
> On Thu, Aug 21, 2025 at 03:41:48PM +0300, Laurent Pinchart wrote:
> > On Thu, Aug 21, 2025 at 03:12:04PM +0300, Sakari Ailus wrote:
> > > Hi folks,
> > > 
> > > This set removes the workaround from v4l2_get_link_freq() that allowed
> > > calling it on a control handler. The Amlogic c3-mipi-csi2 driver is
> > > converted in the process as well.
> > > 
> > > As a result there's also no definition of v4l2_get_link_freq() without
> > > CONFIG_MEDIA_CONTROLLER but that should be fine now. The Amlogic patch is
> > > untested.
> > 
> > Could you please cherry-pick
> > https://lore.kernel.org/linux-media/20250821000944.27849-2-laurent.pinchart@ideasonboard.com/
> > and include it in this series ?
> 
> If a pad is const, I'd expect the container sub-device would be, too. I'll
> see if anything breaks if media_entity_to_v4l2_subdev() would switch to use
> container_of_const().

pad->entity will still not be const, so media_entity_to_v4l2_subdev()
will return a non-const pointer anyway.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 0/3] Drop control handler argument support for v4l2_get_link_freq()

[Sakari Ailus]

On Thu, Aug 21, 2025 at 04:01:25PM +0300, Laurent Pinchart wrote:
> On Thu, Aug 21, 2025 at 03:56:20PM +0300, Sakari Ailus wrote:
> > On Thu, Aug 21, 2025 at 03:41:48PM +0300, Laurent Pinchart wrote:
> > > On Thu, Aug 21, 2025 at 03:12:04PM +0300, Sakari Ailus wrote:
> > > > Hi folks,
> > > > 
> > > > This set removes the workaround from v4l2_get_link_freq() that allowed
> > > > calling it on a control handler. The Amlogic c3-mipi-csi2 driver is
> > > > converted in the process as well.
> > > > 
> > > > As a result there's also no definition of v4l2_get_link_freq() without
> > > > CONFIG_MEDIA_CONTROLLER but that should be fine now. The Amlogic patch is
> > > > untested.
> > > 
> > > Could you please cherry-pick
> > > https://lore.kernel.org/linux-media/20250821000944.27849-2-laurent.pinchart@ideasonboard.com/
> > > and include it in this series ?
> > 
> > If a pad is const, I'd expect the container sub-device would be, too. I'll
> > see if anything breaks if media_entity_to_v4l2_subdev() would switch to use
> > container_of_const().
> 
> pad->entity will still not be const, so media_entity_to_v4l2_subdev()
> will return a non-const pointer anyway.

It may be technically correct but const suggests the argument (and what is
referred from that) is const henceforth, but that's not the case.

I simply wouldn't change this. 

-- 
Sakari Ailus

Re: [PATCH v2 0/3] Drop control handler argument support for v4l2_get_link_freq()

[Laurent Pinchart]

On Thu, Aug 21, 2025 at 04:24:04PM +0300, Sakari Ailus wrote:
> On Thu, Aug 21, 2025 at 04:01:25PM +0300, Laurent Pinchart wrote:
> > On Thu, Aug 21, 2025 at 03:56:20PM +0300, Sakari Ailus wrote:
> > > On Thu, Aug 21, 2025 at 03:41:48PM +0300, Laurent Pinchart wrote:
> > > > On Thu, Aug 21, 2025 at 03:12:04PM +0300, Sakari Ailus wrote:
> > > > > Hi folks,
> > > > > 
> > > > > This set removes the workaround from v4l2_get_link_freq() that allowed
> > > > > calling it on a control handler. The Amlogic c3-mipi-csi2 driver is
> > > > > converted in the process as well.
> > > > > 
> > > > > As a result there's also no definition of v4l2_get_link_freq() without
> > > > > CONFIG_MEDIA_CONTROLLER but that should be fine now. The Amlogic patch is
> > > > > untested.
> > > > 
> > > > Could you please cherry-pick
> > > > https://lore.kernel.org/linux-media/20250821000944.27849-2-laurent.pinchart@ideasonboard.com/
> > > > and include it in this series ?
> > > 
> > > If a pad is const, I'd expect the container sub-device would be, too. I'll
> > > see if anything breaks if media_entity_to_v4l2_subdev() would switch to use
> > > container_of_const().
> > 
> > pad->entity will still not be const, so media_entity_to_v4l2_subdev()
> > will return a non-const pointer anyway.
> 
> It may be technically correct but const suggests the argument (and what is
> referred from that) is const henceforth, but that's not the case.

Yes, and that's why I'm adding a const here. The function retrieves the
link frequency, it should not change the state of the pad, entity or
subdev. That's the meaning conveyed by the API change. Internally, due
to the nature of the C language, we go from a const struct media_pad to
a non-const sturct media_entity. That's fine, it's inside the
v4l2_get_link_freq() function, and it means the function must simply be
careful not to break the API contract. It doesn't change the nature of
the contract.

> I simply wouldn't change this. 

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 0/3] Drop control handler argument support for v4l2_get_link_freq()

[Sakari Ailus]

Hi Laurent,

On Thu, Aug 21, 2025 at 04:27:09PM +0300, Laurent Pinchart wrote:
> On Thu, Aug 21, 2025 at 04:24:04PM +0300, Sakari Ailus wrote:
> > On Thu, Aug 21, 2025 at 04:01:25PM +0300, Laurent Pinchart wrote:
> > > On Thu, Aug 21, 2025 at 03:56:20PM +0300, Sakari Ailus wrote:
> > > > On Thu, Aug 21, 2025 at 03:41:48PM +0300, Laurent Pinchart wrote:
> > > > > On Thu, Aug 21, 2025 at 03:12:04PM +0300, Sakari Ailus wrote:
> > > > > > Hi folks,
> > > > > > 
> > > > > > This set removes the workaround from v4l2_get_link_freq() that allowed
> > > > > > calling it on a control handler. The Amlogic c3-mipi-csi2 driver is
> > > > > > converted in the process as well.
> > > > > > 
> > > > > > As a result there's also no definition of v4l2_get_link_freq() without
> > > > > > CONFIG_MEDIA_CONTROLLER but that should be fine now. The Amlogic patch is
> > > > > > untested.
> > > > > 
> > > > > Could you please cherry-pick
> > > > > https://lore.kernel.org/linux-media/20250821000944.27849-2-laurent.pinchart@ideasonboard.com/
> > > > > and include it in this series ?
> > > > 
> > > > If a pad is const, I'd expect the container sub-device would be, too. I'll
> > > > see if anything breaks if media_entity_to_v4l2_subdev() would switch to use
> > > > container_of_const().
> > > 
> > > pad->entity will still not be const, so media_entity_to_v4l2_subdev()
> > > will return a non-const pointer anyway.
> > 
> > It may be technically correct but const suggests the argument (and what is
> > referred from that) is const henceforth, but that's not the case.
> 
> Yes, and that's why I'm adding a const here. The function retrieves the
> link frequency, it should not change the state of the pad, entity or
> subdev. That's the meaning conveyed by the API change. Internally, due
> to the nature of the C language, we go from a const struct media_pad to
> a non-const sturct media_entity. That's fine, it's inside the
> v4l2_get_link_freq() function, and it means the function must simply be
> careful not to break the API contract. It doesn't change the nature of
> the contract.

I'd like to have a third opinion here. Hans?

> 
> > I simply wouldn't change this. 
> 

-- 
Regards,

Sakari Ailus

Re: [PATCH v2 0/3] Drop control handler argument support for v4l2_get_link_freq()

[Laurent Pinchart]

On Thu, Aug 21, 2025 at 04:30:52PM +0300, Sakari Ailus wrote:
> On Thu, Aug 21, 2025 at 04:27:09PM +0300, Laurent Pinchart wrote:
> > On Thu, Aug 21, 2025 at 04:24:04PM +0300, Sakari Ailus wrote:
> > > On Thu, Aug 21, 2025 at 04:01:25PM +0300, Laurent Pinchart wrote:
> > > > On Thu, Aug 21, 2025 at 03:56:20PM +0300, Sakari Ailus wrote:
> > > > > On Thu, Aug 21, 2025 at 03:41:48PM +0300, Laurent Pinchart wrote:
> > > > > > On Thu, Aug 21, 2025 at 03:12:04PM +0300, Sakari Ailus wrote:
> > > > > > > Hi folks,
> > > > > > > 
> > > > > > > This set removes the workaround from v4l2_get_link_freq() that allowed
> > > > > > > calling it on a control handler. The Amlogic c3-mipi-csi2 driver is
> > > > > > > converted in the process as well.
> > > > > > > 
> > > > > > > As a result there's also no definition of v4l2_get_link_freq() without
> > > > > > > CONFIG_MEDIA_CONTROLLER but that should be fine now. The Amlogic patch is
> > > > > > > untested.
> > > > > > 
> > > > > > Could you please cherry-pick
> > > > > > https://lore.kernel.org/linux-media/20250821000944.27849-2-laurent.pinchart@ideasonboard.com/
> > > > > > and include it in this series ?
> > > > > 
> > > > > If a pad is const, I'd expect the container sub-device would be, too. I'll
> > > > > see if anything breaks if media_entity_to_v4l2_subdev() would switch to use
> > > > > container_of_const().
> > > > 
> > > > pad->entity will still not be const, so media_entity_to_v4l2_subdev()
> > > > will return a non-const pointer anyway.
> > > 
> > > It may be technically correct but const suggests the argument (and what is
> > > referred from that) is const henceforth, but that's not the case.
> > 
> > Yes, and that's why I'm adding a const here. The function retrieves the
> > link frequency, it should not change the state of the pad, entity or
> > subdev. That's the meaning conveyed by the API change. Internally, due
> > to the nature of the C language, we go from a const struct media_pad to
> > a non-const sturct media_entity. That's fine, it's inside the
> > v4l2_get_link_freq() function, and it means the function must simply be
> > careful not to break the API contract. It doesn't change the nature of
> > the contract.
> 
> I'd like to have a third opinion here. Hans?

To make things clear, what this patch does is change the API contract of
the v4l2_get_link_freq() to tell callers that the function won't modify
the state of the pad, entity or subdev. The C language doesn't fully
enforce that, so the compiler won't catch attempts to modify the state
of those objects inside the implementation of v4l2_get_link_freq().

It may be interesting to add helper macros to make V4L2 more
const-correct with enforcement through the compiler. For instance,
instead of accessing pad->entity directly, we would need a
media_pad_to_entity() macro that propagates const. That's a separate
effort though.

> > > I simply wouldn't change this. 

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 0/3] Drop control handler argument support for v4l2_get_link_freq()

[Hans Verkuil]

On 21/08/2025 15:45, Laurent Pinchart wrote:
> On Thu, Aug 21, 2025 at 04:30:52PM +0300, Sakari Ailus wrote:
>> On Thu, Aug 21, 2025 at 04:27:09PM +0300, Laurent Pinchart wrote:
>>> On Thu, Aug 21, 2025 at 04:24:04PM +0300, Sakari Ailus wrote:
>>>> On Thu, Aug 21, 2025 at 04:01:25PM +0300, Laurent Pinchart wrote:
>>>>> On Thu, Aug 21, 2025 at 03:56:20PM +0300, Sakari Ailus wrote:
>>>>>> On Thu, Aug 21, 2025 at 03:41:48PM +0300, Laurent Pinchart wrote:
>>>>>>> On Thu, Aug 21, 2025 at 03:12:04PM +0300, Sakari Ailus wrote:
>>>>>>>> Hi folks,
>>>>>>>>
>>>>>>>> This set removes the workaround from v4l2_get_link_freq() that allowed
>>>>>>>> calling it on a control handler. The Amlogic c3-mipi-csi2 driver is
>>>>>>>> converted in the process as well.
>>>>>>>>
>>>>>>>> As a result there's also no definition of v4l2_get_link_freq() without
>>>>>>>> CONFIG_MEDIA_CONTROLLER but that should be fine now. The Amlogic patch is
>>>>>>>> untested.
>>>>>>>
>>>>>>> Could you please cherry-pick
>>>>>>> https://lore.kernel.org/linux-media/20250821000944.27849-2-laurent.pinchart@ideasonboard.com/
>>>>>>> and include it in this series ?
>>>>>>
>>>>>> If a pad is const, I'd expect the container sub-device would be, too. I'll
>>>>>> see if anything breaks if media_entity_to_v4l2_subdev() would switch to use
>>>>>> container_of_const().
>>>>>
>>>>> pad->entity will still not be const, so media_entity_to_v4l2_subdev()
>>>>> will return a non-const pointer anyway.
>>>>
>>>> It may be technically correct but const suggests the argument (and what is
>>>> referred from that) is const henceforth, but that's not the case.
>>>
>>> Yes, and that's why I'm adding a const here. The function retrieves the
>>> link frequency, it should not change the state of the pad, entity or
>>> subdev. That's the meaning conveyed by the API change. Internally, due
>>> to the nature of the C language, we go from a const struct media_pad to
>>> a non-const sturct media_entity. That's fine, it's inside the
>>> v4l2_get_link_freq() function, and it means the function must simply be
>>> careful not to break the API contract. It doesn't change the nature of
>>> the contract.
>>
>> I'd like to have a third opinion here. Hans?
> 
> To make things clear, what this patch does is change the API contract of
> the v4l2_get_link_freq() to tell callers that the function won't modify
> the state of the pad, entity or subdev. The C language doesn't fully
> enforce that, so the compiler won't catch attempts to modify the state
> of those objects inside the implementation of v4l2_get_link_freq().
> 
> It may be interesting to add helper macros to make V4L2 more
> const-correct with enforcement through the compiler. For instance,
> instead of accessing pad->entity directly, we would need a
> media_pad_to_entity() macro that propagates const. That's a separate
> effort though.

For what it is worth: I've no problem with adding the const patch from
Laurent to this series.

Regards,

	Hans

> 
>>>> I simply wouldn't change this. 
>