On Wed, Nov 30, 2016 at 04:14:11PM -0800, Kevin Hilman wrote: > Sakari Ailus <sakari.ailus@xxxxxx> writes: > > > Hi Kevin, > > > > On Wed, Nov 23, 2016 at 03:25:32PM -0800, Kevin Hilman wrote: > >> Hi Sakari, > >> > >> 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) > >> > > >> > vpif_capture_get_pdata and "largely"? > >> > >> Yes, thanks. > >> > >> >> 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... > >> > >> >> > >> >> #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; > >> >> + 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 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. -- Kind regards, Sakari Ailus e-mail: sakari.ailus@xxxxxx XMPP: sailus@xxxxxxxxxxxxxx -- 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