Message-ID: <25ed971e90324ad596353ba6aade2e14c40c7183.camel@puri.sm>
Subject: Re: [PATCH v2 2/2] media: imx: imx8mq-mipi-csi2: remove unneeded
 state variable and function
From:   Martin Kepplinger <martin.kepplinger@puri.sm>
To:     Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc:     slongerbeam@gmail.com, p.zabel@pengutronix.de, mchehab@kernel.org,
        gregkh@linuxfoundation.org, shawnguo@kernel.org,
        kernel@pengutronix.de, festevam@gmail.com, linux-imx@nxp.com,
        linux-media@vger.kernel.org, linux-staging@lists.linux.dev,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        kernel@puri.sm
Date:   Sun, 12 Mar 2023 15:04:09 +0100
In-Reply-To: <20230312133706.GJ2545@pendragon.ideasonboard.com>
References: <20230307150047.1486186-1-martin.kepplinger@puri.sm>
         <20230307150047.1486186-3-martin.kepplinger@puri.sm>
         <20230312133706.GJ2545@pendragon.ideasonboard.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.38.3-1+deb11u1 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

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);
> >  }
> >  
> >  /* ---------------------------------------------------------------
> > --------------
>