Re: [PATCH v11 10/17] media: rockchip: rkcif: add support for px30 vip dvp capture

[Michael Riesch <michael.riesch@collabora.com>]

Hi Sakari,

Thanks for the review.

On 10/7/25 22:12, Sakari Ailus wrote:
> [...]
>>  
>>  static const char *const rk3568_vicap_clks[] = {
>> @@ -62,11 +64,21 @@ MODULE_DEVICE_TABLE(of, rkcif_plat_of_match);
>>  
>>  static int rkcif_register(struct rkcif_device *rkcif)
>>  {
>> +	int ret;
>> +
>> +	ret = rkcif_dvp_register(rkcif);
>> +	if (ret && ret != -ENODEV)
>> +		goto err;
> 
> 	return ret == -ENODEV ? 0 : ret;

This is written in that way so that the patch 12/17 does not need to
modify recently added lines and simply needs to add new lines. OK with that?

> 
>> +
>>  	return 0;
>> +
>> +err:
>> +	return ret;
>>  }
>>  
>>  static void rkcif_unregister(struct rkcif_device *rkcif)
>>  {
>> +	rkcif_dvp_unregister(rkcif);
>>  }
>>  
>>  static int rkcif_notifier_bound(struct v4l2_async_notifier *notifier,
>> @@ -111,6 +123,9 @@ static irqreturn_t rkcif_isr(int irq, void *ctx)
>>  {
>>  	irqreturn_t ret = IRQ_NONE;
>>  
>> +	if (rkcif_dvp_isr(irq, ctx) == IRQ_HANDLED)
>> +		ret = IRQ_HANDLED;
>> +
>>  	return ret;
>>  }
>>  
>> diff --git a/drivers/media/platform/rockchip/rkcif/rkcif-regs.h b/drivers/media/platform/rockchip/rkcif/rkcif-regs.h
>> new file mode 100644
>> index 000000000000..d50b6e14b5af
>> --- /dev/null
>> +++ b/drivers/media/platform/rockchip/rkcif/rkcif-regs.h
>> @@ -0,0 +1,131 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Rockchip Camera Interface (CIF) Driver
>> + *
>> + * Copyright (C) 2018 Rockchip Electronics Co., Ltd.
>> + * Copyright (C) 2023 Mehdi Djait <mehdi.djait@bootlin.com>
>> + * Copyright (C) 2025 Michael Riesch <michael.riesch@wolfvision.net>
>> + */
>> +
>> +#ifndef _RKCIF_REGS_H
>> +#define _RKCIF_REGS_H
>> +
>> +#define RKCIF_REGISTER_NOTSUPPORTED	      0x420000
>> +#define RKCIF_FETCH_Y(VAL)		      ((VAL) & 0x1fff)
>> +#define RKCIF_XY_COORD(x, y)		       (((y) << 16) | (x))
> 
> Why the use of spaces for alignment to a column not divisible by 8? For a
> new header that doesn't seem to make sense.

Hm. I use clang-format to format my code and apparently it looks for the
longest define name (RKCIF_FORMAT_BT1120_CLOCK_DOUBLE_EDGES ex aequo
with others) and adds a space there. The other defines are aligned to
that one. I don't think this behavior of clang-format can be adjusted
(only option would be not to align anything, i.e.,

#define RKCIF_REGISTER_NOTSUPPORTED 0x420000
#define RKCIF_FETCH_Y(VAL) ((VAL) & 0x1fff)
#define RKCIF_XY_COORD(x, y) (((y) << 16) | (x))
...

If you don't find what clang-format produces appropriate, I'll change it
manually. Is there any strict rule? What should it look like in the end?
Align with tabs only to something divisible by 8?

Best regards,
Michael

> 
>> +
>> +/* DVP register contents */
>> +#define RKCIF_CTRL_ENABLE_CAPTURE	       BIT(0)
>> +#define RKCIF_CTRL_MODE_PINGPONG	       BIT(1)
>> +#define RKCIF_CTRL_MODE_LINELOOP	       BIT(2)
>> +#define RKCIF_CTRL_AXI_BURST_16		       (0xf << 12)
>> +
>> +#define RKCIF_INTEN_FRAME_END_EN	       BIT(0)
>> +#define RKCIF_INTEN_LINE_ERR_EN		       BIT(2)
>> +#define RKCIF_INTEN_BUS_ERR_EN		       BIT(6)
>> +#define RKCIF_INTEN_SCL_ERR_EN		       BIT(7)
>> +#define RKCIF_INTEN_PST_INF_FRAME_END_EN       BIT(9)
>> +
>> +#define RKCIF_INTSTAT_CLS		       0x3ff
>> +#define RKCIF_INTSTAT_FRAME_END		       BIT(0)
>> +#define RKCIF_INTSTAT_LINE_END		       BIT(1)
>> +#define RKCIF_INTSTAT_LINE_ERR		       BIT(2)
>> +#define RKCIF_INTSTAT_PIX_ERR		       BIT(3)
>> +#define RKCIF_INTSTAT_DFIFO_OF		       BIT(5)
>> +#define RKCIF_INTSTAT_BUS_ERR		       BIT(6)
>> +#define RKCIF_INTSTAT_PRE_INF_FRAME_END	       BIT(8)
>> +#define RKCIF_INTSTAT_PST_INF_FRAME_END	       BIT(9)
>> +#define RKCIF_INTSTAT_FRAME_END_CLR	       BIT(0)
>> +#define RKCIF_INTSTAT_LINE_END_CLR	       BIT(1)
>> +#define RKCIF_INTSTAT_LINE_ERR_CLR	       BIT(2)
>> +#define RKCIF_INTSTAT_PST_INF_FRAME_END_CLR    BIT(9)
>> +#define RKCIF_INTSTAT_ERR		       0xfc
>> +
>> +#define RKCIF_FRAME_STAT_CLS		       0x00
>> +#define RKCIF_FRAME_FRM0_STAT_CLS	       0x20
>> +
>> +#define RKCIF_FORMAT_VSY_HIGH_ACTIVE	       BIT(0)
>> +#define RKCIF_FORMAT_HSY_LOW_ACTIVE	       BIT(1)
>> +
>> +#define RKCIF_FORMAT_INPUT_MODE_YUV	       (0x00 << 2)
>> +#define RKCIF_FORMAT_INPUT_MODE_PAL	       (0x02 << 2)
>> +#define RKCIF_FORMAT_INPUT_MODE_NTSC	       (0x03 << 2)
>> +#define RKCIF_FORMAT_INPUT_MODE_BT1120	       (0x07 << 2)
>> +#define RKCIF_FORMAT_INPUT_MODE_RAW	       (0x04 << 2)
>> +#define RKCIF_FORMAT_INPUT_MODE_JPEG	       (0x05 << 2)
>> +#define RKCIF_FORMAT_INPUT_MODE_MIPI	       (0x06 << 2)
>> +
>> +#define RKCIF_FORMAT_YUV_INPUT_ORDER_UYVY      (0x00 << 5)
>> +#define RKCIF_FORMAT_YUV_INPUT_ORDER_YVYU      (0x01 << 5)
>> +#define RKCIF_FORMAT_YUV_INPUT_ORDER_VYUY      (0x02 << 5)
>> +#define RKCIF_FORMAT_YUV_INPUT_ORDER_YUYV      (0x03 << 5)
>> +#define RKCIF_FORMAT_YUV_INPUT_422	       (0x00 << 7)
>> +#define RKCIF_FORMAT_YUV_INPUT_420	       BIT(7)
>> +
>> +#define RKCIF_FORMAT_INPUT_420_ORDER_ODD       BIT(8)
>> +
>> +#define RKCIF_FORMAT_CCIR_INPUT_ORDER_EVEN     BIT(9)
>> +
>> +#define RKCIF_FORMAT_RAW_DATA_WIDTH_8	       (0x00 << 11)
>> +#define RKCIF_FORMAT_RAW_DATA_WIDTH_10	       (0x01 << 11)
>> +#define RKCIF_FORMAT_RAW_DATA_WIDTH_12	       (0x02 << 11)
>> +
>> +#define RKCIF_FORMAT_YUV_OUTPUT_422	       (0x00 << 16)
>> +#define RKCIF_FORMAT_YUV_OUTPUT_420	       BIT(16)
>> +
>> +#define RKCIF_FORMAT_OUTPUT_420_ORDER_EVEN     (0x00 << 17)
>> +#define RKCIF_FORMAT_OUTPUT_420_ORDER_ODD      BIT(17)
>> +
>> +#define RKCIF_FORMAT_RAWD_DATA_LITTLE_ENDIAN   (0x00 << 18)
>> +#define RKCIF_FORMAT_RAWD_DATA_BIG_ENDIAN      BIT(18)
>> +
>> +#define RKCIF_FORMAT_UV_STORAGE_ORDER_UVUV     (0x00 << 19)
>> +#define RKCIF_FORMAT_UV_STORAGE_ORDER_VUVU     BIT(19)
>> +
>> +#define RKCIF_FORMAT_BT1120_CLOCK_SINGLE_EDGES (0x00 << 24)
>> +#define RKCIF_FORMAT_BT1120_CLOCK_DOUBLE_EDGES BIT(24)
>> +#define RKCIF_FORMAT_BT1120_TRANSMIT_INTERFACE (0x00 << 25)
>> +#define RKCIF_FORMAT_BT1120_TRANSMIT_PROGRESS  BIT(25)
>> +#define RKCIF_FORMAT_BT1120_YC_SWAP	       BIT(26)
>> +
>> +#define RKCIF_SCL_CTRL_ENABLE_SCL_DOWN	       BIT(0)
>> +#define RKCIF_SCL_CTRL_ENABLE_SCL_UP	       BIT(1)
>> +#define RKCIF_SCL_CTRL_ENABLE_YUV_16BIT_BYPASS BIT(4)
>> +#define RKCIF_SCL_CTRL_ENABLE_RAW_16BIT_BYPASS BIT(5)
>> +#define RKCIF_SCL_CTRL_ENABLE_32BIT_BYPASS     BIT(6)
>> +#define RKCIF_SCL_CTRL_DISABLE_32BIT_BYPASS    (0x00 << 6)
>> +
>> +#define RKCIF_INTSTAT_F0_READY		       BIT(0)
>> +#define RKCIF_INTSTAT_F1_READY		       BIT(1)
>> +
>> +/* GRF register offsets and contents */
>> +#define RK3568_GRF_VI_CON0		       0x340
>> +#define RK3568_GRF_VI_CON1		       0x344
>> +#define RK3568_GRF_VI_STATUS0		       0x348
>> +
>> +#define RK3568_GRF_VI_CON1_CIF_DATAPATH	       BIT(9)
>> +#define RK3568_GRF_VI_CON1_CIF_CLK_DELAYNUM    GENMASK(6, 0)
>> +
>> +#define RK3568_GRF_WRITE_ENABLE(x)	       ((x) << 16)
>> +
>> +enum rkcif_dvp_register_index {
>> +	RKCIF_DVP_CTRL,
>> +	RKCIF_DVP_INTEN,
>> +	RKCIF_DVP_INTSTAT,
>> +	RKCIF_DVP_FOR,
>> +	RKCIF_DVP_LINE_NUM_ADDR,
>> +	RKCIF_DVP_FRM0_ADDR_Y,
>> +	RKCIF_DVP_FRM0_ADDR_UV,
>> +	RKCIF_DVP_FRM1_ADDR_Y,
>> +	RKCIF_DVP_FRM1_ADDR_UV,
>> +	RKCIF_DVP_VIR_LINE_WIDTH,
>> +	RKCIF_DVP_SET_SIZE,
>> +	RKCIF_DVP_SCL_CTRL,
>> +	RKCIF_DVP_CROP,
>> +	RKCIF_DVP_FRAME_STATUS,
>> +	RKCIF_DVP_LAST_LINE,
>> +	RKCIF_DVP_LAST_PIX,
>> +	RKCIF_DVP_REGISTER_MAX
>> +};
>> +
>> +#endif
>>
>