Re: [PATCH v8 14/55] [media] media: add functions to allow creating interfaces

[Mauro Carvalho Chehab]

Em Fri, 11 Sep 2015 14:57:41 +0200
Hans Verkuil  escreveu:

> On 09/06/2015 02:02 PM, Mauro Carvalho Chehab wrote:
> > Interfaces are different than entities: they represent a
> > Kernel<->userspace interaction, while entities represent a
> > piece of hardware/firmware/software that executes a function.
> > 
> > Let's distinguish them by creating a separate structure to
> > store the interfaces.
> > 
> > Later patches should change the existing drivers and logic
> > to split the current interface embedded inside the entity
> > structure (device nodes) into a separate object of the graph.
> > 
> > Signed-off-by: Mauro Carvalho Chehab 
> 
> Acked-by: Hans Verkuil 
> 
> But see a small note below:
> 
> > 
> > diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> > index a23c93369a04..dc679dfe8ade 100644
> > --- a/drivers/media/media-entity.c
> > +++ b/drivers/media/media-entity.c
> > @@ -44,11 +44,41 @@ static inline const char *gobj_type(enum 
> > media_gobj_type type)
> > return "pad";
> > case MEDIA_GRAPH_LINK:
> > return "link";
> > +   case MEDIA_GRAPH_INTF_DEVNODE:
> > +   return "intf-devnode";
> > default:
> > return "unknown";
> > }
> >  }
> >  
> > +static inline const char *intf_type(struct media_interface *intf)
> > +{
> > +   switch (intf->type) {
> > +   case MEDIA_INTF_T_DVB_FE:
> > +   return "frontend";
> > +   case MEDIA_INTF_T_DVB_DEMUX:
> > +   return "demux";
> > +   case MEDIA_INTF_T_DVB_DVR:
> > +   return "DVR";
> > +   case MEDIA_INTF_T_DVB_CA:
> > +   return  "CA";
> 
> Would lower case be better? "dvr" and "ca"? Although for some reason I feel 
> that "CA"
> is fine too. Not sure why :-)
> 
> What is the name of the associated device node? Upper or lower case? I feel 
> that the
> name here should match the name of the device node.

Not sure if I answered that before. I opted to use upper case for DVR and 
CA because both are initials:
DVR - Digital Video Record
CA - Conditional Access

and initials are in upper case, in English.

The devnode names are whatever the udev rules tell ;) The Kernel actually 
asks to create DVB class devices for the first DVR and CA, located on the first
DVB adapter as:
/dev/dvb/adapter0/dvr0
/dev/dvb/adapter0/ca0

I don't mind changing those to lowercase to match the devnames on some
future patch, if it is a consensus that making those names matching the
device node is a requirement, but, in this case, maybe we should rename the
dvb stuff to:
dvb/adapter/foo, in order to better reflect how they'll appear
at devfs.

Please also notice that:

+   case MEDIA_INTF_T_DVB_NET:
+   return "dvbnet";


This is also not the device node. The device node there is actually:
/dev/dvb/adapter0/net0

So, IMHO, it is fine the way it is, as we don't want big names here
on those printks.

Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 14/55] [media] media: add functions to allow creating interfaces

[Hans Verkuil]

On 09/06/2015 02:02 PM, Mauro Carvalho Chehab wrote:
> Interfaces are different than entities: they represent a
> Kernel<->userspace interaction, while entities represent a
> piece of hardware/firmware/software that executes a function.
> 
> Let's distinguish them by creating a separate structure to
> store the interfaces.
> 
> Later patches should change the existing drivers and logic
> to split the current interface embedded inside the entity
> structure (device nodes) into a separate object of the graph.
> 
> Signed-off-by: Mauro Carvalho Chehab 

Acked-by: Hans Verkuil 

But see a small note below:

> 
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index a23c93369a04..dc679dfe8ade 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -44,11 +44,41 @@ static inline const char *gobj_type(enum media_gobj_type 
> type)
>   return "pad";
>   case MEDIA_GRAPH_LINK:
>   return "link";
> + case MEDIA_GRAPH_INTF_DEVNODE:
> + return "intf-devnode";
>   default:
>   return "unknown";
>   }
>  }
>  
> +static inline const char *intf_type(struct media_interface *intf)
> +{
> + switch (intf->type) {
> + case MEDIA_INTF_T_DVB_FE:
> + return "frontend";
> + case MEDIA_INTF_T_DVB_DEMUX:
> + return "demux";
> + case MEDIA_INTF_T_DVB_DVR:
> + return "DVR";
> + case MEDIA_INTF_T_DVB_CA:
> + return  "CA";

Would lower case be better? "dvr" and "ca"? Although for some reason I feel 
that "CA"
is fine too. Not sure why :-)

What is the name of the associated device node? Upper or lower case? I feel 
that the
name here should match the name of the device node.

> + case MEDIA_INTF_T_DVB_NET:
> + return "dvbnet";
> + case MEDIA_INTF_T_V4L_VIDEO:
> + return "video";
> + case MEDIA_INTF_T_V4L_VBI:
> + return "vbi";
> + case MEDIA_INTF_T_V4L_RADIO:
> + return "radio";
> + case MEDIA_INTF_T_V4L_SUBDEV:
> + return "v4l2-subdev";
> + case MEDIA_INTF_T_V4L_SWRADIO:
> + return "swradio";
> + default:
> + return "unknown-intf";
> + }
> +};

