[PATCH v2 4/6] media: synopsys: csi2rx: Use enum and u32 array for register offsets
Michael Riesch
michael.riesch at collabora.com
Mon Feb 16 00:11:56 PST 2026
Hi Frank,
Thanks for the update.
Some minor nitpicks, but it already looks good.
On 2/13/26 21:25, Frank Li wrote:
> Use enum dw_mipi_csi2rx_regs_index together with a u32 array to describe
> register offsets. This allows supporting new IP versions with different
> register layouts in a structured way.
>
> Add rk3568_regs matching the previous macro definitions and pass it as
> driver data during probe.
>
> No functional change intended.
>
> Signed-off-by: Frank Li <Frank.Li at nxp.com>
> ---
> change in v2
> - change to use enum and u32 array method
> - use order
> - #includes
> - #defines
> - enum and struct definitions
> - the rest
> ---
> drivers/media/platform/synopsys/dw-mipi-csi2rx.c | 91 ++++++++++++++++++++----
> 1 file changed, 78 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/media/platform/synopsys/dw-mipi-csi2rx.c b/drivers/media/platform/synopsys/dw-mipi-csi2rx.c
> index a6d251ca5ad14c5138a6fd0202a970460e64c68f..b00ae5fb328da4cc78fe36b629d6661d438e124a 100644
> --- a/drivers/media/platform/synopsys/dw-mipi-csi2rx.c
> +++ b/drivers/media/platform/synopsys/dw-mipi-csi2rx.c
> @@ -24,15 +24,6 @@
> #include <media/v4l2-mc.h>
> #include <media/v4l2-subdev.h>
>
> -#define DW_MIPI_CSI2RX_N_LANES 0x04
> -#define DW_MIPI_CSI2RX_RESETN 0x10
> -#define DW_MIPI_CSI2RX_PHY_STATE 0x14
> -#define DW_MIPI_CSI2RX_ERR1 0x20
> -#define DW_MIPI_CSI2RX_ERR2 0x24
> -#define DW_MIPI_CSI2RX_MSK1 0x28
> -#define DW_MIPI_CSI2RX_MSK2 0x2c
> -#define DW_MIPI_CSI2RX_CONTROL 0x40
> -
> #define SW_CPHY_EN(x) ((x) << 0)
> #define SW_DSI_EN(x) ((x) << 4)
> #define SW_DATATYPE_FS(x) ((x) << 8)
> @@ -40,12 +31,33 @@
> #define SW_DATATYPE_LS(x) ((x) << 20)
> #define SW_DATATYPE_LE(x) ((x) << 26)
>
> +/* Help check wrong access unexisted register at difference IP version */
Maybe
/* helper for checking whether register exists */
? However, I think the naming speaks for itself and you can leave the
comment away.
> +#define DW_REG_EXIST BIT(31)
> +#define DW_REG(x) (DW_REG_EXIST | (x))
> +
> +enum dw_mipi_csi2rx_regs_index {
> + DW_MIPI_CSI2RX_N_LANES,
> + DW_MIPI_CSI2RX_RESETN,
> + DW_MIPI_CSI2RX_PHY_STATE,
> + DW_MIPI_CSI2RX_ERR1,
> + DW_MIPI_CSI2RX_ERR2,
> + DW_MIPI_CSI2RX_MSK1,
> + DW_MIPI_CSI2RX_MSK2,
> + DW_MIPI_CSI2RX_CONTROL,
> +
> + DW_MIPI_CSI2RX_MAX,
> +};
> +
> enum {
> DW_MIPI_CSI2RX_PAD_SINK,
> DW_MIPI_CSI2RX_PAD_SRC,
> DW_MIPI_CSI2RX_PAD_MAX,
> };
>
> +struct dw_mipi_csi2rx_drvdata {
> + const u32 *regs;
> +};
> +
> struct dw_mipi_csi2rx_format {
> u32 code;
> u8 depth;
> @@ -72,6 +84,23 @@ struct dw_mipi_csi2rx_device {
>
> enum v4l2_mbus_type bus_type;
> u32 lanes_num;
> +
> + const struct dw_mipi_csi2rx_drvdata *drvdata;
> +};
> +
> +static const u32 rk3568_regs[DW_MIPI_CSI2RX_MAX] = {
> + [DW_MIPI_CSI2RX_N_LANES] = DW_REG(0x4),
> + [DW_MIPI_CSI2RX_RESETN] = DW_REG(0x10),
> + [DW_MIPI_CSI2RX_PHY_STATE] = DW_REG(0x14),
> + [DW_MIPI_CSI2RX_ERR1] = DW_REG(0x20),
> + [DW_MIPI_CSI2RX_ERR2] = DW_REG(0x24),
> + [DW_MIPI_CSI2RX_MSK1] = DW_REG(0x28),
> + [DW_MIPI_CSI2RX_MSK2] = DW_REG(0x2c),
> + [DW_MIPI_CSI2RX_CONTROL] = DW_REG(0x40),
> +};
> +
> +static const struct dw_mipi_csi2rx_drvdata rk3568_drvdata = {
> + .regs = rk3568_regs,
> };
>
> static const struct v4l2_mbus_framefmt default_format = {
> @@ -186,16 +215,46 @@ static inline struct dw_mipi_csi2rx_device *to_csi2(struct v4l2_subdev *sd)
> return container_of(sd, struct dw_mipi_csi2rx_device, sd);
> }
>
> +static bool dw_mipi_csi2rx_is_exist(struct dw_mipi_csi2rx_device *csi2,
Sounds a bit weird to me, maybe "dw_mipi_csi2rx_has_reg(ister)" or
"dw_mipi_csi2rx_reg(ister)_exists?
> + enum dw_mipi_csi2rx_regs_index index)
> +{
> + if (index < DW_MIPI_CSI2RX_MAX &&
> + (csi2->drvdata->regs[index] & DW_REG_EXIST))
> + return true;
> +
> + return false;
> +}
> +
> +static void __iomem *
> +dw_mipi_csi2rx_get_regaddr(struct dw_mipi_csi2rx_device *csi2,
> + enum dw_mipi_csi2rx_regs_index index)
> +{
> + u32 off = (~DW_REG_EXIST) & csi2->drvdata->regs[index];
> +
> + return csi2->base_addr + off;
> +}
> +
> static inline void dw_mipi_csi2rx_write(struct dw_mipi_csi2rx_device *csi2,
> - unsigned int addr, u32 val)
> + enum dw_mipi_csi2rx_regs_index index,
> + u32 val)
> {
> - writel(val, csi2->base_addr + addr);
> + if (dw_mipi_csi2rx_is_exist(csi2, index))
> + writel(val, dw_mipi_csi2rx_get_regaddr(csi2, index));
> +
> + dev_err_once(csi2->dev,
> + "write to non-existent register index: %d\n", index);
Not sure about to control flow here, as the error message is shown in
any case. Do you mean
{
if (!dw_mipi_csi2rx_is_exist(csi2, index)) {
dev_err_once(csi2->dev,
"write to non-existent register index: %d\n",
index);
return;
}
writel(val, dw_mipi_csi2rx_get_regaddr(csi2, index));
}
?
> }
>
> static inline u32 dw_mipi_csi2rx_read(struct dw_mipi_csi2rx_device *csi2,
> - unsigned int addr)
> + enum dw_mipi_csi2rx_regs_index index)
> {
> - return readl(csi2->base_addr + addr);
> + if (dw_mipi_csi2rx_is_exist(csi2, index))
> + return readl(dw_mipi_csi2rx_get_regaddr(csi2, index));
Here it seems to be correct, but personally I'd prefer
{
if (!dw_mipi_csi2rx_is_exist(csi2, index)) {
// print error
return 0;
}
return readl(...);
}
Anyway, it should match the write method.
> +
> + dev_err_once(csi2->dev,
> + "read non-existent register index: %d\n", index);
> + /* Return 0 for unexisted registers */
/* return 0 for non-existent registers */
or leave away altogether.
> + return 0;
> }
>
> static const struct dw_mipi_csi2rx_format *
> @@ -631,6 +690,7 @@ static void dw_mipi_csi2rx_unregister(struct dw_mipi_csi2rx_device *csi2)
> static const struct of_device_id dw_mipi_csi2rx_of_match[] = {
> {
> .compatible = "rockchip,rk3568-mipi-csi2",
> + .data = &rk3568_drvdata,
> },
> {}
> };
> @@ -652,6 +712,11 @@ static int dw_mipi_csi2rx_probe(struct platform_device *pdev)
> if (IS_ERR(csi2->base_addr))
> return PTR_ERR(csi2->base_addr);
>
> + csi2->drvdata = device_get_match_data(dev);
> + if (!csi2->drvdata)
> + return dev_err_probe(dev, -EINVAL,
> + "failed to get driver data\n");
> +
> ret = devm_clk_bulk_get_all(dev, &csi2->clks);
> if (ret < 0)
> return dev_err_probe(dev, -ENODEV, "failed to get clocks\n");
>
With the comments above addressed,
Reviewed-by: Michael Riesch <michael.riesch at collabora.com>
Best regards,
Michael
More information about the Linux-rockchip
mailing list