Re: [PATCH 4/6] media: synopsys: use struct dw_mipi_csi2rx_regs to describe register offsets
From: Michael Riesch <michael.riesch@collabora.com>
Date: 2026-02-13 10:02:11
Also in:
imx, linux-devicetree, linux-media, linux-rockchip, lkml
Hi Frank, Thanks for the patch. Please keep the order: - #includes - #defines - enum and struct definitions - the rest On 2/10/26 18:11, Frank Li wrote:
quoted hunk ↗ jump to hunk
Use struct dw_mipi_csi2rx_regs to describe register offsets and support new IP versions with differing register layouts. Add rk3568_regs, matching the previous macro definitions, and pass it as driver data retrieved during probe. No functional change. Signed-off-by: Frank Li <Frank.Li@nxp.com> --- drivers/media/platform/synopsys/dw-mipi-csi2rx.c | 96 +++++++++++++++++------- 1 file changed, 69 insertions(+), 27 deletions(-)diff --git a/drivers/media/platform/synopsys/dw-mipi-csi2rx.c b/drivers/media/platform/synopsys/dw-mipi-csi2rx.c index 4ad4e3b23448affeeaa932a706653818ba4019ba..6a2966c9e3a2eac661fa1f8610c9f021d6e26cf8 100644 --- a/drivers/media/platform/synopsys/dw-mipi-csi2rx.c +++ b/drivers/media/platform/synopsys/dw-mipi-csi2rx.c@@ -24,14 +24,39 @@ #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 +struct dw_mipi_csi2rx_regs { + u32 n_lanes; + u32 resetn; + u32 phy_state; + u32 err1; + u32 err2; + u32 msk1; + u32 msk2; + u32 control; +}; + +struct dw_mipi_csi2rx_drvdata { + const struct dw_mipi_csi2rx_regs *regs; +}; + +/* Help check wrong access unexisted register at difference IP version */ +#define DW_REG_EXIST BIT(31) +#define DW_REG(x) (DW_REG_EXIST | (x)) + +static const struct dw_mipi_csi2rx_regs rk3568_regs = { + .n_lanes = DW_REG(0x4), + .resetn = DW_REG(0x10), + .phy_state = DW_REG(0x14), + .err1 = DW_REG(0x20), + .err2 = DW_REG(0x24), + .msk1 = DW_REG(0x28), + .msk2 = DW_REG(0x2c), + .control = DW_REG(0x40), +}; + +static const struct dw_mipi_csi2rx_drvdata rk3568_drvdata = { + .regs = &rk3568_regs, +}; #define SW_CPHY_EN(x) ((x) << 0) #define SW_DSI_EN(x) ((x) << 4)@@ -74,8 +99,35 @@ struct dw_mipi_csi2rx_device { enum v4l2_mbus_type bus_type; u32 lanes_num; + + const struct dw_mipi_csi2rx_drvdata *drvdata; }; +static int +dw_mipi_csi2rx_reg_err(struct dw_mipi_csi2rx_device *csi2, const char *name) +{ + dev_err_once(csi2->dev, "access to non-existent register: %s\n", name); + return 0; +} + +#define __dw_reg_exist(offset) ((offset) & DW_REG_EXIST) + +#define dw_reg_exist(csi2, __name) __dw_reg_exist((csi2)->drvdata->regs->__name) + +#define dw_mipi_csi2rx_write(csi2, __name, value) \ +({ auto __csi2 = csi2; \ + u32 offset = __csi2->drvdata->regs->__name; \ + __dw_reg_exist(offset) ? \ + writel(value, __csi2->base_addr + (offset & ~DW_REG_EXIST)) : \ + dw_mipi_csi2rx_reg_err(__csi2, #__name); }) + +#define dw_mipi_csi2rx_read(csi2, __name) \ +({ auto __csi2 = csi2; \ + u32 offset = __csi2->drvdata->regs->__name; \ + __dw_reg_exist(offset) ? \ + readl(__csi2->base_addr + (offset & ~DW_REG_EXIST)) : \ + dw_mipi_csi2rx_reg_err(__csi2, #__name); })
Please use an
enum {
DW_MIPI_CSI2RX_N_LANES,
DW_MIPI_CSI2RX_RESETN,
...,
DW_MIPI_CSI2RX_MAX
};
a member regs[DW_MIPI_CSI2RX_MAX] in dw_mipi_csi2rx_drvdata, and adjust
the dw_mipi_csi2rx_{read,write} accordingly. This is cleaner IMHO and
integrates better with the existing code.
quoted hunk ↗ jump to hunk
+ static const struct v4l2_mbus_framefmt default_format = { .width = 3840, .height = 2160,@@ -188,18 +240,6 @@ static inline struct dw_mipi_csi2rx_device *to_csi2(struct v4l2_subdev *sd) return container_of(sd, struct dw_mipi_csi2rx_device, sd); } -static inline void dw_mipi_csi2rx_write(struct dw_mipi_csi2rx_device *csi2, - unsigned int addr, u32 val) -{ - writel(val, csi2->base_addr + addr); -} - -static inline u32 dw_mipi_csi2rx_read(struct dw_mipi_csi2rx_device *csi2, - unsigned int addr) -{ - return readl(csi2->base_addr + addr); -} - static const struct dw_mipi_csi2rx_format * dw_mipi_csi2rx_find_format(struct dw_mipi_csi2rx_device *csi2, u32 mbus_code) {@@ -265,9 +305,9 @@ static int dw_mipi_csi2rx_start(struct dw_mipi_csi2rx_device *csi2) control |= SW_DATATYPE_FS(0x00) | SW_DATATYPE_FE(0x01) | SW_DATATYPE_LS(0x02) | SW_DATATYPE_LE(0x03); - dw_mipi_csi2rx_write(csi2, DW_MIPI_CSI2RX_N_LANES, lanes - 1); - dw_mipi_csi2rx_write(csi2, DW_MIPI_CSI2RX_CONTROL, control); - dw_mipi_csi2rx_write(csi2, DW_MIPI_CSI2RX_RESETN, 1); + dw_mipi_csi2rx_write(csi2, n_lanes, lanes - 1); + dw_mipi_csi2rx_write(csi2, control, control); + dw_mipi_csi2rx_write(csi2, resetn, 1); return phy_power_on(csi2->phy); }@@ -276,9 +316,9 @@ static void dw_mipi_csi2rx_stop(struct dw_mipi_csi2rx_device *csi2) { phy_power_off(csi2->phy); - dw_mipi_csi2rx_write(csi2, DW_MIPI_CSI2RX_RESETN, 0); - dw_mipi_csi2rx_write(csi2, DW_MIPI_CSI2RX_MSK1, ~0); - dw_mipi_csi2rx_write(csi2, DW_MIPI_CSI2RX_MSK2, ~0); + dw_mipi_csi2rx_write(csi2, resetn, 0); + dw_mipi_csi2rx_write(csi2, msk1, ~0); + dw_mipi_csi2rx_write(csi2, msk2, ~0); } static const struct media_entity_operations dw_mipi_csi2rx_media_ops = {@@ -632,7 +672,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", + .compatible = "rockchip,rk3568-mipi-csi2", .data = &rk3568_drvdata,
.compatible = "rockchip,rk3568-mipi-csi2", .data = &rk3568_drvdata, for minimal diff and cleanliness. Please make sure that the last patch of this series is adjusted accordingly and continues this formatting.
quoted hunk ↗ jump to hunk
}, {} };@@ -654,6 +694,8 @@ 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);
Check for (!csi2->drvdata)
+ ret = devm_clk_bulk_get_all(dev, &csi2->clks); if (ret < 0) return dev_err_probe(dev, -ENODEV, "failed to get clocks\n");
Best regards, Michael