RE: [PATCH v7 3/9] media: chips-media: wave6: Add Wave6 VPU interface

[Nas Chung]

Hi, Frank.

>-----Original Message-----
>From: Frank Li <Frank.li@xxxxxxxxxxx>
>Sent: Thursday, September 10, 2026 4:50 AM
>To: Nas Chung <nas.chung@xxxxxxxxxxxxxxx>
>Cc: mchehab@xxxxxxxxxx; hverkuil@xxxxxxxxx; robh@xxxxxxxxxx;
>krzk+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx; shawnguo@xxxxxxxxxx;
>s.hauer@xxxxxxxxxxxxxx; linux-media@xxxxxxxxxxxxxxx;
>devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-imx@xxxxxxx;
>linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; jackson.lee
><jackson.lee@xxxxxxxxxxxxxxx>; lafley.kim <lafley.kim@xxxxxxxxxxxxxxx>;
>marek.vasut@xxxxxxxxxxx; Ming Qian <ming.qian@xxxxxxxxxxx>
>Subject: Re: [PATCH v7 3/9] media: chips-media: wave6: Add Wave6 VPU
>interface
>
>On Fri, Sep 04, 2026 at 03:46:29PM +0900, Nas Chung wrote:
>> Add an interface layer to manage hardware register configuration
>> and communication with the Chips&Media Wave6 video codec IP.
>>
>> The interface provides low-level helper functions used by the
>> Wave6 core driver to implement video encoding and decoding operations.
>> It handles command submission to the firmware via MMIO registers,
>> and waits for a response by polling the firmware busy flag.
>>
>> Signed-off-by: Nas Chung <nas.chung@xxxxxxxxxxxxxxx>
>
>Nit: your s-o-b is last one

OK.

>
>> Tested-by: Ming Qian <ming.qian@xxxxxxxxxxx>
>> Tested-by: Marek Vasut <marek.vasut@xxxxxxxxxxx>
>> ---
>...
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 7387a11facbe..e29018c2546b 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -29239,6 +29239,7 @@ M:	Jackson Lee <jackson.lee@xxxxxxxxxxxxxxx>
>>  L:	linux-media@xxxxxxxxxxxxxxx
>>  S:	Maintained
>>  F:	Documentation/devicetree/bindings/media/nxp,imx95-vpu.yaml
>> +F:	drivers/media/platform/chips-media/wave6/
>
>Consider nxp is one major user, can you add
>
>L: imx@xxxxxxxxxxxxxxx

Sure, I'll add it in v8.

>
>...
>> +/*
>> + * Wave6 series multi-standard codec IP - wave6 backend interface
>> + *
>> + * Copyright (C) 2025 CHIPS&MEDIA INC
>
>2026

I'll update the other files as well.

>
>> + */
>> +
>> +#include <linux/iopoll.h>
>
>Add space line here

OK.

