Re: [PATCH v5 3/3] media: nxp: imx8-isi: add ISI support for i.MX93

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Guoniu,

Thank you for the patch.

On Thu, Jun 29, 2023 at 09:36:21AM +0800, guoniu.zhou@oss.nxp.com wrote:
> From: "Guoniu.zhou" <guoniu.zhou@nxp.com>
> 
> i.MX93 use a different gasket which has different register definition

s/use/uses/
s/definition/definitions/

> compared with i.MX8. Hence implement the gasket callbacks in order to
> add ISI support for i.MX93.
> 
> Signed-off-by: Guoniu.zhou <guoniu.zhou@nxp.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  .../platform/nxp/imx8-isi/imx8-isi-core.c     | 15 ++++++++++
>  .../platform/nxp/imx8-isi/imx8-isi-core.h     |  2 ++
>  .../platform/nxp/imx8-isi/imx8-isi-gasket.c   | 30 +++++++++++++++++++
>  3 files changed, 47 insertions(+)
> 
> diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c
> index 5165f8960c2c..27bd18b7ee65 100644
> --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c
> +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c
> @@ -307,6 +307,20 @@ static const struct mxc_isi_plat_data mxc_imx8mp_data = {
>  	.has_36bit_dma		= true,
>  };
>  
> +static const struct mxc_isi_plat_data mxc_imx93_data = {
> +	.model			= MXC_ISI_IMX93,
> +	.num_ports		= 1,
> +	.num_channels		= 1,
> +	.reg_offset		= 0,
> +	.ier_reg		= &mxc_imx8_isi_ier_v2,
> +	.set_thd		= &mxc_imx8_isi_thd_v1,
> +	.clks			= mxc_imx8mn_clks,
> +	.num_clks		= ARRAY_SIZE(mxc_imx8mn_clks),
> +	.buf_active_reverse	= true,
> +	.gasket_ops		= &mxc_imx93_gasket_ops,
> +	.has_36bit_dma		= false,
> +};
> +
>  /* -----------------------------------------------------------------------------
>   * Power management
>   */
> @@ -518,6 +532,7 @@ static int mxc_isi_remove(struct platform_device *pdev)
>  static const struct of_device_id mxc_isi_of_match[] = {
>  	{ .compatible = "fsl,imx8mn-isi", .data = &mxc_imx8mn_data },
>  	{ .compatible = "fsl,imx8mp-isi", .data = &mxc_imx8mp_data },
> +	{ .compatible = "fsl,imx93-isi", .data = &mxc_imx93_data },
>  	{ /* sentinel */ },
>  };
>  MODULE_DEVICE_TABLE(of, mxc_isi_of_match);
> diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
> index 78ca047d93d1..2810ebe9b5f7 100644
> --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
> +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
> @@ -158,6 +158,7 @@ struct mxc_gasket_ops {
>  enum model {
>  	MXC_ISI_IMX8MN,
>  	MXC_ISI_IMX8MP,
> +	MXC_ISI_IMX93,
>  };
>  
>  struct mxc_isi_plat_data {
> @@ -295,6 +296,7 @@ struct mxc_isi_dev {
>  };
>  
>  extern const struct mxc_gasket_ops mxc_imx8_gasket_ops;
> +extern const struct mxc_gasket_ops mxc_imx93_gasket_ops;
>  
>  int mxc_isi_crossbar_init(struct mxc_isi_dev *isi);
>  void mxc_isi_crossbar_cleanup(struct mxc_isi_crossbar *xbar);
> diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-gasket.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-gasket.c
> index 1d632dc60699..50ac1d3a2b6f 100644
> --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-gasket.c
> +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-gasket.c
> @@ -53,3 +53,33 @@ const struct mxc_gasket_ops mxc_imx8_gasket_ops = {
>  	.enable = mxc_imx8_gasket_enable,
>  	.disable = mxc_imx8_gasket_disable,
>  };
> +
> +/* -----------------------------------------------------------------------------
> + * i.MX93 gasket
> + **/

Extra star here too.

> +
> +#define DISP_MIX_CAMERA_MUX                     0x30
> +#define DISP_MIX_CAMERA_MUX_DATA_TYPE(x)        (((x) & 0x3f) << 3)
> +#define DISP_MIX_CAMERA_MUX_GASKET_ENABLE       BIT(16)

Let's add a blank line here.

With those small issues fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I'll make the changes locally and include the series in my next pull
request.

> +static void mxc_imx93_gasket_enable(struct mxc_isi_dev *isi,
> +				    const struct v4l2_mbus_frame_desc *fd,
> +				    const struct v4l2_mbus_framefmt *fmt,
> +				    const unsigned int port)
> +{
> +	u32 val;
> +
> +	val = DISP_MIX_CAMERA_MUX_DATA_TYPE(fd->entry[0].bus.csi2.dt);
> +	val |= DISP_MIX_CAMERA_MUX_GASKET_ENABLE;
> +	regmap_write(isi->gasket, DISP_MIX_CAMERA_MUX, val);
> +}
> +
> +static void mxc_imx93_gasket_disable(struct mxc_isi_dev *isi,
> +				     unsigned int port)
> +{
> +	regmap_write(isi->gasket, DISP_MIX_CAMERA_MUX, 0);
> +}
> +
> +const struct mxc_gasket_ops mxc_imx93_gasket_ops = {
> +	.enable = mxc_imx93_gasket_enable,
> +	.disable = mxc_imx93_gasket_disable,
> +};

-- 
Regards,

Laurent Pinchart