| Message ID | 20230307150047.1486186-3-martin.kepplinger@puri.sm |
|---|---|
| State | New |
| Headers | show |
| Series | media: imx: imx8mq-mipi-csi2: Use V4L2 subdev state | expand |
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); > } > > /* -----------------------------------------------------------------------------
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); > > } > > > > /* --------------------------------------------------------------- > > -------------- >
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 --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); } /* -----------------------------------------------------------------------------
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(-)