>
>> +#include "wave6-vpu-core.h"
>> +#include "wave6-hw.h"
>> +#include "wave6-regdefine.h"
>> +#include "wave6-trace.h"
>> +
>> +void wave6_vpu_writel(struct vpu_core_device *core, u32 addr, u32 data)
>> +{
>> +	wave6_vdi_writel(core->reg_base, addr, data);
>> +	trace_wave6_vpu_writel(core->dev, addr, data);
>> +}
>> +
>> +u32 wave6_vpu_readl(struct vpu_core_device *core, u32 addr)
>> +{
>> +	u32 data;
>> +
>> +	data = wave6_vdi_readl(core->reg_base, addr);
>> +	trace_wave6_vpu_readl(core->dev, addr, data);
>> +
>> +	return data;
>> +}
>> +
>> +static void wave6_print_reg_err(struct vpu_core_device *core, u32
>fail_reason)
>> +{
>> +	void *caller = __builtin_return_address(0);
>> +	struct device *dev = core->dev;
>> +
>> +	switch (fail_reason) {
>> +	case WAVE6_SYSERR_QUEUEING_FAIL:
>> +		dev_dbg(dev, "%pS: queueing failure 0x%x\n", caller,
>fail_reason);
>
>why here is dev_dbg(), other is dev_err()

I'll change it to dev_err().

>
>> +		break;
>> +	case WAVE6_SYSERR_RESULT_NOT_READY:
>> +		dev_err(dev, "%pS: result not ready 0x%x\n", caller,
>fail_reason);
>> +		break;
>> +	case WAVE6_SYSERR_ACCESS_VIOLATION_HW:
>> +		dev_err(dev, "%pS: access violation 0x%x\n", caller,
>fail_reason);
>> +		break;
>> +	case WAVE6_SYSERR_WATCHDOG_TIMEOUT:
>> +		dev_err(dev, "%pS: watchdog timeout 0x%x\n", caller,
>fail_reason);
>> +		break;
>> +	case WAVE6_SYSERR_BUS_ERROR:
>> +		dev_err(dev, "%pS: bus error 0x%x\n", caller, fail_reason);
>> +		break;
>> +	case WAVE6_SYSERR_DOUBLE_FAULT:
>> +		dev_err(dev, "%pS: double fault 0x%x\n", caller, fail_reason);
>> +		break;
>> +	case WAVE6_SYSERR_VPU_STILL_RUNNING:
>> +		dev_err(dev, "%pS: still running 0x%x\n", caller,
>fail_reason);
>> +		break;
>> +	default:
>> +		dev_err(dev, "%pS: failure: 0x%x\n", caller, fail_reason);
>> +		break;
>> +	}
>> +}
>> +
>> +static void wave6_dec_set_display_buffer(struct vpu_instance *inst,
>struct frame_buffer fb)
>> +{
>> +	struct dec_info *p_dec_info = &inst->codec_info->dec_info;
>> +	int index;
>> +
>> +	for (index = 0; index < WAVE6_MAX_FBS; index++) {
>
>Now, Prefer
>
>	for (int index = 0; ....)

Agreed. I'll check for the same pattern elsewhere.

>
>> +		if (!p_dec_info->disp_buf[index].buf_y) {
>> +			p_dec_info->disp_buf[index] = fb;
>> +			p_dec_info->disp_buf[index].index = index;
>> +			break;
>> +		}
>> +	}
>> +}
>> +
>> +static struct frame_buffer wave6_dec_get_display_buffer(struct
>vpu_instance *inst,
>> +							dma_addr_t addr)
>> +{
>> +	struct dec_info *p_dec_info = &inst->codec_info->dec_info;
>> +	int i;
>> +	struct frame_buffer fb;
>
>struct frame_buffer fb = { .index = -1;};
>
>> +
>> +	for (i = 0; i < WAVE6_MAX_FBS; i++) {
>
>for (int i = 0; ..)
>
>> +		if (p_dec_info->disp_buf[i].buf_y == addr)
>> +			return p_dec_info->disp_buf[i];
>> +	}
>> +
>> +	memset(&fb, 0, sizeof(struct frame_buffer));
>
>needn't memset here if init at declear.

Agreed. I'll fix wave6_dec_get_display_buffer() in v8.

>
>> +	fb.index = -1;
>> +
>> +	return fb;
>> +}
>> +
>...
>> +
>> +static int wave6_send_query(struct vpu_core_device *core, u32 id, u32
>std,
>> +			    enum wave6_query_option query_opt)
>> +{
>> +	int ret;
>> +	u32 reg_val;
>
>try keep reverise Christmas tree order.

OK, I'll fix it.

>
>> +
>> +	lockdep_assert_held(&core->hw_lock);
>> +
>> +	vpu_write_reg(core, W6_QUERY_OPTION, query_opt);
>> +	wave6_send_command(core, id, std, W6_CMD_QUERY);
>> +
>> +	ret = wave6_wait_vpu_busy(core, W6_VPU_BUSY_STATUS);
>> +	if (ret) {
>> +		dev_err(core->dev, "query timed out opt=0x%x\n", query_opt);
>> +		return ret;
>> +	}
>> +
>> +	if (!vpu_read_reg(core, W6_RET_SUCCESS)) {
>> +		reg_val = vpu_read_reg(core, W6_RET_FAIL_REASON);
>> +		wave6_print_reg_err(core, reg_val);
>> +		return -EIO;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +int wave6_vpu_get_version(struct vpu_core_device *core)
>> +{
>> +	struct vpu_attr *attr = &core->attr;
>> +	int ret;
>> +	u32 std_def1, conf_feature;
>> +
>> +	lockdep_assert_held(&core->hw_lock);
>> +
>> +	ret = wave6_send_query(core, 0, 0, W6_QUERY_OPT_GET_VPU_INFO);
>> +	if (ret)
>> +		return ret;
>> +
>> +	attr->product_id = wave6_vpu_get_product_id(core);
>> +	attr->product_code = vpu_read_reg(core, W6_VPU_RET_PRODUCT_CODE);
>> +	attr->product_version = vpu_read_reg(core, W6_RET_PRODUCT_VERSION);
>> +	attr->fw_version = vpu_read_reg(core, W6_RET_FW_API_VERSION);
>> +	attr->fw_revision = vpu_read_reg(core, W6_RET_FW_VERSION);
>> +	attr->hw_version = vpu_read_reg(core, W6_RET_CONF_HW_VERSION);
>> +	std_def1 = vpu_read_reg(core, W6_RET_STD_DEF1);
>> +	conf_feature = vpu_read_reg(core, W6_RET_CONF_FEATURE);
>> +
>> +	attr->support_decoders = 0;
>> +	attr->support_encoders = 0;
>> +	attr->support_decoders |= STD_DEF1_HEVC_DEC(std_def1) << W_HEVC_DEC;
>
>This is depend on STD_DEF1_HEVC_DEC() is 1 bit field.
>I feel like below codes is easier to read
>
>	STD_DEF1_HEVC_DEC(std_def1) ? BIT(W_HEVC_DEC) : 0;

Agreed, I'll change it in v8.

>
>> +	attr->support_hevc10bit_dec =
>CONF_FEATURE_HEVC10BIT_DEC(conf_feature);
>> +	attr->support_decoders |= STD_DEF1_AVC_DEC(std_def1) << W_AVC_DEC;
>> +	attr->support_avc10bit_dec = CONF_FEATURE_AVC10BIT_DEC(conf_feature);
>> +	attr->support_encoders |= STD_DEF1_HEVC_ENC(std_def1) << W_HEVC_ENC;
>> +	attr->support_hevc10bit_enc =
>CONF_FEATURE_HEVC10BIT_ENC(conf_feature);
>> +	attr->support_encoders |= STD_DEF1_AVC_ENC(std_def1) << W_AVC_ENC;
>> +	attr->support_avc10bit_enc = CONF_FEATURE_AVC10BIT_ENC(conf_feature);
>> +
>> +	return 0;
>> +}
>...
>> +
>> +int wave6_vpu_dec_register_display_buffer(struct vpu_instance *inst,
>struct frame_buffer fb)
>> +{
>> +	int ret;
>> +	struct dec_info *p_dec_info;
>> +	u32 reg_val;
>> +	u32 c_fmt_idc, out_fmt, out_mode;
>> +
>> +	guard(mutex)(&inst->dev->hw_lock);
>> +
>> +	p_dec_info = &inst->codec_info->dec_info;
>> +
>> +	vpu_write_reg(inst->dev, W6_CMD_DEC_SET_DISP_SCL_PARAM,
>> +		      inst->scaler_info.enable);
>> +	reg_val = SET_DISP_SCL_PIC_SIZE_WIDTH(inst->scaler_info.width) |
>> +		  SET_DISP_SCL_PIC_SIZE_HEIGHT(inst->scaler_info.height);
>> +	vpu_write_reg(inst->dev, W6_CMD_DEC_SET_DISP_SCL_PIC_SIZE, reg_val);
>> +	reg_val = SET_DISP_PIC_SIZE_WIDTH(p_dec_info->seq_info.pic_width) |
>> +		  SET_DISP_PIC_SIZE_HEIGHT(p_dec_info->seq_info.pic_height);
>> +	vpu_write_reg(inst->dev, W6_CMD_DEC_SET_DISP_PIC_SIZE, reg_val);
>> +
>> +	c_fmt_idc = get_chroma_format_idc(p_dec_info->wtl_format);
>> +	switch (p_dec_info->wtl_format) {
>> +	case FORMAT_420_P10_16BIT_MSB:
>> +	case FORMAT_422_P10_16BIT_MSB:
>> +	case FORMAT_444_P10_16BIT_MSB:
>> +	case FORMAT_400_P10_16BIT_MSB:
>> +		out_mode = (WTL_RIGHT_JUSTIFIED << 2) | WTL_PIXEL_16BIT;
>> +		break;
>> +	case FORMAT_420_P10_16BIT_LSB:
>> +	case FORMAT_422_P10_16BIT_LSB:
>> +	case FORMAT_444_P10_16BIT_LSB:
>> +	case FORMAT_400_P10_16BIT_LSB:
>> +		out_mode = (WTL_LEFT_JUSTIFIED << 2) | WTL_PIXEL_16BIT;
>> +		break;
>> +	case FORMAT_420_P10_32BIT_MSB:
>> +	case FORMAT_422_P10_32BIT_MSB:
>> +	case FORMAT_444_P10_32BIT_MSB:
>> +	case FORMAT_400_P10_32BIT_MSB:
>> +		out_mode = (WTL_RIGHT_JUSTIFIED << 2) | WTL_PIXEL_32BIT;
>> +		break;
>> +	case FORMAT_420_P10_32BIT_LSB:
>> +	case FORMAT_422_P10_32BIT_LSB:
>> +	case FORMAT_444_P10_32BIT_LSB:
>> +	case FORMAT_400_P10_32BIT_LSB:
>> +		out_mode = (WTL_LEFT_JUSTIFIED << 2) | WTL_PIXEL_32BIT;
>> +		break;
>> +	default:
>> +		out_mode = (WTL_RIGHT_JUSTIFIED << 2) | WTL_PIXEL_8BIT;
>> +		break;
>> +	}
>> +	out_fmt = (inst->nv21 << 1) | inst->cbcr_interleave;
>
>Can you use macro for 1 and 2. look like it fill into
>SET_DISP_COMMON_PIC_INFO_OUT_FMT()

Agreed.

>
>Seem you need more detail out_fmt for WTL_PIXEL_16BIT/WTL_PIXEL_32BIT/
>WTL_PIXEL_8BIT and *JUSTIFIED. should use FIELD_PREP() macro for these
>settings.

OK, I'll use FIELD_PREP() for out_mode.

>
>> +
>> +	reg_val = SET_DISP_COMMON_PIC_INFO_BWB_ON |
>> +		  SET_DISP_COMMON_PIC_INFO_C_FMT_IDC(c_fmt_idc) |
>> +		  SET_DISP_COMMON_PIC_INFO_PIXEL_ORDER(PIXEL_ORDER_INCREASING)
>|
>> +		  SET_DISP_COMMON_PIC_INFO_OUT_MODE(out_mode) |
>> +		  SET_DISP_COMMON_PIC_INFO_OUT_FMT(out_fmt) |
>> +		  SET_DISP_COMMON_PIC_INFO_STRIDE(fb.stride);
>> +	vpu_write_reg(inst->dev, W6_CMD_DEC_SET_DISP_COMMON_PIC_INFO,
>reg_val);
>> +	reg_val = SET_DISP_OPTION_ENDIAN(VDI_128BIT_BIG_ENDIAN);
>> +	vpu_write_reg(inst->dev, W6_CMD_DEC_SET_DISP_OPTION, reg_val);
>> +	reg_val = SET_DISP_PIC_INFO_L_BIT_DEPTH(fb.luma_bit_depth) |
>> +		  SET_DISP_PIC_INFO_C_BIT_DEPTH(fb.chroma_bit_depth) |
>> +		  SET_DISP_PIC_INFO_C_FMT_IDC(fb.c_fmt_idc);
>> +	vpu_write_reg(inst->dev, W6_CMD_DEC_SET_DISP_PIC_INFO, reg_val);
>> +	vpu_write_reg(inst->dev, W6_CMD_DEC_SET_DISP_Y_BASE, fb.buf_y);
>> +	vpu_write_reg(inst->dev, W6_CMD_DEC_SET_DISP_CB_BASE, fb.buf_cb);
>> +	vpu_write_reg(inst->dev, W6_CMD_DEC_SET_DISP_CR_BASE, fb.buf_cr);
>> +
>> +	wave6_send_command(inst->dev, inst->id, inst->std,
>W6_CMD_DEC_SET_DISP);
>> +	ret = wave6_wait_vpu_busy(inst->dev, W6_VPU_BUSY_STATUS);
>> +	if (ret) {
>> +		dev_err(inst->dev->dev, "%s: timeout\n", __func__);
>> +		return ret;
>> +	}
>> +
>> +	if (!vpu_read_reg(inst->dev, W6_RET_SUCCESS))
>> +		return -EIO;
>> +
>> +	wave6_dec_set_display_buffer(inst, fb);
>> +
>> +	return 0;
>> +}
>> +
>...
>> +
>> +int wave6_vpu_dec_get_output_info(struct vpu_instance *inst, struct
>dec_output_info *info)
>> +{
>> +	struct dec_info *p_dec_info;
>> +	u32 reg_val, i;
>> +	int decoded_idx = -1, disp_idx = -1;
>> +	int ret;
>> +
>> +	if (WARN_ON(!info))
>> +		return -EINVAL;
>> +
>> +	guard(mutex)(&inst->dev->hw_lock);
>> +
>> +	p_dec_info = &inst->codec_info->dec_info;
>> +
>> +	ret = wave6_send_query(inst->dev, inst->id, inst->std,
>W6_QUERY_OPT_GET_RESULT);
>> +	if (ret) {
>> +		info->rd_ptr = p_dec_info->stream_rd_ptr;
>> +		info->wr_ptr = p_dec_info->stream_wr_ptr;
>> +		return ret;
>> +	}
>> +
>> +	info->decoding_success = vpu_read_reg(inst->dev,
>W6_RET_DEC_DECODING_SUCCESS);
>> +	if (!info->decoding_success)
>> +		info->error_reason = vpu_read_reg(inst->dev,
>W6_RET_DEC_ERR_INFO);
>> +	else
>> +		info->warn_info = vpu_read_reg(inst->dev,
>W6_RET_DEC_WARN_INFO);
>> +
>> +	reg_val = vpu_read_reg(inst->dev, W6_RET_DEC_PIC_TYPE);
>> +	info->ctu_size = DEC_PIC_TYPE_CTU_SIZE(reg_val);
>> +	info->nal_type = DEC_PIC_TYPE_NAL_UNIT_TYPE(reg_val);
>> +
>> +	if (reg_val & DEC_PIC_TYPE_B)
>> +		info->pic_type = PIC_TYPE_B;
>> +	else if (reg_val & DEC_PIC_TYPE_P)
>> +		info->pic_type = PIC_TYPE_P;
>> +	else if (reg_val & DEC_PIC_TYPE_I)
>> +		info->pic_type = PIC_TYPE_I;
>> +	else
>> +		info->pic_type = PIC_TYPE_MAX;
>> +	if (inst->std == W_HEVC_DEC) {
>> +		if (info->pic_type == PIC_TYPE_I &&
>> +		    (info->nal_type == H265_NAL_UNIT_TYPE_IDR_W_RADL ||
>> +		     info->nal_type == H265_NAL_UNIT_TYPE_IDR_N_LP))
>> +			info->pic_type = PIC_TYPE_IDR;
>> +	} else if (inst->std == W_AVC_DEC) {
>> +		if (info->pic_type == PIC_TYPE_I &&
>> +		    info->nal_type == H264_NAL_UNIT_TYPE_IDR_PICTURE)
>> +			info->pic_type = PIC_TYPE_IDR;
>> +	}
>> +
>> +	reg_val = vpu_read_reg(inst->dev, W6_RET_DEC_DECODED_FLAG);
>> +	if (reg_val) {
>> +		struct frame_buffer fb;
>> +		dma_addr_t addr = vpu_read_reg(inst->dev,
>W6_RET_DEC_DECODED_ADDR);
>> +
>> +		fb = wave6_dec_get_display_buffer(inst, addr);
>> +		info->frame_decoded_addr = addr;
>> +		info->frame_decoded = true;
>> +		decoded_idx = fb.index;
>> +	}
>> +
>> +	reg_val = vpu_read_reg(inst->dev, W6_RET_DEC_DISPLAY_FLAG);
>> +	if (reg_val) {
>> +		struct frame_buffer fb;
>> +		dma_addr_t addr = vpu_read_reg(inst->dev,
>W6_RET_DEC_DISPLAY_ADDR);
>> +
>> +		fb = wave6_dec_get_display_buffer(inst, addr);
>> +		info->frame_display_addr = addr;
>> +		info->frame_display = true;
>> +		disp_idx = fb.index;
>> +	}
>> +
>> +	reg_val = vpu_read_reg(inst->dev, W6_RET_DEC_DISP_IDC);
>> +	for (i = 0; i < WAVE6_MAX_FBS; i++) {
>> +		if (reg_val & (1 << i)) {
>
>for_each_set_bit()

OK.

>
>> +			dma_addr_t addr;
>> +
>> +			addr = vpu_read_reg(inst->dev,
>W6_RET_DEC_DISP_LINEAR_ADDR(i));
>> +
>> +			info->disp_frame_addr[info->disp_frame_num] = addr;
>> +			info->disp_frame_num++;
>> +		}
>> +	}
>> +
>> +	reg_val = vpu_read_reg(inst->dev, W6_RET_DEC_RELEASE_IDC);
>> +	for (i = 0; i < WAVE6_MAX_FBS; i++) {
>
>ditto, check other similar logic.

OK.

>
>> +		if (reg_val & (1 << i)) {
>> +			dma_addr_t addr;
>> +
>> +			addr = vpu_read_reg(inst->dev,
>W6_RET_DEC_DISP_LINEAR_ADDR(i));
>> +
>> +			wave6_dec_remove_display_buffer(inst, addr);
>> +			info->release_disp_frame_addr[info-
>>release_disp_frame_num] = addr;
>> +			info->release_disp_frame_num++;
>> +		}
>> +	}
>> +
>...
>> +
>> +int wave6_vpu_enc_start_one_frame(struct vpu_instance *inst, struct
>enc_param *param,
>> +				  u32 *fail_res)
>> +{
>> +	struct enc_cmd_enc_pic_reg reg;
>
>struct enc_cmd_enc_pic_reg reg = {};
>
>move set 0 out of mutex lock, slice better.

OK, I'll fix this in v8.

>
>> +	struct enc_info *p_enc_info;
>> +	int ret;
>> +
>> +	guard(mutex)(&inst->dev->hw_lock);
>> +
>> +	p_enc_info = &inst->codec_info->enc_info;
>> +
>> +	memset(&reg, 0, sizeof(struct enc_cmd_enc_pic_reg));
>> +
>> +	wave6_gen_enc_pic_reg(p_enc_info, inst->cbcr_interleave,
>> +			      inst->nv21, param, &reg);
>> +
>> +	vpu_write_reg(inst->dev, W6_CMD_ENC_PIC_BS_START, reg.bs_start);
>> +	vpu_write_reg(inst->dev, W6_CMD_ENC_PIC_BS_SIZE, reg.bs_size);
>> +	vpu_write_reg(inst->dev, W6_CMD_ENC_PIC_BS_OPTION, reg.bs_option);
>> +	vpu_write_reg(inst->dev, W6_CMD_ENC_PIC_SEC_AXI, reg.sec_axi);
>> +	vpu_write_reg(inst->dev, W6_CMD_ENC_PIC_REPORT, reg.report);
>> +	vpu_write_reg(inst->dev, W6_CMD_ENC_PIC_MV_HISTO0, reg.mv_histo0);
>> +	vpu_write_reg(inst->dev, W6_CMD_ENC_PIC_MV_HISTO1, reg.mv_histo1);
>> +	vpu_write_reg(inst->dev, W6_CMD_ENC_PIC_CUSTOM_MAP_PARAM,
>reg.custom_map_param);
>> +	vpu_write_reg(inst->dev, W6_CMD_ENC_PIC_CUSTOM_MAP_ADDR,
>reg.custom_map_addr);
>> +	vpu_write_reg(inst->dev, W6_CMD_ENC_PIC_SRC_PIC_IDX,
>reg.src_pic_idx);
>> +	vpu_write_reg(inst->dev, W6_CMD_ENC_PIC_SRC_ADDR_Y, reg.src_addr_y);
>> +	vpu_write_reg(inst->dev, W6_CMD_ENC_PIC_SRC_ADDR_U, reg.src_addr_u);
>> +	vpu_write_reg(inst->dev, W6_CMD_ENC_PIC_SRC_ADDR_V, reg.src_addr_v);
>> +	vpu_write_reg(inst->dev, W6_CMD_ENC_PIC_SRC_STRIDE, reg.src_stride);
>> +	vpu_write_reg(inst->dev, W6_CMD_ENC_PIC_SRC_FMT, reg.src_fmt);
>> +	vpu_write_reg(inst->dev, W6_CMD_ENC_PIC_SRC_AXI_SEL,
>reg.src_axi_sel);
>> +	vpu_write_reg(inst->dev, W6_CMD_ENC_PIC_CODE_OPTION,
>reg.code_option);
>> +	vpu_write_reg(inst->dev, W6_CMD_ENC_PIC_PARAM, reg.param);
>> +	vpu_write_reg(inst->dev, W6_CMD_ENC_PIC_LONGTERM_PIC,
>reg.longterm_pic);
>> +	vpu_write_reg(inst->dev, W6_CMD_ENC_PIC_PREFIX_SEI_NAL_ADDR,
>reg.prefix_sei_nal_addr);
>> +	vpu_write_reg(inst->dev, W6_CMD_ENC_PIC_PREFIX_SEI_INFO,
>reg.prefix_sei_info);
>> +	vpu_write_reg(inst->dev, W6_CMD_ENC_PIC_SUFFIX_SEI_NAL_ADDR,
>reg.suffix_sei_nal_addr);
>> +	vpu_write_reg(inst->dev, W6_CMD_ENC_PIC_SUFFIX_SEI_INFO,
>reg.suffix_sei_info);
>> +	vpu_write_reg(inst->dev, W6_CMD_ENC_PIC_TIMESTAMP_LOW,
>reg.timestamp_low);
>> +	vpu_write_reg(inst->dev, W6_CMD_ENC_PIC_TIMESTAMP_HIGH,
>reg.timestamp_high);
>> +	vpu_write_reg(inst->dev, W6_CMD_ENC_PIC_CSC_COEFF0,
>reg.csc_coeff[0]);
>> +	vpu_write_reg(inst->dev, W6_CMD_ENC_PIC_CSC_COEFF1,
>reg.csc_coeff[1]);
>> +	vpu_write_reg(inst->dev, W6_CMD_ENC_PIC_CSC_COEFF2,
>reg.csc_coeff[2]);
>> +	vpu_write_reg(inst->dev, W6_CMD_ENC_PIC_CSC_COEFF3,
>reg.csc_coeff[3]);
>> +
>> +	wave6_send_command(inst->dev, inst->id, inst->std, W6_CMD_ENC_PIC);
>> +	ret = wave6_wait_vpu_busy(inst->dev, W6_VPU_BUSY_STATUS);
>> +	if (ret) {
>> +		dev_err(inst->dev->dev, "%s: timeout\n", __func__);
>> +		return -ETIMEDOUT;
>> +	}
>
>I not sure how heavy register read() write(),  consider use
>readl(writel)_relex(), if need write/read many registers(). Only last one
>need writel() before trigger DMA.

This should be worth it, especially in the multi-instance case.
I'll do this in v8 if I see no regression.

>
>> +
>> +	if (!vpu_read_reg(inst->dev, W6_RET_SUCCESS)) {
>> +		*fail_res = vpu_read_reg(inst->dev, W6_RET_FAIL_REASON);
>> +		wave6_print_reg_err(inst->dev, *fail_res);
>> +		return -EIO;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>...
>> +#endif /* __WAVE6_HW_H__ */
>> diff --git a/drivers/media/platform/chips-media/wave6/wave6-regdefine.h
>b/drivers/media/platform/chips-media/wave6/wave6-regdefine.h
>> new file mode 100644
>> index 000000000000..1d495145bbbd
>> --- /dev/null
>> +++ b/drivers/media/platform/chips-media/wave6/wave6-regdefine.h
>> @@ -0,0 +1,649 @@
>> +/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) */
>> +/*
>> + * Wave6 series multi-standard codec IP - wave6 register definitions
>> + *
>> + * Copyright (C) 2025 CHIPS&MEDIA INC
>> + */
>
>2026

Same as above.

>
>...
>> +
>> +#define W6_MAX_PIC_STRIDE		(4096U * 4)
>> +#define W6_PIC_STRIDE_ALIGNMENT		32
>> +#define W6_FBC_BUF_ALIGNMENT		32
>> +#define W6_DEC_BUF_ALIGNMENT		32
>> +#define W6_DEF_DEC_PIC_WIDTH		720U
>> +#define W6_DEF_DEC_PIC_HEIGHT		480U
>> +#define W6_MIN_DEC_PIC_WIDTH		64U
>> +#define W6_MIN_DEC_PIC_HEIGHT		64U
>> +#define W6_MAX_DEC_PIC_WIDTH		4096U
>> +#define W6_MAX_DEC_PIC_HEIGHT		4096U
>> +#define W6_DEC_PIC_SIZE_STEP		1
>> +
>> +#define W6_DEF_ENC_PIC_WIDTH		416U
>> +#define W6_DEF_ENC_PIC_HEIGHT		240U
>> +#define W6_MIN_ENC_PIC_WIDTH		256U
>> +#define W6_MIN_ENC_PIC_HEIGHT		128U
>> +#define W6_MAX_ENC_PIC_WIDTH		4096U
>> +#define W6_MAX_ENC_PIC_HEIGHT		4096U
>
>needn't U

OK.

>
>> +#define W6_ENC_PIC_SIZE_STEP		8
>> +#define W6_ENC_CROP_X_POS_STEP		32
>> +#define W6_ENC_CROP_Y_POS_STEP		2
>> +#define W6_ENC_CROP_STEP		2
>> +
>> +#define W6_VPU_POLL_DELAY_US		10
>> +#define W6_VPU_POLL_TIMEOUT		300000
>> +#define W6_BOOT_WAIT_TIMEOUT		10000
>> +#define W6_VPU_TIMEOUT			6000
>> +#define W6_VPU_TIMEOUT_CYCLE_COUNT	(8000000 * 4 * 4)
>> +
>...
>> +#define __WAVE6_VPUERROR_H__
>> +
>> +/* WAVE6 COMMON SYSTEM ERROR (FAIL_REASON) */
>> +#define WAVE6_SYSERR_QUEUEING_FAIL			0x00000001
>> +#define WAVE6_SYSERR_DECODER_FUSE			0x00000002
>> +#define WAVE6_SYSERR_INSTRUCTION_ACCESS_VIOLATION	0x00000004
>> +#define WAVE6_SYSERR_PRIVILEGE_VIOLATION		0x00000008
>> +#define WAVE6_SYSERR_DATA_ADDR_ALIGNMENT		0x00000010
>> +#define WAVE6_SYSERR_DATA_ACCESS_VIOLATION		0x00000020
>> +#define WAVE6_SYSERR_ACCESS_VIOLATION_HW		0x00000040
>> +#define WAVE6_SYSERR_INSTRUCTION_ADDR_ALIGNMENT		0x00000080
>> +#define WAVE6_SYSERR_UNKNOWN				0x00000100
>> +#define WAVE6_SYSERR_BUS_ERROR				0x00000200
>> +#define WAVE6_SYSERR_DOUBLE_FAULT			0x00000400
>> +#define WAVE6_SYSERR_RESULT_NOT_READY			0x00000800
>> +#define WAVE6_SYSERR_VPU_STILL_RUNNING			0x00001000
>> +#define WAVE6_SYSERR_UNKNOWN_CMD			0x00002000
>> +#define WAVE6_SYSERR_UNKNOWN_CODEC_STD			0x00004000
>> +#define WAVE6_SYSERR_UNKNOWN_QUERY_OPTION		0x00008000
>> +#define WAVE6_SYSERR_WATCHDOG_TIMEOUT			0x00020000
>> +#define WAVE6_SYSERR_NOT_SUPPORT			0x00100000
>> +#define WAVE6_SYSERR_TEMP_SEC_BUF_OVERFLOW		0x00200000
>> +#define WAVE6_SYSERR_NOT_SUPPORT_PROFILE		0x00400000
>> +#define WAVE6_SYSERR_TIMEOUT_CODEC_FW			0x40000000
>
>Suppose you should get check_patch warning, to prefer use BIT(n) for this
>defination.

OK. I'll use BIT(n) where the value is a single bit.

Thanks.
Nas.

>
>Frank