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 <[email protected]>
---
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);
}
/* -----------------------------------------------------------------------------
--
2.30.2
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 <[email protected]>
Reviewed-by: Laurent Pinchart <[email protected]>
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);
> }
>
> /* -----------------------------------------------------------------------------
--
Regards,
Laurent Pinchart
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 <[email protected]>
>
> Reviewed-by: Laurent Pinchart <[email protected]>
>
> 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);
> > }
> >
> > /* ---------------------------------------------------------------
> > --------------
>
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 <[email protected]>
> >
> > Reviewed-by: Laurent Pinchart <[email protected]>
> >
> > 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
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 <[email protected]>
> > >
> > > Reviewed-by: Laurent Pinchart <[email protected]>
> > >
> > > 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
--
Regards,
Laurent Pinchart