Re: [PATCH v2 3/9] media: rockchip: rkcif: add support for rk3588 vicap mipi capture

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

Hi Mehdi,

On 3/17/26 14:28, Mehdi Djait wrote:
> Hi Michael,
> 
> On Tue, Mar 17, 2026 at 02:21:20PM +0100, Michael Riesch wrote:
>> Hi Mehdi,
>>
>> On 3/17/26 14:08, Mehdi Djait wrote:
>>> Hi Michael,
>>>
>>> Thank you for this nice patch!
>>>
>>> On Tue, Mar 17, 2026 at 10:32:21AM +0100, Michael Riesch via B4 Relay wrote:
>>>> From: Michael Riesch <michael.riesch@collabora.com>
>>>>
>>>> The RK3588 Video Capture (VICAP) unit features a Digital Video Port
>>>> (DVP) and six MIPI CSI-2 capture interfaces. Add initial support
>>>> for this variant to the rkcif driver and enable the MIPI CSI-2
>>>> capture interfaces.
>>>>
>>>> Signed-off-by: Michael Riesch <michael.riesch@collabora.com>
>>>
>>> [...]
>>>
>>>>  static inline unsigned int rkcif_mipi_get_reg(struct rkcif_interface *interface,
>>>>  					      unsigned int index)
>>>>  {
>>>> @@ -631,6 +765,8 @@ static int rkcif_mipi_start_streaming(struct rkcif_stream *stream)
>>>>  	rkcif_mipi_stream_write(stream, RKCIF_MIPI_CTRL1, ctrl1);
>>>>  	rkcif_mipi_stream_write(stream, RKCIF_MIPI_CTRL0, ctrl0);
>>>>  
>>>> +	rkcif_mipi_write(interface, RKCIF_MIPI_CTRL, RKCIF_MIPI_CTRL_CAP_EN);
>>>> +
>>>
>>> while this is the correct solution for rk3588, for the rk3568 vicap this
>>> will write 0x1 to the VICAP_MIPI_CTRL : 0x00A0 which will enable the water line.
>>
>> nice catch ;-) However, the TRM (at least my version) claims that this
>> bit has a reset value of 0x1, so the bit in question should be already
>> set in the first place. Thus I decided to *not* make variant specific
>> code paths.
> 
> Yes, the reset value is indeed 0x1
> 
>>
>> Do you see problems in your setup?
>>
> 
> No problems, it works as expected, I was just confused to see the
> mipi capture enable added with this rk3588 patch and not before.
> 
> I just find it a bit confusing but if a nicer solution is too much
> hassle we can leave it like this.

I agree that this is (while correct) not the nicest way. I am still
bringing up the remaining features of the RK3588 VICAP (MUX + TOISP +
SCALER) and it looks like I need to do some refactoring anyway to
support them. When I do that, I shall rewrite this part. For the time
being it would be great to have this merged in order to provide initial
mainline support for this unit.

Best regards,
Michael