Regards,

Hans

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 14/55] [media] media: add functions to allow creating interfaces

[Javier Martinez Canillas]

On Sun, Sep 6, 2015 at 2:02 PM, Mauro Carvalho Chehab
 wrote:
> Interfaces are different than entities: they represent a
> Kernel<->userspace interaction, while entities represent a
> piece of hardware/firmware/software that executes a function.
>
> Let's distinguish them by creating a separate structure to
> store the interfaces.
>
> Later patches should change the existing drivers and logic
> to split the current interface embedded inside the entity
> structure (device nodes) into a separate object of the graph.
>
> Signed-off-by: Mauro Carvalho Chehab 
>

Reviewed-by: Javier Martinez Canillas 

Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 14/55] [media] media: add functions to allow creating interfaces

[Sakari Ailus]

Hi Mauro,

On Sun, Sep 06, 2015 at 09:02:45AM -0300, Mauro Carvalho Chehab wrote:
> Interfaces are different than entities: they represent a
> Kernel<->userspace interaction, while entities represent a
> piece of hardware/firmware/software that executes a function.
> 
> Let's distinguish them by creating a separate structure to
> store the interfaces.
> 
> Later patches should change the existing drivers and logic
> to split the current interface embedded inside the entity
> structure (device nodes) into a separate object of the graph.
> 
> Signed-off-by: Mauro Carvalho Chehab 
> 
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index a23c93369a04..dc679dfe8ade 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -44,11 +44,41 @@ static inline const char *gobj_type(enum media_gobj_type 
> type)
>   return "pad";
>   case MEDIA_GRAPH_LINK:
>   return "link";
> + case MEDIA_GRAPH_INTF_DEVNODE:
> + return "intf-devnode";
>   default:
>   return "unknown";
>   }
>  }
>  
> +static inline const char *intf_type(struct media_interface *intf)
> +{
> + switch (intf->type) {
> + case MEDIA_INTF_T_DVB_FE:
> + return "frontend";
> + case MEDIA_INTF_T_DVB_DEMUX:
> + return "demux";
> + case MEDIA_INTF_T_DVB_DVR:
> + return "DVR";
> + case MEDIA_INTF_T_DVB_CA:
> + return  "CA";
> + case MEDIA_INTF_T_DVB_NET:
> + return "dvbnet";
> + case MEDIA_INTF_T_V4L_VIDEO:
> + return "video";
> + case MEDIA_INTF_T_V4L_VBI:
> + return "vbi";
> + case MEDIA_INTF_T_V4L_RADIO:
> + return "radio";
> + case MEDIA_INTF_T_V4L_SUBDEV:
> + return "v4l2-subdev";
> + case MEDIA_INTF_T_V4L_SWRADIO:
> + return "swradio";
> + default:
> + return "unknown-intf";
> + }
> +};
> +
>  static void dev_dbg_obj(const char *event_name,  struct media_gobj *gobj)
>  {
>  #if defined(DEBUG) || defined (CONFIG_DYNAMIC_DEBUG)
> @@ -84,6 +114,19 @@ static void dev_dbg_obj(const char *event_name,  struct 
> media_gobj *gobj)
>   "%s: id 0x%08x pad#%d: '%s':%d\n",
>   event_name, gobj->id, media_localid(gobj),
>   pad->entity->name, pad->index);
> + break;
> + }
> + case MEDIA_GRAPH_INTF_DEVNODE:
> + {
> + struct media_interface *intf = gobj_to_intf(gobj);
> + struct media_intf_devnode *devnode = intf_to_devnode(intf);
> +
> + dev_dbg(gobj->mdev->dev,
> + "%s: id 0x%08x intf_devnode#%d: %s - major: %d, minor: 
> %d\n",
> + event_name, gobj->id, media_localid(gobj),
> + intf_type(intf),
> + devnode->major, devnode->minor);
> + break;
>   }
>   }
>  #endif
> @@ -119,6 +162,9 @@ void media_gobj_init(struct media_device *mdev,
>   case MEDIA_GRAPH_LINK:
>   gobj->id = media_gobj_gen_id(type, ++mdev->link_id);
>   break;
> + case MEDIA_GRAPH_INTF_DEVNODE:
> + gobj->id = media_gobj_gen_id(type, ++mdev->intf_devnode_id);
> + break;
>   }
>   dev_dbg_obj(__func__, gobj);
>  }
> @@ -793,3 +839,40 @@ struct media_pad *media_entity_remote_pad(struct 
> media_pad *pad)
>  
>  }
>  EXPORT_SYMBOL_GPL(media_entity_remote_pad);
> +
> +
> +/* Functions related to the media interface via device nodes */
> +
> +struct media_intf_devnode *media_devnode_create(struct media_device *mdev,
> + u32 type, u32 flags,
> + u32 major, u32 minor,
> + gfp_t gfp_flags)
> +{
> + struct media_intf_devnode *devnode;
> + struct media_interface *intf;
> +
> + devnode = kzalloc(sizeof(*devnode), gfp_flags);

Do you think the user might want to specify something else then GFP_KERNEL
as gfp_flags? If not, I'd keep this internal to the function. It can also be
added later if needed.

> + if (!devnode)
> + return NULL;
> +
> + intf = >intf;
> +
> + intf->type = type;
> + intf->flags = flags;
> +
> + devnode->major = major;
> + devnode->minor = minor;
> +
> + media_gobj_init(mdev, MEDIA_GRAPH_INTF_DEVNODE,
> +>intf.graph_obj);
> +
> + return devnode;
> +}
> +EXPORT_SYMBOL_GPL(media_devnode_create);
> +
> +void media_devnode_remove(struct media_intf_devnode *devnode)
> +{
> + media_gobj_remove(>intf.graph_obj);
> + kfree(devnode);
> +}
> +EXPORT_SYMBOL_GPL(media_devnode_remove);
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index 05414e351f8e..3b14394d5701 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -44,6 +44,7 @@ 

