[PATCH 4/6] media: synopsys: use struct dw_mipi_csi2rx_regs to describe register offsets

[Michael Riesch]

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:
> 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 at 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.

> +
>  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.


>  	},
>  	{}
>  };
> @@ -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