From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: guoniu.zhou@oss.nxp.com
Cc: linux-media@vger.kernel.org, linux-imx@nxp.com,
devicetree@vger.kernel.org, mchehab@kernel.org,
robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
conor+dt@kernel.org, xavier.roumegue@oss.nxp.com,
kernel@pengutronix.de, jacopo.mondi@ideasonboard.com,
sakari.ailus@linux.intel.com
Subject: Re: [PATCH v5 2/3] media: nxp: imx8-isi: move i.MX8 gasket configuration to an ops structure
Date: Fri, 28 Jul 2023 00:41:54 +0300 [thread overview]
Message-ID: <20230727214154.GO25174@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20230629013621.2388121-3-guoniu.zhou@oss.nxp.com>
Hi Guoniu,
Thank you for the patch.
On Thu, Jun 29, 2023 at 09:36:20AM +0800, guoniu.zhou@oss.nxp.com wrote:
> From: "Guoniu.zhou" <guoniu.zhou@nxp.com>
>
> The i.MX93 includes an ISI instance compatible with the imx8-isi
> driver, but with a different gasket. To prepare for this, make the
> gasket configuration modular by moving the code to an ops structure.
>
> Signed-off-by: Guoniu.zhou <guoniu.zhou@nxp.com>
> ---
> drivers/media/platform/nxp/imx8-isi/Makefile | 4 +-
> .../platform/nxp/imx8-isi/imx8-isi-core.c | 6 +-
> .../platform/nxp/imx8-isi/imx8-isi-core.h | 12 +++-
> .../platform/nxp/imx8-isi/imx8-isi-crossbar.c | 36 ++----------
> .../platform/nxp/imx8-isi/imx8-isi-gasket.c | 55 +++++++++++++++++++
> 5 files changed, 77 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/media/platform/nxp/imx8-isi/Makefile b/drivers/media/platform/nxp/imx8-isi/Makefile
> index 9bff9297686d..4713c4e8b64b 100644
> --- a/drivers/media/platform/nxp/imx8-isi/Makefile
> +++ b/drivers/media/platform/nxp/imx8-isi/Makefile
> @@ -1,7 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0-only
>
> -imx8-isi-y := imx8-isi-core.o imx8-isi-crossbar.o imx8-isi-hw.o \
> - imx8-isi-pipe.o imx8-isi-video.o
> +imx8-isi-y := imx8-isi-core.o imx8-isi-crossbar.o imx8-isi-gasket.o \
> + imx8-isi-hw.o imx8-isi-pipe.o imx8-isi-video.o
> imx8-isi-$(CONFIG_DEBUG_FS) += imx8-isi-debug.o
> imx8-isi-$(CONFIG_VIDEO_IMX8_ISI_M2M) += imx8-isi-m2m.o
>
> 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 253e77189b69..5165f8960c2c 100644
> --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c
> +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c
> @@ -289,7 +289,7 @@ static const struct mxc_isi_plat_data mxc_imx8mn_data = {
> .clks = mxc_imx8mn_clks,
> .num_clks = ARRAY_SIZE(mxc_imx8mn_clks),
> .buf_active_reverse = false,
> - .has_gasket = true,
> + .gasket_ops = &mxc_imx8_gasket_ops,
> .has_36bit_dma = false,
> };
>
> @@ -303,7 +303,7 @@ static const struct mxc_isi_plat_data mxc_imx8mp_data = {
> .clks = mxc_imx8mn_clks,
> .num_clks = ARRAY_SIZE(mxc_imx8mn_clks),
> .buf_active_reverse = true,
> - .has_gasket = true,
> + .gasket_ops = &mxc_imx8_gasket_ops,
> .has_36bit_dma = true,
> };
>
> @@ -443,7 +443,7 @@ static int mxc_isi_probe(struct platform_device *pdev)
> return PTR_ERR(isi->regs);
> }
>
> - if (isi->pdata->has_gasket) {
> + if (isi->pdata->gasket_ops) {
> isi->gasket = syscon_regmap_lookup_by_phandle(dev->of_node,
> "fsl,blk-ctrl");
> if (IS_ERR(isi->gasket)) {
> 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 e469788a9e6c..78ca047d93d1 100644
> --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
> +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
> @@ -147,6 +147,14 @@ struct mxc_isi_set_thd {
> struct mxc_isi_panic_thd panic_set_thd_v;
> };
>
> +struct mxc_gasket_ops {
> + void (*enable)(struct mxc_isi_dev *isi,
> + const struct v4l2_mbus_frame_desc *fd,
> + const struct v4l2_mbus_framefmt *fmt,
> + const unsigned int port);
> + void (*disable)(struct mxc_isi_dev *isi, const unsigned int port);
> +};
> +
> enum model {
> MXC_ISI_IMX8MN,
> MXC_ISI_IMX8MP,
> @@ -159,10 +167,10 @@ struct mxc_isi_plat_data {
> unsigned int reg_offset;
> const struct mxc_isi_ier_reg *ier_reg;
> const struct mxc_isi_set_thd *set_thd;
> + const struct mxc_gasket_ops *gasket_ops;
> const struct clk_bulk_data *clks;
> unsigned int num_clks;
> bool buf_active_reverse;
> - bool has_gasket;
> bool has_36bit_dma;
> };
>
> @@ -286,6 +294,8 @@ struct mxc_isi_dev {
> struct dentry *debugfs_root;
> };
>
> +extern const struct mxc_gasket_ops mxc_imx8_gasket_ops;
> +
> int mxc_isi_crossbar_init(struct mxc_isi_dev *isi);
> void mxc_isi_crossbar_cleanup(struct mxc_isi_crossbar *xbar);
> int mxc_isi_crossbar_register(struct mxc_isi_crossbar *xbar);
> diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
> index f7447b2f4d77..c6a658ef0c62 100644
> --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
> +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
> @@ -15,7 +15,6 @@
> #include <linux/types.h>
>
> #include <media/media-entity.h>
> -#include <media/mipi-csi2.h>
> #include <media/v4l2-subdev.h>
>
> #include "imx8-isi-core.h"
> @@ -25,32 +24,18 @@ static inline struct mxc_isi_crossbar *to_isi_crossbar(struct v4l2_subdev *sd)
> return container_of(sd, struct mxc_isi_crossbar, sd);
> }
>
> -/* -----------------------------------------------------------------------------
> - * Media block control (i.MX8MN and i.MX8MP only)
> - */
> -#define GASKET_BASE(n) (0x0060 + (n) * 0x30)
> -
> -#define GASKET_CTRL 0x0000
> -#define GASKET_CTRL_DATA_TYPE(dt) ((dt) << 8)
> -#define GASKET_CTRL_DATA_TYPE_MASK (0x3f << 8)
> -#define GASKET_CTRL_DUAL_COMP_ENABLE BIT(1)
> -#define GASKET_CTRL_ENABLE BIT(0)
> -
> -#define GASKET_HSIZE 0x0004
> -#define GASKET_VSIZE 0x0008
> -
> static int mxc_isi_crossbar_gasket_enable(struct mxc_isi_crossbar *xbar,
> struct v4l2_subdev_state *state,
> struct v4l2_subdev *remote_sd,
> u32 remote_pad, unsigned int port)
> {
> struct mxc_isi_dev *isi = xbar->isi;
> + const struct mxc_gasket_ops *gasket_ops = isi->pdata->gasket_ops;
> const struct v4l2_mbus_framefmt *fmt;
> struct v4l2_mbus_frame_desc fd;
> - u32 val;
> int ret;
>
> - if (!isi->pdata->has_gasket)
> + if (!gasket_ops)
> return 0;
>
> /*
> @@ -77,17 +62,7 @@ static int mxc_isi_crossbar_gasket_enable(struct mxc_isi_crossbar *xbar,
> if (!fmt)
> return -EINVAL;
>
> - regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_HSIZE, fmt->width);
> - regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_VSIZE, fmt->height);
> -
> - val = GASKET_CTRL_DATA_TYPE(fd.entry[0].bus.csi2.dt)
> - | GASKET_CTRL_ENABLE;
> -
> - if (fd.entry[0].bus.csi2.dt == MIPI_CSI2_DT_YUV422_8B)
> - val |= GASKET_CTRL_DUAL_COMP_ENABLE;
> -
> - regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_CTRL, val);
> -
> + gasket_ops->enable(isi, &fd, fmt, port);
> return 0;
> }
>
> @@ -95,11 +70,12 @@ static void mxc_isi_crossbar_gasket_disable(struct mxc_isi_crossbar *xbar,
> unsigned int port)
> {
> struct mxc_isi_dev *isi = xbar->isi;
> + const struct mxc_gasket_ops *gasket_ops = isi->pdata->gasket_ops;
>
> - if (!isi->pdata->has_gasket)
> + if (!gasket_ops)
> return;
>
> - regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_CTRL, 0);
> + gasket_ops->disable(isi, port);
> }
>
> /* -----------------------------------------------------------------------------
> diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-gasket.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-gasket.c
> new file mode 100644
> index 000000000000..1d632dc60699
> --- /dev/null
> +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-gasket.c
> @@ -0,0 +1,55 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2019-2023 NXP
> + */
> +
> +#include <linux/regmap.h>
> +
> +#include <media/mipi-csi2.h>
> +
> +#include "imx8-isi-core.h"
> +
> +/* -----------------------------------------------------------------------------
> + * i.MX8MN and i.MX8MP gasket
> + **/
There's an extra star here, it should be
*/
> +
> +#define GASKET_BASE(n) (0x0060 + (n) * 0x30)
> +
> +#define GASKET_CTRL 0x0000
> +#define GASKET_CTRL_DATA_TYPE(dt) ((dt) << 8)
> +#define GASKET_CTRL_DATA_TYPE_MASK (0x3f << 8)
> +#define GASKET_CTRL_DUAL_COMP_ENABLE BIT(1)
> +#define GASKET_CTRL_ENABLE BIT(0)
> +
> +#define GASKET_HSIZE 0x0004
> +#define GASKET_VSIZE 0x0008
> +
> +static void mxc_imx8_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;
> +
> + regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_HSIZE, fmt->width);
> + regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_VSIZE, fmt->height);
> +
> + val = GASKET_CTRL_DATA_TYPE(fd->entry[0].bus.csi2.dt);
> + if (fd->entry[0].bus.csi2.dt == MIPI_CSI2_DT_YUV422_8B)
> + val |= GASKET_CTRL_DUAL_COMP_ENABLE;
> +
> + val |= GASKET_CTRL_ENABLE;
> + regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_CTRL, val);
> +}
> +
> +static void mxc_imx8_gasket_disable(struct mxc_isi_dev *isi,
> + const unsigned int port)
> +{
> + regmap_write(isi->gasket, GASKET_BASE(port) + GASKET_CTRL, 0);
> +}
> +
> +/* Gasket operations for i.MX8MN and i.MX8MP */
This comment can be dropped too, the section header above is enough.
With these minor issues fixed,
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
There's no need to resubmit the series just for this, I can make those
modifications locally if no other changes are needed.
> +const struct mxc_gasket_ops mxc_imx8_gasket_ops = {
> + .enable = mxc_imx8_gasket_enable,
> + .disable = mxc_imx8_gasket_disable,
> +};
--
Regards,
Laurent Pinchart
next prev parent reply other threads:[~2023-07-27 21:41 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-29 1:36 [PATCH v5 0/3] add ISI support for iMX93 guoniu.zhou
2023-06-29 1:36 ` [PATCH v5 1/3] media: dt-bindings: nxp,imx8-isi: add i.MX93 ISI compatible string guoniu.zhou
2023-06-29 1:36 ` [PATCH v5 2/3] media: nxp: imx8-isi: move i.MX8 gasket configuration to an ops structure guoniu.zhou
2023-07-27 21:41 ` Laurent Pinchart [this message]
2023-07-28 1:23 ` G.N. Zhou (OSS)
2023-06-29 1:36 ` [PATCH v5 3/3] media: nxp: imx8-isi: add ISI support for i.MX93 guoniu.zhou
2023-06-29 6:43 ` Alexander Stein
2023-06-29 7:07 ` G.N. Zhou (OSS)
2023-06-29 7:59 ` Alexander Stein
2023-06-29 8:23 ` G.N. Zhou (OSS)
2023-07-27 21:48 ` Laurent Pinchart
2023-07-28 1:23 ` G.N. Zhou (OSS)
2023-07-27 1:22 ` [PATCH v5 0/3] add ISI support for iMX93 G.N. Zhou
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230727214154.GO25174@pendragon.ideasonboard.com \
--to=laurent.pinchart@ideasonboard.com \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=guoniu.zhou@oss.nxp.com \
--cc=jacopo.mondi@ideasonboard.com \
--cc=kernel@pengutronix.de \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-imx@nxp.com \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=robh+dt@kernel.org \
--cc=sakari.ailus@linux.intel.com \
--cc=xavier.roumegue@oss.nxp.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.