On 16 June 2017 at 17:08, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> On 06/16/2017 05:55 PM, Dave Stevenson wrote:
>>
>> On 16 June 2017 at 15:05, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>>>
>>> On 06/15/17 17:11, Dave Stevenson wrote:
>>>>
>>>> On 15 June 2017 at 15:14, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>>>>>
>>>>> On 06/15/17 15:38, Dave Stevenson wrote:
>>>>>>
>>>>>> Hi Hans.
>>>>>>
>>>>>> "On 15 June 2017 at 08:12, Hans Verkuil <hverkuil@xxxxxxxxx> 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@xxxxxxxxxxxxxxx>
>>>>>>>> ---
>>>>>>>> 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.
>>>
>>>
>>> As a csi receiver, yes. But the adv has a mux (or at least the adv7180
>>> does) where it can switch between multiple Composite or S-Video inputs.
>>> s_routing controls the mux.
>>>
>>> But the end-user knows nothing about the internal routing, he only knows
>>> about the connectors on the board. So with VIDIOC_ENUMINPUTS you can
>>> see which inputs there are to choose from with names corresponding to
>>> the labels on the backplane or in the user manual. With VIDIOC_S_INPUT
>>> he can select an input, and the device tree then has to provide the
>>> necessary information on how an input connector is hooked up to the adv.
>>>
>>>> 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?
>>>
>>>
>>> Ah, nice. There are actually already connector bindings. I'd forgotten
>>> those went in.
>>>
>>> Anyway, the tvp5150 is used in two situations:
>>>
>>> either as part of the USB non-MC device, or as part of a platform MC
>>> device.
>>>
>>> You have a platform non-MC device. The tvp5150 doesn't support that, but
>>> if it did, then the DT parsing could wouldn't be under
>>> CONFIG_MEDIA_CONTROLLER.
>>>
>>> It should really be under CONFIG_OF since it is (mostly) unrelated to the
>>> MC.
>>>
>>> But we're missing support for this scenario. For USB/PCI boards we have
>>> card descriptions whose information is used in s_routing, for MC-devices
>>> we leave it up to the user to set the routing (tvp5150_link_setup). But
>>> we don't have anything for a non-MC device that use the device tree.
>>>
>>> All the information is there, but we're missing infrastructure to
>>> give the connector information to the main V4L2 driver so it can
>>> implement
>>> the input ioctls, and to tell the subdev driver how to program the mux.
>>
>>
>> That's the bit that I was stumbling over - there is a bit of
>> infrastructure missing.
>>
>> So I'm reading that as the adv driver is incorrect in that it is just
>> exposing all the potential input configurations. It really needs the
>> DT connectors bit adding to refine the input selection down to the
>> actually available connectors on the physical board.
>> In my eval board case, DT should describe input connectors for 4
>> inputs, corresponding to CVBS_AIN1, CVBS_AIN2, DIFF_CVBS_AIN3_AIN4,
>> and DIFF_CVBS_AIN7_AIN8. The fact that I could externally connect
>> adapters to combine AIN1 and AIN2 to become s-video isn't relevant (or
>> at least secondary).
>>
>> If we have a bit of infrastructure missing, what is the correct way
>> forward for this driver right now?
>> I was comparing against the am437x-vfpe, davinci/vpif_capture, and
>> blackfin/bfin_capture drivers. All of them in some shape or form take
>> VIDIOC_S_INPUT and end up calling s_routing. It may be incorrect, but
>> that is how several platforms are using it. Is it acceptable to adopt
>> this incorrect behaviour until the infrastructure changes are all
>> done?
>>
>> As a relative mainline kernel newbie I'm slightly reluctant to
>> volunteer to implement the missing infrastructure, but with some
>> guidance in advance I'm willing to put together a patchset for
>> discussion, and adapt the adv driver to use DT connectors.
>>
>> Then again the dtbindings for connectors in tvp5150 got reverted
>> although the driver didn't:
>> 31e717d [media] Revert "[media] tvp5150: document input connectors DT
>> bindings"
>> "There are still ongoing discussions about how the input connectors
>> will be supported by the Media Controller framework so until that
>> is settled, it is better to revert the connectors portion of the
>> bindings to avoid known to be broken bindings docs to hit mainline."
>>
>> Did things ever get settled [1], or did it slip through the cracks?
>> There don't appear to be any input connector bindings described
>> anywhere I can find them.
>> This sounds like a topic to be discussed in a new thread rather than
>> hidden within a patch review.
>
>
> This basically ran out of steam. Lack of time and resources prevented this
> from going forward.
No problem, I can relate to that.
> Do you *need* to have support for the adv in the first version of this
> driver? Basically for drivers like this we have good support for sensors,
> and we can handle video receiver devices as long as there is no need to
> call s_routing.
>
> Why not get that done first?
I don't *need* to have support for any specific devices at V1, but
obviously I needed something to hook up as a source.
TC358743, OV5647, and ADV7282-M were the devices I had available to
test with, and the first two of those particularly made sense as they
are easily available in a form to connect to the Pi (Auvidea B101/B102
boards, and the V1 Pi camera). The ADV7282 is a nice one to add as it
then gives drivers supporting each of enum/s/g_std,
enum/g/s_dv_timings, and a sensor supporting neither. I was trying to
support the full feature set of each of them.
My issue was that the approach seemingly taken by other existing
drivers wasn't acceptable here. It looks like I'd missed that when
used as platform devices the drivers I'd looked at had routing
information and were doing the mapping required. My bad.
I'll remove the s_routing call for now and add a FIXME detailing the issue.
Thanks for your help and patience over this.
Dave
