linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/2] media: entity: add operation to help map DT node to media pad
@ 2017-05-24  0:09 Niklas Söderlund
  2017-05-24  0:09 ` [PATCH v2 1/2] media: entity: Add pad_from_fwnode entity operation Niklas Söderlund
  2017-05-24  0:09 ` [PATCH v2 2/2] media: entity: Add media_entity_pad_from_fwnode() function Niklas Söderlund
  0 siblings, 2 replies; 9+ messages in thread
From: Niklas Söderlund @ 2017-05-24  0:09 UTC (permalink / raw)
  To: Mauro Carvalho Chehab, Sakari Ailus, linux-media
  Cc: Kieran Bingham, linux-renesas-soc, Laurent Pinchart,
	Niklas Söderlund

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Hi,

This series add a new entity operation which will aid capture
drivers to map a port/endpoint in DT to a media graph pad.

This series is implemented support for the ongoing ADV748x work by 
Kieran Bingham. In his work he have a driver which registers more then 
one subdevice. So when a driver finds this subdevice it must be able to 
ask the subdevice itself which pad number correspond to the DT endpoint 
the driver used to bind subdevice in the first place.

This is tested on Renesas H3 and M3-W together with the Renesas CSI-2 
and VIN Gen3 driver (posted separately). It is based on top of Sakaris 
pull request '[GIT PULL FOR v4.13] V4L2 fwnode support'.

* Changes since v1
- Rebased work ontop of Sakaris fwnode branch and make use of the fwnode 
  instead of the raw DT port/reg numbers.
- Do not assume DT port is equal to pad number if the driver do not 
  implement the lookup function. Instead search for the first pad with 
  the correct direction and use that. Thanks Sakari for the suggestion!
- Use ENXIO instead of EINVAL to signal lookup error.

Niklas Söderlund (2):
  media: entity: Add pad_from_fwnode entity operation
  media: entity: Add media_entity_pad_from_fwnode() function

 drivers/media/media-entity.c | 39 +++++++++++++++++++++++++++++++++++++++
 include/media/media-entity.h | 28 ++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+)

-- 
2.13.0

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 1/2] media: entity: Add pad_from_fwnode entity operation
  2017-05-24  0:09 [PATCH v2 0/2] media: entity: add operation to help map DT node to media pad Niklas Söderlund
@ 2017-05-24  0:09 ` Niklas Söderlund
  2017-05-24 13:21   ` Sakari Ailus
  2017-05-24  0:09 ` [PATCH v2 2/2] media: entity: Add media_entity_pad_from_fwnode() function Niklas Söderlund
  1 sibling, 1 reply; 9+ messages in thread
From: Niklas Söderlund @ 2017-05-24  0:09 UTC (permalink / raw)
  To: Mauro Carvalho Chehab, Sakari Ailus, linux-media
  Cc: Kieran Bingham, linux-renesas-soc, Laurent Pinchart,
	Niklas Söderlund

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

The optional operation can be used by entities to report how it maps its
fwnode endpoints to media pad numbers. This is useful for devices which
require advanced mappings of pads.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 include/media/media-entity.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index c7c254c5bca1761b..2aea22b0409d1070 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -21,6 +21,7 @@
 
 #include <linux/bitmap.h>
 #include <linux/bug.h>
+#include <linux/fwnode.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/media.h>
@@ -171,6 +172,9 @@ struct media_pad {
 
 /**
  * struct media_entity_operations - Media entity operations
+ * @pad_from_fwnode:	Return the pad number based on a fwnode endpoint.
+ *			This operation can be used to map a fwnode to a
+ *			media pad number. Optional.
  * @link_setup:		Notify the entity of link changes. The operation can
  *			return an error, in which case link setup will be
  *			cancelled. Optional.
@@ -184,6 +188,8 @@ struct media_pad {
  *    mutex held.
  */
 struct media_entity_operations {
+	int (*pad_from_fwnode)(struct fwnode_endpoint *endpoint,
+			       unsigned int *pad);
 	int (*link_setup)(struct media_entity *entity,
 			  const struct media_pad *local,
 			  const struct media_pad *remote, u32 flags);
-- 
2.13.0

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 2/2] media: entity: Add media_entity_pad_from_fwnode() function
  2017-05-24  0:09 [PATCH v2 0/2] media: entity: add operation to help map DT node to media pad Niklas Söderlund
  2017-05-24  0:09 ` [PATCH v2 1/2] media: entity: Add pad_from_fwnode entity operation Niklas Söderlund
