diff mbox series

[68/75] media: imx: imx7_mipi_csis: Calculate Ths_settle from source pixel rate

Message ID 20210105152852.5733-69-laurent.pinchart@ideasonboard.com (mailing list archive)
State New
Headers show
Series media: imx: Miscellaneous fixes and cleanups for i.MX7 | expand

Commit Message

Laurent Pinchart Jan. 5, 2021, 3:28 p.m. UTC
The Ths_settle timing parameter depends solely on the pixel rate of the
source. Calculate it at runtime instead of requiring it to be specified
in the device tree.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/staging/media/imx/imx7-mipi-csis.c | 65 ++++++++++++++++++----
 1 file changed, 53 insertions(+), 12 deletions(-)

Comments

Sakari Ailus Jan. 6, 2021, 10:59 p.m. UTC | #1
Hi Laurent,

Thanks for the patchset.

On Tue, Jan 05, 2021 at 05:28:45PM +0200, Laurent Pinchart wrote:
> The Ths_settle timing parameter depends solely on the pixel rate of the
> source. Calculate it at runtime instead of requiring it to be specified
> in the device tree.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/staging/media/imx/imx7-mipi-csis.c | 65 ++++++++++++++++++----
>  1 file changed, 53 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
> index 3c68ee8b2a59..c83450ac37fa 100644
> --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> @@ -24,6 +24,7 @@
>  #include <linux/reset.h>
>  #include <linux/spinlock.h>
>  
> +#include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-fwnode.h>
>  #include <media/v4l2-mc.h>
> @@ -233,7 +234,11 @@ struct csi_state {
>  	struct media_pad pads[CSIS_PADS_NUM];
>  	struct v4l2_subdev mipi_sd;
>  	struct v4l2_async_notifier notifier;
> -	struct v4l2_subdev *src_sd;
> +
> +	struct {
> +		struct v4l2_subdev *sd;
> +		struct v4l2_ctrl *pixel_rate;
> +	} src;
>  
>  	u8 index;
>  	struct platform_device *pdev;
> @@ -482,6 +487,31 @@ static void __mipi_csis_set_format(struct csi_state *state)
>  	mipi_csis_write(state, MIPI_CSIS_ISPRESOL_CH0, val);
>  }
>  
> +static int mipi_csis_calculate_params(struct csi_state *state)
> +{
> +	u64 pixel_rate;
> +	u32 lane_rate;
> +
> +	/* Calculate the line rate from the pixel rate. */
> +	pixel_rate = v4l2_ctrl_g_ctrl_int64(state->src.pixel_rate);

Could you instead use v4l2_get_link_freq()?

I guess we're also moving to the LINK_FREQ control to tell this.

> +	lane_rate = div_u64(pixel_rate, state->bus.num_data_lanes)
> +		  * state->csis_fmt->width;
> +	if (lane_rate < 80000000 || lane_rate > 1500000000) {
> +		dev_dbg(state->dev, "Out-of-bound lane rate %u\n", lane_rate);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * The HSSETTLE counter value is document in a table, but can also
> +	 * easily be calculated.
> +	 */
> +	state->hs_settle = (lane_rate - 5000000) / 45000000;

Much better, thank you!
Laurent Pinchart Jan. 9, 2021, 12:52 a.m. UTC | #2
Hi Sakari,

On Thu, Jan 07, 2021 at 12:59:24AM +0200, Sakari Ailus wrote:
> On Tue, Jan 05, 2021 at 05:28:45PM +0200, Laurent Pinchart wrote:
> > The Ths_settle timing parameter depends solely on the pixel rate of the
> > source. Calculate it at runtime instead of requiring it to be specified
> > in the device tree.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  drivers/staging/media/imx/imx7-mipi-csis.c | 65 ++++++++++++++++++----
> >  1 file changed, 53 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
> > index 3c68ee8b2a59..c83450ac37fa 100644
> > --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> > +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> > @@ -24,6 +24,7 @@
> >  #include <linux/reset.h>
> >  #include <linux/spinlock.h>
> >  
> > +#include <media/v4l2-ctrls.h>
> >  #include <media/v4l2-device.h>
> >  #include <media/v4l2-fwnode.h>
> >  #include <media/v4l2-mc.h>
> > @@ -233,7 +234,11 @@ struct csi_state {
> >  	struct media_pad pads[CSIS_PADS_NUM];
> >  	struct v4l2_subdev mipi_sd;
> >  	struct v4l2_async_notifier notifier;
> > -	struct v4l2_subdev *src_sd;
> > +
> > +	struct {
> > +		struct v4l2_subdev *sd;
> > +		struct v4l2_ctrl *pixel_rate;
> > +	} src;
> >  
> >  	u8 index;
> >  	struct platform_device *pdev;
> > @@ -482,6 +487,31 @@ static void __mipi_csis_set_format(struct csi_state *state)
> >  	mipi_csis_write(state, MIPI_CSIS_ISPRESOL_CH0, val);
> >  }
> >  
> > +static int mipi_csis_calculate_params(struct csi_state *state)
> > +{
> > +	u64 pixel_rate;
> > +	u32 lane_rate;
> > +
> > +	/* Calculate the line rate from the pixel rate. */
> > +	pixel_rate = v4l2_ctrl_g_ctrl_int64(state->src.pixel_rate);
> 
> Could you instead use v4l2_get_link_freq()?
> 
> I guess we're also moving to the LINK_FREQ control to tell this.

I've developed this patch before we discussed moving to LINQ_FREQ :-)
I'll fix this in v2.

> > +	lane_rate = div_u64(pixel_rate, state->bus.num_data_lanes)
> > +		  * state->csis_fmt->width;
> > +	if (lane_rate < 80000000 || lane_rate > 1500000000) {
> > +		dev_dbg(state->dev, "Out-of-bound lane rate %u\n", lane_rate);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/*
> > +	 * The HSSETTLE counter value is document in a table, but can also
> > +	 * easily be calculated.
> > +	 */
> > +	state->hs_settle = (lane_rate - 5000000) / 45000000;
> 
> Much better, thank you!
Rui Miguel Silva Jan. 11, 2021, 9:39 a.m. UTC | #3
On Tue, Jan 05, 2021 at 05:28:45PM +0200, Laurent Pinchart wrote:
> The Ths_settle timing parameter depends solely on the pixel rate of the
> source. Calculate it at runtime instead of requiring it to be specified
> in the device tree.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/staging/media/imx/imx7-mipi-csis.c | 65 ++++++++++++++++++----
>  1 file changed, 53 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
> index 3c68ee8b2a59..c83450ac37fa 100644
> --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> @@ -24,6 +24,7 @@
>  #include <linux/reset.h>
>  #include <linux/spinlock.h>
>  
> +#include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-fwnode.h>
>  #include <media/v4l2-mc.h>
> @@ -233,7 +234,11 @@ struct csi_state {
>  	struct media_pad pads[CSIS_PADS_NUM];
>  	struct v4l2_subdev mipi_sd;
>  	struct v4l2_async_notifier notifier;
> -	struct v4l2_subdev *src_sd;
> +
> +	struct {
> +		struct v4l2_subdev *sd;
> +		struct v4l2_ctrl *pixel_rate;
> +	} src;
>  
>  	u8 index;
>  	struct platform_device *pdev;
> @@ -482,6 +487,31 @@ static void __mipi_csis_set_format(struct csi_state *state)
>  	mipi_csis_write(state, MIPI_CSIS_ISPRESOL_CH0, val);
>  }
>  
> +static int mipi_csis_calculate_params(struct csi_state *state)
> +{
> +	u64 pixel_rate;
> +	u32 lane_rate;
> +
> +	/* Calculate the line rate from the pixel rate. */
> +	pixel_rate = v4l2_ctrl_g_ctrl_int64(state->src.pixel_rate);
> +	lane_rate = div_u64(pixel_rate, state->bus.num_data_lanes)
> +		  * state->csis_fmt->width;
> +	if (lane_rate < 80000000 || lane_rate > 1500000000) {
> +		dev_dbg(state->dev, "Out-of-bound lane rate %u\n", lane_rate);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * The HSSETTLE counter value is document in a table, but can also
> +	 * easily be calculated.
> +	 */
> +	state->hs_settle = (lane_rate - 5000000) / 45000000;
> +	dev_dbg(state->dev, "pixel rate %llu, lane rate %u, Ths_settle %u\n",
> +		pixel_rate, lane_rate, state->hs_settle);
> +
> +	return 0;
> +}
> +
>  static void mipi_csis_set_params(struct csi_state *state)
>  {
>  	int lanes = state->bus.num_data_lanes;
> @@ -608,16 +638,20 @@ static void mipi_csis_log_counters(struct csi_state *state, bool non_errors)
>  static int mipi_csis_s_stream(struct v4l2_subdev *mipi_sd, int enable)
>  {
>  	struct csi_state *state = mipi_sd_to_csis_state(mipi_sd);
> -	int ret = 0;
> +	int ret;
>  
>  	if (enable) {
> +		ret = mipi_csis_calculate_params(state);
> +		if (ret < 0)
> +			return ret;
> +
>  		mipi_csis_clear_counters(state);
>  		ret = pm_runtime_get_sync(&state->pdev->dev);
>  		if (ret < 0) {
>  			pm_runtime_put_noidle(&state->pdev->dev);
>  			return ret;
>  		}
> -		ret = v4l2_subdev_call(state->src_sd, core, s_power, 1);
> +		ret = v4l2_subdev_call(state->src.sd, core, s_power, 1);
>  		if (ret < 0 && ret != -ENOIOCTLCMD)
>  			return ret;
>  	}
> @@ -630,7 +664,7 @@ static int mipi_csis_s_stream(struct v4l2_subdev *mipi_sd, int enable)
>  		}
>  
>  		mipi_csis_start_stream(state);
> -		ret = v4l2_subdev_call(state->src_sd, video, s_stream, 1);
> +		ret = v4l2_subdev_call(state->src.sd, video, s_stream, 1);
>  		if (ret < 0)
>  			goto unlock;
>  
> @@ -638,8 +672,8 @@ static int mipi_csis_s_stream(struct v4l2_subdev *mipi_sd, int enable)
>  
>  		state->flags |= ST_STREAMING;
>  	} else {
> -		v4l2_subdev_call(state->src_sd, video, s_stream, 0);
> -		ret = v4l2_subdev_call(state->src_sd, core, s_power, 0);
> +		v4l2_subdev_call(state->src.sd, video, s_stream, 0);
> +		ret = v4l2_subdev_call(state->src.sd, core, s_power, 0);
>  		if (ret == -ENOIOCTLCMD)
>  			ret = 0;
>  		mipi_csis_stop_stream(state);
> @@ -677,14 +711,24 @@ static int mipi_csis_link_setup(struct media_entity *entity,
>  	mutex_lock(&state->lock);
>  
>  	if (flags & MEDIA_LNK_FL_ENABLED) {
> -		if (state->src_sd) {
> +		if (state->src.sd) {
>  			ret = -EBUSY;
>  			goto out;
>  		}
>  
> -		state->src_sd = remote_sd;
> +		state->src.pixel_rate = v4l2_ctrl_find(remote_sd->ctrl_handler,
> +						       V4L2_CID_PIXEL_RATE);
> +		if (!state->src.pixel_rate) {
> +			dev_err(state->dev,
> +				"source %s has no pixel rate control\n",
> +				remote_sd->name);
> +			return -EINVAL;

This needs to goto out to release lock.

------
Cheers,
     Rui
> +		}
> +
> +		state->src.sd = remote_sd;
>  	} else {
> -		state->src_sd = NULL;
> +		state->src.pixel_rate = NULL;
> +		state->src.sd = NULL;
>  	}
>  
>  out:
> @@ -943,9 +987,6 @@ static int mipi_csis_parse_dt(struct platform_device *pdev,
>  	if (IS_ERR(state->mrst))
>  		return PTR_ERR(state->mrst);
>  
> -	/* Get MIPI CSI-2 bus configuration from the endpoint node. */
> -	of_property_read_u32(node, "fsl,csis-hs-settle", &state->hs_settle);
> -
>  	return 0;
>  }
>  
> -- 
> Regards,
> 
> Laurent Pinchart
>
diff mbox series

Patch

diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
index 3c68ee8b2a59..c83450ac37fa 100644
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -24,6 +24,7 @@ 
 #include <linux/reset.h>
 #include <linux/spinlock.h>
 
+#include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-fwnode.h>
 #include <media/v4l2-mc.h>
@@ -233,7 +234,11 @@  struct csi_state {
 	struct media_pad pads[CSIS_PADS_NUM];
 	struct v4l2_subdev mipi_sd;
 	struct v4l2_async_notifier notifier;
-	struct v4l2_subdev *src_sd;
+
+	struct {
+		struct v4l2_subdev *sd;
+		struct v4l2_ctrl *pixel_rate;
+	} src;
 
 	u8 index;
 	struct platform_device *pdev;
@@ -482,6 +487,31 @@  static void __mipi_csis_set_format(struct csi_state *state)
 	mipi_csis_write(state, MIPI_CSIS_ISPRESOL_CH0, val);
 }
 
+static int mipi_csis_calculate_params(struct csi_state *state)
+{
+	u64 pixel_rate;
+	u32 lane_rate;
+
+	/* Calculate the line rate from the pixel rate. */
+	pixel_rate = v4l2_ctrl_g_ctrl_int64(state->src.pixel_rate);
+	lane_rate = div_u64(pixel_rate, state->bus.num_data_lanes)
+		  * state->csis_fmt->width;
+	if (lane_rate < 80000000 || lane_rate > 1500000000) {
+		dev_dbg(state->dev, "Out-of-bound lane rate %u\n", lane_rate);
+		return -EINVAL;
+	}
+
+	/*
+	 * The HSSETTLE counter value is document in a table, but can also
+	 * easily be calculated.
+	 */
+	state->hs_settle = (lane_rate - 5000000) / 45000000;
+	dev_dbg(state->dev, "pixel rate %llu, lane rate %u, Ths_settle %u\n",
+		pixel_rate, lane_rate, state->hs_settle);
+
+	return 0;
+}
+
 static void mipi_csis_set_params(struct csi_state *state)
 {
 	int lanes = state->bus.num_data_lanes;
@@ -608,16 +638,20 @@  static void mipi_csis_log_counters(struct csi_state *state, bool non_errors)
 static int mipi_csis_s_stream(struct v4l2_subdev *mipi_sd, int enable)
 {
 	struct csi_state *state = mipi_sd_to_csis_state(mipi_sd);
-	int ret = 0;
+	int ret;
 
 	if (enable) {
+		ret = mipi_csis_calculate_params(state);
+		if (ret < 0)
+			return ret;
+
 		mipi_csis_clear_counters(state);
 		ret = pm_runtime_get_sync(&state->pdev->dev);
 		if (ret < 0) {
 			pm_runtime_put_noidle(&state->pdev->dev);
 			return ret;
 		}
-		ret = v4l2_subdev_call(state->src_sd, core, s_power, 1);
+		ret = v4l2_subdev_call(state->src.sd, core, s_power, 1);
 		if (ret < 0 && ret != -ENOIOCTLCMD)
 			return ret;
 	}
@@ -630,7 +664,7 @@  static int mipi_csis_s_stream(struct v4l2_subdev *mipi_sd, int enable)
 		}
 
 		mipi_csis_start_stream(state);
-		ret = v4l2_subdev_call(state->src_sd, video, s_stream, 1);
+		ret = v4l2_subdev_call(state->src.sd, video, s_stream, 1);
 		if (ret < 0)
 			goto unlock;
 
@@ -638,8 +672,8 @@  static int mipi_csis_s_stream(struct v4l2_subdev *mipi_sd, int enable)
 
 		state->flags |= ST_STREAMING;
 	} else {
-		v4l2_subdev_call(state->src_sd, video, s_stream, 0);
-		ret = v4l2_subdev_call(state->src_sd, core, s_power, 0);
+		v4l2_subdev_call(state->src.sd, video, s_stream, 0);
+		ret = v4l2_subdev_call(state->src.sd, core, s_power, 0);
 		if (ret == -ENOIOCTLCMD)
 			ret = 0;
 		mipi_csis_stop_stream(state);
@@ -677,14 +711,24 @@  static int mipi_csis_link_setup(struct media_entity *entity,
 	mutex_lock(&state->lock);
 
 	if (flags & MEDIA_LNK_FL_ENABLED) {
-		if (state->src_sd) {
+		if (state->src.sd) {
 			ret = -EBUSY;
 			goto out;
 		}
 
-		state->src_sd = remote_sd;
+		state->src.pixel_rate = v4l2_ctrl_find(remote_sd->ctrl_handler,
+						       V4L2_CID_PIXEL_RATE);
+		if (!state->src.pixel_rate) {
+			dev_err(state->dev,
+				"source %s has no pixel rate control\n",
+				remote_sd->name);
+			return -EINVAL;
+		}
+
+		state->src.sd = remote_sd;
 	} else {
-		state->src_sd = NULL;
+		state->src.pixel_rate = NULL;
+		state->src.sd = NULL;
 	}
 
 out:
@@ -943,9 +987,6 @@  static int mipi_csis_parse_dt(struct platform_device *pdev,
 	if (IS_ERR(state->mrst))
 		return PTR_ERR(state->mrst);
 
-	/* Get MIPI CSI-2 bus configuration from the endpoint node. */
-	of_property_read_u32(node, "fsl,csis-hs-settle", &state->hs_settle);
-
 	return 0;
 }