On 12/01/2016 10:16 AM, Laurent Pinchart wrote:
> Hello,
>
> On Thursday 01 Dec 2016 09:57:31 Sakari Ailus wrote:
>> On Wed, Nov 30, 2016 at 04:14:11PM -0800, Kevin Hilman wrote:
>>> Sakari Ailus <sakari.ailus@xxxxxx> writes:
>>>> On Wed, Nov 23, 2016 at 03:25:32PM -0800, Kevin Hilman wrote:
>>>>> Sakari Ailus <sakari.ailus@xxxxxx> writes:
>>>>>> On Tue, Nov 22, 2016 at 07:52:43AM -0800, Kevin Hilman wrote:
>>>>>>> Allow getting of subdevs from DT ports and endpoints.
>>>>>>>
>>>>>>> The _get_pdata() function was larely inspired by (i.e. stolen from)
>>>>>>> am437x-vpfe.c
>>>>>>>
>>>>>>> Signed-off-by: Kevin Hilman <khilman@xxxxxxxxxxxx>
>>>>>>> ---
>>>>>>>
>>>>>>> drivers/media/platform/davinci/vpif_capture.c | 130 +++++++++++++++-
>>>>>>> include/media/davinci/vpif_types.h
>>>>>>> | 9 +-
>>>>>>> 2 files changed, 133 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/media/platform/davinci/vpif_capture.c
>>>>>>> b/drivers/media/platform/davinci/vpif_capture.c index
>>>>>>> 94ee6cf03f02..47a4699157e7 100644
>>>>>>> --- a/drivers/media/platform/davinci/vpif_capture.c
>>>>>>> +++ b/drivers/media/platform/davinci/vpif_capture.c
>>>>>>> @@ -26,6 +26,8 @@
>>>>>>> #include <linux/slab.h>
>>>>>>>
>>>>>>> #include <media/v4l2-ioctl.h>
>>>>>>> +#include <media/v4l2-of.h>
>>>>>>> +#include <media/i2c/tvp514x.h>
>>>>>>
>>>>>> Do you need this header?
>>>>>
>>>>> Yes, based on discussion with Hans, since there is no DT binding for
>>>>> selecting the input pins of the TVP514x, I have to select it in the
>>>>> driver, so I need the defines from this header. More on this below...
>
> That's really ugly :-( The problem should be fixed properly instead of adding
> one more offender.
Do you have time for that, Laurent? I don't. Until that time we just need to
make do with this workaround.
>
>>>>>>> #include "vpif.h"
>>>>>>> #include "vpif_capture.h"
>>>>>>> @@ -650,6 +652,10 @@ static int vpif_input_to_subdev(
>>>>>>>
>>>>>>> vpif_dbg(2, debug, "vpif_input_to_subdev\n");
>>>>>>>
>>>>>>> + if (!chan_cfg)
>>>>>>> + return -1;
>>>>>>> + if (input_index >= chan_cfg->input_count)
>>>>>>> + return -1;
>>>>>>> subdev_name = chan_cfg->inputs[input_index].subdev_name;
>>>>>>> if (subdev_name == NULL)
>>>>>>> return -1;
>>>>>>> @@ -657,7 +663,7 @@ static int vpif_input_to_subdev(
>>>>>>> /* loop through the sub device list to get the sub device info
>>>>>>> */
>>>>>>> for (i = 0; i < vpif_cfg->subdev_count; i++) {
>>>>>>> subdev_info = &vpif_cfg->subdev_info[i];
>>>>>>> - if (!strcmp(subdev_info->name, subdev_name))
>>>>>>> + if (subdev_info && !strcmp(subdev_info->name,
>>>>>>> subdev_name))
>>>>>>> return i;
>>>>>>> }
>>>>>>> return -1;
>>>>>>> @@ -1327,6 +1333,21 @@ static int vpif_async_bound(struct
>>>>>>> v4l2_async_notifier *notifier,> >> >>
>>>>>>> {
>>>>>>> int i;
>>>>>>>
>>>>>>> + for (i = 0; i < vpif_obj.config->asd_sizes[0]; i++) {
>>>>>>> + struct v4l2_async_subdev *_asd = vpif_obj.config
>>>>>>> ->asd[i];
>>>>>>> + const struct device_node *node = _asd->match.of.node;
>>>>>>> +
>>>>>>> + if (node == subdev->of_node) {
>>>>>>> + vpif_obj.sd[i] = subdev;
>>>>>>> + vpif_obj.config->chan_config
>>>>>>> ->inputs[i].subdev_name =
>>>>>>> + (char *)subdev->of_node->full_name;
>
> Can subdev_name be made const instead of blindly casting the full_name pointer
> ? If not this is probably unsafe, and if yes it should be done :-)
>
>>>>>>> + vpif_dbg(2, debug,
>>>>>>> + "%s: setting input %d subdev_name =
>>>>>>> %s\n",
>>>>>>> + __func__, i, subdev->of_node
>>>>>>> ->full_name);
>>>>>>> + return 0;
>>>>>>> + }
>>>>>>> + }
>>>>>>> +
>>>>>>> for (i = 0; i < vpif_obj.config->subdev_count; i++)
>>>>>>> if (!strcmp(vpif_obj.config->subdev_info[i].name,
>>>>>>> subdev->name)) {
>>>>>>> @@ -1422,6 +1443,110 @@ static int vpif_async_complete(struct
>>>>>>> v4l2_async_notifier *notifier)
>>>>>>> return vpif_probe_complete();
>>>>>>> }
>>>>>>>
>>>>>>> +static struct vpif_capture_config *
>>>>>>> +vpif_capture_get_pdata(struct platform_device *pdev)
>>>>>>> +{
>>>>>>> + struct device_node *endpoint = NULL;
>>>>>>> + struct v4l2_of_endpoint bus_cfg;
>>>>>>> + struct vpif_capture_config *pdata;
>>>>>>> + struct vpif_subdev_info *sdinfo;
>>>>>>> + struct vpif_capture_chan_config *chan;
>>>>>>> + unsigned int i;
>>>>>>> +
>>>>>>> + dev_dbg(&pdev->dev, "vpif_get_pdata\n");
>>>>>>> +
>>>>>>> + if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
>>>>>>> + return pdev->dev.platform_data;
>>>>>>> +
>>>>>>> + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
>>>>>>> + if (!pdata)
>>>>>>> + return NULL;
>>>>>>> + pdata->subdev_info =
>>>>>>> + devm_kzalloc(&pdev->dev, sizeof(*pdata->subdev_info) *
>>>>>>> + VPIF_CAPTURE_MAX_CHANNELS, GFP_KERNEL);
>>>>>>> +
>>>>>>> + if (!pdata->subdev_info)
>>>>>>> + return NULL;
>>>>>>> + dev_dbg(&pdev->dev, "%s\n", __func__);
>>>>>>> +
>>>>>>> + for (i = 0; ; i++) {
>>>>>>> + struct device_node *rem;
>>>>>>> + unsigned int flags;
>>>>>>> + int err;
>>>>>>> +
>>>>>>> + endpoint = of_graph_get_next_endpoint(pdev
>>>>>>> ->dev.of_node,
>>>>>>> + endpoint);
>>>>>>> + if (!endpoint)
>>>>>>> + break;
>>>>>>> +
>>>>>>> + sdinfo = &pdata->subdev_info[i];
>>>>>>
>>>>>> subdev_info[] has got VPIF_CAPTURE_MAX_CHANNELS entries only.
>>>>>
>>>>> Right, I need to make the loop only go for a max of
>>>>> VPIF_CAPTURE_MAX_CHANNELS iterations.
>>>>>
>>>>>>> + chan = &pdata->chan_config[i];
>>>>>>> + chan->inputs = devm_kzalloc(&pdev->dev,
>>>>>>> + sizeof(*chan->inputs) *
>>>>>>> + VPIF_DISPLAY_MAX_CHANNELS,
>>>>>>> + GFP_KERNEL);
>>>>>>> +
>>>>>>> + chan->input_count++;
>>>>>>> + chan->inputs[i].input.type = V4L2_INPUT_TYPE_CAMERA;
>>>>>>
>>>>>> I wonder what's the purpose of using index i on this array as well.
>>>>>
>>>>> The number of endpoints in DT is the number of input channels
>>>>> configured (up to a max of VPIF_CAPTURE_MAX_CHANNELS.)
>>>>>
>>>>>> If you use that to access a corresponding entry in a different array,
>>>>>> I'd just create a struct that contains the port configuration and the
>>>>>> async sub-device. The omap3isp driver does that, for instance; see
>>>>>> isp_of_parse_nodes() in drivers/media/platform/omap3isp/isp.c if
>>>>>> you're interested. Up to you.
>>>>>
>>>>> OK, I'll have a look at that driver. The goal here with this series is
>>>>> just to get this working with DT, but also not break the existing
>>>>> legacy platform_device support, so I'm trying not to mess with the
>>>>> driver-interal data structures too much.
>>>>
>>>> Ack.
>>>>
>>>>>>> + chan->inputs[i].input.std = V4L2_STD_ALL;
>>>>>>> + chan->inputs[i].input.capabilities = V4L2_IN_CAP_STD;
>>>>>>> +
>>>>>>> + /* FIXME: need a new property? ch0:composite ch1:
>>>>>>> s-video */
>>>>>>> + if (i == 0)
>>>>>>
>>>>>> Can you assume that the first endopoint has got a particular kind of
>>>>>> input? What if it's not connected?
>>>>>
>>>>> On all the boards I know of (there aren't many using this SoC), it's a
>>>>> safe assumption.
>>>>>
>>>>>> If this is a different physical port (not in the meaning another) in
>>>>>> the device, I'd use the reg property for this. Please see
>>>>>> Documentation/devicetree/bindings/media/video-interfaces.txt .
>>>>>
>>>>> My understanding (which is admittedly somewhat fuzzy) of the TVP514x is
>>>>> that it's not physically a different port. Instead, it's just telling
>>>>> the TVP514x which pin(s) will be active inputs (and what kind of signal
>>>>> will be present.)
>>>>>
>>>>> I'm open to a better way to describe this input select from DT, but
>>>>> based on what I heard from Hans, there isn't currently a good way to do
>>>>> that except for in the driver:
>>>>> (c.f. https://marc.info/?l=linux-arm-kernel&m=147887871615788)
>>>>>
>>>>> Based on further discussion in that thread, it sounds like there may be
>>>>> a way forward coming soon, and I'll be glad to switch to that when it
>>>>> arrives.
>
> I'm afraid I have to disappoint Hans here, I don't have code for that yet.
>
>>>> I'm not sure that properly supporting connectors will provide any help
>>>> here.
>>>>
>>>> Looking at the s_routing() API, it's the calling driver that has to be
>>>> aware of sub-device specific function parameters. As such it's not a
>>>> very good idea to require that a driver is aware of the value range of
>>>> another driver's parameter. I wonder if a simple enumeration interface
>>>> would help here --- if I understand correctly, the purpose is just to
>>>> provide a way to choose the input using VIDIOC_S_INPUT.
>>>>
>>>> I guess that's somehow ok as long as you have no other combinations of
>>>> these devices but this is hardly future-proof. (And certainly not a
>>>> problem created by this patch.)
>>>
>>> Yeah, this is far from future proof.
>>>
>>>> It'd be still nice to fix that as presumably we don't have the option of
>>>> reworking how we expect the device tree to look like.
>>>
>>> Agreed.
>>>
>>> I'm just hoping someone can shed som light on "how we expect the device
>>> tree to look". ;)
>>
>> :-)
>>
>> For the tvp514x, do you need more than a single endpoint on the receiver
>> side? Does the input that's selected affect the bus parameters?
>>
>> If it doesn't, you could create a custom endpoint property for the possible
>> input values. The s_routing() really should be fixed though, but that could
>> be postponed I guess. There are quite a few drivers using it.
>
> There's two ways to look at s_routing() in my opinion, as the calling driver
> should really not hardcode any knowledge specific to a particular subdev. We
> can either have the calling driver discover the possible routing options at
> runtime through the subdev API, or modify the s_routing() API.
>
Some historical perspective: s_routing was added well before the device tree
was ever used for ARM. And at that time the vast majority of drivers were PCI
or USB drivers, very few platform drivers existed (and those typically used
sensors, not video receivers).
Before s_routing existed the situation was even worse.
Basically what s_routing does is a poor-man's device tree entry, telling the
subdev how to route video or audio from connector to the output of the chip.
Typically the card tables in PCI or USB drivers contain the correct arguments
for s_routing. Of course, today we'd do that with the DT, but that was not an
option years ago.
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