@ 2017-05-24  0:09 ` Niklas Söderlund
  2017-05-24 13:27   ` Sakari Ailus
  2017-05-29 10:32   ` Hans Verkuil
  1 sibling, 2 replies; 9+ messages in thread
From: Niklas Söderlund @ 2017-05-24  0:09 UTC (permalink / raw)
  To: Mauro Carvalho Chehab, Sakari Ailus, linux-media
  Cc: Kieran Bingham, linux-renesas-soc, Laurent Pinchart,
	Niklas Söderlund

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

This is a wrapper around the media entity pad_from_fwnode operation.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/media-entity.c | 39 +++++++++++++++++++++++++++++++++++++++
 include/media/media-entity.h | 22 ++++++++++++++++++++++
 2 files changed, 61 insertions(+)

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index bc44193efa4798b4..c124754f739a8b94 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -18,6 +18,7 @@
 
 #include <linux/bitmap.h>
 #include <linux/module.h>
+#include <linux/property.h>
 #include <linux/slab.h>
 #include <media/media-entity.h>
 #include <media/media-device.h>
@@ -386,6 +387,44 @@ struct media_entity *media_graph_walk_next(struct media_graph *graph)
 }
 EXPORT_SYMBOL_GPL(media_graph_walk_next);
 
+int media_entity_pad_from_fwnode(struct media_entity *entity,
+				 struct fwnode_handle *fwnode,
+				 int direction, unsigned int *pad)
+{
+	struct fwnode_endpoint endpoint;
+	int i, tmp, ret;
+
+	if (!entity->ops || !entity->ops->pad_from_fwnode) {
+		for (i = 0; i < entity->num_pads; i++) {
+			if (entity->pads[i].flags & direction) {
+				*pad = i;
+				return 0;
+			}
+		}
+
+		return -ENXIO;
+	}
+
+	ret = fwnode_graph_parse_endpoint(fwnode, &endpoint);
+	if (ret)
+		return ret;
+
+	ret = entity->ops->pad_from_fwnode(&endpoint, &tmp);
+	if (ret)
+		return ret;
+
+	if (tmp >= entity->num_pads)
+		return -ENXIO;
+
+	if (!(entity->pads[tmp].flags & direction))
+		return -ENXIO;
+
+	*pad = tmp;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(media_entity_pad_from_fwnode);
+
 /* -----------------------------------------------------------------------------
  * Pipeline management
  */
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index 2aea22b0409d1070..7507181609bec43c 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -822,6 +822,28 @@ struct media_pad *media_entity_remote_pad(struct media_pad *pad);
 struct media_entity *media_entity_get(struct media_entity *entity);
 
 /**
+ * media_entity_pad_from_fwnode - Get pad number from fwnode
+ *
+ * @entity: The entity
+ * @fwnode: Pointer to fwnode_handle which should be used to find pad
+ * @direction: Expected direction of the pad
+ * @pad: Pointer to pad which will should be filled in
+ *
+ * This function can be used to resolve the media pad number from
+ * a fwnode. This is useful for devices which uses more complex
+ * mappings of media pads.
+ *
+ * If the entity do not implement the pad_from_fwnode() operation
+ * this function searches the entity for the first pad that matches
+ * the @direction.
+ *
+ * Return: return 0 on success.
+ */
+int media_entity_pad_from_fwnode(struct media_entity *entity,
+				 struct fwnode_handle *fwnode,
+				 int direction, unsigned int *pad);
+
+/**
  * media_graph_walk_init - Allocate resources used by graph walk.
  *
  * @graph: Media graph structure that will be used to walk the graph
-- 
2.13.0

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/2] media: entity: Add pad_from_fwnode entity operation
  2017-05-24  0:09 ` [PATCH v2 1/2] media: entity: Add pad_from_fwnode entity operation Niklas Söderlund
@ 2017-05-24 13:21   ` Sakari Ailus
  2017-05-24 14:07     ` Niklas Söderlund
  0 siblings, 1 reply; 9+ messages in thread
From: Sakari Ailus @ 2017-05-24 13:21 UTC (permalink / raw)
  To: Niklas Söderlund
  Cc: Mauro Carvalho Chehab, Sakari Ailus, linux-media, Kieran Bingham,
	linux-renesas-soc, Laurent Pinchart, Niklas Söderlund

Hi Niklas,