Re: [PATCH v8 14/55] [media] media: add functions to allow creating interfaces

[Mauro Carvalho Chehab]

Em Wed, 09 Sep 2015 10:34:05 +0300
Sakari Ailus  escreveu:

> Hi Mauro,
> 
> On Sun, Sep 06, 2015 at 09:02:45AM -0300, Mauro Carvalho Chehab wrote:
> > Interfaces are different than entities: they represent a
> > Kernel<->userspace interaction, while entities represent a
> > piece of hardware/firmware/software that executes a function.
> > 
> > Let's distinguish them by creating a separate structure to
> > store the interfaces.
> > 
> > Later patches should change the existing drivers and logic
> > to split the current interface embedded inside the entity
> > structure (device nodes) into a separate object of the graph.
> > 
> > Signed-off-by: Mauro Carvalho Chehab 
> > 
> > diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> > index a23c93369a04..dc679dfe8ade 100644
> > --- a/drivers/media/media-entity.c
> > +++ b/drivers/media/media-entity.c
> > @@ -44,11 +44,41 @@ static inline const char *gobj_type(enum 
> > media_gobj_type type)
> > return "pad";
> > case MEDIA_GRAPH_LINK:
> > return "link";
> > +   case MEDIA_GRAPH_INTF_DEVNODE:
> > +   return "intf-devnode";
> > default:
> > return "unknown";
> > }
> >  }
> >  
> > +static inline const char *intf_type(struct media_interface *intf)
> > +{
> > +   switch (intf->type) {
> > +   case MEDIA_INTF_T_DVB_FE:
> > +   return "frontend";
> > +   case MEDIA_INTF_T_DVB_DEMUX:
> > +   return "demux";
> > +   case MEDIA_INTF_T_DVB_DVR:
> > +   return "DVR";
> > +   case MEDIA_INTF_T_DVB_CA:
> > +   return  "CA";
> > +   case MEDIA_INTF_T_DVB_NET:
> > +   return "dvbnet";
> > +   case MEDIA_INTF_T_V4L_VIDEO:
> > +   return "video";
> > +   case MEDIA_INTF_T_V4L_VBI:
> > +   return "vbi";
> > +   case MEDIA_INTF_T_V4L_RADIO:
> > +   return "radio";
> > +   case MEDIA_INTF_T_V4L_SUBDEV:
> > +   return "v4l2-subdev";
> > +   case MEDIA_INTF_T_V4L_SWRADIO:
> > +   return "swradio";
> > +   default:
> > +   return "unknown-intf";
> > +   }
> > +};
> > +
> >  static void dev_dbg_obj(const char *event_name,  struct media_gobj *gobj)
> >  {
> >  #if defined(DEBUG) || defined (CONFIG_DYNAMIC_DEBUG)
> > @@ -84,6 +114,19 @@ static void dev_dbg_obj(const char *event_name,  struct 
> > media_gobj *gobj)
> > "%s: id 0x%08x pad#%d: '%s':%d\n",
> > event_name, gobj->id, media_localid(gobj),
> > pad->entity->name, pad->index);
> > +   break;
> > +   }
> > +   case MEDIA_GRAPH_INTF_DEVNODE:
> > +   {
> > +   struct media_interface *intf = gobj_to_intf(gobj);
> > +   struct media_intf_devnode *devnode = intf_to_devnode(intf);
> > +
> > +   dev_dbg(gobj->mdev->dev,
> > +   "%s: id 0x%08x intf_devnode#%d: %s - major: %d, minor: 
> > %d\n",
> > +   event_name, gobj->id, media_localid(gobj),
> > +   intf_type(intf),
> > +   devnode->major, devnode->minor);
> > +   break;
> > }
> > }
> >  #endif
> > @@ -119,6 +162,9 @@ void media_gobj_init(struct media_device *mdev,
> > case MEDIA_GRAPH_LINK:
> > gobj->id = media_gobj_gen_id(type, ++mdev->link_id);
> > break;
> > +   case MEDIA_GRAPH_INTF_DEVNODE:
> > +   gobj->id = media_gobj_gen_id(type, ++mdev->intf_devnode_id);
> > +   break;
> > }
> > dev_dbg_obj(__func__, gobj);
> >  }
> > @@ -793,3 +839,40 @@ struct media_pad *media_entity_remote_pad(struct 
> > media_pad *pad)
> >  
> >  }
> >  EXPORT_SYMBOL_GPL(media_entity_remote_pad);
> > +
> > +
> > +/* Functions related to the media interface via device nodes */
> > +
> > +struct media_intf_devnode *media_devnode_create(struct media_device *mdev,
> > +   u32 type, u32 flags,
> > +   u32 major, u32 minor,
> > +   gfp_t gfp_flags)
> > +{
> > +   struct media_intf_devnode *devnode;
> > +   struct media_interface *intf;
> > +
> > +   devnode = kzalloc(sizeof(*devnode), gfp_flags);
> 
> Do you think the user might want to specify something else then GFP_KERNEL
> as gfp_flags? If not, I'd keep this internal to the function. It can also be
> added later if needed.

Yeah, hardly interfaces would need something different than GFP_KERNEL.

I'll change it.

> 
> > +   if (!devnode)
> > +   return NULL;
> > +
> > +   intf = >intf;
> > +
> > +   intf->type = type;
> > +   intf->flags = flags;
> > +
> > +   devnode->major = major;
> > +   devnode->minor = minor;
> > +
> > +   media_gobj_init(mdev, MEDIA_GRAPH_INTF_DEVNODE,
> > +  >intf.graph_obj);
> > +
> > +   return devnode;
> > +}
> > +EXPORT_SYMBOL_GPL(media_devnode_create);
> > +
> > +void media_devnode_remove(struct media_intf_devnode *devnode)
> > +{
> > +   

Re: [PATCH v8 14/55] [media] media: add functions to allow creating interfaces

[Mauro Carvalho Chehab]

Interfaces are different than entities: they represent a
Kernel<->userspace interaction, while entities represent a
piece of hardware/firmware/software that executes a function.

Let's distinguish them by creating a separate structure to
store the interfaces.

Later patches should change the existing drivers and logic
to split the current interface embedded inside the entity
structure (device nodes) into a separate object of the graph.

Signed-off-by: Mauro Carvalho Chehab 

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index a23c93369a04..dc679dfe8ade 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -44,11 +44,41 @@ static inline const char *gobj_type(enum media_gobj_type 
type)
return "pad";
case MEDIA_GRAPH_LINK:
return "link";
+   case MEDIA_GRAPH_INTF_DEVNODE:
+   return "intf-devnode";
default:
return "unknown";
}
 }
 
