diff mbox series

[v2,2/2] media: imx: imx8mq-mipi-csi2: remove unneeded state variable and function

Message ID 20230307150047.1486186-3-martin.kepplinger@puri.sm (mailing list archive)
State New, archived
Headers show
Series media: imx: imx8mq-mipi-csi2: Use V4L2 subdev state | expand

Commit Message

Martin Kepplinger March 7, 2023, 3 p.m. UTC 
Clean up the driver a bit by inlining the imx8mq_mipi_csi_system_enable()
function to the callsites and removing the hs_settle variable from the
driver state.

Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
---
 drivers/staging/media/imx/imx8mq-mipi-csi2.c | 41 ++++++++------------
 1 file changed, 16 insertions(+), 25 deletions(-)

Comments

Laurent Pinchart March 12, 2023, 1:37 p.m. UTC | #1 
Hi Martin,

Thank you for the patch.

On Tue, Mar 07, 2023 at 04:00:47PM +0100, Martin Kepplinger wrote:
> Clean up the driver a bit by inlining the imx8mq_mipi_csi_system_enable()
> function to the callsites and removing the hs_settle variable from the
> driver state.
> 
> Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>

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

Could I volunteer you to also drop the struct csi_state state field ?
:-)

> ---
>  drivers/staging/media/imx/imx8mq-mipi-csi2.c | 41 ++++++++------------
>  1 file changed, 16 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx8mq-mipi-csi2.c b/drivers/staging/media/imx/imx8mq-mipi-csi2.c
> index 1aa8622a3bae..f10b59b8f1c0 100644
> --- a/drivers/staging/media/imx/imx8mq-mipi-csi2.c
> +++ b/drivers/staging/media/imx/imx8mq-mipi-csi2.c
> @@ -119,9 +119,8 @@ struct csi_state {
>  
>  	struct v4l2_mbus_config_mipi_csi2 bus;
>  
> -	struct mutex lock; /* Protect state and hs_settle */
> +	struct mutex lock; /* Protect state */
>  	u32 state;
> -	u32 hs_settle;
>  
>  	struct regmap *phy_gpr;
>  	u8 phy_gpr_reg;
> @@ -264,23 +263,6 @@ static int imx8mq_mipi_csi_sw_reset(struct csi_state *state)
>  	return 0;
>  }
>  
> -static void imx8mq_mipi_csi_system_enable(struct csi_state *state, int on)
> -{
> -	if (!on) {
> -		imx8mq_mipi_csi_write(state, CSI2RX_CFG_DISABLE_DATA_LANES, 0xf);
> -		return;
> -	}
> -
> -	regmap_update_bits(state->phy_gpr,
> -			   state->phy_gpr_reg,
> -			   0x3fff,
> -			   GPR_CSI2_1_RX_ENABLE |
> -			   GPR_CSI2_1_VID_INTFC_ENB |
> -			   GPR_CSI2_1_HSEL |
> -			   GPR_CSI2_1_CONT_CLK_MODE |
> -			   GPR_CSI2_1_S_PRG_RXHS_SETTLE(state->hs_settle));
> -}
> -
>  static void imx8mq_mipi_csi_set_params(struct csi_state *state)
>  {
>  	int lanes = state->bus.num_data_lanes;
> @@ -321,7 +303,8 @@ static int imx8mq_mipi_csi_clk_get(struct csi_state *state)
>  }
>  
>  static int imx8mq_mipi_csi_calc_hs_settle(struct csi_state *state,
> -					  struct v4l2_subdev_state *sd_state)
> +					  struct v4l2_subdev_state *sd_state,
> +					  u32 *hs_settle)
>  {
>  	s64 link_freq;
>  	u32 lane_rate;
> @@ -377,10 +360,10 @@ static int imx8mq_mipi_csi_calc_hs_settle(struct csi_state *state,
>  	max_ths_settle = 140 + 10 * 1000000 / (lane_rate / 1000);
>  	ths_settle_ns = (min_ths_settle + max_ths_settle) / 2;
>  
> -	state->hs_settle = ths_settle_ns / esc_clk_period_ns - 1;
> +	*hs_settle = ths_settle_ns / esc_clk_period_ns - 1;
>  
>  	dev_dbg(state->dev, "lane rate %u Ths_settle %u hs_settle %u\n",
> -		lane_rate, ths_settle_ns, state->hs_settle);
> +		lane_rate, ths_settle_ns, *hs_settle);
>  
>  	return 0;
>  }
> @@ -389,24 +372,32 @@ static int imx8mq_mipi_csi_start_stream(struct csi_state *state,
>  					struct v4l2_subdev_state *sd_state)
>  {
>  	int ret;
> +	u32 hs_settle;
>  
>  	ret = imx8mq_mipi_csi_sw_reset(state);
>  	if (ret)
>  		return ret;
>  
>  	imx8mq_mipi_csi_set_params(state);
> -	ret = imx8mq_mipi_csi_calc_hs_settle(state, sd_state);
> +	ret = imx8mq_mipi_csi_calc_hs_settle(state, sd_state, &hs_settle);
>  	if (ret)
>  		return ret;
>  
> -	imx8mq_mipi_csi_system_enable(state, true);
> +	regmap_update_bits(state->phy_gpr,
> +			   state->phy_gpr_reg,
> +			   0x3fff,
> +			   GPR_CSI2_1_RX_ENABLE |
> +			   GPR_CSI2_1_VID_INTFC_ENB |
> +			   GPR_CSI2_1_HSEL |
> +			   GPR_CSI2_1_CONT_CLK_MODE |
> +			   GPR_CSI2_1_S_PRG_RXHS_SETTLE(hs_settle));
>  
>  	return 0;
>  }
>  
>  static void imx8mq_mipi_csi_stop_stream(struct csi_state *state)
>  {
> -	imx8mq_mipi_csi_system_enable(state, false);
> +	imx8mq_mipi_csi_write(state, CSI2RX_CFG_DISABLE_DATA_LANES, 0xf);
>  }
>  
>  /* -----------------------------------------------------------------------------
Martin Kepplinger March 12, 2023, 2:04 p.m. UTC | #2 
Am Sonntag, dem 12.03.2023 um 15:37 +0200 schrieb Laurent Pinchart:
> Hi Martin,
> 
> Thank you for the patch.
> 
> On Tue, Mar 07, 2023 at 04:00:47PM +0100, Martin Kepplinger wrote:
> > Clean up the driver a bit by inlining the
> > imx8mq_mipi_csi_system_enable()
> > function to the callsites and removing the hs_settle variable from
> > the
> > driver state.
> > 
> > Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Could I volunteer you to also drop the struct csi_state state field ?
> :-)

sure :) it can become at least a bit more tricky than this patch. I'll
take the time after this is merged.

thanks for the fast reviewing

> 
> > ---
> >  drivers/staging/media/imx/imx8mq-mipi-csi2.c | 41 ++++++++--------
> > ----
> >  1 file changed, 16 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/staging/media/imx/imx8mq-mipi-csi2.c
> > b/drivers/staging/media/imx/imx8mq-mipi-csi2.c
> > index 1aa8622a3bae..f10b59b8f1c0 100644
> > --- a/drivers/staging/media/imx/imx8mq-mipi-csi2.c
> > +++ b/drivers/staging/media/imx/imx8mq-mipi-csi2.c
> > @@ -119,9 +119,8 @@ struct csi_state {
> >  
> >         struct v4l2_mbus_config_mipi_csi2 bus;
> >  
> > -       struct mutex lock; /* Protect state and hs_settle */
> > +       struct mutex lock; /* Protect state */
> >         u32 state;
> > -       u32 hs_settle;
> >  
> >         struct regmap *phy_gpr;
> >         u8 phy_gpr_reg;
> > @@ -264,23 +263,6 @@ static int imx8mq_mipi_csi_sw_reset(struct
> > csi_state *state)
> >         return 0;
> >  }
> >  
> > -static void imx8mq_mipi_csi_system_enable(struct csi_state *state,
> > int on)
> > -{
> > -       if (!on) {
> > -               imx8mq_mipi_csi_write(state,
> > CSI2RX_CFG_DISABLE_DATA_LANES, 0xf);
> > -               return;
> > -       }
> > -
> > -       regmap_update_bits(state->phy_gpr,
> > -                          state->phy_gpr_reg,
> > -                          0x3fff,
> > -                          GPR_CSI2_1_RX_ENABLE |
> > -                          GPR_CSI2_1_VID_INTFC_ENB |
> > -                          GPR_CSI2_1_HSEL |
> > -                          GPR_CSI2_1_CONT_CLK_MODE |
> > -                          GPR_CSI2_1_S_PRG_RXHS_SETTLE(state-
> > >hs_settle));
> > -}
> > -
> >  static void imx8mq_mipi_csi_set_params(struct csi_state *state)
> >  {
> >         int lanes = state->bus.num_data_lanes;
> > @@ -321,7 +303,8 @@ static int imx8mq_mipi_csi_clk_get(struct
> > csi_state *state)
> >  }
> >  
> >  static int imx8mq_mipi_csi_calc_hs_settle(struct csi_state *state,
> > -                                         struct v4l2_subdev_state
> > *sd_state)
> > +                                         struct v4l2_subdev_state
> > *sd_state,
> > +                                         u32 *hs_settle)
> >  {
> >         s64 link_freq;
> >         u32 lane_rate;
> > @@ -377,10 +360,10 @@ static int
> > imx8mq_mipi_csi_calc_hs_settle(struct csi_state *state,
> >         max_ths_settle = 140 + 10 * 1000000 / (lane_rate / 1000);
> >         ths_settle_ns = (min_ths_settle + max_ths_settle) / 2;
> >  
> > -       state->hs_settle = ths_settle_ns / esc_clk_period_ns - 1;
> > +       *hs_settle = ths_settle_ns / esc_clk_period_ns - 1;
> >  
> >         dev_dbg(state->dev, "lane rate %u Ths_settle %u hs_settle
> > %u\n",
> > -               lane_rate, ths_settle_ns, state->hs_settle);
> > +               lane_rate, ths_settle_ns, *hs_settle);
> >  
> >         return 0;
> >  }
> > @@ -389,24 +372,32 @@ static int
> > imx8mq_mipi_csi_start_stream(struct csi_state *state,
> >                                         struct v4l2_subdev_state
> > *sd_state)
> >  {
> >         int ret;
> > +       u32 hs_settle;
> >  
> >         ret = imx8mq_mipi_csi_sw_reset(state);
> >         if (ret)
> >                 return ret;
> >  
> >         imx8mq_mipi_csi_set_params(state);
> > -       ret = imx8mq_mipi_csi_calc_hs_settle(state, sd_state);
> > +       ret = imx8mq_mipi_csi_calc_hs_settle(state, sd_state,
> > &hs_settle);
> >         if (ret)
> >                 return ret;
> >  
> > -       imx8mq_mipi_csi_system_enable(state, true);
> > +       regmap_update_bits(state->phy_gpr,
> > +                          state->phy_gpr_reg,
> > +                          0x3fff,
> > +                          GPR_CSI2_1_RX_ENABLE |
> > +                          GPR_CSI2_1_VID_INTFC_ENB |
> > +                          GPR_CSI2_1_HSEL |
> > +                          GPR_CSI2_1_CONT_CLK_MODE |
> > +                         
> > GPR_CSI2_1_S_PRG_RXHS_SETTLE(hs_settle));
> >  
> >         return 0;
> >  }
> >  
> >  static void imx8mq_mipi_csi_stop_stream(struct csi_state *state)
> >  {
> > -       imx8mq_mipi_csi_system_enable(state, false);
> > +       imx8mq_mipi_csi_write(state, CSI2RX_CFG_DISABLE_DATA_LANES,
> > 0xf);
> >  }
> >  
> >  /* ---------------------------------------------------------------
> > --------------
>
Martin Kepplinger March 25, 2023, 9:59 a.m. UTC | #3 
Am Sonntag, dem 12.03.2023 um 15:04 +0100 schrieb Martin Kepplinger:
> Am Sonntag, dem 12.03.2023 um 15:37 +0200 schrieb Laurent Pinchart:
> > Hi Martin,
> > 
> > Thank you for the patch.
> > 
> > On Tue, Mar 07, 2023 at 04:00:47PM +0100, Martin Kepplinger wrote:
> > > Clean up the driver a bit by inlining the
> > > imx8mq_mipi_csi_system_enable()
> > > function to the callsites and removing the hs_settle variable
> > > from
> > > the
> > > driver state.
> > > 
> > > Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > Could I volunteer you to also drop the struct csi_state state field
> > ?
> > :-)
> 
> sure :) it can become at least a bit more tricky than this patch.
> I'll
> take the time after this is merged.
> 
> thanks for the fast reviewing

Laurent, are these 2 patches queued up somewhere? I'm used to waiting
until they are part of a tree that is part of linux-next before sending
something new. Does that make sense?

thanks,
                      martin
Laurent Pinchart March 25, 2023, 6:49 p.m. UTC | #4 
Hi Martin,

On Sat, Mar 25, 2023 at 10:59:47AM +0100, Martin Kepplinger wrote:
> Am Sonntag, dem 12.03.2023 um 15:04 +0100 schrieb Martin Kepplinger:
> > Am Sonntag, dem 12.03.2023 um 15:37 +0200 schrieb Laurent Pinchart:
> > > On Tue, Mar 07, 2023 at 04:00:47PM +0100, Martin Kepplinger wrote:
> > > > Clean up the driver a bit by inlining the
> > > > imx8mq_mipi_csi_system_enable()
> > > > function to the callsites and removing the hs_settle variable
> > > > from
> > > > the
> > > > driver state.
> > > > 
> > > > Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
> > > 
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > 
> > > Could I volunteer you to also drop the struct csi_state state field ?
> > > :-)
> > 
> > sure :) it can become at least a bit more tricky than this patch. I'll
> > take the time after this is merged.
> > 
> > thanks for the fast reviewing
> 
> Laurent, are these 2 patches queued up somewhere? I'm used to waiting
> until they are part of a tree that is part of linux-next before sending
> something new. Does that make sense?

I've just sent a pull request to Mauro for v6.4. You can find the patch
in my tree at

	git://git.kernel.org/pub/scm/linux/kernel/git/pinchartl/linux.git media-imx-next-20230325
diff mbox series

Patch

diff --git a/drivers/staging/media/imx/imx8mq-mipi-csi2.c b/drivers/staging/media/imx/imx8mq-mipi-csi2.c
index 1aa8622a3bae..f10b59b8f1c0 100644
--- a/drivers/staging/media/imx/imx8mq-mipi-csi2.c
+++ b/drivers/staging/media/imx/imx8mq-mipi-csi2.c
@@ -119,9 +119,8 @@  struct csi_state {
 
 	struct v4l2_mbus_config_mipi_csi2 bus;
 
-	struct mutex lock; /* Protect state and hs_settle */
+	struct mutex lock; /* Protect state */
 	u32 state;
