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

[Michael Riesch]

Hi Mehdi,

On 10/25/23 12:33, Mehdi Djait wrote:
> 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.)

Sorry, had a false memory of this. The media device *does* pop 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 :)

Cool, thanks! Looking forward to it!

Best regards,
Michael

> 
> --
> Kind Regards
> Mehdi Djait