Re: [PATCH 3/8] media: qcom: camss: Implement CSID streams API hw_ops for gen2

[Gjorgji Rosikopulos (Consultant)]

Hi Bryan,

Thanks for the review,

On 9/11/2026 1:46 PM, Bryan O'Donoghue wrote:
> On 11/09/2026 07:22, 
> Gjorgji.Rosikopulos.gjorgji.rosikopulos@xxxxxxxxxxxxxxxx wrote:
>> From: Gjorgji Rosikopulos <gjorgji.rosikopulos@xxxxxxxxxxxxxxxx>
>>
>> Implement the configure_rx/enable_stream/disable_stream hw_ops added to
>> struct csid_hw_ops, for the gen2 CSID hardware backend used by SM8250.
>>
>> __csid_configure_rdi_stream() now takes an explicit data type parameter
>> instead of deriving it from the CSID source pad's format, since the
>> streams API keys each stream by stream_id rather than by pad. The
>> existing configure_stream() path is updated to pass its per-port format
>> lookup through to this parameter, preserving current behavior.
>>
>> Signed-off-by: Gjorgji Rosikopulos <gjorgji.rosikopulos@xxxxxxxxxxxxxxxx>
>> ---
>>   .../platform/qcom/camss/camss-csid-gen2.c     | 59 ++++++++++++++++---
>>   1 file changed, 51 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/camss/camss-csid-gen2.c b/drivers/media/platform/qcom/camss/camss-csid-gen2.c
>> index eadcb2f7e3aa..2512cdb16d94 100644
>> --- a/drivers/media/platform/qcom/camss/camss-csid-gen2.c
>> +++ b/drivers/media/platform/qcom/camss/camss-csid-gen2.c
>> @@ -253,13 +253,8 @@ static void __csid_configure_testgen(struct csid_device *csid, u8 enable, u8 por
>>   	writel_relaxed(val, csid->base + CSID_TPG_CTRL);
>>   }
>>   
>> -static void __csid_configure_rdi_stream(struct csid_device *csid, u8 enable, u8 port, u8 vc)
>> +static void __csid_configure_rdi_stream(struct csid_device *csid, u8 enable, u8 port, u8 vc, u8 dt)
>>   {
>> -	/* Source pads matching RDI channels on hardware. Pad 1 -> RDI0, Pad 2 -> RDI1, etc. */
>> -	struct v4l2_mbus_framefmt *input_format = &csid->fmt[MSM_CSID_PAD_FIRST_SRC + port];
>> -	const struct csid_format_info *format = csid_get_fmt_entry(csid->res->formats->formats,
>> -								   csid->res->formats->nformats,
>> -								   input_format->code);
>>   	u32 val;
>>   
>>   	/*
>> @@ -281,7 +276,7 @@ static void __csid_configure_rdi_stream(struct csid_device *csid, u8 enable, u8
>>   	val |= 1 << RDI_CFG0_TIMESTAMP_EN;
>>   	/* note: for non-RDI path, this should be format->decode_format */
>>   	val |= DECODE_FORMAT_PAYLOAD_ONLY << RDI_CFG0_DECODE_FORMAT;
>> -	val |= format->data_type << RDI_CFG0_DATA_TYPE;
>> +	val |= dt << RDI_CFG0_DATA_TYPE;
>>   	val |= vc << RDI_CFG0_VIRTUAL_CHANNEL;
>>   	val |= dt_id << RDI_CFG0_DT_ID;
>>   	writel_relaxed(val, csid->base + CSID_RDI_CFG0(port));
>> @@ -330,15 +325,60 @@ static void csid_configure_stream(struct csid_device *csid, u8 enable)
>>   	/* Loop through all enabled ports and configure a stream for each */
>>   	for (i = 0; i < MSM_CSID_MAX_SRC_STREAMS; i++)
>>   		if (csid->phy.en_vc & BIT(i)) {
>> +			/* Source pads match RDI channels: pad 1 -> RDI0, pad 2 -> RDI1, etc. */
>> +			struct v4l2_mbus_framefmt *input_format =
>> +				&csid->fmt[MSM_CSID_PAD_FIRST_SRC + i];
>> +			const struct csid_format_info *format =
>> +				csid_get_fmt_entry(csid->res->formats->formats,
>> +						   csid->res->formats->nformats,
>> +						   input_format->code);
>> +			u8 vc = 0;
>> +
>>   			if (tg->enabled)
>>   				__csid_configure_testgen(csid, enable, i, 0);
>>   
>> -			__csid_configure_rdi_stream(csid, enable, i, 0);
>> +			__csid_configure_rdi_stream(csid, enable, i, vc, format->data_type);
>>   			__csid_configure_rx(csid, &csid->phy, 0);
>>   			__csid_ctrl_rdi(csid, enable, i);
>>   		}
>>   }
>>   
>> +/*
>> + * configure_rx - Configure the CSID Rx front-end
>> + */
>> +static void csid_configure_rx(struct csid_device *csid)
>> +{
>> +	__csid_configure_rx(csid, &csid->phy, 0);
>> +}
>> +
>> +/*
>> + * stream_id is used directly as the hardware RDI port index below. This
>> + * assumes a 1:1 stream-to-port mapping and should be revisited once
>> + * per-platform src_streams data (stream id -> hw pipe) is added.
>> + */
>> +static void csid_enable_stream(struct csid_device *csid, u32 stream_id, u8 vc, u8 dt)
>> +{
>> +	struct csid_testgen_config *tg = &csid->testgen;
>> +
>> +	if (tg->enabled)
>> +		__csid_configure_testgen(csid, 1, stream_id, vc);
>> +
>> +	__csid_configure_rdi_stream(csid, 1, stream_id, vc, dt);
>> +	__csid_ctrl_rdi(csid, 1, stream_id);
>> +}
>> +
>> +static void csid_disable_stream(struct csid_device *csid, u32 stream_id)
>> +{
>> +	struct csid_testgen_config *tg = &csid->testgen;
>> +
>> +	__csid_ctrl_rdi(csid, 0, stream_id);
>> +
>> +	if (tg->enabled)
>> +		__csid_configure_testgen(csid, 0, stream_id, 0);
>> +
>> +	__csid_configure_rdi_stream(csid, 0, stream_id, 0, 0);
>> +}
>> +
>>   static int csid_configure_testgen_pattern(struct csid_device *csid, s32 val)
>>   {
>>   	if (val > 0 && val <= csid->testgen.nmodes)
>> @@ -425,6 +465,9 @@ static void csid_subdev_init(struct csid_device *csid)
>>   const struct csid_hw_ops csid_ops_gen2 = {
>>   	.configure_stream = csid_configure_stream,
>>   	.configure_testgen_pattern = csid_configure_testgen_pattern,
>> +	.configure_rx = csid_configure_rx,
> 
> Keep the namespace consistent and include _stream somewhere in this name

I agree it will be added in next patchset.

~Gjorgji