On Wed, May 24, 2017 at 02:09:06AM +0200, Niklas Söderlund wrote:
> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> The optional operation can be used by entities to report how it maps its
> fwnode endpoints to media pad numbers. This is useful for devices which
> require advanced mappings of pads.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  include/media/media-entity.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index c7c254c5bca1761b..2aea22b0409d1070 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -21,6 +21,7 @@
>  
>  #include <linux/bitmap.h>
>  #include <linux/bug.h>
> +#include <linux/fwnode.h>
>  #include <linux/kernel.h>
>  #include <linux/list.h>
>  #include <linux/media.h>
> @@ -171,6 +172,9 @@ struct media_pad {
>  
>  /**
>   * struct media_entity_operations - Media entity operations
> + * @pad_from_fwnode:	Return the pad number based on a fwnode endpoint.
> + *			This operation can be used to map a fwnode to a
> + *			media pad number. Optional.
>   * @link_setup:		Notify the entity of link changes. The operation can
>   *			return an error, in which case link setup will be
>   *			cancelled. Optional.
> @@ -184,6 +188,8 @@ struct media_pad {
>   *    mutex held.
>   */
>  struct media_entity_operations {
> +	int (*pad_from_fwnode)(struct fwnode_endpoint *endpoint,
> +			       unsigned int *pad);

Hmm. How about calling this get_fwnode_pad for instance? I wonder what
others think.

You could just return the pad number still, and a negative value on error. I
think we won't have more than INT_MAX pads. :-)

>  	int (*link_setup)(struct media_entity *entity,
>  			  const struct media_pad *local,
>  			  const struct media_pad *remote, u32 flags);

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] media: entity: Add media_entity_pad_from_fwnode() function
  2017-05-24  0:09 ` [PATCH v2 2/2] media: entity: Add media_entity_pad_from_fwnode() function Niklas Söderlund
@ 2017-05-24 13:27   ` Sakari Ailus
  2017-05-24 14:09     ` Niklas Söderlund
  2017-05-29 10:32   ` Hans Verkuil
  1 sibling, 1 reply; 9+ messages in thread
From: Sakari Ailus @ 2017-05-24 13:27 UTC (permalink / raw)
  To: Niklas Söderlund
  Cc: Mauro Carvalho Chehab, Sakari Ailus, linux-media, Kieran Bingham,
	linux-renesas-soc, Laurent Pinchart, Niklas Söderlund

Hi Niklas,

On Wed, May 24, 2017 at 02:09:07AM +0200, Niklas Söderlund wrote:
> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> This is a wrapper around the media entity pad_from_fwnode operation.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/media-entity.c | 39 +++++++++++++++++++++++++++++++++++++++
>  include/media/media-entity.h | 22 ++++++++++++++++++++++
>  2 files changed, 61 insertions(+)
> 
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index bc44193efa4798b4..c124754f739a8b94 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -18,6 +18,7 @@
>  
>  #include <linux/bitmap.h>
>  #include <linux/module.h>
> +#include <linux/property.h>
>  #include <linux/slab.h>
>  #include <media/media-entity.h>
>  #include <media/media-device.h>
> @@ -386,6 +387,44 @@ struct media_entity *media_graph_walk_next(struct media_graph *graph)
>  }
>  EXPORT_SYMBOL_GPL(media_graph_walk_next);
>  
> +int media_entity_pad_from_fwnode(struct media_entity *entity,
> +				 struct fwnode_handle *fwnode,
> +				 int direction, unsigned int *pad)
> +{
> +	struct fwnode_endpoint endpoint;
> +	int i, tmp, ret;
> +
> +	if (!entity->ops || !entity->ops->pad_from_fwnode) {
> +		for (i = 0; i < entity->num_pads; i++) {
> +			if (entity->pads[i].flags & direction) {
> +				*pad = i;
> +				return 0;
> +			}
> +		}
> +
> +		return -ENXIO;
> +	}
> +
> +	ret = fwnode_graph_parse_endpoint(fwnode, &endpoint);
> +	if (ret)
> +		return ret;
> +
> +	ret = entity->ops->pad_from_fwnode(&endpoint, &tmp);
> +	if (ret)
> +		return ret;
> +
> +	if (tmp >= entity->num_pads)
> +		return -ENXIO;
> +
> +	if (!(entity->pads[tmp].flags & direction))
> +		return -ENXIO;
> +
> +	*pad = tmp;
> +
> +	return 0;

I'd just return the pad number to the caller.

> +}
> +EXPORT_SYMBOL_GPL(media_entity_pad_from_fwnode);
> +
>  /* -----------------------------------------------------------------------------
>   * Pipeline management
>   */
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index 2aea22b0409d1070..7507181609bec43c 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -822,6 +822,28 @@ struct media_pad *media_entity_remote_pad(struct media_pad *pad);
>  struct media_entity *media_entity_get(struct media_entity *entity);
>  
>  /**
> + * media_entity_pad_from_fwnode - Get pad number from fwnode
> + *
> + * @entity: The entity
> + * @fwnode: Pointer to fwnode_handle which should be used to find pad
> + * @direction: Expected direction of the pad

You should document the possible values for this. What would you think about
using a bool called e.g. is_sink? I don't have a strong opinion either way
though.

> + * @pad: Pointer to pad which will should be filled in
> + *
> + * This function can be used to resolve the media pad number from
> + * a fwnode. This is useful for devices which uses more complex
> + * mappings of media pads.
> + *
> + * If the entity do not implement the pad_from_fwnode() operation
> + * this function searches the entity for the first pad that matches
> + * the @direction.
> + *
> + * Return: return 0 on success.
> + */
> +int media_entity_pad_from_fwnode(struct media_entity *entity,
> +				 struct fwnode_handle *fwnode,
> +				 int direction, unsigned int *pad);
> +
> +/**
>   * media_graph_walk_init - Allocate resources used by graph walk.
>   *
>   * @graph: Media graph structure that will be used to walk the graph

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/2] media: entity: Add pad_from_fwnode entity operation
  2017-05-24 13:21   ` Sakari Ailus
@ 2017-05-24 14:07     ` Niklas Söderlund
  2017-05-26 21:52       ` Sakari Ailus
  0 siblings, 1 reply; 9+ messages in thread
From: Niklas Söderlund @ 2017-05-24 14:07 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Mauro Carvalho Chehab, Sakari Ailus, linux-media, Kieran Bingham,
	linux-renesas-soc, Laurent Pinchart

Hi Sakari,

Thanks for your feedback.

On 2017-05-24 16:21:37 +0300, Sakari Ailus wrote:
> Hi Niklas,
> 
> On Wed, May 24, 2017 at 02:09:06AM +0200, Niklas Söderlund wrote:
> > From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > 
> > The optional operation can be used by entities to report how it maps its
> > fwnode endpoints to media pad numbers. This is useful for devices which
> > require advanced mappings of pads.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  include/media/media-entity.h | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > index c7c254c5bca1761b..2aea22b0409d1070 100644
> > --- a/include/media/media-entity.h
> > +++ b/include/media/media-entity.h
> > @@ -21,6 +21,7 @@
> >  
> >  #include <linux/bitmap.h>
> >  #include <linux/bug.h>
> > +#include <linux/fwnode.h>
> >  #include <linux/kernel.h>
> >  #include <linux/list.h>
> >  #include <linux/media.h>
> > @@ -171,6 +172,9 @@ struct media_pad {
> >  
> >  /**
> >   * struct media_entity_operations - Media entity operations
> > + * @pad_from_fwnode:	Return the pad number based on a fwnode endpoint.
> > + *			This operation can be used to map a fwnode to a
> > + *			media pad number. Optional.
> >   * @link_setup:		Notify the entity of link changes. The operation can
> >   *			return an error, in which case link setup will be
> >   *			cancelled. Optional.
> > @@ -184,6 +188,8 @@ struct media_pad {
> >   *    mutex held.
> >   */
> >  struct media_entity_operations {
> > +	int (*pad_from_fwnode)(struct fwnode_endpoint *endpoint,
> > +			       unsigned int *pad);
> 
> Hmm. How about calling this get_fwnode_pad for instance? I wonder what
> others think.

I'm OK with this name change, will update for next version.

> 
> You could just return the pad number still, and a negative value on error. I
> think we won't have more than INT_MAX pads. :-)

I did that at first but then I remembered all the review comments I have 
gotten earlier about using int as the type for pads :-) If you and 
others agree in this case returning the pad as int or a negative value 
as error I have no problem chaining this for the next version.

> 
> >  	int (*link_setup)(struct media_entity *entity,
> >  			  const struct media_pad *local,
> >  			  const struct media_pad *remote, u32 flags);
> 
> -- 
> Regards,
> 
> Sakari Ailus
> e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

-- 
Regards,
Niklas Söderlund

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] media: entity: Add media_entity_pad_from_fwnode() function
  2017-05-24 13:27   ` Sakari Ailus
@ 2017-05-24 14:09     ` Niklas Söderlund
  0 siblings, 0 replies; 9+ messages in thread
From: Niklas Söderlund @ 2017-05-24 14:09 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Mauro Carvalho Chehab, Sakari Ailus, linux-media, Kieran Bingham,
	linux-renesas-soc, Laurent Pinchart

Hi Sakari,

Thanks for your feedback.

On 2017-05-24 16:27:42 +0300, Sakari Ailus wrote:
> Hi Niklas,
> 
> On Wed, May 24, 2017 at 02:09:07AM +0200, Niklas Söderlund wrote:
> > From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > 
> > This is a wrapper around the media entity pad_from_fwnode operation.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/media-entity.c | 39 +++++++++++++++++++++++++++++++++++++++
> >  include/media/media-entity.h | 22 ++++++++++++++++++++++
> >  2 files changed, 61 insertions(+)
> > 
> > diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> > index bc44193efa4798b4..c124754f739a8b94 100644
> > --- a/drivers/media/media-entity.c
> > +++ b/drivers/media/media-entity.c
> > @@ -18,6 +18,7 @@
> >  
> >  #include <linux/bitmap.h>
> >  #include <linux/module.h>
> > +#include <linux/property.h>
> >  #include <linux/slab.h>
> >  #include <media/media-entity.h>
> >  #include <media/media-device.h>
> > @@ -386,6 +387,44 @@ struct media_entity *media_graph_walk_next(struct media_graph *graph)
> >  }
> >  EXPORT_SYMBOL_GPL(media_graph_walk_next);
> >  
> > +int media_entity_pad_from_fwnode(struct media_entity *entity,
> > +				 struct fwnode_handle *fwnode,
> > +				 int direction, unsigned int *pad)
> > +{
> > +	struct fwnode_endpoint endpoint;
> > +	int i, tmp, ret;
> > +
> > +	if (!entity->ops || !entity->ops->pad_from_fwnode) {
> > +		for (i = 0; i < entity->num_pads; i++) {
> > +			if (entity->pads[i].flags & direction) {
> > +				*pad = i;
> > +				return 0;
> > +			}
> > +		}
> > +
> > +		return -ENXIO;
> > +	}
> > +
> > +	ret = fwnode_graph_parse_endpoint(fwnode, &endpoint);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = entity->ops->pad_from_fwnode(&endpoint, &tmp);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (tmp >= entity->num_pads)
> > +		return -ENXIO;
> > +
> > +	if (!(entity->pads[tmp].flags & direction))
> > +		return -ENXIO;
> > +
> > +	*pad = tmp;
> > +
> > +	return 0;
> 
> I'd just return the pad number to the caller.

Depending on if there is any comments about this in the previous patch I 
switch to returning an int from this function, where a negative value 
would signal an error.

> 
> > +}
> > +EXPORT_SYMBOL_GPL(media_entity_pad_from_fwnode);
> > +
> >  /* -----------------------------------------------------------------------------
> >   * Pipeline management
> >   */
> > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > index 2aea22b0409d1070..7507181609bec43c 100644
> > --- a/include/media/media-entity.h
> > +++ b/include/media/media-entity.h
> > @@ -822,6 +822,28 @@ struct media_pad *media_entity_remote_pad(struct media_pad *pad);
> >  struct media_entity *media_entity_get(struct media_entity *entity);
> >  
> >  /**
> > + * media_entity_pad_from_fwnode - Get pad number from fwnode
> > + *
> > + * @entity: The entity
> > + * @fwnode: Pointer to fwnode_handle which should be used to find pad
> > + * @direction: Expected direction of the pad
> 
> You should document the possible values for this. What would you think about
> using a bool called e.g. is_sink? I don't have a strong opinion either way
> though.

Thanks, I think it is better to add documentation about possible values 
here. If no one else disagrees whit this I will do so in the next 
version.

> 
> > + * @pad: Pointer to pad which will should be filled in
> > + *
> > + * This function can be used to resolve the media pad number from
> > + * a fwnode. This is useful for devices which uses more complex
> > + * mappings of media pads.
> > + *
> > + * If the entity do not implement the pad_from_fwnode() operation
> > + * this function searches the entity for the first pad that matches
> > + * the @direction.
> > + *
> > + * Return: return 0 on success.
> > + */
> > +int media_entity_pad_from_fwnode(struct media_entity *entity,
> > +				 struct fwnode_handle *fwnode,
> > +				 int direction, unsigned int *pad);
> > +
> > +/**
> >   * media_graph_walk_init - Allocate resources used by graph walk.
> >   *
> >   * @graph: Media graph structure that will be used to walk the graph
> 
> -- 
> Regards,
> 
> Sakari Ailus
> e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

-- 
Regards,
Niklas Söderlund

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/2] media: entity: Add pad_from_fwnode entity operation
  2017-05-24 14:07     ` Niklas Söderlund
@ 2017-05-26 21:52       ` Sakari Ailus
  0 siblings, 0 replies; 9+ messages in thread
From: Sakari Ailus @ 2017-05-26 21:52 UTC (permalink / raw)
  To: Niklas Söderlund
  Cc: Mauro Carvalho Chehab, Sakari Ailus, linux-media, Kieran Bingham,
	linux-renesas-soc, Laurent Pinchart

Hejssan,

On Wed, May 24, 2017 at 04:07:36PM +0200, Niklas Söderlund wrote:
> Hi Sakari,
> 
> Thanks for your feedback.
> 
> On 2017-05-24 16:21:37 +0300, Sakari Ailus wrote:
> > Hi Niklas,
> > 
> > On Wed, May 24, 2017 at 02:09:06AM +0200, Niklas Söderlund wrote:
> > > From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > 
> > > The optional operation can be used by entities to report how it maps its
> > > fwnode endpoints to media pad numbers. This is useful for devices which
> > > require advanced mappings of pads.
> > > 
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > ---
> > >  include/media/media-entity.h | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > > index c7c254c5bca1761b..2aea22b0409d1070 100644
> > > --- a/include/media/media-entity.h
> > > +++ b/include/media/media-entity.h
> > > @@ -21,6 +21,7 @@
> > >  
> > >  #include <linux/bitmap.h>
> > >  #include <linux/bug.h>
> > > +#include <linux/fwnode.h>
> > >  #include <linux/kernel.h>
> > >  #include <linux/list.h>
> > >  #include <linux/media.h>
> > > @@ -171,6 +172,9 @@ struct media_pad {
> > >  
> > >  /**
> > >   * struct media_entity_operations - Media entity operations
> > > + * @pad_from_fwnode:	Return the pad number based on a fwnode endpoint.
> > > + *			This operation can be used to map a fwnode to a
> > > + *			media pad number. Optional.
> > >   * @link_setup:		Notify the entity of link changes. The operation can
> > >   *			return an error, in which case link setup will be
> > >   *			cancelled. Optional.
> > > @@ -184,6 +188,8 @@ struct media_pad {
> > >   *    mutex held.
> > >   */
> > >  struct media_entity_operations {
> > > +	int (*pad_from_fwnode)(struct fwnode_endpoint *endpoint,
> > > +			       unsigned int *pad);
> > 
> > Hmm. How about calling this get_fwnode_pad for instance? I wonder what
> > others think.
> 
> I'm OK with this name change, will update for next version.
> 
> > 
> > You could just return the pad number still, and a negative value on error. I
> > think we won't have more than INT_MAX pads. :-)
> 
> I did that at first but then I remembered all the review comments I have 
> gotten earlier about using int as the type for pads :-) If you and 
> others agree in this case returning the pad as int or a negative value 
> as error I have no problem chaining this for the next version.