-	u32 hs_settle;
 
 	struct regmap *phy_gpr;
 	u8 phy_gpr_reg;
@@ -264,23 +263,6 @@  static int imx8mq_mipi_csi_sw_reset(struct csi_state *state)
 	return 0;
 }
 
-static void imx8mq_mipi_csi_system_enable(struct csi_state *state, int on)
-{
-	if (!on) {
-		imx8mq_mipi_csi_write(state, CSI2RX_CFG_DISABLE_DATA_LANES, 0xf);
-		return;
-	}
-
-	regmap_update_bits(state->phy_gpr,
-			   state->phy_gpr_reg,
-			   0x3fff,
-			   GPR_CSI2_1_RX_ENABLE |
-			   GPR_CSI2_1_VID_INTFC_ENB |
-			   GPR_CSI2_1_HSEL |
-			   GPR_CSI2_1_CONT_CLK_MODE |
-			   GPR_CSI2_1_S_PRG_RXHS_SETTLE(state->hs_settle));
-}
-
 static void imx8mq_mipi_csi_set_params(struct csi_state *state)
 {
 	int lanes = state->bus.num_data_lanes;
@@ -321,7 +303,8 @@  static int imx8mq_mipi_csi_clk_get(struct csi_state *state)
 }
 
 static int imx8mq_mipi_csi_calc_hs_settle(struct csi_state *state,
-					  struct v4l2_subdev_state *sd_state)
+					  struct v4l2_subdev_state *sd_state,
+					  u32 *hs_settle)
 {
 	s64 link_freq;
 	u32 lane_rate;
@@ -377,10 +360,10 @@  static int imx8mq_mipi_csi_calc_hs_settle(struct csi_state *state,
 	max_ths_settle = 140 + 10 * 1000000 / (lane_rate / 1000);
 	ths_settle_ns = (min_ths_settle + max_ths_settle) / 2;
 
-	state->hs_settle = ths_settle_ns / esc_clk_period_ns - 1;
+	*hs_settle = ths_settle_ns / esc_clk_period_ns - 1;
 
 	dev_dbg(state->dev, "lane rate %u Ths_settle %u hs_settle %u\n",
-		lane_rate, ths_settle_ns, state->hs_settle);
+		lane_rate, ths_settle_ns, *hs_settle);
 
 	return 0;
 }
