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

2016-12-07 Thread Sakari Ailus 
Hi Kevin,

On Tue, Dec 06, 2016 at 11:50:58AM -0800, Kevin Hilman wrote:
> On Tue, Dec 6, 2016 at 9:40 AM, Kevin Hilman  wrote:
> > Hans Verkuil  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  writes:
> >> On Wed, Nov 23, 2016 at 03:25:32PM -0800, Kevin Hilman wrote:
> >>> Sakari Ailus  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 
> > ---
> >
> >  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 
> >
> >  #include 
> > +#include 
> > +#include 
> 
>  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 = _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();
> >  }
> >
>

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

2016-12-06 Thread Kevin Hilman 
On Tue, Dec 6, 2016 at 9:40 AM, Kevin Hilman  wrote:
> Hans Verkuil  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  writes:
>> On Wed, Nov 23, 2016 at 03:25:32PM -0800, Kevin Hilman wrote:
>>> Sakari Ailus  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 
> ---
>
>  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 
>
>  #include 
> +#include 
> +#include 

 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 = _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;
>

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

2016-12-06 Thread Kevin Hilman 
Hans Verkuil  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  writes:
> On Wed, Nov 23, 2016 at 03:25:32PM -0800, Kevin Hilman wrote:
>> Sakari Ailus  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 
 ---

  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 

  #include 
 +#include 
 +#include 
>>>
>>> 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 = _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(>dev, "vpif_get_pdata\n");
 +
 +  if

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

2016-12-05 Thread Hans Verkuil 
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  writes:
 On Wed, Nov 23, 2016 at 03:25:32PM -0800, Kevin Hilman wrote:
> Sakari Ailus  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 
>>> ---
>>>
>>>  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 
>>>
>>>  #include 
>>> +#include 
>>> +#include 
>>
>> 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 = _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(>dev, "vpif_get_pdata\n");
>>> +
>>> +   if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
>>> +   return pdev->dev.platform_data;
>>> +
>>> +   pdata =

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

2016-12-01 Thread Laurent Pinchart 
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  writes:
> >> On Wed, Nov 23, 2016 at 03:25:32PM -0800, Kevin Hilman wrote:
> >>> Sakari Ailus  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 
> > ---
> > 
> >  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 
> > 
> >  #include 
> > +#include 
> > +#include 
>  
>  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.

> >  #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 = _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(>dev, "vpif_get_pdata\n");
> > +
> > +   if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
> > +   return pdev->dev.platform_data;
> > +
> > +   pdata = devm_kzalloc(>dev, sizeof(*pdata), GFP_KERNEL);
> > +   if (!pdata)
> > +   return NULL;
> > +   pdata->subdev_info =
> > +

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

2016-11-30 Thread Sakari Ailus 
On Wed, Nov 30, 2016 at 04:14:11PM -0800, Kevin Hilman wrote:
> Sakari Ailus  writes:
> 
> > Hi Kevin,
> >
> > On Wed, Nov 23, 2016 at 03:25:32PM -0800, Kevin Hilman wrote:
> >> Hi Sakari,
> >> 
> >> Sakari Ailus  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 
> >> >> ---
> >> >>  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 
> >> >>  
> >> >>  #include 
> >> >> +#include 
> >> >> +#include 
> >> >
> >> > 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 = _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(>dev, "vpif_get_pdata\n");
> >> >> +
> >> >> +   if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
> >> >> +   return pdev->dev.platform_data;
> >> >> +
> >> >> +   pdata = devm_kzalloc(>dev, sizeof(*pdata), GFP_KERNEL);
> >> >> +   if (!pdata)
> >> >> +   return NULL;
> >> >> +   pdata->subdev_info =
> >> >> +   devm_kzalloc(>dev, sizeof(*pdata->subdev_info) *
> >> >> +VPIF_CAPTURE_MAX_CHANNELS, GFP_KERNEL);
> >> >> +
> >> >> +   if

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

2016-11-30 Thread Kevin Hilman 
Sakari Ailus  writes:

