mbox series

[v1,0/3] staging: media: imx7-mipi-csis: Small improvements

Message ID 20220106172441.7399-1-laurent.pinchart@ideasonboard.com
Headers show
Series staging: media: imx7-mipi-csis: Small improvements | expand

Message

Laurent Pinchart Jan. 6, 2022, 5:24 p.m. UTC 
Hello,

This small patch series contains small debugging improvements for the
imx7-mipi-csis driver, as well as a fix to make entity names unique for
platforms that have multiple CSI receiver instances (namely the
i.MX8MP).

There's not much more to tell here, please see individual patches for
details.

Laurent Pinchart (3):
  staging: media: imx: imx7-mipi-csis: Dump MIPI_CSIS_FRAME_COUNTER_CH0
    register
  staging: media: imx: imx7_mipi_csis: Add timings override through
    debugfs
  staging: media: imx: imx7-mipi-csis: Make subdev name unique

 drivers/staging/media/imx/imx7-mipi-csis.c | 44 +++++++++++++++++-----
 1 file changed, 34 insertions(+), 10 deletions(-)


base-commit: 68b9bcc8a534cd11fe55f8bc82f948aae7d81b3c

Comments

Rui Miguel Silva Jan. 7, 2022, 10:27 a.m. UTC | #1 
Hi Laurent,
On Thu Jan 6, 2022 at 5:24 PM WET, Laurent Pinchart wrote:

> The frame counter is useful debugging information, add it to the
> register dump printed by mipi_csis_dump_regs().
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
LGTM

Reviewed-by: Rui Miguel Silva <rmfrfs@gmail.com>

Cheers,
  Rui

> ---
>  drivers/staging/media/imx/imx7-mipi-csis.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
> index 2b73fa55c938..c9c0089ad816 100644
> --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> @@ -211,6 +211,8 @@
>  #define MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_FALL	BIT(4)
>  #define MIPI_CSIS_DBG_INTR_SRC_CAM_VSYNC_RISE	BIT(0)
>  
> +#define MIPI_CSIS_FRAME_COUNTER_CH(n)		(0x0100 + (n) * 4)
> +
>  /* Non-image packet data buffers */
>  #define MIPI_CSIS_PKTDATA_ODD			0x2000
>  #define MIPI_CSIS_PKTDATA_EVEN			0x3000
> @@ -773,6 +775,7 @@ static int mipi_csis_dump_regs(struct csi_state *state)
>  		{ MIPI_CSIS_SDW_CONFIG_CH(0), "SDW_CONFIG_CH0" },
>  		{ MIPI_CSIS_SDW_RESOL_CH(0), "SDW_RESOL_CH0" },
>  		{ MIPI_CSIS_DBG_CTRL, "DBG_CTRL" },
> +		{ MIPI_CSIS_FRAME_COUNTER_CH(0), "FRAME_COUNTER_CH0" },
>  	};
>  
>  	unsigned int i;
> -- 
> Regards,
>
> Laurent Pinchart
Rui Miguel Silva Jan. 7, 2022, 10:27 a.m. UTC | #2 
Hi Laurent,
Thanks for the patch.

On Thu Jan 6, 2022 at 5:24 PM WET, Laurent Pinchart wrote:

> When multiple CSIS instances are present in a single graph, they are
> currently all named "imx7-mipi-csis.0", which breaks the entity name
> uniqueness requirement. Fix it by using the device name to create the
> subdev name.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Maybe a Fix tag here to make sure it is backported, since this look to
me a legit bug fix for multiple instances.

Other than that LGTM.

Reviewed-by: Rui Miguel Silva <rmfrfs@gmail.com>

Cheers,
  Rui
> ---
>  drivers/staging/media/imx/imx7-mipi-csis.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
> index d7bcfb1a0c52..6443cca817fe 100644
> --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> @@ -32,7 +32,6 @@
>  #include <media/v4l2-subdev.h>
>  
>  #define CSIS_DRIVER_NAME			"imx7-mipi-csis"
> -#define CSIS_SUBDEV_NAME			CSIS_DRIVER_NAME
>  
>  #define CSIS_PAD_SINK				0
>  #define CSIS_PAD_SOURCE				1
> @@ -313,7 +312,6 @@ struct csi_state {
>  	struct reset_control *mrst;
>  	struct regulator *mipi_phy_regulator;
>  	const struct mipi_csis_info *info;
> -	u8 index;
>  
>  	struct v4l2_subdev sd;
>  	struct media_pad pads[CSIS_PADS_NUM];
> @@ -1329,8 +1327,8 @@ static int mipi_csis_subdev_init(struct csi_state *state)
>  
>  	v4l2_subdev_init(sd, &mipi_csis_subdev_ops);
>  	sd->owner = THIS_MODULE;
> -	snprintf(sd->name, sizeof(sd->name), "%s.%d",
> -		 CSIS_SUBDEV_NAME, state->index);
> +	snprintf(sd->name, sizeof(sd->name), "csis-%s",
> +		 dev_name(state->dev));
>  
>  	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>  	sd->ctrl_handler = NULL;
> -- 
> Regards,
>
> Laurent Pinchart
Sakari Ailus Jan. 7, 2022, 11:52 a.m. UTC | #3 
On Thu, Jan 06, 2022 at 07:24:41PM +0200, Laurent Pinchart wrote:
> When multiple CSIS instances are present in a single graph, they are
> currently all named "imx7-mipi-csis.0", which breaks the entity name
> uniqueness requirement. Fix it by using the device name to create the
> subdev name.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/staging/media/imx/imx7-mipi-csis.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
> index d7bcfb1a0c52..6443cca817fe 100644
> --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> @@ -32,7 +32,6 @@
>  #include <media/v4l2-subdev.h>
>  
>  #define CSIS_DRIVER_NAME			"imx7-mipi-csis"
> -#define CSIS_SUBDEV_NAME			CSIS_DRIVER_NAME
>  
>  #define CSIS_PAD_SINK				0
>  #define CSIS_PAD_SOURCE				1
> @@ -313,7 +312,6 @@ struct csi_state {
>  	struct reset_control *mrst;
>  	struct regulator *mipi_phy_regulator;
>  	const struct mipi_csis_info *info;
> -	u8 index;
>  
>  	struct v4l2_subdev sd;
>  	struct media_pad pads[CSIS_PADS_NUM];
> @@ -1329,8 +1327,8 @@ static int mipi_csis_subdev_init(struct csi_state *state)
>  
>  	v4l2_subdev_init(sd, &mipi_csis_subdev_ops);
>  	sd->owner = THIS_MODULE;
> -	snprintf(sd->name, sizeof(sd->name), "%s.%d",
> -		 CSIS_SUBDEV_NAME, state->index);
> +	snprintf(sd->name, sizeof(sd->name), "csis-%s",
> +		 dev_name(state->dev));
>  
>  	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>  	sd->ctrl_handler = NULL;

Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>