+static inline const char *intf_type(struct media_interface *intf)
+{
+   switch (intf->type) {
+   case MEDIA_INTF_T_DVB_FE:
+   return "frontend";
+   case MEDIA_INTF_T_DVB_DEMUX:
+   return "demux";
+   case MEDIA_INTF_T_DVB_DVR:
+   return "DVR";
+   case MEDIA_INTF_T_DVB_CA:
+   return  "CA";
+   case MEDIA_INTF_T_DVB_NET:
+   return "dvbnet";
+   case MEDIA_INTF_T_V4L_VIDEO:
+   return "video";
+   case MEDIA_INTF_T_V4L_VBI:
+   return "vbi";
+   case MEDIA_INTF_T_V4L_RADIO:
+   return "radio";
+   case MEDIA_INTF_T_V4L_SUBDEV:
+   return "v4l2-subdev";
+   case MEDIA_INTF_T_V4L_SWRADIO:
+   return "swradio";
+   default:
+   return "unknown-intf";
+   }
+};
+
 static void dev_dbg_obj(const char *event_name,  struct media_gobj *gobj)
 {
 #if defined(DEBUG) || defined (CONFIG_DYNAMIC_DEBUG)
@@ -84,6 +114,19 @@ static void dev_dbg_obj(const char *event_name,  struct 
media_gobj *gobj)
"%s: id 0x%08x pad#%d: '%s':%d\n",
event_name, gobj->id, media_localid(gobj),
pad->entity->name, pad->index);
+   break;
+   }
+   case MEDIA_GRAPH_INTF_DEVNODE:
+   {
+   struct media_interface *intf = gobj_to_intf(gobj);
+   struct media_intf_devnode *devnode = intf_to_devnode(intf);
+
+   dev_dbg(gobj->mdev->dev,
+   "%s: id 0x%08x intf_devnode#%d: %s - major: %d, minor: 
%d\n",
+   event_name, gobj->id, media_localid(gobj),
+   intf_type(intf),
+   devnode->major, devnode->minor);
+   break;
}
}
 #endif