> Hi Kevin,
>
> On Wed, Nov 23, 2016 at 03:25:32PM -0800, Kevin Hilman wrote:
>> Hi Sakari,
>> 
>> Sakari Ailus  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 
>> >> ---
>> >>  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 
>> >>  
>> >>  #include 
>> >> +#include 
>> >> +#include 
>> >
>> > 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 = _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(>dev, "vpif_get_pdata\n");
>> >> +
>> >> + if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
>> >> + return pdev->dev.platform_data;
>> >> +
>> >> + pdata = devm_kzalloc(>dev, sizeof(*pdata), GFP_KERNEL);
>> >> + if (!pdata)
>> >> + return NULL;
>> >> + pdata->subdev_info =
>> >> + devm_kzalloc(>dev, sizeof(*pdata->subdev_info) *
>> >> +  VPIF_CAPTURE_MAX_CHANNELS, GFP_KERNEL);
>> >> +
>> >> + if (!pdata->subdev_info)
>> >> + return NULL;
>> >> + dev_dbg(>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 = >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.
>> 
>>

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

2016-11-30 Thread Sakari Ailus 
Hi Kevin,

On Wed, Nov 23, 2016 at 03:25:32PM -0800, Kevin Hilman wrote:
> Hi Sakari,
> 
> Sakari Ailus  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 
> >> ---
> >>  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 
> >>  
> >>  #include 
> >> +#include 
> >> +#include 
> >
> > 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 = _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(>dev, "vpif_get_pdata\n");
> >> +
> >> +  if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
> >> +  return pdev->dev.platform_data;
> >> +
> >> +  pdata = devm_kzalloc(>dev, sizeof(*pdata), GFP_KERNEL);
> >> +  if (!pdata)
> >> +  return NULL;
> >> +  pdata->subdev_info =
> >> +  devm_kzalloc(>dev, sizeof(*pdata->subdev_info) *
> >> +   VPIF_CAPTURE_MAX_CHANNELS, GFP_KERNEL);
> >> +
> >> +  if (!pdata->subdev_info)
> >> +  return NULL;
> >> +  dev_dbg(>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 = >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 = >chan_config[i];
> >> +  chan->inputs = devm_kzalloc(>dev,
> >> +

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

2016-11-23 Thread Kevin Hilman 
Hi Sakari,

Sakari Ailus  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 
>> ---
>>  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 
>>  
>>  #include 
>> +#include 
>> +#include 
>
> 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 = _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(>dev, "vpif_get_pdata\n");
>> +
>> +if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
>> +return pdev->dev.platform_data;
>> +
>> +pdata = devm_kzalloc(>dev, sizeof(*pdata), GFP_KERNEL);
>> +if (!pdata)
>> +return NULL;
>> +pdata->subdev_info =
>> +devm_kzalloc(>dev, sizeof(*pdata->subdev_info) *
>> + VPIF_CAPTURE_MAX_CHANNELS, GFP_KERNEL);
>> +
>> +if (!pdata->subdev_info)
>> +return NULL;
>> +dev_dbg(>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 = >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 = >chan_config[i];
>> +chan->inputs = devm_kzalloc(>dev,
>> +sizeof(*chan->inputs) *
>> +VPIF_DISPLAY_MAX_CHANNELS,
>> +GFP_KERNEL);
>> +
>> +chan->input_count++;
>> +

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

2016-11-23 Thread Kevin Hilman 
Hi Sakari,

Sakari Ailus  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 
>> ---
>>  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 
>>  
>>  #include 
>> +#include 
>> +#include 
>
> 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 = _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(>dev, "vpif_get_pdata\n");
>> +
>> +if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
>> +return pdev->dev.platform_data;
>> +
>> +pdata = devm_kzalloc(>dev, sizeof(*pdata), GFP_KERNEL);
>> +if (!pdata)
>> +return NULL;
>> +pdata->subdev_info =
>> +devm_kzalloc(>dev, sizeof(*pdata->subdev_info) *
>> + VPIF_CAPTURE_MAX_CHANNELS, GFP_KERNEL);
>> +
>> +if (!pdata->subdev_info)
>> +return NULL;
>> +dev_dbg(>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 = >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 = >chan_config[i];
>> +chan->inputs = devm_kzalloc(>dev,
>> +sizeof(*chan->inputs) *
>> +VPIF_DISPLAY_MAX_CHANNELS,
>> +GFP_KERNEL);
>> +
>> +chan->input_count++;
>> +

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

2016-11-23 Thread Sakari Ailus 
Hi Kevin,

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"?

> am437x-vpfe.c
> 
> Signed-off-by: Kevin Hilman 
> ---
>  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 
>  
>  #include 
> +#include 
> +#include 

Do you need this header?

>  
>  #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 = _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(>dev, "vpif_get_pdata\n");
> +
> + if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
> + return pdev->dev.platform_data;
> +
> + pdata = devm_kzalloc(>dev, sizeof(*pdata), GFP_KERNEL);
> + if (!pdata)
> + return NULL;
> + pdata->subdev_info =
> + devm_kzalloc(>dev, sizeof(*pdata->subdev_info) *
> +  VPIF_CAPTURE_MAX_CHANNELS, GFP_KERNEL);
> +
> + if (!pdata->subdev_info)
> + return NULL;
> + dev_dbg(>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 = >subdev_info[i];

subdev_info[] has got VPIF_CAPTURE_MAX_CHANNELS entries only.

> + chan = >chan_config[i];
> + chan->inputs = devm_kzalloc(>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.

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.

> +

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

2016-11-22 Thread Kevin Hilman 
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 
---
 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 
 
 #include 
+#include 
+#include 
 
 #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 = _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(>dev, "vpif_get_pdata\n");
+
+   if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
+   return pdev->dev.platform_data;
+
+   pdata = devm_kzalloc(>dev, sizeof(*pdata), GFP_KERNEL);
+   if (!pdata)
+   return NULL;
+   pdata->subdev_info =
+   devm_kzalloc(>dev, sizeof(*pdata->subdev_info) *
+VPIF_CAPTURE_MAX_CHANNELS, GFP_KERNEL);
+
+   if (!pdata->subdev_info)
+   return NULL;
+   dev_dbg(>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 = >subdev_info[i];
+   chan = >chan_config[i];
+   chan->inputs = devm_kzalloc(>dev,
+   sizeof(*chan->inputs) *
+   VPIF_DISPLAY_MAX_CHANNELS,
+   GFP_KERNEL);
+
+   chan->input_count++;
+   chan->inputs[i].input.type = V4L2_INPUT_TYPE_CAMERA;
+   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)
+   chan->inputs[i].input_route = INPUT_CVBS_VI2B;
+   else
+   chan->inputs[i].input_route = INPUT_SVIDEO_VI2C_VI1C;
+   chan->inputs[i].output_route = OUTPUT_10BIT_422_EMBEDDED_SYNC;
+
+   err = v4l2_of_parse_endpoint(endpoint, _cfg);
+   if (err) {
+   dev_err(>dev, "Could not parse the endpoint\n");
+   goto done;
+