On Tue, Dec 6, 2016 at 9:40 AM, Kevin Hilman <khilman@xxxxxxxxxxxx> wrote: > Hans Verkuil <hverkuil@xxxxxxxxx> writes: > >> 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. > > So I'm still confused on the path forward here. > > I do not have the time (or the V4L2 knowledge/experience) to rework the > V4L2 internals to make this work, but I'm happy to test if someone else > is working on it. > > In the meantime, what do we do with this series? I have a couple minor > things to fixup based on review comments, but other than that, the > s_routing decision is blocking this from getting an update for use on DT > platforms. > > The alternative is to go the OMAP route for legacy drivers like this and > just use pdata quirks for passing the legacy pdata (which has the input > and output routes hard-coded in platform_data). Also, FYI, I have the same issue with the output/display side of this controller. It's using an I2C-connected adv7343, where the input and output routes are configured by the driver using s_routing, and the current code passes the routes in using platform_data. Kevin -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html