[RFC 2/2] [media] bcm2835-unicam: Driver for CCP2/CSI2 camera interface

[Dave Stevenson]

On 15 June 2017 at 15:14, Hans Verkuil <hverkuil at xs4all.nl> wrote:
> On 06/15/17 15:38, Dave Stevenson wrote:
>> Hi Hans.
>>
>> "On 15 June 2017 at 08:12, Hans Verkuil <hverkuil at xs4all.nl> wrote:
>>> Hi Dave,
>>>
>>> Here is a quick review of this driver. Once a v2 is posted I'll do a more
>>> thorough
>>> check.
>>
>> Thank you. I wasn't expecting such a quick response.
>>
>>> On 06/14/2017 05:15 PM, Dave Stevenson wrote:
>>>>
>>>> Add driver for the Unicam camera receiver block on
>>>> BCM283x processors.
>>>>
>>>> Signed-off-by: Dave Stevenson <dave.stevenson at raspberrypi.org>
>>>> ---
>>>>   drivers/media/platform/Kconfig                   |    1 +
>>>>   drivers/media/platform/Makefile                  |    2 +
>>>>   drivers/media/platform/bcm2835/Kconfig           |   14 +
>>>>   drivers/media/platform/bcm2835/Makefile          |    3 +
>>>>   drivers/media/platform/bcm2835/bcm2835-unicam.c  | 2100
>>>> ++++++++++++++++++++++
>>>>   drivers/media/platform/bcm2835/vc4-regs-unicam.h |  257 +++
>>>>   6 files changed, 2377 insertions(+)
>>>>   create mode 100644 drivers/media/platform/bcm2835/Kconfig
>>>>   create mode 100644 drivers/media/platform/bcm2835/Makefile
>>>>   create mode 100644 drivers/media/platform/bcm2835/bcm2835-unicam.c
>>>>   create mode 100644 drivers/media/platform/bcm2835/vc4-regs-unicam.h
>>>>
>>>> +static int unicam_s_input(struct file *file, void *priv, unsigned int i)
>>>> +{
>>>> +       struct unicam_device *dev = video_drvdata(file);
>>>> +       int ret;
>>>> +
>>>> +       if (v4l2_subdev_has_op(dev->sensor, video, s_routing))
>>>> +               ret =  v4l2_subdev_call(dev->sensor, video, s_routing, i,
>>>> 0, 0);
>>>> +       else
>>>> +               ret = -EINVAL;  /* v4l2-compliance insists on -EINVAL */
>>>
>>>
>>> Drop this if-else entirely. s_routing makes really no sense when using a
>>> device
>>> tree. In this particular case there really is just one input, period.
>>
>> I added this due to the ADV7282-M analogue to CSI bridge chip (uses
>> adv7180.c driver). It uses s_routing to select the physical input /
>> input type.
>> If this is dropped, what is the correct mechanism for selecting the
>> input? Unless I've missed it, s_routing is not a call that is exposed
>> to userspace, so we're stuck with composite input 1.
>>
>> I had asked this question in previously [1], and whilst Sakari had
>> kindly replied with "s_routing() video op as it stands now is awful, I
>> hope no-one uses it", the fact is that it is used.
>>
>> [1] http://www.spinics.net/lists/linux-media/msg115550.html
>
> s_routing was developed for USB and PCI(e) devices and predates the device tree.
> Basically USB and PCI drivers will have card definitions where USB/PCI card IDs
> are mapped to card descriptions, and that includes information on the various
> inputs (composite, S-Video, etc) that are available on the backplane and how those
> physical connectors are hooked up to the pins on the video ICs.
>
> The enum/s/g_input ioctls all show the end-user view, i.e. they enumerate the
> inputs on the backpanel of the product. The s_routing op was created to map
> such inputs to actual pins on the ICs.
>
> For platform devices we would do this in the device tree today, but some of
> the necessary bindings are still missing. Specifically those for connectors,
> AFAIK those are not yet defined. It's been discussed, but never finalized.
>
> So if this was done correctly you would use the connector endpoints in the
> device tree to enumerate the inputs and use how they are connected to the
> other blocks as the routing information (i.e. pad number).
>
> I would say that is the advanced course and to do this later.

Certainly the advanced course, but I'm still not seeing how that all
hangs together.

To me that all sounds like stuff that ought to be within the ADV
driver? From my perspective as the CSI-2 receiver I only have one
input.
So how does the application select between those inputs?

Having had a bit of a grep I think the tvp5150 driver is doing what
you're suggesting. However that appears to force you into using the
media controller API. Is that not overkill particularly from an
application perspective?

You also say above that enum/s/g_input is all about switching between
physical connectors, and that's what I'm doing. I'm now getting lost
as to what is intended

> Do you even have hardware where you can switch between inputs?

I have an ADV7282-M eval board [1] sitting in front of me, with the
I2C and CSI-2 output hooked up to a butchered Pi camera board.
6 phono inputs set up as 2 single-ended composite inputs, and 2 pairs
of differentital composite inputs. It can be reconfigured to take
s-video, or component (although it needs a couple of surface mount
resistors changed to do component).
1 CSI-2 (single lane) output going to the Pi.

I haven't got it producing CSI data that the Pi is happy with yet, but
hopefully that is only a matter of time.
The I2C comms is all working.
With the driver as it stands I can use the s_input ioctl to send the
I2C for the relevant input. It was a few weeks ago I last played with
it, but IIRC it did appear to be detecting the incoming video standard
correctly on each of composite 1 and composite 2 (I haven't got
s-video or differential hooked up).

  Dave

[1] http://www.analog.com/en/design-center/evaluation-hardware-and-software/evaluation-boards-kits/EVAL-ADV7282MEBZ.html