| Message ID | 20250210-8qxp_camera-v3-7-324f5105accc@nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: imx8: add camera support | expand |
On Mon, Feb 10, 2025 at 03:59:26PM -0500, Frank Li wrote: > From: "Guoniu.zhou" <guoniu.zhou@nxp.com> > > Introduce `imx8mq_plat_data` along with enable/disable callback operations > to facilitate support for new chips. No functional changes. > > Signed-off-by: Guoniu.zhou <guoniu.zhou@nxp.com> > Signed-off-by: Frank Li <Frank.Li@nxp.com> > --- > Change from v2 to v3 > - none > change from v1 to v2 > - remove internal review tags > --- > drivers/media/platform/nxp/imx8mq-mipi-csi2.c | 60 ++++++++++++++++++++------- > 1 file changed, 46 insertions(+), 14 deletions(-) > > diff --git a/drivers/media/platform/nxp/imx8mq-mipi-csi2.c b/drivers/media/platform/nxp/imx8mq-mipi-csi2.c > index 1f2657cf6e824..b5eae56d92f49 100644 > --- a/drivers/media/platform/nxp/imx8mq-mipi-csi2.c > +++ b/drivers/media/platform/nxp/imx8mq-mipi-csi2.c > @@ -62,6 +62,8 @@ > #define CSI2RX_CFG_VID_P_FIFO_SEND_LEVEL 0x188 > #define CSI2RX_CFG_DISABLE_PAYLOAD_1 0x130 > > +struct csi_state; > + > enum { > ST_POWERED = 1, > ST_STREAMING = 2, > @@ -83,11 +85,11 @@ static const char * const imx8mq_mipi_csi_clk_id[CSI2_NUM_CLKS] = { > > #define CSI2_NUM_CLKS ARRAY_SIZE(imx8mq_mipi_csi_clk_id) > > -#define GPR_CSI2_1_RX_ENABLE BIT(13) > -#define GPR_CSI2_1_VID_INTFC_ENB BIT(12) > -#define GPR_CSI2_1_HSEL BIT(10) > -#define GPR_CSI2_1_CONT_CLK_MODE BIT(8) > -#define GPR_CSI2_1_S_PRG_RXHS_SETTLE(x) (((x) & 0x3f) << 2) > +struct imx8mq_plat_data { > + const char *name; The name is not used, drop it. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + int (*enable)(struct csi_state *state, u32 hs_settle); > + void (*disable)(struct csi_state *state); > +}; > > /* > * The send level configures the number of entries that must accumulate in > @@ -106,6 +108,7 @@ static const char * const imx8mq_mipi_csi_clk_id[CSI2_NUM_CLKS] = { > > struct csi_state { > struct device *dev; > + const struct imx8mq_plat_data *pdata; > void __iomem *regs; > struct clk_bulk_data clks[CSI2_NUM_CLKS]; > struct reset_control *rst; > @@ -137,6 +140,35 @@ struct csi2_pix_format { > u8 width; > }; > > +/* ----------------------------------------------------------------------------- > + * i.MX8MQ GPR > + */ > + > +#define GPR_CSI2_1_RX_ENABLE BIT(13) > +#define GPR_CSI2_1_VID_INTFC_ENB BIT(12) > +#define GPR_CSI2_1_HSEL BIT(10) > +#define GPR_CSI2_1_CONT_CLK_MODE BIT(8) > +#define GPR_CSI2_1_S_PRG_RXHS_SETTLE(x) (((x) & 0x3f) << 2) > + > +static int imx8mq_gpr_enable(struct csi_state *state, u32 hs_settle) > +{ > + 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 const struct imx8mq_plat_data imx8mq_data = { > + .name = "i.MX8MQ", > + .enable = imx8mq_gpr_enable, > +}; > + > static const struct csi2_pix_format imx8mq_mipi_csi_formats[] = { > /* RAW (Bayer and greyscale) formats. */ > { > @@ -364,14 +396,9 @@ static int imx8mq_mipi_csi_start_stream(struct csi_state *state, > if (ret) > return ret; > > - 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)); > + ret = state->pdata->enable(state, hs_settle); > + if (ret) > + return ret; > > return 0; > } > @@ -379,6 +406,9 @@ static int imx8mq_mipi_csi_start_stream(struct csi_state *state, > static void imx8mq_mipi_csi_stop_stream(struct csi_state *state) > { > imx8mq_mipi_csi_write(state, CSI2RX_CFG_DISABLE_DATA_LANES, 0xf); > + > + if (state->pdata->disable) > + state->pdata->disable(state); > } > > /* ----------------------------------------------------------------------------- > @@ -869,6 +899,8 @@ static int imx8mq_mipi_csi_probe(struct platform_device *pdev) > > state->dev = dev; > > + state->pdata = of_device_get_match_data(dev); > + > ret = imx8mq_mipi_csi_parse_dt(state); > if (ret < 0) { > dev_err(dev, "Failed to parse device tree: %d\n", ret); > @@ -946,7 +978,7 @@ static void imx8mq_mipi_csi_remove(struct platform_device *pdev) > } > > static const struct of_device_id imx8mq_mipi_csi_of_match[] = { > - { .compatible = "fsl,imx8mq-mipi-csi2", }, > + { .compatible = "fsl,imx8mq-mipi-csi2", .data = &imx8mq_data }, > { /* sentinel */ }, > }; > MODULE_DEVICE_TABLE(of, imx8mq_mipi_csi_of_match);
On Mon, Feb 10, 2025 at 11:02 PM Frank Li <Frank.Li@nxp.com> wrote: > > From: "Guoniu.zhou" <guoniu.zhou@nxp.com> > > Introduce `imx8mq_plat_data` along with enable/disable callback operations > to facilitate support for new chips. No functional changes. > > Signed-off-by: Guoniu.zhou <guoniu.zhou@nxp.com> > Signed-off-by: Frank Li <Frank.Li@nxp.com> > --- > Change from v2 to v3 > - none > change from v1 to v2 > - remove internal review tags > --- > drivers/media/platform/nxp/imx8mq-mipi-csi2.c | 60 ++++++++++++++++++++------- > 1 file changed, 46 insertions(+), 14 deletions(-) > > diff --git a/drivers/media/platform/nxp/imx8mq-mipi-csi2.c b/drivers/media/platform/nxp/imx8mq-mipi-csi2.c > index 1f2657cf6e824..b5eae56d92f49 100644 > --- a/drivers/media/platform/nxp/imx8mq-mipi-csi2.c > +++ b/drivers/media/platform/nxp/imx8mq-mipi-csi2.c > @@ -62,6 +62,8 @@ > #define CSI2RX_CFG_VID_P_FIFO_SEND_LEVEL 0x188 > #define CSI2RX_CFG_DISABLE_PAYLOAD_1 0x130 > > +struct csi_state; > + > enum { > ST_POWERED = 1, > ST_STREAMING = 2, > @@ -83,11 +85,11 @@ static const char * const imx8mq_mipi_csi_clk_id[CSI2_NUM_CLKS] = { > > #define CSI2_NUM_CLKS ARRAY_SIZE(imx8mq_mipi_csi_clk_id) > > -#define GPR_CSI2_1_RX_ENABLE BIT(13) > -#define GPR_CSI2_1_VID_INTFC_ENB BIT(12) > -#define GPR_CSI2_1_HSEL BIT(10) > -#define GPR_CSI2_1_CONT_CLK_MODE BIT(8) > -#define GPR_CSI2_1_S_PRG_RXHS_SETTLE(x) (((x) & 0x3f) << 2) > +struct imx8mq_plat_data { > + const char *name; > + int (*enable)(struct csi_state *state, u32 hs_settle); > + void (*disable)(struct csi_state *state); > +}; > > /* > * The send level configures the number of entries that must accumulate in > @@ -106,6 +108,7 @@ static const char * const imx8mq_mipi_csi_clk_id[CSI2_NUM_CLKS] = { > > struct csi_state { > struct device *dev; > + const struct imx8mq_plat_data *pdata; > void __iomem *regs; > struct clk_bulk_data clks[CSI2_NUM_CLKS]; > struct reset_control *rst; > @@ -137,6 +140,35 @@ struct csi2_pix_format { > u8 width; > }; > > +/* ----------------------------------------------------------------------------- I would drop this line. It doesn't make code easier to read. > + * i.MX8MQ GPR > + */ Just say: /* i.MX8MQ GPR */ This pattern happens in a lot of places.
On Fri, Mar 28, 2025 at 10:35:44AM +0200, Daniel Baluta wrote: > On Mon, Feb 10, 2025 at 11:02 PM Frank Li <Frank.Li@nxp.com> wrote: > > > > From: "Guoniu.zhou" <guoniu.zhou@nxp.com> > > > > Introduce `imx8mq_plat_data` along with enable/disable callback operations > > to facilitate support for new chips. No functional changes. > > > > Signed-off-by: Guoniu.zhou <guoniu.zhou@nxp.com> > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > --- > > Change from v2 to v3 > > - none > > change from v1 to v2 > > - remove internal review tags > > --- > > drivers/media/platform/nxp/imx8mq-mipi-csi2.c | 60 ++++++++++++++++++++------- > > 1 file changed, 46 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/media/platform/nxp/imx8mq-mipi-csi2.c b/drivers/media/platform/nxp/imx8mq-mipi-csi2.c > > index 1f2657cf6e824..b5eae56d92f49 100644 > > --- a/drivers/media/platform/nxp/imx8mq-mipi-csi2.c > > +++ b/drivers/media/platform/nxp/imx8mq-mipi-csi2.c > > @@ -62,6 +62,8 @@ > > #define CSI2RX_CFG_VID_P_FIFO_SEND_LEVEL 0x188 > > #define CSI2RX_CFG_DISABLE_PAYLOAD_1 0x130 > > > > +struct csi_state; > > + > > enum { > > ST_POWERED = 1, > > ST_STREAMING = 2, > > @@ -83,11 +85,11 @@ static const char * const imx8mq_mipi_csi_clk_id[CSI2_NUM_CLKS] = { > > > > #define CSI2_NUM_CLKS ARRAY_SIZE(imx8mq_mipi_csi_clk_id) > > > > -#define GPR_CSI2_1_RX_ENABLE BIT(13) > > -#define GPR_CSI2_1_VID_INTFC_ENB BIT(12) > > -#define GPR_CSI2_1_HSEL BIT(10) > > -#define GPR_CSI2_1_CONT_CLK_MODE BIT(8) > > -#define GPR_CSI2_1_S_PRG_RXHS_SETTLE(x) (((x) & 0x3f) << 2) > > +struct imx8mq_plat_data { > > + const char *name; > > + int (*enable)(struct csi_state *state, u32 hs_settle); > > + void (*disable)(struct csi_state *state); > > +}; > > > > /* > > * The send level configures the number of entries that must accumulate in > > @@ -106,6 +108,7 @@ static const char * const imx8mq_mipi_csi_clk_id[CSI2_NUM_CLKS] = { > > > > struct csi_state { > > struct device *dev; > > + const struct imx8mq_plat_data *pdata; > > void __iomem *regs; > > struct clk_bulk_data clks[CSI2_NUM_CLKS]; > > struct reset_control *rst; > > @@ -137,6 +140,35 @@ struct csi2_pix_format { > > u8 width; > > }; > > > > +/* ----------------------------------------------------------------------------- > > I would drop this line. It doesn't make code easier to read. I personally find that clear section markers make the code easier to read. It's a personal preference though, so I leave it to individual driver maintainers, and aim for consistency within drivers. > > + * i.MX8MQ GPR > > + */ > > Just say: /* i.MX8MQ GPR */ > > This pattern happens in a lot of places.
diff --git a/drivers/media/platform/nxp/imx8mq-mipi-csi2.c b/drivers/media/platform/nxp/imx8mq-mipi-csi2.c index 1f2657cf6e824..b5eae56d92f49 100644 --- a/drivers/media/platform/nxp/imx8mq-mipi-csi2.c +++ b/drivers/media/platform/nxp/imx8mq-mipi-csi2.c @@ -62,6 +62,8 @@ #define CSI2RX_CFG_VID_P_FIFO_SEND_LEVEL 0x188 #define CSI2RX_CFG_DISABLE_PAYLOAD_1 0x130 +struct csi_state; + enum { ST_POWERED = 1, ST_STREAMING = 2, @@ -83,11 +85,11 @@ static const char * const imx8mq_mipi_csi_clk_id[CSI2_NUM_CLKS] = { #define CSI2_NUM_CLKS ARRAY_SIZE(imx8mq_mipi_csi_clk_id) -#define GPR_CSI2_1_RX_ENABLE BIT(13) -#define GPR_CSI2_1_VID_INTFC_ENB BIT(12) -#define GPR_CSI2_1_HSEL BIT(10) -#define GPR_CSI2_1_CONT_CLK_MODE BIT(8) -#define GPR_CSI2_1_S_PRG_RXHS_SETTLE(x) (((x) & 0x3f) << 2) +struct imx8mq_plat_data { + const char *name; + int (*enable)(struct csi_state *state, u32 hs_settle); + void (*disable)(struct csi_state *state); +}; /* * The send level configures the number of entries that must accumulate in @@ -106,6 +108,7 @@ static const char * const imx8mq_mipi_csi_clk_id[CSI2_NUM_CLKS] = { struct csi_state { struct device *dev; + const struct imx8mq_plat_data *pdata; void __iomem *regs; struct clk_bulk_data clks[CSI2_NUM_CLKS]; struct reset_control *rst; @@ -137,6 +140,35 @@ struct csi2_pix_format { u8 width; }; +/* ----------------------------------------------------------------------------- + * i.MX8MQ GPR + */ + +#define GPR_CSI2_1_RX_ENABLE BIT(13) +#define GPR_CSI2_1_VID_INTFC_ENB BIT(12) +#define GPR_CSI2_1_HSEL BIT(10) +#define GPR_CSI2_1_CONT_CLK_MODE BIT(8) +#define GPR_CSI2_1_S_PRG_RXHS_SETTLE(x) (((x) & 0x3f) << 2) + +static int imx8mq_gpr_enable(struct csi_state *state, u32 hs_settle) +{ + 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 const struct imx8mq_plat_data imx8mq_data = { + .name = "i.MX8MQ", + .enable = imx8mq_gpr_enable, +}; + static const struct csi2_pix_format imx8mq_mipi_csi_formats[] = { /* RAW (Bayer and greyscale) formats. */ { @@ -364,14 +396,9 @@ static int imx8mq_mipi_csi_start_stream(struct csi_state *state, if (ret) return ret; - 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)); + ret = state->pdata->enable(state, hs_settle); + if (ret) + return ret; return 0; } @@ -379,6 +406,9 @@ static int imx8mq_mipi_csi_start_stream(struct csi_state *state, static void imx8mq_mipi_csi_stop_stream(struct csi_state *state) { imx8mq_mipi_csi_write(state, CSI2RX_CFG_DISABLE_DATA_LANES, 0xf); + + if (state->pdata->disable) + state->pdata->disable(state); } /* ----------------------------------------------------------------------------- @@ -869,6 +899,8 @@ static int imx8mq_mipi_csi_probe(struct platform_device *pdev) state->dev = dev; + state->pdata = of_device_get_match_data(dev); + ret = imx8mq_mipi_csi_parse_dt(state); if (ret < 0) { dev_err(dev, "Failed to parse device tree: %d\n", ret); @@ -946,7 +978,7 @@ static void imx8mq_mipi_csi_remove(struct platform_device *pdev) } static const struct of_device_id imx8mq_mipi_csi_of_match[] = { - { .compatible = "fsl,imx8mq-mipi-csi2", }, + { .compatible = "fsl,imx8mq-mipi-csi2", .data = &imx8mq_data }, { /* sentinel */ }, }; MODULE_DEVICE_TABLE(of, imx8mq_mipi_csi_of_match);