Hi Rui,
On Fri, Jan 07, 2022 at 10:27:31AM +0000, Rui Miguel Silva wrote:
> Hi Laurent,
> thanks for the patch.
>
> On Thu Jan 6, 2022 at 5:24 PM WET, Laurent Pinchart wrote:
>
> > Add two debugfs files, ths_settle and tclk_settle, to allow overriding
> > the corresponding timing parameters for test purpose.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > ---
> > drivers/staging/media/imx/imx7-mipi-csis.c | 35 ++++++++++++++++++----
> > 1 file changed, 29 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
> > index c9c0089ad816..d7bcfb1a0c52 100644
> > --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> > +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> > @@ -333,7 +333,11 @@ struct csi_state {
> > spinlock_t slock; /* Protect events */
> > struct mipi_csis_event events[MIPI_CSIS_NUM_EVENTS];
> > struct dentry *debugfs_root;
> > - bool debug;
> > + struct {
> > + bool debug;
>
> I think here would make more sense to call it "enable", I think
> state->debug.enable is more readable than state->debug.debug.
Good idea, I'll do that.
> Other than this LGTM
>
> Reviewed-by: Rui Miguel Silva <rmfrfs@xxxxxxxxx>
>
> > + u32 hs_settle;
> > + u32 clk_settle;
> > + } debug;
> > };
> >
> > /* -----------------------------------------------------------------------------
> > @@ -543,6 +547,18 @@ static int mipi_csis_calculate_params(struct csi_state *state)
> > dev_dbg(state->dev, "lane rate %u, Tclk_settle %u, Ths_settle %u\n",
> > lane_rate, state->clk_settle, state->hs_settle);
> >
> > + if (state->debug.hs_settle < 0xff) {
> > + dev_dbg(state->dev, "overriding Ths_settle with %u\n",
> > + state->debug.hs_settle);
> > + state->hs_settle = state->debug.hs_settle;
> > + }
> > +
> > + if (state->debug.clk_settle < 4) {
> > + dev_dbg(state->dev, "overriding Tclk_settle with %u\n",
> > + state->debug.clk_settle);
> > + state->clk_settle = state->debug.clk_settle;
> > + }
> > +
> > return 0;
> > }
> >
> > @@ -659,7 +675,7 @@ static irqreturn_t mipi_csis_irq_handler(int irq, void *dev_id)
> > spin_lock_irqsave(&state->slock, flags);
> >
> > /* Update the event/error counters */
> > - if ((status & MIPI_CSIS_INT_SRC_ERRORS) || state->debug) {
> > + if ((status & MIPI_CSIS_INT_SRC_ERRORS) || state->debug.debug) {
> > for (i = 0; i < MIPI_CSIS_NUM_EVENTS; i++) {
> > struct mipi_csis_event *event = &state->events[i];
> >
> > @@ -749,7 +765,7 @@ static void mipi_csis_log_counters(struct csi_state *state, bool non_errors)
> > spin_lock_irqsave(&state->slock, flags);
> >
> > for (i = 0; i < num_events; ++i) {
> > - if (state->events[i].counter > 0 || state->debug)
> > + if (state->events[i].counter > 0 || state->debug.debug)
> > dev_info(state->dev, "%s events: %d\n",
> > state->events[i].name,
> > state->events[i].counter);
> > @@ -801,12 +817,19 @@ DEFINE_SHOW_ATTRIBUTE(mipi_csis_dump_regs);
> >
> > static void mipi_csis_debugfs_init(struct csi_state *state)
> > {
> > + state->debug.hs_settle = UINT_MAX;
> > + state->debug.clk_settle = UINT_MAX;
> > +
> > state->debugfs_root = debugfs_create_dir(dev_name(state->dev), NULL);
> >
> > debugfs_create_bool("debug_enable", 0600, state->debugfs_root,
> > - &state->debug);
> > + &state->debug.debug);
> > debugfs_create_file("dump_regs", 0600, state->debugfs_root, state,
> > &mipi_csis_dump_regs_fops);
> > + debugfs_create_u32("tclk_settle", 0600, state->debugfs_root,
> > + &state->debug.clk_settle);
> > + debugfs_create_u32("ths_settle", 0600, state->debugfs_root,
> > + &state->debug.hs_settle);
> > }
> >
> > static void mipi_csis_debugfs_exit(struct csi_state *state)
> > @@ -867,7 +890,7 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
> > ret = 0;
> > mipi_csis_stop_stream(state);
> > state->state &= ~ST_STREAMING;
> > - if (state->debug)
> > + if (state->debug.debug)
> > mipi_csis_log_counters(state, true);
> > }
> >
> > @@ -1064,7 +1087,7 @@ static int mipi_csis_log_status(struct v4l2_subdev *sd)
> >
> > mutex_lock(&state->lock);
> > mipi_csis_log_counters(state, true);
> > - if (state->debug && (state->state & ST_POWERED))
> > + if (state->debug.debug && (state->state & ST_POWERED))
> > mipi_csis_dump_regs(state);
> > mutex_unlock(&state->lock);
> >
--
Regards,
Laurent Pinchart