@@ -119,6 +162,9 @@ void media_gobj_init(struct media_device *mdev,
case MEDIA_GRAPH_LINK:
gobj->id = media_gobj_gen_id(type, ++mdev->link_id);
break;
+   case MEDIA_GRAPH_INTF_DEVNODE:
+   gobj->id = media_gobj_gen_id(type, ++mdev->intf_devnode_id);
+   break;
}
dev_dbg_obj(__func__, gobj);
 }
@@ -793,3 +839,40 @@ struct media_pad *media_entity_remote_pad(struct media_pad 
*pad)
 
 }
 EXPORT_SYMBOL_GPL(media_entity_remote_pad);
+
+
+/* Functions related to the media interface via device nodes */
+
+struct media_intf_devnode *media_devnode_create(struct media_device *mdev,
+   u32 type, u32 flags,
+   u32 major, u32 minor,
+   gfp_t gfp_flags)
+{
+   struct media_intf_devnode *devnode;
+   struct media_interface *intf;
+
+   devnode = kzalloc(sizeof(*devnode), gfp_flags);
+   if (!devnode)
+   return NULL;
+
+   intf = >intf;
+
+   intf->type = type;
+   intf->flags = flags;
+
+   devnode->major = major;
+   devnode->minor = minor;
+
+   media_gobj_init(mdev, MEDIA_GRAPH_INTF_DEVNODE,
+  >intf.graph_obj);
+
+   return devnode;
+}
+EXPORT_SYMBOL_GPL(media_devnode_create);
+
+void media_devnode_remove(struct media_intf_devnode *devnode)
+{
+   media_gobj_remove(>intf.graph_obj);
+   kfree(devnode);
+}
+EXPORT_SYMBOL_GPL(media_devnode_remove);
diff --git a/include/media/media-device.h b/include/media/media-device.h
index 05414e351f8e..3b14394d5701 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -44,6 +44,7 @@ struct device;
  * @entity_id: Unique ID used on the last entity registered
  * @pad_id:Unique ID used on the last pad registered
  * @link_id:   Unique ID used on the last link registered
+ * @intf_devnode_id: Unique ID used on the last interface devnode registered
  * @entities:  List of registered entities
  * @lock:  Entities list lock
  * @graph_mutex: Entities graph 

Re: [PATCH v8 14/55] [media] media: add functions to allow creating interfaces

[Mauro Carvalho Chehab]

Em Mon, 31 Aug 2015 12:20:43 +0200
Hans Verkuil  escreveu:

> On 08/30/2015 05:06 AM, Mauro Carvalho Chehab wrote:
> > Interfaces are different than entities: they represent a
> > Kernel<->userspace interaction, while entities represent a
> > piece of hardware/firmware/software that executes a function.
> > 
> > Let's distinguish them by creating a separate structure to
> > store the interfaces.
> > 
> > Latter patches should change the existing drivers and logic
> 
> s/Latter/Later/
> 
> FYI: you almost never use 'latter' in English texts. The only common use
> of 'latter' is in the phrase "the latter of two options" where 'latter' means
> 'second'.

I did a global replace on the patches, but perhaps the regex
expression I used didn't got this one.

> 
> > to split the current interface embedded inside the entity
> > structure (device nodes) into a separate object of the graph.
> > 
> > Signed-off-by: Mauro Carvalho Chehab 
> > 
> > diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> > index a23c93369a04..583666e2cc25 100644
> > --- a/drivers/media/media-entity.c
> > +++ b/drivers/media/media-entity.c
> > @@ -44,11 +44,41 @@ static inline const char *gobj_type(enum 
> > media_gobj_type type)
> > return "pad";
> > case MEDIA_GRAPH_LINK:
> > return "link";
> > +   case MEDIA_GRAPH_INTF_DEVNODE:
> > +   return "intf_devnode";
> 
> You use '-' below for 'v4l-subdev' and 'unknown-intf', so I think this too 
> should
> use '-'. My opinion though.

Gah, forgot this one ;)

