[PATCH 0/2] media: entity: add operation to help map DT node to media pad

[PATCH 0/2] media: entity: add operation to help map DT node to media pad

[Niklas Söderlund]

Hi,

This small series add a new entity operation which will aid capture 
drivers to map a port/endpoint in DT to a media graph pad. I looked 
around and in my experience most drivers assume the DT port number is 
the same as the media pad number.

This might be true for most devices but there are cases where this 
mapping do not hold true. This series is implemented support to the 
ongoing ADV748x work by Kieran Bingham, [1]. 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.

I have updated my R-Car CSI-2 patch series to use this new function to 
ask it's subdevice to resolve the media pad.

1. [PATCH 0/5] RFC: ADV748x HDMI/Analog video receiver

Niklas Söderlund (2):
  media: entity: Add pad_from_dt_regs entity operation
  media: entity: Add media_entity_pad_from_dt_regs() function

 drivers/media/media-entity.c | 21 +++++++++++++++++++++
 include/media/media-entity.h | 26 ++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

-- 
2.12.2

[PATCH 1/2] media: entity: Add pad_from_dt_regs entity operation

[Niklas Söderlund]

The optional operation can be used by entities to report how it maps its
DT node ports and endpoints to media pad numbers. This is useful for
devices which require more advanced mappings of pads then DT port
number is equivalent with media port number.

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

diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index c7c254c5bca1761b..47efaf4d825e671b 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -171,6 +171,9 @@ struct media_pad {
 
 /**
  * struct media_entity_operations - Media entity operations
+ * @pad_from_dt_regs:	Return the pad number based on DT port and reg
+ *			properties. This operation can be used to map a
+ *			DT port and reg 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 +187,7 @@ struct media_pad {
  *    mutex held.
  */
 struct media_entity_operations {
+	int (*pad_from_dt_regs)(int port_reg, int reg, unsigned int *pad);
 	int (*link_setup)(struct media_entity *entity,
 			  const struct media_pad *local,
 			  const struct media_pad *remote, u32 flags);
-- 
2.12.2

[PATCH 2/2] media: entity: Add media_entity_pad_from_dt_regs() function

[Niklas Söderlund]

This is a wrapper around the media entity pad_from_dt_regs operation.

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

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index 5640ca29da8c9bbc..6ef76186d552724e 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -386,6 +386,27 @@ struct media_entity *media_graph_walk_next(struct media_graph *graph)
 }
 EXPORT_SYMBOL_GPL(media_graph_walk_next);
 
+int media_entity_pad_from_dt_regs(struct media_entity *entity,
+				  int port_reg, int reg, unsigned int *pad)
+{
+	int ret;
+
+	if (!entity->ops || !entity->ops->pad_from_dt_regs) {
+		*pad = port_reg;
+		return 0;
+	}
+
+	ret = entity->ops->pad_from_dt_regs(port_reg, reg, pad);
+	if (ret)
+		return ret;
+
+	if (*pad >= entity->num_pads)
+		return -EINVAL;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(media_entity_pad_from_dt_regs);
+
 /* -----------------------------------------------------------------------------
  * Pipeline management
  */
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index 47efaf4d825e671b..c60a3713d0a21baf 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -820,6 +820,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_dt_regs - Get pad number from DT regs
+ *
+ * @entity: The entity
+ * @port_reg: DT port
+ * @reg: DT reg
+ * @pad: Pointer to pad which will be filled in
+ *
+ * This function can be used to resolve the media pad number from
+ * DT port and reg numbers. This is useful for devices which
+ * uses more complex mappings of media pads then that the
+ * DT port number is equivalent to the media pad number.
+ *
+ * If the entity do not implement the pad_from_dt_regs() operation
+ * this function assumes DT port is equivalent to media pad number
+ * and sets @pad to @port_reg.
+ *
+ * Return: 0 on success else -EINVAL.
+ */
+int media_entity_pad_from_dt_regs(struct media_entity *entity,
+				  int port_reg, int reg, 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.12.2

Re: [PATCH 1/2] media: entity: Add pad_from_dt_regs entity operation

[Sakari Ailus]

Hi Niklas,

On Fri, Apr 28, 2017 at 12:33:22AM +0200, Niklas Söderlund wrote:
> The optional operation can be used by entities to report how it maps its
> DT node ports and endpoints to media pad numbers. This is useful for
> devices which require more advanced mappings of pads then DT port
> number is equivalent with media port number.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  include/media/media-entity.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index c7c254c5bca1761b..47efaf4d825e671b 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -171,6 +171,9 @@ struct media_pad {
>  
>  /**
>   * struct media_entity_operations - Media entity operations
> + * @pad_from_dt_regs:	Return the pad number based on DT port and reg
> + *			properties. This operation can be used to map a
> + *			DT port and reg to a media pad number. Optional.

Don't you need to provide entity as an argument as well? The driver will be
a little bit loss due to lack of context. :-)

How about using the endpoint's device node (or fwnode; you can get it using
of_fwnode_handle() soon) instead? You can always obtain the port node by
just getting the parent.

>   * @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 +187,7 @@ struct media_pad {
>   *    mutex held.
>   */
>  struct media_entity_operations {
> +	int (*pad_from_dt_regs)(int port_reg, int reg, unsigned int *pad);
>  	int (*link_setup)(struct media_entity *entity,
>  			  const struct media_pad *local,
>  			  const struct media_pad *remote, u32 flags);

-- 
Kind regards,

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

Re: [PATCH 2/2] media: entity: Add media_entity_pad_from_dt_regs() function

[Sakari Ailus]

Hejssan!!!

On Fri, Apr 28, 2017 at 12:33:23AM +0200, Niklas Söderlund wrote:
> This is a wrapper around the media entity pad_from_dt_regs operation.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/media-entity.c | 21 +++++++++++++++++++++
>  include/media/media-entity.h | 22 ++++++++++++++++++++++
>  2 files changed, 43 insertions(+)
> 
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index 5640ca29da8c9bbc..6ef76186d552724e 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -386,6 +386,27 @@ struct media_entity *media_graph_walk_next(struct media_graph *graph)
>  }
>  EXPORT_SYMBOL_GPL(media_graph_walk_next);
>  
> +int media_entity_pad_from_dt_regs(struct media_entity *entity,
> +				  int port_reg, int reg, unsigned int *pad)
> +{
> +	int ret;
> +
> +	if (!entity->ops || !entity->ops->pad_from_dt_regs) {
> +		*pad = port_reg;

I don't think we should bind the port number in firmware to a pad in V4L2
sub-device interface.

How about looking for a source pad in the entity instead? That's what some
drivers do.

> +		return 0;
> +	}
> +
> +	ret = entity->ops->pad_from_dt_regs(port_reg, reg, pad);
> +	if (ret)
> +		return ret;
> +
> +	if (*pad >= entity->num_pads)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(media_entity_pad_from_dt_regs);
> +
>  /* -----------------------------------------------------------------------------
>   * Pipeline management
>   */
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index 47efaf4d825e671b..c60a3713d0a21baf 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -820,6 +820,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_dt_regs - Get pad number from DT regs
> + *
> + * @entity: The entity
> + * @port_reg: DT port
> + * @reg: DT reg
> + * @pad: Pointer to pad which will be filled in
> + *
> + * This function can be used to resolve the media pad number from
> + * DT port and reg numbers. This is useful for devices which
> + * uses more complex mappings of media pads then that the
> + * DT port number is equivalent to the media pad number.
> + *
> + * If the entity do not implement the pad_from_dt_regs() operation
> + * this function assumes DT port is equivalent to media pad number
> + * and sets @pad to @port_reg.
> + *
> + * Return: 0 on success else -EINVAL.

-EINVAL suggests the user provided bad parameters, but this isn't the case
here. How about e.g. -ENXIO?

> + */
> +int media_entity_pad_from_dt_regs(struct media_entity *entity,
> +				  int port_reg, int reg, 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

-- 
Hälsningar,

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

Re: [PATCH 0/2] media: entity: add operation to help map DT node to media pad

[Mauro Carvalho Chehab]

Hi Niklas,

Em Fri, 28 Apr 2017 00:33:21 +0200
Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> escreveu:

> Hi,
> 
> This small series add a new entity operation which will aid capture 
> drivers to map a port/endpoint in DT to a media graph pad. I looked 
> around and in my experience most drivers assume the DT port number is 
> the same as the media pad number.
> 
> This might be true for most devices but there are cases where this 
> mapping do not hold true. This series is implemented support to the 
> ongoing ADV748x work by Kieran Bingham, [1]. 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.
> 
> I have updated my R-Car CSI-2 patch series to use this new function to 
> ask it's subdevice to resolve the media pad.

The problem of finding a PAD is not specific for DT-based devices.
So, what we need is a generic way to find a pad.

The non-DT based drivers usually don't implement subdev API. So, they
need to build the pipelines themselves. On such devices, there are
hundreds of different combinations of devices, and the main driver
needs to seek the hardware connected into it. Based on such
runtime knowledge, setup the pipelines.

One such example is em28xx with can use a wide range of different 
tuners, analog TV decoders and digital TV frontends.

The I2C devices like tuners and decoders have pads with different
signals:
	- RF 
	- digital video (encoded with ITU-R BT.656 or similar)
	- audio IF signal
	- chroma IF signal
	- baseband signal
	- luminance IF signal
	- digital audio (using I2S)
	- composite video
	- ...

Right now, this is "solved" by using enums at include/media/v4l2-mc.h,
like this one:

enum tuner_pad_index {
	TUNER_PAD_RF_INPUT,
	TUNER_PAD_OUTPUT,
	TUNER_PAD_AUD_OUT,
	TUNER_NUM_PADS
};

That's not optimal, as even tuners that don't provide, for example,
an audio output pad need to have an unconnected TUNER_PAD_AUD_OUT
pad [1].

[1] With the current model, we're using TUNER_PAD_AUD_OUT for both
IF and digital audio - as currently - drivers don't need to distinguish
and we didn't want to have an excessive number of unconnected PADs.

So, what we really need is a way to represent a set of properties
associated with pads, and a function that would seek for a PAD that
matches a property set.

There is a proposal from Sakari to have a properties API that would
allow such kind of association (among others) and would even let
export such properties to userspace, but he never had time to send
us patches adding such functionality.

- 

IMHO, what we should do, instead of the approach you took, would be
to create a list of properties associated with each PAD (or, actually,
to any graph object, as we may want later to have properties also for
entities, interfaces and links). Something like:

enum media_property_type {
	MEDIA_PROP_PAD_DT_PORT_REG,	// not sure if this is the best name
	MEDIA_PROP_PAD_DT_REG,	// not sure if this is the best name
	MEDIA_PROP_PAD_SIGNAL_TYPE,	// that's for the above example of identifying a pad based on the signal it carries: I2S, RF, IF, ...
	...
};

struct media_properties {
	enum media_property_type type;
	int value;

	struct list_head *list;
};

struct media_graph {
	struct {
		struct media_entity *entity;
		struct list_head *link;
	} stack[MEDIA_ENTITY_ENUM_MAX_DEPTH];

	struct media_entity_enum ent_enum;
	int top;

	struct list_head *props; /* head for struct media_properties */
};

and a generic media_find_property() function that would allow a
driver to seek for an specific set of properties, e. g.:

int find_find_property(struct media_properties *props, struct media_graph *gobj);

This way, if someone would need to seek for an specific set of
properties (like on your DT case), he could use a helper function like
(untested):

find_dt_reg(int _port_reg, int _reg, struct media_graph *gobj)
{
	struct media_properties port_reg, reg;

	port_reg.type =	MEDIA_PROP_DT_PORT_REG;
	port_reg.value = _port_reg;

	reg.type = MEDIA_PROP_DT_REG;
	reg.value = _reg;

	INIT_LIST_HEAD(&port_reg->list);
	list_add_tail(&port->list, &reg);

	find_find_property(&port_reg, gobj);
}


Thanks,
Mauro

Re: [PATCH 1/2] media: entity: Add pad_from_dt_regs entity operation

[Niklas Söderlund]

Hi Sakari,

Thanks for your feedback.

On 2017-04-28 13:32:57 +0300, Sakari Ailus wrote:
> Hi Niklas,
> 
> On Fri, Apr 28, 2017 at 12:33:22AM +0200, Niklas Söderlund wrote:
> > The optional operation can be used by entities to report how it maps its
> > DT node ports and endpoints to media pad numbers. This is useful for
> > devices which require more advanced mappings of pads then DT port
> > number is equivalent with media port number.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  include/media/media-entity.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > index c7c254c5bca1761b..47efaf4d825e671b 100644
> > --- a/include/media/media-entity.h
> > +++ b/include/media/media-entity.h
> > @@ -171,6 +171,9 @@ struct media_pad {
> >  
> >  /**
> >   * struct media_entity_operations - Media entity operations
> > + * @pad_from_dt_regs:	Return the pad number based on DT port and reg
> > + *			properties. This operation can be used to map a
> > + *			DT port and reg to a media pad number. Optional.
> 
> Don't you need to provide entity as an argument as well? The driver will be
> a little bit loss due to lack of context. :-)

I'm not sure I understand you, this is a entity operation so the driver 
will know for which entity the request is mad on. Or am I missing 
something?

> 
> How about using the endpoint's device node (or fwnode; you can get it using
> of_fwnode_handle() soon) instead? You can always obtain the port node by
> just getting the parent.

I did think about that but opted for port_reg and reg since it seemed 
more simple.

But it might be better to base this work on top of your fwnode work,
s/from_dt_regs/from_fwnode/ and use the of_fwnode_handle() as you 
suggest here. Do you think this would be valuable and make this new 
operation more useful?

> 
> >   * @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 +187,7 @@ struct media_pad {
> >   *    mutex held.
> >   */
> >  struct media_entity_operations {
> > +	int (*pad_from_dt_regs)(int port_reg, int reg, unsigned int *pad);
> >  	int (*link_setup)(struct media_entity *entity,
> >  			  const struct media_pad *local,
> >  			  const struct media_pad *remote, u32 flags);
> 
> -- 
> Kind regards,
> 
> Sakari Ailus
> e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

-- 
Regards,
Niklas Söderlund

Re: [PATCH 2/2] media: entity: Add media_entity_pad_from_dt_regs() function

[Niklas Söderlund]

Hej,

Thanks for your feedback.

On 2017-04-28 13:43:39 +0300, Sakari Ailus wrote:
> Hejssan!!!
> 
> On Fri, Apr 28, 2017 at 12:33:23AM +0200, Niklas Söderlund wrote:
> > This is a wrapper around the media entity pad_from_dt_regs operation.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/media-entity.c | 21 +++++++++++++++++++++
> >  include/media/media-entity.h | 22 ++++++++++++++++++++++
> >  2 files changed, 43 insertions(+)
> > 
> > diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> > index 5640ca29da8c9bbc..6ef76186d552724e 100644
> > --- a/drivers/media/media-entity.c
> > +++ b/drivers/media/media-entity.c
> > @@ -386,6 +386,27 @@ struct media_entity *media_graph_walk_next(struct media_graph *graph)
> >  }
> >  EXPORT_SYMBOL_GPL(media_graph_walk_next);
> >  
> > +int media_entity_pad_from_dt_regs(struct media_entity *entity,
> > +				  int port_reg, int reg, unsigned int *pad)
> > +{
> > +	int ret;
> > +
> > +	if (!entity->ops || !entity->ops->pad_from_dt_regs) {
> > +		*pad = port_reg;
> 
> I don't think we should bind the port number in firmware to a pad in V4L2
> sub-device interface.
> 
> How about looking for a source pad in the entity instead? That's what some
> drivers do.

Sure that sounds like a nice approach, will do this for next version.

Would it make sens to extend this operation with a 'direction' parameter 
which could take either MEDIA_PAD_FL_SOURCE or MEDIA_PAD_FL_SINK and if 
!entity->ops->pad_from_dt_regs find the first pad that matches this 
'direction' and if it do exist add a check to make sure the pad that is 
return matches that 'direction'?

> 
> > +		return 0;
> > +	}
> > +
> > +	ret = entity->ops->pad_from_dt_regs(port_reg, reg, pad);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (*pad >= entity->num_pads)
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(media_entity_pad_from_dt_regs);
> > +
> >  /* -----------------------------------------------------------------------------
> >   * Pipeline management
> >   */
> > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > index 47efaf4d825e671b..c60a3713d0a21baf 100644
> > --- a/include/media/media-entity.h
> > +++ b/include/media/media-entity.h
> > @@ -820,6 +820,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_dt_regs - Get pad number from DT regs
> > + *
> > + * @entity: The entity
> > + * @port_reg: DT port
> > + * @reg: DT reg
> > + * @pad: Pointer to pad which will be filled in
> > + *
> > + * This function can be used to resolve the media pad number from
> > + * DT port and reg numbers. This is useful for devices which
> > + * uses more complex mappings of media pads then that the
> > + * DT port number is equivalent to the media pad number.
> > + *
> > + * If the entity do not implement the pad_from_dt_regs() operation
> > + * this function assumes DT port is equivalent to media pad number
> > + * and sets @pad to @port_reg.
> > + *
> > + * Return: 0 on success else -EINVAL.
> 
> -EINVAL suggests the user provided bad parameters, but this isn't the case
> here. How about e.g. -ENXIO?


I reasoned that if a port_reg and reg supplied did result in a  pad 
match the user would have given pad parameters. But sure there might be 
cases where that assumtion might not be true. So I see no problem of 
changing this to -ENXIO in next version.

> 
> > + */
> > +int media_entity_pad_from_dt_regs(struct media_entity *entity,
> > +				  int port_reg, int reg, 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
> 
> -- 
> Hälsningar,
> 
> Sakari Ailus
> e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

-- 
Regards,
Niklas Söderlund

Re: [PATCH 1/2] media: entity: Add pad_from_dt_regs entity operation

[Sakari Ailus]

Hi Niklas,

On Fri, Apr 28, 2017 at 01:57:52PM +0200, Niklas Söderlund wrote:
> Hi Sakari,
> 
> Thanks for your feedback.
> 
> On 2017-04-28 13:32:57 +0300, Sakari Ailus wrote:
> > Hi Niklas,
> > 
> > On Fri, Apr 28, 2017 at 12:33:22AM +0200, Niklas Söderlund wrote:
> > > The optional operation can be used by entities to report how it maps its
> > > DT node ports and endpoints to media pad numbers. This is useful for
> > > devices which require more advanced mappings of pads then DT port
> > > number is equivalent with media port number.
> > > 
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > ---
> > >  include/media/media-entity.h | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > > index c7c254c5bca1761b..47efaf4d825e671b 100644
> > > --- a/include/media/media-entity.h
> > > +++ b/include/media/media-entity.h
> > > @@ -171,6 +171,9 @@ struct media_pad {
> > >  
> > >  /**
> > >   * struct media_entity_operations - Media entity operations
> > > + * @pad_from_dt_regs:	Return the pad number based on DT port and reg
> > > + *			properties. This operation can be used to map a
> > > + *			DT port and reg to a media pad number. Optional.
> > 
> > Don't you need to provide entity as an argument as well? The driver will be
> > a little bit loss due to lack of context. :-)
> 
> I'm not sure I understand you, this is a entity operation so the driver 
> will know for which entity the request is mad on. Or am I missing 
> something?

I actually came to think about the same thing after replying, but then
remembered something else: some drivers support multiple devices with
different capabilities. There could be a reason to know which piece of
hardware this is about.

Although that can always be added later on if needed. Up to you.

> 
> > 
> > How about using the endpoint's device node (or fwnode; you can get it using
> > of_fwnode_handle() soon) instead? You can always obtain the port node by
> > just getting the parent.
> 
> I did think about that but opted for port_reg and reg since it seemed 
> more simple.
> 
> But it might be better to base this work on top of your fwnode work,
> s/from_dt_regs/from_fwnode/ and use the of_fwnode_handle() as you 
> suggest here. Do you think this would be valuable and make this new 
> operation more useful?

The benefit is that it would also work on ACPI based systems, i.e. it is no
longer DT specific. I'd say it's a benefit. :-)

Yeah, I understand that you'd just use integers in the driver and then make
the decision based on them? Makes sense. How about the wrapper function,
should it take an fwnode handle pointer instead? The caller of that function
likely has an fwnode pointer to start with, found through iterating over the
endpoints.

> 
> > 
> > >   * @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 +187,7 @@ struct media_pad {
> > >   *    mutex held.
> > >   */
> > >  struct media_entity_operations {
> > > +	int (*pad_from_dt_regs)(int port_reg, int reg, unsigned int *pad);
> > >  	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

Re: [PATCH 2/2] media: entity: Add media_entity_pad_from_dt_regs() function

[Sakari Ailus]

Hej, Niklas!

On Fri, Apr 28, 2017 at 02:04:15PM +0200, Niklas Söderlund wrote:
> Hej,
> 
> Thanks for your feedback.
> 
> On 2017-04-28 13:43:39 +0300, Sakari Ailus wrote:
> > Hejssan!!!
> > 
> > On Fri, Apr 28, 2017 at 12:33:23AM +0200, Niklas Söderlund wrote:
> > > This is a wrapper around the media entity pad_from_dt_regs operation.
> > > 
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > ---
> > >  drivers/media/media-entity.c | 21 +++++++++++++++++++++
> > >  include/media/media-entity.h | 22 ++++++++++++++++++++++
> > >  2 files changed, 43 insertions(+)
> > > 
> > > diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> > > index 5640ca29da8c9bbc..6ef76186d552724e 100644
> > > --- a/drivers/media/media-entity.c
> > > +++ b/drivers/media/media-entity.c
> > > @@ -386,6 +386,27 @@ struct media_entity *media_graph_walk_next(struct media_graph *graph)
> > >  }
> > >  EXPORT_SYMBOL_GPL(media_graph_walk_next);
> > >  
> > > +int media_entity_pad_from_dt_regs(struct media_entity *entity,
> > > +				  int port_reg, int reg, unsigned int *pad)
> > > +{
> > > +	int ret;
> > > +
> > > +	if (!entity->ops || !entity->ops->pad_from_dt_regs) {
> > > +		*pad = port_reg;
> > 
> > I don't think we should bind the port number in firmware to a pad in V4L2
> > sub-device interface.
> > 
> > How about looking for a source pad in the entity instead? That's what some
> > drivers do.
> 
> Sure that sounds like a nice approach, will do this for next version.
> 
> Would it make sens to extend this operation with a 'direction' parameter 
> which could take either MEDIA_PAD_FL_SOURCE or MEDIA_PAD_FL_SINK and if 
> !entity->ops->pad_from_dt_regs find the first pad that matches this 
> 'direction' and if it do exist add a check to make sure the pad that is 
> return matches that 'direction'?

If you had a transmitter in the graph it'd obviously be needed. I'm not sure
if we have any now. I'm fine with direction though, it's logical to have it.

> 
> > 
> > > +		return 0;
> > > +	}
> > > +
> > > +	ret = entity->ops->pad_from_dt_regs(port_reg, reg, pad);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	if (*pad >= entity->num_pads)
> > > +		return -EINVAL;
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(media_entity_pad_from_dt_regs);
> > > +
> > >  /* -----------------------------------------------------------------------------
> > >   * Pipeline management
> > >   */
> > > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > > index 47efaf4d825e671b..c60a3713d0a21baf 100644
> > > --- a/include/media/media-entity.h
> > > +++ b/include/media/media-entity.h
> > > @@ -820,6 +820,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_dt_regs - Get pad number from DT regs
> > > + *
> > > + * @entity: The entity
> > > + * @port_reg: DT port
> > > + * @reg: DT reg
> > > + * @pad: Pointer to pad which will be filled in
> > > + *
> > > + * This function can be used to resolve the media pad number from
> > > + * DT port and reg numbers. This is useful for devices which
> > > + * uses more complex mappings of media pads then that the
> > > + * DT port number is equivalent to the media pad number.
> > > + *
> > > + * If the entity do not implement the pad_from_dt_regs() operation
> > > + * this function assumes DT port is equivalent to media pad number
> > > + * and sets @pad to @port_reg.
> > > + *
> > > + * Return: 0 on success else -EINVAL.
> > 
> > -EINVAL suggests the user provided bad parameters, but this isn't the case
> > here. How about e.g. -ENXIO?
> 
> 
> I reasoned that if a port_reg and reg supplied did result in a  pad 
> match the user would have given pad parameters. But sure there might be 
> cases where that assumtion might not be true. So I see no problem of 
> changing this to -ENXIO in next version.

The only case -EINVAL is returned is when the driver specific function
returns a non-existent pad number. That's a driver bug, isn't it? We don't
have a specific error code for that though. :-)

> 
> > 
> > > + */
> > > +int media_entity_pad_from_dt_regs(struct media_entity *entity,
> > > +				  int port_reg, int reg, 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
> > 

-- 
Terveisin,

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