@@ -389,24 +372,32 @@  static int imx8mq_mipi_csi_start_stream(struct csi_state *state,
 					struct v4l2_subdev_state *sd_state)
 {
 	int ret;
+	u32 hs_settle;
 
 	ret = imx8mq_mipi_csi_sw_reset(state);
 	if (ret)
 		return ret;
 
 	imx8mq_mipi_csi_set_params(state);
-	ret = imx8mq_mipi_csi_calc_hs_settle(state, sd_state);
+	ret = imx8mq_mipi_csi_calc_hs_settle(state, sd_state, &hs_settle);
 	if (ret)
 		return ret;
 
-	imx8mq_mipi_csi_system_enable(state, true);
+	regmap_update_bits(state->phy_gpr,
+			   state->phy_gpr_reg,
+			   0x3fff,
+			   GPR_CSI2_1_RX_ENABLE |
+			   GPR_CSI2_1_VID_INTFC_ENB |
+			   GPR_CSI2_1_HSEL |
+			   GPR_CSI2_1_CONT_CLK_MODE |
+			   GPR_CSI2_1_S_PRG_RXHS_SETTLE(hs_settle));
 
 	return 0;
 }
 
 static void imx8mq_mipi_csi_stop_stream(struct csi_state *state)
 {
-	imx8mq_mipi_csi_system_enable(state, false);
+	imx8mq_mipi_csi_write(state, CSI2RX_CFG_DISABLE_DATA_LANES, 0xf);
 }
 
 /* -----------------------------------------------------------------------------