> 
> It's a nitpick, so I give my Ack anyway:
> 
> Acked-by: Hans Verkuil 
> 
> Regards,
> 
>   Hans
> 
> > default:
> > return "unknown";
> > }
> >  }
> >  
> > +static inline const char *intf_type(struct media_interface *intf)
> > +{
> > +   switch (intf->type) {
> > +   case MEDIA_INTF_T_DVB_FE:
> > +   return "frontend";
> > +   case MEDIA_INTF_T_DVB_DEMUX:
> > +   return "demux";
> > +   case MEDIA_INTF_T_DVB_DVR:
> > +   return "DVR";
> > +   case MEDIA_INTF_T_DVB_CA:
> > +   return  "CA";
> > +   case MEDIA_INTF_T_DVB_NET:
> > +   return "dvbnet";
> > +   case MEDIA_INTF_T_V4L_VIDEO:
> > +   return "video";
> > +   case MEDIA_INTF_T_V4L_VBI:
> > +   return "vbi";
> > +   case MEDIA_INTF_T_V4L_RADIO:
> > +   return "radio";
> > +   case MEDIA_INTF_T_V4L_SUBDEV:
> > +   return "v4l2-subdev";
> > +   case MEDIA_INTF_T_V4L_SWRADIO:
> > +   return "swradio";
> > +   default:
> > +   return "unknown-intf";
> > +   }
> > +};
> > +
> >  static void dev_dbg_obj(const char *event_name,  struct media_gobj *gobj)
> >  {
> >  #if defined(DEBUG) || defined (CONFIG_DYNAMIC_DEBUG)
> > @@ -84,6 +114,19 @@ static void dev_dbg_obj(const char *event_name,  struct 
> > media_gobj *gobj)
> > "%s: id 0x%08x pad#%d: '%s':%d\n",
> > event_name, gobj->id, media_localid(gobj),
> > pad->entity->name, pad->index);
> > +   break;
> > +   }
> > +   case MEDIA_GRAPH_INTF_DEVNODE:
> > +   {
> > +   struct media_interface *intf = gobj_to_intf(gobj);
> > +   struct media_intf_devnode *devnode = intf_to_devnode(intf);
> > +
> > +   dev_dbg(gobj->mdev->dev,
> > +   "%s: id 0x%08x intf_devnode#%d: %s - major: %d, minor: 
> > %d\n",
> > +   event_name, gobj->id, media_localid(gobj),
> > +   intf_type(intf),
> > +   devnode->major, devnode->minor);
> > +   break;
> > }
> > }
> >  #endif
> > @@ -119,6 +162,9 @@ void media_gobj_init(struct media_device *mdev,
> > case MEDIA_GRAPH_LINK:
> > gobj->id = media_gobj_gen_id(type, ++mdev->link_id);
> > break;
> > +   case MEDIA_GRAPH_INTF_DEVNODE:
> > +   gobj->id = media_gobj_gen_id(type, ++mdev->intf_devnode_id);
> > +   break;
> > }
> > dev_dbg_obj(__func__, gobj);
> >  }
> > @@ -793,3 +839,40 @@ struct media_pad *media_entity_remote_pad(struct 
> > media_pad *pad)
> >  
> >  }
> >  EXPORT_SYMBOL_GPL(media_entity_remote_pad);
> > +
> > +
> > +/* Functions related to the media interface via device nodes */
> > +
> > +struct media_intf_devnode *media_devnode_create(struct media_device *mdev,
> > +   u32 type, u32 flags,
> > +   u32 major, u32 minor,
> > +   gfp_t gfp_flags)
> > +{
> > +   struct media_intf_devnode *devnode;
> > +   struct media_interface *intf;
> > +
> > +   devnode = kzalloc(sizeof(*devnode), gfp_flags);
> > +   if (!devnode)
> > +   return NULL;
> > +
> > +   intf = >intf;
> > +
> > +   intf->type = type;
> > +   intf->flags = flags;
> > +
> > +   devnode->major = major;
> > +   devnode->minor = minor;
> > +
> > +   

Re: [PATCH v8 14/55] [media] media: add functions to allow creating interfaces

[Hans Verkuil]

On 08/30/2015 05:06 AM, Mauro Carvalho Chehab wrote:
> Interfaces are different than entities: they represent a
> Kernel<->userspace interaction, while entities represent a
> piece of hardware/firmware/software that executes a function.
> 
> Let's distinguish them by creating a separate structure to
> store the interfaces.
> 
> Latter patches should change the existing drivers and logic

s/Latter/Later/

FYI: you almost never use 'latter' in English texts. The only common use
of 'latter' is in the phrase "the latter of two options" where 'latter' means
'second'.

> to split the current interface embedded inside the entity
> structure (device nodes) into a separate object of the graph.
> 
> Signed-off-by: Mauro Carvalho Chehab 
> 
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index a23c93369a04..583666e2cc25 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -44,11 +44,41 @@ static inline const char *gobj_type(enum media_gobj_type 
> type)
>   return "pad";
>   case MEDIA_GRAPH_LINK:
>   return "link";
> + case MEDIA_GRAPH_INTF_DEVNODE:
> + return "intf_devnode";

You use '-' below for 'v4l-subdev' and 'unknown-intf', so I think this too 
should
use '-'. My opinion though.

It's a nitpick, so I give my Ack anyway:

Acked-by: Hans Verkuil 

Regards,

Hans

>   default:
>   return "unknown";
>   }
>  }
>  
> +static inline const char *intf_type(struct media_interface *intf)
> +{
> + switch (intf->type) {
> + case MEDIA_INTF_T_DVB_FE:
> + return "frontend";
> + case MEDIA_INTF_T_DVB_DEMUX:
> + return "demux";
> + case MEDIA_INTF_T_DVB_DVR:
> + return "DVR";
> + case MEDIA_INTF_T_DVB_CA:
> + return  "CA";
> + case MEDIA_INTF_T_DVB_NET:
> + return "dvbnet";
> + case MEDIA_INTF_T_V4L_VIDEO:
> + return "video";
> + case MEDIA_INTF_T_V4L_VBI:
> + return "vbi";
> + case MEDIA_INTF_T_V4L_RADIO:
> + return "radio";
> + case MEDIA_INTF_T_V4L_SUBDEV:
> + return "v4l2-subdev";
> + case MEDIA_INTF_T_V4L_SWRADIO:
> + return "swradio";
> + default:
> + return "unknown-intf";
> + }
> +};
> +
>  static void dev_dbg_obj(const char *event_name,  struct media_gobj *gobj)
>  {
>  #if defined(DEBUG) || defined (CONFIG_DYNAMIC_DEBUG)
> @@ -84,6 +114,19 @@ static void dev_dbg_obj(const char *event_name,  struct 
> media_gobj *gobj)
>   "%s: id 0x%08x pad#%d: '%s':%d\n",
>   event_name, gobj->id, media_localid(gobj),
>   pad->entity->name, pad->index);
> + break;
> + }
> + case MEDIA_GRAPH_INTF_DEVNODE:
> + {
> + struct media_interface *intf = gobj_to_intf(gobj);
> + struct media_intf_devnode *devnode = intf_to_devnode(intf);
> +
> + dev_dbg(gobj->mdev->dev,
> + "%s: id 0x%08x intf_devnode#%d: %s - major: %d, minor: 
> %d\n",
> + event_name, gobj->id, media_localid(gobj),
> + intf_type(intf),
> + devnode->major, devnode->minor);
> + break;
>   }
>   }
>  #endif
> @@ -119,6 +162,9 @@ void media_gobj_init(struct media_device *mdev,
>   case MEDIA_GRAPH_LINK:
>   gobj->id = media_gobj_gen_id(type, ++mdev->link_id);
>   break;
> + case MEDIA_GRAPH_INTF_DEVNODE:
> + gobj->id = media_gobj_gen_id(type, ++mdev->intf_devnode_id);
> + break;
>   }
>   dev_dbg_obj(__func__, gobj);
>  }
> @@ -793,3 +839,40 @@ struct media_pad *media_entity_remote_pad(struct 
> media_pad *pad)
>  
>  }
>  EXPORT_SYMBOL_GPL(media_entity_remote_pad);
> +
> +
> +/* Functions related to the media interface via device nodes */
> +
> +struct media_intf_devnode *media_devnode_create(struct media_device *mdev,
> + u32 type, u32 flags,
> + u32 major, u32 minor,
> + gfp_t gfp_flags)
> +{
> + struct media_intf_devnode *devnode;
> + struct media_interface *intf;
> +
> + devnode = kzalloc(sizeof(*devnode), gfp_flags);
> + if (!devnode)
> + return NULL;
> +
> + intf = >intf;
> +
> + intf->type = type;
> + intf->flags = flags;
> +
> + devnode->major = major;
> + devnode->minor = minor;
> +
> + media_gobj_init(mdev, MEDIA_GRAPH_INTF_DEVNODE,
> +>intf.graph_obj);
> +
> + return devnode;
> +}
> +EXPORT_SYMBOL_GPL(media_devnode_create);
> +
> +void media_devnode_remove(struct media_intf_devnode *devnode)
> +{
> + media_gobj_remove(>intf.graph_obj);
> + kfree(devnode);
> +}
> +EXPORT_SYMBOL_GPL(media_devnode_remove);
> diff 

