Re: [PATCH v3 3/4] [media] davinci: vpif_capture: get subdevs from DT

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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".  ;)

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



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux