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

[Mehdi Djait]

Hi Michael,

On Wed, Mar 25, 2026 at 09:04:57AM +0100, Michael Riesch wrote:
> 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@xxxxxxxxxxxxx>
> >>>>
> >>>> 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@xxxxxxxxxxxxx>
> >>>
> >>> [...]
> >>>
> >>>>  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.

That sounds good.

How about adding a TODO comment ?

--
Kind Regards
Mehdi Djait