[PATCH v8 14/55] [media] media: add functions to allow creating interfaces

[Mauro Carvalho Chehab]

Interfaces are different than entities: they represent a
Kernel-userspace interaction, while entities represent a
piece of hardware/firmware/software that executes a function.

Let's distinguish them by creating a separate structure to
store the interfaces.

Latter patches should change the existing drivers and logic
to split the current interface embedded inside the entity
structure (device nodes) into a separate object of the graph.

Signed-off-by: Mauro Carvalho Chehab mche...@osg.samsung.com

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index a23c93369a04..583666e2cc25 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -44,11 +44,41 @@ static inline const char *gobj_type(enum media_gobj_type 
type)
return pad;
case MEDIA_GRAPH_LINK:
return link;
+   case MEDIA_GRAPH_INTF_DEVNODE:
+   return intf_devnode;
default:
return unknown;
}
 }
 
+static inline const char *intf_type(struct media_interface *intf)
+{
+   switch (intf-type) {
+   case MEDIA_INTF_T_DVB_FE:
+   return frontend;
+   case MEDIA_INTF_T_DVB_DEMUX:
+   return demux;
+   case MEDIA_INTF_T_DVB_DVR:
+   return DVR;
+   case MEDIA_INTF_T_DVB_CA:
+   return  CA;
+   case MEDIA_INTF_T_DVB_NET:
+   return dvbnet;
+   case MEDIA_INTF_T_V4L_VIDEO:
+   return video;
+   case MEDIA_INTF_T_V4L_VBI:
+   return vbi;
+   case MEDIA_INTF_T_V4L_RADIO:
+   return radio;
+   case MEDIA_INTF_T_V4L_SUBDEV:
+   return v4l2-subdev;
+   case MEDIA_INTF_T_V4L_SWRADIO:
+   return swradio;
+   default:
+   return unknown-intf;
+   }
+};
+
 static void dev_dbg_obj(const char *event_name,  struct media_gobj *gobj)
 {
 #if defined(DEBUG) || defined (CONFIG_DYNAMIC_DEBUG)
@@ -84,6 +114,19 @@ static void dev_dbg_obj(const char *event_name,  struct 
media_gobj *gobj)
%s: id 0x%08x pad#%d: '%s':%d\n,
event_name, gobj-id, media_localid(gobj),
pad-entity-name, pad-index);
+   break;
+   }
+   case MEDIA_GRAPH_INTF_DEVNODE:
+   {
+   struct media_interface *intf = gobj_to_intf(gobj);
+   struct media_intf_devnode *devnode = intf_to_devnode(intf);
+
+   dev_dbg(gobj-mdev-dev,
+   %s: id 0x%08x intf_devnode#%d: %s - major: %d, minor: 
%d\n,
+   event_name, gobj-id, media_localid(gobj),
+   intf_type(intf),
+   devnode-major, devnode-minor);
+   break;
}
}
 #endif