unsigned int was proposed for there was no need for negative values. In this
case there is.

I don't really have a strong opinion either way. I wonder what Hans or
Laurent thinks.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] media: entity: Add media_entity_pad_from_fwnode() function
  2017-05-24  0:09 ` [PATCH v2 2/2] media: entity: Add media_entity_pad_from_fwnode() function Niklas Söderlund
  2017-05-24 13:27   ` Sakari Ailus
@ 2017-05-29 10:32   ` Hans Verkuil
  1 sibling, 0 replies; 9+ messages in thread
From: Hans Verkuil @ 2017-05-29 10:32 UTC (permalink / raw)
  To: Niklas Söderlund, Mauro Carvalho Chehab, Sakari Ailus,
	linux-media
  Cc: Kieran Bingham, linux-renesas-soc, Laurent Pinchart,
	Niklas Söderlund

On 05/24/2017 02:09 AM, Niklas Söderlund wrote:
> From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> This is a wrapper around the media entity pad_from_fwnode operation.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>   drivers/media/media-entity.c | 39 +++++++++++++++++++++++++++++++++++++++
>   include/media/media-entity.h | 22 ++++++++++++++++++++++
>   2 files changed, 61 insertions(+)
> 
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index bc44193efa4798b4..c124754f739a8b94 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -18,6 +18,7 @@
>   
>   #include <linux/bitmap.h>
>   #include <linux/module.h>
> +#include <linux/property.h>
>   #include <linux/slab.h>
>   #include <media/media-entity.h>
>   #include <media/media-device.h>
> @@ -386,6 +387,44 @@ struct media_entity *media_graph_walk_next(struct media_graph *graph)
>   }
>   EXPORT_SYMBOL_GPL(media_graph_walk_next);
>   
> +int media_entity_pad_from_fwnode(struct media_entity *entity,
> +				 struct fwnode_handle *fwnode,
> +				 int direction, unsigned int *pad)

I'd use unsigned int or u32 for the direction: it's a bitmask so 'int'
is not really appropriate.

> +{
> +	struct fwnode_endpoint endpoint;
> +	int i, tmp, ret;
> +
> +	if (!entity->ops || !entity->ops->pad_from_fwnode) {
> +		for (i = 0; i < entity->num_pads; i++) {
> +			if (entity->pads[i].flags & direction) {
> +				*pad = i;
> +				return 0;
> +			}
> +		}
> +
> +		return -ENXIO;
> +	}
> +
> +	ret = fwnode_graph_parse_endpoint(fwnode, &endpoint);
> +	if (ret)
> +		return ret;
> +
> +	ret = entity->ops->pad_from_fwnode(&endpoint, &tmp);
> +	if (ret)
> +		return ret;
> +
> +	if (tmp >= entity->num_pads)
> +		return -ENXIO;
> +
> +	if (!(entity->pads[tmp].flags & direction))
> +		return -ENXIO;
> +
> +	*pad = tmp;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(media_entity_pad_from_fwnode);
> +
>   /* -----------------------------------------------------------------------------
>    * Pipeline management
>    */
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index 2aea22b0409d1070..7507181609bec43c 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -822,6 +822,28 @@ struct media_pad *media_entity_remote_pad(struct media_pad *pad);
>   struct media_entity *media_entity_get(struct media_entity *entity);
>   
>   /**
> + * media_entity_pad_from_fwnode - Get pad number from fwnode
> + *
> + * @entity: The entity
> + * @fwnode: Pointer to fwnode_handle which should be used to find pad

* @fwnode: Pointer to the fwnode_handle which should be used to find the pad

> + * @direction: Expected direction of the pad
> + * @pad: Pointer to pad which will should be filled in

* @pad: Pointer to the pad which will be filled in

> + *
> + * This function can be used to resolve the media pad number from
> + * a fwnode. This is useful for devices which uses more complex

uses -> use

> + * mappings of media pads.
> + *
> + * If the entity do not implement the pad_from_fwnode() operation

do -> does

> + * this function searches the entity for the first pad that matches

this -> then this

> + * the @direction.
> + *
> + * Return: return 0 on success.
> + */
> +int media_entity_pad_from_fwnode(struct media_entity *entity,
> +				 struct fwnode_handle *fwnode,
> +				 int direction, unsigned int *pad);
> +
> +/**
>    * media_graph_walk_init - Allocate resources used by graph walk.
>    *
>    * @graph: Media graph structure that will be used to walk the graph
> 

Regards,

	Hans

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2017-05-29 10:32 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-05-24  0:09 [PATCH v2 0/2] media: entity: add operation to help map DT node to media pad Niklas Söderlund
2017-05-24  0:09 ` [PATCH v2 1/2] media: entity: Add pad_from_fwnode entity operation Niklas Söderlund
2017-05-24 13:21   ` Sakari Ailus
2017-05-24 14:07     ` Niklas Söderlund
2017-05-26 21:52       ` Sakari Ailus
2017-05-24  0:09 ` [PATCH v2 2/2] media: entity: Add media_entity_pad_from_fwnode() function Niklas Söderlund
2017-05-24 13:27   ` Sakari Ailus
2017-05-24 14:09     ` Niklas Söderlund
2017-05-29 10:32   ` Hans Verkuil

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).