Re: [PATCH v8 0/3] media: rockchip: Add a driver for Rockchip's camera interface

[Mehdi Djait]

Hi Paul, Hi Michael

On Wed, Oct 25, 2023 at 11:54:27AM +0200, Paul Kocialkowski wrote:
> Michael,
> 
> On Wed 25 Oct 23, 11:17, Michael Riesch wrote:
> > Hi Paul,
> > 
> > On 10/25/23 10:43, Paul Kocialkowski wrote:
> > > [...]
> > >>> Here is the detail of my research on the concerned chips. The + at the beginning
> > >>> of the line indicate support in Rockchip's 4.4 tree:
> > >>>
> > >>> - RK3566/RK3568 (2020): CIF pins + VICAP terminology
> > >>> + RK1808 (2019): CIF pins + VIP registers + VIP_MIPI registers
> > >>> + PX30 (2017): VIP pins + VIP registers
> > >>> + RK3328 (2017): CIF pins + VIP terminology
> > >>> - RK3326 (2017): CIF pins + VIP terminology
> > >>> - RK3399 (2016): CIF pins
> > >>> - RK3368 (2015): CIF pins
> > >>> - PX2 (2014-11): CIF pins + CIF registers
> > >>> + RK3126/RK3128 (2014-10): CIF pins + registers
> > >>> + RK3288 (2014-05): CIF pins + VIP terminology
> > >>> - RK3026 (2013): CIF pins + CIF registers
> > >>> - RK3168/RK3188/PX3 (2012): CIF pins + CIF registers
> > >>> - RK3066 (2012): CIF pins + CIF registers
> > >>>
> > >>> Note that there are a few variations over time (added/removed registers), but
> > >>> the offsets of crucial registers are always the same, so we can safely
> > >>> assume this is the same unit in different generations.
> > >>>
> > >>> Since the RK3066 is the first model starting the RK30 lineup I think we can
> > >>> safely use that for the "base" compatible to be used for e.g. the bindings
> > >>> document, instead of px30 which is just one of the many SoCs that use this unit.
> > >>
> > >> Once the name of the driver is defined and adjusted in v9, I can try to
> > >> give the series a shot on my RK3568 board. First attempts to do so

This sounds good!

> > >> basing on Maxime's v5 showed that with a few modifications the DVP
> > >> feature works fine. In a subsequent step, we could discuss the inclusion
> > >> of the MIPI CSI-2 things in order to keep the driver sufficiently general.
> > > 
> > > Nice! I guess there will be a need to introduce a variant structure associated
> > > to each compatible to express the differences betweens these different
> > > generations.
> > 
> > Indeed. If Mehdi and you suggest something, I'd be happy to review.
> 
> Well the be honest the scope of work on our side is really centered on PX30
> and merging this first version.
> 
> > Otherwise, I'll try to come up with something reasonable. IMHO it would
> > make sense (as a first step) to have the clocks and the resets in this
> > structure, as well as a sub-structure that describes the DVP. The latter
> > consists of registers mainly, but maybe supported input/output formats
> > and other things should go in there as well. Also, downstream code has a
> > significant number of
> >     if (some condition including chip_id) A; else B;
> > things that we should probably get rid of with this variant structure.
> 
> Indeed I think we want to try avoid that. Another common option is to define
> capability flags to represent differences between generations in a more
> practical and clean way than explicitly checking chip ids or so.
> 
> > As next step, a sub-structure for MIPI CSI-2 could be defined. RK356X
> > will have one of those, RK3588 will feature even six of them. So we
> > should add a const array to the variant structure.
> > 
> > > Note that we will also probably need to convert the driver over to a MC-centric
> > > approach, but this is of course outside of the scope of this series.
> > 
> > That would absolutely make sense. What is missing, though? (I was
> > wondering that the driver calls media_device_(un)register but no
> > /dev/mediaX device pops up.)
> 
> Switching from video node-centric to MC-centric is more of a semantic change.
> In the first case we expect that subdevs are configured by the video device
> driver and userspace is not expected to change anything in the media topology
> or to configure media entities explicitly.
> 
> In the latter case it's the opposite : the driver should never try to push
> configuration to a subdev and should instead validate that the current
> configuration makes sense.
> 
> Still, I believe should be a media device registered and visible to userspace.
> Mehdi could you take a look at this? Do you see a media device in `media-ctl -p`
> and /dev/mediaX?

Yes I do have a media device

media-ctl -p
Media device information
------------------------
driver          rockchip-cif
model           rk_cif
serial          
bus info        platform:ff490000.video-capture
hw revision     0x0
driver version  6.6.0

Device topology
- entity 1: rockchip_cif (1 pad, 1 link)
            type Node subtype V4L flags 0
            device node name /dev/video0
        pad0: Sink
                <- "tw9900 2-0044":0 [ENABLED]

- entity 5: tw9900 2-0044 (1 pad, 1 link)
            type V4L2 subdev subtype Unknown flags 0
            device node name /dev/v4l-subdev0
        pad0: Source
                [fmt:UYVY8_2X8/720x480 field:none colorspace:smpte170m xfer:709 ycbcr:601]
                -> "rockchip_cif":0 [ENABLED]

> 
> Cheers,
> 
> Paul
> 
> > Best regards,
> > Michael
> > 
> > > 
> > > Cheers,
> > > 
> > > Paul
> > > 
> > >> @Mehdi: If you could Cc: me when you send out v9 it'd be much appreciated.

Of course I will :)

--
Kind Regards
Mehdi Djait