@@ -119,6 +162,9 @@ void media_gobj_init(struct media_device *mdev,
case MEDIA_GRAPH_LINK:
gobj-id = media_gobj_gen_id(type, ++mdev-link_id);
break;
+   case MEDIA_GRAPH_INTF_DEVNODE:
+   gobj-id = media_gobj_gen_id(type, ++mdev-intf_devnode_id);
+   break;
}
dev_dbg_obj(__func__, gobj);
 }
@@ -793,3 +839,40 @@ struct media_pad *media_entity_remote_pad(struct media_pad 
*pad)
 
 }
 EXPORT_SYMBOL_GPL(media_entity_remote_pad);
+
+
+/* Functions related to the media interface via device nodes */
+
+struct media_intf_devnode *media_devnode_create(struct media_device *mdev,
+   u32 type, u32 flags,
+   u32 major, u32 minor,
+   gfp_t gfp_flags)
+{
+   struct media_intf_devnode *devnode;
+   struct media_interface *intf;
+
+   devnode = kzalloc(sizeof(*devnode), gfp_flags);
+   if (!devnode)
+   return NULL;
+
+   intf = devnode-intf;
+
+   intf-type = type;
+   intf-flags = flags;
+
+   devnode-major = major;
+   devnode-minor = minor;
+
+   media_gobj_init(mdev, MEDIA_GRAPH_INTF_DEVNODE,
+  devnode-intf.graph_obj);
+
+   return devnode;
+}
+EXPORT_SYMBOL_GPL(media_devnode_create);
+
+void media_devnode_remove(struct media_intf_devnode *devnode)
+{
+   media_gobj_remove(devnode-intf.graph_obj);
+   kfree(devnode);
+}
+EXPORT_SYMBOL_GPL(media_devnode_remove);
diff --git a/include/media/media-device.h b/include/media/media-device.h
index 05414e351f8e..3b14394d5701 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -44,6 +44,7 @@ struct device;
  * @entity_id: Unique ID used on the last entity registered
  * @pad_id:Unique ID used on the last pad registered
  * @link_id:   Unique ID used on the last link registered
+ * @intf_devnode_id: Unique ID used on the last interface devnode registered
  * @entities:  List of registered entities
  * @lock:  Entities list lock
  * @graph_mutex: Entities graph operation lock
@@ -73,6 +74,7 @@ struct